Re: [PATCH v2 03/12] clk: qcom: Add a driver for PDM GP_MN fractional clock divider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] clk: qcom: clk-gp-mnd: bogus n_val and duty cycle in gp_mnd_clk_recalc_rate()
- [Low] clk: qcom: clk-gp-mnd: 64-bit division in gp_mnd_clk_set_duty_cycle() on 32-bit
--

commit 6c57ad9a80b314f181dd85480b5578fd6c5deb24
Author: Taniya Das <taniya.das@oss.qualcomm.com>

clk: qcom: Add a driver for PDM GP_MN fractional clock divider

This commit adds a standalone platform driver for the Qualcomm PDM GP_MN
fractional clock divider, implementing rate and duty-cycle operations
over regmap with runtime PM clock management.

> diff --git a/drivers/clk/qcom/clk-gp-mnd.c b/drivers/clk/qcom/clk-gp-mnd.c
> new file mode 100644
> index 0000000000000..818726c02b615
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-gp-mnd.c
> @@ -0,0 +1,333 @@

[ ... ]

> +static unsigned long gp_mnd_clk_recalc_rate(struct clk_hw *hw,
> +					    unsigned long parent_rate)
> +{
> +	struct clk_gp_mnd *gp = to_clk_gp_mnd(hw);
> +	unsigned int m_val, n_val;
> +	int ret;
> +
> +	ret = gp_mnd_clk_runtime_resume(gp);
> +	if (ret)
> +		return 0;
> +
> +	ret = regmap_read(gp->regmap, GP_MND_MDIV_REG, &m_val);
> +	if (ret)
> +		goto out_runtime_put;
> +
> +	m_val &= GP_MND_MAX_M;
> +
> +	ret = regmap_read(gp->regmap, GP_MND_NDIV_REG, &n_val);
> +	if (ret)
> +		goto out_runtime_put;
> +
> +	/* Reverse the 1's complement encoding: N = ~NDIV_REG + M */
> +	n_val = (~n_val & GP_MND_MAX_N) + m_val;
> +
> +out_runtime_put:
> +	gp_mnd_clk_runtime_put(gp);
> +
> +	if (ret)
> +		return 0;
> +
> +	if (!n_val)
> +		return 0;
> +
> +	gp->m_val = m_val;
> +	gp->n_val = n_val;
> +
> +	return DIV_ROUND_CLOSEST_ULL((u64)parent_rate * m_val, n_val);
> +}
> +
> +static int gp_mnd_clk_get_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
> +{
> +	struct clk_gp_mnd *gp = to_clk_gp_mnd(hw);
> +	unsigned int d_val;
> +	int ret;
> +
> +	if (!gp->n_val) {
> +		duty->num = 1;
> +		duty->den = 2;
> +		return 0;
> +	}
> +
> +	ret = gp_mnd_clk_runtime_resume(gp);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(gp->regmap, GP_MND_DUTY_REG, &d_val);
> +
> +	gp_mnd_clk_runtime_put(gp);
> +
> +	if (ret)
> +		return ret;
> +
> +	if (d_val >= gp->n_val)
> +		return -EINVAL;
> +
> +	/* DUTY stores the low-phase count; report the high-phase duty cycle. */
> +	duty->num = gp->n_val - d_val;
> +	duty->den = gp->n_val;
> +
> +	return 0;
> +}

[Severity: Medium]
When GP_MND_MDIV_REG and GP_MND_NDIV_REG are at their power-on reset value of
0, gp_mnd_clk_recalc_rate() reads m_val = 0 and computes:

    n_val = (~n_val & GP_MND_MAX_N) + m_val;

which evaluates to 8191. Because only !n_val is checked before caching,
gp_mnd_clk_recalc_rate() stores gp->m_val = 0 and gp->n_val = 8191 even though
the divider is unconfigured and the returned rate is 0.

When gp_mnd_clk_get_duty_cycle() is later called (for example via
clk_get_scaled_duty_cycle() or debugfs clk_summary), the !gp->n_val check for
the 50% default is bypassed because gp->n_val is 8191. Reading d_val = 0 from
GP_MND_DUTY_REG then reports duty->num = 8191 and duty->den = 8191 (100% duty
cycle) instead of 1/2.

In addition, during clk_gp_mnd_probe() -> devm_clk_hw_register() ->
__clk_core_init(), clk_core_update_duty_cycle_nolock() invokes
gp_mnd_clk_get_duty_cycle() before core->ops->recalc_rate() is called, so
gp->n_val is always 0 during initial registration even if the hardware
registers were already programmed prior to probe.

Should gp_mnd_clk_recalc_rate() also check for !m_val (or n_val <= 2 * m_val +
1) before caching gp->m_val and gp->n_val, and should gp_mnd_clk_get_duty_cycle()
handle reading the hardware state or validating M < D < (N - M) directly?

> +
> +static int gp_mnd_clk_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
> +{
> +	struct clk_gp_mnd *gp = to_clk_gp_mnd(hw);
> +	unsigned int d_val;
> +	int ret;
> +
> +	if (!gp->n_val || !gp->m_val || !duty->den || duty->num > duty->den)
> +		return -EINVAL;
> +
> +	/* D = (1 - duty) * N, giving the low-phase count */
> +	d_val = DIV_ROUND_UP((u64)(duty->den - duty->num) * gp->n_val, duty->den);

[Severity: Low]
Since QCOM_CLK_GP_MND depends on ARM64 || COMPILE_TEST, this driver can be
built on 32-bit architectures.

In gp_mnd_clk_set_duty_cycle(), DIV_ROUND_UP() expands to plain C division on a
u64 dividend and a runtime unsigned int divisor (duty->den), which causes a
link failure (__udivdi3 / __aeabi_uldivmod undefined) on 32-bit COMPILE_TEST
builds.

Should this use DIV_ROUND_UP_ULL() instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-b4-pdm_clk_gp_mnd_v2-v2-0-ff8e4bc8d567@oss.qualcomm.com?part=3