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

[Bjorn Andersson <andersson@kernel.org>]

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.

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