Re: [PATCH v2 6/6] arm64: qcom: monaco: Add PCIe Endpoint nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: monaco: disallowed input-enable in pcie ep tlmm states
- [Low] arm64: dts: qcom: monaco: board-specific PCIe EP GPIOs and pinctrl in SoC dtsi
--

commit 59621afda0bee2426503fd1658910b3d133f8ac2
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

arm64: qcom: monaco: Add PCIe Endpoint nodes

This commit adds the PCIe Endpoint device tree nodes and default pin
configuration states for both PCIe controllers on the Qualcomm Monaco
SoC.

> diff --git a/arch/arm64/boot/dts/qcom/monaco.dtsi b/arch/arm64/boot/dts/qcom/monaco.dtsi
> index d7d135e87a8c7..12cd7d5f37a32 100644
> --- a/arch/arm64/boot/dts/qcom/monaco.dtsi
> +++ b/arch/arm64/boot/dts/qcom/monaco.dtsi

[ ... ]

> @@ -2405,6 +2406,57 @@ pcieport0: pcie@0 {
>  			};
>  		};
>  
> +		pcie0_ep: pcie-ep@1c00000 {

[ ... ]

> +			phys = <&pcie0_phy>;
> +			phy-names = "pciephy";
> +			reset-gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
> +			wake-gpios = <&tlmm 0 GPIO_ACTIVE_LOW>;
> +			pinctrl-0 = <&pcie0_ep_default_state>;
> +			pinctrl-names = "default";
> +			num-lanes = <2>;
> +			linux,pci-domain = <0>;
> +
> +			status = "disabled";
> +		};
> +
>  		pcie0_phy: phy@1c04000 {
>  			compatible = "qcom,qcs8300-qmp-gen4x2-pcie-phy";
>  			reg = <0x0 0x01c04000 0x0 0x2000>;
> @@ -2596,6 +2648,57 @@ pcieport1: pcie@0 {
>  			};
>  		};
>  
> +		pcie1_ep: pcie-ep@1c10000 {

[ ... ]

> +			phys = <&pcie1_phy>;
> +			phy-names = "pciephy";
> +			reset-gpios = <&tlmm 23 GPIO_ACTIVE_LOW>;
> +			wake-gpios = <&tlmm 21 GPIO_ACTIVE_LOW>;
> +			pinctrl-0 = <&pcie1_ep_default_state>;
> +			pinctrl-names = "default";
> +			num-lanes = <4>;
> +			linux,pci-domain = <1>;
> +
> +			status = "disabled";
> +		};

[Severity: Low]
Should reset-gpios, wake-gpios, and the pinctrl states be placed in the
board DTS files rather than hardcoded in the SoC-level pcie0_ep and
pcie1_ep nodes in arch/arm64/boot/dts/qcom/monaco.dtsi?

In drivers/pinctrl/qcom/pinctrl-qcs8300.c, only gpio1 (pcie0_clkreq) and
gpio22 (pcie1_clkreq) have dedicated PCIe pinmux functions, whereas
gpio0, gpio2, gpio21, and gpio23 use the generic gpio function (and
gpio21 through gpio23 are shared with qup0_se5). In addition, wake-gpios
is optional in drivers/pci/controller/dwc/pcie-qcom-ep.c via
devm_gpiod_get_optional(dev, "wake", GPIOD_OUT_LOW), and existing Monaco
boards such as monaco-arduino-monza.dts omit wake-gpios.

The Root Complex pcie0 and pcie1 nodes in monaco.dtsi, as well as the
pcie-ep nodes in other Qualcomm SoC DTSI files (such as lemans.dtsi,
sar2130p.dtsi, sm8450.dtsi, qcom-sdx55.dtsi, and qcom-sdx65.dtsi), leave
reset-gpios and wake-gpios to the board DTS files.

>  
>  		pcie1_phy: phy@1c14000 {
>  			compatible = "qcom,qcs8300-qmp-gen4x4-pcie-phy",
>  				     "qcom,sa8775p-qmp-gen4x4-pcie-phy";
> @@ -6225,6 +6328,50 @@ tlmm: pinctrl@f100000 {
>  			#interrupt-cells = <2>;
>  			wakeup-parent = <&pdc>;
>  
> +			pcie0_ep_default_state: pcie0-ep-default-state {
> +				wake-pins {
> +					pins = "gpio0";
> +					function = "gpio";
> +					drive-strength = <2>;
> +				};
> +
> +				clkreq-pins {
> +					pins = "gpio1";
> +					function = "pcie0_clkreq";
> +					drive-strength = <2>;
> +					bias-pull-up;
> +				};
> +
> +				perst-pins {
> +					pins = "gpio2";
> +					function = "gpio";
> +					input-enable;
> +					bias-pull-up;
> +				};
> +			};
> +
> +			pcie1_ep_default_state: pcie1-ep-default-state {
> +				wake-pins {
> +					pins = "gpio21";
> +					function = "gpio";
> +					drive-strength = <2>;
> +				};
> +
> +				clkreq-pins {
> +					pins = "gpio22";
> +					function = "pcie1_clkreq";
> +					drive-strength = <2>;
> +					bias-pull-up;
> +				};
> +
> +				perst-pins {
> +					pins = "gpio23";
> +					function = "gpio";
> +					input-enable;
> +					bias-pull-up;
> +				};
> +			};
> +
>  			cam0_default: cam0-default-state {
>  				pins = "gpio67";
>  				function = "cam_mclk";

[Severity: Medium]
Will using input-enable in the perst-pins subnodes of
pcie0_ep_default_state and pcie1_ep_default_state cause a dtbs_check
validation regression?

The tlmm node uses compatible "qcom,qcs8300-tlmm". In
Documentation/devicetree/bindings/pinctrl/qcom,qcs8300-tlmm.yaml,
qcom-qcs8300-tlmm-state validates all -state and -pins subnodes against
qcom,tlmm-common.yaml#/$defs/qcom-tlmm-state with
unevaluatedProperties: false:

  qcom-qcs8300-tlmm-state:
    type: object
    $ref: qcom,tlmm-common.yaml#/$defs/qcom-tlmm-state
    unevaluatedProperties: false

In Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml,
qcom-tlmm-state explicitly disallows input-enable:

      bias-pull-down: true
      bias-pull-up: true
      bias-disable: true
      input-enable: false
      output-disable: true
      output-enable: true

Qualcomm TLMM hardware does not have an input-buffer enable bit and uses
output-disable instead (or omits the property). Also, if any pin states
remain under tlmm in monaco.dtsi, should pcie0-ep-default-state and
pcie1-ep-default-state be placed in alphabetical order rather than before
cam0-default-state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-monaco_ep-v2-0-852cb9f5d012@oss.qualcomm.com?part=6