2019-03-25 11:17:03

by Chocron, Jonathan

Subject: [PATCH] PCI: al: add pcie-al.c

From: Jonathan Chocron <[email protected]>

Adding support for Amazon's Annapurna Labs pcie driver.
The HW controller is based on Designware's IP.

The HW doesn't support accessing the Root port's config space via
ECAM, so we obtain its base address via a PNP0C02 device.
Furthermore, the DesignWare PCIe controller doesn't filter out
config transactions sent to devices 1 and up on its bus, so they
are filtered by the driver.
All subordinate buses do support ECAM access.

Implementing specific PCI config access functions involves:
- Adding an init function to obtain the Root Port's base address
from a PNP0C02 device.
- Adding a new entry in the mcfg quirk array

Co-developed-by: Vladimir Aerov <[email protected]>
Signed-off-by: Jonathan Chocron <[email protected]>
Signed-off-by: Vladimir Aerov <[email protected]>
Reviewed-by: Benjamin Herrenschmidt <[email protected]>
Reviewed-by: David Woodhouse <[email protected]>
---
MAINTAINERS | 6 +++
drivers/acpi/pci_mcfg.c | 12 +++++
drivers/pci/controller/dwc/Makefile | 1 +
drivers/pci/controller/dwc/pcie-al.c | 92 ++++++++++++++++++++++++++++++++++++
include/linux/pci-ecam.h | 1 +
5 files changed, 112 insertions(+)
create mode 100644 drivers/pci/controller/dwc/pcie-al.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 32d444476a90..7a17017f9f82 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11769,6 +11769,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
S: Supported
F: drivers/pci/controller/

+PCIE DRIVER FOR ANNAPURNA LABS
+M: Jonathan Chocron <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/pci/controller/dwc/pcie-al.c
+
PCIE DRIVER FOR AMLOGIC MESON
M: Yue Wang <[email protected]>
L: [email protected]
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index a4e8432fc2fb..b42be067fb83 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -52,6 +52,18 @@ struct mcfg_fixup {
static struct mcfg_fixup mcfg_quirks[] = {
/* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, ops, cfgres }, */

+#define AL_ECAM(table_id, rev, seg, ops) \
+ { "AMAZON", table_id, rev, seg, MCFG_BUS_ANY, ops }
+
+ AL_ECAM("GRAVITON", 0, 0, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 1, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 2, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 3, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 4, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 5, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 6, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 7, &al_pcie_ops),
+
#define QCOM_ECAM32(seg) \
{ "QCOM ", "QDF2432 ", 1, seg, MCFG_BUS_ANY, &pci_32b_ops }

diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index 7bcdcdf5024e..1ea773c0070d 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -28,5 +28,6 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
# depending on whether ACPI, the DT driver, or both are enabled.

ifdef CONFIG_PCI
+obj-$(CONFIG_ARM64) += pcie-al.o
obj-$(CONFIG_ARM64) += pcie-hisi.o
endif
diff --git a/drivers/pci/controller/dwc/pcie-al.c b/drivers/pci/controller/dwc/pcie-al.c
new file mode 100644
index 000000000000..019497c2c714
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-al.c
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe host controller driver for Amazon's Annapurna Labs IP (used in chips
+ * such as Graviton and Alpine)
+ *
+ * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Author: Jonathan Chocron <[email protected]>
+ */
+
+#include <linux/pci.h>
+#include <linux/pci-ecam.h>
+#include <linux/pci-acpi.h>
+#include "../../pci.h"
+
+#if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
+
+struct al_pcie_acpi {
+ void __iomem *dbi_base;
+};
+
+static void __iomem *al_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
+ int where)
+{
+ struct pci_config_window *cfg = bus->sysdata;
+ struct al_pcie_acpi *pcie = cfg->priv;
+ void __iomem *dbi_base = pcie->dbi_base;
+
+ if (bus->number == cfg->busr.start) {
+ /* The DW PCIe core doesn't filter out transactions to other
+ * devices/functions on the primary bus num, so we do this here.
+ */
+ if (PCI_SLOT(devfn) > 0)
+ return NULL;
+ else
+ return dbi_base + where;
+ }
+
+ return pci_ecam_map_bus(bus, devfn, where);
+}
+
+static int al_pcie_init(struct pci_config_window *cfg)
+{
+ struct device *dev = cfg->parent;
+ struct acpi_device *adev = to_acpi_device(dev);
+ struct acpi_pci_root *root = acpi_driver_data(adev);
+ struct al_pcie_acpi *al_pcie;
+ struct resource *res;
+ int ret;
+
+ al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
+ if (!al_pcie)
+ return -ENOMEM;
+
+ res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
+ if (!res)
+ return -ENOMEM;
+
+ ret = acpi_get_rc_resources(dev, "AMZN0001", root->segment, res);
+ if (ret) {
+ dev_err(dev, "can't get rc dbi base address for SEG %d\n",
+ root->segment);
+ return ret;
+ }
+
+ dev_dbg(dev, "Root port dbi res: %pR\n", res);
+
+ al_pcie->dbi_base = devm_pci_remap_cfg_resource(dev, res);
+ if (IS_ERR(al_pcie->dbi_base)) {
+ long err = PTR_ERR(al_pcie->dbi_base);
+
+ dev_err(dev, "couldn't remap dbi base %pR (err:%ld)\n",
+ res, err);
+ return err;
+ }
+
+ cfg->priv = al_pcie;
+
+ return 0;
+}
+
+struct pci_ecam_ops al_pcie_ops = {
+ .bus_shift = 20,
+ .init = al_pcie_init,
+ .pci_ops = {
+ .map_bus = al_pcie_map_bus,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+ }
+};
+
+#endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
index 29efa09d686b..a73164c85e78 100644
--- a/include/linux/pci-ecam.h
+++ b/include/linux/pci-ecam.h
@@ -56,6 +56,7 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
extern struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */
extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene PCIe v1 */
extern struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene PCIe v2.x */
+extern struct pci_ecam_ops al_pcie_ops; /* Amazon Annapurna Labs PCIe */
#endif

#ifdef CONFIG_PCI_HOST_COMMON
--
1.9.1

2019-03-25 13:00:45

by Bjorn Helgaas

Subject: Re: [PATCH] PCI: al: add pcie-al.c

Hi Jonathan,

Looks good to me. Looks like this hardware is sooooo close to Just
Working with the generic driver; it's too bad we have to add more ECAM
quirks, but sometimes life gives us lemons. Trivial comments below.
Lorenzo may have additional comments.

The subject should be something like:

PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

On Mon, Mar 25, 2019 at 01:07:20PM +0200, [email protected] wrote:
> From: Jonathan Chocron <[email protected]>
>
> Adding support for Amazon's Annapurna Labs pcie driver.

s/pcie/PCIe/

> The HW controller is based on Designware's IP.
>
> The HW doesn't support accessing the Root port's config space via
> ECAM, so we obtain its base address via a PNP0C02 device.

s/port's/Port's/

I think you're actually looking for a AMZN0001 device, not a PNP0C02
device. Your firmware might have a PNP0C02 _HID and AMZN0001 _CID, but
that's not relevant here since you're only filtering by AMZN0001.

> Furthermore, the DesignWare PCIe controller doesn't filter out
> config transactions sent to devices 1 and up on its bus, so they
> are filtered by the driver.

Separate paragraphs with blank lines.

> All subordinate buses do support ECAM access.
>
> Implementing specific PCI config access functions involves:
> - Adding an init function to obtain the Root Port's base address
> from a PNP0C02 device.

s/PNP0C02/AMZN0001/

> - Adding a new entry in the mcfg quirk array
>
> Co-developed-by: Vladimir Aerov <[email protected]>
> Signed-off-by: Jonathan Chocron <[email protected]>
> Signed-off-by: Vladimir Aerov <[email protected]>
> Reviewed-by: Benjamin Herrenschmidt <[email protected]>
> Reviewed-by: David Woodhouse <[email protected]>
> ---
> MAINTAINERS | 6 +++
> drivers/acpi/pci_mcfg.c | 12 +++++
> drivers/pci/controller/dwc/Makefile | 1 +
> drivers/pci/controller/dwc/pcie-al.c | 92 ++++++++++++++++++++++++++++++++++++
> include/linux/pci-ecam.h | 1 +
> 5 files changed, 112 insertions(+)
> create mode 100644 drivers/pci/controller/dwc/pcie-al.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 32d444476a90..7a17017f9f82 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11769,6 +11769,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
> S: Supported
> F: drivers/pci/controller/
>
> +PCIE DRIVER FOR ANNAPURNA LABS
> +M: Jonathan Chocron <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/pci/controller/dwc/pcie-al.c
> +
> PCIE DRIVER FOR AMLOGIC MESON
> M: Yue Wang <[email protected]>
> L: [email protected]
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index a4e8432fc2fb..b42be067fb83 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -52,6 +52,18 @@ struct mcfg_fixup {
> static struct mcfg_fixup mcfg_quirks[] = {
> /* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, ops, cfgres }, */
>
> +#define AL_ECAM(table_id, rev, seg, ops) \
> + { "AMAZON", table_id, rev, seg, MCFG_BUS_ANY, ops }
> +
> + AL_ECAM("GRAVITON", 0, 0, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 1, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 2, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 3, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 4, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 5, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 6, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 7, &al_pcie_ops),
> +
> #define QCOM_ECAM32(seg) \
> { "QCOM ", "QDF2432 ", 1, seg, MCFG_BUS_ANY, &pci_32b_ops }
>
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index 7bcdcdf5024e..1ea773c0070d 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -28,5 +28,6 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
> # depending on whether ACPI, the DT driver, or both are enabled.
>
> ifdef CONFIG_PCI
> +obj-$(CONFIG_ARM64) += pcie-al.o
> obj-$(CONFIG_ARM64) += pcie-hisi.o
> endif
> diff --git a/drivers/pci/controller/dwc/pcie-al.c b/drivers/pci/controller/dwc/pcie-al.c
> new file mode 100644
> index 000000000000..019497c2c714
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-al.c
> @@ -0,0 +1,92 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCIe host controller driver for Amazon's Annapurna Labs IP (used in chips
> + * such as Graviton and Alpine)
> + *
> + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + *
> + * Author: Jonathan Chocron <[email protected]>
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/pci-ecam.h>
> +#include <linux/pci-acpi.h>
> +#include "../../pci.h"
> +
> +#if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
> +
> +struct al_pcie_acpi {
> + void __iomem *dbi_base;
> +};
> +
> +static void __iomem *al_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> + int where)
> +{
> + struct pci_config_window *cfg = bus->sysdata;
> + struct al_pcie_acpi *pcie = cfg->priv;
> + void __iomem *dbi_base = pcie->dbi_base;
> +
> + if (bus->number == cfg->busr.start) {
> + /* The DW PCIe core doesn't filter out transactions to other
> + * devices/functions on the primary bus num, so we do this here.
> + */

Use usual multi-line comment style:

/*
* text ..
*/

> + if (PCI_SLOT(devfn) > 0)
> + return NULL;
> + else
> + return dbi_base + where;
> + }
> +
> + return pci_ecam_map_bus(bus, devfn, where);
> +}
> +
> +static int al_pcie_init(struct pci_config_window *cfg)
> +{
> + struct device *dev = cfg->parent;
> + struct acpi_device *adev = to_acpi_device(dev);
> + struct acpi_pci_root *root = acpi_driver_data(adev);
> + struct al_pcie_acpi *al_pcie;
> + struct resource *res;
> + int ret;
> +
> + al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
> + if (!al_pcie)
> + return -ENOMEM;
> +
> + res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
> + if (!res)
> + return -ENOMEM;
> +
> + ret = acpi_get_rc_resources(dev, "AMZN0001", root->segment, res);
> + if (ret) {
> + dev_err(dev, "can't get rc dbi base address for SEG %d\n",
> + root->segment);
> + return ret;
> + }
> +
> + dev_dbg(dev, "Root port dbi res: %pR\n", res);
> +
> + al_pcie->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> + if (IS_ERR(al_pcie->dbi_base)) {
> + long err = PTR_ERR(al_pcie->dbi_base);
> +
> + dev_err(dev, "couldn't remap dbi base %pR (err:%ld)\n",
> + res, err);
> + return err;
> + }
> +
> + cfg->priv = al_pcie;
> +
> + return 0;
> +}
> +
> +struct pci_ecam_ops al_pcie_ops = {
> + .bus_shift = 20,
> + .init = al_pcie_init,
> + .pci_ops = {
> + .map_bus = al_pcie_map_bus,
> + .read = pci_generic_config_read,
> + .write = pci_generic_config_write,
> + }
> +};
> +
> +#endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
> index 29efa09d686b..a73164c85e78 100644
> --- a/include/linux/pci-ecam.h
> +++ b/include/linux/pci-ecam.h
> @@ -56,6 +56,7 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
> extern struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */
> extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene PCIe v1 */
> extern struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene PCIe v2.x */
> +extern struct pci_ecam_ops al_pcie_ops; /* Amazon Annapurna Labs PCIe */
> #endif
>
> #ifdef CONFIG_PCI_HOST_COMMON
> --
> 1.9.1
>

