[PATCH 0/6] Enable IPQ5018 PCI support

[PATCH 0/6] Enable IPQ5018 PCI support

[Nitheesh Sekar]

This patch series adds the relevant phy and controller
DT configurations for enabling PCI gen2 support
on IPQ5018.

Nitheesh Sekar (6):
  dt-bindings: phy: qcom,uniphy-pcie: Document PCIe uniphy
  dt-bindings: PCI: qcom: Add IPQ5108 SoC
  phy: qcom: Introduce PCIe UNIPHY 28LP driver
  PCI: qcom: Add support for IPQ5018
  arm64: dts: qcom: ipq5018: Add PCIe related nodes
  arm64: dts: qcom: ipq5018: Enable PCIe

 .../devicetree/bindings/pci/qcom,pcie.yaml    |  36 ++
 .../bindings/phy/qcom,uniphy-pcie-28lp.yaml   |  77 ++++
 .../arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts |   9 +
 arch/arm64/boot/dts/qcom/ipq5018.dtsi         | 186 +++++++++-
 drivers/pci/controller/dwc/pcie-qcom.c        |  22 +-
 drivers/phy/qualcomm/Kconfig                  |  12 +
 drivers/phy/qualcomm/Makefile                 |   1 +
 .../phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c  | 336 ++++++++++++++++++
 8 files changed, 663 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/phy/qcom,uniphy-pcie-28lp.yaml
 create mode 100644 drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c

-- 
2.17.1

[PATCH 1/6] dt-bindings: phy: qcom,uniphy-pcie: Document PCIe uniphy

[Nitheesh Sekar]

Document the Qualcomm UNIPHY PCIe 28LP present in IPQ5018.

Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
---
 .../bindings/phy/qcom,uniphy-pcie-28lp.yaml   | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/qcom,uniphy-pcie-28lp.yaml

diff --git a/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie-28lp.yaml b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie-28lp.yaml
new file mode 100644
index 000000000000..6b2574f9532e
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie-28lp.yaml
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/qcom,uniphy-pcie-28lp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm UNIPHY PCIe 28LP PHY driver
+
+maintainers:
+  - Nitheesh Sekar <quic_nsekar@quicinc.com>
+  - Sricharan Ramabadhran <quic_srichara@quicinc.com>
+
+properties:
+  compatible:
+    enum:
+      - qcom,ipq5018-uniphy-pcie-gen2x1
+      - qcom,ipq5018-uniphy-pcie-gen2x2
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: pipe_clk
+
+  resets:
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: phy
+      - const: phy_phy
+
+  "#phy-cells":
+    const: 0
+
+  "#clock-cells":
+    const: 0
+
+  clock-output-names:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - resets
+  - reset-names
+  - clocks
+  - clock-names
+  - "#phy-cells"
+  - "#clock-cells"
+  - clock-output-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-ipq5018.h>
+    #include <dt-bindings/reset/qcom,gcc-ipq5018.h>
+
+    phy@86000 {
+        compatible = "qcom,ipq5018-uniphy-pcie-gen2x2";
+        reg = <0x86000 0x800>;
+        #phy-cells = <0>;
+        #clock-cells = <0>;
+        clocks = <&gcc GCC_PCIE0_PIPE_CLK>;
+        clock-names = "pipe_clk";
+        clock-output-names = "pcie0_pipe_clk";
+        assigned-clocks = <&gcc GCC_PCIE1_PIPE_CLK>;
+        assigned-clock-rates = <125000000>;
+        resets = <&gcc GCC_PCIE0_PHY_BCR>,
+                 <&gcc GCC_PCIE0PHY_PHY_BCR>;
+        reset-names = "phy", "phy_phy";
+    };
-- 
2.17.1

Re: [PATCH 1/6] dt-bindings: phy: qcom,uniphy-pcie: Document PCIe uniphy

[Krzysztof Kozlowski]

On 03/10/2023 14:08, Nitheesh Sekar wrote:
> Document the Qualcomm UNIPHY PCIe 28LP present in IPQ5018.
> 
> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>

Thank you for your patch. There is something to discuss/improve.


> ---
>  .../bindings/phy/qcom,uniphy-pcie-28lp.yaml   | 77 +++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/qcom,uniphy-pcie-28lp.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie-28lp.yaml b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie-28lp.yaml
> new file mode 100644
> index 000000000000..6b2574f9532e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie-28lp.yaml

Filename should match compatibles and they do not use 28lp.

> @@ -0,0 +1,77 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/qcom,uniphy-pcie-28lp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm UNIPHY PCIe 28LP PHY driver

Driver as Linux driver? Drop. Describe the hardware instead.

> +
> +maintainers:
> +  - Nitheesh Sekar <quic_nsekar@quicinc.com>
> +  - Sricharan Ramabadhran <quic_srichara@quicinc.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,ipq5018-uniphy-pcie-gen2x1
> +      - qcom,ipq5018-uniphy-pcie-gen2x2
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: pipe_clk

Drop _clk... or even drop entire clock-names. Not needed for one entry.

> +
> +  resets:
> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: phy
> +      - const: phy_phy

These are absolutely terrible names. If you have third one, it would be
"phy_phy_phy"? Drop or provide something useful.

> +
> +  "#phy-cells":
> +    const: 0
> +
> +  "#clock-cells":
> +    const: 0
> +
> +  clock-output-names:
> +    maxItems: 1

Best regards,
Krzysztof

Re: [PATCH 1/6] dt-bindings: phy: qcom,uniphy-pcie: Document PCIe uniphy

[Nitheesh Sekar]

On 10/4/2023 12:27 PM, Krzysztof Kozlowski wrote:
> On 03/10/2023 14:08, Nitheesh Sekar wrote:
>> Document the Qualcomm UNIPHY PCIe 28LP present in IPQ5018.
>>
>> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
> Thank you for your patch. There is something to discuss/improve.
Sure. Will learn and improve.
>
>
>> ---
>>   .../bindings/phy/qcom,uniphy-pcie-28lp.yaml   | 77 +++++++++++++++++++
>>   1 file changed, 77 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/phy/qcom,uniphy-pcie-28lp.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie-28lp.yaml b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie-28lp.yaml
>> new file mode 100644
>> index 000000000000..6b2574f9532e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie-28lp.yaml
> Filename should match compatibles and they do not use 28lp.
Sure. will remove the 28lp. added it based on the file name.
Is "qcom,uniphy-pcie.yaml" good ? Because this will be used for other 
SoCs as well which will have different compatibles. So i did not include 
the SoC name, lane and speed info which i have used int he compatible 
names.
>
>> @@ -0,0 +1,77 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/phy/qcom,uniphy-pcie-28lp.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm UNIPHY PCIe 28LP PHY driver
> Driver as Linux driver? Drop. Describe the hardware instead.
>
>> +
>> +maintainers:
>> +  - Nitheesh Sekar <quic_nsekar@quicinc.com>
>> +  - Sricharan Ramabadhran <quic_srichara@quicinc.com>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,ipq5018-uniphy-pcie-gen2x1
>> +      - qcom,ipq5018-uniphy-pcie-gen2x2
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    items:
>> +      - const: pipe_clk
> Drop _clk... or even drop entire clock-names. Not needed for one entry.
Sure. Will drop "_clk" part.
>
>> +
>> +  resets:
>> +    maxItems: 2
>> +
>> +  reset-names:
>> +    items:
>> +      - const: phy
>> +      - const: phy_phy
> These are absolutely terrible names. If you have third one, it would be
> "phy_phy_phy"? Drop or provide something useful.
Sure. Will update.

Thanks,
Nitheesh

[PATCH 2/6] dt-bindings: PCI: qcom: Add IPQ5108 SoC

[Nitheesh Sekar]

Add support for the PCIe controller on the Qualcomm
IPQ5108 SoC to the bindings.

Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
---
 .../devicetree/bindings/pci/qcom,pcie.yaml    | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
index eadba38171e1..72e24094ec7e 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
@@ -21,6 +21,7 @@ properties:
           - qcom,pcie-apq8064
           - qcom,pcie-apq8084
           - qcom,pcie-ipq4019
+          - qcom,pcie-ipq5018
           - qcom,pcie-ipq6018
           - qcom,pcie-ipq8064
           - qcom,pcie-ipq8064-v2
@@ -170,6 +171,7 @@ allOf:
         compatible:
           contains:
             enum:
+              - qcom,pcie-ipq5018
               - qcom,pcie-ipq6018
               - qcom,pcie-ipq8074-gen3
     then:
@@ -332,6 +334,39 @@ allOf:
             - const: ahb # AHB reset
             - const: phy_ahb # PHY AHB reset
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,pcie-ipq5018
+    then:
+      properties:
+        clocks:
+          minItems: 6
+          maxItems: 6
+        clock-names:
+          items:
+            - const: iface # PCIe to SysNOC BIU clock
+            - const: axi_m # AXI Master clock
+            - const: axi_s # AXI Slave clock
+            - const: ahb # AHB clock
+            - const: aux # Auxiliary clock
+            - const: axi_bridge # AXI bridge clock
+        resets:
+          minItems: 8
+          maxItems: 8
+        reset-names:
+          items:
+            - const: pipe # PIPE reset
+            - const: sleep # Sleep reset
+            - const: sticky # Core sticky reset
+            - const: axi_m # AXI master reset
+            - const: axi_s # AXI slave reset
+            - const: ahb # AHB reset
+            - const: axi_m_sticky # AXI master sticky reset
+            - const: axi_s_sticky # AXI slave sticky reset
+
   - if:
       properties:
         compatible:
@@ -790,6 +825,7 @@ allOf:
               enum:
                 - qcom,pcie-apq8064
                 - qcom,pcie-ipq4019
+                - qcom,pcie-ipq5018
                 - qcom,pcie-ipq8064
                 - qcom,pcie-ipq8064v2
                 - qcom,pcie-ipq8074
-- 
2.17.1

Re: [PATCH 2/6] dt-bindings: PCI: qcom: Add IPQ5108 SoC

[Krzysztof Kozlowski]

On 03/10/2023 14:08, Nitheesh Sekar wrote:
> Add support for the PCIe controller on the Qualcomm
> IPQ5108 SoC to the bindings.
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

This is an automated instruction, just in case, because many review tags
are being ignored. If you know the process, you can skip it (please do
not feel offended by me posting it here - no bad intentions intended).
If you do not know the process, here is a short explanation:

Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577

Best regards,
Krzysztof

Re: [PATCH 2/6] dt-bindings: PCI: qcom: Add IPQ5108 SoC

[Konrad Dybcio]

On 3.10.2023 14:08, Nitheesh Sekar wrote:
> Add support for the PCIe controller on the Qualcomm
> IPQ5108 SoC to the bindings.
> 
> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
> ---
[...]

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,pcie-ipq5018
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 6
> +          maxItems: 6
> +        clock-names:
> +          items:
> +            - const: iface # PCIe to SysNOC BIU clock
What's a BIU?

Konrad

Re: [PATCH 2/6] dt-bindings: PCI: qcom: Add IPQ5108 SoC

[Nitheesh Sekar]

On 10/7/2023 5:55 AM, Konrad Dybcio wrote:
> On 3.10.2023 14:08, Nitheesh Sekar wrote:
>> Add support for the PCIe controller on the Qualcomm
>> IPQ5108 SoC to the bindings.
>>
>> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
>> ---
> [...]
>
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,pcie-ipq5018
>> +    then:
>> +      properties:
>> +        clocks:
>> +          minItems: 6
>> +          maxItems: 6
>> +        clock-names:
>> +          items:
>> +            - const: iface # PCIe to SysNOC BIU clock
> What's a BIU?
>
> Konrad
It is Bus Interface Unit.

Thanks,
Nitheesh

[PATCH 3/6] phy: qcom: Introduce PCIe UNIPHY 28LP driver

[Nitheesh Sekar]

Add Qualcomm PCIe UNIPHY 28LP driver support present
in Qualcomm IPQ5018 SoC and the phy init sequence.

Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
---
 drivers/phy/qualcomm/Kconfig                  |  12 +
 drivers/phy/qualcomm/Makefile                 |   1 +
 .../phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c  | 336 ++++++++++++++++++
 3 files changed, 349 insertions(+)
 create mode 100644 drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c

diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
index d891058b7c39..b7d37cd98f02 100644
--- a/drivers/phy/qualcomm/Kconfig
+++ b/drivers/phy/qualcomm/Kconfig
@@ -154,6 +154,18 @@ config PHY_QCOM_M31_USB
 	  management. This driver is required even for peripheral only or
 	  host only mode configurations.
 
