On Wed, Jul 20, 2022 at 09:15:12AM +0200, Alexander Stein wrote:
> Hi Matthias,
>
> Am Freitag, 15. Juli 2022, 21:44:12 CEST schrieb Matthias Kaehlcke:
> > On Fri, Jul 15, 2022 at 09:32:59AM +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>
> > > ---
> > > Changes in v2:
> > > * Use device specific sleep times, if present
> > > * Use fsleep instead of usleep_range
> > >
> > > drivers/usb/misc/onboard_usb_hub.c | 29 +++++++++++++++++++++++++++++
> > > drivers/usb/misc/onboard_usb_hub.h | 5 +++++
> > > 2 files changed, 34 insertions(+)
> > >
> > > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > > b/drivers/usb/misc/onboard_usb_hub.c index 6b9b949d17d3..1dd7f4767def
> > > 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,8 @@ struct usbdev_node {
> > >
> > > struct onboard_hub {
> > >
> > > struct regulator *vdd;
> > > struct device *dev;
> > >
> > > + const struct onboard_hub_devtype_data *devtype_data;
> >
> > This kind of device specific data is often call platform data, let's
> > call the struct 'onboard_hub_pdata' and the field 'pdata'?
>
> I took flexcan as a reference, but I do not mind using other names if that is
> preferred.
>
> > > + struct gpio_desc *reset_gpio;
> > >
> > > bool always_powered_in_suspend;
> > > bool is_powered_on;
> > > bool going_away;
> > >
> > > @@ -56,6 +59,11 @@ static int onboard_hub_power_on(struct onboard_hub
> > > *hub)
> > >
> > > return err;
> > >
> > > }
> > >
> > > + if (hub->devtype_data)
> >
> > Instead of these checks let's make sure all hubs have pdata. Actually your
> > onboard_hub_probe() already esnures that the field is assigned.
>
> For Realtek hubs (no platform data yet), of_id->data therefore hub-
> >devtype_data is NULL.
> But I agree that providing platformdata for all hubs seems reasonable to get
> rid of these checks.
>
> > > + fsleep(hub->devtype_data->power_stable_time);
> > > + if (hub->reset_gpio)
> > > + gpiod_set_value_cansleep(hub->reset_gpio, 0);
> >
> > I would have expected:
> >
> > if (hub->reset_gpio) {
> > fsleep(hub->devtype_data->power_stable_time);
> > gpiod_set_value_cansleep(hub->reset_gpio, 0);
> > }
> >
> > For the TUSB8041 the 'power_stable_time' (td2 in the datasheet) is "VDD and
> > VDD33 stable before de-assertion of GRSTz", so no delay without reset.
>
> Your suggestion only works if you control the reset line (GRSTz) by GPIO. If
> this line is controlled by hardware using RC circuits this waiting time still
> has to be respected when power is supplied.
> I prefer that the USB hub is properly reset once this function exits. Without
> waiting time the hardware controlled GRSTz might not yet be at a proper level.
Ok, in that case let's omit the check for 'hub->reset_gpio' and just do:
fsleep(hub->devtype_data->power_stable_time);
gpiod_set_value_cansleep(hub->reset_gpio, 0);
The point of adding the check was to skip the delay when no reset GPIO is
specified, but apparently that's not what we actually want.
