Re: [PATCH 4/5] clk: qcom: clk-rcg2: calculate timeout based on clock frequency

[Xilin Wu <sophon@radxa.com>]

On 4/9/2026 9:55 AM, Bjorn Andersson wrote:
> On Tue, Apr 07, 2026 at 12:13:09PM +0200, Konrad Dybcio wrote:
>> On 4/6/26 5:54 PM, Xilin Wu wrote:
>>> RCGs with extremely low rates (tens of Hz to low kHz) take much longer
>>> to update than the fixed 500 us timeout allows. A 1 kHz clock needs at
>>> least 3 ms (3 cycles) for the configuration handshake.
>>>
>>> Instead of increasing the timeout to a huge fixed value for all clocks,
>>> dynamically compute the required timeout based on both the old and new
>>> clock rates, accounting for 3 cycles at each rate.
>>>
>>> Based on a downstream patch by Mike Tipton:
>>> https://git.codelinaro.org/clo/la/kernel/qcom/-/commit/aa899c2d1fa31e247f04810f125ac9c60927c901
>>>
>>> Fixes: bcd61c0f535a ("clk: qcom: Add support for root clock generators (RCGs)")
>>> Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
>>
>> Having Mike's s-o-b here is odd, given you've decided to go forward
>> without his "From:"
>>
> 
> s/odd/wrong/ Please correct the author of the commit.
> 
> Note thought that it's good etiquette to document the changes you make
> to Mike's original patch, by adding a line "[Xilin: changed x, y, z]"
> between Mike's s-o-b and yours...until you end up having more changes
> than the original author, then you're the author of the patch.
> 

Thanks for pointing this out. I'll correct the author of the commit in v2.

> Regards,
> Bjorn
> 
>> [...]
>>> +static int get_update_timeout(const struct clk_rcg2 *rcg)
>>
>> Let's tack on a '_us'
>>
>>> +{
>>> +	int timeout = 0;
>>> +	unsigned long current_freq;
>>> +
>>> +	/*
>>> +	 * The time it takes an RCG to update is roughly 3 clock cycles of the
>>> +	 * old and new clock rates.
>>> +	 */
>>> +	current_freq = clk_hw_get_rate(&rcg->clkr.hw);
>>> +	if (current_freq)
>>> +		timeout += 3 * (USEC_PER_SEC / current_freq);
>>> +	if (rcg->configured_freq)
>>> +		timeout += 3 * (USEC_PER_SEC / rcg->configured_freq);
>>
>> I suppose both are nonzero if we end up in this path but a check for zerodiv
>> is always welcome
>>
>>> +
>>> +	return max(timeout, 500);
>>> +}
>>> +
>>>   static int update_config(struct clk_rcg2 *rcg)
>>>   {
>>> -	int count, ret;
>>> +	int timeout, count, ret;
>>>   	u32 cmd;
>>>   	struct clk_hw *hw = &rcg->clkr.hw;
>>>   	const char *name = clk_hw_get_name(hw);
>>> @@ -123,8 +141,10 @@ static int update_config(struct clk_rcg2 *rcg)
>>>   	if (ret)
>>>   		return ret;
>>>   
>>> +	timeout = get_update_timeout(rcg);
>>
>> You can just assign count = get_update_timeout() below since you're not
>> reusing this value
>>
>> Konrad
> 


-- 
Best regards,
Xilin Wu <sophon@radxa.com>