Re: [PATCH v1 1/4] clk: sunxi-ng: add cycle-masking divider (maskdiv) clock type

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ccu_maskdiv_determine_rate ignores the CLK_SET_RATE_PARENT flag, breaking rate propagation.
- [Medium] ccu_maskdiv_set_rate fails to apply CCU_FEATURE_UPDATE_BIT and CCU_FEATURE_KEY_FIELD, causing inconsistent state latching.
--

--- Patch [1]: [PATCH v1 1/4] clk: sunxi-ng: add cycle-masking divider (maskdiv) clock type ---
commit 07eec9d5dafd1fbf248e0e56bb3c42f8da46589d
Author: Juan Manuel López Carrillo <[email protected]>

clk: sunxi-ng: add cycle-masking divider (maskdiv) clock type

This commit introduces a cycle-masking divider clock type for sunxi-ng.
Instead of a linear divider, it masks M pulses out of every 2^width parent
cycles, which is used for hardware like the A523/T527 GPU clock.

> diff --git a/drivers/clk/sunxi-ng/ccu_maskdiv.c b/drivers/clk/sunxi-ng/ccu_maskdiv.c
> new file mode 100644
> index 0000000000000..2d3cccf081493
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu_maskdiv.c
> @@ -0,0 +1,199 @@

[ ... ]

> +static int ccu_maskdiv_determine_rate(struct clk_hw *hw,
> +				      struct clk_rate_request *req)
> +{
> +	struct ccu_maskdiv *cmd = hw_to_ccu_maskdiv(hw);
> +	unsigned long best_rate = 0, best_parent_rate = 0;
> +	struct clk_hw *best_parent = NULL;
> +	unsigned int best_m = UINT_MAX;
> +	unsigned int i;
> +
> +	for (i = 0; i < clk_hw_get_num_parents(hw); i++) {
> +		struct clk_hw *parent = clk_hw_get_parent_by_index(hw, i);
> +		unsigned long parent_rate, new_rate;
> +		unsigned int m;
> +
> +		if (!parent)
> +			continue;
> +
> +		parent_rate = clk_hw_get_rate(parent);
> +		m = ccu_maskdiv_find_m(parent_rate, req->rate, cmd->width);
> +		new_rate = ccu_maskdiv_calc_rate(parent_rate, m, cmd->width);

[Severity: High]
Does this implementation ignore the CLK_SET_RATE_PARENT flag?

By using clk_hw_get_rate(parent) instead of clk_hw_round_rate(), this loop
only evaluates parents at their currently locked rates. It appears this
prevents the framework from adjusting the parent's rate (such as the
underlying pll-gpu).

Could this silently break Dynamic Voltage and Frequency Scaling (DVFS) by
locking the GPU to a masked fraction of the PLL's fixed boot rate?

> +
> +		if (new_rate > req->rate)
> +			continue;

[ ... ]

> +static int ccu_maskdiv_set_rate(struct clk_hw *hw, unsigned long rate,
> +				unsigned long parent_rate)
> +{
> +	struct ccu_maskdiv *cmd = hw_to_ccu_maskdiv(hw);
> +	unsigned int m;
> +	unsigned long flags;
> +	u32 reg;
> +
> +	m = ccu_maskdiv_find_m(parent_rate, rate, cmd->width);
> +
> +	spin_lock_irqsave(cmd->common.lock, flags);
> +
> +	reg = readl(cmd->common.base + cmd->common.reg);
> +	reg &= ~GENMASK(cmd->shift + cmd->width - 1, cmd->shift);
> +	writel(reg | (m << cmd->shift), cmd->common.base + cmd->common.reg);

[Severity: Medium]
Will hardware register updates fail to latch here if the clock requires the
CCU_SUNXI_UPDATE_BIT or CCU_FEATURE_KEY_FIELD?

The standard ccu_common framework relies on these feature bits to
synchronize hardware changes, which ccu_mux_helper_set_parent() applies
automatically.

Since this direct write ignores cmd->common.features, could rate changes
silently fail to apply for clocks that require the update bit?

> +
> +	spin_unlock_irqrestore(cmd->common.lock, flags);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1