| Message ID | 20250211-pcie-t6-v1-7-b60e6d2501bb@rosenzweig.io (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: apple: support t6020 | expand |
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.
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
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
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.
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
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.
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 }, { } };