2019-03-25 15:59:01

by Chocron, Jonathan

Subject: Re: [PATCH] PCI: al: add pcie-al.c

On 3/25/19 14:58, Bjorn Helgaas wrote:
> Hi Jonathan,
>
> Looks good to me. Looks like this hardware is sooooo close to Just
> Working with the generic driver; it's too bad we have to add more ECAM
> quirks, but sometimes life gives us lemons. Trivial comments below.
> Lorenzo may have additional comments.
>
> The subject should be something like:
>
> PCI: al: Add Amazon Annapurna Labs PCIe host controller driver
Done.
> On Mon, Mar 25, 2019 at 01:07:20PM +0200, [email protected] wrote:
>> From: Jonathan Chocron <[email protected]>
>>
>> Adding support for Amazon's Annapurna Labs pcie driver.
> s/pcie/PCIe/
>
Done.
>> The HW controller is based on Designware's IP.
>>
>> The HW doesn't support accessing the Root port's config space via
>> ECAM, so we obtain its base address via a PNP0C02 device.
> s/port's/Port's/
Done.
>
> I think you're actually looking for a AMZN0001 device, not a PNP0C02
> device. Your firmware might have a PNP0C02 _HID and AMZN0001 _CID, but
> that's not relevant here since you're only filtering by AMZN0001.
>
Actually, the _HID is AMZN0001 and the _CID is PNP0C02, so that's why

I used PNP0C02 (to state that it is of this "type"). Please let me know if

you still think it should be changed, so I'll integrate it into v2.

>> Furthermore, the DesignWare PCIe controller doesn't filter out
>> config transactions sent to devices 1 and up on its bus, so they
>> are filtered by the driver.
> Separate paragraphs with blank lines.
Done.
>
>> All subordinate buses do support ECAM access.
>>
>> Implementing specific PCI config access functions involves:
>> - Adding an init function to obtain the Root Port's base address
>> from a PNP0C02 device.
> s/PNP0C02/AMZN0001/
>
>> - Adding a new entry in the mcfg quirk array
>>
>> Co-developed-by: Vladimir Aerov <[email protected]>
>> Signed-off-by: Jonathan Chocron <[email protected]>
>> Signed-off-by: Vladimir Aerov <[email protected]>
>> Reviewed-by: Benjamin Herrenschmidt <[email protected]>
>> Reviewed-by: David Woodhouse <[email protected]>
>> ---
>> MAINTAINERS | 6 +++
>> drivers/acpi/pci_mcfg.c | 12 +++++
>> drivers/pci/controller/dwc/Makefile | 1 +
>> drivers/pci/controller/dwc/pcie-al.c | 92 ++++++++++++++++++++++++++++++++++++
>> include/linux/pci-ecam.h | 1 +
>> 5 files changed, 112 insertions(+)
>> create mode 100644 drivers/pci/controller/dwc/pcie-al.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 32d444476a90..7a17017f9f82 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11769,6 +11769,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
>> S: Supported
>> F: drivers/pci/controller/
>>
>> +PCIE DRIVER FOR ANNAPURNA LABS
>> +M: Jonathan Chocron <[email protected]>
>> +L: [email protected]
>> +S: Maintained
>> +F: drivers/pci/controller/dwc/pcie-al.c
>> +
>> PCIE DRIVER FOR AMLOGIC MESON
>> M: Yue Wang <[email protected]>
>> L: [email protected]
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index a4e8432fc2fb..b42be067fb83 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -52,6 +52,18 @@ struct mcfg_fixup {
>> static struct mcfg_fixup mcfg_quirks[] = {
>> /* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, ops, cfgres }, */
>>
>> +#define AL_ECAM(table_id, rev, seg, ops) \
>> + { "AMAZON", table_id, rev, seg, MCFG_BUS_ANY, ops }
>> +
>> + AL_ECAM("GRAVITON", 0, 0, &al_pcie_ops),
>> + AL_ECAM("GRAVITON", 0, 1, &al_pcie_ops),
>> + AL_ECAM("GRAVITON", 0, 2, &al_pcie_ops),
>> + AL_ECAM("GRAVITON", 0, 3, &al_pcie_ops),
>> + AL_ECAM("GRAVITON", 0, 4, &al_pcie_ops),
>> + AL_ECAM("GRAVITON", 0, 5, &al_pcie_ops),
>> + AL_ECAM("GRAVITON", 0, 6, &al_pcie_ops),
>> + AL_ECAM("GRAVITON", 0, 7, &al_pcie_ops),
>> +
>> #define QCOM_ECAM32(seg) \
>> { "QCOM ", "QDF2432 ", 1, seg, MCFG_BUS_ANY, &pci_32b_ops }
>>
>> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
>> index 7bcdcdf5024e..1ea773c0070d 100644
>> --- a/drivers/pci/controller/dwc/Makefile
>> +++ b/drivers/pci/controller/dwc/Makefile
>> @@ -28,5 +28,6 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
>> # depending on whether ACPI, the DT driver, or both are enabled.
>>
>> ifdef CONFIG_PCI
>> +obj-$(CONFIG_ARM64) += pcie-al.o
>> obj-$(CONFIG_ARM64) += pcie-hisi.o
>> endif
>> diff --git a/drivers/pci/controller/dwc/pcie-al.c b/drivers/pci/controller/dwc/pcie-al.c
>> new file mode 100644
>> index 000000000000..019497c2c714
>> --- /dev/null
>> +++ b/drivers/pci/controller/dwc/pcie-al.c
>> @@ -0,0 +1,92 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * PCIe host controller driver for Amazon's Annapurna Labs IP (used in chips
>> + * such as Graviton and Alpine)
>> + *
>> + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
>> + *
>> + * Author: Jonathan Chocron <[email protected]>
>> + */
>> +
>> +#include <linux/pci.h>
>> +#include <linux/pci-ecam.h>
>> +#include <linux/pci-acpi.h>
>> +#include "../../pci.h"
>> +
>> +#if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
>> +
>> +struct al_pcie_acpi {
>> + void __iomem *dbi_base;
>> +};
>> +
>> +static void __iomem *al_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>> + int where)
>> +{
>> + struct pci_config_window *cfg = bus->sysdata;
>> + struct al_pcie_acpi *pcie = cfg->priv;
>> + void __iomem *dbi_base = pcie->dbi_base;
>> +
>> + if (bus->number == cfg->busr.start) {
>> + /* The DW PCIe core doesn't filter out transactions to other
>> + * devices/functions on the primary bus num, so we do this here.
>> + */
> Use usual multi-line comment style:
>
> /*
> * text ..
> */
Done.
>> + if (PCI_SLOT(devfn) > 0)
>> + return NULL;
>> + else
>> + return dbi_base + where;
>> + }
>> +
>> + return pci_ecam_map_bus(bus, devfn, where);
>> +}
>> +
>> +static int al_pcie_init(struct pci_config_window *cfg)
>> +{
>> + struct device *dev = cfg->parent;
>> + struct acpi_device *adev = to_acpi_device(dev);
>> + struct acpi_pci_root *root = acpi_driver_data(adev);
>> + struct al_pcie_acpi *al_pcie;
>> + struct resource *res;
>> + int ret;
>> +
>> + al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
>> + if (!al_pcie)
>> + return -ENOMEM;
>> +
>> + res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
>> + if (!res)
>> + return -ENOMEM;
>> +
>> + ret = acpi_get_rc_resources(dev, "AMZN0001", root->segment, res);
>> + if (ret) {
>> + dev_err(dev, "can't get rc dbi base address for SEG %d\n",
>> + root->segment);
>> + return ret;
>> + }
>> +
>> + dev_dbg(dev, "Root port dbi res: %pR\n", res);
>> +
>> + al_pcie->dbi_base = devm_pci_remap_cfg_resource(dev, res);
>> + if (IS_ERR(al_pcie->dbi_base)) {
>> + long err = PTR_ERR(al_pcie->dbi_base);
>> +
>> + dev_err(dev, "couldn't remap dbi base %pR (err:%ld)\n",
>> + res, err);
>> + return err;
>> + }
>> +
>> + cfg->priv = al_pcie;
>> +
>> + return 0;
>> +}
>> +
>> +struct pci_ecam_ops al_pcie_ops = {
>> + .bus_shift = 20,
>> + .init = al_pcie_init,
>> + .pci_ops = {
>> + .map_bus = al_pcie_map_bus,
>> + .read = pci_generic_config_read,
>> + .write = pci_generic_config_write,
>> + }
>> +};
>> +
>> +#endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
>> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
>> index 29efa09d686b..a73164c85e78 100644
>> --- a/include/linux/pci-ecam.h
>> +++ b/include/linux/pci-ecam.h
>> @@ -56,6 +56,7 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
>> extern struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */
>> extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene PCIe v1 */
>> extern struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene PCIe v2.x */
>> +extern struct pci_ecam_ops al_pcie_ops; /* Amazon Annapurna Labs PCIe */
>> #endif
>>
>> #ifdef CONFIG_PCI_HOST_COMMON
>> --
>> 1.9.1
>>

