[PATCH] usb: host: ohci-generic: Make usage of clock/reset bulk() API
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[] = {
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 !
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 !
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.
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
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
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/202013db5a47ecbac4a53c360ed... https://patchwork.ozlabs.org/project/uboot/patch/464111fca83008503022e8ada53...
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.
participants (3)
-
Fabrice Gasnier -
Marek Vasut -
Xavier Drudis Ferran