Re: [PATCH v2 23/23] arm64: dts: qcom: hawi: Add DCP support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] arm64: dts: qcom: hawi: dcp virtio node placed under root instead of soc@0
- [High] arm64: dts: qcom: hawi: panel reset-gpios pin 210 mismatches pinctrl gpio211
- [Medium] arm64: dts: qcom: hawi: dcp left enabled in dtsi with unconnected endpoint
- [Low] arm64: dts: qcom: hawi: sde_disp0_vdd_active violates qcom,pmic-gpio schema
- [Low] arm64: dts: qcom: hawi: duplicate tlmm block and unsorted nodes in hawi-mtp
- [Low] arm64: dts: qcom: hawi: raw integer 13 used instead of IPCC_MPROC_DCP macro
--

commit 1a3114977b7eb966fe7d2b04c935e54daa02d988
Author: Jayant Shekhar <jayant.shekhar@oss.qualcomm.com>

arm64: dts: qcom: hawi: Add DCP support

This commit adds device tree nodes for the Qualcomm DCP VirtIO display
transport, subsystem, IOMMU mappings, and DSI endpoints in hawi.dtsi, and
adds the DSI panel, regulator, and pinctrl configuration in hawi-mtp.dts.

> diff --git a/arch/arm64/boot/dts/qcom/hawi-mtp.dts b/arch/arm64/boot/dts/qcom/hawi-mtp.dts
> index c38e5fab7a2cb..a27003f69d024 100644
> --- a/arch/arm64/boot/dts/qcom/hawi-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/hawi-mtp.dts

[ ... ]

