Re: [PATCH v23 12/15] arm64: dts: qcom: kaanapali: Add CAMSS and CSIPHY block definitions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] arm64: dts: qcom: kaanapali: csiphy_mxc_opp_table uses invalid 300 MHz OPP
--

commit 0cfa3a0207d1730d9c7ff716309df9aad06f00ba
Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>

arm64: dts: qcom: kaanapali: Add CAMSS and CSIPHY block definitions

This commit adds device tree definitions for the CAMSS ISP, six CSIPHY blocks,
and the associated CSIPHY MXC operating points table on the Qualcomm Kaanapali
platform.

> diff --git a/arch/arm64/boot/dts/qcom/kaanapali.dtsi b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> index e48a179d07220..0c8fd7da55b78 100644
> --- a/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> +++ b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
[ ... ]
> @@ -6548,6 +6927,28 @@ pdp_tx: scp-sram-section@100 {
>  		};
>  	};
>  
> +	csiphy_mxc_opp_table: opp-table-mxc {
> +		compatible = "operating-points-v2";
> +
> +		opp-300000000 {
> +			opp-hz = /bits/ 64 <300000000>;
> +			required-opps = <&rpmhpd_opp_low_svs_d1>,
> +					<&rpmhpd_opp_low_svs_d1>;
> +		};
> +
> +		opp-400000000 {
> +			opp-hz = /bits/ 64 <400000000>;
> +			required-opps = <&rpmhpd_opp_low_svs>,
> +					<&rpmhpd_opp_low_svs>;
> +		};

[Severity: High]
Should the lowest operating point in csiphy_mxc_opp_table be 266666667 Hz
instead of 300000000 Hz?

In drivers/clk/qcom/camcc-kaanapali.c, the parent RCG for
CAM_CC_CSIPHY0_CLK through CAM_CC_CSIPHY5_CLK uses clk_rcg2_shared_ops
(which rounds up via qcom_find_freq()) and defines the following frequency
table:

static const struct freq_tbl ftbl_cam_cc_cphy_rx_clk_src[] = {
	F(266666667, P_CAM_CC_PLL0_OUT_MAIN, 4.5, 0, 0),
	F(400000000, P_CAM_CC_PLL0_OUT_MAIN, 3, 0, 0),
	F(480000000, P_CAM_CC_PLL0_OUT_MAIN, 2.5, 0, 0),
	{ }
};

When a sensor connected to csiphy0..csiphy5 configures a link frequency of
1.2 GHz or lower (so opp_rate = link_freq / 4 <= 300000000 Hz),
phy_qcom_mipi_csi2_configure() -> phy_qcom_mipi_csi2_set_clock_rates() in
drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c looks up the ceiling OPP and
votes the power domains before setting the clock rate:

	opp = dev_pm_opp_find_freq_ceil(dev, &opp_rate);
	...
	for (i = 0; i < csi2phy->pd_list->num_pds; i++) {
		...
		perf = dev_pm_opp_get_required_pstate(opp, pstate);
		pstate += 1;

		ret = dev_pm_genpd_set_performance_state(csi2phy->pd_list->pd_devs[i], perf);
		...
	}
	dev_pm_opp_put(opp);

	ret = dev_pm_opp_set_rate(dev, opp_rate);

Because csiphy_mxc_opp_table lists 300000000 Hz,
phy_qcom_mipi_csi2_set_clock_rates() selects opp-300000000, votes
RPMHPD_MMCX and RPMHPD_MXC to rpmhpd_opp_low_svs_d1, and then calls
dev_pm_opp_set_rate(dev, 300000000).

Won't requesting 300000000 Hz skip the 266.67 MHz frequency and round the
CSIPHY core clock up to 400000000 Hz while leaving MMCX and MXC at
rpmhpd_opp_low_svs_d1 instead of the rpmhpd_opp_low_svs corner required for
400 MHz?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-kaanapali-camss-v23-0-95f250b18612@oss.qualcomm.com?part=12