[PATCH 0/7] PCI: apple: support t6020

[PATCH 0/7] PCI: apple: support t6020

[Alyssa Rosenzweig]

This series adds T6020 support to the Apple PCIe controller. Mostly
Apple shuffled registers around (presumably to accommodate the larger
configurations on those machines). So there's a bit of churn here but
not too much in the way of functional changes.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
Alyssa Rosenzweig (1):
      dt-bindings: pci: apple,pcie: Add t6020 support

Hector Martin (5):
      PCI: apple: Fix missing OF node reference in apple_pcie_setup_port
      PCI: apple: Move port PHY registers to their own reg items
      PCI: apple: Drop poll for CORE_RC_PHYIF_STAT_REFCLK
      PCI: apple: Use gpiod_set_value_cansleep in probe flow
      PCI: apple: Add T602x PCIe support

Janne Grunau (1):
      PCI: apple: Set only available ports up

 .../devicetree/bindings/pci/apple,pcie.yaml        |   1 +
 drivers/pci/controller/pcie-apple.c                | 189 ++++++++++++++++-----
 2 files changed, 146 insertions(+), 44 deletions(-)
---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20250211-pcie-t6-3f4898ce9b1d

Best regards,
-- 
Alyssa Rosenzweig <alyssa@rosenzweig.io>

[PATCH 1/7] dt-bindings: pci: apple,pcie: Add t6020 support

[Alyssa Rosenzweig]

This shuffles some registers versus t6000, so requires a new compatible.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 Documentation/devicetree/bindings/pci/apple,pcie.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
index c8775f9cb071339dfa6f17f9f4a99f031c98b70a..4885788ff623eebe6a79bccc0f8a9a231a01c5ca 100644
--- a/Documentation/devicetree/bindings/pci/apple,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
@@ -35,6 +35,7 @@ properties:
           - apple,t8103-pcie
           - apple,t8112-pcie
           - apple,t6000-pcie
+          - apple,t6020-pcie
       - const: apple,pcie
 
   reg:

-- 
2.48.1

Re: [PATCH 1/7] dt-bindings: pci: apple,pcie: Add t6020 support

[Krzysztof Kozlowski]

On Tue, Feb 11, 2025 at 02:54:26PM -0500, Alyssa Rosenzweig wrote:
> This shuffles some registers versus t6000, so requires a new compatible.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> ---
>  Documentation/devicetree/bindings/pci/apple,pcie.yaml | 1 +
>  1 file changed, 1 insertion(+)

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

Best regards,
Krzysztof

[PATCH 2/7] PCI: apple: Fix missing OF node reference in apple_pcie_setup_port

[Alyssa Rosenzweig]

From: Hector Martin <marcan@marcan.st>

In the success path we hang onto a reference to the node, so make sure
to grab one. The caller iterator puts our borrowed reference when we
return.

Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 drivers/pci/controller/pcie-apple.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index a7e51bc1c2fe8ec31902816e9be6749b756ec77d..8ea3e258fe2768a33ec56f0a8a86d168ed615973 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -584,6 +584,9 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 	list_add_tail(&port->entry, &pcie->ports);
 	init_completion(&pcie->event);
 
+	/* In the success path, we keep a reference to np around */
+	of_node_get(np);
+
 	ret = apple_pcie_port_register_irqs(port);
 	WARN_ON(ret);
 

-- 
2.48.1

[PATCH 3/7] PCI: apple: Set only available ports up

[Alyssa Rosenzweig]

From: Janne Grunau <j@jannau.net>

Fixes "interrupt-map" parsing in of_irq_parse_raw() which takes the
node's availability into account.

This became apparent after disabling unused PCIe ports in the Apple
silicon device trees instead of disabling them.

Link: https://lore.kernel.org/asahi/20230214-apple_dts_pcie_disable_unused-v1-0-5ea0d3ddcde3@jannau.net/
Link: https://lore.kernel.org/asahi/1ea2107a-bb86-8c22-0bbc-82c453ab08ce@linaro.org/
Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
Cc: stable@vger.kernel.org
Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 drivers/pci/controller/pcie-apple.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 8ea3e258fe2768a33ec56f0a8a86d168ed615973..958cf459d4c64dffa1f993e57b7a58cfb2199b8f 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -758,7 +758,7 @@ static int apple_pcie_init(struct pci_config_window *cfg)
 	if (ret)
 		return ret;
 
