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

[Xilin Wu]

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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxx>