| Message ID | 20220712150627.1444761-2-alexander.stein@ew.tq-group.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller | expand |
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 >
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);
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.
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);