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

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

[Fabrice Gasnier]

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

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

[Marek Vasut]

On 8/30/23 10:01, Fabrice Gasnier wrote:
> 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;
> -
> -			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);

Plural of 'clock' is still 'clock' , please fix just the text, keep the 
variable name .

> +		return ret;
> +	}
> +
> +	err = clk_enable_bulk(&priv->clocks);
> +	if (err) {
> +		dev_err(dev, "Failed to enable clocks (err=%d)\n", err);

DTTO here

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

Drop the 'get' from the text here, I think it's copy-paste error.

> +		goto reset_err;
> +	}
> +
>   	err = generic_setup_phy(dev, &priv->phy, 0);
>   	if (err)
>   		goto reset_err;

With that fixed:

Reviewed-by: Marek Vasut <marex@denx.de>

Nice cleanup, thanks !

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

[Fabrice Gasnier]

On 8/30/23 17:18, Marek Vasut wrote:
> On 8/30/23 10:01, Fabrice Gasnier wrote:
>> 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;
>> -
>> -            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);
> 
> Plural of 'clock' is still 'clock' , please fix just the text, keep the
> variable name .

Hi Marek,

Are you sure ? Taking a closer look on the web, also in Linux or u-boot,
I can see plural of clock is clocks. Documentation also deals with
multiple clocks too.

> 
>> +        return ret;
>> +    }
>> +
>> +    err = clk_enable_bulk(&priv->clocks);
>> +    if (err) {
>> +        dev_err(dev, "Failed to enable clocks (err=%d)\n", err);
> 
> DTTO here
> 
>> +        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;
>> -
>> -            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);
> 
> Drop the 'get' from the text here, I think it's copy-paste error.

ack, will update in v2

> 
>> +        goto reset_err;
>> +    }
>> +
>>       err = generic_setup_phy(dev, &priv->phy, 0);
>>       if (err)
>>           goto reset_err;
> 
> With that fixed:
> 
> Reviewed-by: Marek Vasut <marex@denx.de>

Thanks, will add in v2.
BR,
Fabrice

> 
> Nice cleanup, thanks !

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

[Marek Vasut]

On 9/1/23 14:05, Fabrice Gasnier wrote:
> On 8/30/23 17:18, Marek Vasut wrote:
>> On 8/30/23 10:01, Fabrice Gasnier wrote:
>>> 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;
>>> -
>>> -            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);
>>
>> Plural of 'clock' is still 'clock' , please fix just the text, keep the
>> variable name .
> 
> Hi Marek,
> 
> Are you sure ? Taking a closer look on the web, also in Linux or u-boot,
> I can see plural of clock is clocks. Documentation also deals with
> multiple clocks too.

I also looked it up in a dictionary now and even asked about it, 
clearly, I am wrong. Sorry.

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

[Xavier Drudis Ferran]

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
> 

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

[Fabrice Gasnier]

On 9/2/23 11:06, Xavier Drudis Ferran wrote:
> Is the change of behaviour intended when a clock or reset is not found ?
> (see below)

Hi Xavier,

I'd say yes, although I haven't seen that... please find my answers 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()).

IMHO, the OHCI should have failed too in this example, instead of
silently ignoring the error. Hopefully it has probed.

The clk_get_bulk() code does a similar job compared to ohci current
code. It counts all clock entries. So, when trying to get them, these
should be found.

Having a clock listed, but it can't be found or taken rather looks like
a real error, that needs to be fixed.
(e.g. missing config for a clk/reset controller ? Or could it be a bug
in such a driver ? Or a miss-configured device-tree ? something else?)
Ignoring such error may be miss-leading (as you pointed out, one was
working).

I hope I don't miss your point. If this is the case, could you point
more precise example, or how it used to fail ?

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

If I understand correctly, this used to fixed elsewhere (e.g. there used
to be a real bug fixed) ?

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

IMHO, this should be fine. I hope you agree with this change and the
rationale.

Best Regards,
Fabrice

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

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

[Xavier Drudis Ferran]

