Re: [PATCH 5/5] arm64: dts: qcom: Add GP M/N clock controller node for SA8775P and QCS8300

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Intent Mismatch / Incorrect File Modification
- [High] Missing 'status = "disabled";' causes unconditional probe and pin conflict
- [Low] Device Tree Node Sorting violation

Pre-existing issues:
- [High] Hardware Constraint Violation in duty cycle computation
--

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

arm64: dts: qcom: Add GP M/N clock controller node for SA8775P and QCS8300

This commit adds the GP M/N divider clock controller node to the SA8775P and
QCS8300 device trees. The new node uses the qcom,clk-gp-mnd compatible and is
clocked by the PDM XO4 and AHB clocks to expose a single clock output.

> Add the GP M/N divider clock controller node at 0x088d3000 to the
> SA8775P (kodiak, lemans) and QCS8300 (monaco) SoC device trees.

[Severity: High]
Is Kodiak actually an SA8775P variant? Kodiak is typically the core SoC
device tree for the SC7280 family (Snapdragon 7c Gen 3 / QCM6490), while
SA8775P is exclusively Lemans. Does modifying kodiak.dtsi incorrectly apply
this clock controller to the SC7280 family and break its device tree?

> diff --git a/arch/arm64/boot/dts/qcom/kodiak.dtsi b/arch/arm64/boot/dts/qcom/kodiak.dtsi
> --- a/arch/arm64/boot/dts/qcom/kodiak.dtsi
> +++ b/arch/arm64/boot/dts/qcom/kodiak.dtsi
> @@ -4412,6 +4412,20 @@ usb2_role_switch: endpoint {
>  			};
>  		};
>  
> +		gp_mn: clock-controller@88d3000 {

[Severity: Low]
Is this node placed in the correct sorting order? It appears to be inserted
sequentially after the usb@8c00000 node, but 0x088d3000 is less than
0x08c00000, which causes a unit-address sorting violation.

> +			compatible = "qcom,clk-gp-mnd";
> +			reg = <0x0 0x088d3000 0x0 0xc>;
> +			clocks = <&gcc GCC_PDM_XO4_CLK>,
> +				 <&gcc GCC_PDM_AHB_CLK>;
> +			clock-names = "pdm_clk", "ahb_clk";
> +			clock-output-names = "gp_mn_clk";
> +			#clock-cells = <0>;
> +			pinctrl-names = "active";
> +			pinctrl-0 = <&gp_mn_active>;
> +			assigned-clocks = <&gcc GCC_PDM_XO4_CLK>;
> +			assigned-clock-rates = <4800000>;
> +		};

[Severity: High]
Should this node include a status = "disabled" property? IP blocks that
export signals to external pads are usually disabled by default in SoC dtsi
files. 

Without it, does the qcom,clk-gp-mnd driver probe unconditionally on every
board, forcibly multiplexing gpio35 to the gp_mn function? This could
potentially break boards that require gpio35 for other functions like
qup_spi8_cs or qup_uart8_rx.

[ ... ]

> diff --git a/arch/arm64/boot/dts/qcom/lemans.dtsi b/arch/arm64/boot/dts/qcom/lemans.dtsi
> --- a/arch/arm64/boot/dts/qcom/lemans.dtsi
> +++ b/arch/arm64/boot/dts/qcom/lemans.dtsi
> @@ -4353,6 +4353,20 @@ opp-384000000 {
>  			};
>  		};
>  
> +		gp_mn: clock-controller@88d3000 {
> +			compatible = "qcom,clk-gp-mnd";
> +			reg = <0x0 0x088d3000 0x0 0xc>;
> +			clocks = <&gcc GCC_PDM_XO4_CLK>,
> +				 <&gcc GCC_PDM_AHB_CLK>;
> +			clock-names = "pdm_clk", "ahb_clk";
> +			clock-output-names = "gp_mn_clk";
> +			#clock-cells = <0>;
> +			pinctrl-names = "active";
> +			pinctrl-0 = <&gp_mn_active>;
> +			assigned-clocks = <&gcc GCC_PDM_XO4_CLK>;
> +			assigned-clock-rates = <4800000>;
> +		};

[Severity: High]
Would it be necessary to add a status = "disabled" property here as well to
prevent the unconditional multiplexing of gpio35?

[ ... ]

> diff --git a/arch/arm64/boot/dts/qcom/monaco.dtsi b/arch/arm64/boot/dts/qcom/monaco.dtsi
> --- a/arch/arm64/boot/dts/qcom/monaco.dtsi
> +++ b/arch/arm64/boot/dts/qcom/monaco.dtsi
> @@ -4867,6 +4867,20 @@ opp-384000000 {
>  			};
>  		};
>  
> +		gp_mn: clock-controller@88d3000 {
> +			compatible = "qcom,clk-gp-mnd";
> +			reg = <0x0 0x088d3000 0x0 0xc>;
> +			clocks = <&gcc GCC_PDM_XO4_CLK>,
> +				 <&gcc GCC_PDM_AHB_CLK>;
> +			clock-names = "pdm_clk", "ahb_clk";
> +			clock-output-names = "gp_mn_clk";
> +			#clock-cells = <0>;
> +			pinctrl-names = "active";
> +			pinctrl-0 = <&gp_mn_active>;
> +			assigned-clocks = <&gcc GCC_PDM_XO4_CLK>;
> +			assigned-clock-rates = <4800000>;
> +		};

[Severity: High]
Similarly, does this node also need status = "disabled" to ensure safe
default behavior?

[ ... ]

While reviewing this change, a pre-existing issue was noticed in the clock
driver itself regarding hardware duty cycle constraints:

drivers/clk/qcom/clk-gp-mnd.c:gp_mnd_clk_set_rate() {
        /*
         * When N = 2M + 1 the valid D range [M+1, M] is empty; no duty
         * cycle can satisfy M < D < (N - M).  Reject before touching hw.
         */
        if (n == 2 * m + 1)
                return -EINVAL;
        ...
        /* Program the closest-to-50% duty cycle. */
        d_val = n / 2;
        ret = regmap_write(gp->regmap, GP_MND_DUTY_REG, d_val);
}

[Severity: High]
This is a pre-existing issue, but does the driver fail to fully enforce the
hardware duty cycle constraint M < D < (N - M)?

If req->rate >= parent_rate / 2, rational_best_approximation can return M=1,
N=2. In this scenario, N <= 2 * M is true, making the check n == 2 * m + 1
evaluate to false, and the calculation proceeds.

Does the driver then blindly compute d_val = N / 2 = 1 and program M=1, D=1,
N=2 into the hardware, violating the M < D constraint?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-pdm_clk_gp_mnd_v1-v1-0-1522662b6c53@oss.qualcomm.com?part=5