| Message ID | 20170916104220.3742-9-fancer.lancer@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | usb: usb251xb: Add USB2517i hub support and fix some bugs | expand |
On Sat, Sep 16, 2017 at 01:42:19PM +0300, Serge Semin wrote: > This parameters may be varied in accordance with hardware specifics. > So lets add the corresponding settings to the usb251x driver dts > specification. > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > --- > Documentation/devicetree/bindings/usb/usb251xb.txt | 6 ++++++ > drivers/usb/misc/usb251xb.c | 20 ++++++++++++++++---- > 2 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt > index 3d84626d3..dd59a32e7 100644 > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt > @@ -44,6 +44,12 @@ Optional properties : > device connected. > - sp-disabled-ports : Specifies the ports which will be self-power disabled > - bp-disabled-ports : Specifies the ports which will be bus-power disabled > + - sp-max-{power,current} : Indicates the power/current consumed by hub from > + an upstream port (VBUS) when operation as a self-powered hub. The value > + is given in mA in a 0 - 100 range (default is 1mA). > + - bp-max-{power,current} : Indicates the power/current consumed by hub from > + an upstream port (VBUS) when operation as a bus-powered hub. The value > + is given in mA in a 0 - 510 range (default is 100mA). These need units as defined in property-units.txt. Why do you need power and current? Can't you calculate power? > - power-on-time-ms : Specifies the time it takes from the time the host > initiates the power-on sequence to a port until the port has adequate > power. The value is given in ms in a 0 - 510 range (default is 100ms). -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 20, 2017 at 03:52:55PM -0500, Rob Herring <robh@kernel.org> wrote: > On Sat, Sep 16, 2017 at 01:42:19PM +0300, Serge Semin wrote: > > This parameters may be varied in accordance with hardware specifics. > > So lets add the corresponding settings to the usb251x driver dts > > specification. > > > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > > --- > > Documentation/devicetree/bindings/usb/usb251xb.txt | 6 ++++++ > > drivers/usb/misc/usb251xb.c | 20 ++++++++++++++++---- > > 2 files changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt > > index 3d84626d3..dd59a32e7 100644 > > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt > > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt > > @@ -44,6 +44,12 @@ Optional properties : > > device connected. > > - sp-disabled-ports : Specifies the ports which will be self-power disabled > > - bp-disabled-ports : Specifies the ports which will be bus-power disabled > > + - sp-max-{power,current} : Indicates the power/current consumed by hub from > > + an upstream port (VBUS) when operation as a self-powered hub. The value > > + is given in mA in a 0 - 100 range (default is 1mA). > > + - bp-max-{power,current} : Indicates the power/current consumed by hub from > > + an upstream port (VBUS) when operation as a bus-powered hub. The value > > + is given in mA in a 0 - 510 range (default is 100mA). > > These need units as defined in property-units.txt. > Ok. > Why do you need power and current? Can't you calculate power? > These are different parameters of the device. They got different configuration registers and descriptions: max_power* - ... This value also includes the power consumption of a permanently attached peripheral if the hub is configured as a compound device, and the embedded peripheral reports 0mA in its descriptors. max_current* - ... This value does NOT include the power consumption of a permanently attached peripheral if the hub is configured as a compound device. Additionally as you can see, they both are measured in "mA", so it isn't a real physical power. > > - power-on-time-ms : Specifies the time it takes from the time the host > > initiates the power-on sequence to a port until the port has adequate > > power. The value is given in ms in a 0 - 510 range (default is 100ms). -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 20, 2017 at 4:27 PM, Serge Semin <fancer.lancer@gmail.com> wrote: > On Wed, Sep 20, 2017 at 03:52:55PM -0500, Rob Herring <robh@kernel.org> wrote: >> On Sat, Sep 16, 2017 at 01:42:19PM +0300, Serge Semin wrote: >> > This parameters may be varied in accordance with hardware specifics. >> > So lets add the corresponding settings to the usb251x driver dts >> > specification. >> > >> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> >> > --- >> > Documentation/devicetree/bindings/usb/usb251xb.txt | 6 ++++++ >> > drivers/usb/misc/usb251xb.c | 20 ++++++++++++++++---- >> > 2 files changed, 22 insertions(+), 4 deletions(-) >> > >> > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt >> > index 3d84626d3..dd59a32e7 100644 >> > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt >> > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt >> > @@ -44,6 +44,12 @@ Optional properties : >> > device connected. >> > - sp-disabled-ports : Specifies the ports which will be self-power disabled >> > - bp-disabled-ports : Specifies the ports which will be bus-power disabled >> > + - sp-max-{power,current} : Indicates the power/current consumed by hub from >> > + an upstream port (VBUS) when operation as a self-powered hub. The value >> > + is given in mA in a 0 - 100 range (default is 1mA). >> > + - bp-max-{power,current} : Indicates the power/current consumed by hub from >> > + an upstream port (VBUS) when operation as a bus-powered hub. The value >> > + is given in mA in a 0 - 510 range (default is 100mA). >> >> These need units as defined in property-units.txt. >> > > Ok. > >> Why do you need power and current? Can't you calculate power? >> > > These are different parameters of the device. They got different configuration > registers and descriptions: > max_power* - ... This value also includes the power consumption of a > permanently attached peripheral if the hub is configured as a compound > device, and the embedded peripheral reports 0mA in its descriptors. > max_current* - ... This value does NOT include the power consumption of a > permanently attached peripheral if the hub is configured as a compound > device. Then the names here should somehow reflect the above. Perhaps "composite-current" and "hub-current" or something like that. > > Additionally as you can see, they both are measured in "mA", so it isn't > a real physical power. Well, I can't because there's no units. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 21, 2017 at 11:26:04AM -0500, Rob Herring <robh@kernel.org> wrote: > On Wed, Sep 20, 2017 at 4:27 PM, Serge Semin <fancer.lancer@gmail.com> wrote: > > On Wed, Sep 20, 2017 at 03:52:55PM -0500, Rob Herring <robh@kernel.org> wrote: > >> On Sat, Sep 16, 2017 at 01:42:19PM +0300, Serge Semin wrote: > >> > This parameters may be varied in accordance with hardware specifics. > >> > So lets add the corresponding settings to the usb251x driver dts > >> > specification. > >> > > >> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > >> > --- > >> > Documentation/devicetree/bindings/usb/usb251xb.txt | 6 ++++++ > >> > drivers/usb/misc/usb251xb.c | 20 ++++++++++++++++---- > >> > 2 files changed, 22 insertions(+), 4 deletions(-) > >> > > >> > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt > >> > index 3d84626d3..dd59a32e7 100644 > >> > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt > >> > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt > >> > @@ -44,6 +44,12 @@ Optional properties : > >> > device connected. > >> > - sp-disabled-ports : Specifies the ports which will be self-power disabled > >> > - bp-disabled-ports : Specifies the ports which will be bus-power disabled > >> > + - sp-max-{power,current} : Indicates the power/current consumed by hub from > >> > + an upstream port (VBUS) when operation as a self-powered hub. The value > >> > + is given in mA in a 0 - 100 range (default is 1mA). > >> > + - bp-max-{power,current} : Indicates the power/current consumed by hub from > >> > + an upstream port (VBUS) when operation as a bus-powered hub. The value > >> > + is given in mA in a 0 - 510 range (default is 100mA). > >> > >> These need units as defined in property-units.txt. > >> > > > > Ok. > > > >> Why do you need power and current? Can't you calculate power? > >> > > > > These are different parameters of the device. They got different configuration > > registers and descriptions: > > max_power* - ... This value also includes the power consumption of a > > permanently attached peripheral if the hub is configured as a compound > > device, and the embedded peripheral reports 0mA in its descriptors. > > max_current* - ... This value does NOT include the power consumption of a > > permanently attached peripheral if the hub is configured as a compound > > device. > > Then the names here should somehow reflect the above. Perhaps > "composite-current" and "hub-current" or something like that. > I left the naming in accordance with the device datasheet. I thought it would be better since the driver user would still need to consult with the device documentation to properly set them. I don't really get how the difference is reflected with the naming declared there though. So what naming would you prefer then? Might be something like: {sp,bp}-max-total-current - for so named {sp,bp}-max-power, since it includes all the permanently attached peripherals. {sp,bp}-max-removable-current - for so named {sp,bp}-max-current, since it doesn't include the permanently attached peripherals. Or is it better to leave it in compliance with the documentation naming? > > > > Additionally as you can see, they both are measured in "mA", so it isn't > > a real physical power. > > Well, I can't because there's no units. > What this line means then? - sp-max-{power,current} : ... The value is given in mA in a 0 - 100 range (default is 1mA). - bp-max-{power,current} : ... The value is given in mA in a 0 - 510 range (default is 100mA). Maybe I don't know something and the description line should state the units somehow clearer? > Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/21/2017 07:10 PM, Serge Semin wrote: > On Thu, Sep 21, 2017 at 11:26:04AM -0500, Rob Herring <robh@kernel.org> wrote: >> On Wed, Sep 20, 2017 at 4:27 PM, Serge Semin <fancer.lancer@gmail.com> wrote: ... >>> These are different parameters of the device. They got different configuration >>> registers and descriptions: >>> max_power* - ... This value also includes the power consumption of a >>> permanently attached peripheral if the hub is configured as a compound >>> device, and the embedded peripheral reports 0mA in its descriptors. >>> max_current* - ... This value does NOT include the power consumption of a >>> permanently attached peripheral if the hub is configured as a compound >>> device. >> >> Then the names here should somehow reflect the above. Perhaps >> "composite-current" and "hub-current" or something like that. >> > > I left the naming in accordance with the device datasheet. I thought it would be > better since the driver user would still need to consult with the device > documentation to properly set them. I don't really get how the difference is reflected > with the naming declared there though. So what naming would you prefer then? Might be > something like: > {sp,bp}-max-total-current - for so named {sp,bp}-max-power, since it includes all the > permanently attached peripherals. > {sp,bp}-max-removable-current - for so named {sp,bp}-max-current, since it doesn't > include the permanently attached peripherals. > > Or is it better to leave it in compliance with the documentation naming? I'd prefer the naming to be in accordance with the device datasheet too. Changing it will IMHO generate avoidable misunderstandings... But if we should go with Robs proposal please make sure the name from the device datasheet is mentioned somewhere in the description of the binding. > >>> >>> Additionally as you can see, they both are measured in "mA", so it isn't >>> a real physical power. >> >> Well, I can't because there's no units. >> > > What this line means then? > - sp-max-{power,current} : ... The value is given in mA in a 0 - 100 range (default is 1mA). > - bp-max-{power,current} : ... The value is given in mA in a 0 - 510 range (default is 100mA). > > Maybe I don't know something and the description line should state the units somehow > clearer? Append the unit to the binding name, just like in "power-on-time-ms". As there's no "Standard Unit Suffix" for mA stated in the documentation (Documentation/devicetree/bindings/property-units.txt) I think it should be "-milliamp"? regards, Richard.L -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 4, 2017 at 3:12 AM, Richard Leitner <richard.leitner@skidata.com> wrote: > > On 09/21/2017 07:10 PM, Serge Semin wrote: >> On Thu, Sep 21, 2017 at 11:26:04AM -0500, Rob Herring <robh@kernel.org> wrote: >>> On Wed, Sep 20, 2017 at 4:27 PM, Serge Semin <fancer.lancer@gmail.com> wrote: > > ... > >>>> These are different parameters of the device. They got different configuration >>>> registers and descriptions: >>>> max_power* - ... This value also includes the power consumption of a >>>> permanently attached peripheral if the hub is configured as a compound >>>> device, and the embedded peripheral reports 0mA in its descriptors. >>>> max_current* - ... This value does NOT include the power consumption of a >>>> permanently attached peripheral if the hub is configured as a compound >>>> device. >>> >>> Then the names here should somehow reflect the above. Perhaps >>> "composite-current" and "hub-current" or something like that. >>> >> >> I left the naming in accordance with the device datasheet. I thought it would be >> better since the driver user would still need to consult with the device >> documentation to properly set them. I don't really get how the difference is reflected >> with the naming declared there though. So what naming would you prefer then? Might be >> something like: >> {sp,bp}-max-total-current - for so named {sp,bp}-max-power, since it includes all the >> permanently attached peripherals. >> {sp,bp}-max-removable-current - for so named {sp,bp}-max-current, since it doesn't >> include the permanently attached peripherals. >> >> Or is it better to leave it in compliance with the documentation naming? > > I'd prefer the naming to be in accordance with the device datasheet too. > Changing it will IMHO generate avoidable misunderstandings... Okay, then add a vendor prefix. > But if we should go with Robs proposal please make sure the name from > the device datasheet is mentioned somewhere in the description of the > binding. > >> >>>> >>>> Additionally as you can see, they both are measured in "mA", so it isn't >>>> a real physical power. >>> >>> Well, I can't because there's no units. >>> >> >> What this line means then? >> - sp-max-{power,current} : ... The value is given in mA in a 0 - 100 range (default is 1mA). >> - bp-max-{power,current} : ... The value is given in mA in a 0 - 510 range (default is 100mA). >> >> Maybe I don't know something and the description line should state the units somehow >> clearer? The reason we have units in the name is so we don't have to go lookup the documentation to find the units and so people don't use the same property name with different units. > Append the unit to the binding name, just like in "power-on-time-ms". As > there's no "Standard Unit Suffix" for mA stated in the documentation > (Documentation/devicetree/bindings/property-units.txt) I think it should > be "-milliamp"? The reason being is we align around microamps and try to avoid everyone picking their own units (celliamps anyone?). So unless microamps is not enough range for you, I would stick with that. If you have both units and vendor prefix, then I care less about the poorly written datasheet naming used. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt index 3d84626d3..dd59a32e7 100644 --- a/Documentation/devicetree/bindings/usb/usb251xb.txt +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt @@ -44,6 +44,12 @@ Optional properties : device connected. - sp-disabled-ports : Specifies the ports which will be self-power disabled - bp-disabled-ports : Specifies the ports which will be bus-power disabled + - sp-max-{power,current} : Indicates the power/current consumed by hub from + an upstream port (VBUS) when operation as a self-powered hub. The value + is given in mA in a 0 - 100 range (default is 1mA). + - bp-max-{power,current} : Indicates the power/current consumed by hub from + an upstream port (VBUS) when operation as a bus-powered hub. The value + is given in mA in a 0 - 510 range (default is 100mA). - power-on-time-ms : Specifies the time it takes from the time the host initiates the power-on sequence to a port until the port has adequate power. The value is given in ms in a 0 - 510 range (default is 100ms). diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c index c308b0006..71994b883 100644 --- a/drivers/usb/misc/usb251xb.c +++ b/drivers/usb/misc/usb251xb.c @@ -497,6 +497,22 @@ static int usb251xb_get_ofdata(struct usb251xb *hub, } } + hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF; + if (!of_property_read_u32(np, "sp-max-power", &property_u32)) + hub->max_power_sp = min_t(u8, property_u32 / 2, 50); + + hub->max_power_bp = USB251XB_DEF_MAX_POWER_BUS; + if (!of_property_read_u32(np, "bp-max-power", &property_u32)) + hub->max_power_bp = min_t(u8, property_u32 / 2, 255); + + hub->max_current_sp = USB251XB_DEF_MAX_CURRENT_SELF; + if (!of_property_read_u32(np, "sp-max-current", &property_u32)) + hub->max_current_sp = min_t(u8, property_u32 / 2, 50); + + hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS; + if (!of_property_read_u32(np, "bp-max-current", &property_u32)) + hub->max_current_bp = min_t(u8, property_u32 / 2, 255); + hub->power_on_time = USB251XB_DEF_POWER_ON_TIME; if (!of_property_read_u32(np, "power-on-time-ms", &property_u32)) hub->power_on_time = min_t(u8, property_u32 / 2, 255); @@ -536,10 +552,6 @@ static int usb251xb_get_ofdata(struct usb251xb *hub, /* The following parameters are currently not exposed to devicetree, but * may be as soon as needed. */ - hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF; - hub->max_power_bp = USB251XB_DEF_MAX_POWER_BUS; - hub->max_current_sp = USB251XB_DEF_MAX_CURRENT_SELF; - hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS; hub->bat_charge_en = USB251XB_DEF_BATTERY_CHARGING_ENABLE; hub->boost_up = USB251XB_DEF_BOOST_UP; hub->boost_57 = USB251XB_DEF_BOOST_57;
This parameters may be varied in accordance with hardware specifics. So lets add the corresponding settings to the usb251x driver dts specification. Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- Documentation/devicetree/bindings/usb/usb251xb.txt | 6 ++++++ drivers/usb/misc/usb251xb.c | 20 ++++++++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-)