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

From: David Woodhouse <dwmw2@infradead.org>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Jonathan Chocron <jonnyc@amazon.com>
Cc: linux-pci@vger.kernel.org, bhelgaas@google.com,
	linux-kernel@vger.kernel.org, vaerov@amazon.com,
	benh@kernel.crashing.org, alisaidi@amazon.com, zeev@amazon.com,
	ronenk@amazon.com, barakw@amazon.com,
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
	Zhou Wang <wangzhou1@hisilicon.com>
Subject: Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver
Date: Tue, 26 Mar 2019 13:24:41 +0000	[thread overview]
Message-ID: <07838dad0e0cdb14c42080ef19c11829f72c8814.camel@infradead.org> (raw)
In-Reply-To: <20190326121727.GA4171@e107981-ln.cambridge.arm.com>

[-- Attachment #1: Type: text/plain, Size: 2969 bytes --]

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 <vaerov@amazon.com>
> > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > Signed-off-by: Vladimir Aerov <vaerov@amazon.com>
> > Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> 
> 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 <dwmw@amazon.co.uk>

> > 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 <jonnyc@amazon.com>
> > +L:	linux-pci@vger.kernel.org
> > +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.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]