Message-ID: <20160512100103.GA19460@red-moon>
Date:	Thu, 12 May 2016 11:01:03 +0100
From:	Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:	Bjorn Helgaas <helgaas@...nel.org>
Cc:	"Rafael J. Wysocki" <rafael@...nel.org>,
	Tomasz Nowicki <tn@...ihalf.com>,
	Arnd Bergmann <arnd@...db.de>,
	Will Deacon <will.deacon@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	Hanjun Guo <hanjun.guo@...aro.org>,
	Sinan Kaya <okaya@...eaurora.org>, jchandra@...adcom.com,
	robert.richter@...iumnetworks.com, Marcin Wojtas <mw@...ihalf.com>,
	Liviu.Dudau@....com, David Daney <ddaney@...iumnetworks.com>,
	wangyijing@...wei.com,
	Suravee Suthikulanit <Suravee.Suthikulpanit@....com>,
	Mark Salter <msalter@...hat.com>,
	Linux PCI <linux-pci@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linaro-acpi@...ts.linaro.org" <linaro-acpi@...ts.linaro.org>,
	Jon Masters <jcm@...hat.com>,
	Andrea Gallo <andrea.gallo@...aro.org>,
	Duc Dang <dhdang@....com>, jeremy.linton@....com,
	liudongdong3@...wei.com, Christopher Covington <cov@...eaurora.org>
