| Message ID | 20220727141117.909361-1-alexander.stein@ew.tq-group.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/2] usb: misc: onboard_usb_hub: Add reset-gpio support | expand |
On Wed, Jul 27, 2022 at 04:11:17PM +0200, Alexander Stein wrote: > This is a 4-port 3.0 USB hub. > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> Acked-by: Matthias Kaehlcke <mka@chromium.org>
Hello Matthias, Am Mittwoch, 27. Juli 2022, 19:14:57 CEST schrieb Matthias Kaehlcke: > Hi Alexander, > > (copying my comments from v3 to keep the discussion on the latest version) > > On Wed, Jul 27, 2022 at 04:11:16PM +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> > > --- > > * Patch 1 dropped as it already applied > > > > Changes in v4: > > * Rebased to [1] commit e0c6b1f3d757 ("USB: usbsevseg: convert sysfs > > snprintf to sysfs_emit") * Added platform data for usb424 > > > > Reset pulse length taken from [2], Table 3-2 Symbol RESET_N > > Completely untested > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git Branch > > usb-testing [2] > > http://ww1.microchip.com/downloads/en/devicedoc/00001692c.pdf > > > > drivers/usb/misc/onboard_usb_hub.c | 28 ++++++++++++++++++++++++++++ > > drivers/usb/misc/onboard_usb_hub.h | 22 +++++++++++++++++----- > > 2 files changed, 45 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/usb/misc/onboard_usb_hub.c > > b/drivers/usb/misc/onboard_usb_hub.c index de3627af3c84..0c81417dd9a7 > > 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_pdata *pdata; > > + struct gpio_desc *reset_gpio; > > > > bool always_powered_in_suspend; > > bool is_powered_on; > > bool going_away; > > > > @@ -56,6 +59,9 @@ static int onboard_hub_power_on(struct onboard_hub *hub) > > > > return err; > > > > } > > > > + fsleep(hub->pdata->reset_us); > > + gpiod_set_value_cansleep(hub->reset_gpio, 0); > > + > > > > hub->is_powered_on = true; > > > > return 0; > > > > @@ -65,6 +71,11 @@ static int onboard_hub_power_off(struct onboard_hub > > *hub)> > > { > > > > int err; > > > > + if (hub->reset_gpio) { > > + gpiod_set_value_cansleep(hub->reset_gpio, 1); > > + fsleep(hub->pdata->reset_us); > > Is this delay here actually needed? There is a delay in > onboard_hub_power_on(), before de-asserting the reset, isn't that enough? If you see both delays together you are right, but I tend to think in that way it is to ensure whenever we apply a reset it is long enough. As said before the powering on delay is to ensure the pulse length delay even if there is no reset GPIO but it is controlled by hardware. > > + } > > + > > > > err = regulator_disable(hub->vdd); > > if (err) { > > > > dev_err(hub->dev, "failed to disable regulator: %d\n", err); > > > > @@ -219,6 +230,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 +239,26 @@ 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) > > + return -ENODEV; > > + > > + hub->pdata = of_id->data; > > + if (!hub->pdata) > > + return -EINVAL; > > + > > > > 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->reset_gpio) > > + fsleep(hub->pdata->reset_us); > > Same question here: onboard_hub_power_on() is called a few lines below and > has a delay before de-asserting the reset. Is the delay here really needed? This actually looks like the delay is duplicated here. I agree with removing this. How shall we proceed now that the whole series (incl. the bindings patch 1/3 from v3) has landed in usb-testing? I can create a patch on top of this if this is the way to go. Best regards, Alexander
On Thu, Jul 28, 2022 at 08:20:13AM +0200, Alexander Stein wrote: > Hello Matthias, > > Am Mittwoch, 27. Juli 2022, 19:14:57 CEST schrieb Matthias Kaehlcke: > > Hi Alexander, > > > > (copying my comments from v3 to keep the discussion on the latest version) > > > > On Wed, Jul 27, 2022 at 04:11:16PM +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> > > > --- > > > * Patch 1 dropped as it already applied > > > > > > Changes in v4: > > > * Rebased to [1] commit e0c6b1f3d757 ("USB: usbsevseg: convert sysfs > > > snprintf to sysfs_emit") * Added platform data for usb424 > > > > > > Reset pulse length taken from [2], Table 3-2 Symbol RESET_N > > > Completely untested > > > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git Branch > > > usb-testing [2] > > > http://ww1.microchip.com/downloads/en/devicedoc/00001692c.pdf > > > > > > drivers/usb/misc/onboard_usb_hub.c | 28 ++++++++++++++++++++++++++++ > > > drivers/usb/misc/onboard_usb_hub.h | 22 +++++++++++++++++----- > > > 2 files changed, 45 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/usb/misc/onboard_usb_hub.c > > > b/drivers/usb/misc/onboard_usb_hub.c index de3627af3c84..0c81417dd9a7 > > > 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_pdata *pdata; > > > + struct gpio_desc *reset_gpio; > > > > > > bool always_powered_in_suspend; > > > bool is_powered_on; > > > bool going_away; > > > > > > @@ -56,6 +59,9 @@ static int onboard_hub_power_on(struct onboard_hub *hub) > > > > > > return err; > > > > > > } > > > > > > + fsleep(hub->pdata->reset_us); > > > + gpiod_set_value_cansleep(hub->reset_gpio, 0); > > > + > > > > > > hub->is_powered_on = true; > > > > > > return 0; > > > > > > @@ -65,6 +71,11 @@ static int onboard_hub_power_off(struct onboard_hub > > > *hub)> > > > { > > > > > > int err; > > > > > > + if (hub->reset_gpio) { > > > + gpiod_set_value_cansleep(hub->reset_gpio, 1); > > > + fsleep(hub->pdata->reset_us); > > > > Is this delay here actually needed? There is a delay in > > onboard_hub_power_on(), before de-asserting the reset, isn't that enough? > > If you see both delays together you are right, but I tend to think in that way > it is to ensure whenever we apply a reset it is long enough. > As said before the powering on delay is to ensure the pulse length delay even > if there is no reset GPIO but it is controlled by hardware. > > > > + } > > > + > > > > > > err = regulator_disable(hub->vdd); > > > if (err) { > > > > > > dev_err(hub->dev, "failed to disable regulator: %d\n", > err); > > > > > > @@ -219,6 +230,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 +239,26 @@ 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) > > > + return -ENODEV; > > > + > > > + hub->pdata = of_id->data; > > > + if (!hub->pdata) > > > + return -EINVAL; > > > + > > > > > > 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->reset_gpio) > > > + fsleep(hub->pdata->reset_us); > > > > Same question here: onboard_hub_power_on() is called a few lines below and > > has a delay before de-asserting the reset. Is the delay here really needed? > > This actually looks like the delay is duplicated here. I agree with removing > this. > How shall we proceed now that the whole series (incl. the bindings patch 1/3 > from v3) has landed in usb-testing? I can create a patch on top of this if > this is the way to go. Please do.
On Thu, Jul 28, 2022 at 08:20:13AM +0200, Alexander Stein wrote: > Hello Matthias, > > Am Mittwoch, 27. Juli 2022, 19:14:57 CEST schrieb Matthias Kaehlcke: > > Hi Alexander, > > > > (copying my comments from v3 to keep the discussion on the latest version) > > > > On Wed, Jul 27, 2022 at 04:11:16PM +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> > > > --- > > > * Patch 1 dropped as it already applied > > > > > > Changes in v4: > > > * Rebased to [1] commit e0c6b1f3d757 ("USB: usbsevseg: convert sysfs > > > snprintf to sysfs_emit") * Added platform data for usb424 > > > > > > Reset pulse length taken from [2], Table 3-2 Symbol RESET_N > > > Completely untested > > > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git Branch > > > usb-testing [2] > > > http://ww1.microchip.com/downloads/en/devicedoc/00001692c.pdf > > > > > > drivers/usb/misc/onboard_usb_hub.c | 28 ++++++++++++++++++++++++++++ > > > drivers/usb/misc/onboard_usb_hub.h | 22 +++++++++++++++++----- > > > 2 files changed, 45 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/usb/misc/onboard_usb_hub.c > > > b/drivers/usb/misc/onboard_usb_hub.c index de3627af3c84..0c81417dd9a7 > > > 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_pdata *pdata; > > > + struct gpio_desc *reset_gpio; > > > > > > bool always_powered_in_suspend; > > > bool is_powered_on; > > > bool going_away; > > > > > > @@ -56,6 +59,9 @@ static int onboard_hub_power_on(struct onboard_hub *hub) > > > > > > return err; > > > > > > } > > > > > > + fsleep(hub->pdata->reset_us); > > > + gpiod_set_value_cansleep(hub->reset_gpio, 0); > > > + > > > > > > hub->is_powered_on = true; > > > > > > return 0; > > > > > > @@ -65,6 +71,11 @@ static int onboard_hub_power_off(struct onboard_hub > > > *hub)> > > > { > > > > > > int err; > > > > > > + if (hub->reset_gpio) { > > > + gpiod_set_value_cansleep(hub->reset_gpio, 1); > > > + fsleep(hub->pdata->reset_us); > > > > Is this delay here actually needed? There is a delay in > > onboard_hub_power_on(), before de-asserting the reset, isn't that enough? > > If you see both delays together you are right, but I tend to think in that way > it is to ensure whenever we apply a reset it is long enough. > As said before the powering on delay is to ensure the pulse length delay even > if there is no reset GPIO but it is controlled by hardware. In _power_off() the delay is currently only applied when the reset line is *not* controlled by the hardware. In that case the delay in _power_on() already ensures that the reset is applied long enough. Am I missing something? As per my reply on v2 [1] the datasheet does not suggest that there are two different delays: > In both cases the datasheet talks about the reset duration of 3 ms in > relation with the power supplies: > > 7.6 Timing Requirements, Power-Up > > td2: VDD and VDD33 stable before de-assertion of GRSTz > > > 8.3.7 Power-Up and Reset > > A minimum reset duration of 3 ms is required. This is defined as the time when > the power supplies are in the recommended operating range to the de-assertion > of GRSTz. If there is evidence that a reset delay and a separate power stable delay are needed I'm very open to add a second delay, but as of now my interpretation of the datasheet is that the reset should be applied for at least 3ms after the regulators are turned on (and power is stable, which should be ensured by the regulator config). [1] https://lore.kernel.org/all/Ytm5RrXYaKtwEg23@google.com/ > > > + } > > > + > > > > > > err = regulator_disable(hub->vdd); > > > if (err) { > > > > > > dev_err(hub->dev, "failed to disable regulator: %d\n", > err); > > > > > > @@ -219,6 +230,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 +239,26 @@ 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) > > > + return -ENODEV; > > > + > > > + hub->pdata = of_id->data; > > > + if (!hub->pdata) > > > + return -EINVAL; > > > + > > > > > > 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->reset_gpio) > > > + fsleep(hub->pdata->reset_us); > > > > Same question here: onboard_hub_power_on() is called a few lines below and > > has a delay before de-asserting the reset. Is the delay here really needed? > > This actually looks like the delay is duplicated here. I agree with removing > this. > How shall we proceed now that the whole series (incl. the bindings patch 1/3 > from v3) has landed in usb-testing? I can create a patch on top of this if > this is the way to go. > > Best regards, > Alexander > > >
diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c index de3627af3c84..0c81417dd9a7 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_pdata *pdata; + struct gpio_desc *reset_gpio; bool always_powered_in_suspend; bool is_powered_on; bool going_away; @@ -56,6 +59,9 @@ static int onboard_hub_power_on(struct onboard_hub *hub) return err; } + fsleep(hub->pdata->reset_us); + gpiod_set_value_cansleep(hub->reset_gpio, 0); + hub->is_powered_on = true; return 0; @@ -65,6 +71,11 @@ static int onboard_hub_power_off(struct onboard_hub *hub) { int err; + if (hub->reset_gpio) { + gpiod_set_value_cansleep(hub->reset_gpio, 1); + fsleep(hub->pdata->reset_us); + } + err = regulator_disable(hub->vdd); if (err) { dev_err(hub->dev, "failed to disable regulator: %d\n", err); @@ -219,6 +230,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 +239,26 @@ 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) + return -ENODEV; + + hub->pdata = of_id->data; + if (!hub->pdata) + return -EINVAL; + 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->reset_gpio) + fsleep(hub->pdata->reset_us); + 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 3820669eb98c..562fa48fcf10 100644 --- a/drivers/usb/misc/onboard_usb_hub.h +++ b/drivers/usb/misc/onboard_usb_hub.h @@ -6,12 +6,24 @@ #ifndef _USB_MISC_ONBOARD_USB_HUB_H #define _USB_MISC_ONBOARD_USB_HUB_H +struct onboard_hub_pdata { + unsigned long reset_us; /* reset pulse width in us */ +}; + +static const struct onboard_hub_pdata microchip_usb424_data = { + .reset_us = 1, +}; + +static const struct onboard_hub_pdata realtek_rts5411_data = { + .reset_us = 0, +}; + static const struct of_device_id onboard_hub_match[] = { - { .compatible = "usb424,2514" }, - { .compatible = "usbbda,411" }, - { .compatible = "usbbda,5411" }, - { .compatible = "usbbda,414" }, - { .compatible = "usbbda,5414" }, + { .compatible = "usb424,2514", .data = µchip_usb424_data, }, + { .compatible = "usbbda,411", .data = &realtek_rts5411_data, }, + { .compatible = "usbbda,5411", .data = &realtek_rts5411_data, }, + { .compatible = "usbbda,414", .data = &realtek_rts5411_data, }, + { .compatible = "usbbda,5414", .data = &realtek_rts5411_data, }, {} };
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> --- * Patch 1 dropped as it already applied Changes in v4: * Rebased to [1] commit e0c6b1f3d757 ("USB: usbsevseg: convert sysfs snprintf to sysfs_emit") * Added platform data for usb424 Reset pulse length taken from [2], Table 3-2 Symbol RESET_N Completely untested [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git Branch usb-testing [2] http://ww1.microchip.com/downloads/en/devicedoc/00001692c.pdf drivers/usb/misc/onboard_usb_hub.c | 28 ++++++++++++++++++++++++++++ drivers/usb/misc/onboard_usb_hub.h | 22 +++++++++++++++++----- 2 files changed, 45 insertions(+), 5 deletions(-)