2019-03-25 17:37:49

by Bjorn Helgaas

Subject: Re: [PATCH] PCI: al: add pcie-al.c

On Mon, Mar 25, 2019 at 05:56:35PM +0200, Jonathan Chocron wrote:
> On 3/25/19 14:58, Bjorn Helgaas wrote:

> > I think you're actually looking for a AMZN0001 device, not a PNP0C02
> > device. Your firmware might have a PNP0C02 _HID and AMZN0001 _CID, but
> > that's not relevant here since you're only filtering by AMZN0001.
> >
> Actually, the _HID is AMZN0001 and the _CID is PNP0C02, so that's why
> I used PNP0C02 (to state that it is of this "type"). Please let me know if
> you still think it should be changed, so I'll integrate it into v2.

I do still think it should be changed, because there's nothing in the
code that refers to PNP0C02. That _CID could be absent or could
contain any other ID and it wouldn't make any difference to the
driver. But if there's no device with _HID or _CID of AMZN0001, the
driver will fail.

Bjorn

2019-03-26 10:02:00

by Chocron, Jonathan

Subject: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

Adding support for Amazon's Annapurna Labs PCIe driver.
The HW controller is based on DesignWare's IP.

The HW doesn't support accessing the Root Port's config space via
ECAM, so we obtain its base address via an AMZN0001 device.

Furthermore, the DesignWare PCIe controller doesn't filter out
config transactions sent to devices 1 and up on its bus, so they
are filtered by the driver.
All subordinate buses do support ECAM access.

Implementing specific PCI config access functions involves:
- Adding an init function to obtain the Root Port's base address
from an AMZN0001 device.
- Adding a new entry in the mcfg quirk array

Co-developed-by: Vladimir Aerov <[email protected]>
Signed-off-by: Jonathan Chocron <[email protected]>
Signed-off-by: Vladimir Aerov <[email protected]>
Reviewed-by: Benjamin Herrenschmidt <[email protected]>
Reviewed-by: David Woodhouse <[email protected]>
---

Changes from v1:
- Fix commit message comments (incl. using AMZN0001
instead of PNP0C02)
- Use the usual multi-line comment style

MAINTAINERS | 6 +++
drivers/acpi/pci_mcfg.c | 12 +++++
drivers/pci/controller/dwc/Makefile | 1 +
drivers/pci/controller/dwc/pcie-al.c | 93 ++++++++++++++++++++++++++++++++++++
include/linux/pci-ecam.h | 1 +
5 files changed, 113 insertions(+)
create mode 100644 drivers/pci/controller/dwc/pcie-al.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 32d444476a90..7a17017f9f82 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11769,6 +11769,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
S: Supported
F: drivers/pci/controller/

+PCIE DRIVER FOR ANNAPURNA LABS
+M: Jonathan Chocron <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/pci/controller/dwc/pcie-al.c
+
PCIE DRIVER FOR AMLOGIC MESON
M: Yue Wang <[email protected]>
L: [email protected]
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index a4e8432fc2fb..b42be067fb83 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -52,6 +52,18 @@ struct mcfg_fixup {
static struct mcfg_fixup mcfg_quirks[] = {
/* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, ops, cfgres }, */

+#define AL_ECAM(table_id, rev, seg, ops) \
+ { "AMAZON", table_id, rev, seg, MCFG_BUS_ANY, ops }
+
+ AL_ECAM("GRAVITON", 0, 0, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 1, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 2, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 3, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 4, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 5, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 6, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 7, &al_pcie_ops),
+
#define QCOM_ECAM32(seg) \
{ "QCOM ", "QDF2432 ", 1, seg, MCFG_BUS_ANY, &pci_32b_ops }

diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index 7bcdcdf5024e..1ea773c0070d 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -28,5 +28,6 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
# depending on whether ACPI, the DT driver, or both are enabled.

ifdef CONFIG_PCI
+obj-$(CONFIG_ARM64) += pcie-al.o
obj-$(CONFIG_ARM64) += pcie-hisi.o
endif
diff --git a/drivers/pci/controller/dwc/pcie-al.c b/drivers/pci/controller/dwc/pcie-al.c
new file mode 100644
index 000000000000..65a9776c12be
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-al.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe host controller driver for Amazon's Annapurna Labs IP (used in chips
+ * such as Graviton and Alpine)
+ *
+ * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Author: Jonathan Chocron <[email protected]>
+ */
+
+#include <linux/pci.h>
+#include <linux/pci-ecam.h>
+#include <linux/pci-acpi.h>
+#include "../../pci.h"
+
+#if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
+
+struct al_pcie_acpi {
+ void __iomem *dbi_base;
+};
+
+static void __iomem *al_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
+ int where)
+{
+ struct pci_config_window *cfg = bus->sysdata;
+ struct al_pcie_acpi *pcie = cfg->priv;
+ void __iomem *dbi_base = pcie->dbi_base;
+
+ if (bus->number == cfg->busr.start) {
+ /*
+ * The DW PCIe core doesn't filter out transactions to other
+ * devices/functions on the primary bus num, so we do this here.
+ */
+ if (PCI_SLOT(devfn) > 0)
+ return NULL;
+ else
+ return dbi_base + where;
+ }
+
+ return pci_ecam_map_bus(bus, devfn, where);
+}
+
+static int al_pcie_init(struct pci_config_window *cfg)
+{
+ struct device *dev = cfg->parent;
+ struct acpi_device *adev = to_acpi_device(dev);
+ struct acpi_pci_root *root = acpi_driver_data(adev);
+ struct al_pcie_acpi *al_pcie;
+ struct resource *res;
+ int ret;
+
+ al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
+ if (!al_pcie)
+ return -ENOMEM;
+
+ res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
+ if (!res)
+ return -ENOMEM;
+
+ ret = acpi_get_rc_resources(dev, "AMZN0001", root->segment, res);
+ if (ret) {
+ dev_err(dev, "can't get rc dbi base address for SEG %d\n",
+ root->segment);
+ return ret;
+ }
+
+ dev_dbg(dev, "Root port dbi res: %pR\n", res);
+
+ al_pcie->dbi_base = devm_pci_remap_cfg_resource(dev, res);
+ if (IS_ERR(al_pcie->dbi_base)) {
+ long err = PTR_ERR(al_pcie->dbi_base);
+
+ dev_err(dev, "couldn't remap dbi base %pR (err:%ld)\n",
+ res, err);
+ return err;
+ }
+
+ cfg->priv = al_pcie;
+
+ return 0;
+}
+
+struct pci_ecam_ops al_pcie_ops = {
+ .bus_shift = 20,
+ .init = al_pcie_init,
+ .pci_ops = {
+ .map_bus = al_pcie_map_bus,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+ }
+};
+
+#endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
index 29efa09d686b..a73164c85e78 100644
--- a/include/linux/pci-ecam.h
+++ b/include/linux/pci-ecam.h
@@ -56,6 +56,7 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
extern struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */
extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene PCIe v1 */
extern struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene PCIe v2.x */
+extern struct pci_ecam_ops al_pcie_ops; /* Amazon Annapurna Labs PCIe */
#endif

#ifdef CONFIG_PCI_HOST_COMMON
--
1.9.1

2019-03-26 12:18:27

by Lorenzo Pieralisi

Subject: Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

[+Zhou, Gustavo]

On Tue, Mar 26, 2019 at 12:00:55PM +0200, Jonathan Chocron wrote:
> Adding support for Amazon's Annapurna Labs PCIe driver.
> The HW controller is based on DesignWare's IP.
>
> The HW doesn't support accessing the Root Port's config space via
> ECAM, so we obtain its base address via an AMZN0001 device.
>
> Furthermore, the DesignWare PCIe controller doesn't filter out
> config transactions sent to devices 1 and up on its bus, so they
> are filtered by the driver.
> All subordinate buses do support ECAM access.
>
> Implementing specific PCI config access functions involves:
> - Adding an init function to obtain the Root Port's base address
> from an AMZN0001 device.
> - Adding a new entry in the mcfg quirk array
>
> Co-developed-by: Vladimir Aerov <[email protected]>
> Signed-off-by: Jonathan Chocron <[email protected]>
> Signed-off-by: Vladimir Aerov <[email protected]>
> Reviewed-by: Benjamin Herrenschmidt <[email protected]>
> Reviewed-by: David Woodhouse <[email protected]>

Review tags should be given on public mailing lists for public
review and I have not seen them (they were already there in v1) so
you should drop them.

> Changes from v1:
> - Fix commit message comments (incl. using AMZN0001
> instead of PNP0C02)
> - Use the usual multi-line comment style
>
> MAINTAINERS | 6 +++
> drivers/acpi/pci_mcfg.c | 12 +++++
> drivers/pci/controller/dwc/Makefile | 1 +
> drivers/pci/controller/dwc/pcie-al.c | 93 ++++++++++++++++++++++++++++++++++++
> include/linux/pci-ecam.h | 1 +
> 5 files changed, 113 insertions(+)
> create mode 100644 drivers/pci/controller/dwc/pcie-al.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 32d444476a90..7a17017f9f82 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11769,6 +11769,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
> S: Supported
> F: drivers/pci/controller/
>
> +PCIE DRIVER FOR ANNAPURNA LABS
> +M: Jonathan Chocron <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/pci/controller/dwc/pcie-al.c

I do not think we need a maintainer file for that see below, and
actually this quirk should be handled by DWC maintainers since it is a
DWC quirk, not a platform one.

