Re: [PATCH v3 1/2] cpufreq: spacemit: Add K1 cpufreq driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unaligned initial cluster frequencies can cause cluster 1 to undervoltage and crash during the first cpufreq transition.
--

commit de194d59d1b015bd0b960104ac00570cfd3ba072
Author: Shuwei Wu <shuwei.wu@mailbox.org>

cpufreq: spacemit: Add K1 cpufreq driver

This commit adds a CPU frequency scaling driver for the SpacemiT K1 SoC.
It handles two cluster clocks and one shared CPU voltage rail within a single
cpufreq policy.

> diff --git a/drivers/cpufreq/spacemit-k1-cpufreq.c b/drivers/cpufreq/spacemit-k1-cpufreq.c
> new file mode 100644
> index 0000000000000..631897eb80225
> --- /dev/null
> +++ b/drivers/cpufreq/spacemit-k1-cpufreq.c
[ ... ]
> +static int k1_cpufreq_set_target(struct cpufreq_policy *policy,
> +				 unsigned int index)
> +{
> +	struct k1_cpufreq_priv *priv = policy->driver_data;
> +	unsigned long old_freq = policy->cur * 1000UL;
> +	unsigned long new_freq = policy->freq_table[index].frequency * 1000UL;
> +	int ret;
> +
> +	if (!old_freq)
> +		old_freq = clk_get_rate(priv->cluster0_clk);
> +
> +	if (new_freq > old_freq) {
> +		ret = dev_pm_opp_set_rate(priv->cpu_dev, new_freq);

[Severity: High]
Could this cause an undervoltage crash on cluster 1 during the first transition?

The transition logic assumes old_freq (which comes from cluster 0) represents
the current speed of both clusters. If the bootloader leaves cluster 1 running
at a higher frequency than cluster 0 (e.g., cluster 0 at 500 MHz, cluster 1 at
1.2 GHz), and new_freq is an intermediate value like 800 MHz:

new_freq (800) > old_freq (500), so dev_pm_opp_set_rate() sets the shared
regulator to the 800 MHz voltage level. Since cluster 1 is invisible to the
OPP and regulator core, its voltage drops to the 800 MHz level while it is
still physically running at 1.2 GHz.

[ ... ]
> +static int k1_cpufreq_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	priv->cluster0_clk = clk_get(priv->cpu_dev, NULL);
> +	if (IS_ERR(priv->cluster0_clk)) {
> +		ret = PTR_ERR(priv->cluster0_clk);
> +		dev_err_probe(priv->cpu_dev, ret, "failed to get cluster0 clock\n");
> +		goto free_cpumask;
> +	}
> +
> +	priv->cluster1_clk = clk_get(cpu4_dev, NULL);
> +	if (IS_ERR(priv->cluster1_clk)) {

[Severity: High]
Should the driver synchronize both cluster clocks to a safe state (like the
maximum of their initial frequencies) here during probe before registering
the cpufreq driver? 

The clocks are acquired here but their initial rates are never read or aligned,
leaving them vulnerable to the mismatch described above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-shadow-deps-v3-0-2f3ba88611ff@mailbox.org?part=1