[PATCH v5 0/5] Add PCIe support for Qualcomm IPQ5332

[PATCH v5 0/5] Add PCIe support for Qualcomm IPQ5332

[Varadarajan Narayanan]

Patch series adds support for enabling the PCIe controller and
UNIPHY found on Qualcomm IPQ5332 platform. PCIe0 is Gen3 X1 and
PCIe1 is Gen3 X2 are added.

This series combines [1] and [2]. [1] introduces IPQ5018 PCIe
support and [2] depends on [1] to introduce IPQ5332 PCIe support.
Since the community was interested in [2] (please see [3]), tried
to revive IPQ5332's PCIe support with v2 of this patch series.

v2 of this series pulled in the phy driver from [1] tried to
address comments/feedback given in both [1] and [2].

1. Enable IPQ5018 PCI support (Nitheesh Sekar) - https://lore.kernel.org/all/20231003120846.28626-1-quic_nsekar@quicinc.com/
2. Add PCIe support for Qualcomm IPQ5332 (Praveenkumar I) - https://lore.kernel.org/linux-arm-msm/20231214062847.2215542-1-quic_ipkumar@quicinc.com/
3. Community interest - https://lore.kernel.org/linux-arm-msm/20240310132915.GE3390@thinkpad/

v5: phy bindings:
    * Drop '3x1' & '3x2' from compatible string
    * Use 'num-lanes' to differentiate instead of '3x1' or '3x2'
      in compatible string
    * Describe clocks and resets instead of just maxItems

    phy driver:
    * Get num-lanes from DTS
    * Drop compatible specific init data as there is only one
      compatible string

    controller bindings:
    * Re-arrange 5332 and 9574 compatibles to handle fallback usage in dts

    dtsi:
    * Add 'num-lanes' to "pcie1_phy: phy@4b1000"
    * Make ipq5332 as main and ipq9574 as fallback compatible
    * Sort controller nodes per address

    misc:
    Add R-B tag from Konrad to dts and dtsi patches

v4: * phy bindings - Create ipq5332 compatible instead of reusing ipq9574 for bindings
    * phy bindings - Remove reset-names as the resets are handled with bulk APIs
    * phy bindings - Fix order in the 'required' section
    * phy bindings - Remove clock-output-names
    * dtsi - Add missing reset for pcie1_phy
    * dtsi - Convert 'reg-names' to a vertical list
    * dts - Fix nodes sort order
    * dts - Use property-n followed by property-names

v3: * Update the cover letter with the sources of the patches
    * Rename the dt-bindings yaml file similar to other phys
    * Drop ipq5332 specific pcie controllor bindings and reuse
      ipq9574 pcie controller bindings for ipq5332
    * Please see patches for specific changes
    * Set GPL license for phy-qcom-uniphy-pcie-28lp.c

v2: Address review comments from V1
    Drop the 'required clocks' change that would break ABI (in dt-binding, dts, gcc-ipq5332.c)
    Include phy driver from the dependent series

v1: https://lore.kernel.org/linux-arm-msm/20231214062847.2215542-1-quic_ipkumar@quicinc.com/

Nitheesh Sekar (2):
  dt-bindings: phy: qcom,uniphy-pcie: Document PCIe uniphy
  phy: qcom: Introduce PCIe UNIPHY 28LP driver

Praveenkumar I (2):
  arm64: dts: qcom: ipq5332: Add PCIe related nodes
  arm64: dts: qcom: ipq5332-rdp441: Enable PCIe phys and controllers

Varadarajan Narayanan (1):
  dt-bindings: PCI: qcom: Document the IPQ5332 PCIe controller

 .../devicetree/bindings/pci/qcom,pcie.yaml    |  10 +-
 .../phy/qcom,ipq5332-uniphy-pcie-phy.yaml     |  71 +++++
 arch/arm64/boot/dts/qcom/ipq5332-rdp441.dts   |  76 +++++
 arch/arm64/boot/dts/qcom/ipq5332.dtsi         | 221 +++++++++++++-
 drivers/phy/qualcomm/Kconfig                  |  12 +
 drivers/phy/qualcomm/Makefile                 |   1 +
 .../phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c  | 287 ++++++++++++++++++
 7 files changed, 674 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/phy/qcom,ipq5332-uniphy-pcie-phy.yaml
 create mode 100644 drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c


base-commit: 8155b4ef3466f0e289e8fcc9e6e62f3f4dceeac2
-- 
2.34.1

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

[Varadarajan Narayanan]

From: Nitheesh Sekar <quic_nsekar@quicinc.com>

Document the Qualcomm UNIPHY PCIe 28LP present in IPQ5332.

Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
v5: * Drop '3x1' & '3x2' from compatible string
    * Use 'num-lanes' to differentiate instead of '3x1' or '3x2'
      in compatible string
    * Describe clocks and resets instead of just maxItems

v4: Remove reset-names as the resets are not used individually
    Remove clock-output-names as its usage is removed from driver
    Fix order in the 'required' section

v3: Fix compatible string to be similar to other phys and rename file accordingly
    Fix clocks minItems -> maxItems
    Change one of the maintainer from Sricharan to Varadarajan

v2: Rename the file to match the compatible
    Drop 'driver' from title
    Dropped 'clock-names'
    Fixed 'reset-names'
---
 .../phy/qcom,ipq5332-uniphy-pcie-phy.yaml     | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/qcom,ipq5332-uniphy-pcie-phy.yaml

diff --git a/Documentation/devicetree/bindings/phy/qcom,ipq5332-uniphy-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,ipq5332-uniphy-pcie-phy.yaml
new file mode 100644
index 000000000000..9d37f5914c7c
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/qcom,ipq5332-uniphy-pcie-phy.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/qcom,ipq5332-uniphy-pcie-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm UNIPHY PCIe 28LP PHY
+
+maintainers:
+  - Nitheesh Sekar <quic_nsekar@quicinc.com>
+  - Varadarajan Narayanan <quic_varada@quicinc.com>
+
+description:
+  PCIe and USB combo PHY found in Qualcomm IPQ5332 SoC
+
+properties:
+  compatible:
+    enum:
+      - qcom,ipq5332-uniphy-pcie-phy
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: pcie pipe clock
+      - description: pcie ahb clock
+
+  resets:
+    items:
+      - description: phy reset
+      - description: ahb reset
+      - description: cfg reset
+
+  "#phy-cells":
+    const: 0
+
+  "#clock-cells":
+    const: 0
+
+  num-lanes: true
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - resets
+  - "#phy-cells"
+  - "#clock-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
+
+    pcie0_phy: phy@4b0000 {
+        compatible = "qcom,ipq5332-uniphy-pcie-phy";
+        reg = <0x004b0000 0x800>;
+
+        clocks = <&gcc GCC_PCIE3X1_0_PIPE_CLK>,
+                 <&gcc GCC_PCIE3X1_PHY_AHB_CLK>;
+
+        resets = <&gcc GCC_PCIE3X1_0_PHY_BCR>,
+                 <&gcc GCC_PCIE3X1_PHY_AHB_CLK_ARES>,
+                 <&gcc GCC_PCIE3X1_0_PHY_PHY_BCR>;
+
+        #clock-cells = <0>;
+
+        #phy-cells = <0>;
+    };
-- 
2.34.1

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

[Krzysztof Kozlowski]

On Thu, Jan 02, 2025 at 05:00:15PM +0530, Varadarajan Narayanan wrote:
> +  "#phy-cells":
> +    const: 0
> +
> +  "#clock-cells":
> +    const: 0
> +
> +  num-lanes: true

$ref: /schemas/types.yaml#/definitions/uint32
enum:

or this should be moved to some shared schema.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - resets
> +  - "#phy-cells"
> +  - "#clock-cells"

num-lanes should be required. How does it work without it? There is no
default.

Best regards,
Krzysztof

[PATCH v5 2/5] phy: qcom: Introduce PCIe UNIPHY 28LP driver

[Varadarajan Narayanan]

From: Nitheesh Sekar <quic_nsekar@quicinc.com>

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

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
v5: * Use 'num-lanes' to differentiate instead of '3x1' or '3x2'
      in compatible string
    * Drop compatible specific init data as there is only one
      compatible string
    * Fix header file order

v4: Fix uppercase hex digit
    Use phy->id for pipe clock source

v3: Added 'Reviewed-by: Dmitry Baryshkov' and made following updates
    s/unsigned int/u32/g
    Fix 'lane_offset' comments
    Fix #define tab -> space
    Fix mixed case hex numbers
    Fix licensing & owner
    Change for-loop pointer to use [] instead of ->
    Use 'less than max' instead of 'not equal to max' in termination condition
    Smatch and Coccinelle passed

v2: Drop IPQ5018 related code and data
    Use uniform prefix for struct names
    Place "}, {", on the same line
    In qcom_uniphy_pcie_init(), use for-loop instead of while
    Swap reset and clock disable order in qcom_uniphy_pcie_power_off
    Add reset assert to qcom_uniphy_pcie_power_on's error path
    Use macros for usleep duration
    Inlined qcom_uniphy_pcie_get_resources & use devm_platform_get_and_ioremap_resource
    Drop 'clock-output-names' from phy_pipe_clk_register