> +
> PCIE DRIVER FOR AMLOGIC MESON
> M: Yue Wang <[email protected]>
> L: [email protected]
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index a4e8432fc2fb..b42be067fb83 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -52,6 +52,18 @@ struct mcfg_fixup {
> static struct mcfg_fixup mcfg_quirks[] = {
> /* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, ops, cfgres }, */
>
> +#define AL_ECAM(table_id, rev, seg, ops) \
> + { "AMAZON", table_id, rev, seg, MCFG_BUS_ANY, ops }
> +
> + AL_ECAM("GRAVITON", 0, 0, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 1, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 2, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 3, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 4, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 5, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 6, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 7, &al_pcie_ops),
> +
> #define QCOM_ECAM32(seg) \
> { "QCOM ", "QDF2432 ", 1, seg, MCFG_BUS_ANY, &pci_32b_ops }
>
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index 7bcdcdf5024e..1ea773c0070d 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -28,5 +28,6 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
> # depending on whether ACPI, the DT driver, or both are enabled.
>
> ifdef CONFIG_PCI
> +obj-$(CONFIG_ARM64) += pcie-al.o
> obj-$(CONFIG_ARM64) += pcie-hisi.o
> endif
> diff --git a/drivers/pci/controller/dwc/pcie-al.c b/drivers/pci/controller/dwc/pcie-al.c
> new file mode 100644
> index 000000000000..65a9776c12be
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-al.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCIe host controller driver for Amazon's Annapurna Labs IP (used in chips
> + * such as Graviton and Alpine)
> + *
> + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + *
> + * Author: Jonathan Chocron <[email protected]>
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/pci-ecam.h>
> +#include <linux/pci-acpi.h>
> +#include "../../pci.h"
> +
> +#if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
> +
> +struct al_pcie_acpi {
> + void __iomem *dbi_base;
> +};
> +
> +static void __iomem *al_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> + int where)
> +{
> + struct pci_config_window *cfg = bus->sysdata;
> + struct al_pcie_acpi *pcie = cfg->priv;
> + void __iomem *dbi_base = pcie->dbi_base;
> +
> + if (bus->number == cfg->busr.start) {
> + /*
> + * The DW PCIe core doesn't filter out transactions to other
> + * devices/functions on the primary bus num, so we do this here.
> + */
> + if (PCI_SLOT(devfn) > 0)
> + return NULL;
> + else
> + return dbi_base + where;
> + }
> +
> + return pci_ecam_map_bus(bus, devfn, where);
> +}
> +
> +static int al_pcie_init(struct pci_config_window *cfg)
> +{
> + struct device *dev = cfg->parent;
> + struct acpi_device *adev = to_acpi_device(dev);
> + struct acpi_pci_root *root = acpi_driver_data(adev);
> + struct al_pcie_acpi *al_pcie;
> + struct resource *res;
> + int ret;
> +
> + al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
> + if (!al_pcie)
> + return -ENOMEM;
> +
> + res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
> + if (!res)
> + return -ENOMEM;
> +
> + ret = acpi_get_rc_resources(dev, "AMZN0001", root->segment, res);
> + if (ret) {
> + dev_err(dev, "can't get rc dbi base address for SEG %d\n",
> + root->segment);
> + return ret;
> + }
> +
> + dev_dbg(dev, "Root port dbi res: %pR\n", res);
> +
> + al_pcie->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> + if (IS_ERR(al_pcie->dbi_base)) {
> + long err = PTR_ERR(al_pcie->dbi_base);
> +
> + dev_err(dev, "couldn't remap dbi base %pR (err:%ld)\n",
> + res, err);
> + return err;
> + }
> +
> + cfg->priv = al_pcie;
> +
> + return 0;
> +}

This code is basically identical to (apart from the string matching
the DBI resource)

drivers/pci/controller/pcie-hisi.c

because, as you said, that's a DW quirk that is really not
platform specific AFAICS.

Not that I am really fond of the idea but in practice we can
create a quirk that applies to all host controllers DW based,
in case they want to boot with ACPI, this patch is basically
code duplication.

Thanks,
Lorenzo

> +
> +struct pci_ecam_ops al_pcie_ops = {
> + .bus_shift = 20,
> + .init = al_pcie_init,
> + .pci_ops = {
> + .map_bus = al_pcie_map_bus,
> + .read = pci_generic_config_read,
> + .write = pci_generic_config_write,
> + }
> +};
> +
> +#endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
> index 29efa09d686b..a73164c85e78 100644
> --- a/include/linux/pci-ecam.h
> +++ b/include/linux/pci-ecam.h
> @@ -56,6 +56,7 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
> extern struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */
> extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene PCIe v1 */
> extern struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene PCIe v2.x */
> +extern struct pci_ecam_ops al_pcie_ops; /* Amazon Annapurna Labs PCIe */
> #endif
>
> #ifdef CONFIG_PCI_HOST_COMMON
> --
> 1.9.1
>

2019-03-26 12:56:03

by Bjorn Helgaas

Subject: Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

Nits, probably Lorenzo will fix them up unless he sees more substantive
things.

On Tue, Mar 26, 2019 at 12:00:55PM +0200, Jonathan Chocron wrote:
> Adding support for Amazon's Annapurna Labs PCIe driver.

Ideally, use "imperative mood", i.e., write it as a command:

Add support for Amazon's Annapurna Labs PCIe driver.

> The HW controller is based on DesignWare's IP.
>
> The HW doesn't support accessing the Root Port's config space via
> ECAM, so we obtain its base address via an AMZN0001 device.
>
> Furthermore, the DesignWare PCIe controller doesn't filter out
> config transactions sent to devices 1 and up on its bus, so they
> are filtered by the driver.
> All subordinate buses do support ECAM access.

I didn't communicate my point very clearly. The above four lines
should either be (1) a single paragraph, wrapped to fill the entire
width, or (2) two paragraphs, with a blank line before "All
subordinate buses ..."

The fact that "... by the driver" ends in the middle of the line
suggests that it's the end of the paragraph, but the fact that there's
no blank line following suggests that it's not. So it creates an
unnecessary hiccup for the reader.

> Implementing specific PCI config access functions involves:
> - Adding an init function to obtain the Root Port's base address
> from an AMZN0001 device.
> - Adding a new entry in the mcfg quirk array

s/mcfg/MCFG/ since "MCFG" is an ACPI table ID, not a word.

Bjorn

2019-03-26 13:26:01

by David Woodhouse

Subject: Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

On Tue, 2019-03-26 at 12:17 +0000, Lorenzo Pieralisi wrote:
> [+Zhou, Gustavo]
>
> On Tue, Mar 26, 2019 at 12:00:55PM +0200, Jonathan Chocron wrote:
> > Adding support for Amazon's Annapurna Labs PCIe driver.
> > The HW controller is based on DesignWare's IP.
> >
> > The HW doesn't support accessing the Root Port's config space via
> > ECAM, so we obtain its base address via an AMZN0001 device.
> >
> > Furthermore, the DesignWare PCIe controller doesn't filter out
> > config transactions sent to devices 1 and up on its bus, so they
> > are filtered by the driver.
> > All subordinate buses do support ECAM access.
> >
> > Implementing specific PCI config access functions involves:
> > - Adding an init function to obtain the Root Port's base address
> > from an AMZN0001 device.
> > - Adding a new entry in the mcfg quirk array
> >
> > Co-developed-by: Vladimir Aerov <[email protected]>
> > Signed-off-by: Jonathan Chocron <[email protected]>
> > Signed-off-by: Vladimir Aerov <[email protected]>
> > Reviewed-by: Benjamin Herrenschmidt <[email protected]>
> > Reviewed-by: David Woodhouse <[email protected]>
>
> Review tags should be given on public mailing lists for public
> review and I have not seen them (they were already there in v1) so
> you should drop them.

We did that internally. You really don't want me telling engineers to
post to the list *first* without running things by me to get the basics
right. Not to start with, at least.

Reviewed-by: David Woodhouse <[email protected]>


> > Changes from v1:
> > - Fix commit message comments (incl. using AMZN0001
> > instead of PNP0C02)
> > - Use the usual multi-line comment style
> >
> > MAINTAINERS | 6 +++
> > drivers/acpi/pci_mcfg.c | 12 +++++
> > drivers/pci/controller/dwc/Makefile | 1 +
> > drivers/pci/controller/dwc/pcie-al.c | 93 ++++++++++++++++++++++++++++++++++++
> > include/linux/pci-ecam.h | 1 +
> > 5 files changed, 113 insertions(+)
> > create mode 100644 drivers/pci/controller/dwc/pcie-al.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 32d444476a90..7a17017f9f82 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11769,6 +11769,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
> > S: Supported
> > F: drivers/pci/controller/
> >
> > +PCIE DRIVER FOR ANNAPURNA LABS
> > +M: Jonathan Chocron <[email protected]>
> > +L: [email protected]
> > +S: Maintained
> > +F: drivers/pci/controller/dwc/pcie-al.c
>
> I do not think we need a maintainer file for that see below, and
> actually this quirk should be handled by DWC maintainers since it is a
> DWC quirk, not a platform one.

Many of the others already have this, it seems.

It's also fine to drop it, and include it when we add the rest of the
Alpine SOC support and a MAINTAINERS entry for that.

Attachments:

2019-03-26 16:01:15

by Lorenzo Pieralisi

Subject: Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

On Tue, Mar 26, 2019 at 01:24:41PM +0000, David Woodhouse wrote:
> On Tue, 2019-03-26 at 12:17 +0000, Lorenzo Pieralisi wrote:
> > [+Zhou, Gustavo]
> >
> > On Tue, Mar 26, 2019 at 12:00:55PM +0200, Jonathan Chocron wrote:
> > > Adding support for Amazon's Annapurna Labs PCIe driver.
> > > The HW controller is based on DesignWare's IP.
> > >
> > > The HW doesn't support accessing the Root Port's config space via
> > > ECAM, so we obtain its base address via an AMZN0001 device.
> > >
> > > Furthermore, the DesignWare PCIe controller doesn't filter out
> > > config transactions sent to devices 1 and up on its bus, so they
> > > are filtered by the driver.
> > > All subordinate buses do support ECAM access.
> > >
> > > Implementing specific PCI config access functions involves:
> > > - Adding an init function to obtain the Root Port's base address
> > > from an AMZN0001 device.
> > > - Adding a new entry in the mcfg quirk array
> > >
> > > Co-developed-by: Vladimir Aerov <[email protected]>
> > > Signed-off-by: Jonathan Chocron <[email protected]>
> > > Signed-off-by: Vladimir Aerov <[email protected]>
> > > Reviewed-by: Benjamin Herrenschmidt <[email protected]>
> > > Reviewed-by: David Woodhouse <[email protected]>
> >
> > Review tags should be given on public mailing lists for public
> > review and I have not seen them (they were already there in v1) so
> > you should drop them.
>
> We did that internally. You really don't want me telling engineers to
> post to the list *first* without running things by me to get the basics
> right. Not to start with, at least.

Hi David,

I am obviously in favour of internal review and I do not question it was
carried out internally, I just kindly ask developers to drop review tags
given internally when going to public mailing lists - I understand it is
churn for you but I prefer them to be given explicitly.

Thanks !
Lorenzo

> Reviewed-by: David Woodhouse <[email protected]>
>
>
> > > Changes from v1:
> > > - Fix commit message comments (incl. using AMZN0001
> > > instead of PNP0C02)
> > > - Use the usual multi-line comment style
> > >
> > > MAINTAINERS | 6 +++
> > > drivers/acpi/pci_mcfg.c | 12 +++++
> > > drivers/pci/controller/dwc/Makefile | 1 +
> > > drivers/pci/controller/dwc/pcie-al.c | 93 ++++++++++++++++++++++++++++++++++++
> > > include/linux/pci-ecam.h | 1 +
> > > 5 files changed, 113 insertions(+)
> > > create mode 100644 drivers/pci/controller/dwc/pcie-al.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 32d444476a90..7a17017f9f82 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -11769,6 +11769,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
> > > S: Supported
> > > F: drivers/pci/controller/
> > >
> > > +PCIE DRIVER FOR ANNAPURNA LABS
> > > +M: Jonathan Chocron <[email protected]>
> > > +L: [email protected]
> > > +S: Maintained
> > > +F: drivers/pci/controller/dwc/pcie-al.c
> >
> > I do not think we need a maintainer file for that see below, and
> > actually this quirk should be handled by DWC maintainers since it is a
> > DWC quirk, not a platform one.
>
> Many of the others already have this, it seems.
>
> It's also fine to drop it, and include it when we add the rest of the
> Alpine SOC support and a MAINTAINERS entry for that.
>

2019-03-27 09:54:02

by David Woodhouse

Subject: Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

On Tue, 2019-03-26 at 15:58 +0000, Lorenzo Pieralisi wrote:
> > We did that internally. You really don't want me telling engineers to
> > post to the list *first* without running things by me to get the basics
> > right. Not to start with, at least.
>
> Hi David,
>
> I am obviously in favour of internal review and I do not question it was
> carried out internally, I just kindly ask developers to drop review tags
> given internally when going to public mailing lists - I understand it is
> churn for you but I prefer them to be given explicitly.

Sure, I've provided mine in public now.

I will attempt to remember your preference, although I'm not sure I
think it's necessary.

What's the failure mode we're protecting against here? That my
engineers are lying and have *faked* my reviewed-by tag?

Don't you think I'd *eat* them if I ever found that happening?

What's next? That you only accept such tags in signed email, so that
the dishonest engineer in question can't *fake* an email from me to the
list? They know I'm afflicted by Exchange so they can always send that
fake message with a message-id matching another message they know is
already in my inbox, so Exchange will helpfully discard theirs. :)

Attachments:

2019-03-27 10:05:45

by Woodhouse, David

Subject: Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

On Tue, 2019-03-26 at 12:17 +0000, Lorenzo Pieralisi wrote:
> This code is basically identical to (apart from the string matching
> the DBI resource)
>
> drivers/pci/controller/pcie-hisi.c
>
> because, as you said, that's a DW quirk that is really not
> platform specific AFAICS.
>
> Not that I am really fond of the idea but in practice we can
> create a quirk that applies to all host controllers DW based,
> in case they want to boot with ACPI, this patch is basically
> code duplication.

Having chatted to Jonny in a little more detail... this isn't quite
duplicate code. On the Annapurna implementation we have fixed the
alignment constraints so we can just use pci_generic_config_read()
directly instead of forcing alignment. We only need to filter out the
"ghost" devices on bus zero.

There might eventually be scope for some form of consolidation, but it
doesn't really seem clear at this point that it would be worth it.

There are three separate quirks needed for different versions of the DW
controller.

One is that the config space of the controller itself shows up multiple
times in all slots of bus zero.

The second is that bus zero cannot be accessed through ECAM and must be
accessed through a separate MMIO region.

The third is the requirement for 32-bit alignment of the host
controller's config space (through the special MMIO region).

Some vendors have managed to work around some of these issues, for
example Annapurna fixing the alignment one. It looks like the three DT
matches in pci-host-generic.c which use pci_dw_ecam_bus_ops are
assuming the hardware suffers only issue #1 from the list above, and
not the other two.

The Annapurna hardware gives us a new combination, and that's why it
isn't quite a duplicate. Again, there may be scope for consolidation in
the future but it's not clear what it should look like. Especially as
when exposed through DT, both the hisi and al versions seem to do
things quite differently and need their own specific code there anyway.


There will be a DT variant of the AL driver coming in the near future,
but when it's exposed via ACPI in the "as close to ECAM compliant as we
could make it in this iteration of the hardware" mode, it's relatively
simple so we did that patch first.

Attachments:

2019-03-27 11:21:52

by Lorenzo Pieralisi

Subject: Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

On Wed, Mar 27, 2019 at 09:52:15AM +0000, David Woodhouse wrote:
> On Tue, 2019-03-26 at 15:58 +0000, Lorenzo Pieralisi wrote:
> > > We did that internally. You really don't want me telling engineers to
> > > post to the list *first* without running things by me to get the basics
> > > right. Not to start with, at least.
> >
> > Hi David,
> >
> > I am obviously in favour of internal review and I do not question it was
> > carried out internally, I just kindly ask developers to drop review tags
> > given internally when going to public mailing lists - I understand it is
> > churn for you but I prefer them to be given explicitly.
>
> Sure, I've provided mine in public now.
>
> I will attempt to remember your preference, although I'm not sure I
> think it's necessary.
>
> What's the failure mode we're protecting against here? That my
> engineers are lying and have *faked* my reviewed-by tag?
>
> Don't you think I'd *eat* them if I ever found that happening?

As I wrote above, I did not question the internal review process at all,
we do internal review at ARM too in preparation for posting publicly but
I think the patches review should take place on public mailing lists and
tags should be given accordingly, that's it.

You may see it as churn, fair enough, it is not a big deal either.

> What's next? That you only accept such tags in signed email, so that
> the dishonest engineer in question can't *fake* an email from me to the
> list? They know I'm afflicted by Exchange so they can always send that
> fake message with a message-id matching another message they know is
> already in my inbox, so Exchange will helpfully discard theirs. :)

There is nothing next :) - I just would like to see patches discussions
and reviews taking place on [email protected] for PCI patches,
I do not think I am asking too much.

Thanks,
Lorenzo

2019-03-27 11:40:15

by Lorenzo Pieralisi

Subject: Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

On Wed, Mar 27, 2019 at 09:43:26AM +0000, David Woodhouse wrote:
> On Tue, 2019-03-26 at 12:17 +0000, Lorenzo Pieralisi wrote:
> > This code is basically identical to (apart from the string matching
> > the DBI resource)
> >
> > drivers/pci/controller/pcie-hisi.c
> >
> > because, as you said, that's a DW quirk that is really not
> > platform specific AFAICS.
> >
> > Not that I am really fond of the idea but in practice we can
> > create a quirk that applies to all host controllers DW based,
> > in case they want to boot with ACPI, this patch is basically
> > code duplication.
>
> Having chatted to Jonny in a little more detail... this isn't quite
> duplicate code. On the Annapurna implementation we have fixed the
> alignment constraints so we can just use pci_generic_config_read()
> directly instead of forcing alignment. We only need to filter out the
> "ghost" devices on bus zero.
>
> There might eventually be scope for some form of consolidation, but it
> doesn't really seem clear at this point that it would be worth it.

The pci_ecam_ops.init function can be certainly reused but I agree
duplicating it is not a big deal either - I just noticed it and asked.

we can merge code as-is and think about writing a common init function
if/when needed.

> There are three separate quirks needed for different versions of the DW
> controller.
>
> One is that the config space of the controller itself shows up multiple
> times in all slots of bus zero.
>
> The second is that bus zero cannot be accessed through ECAM and must be
> accessed through a separate MMIO region.
>
> The third is the requirement for 32-bit alignment of the host
> controller's config space (through the special MMIO region).

I missed this one - thanks for clarifying.

> Some vendors have managed to work around some of these issues, for
> example Annapurna fixing the alignment one. It looks like the three DT
> matches in pci-host-generic.c which use pci_dw_ecam_bus_ops are
> assuming the hardware suffers only issue #1 from the list above, and
> not the other two.
>
> The Annapurna hardware gives us a new combination, and that's why it
> isn't quite a duplicate. Again, there may be scope for consolidation in
> the future but it's not clear what it should look like. Especially as
> when exposed through DT, both the hisi and al versions seem to do
> things quite differently and need their own specific code there anyway.

DT PCI host bridge bootstrap is a different story and on that
consolidation is all but impossible.

I just want to keep MCFG ECAM quirks as simple as possible, code as it
stands is horrible enough and I wish I could remove the mechanism in
the future rather than adding more to it, hopefully we are getting
there.

> There will be a DT variant of the AL driver coming in the near future,
> but when it's exposed via ACPI in the "as close to ECAM compliant as we
> could make it in this iteration of the hardware" mode, it's relatively
> simple so we did that patch first.

That's fine, no problems with that.

Thanks,
Lorenzo

2019-03-27 11:41:55

by David Woodhouse

Subject: Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

On Wed, 2019-03-27 at 11:20 +0000, Lorenzo Pieralisi wrote:
> On Wed, Mar 27, 2019 at 09:52:15AM +0000, David Woodhouse wrote:
> > On Tue, 2019-03-26 at 15:58 +0000, Lorenzo Pieralisi wrote:
> > > > We did that internally. You really don't want me telling engineers to
> > > > post to the list *first* without running things by me to get the basics
> > > > right. Not to start with, at least.
> > >
> > > Hi David,
> > >
> > > I am obviously in favour of internal review and I do not question it was
> > > carried out internally, I just kindly ask developers to drop review tags
> > > given internally when going to public mailing lists - I understand it is
> > > churn for you but I prefer them to be given explicitly.
> >
> > Sure, I've provided mine in public now.
> >
> > I will attempt to remember your preference, although I'm not sure I
> > think it's necessary.
> >
> > What's the failure mode we're protecting against here? That my
> > engineers are lying and have *faked* my reviewed-by tag?
> >
> > Don't you think I'd *eat* them if I ever found that happening?
>
> As I wrote above, I did not question the internal review process at all,
> we do internal review at ARM too in preparation for posting publicly but
> I think the patches review should take place on public mailing lists and
> tags should be given accordingly, that's it.
>
> You may see it as churn, fair enough, it is not a big deal either.

