Re: [PATCH v5 1/4] dt-bindings: PCI: qcom: document the Shikra PCIe controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Low] dt-bindings: PCI: qcom: legacy qcom,IP-SoC compatible order for
  Shikra
- [Low] dt-bindings: PCI: qcom: wrong interconnect slave and tags in
  Shikra example
- [Low] dt-bindings: PCI: qcom: dt-check-style strict violations in
  Shikra example
- [Low] dt-bindings: PCI: qcom: inactive maintainer email in
  qcom,pcie-shikra.yaml
--

commit 283a34a0a4cfab50d007b36100271b61fd8ac267
Author: Sushrut Shree Trivedi <sushrut.trivedi@oss.qualcomm.com>

dt-bindings: PCI: qcom: document the Shikra PCIe controller

This commit adds a Devicetree binding schema for the Qualcomm Shikra PCIe
root complex controller and registers its compatible string in
qcom-soc.yaml.

> diff --git a/Documentation/devicetree/bindings/arm/qcom-soc.yaml b/Documentation/devicetree/bindings/arm/qcom-soc.yaml
> index 179aadc86a9f0..72e0f1a824c69 100644
> --- a/Documentation/devicetree/bindings/arm/qcom-soc.yaml
> +++ b/Documentation/devicetree/bindings/arm/qcom-soc.yaml
> @@ -43,6 +43,7 @@ properties:
>          - pattern: "^qcom,sar[0-9]+[a-z]?-.*$"
>          - pattern: "^qcom,(sa|sc)8[0-9]+[a-z][a-z]?-.*$"
>          - pattern: "^qcom,(eliza|kaanapali|kuno|glymur|hawi|mahua|maili|milos|nord|shikra)-.+$"
> +        - const: qcom,pcie-shikra
>  
>          # Legacy namings - variations of existing patterns/compatibles are OK,
>          # but do not add completely new entries to these:

[Severity: Low]
Should this new binding use the qcom,SoC-IP naming order (qcom,shikra-pcie
and qcom,shikra-pcie.yaml) instead of legacy qcom,IP-SoC?

Documentation/devicetree/bindings/arm/qcom-soc.yaml requires new bindings
to use the qcom,SoC-IP format, and the pattern immediately above already
matches qcom,shikra-pcie without needing a one-off exception.  Other Shikra
bindings (such as qcom,shikra-gcc and qcom,shikra-qmp-gen2x1-pcie-phy) and
recent codename PCIe bindings (such as qcom,hawi-pcie and qcom,maili-pcie)
follow this convention.

> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-shikra.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-shikra.yaml
> new file mode 100644
> index 0000000000000..69463dae9f183
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-shikra.yaml
> @@ -0,0 +1,192 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/qcom,pcie-shikra.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Shikra PCI Express Root Complex
> +
> +maintainers:
> +  - Bjorn Andersson <andersson@kernel.org>
> +  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

[Severity: Low]
Should Manivannan Sadhasivam's email address be mani@kernel.org here?
Commit e3d7fda088c3 ("dt-bindings: PCI: Update the email address for
Manivannan Sadhasivam") replaced the inactive linaro.org address across
the PCI bindings.

[ ... ]

> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,shikra-gcc.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interconnect/qcom,icc.h>
> +    #include <dt-bindings/interconnect/qcom,shikra.h>

[ ... ]

> +                interconnects = <&system_noc MASTER_PCIE2_0 QCOM_ICC_TAG_ALWAYS
> +                                &mc_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>,
> +                                <&mem_noc MASTER_AMPSS_M0 QCOM_ICC_TAG_ACTIVE_ONLY
> +                                &config_noc SLAVE_PCIE2_0 QCOM_ICC_TAG_ACTIVE_ONLY>;
> +
> +                interconnect-names = "pcie-mem", "cpu-pcie";

[Severity: Low]
Does the cpu-pcie interconnect path point to the wrong slave on config_noc
and use the wrong interconnect tag macros?

In include/dt-bindings/interconnect/qcom,shikra.h, SLAVE_PCIE2_0 (29) is a
system_noc slave ID; on config_noc, index 29 is SLAVE_PKA_WRAPPER_CFG
(&qhs_pka_wrapper in drivers/interconnect/qcom/shikra.c), while the PCIe
config slave on config_noc is SLAVE_PCIE_CFG (26).

In addition, Shikra uses the SMD-RPM interconnect driver
(drivers/interconnect/qcom/icc-rpm.c) and
dt-bindings/interconnect/qcom,rpm-icc.h.  Passing QCOM_ICC_TAG_ACTIVE_ONLY
(0x3) from qcom,icc.h sets both RPM_ACTIVE_TAG (0x1) and RPM_SLEEP_TAG
(0x2), causing qcom_icc_bw_aggregate() to vote for both active and sleep
states instead of active only.

> +
> +                iommu-map = <0x0 &apps_smmu 0x800 0x1>,
> +                            <0x100 &apps_smmu 0x801 0x1>;
> +
> +                resets = <&gcc GCC_PCIE_BCR>;
> +                reset-names = "pci";
> +
> +                power-domains = <&gcc GCC_PCIE_GDSC>;
> +
> +                max-link-speed = <2>;
> +        };
> +
> +    };

[Severity: Low]
The example triggers several scripts/dtc/dt-check-style --mode=strict
warnings:

- pcie@45e8000 has an unused pcie: label.
- Properties inside pcie@45e8000 are indented by 8 spaces instead of 4
  spaces relative to the node, which also pushes several lines past 80
  columns.
- ranges is placed after #address-cells and #size-cells, and standard
  properties such as num-lanes are placed after the vendor-prefixed
  linux,pci-domain property.
- Continuation lines inside interconnects are aligned with the opening '<'
  instead of the first cell value after '<'.
- There is a trailing blank line before the closing brace of the soc node.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112083355.3884737-1-linghui.wu@oss.qualcomm.com?part=1