I have resumed my earlier effort to bring the Apple-M1 into the world
of living by equipping it with a PCIe controller driver. Huge thanks
to Alyssa Rosenzweig for kicking it into shape and providing the first
two versions of this series.
Much has changed since v2[2]. Mark Kettenis is doing a great job with
the binding [0], so I have dropped that from the series, and strictly
focused on the Linux side of thing. I am now using this binding as is,
with the exception of a single line change, which I believe is a fix
[1].
Supporting the per-port interrupt controller has brought in a couple
of fixes for the core DT code. Also, some work has gone into dealing
with excluding the MSI page from the IOVA range, as well as
programming the RID-to-SID mapper.
Overall, the driver is now much cleaner and most probably feature
complete when it comes to supporting internal devices (although I
haven't investigated things like power management). TB support is
another story, and will require some more hacking.
This of course still depends on the clock and pinctrl drivers that are
otherwise in flight, and will affect this driver one way or another.
I have pushed a branch with all the dependencies (and more) at [3].
* From v2 [2]:
- Refactor DT parsing to match the new version of the binding
- Add support for INTx and port-private interrupts
- Signal link-up/down using interrupts
- Export of_phandle_args_to_fwspec
- Fix generic parsing of interrupt map
- Rationalise port setup (data structure, self discovery)
- Tell DART to exclude MSI doorbell from the IOVA mappings
- Get rid of the setup bypass if the link was found up on boot
- Prevent the module from being removed
- Program the RID-to-SID mapper on device discovery
- Rebased on 5.15-rc1
[0] https://lore.kernel.org/r/[email protected]
[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]
[3] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=hack/m1-pcie-v3
Alyssa Rosenzweig (2):
PCI: apple: Add initial hardware bring-up
PCI: apple: Set up reference clocks when probing
Marc Zyngier (8):
irqdomain: Make of_phandle_args_to_fwspec generally available
of/irq: Allow matching of an interrupt-map local to an interrupt
controller
PCI: of: Allow matching of an interrupt-map local to a pci device
PCI: apple: Add INTx and per-port interrupt support
arm64: apple: t8103: Add root port interrupt routing
PCI: apple: Implement MSI support
iommu/dart: Exclude MSI doorbell from PCIe device IOVA range
PCI: apple: Configure RID to SID mapper on device addition
MAINTAINERS | 7 +
arch/arm64/boot/dts/apple/t8103.dtsi | 33 +-
drivers/iommu/apple-dart.c | 25 +
drivers/of/irq.c | 17 +-
drivers/pci/controller/Kconfig | 17 +
drivers/pci/controller/Makefile | 1 +
drivers/pci/controller/pcie-apple.c | 818 +++++++++++++++++++++++++++
drivers/pci/of.c | 10 +-
include/linux/irqdomain.h | 4 +
kernel/irq/irqdomain.c | 6 +-
10 files changed, 925 insertions(+), 13 deletions(-)
create mode 100644 drivers/pci/controller/pcie-apple.c
--
2.30.2
of_phandle_args_to_fwspec() can be generally useful to code
extracting a DT of_phandle and using an irq_fwspec to use the
hierarchical irqdomain API.
Make it visible the the rest of the kernel, including modules.
Signed-off-by: Marc Zyngier <[email protected]>
---
include/linux/irqdomain.h | 4 ++++
kernel/irq/irqdomain.c | 6 +++---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 23e4ee523576..cfd442316f39 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -64,6 +64,10 @@ struct irq_fwspec {
u32 param[IRQ_DOMAIN_IRQ_SPEC_PARAMS];
};
+/* Conversion function from of_phandle_args fields to fwspec */
+void of_phandle_args_to_fwspec(struct device_node *np, const u32 *args,
+ unsigned int count, struct irq_fwspec *fwspec);
+
/*
* Should several domains have the same device node, but serve
* different purposes (for example one domain is for PCI/MSI, and the
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 19e83e9b723c..5a698c1f6cc6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -744,9 +744,8 @@ static int irq_domain_translate(struct irq_domain *d,
return 0;
}
-static void of_phandle_args_to_fwspec(struct device_node *np, const u32 *args,
- unsigned int count,
- struct irq_fwspec *fwspec)
+void of_phandle_args_to_fwspec(struct device_node *np, const u32 *args,
+ unsigned int count, struct irq_fwspec *fwspec)
{
int i;
@@ -756,6 +755,7 @@ static void of_phandle_args_to_fwspec(struct device_node *np, const u32 *args,
for (i = 0; i < count; i++)
fwspec->param[i] = args[i];
}
+EXPORT_SYMBOL_GPL(of_phandle_args_to_fwspec);
unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
{
--
2.30.2
of_irq_parse_raw() has a baked assumption that if a node has an
interrupt-controller property, it cannot possibly also have an
interrupt-map property (the latter being ignored).
This seems to be an odd behaviour, and there are no reason why
we should avoid supporting this use case. This is specially
useful when a PCI root port acts as an interrupt controller for
PCI endpoints, such as this:
pcie0: pcie@690000000 {
[...]
port00: pci@0,0 {
device_type = "pci";
[...]
#address-cells = <3>;
interrupt-controller;
#interrupt-cells = <1>;
interrupt-map-mask = <0 0 0 7>;
interrupt-map = <0 0 0 1 &port00 0 0 0 0>,
<0 0 0 2 &port00 0 0 0 1>,
<0 0 0 3 &port00 0 0 0 2>,
<0 0 0 4 &port00 0 0 0 3>;
};
};
Handle it by detecting that we have an interrupt-map early in the
parsing, and special case the situation where the phandle in the
interrupt map refers to the current node (which is the interesting
case here).
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/of/irq.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 352e14b007e7..32be5a03951f 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -156,10 +156,14 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
/* Now start the actual "proper" walk of the interrupt tree */
while (ipar != NULL) {
- /* Now check if cursor is an interrupt-controller and if it is
- * then we are done
+ /*
+ * Now check if cursor is an interrupt-controller and
+ * if it is then we are done, unless there is an
+ * interrupt-map which takes precedence.
*/
- if (of_property_read_bool(ipar, "interrupt-controller")) {
+ imap = of_get_property(ipar, "interrupt-map", &imaplen);
+ if (imap == NULL &&
+ of_property_read_bool(ipar, "interrupt-controller")) {
pr_debug(" -> got it !\n");
return 0;
}
@@ -173,8 +177,6 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
goto fail;
}
- /* Now look for an interrupt-map */
- imap = of_get_property(ipar, "interrupt-map", &imaplen);
/* No interrupt map, check for an interrupt parent */
if (imap == NULL) {
pr_debug(" -> no map, getting parent\n");
@@ -255,6 +257,11 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
out_irq->args_count = intsize = newintsize;
addrsize = newaddrsize;
+ if (ipar == newpar) {
+ pr_debug("%pOF interrupt-map entry to self\n", ipar);
+ return 0;
+ }
+
skiplevel:
/* Iterate again with new parent */
out_irq->np = newpar;
--
2.30.2
Add the interrupt-map properties that are required for INTx
signalling.
Signed-off-by: Marc Zyngier <[email protected]>
---
arch/arm64/boot/dts/apple/t8103.dtsi | 33 +++++++++++++++++++++++++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
index 0d2872d9b2d0..8e1f24c3f41f 100644
--- a/arch/arm64/boot/dts/apple/t8103.dtsi
+++ b/arch/arm64/boot/dts/apple/t8103.dtsi
@@ -382,7 +382,7 @@ pcie0: pcie@690000000 {
pinctrl-0 = <&pcie_pins>;
pinctrl-names = "default";
- pci@0,0 {
+ port00: pci@0,0 {
device_type = "pci";
reg = <0x0 0x0 0x0 0x0 0x0>;
reset-gpios = <&pinctrl_ap 152 0>;
@@ -391,9 +391,18 @@ pci@0,0 {
#address-cells = <3>;
#size-cells = <2>;
ranges;
+
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ interrupt-map-mask = <0 0 0 7>;
+ interrupt-map = <0 0 0 1 &port00 0 0 0 0>,
+ <0 0 0 2 &port00 0 0 0 1>,
+ <0 0 0 3 &port00 0 0 0 2>,
+ <0 0 0 4 &port00 0 0 0 3>;
};
- pci@1,0 {
+ port01: pci@1,0 {
device_type = "pci";
reg = <0x800 0x0 0x0 0x0 0x0>;
reset-gpios = <&pinctrl_ap 153 0>;
@@ -402,9 +411,18 @@ pci@1,0 {
#address-cells = <3>;
#size-cells = <2>;
ranges;
+
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ interrupt-map-mask = <0 0 0 7>;
+ interrupt-map = <0 0 0 1 &port01 0 0 0 0>,
+ <0 0 0 2 &port01 0 0 0 1>,
+ <0 0 0 3 &port01 0 0 0 2>,
+ <0 0 0 4 &port01 0 0 0 3>;
};
- pci@2,0 {
+ port02: pci@2,0 {
device_type = "pci";
reg = <0x1000 0x0 0x0 0x0 0x0>;
reset-gpios = <&pinctrl_ap 33 0>;
@@ -413,6 +431,15 @@ pci@2,0 {
#address-cells = <3>;
#size-cells = <2>;
ranges;
+
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ interrupt-map-mask = <0 0 0 7>;
+ interrupt-map = <0 0 0 1 &port02 0 0 0 0>,
+ <0 0 0 2 &port02 0 0 0 1>,
+ <0 0 0 3 &port02 0 0 0 2>,
+ <0 0 0 4 &port02 0 0 0 3>;
};
};
};
--
2.30.2
The MSI doorbell on Apple HW can be any address in the low 4GB
range. However, the MSI write is matched by the PCIe block before
hitting the iommu. It must thus be excluded from the IOVA range
that is assigned to any PCIe device.
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/iommu/apple-dart.c | 25 +++++++++++++++++++++++++
drivers/pci/controller/Kconfig | 5 +++++
drivers/pci/controller/pcie-apple.c | 4 +++-
3 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 559db9259e65..d1456663688e 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -721,6 +721,29 @@ static int apple_dart_def_domain_type(struct device *dev)
return 0;
}
+#define DOORBELL_ADDR (CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR & PAGE_MASK)
+
+static void apple_dart_get_resv_regions(struct device *dev,
+ struct list_head *head)
+{
+#ifdef CONFIG_PCIE_APPLE
+ if (dev_is_pci(dev)) {
+ struct iommu_resv_region *region;
+ int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+
+ region = iommu_alloc_resv_region(DOORBELL_ADDR,
+ PAGE_SIZE, prot,
+ IOMMU_RESV_MSI);
+ if (!region)
+ return;
+
+ list_add_tail(®ion->list, head);
+ }
+#endif
+
+ iommu_dma_get_resv_regions(dev, head);
+}
+
static const struct iommu_ops apple_dart_iommu_ops = {
.domain_alloc = apple_dart_domain_alloc,
.domain_free = apple_dart_domain_free,
@@ -737,6 +760,8 @@ static const struct iommu_ops apple_dart_iommu_ops = {
.device_group = apple_dart_device_group,
.of_xlate = apple_dart_of_xlate,
.def_domain_type = apple_dart_def_domain_type,
+ .get_resv_regions = apple_dart_get_resv_regions,
+ .put_resv_regions = generic_iommu_put_resv_regions,
.pgsize_bitmap = -1UL, /* Restricted during dart probe */
};
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 814833a8120d..b6e7410da254 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -312,6 +312,11 @@ config PCIE_HISI_ERR
Say Y here if you want error handling support
for the PCIe controller's errors on HiSilicon HIP SoCs
+config PCIE_APPLE_MSI_DOORBELL_ADDR
+ hex
+ default 0xfffff000
+ depends on PCIE_APPLE
+
config PCIE_APPLE
tristate "Apple PCIe controller"
depends on ARCH_APPLE || COMPILE_TEST
diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 1ed7b90f8360..76344223245d 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -120,8 +120,10 @@
* The doorbell address is set to 0xfffff000, which by convention
* matches what MacOS does, and it is possible to use any other
* address (in the bottom 4GB, as the base register is only 32bit).
+ * However, it has to be excluded from the the IOVA range, and the
+ * DART driver has to know about it.
*/
-#define DOORBELL_ADDR 0xfffff000
+#define DOORBELL_ADDR CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR
struct apple_pcie {
struct mutex lock;
--
2.30.2
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 <[email protected]>
---
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
+
/*
* 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,
--
2.30.2
Just as we now allow an interrupt map to be parsed when part
of an interrupt controller, there is no reason to ignore an
interrupt map that would be part of a pci device node such as
a root port since we already allow interrupt specifiers.
This allows the device itself to use the interrupt map for
for its own purpose.
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/pci/of.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index d84381ce82b5..443cebb0622e 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -423,7 +423,7 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
*/
static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
{
- struct device_node *dn, *ppnode;
+ struct device_node *dn, *ppnode = NULL;
struct pci_dev *ppdev;
__be32 laddr[3];
u8 pin;
@@ -452,8 +452,14 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *
if (pin == 0)
return -ENODEV;
+ /* Local interrupt-map in the device node? Use it! */
+ if (dn && of_get_property(dn, "interrupt-map", NULL)) {
+ pin = pci_swizzle_interrupt_pin(pdev, pin);
+ ppnode = dn;
+ }
+
/* Now we walk up the PCI tree */
- for (;;) {
+ while (!ppnode) {
/* Get the pci_dev of our parent */
ppdev = pdev->bus->self;
--
2.30.2
From: Alyssa Rosenzweig <[email protected]>
Add a minimal driver to bring up the PCIe bus on Apple system-on-chips,
particularly the Apple M1. This driver exposes the internal bus used for
the USB type-A ports, Ethernet, Wi-Fi, and Bluetooth. Bringing up the
radios requires additional drivers beyond what's necessary for PCIe
itself.
At this stage, nothing is functionnal.
Co-developed-by: Stan Skowronek <[email protected]>
Signed-off-by: Stan Skowronek <[email protected]>
Signed-off-by: Alyssa Rosenzweig <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
MAINTAINERS | 7 +
drivers/pci/controller/Kconfig | 12 ++
drivers/pci/controller/Makefile | 1 +
drivers/pci/controller/pcie-apple.c | 243 ++++++++++++++++++++++++++++
4 files changed, 263 insertions(+)
create mode 100644 drivers/pci/controller/pcie-apple.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 813a847e2d64..9905cc48fed9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1280,6 +1280,13 @@ S: Maintained
F: Documentation/devicetree/bindings/iommu/apple,dart.yaml
F: drivers/iommu/apple-dart.c
+APPLE PCIE CONTROLLER DRIVER
+M: Alyssa Rosenzweig <[email protected]>
+M: Marc Zyngier <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/pci/controller/pcie-apple.c
+
APPLE SMC DRIVER
M: Henrik Rydberg <[email protected]>
L: [email protected]
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 326f7d13024f..814833a8120d 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -312,6 +312,18 @@ config PCIE_HISI_ERR
Say Y here if you want error handling support
for the PCIe controller's errors on HiSilicon HIP SoCs
+config PCIE_APPLE
+ tristate "Apple PCIe controller"
+ depends on ARCH_APPLE || COMPILE_TEST
+ depends on OF
+ depends on PCI_MSI_IRQ_DOMAIN
+ help
+ Say Y here if you want to enable PCIe controller support on Apple
+ system-on-chips, like the Apple M1. This is required for the USB
+ type-A ports, Ethernet, Wi-Fi, and Bluetooth.
+
+ If unsure, say Y if you have an Apple Silicon system.
+
source "drivers/pci/controller/dwc/Kconfig"
source "drivers/pci/controller/mobiveil/Kconfig"
source "drivers/pci/controller/cadence/Kconfig"
diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
index aaf30b3dcc14..f9d40bad932c 100644
--- a/drivers/pci/controller/Makefile
+++ b/drivers/pci/controller/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_VMD) += vmd.o
obj-$(CONFIG_PCIE_BRCMSTB) += pcie-brcmstb.o
obj-$(CONFIG_PCI_LOONGSON) += pci-loongson.o
obj-$(CONFIG_PCIE_HISI_ERR) += pcie-hisi-error.o
+obj-$(CONFIG_PCIE_APPLE) += pcie-apple.o
# pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
obj-y += dwc/
obj-y += mobiveil/
diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
new file mode 100644
index 000000000000..f3c414950a10
--- /dev/null
+++ b/drivers/pci/controller/pcie-apple.c
@@ -0,0 +1,243 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe host bridge driver for Apple system-on-chips.
+ *
+ * The HW is ECAM compliant, so once the controller is initialized,
+ * the driver mostly deals MSI mapping and handling of per-port
+ * interrupts (INTx, management and error signals).
+ *
+ * Initialization requires enabling power and clocks, along with a
+ * number of register pokes.
+ *
+ * Copyright (C) 2021 Alyssa Rosenzweig <[email protected]>
+ * Copyright (C) 2021 Google LLC
+ * Copyright (C) 2021 Corellium LLC
+ * Copyright (C) 2021 Mark Kettenis <[email protected]>
+ *
+ * Author: Alyssa Rosenzweig <[email protected]>
+ * Author: Marc Zyngier <[email protected]>
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/iopoll.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/of_irq.h>
+#include <linux/pci-ecam.h>
+
+#define CORE_RC_PHYIF_CTL 0x00024
+#define CORE_RC_PHYIF_CTL_RUN BIT(0)
+#define CORE_RC_PHYIF_STAT 0x00028
+#define CORE_RC_PHYIF_STAT_REFCLK BIT(4)
+#define CORE_RC_CTL 0x00050
+#define CORE_RC_CTL_RUN BIT(0)
+#define CORE_RC_STAT 0x00058
+#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_REFCLK1 BIT(1)
+#define CORE_LANE_CFG_REFCLK0ACK BIT(2)
+#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 PORT_LTSSMCTL 0x00080
+#define PORT_LTSSMCTL_START BIT(0)
+#define PORT_INTSTAT 0x00100
+#define PORT_INT_TUNNEL_ERR 31
+#define PORT_INT_CPL_TIMEOUT 23
+#define PORT_INT_RID2SID_MAPERR 22
+#define PORT_INT_CPL_ABORT 21
+#define PORT_INT_MSI_BAD_DATA 19
+#define PORT_INT_MSI_ERR 18
+#define PORT_INT_REQADDR_GT32 17
+#define PORT_INT_AF_TIMEOUT 15
+#define PORT_INT_LINK_DOWN 14
+#define PORT_INT_LINK_UP 12
+#define PORT_INT_LINK_BWMGMT 11
+#define PORT_INT_AER_MASK (15 << 4)
+#define PORT_INT_PORT_ERR 4
+#define PORT_INT_INTx(i) i
+#define PORT_INT_INTx_MASK 15
+#define PORT_INTMSK 0x00104
+#define PORT_INTMSKSET 0x00108
+#define PORT_INTMSKCLR 0x0010c
+#define PORT_MSICFG 0x00124
+#define PORT_MSICFG_EN BIT(0)
+#define PORT_MSICFG_L2MSINUM_SHIFT 4
+#define PORT_MSIBASE 0x00128
+#define PORT_MSIBASE_1_SHIFT 16
+#define PORT_MSIADDR 0x00168
+#define PORT_LINKSTS 0x00208
+#define PORT_LINKSTS_UP BIT(0)
+#define PORT_LINKSTS_BUSY BIT(2)
+#define PORT_LINKCMDSTS 0x00210
+#define PORT_OUTS_NPREQS 0x00284
+#define PORT_OUTS_NPREQS_REQ BIT(24)
+#define PORT_OUTS_NPREQS_CPL BIT(16)
+#define PORT_RXWR_FIFO 0x00288
+#define PORT_RXWR_FIFO_HDR GENMASK(15, 10)
+#define PORT_RXWR_FIFO_DATA GENMASK(9, 0)
+#define PORT_RXRD_FIFO 0x0028C
+#define PORT_RXRD_FIFO_REQ GENMASK(6, 0)
+#define PORT_OUTS_CPLS 0x00290
+#define PORT_OUTS_CPLS_SHRD GENMASK(14, 8)
+#define PORT_OUTS_CPLS_WAIT GENMASK(6, 0)
+#define PORT_APPCLK 0x00800
+#define PORT_APPCLK_EN BIT(0)
+#define PORT_APPCLK_CGDIS BIT(8)
+#define PORT_STATUS 0x00804
+#define PORT_STATUS_READY BIT(0)
+#define PORT_REFCLK 0x00810
+#define PORT_REFCLK_EN BIT(0)
+#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_VALID BIT(31)
+#define PORT_RID2SID_SID_SHIFT 16
+#define PORT_RID2SID_BUS_SHIFT 8
+#define PORT_RID2SID_DEV_SHIFT 3
+#define PORT_RID2SID_FUNC_SHIFT 0
+#define PORT_OUTS_PREQS_HDR 0x00980
+#define PORT_OUTS_PREQS_HDR_MASK GENMASK(9, 0)
+#define PORT_OUTS_PREQS_DATA 0x00984
+#define PORT_OUTS_PREQS_DATA_MASK GENMASK(15, 0)
+#define PORT_TUNCTRL 0x00988
+#define PORT_TUNCTRL_PERST_ON BIT(0)
+#define PORT_TUNCTRL_PERST_ACK_REQ BIT(1)
+#define PORT_TUNSTAT 0x0098c
+#define PORT_TUNSTAT_PERST_ON BIT(0)
+#define PORT_TUNSTAT_PERST_ACK_PEND BIT(1)
+#define PORT_PREFMEM_ENABLE 0x00994
+
+struct apple_pcie {
+ struct device *dev;
+ void __iomem *base;
+};
+
+struct apple_pcie_port {
+ struct apple_pcie *pcie;
+ struct device_node *np;
+ void __iomem *base;
+ int idx;
+};
+
+static inline void rmwl(u32 clr, u32 set, void __iomem *addr)
+{
+ writel_relaxed((readl_relaxed(addr) & ~clr) | set, addr);
+}
+
+static int apple_pcie_setup_port(struct apple_pcie *pcie,
+ struct device_node *np)
+{
+ struct platform_device *platform = to_platform_device(pcie->dev);
+ struct apple_pcie_port *port;
+ struct gpio_desc *reset;
+ u32 stat, idx;
+ int ret;
+
+ reset = gpiod_get_from_of_node(np, "reset-gpios", 0,
+ GPIOD_OUT_LOW, "#PERST");
+ if (IS_ERR(reset))
+ return PTR_ERR(reset);
+
+ port = devm_kzalloc(pcie->dev, sizeof(*port), GFP_KERNEL);
+ if (!port)
+ return -ENOMEM;
+
+ ret = of_property_read_u32_index(np, "reg", 0, &idx);
+ if (ret)
+ return ret;
+
+ /* Use the first reg entry to work out the port index */
+ port->idx = idx >> 11;
+ port->pcie = pcie;
+ port->np = np;
+
+ port->base = devm_platform_ioremap_resource(platform, port->idx + 2);
+ if (IS_ERR(port->base))
+ return -ENODEV;
+
+ rmwl(0, PORT_APPCLK_EN, port + PORT_APPCLK);
+
+ rmwl(0, PORT_PERST_OFF, port->base + PORT_PERST);
+ gpiod_set_value(reset, 1);
+
+ ret = readl_relaxed_poll_timeout(port->base + PORT_STATUS, stat,
+ stat & PORT_STATUS_READY, 100, 250000);
+ if (ret < 0) {
+ dev_err(pcie->dev, "port %pOF ready wait timeout\n", np);
+ return ret;
+ }
+
+ /* Flush writes and enable the link */
+ dma_wmb();
+
+ writel_relaxed(PORT_LTSSMCTL_START, port->base + PORT_LTSSMCTL);
+
+ return 0;
+}
+
+static int apple_pcie_init(struct pci_config_window *cfg)
+{
+ struct device *dev = cfg->parent;
+ struct platform_device *platform = to_platform_device(dev);
+ struct device_node *of_port;
+ struct apple_pcie *pcie;
+ int ret;
+
+ pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+ if (!pcie)
+ return -ENOMEM;
+
+ pcie->dev = dev;
+
+ mutex_init(&pcie->lock);
+
+ pcie->base = devm_platform_ioremap_resource(platform, 1);
+
+ if (IS_ERR(pcie->base))
+ return -ENODEV;
+
+ for_each_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);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static const struct pci_ecam_ops apple_pcie_cfg_ecam_ops = {
+ .init = apple_pcie_init,
+ .pci_ops = {
+ .map_bus = pci_ecam_map_bus,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+ }
+};
+
+static const struct of_device_id apple_pcie_of_match[] = {
+ { .compatible = "apple,pcie", .data = &apple_pcie_cfg_ecam_ops },
+ { }
+};
+MODULE_DEVICE_TABLE(of, apple_pcie_of_match);
+
+static struct platform_driver apple_pcie_driver = {
+ .probe = pci_host_common_probe,
+ .driver = {
+ .name = "pcie-apple",
+ .of_match_table = apple_pcie_of_match,
+ .suppress_bind_attrs = true,
+ },
+};
+module_platform_driver(apple_pcie_driver);
+
+MODULE_LICENSE("GPL v2");
--
2.30.2
Add support for the per-port interrupt controller that deals
with both INTx signalling and management interrupts.
This allows the Link-up/Link-down interrupts to be wired, allowing
the bring-up to be synchronised (and provide debug information).
The framework can further be used to handle the rest of the per
port events if and when necessary.
Likewise, INTx signalling is implemented so that end-points can
actually be used.
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/pci/controller/pcie-apple.c | 209 ++++++++++++++++++++++++++++
1 file changed, 209 insertions(+)
diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index dabbfc2e1fb0..d174a215a47d 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -21,6 +21,7 @@
#include <linux/gpio/consumer.h>
#include <linux/kernel.h>
#include <linux/iopoll.h>
+#include <linux/irqchip/chained_irq.h>
#include <linux/irqdomain.h>
#include <linux/module.h>
#include <linux/msi.h>
@@ -118,12 +119,14 @@
struct apple_pcie {
struct device *dev;
void __iomem *base;
+ struct completion event;
};
struct apple_pcie_port {
struct apple_pcie *pcie;
struct device_node *np;
void __iomem *base;
+ struct irq_domain *domain;
int idx;
};
@@ -132,6 +135,200 @@ static inline void rmwl(u32 clr, u32 set, void __iomem *addr)
writel_relaxed((readl_relaxed(addr) & ~clr) | set, addr);
}
+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);
+}
+
+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);
+}
+
+static bool hwirq_is_intx(unsigned int hwirq)
+{
+ return BIT(hwirq) & PORT_INT_INTx_MASK;
+}
+
+static void apple_port_irq_ack(struct irq_data *data)
+{
+ struct apple_pcie_port *port = irq_data_get_irq_chip_data(data);
+
+ if (!hwirq_is_intx(data->hwirq))
+ writel_relaxed(BIT(data->hwirq), port->base + PORT_INTSTAT);
+}
+
+static int apple_port_irq_set_type(struct irq_data *data, unsigned int type)
+{
+ /*
+ * It doesn't seem that there is any way to configure the
+ * trigger, so assume INTx have to be level (as per the spec),
+ * and the rest is edge (which looks likely).
+ */
+ if (hwirq_is_intx(data->hwirq) ^ !!(type & IRQ_TYPE_LEVEL_MASK))
+ return -EINVAL;
+
+ irqd_set_trigger_type(data, type);
+ return 0;
+}
+
+static struct irq_chip apple_port_irqchip = {
+ .name = "PCIe",
+ .irq_ack = apple_port_irq_ack,
+ .irq_mask = apple_port_irq_mask,
+ .irq_unmask = apple_port_irq_unmask,
+ .irq_set_type = apple_port_irq_set_type,
+};
+
+static int apple_port_irq_domain_alloc(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs,
+ void *args)
+{
+ struct apple_pcie_port *port = domain->host_data;
+ struct irq_fwspec *fwspec = args;
+ int i;
+
+ for (i = 0; i < nr_irqs; i++) {
+ irq_flow_handler_t flow = handle_edge_irq;
+ unsigned int type = IRQ_TYPE_EDGE_RISING;
+
+ if (hwirq_is_intx(fwspec->param[0] + i)) {
+ flow = handle_level_irq;
+ type = IRQ_TYPE_LEVEL_HIGH;
+ }
+
+ irq_domain_set_info(domain, virq + i, fwspec->param[0] + i,
+ &apple_port_irqchip, port, flow,
+ NULL, NULL);
+
+ irq_set_irq_type(virq + i, type);
+ }
+
+ return 0;
+}
+
+static void apple_port_irq_domain_free(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs)
+{
+ int i;
+
+ for (i = 0; i < nr_irqs; i++) {
+ struct irq_data *d = irq_domain_get_irq_data(domain, virq + i);
+ irq_set_handler(virq + i, NULL);
+ irq_domain_reset_irq_data(d);
+ }
+}
+
+static const struct irq_domain_ops apple_port_irq_domain_ops = {
+ .translate = irq_domain_translate_onecell,
+ .alloc = apple_port_irq_domain_alloc,
+ .free = apple_port_irq_domain_free,
+};
+
+static void apple_port_irq_handler(struct irq_desc *desc)
+{
+ struct apple_pcie_port *port = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ unsigned long stat;
+ int i;
+
+ chained_irq_enter(chip, desc);
+
+ stat = readl_relaxed(port->base + PORT_INTSTAT);
+
+ for_each_set_bit(i, &stat, 32)
+ generic_handle_domain_irq(port->domain, i);
+
+ chained_irq_exit(chip, desc);
+}
+
+static int apple_pcie_port_setup_irq(struct apple_pcie_port *port)
+{
+ struct fwnode_handle *fwnode = &port->np->fwnode;
+ unsigned int irq;
+
+ /* FIXME: consider moving each interrupt under each port */
+ irq = irq_of_parse_and_map(to_of_node(dev_fwnode(port->pcie->dev)),
+ port->idx);
+ if (!irq)
+ return -ENXIO;
+
+ port->domain = irq_domain_create_linear(fwnode, 32,
+ &apple_port_irq_domain_ops,
+ port);
+ if (!port->domain)
+ return -ENOMEM;
+
+ /* Disable all interrupts */
+ writel_relaxed(~0, port->base + PORT_INTMSKSET);
+ writel_relaxed(~0, port->base + PORT_INTSTAT);
+
+ irq_set_chained_handler_and_data(irq, apple_port_irq_handler, port);
+
+ return 0;
+}
+
+static irqreturn_t apple_pcie_port_irq(int irq, void *data)
+{
+ struct apple_pcie_port *port = data;
+ unsigned int hwirq = irq_domain_get_irq_data(port->domain, irq)->hwirq;
+
+ switch (hwirq) {
+ case PORT_INT_LINK_UP:
+ dev_info_ratelimited(port->pcie->dev, "Link up on %pOF\n",
+ port->np);
+ complete_all(&port->pcie->event);
+ break;
+ case PORT_INT_LINK_DOWN:
+ dev_info_ratelimited(port->pcie->dev, "Link down on %pOF\n",
+ port->np);
+ break;
+ default:
+ return IRQ_NONE;
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int apple_pcie_port_register_irqs(struct apple_pcie_port *port)
+{
+ static struct {
+ unsigned int hwirq;
+ const char *name;
+ } port_irqs[] = {
+ { PORT_INT_LINK_UP, "Link up", },
+ { PORT_INT_LINK_DOWN, "Link down", },
+ };
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(port_irqs); i++) {
+ struct irq_fwspec fwspec = {
+ .fwnode = &port->np->fwnode,
+ .param_count = 1,
+ .param = {
+ [0] = port_irqs[i].hwirq,
+ },
+ };
+ unsigned int irq;
+ int ret;
+
+ irq = irq_domain_alloc_irqs(port->domain, 1, NUMA_NO_NODE,
+ &fwspec);
+ if (WARN_ON(!irq))
+ continue;
+
+ ret = request_irq(irq, apple_pcie_port_irq, 0,
+ port_irqs[i].name, port);
+ WARN_ON(ret);
+ }
+
+ return 0;
+}
+
static int apple_pcie_setup_refclk(struct apple_pcie *pcie,
struct apple_pcie_port *port)
{
@@ -222,8 +419,20 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
/* Flush writes and enable the link */
dma_wmb();
+ ret = apple_pcie_port_setup_irq(port);
+ if (ret)
+ return ret;
+
+ init_completion(&pcie->event);
+
+ ret = apple_pcie_port_register_irqs(port);
+ WARN_ON(ret);
+
writel_relaxed(PORT_LTSSMCTL_START, port->base + PORT_LTSSMCTL);
+ if (!wait_for_completion_timeout(&pcie->event, HZ / 10))
+ dev_warn(pcie->dev, "%pOF link didn't come up\n", np);
+
return 0;
}
--
2.30.2
From: Alyssa Rosenzweig <[email protected]>
Apple's PCIe controller requires clocks to be configured in order to
bring up the hardware. Add the register pokes required to do so.
Adapted from Corellium's driver via Mark Kettenis's U-Boot patches.
Co-developed-by: Stan Skowronek <[email protected]>
Signed-off-by: Stan Skowronek <[email protected]>
Signed-off-by: Alyssa Rosenzweig <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/pci/controller/pcie-apple.c | 44 +++++++++++++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index f3c414950a10..dabbfc2e1fb0 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -132,6 +132,46 @@ static inline void rmwl(u32 clr, u32 set, void __iomem *addr)
writel_relaxed((readl_relaxed(addr) & ~clr) | set, addr);
}
+static int apple_pcie_setup_refclk(struct apple_pcie *pcie,
+ struct apple_pcie_port *port)
+{
+ 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;
+
+ rmwl(0, CORE_LANE_CTL_CFGACC, pcie->base + CORE_LANE_CTL(port->idx));
+ rmwl(0, CORE_LANE_CFG_REFCLK0REQ, pcie->base + CORE_LANE_CFG(port->idx));
+
+ res = readl_relaxed_poll_timeout(pcie->base + CORE_LANE_CFG(port->idx),
+ stat, stat & CORE_LANE_CFG_REFCLK0ACK,
+ 100, 50000);
+ if (res < 0)
+ return res;
+
+ rmwl(0, CORE_LANE_CFG_REFCLK1, pcie->base + CORE_LANE_CFG(port->idx));
+ res = readl_relaxed_poll_timeout(pcie->base + CORE_LANE_CFG(port->idx),
+ stat, stat & CORE_LANE_CFG_REFCLK1,
+ 100, 50000);
+
+ if (res < 0)
+ return res;
+
+ rmwl(CORE_LANE_CTL_CFGACC, 0, pcie->base + CORE_LANE_CTL(port->idx));
+
+ /* Flush writes before enabling the clocks */
+ dma_wmb();
+
+ rmwl(0, CORE_LANE_CFG_REFCLKEN, pcie->base + CORE_LANE_CFG(port->idx));
+ rmwl(0, PORT_REFCLK_EN, port->base + PORT_REFCLK);
+
+ return 0;
+}
+
static int apple_pcie_setup_port(struct apple_pcie *pcie,
struct device_node *np)
{
@@ -165,6 +205,10 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
rmwl(0, PORT_APPCLK_EN, port + PORT_APPCLK);
+ ret = apple_pcie_setup_refclk(pcie, port);
+ if (ret < 0)
+ return ret;
+
rmwl(0, PORT_PERST_OFF, port->base + PORT_PERST);
gpiod_set_value(reset, 1);
--
2.30.2
> +config PCIE_APPLE_MSI_DOORBELL_ADDR
> + hex
> + default 0xfffff000
> + depends on PCIE_APPLE
> +
> config PCIE_APPLE
> tristate "Apple PCIe controller"
> depends on ARCH_APPLE || COMPILE_TEST
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 1ed7b90f8360..76344223245d 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -120,8 +120,10 @@
> * The doorbell address is set to 0xfffff000, which by convention
> * matches what MacOS does, and it is possible to use any other
> * address (in the bottom 4GB, as the base register is only 32bit).
> + * However, it has to be excluded from the the IOVA range, and the
> + * DART driver has to know about it.
> */
> -#define DOORBELL_ADDR 0xfffff000
> +#define DOORBELL_ADDR CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR
I'm unsure if Kconfig is the right place for this. But if it is, these
hunks should be moved earlier in the series (so the deletion gets
squashed away of the hardcoded-in-the-C.)
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 <[email protected]>
> ---
> 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.
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.
Sven
On Mon, Sep 13, 2021, at 20:25, Marc Zyngier wrote:
> From: Alyssa Rosenzweig <[email protected]>
>
> Add a minimal driver to bring up the PCIe bus on Apple system-on-chips,
> particularly the Apple M1. This driver exposes the internal bus used for
> the USB type-A ports, Ethernet, Wi-Fi, and Bluetooth. Bringing up the
> radios requires additional drivers beyond what's necessary for PCIe
> itself.
>
> At this stage, nothing is functionnal.
>
> Co-developed-by: Stan Skowronek <[email protected]>
> Signed-off-by: Stan Skowronek <[email protected]>
> Signed-off-by: Alyssa Rosenzweig <[email protected]>
> Signed-off-by: Marc Zyngier <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> ---
> MAINTAINERS | 7 +
> drivers/pci/controller/Kconfig | 12 ++
> drivers/pci/controller/Makefile | 1 +
> drivers/pci/controller/pcie-apple.c | 243 ++++++++++++++++++++++++++++
> 4 files changed, 263 insertions(+)
> create mode 100644 drivers/pci/controller/pcie-apple.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 813a847e2d64..9905cc48fed9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1280,6 +1280,13 @@ S: Maintained
> F: Documentation/devicetree/bindings/iommu/apple,dart.yaml
> F: drivers/iommu/apple-dart.c
>
> +APPLE PCIE CONTROLLER DRIVER
> +M: Alyssa Rosenzweig <[email protected]>
> +M: Marc Zyngier <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/pci/controller/pcie-apple.c
> +
> APPLE SMC DRIVER
> M: Henrik Rydberg <[email protected]>
> L: [email protected]
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 326f7d13024f..814833a8120d 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -312,6 +312,18 @@ config PCIE_HISI_ERR
> Say Y here if you want error handling support
> for the PCIe controller's errors on HiSilicon HIP SoCs
>
> +config PCIE_APPLE
> + tristate "Apple PCIe controller"
> + depends on ARCH_APPLE || COMPILE_TEST
> + depends on OF
> + depends on PCI_MSI_IRQ_DOMAIN
> + help
> + Say Y here if you want to enable PCIe controller support on Apple
> + system-on-chips, like the Apple M1. This is required for the USB
> + type-A ports, Ethernet, Wi-Fi, and Bluetooth.
> +
> + If unsure, say Y if you have an Apple Silicon system.
> +
> source "drivers/pci/controller/dwc/Kconfig"
> source "drivers/pci/controller/mobiveil/Kconfig"
> source "drivers/pci/controller/cadence/Kconfig"
> diff --git a/drivers/pci/controller/Makefile
> b/drivers/pci/controller/Makefile
> index aaf30b3dcc14..f9d40bad932c 100644
> --- a/drivers/pci/controller/Makefile
> +++ b/drivers/pci/controller/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_VMD) += vmd.o
> obj-$(CONFIG_PCIE_BRCMSTB) += pcie-brcmstb.o
> obj-$(CONFIG_PCI_LOONGSON) += pci-loongson.o
> obj-$(CONFIG_PCIE_HISI_ERR) += pcie-hisi-error.o
> +obj-$(CONFIG_PCIE_APPLE) += pcie-apple.o
> # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
> obj-y += dwc/
> obj-y += mobiveil/
> diff --git a/drivers/pci/controller/pcie-apple.c
> b/drivers/pci/controller/pcie-apple.c
> new file mode 100644
> index 000000000000..f3c414950a10
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -0,0 +1,243 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCIe host bridge driver for Apple system-on-chips.
> + *
> + * The HW is ECAM compliant, so once the controller is initialized,
> + * the driver mostly deals MSI mapping and handling of per-port
> + * interrupts (INTx, management and error signals).
> + *
> + * Initialization requires enabling power and clocks, along with a
> + * number of register pokes.
> + *
> + * Copyright (C) 2021 Alyssa Rosenzweig <[email protected]>
> + * Copyright (C) 2021 Google LLC
> + * Copyright (C) 2021 Corellium LLC
> + * Copyright (C) 2021 Mark Kettenis <[email protected]>
> + *
> + * Author: Alyssa Rosenzweig <[email protected]>
> + * Author: Marc Zyngier <[email protected]>
> + */
> +
> [...]
> +
> +static inline void rmwl(u32 clr, u32 set, void __iomem *addr)
> +{
> + writel_relaxed((readl_relaxed(addr) & ~clr) | set, addr);
> +}
This helper is a bit strange, especially since it's always only used
with either clr != 0 or set != 0 but never (clr = 0 and set = 0) afaict.
Maybe create two instead for setting and clearing bits?
> +
> +static int apple_pcie_setup_port(struct apple_pcie *pcie,
> + struct device_node *np)
> +{
> + struct platform_device *platform = to_platform_device(pcie->dev);
> + struct apple_pcie_port *port;
> + struct gpio_desc *reset;
> + u32 stat, idx;
> + int ret;
> +
> + reset = gpiod_get_from_of_node(np, "reset-gpios", 0,
> + GPIOD_OUT_LOW, "#PERST");
> + if (IS_ERR(reset))
> + return PTR_ERR(reset);
> +
> + port = devm_kzalloc(pcie->dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> +
> + ret = of_property_read_u32_index(np, "reg", 0, &idx);
> + if (ret)
> + return ret;
> +
> + /* Use the first reg entry to work out the port index */
> + port->idx = idx >> 11;
> + port->pcie = pcie;
> + port->np = np;
> +
> + port->base = devm_platform_ioremap_resource(platform, port->idx + 2);
> + if (IS_ERR(port->base))
> + return -ENODEV;
> +
> + rmwl(0, PORT_APPCLK_EN, port + PORT_APPCLK);
> +
> + rmwl(0, PORT_PERST_OFF, port->base + PORT_PERST);
> + gpiod_set_value(reset, 1);
> +
> + ret = readl_relaxed_poll_timeout(port->base + PORT_STATUS, stat,
> + stat & PORT_STATUS_READY, 100, 250000);
> + if (ret < 0) {
> + dev_err(pcie->dev, "port %pOF ready wait timeout\n", np);
> + return ret;
> + }
> +
> + /* Flush writes and enable the link */
> + dma_wmb();
This is a DMA barrier but there's no DMA you need to order this against
here. I think you can just drop it.
> +
> + writel_relaxed(PORT_LTSSMCTL_START, port->base + PORT_LTSSMCTL);
> +
> + return 0;
> +}
> +
Sven
On Mon, Sep 13, 2021 at 1:26 PM Marc Zyngier <[email protected]> wrote:
>
> Just as we now allow an interrupt map to be parsed when part
> of an interrupt controller, there is no reason to ignore an
> interrupt map that would be part of a pci device node such as
> a root port since we already allow interrupt specifiers.
>
> This allows the device itself to use the interrupt map for
> for its own purpose.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> drivers/pci/of.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index d84381ce82b5..443cebb0622e 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -423,7 +423,7 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
> */
> static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
> {
> - struct device_node *dn, *ppnode;
> + struct device_node *dn, *ppnode = NULL;
> struct pci_dev *ppdev;
> __be32 laddr[3];
> u8 pin;
> @@ -452,8 +452,14 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *
> if (pin == 0)
> return -ENODEV;
>
> + /* Local interrupt-map in the device node? Use it! */
> + if (dn && of_get_property(dn, "interrupt-map", NULL)) {
No need to check 'dn' is not NULL.
Otherwise,
Reviewed-by: Rob Herring <[email protected]>
> + pin = pci_swizzle_interrupt_pin(pdev, pin);
> + ppnode = dn;
> + }
> +
> /* Now we walk up the PCI tree */
> - for (;;) {
> + while (!ppnode) {
> /* Get the pci_dev of our parent */
> ppdev = pdev->bus->self;
>
> --
> 2.30.2
>
On Mon, Sep 13, 2021 at 1:26 PM Marc Zyngier <[email protected]> wrote:
>
> of_irq_parse_raw() has a baked assumption that if a node has an
> interrupt-controller property, it cannot possibly also have an
> interrupt-map property (the latter being ignored).
>
> This seems to be an odd behaviour, and there are no reason why
s/are/is/
> we should avoid supporting this use case. This is specially
> useful when a PCI root port acts as an interrupt controller for
> PCI endpoints, such as this:
>
> pcie0: pcie@690000000 {
> [...]
> port00: pci@0,0 {
> device_type = "pci";
> [...]
> #address-cells = <3>;
>
> interrupt-controller;
> #interrupt-cells = <1>;
>
> interrupt-map-mask = <0 0 0 7>;
> interrupt-map = <0 0 0 1 &port00 0 0 0 0>,
> <0 0 0 2 &port00 0 0 0 1>,
> <0 0 0 3 &port00 0 0 0 2>,
> <0 0 0 4 &port00 0 0 0 3>;
> };
> };
>
> Handle it by detecting that we have an interrupt-map early in the
> parsing, and special case the situation where the phandle in the
> interrupt map refers to the current node (which is the interesting
> case here).
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> drivers/of/irq.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
Reviewed-by: Rob Herring <[email protected]>
Probe for the 'msi-ranges' property, and implement the MSI
support in the form of the usual two-level hierarchy.
Note that contrary to the wired interrupts, MSIs are shared among
all the ports.
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/pci/controller/pcie-apple.c | 168 +++++++++++++++++++++++++++-
1 file changed, 167 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index d174a215a47d..1ed7b90f8360 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -116,10 +116,22 @@
#define PORT_TUNSTAT_PERST_ACK_PEND BIT(1)
#define PORT_PREFMEM_ENABLE 0x00994
+/*
+ * The doorbell address is set to 0xfffff000, which by convention
+ * matches what MacOS does, and it is possible to use any other
+ * address (in the bottom 4GB, as the base register is only 32bit).
+ */
+#define DOORBELL_ADDR 0xfffff000
+
struct apple_pcie {
+ struct mutex lock;
struct device *dev;
void __iomem *base;
+ struct irq_domain *domain;
+ unsigned long *bitmap;
struct completion event;
+ struct irq_fwspec fwspec;
+ u32 nvecs;
};
struct apple_pcie_port {
@@ -135,6 +147,103 @@ static inline void rmwl(u32 clr, u32 set, void __iomem *addr)
writel_relaxed((readl_relaxed(addr) & ~clr) | set, addr);
}
+static void apple_msi_top_irq_mask(struct irq_data *d)
+{
+ pci_msi_mask_irq(d);
+ irq_chip_mask_parent(d);
+}
+
+static void apple_msi_top_irq_unmask(struct irq_data *d)
+{
+ pci_msi_unmask_irq(d);
+ irq_chip_unmask_parent(d);
+}
+
+static struct irq_chip apple_msi_top_chip = {
+ .name = "PCIe MSI",
+ .irq_mask = apple_msi_top_irq_mask,
+ .irq_unmask = apple_msi_top_irq_unmask,
+ .irq_eoi = irq_chip_eoi_parent,
+ .irq_set_affinity = irq_chip_set_affinity_parent,
+ .irq_set_type = irq_chip_set_type_parent,
+};
+
+static void apple_msi_compose_msg(struct irq_data *data, struct msi_msg *msg)
+{
+ BUILD_BUG_ON(upper_32_bits(DOORBELL_ADDR));
+
+ msg->address_hi = upper_32_bits(DOORBELL_ADDR);
+ msg->address_lo = lower_32_bits(DOORBELL_ADDR);
+ msg->data = data->hwirq;
+}
+
+static struct irq_chip apple_msi_bottom_chip = {
+ .name = "MSI",
+ .irq_mask = irq_chip_mask_parent,
+ .irq_unmask = irq_chip_unmask_parent,
+ .irq_eoi = irq_chip_eoi_parent,
+ .irq_set_affinity = irq_chip_set_affinity_parent,
+ .irq_set_type = irq_chip_set_type_parent,
+ .irq_compose_msi_msg = apple_msi_compose_msg,
+};
+
+static int apple_msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *args)
+{
+ struct apple_pcie *pcie = domain->host_data;
+ struct irq_fwspec fwspec = pcie->fwspec;
+ unsigned int i;
+ int ret, hwirq;
+
+ mutex_lock(&pcie->lock);
+
+ hwirq = bitmap_find_free_region(pcie->bitmap, pcie->nvecs,
+ order_base_2(nr_irqs));
+
+ mutex_unlock(&pcie->lock);
+
+ if (hwirq < 0)
+ return -ENOSPC;
+
+ fwspec.param[1] += hwirq;
+
+ ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &fwspec);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < nr_irqs; i++) {
+ irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+ &apple_msi_bottom_chip,
+ domain->host_data);
+ }
+
+ return 0;
+}
+
+static void apple_msi_domain_free(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs)
+{
+ struct irq_data *d = irq_domain_get_irq_data(domain, virq);
+ struct apple_pcie *pcie = domain->host_data;
+
+ mutex_lock(&pcie->lock);
+
+ bitmap_release_region(pcie->bitmap, d->hwirq, order_base_2(nr_irqs));
+
+ mutex_unlock(&pcie->lock);
+}
+
+static const struct irq_domain_ops apple_msi_domain_ops = {
+ .alloc = apple_msi_domain_alloc,
+ .free = apple_msi_domain_free,
+};
+
+static struct msi_domain_info apple_msi_info = {
+ .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+ MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX),
+ .chip = &apple_msi_top_chip,
+};
+
static void apple_port_irq_mask(struct irq_data *data)
{
struct apple_pcie_port *port = irq_data_get_irq_chip_data(data);
@@ -269,6 +378,14 @@ static int apple_pcie_port_setup_irq(struct apple_pcie_port *port)
irq_set_chained_handler_and_data(irq, apple_port_irq_handler, port);
+ /* Configure MSI base address */
+ writel_relaxed(lower_32_bits(DOORBELL_ADDR), port->base + PORT_MSIADDR);
+
+ /* 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);
+
return 0;
}
@@ -436,6 +553,55 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
return 0;
}
+static int apple_msi_init(struct apple_pcie *pcie)
+{
+ struct fwnode_handle *fwnode = dev_fwnode(pcie->dev);
+ struct of_phandle_args args = {};
+ struct irq_domain *parent;
+ int ret;
+
+ ret = of_parse_phandle_with_args(to_of_node(fwnode), "msi-ranges",
+ "#interrupt-cells", 0, &args);
+ if (ret)
+ return ret;
+
+ ret = of_property_read_u32_index(to_of_node(fwnode), "msi-ranges",
+ args.args_count + 1, &pcie->nvecs);
+ if (ret)
+ return ret;
+
+ of_phandle_args_to_fwspec(args.np, args.args, args.args_count,
+ &pcie->fwspec);
+
+ pcie->bitmap = devm_bitmap_zalloc(pcie->dev, pcie->nvecs, GFP_KERNEL);
+ if (!pcie->bitmap)
+ return -ENOMEM;
+
+ parent = irq_find_matching_fwspec(&pcie->fwspec, DOMAIN_BUS_WIRED);
+ if (!parent) {
+ dev_err(pcie->dev, "failed to find parent domain\n");
+ return -ENXIO;
+ }
+
+ parent = irq_domain_create_hierarchy(parent, 0, pcie->nvecs, fwnode,
+ &apple_msi_domain_ops, pcie);
+ if (!parent) {
+ dev_err(pcie->dev, "failed to create IRQ domain\n");
+ return -ENOMEM;
+ }
+ irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
+
+ pcie->domain = pci_msi_create_irq_domain(fwnode, &apple_msi_info,
+ parent);
+ if (!pcie->domain) {
+ dev_err(pcie->dev, "failed to create MSI domain\n");
+ irq_domain_remove(parent);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
static int apple_pcie_init(struct pci_config_window *cfg)
{
struct device *dev = cfg->parent;
@@ -465,7 +631,7 @@ static int apple_pcie_init(struct pci_config_window *cfg)
}
}
- return 0;
+ return apple_msi_init(pcie);
}
static const struct pci_ecam_ops apple_pcie_cfg_ecam_ops = {
--
2.30.2
On Mon, 13 Sep 2021 21:45:13 +0100,
"Sven Peter" <[email protected]> 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 <[email protected]>
> > ---
> > 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));
--
Without deviation from the norm, progress is not possible.
> Date: Tue, 14 Sep 2021 10:35:32 +0100
> From: Marc Zyngier <[email protected]>
>
> On Mon, 13 Sep 2021 21:45:13 +0100,
> "Sven Peter" <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
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 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.
Cheers,
Mark
> 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));
>
> --
> Without deviation from the norm, progress is not possible.
>
On Mon, Sep 13, 2021, at 20:25, Marc Zyngier wrote:
> The MSI doorbell on Apple HW can be any address in the low 4GB
> range. However, the MSI write is matched by the PCIe block before
> hitting the iommu. It must thus be excluded from the IOVA range
> that is assigned to any PCIe device.
>
> Signed-off-by: Marc Zyngier <[email protected]>
It's not pretty but I'm not aware of any better solution and this should
work as long as these two are always paired. With the small nit below
addressed:
Reviewed-by: Sven Peter <[email protected]>
> ---
> drivers/iommu/apple-dart.c | 25 +++++++++++++++++++++++++
> drivers/pci/controller/Kconfig | 5 +++++
> drivers/pci/controller/pcie-apple.c | 4 +++-
> 3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
> index 559db9259e65..d1456663688e 100644
> --- a/drivers/iommu/apple-dart.c
> +++ b/drivers/iommu/apple-dart.c
> @@ -721,6 +721,29 @@ static int apple_dart_def_domain_type(struct device *dev)
> return 0;
> }
>
> +#define DOORBELL_ADDR (CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR & PAGE_MASK)
> +
> +static void apple_dart_get_resv_regions(struct device *dev,
> + struct list_head *head)
> +{
> +#ifdef CONFIG_PCIE_APPLE
I think using IS_ENABLED would be better here in case the pcie driver is built as
a module which would then only define CONFIG_PCIE_APPLE_MODULE AIUI.
Thanks,
Sven
On Tue, Sep 14, 2021, at 11:35, Marc Zyngier wrote:
> On Mon, 13 Sep 2021 21:45:13 +0100,
> "Sven Peter" <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
Thanks,
Sven
On Mon, Sep 13, 2021 at 07:25:43PM +0100, Marc Zyngier wrote:
> Just as we now allow an interrupt map to be parsed when part
> of an interrupt controller, there is no reason to ignore an
> interrupt map that would be part of a pci device node such as
> a root port since we already allow interrupt specifiers.
>
> This allows the device itself to use the interrupt map for
> for its own purpose.
>
> Signed-off-by: Marc Zyngier <[email protected]>
Acked-by: Bjorn Helgaas <[email protected]>
I assume Lorenzo would merge this along with the rest of the series.
If I were applying I would silently wrap to 75 characters, s/pci/PCI/
in subject and above, and maybe incorporate a version of the subject
line in the commit log so it says what the patch does.
> ---
> drivers/pci/of.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index d84381ce82b5..443cebb0622e 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -423,7 +423,7 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
> */
> static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
> {
> - struct device_node *dn, *ppnode;
> + struct device_node *dn, *ppnode = NULL;
> struct pci_dev *ppdev;
> __be32 laddr[3];
> u8 pin;
> @@ -452,8 +452,14 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *
> if (pin == 0)
> return -ENODEV;
>
> + /* Local interrupt-map in the device node? Use it! */
> + if (dn && of_get_property(dn, "interrupt-map", NULL)) {
> + pin = pci_swizzle_interrupt_pin(pdev, pin);
> + ppnode = dn;
> + }
> +
> /* Now we walk up the PCI tree */
> - for (;;) {
> + while (!ppnode) {
> /* Get the pci_dev of our parent */
> ppdev = pdev->bus->self;
>
> --
> 2.30.2
>
On Mon, 13 Sep 2021 21:48:47 +0100,
"Sven Peter" <[email protected]> wrote:
>
>
>
> On Mon, Sep 13, 2021, at 20:25, Marc Zyngier wrote:
> > From: Alyssa Rosenzweig <[email protected]>
> >
> > Add a minimal driver to bring up the PCIe bus on Apple system-on-chips,
> > particularly the Apple M1. This driver exposes the internal bus used for
> > the USB type-A ports, Ethernet, Wi-Fi, and Bluetooth. Bringing up the
> > radios requires additional drivers beyond what's necessary for PCIe
> > itself.
> >
> > At this stage, nothing is functionnal.
> >
> > Co-developed-by: Stan Skowronek <[email protected]>
> > Signed-off-by: Stan Skowronek <[email protected]>
> > Signed-off-by: Alyssa Rosenzweig <[email protected]>
> > Signed-off-by: Marc Zyngier <[email protected]>
> > Link: https://lore.kernel.org/r/[email protected]
> > ---
> > MAINTAINERS | 7 +
> > drivers/pci/controller/Kconfig | 12 ++
> > drivers/pci/controller/Makefile | 1 +
> > drivers/pci/controller/pcie-apple.c | 243 ++++++++++++++++++++++++++++
> > 4 files changed, 263 insertions(+)
> > create mode 100644 drivers/pci/controller/pcie-apple.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 813a847e2d64..9905cc48fed9 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1280,6 +1280,13 @@ S: Maintained
> > F: Documentation/devicetree/bindings/iommu/apple,dart.yaml
> > F: drivers/iommu/apple-dart.c
> >
> > +APPLE PCIE CONTROLLER DRIVER
> > +M: Alyssa Rosenzweig <[email protected]>
> > +M: Marc Zyngier <[email protected]>
> > +L: [email protected]
> > +S: Maintained
> > +F: drivers/pci/controller/pcie-apple.c
> > +
> > APPLE SMC DRIVER
> > M: Henrik Rydberg <[email protected]>
> > L: [email protected]
> > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > index 326f7d13024f..814833a8120d 100644
> > --- a/drivers/pci/controller/Kconfig
> > +++ b/drivers/pci/controller/Kconfig
> > @@ -312,6 +312,18 @@ config PCIE_HISI_ERR
> > Say Y here if you want error handling support
> > for the PCIe controller's errors on HiSilicon HIP SoCs
> >
> > +config PCIE_APPLE
> > + tristate "Apple PCIe controller"
> > + depends on ARCH_APPLE || COMPILE_TEST
> > + depends on OF
> > + depends on PCI_MSI_IRQ_DOMAIN
> > + help
> > + Say Y here if you want to enable PCIe controller support on Apple
> > + system-on-chips, like the Apple M1. This is required for the USB
> > + type-A ports, Ethernet, Wi-Fi, and Bluetooth.
> > +
> > + If unsure, say Y if you have an Apple Silicon system.
> > +
> > source "drivers/pci/controller/dwc/Kconfig"
> > source "drivers/pci/controller/mobiveil/Kconfig"
> > source "drivers/pci/controller/cadence/Kconfig"
> > diff --git a/drivers/pci/controller/Makefile
> > b/drivers/pci/controller/Makefile
> > index aaf30b3dcc14..f9d40bad932c 100644
> > --- a/drivers/pci/controller/Makefile
> > +++ b/drivers/pci/controller/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_VMD) += vmd.o
> > obj-$(CONFIG_PCIE_BRCMSTB) += pcie-brcmstb.o
> > obj-$(CONFIG_PCI_LOONGSON) += pci-loongson.o
> > obj-$(CONFIG_PCIE_HISI_ERR) += pcie-hisi-error.o
> > +obj-$(CONFIG_PCIE_APPLE) += pcie-apple.o
> > # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
> > obj-y += dwc/
> > obj-y += mobiveil/
> > diff --git a/drivers/pci/controller/pcie-apple.c
> > b/drivers/pci/controller/pcie-apple.c
> > new file mode 100644
> > index 000000000000..f3c414950a10
> > --- /dev/null
> > +++ b/drivers/pci/controller/pcie-apple.c
> > @@ -0,0 +1,243 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * PCIe host bridge driver for Apple system-on-chips.
> > + *
> > + * The HW is ECAM compliant, so once the controller is initialized,
> > + * the driver mostly deals MSI mapping and handling of per-port
> > + * interrupts (INTx, management and error signals).
> > + *
> > + * Initialization requires enabling power and clocks, along with a
> > + * number of register pokes.
> > + *
> > + * Copyright (C) 2021 Alyssa Rosenzweig <[email protected]>
> > + * Copyright (C) 2021 Google LLC
> > + * Copyright (C) 2021 Corellium LLC
> > + * Copyright (C) 2021 Mark Kettenis <[email protected]>
> > + *
> > + * Author: Alyssa Rosenzweig <[email protected]>
> > + * Author: Marc Zyngier <[email protected]>
> > + */
> > +
> > [...]
> > +
> > +static inline void rmwl(u32 clr, u32 set, void __iomem *addr)
> > +{
> > + writel_relaxed((readl_relaxed(addr) & ~clr) | set, addr);
> > +}
>
> This helper is a bit strange, especially since it's always only used
> with either clr != 0 or set != 0 but never (clr = 0 and set = 0) afaict.
> Maybe create two instead for setting and clearing bits?
That's indeed nicer, and it makes the code more readable.
>
> > +
> > +static int apple_pcie_setup_port(struct apple_pcie *pcie,
> > + struct device_node *np)
> > +{
> > + struct platform_device *platform = to_platform_device(pcie->dev);
> > + struct apple_pcie_port *port;
> > + struct gpio_desc *reset;
> > + u32 stat, idx;
> > + int ret;
> > +
> > + reset = gpiod_get_from_of_node(np, "reset-gpios", 0,
> > + GPIOD_OUT_LOW, "#PERST");
> > + if (IS_ERR(reset))
> > + return PTR_ERR(reset);
> > +
> > + port = devm_kzalloc(pcie->dev, sizeof(*port), GFP_KERNEL);
> > + if (!port)
> > + return -ENOMEM;
> > +
> > + ret = of_property_read_u32_index(np, "reg", 0, &idx);
> > + if (ret)
> > + return ret;
> > +
> > + /* Use the first reg entry to work out the port index */
> > + port->idx = idx >> 11;
> > + port->pcie = pcie;
> > + port->np = np;
> > +
> > + port->base = devm_platform_ioremap_resource(platform, port->idx + 2);
> > + if (IS_ERR(port->base))
> > + return -ENODEV;
> > +
> > + rmwl(0, PORT_APPCLK_EN, port + PORT_APPCLK);
> > +
> > + rmwl(0, PORT_PERST_OFF, port->base + PORT_PERST);
> > + gpiod_set_value(reset, 1);
> > +
> > + ret = readl_relaxed_poll_timeout(port->base + PORT_STATUS, stat,
> > + stat & PORT_STATUS_READY, 100, 250000);
> > + if (ret < 0) {
> > + dev_err(pcie->dev, "port %pOF ready wait timeout\n", np);
> > + return ret;
> > + }
> > +
> > + /* Flush writes and enable the link */
> > + dma_wmb();
>
> This is a DMA barrier but there's no DMA you need to order this against
> here. I think you can just drop it.
Indeed, this is all MMIO based, and doesn't refer to anything in
memory. Barrier gone.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
> Date: Fri, 17 Sep 2021 10:19:02 +0100
> From: Marc Zyngier <[email protected]>
>
> On Tue, 14 Sep 2021 10:56:05 +0100,
> Mark Kettenis <[email protected]> wrote:
> >
> > > Date: Tue, 14 Sep 2021 10:35:32 +0100
> > > From: Marc Zyngier <[email protected]>
> > >
> > > On Mon, 13 Sep 2021 21:45:13 +0100,
> > > "Sven Peter" <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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.
On Mon, 13 Sep 2021 19:55:53 +0100,
Alyssa Rosenzweig <[email protected]> wrote:
>
> > +config PCIE_APPLE_MSI_DOORBELL_ADDR
> > + hex
> > + default 0xfffff000
> > + depends on PCIE_APPLE
> > +
> > config PCIE_APPLE
> > tristate "Apple PCIe controller"
> > depends on ARCH_APPLE || COMPILE_TEST
> > diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> > index 1ed7b90f8360..76344223245d 100644
> > --- a/drivers/pci/controller/pcie-apple.c
> > +++ b/drivers/pci/controller/pcie-apple.c
> > @@ -120,8 +120,10 @@
> > * The doorbell address is set to 0xfffff000, which by convention
> > * matches what MacOS does, and it is possible to use any other
> > * address (in the bottom 4GB, as the base register is only 32bit).
> > + * However, it has to be excluded from the the IOVA range, and the
> > + * DART driver has to know about it.
> > */
> > -#define DOORBELL_ADDR 0xfffff000
> > +#define DOORBELL_ADDR CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR
>
> I'm unsure if Kconfig is the right place for this. But if it is, these
> hunks should be moved earlier in the series (so the deletion gets
> squashed away of the hardcoded-in-the-C.)
I'd rather not doing that. There is a progression in the series, and
moving the value over to Kconfig is part of that progression.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Tue, 14 Sep 2021 10:56:05 +0100,
Mark Kettenis <[email protected]> wrote:
>
> > Date: Tue, 14 Sep 2021 10:35:32 +0100
> > From: Marc Zyngier <[email protected]>
> >
> > On Mon, 13 Sep 2021 21:45:13 +0100,
> > "Sven Peter" <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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.
On Tue, 14 Sep 2021 14:54:07 +0100,
"Sven Peter" <[email protected]> wrote:
>
>
>
> On Mon, Sep 13, 2021, at 20:25, Marc Zyngier wrote:
> > The MSI doorbell on Apple HW can be any address in the low 4GB
> > range. However, the MSI write is matched by the PCIe block before
> > hitting the iommu. It must thus be excluded from the IOVA range
> > that is assigned to any PCIe device.
> >
> > Signed-off-by: Marc Zyngier <[email protected]>
>
> It's not pretty but I'm not aware of any better solution and this should
> work as long as these two are always paired. With the small nit below
> addressed:
>
> Reviewed-by: Sven Peter <[email protected]>
>
> > ---
> > drivers/iommu/apple-dart.c | 25 +++++++++++++++++++++++++
> > drivers/pci/controller/Kconfig | 5 +++++
> > drivers/pci/controller/pcie-apple.c | 4 +++-
> > 3 files changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
> > index 559db9259e65..d1456663688e 100644
> > --- a/drivers/iommu/apple-dart.c
> > +++ b/drivers/iommu/apple-dart.c
> > @@ -721,6 +721,29 @@ static int apple_dart_def_domain_type(struct device *dev)
> > return 0;
> > }
> >
> > +#define DOORBELL_ADDR (CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR & PAGE_MASK)
> > +
> > +static void apple_dart_get_resv_regions(struct device *dev,
> > + struct list_head *head)
> > +{
> > +#ifdef CONFIG_PCIE_APPLE
>
> I think using IS_ENABLED would be better here in case the pcie
> driver is built as a module which would then only define
> CONFIG_PCIE_APPLE_MODULE AIUI.
You're right, this isn't great. However, IS_ENABLED() still results in
the evaluation of the following code by the compiler, even if it would
be dropped by the optimiser.
I resorted to the following construct:
<quote>
#ifndef CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR
/* Keep things compiling when CONFIG_PCI_APPLE isn't selected */
#define CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR 0
#endif
#define DOORBELL_ADDR (CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR & PAGE_MASK)
static void apple_dart_get_resv_regions(struct device *dev,
struct list_head *head)
{
if (IS_ENABLED(CONFIG_PCIE_APPLE) && dev_is_pci(dev)) {
</quote>
which is ugly, but works. The alternative is, of course, to add a new
include file for one single value, which I find even worse.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On 17/09/2021 18.20, Marc Zyngier wrote:
> On Mon, 13 Sep 2021 21:48:47 +0100,
>> On Mon, Sep 13, 2021, at 20:25, Marc Zyngier wrote:
>>> +static inline void rmwl(u32 clr, u32 set, void __iomem *addr)
>>> +{
>>> + writel_relaxed((readl_relaxed(addr) & ~clr) | set, addr);
>>> +}
>>
>> This helper is a bit strange, especially since it's always only used
>> with either clr != 0 or set != 0 but never (clr = 0 and set = 0) afaict.
>> Maybe create two instead for setting and clearing bits?
>
> That's indeed nicer, and it makes the code more readable.
This seems to be a pattern in Corellium code; the cpufreq code is the
same and I honestly found it quite hard to read when used for simple set
or clear operations. I find set32/clear32 style helpers much more
readable, so it's probably a good idea to make this change any time we
upstream stuff derived from their tree.
--
Hector Martin ([email protected])
Public Key: https://mrcn.st/pub
Thanks for giving this another push, the changes look great. The series
is
Tested-by: Alyssa Rosenzweig <[email protected]>
On Mon, Sep 13, 2021 at 07:25:40PM +0100, Marc Zyngier wrote:
> I have resumed my earlier effort to bring the Apple-M1 into the world
> of living by equipping it with a PCIe controller driver. Huge thanks
> to Alyssa Rosenzweig for kicking it into shape and providing the first
> two versions of this series.
>
> Much has changed since v2[2]. Mark Kettenis is doing a great job with
> the binding [0], so I have dropped that from the series, and strictly
> focused on the Linux side of thing. I am now using this binding as is,
> with the exception of a single line change, which I believe is a fix
> [1].
>
> Supporting the per-port interrupt controller has brought in a couple
> of fixes for the core DT code. Also, some work has gone into dealing
> with excluding the MSI page from the IOVA range, as well as
> programming the RID-to-SID mapper.
>
> Overall, the driver is now much cleaner and most probably feature
> complete when it comes to supporting internal devices (although I
> haven't investigated things like power management). TB support is
> another story, and will require some more hacking.
>
> This of course still depends on the clock and pinctrl drivers that are
> otherwise in flight, and will affect this driver one way or another.
> I have pushed a branch with all the dependencies (and more) at [3].
>
> * From v2 [2]:
> - Refactor DT parsing to match the new version of the binding
> - Add support for INTx and port-private interrupts
> - Signal link-up/down using interrupts
> - Export of_phandle_args_to_fwspec
> - Fix generic parsing of interrupt map
> - Rationalise port setup (data structure, self discovery)
> - Tell DART to exclude MSI doorbell from the IOVA mappings
> - Get rid of the setup bypass if the link was found up on boot
> - Prevent the module from being removed
> - Program the RID-to-SID mapper on device discovery
> - Rebased on 5.15-rc1
>
> [0] https://lore.kernel.org/r/[email protected]
> [1] https://lore.kernel.org/r/[email protected]
> [2] https://lore.kernel.org/r/[email protected]
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=hack/m1-pcie-v3
>
> Alyssa Rosenzweig (2):
> PCI: apple: Add initial hardware bring-up
> PCI: apple: Set up reference clocks when probing
>
> Marc Zyngier (8):
> irqdomain: Make of_phandle_args_to_fwspec generally available
> of/irq: Allow matching of an interrupt-map local to an interrupt
> controller
> PCI: of: Allow matching of an interrupt-map local to a pci device
> PCI: apple: Add INTx and per-port interrupt support
> arm64: apple: t8103: Add root port interrupt routing
> PCI: apple: Implement MSI support
> iommu/dart: Exclude MSI doorbell from PCIe device IOVA range
> PCI: apple: Configure RID to SID mapper on device addition
>
> MAINTAINERS | 7 +
> arch/arm64/boot/dts/apple/t8103.dtsi | 33 +-
> drivers/iommu/apple-dart.c | 25 +
> drivers/of/irq.c | 17 +-
> drivers/pci/controller/Kconfig | 17 +
> drivers/pci/controller/Makefile | 1 +
> drivers/pci/controller/pcie-apple.c | 818 +++++++++++++++++++++++++++
> drivers/pci/of.c | 10 +-
> include/linux/irqdomain.h | 4 +
> kernel/irq/irqdomain.c | 6 +-
> 10 files changed, 925 insertions(+), 13 deletions(-)
> create mode 100644 drivers/pci/controller/pcie-apple.c
>
> --
> 2.30.2
>