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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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.
