[PATCH RESEND 5/5] clk: ccf: call clock provided ops directly for endisable()

[Sean Anderson]

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