Understood. As I said, we will endeavour to comply.

Note that we may occasionally forget. Our internal review tooling
automatically *adds* those tags, when internal review happens. And we
have reduced the "legal" approval process to the point where myself or
a handful of others only need to give the nod in that same internal
technical review tool, for a patch to be sent upstream.

This means that engineers will have to remember to actively *remove*
those tags when they're sending PCI patches. We'll try to remember :)

> > What's next? That you only accept such tags in signed email, so that
> > the dishonest engineer in question can't *fake* an email from me to the
> > list? They know I'm afflicted by Exchange so they can always send that
> > fake message with a message-id matching another message they know is
> > already in my inbox, so Exchange will helpfully discard theirs. :)
>
> There is nothing next :) - I just would like to see patches discussions
> and reviews taking place on [email protected] for PCI patches,
> I do not think I am asking too much.

Not asking too much at all. We will do as you request.

I do think it's pointless — you really *don't* want to see the first
rounds of review that happened internally, and once the patch makes it
to the list, it doesn't make a lot of difference whether my Reviewed-
By: tag is already there or not; you don't get to see the previous
iterations of the patch or any of those prior review comments anyway.

If *all* there is in my public response is that Reviewed-by: tag, there
is literally no benefit to that at all, except that you know the
engineer isn't lying. None at all.

But it's fine. If it really annoys me, I can set up an autoresponder
which sends an empty mail with a 'Reviewed-by:' tag, and my engineers
can include a header in their patch submission which triggers that. We
can even automate the tooling, to turn the normal Reviewed-by: tag into
that header which triggers the response.

We will comply with your request, even if we don't understand it :)

Attachments:

2019-03-27 12:02:55

by David Woodhouse

Subject: Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

On Wed, 2019-03-27 at 11:39 +0000, Lorenzo Pieralisi wrote:
> I just want to keep MCFG ECAM quirks as simple as possible, code as it
> stands is horrible enough and I wish I could remove the mechanism in
> the future rather than adding more to it, hopefully we are getting
> there.

Obviously I cannot talk about future hardware or even admit that there
is such as thing as "future hardware".

But all the issues with the DW controller *have* already been worked
around by different implementations, in different combinations. And I
will be very sad if *we* somehow manage to make a new board which needs
a new and different combination of quirks.

Actively, percussively, sad.

Attachments:

2019-03-28 10:58:10

by Chocron, Jonathan

Subject: Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

On 3/26/19 14:55, Bjorn Helgaas wrote:
> Nits, probably Lorenzo will fix them up unless he sees more substantive
> things.
>
> On Tue, Mar 26, 2019 at 12:00:55PM +0200, Jonathan Chocron wrote:
>> Adding support for Amazon's Annapurna Labs PCIe driver.
> Ideally, use "imperative mood", i.e., write it as a command:
>
> Add support for Amazon's Annapurna Labs PCIe driver.
Done.
>> The HW controller is based on DesignWare's IP.
>>
>> The HW doesn't support accessing the Root Port's config space via
>> ECAM, so we obtain its base address via an AMZN0001 device.
>>
>> Furthermore, the DesignWare PCIe controller doesn't filter out
>> config transactions sent to devices 1 and up on its bus, so they
>> are filtered by the driver.
>> All subordinate buses do support ECAM access.
> I didn't communicate my point very clearly. The above four lines
> should either be (1) a single paragraph, wrapped to fill the entire
> width, or (2) two paragraphs, with a blank line before "All
> subordinate buses ..."
>
> The fact that "... by the driver" ends in the middle of the line
> suggests that it's the end of the paragraph, but the fact that there's
> no blank line following suggests that it's not. So it creates an
> unnecessary hiccup for the reader.
Added a blank line.
>> Implementing specific PCI config access functions involves:
>> - Adding an init function to obtain the Root Port's base address
>> from an AMZN0001 device.
>> - Adding a new entry in the mcfg quirk array
> s/mcfg/MCFG/ since "MCFG" is an ACPI table ID, not a word.
Done.
> Bjorn

2019-03-28 11:59:28

by Chocron, Jonathan

Subject: [PATCH v3] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

Add support for Amazon's Annapurna Labs PCIe driver. The HW controller
is based on DesignWare's IP.

The HW doesn't support accessing the Root Port's config space via ECAM,
so we obtain its base address via an AMZN0001 device.

Furthermore, the DesignWare PCIe controller doesn't filter out config
transactions sent to devices 1 and up on its bus, so they are filtered
by the driver.

All subordinate buses do support ECAM access.

Implementing specific PCI config access functions involves:
- Adding an init function to obtain the Root Port's base address from
an AMZN0001 device.
- Adding a new entry in the MCFG quirk array

Co-developed-by: Vladimir Aerov <[email protected]>
Signed-off-by: Jonathan Chocron <[email protected]>
Signed-off-by: Vladimir Aerov <[email protected]>
Reviewed-by: David Woodhouse <[email protected]>
---

--v2:
- Fix commit message comments (incl. using AMZN0001 instead of
PNP0C02)
- Use the usual multi-line comment style

--v3:
- Fix additional commit message comments

MAINTAINERS | 6 +++
drivers/acpi/pci_mcfg.c | 12 +++++
drivers/pci/controller/dwc/Makefile | 1 +
drivers/pci/controller/dwc/pcie-al.c | 93 ++++++++++++++++++++++++++++++++++++
include/linux/pci-ecam.h | 1 +
5 files changed, 113 insertions(+)
create mode 100644 drivers/pci/controller/dwc/pcie-al.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 32d444476a90..7a17017f9f82 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11769,6 +11769,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
S: Supported
F: drivers/pci/controller/

+PCIE DRIVER FOR ANNAPURNA LABS
+M: Jonathan Chocron <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/pci/controller/dwc/pcie-al.c
+
PCIE DRIVER FOR AMLOGIC MESON
M: Yue Wang <[email protected]>
L: [email protected]
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index a4e8432fc2fb..b42be067fb83 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -52,6 +52,18 @@ struct mcfg_fixup {
static struct mcfg_fixup mcfg_quirks[] = {
/* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, ops, cfgres }, */

+#define AL_ECAM(table_id, rev, seg, ops) \
+ { "AMAZON", table_id, rev, seg, MCFG_BUS_ANY, ops }
+
+ AL_ECAM("GRAVITON", 0, 0, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 1, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 2, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 3, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 4, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 5, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 6, &al_pcie_ops),
+ AL_ECAM("GRAVITON", 0, 7, &al_pcie_ops),
+
#define QCOM_ECAM32(seg) \
{ "QCOM ", "QDF2432 ", 1, seg, MCFG_BUS_ANY, &pci_32b_ops }

diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index 7bcdcdf5024e..1ea773c0070d 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -28,5 +28,6 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
# depending on whether ACPI, the DT driver, or both are enabled.

ifdef CONFIG_PCI
+obj-$(CONFIG_ARM64) += pcie-al.o
obj-$(CONFIG_ARM64) += pcie-hisi.o
endif
diff --git a/drivers/pci/controller/dwc/pcie-al.c b/drivers/pci/controller/dwc/pcie-al.c
new file mode 100644
index 000000000000..65a9776c12be
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-al.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe host controller driver for Amazon's Annapurna Labs IP (used in chips
+ * such as Graviton and Alpine)
+ *
+ * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Author: Jonathan Chocron <[email protected]>
+ */
+
+#include <linux/pci.h>
+#include <linux/pci-ecam.h>
+#include <linux/pci-acpi.h>
+#include "../../pci.h"
+
+#if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
+
+struct al_pcie_acpi {
+ void __iomem *dbi_base;
+};
+
+static void __iomem *al_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
+ int where)
+{
+ struct pci_config_window *cfg = bus->sysdata;
+ struct al_pcie_acpi *pcie = cfg->priv;
+ void __iomem *dbi_base = pcie->dbi_base;
+
+ if (bus->number == cfg->busr.start) {
+ /*
+ * The DW PCIe core doesn't filter out transactions to other
+ * devices/functions on the primary bus num, so we do this here.
+ */
+ if (PCI_SLOT(devfn) > 0)
+ return NULL;
+ else
+ return dbi_base + where;
+ }
+
+ return pci_ecam_map_bus(bus, devfn, where);
+}
+
+static int al_pcie_init(struct pci_config_window *cfg)
+{
+ struct device *dev = cfg->parent;
+ struct acpi_device *adev = to_acpi_device(dev);
+ struct acpi_pci_root *root = acpi_driver_data(adev);
+ struct al_pcie_acpi *al_pcie;
+ struct resource *res;
+ int ret;
+
+ al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
+ if (!al_pcie)
+ return -ENOMEM;
+
+ res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
+ if (!res)
+ return -ENOMEM;
+
+ ret = acpi_get_rc_resources(dev, "AMZN0001", root->segment, res);
+ if (ret) {
+ dev_err(dev, "can't get rc dbi base address for SEG %d\n",
+ root->segment);
+ return ret;
+ }
+
+ dev_dbg(dev, "Root port dbi res: %pR\n", res);
+
+ al_pcie->dbi_base = devm_pci_remap_cfg_resource(dev, res);
+ if (IS_ERR(al_pcie->dbi_base)) {
+ long err = PTR_ERR(al_pcie->dbi_base);
+
+ dev_err(dev, "couldn't remap dbi base %pR (err:%ld)\n",
+ res, err);
+ return err;
+ }
+
+ cfg->priv = al_pcie;
+
+ return 0;
+}
+
+struct pci_ecam_ops al_pcie_ops = {
+ .bus_shift = 20,
+ .init = al_pcie_init,
+ .pci_ops = {
+ .map_bus = al_pcie_map_bus,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+ }
+};
+
+#endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
index 29efa09d686b..a73164c85e78 100644
--- a/include/linux/pci-ecam.h
+++ b/include/linux/pci-ecam.h
@@ -56,6 +56,7 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
extern struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */
extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene PCIe v1 */
extern struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene PCIe v2.x */
+extern struct pci_ecam_ops al_pcie_ops; /* Amazon Annapurna Labs PCIe */
#endif

#ifdef CONFIG_PCI_HOST_COMMON
--
1.9.1

2019-04-08 23:36:11

by Benjamin Herrenschmidt

Subject: Re: [PATCH v3] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

