| Message ID | 20241204113329.3195627-2-quic_varada@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/6] dt-bindings: phy: qcom,uniphy-pcie: Document PCIe uniphy | expand |
On Wed, Dec 04, 2024 at 05:03:24PM +0530, Varadarajan Narayanan wrote: > From: Nitheesh Sekar <quic_nsekar@quicinc.com> > > Document the Qualcomm UNIPHY PCIe 28LP present in IPQ5332. > > Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > v2: Rename the file to match the compatible Either I look at wrong v1 from your cover letter or there was no such file in v1, so how it can be a rename? What happened here? > Drop 'driver' from title > Dropped 'clock-names' > Fixed 'reset-names' > -- > .../bindings/phy/qcom,uniphy-pcie.yaml | 82 +++++++++++++++++++ > 1 file changed, 82 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml > > diff --git a/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml > new file mode 100644 > index 000000000000..e0ad98a9f324 > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml This does not match compatible, so I don't see how it even matches your changelog. > @@ -0,0 +1,82 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/phy/qcom,uniphy-pcie.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm UNIPHY PCIe 28LP PHY > + > +maintainers: > + - Nitheesh Sekar <quic_nsekar@quicinc.com> > + - Varadarajan Narayanan <quic_varada@quicinc.com> > + > +description: > + PCIe and USB combo PHY found in Qualcomm IPQ5332 SoC > + > +properties: > + compatible: > + enum: > + - qcom,ipq5332-uniphy-pcie-gen3x1 Odd naming. Did anyone suggest this? I would expect something matches like everything else recent (see X1 for example). > + - qcom,ipq5332-uniphy-pcie-gen3x2 > + > + reg: > + maxItems: 1 > + > + clocks: > + minItems: 2 What happened here? This cannot be minItems and it never was. > + > + resets: > + minItems: 2 > + maxItems: 3 Why this varies? This patch is odd. Confusing changelog, v1 entirely different and not matching what is here, unusual and incorrect code in the binding itself. Provide changelog explaining WHY you did such odd changes. Open *LATEST* existing Qcom bindings and look how they do it. Do not implement things differently. Best regards, Krzysztof
On Thu, Dec 05, 2024 at 10:38:05AM +0100, Krzysztof Kozlowski wrote: > On Wed, Dec 04, 2024 at 05:03:24PM +0530, Varadarajan Narayanan wrote: > > From: Nitheesh Sekar <quic_nsekar@quicinc.com> > > > > Document the Qualcomm UNIPHY PCIe 28LP present in IPQ5332. > > > > Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com> > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > --- > > v2: Rename the file to match the compatible > > Either I look at wrong v1 from your cover letter or there was no such > file in v1, so how it can be a rename? > > What happened here? This driver was pulled in from [1] "Enable IPQ5018 PCI support (Nitheesh Sekar)" In that review, there was this feedback [4] ------------------------------- > +++ > b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie-28lp.yaml Filename should match compatibles and they do not use 28lp. ------------------------------- > > Drop 'driver' from title > > Dropped 'clock-names' > > Fixed 'reset-names' > > -- > > .../bindings/phy/qcom,uniphy-pcie.yaml | 82 +++++++++++++++++++ > > 1 file changed, 82 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml > > > > diff --git a/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml > > new file mode 100644 > > index 000000000000..e0ad98a9f324 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml > > This does not match compatible, so I don't see how it even matches your > changelog. Since this phy has both single and dual line capabilities I used the phy's name alone for the file name. Will rename this as qcom,ipq5332-uniphy-pcie-phy.yaml If this is not suitable, can you please suggest one that would be apt for this phy. > > @@ -0,0 +1,82 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/phy/qcom,uniphy-pcie.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Qualcomm UNIPHY PCIe 28LP PHY > > + > > +maintainers: > > + - Nitheesh Sekar <quic_nsekar@quicinc.com> > > + - Varadarajan Narayanan <quic_varada@quicinc.com> > > + > > +description: > > + PCIe and USB combo PHY found in Qualcomm IPQ5332 SoC > > + > > +properties: > > + compatible: > > + enum: > > + - qcom,ipq5332-uniphy-pcie-gen3x1 > > Odd naming. Did anyone suggest this? I would expect something matches > like everything else recent (see X1 for example). It was not suggested by anyone. Since [4] didn't comment on this continued to use it. Will change it as follows (similar to qcom,x1e80100-qmp-gen4x2-pcie-phy) qcom,ipq5332-uniphy-gen3x1-pcie-phy qcom,ipq5332-uniphy-gen3x2-pcie-phy > > > + - qcom,ipq5332-uniphy-pcie-gen3x2 > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + minItems: 2 > > What happened here? This cannot be minItems and it never was. Will fix this. > > + > > + resets: > > + minItems: 2 > > + maxItems: 3 > > Why this varies? > > This patch is odd. Confusing changelog, v1 entirely different and not > matching what is here, unusual and incorrect code in the binding itself. > > Provide changelog explaining WHY you did such odd changes. This series combines [1] and [2]. [1] introduces IPQ5018 PCIe support and [2] depends on [1] to introduce IPQ5332 PCIe support. Since the community was interested in [2] (please see [3]), tried to revive IPQ5332's PCIe support with this patch. Apologies for not expressing this in the cover letter. > Open *LATEST* existing Qcom bindings and look how they do it. Do not > implement things differently. Sure. Thanks Varada 1. Enable IPQ5018 PCI support (Nitheesh Sekar) - https://lore.kernel.org/all/20231003120846.28626-1-quic_nsekar@quicinc.com/ 2. Add PCIe support for Qualcomm IPQ5332 (Praveenkumar I) - https://lore.kernel.org/linux-arm-msm/20231214062847.2215542-1-quic_ipkumar@quicinc.com/ 3. Community interest - https://lore.kernel.org/linux-arm-msm/20240310132915.GE3390@thinkpad/ 4. dt-bindings feedback - https://lore.kernel.org/all/4bc021c1-0198-41a4-aa73-bf0cf0c0420a@linaro.org/
diff --git a/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml new file mode 100644 index 000000000000..e0ad98a9f324 --- /dev/null +++ b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml @@ -0,0 +1,82 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/phy/qcom,uniphy-pcie.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm UNIPHY PCIe 28LP PHY + +maintainers: + - Nitheesh Sekar <quic_nsekar@quicinc.com> + - Varadarajan Narayanan <quic_varada@quicinc.com> + +description: + PCIe and USB combo PHY found in Qualcomm IPQ5332 SoC + +properties: + compatible: + enum: + - qcom,ipq5332-uniphy-pcie-gen3x1 + - qcom,ipq5332-uniphy-pcie-gen3x2 + + reg: + maxItems: 1 + + clocks: + minItems: 2 + + resets: + minItems: 2 + maxItems: 3 + + reset-names: + minItems: 2 + items: + - const: phy + - const: phy_ahb + - const: phy_cfg + + "#phy-cells": + const: 0 + + "#clock-cells": + const: 0 + + clock-output-names: + maxItems: 1 + +required: + - compatible + - reg + - resets + - reset-names + - clocks + - "#phy-cells" + - "#clock-cells" + - clock-output-names + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/qcom,ipq5332-gcc.h> + + pcie0_phy: phy@4b0000 { + compatible = "qcom,ipq5332-uniphy-pcie-gen3x1"; + reg = <0x004b0000 0x800>; + + clocks = <&gcc GCC_PCIE3X1_0_PIPE_CLK>, + <&gcc GCC_PCIE3X1_PHY_AHB_CLK>; + + resets = <&gcc GCC_PCIE3X1_0_PHY_BCR>, + <&gcc GCC_PCIE3X1_PHY_AHB_CLK_ARES>, + <&gcc GCC_PCIE3X1_0_PHY_PHY_BCR>; + reset-names = "phy", + "phy_ahb", + "phy_cfg"; + + #clock-cells = <0>; + clock-output-names = "pcie0_pipe_clk_src"; + + #phy-cells = <0>; + };