Re: [PATCH RESEND 5/5] clk: ccf: call clock provided ops directly for endisable()
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
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, )) {
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
Re: [PATCH RESEND 5/5] clk: ccf: call clock provided ops directly for endisable()
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
>>>
>>> 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, )) {
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
>>> ---
>>> 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
>>> #include
>>> #include
>>> +#include
>>> 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, );
>>> 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-1e29f4005...@outlook.com/
>
--
Best regards,
Yang Xiwen
Re: [PATCH RESEND 5/5] clk: ccf: call clock provided ops directly for endisable()
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
>>
>> 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
>> ---
>> 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
>> #include
>> #include
>> +#include
>> 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, );
>> 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-1e29f4005...@outlook.com/
--
Best regards,
Yang Xiwen
Re: [PATCH RESEND 5/5] clk: ccf: call clock provided ops directly for endisable()
On 8/17/23 13:04, Yang Xiwen via B4 Relay wrote:
From: Yang Xiwen
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
---
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
#include
#include
+#include
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, );
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)
[PATCH RESEND 5/5] clk: ccf: call clock provided ops directly for endisable()
From: Yang Xiwen
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.
Signed-off-by: Yang Xiwen
---
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
#include
#include
+#include
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, );
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)
--
2.34.1
