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 <[email protected]> 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/[email protected]?part=23