On Thu, 2019-03-28 at 13:57 +0200, Jonathan Chocron wrote:
> Add support for Amazon's Annapurna Labs PCIe driver. The HW
> controller
> is based on DesignWare's IP.
>
> The HW doesn't support accessing the Root Port's config space via
> ECAM,
> so we obtain its base address via an AMZN0001 device.
>
> Furthermore, the DesignWare PCIe controller doesn't filter out config
> transactions sent to devices 1 and up on its bus, so they are
> filtered
> by the driver.
>
> All subordinate buses do support ECAM access.
>
> Implementing specific PCI config access functions involves:
> - Adding an init function to obtain the Root Port's base address
> from
> an AMZN0001 device.
> - Adding a new entry in the MCFG quirk array
>
> Co-developed-by: Vladimir Aerov <[email protected]>
> Signed-off-by: Jonathan Chocron <[email protected]>
> Signed-off-by: Vladimir Aerov <[email protected]>
> Reviewed-by: David Woodhouse <[email protected]>

Late to the party, sorry :-) That kernel.crashing.org email is on its
last legs...

Reviewed-by: Benjamin Herrenschmidt <[email protected]>

> ---
>
> --v2:
> - Fix commit message comments (incl. using AMZN0001 instead of
> PNP0C02)
> - Use the usual multi-line comment style
>
> --v3:
> - Fix additional commit message comments
>
> MAINTAINERS | 6 +++
> drivers/acpi/pci_mcfg.c | 12 +++++
> drivers/pci/controller/dwc/Makefile | 1 +
> drivers/pci/controller/dwc/pcie-al.c | 93
> ++++++++++++++++++++++++++++++++++++
> include/linux/pci-ecam.h | 1 +
> 5 files changed, 113 insertions(+)
> create mode 100644 drivers/pci/controller/dwc/pcie-al.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 32d444476a90..7a17017f9f82 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11769,6 +11769,12 @@ T: git
> git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
> S: Supported
> F: drivers/pci/controller/
>
> +PCIE DRIVER FOR ANNAPURNA LABS
> +M: Jonathan Chocron <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/pci/controller/dwc/pcie-al.c
> +
> PCIE DRIVER FOR AMLOGIC MESON
> M: Yue Wang <[email protected]>
> L: [email protected]
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index a4e8432fc2fb..b42be067fb83 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -52,6 +52,18 @@ struct mcfg_fixup {
> static struct mcfg_fixup mcfg_quirks[] = {
> /* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, ops, cfgres },
> */
>
> +#define AL_ECAM(table_id, rev, seg, ops) \
> + { "AMAZON", table_id, rev, seg, MCFG_BUS_ANY, ops }
> +
> + AL_ECAM("GRAVITON", 0, 0, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 1, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 2, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 3, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 4, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 5, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 6, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 7, &al_pcie_ops),
> +
> #define QCOM_ECAM32(seg) \
> { "QCOM ", "QDF2432 ", 1, seg, MCFG_BUS_ANY, &pci_32b_ops }
>
> diff --git a/drivers/pci/controller/dwc/Makefile
> b/drivers/pci/controller/dwc/Makefile
> index 7bcdcdf5024e..1ea773c0070d 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -28,5 +28,6 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
> # depending on whether ACPI, the DT driver, or both are enabled.
>
> ifdef CONFIG_PCI
> +obj-$(CONFIG_ARM64) += pcie-al.o
> obj-$(CONFIG_ARM64) += pcie-hisi.o
> endif
> diff --git a/drivers/pci/controller/dwc/pcie-al.c
> b/drivers/pci/controller/dwc/pcie-al.c
> new file mode 100644
> index 000000000000..65a9776c12be
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-al.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCIe host controller driver for Amazon's Annapurna Labs IP (used
> in chips
> + * such as Graviton and Alpine)
> + *
> + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights
> Reserved.
> + *
> + * Author: Jonathan Chocron <[email protected]>
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/pci-ecam.h>
> +#include <linux/pci-acpi.h>
> +#include "../../pci.h"
> +
> +#if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
> +
> +struct al_pcie_acpi {
> + void __iomem *dbi_base;
> +};
> +
> +static void __iomem *al_pcie_map_bus(struct pci_bus *bus, unsigned
> int devfn,
> + int where)
> +{
> + struct pci_config_window *cfg = bus->sysdata;
> + struct al_pcie_acpi *pcie = cfg->priv;
> + void __iomem *dbi_base = pcie->dbi_base;
> +
> + if (bus->number == cfg->busr.start) {
> + /*
> + * The DW PCIe core doesn't filter out transactions to
> other
> + * devices/functions on the primary bus num, so we do
> this here.
> + */
> + if (PCI_SLOT(devfn) > 0)
> + return NULL;
> + else
> + return dbi_base + where;
> + }
> +
> + return pci_ecam_map_bus(bus, devfn, where);
> +}
> +
> +static int al_pcie_init(struct pci_config_window *cfg)
> +{
> + struct device *dev = cfg->parent;
> + struct acpi_device *adev = to_acpi_device(dev);
> + struct acpi_pci_root *root = acpi_driver_data(adev);
> + struct al_pcie_acpi *al_pcie;
> + struct resource *res;
> + int ret;
> +
> + al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
> + if (!al_pcie)
> + return -ENOMEM;
> +
> + res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
> + if (!res)
> + return -ENOMEM;
> +
> + ret = acpi_get_rc_resources(dev, "AMZN0001", root->segment,
> res);
> + if (ret) {
> + dev_err(dev, "can't get rc dbi base address for SEG
> %d\n",
> + root->segment);
> + return ret;
> + }
> +
> + dev_dbg(dev, "Root port dbi res: %pR\n", res);
> +
> + al_pcie->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> + if (IS_ERR(al_pcie->dbi_base)) {
> + long err = PTR_ERR(al_pcie->dbi_base);
> +
> + dev_err(dev, "couldn't remap dbi base %pR (err:%ld)\n",
> + res, err);
> + return err;
> + }
> +
> + cfg->priv = al_pcie;
> +
> + return 0;
> +}
> +
> +struct pci_ecam_ops al_pcie_ops = {
> + .bus_shift = 20,
> + .init = al_pcie_init,
> + .pci_ops = {
> + .map_bus = al_pcie_map_bus,
> + .read = pci_generic_config_read,
> + .write = pci_generic_config_write,
> + }
> +};
> +
> +#endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
> index 29efa09d686b..a73164c85e78 100644
> --- a/include/linux/pci-ecam.h
> +++ b/include/linux/pci-ecam.h
> @@ -56,6 +56,7 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus,
> unsigned int devfn,
> extern struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX
> 1.x */
> extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene
> PCIe v1 */
> extern struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene
> PCIe v2.x */
> +extern struct pci_ecam_ops al_pcie_ops; /* Amazon Annapurna Labs
> PCIe */
> #endif
>
> #ifdef CONFIG_PCI_HOST_COMMON

2019-04-16 13:17:10

by David Woodhouse

Subject: Re: [PATCH v3] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

On Tue, 2019-04-09 at 08:57 +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2019-03-28 at 13:57 +0200, Jonathan Chocron wrote:
> > Add support for Amazon's Annapurna Labs PCIe driver. The HW
> > controller
> > is based on DesignWare's IP.
> >
> > The HW doesn't support accessing the Root Port's config space via
> > ECAM,
> > so we obtain its base address via an AMZN0001 device.
> >
> > Furthermore, the DesignWare PCIe controller doesn't filter out
> > config
> > transactions sent to devices 1 and up on its bus, so they are
> > filtered
> > by the driver.
> >
> > All subordinate buses do support ECAM access.
> >
> > Implementing specific PCI config access functions involves:
> > - Adding an init function to obtain the Root Port's base address
> > from
> > an AMZN0001 device.
> > - Adding a new entry in the MCFG quirk array
> >
> > Co-developed-by: Vladimir Aerov <[email protected]>
> > Signed-off-by: Jonathan Chocron <[email protected]>
> > Signed-off-by: Vladimir Aerov <[email protected]>
> > Reviewed-by: David Woodhouse <[email protected]>
>
> Late to the party, sorry :-) That kernel.crashing.org email is on its
> last legs...
>
> Reviewed-by: Benjamin Herrenschmidt <[email protected]>

Lorenzo, is there anything else you need from Jonny before this can be
applied?

Thanks.

Attachments:

2019-04-16 14:03:12

by Lorenzo Pieralisi

Subject: Re: [PATCH v3] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

On Thu, Mar 28, 2019 at 01:57:56PM +0200, Jonathan Chocron wrote:
> Add support for Amazon's Annapurna Labs PCIe driver. The HW controller
> is based on DesignWare's IP.
>
> The HW doesn't support accessing the Root Port's config space via ECAM,
> so we obtain its base address via an AMZN0001 device.
>
> Furthermore, the DesignWare PCIe controller doesn't filter out config
> transactions sent to devices 1 and up on its bus, so they are filtered
> by the driver.
>
> All subordinate buses do support ECAM access.
>
> Implementing specific PCI config access functions involves:
> - Adding an init function to obtain the Root Port's base address from
> an AMZN0001 device.
> - Adding a new entry in the MCFG quirk array
>
> Co-developed-by: Vladimir Aerov <[email protected]>
> Signed-off-by: Jonathan Chocron <[email protected]>
> Signed-off-by: Vladimir Aerov <[email protected]>
> Reviewed-by: David Woodhouse <[email protected]>
> ---
>
> --v2:
> - Fix commit message comments (incl. using AMZN0001 instead of
> PNP0C02)
> - Use the usual multi-line comment style
>
> --v3:
> - Fix additional commit message comments
>
> MAINTAINERS | 6 +++
> drivers/acpi/pci_mcfg.c | 12 +++++
> drivers/pci/controller/dwc/Makefile | 1 +
> drivers/pci/controller/dwc/pcie-al.c | 93 ++++++++++++++++++++++++++++++++++++
> include/linux/pci-ecam.h | 1 +
> 5 files changed, 113 insertions(+)
> create mode 100644 drivers/pci/controller/dwc/pcie-al.c

I think Bjorn can take this if he is OK with it, it is not really
necessary to queue it via the controller/dwc tree so:

