| Message ID | 20210913182550.264165-11-maz@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Add support for Apple M1 | expand |
On Mon, 13 Sep 2021 21:45:13 +0100, "Sven Peter" <sven@svenpeter.dev> wrote: > > > > On Mon, Sep 13, 2021, at 20:25, Marc Zyngier wrote: > > The Apple PCIe controller doesn't directly feed the endpoint's > > Requester ID to the IOMMU (DART), but instead maps RIDs onto > > Stream IDs (SIDs). The DART and the PCIe controller must thus > > agree on the SIDs that are used for translation (by using > > the 'iommu-map' property). > > > > For this purpose, parse the 'iommu-map' property each time a > > device gets added, and use the resulting translation to configure > > the PCIe RID-to-SID mapper. Similarily, remove the translation > > if/when the device gets removed. > > > > This is all driven from a bus notifier which gets registered at > > probe time. Hopefully this is the only PCI controller driver > > in the whole system. > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > drivers/pci/controller/pcie-apple.c | 158 +++++++++++++++++++++++++++- > > 1 file changed, 156 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-apple.c > > b/drivers/pci/controller/pcie-apple.c > > index 76344223245d..68d71eabe708 100644 > > --- a/drivers/pci/controller/pcie-apple.c > > +++ b/drivers/pci/controller/pcie-apple.c > > @@ -23,8 +23,10 @@ > > #include <linux/iopoll.h> > > #include <linux/irqchip/chained_irq.h> > > #include <linux/irqdomain.h> > > +#include <linux/list.h> > > #include <linux/module.h> > > #include <linux/msi.h> > > +#include <linux/notifier.h> > > #include <linux/of_irq.h> > > #include <linux/pci-ecam.h> > > > > @@ -116,6 +118,8 @@ > > #define PORT_TUNSTAT_PERST_ACK_PEND BIT(1) > > #define PORT_PREFMEM_ENABLE 0x00994 > > > > +#define MAX_RID2SID 64 > > Do these actually have 64 slots? I thought that was only for > the Thunderbolt controllers and that these only had 16. You are indeed right, and I blindly used the limit used in the Correlium driver. Using entries from 16 onward result in a non booting system. The registers do not fault though, and simply ignore writes. I came up with an simple fix for this, see below. > I never checked it myself though and it doesn't make much > of a difference for now since only four different RIDs will > ever be connected anyway. Four? I guess the radios expose more than a single RID? Thanks, M. diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c index 68d71eabe708..ec9e7abd2aca 100644 --- a/drivers/pci/controller/pcie-apple.c +++ b/drivers/pci/controller/pcie-apple.c @@ -148,6 +148,7 @@ struct apple_pcie_port { struct irq_domain *domain; struct list_head entry; DECLARE_BITMAP( sid_map, MAX_RID2SID); + int sid_map_sz; int idx; }; @@ -495,12 +496,12 @@ static int apple_pcie_setup_refclk(struct apple_pcie *pcie, return 0; } -static void apple_pcie_rid2sid_write(struct apple_pcie_port *port, +static u32 apple_pcie_rid2sid_write(struct apple_pcie_port *port, int idx, u32 val) { writel_relaxed(val, port->base + PORT_RID2SID(idx)); /* Read back to ensure completion of the write */ - (void)readl_relaxed(port->base + PORT_RID2SID(idx)); + return readl_relaxed(port->base + PORT_RID2SID(idx)); } static int apple_pcie_setup_port(struct apple_pcie *pcie, @@ -557,9 +558,16 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie, if (ret) return ret; - /* Reset all RID/SID mappings */ - for (i = 0; i < MAX_RID2SID; i++) + /* Reset all RID/SID mappings, and check for RAZ/WI registers */ + for (i = 0; i < MAX_RID2SID; i++) { + if (apple_pcie_rid2sid_write(port, i, 0xbad1d) != 0xbad1d) + break; apple_pcie_rid2sid_write(port, i, 0); + } + + dev_dbg(pcie->dev, "%pOF: %d RID/SID mapping entries\n", np, i); + + port->sid_map_sz = i; list_add_tail(&port->entry, &pcie->ports); init_completion(&pcie->event); @@ -667,7 +675,7 @@ static int apple_pcie_add_device(struct pci_dev *pdev) return err; mutex_lock(&port->pcie->lock); - sid_idx = bitmap_find_free_region(port->sid_map, MAX_RID2SID, 0); + sid_idx = bitmap_find_free_region(port->sid_map, port->sid_map_sz, 0); mutex_unlock(&port->pcie->lock); if (sid_idx < 0) @@ -696,7 +704,7 @@ static void apple_pcie_release_device(struct pci_dev *pdev) mutex_lock(&port->pcie->lock); - for_each_set_bit(idx, port->sid_map, MAX_RID2SID) { + for_each_set_bit(idx, port->sid_map, port->sid_map_sz) { u32 val; val = readl_relaxed(port->base + PORT_RID2SID(idx));
On Tue, Sep 14, 2021, at 11:35, Marc Zyngier wrote: > On Mon, 13 Sep 2021 21:45:13 +0100, > "Sven Peter" <sven@svenpeter.dev> wrote: > > > > > > > > On Mon, Sep 13, 2021, at 20:25, Marc Zyngier wrote: > > > The Apple PCIe controller doesn't directly feed the endpoint's > > > Requester ID to the IOMMU (DART), but instead maps RIDs onto > > > Stream IDs (SIDs). The DART and the PCIe controller must thus > > > agree on the SIDs that are used for translation (by using > > > the 'iommu-map' property). > > > > > > For this purpose, parse the 'iommu-map' property each time a > > > device gets added, and use the resulting translation to configure > > > the PCIe RID-to-SID mapper. Similarily, remove the translation > > > if/when the device gets removed. > > > > > > This is all driven from a bus notifier which gets registered at > > > probe time. Hopefully this is the only PCI controller driver > > > in the whole system. > > > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > > --- > > > drivers/pci/controller/pcie-apple.c | 158 +++++++++++++++++++++++++++- > > > 1 file changed, 156 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pci/controller/pcie-apple.c > > > b/drivers/pci/controller/pcie-apple.c > > > index 76344223245d..68d71eabe708 100644 > > > --- a/drivers/pci/controller/pcie-apple.c > > > +++ b/drivers/pci/controller/pcie-apple.c > > > @@ -23,8 +23,10 @@ > > > #include <linux/iopoll.h> > > > #include <linux/irqchip/chained_irq.h> > > > #include <linux/irqdomain.h> > > > +#include <linux/list.h> > > > #include <linux/module.h> > > > #include <linux/msi.h> > > > +#include <linux/notifier.h> > > > #include <linux/of_irq.h> > > > #include <linux/pci-ecam.h> > > > > > > @@ -116,6 +118,8 @@ > > > #define PORT_TUNSTAT_PERST_ACK_PEND BIT(1) > > > #define PORT_PREFMEM_ENABLE 0x00994 > > > > > > +#define MAX_RID2SID 64 > > > > Do these actually have 64 slots? I thought that was only for > > the Thunderbolt controllers and that these only had 16. > > You are indeed right, and I blindly used the limit used in the > Correlium driver. Using entries from 16 onward result in a non booting > system. The registers do not fault though, and simply ignore writes. I > came up with an simple fix for this, see below. Looks good to me and at least I prefer that to an additional property in the device tree. Reviewed-by: Sven Peter <sven@svenpeter.dev> Thanks, Sven
On Tue, 14 Sep 2021 10:56:05 +0100, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > Date: Tue, 14 Sep 2021 10:35:32 +0100 > > From: Marc Zyngier <maz@kernel.org> > > > > On Mon, 13 Sep 2021 21:45:13 +0100, > > "Sven Peter" <sven@svenpeter.dev> wrote: > > > > > > On Mon, Sep 13, 2021, at 20:25, Marc Zyngier wrote: > > > > The Apple PCIe controller doesn't directly feed the endpoint's > > > > Requester ID to the IOMMU (DART), but instead maps RIDs onto > > > > Stream IDs (SIDs). The DART and the PCIe controller must thus > > > > agree on the SIDs that are used for translation (by using > > > > the 'iommu-map' property). > > > > > > > > For this purpose, parse the 'iommu-map' property each time a > > > > device gets added, and use the resulting translation to configure > > > > the PCIe RID-to-SID mapper. Similarily, remove the translation > > > > if/when the device gets removed. > > > > > > > > This is all driven from a bus notifier which gets registered at > > > > probe time. Hopefully this is the only PCI controller driver > > > > in the whole system. > > > > > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > > > --- > > > > drivers/pci/controller/pcie-apple.c | 158 +++++++++++++++++++++++++++- > > > > 1 file changed, 156 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/pci/controller/pcie-apple.c > > > > b/drivers/pci/controller/pcie-apple.c > > > > index 76344223245d..68d71eabe708 100644 > > > > --- a/drivers/pci/controller/pcie-apple.c > > > > +++ b/drivers/pci/controller/pcie-apple.c > > > > @@ -23,8 +23,10 @@ > > > > #include <linux/iopoll.h> > > > > #include <linux/irqchip/chained_irq.h> > > > > #include <linux/irqdomain.h> > > > > +#include <linux/list.h> > > > > #include <linux/module.h> > > > > #include <linux/msi.h> > > > > +#include <linux/notifier.h> > > > > #include <linux/of_irq.h> > > > > #include <linux/pci-ecam.h> > > > > > > > > @@ -116,6 +118,8 @@ > > > > #define PORT_TUNSTAT_PERST_ACK_PEND BIT(1) > > > > #define PORT_PREFMEM_ENABLE 0x00994 > > > > > > > > +#define MAX_RID2SID 64 > > > > > > Do these actually have 64 slots? I thought that was only for > > > the Thunderbolt controllers and that these only had 16. > > > > You are indeed right, and I blindly used the limit used in the > > Correlium driver. Using entries from 16 onward result in a non booting > > system. The registers do not fault though, and simply ignore writes. I > > came up with an simple fix for this, see below. > > Or should be add a property to the DT binding to indicate the number > of entries (using a default of 16)? We don't have to add that > property right away; we can delay that until we actually try to > support the Thunderbolt ports. I'd rather only add a property for things we cannot discover ourselves. And indeed, we don't have to decide on this right now. > In case you didn't know already, RIDs that have no mapping in the > RID2SID table map to SID 0. That's why I picked 1 as the SID in the > iommu-map property for the port. I sort-off guessed, as using 0 made everything work by 'magic', while using your DT prevented the machine from booting. I tend to dislike magic, hence this patch. > > > > I never checked it myself though and it doesn't make much > > > of a difference for now since only four different RIDs will > > > ever be connected anyway. > > > > Four? I guess the radios expose more than a single RID? > > At this point, on the M1 mini there is the Broadcom BCM4378 WiFi/BT > device (which has two functions), the Fresco Logic FL1100 xHCI > controller (single function) and the Broadcom BCM57765 Ethernet > controller. So yes, there are for RIDs. But as far as I can see, the RID-to-SID mapping is per port. So at most, we have two RIDs per port/DART, not four. Or am I missing something altogether? M. -- Without deviation from the norm, progress is not possible.
> Date: Fri, 17 Sep 2021 10:19:02 +0100 > From: Marc Zyngier <maz@kernel.org> > > On Tue, 14 Sep 2021 10:56:05 +0100, > Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > Date: Tue, 14 Sep 2021 10:35:32 +0100 > > > From: Marc Zyngier <maz@kernel.org> > > > > > > On Mon, 13 Sep 2021 21:45:13 +0100, > > > "Sven Peter" <sven@svenpeter.dev> wrote: > > > > > > > > On Mon, Sep 13, 2021, at 20:25, Marc Zyngier wrote: > > > > > The Apple PCIe controller doesn't directly feed the endpoint's > > > > > Requester ID to the IOMMU (DART), but instead maps RIDs onto > > > > > Stream IDs (SIDs). The DART and the PCIe controller must thus > > > > > agree on the SIDs that are used for translation (by using > > > > > the 'iommu-map' property). > > > > > > > > > > For this purpose, parse the 'iommu-map' property each time a > > > > > device gets added, and use the resulting translation to configure > > > > > the PCIe RID-to-SID mapper. Similarily, remove the translation > > > > > if/when the device gets removed. > > > > > > > > > > This is all driven from a bus notifier which gets registered at > > > > > probe time. Hopefully this is the only PCI controller driver > > > > > in the whole system. > > > > > > > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > > > > --- > > > > > drivers/pci/controller/pcie-apple.c | 158 +++++++++++++++++++++++++++- > > > > > 1 file changed, 156 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/pci/controller/pcie-apple.c > > > > > b/drivers/pci/controller/pcie-apple.c > > > > > index 76344223245d..68d71eabe708 100644 > > > > > --- a/drivers/pci/controller/pcie-apple.c > > > > > +++ b/drivers/pci/controller/pcie-apple.c > > > > > @@ -23,8 +23,10 @@ > > > > > #include <linux/iopoll.h> > > > > > #include <linux/irqchip/chained_irq.h> > > > > > #include <linux/irqdomain.h> > > > > > +#include <linux/list.h> > > > > > #include <linux/module.h> > > > > > #include <linux/msi.h> > > > > > +#include <linux/notifier.h> > > > > > #include <linux/of_irq.h> > > > > > #include <linux/pci-ecam.h> > > > > > > > > > > @@ -116,6 +118,8 @@ > > > > > #define PORT_TUNSTAT_PERST_ACK_PEND BIT(1) > > > > > #define PORT_PREFMEM_ENABLE 0x00994 > > > > > > > > > > +#define MAX_RID2SID 64 > > > > > > > > Do these actually have 64 slots? I thought that was only for > > > > the Thunderbolt controllers and that these only had 16. > > > > > > You are indeed right, and I blindly used the limit used in the > > > Correlium driver. Using entries from 16 onward result in a non booting > > > system. The registers do not fault though, and simply ignore writes. I > > > came up with an simple fix for this, see below. > > > > Or should be add a property to the DT binding to indicate the number > > of entries (using a default of 16)? We don't have to add that > > property right away; we can delay that until we actually try to > > support the Thunderbolt ports. > > I'd rather only add a property for things we cannot discover > ourselves. And indeed, we don't have to decide on this right now. > > > In case you didn't know already, RIDs that have no mapping in the > > RID2SID table map to SID 0. That's why I picked 1 as the SID in the > > iommu-map property for the port. > > I sort-off guessed, as using 0 made everything work by 'magic', while > using your DT prevented the machine from booting. I tend to dislike > magic, hence this patch. Right. I deliberately used SID 1 in the DT to make sure other devices on the bus couldn't accidentally use IOMMU mappings for a device that was mapped to SID 0. > > > > I never checked it myself though and it doesn't make much > > > > of a difference for now since only four different RIDs will > > > > ever be connected anyway. > > > > > > Four? I guess the radios expose more than a single RID? > > > > At this point, on the M1 mini there is the Broadcom BCM4378 WiFi/BT > > device (which has two functions), the Fresco Logic FL1100 xHCI > > controller (single function) and the Broadcom BCM57765 Ethernet > > controller. So yes, there are for RIDs. > > But as far as I can see, the RID-to-SID mapping is per port. So at > most, we have two RIDs per port/DART, not four. Or am I missing > something altogether? No you're not missing anything.
diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c index 76344223245d..68d71eabe708 100644 --- a/drivers/pci/controller/pcie-apple.c +++ b/drivers/pci/controller/pcie-apple.c @@ -23,8 +23,10 @@ #include <linux/iopoll.h> #include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h> +#include <linux/list.h> #include <linux/module.h> #include <linux/msi.h> +#include <linux/notifier.h> #include <linux/of_irq.h> #include <linux/pci-ecam.h> @@ -116,6 +118,8 @@ #define PORT_TUNSTAT_PERST_ACK_PEND BIT(1) #define PORT_PREFMEM_ENABLE 0x00994 +#define MAX_RID2SID 64 + /* * The doorbell address is set to 0xfffff000, which by convention * matches what MacOS does, and it is possible to use any other @@ -131,6 +135,7 @@ struct apple_pcie { void __iomem *base; struct irq_domain *domain; unsigned long *bitmap; + struct list_head ports; struct completion event; struct irq_fwspec fwspec; u32 nvecs; @@ -141,6 +146,8 @@ struct apple_pcie_port { struct device_node *np; void __iomem *base; struct irq_domain *domain; + struct list_head entry; + DECLARE_BITMAP( sid_map, MAX_RID2SID); int idx; }; @@ -488,6 +495,14 @@ static int apple_pcie_setup_refclk(struct apple_pcie *pcie, return 0; } +static void apple_pcie_rid2sid_write(struct apple_pcie_port *port, + int idx, u32 val) +{ + writel_relaxed(val, port->base + PORT_RID2SID(idx)); + /* Read back to ensure completion of the write */ + (void)readl_relaxed(port->base + PORT_RID2SID(idx)); +} + static int apple_pcie_setup_port(struct apple_pcie *pcie, struct device_node *np) { @@ -495,7 +510,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie, struct apple_pcie_port *port; struct gpio_desc *reset; u32 stat, idx; - int ret; + int ret, i; reset = gpiod_get_from_of_node(np, "reset-gpios", 0, GPIOD_OUT_LOW, "#PERST"); @@ -542,6 +557,11 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie, if (ret) return ret; + /* Reset all RID/SID mappings */ + for (i = 0; i < MAX_RID2SID; i++) + apple_pcie_rid2sid_write(port, i, 0); + + list_add_tail(&port->entry, &pcie->ports); init_completion(&pcie->event); ret = apple_pcie_port_register_irqs(port); @@ -604,6 +624,122 @@ static int apple_msi_init(struct apple_pcie *pcie) return 0; } +static struct apple_pcie_port *apple_pcie_get_port(struct pci_dev *pdev) +{ + struct pci_config_window *cfg = pdev->sysdata; + struct apple_pcie *pcie = cfg->priv; + struct pci_dev *port_pdev = pdev; + struct apple_pcie_port *port; + + /* Find the root port this device is on */ + while (!pci_is_root_bus(port_pdev->bus)) + port_pdev = pci_upstream_bridge(port_pdev); + + /* If finding the port itself, nothing to do */ + if (pdev == port_pdev) + return NULL; + + list_for_each_entry(port, &pcie->ports, entry) { + if (port->idx == PCI_SLOT(port_pdev->devfn)) + return port; + } + + return NULL; +} + +static int apple_pcie_add_device(struct pci_dev *pdev) +{ + struct apple_pcie_port *port; + int sid_idx, err; + u32 rid, sid; + + port = apple_pcie_get_port(pdev); + if (!port) + return 0; + + dev_dbg(&pdev->dev, "added to bus %s, index %d\n", + pci_name(pdev->bus->self), port->idx); + + rid = PCI_DEVID(pdev->bus->number, pdev->devfn); + err = of_map_id(port->pcie->dev->of_node, rid, "iommu-map", + "iommu-map-mask", NULL, &sid); + if (err) + return err; + + mutex_lock(&port->pcie->lock); + sid_idx = bitmap_find_free_region(port->sid_map, MAX_RID2SID, 0); + mutex_unlock(&port->pcie->lock); + + if (sid_idx < 0) + return -ENOSPC; + + apple_pcie_rid2sid_write(port, sid_idx, + PORT_RID2SID_VALID | + (sid << PORT_RID2SID_SID_SHIFT) | rid); + + dev_dbg(&pdev->dev, "mapping RID%x to SID%x (index %d)\n", + rid, sid, sid_idx); + return 0; +} + +static void apple_pcie_release_device(struct pci_dev *pdev) +{ + struct apple_pcie_port *port; + u32 rid; + int idx; + + port = apple_pcie_get_port(pdev); + if (!port) + return; + + rid = PCI_DEVID(pdev->bus->number, pdev->devfn); + + mutex_lock(&port->pcie->lock); + + for_each_set_bit(idx, port->sid_map, MAX_RID2SID) { + u32 val; + + val = readl_relaxed(port->base + PORT_RID2SID(idx)); + if ((val & 0xffff) == rid) { + apple_pcie_rid2sid_write(port, idx, 0); + bitmap_release_region(port->sid_map, idx, 0); + dev_dbg(&pdev->dev, "Released %x (%d)\n", val, idx); + break; + } + } + + mutex_unlock(&port->pcie->lock); +} + +static int apple_pcie_bus_notifier(struct notifier_block *nb, + unsigned long action, + void *data) +{ + struct device *dev = data; + struct pci_dev *pdev = to_pci_dev(dev); + + /* + * This is a bit ugly. We assume that if we get notified for + * any PCI device, we must be in charge for it, and that there + * is no other PCI controller in the whole system. It probably + * holds for now, but for how long? + */ + switch (action) { + case BUS_NOTIFY_ADD_DEVICE: + apple_pcie_add_device(pdev); + break; + case BUS_NOTIFY_DEL_DEVICE: + apple_pcie_release_device(pdev); + break; + } + + return 0; +} + +static struct notifier_block apple_pcie_nb = { + .notifier_call = apple_pcie_bus_notifier, +}; + static int apple_pcie_init(struct pci_config_window *cfg) { struct device *dev = cfg->parent; @@ -625,6 +761,9 @@ static int apple_pcie_init(struct pci_config_window *cfg) if (IS_ERR(pcie->base)) return -ENODEV; + cfg->priv = pcie; + INIT_LIST_HEAD(&pcie->ports); + for_each_child_of_node(dev->of_node, of_port) { ret = apple_pcie_setup_port(pcie, of_port); if (ret) { @@ -636,6 +775,21 @@ static int apple_pcie_init(struct pci_config_window *cfg) return apple_msi_init(pcie); } +static int apple_pcie_probe(struct platform_device *pdev) +{ + int ret; + + ret = bus_register_notifier(&pci_bus_type, &apple_pcie_nb); + if (ret) + return ret; + + ret = pci_host_common_probe(pdev); + if (ret) + bus_unregister_notifier(&pci_bus_type, &apple_pcie_nb); + + return ret; +} + static const struct pci_ecam_ops apple_pcie_cfg_ecam_ops = { .init = apple_pcie_init, .pci_ops = { @@ -652,7 +806,7 @@ static const struct of_device_id apple_pcie_of_match[] = { MODULE_DEVICE_TABLE(of, apple_pcie_of_match); static struct platform_driver apple_pcie_driver = { - .probe = pci_host_common_probe, + .probe = apple_pcie_probe, .driver = { .name = "pcie-apple", .of_match_table = apple_pcie_of_match,
The Apple PCIe controller doesn't directly feed the endpoint's Requester ID to the IOMMU (DART), but instead maps RIDs onto Stream IDs (SIDs). The DART and the PCIe controller must thus agree on the SIDs that are used for translation (by using the 'iommu-map' property). For this purpose, parse the 'iommu-map' property each time a device gets added, and use the resulting translation to configure the PCIe RID-to-SID mapper. Similarily, remove the translation if/when the device gets removed. This is all driven from a bus notifier which gets registered at probe time. Hopefully this is the only PCI controller driver in the whole system. Signed-off-by: Marc Zyngier <maz@kernel.org> --- drivers/pci/controller/pcie-apple.c | 158 +++++++++++++++++++++++++++- 1 file changed, 156 insertions(+), 2 deletions(-)