* [PATCH 2/3] usb: misc: onboard_usb_hub: Add reset-gpio support
2022-07-12 15:06 [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller Alexander Stein
@ 2022-07-12 15:06 ` Alexander Stein
2022-07-12 18:18 ` Matthias Kaehlcke
2022-07-12 15:06 ` [PATCH 3/3] usb: misc: onboard_usb_hub: Add TI USB8041 hub support Alexander Stein
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Alexander Stein @ 2022-07-12 15:06 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Matthias Kaehlcke
Cc: Alexander Stein, linux-usb, devicetree
Despite default reset upon probe, release reset line after powering up
the hub and assert reset again before powering down.
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
My current DT node on my TQMa8MPxL looks like this
```
&usb_dwc3_1 {
dr_mode = "host";
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_usbhub>;
status = "okay";
hub_2_0: hub@1 {
compatible = "usb451,8142";
reg = <1>;
peer-hub = <&hub_3_0>;
reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
};
hub_3_0: hub@2 {
compatible = "usb451,8140";
reg = <2>;
peer-hub = <&hub_2_0>;
reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
};
};
```
which I don't like much for 2 reasons:
* the pinctrl has to be put in a common top-node of USB hub node. The pinctrl
can not be requested twice.
* Apparently there is no conflict on the reset-gpio only because just one device
gets probed here:
> $ ls /sys/bus/platform/drivers/onboard-usb-hub/
> 38200000.usb:hub@1 bind uevent unbind
But this seems better than to use a common fixed-regulator referenced by both
hub nodes, which just is controlled by GPIO and does not supply any voltages.
Note: It might also be necessary to add bindings to specify ramp up times and/or
reset timeouts.
drivers/usb/misc/onboard_usb_hub.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index 6b9b949d17d3..348fb5270266 100644
--- a/drivers/usb/misc/onboard_usb_hub.c
+++ b/drivers/usb/misc/onboard_usb_hub.c
@@ -7,6 +7,7 @@
#include <linux/device.h>
#include <linux/export.h>
+#include <linux/gpio/consumer.h>
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/list.h>
@@ -38,6 +39,7 @@ struct usbdev_node {
struct onboard_hub {
struct regulator *vdd;
struct device *dev;
+ struct gpio_desc *reset_gpio;
bool always_powered_in_suspend;
bool is_powered_on;
bool going_away;
@@ -56,6 +58,10 @@ static int onboard_hub_power_on(struct onboard_hub *hub)
return err;
}
+ /* Deassert reset */
+ usleep_range(3000, 3100);
+ gpiod_set_value_cansleep(hub->reset_gpio, 0);
+
hub->is_powered_on = true;
return 0;
@@ -65,6 +71,10 @@ static int onboard_hub_power_off(struct onboard_hub *hub)
{
int err;
+ /* Assert reset */
+ gpiod_set_value_cansleep(hub->reset_gpio, 1);
+ usleep_range(4000, 5000);
+
err = regulator_disable(hub->vdd);
if (err) {
dev_err(hub->dev, "failed to disable regulator: %d\n", err);
@@ -231,6 +241,14 @@ static int onboard_hub_probe(struct platform_device *pdev)
if (IS_ERR(hub->vdd))
return PTR_ERR(hub->vdd);
+ /* Put the hub into reset, pull reset line low, and assure 4ms reset low timing. */
+ hub->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(hub->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(hub->reset_gpio), "failed to get reset GPIO\n");
+
+ usleep_range(4000, 5000);
+
hub->dev = dev;
mutex_init(&hub->lock);
INIT_LIST_HEAD(&hub->udev_list);
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 2/3] usb: misc: onboard_usb_hub: Add reset-gpio support
2022-07-12 15:06 ` [PATCH 2/3] usb: misc: onboard_usb_hub: Add reset-gpio support Alexander Stein
@ 2022-07-12 18:18 ` Matthias Kaehlcke
2022-07-13 6:46 ` Alexander Stein
0 siblings, 1 reply; 17+ messages in thread
From: Matthias Kaehlcke @ 2022-07-12 18:18 UTC (permalink / raw)
To: Alexander Stein
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski, linux-usb,
devicetree
On Tue, Jul 12, 2022 at 05:06:26PM +0200, Alexander Stein wrote:
> Despite default reset upon probe, release reset line after powering up
> the hub and assert reset again before powering down.
>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> My current DT node on my TQMa8MPxL looks like this
> ```
> &usb_dwc3_1 {
> dr_mode = "host";
> #address-cells = <1>;
> #size-cells = <0>;
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_usbhub>;
> status = "okay";
>
> hub_2_0: hub@1 {
> compatible = "usb451,8142";
> reg = <1>;
> peer-hub = <&hub_3_0>;
> reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
> };
>
> hub_3_0: hub@2 {
> compatible = "usb451,8140";
> reg = <2>;
> peer-hub = <&hub_2_0>;
> reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
> };
> };
> ```
> which I don't like much for 2 reasons:
> * the pinctrl has to be put in a common top-node of USB hub node. The pinctrl
> can not be requested twice.
Agreed, that's not great. The pinctrl doesn't have to be necessarily in the USB
controller node, it could also be in the static section of the board, but that
isn't really much of an improvement :( Not sure there is much to do given that
the USB devices also process the pinctrl info (besides the onboard_hub platform
device doing the same).
> * Apparently there is no conflict on the reset-gpio only because just one device
> gets probed here:
> > $ ls /sys/bus/platform/drivers/onboard-usb-hub/
> > 38200000.usb:hub@1 bind uevent unbind
Right, the driver creates a single platform device for each physical hub.
> But this seems better than to use a common fixed-regulator referenced by both
> hub nodes, which just is controlled by GPIO and does not supply any voltages.
Agreed, if the GPIO controls a reset line it should be implemented as such.
> Note: It might also be necessary to add bindings to specify ramp up times and/or
> reset timeouts.
The times are hub specific, not board specific, right? If that's the case then
a binding shouldn't be needed, the timing can be derived from the compatible
string.
> drivers/usb/misc/onboard_usb_hub.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> index 6b9b949d17d3..348fb5270266 100644
> --- a/drivers/usb/misc/onboard_usb_hub.c
> +++ b/drivers/usb/misc/onboard_usb_hub.c
> @@ -7,6 +7,7 @@
>
> #include <linux/device.h>
> #include <linux/export.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/list.h>
> @@ -38,6 +39,7 @@ struct usbdev_node {
> struct onboard_hub {
> struct regulator *vdd;
> struct device *dev;
> + struct gpio_desc *reset_gpio;
> bool always_powered_in_suspend;
> bool is_powered_on;
> bool going_away;
> @@ -56,6 +58,10 @@ static int onboard_hub_power_on(struct onboard_hub *hub)
> return err;
> }
>
> + /* Deassert reset */
The comment isn't really needed, it's clear from the context.
> + usleep_range(3000, 3100);
These shouldn't be hard coded. Instead you could add a model specific struct
'hub_data' (or similar) and associate it with the compatible string through
onboard_hub_match.data
You could use fsleep() instead of usleep_range(). It does the _range part
automatically (with a value of 2x).
> + gpiod_set_value_cansleep(hub->reset_gpio, 0);
Since this includes delays maybe put the reset inside an 'if (hub->reset_gpio)'
block. Not super important for these short delays, but they might be longer
for some hubs.
> +
> hub->is_powered_on = true;
>
> return 0;
> @@ -65,6 +71,10 @@ static int onboard_hub_power_off(struct onboard_hub *hub)
> {
> int err;
>
> + /* Assert reset */
drop comment
> + gpiod_set_value_cansleep(hub->reset_gpio, 1);
Put inside 'if (hub->reset_gpio)' to avoid unnecessary delays when no reset
is configured.
> + usleep_range(4000, 5000);
Use per-model values.
> +
> err = regulator_disable(hub->vdd);
> if (err) {
> dev_err(hub->dev, "failed to disable regulator: %d\n", err);
> @@ -231,6 +241,14 @@ static int onboard_hub_probe(struct platform_device *pdev)
> if (IS_ERR(hub->vdd))
> return PTR_ERR(hub->vdd);
>
> + /* Put the hub into reset, pull reset line low, and assure 4ms reset low timing. */
drop comment, it's mostly evident from the code. Maybe not the usleep_range()
part, but that should become clearer when per model values are used.
> + hub->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(hub->reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(hub->reset_gpio), "failed to get reset GPIO\n");
> +
> + usleep_range(4000, 5000);
> +
> hub->dev = dev;
> mutex_init(&hub->lock);
> INIT_LIST_HEAD(&hub->udev_list);
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/3] usb: misc: onboard_usb_hub: Add reset-gpio support
2022-07-12 18:18 ` Matthias Kaehlcke
@ 2022-07-13 6:46 ` Alexander Stein
2022-07-13 16:59 ` Matthias Kaehlcke
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Stein @ 2022-07-13 6:46 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski, linux-usb,
devicetree
Hi Matthias,
Am Dienstag, 12. Juli 2022, 20:18:05 CEST schrieb Matthias Kaehlcke:
> On Tue, Jul 12, 2022 at 05:06:26PM +0200, Alexander Stein wrote:
> > Despite default reset upon probe, release reset line after powering up
> > the hub and assert reset again before powering down.
> >
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > My current DT node on my TQMa8MPxL looks like this
> > ```
> > &usb_dwc3_1 {
> >
> > dr_mode = "host";
> > #address-cells = <1>;
> > #size-cells = <0>;
> > pinctrl-names = "default";
> > pinctrl-0 = <&pinctrl_usbhub>;
> > status = "okay";
> >
> > hub_2_0: hub@1 {
> >
> > compatible = "usb451,8142";
> > reg = <1>;
> > peer-hub = <&hub_3_0>;
> > reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
> >
> > };
> >
> > hub_3_0: hub@2 {
> >
> > compatible = "usb451,8140";
> > reg = <2>;
> > peer-hub = <&hub_2_0>;
> > reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
> >
> > };
> >
> > };
> > ```
> > which I don't like much for 2 reasons:
> > * the pinctrl has to be put in a common top-node of USB hub node. The
> > pinctrl>
> > can not be requested twice.
>
> Agreed, that's not great. The pinctrl doesn't have to be necessarily in the
> USB controller node, it could also be in the static section of the board,
> but that isn't really much of an improvement :( Not sure there is much to
> do given that the USB devices also process the pinctrl info (besides the
> onboard_hub platform device doing the same).
I tend to keep the pinctrl property next to the ones actually using them. But
in this case it's not possible unfortunately.
> > * Apparently there is no conflict on the reset-gpio only because just one
> > device>
> > gets probed here:
> > > $ ls /sys/bus/platform/drivers/onboard-usb-hub/
> > > 38200000.usb:hub@1 bind uevent unbind
>
> Right, the driver creates a single platform device for each physical hub.
Thanks for confirmation.
> > But this seems better than to use a common fixed-regulator referenced by
> > both hub nodes, which just is controlled by GPIO and does not supply any
> > voltages.
> Agreed, if the GPIO controls a reset line it should be implemented as such.
>
> > Note: It might also be necessary to add bindings to specify ramp up times
> > and/or reset timeouts.
>
> The times are hub specific, not board specific, right? If that's the case
> then a binding shouldn't be needed, the timing can be derived from the
> compatible string.
Well, yes they are hub specific, but board design might influence them as
well, as in increased ramp up delay.
> > drivers/usb/misc/onboard_usb_hub.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > b/drivers/usb/misc/onboard_usb_hub.c index 6b9b949d17d3..348fb5270266
> > 100644
> > --- a/drivers/usb/misc/onboard_usb_hub.c
> > +++ b/drivers/usb/misc/onboard_usb_hub.c
> > @@ -7,6 +7,7 @@
> >
> > #include <linux/device.h>
> > #include <linux/export.h>
> >
> > +#include <linux/gpio/consumer.h>
> >
> > #include <linux/init.h>
> > #include <linux/kernel.h>
> > #include <linux/list.h>
> >
> > @@ -38,6 +39,7 @@ struct usbdev_node {
> >
> > struct onboard_hub {
> >
> > struct regulator *vdd;
> > struct device *dev;
> >
> > + struct gpio_desc *reset_gpio;
> >
> > bool always_powered_in_suspend;
> > bool is_powered_on;
> > bool going_away;
> >
> > @@ -56,6 +58,10 @@ static int onboard_hub_power_on(struct onboard_hub
> > *hub)
> >
> > return err;
> >
> > }
> >
> > + /* Deassert reset */
>
> The comment isn't really needed, it's clear from the context.
Ok, removed.
> > + usleep_range(3000, 3100);
>
> These shouldn't be hard coded. Instead you could add a model specific struct
> 'hub_data' (or similar) and associate it with the compatible string through
> onboard_hub_match.data
Will do.
> You could use fsleep() instead of usleep_range(). It does the _range part
> automatically (with a value of 2x).
Nice idea.
> > + gpiod_set_value_cansleep(hub->reset_gpio, 0);
>
> Since this includes delays maybe put the reset inside an 'if
> (hub->reset_gpio)' block. Not super important for these short delays, but
> they might be longer for some hubs.
gpiod_set_value_cansleep includes delays? Without gpio_desc it returns early
on. Or do you mean the usleep_range before?
Actually in this case the 3ms is the minimum time from VDD stable to de-
assertion of GRST. So even in case the GPIO is manged by hardware itself,
software has to wait here before proceeding, IMHO.
> > +
> >
> > hub->is_powered_on = true;
> >
> > return 0;
> >
> > @@ -65,6 +71,10 @@ static int onboard_hub_power_off(struct onboard_hub
> > *hub)>
> > {
> >
> > int err;
> >
> > + /* Assert reset */
>
> drop comment
Will do.
> > + gpiod_set_value_cansleep(hub->reset_gpio, 1);
>
> Put inside 'if (hub->reset_gpio)' to avoid unnecessary delays when no reset
> is configured.
>
> > + usleep_range(4000, 5000);
>
> Use per-model values.
Will do.
> > +
> >
> > err = regulator_disable(hub->vdd);
> > if (err) {
> >
> > dev_err(hub->dev, "failed to disable regulator: %d\n",
err);
> >
> > @@ -231,6 +241,14 @@ static int onboard_hub_probe(struct platform_device
> > *pdev)>
> > if (IS_ERR(hub->vdd))
> >
> > return PTR_ERR(hub->vdd);
> >
> > + /* Put the hub into reset, pull reset line low, and assure 4ms
reset low
> > timing. */
> drop comment, it's mostly evident from the code. Maybe not the
> usleep_range() part, but that should become clearer when per model values
> are used.
Will do.
> > + hub->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > +
GPIOD_OUT_HIGH);
> > + if (IS_ERR(hub->reset_gpio))
> > + return dev_err_probe(dev, PTR_ERR(hub->reset_gpio),
"failed to get
> > reset GPIO\n"); +
> > + usleep_range(4000, 5000);
> > +
> >
> > hub->dev = dev;
> > mutex_init(&hub->lock);
> > INIT_LIST_HEAD(&hub->udev_list);
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/3] usb: misc: onboard_usb_hub: Add reset-gpio support
2022-07-13 6:46 ` Alexander Stein
@ 2022-07-13 16:59 ` Matthias Kaehlcke
2022-07-14 6:10 ` Alexander Stein
0 siblings, 1 reply; 17+ messages in thread
From: Matthias Kaehlcke @ 2022-07-13 16:59 UTC (permalink / raw)
To: Alexander Stein
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski, linux-usb,
devicetree
Hi Alexander,
On Wed, Jul 13, 2022 at 08:46:56AM +0200, Alexander Stein wrote:
> Hi Matthias,
>
> Am Dienstag, 12. Juli 2022, 20:18:05 CEST schrieb Matthias Kaehlcke:
> > On Tue, Jul 12, 2022 at 05:06:26PM +0200, Alexander Stein wrote:
> > > Despite default reset upon probe, release reset line after powering up
> > > the hub and assert reset again before powering down.
> > >
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > ---
> > > My current DT node on my TQMa8MPxL looks like this
> > > ```
> > > &usb_dwc3_1 {
> > >
> > > dr_mode = "host";
> > > #address-cells = <1>;
> > > #size-cells = <0>;
> > > pinctrl-names = "default";
> > > pinctrl-0 = <&pinctrl_usbhub>;
> > > status = "okay";
> > >
> > > hub_2_0: hub@1 {
> > >
> > > compatible = "usb451,8142";
> > > reg = <1>;
> > > peer-hub = <&hub_3_0>;
> > > reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
> > >
> > > };
> > >
> > > hub_3_0: hub@2 {
> > >
> > > compatible = "usb451,8140";
> > > reg = <2>;
> > > peer-hub = <&hub_2_0>;
> > > reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
> > >
> > > };
> > >
> > > };
> > > ```
> > > which I don't like much for 2 reasons:
> > > * the pinctrl has to be put in a common top-node of USB hub node. The
> > > pinctrl>
> > > can not be requested twice.
> >
> > Agreed, that's not great. The pinctrl doesn't have to be necessarily in the
> > USB controller node, it could also be in the static section of the board,
> > but that isn't really much of an improvement :( Not sure there is much to
> > do given that the USB devices also process the pinctrl info (besides the
> > onboard_hub platform device doing the same).
>
> I tend to keep the pinctrl property next to the ones actually using them. But
> in this case it's not possible unfortunately.
>
> > > * Apparently there is no conflict on the reset-gpio only because just one
> > > device>
> > > gets probed here:
> > > > $ ls /sys/bus/platform/drivers/onboard-usb-hub/
> > > > 38200000.usb:hub@1 bind uevent unbind
> >
> > Right, the driver creates a single platform device for each physical hub.
>
> Thanks for confirmation.
>
> > > But this seems better than to use a common fixed-regulator referenced by
> > > both hub nodes, which just is controlled by GPIO and does not supply any
> > > voltages.
> > Agreed, if the GPIO controls a reset line it should be implemented as such.
> >
> > > Note: It might also be necessary to add bindings to specify ramp up times
> > > and/or reset timeouts.
> >
> > The times are hub specific, not board specific, right? If that's the case
> > then a binding shouldn't be needed, the timing can be derived from the
> > compatible string.
>
> Well, yes they are hub specific, but board design might influence them as
> well, as in increased ramp up delay.
Isn't the ramp up delay something that should be configured on the regulator
side with 'regulator-ramp-delay'?
> > > drivers/usb/misc/onboard_usb_hub.c | 18 ++++++++++++++++++
> > > 1 file changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > > b/drivers/usb/misc/onboard_usb_hub.c index 6b9b949d17d3..348fb5270266
> > > 100644
> > > --- a/drivers/usb/misc/onboard_usb_hub.c
> > > +++ b/drivers/usb/misc/onboard_usb_hub.c
> > > @@ -7,6 +7,7 @@
> > >
> > > #include <linux/device.h>
> > > #include <linux/export.h>
> > >
> > > +#include <linux/gpio/consumer.h>
> > >
> > > #include <linux/init.h>
> > > #include <linux/kernel.h>
> > > #include <linux/list.h>
> > >
> > > @@ -38,6 +39,7 @@ struct usbdev_node {
> > >
> > > struct onboard_hub {
> > >
> > > struct regulator *vdd;
> > > struct device *dev;
> > >
> > > + struct gpio_desc *reset_gpio;
> > >
> > > bool always_powered_in_suspend;
> > > bool is_powered_on;
> > > bool going_away;
> > >
> > > @@ -56,6 +58,10 @@ static int onboard_hub_power_on(struct onboard_hub
> > > *hub)
> > >
> > > return err;
> > >
> > > }
> > >
> > > + /* Deassert reset */
> >
> > The comment isn't really needed, it's clear from the context.
>
> Ok, removed.
>
> > > + usleep_range(3000, 3100);
> >
> > These shouldn't be hard coded. Instead you could add a model specific struct
> > 'hub_data' (or similar) and associate it with the compatible string through
> > onboard_hub_match.data
>
> Will do.
>
> > You could use fsleep() instead of usleep_range(). It does the _range part
> > automatically (with a value of 2x).
>
> Nice idea.
>
> > > + gpiod_set_value_cansleep(hub->reset_gpio, 0);
> >
> > Since this includes delays maybe put the reset inside an 'if
> > (hub->reset_gpio)' block. Not super important for these short delays, but
> > they might be longer for some hubs.
>
> gpiod_set_value_cansleep includes delays? Without gpio_desc it returns early
> on. Or do you mean the usleep_range before?
Yes, I was referring to the usleep_range() before.
> Actually in this case the 3ms is the minimum time from VDD stable to de-
> assertion of GRST. So even in case the GPIO is manged by hardware itself,
> software has to wait here before proceeding, IMHO.
Agreed.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: Re: [PATCH 2/3] usb: misc: onboard_usb_hub: Add reset-gpio support
2022-07-13 16:59 ` Matthias Kaehlcke
@ 2022-07-14 6:10 ` Alexander Stein
0 siblings, 0 replies; 17+ messages in thread
From: Alexander Stein @ 2022-07-14 6:10 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski, linux-usb,
devicetree
Hi Matthias,
Am Mittwoch, 13. Juli 2022, 18:59:44 CEST schrieb Matthias Kaehlcke:
> Hi Alexander,
>
> On Wed, Jul 13, 2022 at 08:46:56AM +0200, Alexander Stein wrote:
> > Hi Matthias,
> >
> > Am Dienstag, 12. Juli 2022, 20:18:05 CEST schrieb Matthias Kaehlcke:
> > > On Tue, Jul 12, 2022 at 05:06:26PM +0200, Alexander Stein wrote:
> > > > Despite default reset upon probe, release reset line after powering up
> > > > the hub and assert reset again before powering down.
> > > >
> > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > > ---
> > > > My current DT node on my TQMa8MPxL looks like this
> > > > ```
> > > > &usb_dwc3_1 {
> > > >
> > > > dr_mode = "host";
> > > > #address-cells = <1>;
> > > > #size-cells = <0>;
> > > > pinctrl-names = "default";
> > > > pinctrl-0 = <&pinctrl_usbhub>;
> > > > status = "okay";
> > > >
> > > > hub_2_0: hub@1 {
> > > >
> > > > compatible = "usb451,8142";
> > > > reg = <1>;
> > > > peer-hub = <&hub_3_0>;
> > > > reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
> > > >
> > > > };
> > > >
> > > > hub_3_0: hub@2 {
> > > >
> > > > compatible = "usb451,8140";
> > > > reg = <2>;
> > > > peer-hub = <&hub_2_0>;
> > > > reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
> > > >
> > > > };
> > > >
> > > > };
> > > > ```
> > > > which I don't like much for 2 reasons:
> > > > * the pinctrl has to be put in a common top-node of USB hub node. The
> > > > pinctrl>
> > > >
> > > > can not be requested twice.
> > >
> > > Agreed, that's not great. The pinctrl doesn't have to be necessarily in
> > > the
> > > USB controller node, it could also be in the static section of the
> > > board,
> > > but that isn't really much of an improvement :( Not sure there is much
> > > to
> > > do given that the USB devices also process the pinctrl info (besides the
> > > onboard_hub platform device doing the same).
> >
> > I tend to keep the pinctrl property next to the ones actually using them.
> > But in this case it's not possible unfortunately.
> >
> > > > * Apparently there is no conflict on the reset-gpio only because just
> > > > one
> > > > device>
> > > >
> > > > gets probed here:
> > > > > $ ls /sys/bus/platform/drivers/onboard-usb-hub/
> > > > > 38200000.usb:hub@1 bind uevent unbind
> > >
> > > Right, the driver creates a single platform device for each physical
> > > hub.
> >
> > Thanks for confirmation.
> >
> > > > But this seems better than to use a common fixed-regulator referenced
> > > > by
> > > > both hub nodes, which just is controlled by GPIO and does not supply
> > > > any
> > > > voltages.
> > >
> > > Agreed, if the GPIO controls a reset line it should be implemented as
> > > such.
> > >
> > > > Note: It might also be necessary to add bindings to specify ramp up
> > > > times
> > > > and/or reset timeouts.
> > >
> > > The times are hub specific, not board specific, right? If that's the
> > > case
> > > then a binding shouldn't be needed, the timing can be derived from the
> > > compatible string.
> >
> > Well, yes they are hub specific, but board design might influence them as
> > well, as in increased ramp up delay.
>
> Isn't the ramp up delay something that should be configured on the regulator
> side with 'regulator-ramp-delay'?
Sure, if you have a regulators you can do that. But even for the reset GPIO
lines an RC circuit can stretch the ramp up. AFAIK there is no way to handle
this despite inserting a waiting time in driver code itself.
For now this is good, but it might be necessary to accompany for that at some
point.
Regards,
Alexander
> > > > drivers/usb/misc/onboard_usb_hub.c | 18 ++++++++++++++++++
> > > > 1 file changed, 18 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > > > b/drivers/usb/misc/onboard_usb_hub.c index 6b9b949d17d3..348fb5270266
> > > > 100644
> > > > --- a/drivers/usb/misc/onboard_usb_hub.c
> > > > +++ b/drivers/usb/misc/onboard_usb_hub.c
> > > > @@ -7,6 +7,7 @@
> > > >
> > > > #include <linux/device.h>
> > > > #include <linux/export.h>
> > > >
> > > > +#include <linux/gpio/consumer.h>
> > > >
> > > > #include <linux/init.h>
> > > > #include <linux/kernel.h>
> > > > #include <linux/list.h>
> > > >
> > > > @@ -38,6 +39,7 @@ struct usbdev_node {
> > > >
> > > > struct onboard_hub {
> > > >
> > > > struct regulator *vdd;
> > > > struct device *dev;
> > > >
> > > > + struct gpio_desc *reset_gpio;
> > > >
> > > > bool always_powered_in_suspend;
> > > > bool is_powered_on;
> > > > bool going_away;
> > > >
> > > > @@ -56,6 +58,10 @@ static int onboard_hub_power_on(struct onboard_hub
> > > > *hub)
> > > >
> > > > return err;
> > > >
> > > > }
> > > >
> > > > + /* Deassert reset */
> > >
> > > The comment isn't really needed, it's clear from the context.
> >
> > Ok, removed.
> >
> > > > + usleep_range(3000, 3100);
> > >
> > > These shouldn't be hard coded. Instead you could add a model specific
> > > struct 'hub_data' (or similar) and associate it with the compatible
> > > string through onboard_hub_match.data
> >
> > Will do.
> >
> > > You could use fsleep() instead of usleep_range(). It does the _range
> > > part
> > > automatically (with a value of 2x).
> >
> > Nice idea.
> >
> > > > + gpiod_set_value_cansleep(hub->reset_gpio, 0);
> > >
> > > Since this includes delays maybe put the reset inside an 'if
> > > (hub->reset_gpio)' block. Not super important for these short delays,
> > > but
> > > they might be longer for some hubs.
> >
> > gpiod_set_value_cansleep includes delays? Without gpio_desc it returns
> > early on. Or do you mean the usleep_range before?
>
> Yes, I was referring to the usleep_range() before.
>
> > Actually in this case the 3ms is the minimum time from VDD stable to de-
> > assertion of GRST. So even in case the GPIO is manged by hardware itself,
> > software has to wait here before proceeding, IMHO.
>
> Agreed.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] usb: misc: onboard_usb_hub: Add TI USB8041 hub support
2022-07-12 15:06 [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller Alexander Stein
2022-07-12 15:06 ` [PATCH 2/3] usb: misc: onboard_usb_hub: Add reset-gpio support Alexander Stein
@ 2022-07-12 15:06 ` Alexander Stein
2022-07-12 17:25 ` [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller Matthias Kaehlcke
2022-07-12 21:16 ` Krzysztof Kozlowski
3 siblings, 0 replies; 17+ messages in thread
From: Alexander Stein @ 2022-07-12 15:06 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Matthias Kaehlcke
Cc: Alexander Stein, linux-usb, devicetree
This is a 4-port 3.0 USB hub.
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
drivers/usb/misc/onboard_usb_hub.c | 3 +++
drivers/usb/misc/onboard_usb_hub.h | 2 ++
2 files changed, 5 insertions(+)
diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index 348fb5270266..5c07cd64a4ef 100644
--- a/drivers/usb/misc/onboard_usb_hub.c
+++ b/drivers/usb/misc/onboard_usb_hub.c
@@ -328,6 +328,7 @@ static struct platform_driver onboard_hub_driver = {
/************************** USB driver **************************/
#define VENDOR_ID_REALTEK 0x0bda
+#define VENDOR_ID_TI 0x0451
/*
* Returns the onboard_hub platform device that is associated with the USB
@@ -405,6 +406,8 @@ static const struct usb_device_id onboard_hub_id_table[] = {
{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
{ USB_DEVICE(VENDOR_ID_REALTEK, 0x0414) }, /* RTS5414 USB 3.2 */
{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5414) }, /* RTS5414 USB 2.1 */
+ { USB_DEVICE(VENDOR_ID_TI, 0x8140) }, /* TI USB8041 3.0 */
+ { USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
{}
};
MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
index d3a5b6938582..a3a988fbc740 100644
--- a/drivers/usb/misc/onboard_usb_hub.h
+++ b/drivers/usb/misc/onboard_usb_hub.h
@@ -7,6 +7,8 @@
#define _USB_MISC_ONBOARD_USB_HUB_H
static const struct of_device_id onboard_hub_match[] = {
+ { .compatible = "usb451,8140" },
+ { .compatible = "usb451,8142" },
{ .compatible = "usbbda,411" },
{ .compatible = "usbbda,5411" },
{ .compatible = "usbbda,414" },
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller
2022-07-12 15:06 [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller Alexander Stein
2022-07-12 15:06 ` [PATCH 2/3] usb: misc: onboard_usb_hub: Add reset-gpio support Alexander Stein
2022-07-12 15:06 ` [PATCH 3/3] usb: misc: onboard_usb_hub: Add TI USB8041 hub support Alexander Stein
@ 2022-07-12 17:25 ` Matthias Kaehlcke
2022-07-12 21:12 ` Krzysztof Kozlowski
2022-07-12 21:16 ` Krzysztof Kozlowski
3 siblings, 1 reply; 17+ messages in thread
From: Matthias Kaehlcke @ 2022-07-12 17:25 UTC (permalink / raw)
To: Alexander Stein
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski, linux-usb,
devicetree
Hi Alexander,
On Tue, Jul 12, 2022 at 05:06:25PM +0200, Alexander Stein wrote:
> The TI USB8041 is a USB 3.0 hub controller with 4 ports.
>
> This initial version of the binding only describes USB related aspects
> of the USB8041, it does not cover the option of connecting the controller
> as an i2c slave.
>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Well, this is essentially a ripoff of
> Documentation/devicetree/bindings/usb/realtek,rts5411.yaml with USB IDs
> replaced, reset-gpio added and example adjusted.
> IMHO this should be merged together with realtek,rts5411.yaml. Is it ok
> to rename bindings files? I guess a common onboard-usb-hub.yaml matching
> the driver seens reasonable. Any recommendations how to proceed?
It's a tradeoff between keeping the individual bindings simple and avoid
unnecessary duplication. The current RTS5411 and TI USB8041 bindings are
very similar, which suggests combining them. However over time hubs with
diverging features could be added (e.g. with multiple regulators, a link
to an I2C/SPI bus, a clock, ...). With that a common binding might become
too messy.
From a quick look through Documentation/devicetree/bindings it doesn't
seem common to have generic bindings that cover components from multiple
vendors. In that sense I'm leaning towards separate bindings.
Rob, do you have any particular preference or suggestion?
m.
> .../devicetree/bindings/usb/ti,usb8041.yaml | 69 +++++++++++++++++++
> 1 file changed, 69 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/usb/ti,usb8041.yaml
>
> diff --git a/Documentation/devicetree/bindings/usb/ti,usb8041.yaml b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> new file mode 100644
> index 000000000000..9a49d60527b1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/ti,usb8041.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Binding for the TI USB8041 USB 3.0 hub controller
> +
> +maintainers:
> + - Matthias Kaehlcke <mka@chromium.org>
> +
> +allOf:
> + - $ref: usb-device.yaml#
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - usb451,8140
> + - usb451,8142
> +
> + reg: true
> +
> + reset-gpio:
> + maxItems: 1
> + description:
> + GPIO specifier for GSRT# pin.
> +
> + vdd-supply:
> + description:
> + phandle to the regulator that provides power to the hub.
> +
> + peer-hub:
> + $ref: '/schemas/types.yaml#/definitions/phandle'
> + description:
> + phandle to the peer hub on the controller.
> +
> +required:
> + - peer-hub
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> +
> + usb {
> + dr_mode = "host";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + /* 2.0 hub on port 1 */
> + hub_2_0: hub@1 {
> + compatible = "usb451,8142";
> + reg = <1>;
> + peer-hub = <&hub_3_0>;
> + reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
> + };
> +
> + /* 3.0 hub on port 2 */
> + hub_3_0: hub@2 {
> + compatible = "usb451,8140";
> + reg = <2>;
> + peer-hub = <&hub_2_0>;
> + reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
> + };
> + };
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller
2022-07-12 17:25 ` [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller Matthias Kaehlcke
@ 2022-07-12 21:12 ` Krzysztof Kozlowski
2022-07-12 21:25 ` Matthias Kaehlcke
0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-12 21:12 UTC (permalink / raw)
To: Matthias Kaehlcke, Alexander Stein
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski, linux-usb,
devicetree
On 12/07/2022 19:25, Matthias Kaehlcke wrote:
> Hi Alexander,
>
> On Tue, Jul 12, 2022 at 05:06:25PM +0200, Alexander Stein wrote:
>> The TI USB8041 is a USB 3.0 hub controller with 4 ports.
>>
>> This initial version of the binding only describes USB related aspects
>> of the USB8041, it does not cover the option of connecting the controller
>> as an i2c slave.
>>
>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>> ---
>> Well, this is essentially a ripoff of
>> Documentation/devicetree/bindings/usb/realtek,rts5411.yaml with USB IDs
>> replaced, reset-gpio added and example adjusted.
>> IMHO this should be merged together with realtek,rts5411.yaml. Is it ok
>> to rename bindings files? I guess a common onboard-usb-hub.yaml matching
>> the driver seens reasonable. Any recommendations how to proceed?
>
> It's a tradeoff between keeping the individual bindings simple and avoid
> unnecessary duplication. The current RTS5411 and TI USB8041 bindings are
> very similar, which suggests combining them. However over time hubs with
> diverging features could be added (e.g. with multiple regulators, a link
> to an I2C/SPI bus, a clock, ...). With that a common binding might become
> too messy.
>
> From a quick look through Documentation/devicetree/bindings it doesn't
> seem common to have generic bindings that cover components from multiple
> vendors. In that sense I'm leaning towards separate bindings.
>
> Rob, do you have any particular preference or suggestion?
Not Rob, but my suggestion is not to merge bindings of unrelated
devices, even if they are the same class. By unrelated I mean, made by
different companies, designed differently and having nothing in common
by design. Bindings can be still similar, but should not be merged just
because they are similar.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller
2022-07-12 21:12 ` Krzysztof Kozlowski
@ 2022-07-12 21:25 ` Matthias Kaehlcke
2022-07-12 21:32 ` Krzysztof Kozlowski
0 siblings, 1 reply; 17+ messages in thread
From: Matthias Kaehlcke @ 2022-07-12 21:25 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Alexander Stein, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, linux-usb, devicetree
On Tue, Jul 12, 2022 at 11:12:06PM +0200, Krzysztof Kozlowski wrote:
> On 12/07/2022 19:25, Matthias Kaehlcke wrote:
> > Hi Alexander,
> >
> > On Tue, Jul 12, 2022 at 05:06:25PM +0200, Alexander Stein wrote:
> >> The TI USB8041 is a USB 3.0 hub controller with 4 ports.
> >>
> >> This initial version of the binding only describes USB related aspects
> >> of the USB8041, it does not cover the option of connecting the controller
> >> as an i2c slave.
> >>
> >> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> >> ---
> >> Well, this is essentially a ripoff of
> >> Documentation/devicetree/bindings/usb/realtek,rts5411.yaml with USB IDs
> >> replaced, reset-gpio added and example adjusted.
> >> IMHO this should be merged together with realtek,rts5411.yaml. Is it ok
> >> to rename bindings files? I guess a common onboard-usb-hub.yaml matching
> >> the driver seens reasonable. Any recommendations how to proceed?
> >
> > It's a tradeoff between keeping the individual bindings simple and avoid
> > unnecessary duplication. The current RTS5411 and TI USB8041 bindings are
> > very similar, which suggests combining them. However over time hubs with
> > diverging features could be added (e.g. with multiple regulators, a link
> > to an I2C/SPI bus, a clock, ...). With that a common binding might become
> > too messy.
> >
> > From a quick look through Documentation/devicetree/bindings it doesn't
> > seem common to have generic bindings that cover components from multiple
> > vendors. In that sense I'm leaning towards separate bindings.
> >
> > Rob, do you have any particular preference or suggestion?
>
> Not Rob, but my suggestion is not to merge bindings of unrelated
> devices, even if they are the same class. By unrelated I mean, made by
> different companies, designed differently and having nothing in common
> by design. Bindings can be still similar, but should not be merged just
> because they are similar.
Thanks for your advice, let's keep separate bindings then.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller
2022-07-12 21:25 ` Matthias Kaehlcke
@ 2022-07-12 21:32 ` Krzysztof Kozlowski
2022-07-13 6:09 ` Alexander Stein
0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-12 21:32 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Alexander Stein, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, linux-usb, devicetree
On 12/07/2022 23:25, Matthias Kaehlcke wrote:
> On Tue, Jul 12, 2022 at 11:12:06PM +0200, Krzysztof Kozlowski wrote:
>> On 12/07/2022 19:25, Matthias Kaehlcke wrote:
>>> Hi Alexander,
>>>
>>> On Tue, Jul 12, 2022 at 05:06:25PM +0200, Alexander Stein wrote:
>>>> The TI USB8041 is a USB 3.0 hub controller with 4 ports.
>>>>
>>>> This initial version of the binding only describes USB related aspects
>>>> of the USB8041, it does not cover the option of connecting the controller
>>>> as an i2c slave.
>>>>
>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>>> ---
>>>> Well, this is essentially a ripoff of
>>>> Documentation/devicetree/bindings/usb/realtek,rts5411.yaml with USB IDs
>>>> replaced, reset-gpio added and example adjusted.
>>>> IMHO this should be merged together with realtek,rts5411.yaml. Is it ok
>>>> to rename bindings files? I guess a common onboard-usb-hub.yaml matching
>>>> the driver seens reasonable. Any recommendations how to proceed?
>>>
>>> It's a tradeoff between keeping the individual bindings simple and avoid
>>> unnecessary duplication. The current RTS5411 and TI USB8041 bindings are
>>> very similar, which suggests combining them. However over time hubs with
>>> diverging features could be added (e.g. with multiple regulators, a link
>>> to an I2C/SPI bus, a clock, ...). With that a common binding might become
>>> too messy.
>>>
>>> From a quick look through Documentation/devicetree/bindings it doesn't
>>> seem common to have generic bindings that cover components from multiple
>>> vendors. In that sense I'm leaning towards separate bindings.
>>>
>>> Rob, do you have any particular preference or suggestion?
>>
>> Not Rob, but my suggestion is not to merge bindings of unrelated
>> devices, even if they are the same class. By unrelated I mean, made by
>> different companies, designed differently and having nothing in common
>> by design. Bindings can be still similar, but should not be merged just
>> because they are similar.
>
> Thanks for your advice, let's keep separate bindings then.
Although for the record let me add that we did merge some trivial hwmon
devices like LM75 or LM90 but their bindings are trivial and programming
model is also similar between each other (handled by same device
driver). I guess we can be here flexible, so the question would be how
similar these USB hubs are.
If in doubt, just keep it separate.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller
2022-07-12 21:32 ` Krzysztof Kozlowski
@ 2022-07-13 6:09 ` Alexander Stein
0 siblings, 0 replies; 17+ messages in thread
From: Alexander Stein @ 2022-07-13 6:09 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Matthias Kaehlcke, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, linux-usb, devicetree
Hello Krzysztof,
Am Dienstag, 12. Juli 2022, 23:32:12 CEST schrieb Krzysztof Kozlowski:
> On 12/07/2022 23:25, Matthias Kaehlcke wrote:
> > On Tue, Jul 12, 2022 at 11:12:06PM +0200, Krzysztof Kozlowski wrote:
> >> On 12/07/2022 19:25, Matthias Kaehlcke wrote:
> >>> Hi Alexander,
> >>>
> >>> On Tue, Jul 12, 2022 at 05:06:25PM +0200, Alexander Stein wrote:
> >>>> The TI USB8041 is a USB 3.0 hub controller with 4 ports.
> >>>>
> >>>> This initial version of the binding only describes USB related aspects
> >>>> of the USB8041, it does not cover the option of connecting the
> >>>> controller
> >>>> as an i2c slave.
> >>>>
> >>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> >>>> ---
> >>>> Well, this is essentially a ripoff of
> >>>> Documentation/devicetree/bindings/usb/realtek,rts5411.yaml with USB IDs
> >>>> replaced, reset-gpio added and example adjusted.
> >>>> IMHO this should be merged together with realtek,rts5411.yaml. Is it ok
> >>>> to rename bindings files? I guess a common onboard-usb-hub.yaml
> >>>> matching
> >>>> the driver seens reasonable. Any recommendations how to proceed?
> >>>
> >>> It's a tradeoff between keeping the individual bindings simple and avoid
> >>> unnecessary duplication. The current RTS5411 and TI USB8041 bindings are
> >>> very similar, which suggests combining them. However over time hubs with
> >>> diverging features could be added (e.g. with multiple regulators, a link
> >>> to an I2C/SPI bus, a clock, ...). With that a common binding might
> >>> become
> >>> too messy.
> >>>
> >>> From a quick look through Documentation/devicetree/bindings it doesn't
> >>> seem common to have generic bindings that cover components from multiple
> >>> vendors. In that sense I'm leaning towards separate bindings.
> >>>
> >>> Rob, do you have any particular preference or suggestion?
> >>
> >> Not Rob, but my suggestion is not to merge bindings of unrelated
> >> devices, even if they are the same class. By unrelated I mean, made by
> >> different companies, designed differently and having nothing in common
> >> by design. Bindings can be still similar, but should not be merged just
> >> because they are similar.
> >
> > Thanks for your advice, let's keep separate bindings then.
Ok, thanks for the feedback.
> Although for the record let me add that we did merge some trivial hwmon
> devices like LM75 or LM90 but their bindings are trivial and programming
> model is also similar between each other (handled by same device
> driver). I guess we can be here flexible, so the question would be how
> similar these USB hubs are.
>
> If in doubt, just keep it separate.
Right now it might seem sensible to have the bindings merged, as the features
are quite similar. But things might change, if/once i2c support is added. So
this is one additional matter to keep them separated.
Best regards,
Alexander
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller
2022-07-12 15:06 [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller Alexander Stein
` (2 preceding siblings ...)
2022-07-12 17:25 ` [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller Matthias Kaehlcke
@ 2022-07-12 21:16 ` Krzysztof Kozlowski
2022-07-12 21:28 ` Matthias Kaehlcke
2022-07-13 7:20 ` Alexander Stein
3 siblings, 2 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-12 21:16 UTC (permalink / raw)
To: Alexander Stein, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Matthias Kaehlcke
Cc: linux-usb, devicetree
On 12/07/2022 17:06, Alexander Stein wrote:
> The TI USB8041 is a USB 3.0 hub controller with 4 ports.
>
> This initial version of the binding only describes USB related aspects
> of the USB8041, it does not cover the option of connecting the controller
> as an i2c slave.
>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Well, this is essentially a ripoff of
> Documentation/devicetree/bindings/usb/realtek,rts5411.yaml with USB IDs
> replaced, reset-gpio added and example adjusted.
> IMHO this should be merged together with realtek,rts5411.yaml. Is it ok
> to rename bindings files? I guess a common onboard-usb-hub.yaml matching
> the driver seens reasonable. Any recommendations how to proceed?
>
> .../devicetree/bindings/usb/ti,usb8041.yaml | 69 +++++++++++++++++++
> 1 file changed, 69 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/usb/ti,usb8041.yaml
>
> diff --git a/Documentation/devicetree/bindings/usb/ti,usb8041.yaml b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> new file mode 100644
> index 000000000000..9a49d60527b1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/ti,usb8041.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Binding for the TI USB8041 USB 3.0 hub controller
> +
> +maintainers:
> + - Matthias Kaehlcke <mka@chromium.org>
> +
> +allOf:
> + - $ref: usb-device.yaml#
> +
> +properties:
> + compatible:
> + items:
No items. It's just one item.
> + - enum:
> + - usb451,8140
> + - usb451,8142
> +
> + reg: true
> +
> + reset-gpio:
reset-gpios
> + maxItems: 1
> + description:
> + GPIO specifier for GSRT# pin.
Combine maxItems and above into:
items:
- description: GPIO specifier for GSRT# pin.
> +
> + vdd-supply:
> + description:
> + phandle to the regulator that provides power to the hub.
s/phandle to the regulator that provides//
and create some nice sentence from left-over, like "VDD power supply to
the hub"
> +
> + peer-hub:
> + $ref: '/schemas/types.yaml#/definitions/phandle'
No quotes.
> + description:
> + phandle to the peer hub on the controller.
> +
> +required:
> + - peer-hub
> + - compatible
> + - reg
Messed order. Use same as they appear in properties, so
compatible+reg+peer-hub.
But another question - why "peer-hub"? I remember some discussion about
naming, so was peer preferred over companion?
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> +
> + usb {
> + dr_mode = "host";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + /* 2.0 hub on port 1 */
> + hub_2_0: hub@1 {
> + compatible = "usb451,8142";
> + reg = <1>;
> + peer-hub = <&hub_3_0>;
> + reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
reset-gpios
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller
2022-07-12 21:16 ` Krzysztof Kozlowski
@ 2022-07-12 21:28 ` Matthias Kaehlcke
2022-07-12 21:32 ` Krzysztof Kozlowski
2022-07-13 7:20 ` Alexander Stein
1 sibling, 1 reply; 17+ messages in thread
From: Matthias Kaehlcke @ 2022-07-12 21:28 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Alexander Stein, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, linux-usb, devicetree
On Tue, Jul 12, 2022 at 11:16:21PM +0200, Krzysztof Kozlowski wrote:
> On 12/07/2022 17:06, Alexander Stein wrote:
> > The TI USB8041 is a USB 3.0 hub controller with 4 ports.
> >
> > This initial version of the binding only describes USB related aspects
> > of the USB8041, it does not cover the option of connecting the controller
> > as an i2c slave.
> >
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Well, this is essentially a ripoff of
> > Documentation/devicetree/bindings/usb/realtek,rts5411.yaml with USB IDs
> > replaced, reset-gpio added and example adjusted.
> > IMHO this should be merged together with realtek,rts5411.yaml. Is it ok
> > to rename bindings files? I guess a common onboard-usb-hub.yaml matching
> > the driver seens reasonable. Any recommendations how to proceed?
> >
> > .../devicetree/bindings/usb/ti,usb8041.yaml | 69 +++++++++++++++++++
> > 1 file changed, 69 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ti,usb8041.yaml b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> > new file mode 100644
> > index 000000000000..9a49d60527b1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/ti,usb8041.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Binding for the TI USB8041 USB 3.0 hub controller
> > +
> > +maintainers:
> > + - Matthias Kaehlcke <mka@chromium.org>
> > +
> > +allOf:
> > + - $ref: usb-device.yaml#
> > +
> > +properties:
> > + compatible:
> > + items:
>
> No items. It's just one item.
>
> > + - enum:
> > + - usb451,8140
> > + - usb451,8142
> > +
> > + reg: true
> > +
> > + reset-gpio:
>
> reset-gpios
>
> > + maxItems: 1
> > + description:
> > + GPIO specifier for GSRT# pin.
>
> Combine maxItems and above into:
> items:
> - description: GPIO specifier for GSRT# pin.
>
> > +
> > + vdd-supply:
> > + description:
> > + phandle to the regulator that provides power to the hub.
>
> s/phandle to the regulator that provides//
> and create some nice sentence from left-over, like "VDD power supply to
> the hub"
>
>
> > +
> > + peer-hub:
> > + $ref: '/schemas/types.yaml#/definitions/phandle'
>
> No quotes.
>
> > + description:
> > + phandle to the peer hub on the controller.
> > +
> > +required:
> > + - peer-hub
> > + - compatible
> > + - reg
>
> Messed order. Use same as they appear in properties, so
> compatible+reg+peer-hub.
>
> But another question - why "peer-hub"? I remember some discussion about
> naming, so was peer preferred over companion?
Yes, Alan Stern pointed out that 'companion' can be confusing in the context
of USB:
What do you mean by "companion hub"? I think you are using the wrong
word here. If you're talking about the relation between the two logical
hubs (one attached to the SuperSpeed bus and one attached to the
Low/Full/High-speed bus) within a physical USB-3 hub, the correct term
for this is "peer". See the existing usages in hub.h, hub.c, and
port.c.
"Companion" refers to something completely different (i.e., the UHCI or
OHCI controllers that handle Low/Full-speed connections on behalf of a
High-speed EHCI controller).
https://patchwork.kernel.org/comment/24912563/
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller
2022-07-12 21:28 ` Matthias Kaehlcke
@ 2022-07-12 21:32 ` Krzysztof Kozlowski
0 siblings, 0 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-12 21:32 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Alexander Stein, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, linux-usb, devicetree
On 12/07/2022 23:28, Matthias Kaehlcke wrote:
>>
>> But another question - why "peer-hub"? I remember some discussion about
>> naming, so was peer preferred over companion?
>
> Yes, Alan Stern pointed out that 'companion' can be confusing in the context
> of USB:
>
> What do you mean by "companion hub"? I think you are using the wrong
> word here. If you're talking about the relation between the two logical
> hubs (one attached to the SuperSpeed bus and one attached to the
> Low/Full/High-speed bus) within a physical USB-3 hub, the correct term
> for this is "peer". See the existing usages in hub.h, hub.c, and
> port.c.
>
> "Companion" refers to something completely different (i.e., the UHCI or
> OHCI controllers that handle Low/Full-speed connections on behalf of a
> High-speed EHCI controller).
>
> https://patchwork.kernel.org/comment/24912563/
Thanks, that explains a lot!
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re: [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller
2022-07-12 21:16 ` Krzysztof Kozlowski
2022-07-12 21:28 ` Matthias Kaehlcke
@ 2022-07-13 7:20 ` Alexander Stein
2022-07-13 7:58 ` Krzysztof Kozlowski
1 sibling, 1 reply; 17+ messages in thread
From: Alexander Stein @ 2022-07-13 7:20 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Matthias Kaehlcke, linux-usb, devicetree
Hi Krzysztof,
thanks for the feedback on DT bindings.
Am Dienstag, 12. Juli 2022, 23:16:21 CEST schrieb Krzysztof Kozlowski:
> On 12/07/2022 17:06, Alexander Stein wrote:
> > The TI USB8041 is a USB 3.0 hub controller with 4 ports.
> >
> > This initial version of the binding only describes USB related aspects
> > of the USB8041, it does not cover the option of connecting the controller
> > as an i2c slave.
> >
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Well, this is essentially a ripoff of
> > Documentation/devicetree/bindings/usb/realtek,rts5411.yaml with USB IDs
> > replaced, reset-gpio added and example adjusted.
> > IMHO this should be merged together with realtek,rts5411.yaml. Is it ok
> > to rename bindings files? I guess a common onboard-usb-hub.yaml matching
> > the driver seens reasonable. Any recommendations how to proceed?
> >
> > .../devicetree/bindings/usb/ti,usb8041.yaml | 69 +++++++++++++++++++
> > 1 file changed, 69 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> > b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml new file mode
> > 100644
> > index 000000000000..9a49d60527b1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/ti,usb8041.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/ti,usb8041.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Binding for the TI USB8041 USB 3.0 hub controller
> > +
> > +maintainers:
> > + - Matthias Kaehlcke <mka@chromium.org>
> > +
> > +allOf:
> > + - $ref: usb-device.yaml#
> > +
> > +properties:
> > + compatible:
>
> > + items:
> No items. It's just one item.
Sure, will change.
> > + - enum:
> > + - usb451,8140
> > + - usb451,8142
> > +
> > + reg: true
> > +
>
> > + reset-gpio:
> reset-gpios
Will change.
> > + maxItems: 1
> > + description:
> > + GPIO specifier for GSRT# pin.
>
> Combine maxItems and above into:
> items:
> - description: GPIO specifier for GSRT# pin.
Will change, looks much nicer.
> > +
> > + vdd-supply:
> > + description:
> > + phandle to the regulator that provides power to the hub.
>
> s/phandle to the regulator that provides//
> and create some nice sentence from left-over, like "VDD power supply to
> the hub"
Thanks for that suggestion. Will change.
> > +
> > + peer-hub:
> > + $ref: '/schemas/types.yaml#/definitions/phandle'
>
> No quotes.
Sure, will do so.
> > + description:
> > + phandle to the peer hub on the controller.
> > +
> > +required:
> > + - peer-hub
> > + - compatible
> > + - reg
>
> Messed order. Use same as they appear in properties, so
> compatible+reg+peer-hub.
>
> But another question - why "peer-hub"? I remember some discussion about
> naming, so was peer preferred over companion?
>
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/gpio/gpio.h>
> > +
> > + usb {
> > + dr_mode = "host";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + /* 2.0 hub on port 1 */
> > + hub_2_0: hub@1 {
> > + compatible = "usb451,8142";
> > + reg = <1>;
> > + peer-hub = <&hub_3_0>;
> > + reset-gpio = <&gpio1 11 GPIO_ACTIVE_LOW>;
>
> reset-gpios
Yes, 'make dt_binding_check' does not raise any error about this binding.
Thanks and best regards,
Alexander
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller
2022-07-13 7:20 ` Alexander Stein
@ 2022-07-13 7:58 ` Krzysztof Kozlowski
0 siblings, 0 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-13 7:58 UTC (permalink / raw)
To: Alexander Stein
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Matthias Kaehlcke, linux-usb, devicetree
On 13/07/2022 09:20, Alexander Stein wrote:
> Yes, 'make dt_binding_check' does not raise any error about this binding.
Yes, I know. reset-gpio is still accepted, but the preferred (and
documented) naming for all GPIO properties is always "-gpios", even for
one-GPIO properties.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread