Received: by 2002:a05:6a10:c7d3:0:0:0:0 with SMTP id h19csp807102pxy; Sun, 15 Aug 2021 00:46:21 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyapPM69HVTi5s/UWFQt9KkJOzjROMSkzHDBKgabeAVE99OVRFCCrNwz8EihTx3W6B/+Jb7 X-Received: by 2002:a17:906:38db:: with SMTP id r27mr11004872ejd.338.1629013581172; Sun, 15 Aug 2021 00:46:21 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1629013581; cv=none; d=google.com; s=arc-20160816; b=bnA/vMll7jSDhBXtn3KeYyDJhO5AbDVU4XPPPssSQa5xD2uXjeiyGFUSH2lwfpyEng NOKIDlsFXaosMRG15bGr/x4WVos1jZSlxojMHwpQWjnzeo7moOzC98xW2QE8MaNXpv6/ dNeDiYFO7vzvmqmHeuhSmldkBLnHw9a3rHAbWhbS9YjeekRhVp/biUwmQ0rexutx2cVz Psa8llIEeTheI6IWUPM8Huc38GeIKeox39DDjolc81mBxhN363CH+7L6poL69VUTrQYE k8kJyjtmNGtJhCS59DnmGFQRkZXsy5ENkJWsgI1Ex4gecJa1G88D81gg7mTGiRAZ4PeU 3CDQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:user-agent:references:in-reply-to :subject:cc:to:from:message-id:date; bh=BLvM837YI4QvJiyryh/bkGV8v/vBJmnA7hD9hNUmvQg=; b=OO5QeQbeR0x55ATNlc5utB7B1RPX3a7XewjWkglhI61FLzNzwijgh8CpoRq3qtsjAn NDb+4uVfA4ty0Qe8TfeWpu9qpzw0M/gAaoxk2JCUQbTRg0biPlh5azjV3pArmlfUDn27 OPfcev/NWUc/Xsd3j72Q/cplmVvQvOF/8tnzbgHBUW6HY+doivEgWov0P32XzkMujR7W qa0JoQNy/XyMtduNcihShaHnylSpl3+tDsWZopwc28qMse5QkJ06BNMSbxEFQJ2jLb5K bq8X8vuFGo4nWXK+ez1onfRHwzWXYZddu5/7LCwGoXF2t4UKuwfkdfO0T3dxESqk7Lfq VFmg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id j23si8351338eje.719.2021.08.15.00.45.44; Sun, 15 Aug 2021 00:46:21 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236141AbhHOHnX (ORCPT + 99 others); Sun, 15 Aug 2021 03:43:23 -0400 Received: from mail.kernel.org ([198.145.29.99]:42614 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232774AbhHOHnW (ORCPT ); Sun, 15 Aug 2021 03:43:22 -0400 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 092BE60C40; Sun, 15 Aug 2021 07:42:53 +0000 (UTC) Received: from 109-170-232-56.xdsl.murphx.net ([109.170.232.56] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1mFAnD-00556V-01; Sun, 15 Aug 2021 08:42:51 +0100 Date: Sun, 15 Aug 2021 08:42:50 +0100 Message-ID: <87a6lj17d1.wl-maz@kernel.org> From: Marc Zyngier To: Alyssa Rosenzweig Cc: linux-pci@vger.kernel.org, Bjorn Helgaas , Rob Herring , Lorenzo Pieralisi , Krzysztof =?UTF-8?B?V2lsY3p5xYRza2k=?= , Stan Skowronek , Mark Kettenis , Sven Peter , Hector Martin , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 2/2] PCI: apple: Add driver for the Apple M1 In-Reply-To: <20210815042525.36878-3-alyssa@rosenzweig.io> References: <20210815042525.36878-1-alyssa@rosenzweig.io> <20210815042525.36878-3-alyssa@rosenzweig.io> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 109.170.232.56 X-SA-Exim-Rcpt-To: alyssa@rosenzweig.io, linux-pci@vger.kernel.org, bhelgaas@google.com, robh+dt@kernel.org, lorenzo.pieralisi@arm.com, kw@linux.com, stan@corellium.com, kettenis@openbsd.org, sven@svenpeter.dev, marcan@marcan.st, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 15 Aug 2021 05:25:25 +0100, Alyssa Rosenzweig wrote: > > Add a driver for the PCIe controller found in 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. This patch brings > up the USB type-A ports and Ethernet. Bringing up the radios requires > interfacing with the System Management Coprocessor via Apple's > mailboxes, so that's left to a future patch. > > In this state, the driver consists of two major parts: hardware > initialization and MSI handling. The hardware initialization is derived > from Mark Kettenis's U-Boot patches which in turn is derived from > Corellium's patches for the hardware. The rest of the driver is derived > from Marc Zyngier's driver for the hardware. This really needs to be split into multiple patches: - PCI probing - Clock management - MSI implementation A couple of comments below: > > Co-developed-by: Stan Skowronek > Signed-off-by: Stan Skowronek > Co-developed-by: Marc Zyngier > Signed-off-by: Marc Zyngier > Signed-off-by: Alyssa Rosenzweig > --- > MAINTAINERS | 1 + > drivers/pci/controller/Kconfig | 13 + > drivers/pci/controller/Makefile | 1 + > drivers/pci/controller/pcie-apple.c | 466 ++++++++++++++++++++++++++++ > 4 files changed, 481 insertions(+) > create mode 100644 drivers/pci/controller/pcie-apple.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index d7d2e1d1e2f2..f13f65a844f7 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1274,6 +1274,7 @@ M: Alyssa Rosenzweig > L: linux-pci@vger.kernel.org > S: Maintained > F: Documentation/devicetree/bindings/pci/apple,pcie.yaml > +F: drivers/pci/controller/pcie-apple.c > > APPLE SMC DRIVER > M: Henrik Rydberg > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > index 326f7d13024f..881a6a81f3e2 100644 > --- a/drivers/pci/controller/Kconfig > +++ b/drivers/pci/controller/Kconfig > @@ -312,6 +312,19 @@ 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 > + depends on GPIOLIB > + 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..08088e06460f > --- /dev/null > +++ b/drivers/pci/controller/pcie-apple.c > @@ -0,0 +1,466 @@ > +// 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 only needs MSI handling. Initialization requires enabling power and > + * clocks, along with a number of register pokes. > + * > + * Copyright (C) 2021 Google LLC > + * Copyright (C) 2021 Corellium LLC > + * Copyright (C) 2021 Mark Kettenis > + * Copyright (C) 2021 Alyssa Rosenzweig > + * Author: Marc Zyngier > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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_PHY_CTL 0x80000 > +#define CORE_PHY_CTL_CLK0REQ BIT(0) > +#define CORE_PHY_CTL_CLK1REQ BIT(1) > +#define CORE_PHY_CTL_CLK0ACK BIT(2) > +#define CORE_PHY_CTL_CLK1ACK BIT(3) > +#define CORE_PHY_CTL_RESET BIT(7) > +#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 BIT(31) > +#define PORT_INT_CPL_TIMEOUT BIT(23) > +#define PORT_INT_RID2SID_MAPERR BIT(22) > +#define PORT_INT_CPL_ABORT BIT(21) > +#define PORT_INT_MSI_BAD_DATA BIT(19) > +#define PORT_INT_MSI_ERR BIT(18) > +#define PORT_INT_REQADDR_GT32 BIT(17) > +#define PORT_INT_AF_TIMEOUT BIT(15) > +#define PORT_INT_LINK_DOWN BIT(14) > +#define PORT_INT_LINK_UP BIT(12) > +#define PORT_INT_LINK_BWMGMT BIT(11) > +#define PORT_INT_AER_MASK (15 << 4) > +#define PORT_INT_PORT_ERR BIT(4) > +#define PORT_INT_INTx(i) BIT(i) > +#define PORT_INT_INTxALL 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 > + > +/* The doorbell address is "well known" */ > +#define DOORBELL_ADDR 0xfffff000 > + > +/* The hardware exposes 3 ports. Port 0 (WiFi and Bluetooth) is special, as it > + * is power-gated by SMC to facilitate rfkill. > + */ > +enum apple_pcie_port { > + APPLE_PCIE_PORT_RADIO = 0, > + APPLE_PCIE_PORT_USB = 1, > + APPLE_PCIE_PORT_ETHERNET = 2, This really doesn't belong in a low-level PCIe controller driver. The ports should be purely generic. > + APPLE_PCIE_NUM_PORTS > +}; > + > +struct apple_pcie { > + u32 msi_base; > + u32 nvecs; > + struct mutex lock; > + struct device *dev; > + struct irq_domain *domain; > + unsigned long *bitmap; > + void __iomem *rc; > +}; > + > +static inline void rmwl(u32 clr, u32 set, void __iomem *addr) > +{ > + writel((readl(addr) & ~clr) | set, addr); Please use relaxed accessors. If the barriers are actually needed, please document what you are ordering against. This applies throughout the patch. This also begs the question: can this be called concurrently? > +} > + > +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 void apple_msi_top_irq_eoi(struct irq_data *d) > +{ > + irq_chip_eoi_parent(d); > +} Drop this and use the irq_chip_eoi_parent() directly in the irqchip. This was only here for debug. > + > +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 = apple_msi_top_irq_eoi, > + .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) > +{ > + msg->address_hi = 0; > + msg->address_lo = DOORBELL_ADDR; What was never clear is whether this doorbell address really is always at this location, or whether it is actually programmed by *something*. In any case, please use the lower_32bit/upper_32bit helpers, just in case we can move the address somewhere else. > + 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_set_affinity = irq_chip_set_affinity_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; > + 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.fwnode = domain->parent->fwnode; > + fwspec.param_count = 3; > + fwspec.param[0] = 0; > + fwspec.param[1] = hwirq + pcie->msi_base; > + fwspec.param[2] = IRQ_TYPE_EDGE_RISING; > + > + 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 int apple_pcie_setup_refclk(void __iomem *rc, > + void __iomem *port, > + unsigned int idx) > +{ > + u32 stat; > + int res; > + > + res = readl_poll_timeout(rc + CORE_RC_PHYIF_STAT, stat, > + stat & CORE_RC_PHYIF_STAT_REFCLK, 100, 50000); > + if (res < 0) > + return res; > + > + rmwl(0, CORE_LANE_CTL_CFGACC, rc + CORE_LANE_CTL(idx)); > + rmwl(0, CORE_LANE_CFG_REFCLK0REQ, rc + CORE_LANE_CFG(idx)); > + > + res = readl_poll_timeout(rc + CORE_LANE_CFG(idx), stat, > + stat & CORE_LANE_CFG_REFCLK0ACK, 100, 50000); > + if (res < 0) > + return res; > + > + rmwl(0, CORE_LANE_CFG_REFCLK1, rc + CORE_LANE_CFG(idx)); > + res = readl_poll_timeout(rc + CORE_LANE_CFG(idx), stat, > + stat & CORE_LANE_CFG_REFCLK1, 100, 50000); > + > + if (res < 0) > + return res; > + > + rmwl(CORE_LANE_CTL_CFGACC, 0, rc + CORE_LANE_CTL(idx)); > + udelay(1); > + rmwl(0, CORE_LANE_CFG_REFCLKEN, rc + CORE_LANE_CFG(idx)); > + > + rmwl(0, PORT_REFCLK_EN, port + PORT_REFCLK); > + > + return 0; > +} > + > +static int apple_pcie_setup_port(struct apple_pcie *pcie, unsigned int i) > +{ > + struct fwnode_handle *fwnode = dev_fwnode(pcie->dev); > + void __iomem *port; > + struct gpio_desc *reset; > + uint32_t stat; > + int ret; > + > + port = devm_of_iomap(pcie->dev, to_of_node(fwnode), i + 3, NULL); > + > + if (IS_ERR(port)) > + return -ENODEV; > + > + reset = devm_gpiod_get_index(pcie->dev, "reset", i, 0); > + if (IS_ERR(reset)) > + return PTR_ERR(reset); > + > + gpiod_direction_output(reset, 0); > + > + rmwl(0, PORT_APPCLK_EN, port + PORT_APPCLK); > + > + ret = apple_pcie_setup_refclk(pcie->rc, port, i); > + if (ret < 0) > + return ret; > + > + rmwl(0, PORT_PERST_OFF, port + PORT_PERST); > + gpiod_set_value(reset, 1); > + > + ret = readl_poll_timeout(port + PORT_STATUS, stat, > + stat & PORT_STATUS_READY, 100, 250000); > + if (ret < 0) { > + dev_err(pcie->dev, "port %u ready wait timeout\n", i); > + return ret; > + } > + > + rmwl(PORT_REFCLK_CGDIS, 0, port + PORT_REFCLK); > + rmwl(PORT_APPCLK_CGDIS, 0, port + PORT_APPCLK); > + > + ret = readl_poll_timeout(port + PORT_LINKSTS, stat, > + !(stat & PORT_LINKSTS_BUSY), 100, 250000); > + if (ret < 0) { > + dev_err(pcie->dev, "port %u link not busy timeout\n", i); > + return ret; > + } > + > + writel(0xfb512fff, port + PORT_INTMSKSET); Magic. What is this for? > + > + writel(PORT_INT_LINK_UP | PORT_INT_LINK_DOWN | PORT_INT_AF_TIMEOUT | > + PORT_INT_REQADDR_GT32 | PORT_INT_MSI_ERR | > + PORT_INT_MSI_BAD_DATA | PORT_INT_CPL_ABORT | > + PORT_INT_CPL_TIMEOUT | (1 << 26), port + PORT_INTSTAT); > + > + usleep_range(5000, 10000); > + > + rmwl(0, PORT_LTSSMCTL_START, port + PORT_LTSSMCTL); > + > + ret = readl_poll_timeout(port + PORT_LINKSTS, stat, > + stat & PORT_LINKSTS_UP, 100, 500000); > + if (ret < 0) { > + dev_err(pcie->dev, "port %u link up wait timeout\n", i); > + return ret; > + } I have the strong feeling that a lot of things in the above is to get an interrupt when the port reports an event. Why the polling then? > + > + writel(DOORBELL_ADDR, port + PORT_MSIADDR); > + writel(0, port + PORT_MSIBASE); So here you go, the MSI doorbell *is* configurable. Should it be placed somewhere else? Shouldn't it be configured before the link is up? > + writel((5 << PORT_MSICFG_L2MSINUM_SHIFT) | PORT_MSICFG_EN, > + port + PORT_MSICFG); Ah, that one actually makes sense (enables 32 MSIs for the port). > + > + return 0; > +} > + > +static int apple_msi_init(struct apple_pcie *pcie) > +{ > + struct fwnode_handle *fwnode = dev_fwnode(pcie->dev); > + struct device_node *parent_intc; > + struct irq_domain *parent; > + int ret, i; > + > + pcie->rc = devm_of_iomap(pcie->dev, to_of_node(fwnode), 1, NULL); > + > + if (IS_ERR(pcie->rc)) > + return -ENODEV; > + > + for (i = 0; i < APPLE_PCIE_NUM_PORTS; ++i) { > + /* Bringing up the radios requires SMC. Skip for now. */ > + if (i == APPLE_PCIE_PORT_RADIO) > + continue; Why? Shouldn't this be moved into the driver for the endpoint, rather than hardcoding something here which is likely to change from one system to another? If establishing the link actually requires talking to another part of the system, then it should be described in DT. > + > + ret = apple_pcie_setup_port(pcie, i); > + > + if (ret) { > + dev_err(pcie->dev, "Port %u setup fail: %d\n", i, ret); > + return ret; > + } > + } > + > + ret = of_property_read_u32_index(to_of_node(fwnode), "msi-interrupts", > + 0, &pcie->msi_base); > + if (ret) > + return ret; > + > + ret = of_property_read_u32_index(to_of_node(fwnode), "msi-interrupts", > + 1, &pcie->nvecs); > + if (ret) > + return ret; > + > + pcie->bitmap = devm_kcalloc(pcie->dev, BITS_TO_LONGS(pcie->nvecs), > + sizeof(long), GFP_KERNEL); > + if (!pcie->bitmap) > + return -ENOMEM; > + > + parent_intc = of_irq_find_parent(to_of_node(fwnode)); > + parent = irq_find_host(parent_intc); > + if (!parent_intc || !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_m1_pci_init(struct pci_config_window *cfg) > +{ > + struct device *dev = cfg->parent; > + struct apple_pcie *pcie; > + > + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); > + if (!pcie) > + return -ENOMEM; > + > + pcie->dev = dev; > + > + mutex_init(&pcie->lock); > + > + return apple_msi_init(pcie); > +} > + > +static const struct pci_ecam_ops apple_m1_cfg_ecam_ops = { > + .init = apple_m1_pci_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_pci_of_match[] = { > + { .compatible = "apple,pcie", .data = &apple_m1_cfg_ecam_ops }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, gen_pci_of_match); > + > +static struct platform_driver apple_pci_driver = { > + .driver = { > + .name = "pcie-apple", > + .of_match_table = apple_pci_of_match, > + }, > + .probe = pci_host_common_probe, > + .remove = pci_host_common_remove, > +}; > +module_platform_driver(apple_pci_driver); > + > +MODULE_LICENSE("GPL v2"); Thanks, M. -- Without deviation from the norm, progress is not possible.