[PATCH 0/5] clk: qcom: Fix RCG/branch MND divider and timeout issues

[PATCH 0/5] clk: qcom: Fix RCG/branch MND divider and timeout issues

[Xilin Wu]

This series fixes several bugs in the Qualcomm clock RCG and branch
drivers that surface when clocks are used at low frequencies or with
the MND duty cycle control path (clk_rcg2_gp_ops).

Patches 1-2 fix the GP clock MND divider computation
(clk_rcg2_calc_mnd) and the duty cycle setting path
(clk_rcg2_set_duty_cycle). The calc_mnd function has u16 overflow
issues and incorrect n_max/pre_div calculations that produce wrong
frequencies. The set_duty_cycle function has a u32 overflow that
produces wrong duty cycles when n is large.

Patches 3-4 replace the fixed polling timeouts in clk_branch_wait()
and update_config() with dynamically computed values based on the
configured clock rate. The existing fixed timeouts (200 us / 500 us)
are too short for clocks running at low rates (tens of Hz to low kHz),
causing spurious timeout warnings and clock configuration failures.

Patch 5 fixes an integer truncation bug in the duty cycle boundary
checks that allows out-of-range values to be written to the D
register, causing RCG configuration update failures.

These bugs do not affect clocks using the standard clk_rcg2_ops with
pre-defined frequency tables, but they are triggered when GP clocks
are used with clk_rcg2_gp_ops for PWM-style output at arbitrary
frequencies and duty cycles.

Signed-off-by: Xilin Wu <sophon@radxa.com>
---
Xilin Wu (5):
      clk: qcom: clk-rcg2: fix clk_rcg2_calc_mnd() producing wrong M/N/pre_div
      clk: qcom: clk-rcg2: use 64-bit arithmetic in set_duty_cycle()
      clk: qcom: clk-branch: calculate timeout based on clock frequency
      clk: qcom: clk-rcg2: calculate timeout based on clock frequency
      clk: qcom: clk-rcg2: fix set_duty_cycle() integer overflow in boundary checks

 drivers/clk/qcom/clk-branch.c | 22 ++++++++++++++++--
 drivers/clk/qcom/clk-rcg.h    |  2 ++
 drivers/clk/qcom/clk-rcg2.c   | 53 ++++++++++++++++++++++++++++++++++---------
 3 files changed, 64 insertions(+), 13 deletions(-)
---
base-commit: 2febe6e6ee6e34c7754eff3c4d81aa7b0dcb7979
change-id: 20260406-clk-qcom-gpclk-fixes-fde4e81a00d2

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

[PATCH 1/5] clk: qcom: clk-rcg2: fix clk_rcg2_calc_mnd() producing wrong M/N/pre_div

[Xilin Wu]

Fix three related bugs in clk_rcg2_calc_mnd() that cause the GP clock
MND divider to produce incorrect frequencies and duty cycles:

1) n_max and n_candidate are declared as u16 but can hold values
   exceeding 65535 during computation. When mnd_width is 16 and
   mnd_max is 65535, even m=1 gives n_max=65536 which overflows u16
   to 0, making "n_candidate < n_max" always false. Similarly
   n_candidate overflows on intermediate products (e.g., 15360 * 5 =
   76800 wraps to 11264), causing the wrong value to be accepted as n.

   Fix by changing both n_max and n_candidate from u16 to u32.

2) n_max is computed as (m + mnd_max), which only accounts for the N
   register constraint (n - m must fit in mnd_width bits). However,
   the D register shares the same mnd_width bits but must store values
   up to 2*n for duty cycle control. When n > mnd_max/2, the D
   register cannot represent high duty cycles, silently clamping the
   maximum achievable duty cycle (e.g. to 85% instead of 99%).

   Fix by computing n_max as (mnd_max + 1) / 2, ensuring 2*n always
   fits within the D register's bit width.

3) When no pre-division is needed (pre_div stays at its initial value
   of 1), "pre_div > 1 ? pre_div : 0" sets f->pre_div to 0. The
   subsequent convert_to_reg_val() computes (0 * 2 - 1) which
   underflows the u8 pre_div field to 255, programming a 128x
   pre-divider into hardware.

   Fix by assigning pre_div unconditionally. Since pre_div is
   initialized to 1 and only multiplied, it is always >= 1. A value
   of 1 correctly converts to register value 1 via
   convert_to_reg_val(), which means no pre-division in calc_rate().

Example with parent=19.2 MHz XO, requesting 25 kHz:

  Before: m=1, n=48,  pre_div=15 -> 26666 Hz  (6.7% error)
  After:  m=1, n=768, pre_div=1  -> 25000 Hz  (exact)

