Re: [PATCH v4 2/4] clk: rockchip: Add internal support for 24-bit fractional dividers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] clk: rockchip: broken rate calculation in clk_fd_v2_determine_rate()
- [Medium] clk: rockchip: broken zero-based divider math in clk_fd_v2_get_divider()
- [Medium] clk: rockchip: wrong low-bit mask and shift in clk_fd_v2_set_rate()
--

commit dfce7c81ca5864499aad504a0c7023a553587e91
Author: Elaine Zhang <zhangqing@rock-chips.com>

clk: rockchip: Add internal support for 24-bit fractional dividers

This commit adds an internal Rockchip fractional-divider helper and
branch_fraction_divider_v2 clock type for 24-bit fractional dividers that
split numerator and denominator across low (16-bit) and high (8-bit)
registers.

> diff --git a/drivers/clk/rockchip/clk-fractional-divider-v2.c b/drivers/clk/rockchip/clk-fractional-divider-v2.c
> new file mode 100644
> index 0000000000000..102e03f9d6e60
> --- /dev/null
> +++ b/drivers/clk/rockchip/clk-fractional-divider-v2.c

[ ... ]

> +static void clk_fractional_divider_general_approximation_v2(struct clk_hw *hw,
> +							    unsigned long rate,
> +							    unsigned long *parent_rate,
> +							    unsigned long *m, unsigned long *n)
> +{
> +	struct clk_fractional_divider_v2 *fd = to_clk_fd_v2(hw);
> +	unsigned long scale;
> +
> +	/*
> +	 * Get rate closer to *parent_rate to guarantee there is no overflow
> +	 * for m and n. In the result it will be the nearest rate left shifted
> +	 * by (scale - fd->nwidth) bits.
> +	 *
> +	 * For the detailed explanation see the top comment in this file.
> +	 */
> +	scale = fls_long(*parent_rate / rate - 1);
> +	if (scale > fd->nwidth)
> +		rate <<= scale - fd->nwidth;
> +
> +	rational_best_approximation(rate, *parent_rate,
> +				    GENMASK(fd->mwidth - 1, 0),
> +				    GENMASK(fd->nwidth - 1, 0),
> +				    m, n);
> +}
> +
> +static int clk_fd_v2_get_divider(struct clk_hw *hw, unsigned long rate,
> +				 unsigned long parent_rate,
> +				 unsigned long *m, unsigned long *n)
> +{
> +	struct clk_fractional_divider_v2 *fd = to_clk_fd_v2(hw);
> +	u32 factor;
> +
> +	rational_best_approximation(rate, parent_rate,
> +				    GENMASK(fd->mwidth - 1, 0),
> +				    GENMASK(fd->nwidth - 1, 0),
> +				    m, n);
> +
> +	if (fd->flags & CLK_FRAC_DIVIDER_ZERO_BASED) {
> +		(*m)--;
> +		(*n)--;
> +	}
> +	/*
> +	 * When compensation the fractional divider,
> +	 * the [1:0] bits of the numerator register are omitted,
> +	 * which will lead to a large deviation in the result.
> +	 * Therefore, it is required that the numerator must
> +	 * be at least 4.
> +	 *
> +	 * Note that there are some exceptions here:
> +	 * If there is an even frac div, we need to keep the original
> +	 * numerator(<4) and denominator. Otherwise, it may cause the
> +	 * issue that the duty ratio is not 50%.
> +	 */
> +	if (*m < 4 && *m != 0) {
> +		if (*n % 2 == 0)
> +			factor = 1;
> +		else
> +			factor = DIV_ROUND_UP(4, *m);
> +
> +		if (*m > GENMASK(fd->mwidth - 1, 0) / factor ||
> +		    *n > GENMASK(fd->nwidth - 1, 0) / factor)
> +			return -EINVAL;
> +
> +		*n *= factor;
> +		*m *= factor;
> +	}

[Severity: Medium]
In clk_fd_v2_get_divider(), when CLK_FRAC_DIVIDER_ZERO_BASED is set in
fd->flags, (*m)-- and (*n)-- convert the 1-based values from
rational_best_approximation() into 0-based register values before the
*m < 4 compensation block runs.

Does this cause several regressions when CLK_FRAC_DIVIDER_ZERO_BASED is used?

1. If rational_best_approximation() returns *m == 0 for a very small rate,
   (*m)-- underflows the unsigned long *m to ULONG_MAX, which bypasses the
   (*m < 4 && *m != 0) check and writes all-ones to the numerator register
   fields.

2. If the 1-based numerator is *m == 1 (for example, 1/3), (*m)-- sets
   *m = 0, which skips the (*m < 4 && *m != 0) compensation completely.

3. Decrementing *n flips its parity before (*n % 2 == 0) is checked, so
   factor = 1 is selected when the actual denominator n is odd rather than
   even.

4. Multiplying the already-decremented values by factor produces register
   values (m - 1) * factor and (n - 1) * factor. When clk_fd_v2_recalc_rate()
   adds 1 back:

	if (fd->flags & CLK_FRAC_DIVIDER_ZERO_BASED) {
		m++;
		n++;
	}

   the effective ratio becomes ((m - 1) * factor + 1) / ((n - 1) * factor + 1)
   instead of m / n (for example, m = 3, n = 4 becomes 5/7 instead of 3/4).

5. Neither clk_fractional_divider_general_approximation_v2() nor
   clk_fd_v2_get_divider() increases the maximum bounds passed to
   rational_best_approximation() from GENMASK(width - 1, 0) to BIT(width)
   when CLK_FRAC_DIVIDER_ZERO_BASED is set.

> +
> +	return 0;
> +}
> +
> +static int clk_fd_v2_determine_rate(struct clk_hw *hw,
> +				    struct clk_rate_request *req)
> +{
> +	struct clk_fractional_divider_v2 *fd = to_clk_fd_v2(hw);
> +	unsigned long m, n;
> +	u64 ret;
> +
> +	if (!req->rate || req->rate >= req->best_parent_rate) {
> +		req->rate = req->best_parent_rate;
> +		return 0;
> +	}
> +
> +	if (fd->approximation)
> +		fd->approximation(hw, req->rate, &req->best_parent_rate, &m, &n);
> +	else
> +		clk_fractional_divider_general_approximation_v2(hw, req->rate,
> +								&req->best_parent_rate,
> +								&m, &n);
> +
> +	ret = (u64)req->best_parent_rate * m;
> +	do_div(ret, n);
> +	req->rate = ret;
> +
> +	/* Validate the rounded rate that set_rate() will receive. */
> +	return clk_fd_v2_get_divider(hw, req->rate, req->best_parent_rate, &m, &n);
> +}