+config PHY_QCOM_UNIPHY_PCIE_28LP
+	bool "PCIE UNIPHY 28LP PHY driver"
+	depends on ARCH_QCOM
+	depends on HAS_IOMEM
+	depends on OF
+	select GENERIC_PHY
+	help
+	  Enable this to support the PCIe UNIPHY 28LP phy transceiver that
+	  is used with PCIe controllers on Qualcomm IPQ5018 chips. It
+	  handles PHY initialization, clock management required after
+	  resetting the hardware and power management.
+
 config PHY_QCOM_USB_HS
 	tristate "Qualcomm USB HS PHY module"
 	depends on USB_ULPI_BUS
diff --git a/drivers/phy/qualcomm/Makefile b/drivers/phy/qualcomm/Makefile
index ffd609ac6233..31105cd17bc9 100644
--- a/drivers/phy/qualcomm/Makefile
+++ b/drivers/phy/qualcomm/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_PHY_QCOM_QMP_USB_LEGACY)	+= phy-qcom-qmp-usb-legacy.o
 obj-$(CONFIG_PHY_QCOM_QUSB2)		+= phy-qcom-qusb2.o
 obj-$(CONFIG_PHY_QCOM_SNPS_EUSB2)	+= phy-qcom-snps-eusb2.o
 obj-$(CONFIG_PHY_QCOM_EUSB2_REPEATER)	+= phy-qcom-eusb2-repeater.o
+obj-$(CONFIG_PHY_QCOM_UNIPHY_PCIE_28LP)	+= phy-qcom-uniphy-pcie-28lp.o
 obj-$(CONFIG_PHY_QCOM_USB_HS) 		+= phy-qcom-usb-hs.o
 obj-$(CONFIG_PHY_QCOM_USB_HSIC) 	+= phy-qcom-usb-hsic.o
 obj-$(CONFIG_PHY_QCOM_USB_HS_28NM)	+= phy-qcom-usb-hs-28nm.o
