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