Re: [PATCH] usb: host: ohci-generic: Make usage of clock/reset bulk() API

[Xavier Drudis Ferran <xdrudis@tinet.cat>]

Is the change of behaviour intended when a clock or reset is not found ?
(see below)

El Wed, Aug 30, 2023 at 10:01:49AM +0200, Fabrice Gasnier deia:
> Make usage of clock and reset bulk API in order to simplify the code
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> ---
> 
>  drivers/usb/host/ohci-generic.c | 92 +++++++++++----------------------
>  1 file changed, 29 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
> index 2d8d38ce9a40..95aa608d8c19 100644
> --- a/drivers/usb/host/ohci-generic.c
> +++ b/drivers/usb/host/ohci-generic.c
> @@ -16,75 +16,41 @@
>  
>  struct generic_ohci {
>  	ohci_t ohci;
> -	struct clk *clocks;	/* clock list */
> -	struct reset_ctl *resets; /* reset list */
> +	struct clk_bulk clocks;	/* clock list */
> +	struct reset_ctl_bulk resets; /* reset list */
>  	struct phy phy;
> -	int clock_count;	/* number of clock in clock list */
> -	int reset_count;	/* number of reset in reset list */
>  };
>  
>  static int ohci_usb_probe(struct udevice *dev)
>  {
>  	struct ohci_regs *regs = dev_read_addr_ptr(dev);
>  	struct generic_ohci *priv = dev_get_priv(dev);
> -	int i, err, ret, clock_nb, reset_nb;
> -
> -	err = 0;
> -	priv->clock_count = 0;
> -	clock_nb = dev_count_phandle_with_args(dev, "clocks", "#clock-cells",
> -					       0);
> -	if (clock_nb > 0) {
> -		priv->clocks = devm_kcalloc(dev, clock_nb, sizeof(struct clk),
> -					    GFP_KERNEL);
> -		if (!priv->clocks)
> -			return -ENOMEM;
> -
> -		for (i = 0; i < clock_nb; i++) {
> -			err = clk_get_by_index(dev, i, &priv->clocks[i]);
> -			if (err < 0)
> -				break;
> -

Please note the old code was tolerant of not finding some clocks. It
ignored any clock not found and any other listed after it in the
"clocks" property and enabled the clocks before it.

The clk_get_bulk() function instead returns an error when any clock in
"clocks" is not found and releases (disables again and frees) those
before it.

I'm not aware of any case that breaks because of this, but I once saw
a case of ehci not working and ohci working because one of the listed
clocks not being found (ehci called clk_get_bulk(),
clk_enable_blk()). In that case, a fix by ignoring the missing clock
in ehci was rejected, so maybe that criteria applies here as well and
your patch is deemed correct. I don't know. That case won't break now,
I think, either with or without your patch, because after another fix,
that clock will be found. But I don't know if there might be similar
cases.

I just wanted to point out the change in behaviour. If the change is
intended, then all is fine.

> -			err = clk_enable(&priv->clocks[i]);
> -			if (err && err != -ENOSYS) {
> -				dev_err(dev, "failed to enable clock %d\n", i);
> -				clk_free(&priv->clocks[i]);
> -				goto clk_err;
> -			}
> -			priv->clock_count++;
> -		}
> -	} else if (clock_nb != -ENOENT) {
> -		dev_err(dev, "failed to get clock phandle(%d)\n", clock_nb);
> -		return clock_nb;
> +	int err, ret;
> +
> +	ret = clk_get_bulk(dev, &priv->clocks);
> +	if (ret && ret != -ENOENT) {
> +		dev_err(dev, "Failed to get clocks (ret=%d)\n", ret);
> +		return ret;
> +	}
> +
> +	err = clk_enable_bulk(&priv->clocks);
> +	if (err) {
> +		dev_err(dev, "Failed to enable clocks (err=%d)\n", err);
> +		goto clk_err;
>  	}
>  
> -	priv->reset_count = 0;
> -	reset_nb = dev_count_phandle_with_args(dev, "resets", "#reset-cells",
> -					       0);
> -	if (reset_nb > 0) {
> -		priv->resets = devm_kcalloc(dev, reset_nb,
> -					    sizeof(struct reset_ctl),
> -					    GFP_KERNEL);
> -		if (!priv->resets)
> -			return -ENOMEM;
> -
> -		for (i = 0; i < reset_nb; i++) {
> -			err = reset_get_by_index(dev, i, &priv->resets[i]);
> -			if (err < 0)
> -				break;
> -

Similar here.

> -			err = reset_deassert(&priv->resets[i]);
> -			if (err) {
> -				dev_err(dev, "failed to deassert reset %d\n", i);
> -				reset_free(&priv->resets[i]);
> -				goto reset_err;
> -			}
> -			priv->reset_count++;
> -		}
> -	} else if (reset_nb != -ENOENT) {
> -		dev_err(dev, "failed to get reset phandle(%d)\n", reset_nb);
> +	err = reset_get_bulk(dev, &priv->resets);
> +	if (err && err != -ENOENT) {
> +		dev_err(dev, "failed to get resets (err=%d)\n", err);
>  		goto clk_err;
>  	}
>  
> +	err = reset_deassert_bulk(&priv->resets);
> +	if (err) {
> +		dev_err(dev, "failed to get deassert resets (err=%d)\n", err);
> +		goto reset_err;
> +	}
> +
>  	err = generic_setup_phy(dev, &priv->phy, 0);
>  	if (err)
>  		goto reset_err;
> @@ -101,13 +67,13 @@ phy_err:
>  		dev_err(dev, "failed to shutdown usb phy\n");
>  
>  reset_err:
> -	ret = reset_release_all(priv->resets, priv->reset_count);
> +	ret = reset_release_bulk(&priv->resets);
>  	if (ret)
> -		dev_err(dev, "failed to assert all resets\n");
> +		dev_err(dev, "failed to release resets (ret=%d)\n", ret);
>  clk_err:
> -	ret = clk_release_all(priv->clocks, priv->clock_count);
> +	ret = clk_release_bulk(&priv->clocks);
>  	if (ret)
> -		dev_err(dev, "failed to disable all clocks\n");
> +		dev_err(dev, "failed to release clocks (ret=%d)\n", ret);
>  
>  	return err;
>  }
> @@ -125,11 +91,11 @@ static int ohci_usb_remove(struct udevice *dev)
>  	if (ret)
>  		return ret;
>  
> -	ret = reset_release_all(priv->resets, priv->reset_count);
> +	ret = reset_release_bulk(&priv->resets);
>  	if (ret)
>  		return ret;
>  
> -	return clk_release_all(priv->clocks, priv->clock_count);
> +	return clk_release_bulk(&priv->clocks);
>  }
>  
>  static const struct udevice_id ohci_usb_ids[] = {
> -- 
> 2.25.1
>