Re: [PATCH v4] ufs: core: fix lockdep warning of clk_scaling_lock

[Peter Wang <peter.wang@mediatek.com>]

On 7/28/22 2:12 AM, Bart Van Assche wrote:
> On 7/26/22 20:21, peter.wang@mediatek.com wrote:
>> -    /* Enable Write Booster if we have scaled up else disable it */
>> -    downgrade_write(&hba->clk_scaling_lock);
>> -    is_writelock = false;
>> -    ufshcd_wb_toggle(hba, scale_up);
>> +    /* Disable clk_scaling until ufshcd_wb_toggle finish */
>> +    hba->clk_scaling.is_allowed = false;
>> +    wb_toggle = true;
>>     out_unprepare:
>> -    ufshcd_clock_scaling_unprepare(hba, is_writelock);
>> +    ufshcd_clock_scaling_unprepare(hba);
>> +
>> +    /* Enable Write Booster if we have scaled up else disable it */
>> +    if (wb_toggle) {
>> +        ufshcd_wb_toggle(hba, scale_up);
>> +        ufshcd_clk_scaling_allow(hba, true);
>> +    }
>
> I'm concerned that briefly disabling clock scaling may cause the clock 
> to remain at a high frequency even if it shouldn't. Has the following 
> approach been considered? Instead of moving the 
> ufshcd_clk_scaling_allow() call, convert dev_cmd.lock into a 
> semaphore, lock it near the start of ufshcd_devfreq_scale() and unlock 
> it near the end of the same function.
>
> Thanks,
>
> Bart.


Hi Bart,

Clock scaling up/down have a polling_ms, so it shouldn't have this 
condition that scale up block scale down.
Convert dev_cmd.lock into a semaphore is more risky, and dev_cmd.lock 
should hold when send dev command only.
I think it is not suitable to hold this dev_cmd.lock in 
ufshcd_devfreq_scale.

Maybe we can have another choice, let vendor decide ufshcd_wb_toggle 
with clock scaling or not?

Thanks.
Peter