Re: [PATCH 3/3] ARM: dts: qcom: msm8960: Add CPU frequency scaling support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `opp-supported-hw` property specifies an array of three all-1s masks, misusing the OPP core API and completely bypassing hardware speed bin filtering.
- [Medium] The commit implicitly corrects the `saw0` and `saw1` compatible strings without documenting this behavioral change in the commit message.
- [Low] The newly added properties `opp-microvolt-speed0-pvs0-v0` (and similar `-v0` properties) violate the device tree binding schema and will cause `make dt_binding_check` to fail.
--

commit 06323598829c4bb247922fa2b1d518f8024bc3f5
Author: Rudraksha Gupta <guptarud@gmail.com>

ARM: dts: qcom: msm8960: Add CPU frequency scaling support

Enable Krait DVFS on MSM8960 by adding the required device tree nodes:

- OPP table with 12 operating points from 384 MHz to 1.512 GHz, with
  per-PVS voltages for slow, nominal, and fast silicon bins.
- Krait clock controller (krait-cc-v1) driving the CPU muxes from
  PLL9/PLL10, ACC aux outputs, and PXO.
- PVS efuse nvmem cell in qfprom for the cpufreq-nvmem driver to
  read the speed-bin and process voltage class.
- CPU idle state for Standalone Power Collapse (SPC).
- operating-points-v2, clocks, cpu-supply, and cpu-idle-states wired
  into both CPU nodes.

Link: https://github.com/CyanogenMod/android_kernel_samsung_d2/blob/0dbe2b56847b304d30b809dfd08ba3b4a61d9af8/arch/arm/mach-msm/acpuclock-8960.c#L120-L235

> diff --git a/arch/arm/boot/dts/qcom/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom/qcom-msm8960.dtsi
> index fd28401cebb5e..50e51021e2d7c 100644
> --- a/arch/arm/boot/dts/qcom/qcom-msm8960.dtsi
> +++ b/arch/arm/boot/dts/qcom/qcom-msm8960.dtsi

[ ... ]

> +	cpu_opp_table: opp-table-cpu {
> +		compatible = "operating-points-v2-krait-cpu";
> +		nvmem-cells = <&pvs_efuse>;
> +
> +		opp-384000000 {
> +			opp-hz = /bits/ 64 <384000000>;
> +			opp-microvolt-speed0-pvs0-v0 = <950000>;

Does the opp-v2-kryo-cpu.yaml binding schema support this -v0 suffix?

The schema currently restricts microvolt property names using the regex
pattern ^opp-microvolt-speed[0-9]+-pvs[0-9]+$ and enforces
additionalProperties: false for OPP nodes.

Appending the -v0 suffix here will likely cause dtbs_check to reject it as
an undocumented property, introducing a schema validation regression.

> +			opp-microvolt-speed0-pvs1-v0 = <900000>;
> +			opp-microvolt-speed0-pvs3-v0 = <850000>;
> +			opp-supported-hw = <0xFFFFFFFF 0xFFFFFFFF 0xFFFFFFFF>;

Is this opp-supported-hw mask configured correctly?

The qcom-cpufreq-nvmem driver configures the OPP core to expect a single
32-bit hardware version mask (config.supported_hw_count = 1). In
drivers/opp/of.c:_read_supported_hw(), having three elements is interpreted as
three separate hardware versions.

Because every mask has all bits set to 1, the validation check
(val & drv->versions) will always evaluate to true, bypassing the
hardware speed bin filtering. If a chip evaluates to a higher speed bin (e.g.,
speed1), the OPP core will register these operating points but fail to find the
corresponding voltage properties, as only speed0 voltages are defined here.

Will this scale the CPU clock without appropriately scaling its voltage supply?

Should this be a single 32-bit mask representing the supported speed bin (e.g.,
<0x1> for speed0)?

> +		};

[ ... ]

>  		saw0: power-manager@2089000 {
> -			compatible = "qcom,msm8960-saw2-cpu", "qcom,saw2";
> +			compatible = "qcom,msm8960-saw2-v1.1-cpu", "qcom,saw2";

This isn't a bug, but this compatible string change isn't mentioned in the
commit message.

Since this alters the SPM node compatible strings and is functionally required
for the newly introduced cpu_spc idle state, could this correction be
documented in the commit message?

>  			reg = <0x02089000 0x1000>, <0x02009000 0x1000>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-expressatt_cpufreq-v1-0-487fd2d78859@gmail.com?part=3