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