Re: [PATCH 2/3] arm64: dts: qcom: monaco: add CSI2 PHY common-phy providers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] arm64: dts: qcom: monaco: undocumented csiphy compatible and camss conflict
- [Medium] arm64: dts: qcom: monaco: missing #phy-cells on csiphy0..2 provider nodes
- [Medium] arm64: dts: qcom: monaco: unsupported 300 MHz OPP in csiphy_opp_table
- [Low] arm64: dts: qcom: monaco: csiphy_opp_table placed inside simple-bus soc@0
--

commit 6a19d429f6610cefdfd6dfc21d55e9665c60129f
Author: Anusha Arun Nandi <anusha.nandi@oss.qualcomm.com>

arm64: dts: qcom: monaco: add CSI2 PHY common-phy providers

This commit adds standalone qcom,qcs8300-csi2-phy device tree nodes
(csiphy0..2) and an associated operating points table (csiphy_opp_table)
to arch/arm64/boot/dts/qcom/monaco.dtsi.

> diff --git a/arch/arm64/boot/dts/qcom/monaco.dtsi b/arch/arm64/boot/dts/qcom/monaco.dtsi
> index cfe39f923224f..3d173a017122e 100644
> --- a/arch/arm64/boot/dts/qcom/monaco.dtsi
> +++ b/arch/arm64/boot/dts/qcom/monaco.dtsi
> @@ -5563,6 +5563,120 @@ cci2_i2c1: i2c-bus@1 {
>  			};
>  		};
>  
> +		csiphy0: phy@ac9c000 {
> +			compatible = "qcom,qcs8300-csi2-phy";
> +			reg = <0x0 0x0ac9c000 0x0 0x2000>;
> +
> +			clocks = <&camcc CAM_CC_CSIPHY0_CLK>,
> +				 <&camcc CAM_CC_CSI0PHYTIMER_CLK>,
> +				 <&camcc CAM_CC_CSID_CSIPHY_RX_CLK>;
> +			clock-names = "core",
> +				      "timer",
> +				      "csiphy_rx";
> +
> +			interrupts = <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>;

[Severity: High]
How are csiphy0, csiphy1, and csiphy2 expected to bind and coexist with the
camss node when enabled by a board or overlay?

First, the compatible string "qcom,qcs8300-csi2-phy" does not appear to be
documented under Documentation/devicetree/bindings/ or matched by any driver
in drivers/phy/ or drivers/media/, which will cause dtbs_check failures and
leave the nodes unbound.

Second, the commit message mentions that camss_detect_legacy_phy() selects
the non-legacy path while leaving the CSIPHY resources in the camss node
(isp@ac78000) intact, but camss_detect_legacy_phy() does not exist in the
tree. Instead, camss_probe() -> camss_init_subdevices() in
drivers/media/platform/qcom/camss/camss.c unconditionally initializes all
CSIPHY entries in qcs8300_resources:

drivers/media/platform/qcom/camss/camss.c:camss_init_subdevices() {
	for (i = 0; i < camss->res->csiphy_num; i++) {
		ret = msm_csiphy_subdev_init(camss, &camss->csiphy[i],
					     &res->csiphy_res[i],
					     res->csiphy_res[i].csiphy.id);
}

And msm_csiphy_subdev_init() in
drivers/media/platform/qcom/camss/camss-csiphy.c exclusively requests the
same MMIO regions (0xac9c000, 0xac9e000, 0xaca0000) and non-shared interrupts
(GIC_SPI 477..479):

drivers/media/platform/qcom/camss/camss-csiphy.c:msm_csiphy_subdev_init() {
	csiphy->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
	if (IS_ERR(csiphy->base))
		return PTR_ERR(csiphy->base);
	...
	ret = devm_request_irq(dev, csiphy->irq, csiphy->ops->isr,
			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
			       csiphy->irq_name, csiphy);
}

Wouldn't enabling any of csiphy0..2 alongside camss result in -EBUSY MMIO and
IRQ resource conflicts unless the dependent driver and binding patches are
present?

> +
> +			operating-points-v2 = <&csiphy_opp_table>;
> +
> +			power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> +			power-domain-names = "top";
> +
> +			status = "disabled";

[Severity: Medium]
Should csiphy0, csiphy1, and csiphy2 also include the #phy-cells property?

Because these nodes are named phy@... and described as generic-PHY provider
nodes, the phy-provider.yaml schema requires #phy-cells, and at runtime
_of_phy_get() in drivers/phy/phy-core.c requires it when resolving a phys
phandle:

drivers/phy/phy-core.c:_of_phy_get() {
	ret = of_parse_phandle_with_args(np, "phys", "#phy-cells",
		index, &args);
	if (ret)
		return ERR_PTR(-ENODEV);
}

Without #phy-cells on csiphy0..2, won't dtbs_check fail and _of_phy_get()
return ERR_PTR(-ENODEV) for any consumer referencing these providers?

[ ... ]

> +		csiphy_opp_table: opp-table-csiphy {
> +			compatible = "operating-points-v2";
> +
> +			opp-300000000 {
> +				opp-hz = /bits/ 64 <300000000>;
> +			};
> +
> +			opp-400000000 {
> +				opp-hz = /bits/ 64 <400000000>;
> +			};
> +		};
> +
>  		camss: isp@ac78000 {
>  			compatible = "qcom,qcs8300-camss";
>  

[Severity: Medium]
Is the 300 MHz entry (opp-300000000) in csiphy_opp_table supported by the
QCS8300 CAMCC clock driver?

In drivers/clk/qcom/camcc-sa8775p.c, the parent RCGs for these clocks
(cam_cc_cphy_rx_clk_src and cam_cc_csi0phytimer_clk_src through
cam_cc_csi2phytimer_clk_src) all use ftbl_cam_cc_cphy_rx_clk_src, which only
defines a 400 MHz frequency entry:

drivers/clk/qcom/camcc-sa8775p.c:
static const struct freq_tbl ftbl_cam_cc_cphy_rx_clk_src[] = {
	F(400000000, P_CAM_CC_PLL0_OUT_ODD, 1, 0, 0),
	{ }
};

Similarly, csiphy_res_8300 in drivers/media/platform/qcom/camss/camss.c only
lists 400000000 Hz for the QCS8300 CSIPHY clocks. Will requesting the 300 MHz
OPP fail clock rate lookup or round up to 400 MHz?

[Severity: Low]
Also, should csiphy_opp_table be placed under the root node (alongside
cpu0_opp_table and qup_opp_table) rather than directly inside soc@0, and
should phy@ac9c000..phy@aca0000 be placed after isp@ac78000 to maintain
unit-address ordering?

Because soc@0 is a simple-bus node with #address-cells = <2> and
#size-cells = <2>, placing opp-table-csiphy inside soc@0 without reg or
ranges triggers the check_simple_bus_reg() warning in scripts/dtc/checks.c
and fails simple-bus schema validation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017001116.2318647-1-anusha.nandi@oss.qualcomm.com?part=2