Re: [PATCH v3 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 and unmatched qcom,qcs8300-csi2-phy
- [High] arm64: dts: qcom: monaco: single power-domain on csiphy0..2 causes -EEXIST
- [Low] arm64: dts: qcom: monaco: csiphy0..2 placed before camss out of address order
- [Low] arm64: dts: qcom: monaco: commit message describes inter-version deltas
--

commit 73bdfe66a74e3ebe09640aa912bd64a757bca278
Author: Anusha Arun Nandi <anusha.nandi@oss.qualcomm.com>

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

This commit adds an OPP table and three disabled standalone CSI2 PHY
provider nodes (csiphy0, csiphy1, and csiphy2) to the Qualcomm Monaco
(QCS8300) devicetree.

>     arm64: dts: qcom: monaco: add CSI2 PHY common-phy providers
>     
>     Expose the three QCS8300 CSIPHYs as standalone
>     qcom,qcs8300-csi2-phy generic-PHY provider nodes (csiphy0..2) at their
>     existing register addresses, each with its own OF graph (port@0
>     sensor-facing, port@1 CAMSS-facing) so camss_detect_legacy_phy() picks
>     the non-legacy path once a board wires a sensor through one of them.
>     The camss node's own reg-names/clocks/interrupts for the CSIPHYs are
>     unchanged, so boards that don't use a provider still get the legacy
>     binding.
>     
>     csiphy0..2 drop the extra "csiphy_rx" clock: QCS8300 binds through the
>     sa8775p soc_cfg (no dedicated QCS8300 camcc driver exists), which only
>     declares two clocks ("core", "timer") for this PHY, not three.
>     
>     csiphy_opp_table moves to the root node, not inside soc@0 (a
>     simple-bus), since it has no reg/ranges property of its own; its
>     opp-300000000 entry is dropped too, since QCS8300 shares SA8775P's
>     camcc, whose cam_cc_cphy_rx_clk_src only defines a 400 MHz rate.
>     
>     The provider nodes are disabled by default; a board or camera overlay
>     enables the one(s) it uses.
>     
>     Reviewed-by: Jigarkumar Zala <jigarkumar.zala@oss.qualcomm.com>
>     Signed-off-by: Anusha Arun Nandi <anusha.nandi@oss.qualcomm.com>

[Severity: Low]
This isn't a bug, but arch/arm64/boot/dts/qcom/monaco.dtsi did not previously
define csiphy0..2 or csiphy_opp_table. The second and third paragraphs
describe changes between patch revisions (dropping the csiphy_rx clock,
moving csiphy_opp_table to the root node, and dropping opp-300000000) as if
modifying existing definitions in the tree, and also refer to the CSI2 PHY
driver soc_cfg as a camcc driver.

Should the inter-version changelog notes be moved below the --- tear line so
the commit description only explains the final state being added?

> diff --git a/arch/arm64/boot/dts/qcom/monaco.dtsi b/arch/arm64/boot/dts/qcom/monaco.dtsi
> index 35fbee63d4c65..a66c04a1d3063 100644
> --- a/arch/arm64/boot/dts/qcom/monaco.dtsi
> +++ b/arch/arm64/boot/dts/qcom/monaco.dtsi

[ ... ]

> @@ -5577,6 +5585,102 @@ 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>;
> +			clock-names = "core",
> +				      "timer";

[Severity: High]
As reported by Sashiko on v2 [1], csiphy0, csiphy1, and csiphy2 declare
compatible = "qcom,qcs8300-csi2-phy" with two clocks and no fallback
compatible string such as "qcom,sa8775p-csi2-phy".

In Documentation/devicetree/bindings/phy/qcom,x1e80100-csi2-phy.yaml, only
qcom,x1e80100-csi2-phy and qcom,glymur-csi2-phy are documented, and three
clocks (core, timer, ahb) are required:

  compatible:
    oneOf:
      - const: qcom,x1e80100-csi2-phy
      - items:
          - const: qcom,glymur-csi2-phy
          - const: qcom,x1e80100-csi2-phy

In drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c,
phy_qcom_mipi_csi2_of_match_table only matches qcom,x1e80100-csi2-phy, and no
qcom,qcs8300-csi2-phy or sa8775p soc_cfg entry exists in the driver:

  static const struct of_device_id phy_qcom_mipi_csi2_of_match_table[] = {
  	{ .compatible	= "qcom,x1e80100-csi2-phy", .data = &mipi_csi2_dphy_4nm_x1e },
  	{ }
  };

When a board or overlay enables any of csiphy0..2 and wires a sensor endpoint
through it, camss_detect_legacy_phy() selects non-legacy PHY mode and
msm_csiphy_subdev_init() in drivers/media/platform/qcom/camss/camss-csiphy.c
calls devm_phy_get_by_of_node():

  	csiphy->phy = devm_phy_get_by_of_node(dev, remote);
  	of_node_put(remote);
  	if (IS_ERR(csiphy->phy)) {
  		ret = PTR_ERR(csiphy->phy);
  		goto done;
  	}

Without a matching PHY driver and binding schema, will dtbs_check fail on the
undocumented compatible string and camss_probe() defer indefinitely with
-EPROBE_DEFER whenever one of these PHY nodes is enabled?

> +
> +			interrupts = <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>;
> +
> +			operating-points-v2 = <&csiphy_opp_table>;
> +
> +			power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> +			power-domain-names = "top";

[Severity: High]
As reported by Sashiko on v2 [2], csiphy0, csiphy1, and csiphy2 each specify
only a single power-domains entry.

When a platform device has a single power-domains phandle, platform_probe()
calls dev_pm_domain_attach() -> genpd_dev_pm_attach() in
drivers/pmdomain/core.c, which attaches that domain to dev->pm_domain before
the driver's probe callback runs:

  	if (of_count_phandle_with_args(dev->of_node, "power-domains",
  				       "#power-domain-cells") != 1)
  		return 0;

  	return __genpd_dev_pm_attach(dev, dev, 0, 1, true);

When phy_qcom_mipi_csi2_probe() in
drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c subsequently calls
phy_qcom_mipi_csi2_attach_pm_domains() -> devm_pm_domain_attach_list() ->
dev_pm_domain_attach_list() in drivers/base/power/common.c, it checks
dev->pm_domain and fails with -EEXIST:

  	if (dev->pm_domain)
  		return -EEXIST;

Furthermore, Documentation/devicetree/bindings/phy/qcom,x1e80100-csi2-phy.yaml
and drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c expect three power
domains (top, mmcx, and mx). Will having a single power-domains entry cause
phy_qcom_mipi_csi2_probe() to fail unconditionally with -EEXIST?

[ ... ]

> +		csiphy2: phy@aca0000 {
> +			compatible = "qcom,qcs8300-csi2-phy";
> +			reg = <0x0 0x0aca0000 0x0 0x2000>;

[ ... ]

> +		};
> +
>  		camss: isp@ac78000 {
>  			compatible = "qcom,qcs8300-camss";

[Severity: Low]
As reported by Sashiko on v2 [3], this isn't a bug, but under soc@0 in
arch/arm64/boot/dts/qcom/monaco.dtsi, MMIO child nodes are ordered by
ascending unit address. Here csiphy0 (phy@ac9c000), csiphy1 (phy@ac9e000),
and csiphy2 (phy@aca0000) are placed before camss (isp@ac78000) even though
0xac9c000..0xaca0000 are higher than 0xac78000.

Should csiphy0..2 be placed after camss: isp@ac78000 to maintain unit-address
order?

[1] https://lore.kernel.org/r/20261103221026.3662383-3-anusha.nandi@oss.qualcomm.com
[2] https://lore.kernel.org/r/20261103221026.3662383-3-anusha.nandi@oss.qualcomm.com
[3] https://lore.kernel.org/r/20261103221026.3662383-3-anusha.nandi@oss.qualcomm.com

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