-	for_each_child_of_node(dev->of_node, of_port) {
+	for_each_available_child_of_node(dev->of_node, of_port) {
 		ret = apple_pcie_setup_port(pcie, of_port);
 		if (ret) {
 			dev_err(pcie->dev, "Port %pOF setup fail: %d\n", of_port, ret);

-- 
2.48.1

Re: [PATCH 3/7] PCI: apple: Set only available ports up

[Bjorn Helgaas]

On Tue, Feb 11, 2025 at 02:54:28PM -0500, Alyssa Rosenzweig wrote:
> From: Janne Grunau <j@jannau.net>
> 
> Fixes "interrupt-map" parsing in of_irq_parse_raw() which takes the
> node's availability into account.
> 
> This became apparent after disabling unused PCIe ports in the Apple
> silicon device trees instead of disabling them.

Is there something missing from this sentence?  "... after disabling
unused ports instead of disabling them" doesn't sound quite complete.

> Link: https://lore.kernel.org/asahi/20230214-apple_dts_pcie_disable_unused-v1-0-5ea0d3ddcde3@jannau.net/
> Link: https://lore.kernel.org/asahi/1ea2107a-bb86-8c22-0bbc-82c453ab08ce@linaro.org/
> Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> Cc: stable@vger.kernel.org

Can we have a hint about what makes this "stable" material?  I can't
tell from the commit log what the impact of this change is.

Bjorn

[PATCH 4/7] PCI: apple: Move port PHY registers to their own reg items

[Alyssa Rosenzweig]

From: Hector Martin <marcan@marcan.st>

T602x PCIe cores move these registers around. Instead of hardcoding in
another offset, let's move them into their own reg entries. This matches
what Apple does on macOS device trees too.

Maintains backwards compatibility with old DTs by using the old offsets.

Note that we open code devm_platform_ioremap_resource_byname() to avoid
error messages on older platforms with missing resources in the pcie
node. ("pcie-apple 590000000.pcie: invalid resource (null)" on probe)

Co-developed-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 drivers/pci/controller/pcie-apple.c | 55 +++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 958cf459d4c64dffa1f993e57b7a58cfb2199b8f..806eeb911bbd4f1ae5832b34f775fa18c866670b 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -39,14 +39,18 @@
 #define   CORE_RC_STAT_READY		BIT(0)
 #define CORE_FABRIC_STAT		0x04000
 #define   CORE_FABRIC_STAT_MASK		0x001F001F
-#define CORE_LANE_CFG(port)		(0x84000 + 0x4000 * (port))
-#define   CORE_LANE_CFG_REFCLK0REQ	BIT(0)
-#define   CORE_LANE_CFG_REFCLK1REQ	BIT(1)
-#define   CORE_LANE_CFG_REFCLK0ACK	BIT(2)
-#define   CORE_LANE_CFG_REFCLK1ACK	BIT(3)
-#define   CORE_LANE_CFG_REFCLKEN	(BIT(9) | BIT(10))
-#define CORE_LANE_CTL(port)		(0x84004 + 0x4000 * (port))
-#define   CORE_LANE_CTL_CFGACC		BIT(15)
+
+#define CORE_PHY_DEFAULT_BASE(port)	(0x84000 + 0x4000 * (port))
+
+#define PHY_LANE_CFG			0x00000
+#define   PHY_LANE_CFG_REFCLK0REQ	BIT(0)
+#define   PHY_LANE_CFG_REFCLK1REQ	BIT(1)
+#define   PHY_LANE_CFG_REFCLK0ACK	BIT(2)
+#define   PHY_LANE_CFG_REFCLK1ACK	BIT(3)
+#define   PHY_LANE_CFG_REFCLKEN		(BIT(9) | BIT(10))
+#define   PHY_LANE_CFG_REFCLKCGEN	(BIT(30) | BIT(31))
+#define PHY_LANE_CTL			0x00004
+#define   PHY_LANE_CTL_CFGACC		BIT(15)
 
 #define PORT_LTSSMCTL			0x00080
 #define   PORT_LTSSMCTL_START		BIT(0)
@@ -145,6 +149,7 @@ struct apple_pcie_port {
 	struct apple_pcie	*pcie;
 	struct device_node	*np;
 	void __iomem		*base;
+	void __iomem		*phy;
 	struct irq_domain	*domain;
 	struct list_head	entry;
 	DECLARE_BITMAP(sid_map, MAX_RID2SID);
@@ -473,26 +478,26 @@ static int apple_pcie_setup_refclk(struct apple_pcie *pcie,
 	if (res < 0)
 		return res;
 
-	rmw_set(CORE_LANE_CTL_CFGACC, pcie->base + CORE_LANE_CTL(port->idx));
-	rmw_set(CORE_LANE_CFG_REFCLK0REQ, pcie->base + CORE_LANE_CFG(port->idx));
+	rmw_set(PHY_LANE_CTL_CFGACC, port->phy + PHY_LANE_CTL);
+	rmw_set(PHY_LANE_CFG_REFCLK0REQ, port->phy + PHY_LANE_CFG);
 
-	res = readl_relaxed_poll_timeout(pcie->base + CORE_LANE_CFG(port->idx),
-					 stat, stat & CORE_LANE_CFG_REFCLK0ACK,
+	res = readl_relaxed_poll_timeout(port->phy + PHY_LANE_CFG,
+					 stat, stat & PHY_LANE_CFG_REFCLK0ACK,
 					 100, 50000);
 	if (res < 0)
 		return res;
 
-	rmw_set(CORE_LANE_CFG_REFCLK1REQ, pcie->base + CORE_LANE_CFG(port->idx));
-	res = readl_relaxed_poll_timeout(pcie->base + CORE_LANE_CFG(port->idx),
-					 stat, stat & CORE_LANE_CFG_REFCLK1ACK,
+	rmw_set(PHY_LANE_CFG_REFCLK1REQ, port->phy + PHY_LANE_CFG);
+	res = readl_relaxed_poll_timeout(port->phy + PHY_LANE_CFG,
+					 stat, stat & PHY_LANE_CFG_REFCLK1ACK,
 					 100, 50000);
 
 	if (res < 0)
 		return res;
 
-	rmw_clear(CORE_LANE_CTL_CFGACC, pcie->base + CORE_LANE_CTL(port->idx));
+	rmw_clear(PHY_LANE_CTL_CFGACC, port->phy + PHY_LANE_CTL);
 
-	rmw_set(CORE_LANE_CFG_REFCLKEN, pcie->base + CORE_LANE_CFG(port->idx));
+	rmw_set(PHY_LANE_CFG_REFCLKEN, port->phy + PHY_LANE_CFG);
 	rmw_set(PORT_REFCLK_EN, port->base + PORT_REFCLK);
 
 	return 0;
@@ -512,8 +517,10 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 	struct platform_device *platform = to_platform_device(pcie->dev);
 	struct apple_pcie_port *port;
 	struct gpio_desc *reset;
+	struct resource *res;
 	u32 stat, idx;
 	int ret, i;
+	char name[16];
 
 	reset = devm_fwnode_gpiod_get(pcie->dev, of_fwnode_handle(np), "reset",
 				      GPIOD_OUT_LOW, "PERST#");
@@ -533,10 +540,22 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 	port->pcie = pcie;
 	port->np = np;
 
-	port->base = devm_platform_ioremap_resource(platform, port->idx + 2);
+	snprintf(name, sizeof(name), "port%d", port->idx);
+	res = platform_get_resource_byname(platform, IORESOURCE_MEM, name);
+	if (res)
+		port->base = devm_ioremap_resource(&platform->dev, res);
+	else
+		port->base = devm_platform_ioremap_resource(platform, port->idx + 2);
 	if (IS_ERR(port->base))
 		return PTR_ERR(port->base);
 
+	snprintf(name, sizeof(name), "phy%d", port->idx);
+	res = platform_get_resource_byname(platform, IORESOURCE_MEM, name);
+	if (res)
+		port->phy = devm_ioremap_resource(&platform->dev, res);
+	else
+		port->phy = pcie->base + CORE_PHY_DEFAULT_BASE(port->idx);
+
 	rmw_set(PORT_APPCLK_EN, port->base + PORT_APPCLK);
 
 	/* Assert PERST# before setting up the clock */

-- 
2.48.1

[PATCH 5/7] PCI: apple: Drop poll for CORE_RC_PHYIF_STAT_REFCLK

[Alyssa Rosenzweig]

From: Hector Martin <marcan@marcan.st>

This is checking a core refclk in per-port setup which doesn't make a
lot of sense, and the bootloader needs to have gone through this anyway.

It doesn't work on T602x, so just drop it across the board.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 drivers/pci/controller/pcie-apple.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 806eeb911bbd4f1ae5832b34f775fa18c866670b..209535db7855fa1ee0d75290b33525dce18f560d 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -472,12 +472,6 @@ static int apple_pcie_setup_refclk(struct apple_pcie *pcie,
 	u32 stat;
 	int res;
 
-	res = readl_relaxed_poll_timeout(pcie->base + CORE_RC_PHYIF_STAT, stat,
-					 stat & CORE_RC_PHYIF_STAT_REFCLK,
-					 100, 50000);
-	if (res < 0)
-		return res;
-
 	rmw_set(PHY_LANE_CTL_CFGACC, port->phy + PHY_LANE_CTL);
 	rmw_set(PHY_LANE_CFG_REFCLK0REQ, port->phy + PHY_LANE_CFG);
 

-- 
2.48.1

[PATCH 6/7] PCI: apple: Use gpiod_set_value_cansleep in probe flow

[Alyssa Rosenzweig]

From: Hector Martin <marcan@marcan.st>

We're allowed to sleep here, so tell the GPIO core by using
gpiod_set_value_cansleep instead of gpiod_set_value.

Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 drivers/pci/controller/pcie-apple.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 209535db7855fa1ee0d75290b33525dce18f560d..7f4839fb0a5b15a9ca87337f53c14a1ce08301fc 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -553,7 +553,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 	rmw_set(PORT_APPCLK_EN, port->base + PORT_APPCLK);
 
 	/* Assert PERST# before setting up the clock */
-	gpiod_set_value(reset, 1);
+	gpiod_set_value_cansleep(reset, 1);
 
 	ret = apple_pcie_setup_refclk(pcie, port);
 	if (ret < 0)
@@ -564,7 +564,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 
 	/* Deassert PERST# */
 	rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
-	gpiod_set_value(reset, 0);
+	gpiod_set_value_cansleep(reset, 0);
 
 	/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
 	msleep(100);

-- 
2.48.1

[PATCH 7/7] PCI: apple: Add T602x PCIe support

[Alyssa Rosenzweig]

From: Hector Martin <marcan@marcan.st>

This version of the hardware moved around a bunch of registers, so we
drop the old compatible for these and introduce register offset
structures to handle the differences.

Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 drivers/pci/controller/pcie-apple.c | 125 ++++++++++++++++++++++++++++++------
 1 file changed, 105 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 7f4839fb0a5b15a9ca87337f53c14a1ce08301fc..7c598334427cb56ca066890ac61143ae1d3ed744 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -26,6 +26,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/msi.h>
+#include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/pci-ecam.h>
 
@@ -104,7 +105,7 @@
 #define   PORT_REFCLK_CGDIS		BIT(8)
 #define PORT_PERST			0x00814
 #define   PORT_PERST_OFF		BIT(0)
-#define PORT_RID2SID(i16)		(0x00828 + 4 * (i16))
+#define PORT_RID2SID			0x00828
 #define   PORT_RID2SID_VALID		BIT(31)
 #define   PORT_RID2SID_SID_SHIFT	16
 #define   PORT_RID2SID_BUS_SHIFT	8
@@ -122,7 +123,7 @@
 #define   PORT_TUNSTAT_PERST_ACK_PEND	BIT(1)
 #define PORT_PREFMEM_ENABLE		0x00994
 
-#define MAX_RID2SID			64
+#define MAX_RID2SID			512
 
 /*
  * The doorbell address is set to 0xfffff000, which by convention
@@ -133,6 +134,57 @@
  */
 #define DOORBELL_ADDR		CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR
 
+struct reg_info {
+	u32 phy_lane_ctl;
+	u32 port_msiaddr;
+	u32 port_msiaddr_hi;
+	u32 port_refclk;
+	u32 port_perst;
+	u32 port_rid2sid;
+	u32 port_msimap;
+	u32 max_rid2sid;
+	u32 max_msimap;
+};
+
+const struct reg_info t8103_hw = {
+	.phy_lane_ctl = PHY_LANE_CTL,
+	.port_msiaddr = PORT_MSIADDR,
+	.port_msiaddr_hi = 0,
+	.port_refclk = PORT_REFCLK,
+	.port_perst = PORT_PERST,
+	.port_rid2sid = PORT_RID2SID,
+	.port_msimap = 0,
+	.max_rid2sid = 64,
+	.max_msimap = 0,
+};
+
+#define PORT_T602X_MSIADDR		0x016c
+#define PORT_T602X_MSIADDR_HI		0x0170
+#define PORT_T602X_PERST		0x082c
+#define PORT_T602X_RID2SID		0x3000
+#define PORT_T602X_MSIMAP		0x3800
+
+#define PORT_MSIMAP_ENABLE		BIT(31)
+#define PORT_MSIMAP_TARGET		GENMASK(7, 0)
+
+const struct reg_info t602x_hw = {
+	.phy_lane_ctl = 0,
+	.port_msiaddr = PORT_T602X_MSIADDR,
+	.port_msiaddr_hi = PORT_T602X_MSIADDR_HI,
+	.port_refclk = 0,
+	.port_perst = PORT_T602X_PERST,
+	.port_rid2sid = PORT_T602X_RID2SID,
+	.port_msimap = PORT_T602X_MSIMAP,
+	.max_rid2sid = 512, /* 16 on t602x, guess for autodetect on future HW */
+	.max_msimap = 512, /* 96 on t602x, guess for autodetect on future HW */
+};
+
+static const struct of_device_id apple_pcie_of_match_hw[] = {
+	{ .compatible = "apple,t6020-pcie", .data = &t602x_hw },
+	{ .compatible = "apple,pcie", .data = &t8103_hw },
+	{ }
+};
+
 struct apple_pcie {
 	struct mutex		lock;
 	struct device		*dev;
@@ -143,6 +195,7 @@ struct apple_pcie {
 	struct completion	event;
 	struct irq_fwspec	fwspec;
 	u32			nvecs;
+	const struct reg_info	*hw;
 };
 
 struct apple_pcie_port {
@@ -266,14 +319,14 @@ static void apple_port_irq_mask(struct irq_data *data)
 {
 	struct apple_pcie_port *port = irq_data_get_irq_chip_data(data);
 
-	writel_relaxed(BIT(data->hwirq), port->base + PORT_INTMSKSET);
+	rmw_set(BIT(data->hwirq), port->base + PORT_INTMSK);
 }
 
 static void apple_port_irq_unmask(struct irq_data *data)
 {
 	struct apple_pcie_port *port = irq_data_get_irq_chip_data(data);
 
-	writel_relaxed(BIT(data->hwirq), port->base + PORT_INTMSKCLR);
+	rmw_clear(BIT(data->hwirq), port->base + PORT_INTMSK);
 }
 
 static bool hwirq_is_intx(unsigned int hwirq)
@@ -377,6 +430,7 @@ static void apple_port_irq_handler(struct irq_desc *desc)
 static int apple_pcie_port_setup_irq(struct apple_pcie_port *port)
 {
 	struct fwnode_handle *fwnode = &port->np->fwnode;
+	struct apple_pcie *pcie = port->pcie;
 	unsigned int irq;
 
 	/* FIXME: consider moving each interrupt under each port */
@@ -392,19 +446,35 @@ static int apple_pcie_port_setup_irq(struct apple_pcie_port *port)
 		return -ENOMEM;
 
 	/* Disable all interrupts */
-	writel_relaxed(~0, port->base + PORT_INTMSKSET);
+	writel_relaxed(~0, port->base + PORT_INTMSK);
 	writel_relaxed(~0, port->base + PORT_INTSTAT);
+	writel_relaxed(~0, port->base + PORT_LINKCMDSTS);
 
 	irq_set_chained_handler_and_data(irq, apple_port_irq_handler, port);
 
 	/* Configure MSI base address */
 	BUILD_BUG_ON(upper_32_bits(DOORBELL_ADDR));
-	writel_relaxed(lower_32_bits(DOORBELL_ADDR), port->base + PORT_MSIADDR);
+	writel_relaxed(lower_32_bits(DOORBELL_ADDR),
+		       port->base + pcie->hw->port_msiaddr);
+	if (pcie->hw->port_msiaddr_hi)
+		writel_relaxed(0, port->base + pcie->hw->port_msiaddr_hi);
 
 	/* Enable MSIs, shared between all ports */
-	writel_relaxed(0, port->base + PORT_MSIBASE);
-	writel_relaxed((ilog2(port->pcie->nvecs) << PORT_MSICFG_L2MSINUM_SHIFT) |
-		       PORT_MSICFG_EN, port->base + PORT_MSICFG);
+	if (pcie->hw->port_msimap) {
+		int i;
+
+		for (i = 0; i < pcie->nvecs; i++) {
+			writel_relaxed(FIELD_PREP(PORT_MSIMAP_TARGET, i) |
+				       PORT_MSIMAP_ENABLE,
+				       port->base + pcie->hw->port_msimap + 4 * i);
+		}
+
+		writel_relaxed(PORT_MSICFG_EN, port->base + PORT_MSICFG);
+	} else {
+		writel_relaxed(0, port->base + PORT_MSIBASE);
+		writel_relaxed((ilog2(pcie->nvecs) << PORT_MSICFG_L2MSINUM_SHIFT) |
+			PORT_MSICFG_EN, port->base + PORT_MSICFG);
+	}
 
 	return 0;
 }
@@ -472,7 +542,9 @@ static int apple_pcie_setup_refclk(struct apple_pcie *pcie,
 	u32 stat;
 	int res;
 
-	rmw_set(PHY_LANE_CTL_CFGACC, port->phy + PHY_LANE_CTL);
+	if (pcie->hw->phy_lane_ctl)
+		rmw_set(PHY_LANE_CTL_CFGACC, port->phy + pcie->hw->phy_lane_ctl);
+
 	rmw_set(PHY_LANE_CFG_REFCLK0REQ, port->phy + PHY_LANE_CFG);
 
 	res = readl_relaxed_poll_timeout(port->phy + PHY_LANE_CFG,
@@ -489,10 +561,13 @@ static int apple_pcie_setup_refclk(struct apple_pcie *pcie,
 	if (res < 0)
 		return res;
 
-	rmw_clear(PHY_LANE_CTL_CFGACC, port->phy + PHY_LANE_CTL);
+	if (pcie->hw->phy_lane_ctl)
+		rmw_clear(PHY_LANE_CTL_CFGACC, port->phy + pcie->hw->phy_lane_ctl);
 
 	rmw_set(PHY_LANE_CFG_REFCLKEN, port->phy + PHY_LANE_CFG);
-	rmw_set(PORT_REFCLK_EN, port->base + PORT_REFCLK);
+
+	if (pcie->hw->port_refclk)
+		rmw_set(PORT_REFCLK_EN, port->base + pcie->hw->port_refclk);
 
 	return 0;
 }
@@ -500,9 +575,9 @@ static int apple_pcie_setup_refclk(struct apple_pcie *pcie,
 static u32 apple_pcie_rid2sid_write(struct apple_pcie_port *port,
 				    int idx, u32 val)
 {
-	writel_relaxed(val, port->base + PORT_RID2SID(idx));
+	writel_relaxed(val, port->base + port->pcie->hw->port_rid2sid + 4 * idx);
 	/* Read back to ensure completion of the write */
-	return readl_relaxed(port->base + PORT_RID2SID(idx));
+	return readl_relaxed(port->base + port->pcie->hw->port_rid2sid + 4 * idx);
 }
 
 static int apple_pcie_setup_port(struct apple_pcie *pcie,
@@ -563,7 +638,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 	usleep_range(100, 200);
 
 	/* Deassert PERST# */
-	rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
+	rmw_set(PORT_PERST_OFF, port->base + pcie->hw->port_perst);
 	gpiod_set_value_cansleep(reset, 0);
 
 	/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
@@ -576,15 +651,12 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 		return ret;
 	}
 
-	rmw_clear(PORT_REFCLK_CGDIS, port->base + PORT_REFCLK);
-	rmw_clear(PORT_APPCLK_CGDIS, port->base + PORT_APPCLK);
-
 	ret = apple_pcie_port_setup_irq(port);
 	if (ret)
 		return ret;
 
 	/* Reset all RID/SID mappings, and check for RAZ/WI registers */
-	for (i = 0; i < MAX_RID2SID; i++) {
+	for (i = 0; i < pcie->hw->max_rid2sid; i++) {
 		if (apple_pcie_rid2sid_write(port, i, 0xbad1d) != 0xbad1d)
 			break;
 		apple_pcie_rid2sid_write(port, i, 0);
@@ -608,6 +680,12 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 	if (!wait_for_completion_timeout(&pcie->event, HZ / 10))
 		dev_warn(pcie->dev, "%pOF link didn't come up\n", np);
 
+	if (pcie->hw->port_refclk)
+		rmw_clear(PORT_REFCLK_CGDIS, port->base + PORT_REFCLK);
+	else
+		rmw_set(PHY_LANE_CFG_REFCLKCGEN, port->phy + PHY_LANE_CFG);
+	rmw_clear(PORT_APPCLK_CGDIS, port->base + PORT_APPCLK);
+
 	return 0;
 }
 
@@ -732,7 +810,7 @@ static void apple_pcie_disable_device(struct pci_host_bridge *bridge, struct pci
 	for_each_set_bit(idx, port->sid_map, port->sid_map_sz) {
 		u32 val;
 
-		val = readl_relaxed(port->base + PORT_RID2SID(idx));
+		val = readl_relaxed(port->base + port->pcie->hw->port_rid2sid + 4 * idx);
 		if ((val & 0xffff) == rid) {
 			apple_pcie_rid2sid_write(port, idx, 0);
 			bitmap_release_region(port->sid_map, idx, 0);
@@ -750,13 +828,19 @@ static int apple_pcie_init(struct pci_config_window *cfg)
 	struct platform_device *platform = to_platform_device(dev);
 	struct device_node *of_port;
 	struct apple_pcie *pcie;
+	const struct of_device_id *match;
 	int ret;
 
+	match = of_match_device(apple_pcie_of_match_hw, dev);
+	if (!match)
+		return -ENODEV;
+
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
 	if (!pcie)
 		return -ENOMEM;
 
 	pcie->dev = dev;
+	pcie->hw = match->data;
 
 	mutex_init(&pcie->lock);
 
@@ -795,6 +879,7 @@ static const struct pci_ecam_ops apple_pcie_cfg_ecam_ops = {
 };
 
 static const struct of_device_id apple_pcie_of_match[] = {
+	{ .compatible = "apple,t6020-pcie", .data = &apple_pcie_cfg_ecam_ops },
 	{ .compatible = "apple,pcie", .data = &apple_pcie_cfg_ecam_ops },
 	{ }
 };

-- 
2.48.1

Re: [PATCH 7/7] PCI: apple: Add T602x PCIe support

[Marc Zyngier]

On Tue, 11 Feb 2025 19:54:32 +0000,
Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
> 
> From: Hector Martin <marcan@marcan.st>
> 
> This version of the hardware moved around a bunch of registers, so we
> drop the old compatible for these and introduce register offset
> structures to handle the differences.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> ---
>  drivers/pci/controller/pcie-apple.c | 125 ++++++++++++++++++++++++++++++------
>  1 file changed, 105 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 7f4839fb0a5b15a9ca87337f53c14a1ce08301fc..7c598334427cb56ca066890ac61143ae1d3ed744 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -26,6 +26,7 @@
>  #include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/msi.h>
> +#include <linux/of_device.h>
>  #include <linux/of_irq.h>
>  #include <linux/pci-ecam.h>
>  
> @@ -104,7 +105,7 @@
>  #define   PORT_REFCLK_CGDIS		BIT(8)
>  #define PORT_PERST			0x00814
>  #define   PORT_PERST_OFF		BIT(0)
> -#define PORT_RID2SID(i16)		(0x00828 + 4 * (i16))
> +#define PORT_RID2SID			0x00828
>  #define   PORT_RID2SID_VALID		BIT(31)
>  #define   PORT_RID2SID_SID_SHIFT	16
>  #define   PORT_RID2SID_BUS_SHIFT	8
> @@ -122,7 +123,7 @@
>  #define   PORT_TUNSTAT_PERST_ACK_PEND	BIT(1)
>  #define PORT_PREFMEM_ENABLE		0x00994
>  
> -#define MAX_RID2SID			64
> +#define MAX_RID2SID			512
>  
>  /*
>   * The doorbell address is set to 0xfffff000, which by convention
> @@ -133,6 +134,57 @@
>   */
>  #define DOORBELL_ADDR		CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR
>  
> +struct reg_info {
> +	u32 phy_lane_ctl;
> +	u32 port_msiaddr;
> +	u32 port_msiaddr_hi;
> +	u32 port_refclk;
> +	u32 port_perst;
> +	u32 port_rid2sid;
> +	u32 port_msimap;
> +	u32 max_rid2sid;
> +	u32 max_msimap;
> +};
> +
> +const struct reg_info t8103_hw = {
> +	.phy_lane_ctl = PHY_LANE_CTL,
> +	.port_msiaddr = PORT_MSIADDR,
> +	.port_msiaddr_hi = 0,
> +	.port_refclk = PORT_REFCLK,
> +	.port_perst = PORT_PERST,
> +	.port_rid2sid = PORT_RID2SID,
> +	.port_msimap = 0,
> +	.max_rid2sid = 64,
> +	.max_msimap = 0,
> +};
> +
> +#define PORT_T602X_MSIADDR		0x016c
> +#define PORT_T602X_MSIADDR_HI		0x0170
> +#define PORT_T602X_PERST		0x082c
> +#define PORT_T602X_RID2SID		0x3000
> +#define PORT_T602X_MSIMAP		0x3800
> +
> +#define PORT_MSIMAP_ENABLE		BIT(31)
> +#define PORT_MSIMAP_TARGET		GENMASK(7, 0)
> +
> +const struct reg_info t602x_hw = {
> +	.phy_lane_ctl = 0,
> +	.port_msiaddr = PORT_T602X_MSIADDR,
> +	.port_msiaddr_hi = PORT_T602X_MSIADDR_HI,
> +	.port_refclk = 0,
> +	.port_perst = PORT_T602X_PERST,
> +	.port_rid2sid = PORT_T602X_RID2SID,
> +	.port_msimap = PORT_T602X_MSIMAP,
> +	.max_rid2sid = 512, /* 16 on t602x, guess for autodetect on future HW */
> +	.max_msimap = 512, /* 96 on t602x, guess for autodetect on future HW */

What is max_msimap for? It is never used.

> +};
> +
> +static const struct of_device_id apple_pcie_of_match_hw[] = {
> +	{ .compatible = "apple,t6020-pcie", .data = &t602x_hw },
> +	{ .compatible = "apple,pcie", .data = &t8103_hw },
> +	{ }
> +};
> +
>  struct apple_pcie {
>  	struct mutex		lock;
>  	struct device		*dev;
> @@ -143,6 +195,7 @@ struct apple_pcie {
>  	struct completion	event;
>  	struct irq_fwspec	fwspec;
>  	u32			nvecs;
> +	const struct reg_info	*hw;
>  };
>  
>  struct apple_pcie_port {
> @@ -266,14 +319,14 @@ static void apple_port_irq_mask(struct irq_data *data)
>  {
>  	struct apple_pcie_port *port = irq_data_get_irq_chip_data(data);
>  
> -	writel_relaxed(BIT(data->hwirq), port->base + PORT_INTMSKSET);
> +	rmw_set(BIT(data->hwirq), port->base + PORT_INTMSK);
>  }
>  
>  static void apple_port_irq_unmask(struct irq_data *data)
>  {
>  	struct apple_pcie_port *port = irq_data_get_irq_chip_data(data);
>  
> -	writel_relaxed(BIT(data->hwirq), port->base + PORT_INTMSKCLR);
> +	rmw_clear(BIT(data->hwirq), port->base + PORT_INTMSK);
>  }
>  
>  static bool hwirq_is_intx(unsigned int hwirq)
> @@ -377,6 +430,7 @@ static void apple_port_irq_handler(struct irq_desc *desc)
>  static int apple_pcie_port_setup_irq(struct apple_pcie_port *port)
>  {
>  	struct fwnode_handle *fwnode = &port->np->fwnode;
> +	struct apple_pcie *pcie = port->pcie;
>  	unsigned int irq;
>  
>  	/* FIXME: consider moving each interrupt under each port */
> @@ -392,19 +446,35 @@ static int apple_pcie_port_setup_irq(struct apple_pcie_port *port)
>  		return -ENOMEM;
>  
>  	/* Disable all interrupts */
> -	writel_relaxed(~0, port->base + PORT_INTMSKSET);
> +	writel_relaxed(~0, port->base + PORT_INTMSK);
>  	writel_relaxed(~0, port->base + PORT_INTSTAT);
> +	writel_relaxed(~0, port->base + PORT_LINKCMDSTS);

Can you elaborate on this change?

>  
>  	irq_set_chained_handler_and_data(irq, apple_port_irq_handler, port);
>  
>  	/* Configure MSI base address */
>  	BUILD_BUG_ON(upper_32_bits(DOORBELL_ADDR));
> -	writel_relaxed(lower_32_bits(DOORBELL_ADDR), port->base + PORT_MSIADDR);
> +	writel_relaxed(lower_32_bits(DOORBELL_ADDR),
> +		       port->base + pcie->hw->port_msiaddr);
> +	if (pcie->hw->port_msiaddr_hi)
> +		writel_relaxed(0, port->base + pcie->hw->port_msiaddr_hi);
>  
>  	/* Enable MSIs, shared between all ports */
> -	writel_relaxed(0, port->base + PORT_MSIBASE);
> -	writel_relaxed((ilog2(port->pcie->nvecs) << PORT_MSICFG_L2MSINUM_SHIFT) |
> -		       PORT_MSICFG_EN, port->base + PORT_MSICFG);
> +	if (pcie->hw->port_msimap) {
> +		int i;
> +
> +		for (i = 0; i < pcie->nvecs; i++) {
> +			writel_relaxed(FIELD_PREP(PORT_MSIMAP_TARGET, i) |
> +				       PORT_MSIMAP_ENABLE,
> +				       port->base + pcie->hw->port_msimap + 4 * i);
> +		}
> +
> +		writel_relaxed(PORT_MSICFG_EN, port->base + PORT_MSICFG);
> +	} else {
> +		writel_relaxed(0, port->base + PORT_MSIBASE);
> +		writel_relaxed((ilog2(pcie->nvecs) << PORT_MSICFG_L2MSINUM_SHIFT) |
> +			PORT_MSICFG_EN, port->base + PORT_MSICFG);
> +	}
>  
>  	return 0;
>  }
> @@ -472,7 +542,9 @@ static int apple_pcie_setup_refclk(struct apple_pcie *pcie,
>  	u32 stat;
>  	int res;
>  
> -	rmw_set(PHY_LANE_CTL_CFGACC, port->phy + PHY_LANE_CTL);
> +	if (pcie->hw->phy_lane_ctl)
> +		rmw_set(PHY_LANE_CTL_CFGACC, port->phy + pcie->hw->phy_lane_ctl);
> +
>  	rmw_set(PHY_LANE_CFG_REFCLK0REQ, port->phy + PHY_LANE_CFG);
>  
>  	res = readl_relaxed_poll_timeout(port->phy + PHY_LANE_CFG,
> @@ -489,10 +561,13 @@ static int apple_pcie_setup_refclk(struct apple_pcie *pcie,
>  	if (res < 0)
>  		return res;
>  
> -	rmw_clear(PHY_LANE_CTL_CFGACC, port->phy + PHY_LANE_CTL);
> +	if (pcie->hw->phy_lane_ctl)
> +		rmw_clear(PHY_LANE_CTL_CFGACC, port->phy + pcie->hw->phy_lane_ctl);
>  
>  	rmw_set(PHY_LANE_CFG_REFCLKEN, port->phy + PHY_LANE_CFG);
> -	rmw_set(PORT_REFCLK_EN, port->base + PORT_REFCLK);
> +
> +	if (pcie->hw->port_refclk)
> +		rmw_set(PORT_REFCLK_EN, port->base + pcie->hw->port_refclk);
>  
>  	return 0;
>  }
> @@ -500,9 +575,9 @@ static int apple_pcie_setup_refclk(struct apple_pcie *pcie,
>  static u32 apple_pcie_rid2sid_write(struct apple_pcie_port *port,
>  				    int idx, u32 val)
>  {
> -	writel_relaxed(val, port->base + PORT_RID2SID(idx));
> +	writel_relaxed(val, port->base + port->pcie->hw->port_rid2sid + 4 * idx);
>  	/* Read back to ensure completion of the write */
> -	return readl_relaxed(port->base + PORT_RID2SID(idx));
> +	return readl_relaxed(port->base + port->pcie->hw->port_rid2sid + 4 * idx);
>  }
>  
>  static int apple_pcie_setup_port(struct apple_pcie *pcie,
> @@ -563,7 +638,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
>  	usleep_range(100, 200);
>  
>  	/* Deassert PERST# */
> -	rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
> +	rmw_set(PORT_PERST_OFF, port->base + pcie->hw->port_perst);
>  	gpiod_set_value_cansleep(reset, 0);
>  
>  	/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
> @@ -576,15 +651,12 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
>  		return ret;
>  	}
>  
> -	rmw_clear(PORT_REFCLK_CGDIS, port->base + PORT_REFCLK);
> -	rmw_clear(PORT_APPCLK_CGDIS, port->base + PORT_APPCLK);
> -
>  	ret = apple_pcie_port_setup_irq(port);
>  	if (ret)
>  		return ret;
>  
>  	/* Reset all RID/SID mappings, and check for RAZ/WI registers */
> -	for (i = 0; i < MAX_RID2SID; i++) {
> +	for (i = 0; i < pcie->hw->max_rid2sid; i++) {
>  		if (apple_pcie_rid2sid_write(port, i, 0xbad1d) != 0xbad1d)
>  			break;
>  		apple_pcie_rid2sid_write(port, i, 0);
> @@ -608,6 +680,12 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
>  	if (!wait_for_completion_timeout(&pcie->event, HZ / 10))
>  		dev_warn(pcie->dev, "%pOF link didn't come up\n", np);
>  
> +	if (pcie->hw->port_refclk)
> +		rmw_clear(PORT_REFCLK_CGDIS, port->base + PORT_REFCLK);

Shouldn't this be using the actual value for port_refclk?

> +	else
> +		rmw_set(PHY_LANE_CFG_REFCLKCGEN, port->phy + PHY_LANE_CFG);
> +	rmw_clear(PORT_APPCLK_CGDIS, port->base + PORT_APPCLK);
> +

Can you elaborate on this particular change?

I always assumed this was some clock-gating that needed to occur
*before* the link training was started. This is now taking place after
training, and the commit message doesn't say anything about it.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 7/7] PCI: apple: Add T602x PCIe support

[Sven Peter]

Hi,

On Wed, Feb 12, 2025, at 10:55, Marc Zyngier wrote:
> On Tue, 11 Feb 2025 19:54:32 +0000,
> Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
>> 
>> From: Hector Martin <marcan@marcan.st>
>> 
>> This version of the hardware moved around a bunch of registers, so we
>> drop the old compatible for these and introduce register offset
>> structures to handle the differences.
>> 
>> Signed-off-by: Hector Martin <marcan@marcan.st>
>> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
>> ---
>>  drivers/pci/controller/pcie-apple.c | 125 ++++++++++++++++++++++++++++++------
>>  1 file changed, 105 insertions(+), 20 deletions(-)
>> 
>> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
>> index 7f4839fb0a5b15a9ca87337f53c14a1ce08301fc..7c598334427cb56ca066890ac61143ae1d3ed744 100644
...
>
>> +	else
>> +		rmw_set(PHY_LANE_CFG_REFCLKCGEN, port->phy + PHY_LANE_CFG);
>> +	rmw_clear(PORT_APPCLK_CGDIS, port->base + PORT_APPCLK);
>> +
>
> Can you elaborate on this particular change?
>
> I always assumed this was some clock-gating that needed to occur
> *before* the link training was started. This is now taking place after
> training, and the commit message doesn't say anything about it.

It's been a while but as far as I can tell APPCLK seems to be related
to the IOMMUs attached to this controller. If it's disabled all reads
from the respective IOMMU MMIO either came back as 0xffff.. or SError
(don't remember which one it was) but pcie itself worked just fine
(until any device tried DMA ofc).

At least on M1 this entire sequence only works because we already
setup PORT_APPCLK_EN inside m1n1. If we didn't do this (like e.g
for the thunderbolt pcie/dart) the DART probe would already fail.



Sven

Re: [PATCH 7/7] PCI: apple: Add T602x PCIe support

[Marc Zyngier]

On Thu, 13 Feb 2025 19:51:31 +0000,
"Sven Peter" <sven@svenpeter.dev> wrote:
> 
> Hi,
> 
> On Wed, Feb 12, 2025, at 10:55, Marc Zyngier wrote:
> > On Tue, 11 Feb 2025 19:54:32 +0000,
> > Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
> >> 
> >> From: Hector Martin <marcan@marcan.st>
> >> 
> >> This version of the hardware moved around a bunch of registers, so we
> >> drop the old compatible for these and introduce register offset
> >> structures to handle the differences.
> >> 
> >> Signed-off-by: Hector Martin <marcan@marcan.st>
> >> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> >> ---
> >>  drivers/pci/controller/pcie-apple.c | 125 ++++++++++++++++++++++++++++++------
> >>  1 file changed, 105 insertions(+), 20 deletions(-)
> >> 
> >> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> >> index 7f4839fb0a5b15a9ca87337f53c14a1ce08301fc..7c598334427cb56ca066890ac61143ae1d3ed744 100644
> ...
> >
> >> +	else
> >> +		rmw_set(PHY_LANE_CFG_REFCLKCGEN, port->phy + PHY_LANE_CFG);
> >> +	rmw_clear(PORT_APPCLK_CGDIS, port->base + PORT_APPCLK);
> >> +
> >
> > Can you elaborate on this particular change?
> >
> > I always assumed this was some clock-gating that needed to occur
> > *before* the link training was started. This is now taking place after
> > training, and the commit message doesn't say anything about it.
> 
> It's been a while but as far as I can tell APPCLK seems to be related
> to the IOMMUs attached to this controller. If it's disabled all reads
> from the respective IOMMU MMIO either came back as 0xffff.. or SError
> (don't remember which one it was) but pcie itself worked just fine
> (until any device tried DMA ofc).
> 
> At least on M1 this entire sequence only works because we already
> setup PORT_APPCLK_EN inside m1n1. If we didn't do this (like e.g
> for the thunderbolt pcie/dart) the DART probe would already fail.

OK, so the exact location of this particular write doesn't matter as
long as it happens before we start enabling a device on that port.

I'm still perplexed by this one though:

+	if (pcie->hw->port_refclk)
+		rmw_clear(PORT_REFCLK_CGDIS, port->base + PORT_REFCLK);
+	else
+		rmw_set(PHY_LANE_CFG_REFCLKCGEN, port->phy + PHY_LANE_CFG);

which looks like it switches on the reference clock for the port. I
have a very vague recollection that it was required early before
m1n1/u-boot grew some PCI initialisation (yes, a long while ago).

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 7/7] PCI: apple: Add T602x PCIe support

[kernel test robot]

Hi Alyssa,

kernel test robot noticed the following build errors:

[auto build test ERROR on 2014c95afecee3e76ca4a56956a936e23283f05b]

url:    https://github.com/intel-lab-lkp/linux/commits/Alyssa-Rosenzweig/dt-bindings-pci-apple-pcie-Add-t6020-support/20250212-035900
base:   2014c95afecee3e76ca4a56956a936e23283f05b
patch link:    https://lore.kernel.org/r/20250211-pcie-t6-v1-7-b60e6d2501bb%40rosenzweig.io
patch subject: [PATCH 7/7] PCI: apple: Add T602x PCIe support
config: s390-randconfig-002-20250213 (https://download.01.org/0day-ci/archive/20250213/202502131258.hhEIy45J-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project 6807164500e9920638e2ab0cdb4bf8321d24f8eb)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250213/202502131258.hhEIy45J-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502131258.hhEIy45J-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/pci/controller/pcie-apple.c:467:19: error: call to undeclared function 'FIELD_PREP'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     467 |                         writel_relaxed(FIELD_PREP(PORT_MSIMAP_TARGET, i) |
         |                                        ^
   1 error generated.


vim +/FIELD_PREP +467 drivers/pci/controller/pcie-apple.c

   429	
   430	static int apple_pcie_port_setup_irq(struct apple_pcie_port *port)
   431	{
   432		struct fwnode_handle *fwnode = &port->np->fwnode;
   433		struct apple_pcie *pcie = port->pcie;
   434		unsigned int irq;
   435	
   436		/* FIXME: consider moving each interrupt under each port */
   437		irq = irq_of_parse_and_map(to_of_node(dev_fwnode(port->pcie->dev)),
   438					   port->idx);
   439		if (!irq)
   440			return -ENXIO;
   441	
   442		port->domain = irq_domain_create_linear(fwnode, 32,
   443							&apple_port_irq_domain_ops,
   444							port);
   445		if (!port->domain)
   446			return -ENOMEM;
   447	
   448		/* Disable all interrupts */
   449		writel_relaxed(~0, port->base + PORT_INTMSK);
   450		writel_relaxed(~0, port->base + PORT_INTSTAT);
   451		writel_relaxed(~0, port->base + PORT_LINKCMDSTS);
   452	
   453		irq_set_chained_handler_and_data(irq, apple_port_irq_handler, port);
   454	
   455		/* Configure MSI base address */
   456		BUILD_BUG_ON(upper_32_bits(DOORBELL_ADDR));
   457		writel_relaxed(lower_32_bits(DOORBELL_ADDR),
   458			       port->base + pcie->hw->port_msiaddr);
   459		if (pcie->hw->port_msiaddr_hi)
   460			writel_relaxed(0, port->base + pcie->hw->port_msiaddr_hi);
   461	
   462		/* Enable MSIs, shared between all ports */
   463		if (pcie->hw->port_msimap) {
   464			int i;
   465	
   466			for (i = 0; i < pcie->nvecs; i++) {
 > 467				writel_relaxed(FIELD_PREP(PORT_MSIMAP_TARGET, i) |
   468					       PORT_MSIMAP_ENABLE,
   469					       port->base + pcie->hw->port_msimap + 4 * i);
   470			}
   471	
   472			writel_relaxed(PORT_MSICFG_EN, port->base + PORT_MSICFG);
   473		} else {
   474			writel_relaxed(0, port->base + PORT_MSIBASE);
   475			writel_relaxed((ilog2(pcie->nvecs) << PORT_MSICFG_L2MSINUM_SHIFT) |
   476				PORT_MSICFG_EN, port->base + PORT_MSICFG);
   477		}
   478	
   479		return 0;
   480	}
   481	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 7/7] PCI: apple: Add T602x PCIe support

[Rob Herring]

On Tue, Feb 11, 2025 at 1:54 PM Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
>
> From: Hector Martin <marcan@marcan.st>
>
> This version of the hardware moved around a bunch of registers, so we
> drop the old compatible for these and introduce register offset
> structures to handle the differences.
>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> ---
>  drivers/pci/controller/pcie-apple.c | 125 ++++++++++++++++++++++++++++++------
>  1 file changed, 105 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 7f4839fb0a5b15a9ca87337f53c14a1ce08301fc..7c598334427cb56ca066890ac61143ae1d3ed744 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -26,6 +26,7 @@
>  #include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/msi.h>
> +#include <linux/of_device.h>

Drivers should not need this...

> +const struct reg_info t602x_hw = {
> +       .phy_lane_ctl = 0,
> +       .port_msiaddr = PORT_T602X_MSIADDR,
> +       .port_msiaddr_hi = PORT_T602X_MSIADDR_HI,
> +       .port_refclk = 0,
> +       .port_perst = PORT_T602X_PERST,
> +       .port_rid2sid = PORT_T602X_RID2SID,
> +       .port_msimap = PORT_T602X_MSIMAP,
> +       .max_rid2sid = 512, /* 16 on t602x, guess for autodetect on future HW */
> +       .max_msimap = 512, /* 96 on t602x, guess for autodetect on future HW */
> +};
> +
> +static const struct of_device_id apple_pcie_of_match_hw[] = {
> +       { .compatible = "apple,t6020-pcie", .data = &t602x_hw },
> +       { .compatible = "apple,pcie", .data = &t8103_hw },
> +       { }
> +};

You should not have 2 match tables.

> @@ -750,13 +828,19 @@ static int apple_pcie_init(struct pci_config_window *cfg)
>         struct platform_device *platform = to_platform_device(dev);
>         struct device_node *of_port;
>         struct apple_pcie *pcie;
> +       const struct of_device_id *match;
>         int ret;
>
> +       match = of_match_device(apple_pcie_of_match_hw, dev);
> +       if (!match)
> +               return -ENODEV;
> +
>         pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>         if (!pcie)
>                 return -ENOMEM;
>
>         pcie->dev = dev;
> +       pcie->hw = match->data;
>
>         mutex_init(&pcie->lock);
>
> @@ -795,6 +879,7 @@ static const struct pci_ecam_ops apple_pcie_cfg_ecam_ops = {
>  };
>
>  static const struct of_device_id apple_pcie_of_match[] = {
> +       { .compatible = "apple,t6020-pcie", .data = &apple_pcie_cfg_ecam_ops },
>         { .compatible = "apple,pcie", .data = &apple_pcie_cfg_ecam_ops },
>         { }

You are going to need to merge the data to 1 struct.

And then use (of_)?device_get_match_data() in probe().

Rob

Re: [PATCH 7/7] PCI: apple: Add T602x PCIe support

[Marc Zyngier]

On Thu, 13 Feb 2025 17:56:19 +0000,
Rob Herring <robh@kernel.org> wrote:
> 
> On Tue, Feb 11, 2025 at 1:54 PM Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
> >
> > From: Hector Martin <marcan@marcan.st>
> >
> > This version of the hardware moved around a bunch of registers, so we
> > drop the old compatible for these and introduce register offset
> > structures to handle the differences.
> >
> > Signed-off-by: Hector Martin <marcan@marcan.st>
> > Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> > ---
> >  drivers/pci/controller/pcie-apple.c | 125 ++++++++++++++++++++++++++++++------
> >  1 file changed, 105 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> > index 7f4839fb0a5b15a9ca87337f53c14a1ce08301fc..7c598334427cb56ca066890ac61143ae1d3ed744 100644
> > --- a/drivers/pci/controller/pcie-apple.c
> > +++ b/drivers/pci/controller/pcie-apple.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/list.h>
> >  #include <linux/module.h>
> >  #include <linux/msi.h>
> > +#include <linux/of_device.h>
> 
> Drivers should not need this...
> 
> > +const struct reg_info t602x_hw = {
> > +       .phy_lane_ctl = 0,
> > +       .port_msiaddr = PORT_T602X_MSIADDR,
> > +       .port_msiaddr_hi = PORT_T602X_MSIADDR_HI,
> > +       .port_refclk = 0,
> > +       .port_perst = PORT_T602X_PERST,
> > +       .port_rid2sid = PORT_T602X_RID2SID,
> > +       .port_msimap = PORT_T602X_MSIMAP,
> > +       .max_rid2sid = 512, /* 16 on t602x, guess for autodetect on future HW */
> > +       .max_msimap = 512, /* 96 on t602x, guess for autodetect on future HW */
> > +};
> > +
> > +static const struct of_device_id apple_pcie_of_match_hw[] = {
> > +       { .compatible = "apple,t6020-pcie", .data = &t602x_hw },
> > +       { .compatible = "apple,pcie", .data = &t8103_hw },
> > +       { }
> > +};
> 
> You should not have 2 match tables.
> 
> > @@ -750,13 +828,19 @@ static int apple_pcie_init(struct pci_config_window *cfg)
> >         struct platform_device *platform = to_platform_device(dev);
> >         struct device_node *of_port;
> >         struct apple_pcie *pcie;
> > +       const struct of_device_id *match;
> >         int ret;
> >
> > +       match = of_match_device(apple_pcie_of_match_hw, dev);
> > +       if (!match)
> > +               return -ENODEV;
> > +
> >         pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> >         if (!pcie)
> >                 return -ENOMEM;
> >
> >         pcie->dev = dev;
> > +       pcie->hw = match->data;
> >
> >         mutex_init(&pcie->lock);
> >
> > @@ -795,6 +879,7 @@ static const struct pci_ecam_ops apple_pcie_cfg_ecam_ops = {
> >  };
> >
> >  static const struct of_device_id apple_pcie_of_match[] = {
> > +       { .compatible = "apple,t6020-pcie", .data = &apple_pcie_cfg_ecam_ops },
> >         { .compatible = "apple,pcie", .data = &apple_pcie_cfg_ecam_ops },
> >         { }
> 
> You are going to need to merge the data to 1 struct.
> 
> And then use (of_)?device_get_match_data() in probe().

No, that will break the driver. This isn't a standalone driver, but
only an ECAM shim (as you can tell from the actual probe function).

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 7/7] PCI: apple: Add T602x PCIe support

[Rob Herring]

On Thu, Feb 13, 2025 at 12:01 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 13 Feb 2025 17:56:19 +0000,
> Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Feb 11, 2025 at 1:54 PM Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
> > >
> > > From: Hector Martin <marcan@marcan.st>
> > >
> > > This version of the hardware moved around a bunch of registers, so we
> > > drop the old compatible for these and introduce register offset
> > > structures to handle the differences.
> > >
> > > Signed-off-by: Hector Martin <marcan@marcan.st>
> > > Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> > > ---
> > >  drivers/pci/controller/pcie-apple.c | 125 ++++++++++++++++++++++++++++++------
> > >  1 file changed, 105 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> > > index 7f4839fb0a5b15a9ca87337f53c14a1ce08301fc..7c598334427cb56ca066890ac61143ae1d3ed744 100644
> > > --- a/drivers/pci/controller/pcie-apple.c
> > > +++ b/drivers/pci/controller/pcie-apple.c
> > > @@ -26,6 +26,7 @@
> > >  #include <linux/list.h>
> > >  #include <linux/module.h>
> > >  #include <linux/msi.h>
> > > +#include <linux/of_device.h>
> >
> > Drivers should not need this...
> >
> > > +const struct reg_info t602x_hw = {
> > > +       .phy_lane_ctl = 0,
> > > +       .port_msiaddr = PORT_T602X_MSIADDR,
> > > +       .port_msiaddr_hi = PORT_T602X_MSIADDR_HI,
> > > +       .port_refclk = 0,
> > > +       .port_perst = PORT_T602X_PERST,
> > > +       .port_rid2sid = PORT_T602X_RID2SID,
> > > +       .port_msimap = PORT_T602X_MSIMAP,
> > > +       .max_rid2sid = 512, /* 16 on t602x, guess for autodetect on future HW */
> > > +       .max_msimap = 512, /* 96 on t602x, guess for autodetect on future HW */
> > > +};
> > > +
> > > +static const struct of_device_id apple_pcie_of_match_hw[] = {
> > > +       { .compatible = "apple,t6020-pcie", .data = &t602x_hw },
> > > +       { .compatible = "apple,pcie", .data = &t8103_hw },
> > > +       { }
> > > +};
> >
> > You should not have 2 match tables.
> >
> > > @@ -750,13 +828,19 @@ static int apple_pcie_init(struct pci_config_window *cfg)
> > >         struct platform_device *platform = to_platform_device(dev);
> > >         struct device_node *of_port;
> > >         struct apple_pcie *pcie;
> > > +       const struct of_device_id *match;
> > >         int ret;
> > >
> > > +       match = of_match_device(apple_pcie_of_match_hw, dev);
> > > +       if (!match)
> > > +               return -ENODEV;
> > > +
> > >         pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> > >         if (!pcie)
> > >                 return -ENOMEM;
> > >
> > >         pcie->dev = dev;
> > > +       pcie->hw = match->data;
> > >
> > >         mutex_init(&pcie->lock);
> > >
> > > @@ -795,6 +879,7 @@ static const struct pci_ecam_ops apple_pcie_cfg_ecam_ops = {
> > >  };
> > >
> > >  static const struct of_device_id apple_pcie_of_match[] = {
> > > +       { .compatible = "apple,t6020-pcie", .data = &apple_pcie_cfg_ecam_ops },
> > >         { .compatible = "apple,pcie", .data = &apple_pcie_cfg_ecam_ops },
> > >         { }
> >
> > You are going to need to merge the data to 1 struct.
> >
> > And then use (of_)?device_get_match_data() in probe().
>
> No, that will break the driver. This isn't a standalone driver, but
> only an ECAM shim (as you can tell from the actual probe function).

Yes, I'm aware of that issue. The self-contained solution is just:

struct apple_match_data {
  const struct pci_ecam_ops ops;
  const struct reg_info info;
};

That works for pci_host_common_probe and apple_pcie_init. You don't
actually have to match again, but just cast the ops pointer. Yeah, no
type checking, but that already happens with of_match_table data.
Another solution is you could have 2 .init() hooks.

The somewhat more invasive solution is to define a struct with
pci_ecam_ops and a void pointer for extra data for ECAM match data.
Last time I looked at this, I think I needed something like this to
share more of the ECAM code with host drivers needing more setup.
There's a few cases of host controllers that have an ECAM space, but
still need a full driver.

The bottom line is drivers shouldn't be including of_device.h because
it's for bus implementations. I've worked to remove a bunch, but
there's still a bunch left. Please don't add more.

Rob