Re: [PATCH v22 11/14] arm64: dts: qcom: kaanapali: Add CAMSS and CSIPHY block definitions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm64: dts: qcom: kaanapali: CSIPHY opp-300000000 mismatches 266.67 MHz RCG
- [Low] arm64: dts: qcom: kaanapali: unused dt-bindings/phy/phy.h include
--

commit 42e005ff9edc4076b7d2be241a23c845a3953115
Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>

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

This commit adds the CAMSS and six CSIPHY device tree nodes along with the
CSIPHY OPP table to the Kaanapali SoC device tree.

> diff --git a/arch/arm64/boot/dts/qcom/kaanapali.dtsi b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> index e48a179d07220..11df1c7c6e31d 100644
> --- a/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> +++ b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> @@ -20,6 +20,7 @@
>  #include <dt-bindings/interconnect/qcom,kaanapali-rpmh.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/mailbox/qcom-ipcc.h>
> +#include <dt-bindings/phy/phy.h>

[Severity: Low]
This isn't a bug, but is the dt-bindings/phy/phy.h include needed here? None
of the PHY_NONE, PHY_TYPE_*, or PHY_POL_* constants from this header appear
to be used in kaanapali.dtsi or its board overlays.

[ ... ]

> @@ -6548,6 +6925,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>;
> +		};

[Severity: High]
Should this lowest OPP in csiphy_mxc_opp_table be 266666667 Hz instead of
300000000 Hz?

Each csiphy0..csiphy5 node uses CAM_CC_CSIPHYn_CLK as its core clock, which is
parented to cam_cc_cphy_rx_clk_src with CLK_SET_RATE_PARENT. In
drivers/clk/qcom/camcc-kaanapali.c, ftbl_cam_cc_cphy_rx_clk_src only supports
266.67 MHz, 400 MHz, and 480 MHz:

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 phy_qcom_mipi_csi2_power_on() -> phy_qcom_mipi_csi2_set_clock_rates() in
drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c runs for a CSI-2 link
frequency where link_freq / 4 <= 300000000 Hz:

	opp = dev_pm_opp_find_freq_ceil(dev, &opp_rate);
	...
		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);
	...
	ret = dev_pm_opp_set_rate(dev, opp_rate);

dev_pm_opp_find_freq_ceil() selects opp-300000000 and votes RPMHPD_MMCX and
RPMHPD_MXC to rpmhpd_opp_low_svs_d1. However, because cam_cc_cphy_rx_clk_src
uses clk_rcg2_shared_ops (ceiling lookup), dev_pm_opp_set_rate(dev, 300000000)
rounds the clock rate up to 400000000 Hz.

Doesn't this leave the CSIPHY core clock running at 400 MHz while MMCX and
MXC are only voted to rpmhpd_opp_low_svs_d1 instead of rpmhpd_opp_low_svs,
and make the 266666667 Hz clock rate unreachable?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-kaanapali-camss-v22-0-7da3ea49c8a1@oss.qualcomm.com?part=11

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy