| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 17 Sep 2021 10:20:21 +0100
From: Marc Zyngier <maz@...nel.org>
To: "Sven Peter" <sven@...npeter.dev>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pci@...r.kernel.org, "Bjorn Helgaas" <bhelgaas@...gle.com>,
"Rob Herring" <robh+dt@...nel.org>,
"Lorenzo Pieralisi" <lorenzo.pieralisi@....com>,
Krzysztof Wilczyński <kw@...ux.com>,
"Alyssa Rosenzweig" <alyssa@...enzweig.io>,
"Stan Skowronek" <stan@...ellium.com>,
"Mark Kettenis" <kettenis@...nbsd.org>,
"Hector Martin" <marcan@...can.st>,
"Robin Murphy" <Robin.Murphy@....com>, kernel-team@...roid.com
Subject: Re: [PATCH v3 04/10] PCI: apple: Add initial hardware bring-up
On Mon, 13 Sep 2021 21:48:47 +0100,
"Sven Peter" <sven@...npeter.dev> wrote:
>
>
>
> On Mon, Sep 13, 2021, at 20:25, Marc Zyngier wrote:
> > From: Alyssa Rosenzweig <alyssa@...enzweig.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@...ellium.com>
> > Signed-off-by: Stan Skowronek <stan@...ellium.com>
> > Signed-off-by: Alyssa Rosenzweig <alyssa@...enzweig.io>
> > Signed-off-by: Marc Zyngier <maz@...nel.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@...enzweig.io>
> > +M: Marc Zyngier <maz@...nel.org>
> > +L: linux-pci@...r.kernel.org
> > +S: Maintained
> > +F: drivers/pci/controller/pcie-apple.c
> > +
> > APPLE SMC DRIVER
> > M: Henrik Rydberg <rydberg@...math.org>
> > L: linux-hwmon@...r.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@...enzweig.io>
> > + * Copyright (C) 2021 Google LLC
> > + * Copyright (C) 2021 Corellium LLC
> > + * Copyright (C) 2021 Mark Kettenis <kettenis@...nbsd.org>
> > + *
> > + * Author: Alyssa Rosenzweig <alyssa@...enzweig.io>
> > + * Author: Marc Zyngier <maz@...nel.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?
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.
Powered by blists - more mailing lists