[Severity: High]
Can the interaction between clk_fractional_divider_general_approximation_v2(),
clk_fd_v2_determine_rate(), and clk_fd_v2_get_divider() cause valid rate
requests to fail with -EINVAL or program a different divider than expected?

1. In clk_fd_v2_get_divider(), rational_best_approximation() is given the full
   24-bit range GENMASK(fd->mwidth - 1, 0) and GENMASK(fd->nwidth - 1, 0)
   (16,777,215). If the closest uncompensated ratio has *m < 4 and an odd
   denominator *n > GENMASK(fd->nwidth - 1, 0) / factor (any odd n in
   [4194304, 16777215] for m = 1, or [8388608, 16777215] for m = 2 or 3),
   clk_fd_v2_get_divider() returns -EINVAL instead of choosing a valid 24-bit
   approximation (such as an adjacent even denominator where factor = 1, or an
   approximation constrained so the compensated values fit).

2. clk_fd_v2_determine_rate() computes (m, n) once, floor-truncates req->rate
   to an integer via do_div(ret, n), and then calls clk_fd_v2_get_divider()
   (and later clk_fd_v2_set_rate()), which runs rational_best_approximation()
   a second time on the truncated req->rate without updating req->rate.

   Because a 24-bit divider has sub-Hz resolution at typical PLL rates,
   re-approximating the truncated integer rate can pick a different (m, n)
   pair than the first pass. For example:

   - With best_parent_rate = 100000000 and req->rate = 19, the first pass
     picks m = 1, n = 5263158 (even) and truncates req->rate to 18. The
     second pass in clk_fd_v2_get_divider() approximates 18 / 100000000 as
     m = 2, n = 11111111 (odd), which exceeds 16777215 / 2 and fails with
     -EINVAL.

   - With a fractional PLL parent such asbest_parent_rate = 786431991 and
     req->rate = 6144000, the first pass finds the intended divider m = 1,
     n = 128 and truncates req->rate to 6143999. The second pass in
     clk_fd_v2_get_divider() and clk_fd_v2_set_rate() re-approximates
     6143999 / 786431991 as m = 51630, n = 6608641 instead of 1 / 128.

