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.
> > 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);
