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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] clk: rockchip: wrong mask in clk_fd_v2_set_rate() and shift in recalc_rate()
- [Medium] clk: rockchip: broken zero-based compensation in clk_fd_v2_get_divider()
--

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

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

This commit adds an internal fractional-divider helper and clock branch type
for Rockchip 24-bit fractional dividers that split the numerator and
denominator across a low register and a high register.

> 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
> @@ -0,0 +1,354 @@

[ ... ]

> +static unsigned long clk_fd_v2_recalc_rate(struct clk_hw *hw,
> +					   unsigned long parent_rate)
> +{
> +	struct clk_fractional_divider_v2 *fd = to_clk_fd_v2(hw);
> +	unsigned long flags = 0;
> +	unsigned long m_low, n_low, m_high, n_high, m, n;
> +	u32 val_low, val_high;
> +	u64 ret;
> +
> +	if (fd->lock)
> +		spin_lock_irqsave(fd->lock, flags);
> +	else
> +		__acquire(fd->lock);
> +
> +	val_low = clk_fd_v2_readl(fd, fd->reg);
> +	val_high = clk_fd_v2_readl(fd, fd->high_reg);
> +
> +	if (fd->lock)
> +		spin_unlock_irqrestore(fd->lock, flags);
> +	else
> +		__release(fd->lock);
> +
> +	m_low = (val_low & fd->mmask) >> fd->mshift;
> +	n_low = (val_low & fd->nmask) >> fd->nshift;
> +	m_high = (val_high & fd->high_mmask) >> fd->high_mshift;
> +	n_high = (val_high & fd->high_nmask) >> fd->high_nshift;
> +	m = (m_high << 16) | m_low;
> +	n = (n_high << 16) | n_low;
> +
> +	if (fd->flags & CLK_FRAC_DIVIDER_ZERO_BASED) {
> +		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;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
When CLK_FRAC_DIVIDER_ZERO_BASED is set in fd->flags, does decrementing *m
and *n before the low-numerator compensation block in clk_fd_v2_get_divider()
corrupt the divider ratio?

In clk_fd_v2_recalc_rate(), the register values are incremented by 1 before
calculating the rate:

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

However, clk_fd_v2_get_divider() decrements *m and *n right after
rational_best_approximation(), and then runs the compensation logic on the
decremented values:

1. The parity check (*n % 2 == 0) tests the decremented register value (n - 1)
   rather than the actual denominator n.
2. When the approximated numerator m is 1, (*m)-- sets *m to 0, which skips
   the (*m < 4 && *m != 0) compensation entirely.
3. For 1 < m <= 4, multiplying the already-decremented (m - 1) and (n - 1) by
   factor produces register values that hardware and clk_fd_v2_recalc_rate()
   evaluate as ((m - 1) * factor + 1) / ((n - 1) * factor + 1) instead of
   m / n.
4. If rational_best_approximation() returns *m == 0, (*m)-- underflows
   unsigned long to ULONG_MAX, bypassing the *m < 4 check and writing all-ones
   into the numerator register fields.

Should the CLK_FRAC_DIVIDER_ZERO_BASED adjustment happen after the
compensation multiplication (or should rational_best_approximation() bound
the maximum numerator and denominator to 1 << width before decrementing)?

[ ... ]

> +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_fd_v2_set_rate(), m_low and n_low are both masked with fd->nmask
before being shifted by fd->mshift and fd->nshift:

	m_low = m & fd->nmask;
	n_low = n & fd->nmask;
	...
	val |= (m_low << fd->mshift) | (n_low << fd->nshift);

Looking at clk_hw_register_fractional_divider_v2(), fd->mmask and fd->nmask
are initialized as pre-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;

Doesn't masking m with fd->nmask apply the denominator mask instead of the
unshifted low-numerator mask (fd->mmask >> fd->mshift), and wouldn't any
non-zero fd->nshift cause n_low to be masked with already-shifted bits and
then shifted by fd->nshift a second time?

Also, while clk_fd_v2_set_rate() splits the high bits at
(fd->mwidth - fd->high_mwidth) and (fd->nwidth - fd->high_nwidth),
clk_fd_v2_recalc_rate() hardcodes a shift of 16:

	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
(fd->mwidth - fd->high_mwidth) and (fd->nwidth - fd->high_nwidth) as well so
both functions agree when the low field width is not 16?

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