3. In clk_fractional_divider_general_approximation_v2(), when
   scale = fls_long(*parent_rate / rate - 1) exceeds fd->nwidth, rate is
   shifted left by (scale - fd->nwidth) unconditionally without checking
   CLK_FRAC_DIVIDER_POWER_OF_TWO_PS in fd->flags. That causes
   clk_fd_v2_determine_rate() to return a power-of-two multiple of the
   requested rate, whereas clk_fd_v2_get_divider() does not perform that shift
   (and if rational_best_approximation() saturates to *m = 0, *n = 1 for a
   very small rate, clk_fd_v2_get_divider() leaves *m = 0 uncompensated,
   causing clk_fd_v2_recalc_rate() to return parent_rate).

> +
> +static int clk_fd_v2_set_rate(struct clk_hw *hw, unsigned long rate,
> +			      unsigned long parent_rate)
> +{
> +	struct clk_fractional_divider_v2 *fd = to_clk_fd_v2(hw);
> +	unsigned long flags = 0;
> +	unsigned long m, n, m_low, n_low, m_high, n_high;
> +	u32 val, val_high;
> +	int ret;
> +
> +	ret = clk_fd_v2_get_divider(hw, rate, parent_rate, &m, &n);
> +	if (ret)
> +		return ret;
> +
> +	if (fd->lock)
> +		spin_lock_irqsave(fd->lock, flags);
> +	else
> +		__acquire(fd->lock);
> +
> +	m_low = m & fd->nmask;
> +	n_low = n & fd->nmask;
> +	m_high = (m & GENMASK(fd->mwidth - 1, (fd->mwidth - fd->high_mwidth))) >>
> +		 (fd->mwidth - fd->high_mwidth);
> +	n_high = (n & GENMASK(fd->nwidth - 1, (fd->nwidth - fd->high_nwidth))) >>
> +		 (fd->nwidth - fd->high_nwidth);
> +
> +	val_high = clk_fd_v2_readl(fd, fd->high_reg);
> +	val_high &= ~(fd->high_mmask | fd->high_nmask);
> +	val_high |= (m_high << fd->high_mshift) | (n_high << fd->high_nshift);
> +
> +	clk_fd_v2_writel(fd, fd->high_reg, val_high);
> +
> +	val = clk_fd_v2_readl(fd, fd->reg);
> +	val &= ~(fd->mmask | fd->nmask);
> +	val |= (m_low << fd->mshift) | (n_low << fd->nshift);
> +	clk_fd_v2_writel(fd, fd->reg, val);

[Severity: Medium]
In clk_hw_register_fractional_divider_v2(), fd->mmask and fd->nmask are
initialized as shifted register masks:

	fd->mshift = mshift;
	fd->mwidth = mwidth + high_mwidth;
	fd->mmask = GENMASK(mwidth - 1, 0) << mshift;
	fd->nshift = nshift;
	fd->nwidth = nwidth + high_nwidth;
	fd->nmask = GENMASK(nwidth - 1, 0) << nshift;

In clk_fd_v2_set_rate(), both m_low and n_low are masked with fd->nmask and
then shifted by fd->mshift and fd->nshift when writing fd->reg. Shouldn't
m_low and n_low be masked with unshifted masks for their respective low
widths (or shifted first and then masked with fd->mmask and fd->nmask)?

Also, clk_fd_v2_set_rate() extracts m_high and n_high using the configured
low widths (fd->mwidth - fd->high_mwidth) and (fd->nwidth - fd->high_nwidth),
whereas clk_fd_v2_recalc_rate() hardcodes a 16-bit shift:

	m = (m_high << 16) | m_low;
	n = (n_high << 16) | n_low;

Should clk_fd_v2_recalc_rate() shift m_high and n_high by the configured low
widths as well so both functions stay consistent if a width other than 16 is
passed to COMPOSITE_FRAC_V2() or rockchip_clk_register_frac_v2()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-add-board-ok3572-c-clk-v4-0-d4152b4fedaa@qq.com?part=2