diff mbox

[v7,5/5] PCI: ACPI: Add a generic ACPI based host controller

Message ID 1454058340-7904-6-git-send-email-jchandra@broadcom.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Jayachandran C. Jan. 29, 2016, 9:05 a.m. UTC
Add a simple ACPI based PCI host controller under config option
ACPI_PCI_HOST_GENERIC. This is done by providing an implementation
of pci_acpi_scan_root().

The pci_mmcfg_list handling is done by the ACPI code, so we keep a
reference to the pci_mmcfg_region in sysdata. The ECAM region will
be already mapped, so map_bus can be implemented by using the
virt pointer for the pci_mmcfg_region. pci_generic_config_read
and pci_generic_config_write are used for config space read/write.

Also, we provide implementations of raw_pci_read and raw_pci_write
hat are needed by ACPI based on the pci_mmcfg_list.

pci_acpi_set_companion() and acpi_pci_get_segment() are defined
using sysdata of generic ACPI host controller so that PCI domain
and ACPI companion are set in PCI code rather than platform code.

This code is currently enabled only for ARM64.

Signed-off-by: Jayachandran C <jchandra@broadcom.com>
---
 drivers/acpi/Kconfig         |   8 ++
 drivers/acpi/Makefile        |   1 +
 drivers/acpi/pci_host_acpi.c | 186 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h     |  17 ++++
 4 files changed, 212 insertions(+)
 create mode 100644 drivers/acpi/pci_host_acpi.c

Comments

Bjorn Helgaas Feb. 5, 2016, 12:19 a.m. UTC | #1
Hi Jayachandran,

On Fri, Jan 29, 2016 at 02:35:40PM +0530, Jayachandran C wrote:
> Add a simple ACPI based PCI host controller under config option
> ACPI_PCI_HOST_GENERIC. This is done by providing an implementation
> of pci_acpi_scan_root().
> 
> The pci_mmcfg_list handling is done by the ACPI code, so we keep a
> reference to the pci_mmcfg_region in sysdata. The ECAM region will
> be already mapped, so map_bus can be implemented by using the
> virt pointer for the pci_mmcfg_region. pci_generic_config_read
> and pci_generic_config_write are used for config space read/write.
> 
> Also, we provide implementations of raw_pci_read and raw_pci_write
> hat are needed by ACPI based on the pci_mmcfg_list.
> 
> pci_acpi_set_companion() and acpi_pci_get_segment() are defined
> using sysdata of generic ACPI host controller so that PCI domain
> and ACPI companion are set in PCI code rather than platform code.
> 
> This code is currently enabled only for ARM64.
> 
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> ---
>  drivers/acpi/Kconfig         |   8 ++
>  drivers/acpi/Makefile        |   1 +
>  drivers/acpi/pci_host_acpi.c | 186 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-acpi.h     |  17 ++++
>  4 files changed, 212 insertions(+)
>  create mode 100644 drivers/acpi/pci_host_acpi.c

I'm speaking a little bit out of turn here, because this is ACPI code,
but I'm confused about pci_host_acpi.c.  We already have pci_root.c,
which is *supposed* to be arch-independent.  I know pci_root.c is
crufty and could be improved, but it does work today on x86 and ia64,
and it handles some generic things that pci_host_acpi.c does not,
e.g., _OSC, NUMA, host bridge hotplug, etc.

I'd really like to see pci_root.c improved so it could work on x86,
ia64, and arm64.  I'm sure that was probably the first thing you
tried, so likely there are issues there.  Are they insurmountable?

Bjorn

> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 82b96ee..65fb483 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -294,6 +294,14 @@ config ACPI_NUMA
>  	depends on (X86 || IA64)
>  	default y if IA64_GENERIC || IA64_SGI_SN2
>  
> +config ACPI_PCI_HOST_GENERIC
> +	bool "Generic ACPI PCI host controller"
> +	depends on ARM64 && ACPI
> +	help
> +	  Say Y here if you want to support a simple generic ACPI PCI host
> +	  controller.
> +
> +
>  config ACPI_CUSTOM_DSDT_FILE
>  	string "Custom DSDT Table file to include"
>  	default ""
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 74976f1..346101c 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -41,6 +41,7 @@ acpi-y				+= ec.o
>  acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
>  acpi-y				+= pci_root.o pci_link.o pci_irq.o
>  acpi-$(CONFIG_PCI_MMCONFIG)	+= pci_mcfg.o
> +acpi-$(CONFIG_ACPI_PCI_HOST_GENERIC) += pci_host_acpi.o
>  acpi-y				+= acpi_lpss.o acpi_apd.o
>  acpi-y				+= acpi_platform.o
>  acpi-y				+= acpi_pnp.o
> diff --git a/drivers/acpi/pci_host_acpi.c b/drivers/acpi/pci_host_acpi.c
> new file mode 100644
> index 0000000..9dbdd81
> --- /dev/null
> +++ b/drivers/acpi/pci_host_acpi.c
> @@ -0,0 +1,186 @@
> +/*
> + * Generic PCI host controller driver for ACPI based systems
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Copyright (c) 2015 Broadcom Corporation
> + *
> + * Based on drivers/pci/host/pci-host-generic.c
> + * Copyright (C) 2014 ARM Limited
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/acpi.h>
> +#include <linux/pci-acpi.h>
> +#include <linux/sfi_acpi.h>
> +#include <linux/slab.h>
> +
> +#define PREFIX			"pci-host-acpi:"
> +
> +/* sysdata pointer is ->root_info */
> +struct gen_acpi_root_info {
> +	struct acpi_pci_root_info	common;
> +	struct pci_mmcfg_region		*mcfg;
> +	bool				mcfg_added;
> +};
> +
> +/* find mapping of a MCFG area */
> +static void __iomem *gen_acpi_map_cfg_bus(struct pci_bus *bus,
> +			unsigned int devfn, int where)
> +{
> +	struct gen_acpi_root_info *pci = bus->sysdata;
> +	struct pci_mmcfg_region *mcfg = pci->mcfg;
> +
> +	if (bus->number < mcfg->start_bus || bus->number > mcfg->end_bus)
> +		return NULL;
> +
> +	return mcfg->virt +
> +		PCI_MMCFG_OFFSET(bus->number - mcfg->start_bus, devfn) +
> +		where;
> +}
> +
> +static struct pci_ops gen_acpi_pci_ops = {
> +	.map_bus	= gen_acpi_map_cfg_bus,
> +	.read		= pci_generic_config_read,
> +	.write		= pci_generic_config_write,
> +};
> +
> +/* Insert the ECFG area for a root bus */
> +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> +{
> +	struct gen_acpi_root_info *info;
> +	struct acpi_pci_root *root = ci->root;
> +	struct device *dev = &ci->bridge->dev;
> +	int err;
> +
> +	info = container_of(ci, struct gen_acpi_root_info, common);
> +	err = pci_mmconfig_insert(dev, root->segment, root->secondary.start,
> +			root->secondary.end, root->mcfg_addr);
> +	if (err && err != -EEXIST)
> +		return err;
> +
> +	info->mcfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> +	WARN_ON(info->mcfg == NULL);
> +	info->mcfg_added = (err == -EEXIST);
> +	return 0;
> +}
> +
> +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> +{
> +	struct gen_acpi_root_info *info;
> +	struct acpi_pci_root *root = ci->root;
> +
> +	info = container_of(ci, struct gen_acpi_root_info, common);
> +	if (info->mcfg_added)
> +		pci_mmconfig_delete(root->segment, root->secondary.start,
> +					root->secondary.end);
> +	info->mcfg = NULL;
> +}
> +
> +static struct acpi_pci_root_ops pci_acpi_root_ops = {
> +	.pci_ops = &gen_acpi_pci_ops,
> +	.init_info = pci_acpi_root_init_info,
> +	.release_info = pci_acpi_root_release_info,
> +};
> +
> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> +{
> +	struct acpi_device *device = root->device;
> +	struct gen_acpi_root_info *ri;
> +	struct pci_bus *bus, *child;
> +
> +	/* allocate acpi_info/sysdata */
> +	ri = devm_kzalloc(&device->dev, sizeof(*ri), GFP_KERNEL);
> +	if (!ri) {
> +		dev_err(&device->dev,
> +			"pci_bus %04x:%02x: ignored (out of memory)\n",
> +			root->segment, (int)root->secondary.start);
> +		return NULL;
> +	}
> +
> +	bus =  acpi_pci_root_create(root, &pci_acpi_root_ops,
> +					&ri->common, ri);
> +	if (!bus) {
> +		dev_err(&device->dev, "Scanning rootbus failed");
> +		return NULL;
> +	}
> +
> +	pci_bus_size_bridges(bus);
> +	pci_bus_assign_resources(bus);
> +	list_for_each_entry(child, &bus->children, node)
> +		pcie_bus_configure_settings(child);
> +
> +	return bus;
> +}
> +
> +int raw_pci_read(unsigned int seg, unsigned int bus,
> +		  unsigned int devfn, int reg, int len, u32 *val)
> +{
> +	struct pci_mmcfg_region *mcfg;
> +	void __iomem *addr;
> +	int err = -EINVAL;
> +
> +	rcu_read_lock();
> +	mcfg = pci_mmconfig_lookup(seg, bus);
> +	if (!mcfg || !mcfg->virt)
> +		goto err;
> +
> +	addr = mcfg->virt + PCI_MMCFG_OFFSET(bus, devfn);
> +	switch (len) {
> +	case 1:
> +		*val = readb(addr + reg);
> +		break;
> +	case 2:
> +		*val = readw(addr + reg);
> +		break;
> +	case 4:
> +		*val = readl(addr + reg);
> +		break;
> +	}
> +	err = 0;
> +err:
> +	rcu_read_unlock();
> +	return err;
> +}
> +
> +int raw_pci_write(unsigned int seg, unsigned int bus,
> +		unsigned int devfn, int reg, int len, u32 val)
> +{
> +	struct pci_mmcfg_region *mcfg;
> +	void __iomem *addr;
> +	int err = -EINVAL;
> +
> +	rcu_read_lock();
> +	mcfg = pci_mmconfig_lookup(seg, bus);
> +	if (!mcfg || !mcfg->virt)
> +		goto err;
> +
> +	addr = mcfg->virt + PCI_MMCFG_OFFSET(bus, devfn);
> +	switch (len) {
> +	case 1:
> +		writeb(val, addr + reg);
> +		break;
> +	case 2:
> +		writew(val, addr + reg);
> +		break;
> +	case 4:
> +		writel(val, addr + reg);
> +		break;
> +	}
> +	err = 0;
> +err:
> +	rcu_read_unlock();
> +	return err;
> +}
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index d410885..f8d62e3 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -143,6 +143,22 @@ 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 */
>  
> +#if defined(CONFIG_ACPI) && defined(CONFIG_ACPI_PCI_HOST_GENERIC)
> +static inline void pci_acpi_set_companion(struct pci_host_bridge *bridge)
> +{
> +	struct pci_bus *b = bridge->bus;
> +	struct acpi_pci_root_info *ci = (struct acpi_pci_root_info *)b->sysdata;
> +
> +	ACPI_COMPANION_SET(&bridge->dev, ci->bridge);
> +}
> +
> +static inline u16 acpi_pci_get_segment(void *sysdata)
> +{
> +	struct acpi_pci_root_info *ci = (struct acpi_pci_root_info *)sysdata;
> +
> +	return ci->root->segment;
> +}
> +#else
>  static inline void pci_acpi_set_companion(struct pci_host_bridge *bridge)
>  {
>  	/* leave it to the platform for now */
> @@ -152,6 +168,7 @@ static inline u16 acpi_pci_get_segment(void *sysdata)
>  {
>  	return 0;
>  }
> +#endif
>  
>  #ifdef CONFIG_ACPI_APEI
>  extern bool aer_acpi_firmware_first(void);
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jayachandran Chandrashekaran Nair Feb. 5, 2016, 8:35 a.m. UTC | #2
Hi Bjorn,

On Fri, Feb 5, 2016 at 5:49 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> Hi Jayachandran,
>
> On Fri, Jan 29, 2016 at 02:35:40PM +0530, Jayachandran C wrote:
>> Add a simple ACPI based PCI host controller under config option
>> ACPI_PCI_HOST_GENERIC. This is done by providing an implementation
>> of pci_acpi_scan_root().
>>
>> The pci_mmcfg_list handling is done by the ACPI code, so we keep a
>> reference to the pci_mmcfg_region in sysdata. The ECAM region will
>> be already mapped, so map_bus can be implemented by using the
>> virt pointer for the pci_mmcfg_region. pci_generic_config_read
>> and pci_generic_config_write are used for config space read/write.
>>
>> Also, we provide implementations of raw_pci_read and raw_pci_write
>> hat are needed by ACPI based on the pci_mmcfg_list.
>>
>> pci_acpi_set_companion() and acpi_pci_get_segment() are defined
>> using sysdata of generic ACPI host controller so that PCI domain
>> and ACPI companion are set in PCI code rather than platform code.
>>
>> This code is currently enabled only for ARM64.
>>
>> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>> ---
>>  drivers/acpi/Kconfig         |   8 ++
>>  drivers/acpi/Makefile        |   1 +
>>  drivers/acpi/pci_host_acpi.c | 186 +++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/pci-acpi.h     |  17 ++++
>>  4 files changed, 212 insertions(+)
>>  create mode 100644 drivers/acpi/pci_host_acpi.c
>
> I'm speaking a little bit out of turn here, because this is ACPI code,
> but I'm confused about pci_host_acpi.c.  We already have pci_root.c,
> which is *supposed* to be arch-independent.  I know pci_root.c is
> crufty and could be improved, but it does work today on x86 and ia64,
> and it handles some generic things that pci_host_acpi.c does not,
> e.g., _OSC, NUMA, host bridge hotplug, etc.
>
> I'd really like to see pci_root.c improved so it could work on x86,
> ia64, and arm64.  I'm sure that was probably the first thing you
> tried, so likely there are issues there.  Are they insurmountable?

pci_root.c leaves the implementation of pci_acpi_scan_root() to the
architecture. Implementing pci_acpi_scan_root needs a
pci_acpi_root_ops instance and a pci_ops instance and related functions.
The architecture is also expected to implement raw_pci_read and
raw_pci_write.

pci_host_acpi.c is a generic implementation of these using a sysdata
pointing to acpi_pci_root_info, and using a pointer to the pci_mmcfg_region
to access ECAM area, Maybe I can rename this file to
pci_acpi_host_generic.c to reflect this better.

arm64 is the only user of this generic implementation now. The config
option CONFIG_ACPI_PCI_HOST_GENERIC has to be set on the
architecture that chooses to use this generic code instead of its own
implementation.

JC.
{Sorry if the formatting is wrong, using webmail due to internal IT changes]
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi Feb. 5, 2016, 9:47 a.m. UTC | #3
On Fri, Feb 05, 2016 at 02:05:37PM +0530, Jayachandran Chandrashekaran Nair wrote:

[...]

> pci_host_acpi.c is a generic implementation of these using a sysdata
> pointing to acpi_pci_root_info, and using a pointer to the pci_mmcfg_region
> to access ECAM area, Maybe I can rename this file to
> pci_acpi_host_generic.c to reflect this better.

Maybe you should stop sending this series and work with Tomasz to
get this done, you are confusing everyone and I am really really
annoyed about this.

Do you realize there is no point in having two patch series doing
the same thing and wasting everyone's review time ?

Do you realize he started this work long before you and went through
several rounds of review already (I told you before but in case you
forgot) ?

Tomasz posted a version yesterday, integrating comments following months
of review and testing and I think it is ready to get upstream:

https://lkml.org/lkml/2016/2/4/646

Did you even consider reviewing his code or helping him instead of
churning out more patches doing the *SAME* thing ?

Do you want all of us to go through your code and re-fix what has
already been fixed in Tomasz's series with the end result of missing
yet another merge window ?

This is really annoying, stop it please, really.

Thank you,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Feb. 5, 2016, 11:26 p.m. UTC | #4
On Friday, February 05, 2016 09:47:40 AM Lorenzo Pieralisi wrote:
> On Fri, Feb 05, 2016 at 02:05:37PM +0530, Jayachandran Chandrashekaran Nair wrote:
> 
> [...]
> 
> > pci_host_acpi.c is a generic implementation of these using a sysdata
> > pointing to acpi_pci_root_info, and using a pointer to the pci_mmcfg_region
> > to access ECAM area, Maybe I can rename this file to
> > pci_acpi_host_generic.c to reflect this better.
> 
> Maybe you should stop sending this series and work with Tomasz to
> get this done, you are confusing everyone and I am really really
> annoyed about this.
> 
> Do you realize there is no point in having two patch series doing
> the same thing and wasting everyone's review time ?
> 
> Do you realize he started this work long before you and went through
> several rounds of review already (I told you before but in case you
> forgot) ?
> 
> Tomasz posted a version yesterday, integrating comments following months
> of review and testing and I think it is ready to get upstream:
> 
> https://lkml.org/lkml/2016/2/4/646
> 
> Did you even consider reviewing his code or helping him instead of
> churning out more patches doing the *SAME* thing ?
> 
> Do you want all of us to go through your code and re-fix what has
> already been fixed in Tomasz's series with the end result of missing
> yet another merge window ?
> 
> This is really annoying, stop it please, really.

OK, so to be crystal clear here.

I'm going to ignore the series the $subject patch belongs to going forward.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jayachandran Chandrashekaran Nair Feb. 6, 2016, 9:58 a.m. UTC | #5
Hi Rafael,

On Sat, Feb 6, 2016 at 4:56 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, February 05, 2016 09:47:40 AM Lorenzo Pieralisi wrote:
>> On Fri, Feb 05, 2016 at 02:05:37PM +0530, Jayachandran Chandrashekaran Nair wrote:
>>
>> [...]
>>
>> > pci_host_acpi.c is a generic implementation of these using a sysdata
>> > pointing to acpi_pci_root_info, and using a pointer to the pci_mmcfg_region
>> > to access ECAM area, Maybe I can rename this file to
>> > pci_acpi_host_generic.c to reflect this better.
>>
>> Maybe you should stop sending this series and work with Tomasz to
>> get this done, you are confusing everyone and I am really really
>> annoyed about this.
>>
>> Do you realize there is no point in having two patch series doing
>> the same thing and wasting everyone's review time ?
>>
>> Do you realize he started this work long before you and went through
>> several rounds of review already (I told you before but in case you
>> forgot) ?
>>
>> Tomasz posted a version yesterday, integrating comments following months
>> of review and testing and I think it is ready to get upstream:
>>
>> https://lkml.org/lkml/2016/2/4/646
>>
>> Did you even consider reviewing his code or helping him instead of
>> churning out more patches doing the *SAME* thing ?
>>
>> Do you want all of us to go through your code and re-fix what has
>> already been fixed in Tomasz's series with the end result of missing
>> yet another merge window ?
>>
>> This is really annoying, stop it please, really.
>
> OK, so to be crystal clear here.
>
> I'm going to ignore the series the $subject patch belongs to going forward.

It looks like you can already reviewed Tomasz's patchset and as Lorenzo
says thinks that it can be merged.

When I first posted the patchset the state of arm64 ACPI/PCI was not that
clear - and I thought an alternative will help. Ideally I would expect
maintainers to look at technical merit, but in this case any working solution
merged will be great news.

Thanks,
JC.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jayachandran Chandrashekaran Nair Feb. 8, 2016, 11:27 a.m. UTC | #6
Lorenzo,

On Fri, Feb 5, 2016 at 3:17 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Fri, Feb 05, 2016 at 02:05:37PM +0530, Jayachandran Chandrashekaran Nair wrote:
>
> [...]
>
>> pci_host_acpi.c is a generic implementation of these using a sysdata
>> pointing to acpi_pci_root_info, and using a pointer to the pci_mmcfg_region
>> to access ECAM area, Maybe I can rename this file to
>> pci_acpi_host_generic.c to reflect this better.
>
> Maybe you should stop sending this series and work with Tomasz to
> get this done, you are confusing everyone and I am really really
> annoyed about this.
>
> Do you realize there is no point in having two patch series doing
> the same thing and wasting everyone's review time ?
>
> Do you realize he started this work long before you and went through
> several rounds of review already (I told you before but in case you
> forgot) ?
>
> Tomasz posted a version yesterday, integrating comments following months
> of review and testing and I think it is ready to get upstream:
>
> https://lkml.org/lkml/2016/2/4/646
>
> Did you even consider reviewing his code or helping him instead of
> churning out more patches doing the *SAME* thing ?

This is getting ridiculous, I had replied to your earlier mails on why
my patchset is NOT doing the exact same thing. I had also explained
why helping was not feasible.

The basic point again: I am trying to give a much simpler patchset
to solve the same problem, I take it that you haven't reviewed my
patchset before writing this mail. I would have appreciated
a technical discussion rather than this pointless flamefest.

If you have reviewed it, you can see that there are just 5 patches
instead of 23, and that overall it is a much simpler approach.

> Do you want all of us to go through your code and re-fix what has
> already been fixed in Tomasz's series with the end result of missing
> yet another merge window ?

Again, as I said, the whole point of my patchset is that is much  easier
to go thru and validate and merge or drop as needed.

Anyway, if you are on target to hit this merge window, I really hope
that happens and there will no need for me to spend further time on
this.

Regards,
JC.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi Feb. 10, 2016, 1:30 p.m. UTC | #7
On Mon, Feb 08, 2016 at 04:57:46PM +0530, Jayachandran Chandrashekaran Nair wrote:
> Lorenzo,
> 
> On Fri, Feb 5, 2016 at 3:17 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Fri, Feb 05, 2016 at 02:05:37PM +0530, Jayachandran Chandrashekaran Nair wrote:
> >
> > [...]
> >
> >> pci_host_acpi.c is a generic implementation of these using a sysdata
> >> pointing to acpi_pci_root_info, and using a pointer to the pci_mmcfg_region
> >> to access ECAM area, Maybe I can rename this file to
> >> pci_acpi_host_generic.c to reflect this better.
> >
> > Maybe you should stop sending this series and work with Tomasz to
> > get this done, you are confusing everyone and I am really really
> > annoyed about this.
> >
> > Do you realize there is no point in having two patch series doing
> > the same thing and wasting everyone's review time ?
> >
> > Do you realize he started this work long before you and went through
> > several rounds of review already (I told you before but in case you
> > forgot) ?
> >
> > Tomasz posted a version yesterday, integrating comments following months
> > of review and testing and I think it is ready to get upstream:
> >
> > https://lkml.org/lkml/2016/2/4/646
> >
> > Did you even consider reviewing his code or helping him instead of
> > churning out more patches doing the *SAME* thing ?
> 
> This is getting ridiculous, I had replied to your earlier mails on why
> my patchset is NOT doing the exact same thing. I had also explained
> why helping was not feasible.
> 
> The basic point again: I am trying to give a much simpler patchset
> to solve the same problem, I take it that you haven't reviewed my
> patchset before writing this mail. I would have appreciated
> a technical discussion rather than this pointless flamefest.
> 
> If you have reviewed it, you can see that there are just 5 patches
> instead of 23, and that overall it is a much simpler approach.

Ok, let's make it constructive. I think there is part of your
implementation that definitely makes sense (in particular the way you
cleaned-up the x86 MCFG unadulterated mess - patch 1), I will ask
Tomasz to integrate it, please work together on this.

I have nothing against your patchset, my point is that we can't keep
reviewing and testing two series (and I mean on ARM64 AND x86), please
understand my point, it is very time consuming to understand the
differences and make sure we don't break x86 in the process and I would
have to ask you to add all the code that I already reviewed in Tomasz's
set, I just do not want to do that.

I will reply to Tomasz, let's work together to have a single final
implementation please, I do not think I am asking too much here and
yes, by integrating part of your code I think Tomasz's patchset is
ready to go, obviously subject to Bjorn's review and opinion.

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 82b96ee..65fb483 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -294,6 +294,14 @@  config ACPI_NUMA
 	depends on (X86 || IA64)
 	default y if IA64_GENERIC || IA64_SGI_SN2
 
+config ACPI_PCI_HOST_GENERIC
+	bool "Generic ACPI PCI host controller"
+	depends on ARM64 && ACPI
+	help
+	  Say Y here if you want to support a simple generic ACPI PCI host
+	  controller.
+
+
 config ACPI_CUSTOM_DSDT_FILE
 	string "Custom DSDT Table file to include"
 	default ""
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 74976f1..346101c 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -41,6 +41,7 @@  acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
 acpi-y				+= pci_root.o pci_link.o pci_irq.o
 acpi-$(CONFIG_PCI_MMCONFIG)	+= pci_mcfg.o
+acpi-$(CONFIG_ACPI_PCI_HOST_GENERIC) += pci_host_acpi.o
 acpi-y				+= acpi_lpss.o acpi_apd.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
diff --git a/drivers/acpi/pci_host_acpi.c b/drivers/acpi/pci_host_acpi.c
new file mode 100644
index 0000000..9dbdd81
--- /dev/null
+++ b/drivers/acpi/pci_host_acpi.c
@@ -0,0 +1,186 @@ 
+/*
+ * Generic PCI host controller driver for ACPI based systems
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (c) 2015 Broadcom Corporation
+ *
+ * Based on drivers/pci/host/pci-host-generic.c
+ * Copyright (C) 2014 ARM Limited
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/acpi.h>
+#include <linux/pci-acpi.h>
+#include <linux/sfi_acpi.h>
+#include <linux/slab.h>
+
+#define PREFIX			"pci-host-acpi:"
+
+/* sysdata pointer is ->root_info */
+struct gen_acpi_root_info {
+	struct acpi_pci_root_info	common;
+	struct pci_mmcfg_region		*mcfg;
+	bool				mcfg_added;
+};
+
+/* find mapping of a MCFG area */
+static void __iomem *gen_acpi_map_cfg_bus(struct pci_bus *bus,
+			unsigned int devfn, int where)
+{
+	struct gen_acpi_root_info *pci = bus->sysdata;
+	struct pci_mmcfg_region *mcfg = pci->mcfg;
+
+	if (bus->number < mcfg->start_bus || bus->number > mcfg->end_bus)
+		return NULL;
+
+	return mcfg->virt +
+		PCI_MMCFG_OFFSET(bus->number - mcfg->start_bus, devfn) +
+		where;
+}
+
+static struct pci_ops gen_acpi_pci_ops = {
+	.map_bus	= gen_acpi_map_cfg_bus,
+	.read		= pci_generic_config_read,
+	.write		= pci_generic_config_write,
+};
+
+/* Insert the ECFG area for a root bus */
+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
+{
+	struct gen_acpi_root_info *info;
+	struct acpi_pci_root *root = ci->root;
+	struct device *dev = &ci->bridge->dev;
+	int err;
+
+	info = container_of(ci, struct gen_acpi_root_info, common);
+	err = pci_mmconfig_insert(dev, root->segment, root->secondary.start,
+			root->secondary.end, root->mcfg_addr);
+	if (err && err != -EEXIST)
+		return err;
+
+	info->mcfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+	WARN_ON(info->mcfg == NULL);
+	info->mcfg_added = (err == -EEXIST);
+	return 0;
+}
+
+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
+{
+	struct gen_acpi_root_info *info;
+	struct acpi_pci_root *root = ci->root;
+
+	info = container_of(ci, struct gen_acpi_root_info, common);
+	if (info->mcfg_added)
+		pci_mmconfig_delete(root->segment, root->secondary.start,
+					root->secondary.end);
+	info->mcfg = NULL;
+}
+
+static struct acpi_pci_root_ops pci_acpi_root_ops = {
+	.pci_ops = &gen_acpi_pci_ops,
+	.init_info = pci_acpi_root_init_info,
+	.release_info = pci_acpi_root_release_info,
+};
+
+struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
+{
+	struct acpi_device *device = root->device;
+	struct gen_acpi_root_info *ri;
+	struct pci_bus *bus, *child;
+
+	/* allocate acpi_info/sysdata */
+	ri = devm_kzalloc(&device->dev, sizeof(*ri), GFP_KERNEL);
+	if (!ri) {
+		dev_err(&device->dev,
+			"pci_bus %04x:%02x: ignored (out of memory)\n",
+			root->segment, (int)root->secondary.start);
+		return NULL;
+	}
+
+	bus =  acpi_pci_root_create(root, &pci_acpi_root_ops,
+					&ri->common, ri);
+	if (!bus) {
+		dev_err(&device->dev, "Scanning rootbus failed");
+		return NULL;
+	}
+
+	pci_bus_size_bridges(bus);
+	pci_bus_assign_resources(bus);
+	list_for_each_entry(child, &bus->children, node)
+		pcie_bus_configure_settings(child);
+
+	return bus;
+}
+
+int raw_pci_read(unsigned int seg, unsigned int bus,
+		  unsigned int devfn, int reg, int len, u32 *val)
+{
+	struct pci_mmcfg_region *mcfg;
+	void __iomem *addr;
+	int err = -EINVAL;
+
+	rcu_read_lock();
+	mcfg = pci_mmconfig_lookup(seg, bus);
+	if (!mcfg || !mcfg->virt)
+		goto err;
+
+	addr = mcfg->virt + PCI_MMCFG_OFFSET(bus, devfn);
+	switch (len) {
+	case 1:
+		*val = readb(addr + reg);
+		break;
+	case 2:
+		*val = readw(addr + reg);
+		break;
+	case 4:
+		*val = readl(addr + reg);
+		break;
+	}
+	err = 0;
+err:
+	rcu_read_unlock();
+	return err;
+}
+
+int raw_pci_write(unsigned int seg, unsigned int bus,
+		unsigned int devfn, int reg, int len, u32 val)
+{
+	struct pci_mmcfg_region *mcfg;
+	void __iomem *addr;
+	int err = -EINVAL;
+
+	rcu_read_lock();
+	mcfg = pci_mmconfig_lookup(seg, bus);
+	if (!mcfg || !mcfg->virt)
+		goto err;
+
+	addr = mcfg->virt + PCI_MMCFG_OFFSET(bus, devfn);
+	switch (len) {
+	case 1:
+		writeb(val, addr + reg);
+		break;
+	case 2:
+		writew(val, addr + reg);
+		break;
+	case 4:
+		writel(val, addr + reg);
+		break;
+	}
+	err = 0;
+err:
+	rcu_read_unlock();
+	return err;
+}
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index d410885..f8d62e3 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -143,6 +143,22 @@  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 */
 
+#if defined(CONFIG_ACPI) && defined(CONFIG_ACPI_PCI_HOST_GENERIC)
+static inline void pci_acpi_set_companion(struct pci_host_bridge *bridge)
+{
+	struct pci_bus *b = bridge->bus;
+	struct acpi_pci_root_info *ci = (struct acpi_pci_root_info *)b->sysdata;
+
+	ACPI_COMPANION_SET(&bridge->dev, ci->bridge);
+}
+
+static inline u16 acpi_pci_get_segment(void *sysdata)
+{
+	struct acpi_pci_root_info *ci = (struct acpi_pci_root_info *)sysdata;
+
+	return ci->root->segment;
+}
+#else
 static inline void pci_acpi_set_companion(struct pci_host_bridge *bridge)
 {
 	/* leave it to the platform for now */
@@ -152,6 +168,7 @@  static inline u16 acpi_pci_get_segment(void *sysdata)
 {
 	return 0;
 }
+#endif
 
 #ifdef CONFIG_ACPI_APEI
 extern bool aer_acpi_firmware_first(void);