El Mon, Sep 04, 2023 at 11:25:06AM +0200, Fabrice Gasnier deia:
> 
> IMHO, the OHCI should have failed too in this example, instead of
> silently ignoring the error. Hopefully it has probed.
>
> The clk_get_bulk() code does a similar job compared to ohci current
> code. It counts all clock entries. So, when trying to get them, these
> should be found.
> 
> Having a clock listed, but it can't be found or taken rather looks like
> a real error, that needs to be fixed.
> (e.g. missing config for a clk/reset controller ? Or could it be a bug
> in such a driver ? Or a miss-configured device-tree ? something else?)
> Ignoring such error may be miss-leading (as you pointed out, one was
> working).
> 
> I hope I don't miss your point. If this is the case, could you point
> more precise example, or how it used to fail ?
>

No, you don't miss my point. I'll give you pointers to the case I
meant, but I'm afraid it might mislead, because it's already solved,
and for current U-Boot it should pose no problem with or without your
patch.

The general problem might be that dts come from linux, and the drivers
come from U-Boot, so U-Boot might ignore some hardware described in
the linux dts that it doesn't need. Now this is more typical for, say,
a VPU than a clock or reset. But it once was a missing clock driver in
U-Boot that linux used for suspend/resume and happened to be at the
end of the clock list. So it worked when ohci probe ignored the missing
clock, because U-Boot doesn't need suspend, but it didn't work for ehci
that called clk_get_bulk().

There might be other cases like that example somewhere, but I'm not
saying it's likely. I guess we'll know if some board breaks.


If you really want the gory details...

https://patchwork.ozlabs.org/project/uboot/patch/20220701185959.GC1700@begut/
https://patchwork.ozlabs.org/project/uboot/patch/Y44+ayJfUlI08ptM@localhost/
https://patchwork.ozlabs.org/project/uboot/patch/Y5IWpjYLB4aXMy9o@localhost/
https://lists.denx.de/pipermail/u-boot/2022-December/501811.html
https://lists.denx.de/pipermail/u-boot/2023-February/510676.html
https://lists.denx.de/pipermail/u-boot/2023-February/510678.html
https://patchwork.ozlabs.org/project/uboot/patch/202013db5a47ecbac4a53c360ed1ca91ca663996.1685974993.git.xdrudis@tinet.cat/
https://patchwork.ozlabs.org/project/uboot/patch/464111fca83008503022e8ada5305e69ffd1afbd.1685974993.git.xdrudis@tinet.cat/


> > 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.
> 
> If I understand correctly, this used to fixed elsewhere (e.g. there used
> to be a real bug fixed) ?
>

Yes. See above. Or don't, it's not that important.
A clock driver was missing, only needed for suspend/resume.
ohci ignored it and worked (U-Boot doesn't suspend)
ehci failed probing and dind't work.
Current situation is this particular clock driver is no longer missing.

> > 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.
> 
> IMHO, this should be fine. I hope you agree with this change and the
> rationale.
>

I do.

I just wanted to point it out in case anyone knew why ohci wasn't
calling clk_get_bulk(). It might have been on purpose.

In fact Kever Yang once proposed to change ehci to be tolerant to a
missing clock like ohci was (but with an explicit warning). But Marek
Vasut proposed adding a clock driver and Kever didn't complain, so I
don't think this is his very strong opinion, he may just be happy when
things work and others are happy, I can't read minds.

https://lists.denx.de/pipermail/u-boot/2022-December/501811.html

FWIW

ohci_probe introduced:
fee331f66c9 (Alexey Brodkin   2015-12-14 17:18:50 +0300 

loop for clocks introduced in ohci_probe:
155d9f65d3b (Patrice Chotard  2017-07-18 11:57:12 +0200 

clk_get_bulk introduced:
a855be87da4 (Neil Armstrong      2018-04-03 11:44:18 +0200 156)

So ochi_probe() didn't call clk_get_bulk() most likely because it
din't exist back then.

So, unless someone else has a failing case, I agree to your change.

I'd welcome if the commit message would say that the new policy is any
missing clocks or resets cause the probe to fail. But since you
already sent v2, it doesn't matter.