---
 drivers/phy/qualcomm/Kconfig                  |  12 +
 drivers/phy/qualcomm/Makefile                 |   1 +
 .../phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c  | 287 ++++++++++++++++++
 3 files changed, 300 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 846f8c99547f..a6b71fda1b9c 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 IPQ5332 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 eb60e950ad53..42038bc30974 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..536302f81105
--- /dev/null
+++ b/drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c
@@ -0,0 +1,287 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2025, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#define RST_ASSERT_DELAY_MIN_US		100
+#define RST_ASSERT_DELAY_MAX_US		150
+#define PIPE_CLK_DELAY_MIN_US		5000
+#define PIPE_CLK_DELAY_MAX_US		5100
+#define CLK_EN_DELAY_MIN_US		30
+#define CLK_EN_DELAY_MAX_US		50
+#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_CFG_PLLCFG				0x220
+#define PHY_CFG_EIOS_DTCT_REG			0x3e4
+#define PHY_CFG_GEN3_ALIGN_HOLDOFF_TIME		0x3e8
+
+#define PHY_MODE_FIXED		0x1
+
+enum qcom_uniphy_pcie_type {
+	PHY_TYPE_PCIE = 1,
+	PHY_TYPE_PCIE_GEN2,
+	PHY_TYPE_PCIE_GEN3,
+};
+
+struct qcom_uniphy_pcie_regs {
+	u32 offset;
+	u32 val;
+};
+
+struct qcom_uniphy_pcie_data {
+	int lane_offset; /* offset between the lane register bases */
+	u32 phy_type;
+	const struct qcom_uniphy_pcie_regs *init_seq;
+	u32 init_seq_num;
+	u32 pipe_clk_rate;
+};
+
+struct qcom_uniphy_pcie {
+	struct phy phy;
+	struct device *dev;
+	const struct qcom_uniphy_pcie_data *data;
+	struct clk_bulk_data *clks;
+	int num_clks;
+	struct reset_control *resets;
+	void __iomem *base;
+	int lanes;
+};
+
+#define phy_to_dw_phy(x)	container_of((x), struct qca_uni_pcie_phy, phy)
+
+static const struct qcom_uniphy_pcie_regs ipq5332_regs[] = {
+	{
+		.offset = PHY_CFG_PLLCFG,
+		.val = 0x30,
+	}, {
+		.offset = PHY_CFG_EIOS_DTCT_REG,
+		.val = 0x53ef,
+	}, {
+		.offset = PHY_CFG_GEN3_ALIGN_HOLDOFF_TIME,
+		.val = 0xcf,
+	},
+};
+
+static const struct qcom_uniphy_pcie_data ipq5332_data = {
+	.lane_offset	= 0x800,
+	.phy_type	= PHY_TYPE_PCIE_GEN3,
+	.init_seq	= ipq5332_regs,
+	.init_seq_num	= ARRAY_SIZE(ipq5332_regs),
+	.pipe_clk_rate	= 250000000,
+};
+
+static void qcom_uniphy_pcie_init(struct qcom_uniphy_pcie *phy)
+{
+	const struct qcom_uniphy_pcie_data *data = phy->data;
+	const struct qcom_uniphy_pcie_regs *init_seq;
+	void __iomem *base = phy->base;
+	int lane, i;
+
+	for (lane = 0; lane < phy->lanes; lane++) {
+		init_seq = data->init_seq;
+
+		for (i = 0; i < data->init_seq_num; i++)
+			writel(init_seq[i].val, base + init_seq[i].offset);
+
+		base += data->lane_offset;
+	}
+}
+
+static int qcom_uniphy_pcie_power_off(struct phy *x)
+{
+	struct qcom_uniphy_pcie *phy = phy_get_drvdata(x);
+
+	clk_bulk_disable_unprepare(phy->num_clks, phy->clks);
+
+	return reset_control_assert(phy->resets);
+}
+
+static int qcom_uniphy_pcie_power_on(struct phy *x)
+{
+	struct qcom_uniphy_pcie *phy = phy_get_drvdata(x);
+	int ret;
+
+	ret = reset_control_assert(phy->resets);
+	if (ret) {
+		dev_err(phy->dev, "reset assert failed (%d)\n", ret);
+		return ret;
+	}
+
+	usleep_range(RST_ASSERT_DELAY_MIN_US, RST_ASSERT_DELAY_MAX_US);
+
+	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(CLK_EN_DELAY_MIN_US, CLK_EN_DELAY_MAX_US);
+
+	qcom_uniphy_pcie_init(phy);
+	return 0;
+}
+
+static inline int qcom_uniphy_pcie_get_resources(struct platform_device *pdev,
+						 struct qcom_uniphy_pcie *phy)
+{
+	struct resource *res;
+
+	phy->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(phy->base))
+		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 inline int phy_pipe_clk_register(struct qcom_uniphy_pcie *phy, int id)
+{
+	const struct qcom_uniphy_pcie_data *data = phy->data;
+	struct clk_hw *hw;
+	char name[64];
+
+	snprintf(name, sizeof(name), "phy%d_pipe_clk_src", id);
+	hw = devm_clk_hw_register_fixed_rate(phy->dev, name, NULL, 0,
+					     data->pipe_clk_rate);
+	if (IS_ERR(hw))
+		return dev_err_probe(phy->dev, PTR_ERR(hw),
+				     "Unable to register %s\n", name);
+
+	return devm_of_clk_add_hw_provider(phy->dev, of_clk_hw_simple_get, hw);
+}
+
+static const struct of_device_id qcom_uniphy_pcie_id_table[] = {
+	{
+		.compatible = "qcom,ipq5332-uniphy-pcie-phy",
+		.data = &ipq5332_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 phy_provider *phy_provider;
+	struct device *dev = &pdev->dev;
+	struct qcom_uniphy_pcie *phy;
+	struct phy *generic_phy;
+	int ret;
+
+	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 = of_property_read_u32(of_node_get(dev->of_node), "num-lanes",
+				   &phy->lanes);
+	if (ret)
+		phy->lanes = 1;
+
+	ret = qcom_uniphy_pcie_get_resources(pdev, phy);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to get resources: %d\n", ret);
+
+	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);
+
+	ret = phy_pipe_clk_register(phy, generic_phy->id);
+	if (ret)
+		dev_err(&pdev->dev, "failed to register phy pipe clk\n");
+
+	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",
+		.of_match_table = qcom_uniphy_pcie_id_table,
+	},
+};
+
+module_platform_driver(qcom_uniphy_pcie_driver);
+
+MODULE_DESCRIPTION("PCIE QCOM UNIPHY driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v5 2/5] phy: qcom: Introduce PCIe UNIPHY 28LP driver

[Dmitry Baryshkov]

On Thu, Jan 02, 2025 at 05:00:16PM +0530, Varadarajan Narayanan wrote:
> From: Nitheesh Sekar <quic_nsekar@quicinc.com>
> 
> Add Qualcomm PCIe UNIPHY 28LP driver support present
> in Qualcomm IPQ5332 SoC and the phy init sequence.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> v5: * Use 'num-lanes' to differentiate instead of '3x1' or '3x2'
>       in compatible string
>     * Drop compatible specific init data as there is only one
>       compatible string
>     * Fix header file order
> 
> v4: Fix uppercase hex digit
>     Use phy->id for pipe clock source
> 
> v3: Added 'Reviewed-by: Dmitry Baryshkov' and made following updates
>     s/unsigned int/u32/g
>     Fix 'lane_offset' comments
>     Fix #define tab -> space
>     Fix mixed case hex numbers
>     Fix licensing & owner
>     Change for-loop pointer to use [] instead of ->
>     Use 'less than max' instead of 'not equal to max' in termination condition
>     Smatch and Coccinelle passed
> 
> v2: Drop IPQ5018 related code and data
>     Use uniform prefix for struct names
>     Place "}, {", on the same line
>     In qcom_uniphy_pcie_init(), use for-loop instead of while
>     Swap reset and clock disable order in qcom_uniphy_pcie_power_off
>     Add reset assert to qcom_uniphy_pcie_power_on's error path
>     Use macros for usleep duration
>     Inlined qcom_uniphy_pcie_get_resources & use devm_platform_get_and_ioremap_resource
>     Drop 'clock-output-names' from phy_pipe_clk_register
> ---
>  drivers/phy/qualcomm/Kconfig                  |  12 +
>  drivers/phy/qualcomm/Makefile                 |   1 +
>  .../phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c  | 287 ++++++++++++++++++
>  3 files changed, 300 insertions(+)
>  create mode 100644 drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c
> 

[...]

> +
> +static int qcom_uniphy_pcie_probe(struct platform_device *pdev)
> +{
> +	struct phy_provider *phy_provider;
> +	struct device *dev = &pdev->dev;
> +	struct qcom_uniphy_pcie *phy;
> +	struct phy *generic_phy;
> +	int ret;
> +
> +	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 = of_property_read_u32(of_node_get(dev->of_node), "num-lanes",

Who will put the reference count which you have just got?

> +				   &phy->lanes);
> +	if (ret)
> +		phy->lanes = 1;

phy->lanes = 1;
of_property_read_u32(np, "num-lanes", &phy->lanes);

> +
> +	ret = qcom_uniphy_pcie_get_resources(pdev, phy);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to get resources: %d\n", ret);
> +
> +	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);
> +
> +	ret = phy_pipe_clk_register(phy, generic_phy->id);
> +	if (ret)
> +		dev_err(&pdev->dev, "failed to register phy pipe clk\n");
> +
> +	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",
> +		.of_match_table = qcom_uniphy_pcie_id_table,
> +	},
> +};
> +
> +module_platform_driver(qcom_uniphy_pcie_driver);
> +
> +MODULE_DESCRIPTION("PCIE QCOM UNIPHY driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
> 
> 
> -- 
> linux-phy mailing list
> linux-phy@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/linux-phy

-- 
With best wishes
Dmitry

Re: [PATCH v5 2/5] phy: qcom: Introduce PCIe UNIPHY 28LP driver

[Krzysztof Kozlowski]

On Fri, Jan 03, 2025 at 07:48:10AM +0200, Dmitry Baryshkov wrote:
> > +	platform_set_drvdata(pdev, phy);
> > +	phy->dev = &pdev->dev;
> > +
> > +	phy->data = of_device_get_match_data(dev);
> > +	if (!phy->data)
> > +		return -EINVAL;
> > +
> > +	ret = of_property_read_u32(of_node_get(dev->of_node), "num-lanes",
> 
> Who will put the reference count which you have just got?

More important: why introducing own pattern of code...

Best regards,
Krzysztof

[PATCH v5 3/5] dt-bindings: PCI: qcom: Document the IPQ5332 PCIe controller

[Varadarajan Narayanan]

Document the PCIe controller on IPQ5332 platform.

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
v5: Re-arrange 5332 and 9574 compatibles to handle fallback usage in dts

v4: * v3 reused ipq9574 bindings for ipq5332. Instead add one for ipq5332
    * DTS uses ipq9574 compatible as fallback. Hence move ipq9574 to be able
      to use the 'reg' section for both ipq5332 and ipq9574. Else, dtbs_check
      and dt_binding_check flag errors.
---
 Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
index bd87f6b49d68..9f37eca1ce0d 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
@@ -26,7 +26,6 @@ properties:
           - qcom,pcie-ipq8064-v2
           - qcom,pcie-ipq8074
           - qcom,pcie-ipq8074-gen3
-          - qcom,pcie-ipq9574
           - qcom,pcie-msm8996
           - qcom,pcie-qcs404
           - qcom,pcie-sdm845
@@ -34,6 +33,10 @@ properties:
       - items:
           - const: qcom,pcie-msm8998
           - const: qcom,pcie-msm8996
+      - items:
+          - enum:
+              - qcom,pcie-ipq5332
+          - const: qcom,pcie-ipq9574
 
   reg:
     minItems: 4
@@ -165,7 +168,6 @@ allOf:
             enum:
               - qcom,pcie-ipq6018
               - qcom,pcie-ipq8074-gen3
-              - qcom,pcie-ipq9574
     then:
       properties:
         reg:
@@ -206,6 +208,8 @@ allOf:
         compatible:
           contains:
             enum:
+              - qcom,pcie-ipq5332
+              - qcom,pcie-ipq9574
               - qcom,pcie-sdx55
     then:
       properties:
@@ -407,6 +411,7 @@ allOf:
         compatible:
           contains:
             enum:
+              - qcom,pcie-ipq5332
               - qcom,pcie-ipq9574
     then:
       properties:
@@ -555,6 +560,7 @@ allOf:
               enum:
                 - qcom,pcie-apq8064
                 - qcom,pcie-ipq4019
+                - qcom,pcie-ipq5332
                 - qcom,pcie-ipq8064
                 - qcom,pcie-ipq8064v2
                 - qcom,pcie-ipq8074
-- 
2.34.1

Re: [PATCH v5 3/5] dt-bindings: PCI: qcom: Document the IPQ5332 PCIe controller

[Krzysztof Kozlowski]

On Thu, Jan 02, 2025 at 05:00:17PM +0530, Varadarajan Narayanan wrote:
> Document the PCIe controller on IPQ5332 platform.
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> v5: Re-arrange 5332 and 9574 compatibles to handle fallback usage in dts

What? How this is related to commit msg?

> 
> v4: * v3 reused ipq9574 bindings for ipq5332. Instead add one for ipq5332
>     * DTS uses ipq9574 compatible as fallback. Hence move ipq9574 to be able
>       to use the 'reg' section for both ipq5332 and ipq9574. Else, dtbs_check
>       and dt_binding_check flag errors.
> ---
>  Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> index bd87f6b49d68..9f37eca1ce0d 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> @@ -26,7 +26,6 @@ properties:
>            - qcom,pcie-ipq8064-v2
>            - qcom,pcie-ipq8074
>            - qcom,pcie-ipq8074-gen3
> -          - qcom,pcie-ipq9574

I don't understand this change at all and your commit msg explains
here nothing.

Best regards,
Krzysztof

Re: [PATCH v5 3/5] dt-bindings: PCI: qcom: Document the IPQ5332 PCIe controller

[Varadarajan Narayanan]

On Fri, Jan 03, 2025 at 08:45:14AM +0100, Krzysztof Kozlowski wrote:
> On Thu, Jan 02, 2025 at 05:00:17PM +0530, Varadarajan Narayanan wrote:
> > Document the PCIe controller on IPQ5332 platform.
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> > v5: Re-arrange 5332 and 9574 compatibles to handle fallback usage in dts
>
> What? How this is related to commit msg?
>
> >
> > v4: * v3 reused ipq9574 bindings for ipq5332. Instead add one for ipq5332
> >     * DTS uses ipq9574 compatible as fallback. Hence move ipq9574 to be able
> >       to use the 'reg' section for both ipq5332 and ipq9574. Else, dtbs_check
> >       and dt_binding_check flag errors.
> > ---
> >  Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > index bd87f6b49d68..9f37eca1ce0d 100644
> > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > @@ -26,7 +26,6 @@ properties:
> >            - qcom,pcie-ipq8064-v2
> >            - qcom,pcie-ipq8074
> >            - qcom,pcie-ipq8074-gen3
> > -          - qcom,pcie-ipq9574
>
> I don't understand this change at all and your commit msg explains
> here nothing.

All DT entries except "reg" is similar between ipq5332 and
ipq9574. ipq9574 has 5 registers while ipq5332 has 6. MHI is the
additional (i.e. sixth) entry for ipq5332.

If ipq9574 is not removed from here, dt_binding_check gives the
following errors

1.	/local/mnt/workspace/varada/upstream/pci-v6/arch/arm64/boot/dts/qcom/ipq5332-rdp474.dtb: pcie@18000000: reg: [[557056, 12288], [402653184, 3869], [402657056, 168], [402657280, 4096], [403701760, 4096], [569344, 4096]] is too long

	Failed validating 'maxItems' in schema['allOf'][2]['then']['properties']['reg']:
	    {'maxItems': 5, 'minItems': 5}

2.	/local/mnt/workspace/varada/upstream/pci-v6/arch/arm64/boot/dts/qcom/ipq5332-rdp474.dtb: pcie@18000000: reg-names: ['parf', 'dbi', 'elbi', 'atu', 'config', 'mhi'] is too long

	Failed validating 'maxItems' in schema['allOf'][2]['then']['properties']['reg-names']:
	    {'items': [{'const': 'dbi'},
		       {'const': 'elbi'},
		       {'const': 'atu'},
		       {'const': 'parf'},
		       {'const': 'config'}],
	     'maxItems': 5,
	     'minItems': 5,
	     'type': 'array'}

Hence had to remove it from here and add it to the sdx55 reg
definition.

Will capture this in the commit message.

Thanks
Varada

Re: [PATCH v5 3/5] dt-bindings: PCI: qcom: Document the IPQ5332 PCIe controller

[Krzysztof Kozlowski]

On 07/01/2025 12:05, Varadarajan Narayanan wrote:
>>> ---
>>>  Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>> index bd87f6b49d68..9f37eca1ce0d 100644
>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>> @@ -26,7 +26,6 @@ properties:
>>>            - qcom,pcie-ipq8064-v2
>>>            - qcom,pcie-ipq8074
>>>            - qcom,pcie-ipq8074-gen3
>>> -          - qcom,pcie-ipq9574
>>
>> I don't understand this change at all and your commit msg explains
>> here nothing.
> 
> All DT entries except "reg" is similar between ipq5332 and
> ipq9574. ipq9574 has 5 registers while ipq5332 has 6. MHI is the
> additional (i.e. sixth) entry for ipq5332.
> 
> If ipq9574 is not removed from here, dt_binding_check gives the
> following errors
> 
> 1.	/local/mnt/workspace/varada/upstream/pci-v6/arch/arm64/boot/dts/qcom/ipq5332-rdp474.dtb: pcie@18000000: reg: [[557056, 12288], [402653184, 3869], [402657056, 168], [402657280, 4096], [403701760, 4096], [569344, 4096]] is too long
> 
> 	Failed validating 'maxItems' in schema['allOf'][2]['then']['properties']['reg']:
> 	    {'maxItems': 5, 'minItems': 5}
> 
> 2.	/local/mnt/workspace/varada/upstream/pci-v6/arch/arm64/boot/dts/qcom/ipq5332-rdp474.dtb: pcie@18000000: reg-names: ['parf', 'dbi', 'elbi', 'atu', 'config', 'mhi'] is too long
> 
> 	Failed validating 'maxItems' in schema['allOf'][2]['then']['properties']['reg-names']:
> 	    {'items': [{'const': 'dbi'},
> 		       {'const': 'elbi'},
> 		       {'const': 'atu'},
> 		       {'const': 'parf'},
> 		       {'const': 'config'}],
> 	     'maxItems': 5,
> 	     'minItems': 5,
> 	     'type': 'array'}
> 
> Hence had to remove it from here and add it to the sdx55 reg
> definition.

So you entirely dropped constrain for regs. No. This has to be fixed,
not dropped.
Best regards,
Krzysztof

Re: [PATCH v5 3/5] dt-bindings: PCI: qcom: Document the IPQ5332 PCIe controller

[Varadarajan Narayanan]

On Wed, Jan 08, 2025 at 08:19:19AM +0100, Krzysztof Kozlowski wrote:
> On 07/01/2025 12:05, Varadarajan Narayanan wrote:
> >>> ---
> >>>  Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 10 ++++++++--
> >>>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >>> index bd87f6b49d68..9f37eca1ce0d 100644
> >>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >>> @@ -26,7 +26,6 @@ properties:
> >>>            - qcom,pcie-ipq8064-v2
> >>>            - qcom,pcie-ipq8074
> >>>            - qcom,pcie-ipq8074-gen3
> >>> -          - qcom,pcie-ipq9574
> >>
> >> I don't understand this change at all and your commit msg explains
> >> here nothing.
> >
> > All DT entries except "reg" is similar between ipq5332 and
> > ipq9574. ipq9574 has 5 registers while ipq5332 has 6. MHI is the
> > additional (i.e. sixth) entry for ipq5332.
> >
> > If ipq9574 is not removed from here, dt_binding_check gives the
> > following errors
> >
> > 1.	/local/mnt/workspace/varada/upstream/pci-v6/arch/arm64/boot/dts/qcom/ipq5332-rdp474.dtb: pcie@18000000: reg: [[557056, 12288], [402653184, 3869], [402657056, 168], [402657280, 4096], [403701760, 4096], [569344, 4096]] is too long
> >
> > 	Failed validating 'maxItems' in schema['allOf'][2]['then']['properties']['reg']:
> > 	    {'maxItems': 5, 'minItems': 5}
> >
> > 2.	/local/mnt/workspace/varada/upstream/pci-v6/arch/arm64/boot/dts/qcom/ipq5332-rdp474.dtb: pcie@18000000: reg-names: ['parf', 'dbi', 'elbi', 'atu', 'config', 'mhi'] is too long
> >
> > 	Failed validating 'maxItems' in schema['allOf'][2]['then']['properties']['reg-names']:
> > 	    {'items': [{'const': 'dbi'},
> > 		       {'const': 'elbi'},
> > 		       {'const': 'atu'},
> > 		       {'const': 'parf'},
> > 		       {'const': 'config'}],
> > 	     'maxItems': 5,
> > 	     'minItems': 5,
> > 	     'type': 'array'}
> >
> > Hence had to remove it from here and add it to the sdx55 reg
> > definition.
>
> So you entirely dropped constrain for regs. No. This has to be fixed,
> not dropped.

ipq9574 is not dropped entirely. It is clubbed with sdx55's
constraints. Please see this

	@@ -206,6 +208,8 @@ allOf:
		 compatible:
		   contains:
		     enum:
	+              - qcom,pcie-ipq5332
	+              - qcom,pcie-ipq9574
		       - qcom,pcie-sdx55
	     then:
	       properties:

Thanks
Varada

Re: [PATCH v5 3/5] dt-bindings: PCI: qcom: Document the IPQ5332 PCIe controller

[Krzysztof Kozlowski]

On 08/01/2025 08:40, Varadarajan Narayanan wrote:
> On Wed, Jan 08, 2025 at 08:19:19AM +0100, Krzysztof Kozlowski wrote:
>> On 07/01/2025 12:05, Varadarajan Narayanan wrote:
>>>>> ---
>>>>>  Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 10 ++++++++--
>>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>> index bd87f6b49d68..9f37eca1ce0d 100644
>>>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>> @@ -26,7 +26,6 @@ properties:
>>>>>            - qcom,pcie-ipq8064-v2
>>>>>            - qcom,pcie-ipq8074
>>>>>            - qcom,pcie-ipq8074-gen3
>>>>> -          - qcom,pcie-ipq9574
>>>>
>>>> I don't understand this change at all and your commit msg explains
>>>> here nothing.
>>>
>>> All DT entries except "reg" is similar between ipq5332 and
>>> ipq9574. ipq9574 has 5 registers while ipq5332 has 6. MHI is the
>>> additional (i.e. sixth) entry for ipq5332.
>>>
>>> If ipq9574 is not removed from here, dt_binding_check gives the
>>> following errors
>>>
>>> 1.	/local/mnt/workspace/varada/upstream/pci-v6/arch/arm64/boot/dts/qcom/ipq5332-rdp474.dtb: pcie@18000000: reg: [[557056, 12288], [402653184, 3869], [402657056, 168], [402657280, 4096], [403701760, 4096], [569344, 4096]] is too long
>>>
>>> 	Failed validating 'maxItems' in schema['allOf'][2]['then']['properties']['reg']:
>>> 	    {'maxItems': 5, 'minItems': 5}
>>>
>>> 2.	/local/mnt/workspace/varada/upstream/pci-v6/arch/arm64/boot/dts/qcom/ipq5332-rdp474.dtb: pcie@18000000: reg-names: ['parf', 'dbi', 'elbi', 'atu', 'config', 'mhi'] is too long
>>>
>>> 	Failed validating 'maxItems' in schema['allOf'][2]['then']['properties']['reg-names']:
>>> 	    {'items': [{'const': 'dbi'},
>>> 		       {'const': 'elbi'},
>>> 		       {'const': 'atu'},
>>> 		       {'const': 'parf'},
>>> 		       {'const': 'config'}],
>>> 	     'maxItems': 5,
>>> 	     'minItems': 5,
>>> 	     'type': 'array'}
>>>
>>> Hence had to remove it from here and add it to the sdx55 reg
>>> definition.
>>
>> So you entirely dropped constrain for regs. No. This has to be fixed,
>> not dropped.
> 
> ipq9574 is not dropped entirely. It is clubbed with sdx55's
> constraints. Please see this
> 
> 	@@ -206,6 +208,8 @@ allOf:
> 		 compatible:
> 		   contains:
> 		     enum:
> 	+              - qcom,pcie-ipq5332
> 	+              - qcom,pcie-ipq9574
> 		       - qcom,pcie-sdx55
Correct, not dropped entirely, but now it receives mhi for no reason.
This should be separate commit with its own explanation - why ipq9574
has now MHI address space.

Best regards,
Krzysztof

[PATCH v5 4/5] arm64: dts: qcom: ipq5332: Add PCIe related nodes

[Varadarajan Narayanan]

From: Praveenkumar I <quic_ipkumar@quicinc.com>

Add phy and controller nodes for pcie0_x1 and pcie1_x2.

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
v5: Add 'num-lanes' to "pcie1_phy: phy@4b1000"
    Make ipq5332 as main and ipq9574 as fallback compatible
    Move controller nodes per address
    Having Konrad's Reviewed-By

v4: Remove 'reset-names' as driver uses bulk APIs
    Remove 'clock-output-names' as driver uses bulk APIs
    Add missing reset for pcie1_phy
    Convert 'reg-names' to a vertical list
    Move 'msi-map' before interrupts

v3: Fix compatible string for phy nodes
    Use ipq9574 as backup compatible instead of new compatible for ipq5332
    Fix mixed case hex addresses
    Add "mhi" space
    Removed unnecessary comments and stray blank lines

v2: Fix nodes' location per address
---
 arch/arm64/boot/dts/qcom/ipq5332.dtsi | 221 +++++++++++++++++++++++++-
 1 file changed, 219 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
index d3c3e215a15c..89daf955e4bd 100644
--- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
@@ -186,6 +186,43 @@ rng: rng@e3000 {
 			clock-names = "core";
 		};
 
+		pcie0_phy: phy@4b0000 {
+			compatible = "qcom,ipq5332-uniphy-pcie-phy";
+			reg = <0x004b0000 0x800>;
+
+			clocks = <&gcc GCC_PCIE3X1_0_PIPE_CLK>,
+				 <&gcc GCC_PCIE3X1_PHY_AHB_CLK>;
+
+			resets = <&gcc GCC_PCIE3X1_0_PHY_BCR>,
+				 <&gcc GCC_PCIE3X1_PHY_AHB_CLK_ARES>,
+				 <&gcc GCC_PCIE3X1_0_PHY_PHY_BCR>;
+
+			#clock-cells = <0>;
+
+			#phy-cells = <0>;
+			status = "disabled";
+		};
+
+		pcie1_phy: phy@4b1000 {
+			compatible = "qcom,ipq5332-uniphy-pcie-phy";
+			reg = <0x004b1000 0x1000>;
+
+			clocks = <&gcc GCC_PCIE3X2_PIPE_CLK>,
+				 <&gcc GCC_PCIE3X2_PHY_AHB_CLK>;
+
+			resets = <&gcc GCC_PCIE3X2_PHY_BCR>,
+				 <&gcc GCC_PCIE3X2_PHY_AHB_CLK_ARES>,
+				 <&gcc GCC_PCIE3X2PHY_PHY_BCR>;
+
+			#clock-cells = <0>;
+
+			#phy-cells = <0>;
+
+			num-lanes = <2>;
+
+			status = "disabled";
+		};
+
 		tlmm: pinctrl@1000000 {
 			compatible = "qcom,ipq5332-tlmm";
 			reg = <0x01000000 0x300000>;
@@ -212,8 +249,8 @@ gcc: clock-controller@1800000 {
 			#interconnect-cells = <1>;
 			clocks = <&xo_board>,
 				 <&sleep_clk>,
-				 <0>,
-				 <0>,
+				 <&pcie1_phy>,
+				 <&pcie0_phy>,
 				 <0>;
 		};
 
@@ -479,6 +516,186 @@ frame@b128000 {
 				status = "disabled";
 			};
 		};
+
+		pcie1: pcie@18000000 {
+			compatible = "qcom,pcie-ipq5332", "qcom,pcie-ipq9574";
+			reg = <0x00088000 0x3000>,
+			      <0x18000000 0xf1d>,
+			      <0x18000f20 0xa8>,
+			      <0x18001000 0x1000>,
+			      <0x18100000 0x1000>,
+			      <0x0008b000 0x1000>;
+			reg-names = "parf",
+				    "dbi",
+				    "elbi",
+				    "atu",
+				    "config",
+				    "mhi";
+			device_type = "pci";
+			linux,pci-domain = <1>;
+			bus-range = <0x00 0xff>;
+			num-lanes = <2>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			ranges = <0x01000000 0 0x18200000 0x18200000 0 0x00100000>,
+				 <0x02000000 0 0x18300000 0x18300000 0 0x07d00000>;
+
+			msi-map = <0x0 &v2m0 0x0 0xffd>;
+
+			interrupts = <GIC_SPI 403 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 404 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 406 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 407 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 408 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 409 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 410 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi0",
+					  "msi1",
+					  "msi2",
+					  "msi3",
+					  "msi4",
+					  "msi5",
+					  "msi6",
+					  "msi7";
+
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0x7>;
+			interrupt-map = <0 0 0 1 &intc 0 0 412 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 2 &intc 0 0 413 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 3 &intc 0 0 414 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 4 &intc 0 0 415 IRQ_TYPE_LEVEL_HIGH>;
+
+			clocks = <&gcc GCC_PCIE3X2_AXI_M_CLK>,
+				 <&gcc GCC_PCIE3X2_AXI_S_CLK>,
+				 <&gcc GCC_PCIE3X2_AXI_S_BRIDGE_CLK>,
+				 <&gcc GCC_PCIE3X2_RCHG_CLK>,
+				 <&gcc GCC_PCIE3X2_AHB_CLK>,
+				 <&gcc GCC_PCIE3X2_AUX_CLK>;
+			clock-names = "axi_m",
+				      "axi_s",
+				      "axi_bridge",
+				      "rchng",
+				      "ahb",
+				      "aux";
+
+			resets = <&gcc GCC_PCIE3X2_PIPE_ARES>,
+				 <&gcc GCC_PCIE3X2_CORE_STICKY_ARES>,
+				 <&gcc GCC_PCIE3X2_AXI_S_STICKY_ARES>,
+				 <&gcc GCC_PCIE3X2_AXI_S_CLK_ARES>,
+				 <&gcc GCC_PCIE3X2_AXI_M_STICKY_ARES>,
+				 <&gcc GCC_PCIE3X2_AXI_M_CLK_ARES>,
+				 <&gcc GCC_PCIE3X2_AUX_CLK_ARES>,
+				 <&gcc GCC_PCIE3X2_AHB_CLK_ARES>;
+			reset-names = "pipe",
+				      "sticky",
+				      "axi_s_sticky",
+				      "axi_s",
+				      "axi_m_sticky",
+				      "axi_m",
+				      "aux",
+				      "ahb";
+
+			phys = <&pcie1_phy>;
+			phy-names = "pciephy";
+
+			interconnects = <&gcc MASTER_SNOC_PCIE3_2_M &gcc SLAVE_SNOC_PCIE3_2_M>,
+					<&gcc MASTER_ANOC_PCIE3_2_S &gcc SLAVE_ANOC_PCIE3_2_S>;
+			interconnect-names = "pcie-mem", "cpu-pcie";
+
+			status = "disabled";
+		};
+
+		pcie0: pcie@20000000 {
+			compatible = "qcom,pcie-ipq5332", "qcom,pcie-ipq9574";
+			reg = <0x00080000 0x3000>,
+			      <0x20000000 0xf1d>,
+			      <0x20000f20 0xa8>,
+			      <0x20001000 0x1000>,
+			      <0x20100000 0x1000>,
+			      <0x00083000 0x1000>;
+			reg-names = "parf",
+				    "dbi",
+				    "elbi",
+				    "atu",
+				    "config",
+				    "mhi";
+			device_type = "pci";
+			linux,pci-domain = <0>;
+			bus-range = <0x00 0xff>;
+			num-lanes = <1>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			ranges = <0x01000000 0 0x20200000 0x20200000 0 0x00100000>,
+				 <0x02000000 0 0x20300000 0x20300000 0 0x0fd00000>;
+
+			msi-map = <0x0 &v2m0 0x0 0xffd>;
+
+			interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi0",
+					  "msi1",
+					  "msi2",
+					  "msi3",
+					  "msi4",
+					  "msi5",
+					  "msi6",
+					  "msi7";
+
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0x7>;
+			interrupt-map = <0 0 0 1 &intc 0 0 35 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 2 &intc 0 0 36 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 3 &intc 0 0 37 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 4 &intc 0 0 38 IRQ_TYPE_LEVEL_HIGH>;
+
+			clocks = <&gcc GCC_PCIE3X1_0_AXI_M_CLK>,
+				 <&gcc GCC_PCIE3X1_0_AXI_S_CLK>,
+				 <&gcc GCC_PCIE3X1_0_AXI_S_BRIDGE_CLK>,
+				 <&gcc GCC_PCIE3X1_0_RCHG_CLK>,
+				 <&gcc GCC_PCIE3X1_0_AHB_CLK>,
+				 <&gcc GCC_PCIE3X1_0_AUX_CLK>;
+			clock-names = "axi_m",
+				      "axi_s",
+				      "axi_bridge",
+				      "rchng",
+				      "ahb",
+				      "aux";
+
+			resets = <&gcc GCC_PCIE3X1_0_PIPE_ARES>,
+				 <&gcc GCC_PCIE3X1_0_CORE_STICKY_ARES>,
+				 <&gcc GCC_PCIE3X1_0_AXI_S_STICKY_ARES>,
+				 <&gcc GCC_PCIE3X1_0_AXI_S_CLK_ARES>,
+				 <&gcc GCC_PCIE3X1_0_AXI_M_STICKY_ARES>,
+				 <&gcc GCC_PCIE3X1_0_AXI_M_CLK_ARES>,
+				 <&gcc GCC_PCIE3X1_0_AUX_CLK_ARES>,
+				 <&gcc GCC_PCIE3X1_0_AHB_CLK_ARES>;
+			reset-names = "pipe",
+				      "sticky",
+				      "axi_s_sticky",
+				      "axi_s",
+				      "axi_m_sticky",
+				      "axi_m",
+				      "aux",
+				      "ahb";
+
+			phys = <&pcie0_phy>;
+			phy-names = "pciephy";
+
+			interconnects = <&gcc MASTER_SNOC_PCIE3_1_M &gcc SLAVE_SNOC_PCIE3_1_M>,
+					<&gcc MASTER_ANOC_PCIE3_1_S &gcc SLAVE_ANOC_PCIE3_1_S>;
+			interconnect-names = "pcie-mem", "cpu-pcie";
+
+			status = "disabled";
+		};
 	};
 
 	timer {
-- 
2.34.1

Re: [PATCH v5 4/5] arm64: dts: qcom: ipq5332: Add PCIe related nodes

[Manivannan Sadhasivam]

On Thu, Jan 02, 2025 at 05:00:18PM +0530, Varadarajan Narayanan wrote:
> From: Praveenkumar I <quic_ipkumar@quicinc.com>
> 
> Add phy and controller nodes for pcie0_x1 and pcie1_x2.
> 
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> v5: Add 'num-lanes' to "pcie1_phy: phy@4b1000"
>     Make ipq5332 as main and ipq9574 as fallback compatible
>     Move controller nodes per address
>     Having Konrad's Reviewed-By
> 
> v4: Remove 'reset-names' as driver uses bulk APIs
>     Remove 'clock-output-names' as driver uses bulk APIs
>     Add missing reset for pcie1_phy
>     Convert 'reg-names' to a vertical list
>     Move 'msi-map' before interrupts
> 
> v3: Fix compatible string for phy nodes
>     Use ipq9574 as backup compatible instead of new compatible for ipq5332
>     Fix mixed case hex addresses
>     Add "mhi" space
>     Removed unnecessary comments and stray blank lines
> 
> v2: Fix nodes' location per address
> ---
>  arch/arm64/boot/dts/qcom/ipq5332.dtsi | 221 +++++++++++++++++++++++++-
>  1 file changed, 219 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> index d3c3e215a15c..89daf955e4bd 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> @@ -186,6 +186,43 @@ rng: rng@e3000 {
>  			clock-names = "core";
>  		};
>  
> +		pcie0_phy: phy@4b0000 {
> +			compatible = "qcom,ipq5332-uniphy-pcie-phy";
> +			reg = <0x004b0000 0x800>;
> +
> +			clocks = <&gcc GCC_PCIE3X1_0_PIPE_CLK>,
> +				 <&gcc GCC_PCIE3X1_PHY_AHB_CLK>;
> +
> +			resets = <&gcc GCC_PCIE3X1_0_PHY_BCR>,
> +				 <&gcc GCC_PCIE3X1_PHY_AHB_CLK_ARES>,
> +				 <&gcc GCC_PCIE3X1_0_PHY_PHY_BCR>;
> +
> +			#clock-cells = <0>;
> +
> +			#phy-cells = <0>;
> +			status = "disabled";
> +		};
> +
> +		pcie1_phy: phy@4b1000 {
> +			compatible = "qcom,ipq5332-uniphy-pcie-phy";
> +			reg = <0x004b1000 0x1000>;
> +
> +			clocks = <&gcc GCC_PCIE3X2_PIPE_CLK>,
> +				 <&gcc GCC_PCIE3X2_PHY_AHB_CLK>;
> +
> +			resets = <&gcc GCC_PCIE3X2_PHY_BCR>,
> +				 <&gcc GCC_PCIE3X2_PHY_AHB_CLK_ARES>,
> +				 <&gcc GCC_PCIE3X2PHY_PHY_BCR>;
> +
> +			#clock-cells = <0>;
> +
> +			#phy-cells = <0>;
> +
> +			num-lanes = <2>;
> +
> +			status = "disabled";
> +		};
> +
>  		tlmm: pinctrl@1000000 {
>  			compatible = "qcom,ipq5332-tlmm";
>  			reg = <0x01000000 0x300000>;
> @@ -212,8 +249,8 @@ gcc: clock-controller@1800000 {
>  			#interconnect-cells = <1>;
>  			clocks = <&xo_board>,
>  				 <&sleep_clk>,
> -				 <0>,
> -				 <0>,
> +				 <&pcie1_phy>,
> +				 <&pcie0_phy>,
>  				 <0>;
>  		};
>  
> @@ -479,6 +516,186 @@ frame@b128000 {
>  				status = "disabled";
>  			};
>  		};
> +
> +		pcie1: pcie@18000000 {

pcie@

> +			compatible = "qcom,pcie-ipq5332", "qcom,pcie-ipq9574";
> +			reg = <0x00088000 0x3000>,
> +			      <0x18000000 0xf1d>,
> +			      <0x18000f20 0xa8>,
> +			      <0x18001000 0x1000>,
> +			      <0x18100000 0x1000>,
> +			      <0x0008b000 0x1000>;
> +			reg-names = "parf",
> +				    "dbi",
> +				    "elbi",
> +				    "atu",
> +				    "config",
> +				    "mhi";
> +			device_type = "pci";
> +			linux,pci-domain = <1>;
> +			bus-range = <0x00 0xff>;
> +			num-lanes = <2>;
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +
> +			ranges = <0x01000000 0 0x18200000 0x18200000 0 0x00100000>,

I/O address space should start from 0. Please refer other SoCs.

Also, use 0x0 for consistency.

> +				 <0x02000000 0 0x18300000 0x18300000 0 0x07d00000>;
> +
> +			msi-map = <0x0 &v2m0 0x0 0xffd>;
> +
> +			interrupts = <GIC_SPI 403 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 404 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 406 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 407 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 408 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 409 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 410 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "msi0",
> +					  "msi1",
> +					  "msi2",
> +					  "msi3",
> +					  "msi4",
> +					  "msi5",
> +					  "msi6",
> +					  "msi7";

Is there a 'global' interrupt? If so, please add it.

> +
> +			#interrupt-cells = <1>;
> +			interrupt-map-mask = <0 0 0 0x7>;
> +			interrupt-map = <0 0 0 1 &intc 0 0 412 IRQ_TYPE_LEVEL_HIGH>,
> +					<0 0 0 2 &intc 0 0 413 IRQ_TYPE_LEVEL_HIGH>,
> +					<0 0 0 3 &intc 0 0 414 IRQ_TYPE_LEVEL_HIGH>,
> +					<0 0 0 4 &intc 0 0 415 IRQ_TYPE_LEVEL_HIGH>;
> +
> +			clocks = <&gcc GCC_PCIE3X2_AXI_M_CLK>,
> +				 <&gcc GCC_PCIE3X2_AXI_S_CLK>,
> +				 <&gcc GCC_PCIE3X2_AXI_S_BRIDGE_CLK>,
> +				 <&gcc GCC_PCIE3X2_RCHG_CLK>,
> +				 <&gcc GCC_PCIE3X2_AHB_CLK>,
> +				 <&gcc GCC_PCIE3X2_AUX_CLK>;
> +			clock-names = "axi_m",
> +				      "axi_s",
> +				      "axi_bridge",
> +				      "rchng",
> +				      "ahb",
> +				      "aux";
> +
> +			resets = <&gcc GCC_PCIE3X2_PIPE_ARES>,
> +				 <&gcc GCC_PCIE3X2_CORE_STICKY_ARES>,
> +				 <&gcc GCC_PCIE3X2_AXI_S_STICKY_ARES>,
> +				 <&gcc GCC_PCIE3X2_AXI_S_CLK_ARES>,
> +				 <&gcc GCC_PCIE3X2_AXI_M_STICKY_ARES>,
> +				 <&gcc GCC_PCIE3X2_AXI_M_CLK_ARES>,
> +				 <&gcc GCC_PCIE3X2_AUX_CLK_ARES>,
> +				 <&gcc GCC_PCIE3X2_AHB_CLK_ARES>;
> +			reset-names = "pipe",
> +				      "sticky",
> +				      "axi_s_sticky",
> +				      "axi_s",
> +				      "axi_m_sticky",
> +				      "axi_m",
> +				      "aux",
> +				      "ahb";
> +
> +			phys = <&pcie1_phy>;
> +			phy-names = "pciephy";
> +
> +			interconnects = <&gcc MASTER_SNOC_PCIE3_2_M &gcc SLAVE_SNOC_PCIE3_2_M>,
> +					<&gcc MASTER_ANOC_PCIE3_2_S &gcc SLAVE_ANOC_PCIE3_2_S>;
> +			interconnect-names = "pcie-mem", "cpu-pcie";

Can you check if the controller supports cache coherency? If so, you need to add
'dma-coherent'.

> +
> +			status = "disabled";

Please define the root port node as well.

All the above comments applies to 2nd controller node as well.

- Mani

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

Re: [PATCH v5 4/5] arm64: dts: qcom: ipq5332: Add PCIe related nodes

[Varadarajan Narayanan]

On Wed, Jan 08, 2025 at 06:52:35PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jan 02, 2025 at 05:00:18PM +0530, Varadarajan Narayanan wrote:
> > From: Praveenkumar I <quic_ipkumar@quicinc.com>
> >
> > Add phy and controller nodes for pcie0_x1 and pcie1_x2.
> >
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> > v5: Add 'num-lanes' to "pcie1_phy: phy@4b1000"
> >     Make ipq5332 as main and ipq9574 as fallback compatible
> >     Move controller nodes per address
> >     Having Konrad's Reviewed-By
> >
> > v4: Remove 'reset-names' as driver uses bulk APIs
> >     Remove 'clock-output-names' as driver uses bulk APIs
> >     Add missing reset for pcie1_phy
> >     Convert 'reg-names' to a vertical list
> >     Move 'msi-map' before interrupts
> >
> > v3: Fix compatible string for phy nodes
> >     Use ipq9574 as backup compatible instead of new compatible for ipq5332
> >     Fix mixed case hex addresses
> >     Add "mhi" space
> >     Removed unnecessary comments and stray blank lines
> >
> > v2: Fix nodes' location per address
> > ---
> >  arch/arm64/boot/dts/qcom/ipq5332.dtsi | 221 +++++++++++++++++++++++++-
> >  1 file changed, 219 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> > index d3c3e215a15c..89daf955e4bd 100644
> > --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> > @@ -186,6 +186,43 @@ rng: rng@e3000 {
> >  			clock-names = "core";
> >  		};
> >
> > +		pcie0_phy: phy@4b0000 {
> > +			compatible = "qcom,ipq5332-uniphy-pcie-phy";
> > +			reg = <0x004b0000 0x800>;
> > +
> > +			clocks = <&gcc GCC_PCIE3X1_0_PIPE_CLK>,
> > +				 <&gcc GCC_PCIE3X1_PHY_AHB_CLK>;
> > +
> > +			resets = <&gcc GCC_PCIE3X1_0_PHY_BCR>,
> > +				 <&gcc GCC_PCIE3X1_PHY_AHB_CLK_ARES>,
> > +				 <&gcc GCC_PCIE3X1_0_PHY_PHY_BCR>;
> > +
> > +			#clock-cells = <0>;
> > +
> > +			#phy-cells = <0>;
> > +			status = "disabled";
> > +		};
> > +
> > +		pcie1_phy: phy@4b1000 {
> > +			compatible = "qcom,ipq5332-uniphy-pcie-phy";
> > +			reg = <0x004b1000 0x1000>;
> > +
> > +			clocks = <&gcc GCC_PCIE3X2_PIPE_CLK>,
> > +				 <&gcc GCC_PCIE3X2_PHY_AHB_CLK>;
> > +
> > +			resets = <&gcc GCC_PCIE3X2_PHY_BCR>,
> > +				 <&gcc GCC_PCIE3X2_PHY_AHB_CLK_ARES>,
> > +				 <&gcc GCC_PCIE3X2PHY_PHY_BCR>;
> > +
> > +			#clock-cells = <0>;
> > +
> > +			#phy-cells = <0>;
> > +
> > +			num-lanes = <2>;
> > +
> > +			status = "disabled";
> > +		};
> > +
> >  		tlmm: pinctrl@1000000 {
> >  			compatible = "qcom,ipq5332-tlmm";
> >  			reg = <0x01000000 0x300000>;
> > @@ -212,8 +249,8 @@ gcc: clock-controller@1800000 {
> >  			#interconnect-cells = <1>;
> >  			clocks = <&xo_board>,
> >  				 <&sleep_clk>,
> > -				 <0>,
> > -				 <0>,
> > +				 <&pcie1_phy>,
> > +				 <&pcie0_phy>,
> >  				 <0>;
> >  		};
> >
> > @@ -479,6 +516,186 @@ frame@b128000 {
> >  				status = "disabled";
> >  			};
> >  		};
> > +
> > +		pcie1: pcie@18000000 {
>
> pcie@

Not able to understand. Can you please let me know if you want
the label 'pcie1' to be dropped? This label is used in the board
dts to enable this node, so cannot drop.

> > +			compatible = "qcom,pcie-ipq5332", "qcom,pcie-ipq9574";
> > +			reg = <0x00088000 0x3000>,
> > +			      <0x18000000 0xf1d>,
> > +			      <0x18000f20 0xa8>,
> > +			      <0x18001000 0x1000>,
> > +			      <0x18100000 0x1000>,
> > +			      <0x0008b000 0x1000>;
> > +			reg-names = "parf",
> > +				    "dbi",
> > +				    "elbi",
> > +				    "atu",
> > +				    "config",
> > +				    "mhi";
> > +			device_type = "pci";
> > +			linux,pci-domain = <1>;
> > +			bus-range = <0x00 0xff>;
> > +			num-lanes = <2>;
> > +			#address-cells = <3>;
> > +			#size-cells = <2>;
> > +
> > +			ranges = <0x01000000 0 0x18200000 0x18200000 0 0x00100000>,
>
> I/O address space should start from 0. Please refer other SoCs.
>
> Also, use 0x0 for consistency.

Ok.

> > +				 <0x02000000 0 0x18300000 0x18300000 0 0x07d00000>;
> > +
> > +			msi-map = <0x0 &v2m0 0x0 0xffd>;
> > +
> > +			interrupts = <GIC_SPI 403 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 404 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 406 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 407 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 408 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 409 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 410 IRQ_TYPE_LEVEL_HIGH>;
> > +			interrupt-names = "msi0",
> > +					  "msi1",
> > +					  "msi2",
> > +					  "msi3",
> > +					  "msi4",
> > +					  "msi5",
> > +					  "msi6",
> > +					  "msi7";
>
> Is there a 'global' interrupt? If so, please add it.

Ok.

> > +
> > +			#interrupt-cells = <1>;
> > +			interrupt-map-mask = <0 0 0 0x7>;
> > +			interrupt-map = <0 0 0 1 &intc 0 0 412 IRQ_TYPE_LEVEL_HIGH>,
> > +					<0 0 0 2 &intc 0 0 413 IRQ_TYPE_LEVEL_HIGH>,
> > +					<0 0 0 3 &intc 0 0 414 IRQ_TYPE_LEVEL_HIGH>,
> > +					<0 0 0 4 &intc 0 0 415 IRQ_TYPE_LEVEL_HIGH>;
> > +
> > +			clocks = <&gcc GCC_PCIE3X2_AXI_M_CLK>,
> > +				 <&gcc GCC_PCIE3X2_AXI_S_CLK>,
> > +				 <&gcc GCC_PCIE3X2_AXI_S_BRIDGE_CLK>,
> > +				 <&gcc GCC_PCIE3X2_RCHG_CLK>,
> > +				 <&gcc GCC_PCIE3X2_AHB_CLK>,
> > +				 <&gcc GCC_PCIE3X2_AUX_CLK>;
> > +			clock-names = "axi_m",
> > +				      "axi_s",
> > +				      "axi_bridge",
> > +				      "rchng",
> > +				      "ahb",
> > +				      "aux";
> > +
> > +			resets = <&gcc GCC_PCIE3X2_PIPE_ARES>,
> > +				 <&gcc GCC_PCIE3X2_CORE_STICKY_ARES>,
> > +				 <&gcc GCC_PCIE3X2_AXI_S_STICKY_ARES>,
> > +				 <&gcc GCC_PCIE3X2_AXI_S_CLK_ARES>,
> > +				 <&gcc GCC_PCIE3X2_AXI_M_STICKY_ARES>,
> > +				 <&gcc GCC_PCIE3X2_AXI_M_CLK_ARES>,
> > +				 <&gcc GCC_PCIE3X2_AUX_CLK_ARES>,
> > +				 <&gcc GCC_PCIE3X2_AHB_CLK_ARES>;
> > +			reset-names = "pipe",
> > +				      "sticky",
> > +				      "axi_s_sticky",
> > +				      "axi_s",
> > +				      "axi_m_sticky",
> > +				      "axi_m",
> > +				      "aux",
> > +				      "ahb";
> > +
> > +			phys = <&pcie1_phy>;
> > +			phy-names = "pciephy";
> > +
> > +			interconnects = <&gcc MASTER_SNOC_PCIE3_2_M &gcc SLAVE_SNOC_PCIE3_2_M>,
> > +					<&gcc MASTER_ANOC_PCIE3_2_S &gcc SLAVE_ANOC_PCIE3_2_S>;
> > +			interconnect-names = "pcie-mem", "cpu-pcie";
>
> Can you check if the controller supports cache coherency? If so, you need to add
> 'dma-coherent'.

Ok.

> > +
> > +			status = "disabled";
>
> Please define the root port node as well.
>
> All the above comments applies to 2nd controller node as well.

Ok.

Thanks
Varada

Re: [PATCH v5 4/5] arm64: dts: qcom: ipq5332: Add PCIe related nodes

[Varadarajan Narayanan]

Manivannan,

	[ . . . ]
> > > +			phys = <&pcie1_phy>;
> > > +			phy-names = "pciephy";
> > > +
> > > +			interconnects = <&gcc MASTER_SNOC_PCIE3_2_M &gcc SLAVE_SNOC_PCIE3_2_M>,
> > > +					<&gcc MASTER_ANOC_PCIE3_2_S &gcc SLAVE_ANOC_PCIE3_2_S>;
> > > +			interconnect-names = "pcie-mem", "cpu-pcie";
> >
> > Can you check if the controller supports cache coherency? If so, you need to add
> > 'dma-coherent'.
>
> Ok.

Confirmed with h/w person. The controller doesn't support cache coherance.

Thanks
Varada

Re: [PATCH v5 4/5] arm64: dts: qcom: ipq5332: Add PCIe related nodes

[Bjorn Helgaas]

On Thu, Jan 02, 2025 at 05:00:18PM +0530, Varadarajan Narayanan wrote:
> From: Praveenkumar I <quic_ipkumar@quicinc.com>
> 
> Add phy and controller nodes for pcie0_x1 and pcie1_x2.

> +		pcie1: pcie@18000000 {
> +			compatible = "qcom,pcie-ipq5332", "qcom,pcie-ipq9574";
> +			reg = <0x00088000 0x3000>,
> +			      <0x18000000 0xf1d>,
> +			      <0x18000f20 0xa8>,
> +			      <0x18001000 0x1000>,
> +			      <0x18100000 0x1000>,
> +			      <0x0008b000 0x1000>;
> +			reg-names = "parf",
> +				    "dbi",
> +				    "elbi",
> +				    "atu",
> +				    "config",
> +				    "mhi";
> +			device_type = "pci";
> +			linux,pci-domain = <1>;
> +			bus-range = <0x00 0xff>;

This bus-range isn't needed, is it?  pci_parse_request_of_pci_ranges()
should default to 0x00-0xff if no bus-range property is present.

> +			num-lanes = <2>;
> +			phys = <&pcie1_phy>;
> +			phy-names = "pciephy";

I think num-lanes and PHY info are per-Root Port properties, not a
host controller properties, aren't they?  Some of the clock and reset
properties might also be per-Root Port.

Ideally, I think per-Root Port properties should be in a child device
as they are here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/mvebu-pci.txt?id=v6.12#n137
but it looks like the num-lanes parsing is done in
dw_pcie_get_resources(), which can only handle a single num-lanes per
DWC controller, so maybe it's impractical to add a child device here.

But I wonder if it would be useful to at least group the per-Root Port
things together in the binding to help us start thinking about the
difference between the controller and the Root Port(s).

Bjorn

Re: [PATCH v5 4/5] arm64: dts: qcom: ipq5332: Add PCIe related nodes

[Varadarajan Narayanan]

On Wed, Jan 08, 2025 at 12:32:35PM -0600, Bjorn Helgaas wrote:
> On Thu, Jan 02, 2025 at 05:00:18PM +0530, Varadarajan Narayanan wrote:
> > From: Praveenkumar I <quic_ipkumar@quicinc.com>
> >
> > Add phy and controller nodes for pcie0_x1 and pcie1_x2.
>
> > +		pcie1: pcie@18000000 {
> > +			compatible = "qcom,pcie-ipq5332", "qcom,pcie-ipq9574";
> > +			reg = <0x00088000 0x3000>,
> > +			      <0x18000000 0xf1d>,
> > +			      <0x18000f20 0xa8>,
> > +			      <0x18001000 0x1000>,
> > +			      <0x18100000 0x1000>,
> > +			      <0x0008b000 0x1000>;
> > +			reg-names = "parf",
> > +				    "dbi",
> > +				    "elbi",
> > +				    "atu",
> > +				    "config",
> > +				    "mhi";
> > +			device_type = "pci";
> > +			linux,pci-domain = <1>;
> > +			bus-range = <0x00 0xff>;
>
> This bus-range isn't needed, is it?  pci_parse_request_of_pci_ranges()
> should default to 0x00-0xff if no bus-range property is present.

Ok.

>
> > +			num-lanes = <2>;
> > +			phys = <&pcie1_phy>;
> > +			phy-names = "pciephy";
>
> I think num-lanes and PHY info are per-Root Port properties, not a
> host controller properties, aren't they?  Some of the clock and reset
> properties might also be per-Root Port.
>
> Ideally, I think per-Root Port properties should be in a child device
> as they are here:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/mvebu-pci.txt?id=v6.12#n137
> but it looks like the num-lanes parsing is done in
> dw_pcie_get_resources(), which can only handle a single num-lanes per
> DWC controller, so maybe it's impractical to add a child device here.
>
> But I wonder if it would be useful to at least group the per-Root Port
> things together in the binding to help us start thinking about the
> difference between the controller and the Root Port(s).

This looks like a big change and might impact the existing
SoCs/platforms. To minimize the impact, should we continue
supporting the legacy method in addition to the new per-Root port
approach. Should we take this up separately? Kindly advice.

Thanks
Varada

Re: [PATCH v5 4/5] arm64: dts: qcom: ipq5332: Add PCIe related nodes

[Bjorn Helgaas]

On Wed, Jan 15, 2025 at 01:28:22PM +0530, Varadarajan Narayanan wrote:
> On Wed, Jan 08, 2025 at 12:32:35PM -0600, Bjorn Helgaas wrote:
> > On Thu, Jan 02, 2025 at 05:00:18PM +0530, Varadarajan Narayanan wrote:
> > > From: Praveenkumar I <quic_ipkumar@quicinc.com>
> > >
> > > Add phy and controller nodes for pcie0_x1 and pcie1_x2.

> > > +		pcie1: pcie@18000000 {
> > > +			compatible = "qcom,pcie-ipq5332", "qcom,pcie-ipq9574";
> > > +			reg = <0x00088000 0x3000>,
> > > +			      <0x18000000 0xf1d>,
> > > +			      <0x18000f20 0xa8>,
> > > +			      <0x18001000 0x1000>,
> > > +			      <0x18100000 0x1000>,
> > > +			      <0x0008b000 0x1000>;
> > > +			reg-names = "parf",
> > > +				    "dbi",
> > > +				    "elbi",
> > > +				    "atu",
> > > +				    "config",
> > > +				    "mhi";
> > > +			device_type = "pci";
> > > +			linux,pci-domain = <1>;
> > > +			bus-range = <0x00 0xff>;

> > > +			num-lanes = <2>;
> > > +			phys = <&pcie1_phy>;
> > > +			phy-names = "pciephy";
> >
> > I think num-lanes and PHY info are per-Root Port properties, not a
> > host controller properties, aren't they?  Some of the clock and reset
> > properties might also be per-Root Port.
> >
> > Ideally, I think per-Root Port properties should be in a child device
> > as they are here:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/mvebu-pci.txt?id=v6.12#n137
> > but it looks like the num-lanes parsing is done in
> > dw_pcie_get_resources(), which can only handle a single num-lanes per
> > DWC controller, so maybe it's impractical to add a child device here.
> >
> > But I wonder if it would be useful to at least group the per-Root Port
> > things together in the binding to help us start thinking about the
> > difference between the controller and the Root Port(s).
> 
> This looks like a big change and might impact the existing
> SoCs/platforms. To minimize the impact, should we continue
> supporting the legacy method in addition to the new per-Root port
> approach. Should we take this up separately? Kindly advice.

I just meant to change the order they're listed in the binding, not
add any new device stanzas.

E.g., maybe it could be arranged like this, where things that apply to
the Root Complex as a whole (bus-range, #address-cells, #size-cells,
ranges, etc) are listed first, and the Root Port-related things
(num-lanes, phys, etc) are listed later:

+               pcie1: pcie@18000000 {
+                       compatible = "qcom,pcie-ipq5332", "qcom,pcie-ipq9574";
+                       device_type = "pci";
+                       linux,pci-domain = <1>;
+                       bus-range = <0x00 0xff>;
+                       #address-cells = <3>;
+                       #size-cells = <2>;
+
+                       ranges = <0x01000000 0 0x18200000 0x18200000 0 0x00100000>,
+                                <0x02000000 0 0x18300000 0x18300000 0 0x07d00000>;
+                       interrupts = <GIC_SPI 403 IRQ_TYPE_LEVEL_HIGH>, ...
+                       clocks = <&gcc GCC_PCIE3X2_AXI_M_CLK>, ...
+                       resets = <&gcc GCC_PCIE3X2_PIPE_ARES>, ...
+                       interconnects = <&gcc MASTER_SNOC_PCIE3_2_M ...

Everything above is potentially shared; everything below applies to a
single (the only one in this case) Root Port.

+                       num-lanes = <2>;
+                       phys = <&pcie1_phy>;
+
+                       status = "disabled";
+               };

I want people to think about which things belong to the Root Port and
which are shared for the whole Root Complex.

For new drivers, I think we should actually add "pcie@1,0" stanzas for
each Root Port, even if there is only one.

But for existing drivers that would have to be modified to comprehend
new stanzas, collecting the Root Port things so they are together in
the PCI controller stanza would be a good start (unless the order of
properties in the DT makes a functional difference, of course).

Bjorn

[PATCH v5 5/5] arm64: dts: qcom: ipq5332-rdp441: Enable PCIe phys and controllers

[Varadarajan Narayanan]

From: Praveenkumar I <quic_ipkumar@quicinc.com>

Enable the PCIe controller and PHY nodes for RDP 441.

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
v5: Add 'Reviewed-by: Konrad Dybcio'

v4: Fix nodes sort order
    Use property-n followed by property-names

v3: Reorder nodes alphabetically
    Fix commit subject
---
 arch/arm64/boot/dts/qcom/ipq5332-rdp441.dts | 76 +++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5332-rdp441.dts b/arch/arm64/boot/dts/qcom/ipq5332-rdp441.dts
index 846413817e9a..79ec77cfe552 100644
--- a/arch/arm64/boot/dts/qcom/ipq5332-rdp441.dts
+++ b/arch/arm64/boot/dts/qcom/ipq5332-rdp441.dts
@@ -32,6 +32,34 @@ &sdhc {
 	status = "okay";
 };
 
+&pcie0 {
+	pinctrl-0 = <&pcie0_default>;
+	pinctrl-names = "default";
+
+	perst-gpios = <&tlmm 38 GPIO_ACTIVE_LOW>;
+	wake-gpios = <&tlmm 39 GPIO_ACTIVE_LOW>;
+
+	status = "okay";
+};
+
+&pcie0_phy {
+	status = "okay";
+};
+
+&pcie1 {
+	pinctrl-0 = <&pcie1_default>;
+	pinctrl-names = "default";
+
+	perst-gpios = <&tlmm 47 GPIO_ACTIVE_LOW>;
+	wake-gpios = <&tlmm 48 GPIO_ACTIVE_LOW>;
+
+	status = "okay";
+};
+
+&pcie1_phy {
+	status = "okay";
+};
+
 &tlmm {
 	i2c_1_pins: i2c-1-state {
 		pins = "gpio29", "gpio30";
@@ -40,6 +68,54 @@ i2c_1_pins: i2c-1-state {
 		bias-pull-up;
 	};
 
+	pcie0_default: pcie0-default-state {
+		clkreq-n-pins {
+			pins = "gpio37";
+			function = "pcie0_clk";
+			drive-strength = <8>;
+			bias-pull-up;
+		};
+
+		perst-n-pins {
+			pins = "gpio38";
+			function = "gpio";
+			drive-strength = <8>;
+			bias-pull-up;
+			output-low;
+		};
+
+		wake-n-pins {
+			pins = "gpio39";
+			function = "pcie0_wake";
+			drive-strength = <8>;
+			bias-pull-up;
+		};
+	};
+
+	pcie1_default: pcie1-default-state {
+		clkreq-n-pins {
+			pins = "gpio46";
+			function = "pcie1_clk";
+			drive-strength = <8>;
+			bias-pull-up;
+		};
+
+		perst-n-pins {
+			pins = "gpio47";
+			function = "gpio";
+			drive-strength = <8>;
+			bias-pull-up;
+			output-low;
+		};
+
+		wake-n-pins {
+			pins = "gpio48";
+			function = "pcie1_wake";
+			drive-strength = <8>;
+			bias-pull-up;
+		};
+	};
+
 	sdc_default_state: sdc-default-state {
 		clk-pins {
 			pins = "gpio13";
-- 
2.34.1