diff --git a/drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c b/drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c
new file mode 100644
index 000000000000..5ef6ae7276cf
--- /dev/null
+++ b/drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c
@@ -0,0 +1,336 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2023, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
+#include <linux/reset.h>
+#include <linux/of_device.h>
+#include <linux/delay.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+
+#define PIPE_CLK_DELAY_MIN_US			5000
+#define PIPE_CLK_DELAY_MAX_US			5100
+#define CDR_CTRL_REG_1		0x80
+#define CDR_CTRL_REG_2		0x84
+#define CDR_CTRL_REG_3		0x88
+#define CDR_CTRL_REG_4		0x8C
+#define CDR_CTRL_REG_5		0x90
+#define CDR_CTRL_REG_6		0x94
+#define CDR_CTRL_REG_7		0x98
+#define SSCG_CTRL_REG_1		0x9c
+#define SSCG_CTRL_REG_2		0xa0
+#define SSCG_CTRL_REG_3		0xa4
+#define SSCG_CTRL_REG_4		0xa8
+#define SSCG_CTRL_REG_5		0xac
+#define SSCG_CTRL_REG_6		0xb0
+#define PCS_INTERNAL_CONTROL_2	0x2d8
+
+#define PHY_MODE_FIXED		0x1
+
+enum qcom_uniphy_pcie_type {
+	PHY_TYPE_PCIE = 1,
+	PHY_TYPE_PCIE_GEN2,
+	PHY_TYPE_PCIE_GEN3,
+};
+
+struct uniphy_regs {
+	unsigned int offset;
+	unsigned int val;
+};
+
+struct uniphy_pcie_data {
+	int lanes;
+	/* 2nd lane offset */
+	int lane_offset;
+	unsigned int phy_type;
+	const struct uniphy_regs *init_seq;
+	unsigned int init_seq_num;
+};
+
+struct qcom_uniphy_pcie {
+	struct phy phy;
+	struct device *dev;
+	const struct uniphy_pcie_data *data;
+	struct clk_bulk_data *clks;
+	int num_clks;
+	struct reset_control *resets;
+	void __iomem *base;
+};
+
+#define	phy_to_dw_phy(x)	container_of((x), struct qca_uni_pcie_phy, phy)
+
+static const struct uniphy_regs ipq5018_regs[] = {
+	{
+		.offset = SSCG_CTRL_REG_4,
+		.val = 0x1cb9,
+	},
+	{
+		.offset = SSCG_CTRL_REG_5,
+		.val = 0x023a,
+	},
+	{
+		.offset = SSCG_CTRL_REG_3,
+		.val = 0xd360,
+	},
+	{
+		.offset = SSCG_CTRL_REG_1,
+		.val = 0x1,
+	},
+	{
+		.offset = SSCG_CTRL_REG_2,
+		.val = 0xeb,
+	},
+	{
+		.offset = CDR_CTRL_REG_4,
+		.val = 0x3f9,
+	},
+	{
+		.offset = CDR_CTRL_REG_5,
+		.val = 0x1c9,
+	},
+	{
+		.offset = CDR_CTRL_REG_2,
+		.val = 0x419,
+	},
+	{
+		.offset = CDR_CTRL_REG_1,
+		.val = 0x200,
+	},
+	{
+		.offset = PCS_INTERNAL_CONTROL_2,
+		.val = 0xf101,
+	},
+};
+
+static const struct uniphy_pcie_data ipq5018_2x2_data = {
+	.lanes		= 2,
+	.lane_offset	= 0x800,
+	.phy_type	= PHY_TYPE_PCIE_GEN2,
+	.init_seq	= ipq5018_regs,
+	.init_seq_num	= ARRAY_SIZE(ipq5018_regs),
+};
+
+static void qcom_uniphy_pcie_init(struct qcom_uniphy_pcie *phy)
+{
+	const struct uniphy_pcie_data *data = phy->data;
+	const struct uniphy_regs *init_seq;
+	void __iomem *base = phy->base;
+	int lane = 0;
+	int i;
+
+	while (lane != data->lanes) {
+		init_seq = data->init_seq;
+
+		for (i = 0; i < data->init_seq_num; i++, init_seq++)
+			writel(init_seq->val, base + init_seq->offset);
+
+		if (data->lanes == 2)
+			base = base + data->lane_offset;
+
+		lane++;
+	}
+}
+
+static int qcom_uniphy_pcie_power_off(struct phy *x)
+{
+	struct qcom_uniphy_pcie *phy = phy_get_drvdata(x);
+
+	reset_control_assert(phy->resets);
+
+	clk_bulk_disable_unprepare(phy->num_clks, phy->clks);
+
+	return 0;
+}
+
+static int qcom_uniphy_pcie_power_on(struct phy *x)
+{
+	int ret;
+	struct qcom_uniphy_pcie *phy = phy_get_drvdata(x);
+
+	ret = reset_control_assert(phy->resets);
+	if (ret) {
+		dev_err(phy->dev, "reset assert failed (%d)\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Delay periods before and after reset deassert are working values
+	 * from downstream Codeaurora kernel
+	 */
+	usleep_range(100, 150);
+
+	ret = reset_control_deassert(phy->resets);
+	if (ret) {
+		dev_err(phy->dev, "reset deassert failed (%d)\n", ret);
+		return ret;
+	}
+
+	usleep_range(PIPE_CLK_DELAY_MIN_US, PIPE_CLK_DELAY_MAX_US);
+
+	ret = clk_bulk_prepare_enable(phy->num_clks, phy->clks);
+	if (ret) {
+		dev_err(phy->dev, "clk prepare and enable failed %d\n", ret);
+		return ret;
+	}
+
+	usleep_range(30, 50);
+
+	qcom_uniphy_pcie_init(phy);
+	return 0;
+}
+
+static int qcom_uniphy_pcie_get_resources(struct platform_device *pdev,
+					  struct qcom_uniphy_pcie *phy)
+{
+	struct resource *res;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	phy->base = devm_ioremap_resource(phy->dev, res);
+	if (IS_ERR(phy->base)) {
+		dev_err(phy->dev, "cannot get phy registers\n");
+		return PTR_ERR(phy->base);
+	}
+
+	phy->num_clks = devm_clk_bulk_get_all(phy->dev, &phy->clks);
+	if (phy->num_clks < 0)
+		return phy->num_clks;
+
+	phy->resets = devm_reset_control_array_get_exclusive(phy->dev);
+	if (IS_ERR(phy->resets))
+		return PTR_ERR(phy->resets);
+
+	return 0;
+}
+
+/*
+ * Register a fixed rate pipe clock.
+ *
+ * The <s>_pipe_clksrc generated by PHY goes to the GCC that gate
+ * controls it. The <s>_pipe_clk coming out of the GCC is requested
+ * by the PHY driver for its operations.
+ * We register the <s>_pipe_clksrc here. The gcc driver takes care
+ * of assigning this <s>_pipe_clksrc as parent to <s>_pipe_clk.
+ * Below picture shows this relationship.
+ *
+ *         +---------------+
+ *         |   PHY block   |<<---------------------------------------+
+ *         |               |                                         |
+ *         |   +-------+   |                   +-----+               |
+ *   I/P---^-->|  PLL  |---^--->pipe_clksrc--->| GCC |--->pipe_clk---+
+ *    clk  |   +-------+   |                   +-----+
+ *         +---------------+
+ */
+static int phy_pipe_clk_register(struct qcom_uniphy_pcie  *phy,
+				 struct device_node *np)
+{
+	struct clk_fixed_rate *fixed;
+	struct clk_init_data init = { };
+	int ret;
+
+	ret = of_property_read_string(np, "clock-output-names", &init.name);
+	if (ret) {
+		dev_err(phy->dev, "%pOFn: No clock-output-names\n", np);
+		return ret;
+	}
+
+	fixed = devm_kzalloc(phy->dev, sizeof(*fixed), GFP_KERNEL);
+	if (!fixed)
+		return -ENOMEM;
+
+	init.ops = &clk_fixed_rate_ops;
+	fixed->fixed_rate = 125000000;
+	fixed->hw.init = &init;
+
+	ret = devm_clk_hw_register(phy->dev, &fixed->hw);
+	if (ret)
+		return ret;
+
+	ret = devm_of_clk_add_hw_provider(phy->dev, of_clk_hw_simple_get,
+					  &fixed->hw);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct of_device_id qcom_uniphy_pcie_id_table[] = {
+	{
+		.compatible = "qcom,ipq5018-uniphy-pcie-gen2x2",
+		.data = &ipq5018_2x2_data,
+	},
+	{ /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, qcom_uniphy_pcie_id_table);
+
+static const struct phy_ops pcie_ops = {
+	.power_on	= qcom_uniphy_pcie_power_on,
+	.power_off	= qcom_uniphy_pcie_power_off,
+	.owner          = THIS_MODULE,
+};
+
+static int qcom_uniphy_pcie_probe(struct platform_device *pdev)
+{
+	struct qcom_uniphy_pcie *phy;
+	int ret;
+	struct phy *generic_phy;
+	struct phy_provider *phy_provider;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = of_node_get(dev->of_node);
+
+	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
+	if (!phy)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, phy);
+	phy->dev = &pdev->dev;
+
+	phy->data = of_device_get_match_data(dev);
+	if (!phy->data)
+		return -EINVAL;
+
+	ret = qcom_uniphy_pcie_get_resources(pdev, phy);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to get resources: %d\n", ret);
+		return ret;
+	}
+
+	ret = phy_pipe_clk_register(phy, np);
+	if (ret)
+		dev_err(&pdev->dev, "failed to register phy pipe clk\n");
+
+	generic_phy = devm_phy_create(phy->dev, NULL, &pcie_ops);
+	if (IS_ERR(generic_phy))
+		return PTR_ERR(generic_phy);
+
+	phy_set_drvdata(generic_phy, phy);
+	phy_provider = devm_of_phy_provider_register(phy->dev,
+						     of_phy_simple_xlate);
+	if (IS_ERR(phy_provider))
+		return PTR_ERR(phy_provider);
+
+	return 0;
+}
+
+static struct platform_driver qcom_uniphy_pcie_driver = {
+	.probe		= qcom_uniphy_pcie_probe,
+	.driver		= {
+		.name	= "qcom-uniphy-pcie",
+		.owner	= THIS_MODULE,
+		.of_match_table = qcom_uniphy_pcie_id_table,
+	},
+};
+
+module_platform_driver(qcom_uniphy_pcie_driver);
+
+MODULE_ALIAS("platform:qcom-uniphy-pcie");
+MODULE_LICENSE("Dual BSD/GPL");
+MODULE_DESCRIPTION("PCIE QCOM UNIPHY driver");
-- 
2.17.1

Re: [PATCH 3/6] phy: qcom: Introduce PCIe UNIPHY 28LP driver

[Dmitry Baryshkov]

On Tue, 3 Oct 2023 at 15:09, Nitheesh Sekar <quic_nsekar@quicinc.com> wrote:
>
> Add Qualcomm PCIe UNIPHY 28LP driver support present
> in Qualcomm IPQ5018 SoC and the phy init sequence.
>
> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
> ---
>  drivers/phy/qualcomm/Kconfig                  |  12 +
>  drivers/phy/qualcomm/Makefile                 |   1 +
>  .../phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c  | 336 ++++++++++++++++++
>  3 files changed, 349 insertions(+)
>  create mode 100644 drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c
>
> diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
> index d891058b7c39..b7d37cd98f02 100644
> --- a/drivers/phy/qualcomm/Kconfig
> +++ b/drivers/phy/qualcomm/Kconfig
> @@ -154,6 +154,18 @@ config PHY_QCOM_M31_USB
>           management. This driver is required even for peripheral only or
>           host only mode configurations.
>
> +config PHY_QCOM_UNIPHY_PCIE_28LP
> +       bool "PCIE UNIPHY 28LP PHY driver"
> +       depends on ARCH_QCOM
> +       depends on HAS_IOMEM
> +       depends on OF
> +       select GENERIC_PHY
> +       help
> +         Enable this to support the PCIe UNIPHY 28LP phy transceiver that
> +         is used with PCIe controllers on Qualcomm IPQ5018 chips. It
> +         handles PHY initialization, clock management required after
> +         resetting the hardware and power management.
> +
>  config PHY_QCOM_USB_HS
>         tristate "Qualcomm USB HS PHY module"
>         depends on USB_ULPI_BUS
> diff --git a/drivers/phy/qualcomm/Makefile b/drivers/phy/qualcomm/Makefile
> index ffd609ac6233..31105cd17bc9 100644
> --- a/drivers/phy/qualcomm/Makefile
> +++ b/drivers/phy/qualcomm/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_PHY_QCOM_QMP_USB_LEGACY) += phy-qcom-qmp-usb-legacy.o
>  obj-$(CONFIG_PHY_QCOM_QUSB2)           += phy-qcom-qusb2.o
>  obj-$(CONFIG_PHY_QCOM_SNPS_EUSB2)      += phy-qcom-snps-eusb2.o
>  obj-$(CONFIG_PHY_QCOM_EUSB2_REPEATER)  += phy-qcom-eusb2-repeater.o
> +obj-$(CONFIG_PHY_QCOM_UNIPHY_PCIE_28LP)        += phy-qcom-uniphy-pcie-28lp.o
>  obj-$(CONFIG_PHY_QCOM_USB_HS)          += phy-qcom-usb-hs.o
>  obj-$(CONFIG_PHY_QCOM_USB_HSIC)        += phy-qcom-usb-hsic.o
>  obj-$(CONFIG_PHY_QCOM_USB_HS_28NM)     += phy-qcom-usb-hs-28nm.o
> diff --git a/drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c b/drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c
> new file mode 100644
> index 000000000000..5ef6ae7276cf
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c
> @@ -0,0 +1,336 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2023, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
> +#include <linux/reset.h>
> +#include <linux/of_device.h>
> +#include <linux/delay.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +
> +#define PIPE_CLK_DELAY_MIN_US                  5000
> +#define PIPE_CLK_DELAY_MAX_US                  5100
> +#define CDR_CTRL_REG_1         0x80
> +#define CDR_CTRL_REG_2         0x84
> +#define CDR_CTRL_REG_3         0x88
> +#define CDR_CTRL_REG_4         0x8C
> +#define CDR_CTRL_REG_5         0x90
> +#define CDR_CTRL_REG_6         0x94
> +#define CDR_CTRL_REG_7         0x98
> +#define SSCG_CTRL_REG_1                0x9c
> +#define SSCG_CTRL_REG_2                0xa0
> +#define SSCG_CTRL_REG_3                0xa4
> +#define SSCG_CTRL_REG_4                0xa8
> +#define SSCG_CTRL_REG_5                0xac
> +#define SSCG_CTRL_REG_6                0xb0
> +#define PCS_INTERNAL_CONTROL_2 0x2d8
> +
> +#define PHY_MODE_FIXED         0x1
> +
> +enum qcom_uniphy_pcie_type {
> +       PHY_TYPE_PCIE = 1,
> +       PHY_TYPE_PCIE_GEN2,
> +       PHY_TYPE_PCIE_GEN3,
> +};
> +
> +struct uniphy_regs {
> +       unsigned int offset;
> +       unsigned int val;
> +};
> +
> +struct uniphy_pcie_data {

Please stick to a single symbol/struct prefix.

> +       int lanes;
> +       /* 2nd lane offset */
> +       int lane_offset;
> +       unsigned int phy_type;
> +       const struct uniphy_regs *init_seq;
> +       unsigned int init_seq_num;
> +};
> +
> +struct qcom_uniphy_pcie {
> +       struct phy phy;
> +       struct device *dev;
> +       const struct uniphy_pcie_data *data;
> +       struct clk_bulk_data *clks;
> +       int num_clks;
> +       struct reset_control *resets;
> +       void __iomem *base;
> +};
> +
> +#define        phy_to_dw_phy(x)        container_of((x), struct qca_uni_pcie_phy, phy)
> +
> +static const struct uniphy_regs ipq5018_regs[] = {
> +       {
> +               .offset = SSCG_CTRL_REG_4,
> +               .val = 0x1cb9,
> +       },
> +       {

"}, {", on the same line.

> +               .offset = SSCG_CTRL_REG_5,
> +               .val = 0x023a,
> +       },
> +       {
> +               .offset = SSCG_CTRL_REG_3,
> +               .val = 0xd360,
> +       },
> +       {
> +               .offset = SSCG_CTRL_REG_1,
> +               .val = 0x1,
> +       },
> +       {
> +               .offset = SSCG_CTRL_REG_2,
> +               .val = 0xeb,
> +       },
> +       {
> +               .offset = CDR_CTRL_REG_4,
> +               .val = 0x3f9,
> +       },
> +       {
> +               .offset = CDR_CTRL_REG_5,
> +               .val = 0x1c9,
> +       },
> +       {
> +               .offset = CDR_CTRL_REG_2,
> +               .val = 0x419,
> +       },
> +       {
> +               .offset = CDR_CTRL_REG_1,
> +               .val = 0x200,
> +       },
> +       {
> +               .offset = PCS_INTERNAL_CONTROL_2,
> +               .val = 0xf101,
> +       },
> +};
> +
> +static const struct uniphy_pcie_data ipq5018_2x2_data = {
> +       .lanes          = 2,
> +       .lane_offset    = 0x800,
> +       .phy_type       = PHY_TYPE_PCIE_GEN2,
> +       .init_seq       = ipq5018_regs,
> +       .init_seq_num   = ARRAY_SIZE(ipq5018_regs),
> +};
> +
> +static void qcom_uniphy_pcie_init(struct qcom_uniphy_pcie *phy)
> +{
> +       const struct uniphy_pcie_data *data = phy->data;
> +       const struct uniphy_regs *init_seq;
> +       void __iomem *base = phy->base;
> +       int lane = 0;
> +       int i;
> +
> +       while (lane != data->lanes) {

for (lane = 0; ...)

> +               init_seq = data->init_seq;
> +
> +               for (i = 0; i < data->init_seq_num; i++, init_seq++)
> +                       writel(init_seq->val, base + init_seq->offset);
> +
> +               if (data->lanes == 2)
> +                       base = base + data->lane_offset;

Drop the if(). Use +=.

> +
> +               lane++;
> +       }
> +}
> +
> +static int qcom_uniphy_pcie_power_off(struct phy *x)
> +{
> +       struct qcom_uniphy_pcie *phy = phy_get_drvdata(x);
> +
> +       reset_control_assert(phy->resets);
> +
> +       clk_bulk_disable_unprepare(phy->num_clks, phy->clks);

Judging from power_on(), the order should be opposite.

> +
> +       return 0;
> +}
> +
> +static int qcom_uniphy_pcie_power_on(struct phy *x)
> +{
> +       int ret;
> +       struct qcom_uniphy_pcie *phy = phy_get_drvdata(x);
> +
> +       ret = reset_control_assert(phy->resets);
> +       if (ret) {
> +               dev_err(phy->dev, "reset assert failed (%d)\n", ret);
> +               return ret;
> +       }
> +
> +       /*
> +        * Delay periods before and after reset deassert are working values
> +        * from downstream Codeaurora kernel
> +        */
> +       usleep_range(100, 150);
> +
> +       ret = reset_control_deassert(phy->resets);
> +       if (ret) {
> +               dev_err(phy->dev, "reset deassert failed (%d)\n", ret);
> +               return ret;
> +       }
> +
> +       usleep_range(PIPE_CLK_DELAY_MIN_US, PIPE_CLK_DELAY_MAX_US);
> +
> +       ret = clk_bulk_prepare_enable(phy->num_clks, phy->clks);
> +       if (ret) {
> +               dev_err(phy->dev, "clk prepare and enable failed %d\n", ret);
> +               return ret;

Shouldn't the reset be asserted again?

> +       }
> +
> +       usleep_range(30, 50);

In the same function you have one usleep_range with the defined
constants and another one using plain numbers. Could you please stick
to the uniform scheme?

> +
> +       qcom_uniphy_pcie_init(phy);
> +       return 0;
> +}
> +
> +static int qcom_uniphy_pcie_get_resources(struct platform_device *pdev,
> +                                         struct qcom_uniphy_pcie *phy)

inline it

> +{
> +       struct resource *res;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       phy->base = devm_ioremap_resource(phy->dev, res);

devm_platform_get_and_ioremap_resource()

> +       if (IS_ERR(phy->base)) {
> +               dev_err(phy->dev, "cannot get phy registers\n");
> +               return PTR_ERR(phy->base);
> +       }
> +
> +       phy->num_clks = devm_clk_bulk_get_all(phy->dev, &phy->clks);
> +       if (phy->num_clks < 0)
> +               return phy->num_clks;
> +
> +       phy->resets = devm_reset_control_array_get_exclusive(phy->dev);
> +       if (IS_ERR(phy->resets))
> +               return PTR_ERR(phy->resets);
> +
> +       return 0;
> +}
> +
> +/*
> + * Register a fixed rate pipe clock.
> + *
> + * The <s>_pipe_clksrc generated by PHY goes to the GCC that gate
> + * controls it. The <s>_pipe_clk coming out of the GCC is requested
> + * by the PHY driver for its operations.
> + * We register the <s>_pipe_clksrc here. The gcc driver takes care
> + * of assigning this <s>_pipe_clksrc as parent to <s>_pipe_clk.
> + * Below picture shows this relationship.
> + *
> + *         +---------------+
> + *         |   PHY block   |<<---------------------------------------+
> + *         |               |                                         |
> + *         |   +-------+   |                   +-----+               |
> + *   I/P---^-->|  PLL  |---^--->pipe_clksrc--->| GCC |--->pipe_clk---+
> + *    clk  |   +-------+   |                   +-----+
> + *         +---------------+
> + */
> +static int phy_pipe_clk_register(struct qcom_uniphy_pcie  *phy,
> +                                struct device_node *np)
> +{
> +       struct clk_fixed_rate *fixed;
> +       struct clk_init_data init = { };
> +       int ret;
> +
> +       ret = of_property_read_string(np, "clock-output-names", &init.name);
> +       if (ret) {
> +               dev_err(phy->dev, "%pOFn: No clock-output-names\n", np);
> +               return ret;
> +       }

Can we drop clock-output-names please.

> +
> +       fixed = devm_kzalloc(phy->dev, sizeof(*fixed), GFP_KERNEL);
> +       if (!fixed)
> +               return -ENOMEM;
> +
> +       init.ops = &clk_fixed_rate_ops;
> +       fixed->fixed_rate = 125000000;
> +       fixed->hw.init = &init;
> +
> +       ret = devm_clk_hw_register(phy->dev, &fixed->hw);
> +       if (ret)
> +               return ret;

devm_clk_hw_register_fixed_rate().  Then the whole function can be inlined.

> +
> +       ret = devm_of_clk_add_hw_provider(phy->dev, of_clk_hw_simple_get,
> +                                         &fixed->hw);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id qcom_uniphy_pcie_id_table[] = {
> +       {
> +               .compatible = "qcom,ipq5018-uniphy-pcie-gen2x2",

Bindings describe both 2x1 and 2x2 PHYs, but the driver supports only
2x2. Either of that should be fixed.


> +               .data = &ipq5018_2x2_data,
> +       },
> +       { /* Sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_uniphy_pcie_id_table);
> +
> +static const struct phy_ops pcie_ops = {
> +       .power_on       = qcom_uniphy_pcie_power_on,
> +       .power_off      = qcom_uniphy_pcie_power_off,
> +       .owner          = THIS_MODULE,
> +};
> +
> +static int qcom_uniphy_pcie_probe(struct platform_device *pdev)
> +{
> +       struct qcom_uniphy_pcie *phy;
> +       int ret;
> +       struct phy *generic_phy;
> +       struct phy_provider *phy_provider;
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = of_node_get(dev->of_node);
> +
> +       phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
> +       if (!phy)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, phy);
> +       phy->dev = &pdev->dev;
> +
> +       phy->data = of_device_get_match_data(dev);
> +       if (!phy->data)
> +               return -EINVAL;
> +
> +       ret = qcom_uniphy_pcie_get_resources(pdev, phy);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "failed to get resources: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = phy_pipe_clk_register(phy, np);
> +       if (ret)
> +               dev_err(&pdev->dev, "failed to register phy pipe clk\n");
> +
> +       generic_phy = devm_phy_create(phy->dev, NULL, &pcie_ops);
> +       if (IS_ERR(generic_phy))
> +               return PTR_ERR(generic_phy);
> +
> +       phy_set_drvdata(generic_phy, phy);
> +       phy_provider = devm_of_phy_provider_register(phy->dev,
> +                                                    of_phy_simple_xlate);
> +       if (IS_ERR(phy_provider))
> +               return PTR_ERR(phy_provider);
> +
> +       return 0;
> +}
> +
> +static struct platform_driver qcom_uniphy_pcie_driver = {
> +       .probe          = qcom_uniphy_pcie_probe,
> +       .driver         = {
> +               .name   = "qcom-uniphy-pcie",
> +               .owner  = THIS_MODULE,
> +               .of_match_table = qcom_uniphy_pcie_id_table,
> +       },
> +};
> +
> +module_platform_driver(qcom_uniphy_pcie_driver);
> +
> +MODULE_ALIAS("platform:qcom-uniphy-pcie");
> +MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_DESCRIPTION("PCIE QCOM UNIPHY driver");
> --
> 2.17.1
>


--
With best wishes
Dmitry

Re: [PATCH 3/6] phy: qcom: Introduce PCIe UNIPHY 28LP driver

[Krzysztof Kozlowski]

On 03/10/2023 14:08, Nitheesh Sekar wrote:
> Add Qualcomm PCIe UNIPHY 28LP driver support present
> in Qualcomm IPQ5018 SoC and the phy init sequence.
> 
> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
> ---

...

> +static int qcom_uniphy_pcie_probe(struct platform_device *pdev)
> +{
> +	struct qcom_uniphy_pcie *phy;
> +	int ret;
> +	struct phy *generic_phy;
> +	struct phy_provider *phy_provider;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = of_node_get(dev->of_node);
> +
> +	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
> +	if (!phy)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, phy);
> +	phy->dev = &pdev->dev;
> +
> +	phy->data = of_device_get_match_data(dev);
> +	if (!phy->data)
> +		return -EINVAL;
> +
> +	ret = qcom_uniphy_pcie_get_resources(pdev, phy);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to get resources: %d\n", ret);
> +		return ret;

Syntax is:
return dev_err_probe()


> +	}
> +
> +	ret = phy_pipe_clk_register(phy, np);
> +	if (ret)
> +		dev_err(&pdev->dev, "failed to register phy pipe clk\n");
> +
> +	generic_phy = devm_phy_create(phy->dev, NULL, &pcie_ops);
> +	if (IS_ERR(generic_phy))
> +		return PTR_ERR(generic_phy);
> +
> +	phy_set_drvdata(generic_phy, phy);
> +	phy_provider = devm_of_phy_provider_register(phy->dev,
> +						     of_phy_simple_xlate);
> +	if (IS_ERR(phy_provider))
> +		return PTR_ERR(phy_provider);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver qcom_uniphy_pcie_driver = {
> +	.probe		= qcom_uniphy_pcie_probe,
> +	.driver		= {
> +		.name	= "qcom-uniphy-pcie",
> +		.owner	= THIS_MODULE,

Run coccinelle/coccicheck.

> +		.of_match_table = qcom_uniphy_pcie_id_table,
> +	},
> +};
> +
> +module_platform_driver(qcom_uniphy_pcie_driver);
> +
> +MODULE_ALIAS("platform:qcom-uniphy-pcie");

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong.


Best regards,
Krzysztof

Re: [PATCH 3/6] phy: qcom: Introduce PCIe UNIPHY 28LP driver

[Nitheesh Sekar]

On 10/4/2023 1:43 PM, Krzysztof Kozlowski wrote:
> On 03/10/2023 14:08, Nitheesh Sekar wrote:
>> Add Qualcomm PCIe UNIPHY 28LP driver support present
>> in Qualcomm IPQ5018 SoC and the phy init sequence.
>>
>> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
>> ---
> ...
>
>> +static int qcom_uniphy_pcie_probe(struct platform_device *pdev)
>> +{
>> +	struct qcom_uniphy_pcie *phy;
>> +	int ret;
>> +	struct phy *generic_phy;
>> +	struct phy_provider *phy_provider;
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = of_node_get(dev->of_node);
>> +
>> +	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
>> +	if (!phy)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, phy);
>> +	phy->dev = &pdev->dev;
>> +
>> +	phy->data = of_device_get_match_data(dev);
>> +	if (!phy->data)
>> +		return -EINVAL;
>> +
>> +	ret = qcom_uniphy_pcie_get_resources(pdev, phy);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to get resources: %d\n", ret);
>> +		return ret;
> Syntax is:
> return dev_err_probe()
Sure. will update it and take care of this in future.
>
>
>> +	}
>> +
>> +	ret = phy_pipe_clk_register(phy, np);
>> +	if (ret)
>> +		dev_err(&pdev->dev, "failed to register phy pipe clk\n");
>> +
>> +	generic_phy = devm_phy_create(phy->dev, NULL, &pcie_ops);
>> +	if (IS_ERR(generic_phy))
>> +		return PTR_ERR(generic_phy);
>> +
>> +	phy_set_drvdata(generic_phy, phy);
>> +	phy_provider = devm_of_phy_provider_register(phy->dev,
>> +						     of_phy_simple_xlate);
>> +	if (IS_ERR(phy_provider))
>> +		return PTR_ERR(phy_provider);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver qcom_uniphy_pcie_driver = {
>> +	.probe		= qcom_uniphy_pcie_probe,
>> +	.driver		= {
>> +		.name	= "qcom-uniphy-pcie",
>> +		.owner	= THIS_MODULE,
> Run coccinelle/coccicheck.
Sure. Will do it.
>
>> +		.of_match_table = qcom_uniphy_pcie_id_table,
>> +	},
>> +};
>> +
>> +module_platform_driver(qcom_uniphy_pcie_driver);
>> +
>> +MODULE_ALIAS("platform:qcom-uniphy-pcie");
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong.
Ok. Will remove it.

Thanks,
Nitheesh

Re: [PATCH 3/6] phy: qcom: Introduce PCIe UNIPHY 28LP driver

[Robert Marko]

On 03. 10. 2023. 14:08, Nitheesh Sekar wrote:
> Add Qualcomm PCIe UNIPHY 28LP driver support present
> in Qualcomm IPQ5018 SoC and the phy init sequence.
>
> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
> ---
>   drivers/phy/qualcomm/Kconfig                  |  12 +
>   drivers/phy/qualcomm/Makefile                 |   1 +
>   .../phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c  | 336 ++++++++++++++++++
>   3 files changed, 349 insertions(+)
>   create mode 100644 drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c
>
> diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
> index d891058b7c39..b7d37cd98f02 100644
> --- a/drivers/phy/qualcomm/Kconfig
> +++ b/drivers/phy/qualcomm/Kconfig
> @@ -154,6 +154,18 @@ config PHY_QCOM_M31_USB
>   	  management. This driver is required even for peripheral only or
>   	  host only mode configurations.
>   
> +config PHY_QCOM_UNIPHY_PCIE_28LP
> +	bool "PCIE UNIPHY 28LP PHY driver"
> +	depends on ARCH_QCOM
> +	depends on HAS_IOMEM
> +	depends on OF
> +	select GENERIC_PHY
> +	help
> +	  Enable this to support the PCIe UNIPHY 28LP phy transceiver that
> +	  is used with PCIe controllers on Qualcomm IPQ5018 chips. It
> +	  handles PHY initialization, clock management required after
> +	  resetting the hardware and power management.
> +
>   config PHY_QCOM_USB_HS
>   	tristate "Qualcomm USB HS PHY module"
>   	depends on USB_ULPI_BUS
> diff --git a/drivers/phy/qualcomm/Makefile b/drivers/phy/qualcomm/Makefile
> index ffd609ac6233..31105cd17bc9 100644
> --- a/drivers/phy/qualcomm/Makefile
> +++ b/drivers/phy/qualcomm/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_PHY_QCOM_QMP_USB_LEGACY)	+= phy-qcom-qmp-usb-legacy.o
>   obj-$(CONFIG_PHY_QCOM_QUSB2)		+= phy-qcom-qusb2.o
>   obj-$(CONFIG_PHY_QCOM_SNPS_EUSB2)	+= phy-qcom-snps-eusb2.o
>   obj-$(CONFIG_PHY_QCOM_EUSB2_REPEATER)	+= phy-qcom-eusb2-repeater.o
> +obj-$(CONFIG_PHY_QCOM_UNIPHY_PCIE_28LP)	+= phy-qcom-uniphy-pcie-28lp.o
>   obj-$(CONFIG_PHY_QCOM_USB_HS) 		+= phy-qcom-usb-hs.o
>   obj-$(CONFIG_PHY_QCOM_USB_HSIC) 	+= phy-qcom-usb-hsic.o
>   obj-$(CONFIG_PHY_QCOM_USB_HS_28NM)	+= phy-qcom-usb-hs-28nm.o
> diff --git a/drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c b/drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c
> new file mode 100644
> index 000000000000..5ef6ae7276cf
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c
> @@ -0,0 +1,336 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2023, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
> +#include <linux/reset.h>
> +#include <linux/of_device.h>
> +#include <linux/delay.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +
> +#define PIPE_CLK_DELAY_MIN_US			5000
> +#define PIPE_CLK_DELAY_MAX_US			5100
> +#define CDR_CTRL_REG_1		0x80
> +#define CDR_CTRL_REG_2		0x84
> +#define CDR_CTRL_REG_3		0x88
> +#define CDR_CTRL_REG_4		0x8C
> +#define CDR_CTRL_REG_5		0x90
> +#define CDR_CTRL_REG_6		0x94
> +#define CDR_CTRL_REG_7		0x98
> +#define SSCG_CTRL_REG_1		0x9c
> +#define SSCG_CTRL_REG_2		0xa0
> +#define SSCG_CTRL_REG_3		0xa4
> +#define SSCG_CTRL_REG_4		0xa8
> +#define SSCG_CTRL_REG_5		0xac
> +#define SSCG_CTRL_REG_6		0xb0
> +#define PCS_INTERNAL_CONTROL_2	0x2d8
> +
> +#define PHY_MODE_FIXED		0x1
> +
> +enum qcom_uniphy_pcie_type {
> +	PHY_TYPE_PCIE = 1,
> +	PHY_TYPE_PCIE_GEN2,
> +	PHY_TYPE_PCIE_GEN3,
> +};
> +
> +struct uniphy_regs {
> +	unsigned int offset;
> +	unsigned int val;
> +};
> +
> +struct uniphy_pcie_data {
> +	int lanes;
> +	/* 2nd lane offset */
> +	int lane_offset;
> +	unsigned int phy_type;
> +	const struct uniphy_regs *init_seq;
> +	unsigned int init_seq_num;
> +};
> +
> +struct qcom_uniphy_pcie {
> +	struct phy phy;
> +	struct device *dev;
> +	const struct uniphy_pcie_data *data;
> +	struct clk_bulk_data *clks;
> +	int num_clks;
> +	struct reset_control *resets;
> +	void __iomem *base;
> +};
> +
> +#define	phy_to_dw_phy(x)	container_of((x), struct qca_uni_pcie_phy, phy)
> +
> +static const struct uniphy_regs ipq5018_regs[] = {
> +	{
> +		.offset = SSCG_CTRL_REG_4,
> +		.val = 0x1cb9,
> +	},
> +	{
> +		.offset = SSCG_CTRL_REG_5,
> +		.val = 0x023a,
> +	},
> +	{
> +		.offset = SSCG_CTRL_REG_3,
> +		.val = 0xd360,
> +	},
> +	{
> +		.offset = SSCG_CTRL_REG_1,
> +		.val = 0x1,
> +	},
> +	{
> +		.offset = SSCG_CTRL_REG_2,
> +		.val = 0xeb,
> +	},
> +	{
> +		.offset = CDR_CTRL_REG_4,
> +		.val = 0x3f9,
> +	},
> +	{
> +		.offset = CDR_CTRL_REG_5,
> +		.val = 0x1c9,
> +	},
> +	{
> +		.offset = CDR_CTRL_REG_2,
> +		.val = 0x419,
> +	},
> +	{
> +		.offset = CDR_CTRL_REG_1,
> +		.val = 0x200,
> +	},
> +	{
> +		.offset = PCS_INTERNAL_CONTROL_2,
> +		.val = 0xf101,
> +	},
> +};
> +
> +static const struct uniphy_pcie_data ipq5018_2x2_data = {
> +	.lanes		= 2,
> +	.lane_offset	= 0x800,
> +	.phy_type	= PHY_TYPE_PCIE_GEN2,
> +	.init_seq	= ipq5018_regs,
> +	.init_seq_num	= ARRAY_SIZE(ipq5018_regs),
> +};
> +
> +static void qcom_uniphy_pcie_init(struct qcom_uniphy_pcie *phy)
> +{
> +	const struct uniphy_pcie_data *data = phy->data;
> +	const struct uniphy_regs *init_seq;
> +	void __iomem *base = phy->base;
> +	int lane = 0;
> +	int i;
> +
> +	while (lane != data->lanes) {
> +		init_seq = data->init_seq;
> +
> +		for (i = 0; i < data->init_seq_num; i++, init_seq++)
> +			writel(init_seq->val, base + init_seq->offset);
> +
> +		if (data->lanes == 2)
> +			base = base + data->lane_offset;
> +
> +		lane++;
> +	}
> +}
> +
> +static int qcom_uniphy_pcie_power_off(struct phy *x)
> +{
> +	struct qcom_uniphy_pcie *phy = phy_get_drvdata(x);
> +
> +	reset_control_assert(phy->resets);
> +
> +	clk_bulk_disable_unprepare(phy->num_clks, phy->clks);
> +
> +	return 0;
> +}
> +
> +static int qcom_uniphy_pcie_power_on(struct phy *x)
> +{
> +	int ret;
> +	struct qcom_uniphy_pcie *phy = phy_get_drvdata(x);
> +
> +	ret = reset_control_assert(phy->resets);
> +	if (ret) {
> +		dev_err(phy->dev, "reset assert failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Delay periods before and after reset deassert are working values
> +	 * from downstream Codeaurora kernel
> +	 */
> +	usleep_range(100, 150);
> +
> +	ret = reset_control_deassert(phy->resets);
> +	if (ret) {
> +		dev_err(phy->dev, "reset deassert failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	usleep_range(PIPE_CLK_DELAY_MIN_US, PIPE_CLK_DELAY_MAX_US);
> +
> +	ret = clk_bulk_prepare_enable(phy->num_clks, phy->clks);
> +	if (ret) {
> +		dev_err(phy->dev, "clk prepare and enable failed %d\n", ret);
> +		return ret;
> +	}
> +
> +	usleep_range(30, 50);
> +
> +	qcom_uniphy_pcie_init(phy);
> +	return 0;
> +}
> +
> +static int qcom_uniphy_pcie_get_resources(struct platform_device *pdev,
> +					  struct qcom_uniphy_pcie *phy)
> +{
> +	struct resource *res;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	phy->base = devm_ioremap_resource(phy->dev, res);
> +	if (IS_ERR(phy->base)) {
> +		dev_err(phy->dev, "cannot get phy registers\n");
> +		return PTR_ERR(phy->base);
> +	}
> +
> +	phy->num_clks = devm_clk_bulk_get_all(phy->dev, &phy->clks);
> +	if (phy->num_clks < 0)
> +		return phy->num_clks;
> +
> +	phy->resets = devm_reset_control_array_get_exclusive(phy->dev);
> +	if (IS_ERR(phy->resets))
> +		return PTR_ERR(phy->resets);
> +
> +	return 0;
> +}
> +
> +/*
> + * Register a fixed rate pipe clock.
> + *
> + * The <s>_pipe_clksrc generated by PHY goes to the GCC that gate
> + * controls it. The <s>_pipe_clk coming out of the GCC is requested
> + * by the PHY driver for its operations.
> + * We register the <s>_pipe_clksrc here. The gcc driver takes care
> + * of assigning this <s>_pipe_clksrc as parent to <s>_pipe_clk.
> + * Below picture shows this relationship.
> + *
> + *         +---------------+
> + *         |   PHY block   |<<---------------------------------------+
> + *         |               |                                         |
> + *         |   +-------+   |                   +-----+               |
> + *   I/P---^-->|  PLL  |---^--->pipe_clksrc--->| GCC |--->pipe_clk---+
> + *    clk  |   +-------+   |                   +-----+
> + *         +---------------+
> + */
> +static int phy_pipe_clk_register(struct qcom_uniphy_pcie  *phy,
> +				 struct device_node *np)
> +{
> +	struct clk_fixed_rate *fixed;
> +	struct clk_init_data init = { };
> +	int ret;
> +
> +	ret = of_property_read_string(np, "clock-output-names", &init.name);
> +	if (ret) {
> +		dev_err(phy->dev, "%pOFn: No clock-output-names\n", np);
> +		return ret;
> +	}
> +
> +	fixed = devm_kzalloc(phy->dev, sizeof(*fixed), GFP_KERNEL);
> +	if (!fixed)
> +		return -ENOMEM;
> +
> +	init.ops = &clk_fixed_rate_ops;
> +	fixed->fixed_rate = 125000000;
> +	fixed->hw.init = &init;
> +
> +	ret = devm_clk_hw_register(phy->dev, &fixed->hw);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_of_clk_add_hw_provider(phy->dev, of_clk_hw_simple_get,
> +					  &fixed->hw);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id qcom_uniphy_pcie_id_table[] = {
> +	{
> +		.compatible = "qcom,ipq5018-uniphy-pcie-gen2x2",
> +		.data = &ipq5018_2x2_data,

Hi, did you also forget the single lane PHY?
As you have the "qcom,ipq5018-uniphy-pcie-gen2x1" compatible used later 
in the DTS
but its not implemented in the driver at all.

Regards,
Robert

> +	},
> +	{ /* Sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_uniphy_pcie_id_table);
> +
> +static const struct phy_ops pcie_ops = {
> +	.power_on	= qcom_uniphy_pcie_power_on,
> +	.power_off	= qcom_uniphy_pcie_power_off,
> +	.owner          = THIS_MODULE,
> +};
> +
> +static int qcom_uniphy_pcie_probe(struct platform_device *pdev)
> +{
> +	struct qcom_uniphy_pcie *phy;
> +	int ret;
> +	struct phy *generic_phy;
> +	struct phy_provider *phy_provider;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = of_node_get(dev->of_node);
> +
> +	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
> +	if (!phy)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, phy);
> +	phy->dev = &pdev->dev;
> +
> +	phy->data = of_device_get_match_data(dev);
> +	if (!phy->data)
> +		return -EINVAL;
> +
> +	ret = qcom_uniphy_pcie_get_resources(pdev, phy);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to get resources: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = phy_pipe_clk_register(phy, np);
> +	if (ret)
> +		dev_err(&pdev->dev, "failed to register phy pipe clk\n");
> +
> +	generic_phy = devm_phy_create(phy->dev, NULL, &pcie_ops);
> +	if (IS_ERR(generic_phy))
> +		return PTR_ERR(generic_phy);
> +
> +	phy_set_drvdata(generic_phy, phy);
> +	phy_provider = devm_of_phy_provider_register(phy->dev,
> +						     of_phy_simple_xlate);
> +	if (IS_ERR(phy_provider))
> +		return PTR_ERR(phy_provider);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver qcom_uniphy_pcie_driver = {
> +	.probe		= qcom_uniphy_pcie_probe,
> +	.driver		= {
> +		.name	= "qcom-uniphy-pcie",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = qcom_uniphy_pcie_id_table,
> +	},
> +};
> +
> +module_platform_driver(qcom_uniphy_pcie_driver);
> +
> +MODULE_ALIAS("platform:qcom-uniphy-pcie");
> +MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_DESCRIPTION("PCIE QCOM UNIPHY driver");

[PATCH 4/6] PCI: qcom: Add support for IPQ5018

[Nitheesh Sekar]

Added a new compatible 'qcom,pcie-ipq5018' and modified
get_resources of 'ops 2_9_0' to get the clocks from the
device-tree.

Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
Co-developed-by: Devi Priya <quic_devipriy@quicinc.com>
Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index e2f29404c84e..bb0717190920 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -197,10 +197,10 @@ struct qcom_pcie_resources_2_7_0 {
 	struct reset_control *rst;
 };
 
-#define QCOM_PCIE_2_9_0_MAX_CLOCKS		5
 struct qcom_pcie_resources_2_9_0 {
-	struct clk_bulk_data clks[QCOM_PCIE_2_9_0_MAX_CLOCKS];
+	struct clk_bulk_data *clks;
 	struct reset_control *rst;
+	int num_clks;
 };
 
 union qcom_pcie_resources {
@@ -1048,17 +1048,10 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
-	int ret;
 
-	res->clks[0].id = "iface";
-	res->clks[1].id = "axi_m";
-	res->clks[2].id = "axi_s";
-	res->clks[3].id = "axi_bridge";
-	res->clks[4].id = "rchng";
-
-	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
-	if (ret < 0)
-		return ret;
+	res->num_clks = devm_clk_bulk_get_all(dev, &res->clks);
+	if (res->num_clks < 0)
+		return res->num_clks;
 
 	res->rst = devm_reset_control_array_get_exclusive(dev);
 	if (IS_ERR(res->rst))
@@ -1071,7 +1064,7 @@ static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
 
-	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
+	clk_bulk_disable_unprepare(res->num_clks, res->clks);
 }
 
 static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
@@ -1100,7 +1093,7 @@ static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
 
 	usleep_range(2000, 2500);
 
-	return clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
+	return clk_bulk_prepare_enable(res->num_clks, res->clks);
 }
 
 static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
@@ -1605,6 +1598,7 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
 	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
 	{ .compatible = "qcom,pcie-ipq4019", .data = &cfg_2_4_0 },
+	{ .compatible = "qcom,pcie-ipq5018", .data = &cfg_2_9_0 },
 	{ .compatible = "qcom,pcie-ipq6018", .data = &cfg_2_9_0 },
 	{ .compatible = "qcom,pcie-ipq8064", .data = &cfg_2_1_0 },
 	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &cfg_2_1_0 },
-- 
2.17.1

Re: [PATCH 4/6] PCI: qcom: Add support for IPQ5018

[Dmitry Baryshkov]

On Tue, 3 Oct 2023 at 15:10, Nitheesh Sekar <quic_nsekar@quicinc.com> wrote:
>
> Added a new compatible 'qcom,pcie-ipq5018' and modified
> get_resources of 'ops 2_9_0' to get the clocks from the
> device-tree.
>
> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
> Co-developed-by: Devi Priya <quic_devipriy@quicinc.com>
> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index e2f29404c84e..bb0717190920 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -197,10 +197,10 @@ struct qcom_pcie_resources_2_7_0 {
>         struct reset_control *rst;
>  };
>
> -#define QCOM_PCIE_2_9_0_MAX_CLOCKS             5
>  struct qcom_pcie_resources_2_9_0 {
> -       struct clk_bulk_data clks[QCOM_PCIE_2_9_0_MAX_CLOCKS];
> +       struct clk_bulk_data *clks;
>         struct reset_control *rst;
> +       int num_clks;
>  };
>
>  union qcom_pcie_resources {
> @@ -1048,17 +1048,10 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
>         struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>         struct dw_pcie *pci = pcie->pci;
>         struct device *dev = pci->dev;
> -       int ret;
>
> -       res->clks[0].id = "iface";
> -       res->clks[1].id = "axi_m";
> -       res->clks[2].id = "axi_s";
> -       res->clks[3].id = "axi_bridge";
> -       res->clks[4].id = "rchng";
> -
> -       ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);

Changing this to devm_clk_bulk_get_optional would be easier and will
follow the design of the driver.

> -       if (ret < 0)
> -               return ret;
> +       res->num_clks = devm_clk_bulk_get_all(dev, &res->clks);
> +       if (res->num_clks < 0)
> +               return res->num_clks;
>
>         res->rst = devm_reset_control_array_get_exclusive(dev);
>         if (IS_ERR(res->rst))
> @@ -1071,7 +1064,7 @@ static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
>  {
>         struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>
> -       clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> +       clk_bulk_disable_unprepare(res->num_clks, res->clks);
>  }
>
>  static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> @@ -1100,7 +1093,7 @@ static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
>
>         usleep_range(2000, 2500);
>
> -       return clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> +       return clk_bulk_prepare_enable(res->num_clks, res->clks);
>  }
>
>  static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> @@ -1605,6 +1598,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>         { .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
>         { .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
>         { .compatible = "qcom,pcie-ipq4019", .data = &cfg_2_4_0 },
> +       { .compatible = "qcom,pcie-ipq5018", .data = &cfg_2_9_0 },
>         { .compatible = "qcom,pcie-ipq6018", .data = &cfg_2_9_0 },
>         { .compatible = "qcom,pcie-ipq8064", .data = &cfg_2_1_0 },
>         { .compatible = "qcom,pcie-ipq8064-v2", .data = &cfg_2_1_0 },
> --
> 2.17.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 4/6] PCI: qcom: Add support for IPQ5018

[Manivannan Sadhasivam]

On Tue, Oct 03, 2023 at 05:38:44PM +0530, Nitheesh Sekar wrote:
> Added a new compatible 'qcom,pcie-ipq5018' and modified
> get_resources of 'ops 2_9_0' to get the clocks from the
> device-tree.
> 

As per Documentation/process/submitting-patches.rst:

Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour.

Also, please elaborate your change in a detailed manner. For instance, saying
that you modified "get_resources of 'ops 2_9_0' to get the clocks from the
devicetree" is not sufficient since all clocks are being parsed based on the
devicetree info only.

- Mani

> Co-developed-by: Anusha Rao <quic_anusha@quicinc.com>
> Signed-off-by: Anusha Rao <quic_anusha@quicinc.com>
> Co-developed-by: Devi Priya <quic_devipriy@quicinc.com>
> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index e2f29404c84e..bb0717190920 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -197,10 +197,10 @@ struct qcom_pcie_resources_2_7_0 {
>  	struct reset_control *rst;
>  };
>  
> -#define QCOM_PCIE_2_9_0_MAX_CLOCKS		5
>  struct qcom_pcie_resources_2_9_0 {
> -	struct clk_bulk_data clks[QCOM_PCIE_2_9_0_MAX_CLOCKS];
> +	struct clk_bulk_data *clks;
>  	struct reset_control *rst;
> +	int num_clks;
>  };
>  
>  union qcom_pcie_resources {
> @@ -1048,17 +1048,10 @@ static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>  	struct dw_pcie *pci = pcie->pci;
>  	struct device *dev = pci->dev;
> -	int ret;
>  
> -	res->clks[0].id = "iface";
> -	res->clks[1].id = "axi_m";
> -	res->clks[2].id = "axi_s";
> -	res->clks[3].id = "axi_bridge";
> -	res->clks[4].id = "rchng";
> -
> -	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> -	if (ret < 0)
> -		return ret;
> +	res->num_clks = devm_clk_bulk_get_all(dev, &res->clks);
> +	if (res->num_clks < 0)
> +		return res->num_clks;
>  
>  	res->rst = devm_reset_control_array_get_exclusive(dev);
>  	if (IS_ERR(res->rst))
> @@ -1071,7 +1064,7 @@ static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>  
> -	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
>  }
>  
>  static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> @@ -1100,7 +1093,7 @@ static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
>  
>  	usleep_range(2000, 2500);
>  
> -	return clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> +	return clk_bulk_prepare_enable(res->num_clks, res->clks);
>  }
>  
>  static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> @@ -1605,6 +1598,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
>  	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
>  	{ .compatible = "qcom,pcie-ipq4019", .data = &cfg_2_4_0 },
> +	{ .compatible = "qcom,pcie-ipq5018", .data = &cfg_2_9_0 },
>  	{ .compatible = "qcom,pcie-ipq6018", .data = &cfg_2_9_0 },
>  	{ .compatible = "qcom,pcie-ipq8064", .data = &cfg_2_1_0 },
>  	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &cfg_2_1_0 },
> -- 
> 2.17.1
> 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 5/6] arm64: dts: qcom: ipq5018: Add PCIe related nodes

[Nitheesh Sekar]

Add phy and controller nodes for PCIe_x2 and PCIe_x1.
PCIe_x2 is 2-lane Gen2 and PCIe_x1 is 1-lane Gen2.

Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq5018.dtsi | 186 +++++++++++++++++++++++++-
 1 file changed, 184 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
index 38ffdc3cbdcd..0818fdd1e693 100644
--- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
@@ -8,6 +8,7 @@
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/clock/qcom,gcc-ipq5018.h>
 #include <dt-bindings/reset/qcom,gcc-ipq5018.h>
+#include <dt-bindings/gpio/gpio.h>
 
 / {
 	interrupt-parent = <&intc>;
@@ -94,6 +95,38 @@
 		#size-cells = <1>;
 		ranges = <0 0 0 0xffffffff>;
 
+		pcie_x1phy: phy@7e000{
+			compatible = "qcom,ipq5018-uniphy-pcie-gen2x1";
+			reg = <0x0007e000 0x800>;
+			#phy-cells = <0>;
+			#clock-cells = <0>;
+			clocks = <&gcc GCC_PCIE1_PIPE_CLK>;
+			clock-names = "pipe_clk";
+			clock-output-names = "pcie1_pipe_clk";
+			assigned-clocks = <&gcc GCC_PCIE1_PIPE_CLK>;
+			assigned-clock-rates = <125000000>;
+			resets = <&gcc GCC_PCIE1_PHY_BCR>,
+				 <&gcc GCC_PCIE1PHY_PHY_BCR>;
+			reset-names = "phy", "phy_phy";
+			status = "disabled";
+		};
+
+		pcie_x2phy: phy@86000{
+			compatible = "qcom,ipq5018-uniphy-pcie-gen2x2";
+			reg = <0x00086000 0x800>;
+			#phy-cells = <0>;
+			#clock-cells = <0>;
+			clocks = <&gcc GCC_PCIE0_PIPE_CLK>;
+			clock-names = "pipe_clk";
+			clock-output-names = "pcie0_pipe_clk";
+			assigned-clocks = <&gcc GCC_PCIE0_PIPE_CLK>;
+			assigned-clock-rates = <125000000>;
+			resets = <&gcc GCC_PCIE0_PHY_BCR>,
+				 <&gcc GCC_PCIE0PHY_PHY_BCR>;
+			reset-names = "phy", "phy_phy";
+			status = "disabled";
+		};
+
 		tlmm: pinctrl@1000000 {
 			compatible = "qcom,ipq5018-tlmm";
 			reg = <0x01000000 0x300000>;
@@ -117,8 +150,8 @@
 			reg = <0x01800000 0x80000>;
 			clocks = <&xo_board_clk>,
 				 <&sleep_clk>,
-				 <0>,
-				 <0>,
+				 <&pcie_x2phy>,
+				 <&pcie_x1phy>,
 				 <0>,
 				 <0>,
 				 <0>,
@@ -246,6 +279,155 @@
 				status = "disabled";
 			};
 		};
+
+		pcie_x1: pci@80000000 {
+			compatible = "qcom,pcie-ipq5018";
+			reg =  <0x80000000 0xf1d
+				0x80000F20 0xa8
+				0x80001000 0x1000
+				0x78000 0x3000
+				0x80100000 0x1000>;
+			reg-names = "dbi", "elbi", "atu", "parf", "config";
+			device_type = "pci";
+			linux,pci-domain = <0>;
+			bus-range = <0x00 0xff>;
+			num-lanes = <1>;
+			max-link-speed = <2>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			phys = <&pcie_x1phy>;
+			phy-names ="pciephy";
+
+			ranges = <0x81000000 0 0x80200000 0x80200000
+				  0 0x00100000   /* downstream I/O */
+				  0x82000000 0 0x80300000 0x80300000
+				  0 0x10000000>; /* non-prefetchable memory */
+
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0x7>;
+			interrupt-map = <0 0 0 1 &intc 0 142
+					 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+					<0 0 0 2 &intc 0 143
+					 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+					<0 0 0 3 &intc 0 144
+					 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+					<0 0 0 4 &intc 0 145
+					 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+
+			interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "global_irq";
+
+			clocks = <&gcc GCC_SYS_NOC_PCIE1_AXI_CLK>,
+				 <&gcc GCC_PCIE1_AXI_M_CLK>,
+				 <&gcc GCC_PCIE1_AXI_S_CLK>,
+				 <&gcc GCC_PCIE1_AHB_CLK>,
+				 <&gcc GCC_PCIE1_AUX_CLK>,
+				 <&gcc GCC_PCIE1_AXI_S_BRIDGE_CLK>;
+
+			clock-names = "iface",
+				      "axi_m",
+				      "axi_s",
+				      "ahb",
+				      "aux",
+				      "axi_bridge";
+
+			resets = <&gcc GCC_PCIE1_PIPE_ARES>,
+				 <&gcc GCC_PCIE1_SLEEP_ARES>,
+				 <&gcc GCC_PCIE1_CORE_STICKY_ARES>,
+				 <&gcc GCC_PCIE1_AXI_MASTER_ARES>,
+				 <&gcc GCC_PCIE1_AXI_SLAVE_ARES>,
+				 <&gcc GCC_PCIE1_AHB_ARES>,
+				 <&gcc GCC_PCIE1_AXI_MASTER_STICKY_ARES>,
+				 <&gcc GCC_PCIE1_AXI_SLAVE_STICKY_ARES>;
+
+			reset-names = "pipe",
+				      "sleep",
+				      "sticky",
+				      "axi_m",
+				      "axi_s",
+				      "ahb",
+				      "axi_m_sticky",
+				      "axi_s_sticky";
+
+			msi-map = <0x0 &v2m0 0x0 0xff8>;
+			status = "disabled";
+		};
+
+		pcie_x2: pci@a0000000 {
+			compatible = "qcom,pcie-ipq5018";
+			reg =  <0xa0000000 0xf1d
+				0xa0000F20 0xa8
+				0xa0001000 0x1000
+				0x80000 0x3000
+				0xa0100000 0x1000>;
+			reg-names = "dbi", "elbi", "atu", "parf", "config";
+			device_type = "pci";
+			linux,pci-domain = <1>;
+			bus-range = <0x00 0xff>;
+			num-lanes = <2>;
+			max-link-speed = <2>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			phys = <&pcie_x2phy>;
+			phy-names ="pciephy";
+
+			ranges = <0x81000000 0 0xa0200000 0xa0200000
+				  0 0x00100000   /* downstream I/O */
+				  0x82000000 0 0xa0300000 0xa0300000
+				  0 0x10000000>; /* non-prefetchable memory */
+
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0x7>;
+			interrupt-map = <0 0 0 1 &intc 0 75
+					 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+					<0 0 0 2 &intc 0 78
+					 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+					<0 0 0 3 &intc 0 79
+					 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+					<0 0 0 4 &intc 0 83
+					 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+
+			interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "global_irq";
+
+			clocks = <&gcc GCC_SYS_NOC_PCIE0_AXI_CLK>,
+				 <&gcc GCC_PCIE0_AXI_M_CLK>,
+				 <&gcc GCC_PCIE0_AXI_S_CLK>,
+				 <&gcc GCC_PCIE0_AHB_CLK>,
+				 <&gcc GCC_PCIE0_AUX_CLK>,
+				 <&gcc GCC_PCIE0_AXI_S_BRIDGE_CLK>;
+
+			clock-names = "iface",
+				      "axi_m",
+				      "axi_s",
+				      "ahb",
+				      "aux",
+				      "axi_bridge";
+
+			resets = <&gcc GCC_PCIE0_PIPE_ARES>,
+				 <&gcc GCC_PCIE0_SLEEP_ARES>,
+				 <&gcc GCC_PCIE0_CORE_STICKY_ARES>,
+				 <&gcc GCC_PCIE0_AXI_MASTER_ARES>,
+				 <&gcc GCC_PCIE0_AXI_SLAVE_ARES>,
+				 <&gcc GCC_PCIE0_AHB_ARES>,
+				 <&gcc GCC_PCIE0_AXI_MASTER_STICKY_ARES>,
+				 <&gcc GCC_PCIE0_AXI_SLAVE_STICKY_ARES>;
+
+			reset-names = "pipe",
+				      "sleep",
+				      "sticky",
+				      "axi_m",
+				      "axi_s",
+				      "ahb",
+				      "axi_m_sticky",
+				      "axi_s_sticky";
+
+			msi-map = <0x0 &v2m0 0x0 0xff8>;
+			status = "disabled";
+		};
+
 	};
 
 	timer {
-- 
2.17.1

Re: [PATCH 5/6] arm64: dts: qcom: ipq5018: Add PCIe related nodes

[Dmitry Baryshkov]

On Tue, 3 Oct 2023 at 15:10, Nitheesh Sekar <quic_nsekar@quicinc.com> wrote:
>
> Add phy and controller nodes for PCIe_x2 and PCIe_x1.
> PCIe_x2 is 2-lane Gen2 and PCIe_x1 is 1-lane Gen2.
>
> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq5018.dtsi | 186 +++++++++++++++++++++++++-
>  1 file changed, 184 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> index 38ffdc3cbdcd..0818fdd1e693 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> @@ -8,6 +8,7 @@
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/clock/qcom,gcc-ipq5018.h>
>  #include <dt-bindings/reset/qcom,gcc-ipq5018.h>
> +#include <dt-bindings/gpio/gpio.h>
>
>  / {
>         interrupt-parent = <&intc>;
> @@ -94,6 +95,38 @@
>                 #size-cells = <1>;
>                 ranges = <0 0 0 0xffffffff>;
>
> +               pcie_x1phy: phy@7e000{
> +                       compatible = "qcom,ipq5018-uniphy-pcie-gen2x1";
> +                       reg = <0x0007e000 0x800>;
> +                       #phy-cells = <0>;
> +                       #clock-cells = <0>;
> +                       clocks = <&gcc GCC_PCIE1_PIPE_CLK>;
> +                       clock-names = "pipe_clk";
> +                       clock-output-names = "pcie1_pipe_clk";
> +                       assigned-clocks = <&gcc GCC_PCIE1_PIPE_CLK>;
> +                       assigned-clock-rates = <125000000>;
> +                       resets = <&gcc GCC_PCIE1_PHY_BCR>,
> +                                <&gcc GCC_PCIE1PHY_PHY_BCR>;
> +                       reset-names = "phy", "phy_phy";
> +                       status = "disabled";
> +               };
> +
> +               pcie_x2phy: phy@86000{
> +                       compatible = "qcom,ipq5018-uniphy-pcie-gen2x2";
> +                       reg = <0x00086000 0x800>;
> +                       #phy-cells = <0>;
> +                       #clock-cells = <0>;
> +                       clocks = <&gcc GCC_PCIE0_PIPE_CLK>;
> +                       clock-names = "pipe_clk";
> +                       clock-output-names = "pcie0_pipe_clk";
> +                       assigned-clocks = <&gcc GCC_PCIE0_PIPE_CLK>;
> +                       assigned-clock-rates = <125000000>;

Can this go into the PHY driver?

> +                       resets = <&gcc GCC_PCIE0_PHY_BCR>,
> +                                <&gcc GCC_PCIE0PHY_PHY_BCR>;
> +                       reset-names = "phy", "phy_phy";
> +                       status = "disabled";
> +               };
> +
>                 tlmm: pinctrl@1000000 {
>                         compatible = "qcom,ipq5018-tlmm";
>                         reg = <0x01000000 0x300000>;
> @@ -117,8 +150,8 @@
>                         reg = <0x01800000 0x80000>;
>                         clocks = <&xo_board_clk>,
>                                  <&sleep_clk>,
> -                                <0>,
> -                                <0>,
> +                                <&pcie_x2phy>,
> +                                <&pcie_x1phy>,
>                                  <0>,
>                                  <0>,
>                                  <0>,
> @@ -246,6 +279,155 @@
>                                 status = "disabled";
>                         };
>                 };
> +
> +               pcie_x1: pci@80000000 {
> +                       compatible = "qcom,pcie-ipq5018";
> +                       reg =  <0x80000000 0xf1d

Each address/size tuple should be a separate <> entry.

> +                               0x80000F20 0xa8

lowercase

> +                               0x80001000 0x1000
> +                               0x78000 0x3000

Would you notice why this line stands away from the rest of entries here?

> +                               0x80100000 0x1000>;
> +                       reg-names = "dbi", "elbi", "atu", "parf", "config";
> +                       device_type = "pci";
> +                       linux,pci-domain = <0>;
> +                       bus-range = <0x00 0xff>;
> +                       num-lanes = <1>;
> +                       max-link-speed = <2>;
> +                       #address-cells = <3>;
> +                       #size-cells = <2>;
> +
> +                       phys = <&pcie_x1phy>;
> +                       phy-names ="pciephy";
> +
> +                       ranges = <0x81000000 0 0x80200000 0x80200000
> +                                 0 0x00100000   /* downstream I/O */
> +                                 0x82000000 0 0x80300000 0x80300000
> +                                 0 0x10000000>; /* non-prefetchable memory */
> +
> +                       #interrupt-cells = <1>;
> +                       interrupt-map-mask = <0 0 0 0x7>;
> +                       interrupt-map = <0 0 0 1 &intc 0 142
> +                                        IRQ_TYPE_LEVEL_HIGH>, /* int_a */
> +                                       <0 0 0 2 &intc 0 143
> +                                        IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> +                                       <0 0 0 3 &intc 0 144
> +                                        IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> +                                       <0 0 0 4 &intc 0 145
> +                                        IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> +
> +                       interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>;
> +                       interrupt-names = "global_irq";
> +
> +                       clocks = <&gcc GCC_SYS_NOC_PCIE1_AXI_CLK>,
> +                                <&gcc GCC_PCIE1_AXI_M_CLK>,
> +                                <&gcc GCC_PCIE1_AXI_S_CLK>,
> +                                <&gcc GCC_PCIE1_AHB_CLK>,
> +                                <&gcc GCC_PCIE1_AUX_CLK>,
> +                                <&gcc GCC_PCIE1_AXI_S_BRIDGE_CLK>;
> +
> +                       clock-names = "iface",
> +                                     "axi_m",
> +                                     "axi_s",
> +                                     "ahb",
> +                                     "aux",
> +                                     "axi_bridge";
> +
> +                       resets = <&gcc GCC_PCIE1_PIPE_ARES>,
> +                                <&gcc GCC_PCIE1_SLEEP_ARES>,
> +                                <&gcc GCC_PCIE1_CORE_STICKY_ARES>,
> +                                <&gcc GCC_PCIE1_AXI_MASTER_ARES>,
> +                                <&gcc GCC_PCIE1_AXI_SLAVE_ARES>,
> +                                <&gcc GCC_PCIE1_AHB_ARES>,
> +                                <&gcc GCC_PCIE1_AXI_MASTER_STICKY_ARES>,
> +                                <&gcc GCC_PCIE1_AXI_SLAVE_STICKY_ARES>;
> +
> +                       reset-names = "pipe",
> +                                     "sleep",
> +                                     "sticky",
> +                                     "axi_m",
> +                                     "axi_s",
> +                                     "ahb",
> +                                     "axi_m_sticky",
> +                                     "axi_s_sticky";
> +
> +                       msi-map = <0x0 &v2m0 0x0 0xff8>;
> +                       status = "disabled";
> +               };
> +
> +               pcie_x2: pci@a0000000 {
> +                       compatible = "qcom,pcie-ipq5018";
> +                       reg =  <0xa0000000 0xf1d
> +                               0xa0000F20 0xa8
> +                               0xa0001000 0x1000
> +                               0x80000 0x3000
> +                               0xa0100000 0x1000>;
> +                       reg-names = "dbi", "elbi", "atu", "parf", "config";
> +                       device_type = "pci";
> +                       linux,pci-domain = <1>;
> +                       bus-range = <0x00 0xff>;
> +                       num-lanes = <2>;
> +                       max-link-speed = <2>;
> +                       #address-cells = <3>;
> +                       #size-cells = <2>;
> +
> +                       phys = <&pcie_x2phy>;
> +                       phy-names ="pciephy";
> +
> +                       ranges = <0x81000000 0 0xa0200000 0xa0200000
> +                                 0 0x00100000   /* downstream I/O */
> +                                 0x82000000 0 0xa0300000 0xa0300000
> +                                 0 0x10000000>; /* non-prefetchable memory */
> +
> +                       #interrupt-cells = <1>;
> +                       interrupt-map-mask = <0 0 0 0x7>;
> +                       interrupt-map = <0 0 0 1 &intc 0 75
> +                                        IRQ_TYPE_LEVEL_HIGH>, /* int_a */
> +                                       <0 0 0 2 &intc 0 78
> +                                        IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> +                                       <0 0 0 3 &intc 0 79
> +                                        IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> +                                       <0 0 0 4 &intc 0 83
> +                                        IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> +
> +                       interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>;
> +                       interrupt-names = "global_irq";
> +
> +                       clocks = <&gcc GCC_SYS_NOC_PCIE0_AXI_CLK>,
> +                                <&gcc GCC_PCIE0_AXI_M_CLK>,
> +                                <&gcc GCC_PCIE0_AXI_S_CLK>,
> +                                <&gcc GCC_PCIE0_AHB_CLK>,
> +                                <&gcc GCC_PCIE0_AUX_CLK>,
> +                                <&gcc GCC_PCIE0_AXI_S_BRIDGE_CLK>;
> +
> +                       clock-names = "iface",
> +                                     "axi_m",
> +                                     "axi_s",
> +                                     "ahb",
> +                                     "aux",
> +                                     "axi_bridge";
> +
> +                       resets = <&gcc GCC_PCIE0_PIPE_ARES>,
> +                                <&gcc GCC_PCIE0_SLEEP_ARES>,
> +                                <&gcc GCC_PCIE0_CORE_STICKY_ARES>,
> +                                <&gcc GCC_PCIE0_AXI_MASTER_ARES>,
> +                                <&gcc GCC_PCIE0_AXI_SLAVE_ARES>,
> +                                <&gcc GCC_PCIE0_AHB_ARES>,
> +                                <&gcc GCC_PCIE0_AXI_MASTER_STICKY_ARES>,
> +                                <&gcc GCC_PCIE0_AXI_SLAVE_STICKY_ARES>;
> +
> +                       reset-names = "pipe",
> +                                     "sleep",
> +                                     "sticky",
> +                                     "axi_m",
> +                                     "axi_s",
> +                                     "ahb",
> +                                     "axi_m_sticky",
> +                                     "axi_s_sticky";
> +
> +                       msi-map = <0x0 &v2m0 0x0 0xff8>;
> +                       status = "disabled";
> +               };
> +
>         };
>
>         timer {
> --
> 2.17.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 5/6] arm64: dts: qcom: ipq5018: Add PCIe related nodes

[Nitheesh Sekar]

On 10/3/2023 8:53 PM, Dmitry Baryshkov wrote:
> On Tue, 3 Oct 2023 at 15:10, Nitheesh Sekar <quic_nsekar@quicinc.com> wrote:
>> Add phy and controller nodes for PCIe_x2 and PCIe_x1.
>> PCIe_x2 is 2-lane Gen2 and PCIe_x1 is 1-lane Gen2.
>>
>> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq5018.dtsi | 186 +++++++++++++++++++++++++-
>>   1 file changed, 184 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>> index 38ffdc3cbdcd..0818fdd1e693 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>> @@ -8,6 +8,7 @@
>>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>>   #include <dt-bindings/clock/qcom,gcc-ipq5018.h>
>>   #include <dt-bindings/reset/qcom,gcc-ipq5018.h>
>> +#include <dt-bindings/gpio/gpio.h>
>>
>>   / {
>>          interrupt-parent = <&intc>;
>> @@ -94,6 +95,38 @@
>>                  #size-cells = <1>;
>>                  ranges = <0 0 0 0xffffffff>;
>>
>> +               pcie_x1phy: phy@7e000{
>> +                       compatible = "qcom,ipq5018-uniphy-pcie-gen2x1";
>> +                       reg = <0x0007e000 0x800>;
>> +                       #phy-cells = <0>;
>> +                       #clock-cells = <0>;
>> +                       clocks = <&gcc GCC_PCIE1_PIPE_CLK>;
>> +                       clock-names = "pipe_clk";
>> +                       clock-output-names = "pcie1_pipe_clk";
>> +                       assigned-clocks = <&gcc GCC_PCIE1_PIPE_CLK>;
>> +                       assigned-clock-rates = <125000000>;
>> +                       resets = <&gcc GCC_PCIE1_PHY_BCR>,
>> +                                <&gcc GCC_PCIE1PHY_PHY_BCR>;
>> +                       reset-names = "phy", "phy_phy";
>> +                       status = "disabled";
>> +               };
>> +
>> +               pcie_x2phy: phy@86000{
>> +                       compatible = "qcom,ipq5018-uniphy-pcie-gen2x2";
>> +                       reg = <0x00086000 0x800>;
>> +                       #phy-cells = <0>;
>> +                       #clock-cells = <0>;
>> +                       clocks = <&gcc GCC_PCIE0_PIPE_CLK>;
>> +                       clock-names = "pipe_clk";
>> +                       clock-output-names = "pcie0_pipe_clk";
>> +                       assigned-clocks = <&gcc GCC_PCIE0_PIPE_CLK>;
>> +                       assigned-clock-rates = <125000000>;
> Can this go into the PHY driver?
Sure. Will check and update.
>
>> +                       resets = <&gcc GCC_PCIE0_PHY_BCR>,
>> +                                <&gcc GCC_PCIE0PHY_PHY_BCR>;
>> +                       reset-names = "phy", "phy_phy";
>> +                       status = "disabled";
>> +               };
>> +
>>                  tlmm: pinctrl@1000000 {
>>                          compatible = "qcom,ipq5018-tlmm";
>>                          reg = <0x01000000 0x300000>;
>> @@ -117,8 +150,8 @@
>>                          reg = <0x01800000 0x80000>;
>>                          clocks = <&xo_board_clk>,
>>                                   <&sleep_clk>,
>> -                                <0>,
>> -                                <0>,
>> +                                <&pcie_x2phy>,
>> +                                <&pcie_x1phy>,
>>                                   <0>,
>>                                   <0>,
>>                                   <0>,
>> @@ -246,6 +279,155 @@
>>                                  status = "disabled";
>>                          };
>>                  };
>> +
>> +               pcie_x1: pci@80000000 {
>> +                       compatible = "qcom,pcie-ipq5018";
>> +                       reg =  <0x80000000 0xf1d
> Each address/size tuple should be a separate <> entry.
Sure. will update it.
>
>> +                               0x80000F20 0xa8
> lowercase
Sure. Will update.
>
>> +                               0x80001000 0x1000
>> +                               0x78000 0x3000
> Would you notice why this line stands away from the rest of entries here?

Sure. Will pad it Zeros.

Thanks,
Nitheesh

Re: [PATCH 5/6] arm64: dts: qcom: ipq5018: Add PCIe related nodes

[Manivannan Sadhasivam]

On Tue, Oct 03, 2023 at 05:38:45PM +0530, Nitheesh Sekar wrote:
> Add phy and controller nodes for PCIe_x2 and PCIe_x1.
> PCIe_x2 is 2-lane Gen2 and PCIe_x1 is 1-lane Gen2.
> 
> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq5018.dtsi | 186 +++++++++++++++++++++++++-
>  1 file changed, 184 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> index 38ffdc3cbdcd..0818fdd1e693 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> @@ -8,6 +8,7 @@
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/clock/qcom,gcc-ipq5018.h>
>  #include <dt-bindings/reset/qcom,gcc-ipq5018.h>
> +#include <dt-bindings/gpio/gpio.h>
>  
>  / {
>  	interrupt-parent = <&intc>;
> @@ -94,6 +95,38 @@
>  		#size-cells = <1>;
>  		ranges = <0 0 0 0xffffffff>;
>  
> +		pcie_x1phy: phy@7e000{
> +			compatible = "qcom,ipq5018-uniphy-pcie-gen2x1";
> +			reg = <0x0007e000 0x800>;
> +			#phy-cells = <0>;
> +			#clock-cells = <0>;
> +			clocks = <&gcc GCC_PCIE1_PIPE_CLK>;
> +			clock-names = "pipe_clk";
> +			clock-output-names = "pcie1_pipe_clk";
> +			assigned-clocks = <&gcc GCC_PCIE1_PIPE_CLK>;
> +			assigned-clock-rates = <125000000>;
> +			resets = <&gcc GCC_PCIE1_PHY_BCR>,
> +				 <&gcc GCC_PCIE1PHY_PHY_BCR>;
> +			reset-names = "phy", "phy_phy";
> +			status = "disabled";
> +		};
> +
> +		pcie_x2phy: phy@86000{
> +			compatible = "qcom,ipq5018-uniphy-pcie-gen2x2";
> +			reg = <0x00086000 0x800>;
> +			#phy-cells = <0>;
> +			#clock-cells = <0>;
> +			clocks = <&gcc GCC_PCIE0_PIPE_CLK>;
> +			clock-names = "pipe_clk";
> +			clock-output-names = "pcie0_pipe_clk";
> +			assigned-clocks = <&gcc GCC_PCIE0_PIPE_CLK>;
> +			assigned-clock-rates = <125000000>;
> +			resets = <&gcc GCC_PCIE0_PHY_BCR>,
> +				 <&gcc GCC_PCIE0PHY_PHY_BCR>;
> +			reset-names = "phy", "phy_phy";
> +			status = "disabled";
> +		};
> +
>  		tlmm: pinctrl@1000000 {
>  			compatible = "qcom,ipq5018-tlmm";
>  			reg = <0x01000000 0x300000>;
> @@ -117,8 +150,8 @@
>  			reg = <0x01800000 0x80000>;
>  			clocks = <&xo_board_clk>,
>  				 <&sleep_clk>,
> -				 <0>,
> -				 <0>,
> +				 <&pcie_x2phy>,
> +				 <&pcie_x1phy>,
>  				 <0>,
>  				 <0>,
>  				 <0>,
> @@ -246,6 +279,155 @@
>  				status = "disabled";
>  			};
>  		};
> +
> +		pcie_x1: pci@80000000 {
> +			compatible = "qcom,pcie-ipq5018";
> +			reg =  <0x80000000 0xf1d
> +				0x80000F20 0xa8
> +				0x80001000 0x1000
> +				0x78000 0x3000
> +				0x80100000 0x1000>;
> +			reg-names = "dbi", "elbi", "atu", "parf", "config";
> +			device_type = "pci";
> +			linux,pci-domain = <0>;
> +			bus-range = <0x00 0xff>;
> +			num-lanes = <1>;
> +			max-link-speed = <2>;
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +
> +			phys = <&pcie_x1phy>;
> +			phy-names ="pciephy";
> +
> +			ranges = <0x81000000 0 0x80200000 0x80200000

Why do you need "relocatable" flag? Same question for other range also.

> +				  0 0x00100000   /* downstream I/O */
> +				  0x82000000 0 0x80300000 0x80300000
> +				  0 0x10000000>; /* non-prefetchable memory */
> +

Don't you need "dma-coherent" to specify the devices as cache coherent? I assume
all the recent PCIe generations are cache coherent.

- Mani

> +			#interrupt-cells = <1>;
> +			interrupt-map-mask = <0 0 0 0x7>;
> +			interrupt-map = <0 0 0 1 &intc 0 142
> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
> +					<0 0 0 2 &intc 0 143
> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> +					<0 0 0 3 &intc 0 144
> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> +					<0 0 0 4 &intc 0 145
> +					 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> +
> +			interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "global_irq";
> +
> +			clocks = <&gcc GCC_SYS_NOC_PCIE1_AXI_CLK>,
> +				 <&gcc GCC_PCIE1_AXI_M_CLK>,
> +				 <&gcc GCC_PCIE1_AXI_S_CLK>,
> +				 <&gcc GCC_PCIE1_AHB_CLK>,
> +				 <&gcc GCC_PCIE1_AUX_CLK>,
> +				 <&gcc GCC_PCIE1_AXI_S_BRIDGE_CLK>;
> +
> +			clock-names = "iface",
> +				      "axi_m",
> +				      "axi_s",
> +				      "ahb",
> +				      "aux",
> +				      "axi_bridge";
> +
> +			resets = <&gcc GCC_PCIE1_PIPE_ARES>,
> +				 <&gcc GCC_PCIE1_SLEEP_ARES>,
> +				 <&gcc GCC_PCIE1_CORE_STICKY_ARES>,
> +				 <&gcc GCC_PCIE1_AXI_MASTER_ARES>,
> +				 <&gcc GCC_PCIE1_AXI_SLAVE_ARES>,
> +				 <&gcc GCC_PCIE1_AHB_ARES>,
> +				 <&gcc GCC_PCIE1_AXI_MASTER_STICKY_ARES>,
> +				 <&gcc GCC_PCIE1_AXI_SLAVE_STICKY_ARES>;
> +
> +			reset-names = "pipe",
> +				      "sleep",
> +				      "sticky",
> +				      "axi_m",
> +				      "axi_s",
> +				      "ahb",
> +				      "axi_m_sticky",
> +				      "axi_s_sticky";
> +
> +			msi-map = <0x0 &v2m0 0x0 0xff8>;
> +			status = "disabled";
> +		};
> +
> +		pcie_x2: pci@a0000000 {
> +			compatible = "qcom,pcie-ipq5018";
> +			reg =  <0xa0000000 0xf1d
> +				0xa0000F20 0xa8
> +				0xa0001000 0x1000
> +				0x80000 0x3000
> +				0xa0100000 0x1000>;
> +			reg-names = "dbi", "elbi", "atu", "parf", "config";
> +			device_type = "pci";
> +			linux,pci-domain = <1>;
> +			bus-range = <0x00 0xff>;
> +			num-lanes = <2>;
> +			max-link-speed = <2>;
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +
> +			phys = <&pcie_x2phy>;
> +			phy-names ="pciephy";
> +
> +			ranges = <0x81000000 0 0xa0200000 0xa0200000
> +				  0 0x00100000   /* downstream I/O */
> +				  0x82000000 0 0xa0300000 0xa0300000
> +				  0 0x10000000>; /* non-prefetchable memory */
> +
> +			#interrupt-cells = <1>;
> +			interrupt-map-mask = <0 0 0 0x7>;
> +			interrupt-map = <0 0 0 1 &intc 0 75
> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
> +					<0 0 0 2 &intc 0 78
> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> +					<0 0 0 3 &intc 0 79
> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> +					<0 0 0 4 &intc 0 83
> +					 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> +
> +			interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "global_irq";
> +
> +			clocks = <&gcc GCC_SYS_NOC_PCIE0_AXI_CLK>,
> +				 <&gcc GCC_PCIE0_AXI_M_CLK>,
> +				 <&gcc GCC_PCIE0_AXI_S_CLK>,
> +				 <&gcc GCC_PCIE0_AHB_CLK>,
> +				 <&gcc GCC_PCIE0_AUX_CLK>,
> +				 <&gcc GCC_PCIE0_AXI_S_BRIDGE_CLK>;
> +
> +			clock-names = "iface",
> +				      "axi_m",
> +				      "axi_s",
> +				      "ahb",
> +				      "aux",
> +				      "axi_bridge";
> +
> +			resets = <&gcc GCC_PCIE0_PIPE_ARES>,
> +				 <&gcc GCC_PCIE0_SLEEP_ARES>,
> +				 <&gcc GCC_PCIE0_CORE_STICKY_ARES>,
> +				 <&gcc GCC_PCIE0_AXI_MASTER_ARES>,
> +				 <&gcc GCC_PCIE0_AXI_SLAVE_ARES>,
> +				 <&gcc GCC_PCIE0_AHB_ARES>,
> +				 <&gcc GCC_PCIE0_AXI_MASTER_STICKY_ARES>,
> +				 <&gcc GCC_PCIE0_AXI_SLAVE_STICKY_ARES>;
> +
> +			reset-names = "pipe",
> +				      "sleep",
> +				      "sticky",
> +				      "axi_m",
> +				      "axi_s",
> +				      "ahb",
> +				      "axi_m_sticky",
> +				      "axi_s_sticky";
> +
> +			msi-map = <0x0 &v2m0 0x0 0xff8>;
> +			status = "disabled";
> +		};
> +
>  	};
>  
>  	timer {
> -- 
> 2.17.1
> 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 6/6] arm64: dts: qcom: ipq5018: Enable PCIe

[Nitheesh Sekar]

Enable the PCIe controller and PHY nodes for RDP 432-c2.

Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts b/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
index e636a1cb9b77..be7d92700517 100644
--- a/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
+++ b/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
@@ -28,6 +28,15 @@
 	status = "okay";
 };
 
+&pcie_x2 {
+	status = "ok";
+	perst-gpios = <&tlmm 15 GPIO_ACTIVE_LOW>;
+};
+
+&pcie_x2phy {
+	status = "ok";
+};
+
 &sdhc_1 {
 	pinctrl-0 = <&sdc_default_state>;
 	pinctrl-names = "default";
-- 
2.17.1

Re: [PATCH 6/6] arm64: dts: qcom: ipq5018: Enable PCIe

[Konrad Dybcio]

On 3.10.2023 14:08, Nitheesh Sekar wrote:
> Enable the PCIe controller and PHY nodes for RDP 432-c2.
> 
> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts b/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
> index e636a1cb9b77..be7d92700517 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
> +++ b/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
> @@ -28,6 +28,15 @@
>  	status = "okay";
>  };
>  
> +&pcie_x2 {
> +	status = "ok";
"okay" is preferred

It's also preferred to keep status as the last property within
a node.

Konrad

Re: [PATCH 6/6] arm64: dts: qcom: ipq5018: Enable PCIe

[Nitheesh Sekar]

On 10/7/2023 5:57 AM, Konrad Dybcio wrote:
> On 3.10.2023 14:08, Nitheesh Sekar wrote:
>> Enable the PCIe controller and PHY nodes for RDP 432-c2.
>>
>> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts b/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
>> index e636a1cb9b77..be7d92700517 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
>> +++ b/arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dts
>> @@ -28,6 +28,15 @@
>>   	status = "okay";
>>   };
>>   
>> +&pcie_x2 {
>> +	status = "ok";
> "okay" is preferred
>
> It's also preferred to keep status as the last property within
> a node.
>
> Konrad

Sure. will update.

Thanks,
Nitheesh