| Message ID | 20220715073300.868087-2-alexander.stein@ew.tq-group.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller | expand |
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@ew.tq-group.com> > --- > 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'? > + 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. > + 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. > + > hub->is_powered_on = true; > > return 0; > @@ -65,6 +73,12 @@ static int onboard_hub_power_off(struct onboard_hub *hub) > { > int err; > > + if (hub->reset_gpio) { > + gpiod_set_value_cansleep(hub->reset_gpio, 1); > + if (hub->devtype_data) > + fsleep(hub->devtype_data->reset_duration); > + } > + > err = regulator_disable(hub->vdd); > if (err) { > dev_err(hub->dev, "failed to disable regulator: %d\n", err); > @@ -219,6 +233,7 @@ static void onboard_hub_attach_usb_driver(struct work_struct *work) > > static int onboard_hub_probe(struct platform_device *pdev) > { > + const struct of_device_id *of_id; > struct device *dev = &pdev->dev; > struct onboard_hub *hub; > int err; > @@ -227,10 +242,24 @@ static int onboard_hub_probe(struct platform_device *pdev) > if (!hub) > return -ENOMEM; > > + of_id = of_match_device(onboard_hub_match, &pdev->dev); > + if (of_id) > + hub->devtype_data = of_id->data; > + else > + return -ENODEV; Please change to: if (!of_id) return -ENODEV; hub->pdata = of_id->data; With the currently supported Realtek hubs will fail to probe. Please add a pdata for the RTS541x hubs. The have no reset signal, so the values can be 0. > + > hub->vdd = devm_regulator_get(dev, "vdd"); > if (IS_ERR(hub->vdd)) > return PTR_ERR(hub->vdd); > > + 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"); > + > + if (hub->devtype_data && hub->reset_gpio) drop check 'hub->devtype_data' check > + fsleep(hub->devtype_data->reset_duration); > + > hub->dev = dev; > mutex_init(&hub->lock); > INIT_LIST_HEAD(&hub->udev_list); > diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h > index d3a5b6938582..7e743f4c8aaa 100644 > --- a/drivers/usb/misc/onboard_usb_hub.h > +++ b/drivers/usb/misc/onboard_usb_hub.h > @@ -6,6 +6,11 @@ > #ifndef _USB_MISC_ONBOARD_USB_HUB_H > #define _USB_MISC_ONBOARD_USB_HUB_H > > +struct onboard_hub_devtype_data { > + unsigned long power_stable_time; /* power stabilization time in us */ > + unsigned long reset_duration; /* reset pulse width in us */ Let's encode the unit in the field name: unsigned long power_stable_us; unsigned long reset_us; > +}; Is it necessary to define this struct in onboard_usb_hub.h? It seems it could as well be defined in onboard_usb_hub.c.
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@ew.tq-group.com> > > --- > > 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. > > + > > > > hub->is_powered_on = true; > > > > return 0; > > > > @@ -65,6 +73,12 @@ static int onboard_hub_power_off(struct onboard_hub > > *hub)> > > { > > > > int err; > > > > + if (hub->reset_gpio) { > > + gpiod_set_value_cansleep(hub->reset_gpio, 1); > > + if (hub->devtype_data) > > + fsleep(hub->devtype_data->reset_duration); > > + } > > + > > > > err = regulator_disable(hub->vdd); > > if (err) { > > > > dev_err(hub->dev, "failed to disable regulator: %d\n", err); > > > > @@ -219,6 +233,7 @@ static void onboard_hub_attach_usb_driver(struct > > work_struct *work)> > > static int onboard_hub_probe(struct platform_device *pdev) > > { > > > > + const struct of_device_id *of_id; > > > > struct device *dev = &pdev->dev; > > struct onboard_hub *hub; > > int err; > > > > @@ -227,10 +242,24 @@ static int onboard_hub_probe(struct platform_device > > *pdev)> > > if (!hub) > > > > return -ENOMEM; > > > > + of_id = of_match_device(onboard_hub_match, &pdev->dev); > > + if (of_id) > > + hub->devtype_data = of_id->data; > > + else > > + return -ENODEV; > > Please change to: > > if (!of_id) > return -ENODEV; > > hub->pdata = of_id->data; > > With the currently supported Realtek hubs will fail to probe. Please add a > pdata for the RTS541x hubs. The have no reset signal, so the values can be > 0. RTS541x hubs should still probe, but their of_id->data is NULL. Nevertheless providing platform data for all hubs seems reasonable. > > + > > > > hub->vdd = devm_regulator_get(dev, "vdd"); > > if (IS_ERR(hub->vdd)) > > > > return PTR_ERR(hub->vdd); > > > > + 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"); + > > + if (hub->devtype_data && hub->reset_gpio) > > drop check 'hub->devtype_data' check With platform data always provided this will be removed. > > + fsleep(hub->devtype_data->reset_duration); > > + > > > > hub->dev = dev; > > mutex_init(&hub->lock); > > INIT_LIST_HEAD(&hub->udev_list); > > > > diff --git a/drivers/usb/misc/onboard_usb_hub.h > > b/drivers/usb/misc/onboard_usb_hub.h index d3a5b6938582..7e743f4c8aaa > > 100644 > > --- a/drivers/usb/misc/onboard_usb_hub.h > > +++ b/drivers/usb/misc/onboard_usb_hub.h > > @@ -6,6 +6,11 @@ > > > > #ifndef _USB_MISC_ONBOARD_USB_HUB_H > > #define _USB_MISC_ONBOARD_USB_HUB_H > > > > +struct onboard_hub_devtype_data { > > + unsigned long power_stable_time; /* power stabilization time in us */ > > + unsigned long reset_duration; /* reset pulse width in us */ > > Let's encode the unit in the field name: > > unsigned long power_stable_us; > unsigned long reset_us; Sure thing. Will be changed. > > +}; > > Is it necessary to define this struct in onboard_usb_hub.h? It seems it > could as well be defined in onboard_usb_hub.c. As you pointed out in Patch 3 not possible, unfortunately. Thanks and best regards, Alexander
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@ew.tq-group.com> > > > --- > > > 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.
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; + 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) + fsleep(hub->devtype_data->power_stable_time); + if (hub->reset_gpio) + gpiod_set_value_cansleep(hub->reset_gpio, 0); + hub->is_powered_on = true; return 0; @@ -65,6 +73,12 @@ static int onboard_hub_power_off(struct onboard_hub *hub) { int err; + if (hub->reset_gpio) { + gpiod_set_value_cansleep(hub->reset_gpio, 1); + if (hub->devtype_data) + fsleep(hub->devtype_data->reset_duration); + } + err = regulator_disable(hub->vdd); if (err) { dev_err(hub->dev, "failed to disable regulator: %d\n", err); @@ -219,6 +233,7 @@ static void onboard_hub_attach_usb_driver(struct work_struct *work) static int onboard_hub_probe(struct platform_device *pdev) { + const struct of_device_id *of_id; struct device *dev = &pdev->dev; struct onboard_hub *hub; int err; @@ -227,10 +242,24 @@ static int onboard_hub_probe(struct platform_device *pdev) if (!hub) return -ENOMEM; + of_id = of_match_device(onboard_hub_match, &pdev->dev); + if (of_id) + hub->devtype_data = of_id->data; + else + return -ENODEV; + hub->vdd = devm_regulator_get(dev, "vdd"); if (IS_ERR(hub->vdd)) return PTR_ERR(hub->vdd); + 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"); + + if (hub->devtype_data && hub->reset_gpio) + fsleep(hub->devtype_data->reset_duration); + hub->dev = dev; mutex_init(&hub->lock); INIT_LIST_HEAD(&hub->udev_list); diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h index d3a5b6938582..7e743f4c8aaa 100644 --- a/drivers/usb/misc/onboard_usb_hub.h +++ b/drivers/usb/misc/onboard_usb_hub.h @@ -6,6 +6,11 @@ #ifndef _USB_MISC_ONBOARD_USB_HUB_H #define _USB_MISC_ONBOARD_USB_HUB_H +struct onboard_hub_devtype_data { + unsigned long power_stable_time; /* power stabilization time in us */ + unsigned long reset_duration; /* reset pulse width in us */ +}; + static const struct of_device_id onboard_hub_match[] = { { .compatible = "usbbda,411" }, { .compatible = "usbbda,5411" },
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> --- 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(+)