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

Commit Message

Chocron, Jonathan March 26, 2019, 10 a.m. UTC

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 <[e-mail]>
Signed-off-by: Jonathan Chocron <[e-mail]>
Signed-off-by: Vladimir Aerov <[e-mail]>
Reviewed-by: Benjamin Herrenschmidt <[e-mail]>
Reviewed-by: David Woodhouse <[e-mail]>
---

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

Comments

Lorenzo Pieralisi March 26, 2019, 12:17 p.m. UTC | #1

[+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 <[e-mail]>
> Signed-off-by: Jonathan Chocron <[e-mail]>
> Signed-off-by: Vladimir Aerov <[e-mail]>
> Reviewed-by: Benjamin Herrenschmidt <[e-mail]>
> Reviewed-by: David Woodhouse <[e-mail]>

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 <[e-mail]>
> +L:	[e-mail]
> +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 <[e-mail]>
>  L:	[e-mail]
> 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 <[e-mail]>
> + */
> +
> +#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
>

Bjorn Helgaas March 26, 2019, 12:55 p.m. UTC | #2

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

David Woodhouse March 26, 2019, 1:24 p.m. UTC | #3

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 <[e-mail]>
> > Signed-off-by: Jonathan Chocron <[e-mail]>
> > Signed-off-by: Vladimir Aerov <[e-mail]>
> > Reviewed-by: Benjamin Herrenschmidt <[e-mail]>
> > Reviewed-by: David Woodhouse <[e-mail]>
> 
> 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 <[e-mail]>


> > 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 <[e-mail]>
> > +L:	[e-mail]
> > +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.

Lorenzo Pieralisi March 26, 2019, 3:58 p.m. UTC | #4

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 <[e-mail]>
> > > Signed-off-by: Jonathan Chocron <[e-mail]>
> > > Signed-off-by: Vladimir Aerov <[e-mail]>
> > > Reviewed-by: Benjamin Herrenschmidt <[e-mail]>
> > > Reviewed-by: David Woodhouse <[e-mail]>
> > 
> > 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 <[e-mail]>
> 
> 
> > > 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 <[e-mail]>
> > > +L:	[e-mail]
> > > +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.
>

Woodhouse, David March 27, 2019, 9:43 a.m. UTC | #5

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.

David Woodhouse March 27, 2019, 9:52 a.m. UTC | #6

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. :)

Lorenzo Pieralisi March 27, 2019, 11:20 a.m. UTC | #7

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 [e-mail] for PCI patches,
I do not think I am asking too much.

Thanks,
Lorenzo

Lorenzo Pieralisi March 27, 2019, 11:39 a.m. UTC | #8

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

David Woodhouse March 27, 2019, 11:40 a.m. UTC | #9

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 [e-mail] 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 :)

David Woodhouse March 27, 2019, 12:01 p.m. UTC | #10

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.

Chocron, Jonathan March 28, 2019, 10:55 a.m. UTC | #11

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

Patch

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 <[e-mail]>
+L:	[e-mail]
+S:	Maintained
+F:	drivers/pci/controller/dwc/pcie-al.c
+
 PCIE DRIVER FOR AMLOGIC MESON
 M:	Yue Wang <[e-mail]>
 L:	[e-mail]
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 <[e-mail]>
+ */
+
+#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