Subject: Re: [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment.

On Wed, May 11, 2016 at 05:43:14PM -0500, Bjorn Helgaas wrote:
> On Wed, May 11, 2016 at 10:30:51PM +0200, Rafael J. Wysocki wrote:
> > On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi
> > <lorenzo.pieralisi@....com> wrote:
> > > On Tue, May 10, 2016 at 08:37:00PM +0200, Rafael J. Wysocki wrote:
> > >> On Tue, May 10, 2016 at 5:19 PM, Tomasz Nowicki <tn@...ihalf.com> wrote:
> > >> > This patch provides a way to set the ACPI companion in PCI code.
> > >> > We define acpi_pci_set_companion() to set the ACPI companion pointer and
> > >> > call it from PCI core code. The function is stub for now.
> > >> >
> > >> > Signed-off-by: Jayachandran C <jchandra@...adcom.com>
> > >> > Signed-off-by: Tomasz Nowicki <tn@...ihalf.com>
> > >> > ---
> > >> >  drivers/pci/probe.c      | 2 ++
> > >> >  include/linux/pci-acpi.h | 4 ++++
> > >> >  2 files changed, 6 insertions(+)
> > >> >
> > >> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > >> > index 8004f67..fb0b752 100644
> > >> > --- a/drivers/pci/probe.c
> > >> > +++ b/drivers/pci/probe.c
> > >> > @@ -12,6 +12,7 @@
> > >> >  #include <linux/slab.h>
> > >> >  #include <linux/module.h>
> > >> >  #include <linux/cpumask.h>
> > >> > +#include <linux/pci-acpi.h>
> > >> >  #include <linux/pci-aspm.h>
> > >> >  #include <linux/aer.h>
> > >> >  #include <linux/acpi.h>
> > >> > @@ -2141,6 +2142,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> > >> >         bridge->dev.parent = parent;
> > >> >         bridge->dev.release = pci_release_host_bridge_dev;
> > >> >         dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> > >> > +       acpi_pci_set_companion(bridge);
> > >>
> > >> Yes, we'll probably add something similar here.
> > >>
> > >> Do I think now is the right time to do that?  No.
> > >>
> > >> >         error = pcibios_root_bridge_prepare(bridge);
> > >> >         if (error) {
> > >> >                 kfree(bridge);
> > >> > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> > >> > index 09f9f02..1baa515 100644
> > >> > --- a/include/linux/pci-acpi.h
> > >> > +++ b/include/linux/pci-acpi.h
> > >> > @@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> > >> >  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> > >> >  #endif /* CONFIG_ACPI */
> > >> >
> > >> > +static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
> > >> > +{
> > >> > +}
> > >> > +
> > >> >  static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
> > >> >  {
> > >> >         return 0;
> > >> > --
> > >>
> > >> Honestly, to me it looks like this series is trying very hard to avoid
> > >> doing any PCI host bridge configuration stuff from arch/arm64/
> > >> although (a) that might be simpler and (b) it would allow us to
> > >> identify the code that's common between *all* architectures using ACPI
> > >> support for host bridge configuration and to move *that* to a common
> > >> place later.  As done here it seems to be following the "ARM64 is
> > >> generic and the rest of the world is special" line which isn't really
> > >> helpful.
> > >
> > > I think patch [1-2] should be merged regardless (they may require minor
> > > tweaks if we decide to move pci_acpi_scan_root() to arch/arm64 though,
> > > for include files location). I guess you are referring to patch 8 in
> > > your comments above, which boils down to deciding whether:
> > >
> > > - pci_acpi_scan_root() (and unfortunately all the MCFG/ECAM handling that
> > >   goes with it) should live in arch/arm64 or drivers/acpi
> > 
> > To be precise, everything under #ifdef CONFIG_ACPI_PCI_HOST_GENERIC or
> > equivalent is de facto ARM64-specific, because (as it stands in the
> > patch series) ARM64 is the only architecture that will select that
> > option.  Unless you are aware of any more architectures planning to
> > use ACPI (and I'm not aware of any), it will stay the only
> > architecture selecting it in the foreseeable future.
> > 
> > Therefore you could replace CONFIG_ACPI_PCI_HOST_GENERIC with
> > CONFIG_ARM64 everywhere in that code which is why in my opinion the
> > code should live somewhere under arch/arm64/.
> > 
> > Going forward, it should be possible to identify common parts of the
> > PCI host bridge configuration code in arch/ and move it to
> > drivers/acpi/ or drivers/pci/, but I bet that won't be the entire code
> > this series puts under CONFIG_ACPI_PCI_HOST_GENERIC.
> > 
> > The above leads to a quite straightforward conclusion about the order
> > in which to do things: I'd add ACPI support for PCI host bridge on
> > ARM64 following what's been done on ia64 (as x86 is more quirky and
> > kludgy overall) as far as reasonably possible first and then think
> > about moving common stuff to a common place.
> 
> That does seem like a reasonable approach.  I had hoped to get more of
> this in for v4.7, but we don't have much time left.  Maybe some of
> Rafael's comments can be addressed by moving and slight restructuring
> and we can still squeeze it in.

Yes, it seems like a reasonable approach, as long as we accept that
part of this series has to live in arch/arm64 otherwise we are going
round in circles (because that's the gist of this discussion, to
decide where this code has to live, I do not think there is any objection
to the code per-se anymore).

I suggest we post a v8 (with code move to arch/arm64) end of merge
window (or you prefer seeing patches now to prevent any additional
changes later ?), my aim is to get this into -next (whether via arm64 or
pci tree it has to be decided) as early as possible for next cycle (-rc1)
so that it can get exposure and testing, I do not think that missing the
merge window is a big issue if we agree that the code is ready to go.

> The first three patches:
> 
>   PCI: Provide common functions for ECAM mapping
>   PCI: generic, thunder: Use generic ECAM API
>   PCI, of: Move PCI I/O space management to PCI core code
> 
> seem relatively straightforward, and I applied them to pci/arm64 with
> the intent of merging them unless there are objections.  I made the
> following tweaks, mainly to try to improve some error messages:

Ok, thanks a lot !

Lorenzo

> diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
> index 3d52005..e1add01 100644
> --- a/drivers/pci/ecam.c
> +++ b/drivers/pci/ecam.c
> @@ -24,9 +24,9 @@
>  #include "ecam.h"
>  
>  /*
> - * On 64 bit systems, we do a single ioremap for the whole config space
> - * since we have enough virtual address range available. On 32 bit, do an
> - * ioremap per bus.
> + * On 64-bit systems, we do a single ioremap for the whole config space
> + * since we have enough virtual address range available.  On 32-bit, we
> + * ioremap the config space for each bus individually.
>   */
>  static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT);
>  
> @@ -42,6 +42,7 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
>  {
>  	struct pci_config_window *cfg;
>  	unsigned int bus_range, bus_range_max, bsz;
> +	struct resource *conflict;
>  	int i, err;
>  
>  	if (busr->start > busr->end)
> @@ -58,10 +59,10 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
>  	bus_range = resource_size(&cfg->busr);
>  	bus_range_max = resource_size(cfgres) >> ops->bus_shift;
>  	if (bus_range > bus_range_max) {
> -		dev_warn(dev, "bus max %#x reduced to %#x",
> -					bus_range, bus_range_max);
>  		bus_range = bus_range_max;
>  		cfg->busr.end = busr->start + bus_range - 1;
> +		dev_warn(dev, "ECAM area %pR can only accommodate %pR (reduced from %pR desired)\n",
> +			 cfgres, &cfg->busr, busr);
>  	}
>  	bsz = 1 << ops->bus_shift;
>  
> @@ -70,9 +71,11 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
>  	cfg->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>  	cfg->res.name = "PCI ECAM";
>  
> -	err = request_resource(&iomem_resource, &cfg->res);
> -	if (err) {
> -		dev_err(dev, "request ECAM res %pR failed\n", &cfg->res);
> +	conflict = request_resource(&iomem_resource, &cfg->res);
> +	if (conflict) {
> +		err = -EBUSY;
> +		dev_err(dev, "can't claim ECAM area %pR: address conflict with %s %pR\n",
> +			&cfg->res, conflict->name, conflict);
>  		goto err_exit;
>  	}
>  
> diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
> index 1ad2176..9878beb 100644
> --- a/drivers/pci/ecam.h
> +++ b/drivers/pci/ecam.h
> @@ -33,7 +33,7 @@ struct pci_ecam_ops {
>  
>  /*
>   * struct to hold the mappings of a config space window. This
> - * is expected to be used as sysdata for PCI controlllers which
> + * is expected to be used as sysdata for PCI controllers that
>   * use ECAM.
>   */
>  struct pci_config_window {
> @@ -43,11 +43,11 @@ struct pci_config_window {
>  	struct pci_ecam_ops		*ops;
>  	union {
>  		void __iomem		*win;	/* 64-bit single mapping */
> -		void __iomem		**winp; /* 32-bit per bus mapping */
> +		void __iomem		**winp; /* 32-bit per-bus mapping */
>  	};
>  };
>  
> -/* create and free for pci_config_window */
> +/* create and free pci_config_window */
>  struct pci_config_window *pci_ecam_create(struct device *dev,
>  		struct resource *cfgres, struct resource *busr,
>  		struct pci_ecam_ops *ops);
> @@ -56,11 +56,11 @@ void pci_ecam_free(struct pci_config_window *cfg);
>  /* map_bus when ->sysdata is an instance of pci_config_window */
>  void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
>  			       int where);
> -/* default ECAM ops, bus shift 20, generic read and write */
> +/* default ECAM ops */
>  extern struct pci_ecam_ops pci_generic_ecam_ops;
>  
>  #ifdef CONFIG_PCI_HOST_GENERIC
> -/* for DT based pci controllers that support ECAM */
> +/* for DT-based PCI controllers that support ECAM */
>  int pci_host_common_probe(struct platform_device *pdev,
>  			  struct pci_ecam_ops *ops);
>  #endif
> 

Powered by blists - more mailing lists