Fixes: 898b72fa44f5 ("clk: qcom: gcc-sdm845: Add general purpose clock ops")
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/clk/qcom/clk-rcg2.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index 6064a0e17d51..82ee7ca1703a 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -437,27 +437,35 @@ static void clk_rcg2_calc_mnd(u64 parent_rate, u64 rate, struct freq_tbl *f,
 	int i = 2;
 	unsigned int pre_div = 1;
 	unsigned long rates_gcd, scaled_parent_rate;
-	u16 m, n = 1, n_candidate = 1, n_max;
+	u32 n_max, n_candidate = 1;
+	u16 m, n = 1;
 
 	rates_gcd = gcd(parent_rate, rate);
 	m = div64_u64(rate, rates_gcd);
 	scaled_parent_rate = div64_u64(parent_rate, rates_gcd);
-	while (scaled_parent_rate > (mnd_max + m) * pre_div_max) {
+
+	/*
+	 * Limit n so that the D register can represent the full duty cycle
+	 * range. The D register stores values up to 2*(n-m) using mnd_width
+	 * bits. Since m >= 1, n <= (mnd_max + 1) / 2 guarantees
+	 * 2*(n-m) <= mnd_max - 1.
+	 */
+	n_max = (mnd_max + 1) / 2;
+
+	while (scaled_parent_rate > (unsigned long)n_max * pre_div_max) {
 		// we're exceeding divisor's range, trying lower scale.
 		if (m > 1) {
 			m--;
 			scaled_parent_rate = mult_frac(scaled_parent_rate, m, (m + 1));
 		} else {
 			// cannot lower scale, just set max divisor values.
-			f->n = mnd_max + m;
+			f->n = n_max;
 			f->pre_div = pre_div_max;
 			f->m = m;
 			return;
 		}
 	}
 
-	n_max = m + mnd_max;
-
 	while (scaled_parent_rate > 1) {
 		while (scaled_parent_rate % i == 0) {
 			n_candidate *= i;
@@ -475,7 +483,7 @@ static void clk_rcg2_calc_mnd(u64 parent_rate, u64 rate, struct freq_tbl *f,
 
 	f->m = m;
 	f->n = n;
-	f->pre_div = pre_div > 1 ? pre_div : 0;
+	f->pre_div = pre_div;
 }
 
 static int clk_rcg2_determine_gp_rate(struct clk_hw *hw,

-- 
2.53.0

[PATCH 2/5] clk: qcom: clk-rcg2: use 64-bit arithmetic in set_duty_cycle()

[Xilin Wu]

The duty cycle calculation in clk_rcg2_set_duty_cycle() computes
"n * duty->num * 2" using u32 arithmetic. When n is large and
duty->num is also large, the intermediate result overflows u32.

For example, requesting 50% duty on a 1 kHz output derived from a
19.2 MHz parent gives n=19200, duty->num=500000, duty->den=1000000:

  19200 * 500000 * 2 = 19,200,000,000 > U32_MAX (4,294,967,295)

The truncated result produces a completely wrong duty cycle (5.26%
instead of the requested 50%).

Use DIV_ROUND_CLOSEST_ULL() with an explicit (u64) cast to prevent
the overflow.

Fixes: 7f891faf596e ("clk: qcom: clk-rcg2: Add support for duty-cycle for RCG")
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/clk/qcom/clk-rcg2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index 82ee7ca1703a..0e8f0473897e 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -783,7 +783,7 @@ static int clk_rcg2_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
 	n = (~(notn_m) + m) & mask;
 
 	/* Calculate 2d value */
-	d = DIV_ROUND_CLOSEST(n * duty->num * 2, duty->den);
+	d = DIV_ROUND_CLOSEST_ULL((u64)n * duty->num * 2, duty->den);
 
 	/*
 	 * Check bit widths of 2d. If D is too big reduce duty cycle.

-- 
2.53.0

Re: [PATCH 2/5] clk: qcom: clk-rcg2: use 64-bit arithmetic in set_duty_cycle()

[Konrad Dybcio]

On 4/6/26 5:54 PM, Xilin Wu wrote:
> The duty cycle calculation in clk_rcg2_set_duty_cycle() computes
> "n * duty->num * 2" using u32 arithmetic. When n is large and
> duty->num is also large, the intermediate result overflows u32.
> 
> For example, requesting 50% duty on a 1 kHz output derived from a
> 19.2 MHz parent gives n=19200, duty->num=500000, duty->den=1000000:
> 
>   19200 * 500000 * 2 = 19,200,000,000 > U32_MAX (4,294,967,295)
> 
> The truncated result produces a completely wrong duty cycle (5.26%
> instead of the requested 50%).
> 
> Use DIV_ROUND_CLOSEST_ULL() with an explicit (u64) cast to prevent
> the overflow.
> 
> Fixes: 7f891faf596e ("clk: qcom: clk-rcg2: Add support for duty-cycle for RCG")
> Signed-off-by: Xilin Wu <sophon@radxa.com>
> ---

I hate that we're still hitting this class of bugs in 2026

>  drivers/clk/qcom/clk-rcg2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index 82ee7ca1703a..0e8f0473897e 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -783,7 +783,7 @@ static int clk_rcg2_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
>  	n = (~(notn_m) + m) & mask;
>  
>  	/* Calculate 2d value */
> -	d = DIV_ROUND_CLOSEST(n * duty->num * 2, duty->den);
> +	d = DIV_ROUND_CLOSEST_ULL((u64)n * duty->num * 2, duty->den);

This may look scary given we feed it back into a u32 again, but given the
CCF has a check that ensures 0 < duty <= 100% and I think n is generally
< 16b wide, this should indeed never overflow

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

[PATCH 3/5] clk: qcom: clk-branch: calculate timeout based on clock frequency

[Xilin Wu]

Clock branches with extremely low rates (tens of Hz to low kHz) take
much longer to toggle than the fixed 200 us timeout allows. A 1 kHz
clock needs at least 3 ms (3 cycles) to toggle.

Instead of increasing the timeout to a huge fixed value for all clocks,
dynamically compute the required timeout based on the current clock
rate, accounting for 3 cycles at the current clock rate.

Based on a downstream patch by Mike Tipton:
https://git.codelinaro.org/clo/la/kernel/qcom/-/commit/aa899c2d1fa31e247f04810f125ac9c60927c901

Fixes: 6e0ad1b6c1c9 ("clk: qcom: Add support for branches/gate clocks")
Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/clk/qcom/clk-branch.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/clk-branch.c b/drivers/clk/qcom/clk-branch.c
index 444e7d8648d4..2641dcf93277 100644
--- a/drivers/clk/qcom/clk-branch.c
+++ b/drivers/clk/qcom/clk-branch.c
@@ -59,9 +59,27 @@ static bool clk_branch2_check_halt(const struct clk_branch *br, bool enabling)
 	return (val & CBCR_CLK_OFF) == (invert ? 0 : CBCR_CLK_OFF);
 }
 
+static int get_branch_timeout(const struct clk_branch *br)
+{
+	unsigned long rate;
+	int timeout;
+
+	/*
+	 * The time it takes a clock branch to toggle is roughly 3 clock cycles.
+	 */
+	rate = clk_hw_get_rate(&br->clkr.hw);
+	if (!rate)
+		return 200;
+
+	timeout = 3 * (USEC_PER_SEC / rate);
+
+	return max(timeout, 200);
+}
+
 static int clk_branch_wait(const struct clk_branch *br, bool enabling,
 		bool (check_halt)(const struct clk_branch *, bool))
 {
+	int timeout, count;
 	bool voted = br->halt_check & BRANCH_VOTED;
 	const char *name = clk_hw_get_name(&br->clkr.hw);
 
@@ -77,9 +95,9 @@ static int clk_branch_wait(const struct clk_branch *br, bool enabling,
 	} else if (br->halt_check == BRANCH_HALT_ENABLE ||
 		   br->halt_check == BRANCH_HALT ||
 		   (enabling && voted)) {
-		int count = 200;
+		timeout = get_branch_timeout(br);
 
-		while (count-- > 0) {
+		for (count = timeout; count > 0; count--) {
 			if (check_halt(br, enabling))
 				return 0;
 			udelay(1);

-- 
2.53.0

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

[Xilin Wu]

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>
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/clk/qcom/clk-rcg.h  |  2 ++
 drivers/clk/qcom/clk-rcg2.c | 27 +++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
index 4fbdf4880d03..12c1ce0f774c 100644
--- a/drivers/clk/qcom/clk-rcg.h
+++ b/drivers/clk/qcom/clk-rcg.h
@@ -159,6 +159,7 @@ extern const struct clk_ops clk_dyn_rcg_ops;
  * @clkr: regmap clock handle
  * @cfg_off: defines the cfg register offset from the CMD_RCGR + CFG_REG
  * @parked_cfg: cached value of the CFG register for parked RCGs
+ * @configured_freq: last configured frequency, used for timeout calculation
  * @hw_clk_ctrl: whether to enable hardware clock control
  */
 struct clk_rcg2 {
@@ -174,6 +175,7 @@ struct clk_rcg2 {
 	struct clk_regmap	clkr;
 	u8			cfg_off;
 	u32			parked_cfg;
+	unsigned long		configured_freq;
 	bool			hw_clk_ctrl;
 };
 
diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index 0e8f0473897e..732f34629ea2 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -111,9 +111,27 @@ static u8 clk_rcg2_get_parent(struct clk_hw *hw)
 	return __clk_rcg2_get_parent(hw, cfg);
 }
 
+static int get_update_timeout(const struct clk_rcg2 *rcg)
+{
+	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);
+
+	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);
+
 	/* Wait for update to take effect */
-	for (count = 500; count > 0; count--) {
+	for (count = timeout; count > 0; count--) {
 		ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CMD_REG, &cmd);
 		if (ret)
 			return ret;
@@ -602,6 +622,8 @@ static int clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f)
 	if (ret)
 		return ret;
 
+	rcg->configured_freq = f->freq;
+
 	return update_config(rcg);
 }
 
@@ -689,6 +711,7 @@ static int clk_rcg2_set_gp_rate(struct clk_hw *hw, unsigned long rate,
 
 	clk_rcg2_calc_mnd(parent_rate, rate, f, mnd_max, hid_max / 2);
 	convert_to_reg_val(f);
+	rcg->configured_freq = rate;
 	ret = clk_rcg2_configure_gp(rcg, f);
 
 	return ret;

-- 
2.53.0

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

[Konrad Dybcio]

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:"

[...]
> +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

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

[Bjorn Andersson]

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

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

[PATCH 5/5] clk: qcom: clk-rcg2: fix set_duty_cycle() integer overflow in boundary checks

[Xilin Wu]

The duty cycle boundary checks in clk_rcg2_set_duty_cycle() use
integer division to compare the 2d value against hardware limits:

    if ((d / 2) > (n - m))
        d = (n - m) * 2;
    else if ((d / 2) < (m / 2))
        d = m;

When d is odd, d/2 truncates, allowing values one beyond the hardware
maximum to pass. For example with n=7680, m=1, requesting 99.995%
duty:

    d = 15359  (raw 2d value)
    d / 2 = 7679  (truncated)
    n - m = 7679
    7679 > 7679 → false, check passes

But d=15359 exceeds the hardware limit of 2*(n-m)=15358. Writing this
invalid value causes the RCG to fail its configuration update, the
CMD_UPDATE bit never clears, and the clock output stops entirely.

The initial D value in __clk_rcg2_configure_mnd() correctly uses
direct comparison without division:

    d_val = clamp_t(u32, d_val, f->m, 2 * (f->n - f->m));

Align set_duty_cycle() with the same bounds by comparing directly:

    if (d > (n - m) * 2)
    else if (d < m)

Fixes: 7f891faf596e ("clk: qcom: clk-rcg2: Add support for duty-cycle for RCG")
Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 drivers/clk/qcom/clk-rcg2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index 732f34629ea2..153d2058c2a9 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -814,9 +814,9 @@ static int clk_rcg2_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
 	 */
 	d = clamp_val(d, 1, mask);
 
-	if ((d / 2) > (n - m))
+	if (d > (n - m) * 2)
 		d = (n - m) * 2;
-	else if ((d / 2) < (m / 2))
+	else if (d < m)
 		d = m;
 
 	not2d = ~d & mask;

-- 
2.53.0

Re: [PATCH 5/5] clk: qcom: clk-rcg2: fix set_duty_cycle() integer overflow in boundary checks

[Konrad Dybcio]

On 4/6/26 5:54 PM, Xilin Wu wrote:
> The duty cycle boundary checks in clk_rcg2_set_duty_cycle() use
> integer division to compare the 2d value against hardware limits:
> 
>     if ((d / 2) > (n - m))
>         d = (n - m) * 2;
>     else if ((d / 2) < (m / 2))
>         d = m;
> 
> When d is odd, d/2 truncates, allowing values one beyond the hardware
> maximum to pass. For example with n=7680, m=1, requesting 99.995%
> duty:
> 
>     d = 15359  (raw 2d value)
>     d / 2 = 7679  (truncated)
>     n - m = 7679
>     7679 > 7679 → false, check passes
> 
> But d=15359 exceeds the hardware limit of 2*(n-m)=15358. Writing this
> invalid value causes the RCG to fail its configuration update, the
> CMD_UPDATE bit never clears, and the clock output stops entirely.
> 
> The initial D value in __clk_rcg2_configure_mnd() correctly uses
> direct comparison without division:
> 
>     d_val = clamp_t(u32, d_val, f->m, 2 * (f->n - f->m));
> 
> Align set_duty_cycle() with the same bounds by comparing directly:
> 
>     if (d > (n - m) * 2)
>     else if (d < m)
> 
> Fixes: 7f891faf596e ("clk: qcom: clk-rcg2: Add support for duty-cycle for RCG")
> Signed-off-by: Xilin Wu <sophon@radxa.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad