| Message ID | 20230818-clk-fix-v1-5-49ec18f820bf@outlook.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Sean Anderson |
| Headers | show |
| Series | clk: A few bugfixes/enhancements for CCF | expand |
On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote: > From: Yang Xiwen <forbidden405@outlook.com> > > Calling into CCF framework will cause a clock being enabled twice > instead of once (clk->enable_count becomes 2 rather than 1), thus making > it hard to disable (needs to call clk_disable() twice). > Fix that by calling clock provided ops directly. Can you describe this scenario more? From what I can tell, clk_enable doesn't increment enable_count for CCF clocks. --Sean > Signed-off-by: Yang Xiwen <forbidden405@outlook.com> > --- > drivers/clk/clk.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index a38daaac0c..00d082c46f 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -14,6 +14,7 @@ > #include <dm/uclass.h> > #include <dm/lists.h> > #include <dm/device-internal.h> > +#include <linux/clk-provider.h> > > int clk_register(struct clk *clk, const char *drv_name, > const char *name, const char *parent_name) > @@ -115,11 +116,20 @@ int ccf_clk_set_parent(struct clk *clk, struct clk *parent) > static int ccf_clk_endisable(struct clk *clk, bool enable) > { > struct clk *c; > + const struct clk_ops *ops; > int err = clk_get_by_id(clk->id, &c); > > if (err) > return err; > - return enable ? clk_enable(c) : clk_disable(c); > + else > + ops = clk_dev_ops(c->dev); > + > + if (enable && ops->enable) > + return ops->enable(c); > + else if (!enable && ops->disable) > + return ops->disable(c); > + > + return -ENOSYS; > } > > int ccf_clk_enable(struct clk *clk) >
On 11/2/2023 2:19 AM, Sean Anderson wrote: > On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote: >> From: Yang Xiwen <forbidden405@outlook.com> >> >> Calling into CCF framework will cause a clock being enabled twice >> instead of once (clk->enable_count becomes 2 rather than 1), thus making >> it hard to disable (needs to call clk_disable() twice). >> Fix that by calling clock provided ops directly. > > Can you describe this scenario more? From what I can tell, clk_enable > doesn't > increment enable_count for CCF clocks. > Well, it's hard to describe clearly. But I can only tell this patch fixed the issue when i was trying to write an Ethernet driver[1] which calls clk_disable() and expects the clock to be disabled after that. Also I found that CCF driver does not have a corresponding test file. I will try to write a test for that in next release. > --Sean > >> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >> --- >> drivers/clk/clk.c | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index a38daaac0c..00d082c46f 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -14,6 +14,7 @@ >> #include <dm/uclass.h> >> #include <dm/lists.h> >> #include <dm/device-internal.h> >> +#include <linux/clk-provider.h> >> int clk_register(struct clk *clk, const char *drv_name, >> const char *name, const char *parent_name) >> @@ -115,11 +116,20 @@ int ccf_clk_set_parent(struct clk *clk, struct >> clk *parent) >> static int ccf_clk_endisable(struct clk *clk, bool enable) >> { >> struct clk *c; >> + const struct clk_ops *ops; >> int err = clk_get_by_id(clk->id, &c); >> if (err) >> return err; >> - return enable ? clk_enable(c) : clk_disable(c); >> + else >> + ops = clk_dev_ops(c->dev); >> + >> + if (enable && ops->enable) >> + return ops->enable(c); >> + else if (!enable && ops->disable) >> + return ops->disable(c); >> + >> + return -ENOSYS; >> } >> int ccf_clk_enable(struct clk *clk) >> > [1] https://lore.kernel.org/all/20230814-wip-hisi_femac-trunk-v2-0-1e29f400585d@outlook.com/
On 11/2/2023 2:50 AM, Yang Xiwen wrote: > On 11/2/2023 2:19 AM, Sean Anderson wrote: >> On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote: >>> From: Yang Xiwen <forbidden405@outlook.com> >>> >>> Calling into CCF framework will cause a clock being enabled twice >>> instead of once (clk->enable_count becomes 2 rather than 1), thus making >>> it hard to disable (needs to call clk_disable() twice). >>> Fix that by calling clock provided ops directly. >> >> Can you describe this scenario more? From what I can tell, clk_enable >> doesn't >> increment enable_count for CCF clocks. >> > Well, it's hard to describe clearly. But I can only tell this patch > fixed the issue when i was trying to write an Ethernet driver[1] which > calls clk_disable() and expects the clock to be disabled after that. > Also I found that CCF driver does not have a corresponding test file. I > will try to write a test for that in next release. Okay, fine. I read the source again and let me try to explain the whole thing to you briefly. Let's see what happens when we are calling clk_enable(gate). The source of clk.c is listed below and labeled for clarity: ``` 1 if (CONFIG_IS_ENABLED(CLK_CCF)) { 2 /* Take id 0 as a non-valid clk, such as dummy */ 3 if (clk->id && !clk_get_by_id(clk->id, &clkp)) { 4 if (clkp->enable_count) { 5 clkp->enable_count++; 6 return 0; 7 } 8 if (clkp->dev->parent && 9 device_get_uclass_id(clkp->dev->parent) == UCLASS_CLK) { 10 ret = clk_enable(dev_get_clk_ptr(clkp->dev->parent)); 11 if (ret) { 12 printf("Enable %s failed\n", 13 clkp->dev->parent->name); 14 return ret; 15 } 16 } 17 } 18 19 if (ops->enable) { 20 ret = ops->enable(clk); 21 if (ret) { 22 printf("Enable %s failed\n", clk->dev->name); 23 return ret; 24 } 25 } 26 if (clkp) 27 clkp->enable_count++; 28 } else { 29 if (!ops->enable) 30 return -ENOSYS; 31 return ops->enable(clk); ``` The following steps are executed: 1. Actually, a "fake" clk is passed to clk_enable() and only clk->id is valid. The actual clk is "clkp"; 2. Initially, we runs till `ret = ops->enable(clk)`(line 20), i.e. ccf_clk_enable(clk); 3. Thankfully, ccf_clk_enable() calls clk_get_by_id() to get the real clk and call clk_enable(clkp) again so we won't have an endless loop here. 4. So ops->enable(clk) actually equals to clk_enable(clkp). It's obvious that there is a `clkp->enable_count++` inside the nested function call since it's still 0. Now it becomes 1; 5. The nested clk_enable(clkp) now returns to the outer clk_enable(clk); 6. Unfortunately, there is a `if (clkp) clkp->enable_count++;`(line 26) afterwards. Now it becomes 2. 7. Finally, we got a clk being enabled twice. "clkp->enable_count" is 2 now. Obviously it's not the intended behavior. We can either fix clk_enable() or ccf_clk_endisable() to resolve this. But I choose to touch ccf_clk_endisable() since it's less commonly used. >> --Sean >> >>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >>> --- >>> drivers/clk/clk.c | 12 +++++++++++- >>> 1 file changed, 11 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >>> index a38daaac0c..00d082c46f 100644 >>> --- a/drivers/clk/clk.c >>> +++ b/drivers/clk/clk.c >>> @@ -14,6 +14,7 @@ >>> #include <dm/uclass.h> >>> #include <dm/lists.h> >>> #include <dm/device-internal.h> >>> +#include <linux/clk-provider.h> >>> int clk_register(struct clk *clk, const char *drv_name, >>> const char *name, const char *parent_name) >>> @@ -115,11 +116,20 @@ int ccf_clk_set_parent(struct clk *clk, struct >>> clk *parent) >>> static int ccf_clk_endisable(struct clk *clk, bool enable) >>> { >>> struct clk *c; >>> + const struct clk_ops *ops; >>> int err = clk_get_by_id(clk->id, &c); >>> if (err) >>> return err; >>> - return enable ? clk_enable(c) : clk_disable(c); >>> + else >>> + ops = clk_dev_ops(c->dev); >>> + >>> + if (enable && ops->enable) >>> + return ops->enable(c); >>> + else if (!enable && ops->disable) >>> + return ops->disable(c); >>> + >>> + return -ENOSYS; >>> } >>> int ccf_clk_enable(struct clk *clk) >>> >> > [1] > https://lore.kernel.org/all/20230814-wip-hisi_femac-trunk-v2-0-1e29f400585d@outlook.com/ >
On 11/1/23 16:33, Yang Xiwen wrote: > On 11/2/2023 2:50 AM, Yang Xiwen wrote: >> On 11/2/2023 2:19 AM, Sean Anderson wrote: >>> On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote: >>>> From: Yang Xiwen <forbidden405@outlook.com> >>>> >>>> Calling into CCF framework will cause a clock being enabled twice >>>> instead of once (clk->enable_count becomes 2 rather than 1), thus making >>>> it hard to disable (needs to call clk_disable() twice). >>>> Fix that by calling clock provided ops directly. >>> >>> Can you describe this scenario more? From what I can tell, clk_enable >>> doesn't >>> increment enable_count for CCF clocks. >>> >> Well, it's hard to describe clearly. But I can only tell this patch >> fixed the issue when i was trying to write an Ethernet driver[1] which >> calls clk_disable() and expects the clock to be disabled after that. >> Also I found that CCF driver does not have a corresponding test file. I >> will try to write a test for that in next release. > Okay, fine. I read the source again and let me try to explain the whole > thing to you briefly. Let's see what happens when we are calling > clk_enable(gate). > > The source of clk.c is listed below and labeled for clarity: > > ``` > 1 if (CONFIG_IS_ENABLED(CLK_CCF)) { > 2 /* Take id 0 as a non-valid clk, such as dummy */ > 3 if (clk->id && !clk_get_by_id(clk->id, &clkp)) { > 4 if (clkp->enable_count) { > 5 clkp->enable_count++; > 6 return 0; > 7 } > 8 if (clkp->dev->parent && > 9 device_get_uclass_id(clkp->dev->parent) == UCLASS_CLK) { > 10 ret = clk_enable(dev_get_clk_ptr(clkp->dev->parent)); > 11 if (ret) { > 12 printf("Enable %s failed\n", > 13 clkp->dev->parent->name); > 14 return ret; > 15 } > 16 } > 17 } > 18 > 19 if (ops->enable) { > 20 ret = ops->enable(clk); > 21 if (ret) { > 22 printf("Enable %s failed\n", clk->dev->name); > 23 return ret; > 24 } > 25 } > 26 if (clkp) > 27 clkp->enable_count++; > 28 } else { > 29 if (!ops->enable) > 30 return -ENOSYS; > 31 return ops->enable(clk); > ``` > > The following steps are executed: > > 1. Actually, a "fake" clk is passed to clk_enable() and only clk->id is > valid. The actual clk is "clkp"; > 2. Initially, we runs till `ret = ops->enable(clk)`(line 20), i.e. > ccf_clk_enable(clk); > 3. Thankfully, ccf_clk_enable() calls clk_get_by_id() to get the real > clk and call clk_enable(clkp) again so we won't have an endless loop here. > 4. So ops->enable(clk) actually equals to clk_enable(clkp). It's obvious > that there is a `clkp->enable_count++` inside the nested function call > since it's still 0. Now it becomes 1; > 5. The nested clk_enable(clkp) now returns to the outer clk_enable(clk); > 6. Unfortunately, there is a `if (clkp) clkp->enable_count++;`(line 26) > afterwards. Now it becomes 2. > 7. Finally, we got a clk being enabled twice. "clkp->enable_count" is 2 now. OK, thank you for writing this up; it is clearer now. Please include this in your commit message. > Obviously it's not the intended behavior. We can either fix clk_enable() > or ccf_clk_endisable() to resolve this. But I choose to touch > ccf_clk_endisable() since it's less commonly used. Hm, what if we added something like clk_raw_enable, which just did > 19 if (ops->enable) { > 20 ret = ops->enable(clk); > 21 if (ret) { > 22 printf("Enable %s failed\n", clk->dev->name); > 23 return ret; > 24 } > 25 } and the same thing for disable. --Sean
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index a38daaac0c..00d082c46f 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -14,6 +14,7 @@ #include <dm/uclass.h> #include <dm/lists.h> #include <dm/device-internal.h> +#include <linux/clk-provider.h> int clk_register(struct clk *clk, const char *drv_name, const char *name, const char *parent_name) @@ -115,11 +116,20 @@ int ccf_clk_set_parent(struct clk *clk, struct clk *parent) static int ccf_clk_endisable(struct clk *clk, bool enable) { struct clk *c; + const struct clk_ops *ops; int err = clk_get_by_id(clk->id, &c); if (err) return err; - return enable ? clk_enable(c) : clk_disable(c); + else + ops = clk_dev_ops(c->dev); + + if (enable && ops->enable) + return ops->enable(c); + else if (!enable && ops->disable) + return ops->disable(c); + + return -ENOSYS; } int ccf_clk_enable(struct clk *clk)