Re: [PATCH 2/3] usb: misc: onboard_usb_hub: Add reset-gpio support

[Matthias Kaehlcke <mka@chromium.org>]

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@ew.tq-group.com>
> ---
> 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).

> * 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.

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

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

> +	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

You could use fsleep() instead of usleep_range(). It does the _range part
automatically (with a value of 2x).

> +	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.

> +
>  	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

> +	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.

> +
>  	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.

> +	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);
> -- 
> 2.25.1
>