Acked-by: Lorenzo Pieralisi <[email protected]>

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 32d444476a90..7a17017f9f82 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11769,6 +11769,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
> S: Supported
> F: drivers/pci/controller/
>
> +PCIE DRIVER FOR ANNAPURNA LABS
> +M: Jonathan Chocron <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/pci/controller/dwc/pcie-al.c
> +
> PCIE DRIVER FOR AMLOGIC MESON
> M: Yue Wang <[email protected]>
> L: [email protected]
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index a4e8432fc2fb..b42be067fb83 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -52,6 +52,18 @@ struct mcfg_fixup {
> static struct mcfg_fixup mcfg_quirks[] = {
> /* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, ops, cfgres }, */
>
> +#define AL_ECAM(table_id, rev, seg, ops) \
> + { "AMAZON", table_id, rev, seg, MCFG_BUS_ANY, ops }
> +
> + AL_ECAM("GRAVITON", 0, 0, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 1, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 2, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 3, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 4, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 5, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 6, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 7, &al_pcie_ops),
> +
> #define QCOM_ECAM32(seg) \
> { "QCOM ", "QDF2432 ", 1, seg, MCFG_BUS_ANY, &pci_32b_ops }
>
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index 7bcdcdf5024e..1ea773c0070d 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -28,5 +28,6 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
> # depending on whether ACPI, the DT driver, or both are enabled.
>
> ifdef CONFIG_PCI
> +obj-$(CONFIG_ARM64) += pcie-al.o
> obj-$(CONFIG_ARM64) += pcie-hisi.o
> endif
> diff --git a/drivers/pci/controller/dwc/pcie-al.c b/drivers/pci/controller/dwc/pcie-al.c
> new file mode 100644
> index 000000000000..65a9776c12be
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-al.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCIe host controller driver for Amazon's Annapurna Labs IP (used in chips
> + * such as Graviton and Alpine)
> + *
> + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + *
> + * Author: Jonathan Chocron <[email protected]>
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/pci-ecam.h>
> +#include <linux/pci-acpi.h>
> +#include "../../pci.h"
> +
> +#if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
> +
> +struct al_pcie_acpi {
> + void __iomem *dbi_base;
> +};
> +
> +static void __iomem *al_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> + int where)
> +{
> + struct pci_config_window *cfg = bus->sysdata;
> + struct al_pcie_acpi *pcie = cfg->priv;
> + void __iomem *dbi_base = pcie->dbi_base;
> +
> + if (bus->number == cfg->busr.start) {
> + /*
> + * The DW PCIe core doesn't filter out transactions to other
> + * devices/functions on the primary bus num, so we do this here.
> + */
> + if (PCI_SLOT(devfn) > 0)
> + return NULL;
> + else
> + return dbi_base + where;
> + }
> +
> + return pci_ecam_map_bus(bus, devfn, where);
> +}
> +
> +static int al_pcie_init(struct pci_config_window *cfg)
> +{
> + struct device *dev = cfg->parent;
> + struct acpi_device *adev = to_acpi_device(dev);
> + struct acpi_pci_root *root = acpi_driver_data(adev);
> + struct al_pcie_acpi *al_pcie;
> + struct resource *res;
> + int ret;
> +
> + al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
> + if (!al_pcie)
> + return -ENOMEM;
> +
> + res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
> + if (!res)
> + return -ENOMEM;
> +
> + ret = acpi_get_rc_resources(dev, "AMZN0001", root->segment, res);
> + if (ret) {
> + dev_err(dev, "can't get rc dbi base address for SEG %d\n",
> + root->segment);
> + return ret;
> + }
> +
> + dev_dbg(dev, "Root port dbi res: %pR\n", res);
> +
> + al_pcie->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> + if (IS_ERR(al_pcie->dbi_base)) {
> + long err = PTR_ERR(al_pcie->dbi_base);
> +
> + dev_err(dev, "couldn't remap dbi base %pR (err:%ld)\n",
> + res, err);
> + return err;
> + }
> +
> + cfg->priv = al_pcie;
> +
> + return 0;
> +}
> +
> +struct pci_ecam_ops al_pcie_ops = {
> + .bus_shift = 20,
> + .init = al_pcie_init,
> + .pci_ops = {
> + .map_bus = al_pcie_map_bus,
> + .read = pci_generic_config_read,
> + .write = pci_generic_config_write,
> + }
> +};
> +
> +#endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
> index 29efa09d686b..a73164c85e78 100644
> --- a/include/linux/pci-ecam.h
> +++ b/include/linux/pci-ecam.h
> @@ -56,6 +56,7 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
> extern struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */
> extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene PCIe v1 */
> extern struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene PCIe v2.x */
> +extern struct pci_ecam_ops al_pcie_ops; /* Amazon Annapurna Labs PCIe */
> #endif
>
> #ifdef CONFIG_PCI_HOST_COMMON
> --
> 1.9.1
>

2019-04-25 14:24:45

by Bjorn Helgaas

Subject: Re: [PATCH v3] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

On Thu, Mar 28, 2019 at 01:57:56PM +0200, Jonathan Chocron wrote:
> Add support for Amazon's Annapurna Labs PCIe driver. The HW controller
> is based on DesignWare's IP.
>
> The HW doesn't support accessing the Root Port's config space via ECAM,
> so we obtain its base address via an AMZN0001 device.
>
> Furthermore, the DesignWare PCIe controller doesn't filter out config
> transactions sent to devices 1 and up on its bus, so they are filtered
> by the driver.
>
> All subordinate buses do support ECAM access.
>
> Implementing specific PCI config access functions involves:
> - Adding an init function to obtain the Root Port's base address from
> an AMZN0001 device.
> - Adding a new entry in the MCFG quirk array
>
> Co-developed-by: Vladimir Aerov <[email protected]>
> Signed-off-by: Jonathan Chocron <[email protected]>
> Signed-off-by: Vladimir Aerov <[email protected]>
> Reviewed-by: David Woodhouse <[email protected]>

Applied with Ben's reviewed-by and Lorenzo's ack to pci/host/al for v5.2,
thanks!

> ---
>
> --v2:
> - Fix commit message comments (incl. using AMZN0001 instead of
> PNP0C02)
> - Use the usual multi-line comment style
>
> --v3:
> - Fix additional commit message comments
>
> MAINTAINERS | 6 +++
> drivers/acpi/pci_mcfg.c | 12 +++++
> drivers/pci/controller/dwc/Makefile | 1 +
> drivers/pci/controller/dwc/pcie-al.c | 93 ++++++++++++++++++++++++++++++++++++
> include/linux/pci-ecam.h | 1 +
> 5 files changed, 113 insertions(+)
> create mode 100644 drivers/pci/controller/dwc/pcie-al.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 32d444476a90..7a17017f9f82 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11769,6 +11769,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
> S: Supported
> F: drivers/pci/controller/
>
> +PCIE DRIVER FOR ANNAPURNA LABS
> +M: Jonathan Chocron <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/pci/controller/dwc/pcie-al.c
> +
> PCIE DRIVER FOR AMLOGIC MESON
> M: Yue Wang <[email protected]>
> L: [email protected]
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index a4e8432fc2fb..b42be067fb83 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -52,6 +52,18 @@ struct mcfg_fixup {
> static struct mcfg_fixup mcfg_quirks[] = {
> /* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, ops, cfgres }, */
>
> +#define AL_ECAM(table_id, rev, seg, ops) \
> + { "AMAZON", table_id, rev, seg, MCFG_BUS_ANY, ops }
> +
> + AL_ECAM("GRAVITON", 0, 0, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 1, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 2, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 3, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 4, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 5, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 6, &al_pcie_ops),
> + AL_ECAM("GRAVITON", 0, 7, &al_pcie_ops),
> +
> #define QCOM_ECAM32(seg) \
> { "QCOM ", "QDF2432 ", 1, seg, MCFG_BUS_ANY, &pci_32b_ops }
>
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index 7bcdcdf5024e..1ea773c0070d 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -28,5 +28,6 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
> # depending on whether ACPI, the DT driver, or both are enabled.
>
> ifdef CONFIG_PCI
> +obj-$(CONFIG_ARM64) += pcie-al.o
> obj-$(CONFIG_ARM64) += pcie-hisi.o
> endif
> diff --git a/drivers/pci/controller/dwc/pcie-al.c b/drivers/pci/controller/dwc/pcie-al.c
> new file mode 100644
> index 000000000000..65a9776c12be
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-al.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCIe host controller driver for Amazon's Annapurna Labs IP (used in chips
> + * such as Graviton and Alpine)
> + *
> + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + *
> + * Author: Jonathan Chocron <[email protected]>
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/pci-ecam.h>
> +#include <linux/pci-acpi.h>
> +#include "../../pci.h"
> +
> +#if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
> +
> +struct al_pcie_acpi {
> + void __iomem *dbi_base;
> +};
> +
> +static void __iomem *al_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> + int where)
> +{
> + struct pci_config_window *cfg = bus->sysdata;
> + struct al_pcie_acpi *pcie = cfg->priv;
> + void __iomem *dbi_base = pcie->dbi_base;
> +
> + if (bus->number == cfg->busr.start) {
> + /*
> + * The DW PCIe core doesn't filter out transactions to other
> + * devices/functions on the primary bus num, so we do this here.
> + */
> + if (PCI_SLOT(devfn) > 0)
> + return NULL;
> + else
> + return dbi_base + where;
> + }
> +
> + return pci_ecam_map_bus(bus, devfn, where);
> +}
> +
> +static int al_pcie_init(struct pci_config_window *cfg)
> +{
> + struct device *dev = cfg->parent;
> + struct acpi_device *adev = to_acpi_device(dev);
> + struct acpi_pci_root *root = acpi_driver_data(adev);
> + struct al_pcie_acpi *al_pcie;
> + struct resource *res;
> + int ret;
> +
> + al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
> + if (!al_pcie)
> + return -ENOMEM;
> +
> + res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
> + if (!res)
> + return -ENOMEM;
> +
> + ret = acpi_get_rc_resources(dev, "AMZN0001", root->segment, res);
> + if (ret) {
> + dev_err(dev, "can't get rc dbi base address for SEG %d\n",
> + root->segment);
> + return ret;
> + }
> +
> + dev_dbg(dev, "Root port dbi res: %pR\n", res);
> +
> + al_pcie->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> + if (IS_ERR(al_pcie->dbi_base)) {
> + long err = PTR_ERR(al_pcie->dbi_base);
> +
> + dev_err(dev, "couldn't remap dbi base %pR (err:%ld)\n",
> + res, err);
> + return err;
> + }
> +
> + cfg->priv = al_pcie;
> +
> + return 0;
> +}
> +
> +struct pci_ecam_ops al_pcie_ops = {
> + .bus_shift = 20,
> + .init = al_pcie_init,
> + .pci_ops = {
> + .map_bus = al_pcie_map_bus,
> + .read = pci_generic_config_read,
> + .write = pci_generic_config_write,
> + }
> +};
> +
> +#endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
> index 29efa09d686b..a73164c85e78 100644
> --- a/include/linux/pci-ecam.h
> +++ b/include/linux/pci-ecam.h
> @@ -56,6 +56,7 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
> extern struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */
> extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene PCIe v1 */
> extern struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene PCIe v2.x */
> +extern struct pci_ecam_ops al_pcie_ops; /* Amazon Annapurna Labs PCIe */
> #endif
>
> #ifdef CONFIG_PCI_HOST_COMMON
> --
> 1.9.1
>