> @@ -995,3 +1008,119 @@ &usb_dp_qmpphy {
>  &usb_dp_qmpphy_out {
>  	remote-endpoint = <&pmic_glink_ss_in>;
>  };
> +
> +&dcp_dsi0 {
> +	panel@0 {
> +		compatible = "novatek,nt37801";
> +		reg = <0>;
> +
> +		pinctrl-0 = <&sde_dsi_active &sde_te_active &sde_esync0_suspend
> +			     &sde_mdp_vsync_p_1p2_active &sde_mdp_vsync_p_1p8_active
> +			     &sde_disp0_rst_1p2_active &sde_disp0_rst_1p8_active>;
> +		pinctrl-1 = <&sde_dsi_suspend &sde_te_suspend &sde_esync0_suspend
> +			     &sde_mdp_vsync_p_1p2_active &sde_mdp_vsync_p_1p8_active
> +			     &sde_disp0_rst_1p2_active &sde_disp0_rst_1p8_active>;
> +		pinctrl-names = "default", "sleep";
> +
> +		vddio-supply = <&vreg_l12b_1p8>;
> +		vci-supply = <&vreg_l13b_3p0>;
> +		vdd-supply = <&display_panel_vdd>;
> +
> +		reset-gpios = <&tlmm 210 GPIO_ACTIVE_LOW>;
> +
> +		port {
> +			panel_dcp_in: endpoint {
> +				remote-endpoint = <&dcp_intf0_out>;
> +			};
> +		};
> +	};
> +};
> +
> +&dcp_intf0_out {
> +	remote-endpoint = <&panel_dcp_in>;
> +};
> +
> +&pmd8028_e1_gpios {
> +	sde_disp0_vdd_active: sde_disp0_vdd_active {
> +		pins = "gpio2";

[Severity: Low]
Does the child node name sde_disp0_vdd_active under &pmd8028_e1_gpios in
arch/arm64/boot/dts/qcom/hawi-mtp.dts cause a dtbs_check failure?

Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml sets
additionalProperties: false and requires pin configuration child node names
to match:

patternProperties:
  '-state$':
    oneOf:
      - $ref: '#/$defs/qcom-pmic-gpio-state'

Should this node be named sde-disp0-vdd-active-state to match the schema and
the other PMIC GPIO state nodes in this file?

[ ... ]

> +&tlmm {
> +	sde_te_active: sde-te-active-state {

[Severity: Low]
This isn't a bug, but arch/arm64/boot/dts/qcom/hawi-mtp.dts already has a
&tlmm block earlier in the file and keeps &label overrides sorted
alphabetically. Should the new TLMM pin states be merged into the existing
&tlmm node, and the new &dcp_dsi0, &dcp_intf0_out, &pmd8028_e1_gpios,
&pmh0110_d_e0_gpios, and &pmh0110_f_e0_gpios nodes be placed in alphabetical
order?

[ ... ]

> +	sde_dsi_active: sde-dsi-active-state {
> +		pins = "gpio211";
> +		function = "gpio";
> +		drive-strength = <8>;
> +		bias-disable;
> +	};
> +
> +	sde_dsi_suspend: sde-dsi-suspend-state {
> +		pins = "gpio211";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-pull-down;
> +	};
> +};

[Severity: High]
Is there a pin number mismatch between reset-gpios in panel@0 and the
sde_dsi_active / sde_dsi_suspend pinctrl states in
arch/arm64/boot/dts/qcom/hawi-mtp.dts?

When the novatek,nt37801 panel driver probes and transitions between default
and sleep pinctrl states, panel@0 under &dcp_dsi0 requests pin 210 for reset:

	reset-gpios = <&tlmm 210 GPIO_ACTIVE_LOW>;

However, sde_dsi_active and sde_dsi_suspend under &tlmm configure "gpio211"
instead of "gpio210". Since the Qualcomm TLMM controller uses 0-based pin
numbering, does this leave pin 210 without its active/sleep drive-strength
and pull-down settings while reconfiguring pin 211 instead, or should
reset-gpios and the pinctrl states reference the same pin?

> diff --git a/arch/arm64/boot/dts/qcom/hawi.dtsi b/arch/arm64/boot/dts/qcom/hawi.dtsi
> index 6b0a0ff0faa45..836970cdef718 100644
> --- a/arch/arm64/boot/dts/qcom/hawi.dtsi
> +++ b/arch/arm64/boot/dts/qcom/hawi.dtsi
> @@ -6841,4 +6841,44 @@ timer {
>  			     <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
>  			     <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
>  	};
> +
> +	dcp: virtio@9be8000 {
> +		compatible = "qcom,dcp-virtio";
> +		reg = <0x0 0x09be8000 0x0 0x1000>;

[Severity: High]
Was dcp: virtio@9be8000 in arch/arm64/boot/dts/qcom/hawi.dtsi meant to be
placed inside soc@0 rather than directly under the root node?

In arch/arm64/boot/dts/qcom/hawi.dtsi, all SoC MMIO peripherals live under
soc@0, which defines the bus ranges and 36-bit dma-ranges:

	soc@0 {
		compatible = "simple-bus";

		#address-cells = <2>;
		#size-cells = <2>;
		dma-ranges = <0x0 0x0 0x0 0x0 0x10 0x0>;
		ranges = <0x0 0x0 0x0 0x0 0x10 0x0>;

Because dcp: virtio@9be8000 is added after timer directly under the root
node, which has no ranges or dma-ranges properties, root-node schema
validation fails and of_dma_configure_id()->of_dma_get_range() cannot find
the SoC dma-ranges when configuring either the virtio@9be8000 transport
device or the child dcp-subsystem device in
drivers/gpu/drm/msm/dcp/dcp_drv.c:dcp_subsystem_dev_init():

	ret = of_dma_configure_id(dev, dcp_subsystem_node, true, NULL);
	if (ret) {
		dev_err(dev, "of_dma_configure_id failed: %d\n", ret);
		goto err_destroy;
	}

Should dcp: virtio@9be8000 be moved inside soc@0 (ordered by unit address
between camcc: clock-controller@956d000 and usb_1: usb@a600000)?

> +		interrupts-extended = <&intc GIC_ESPI 956 IRQ_TYPE_LEVEL_HIGH>,
> +					      <&ipcc 13 5 IRQ_TYPE_EDGE_RISING>;
> +		iommus = <&apps_smmu 0x15a3 0x0>;
> +		iommu-ranges = <0x0 0x84000000 0x0 0x04000000>;
> +		dma-coherent;
> +		mboxes = <&ipcc 13 5>;

[Severity: Low]
This isn't a bug, but since arch/arm64/boot/dts/qcom/hawi.dtsi includes
hawi-ipcc.h, could interrupts-extended and mboxes in dcp: virtio@9be8000 use
the symbolic constant for client 13 (defined as IPCC_MPROC_DCP in
hawi-ipcc.h) to match the other IPCC consumers in this file?

> +		mbox-names = "dcp-power-tx";
> +
> +		dcp-subsystem {
> +			compatible = "virtio,device34";
> +
> +			iommus = <&apps_smmu 0x400 0x2>;
> +
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				port@0 {
> +					reg = <0>;
> +					dcp_intf0_out: endpoint {
> +
> +					};
> +				};
> +
> +			};

[Severity: Medium]
Should dcp: virtio@9be8000 in arch/arm64/boot/dts/qcom/hawi.dtsi have
status = "disabled"; by default, with arch/arm64/boot/dts/qcom/hawi-mtp.dts
enabling &dcp?

The commit message notes "Enable the DCP display device on the Hawi MTP
platform", but dcp is left enabled in hawi.dtsi with an empty
dcp_intf0_out: endpoint node that has no remote-endpoint property.

In Documentation/devicetree/bindings/display/msm/qcom,dcp.yaml, every port
endpoint requires remote-endpoint:

    patternProperties:
      "^port@[0-9a-f]+$":
        $ref: /schemas/graph.yaml#/$defs/port-base
        unevaluatedProperties: false
        properties:
          endpoint:
            $ref: /schemas/graph.yaml#/$defs/endpoint-base
            unevaluatedProperties: false
            required:
              - remote-endpoint

In addition, on any Hawi board DTS that includes hawi.dtsi without attaching
a panel to dcp_intf0_out and dcp_dsi0, leaving dcp enabled causes
drivers/gpu/drm/msm/dcp/dcp_dpu_kms.c:dcp_dpu_kms_init_dsi_bridges() to add
dsi@0 to the DRM component match list even though no panel ever attaches to
register the component:

		dcp_kms->dsi_bridge[dsi_idx] = to_dcp_dsi_bridge(bridge);
		drm_of_component_match_add(dcp_kms->pm_dev, matchptr,
					   component_compare_of, dsi_np);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-jshekhar-dcp-hlos-v2-0-7cf014efdee1@oss.qualcomm.com?part=23