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;
User-Agent: Cyrus-JMAP/3.5.0-alpha0-1229-g7ca81dfce5-fm-20210908.005-g7ca81dfc
Mime-Version: 1.0
Message-Id: <6eb53661-e11e-4634-9fa5-5e7e62d57a15@www.fastmail.com>
In-Reply-To: <20210913182550.264165-5-maz@kernel.org>
References: <20210913182550.264165-1-maz@kernel.org>
 <20210913182550.264165-5-maz@kernel.org>
Date:   Mon, 13 Sep 2021 22:48:47 +0200
From:   "Sven Peter" <sven@svenpeter.dev>
To:     "Marc Zyngier" <maz@kernel.org>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
Cc:     "Bjorn Helgaas" <bhelgaas@google.com>,
        "Rob Herring" <robh+dt@kernel.org>,
        "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
        =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>,
        "Alyssa Rosenzweig" <alyssa@rosenzweig.io>,
        "Stan Skowronek" <stan@corellium.com>,
        "Mark Kettenis" <kettenis@openbsd.org>,
        "Hector Martin" <marcan@marcan.st>,
        "Robin Murphy" <Robin.Murphy@arm.com>, kernel-team@android.com
Subject: Re: [PATCH v3 04/10] PCI: apple: Add initial hardware bring-up
Content-Type: text/plain
Precedence: bulk


On Mon, Sep 13, 2021, at 20:25, Marc Zyngier wrote:
> From: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> 
> 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 <stan@corellium.com>
> Signed-off-by: Stan Skowronek <stan@corellium.com>
> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Link: https://lore.kernel.org/r/20210816031621.240268-3-alyssa@rosenzweig.io
> ---
>  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 <alyssa@rosenzweig.io>
> +M:	Marc Zyngier <maz@kernel.org>
> +L:	linux-pci@vger.kernel.org
> +S:	Maintained
> +F:	drivers/pci/controller/pcie-apple.c
> +
>  APPLE SMC DRIVER
>  M:	Henrik Rydberg <rydberg@bitmath.org>
>  L:	linux-hwmon@vger.kernel.org
> 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 <alyssa@rosenzweig.io>
> + * Copyright (C) 2021 Google LLC
> + * Copyright (C) 2021 Corellium LLC
> + * Copyright (C) 2021 Mark Kettenis <kettenis@openbsd.org>
> + *
> + * Author: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> + * Author: Marc Zyngier <maz@kernel.org>
> + */
> +
> [...]
> +
> +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