diff mbox

[V6,08/13] PCI: generic, thunder: update to use generic ECAM API

Message ID 1460740008-19489-9-git-send-email-tn@semihalf.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomasz Nowicki April 15, 2016, 5:06 p.m. UTC
From: Jayachandran C <jchandra@broadcom.com>

Use functions provided by drivers/pci/ecam.h for mapping the config
space in drivers/pci/host/pci-host-common.c, and update its users to
use 'struct pci_config_window' and 'struct pci_generic_ecam_ops'

The changes are mostly to use 'struct pci_config_window' in place of
'struct gen_pci'. Some of the fields of gen_pci were only used
temporarily and can be eliminated by using local variables or function
arguments, these are not carried over to struct pci_config_window.

pci-thunder-ecam.c and pci-thunder-pem.c are the only users of the
pci_host_common_probe function and the gen_pci structure, these have
been updated to use the new API as well.

The patch does not introduce any functional changes other than a very
minor one: with the new code, on 64-bit platforms, we do just a single
ioremap for the whole config space.

Signed-off-by: Jayachandran C <jchandra@broadcom.com>
---
 drivers/pci/ecam.h                  |   5 ++
 drivers/pci/host/Kconfig            |   1 +
 drivers/pci/host/pci-host-common.c  | 119 ++++++++++++++++--------------------
 drivers/pci/host/pci-host-common.h  |  47 --------------
 drivers/pci/host/pci-host-generic.c |  52 +++-------------
 drivers/pci/host/pci-thunder-ecam.c |  39 +++---------
 drivers/pci/host/pci-thunder-pem.c  |  88 ++++++++++++--------------
 7 files changed, 115 insertions(+), 236 deletions(-)
 delete mode 100644 drivers/pci/host/pci-host-common.h

Comments

Arnd Bergmann April 15, 2016, 6:39 p.m. UTC | #1
On Friday 15 April 2016 19:06:43 Tomasz Nowicki wrote:
> -MODULE_DEVICE_TABLE(of, thunder_pem_of_match);
> -
> -static int thunder_pem_probe(struct platform_device *pdev)
> +static int thunder_pem_init(struct device *dev, struct pci_config_window *cfg)
>  {
> -       struct device *dev = &pdev->dev;
> -       const struct of_device_id *of_id;
>         resource_size_t bar4_start;
>         struct resource *res_pem;
>         struct thunder_pem_pci *pem_pci;
> +       struct platform_device *pdev;
>  
>         pem_pci = devm_kzalloc(dev, sizeof(*pem_pci), GFP_KERNEL);
>         if (!pem_pci)
>                 return -ENOMEM;
>  
> -       of_id = of_match_node(thunder_pem_of_match, dev->of_node);
> -       pem_pci->gen_pci.cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data;
> +       pdev = to_platform_device(dev);
>  
>         /*
>          * The second register range is the PEM bridge to the PCIe
> @@ -330,7 +298,29 @@ static int thunder_pem_probe(struct platform_device *pdev)
>         pem_pci->ea_entry[1] = (u32)(res_pem->end - bar4_start) & ~3u;
>         pem_pci->ea_entry[2] = (u32)(bar4_start >> 32);
>  
> -       return pci_host_common_probe(pdev, &pem_pci->gen_pci);
> +       cfg->priv = pem_pci;
> +       return 0;
> +}
> +
> 

I still think it would be better to keep the loadable PCI host drivers
separate from the ACPI PCI infrastructure. There are a number of
simplifications that we want to do to the DT based drivers in the long
run, so it's better if that code is not shared at this level. Abstracting
out the ECAM code is fine, but at that point you should be able to just
call it from the ACPI layer.

	Arnd
Jayachandran C. April 16, 2016, 7:20 a.m. UTC | #2
On Sat, Apr 16, 2016 at 12:09 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 15 April 2016 19:06:43 Tomasz Nowicki wrote:
>> -MODULE_DEVICE_TABLE(of, thunder_pem_of_match);
>> -
>> -static int thunder_pem_probe(struct platform_device *pdev)
>> +static int thunder_pem_init(struct device *dev, struct pci_config_window *cfg)
>>  {
>> -       struct device *dev = &pdev->dev;
>> -       const struct of_device_id *of_id;
>>         resource_size_t bar4_start;
>>         struct resource *res_pem;
>>         struct thunder_pem_pci *pem_pci;
>> +       struct platform_device *pdev;
>>
>>         pem_pci = devm_kzalloc(dev, sizeof(*pem_pci), GFP_KERNEL);
>>         if (!pem_pci)
>>                 return -ENOMEM;
>>
>> -       of_id = of_match_node(thunder_pem_of_match, dev->of_node);
>> -       pem_pci->gen_pci.cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data;
>> +       pdev = to_platform_device(dev);
>>
>>         /*
>>          * The second register range is the PEM bridge to the PCIe
>> @@ -330,7 +298,29 @@ static int thunder_pem_probe(struct platform_device *pdev)
>>         pem_pci->ea_entry[1] = (u32)(res_pem->end - bar4_start) & ~3u;
>>         pem_pci->ea_entry[2] = (u32)(bar4_start >> 32);
>>
>> -       return pci_host_common_probe(pdev, &pem_pci->gen_pci);
>> +       cfg->priv = pem_pci;
>> +       return 0;
>> +}
>> +
>>
>
> I still think it would be better to keep the loadable PCI host drivers
> separate from the ACPI PCI infrastructure. There are a number of
> simplifications that we want to do to the DT based drivers in the long
> run, so it's better if that code is not shared at this level. Abstracting
> out the ECAM code is fine, but at that point you should be able to just
> call it from the ACPI layer.

The issue is not with this patch (in my opinion). This patch is just
re-arranging how thunder specific data is maintained. Earlier it was
a container_of gen_pci, now it is ->priv of pci_config_window.

I can see the issue in patches 12 and 13 of this patchset which adds
ACPI fixups into the thunder OF driver.

The simple approach when doing modular PCI drivers would be to make
pci-thunder-*.c like pci-host-common.c, to be compiled in if configured.
The fie will contain all the Thunder quirks and can export
pci_thunder_ecam_ops.

Then the OF driver part will be trivial and can be merged into
pci-host-generic.c which can be a module. The ACPI hooks can be
moved to the ACPI PCI host driver file.

Would appreciate any suggestions on the way forward.

Thanks,
JC.
Arnd Bergmann April 16, 2016, 7:31 a.m. UTC | #3
On Saturday 16 April 2016 12:50:13 Jayachandran C wrote:
> >
> > I still think it would be better to keep the loadable PCI host drivers
> > separate from the ACPI PCI infrastructure. There are a number of
> > simplifications that we want to do to the DT based drivers in the long
> > run, so it's better if that code is not shared at this level. Abstracting
> > out the ECAM code is fine, but at that point you should be able to just
> > call it from the ACPI layer.
> 
> The issue is not with this patch (in my opinion). This patch is just
> re-arranging how thunder specific data is maintained. Earlier it was
> a container_of gen_pci, now it is ->priv of pci_config_window.
> 
> I can see the issue in patches 12 and 13 of this patchset which adds
> ACPI fixups into the thunder OF driver.

Right, I commented on this one, because it seems to rearrange the code
in order to do the later one.

> The simple approach when doing modular PCI drivers would be to make
> pci-thunder-*.c like pci-host-common.c, to be compiled in if configured.
> The fie will contain all the Thunder quirks and can export
> pci_thunder_ecam_ops.

I would argue that we should not export anything from drivers/pci/host,
those should really be standalone drivers that do not interact with other
subsystems.

How much code would you need to duplicate from thunder-ecam to have
the same functionality available in ACPI? My expectation is that it's
not really that much more compared to the code you need for sharing
a single implementation, but you get a lower complexity here, which
makes it easier to understand and to rework.

	Arnd
Jayachandran C. April 16, 2016, 2:36 p.m. UTC | #4
On Sat, Apr 16, 2016 at 1:01 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 16 April 2016 12:50:13 Jayachandran C wrote:
>> >
>> > I still think it would be better to keep the loadable PCI host drivers
>> > separate from the ACPI PCI infrastructure. There are a number of
>> > simplifications that we want to do to the DT based drivers in the long
>> > run, so it's better if that code is not shared at this level. Abstracting
>> > out the ECAM code is fine, but at that point you should be able to just
>> > call it from the ACPI layer.
>>
>> The issue is not with this patch (in my opinion). This patch is just
>> re-arranging how thunder specific data is maintained. Earlier it was
>> a container_of gen_pci, now it is ->priv of pci_config_window.
>>
>> I can see the issue in patches 12 and 13 of this patchset which adds
>> ACPI fixups into the thunder OF driver.
>
> Right, I commented on this one, because it seems to rearrange the code
> in order to do the later one.

Patches 11- 13 are not from me, and I am not completely on board
on the approach of adding the sections.We can look at reworking this.

>> The simple approach when doing modular PCI drivers would be to make
>> pci-thunder-*.c like pci-host-common.c, to be compiled in if configured.
>> The fie will contain all the Thunder quirks and can export
>> pci_thunder_ecam_ops.
>
> I would argue that we should not export anything from drivers/pci/host,
> those should really be standalone drivers that do not interact with other
> subsystems.

pci-host-common.c goes against being standalone. The files calling
pci_host_common_probe() are expected to have custom ECAM ops
the way it is written now. We need to have a reasonable way to share
those ECAM ops if needed by ACPI.

> How much code would you need to duplicate from thunder-ecam to have
> the same functionality available in ACPI? My expectation is that it's
> not really that much more compared to the code you need for sharing
> a single implementation, but you get a lower complexity here, which
> makes it easier to understand and to rework.

Like I wrote above, the sharing is really simple because both generic
ACPI and pci-host-common.c have been written for "ECAM with quirks".

The whole pci-thunder-*.c is to support thunder PCI quirks since the
generic OF is handled by pci-host-common.c and generic ECAM is now
separated -  duplicating the whole file for ACPI will be bad.

Any suggestions on how to do this better would be really welcome.

Thanks,
JC.
Tomasz Nowicki April 18, 2016, 1:03 p.m. UTC | #5
On 16.04.2016 16:36, Jayachandran C wrote:
> On Sat, Apr 16, 2016 at 1:01 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Saturday 16 April 2016 12:50:13 Jayachandran C wrote:
>>>>
>>>> I still think it would be better to keep the loadable PCI host drivers
>>>> separate from the ACPI PCI infrastructure. There are a number of
>>>> simplifications that we want to do to the DT based drivers in the long
>>>> run, so it's better if that code is not shared at this level. Abstracting
>>>> out the ECAM code is fine, but at that point you should be able to just
>>>> call it from the ACPI layer.
>>>
>>> The issue is not with this patch (in my opinion). This patch is just
>>> re-arranging how thunder specific data is maintained. Earlier it was
>>> a container_of gen_pci, now it is ->priv of pci_config_window.
>>>
>>> I can see the issue in patches 12 and 13 of this patchset which adds
>>> ACPI fixups into the thunder OF driver.
>>
>> Right, I commented on this one, because it seems to rearrange the code
>> in order to do the later one.
>
> Patches 11- 13 are not from me, and I am not completely on board
> on the approach of adding the sections.We can look at reworking this.
>
>>> The simple approach when doing modular PCI drivers would be to make
>>> pci-thunder-*.c like pci-host-common.c, to be compiled in if configured.
>>> The fie will contain all the Thunder quirks and can export
>>> pci_thunder_ecam_ops.
>>
>> I would argue that we should not export anything from drivers/pci/host,
>> those should really be standalone drivers that do not interact with other
>> subsystems.
>
> pci-host-common.c goes against being standalone. The files calling
> pci_host_common_probe() are expected to have custom ECAM ops
> the way it is written now. We need to have a reasonable way to share
> those ECAM ops if needed by ACPI.
>
>> How much code would you need to duplicate from thunder-ecam to have
>> the same functionality available in ACPI? My expectation is that it's
>> not really that much more compared to the code you need for sharing
>> a single implementation, but you get a lower complexity here, which
>> makes it easier to understand and to rework.
>
> Like I wrote above, the sharing is really simple because both generic
> ACPI and pci-host-common.c have been written for "ECAM with quirks".
>
> The whole pci-thunder-*.c is to support thunder PCI quirks since the
> generic OF is handled by pci-host-common.c and generic ECAM is now
> separated -  duplicating the whole file for ACPI will be bad.

Yes, it would be too much code duplication. Also, we already know 
drivers which need quirks.

We really need to agree on best approach here. Here are requirements 
which came up (please correct me if misunderstood sth):

Arnd:
1. Initial DT driver should be standalone [Arnd]
2. No exported symbols [Arnd]
3. Duplicate necessary code to ACPI framework.

JC:
1. Adding linker section is wrong.
2. Quirks should be exported (pci_thunder_ecam_ops), then no need for 
adding linker section
3. To much duplication to copy code into the ACPI framework.

My opinion:
1. I like linker section because it is easy to maintain and no need to 
export symbols.
2. We need more sophisticated algorithm for matching quirks (DMI is not 
enough and not only for ThunderX drivers). Of course I am open to any 
new suggestions.
3. To much duplication to copy code into the ACPI framework.

Thanks in advance for any pointers.

Thanks,
Tomasz
Arnd Bergmann April 18, 2016, 2:44 p.m. UTC | #6
On Monday 18 April 2016 15:03:51 Tomasz Nowicki wrote:
> On 16.04.2016 16:36, Jayachandran C wrote:
> > On Sat, Apr 16, 2016 at 1:01 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Saturday 16 April 2016 12:50:13 Jayachandran C wrote:
> > The whole pci-thunder-*.c is to support thunder PCI quirks since the
> > generic OF is handled by pci-host-common.c and generic ECAM is now
> > separated -  duplicating the whole file for ACPI will be bad.
> 
> Yes, it would be too much code duplication. Also, we already know 
> drivers which need quirks.
> 
> We really need to agree on best approach here. Here are requirements 
> which came up (please correct me if misunderstood sth):
> 
> Arnd:
> 1. Initial DT driver should be standalone [Arnd]
> 2. No exported symbols [Arnd]
> 3. Duplicate necessary code to ACPI framework.

Correct.

> JC:
> 1. Adding linker section is wrong.
> 2. Quirks should be exported (pci_thunder_ecam_ops), then no need for 
> adding linker section
> 3. To much duplication to copy code into the ACPI framework.
> 
> My opinion:
> 1. I like linker section because it is easy to maintain and no need to 
> export symbols.
> 2. We need more sophisticated algorithm for matching quirks (DMI is not 
> enough and not only for ThunderX drivers). Of course I am open to any 
> new suggestions.

Agreed.

> 3. To much duplication to copy code into the ACPI framework.
> 
> Thanks in advance for any pointers.

Can you be more specific about what code actually would need to
be duplicated? Anything besides the config space operations?

	Arnd
Tomasz Nowicki April 18, 2016, 7:31 p.m. UTC | #7
On 18.04.2016 16:44, Arnd Bergmann wrote:
> On Monday 18 April 2016 15:03:51 Tomasz Nowicki wrote:
>> On 16.04.2016 16:36, Jayachandran C wrote:
>>> On Sat, Apr 16, 2016 at 1:01 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Saturday 16 April 2016 12:50:13 Jayachandran C wrote:
>>> The whole pci-thunder-*.c is to support thunder PCI quirks since the
>>> generic OF is handled by pci-host-common.c and generic ECAM is now
>>> separated -  duplicating the whole file for ACPI will be bad.
>>
>> Yes, it would be too much code duplication. Also, we already know
>> drivers which need quirks.
>>
>> We really need to agree on best approach here. Here are requirements
>> which came up (please correct me if misunderstood sth):
>>
>> Arnd:
>> 1. Initial DT driver should be standalone [Arnd]
>> 2. No exported symbols [Arnd]
>> 3. Duplicate necessary code to ACPI framework.
>
> Correct.
>
>> JC:
>> 1. Adding linker section is wrong.
>> 2. Quirks should be exported (pci_thunder_ecam_ops), then no need for
>> adding linker section
>> 3. To much duplication to copy code into the ACPI framework.
>>
>> My opinion:
>> 1. I like linker section because it is easy to maintain and no need to
>> export symbols.
>> 2. We need more sophisticated algorithm for matching quirks (DMI is not
>> enough and not only for ThunderX drivers). Of course I am open to any
>> new suggestions.
>
> Agreed.
>
>> 3. To much duplication to copy code into the ACPI framework.
>>
>> Thanks in advance for any pointers.
>
> Can you be more specific about what code actually would need to
> be duplicated? Anything besides the config space operations?
>

Basically the whole content of pci-thunder-ecam.c and pci-thunder-pem.c.

pci-thunder-ecam.c contains config space accessors. Similar for 
pci-thunder-pem.c but it also has extra init call (it is now called 
thunder_pem_init) which finds and maps related registers.

Thanks,
Tomasz
Arnd Bergmann April 19, 2016, 1:06 p.m. UTC | #8
On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
> 
> Basically the whole content of pci-thunder-ecam.c and pci-thunder-pem.c.
> 
> pci-thunder-ecam.c contains config space accessors. Similar for 
> pci-thunder-pem.c but it also has extra init call (it is now called 
> thunder_pem_init) which finds and maps related registers.

They seem to do much more than just override the accessors, they actually
change the contents of the config space as well. Is that really necessary
on ACPI based systems as well?

Another idea: how about moving all of this logic into ACPI and calling
some AML method to access the config space if the devices are that
far out of spec.

	Arnd
Arnd Bergmann April 19, 2016, 9:40 p.m. UTC | #9
On Friday 15 April 2016 19:06:43 Tomasz Nowicki wrote:
> From: Jayachandran C <jchandra@broadcom.com>
>
> Add config option PCI_GENERIC_ECAM and file drivers/pci/ecam.c to
> provide generic functions for accessing memory mapped PCI config space.
>
> The API is defined in drivers/pci/ecam.h and is written to replace the
> API in drivers/pci/host/pci-host-common.h. The file defines a new
> 'struct pci_config_window' to hold the information related to a PCI
> config area and its mapping. This structure is expected to be used as
> sysdata for controllers that have ECAM based mapping.
>
> Helper functions are provided to setup the mapping, free the mapping
> and to implement the map_bus method in 'struct pci_ops'
>
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>

I've taken a fresh look now at what is going on here.

> @@ -58,4 +58,9 @@ void __iomem *pci_generic_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
>  /* default ECAM ops, bus shift 20, generic read and write */
>  extern struct pci_generic_ecam_ops pci_generic_ecam_default_ops;
>  
> +#ifdef CONFIG_PCI_HOST_GENERIC
> +/* for DT based pci controllers that support ECAM */
> +int pci_host_common_probe(struct platform_device *pdev,
> +			  struct pci_generic_ecam_ops *ops);
> +#endif
>  #endif

This doesn't seem to belong here: just leave the declaration
in the existing file. 

> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 7a0780d..31d6eb5 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -82,6 +82,7 @@ config PCI_HOST_GENERIC
>  	bool "Generic PCI host controller"
>  	depends on (ARM || ARM64) && OF
>  	select PCI_HOST_COMMON
> +	select PCI_GENERIC_ECAM
>  	help
>  	  Say Y here if you want to support a simple generic PCI host
>  	  controller, such as the one emulated by kvmtool.
> diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
> index e9f850f..99d99b3 100644
> --- a/drivers/pci/host/pci-host-common.c
> +++ b/drivers/pci/host/pci-host-common.c
> @@ -22,27 +22,21 @@
>  #include <linux/of_pci.h>
>  #include <linux/platform_device.h>
>  
> -#include "pci-host-common.h"
> +#include "../ecam.h"

As mentioned, don't use headers from parent directories, anything
that needs to be shared must go into include/linux, while the parts
that are only needed in one directory should be declared there.

> -static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
> +static void gen_pci_generic_unmap_cfg(void *ptr)
> +{
> +	pci_generic_ecam_free((struct pci_config_window *)ptr);
> +}

Why the void pointer?

> +static struct pci_generic_ecam_ops pci_thunder_pem_ops = {
> +	.bus_shift	= 24,
> +	.init		= thunder_pem_init,
> +	.pci_ops	= {
> +		.map_bus	= pci_generic_ecam_map_bus,
> +		.read		= thunder_pem_config_read,
> +		.write		= thunder_pem_config_write,
> +	}
> +};

Adding the callback pointer for init here and yet another structure
pci_config_window really seems to go too far with the number of
abstraction levels.

I think here it makes much more sense to just implement ECAM pci_ops
in ACPI separately, as the implementation really trivial to start with,
and all the complexity comes just from trying to share it with other
stuff. Doesn't ACPI already have an ECAM implementation for x86
that you could simply use?

	Arnd
Jayachandran C. April 20, 2016, 12:22 a.m. UTC | #10
On Wed, Apr 20, 2016 at 3:10 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 15 April 2016 19:06:43 Tomasz Nowicki wrote:
>> From: Jayachandran C <jchandra@broadcom.com>
>>
>> Add config option PCI_GENERIC_ECAM and file drivers/pci/ecam.c to
>> provide generic functions for accessing memory mapped PCI config space.
>>
>> The API is defined in drivers/pci/ecam.h and is written to replace the
>> API in drivers/pci/host/pci-host-common.h. The file defines a new
>> 'struct pci_config_window' to hold the information related to a PCI
>> config area and its mapping. This structure is expected to be used as
>> sysdata for controllers that have ECAM based mapping.
>>
>> Helper functions are provided to setup the mapping, free the mapping
>> and to implement the map_bus method in 'struct pci_ops'
>>
>> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>
> I've taken a fresh look now at what is going on here.
>
>> @@ -58,4 +58,9 @@ void __iomem *pci_generic_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
>>  /* default ECAM ops, bus shift 20, generic read and write */
>>  extern struct pci_generic_ecam_ops pci_generic_ecam_default_ops;
>>
>> +#ifdef CONFIG_PCI_HOST_GENERIC
>> +/* for DT based pci controllers that support ECAM */
>> +int pci_host_common_probe(struct platform_device *pdev,
>> +                       struct pci_generic_ecam_ops *ops);
>> +#endif
>>  #endif
>
> This doesn't seem to belong here: just leave the declaration
> in the existing file.

This can be done, the file would just have one line so I thought
it made sense to move it to ecam.h where the struct is defined.

>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index 7a0780d..31d6eb5 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -82,6 +82,7 @@ config PCI_HOST_GENERIC
>>       bool "Generic PCI host controller"
>>       depends on (ARM || ARM64) && OF
>>       select PCI_HOST_COMMON
>> +     select PCI_GENERIC_ECAM
>>       help
>>         Say Y here if you want to support a simple generic PCI host
>>         controller, such as the one emulated by kvmtool.
>> diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
>> index e9f850f..99d99b3 100644
>> --- a/drivers/pci/host/pci-host-common.c
>> +++ b/drivers/pci/host/pci-host-common.c
>> @@ -22,27 +22,21 @@
>>  #include <linux/of_pci.h>
>>  #include <linux/platform_device.h>
>>
>> -#include "pci-host-common.h"
>> +#include "../ecam.h"
>
> As mentioned, don't use headers from parent directories, anything
> that needs to be shared must go into include/linux, while the parts
> that are only needed in one directory should be declared there.

This is also ok - It can either go to pci.h or a separate pci-ecam.h

>> -static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>> +static void gen_pci_generic_unmap_cfg(void *ptr)
>> +{
>> +     pci_generic_ecam_free((struct pci_config_window *)ptr);
>> +}
>
> Why the void pointer?

devm_add_action() needs it.

>> +static struct pci_generic_ecam_ops pci_thunder_pem_ops = {
>> +     .bus_shift      = 24,
>> +     .init           = thunder_pem_init,
>> +     .pci_ops        = {
>> +             .map_bus        = pci_generic_ecam_map_bus,
>> +             .read           = thunder_pem_config_read,
>> +             .write          = thunder_pem_config_write,
>> +     }
>> +};
>
> Adding the callback pointer for init here and yet another structure
> pci_config_window really seems to go too far with the number of
> abstraction levels.

The abstraction was already there in pci-host-common.h for
ops for ECAM/CAM based controllers. It made sense to move it
to ecam.h and use it for ECAM based ACPI [1].

We need to pass pci_ops, bus_shift and an additional pointer
for quirks for ECAM based host controllers. Having it as a
structure pci_generic_ecam_ops reduces the function arguments,
and also keeps most of the older API.

> I think here it makes much more sense to just implement ECAM pci_ops
> in ACPI separately, as the implementation really trivial to start with,
> and all the complexity comes just from trying to share it with other
> stuff. Doesn't ACPI already have an ECAM implementation for x86
> that you could simply use?

The implementation is extremely trivial on 64 bit, and slightly more
complex in 32bit (pci-host-common.c per bus mapping and set_pte
based mapping on  x86). The generic ACPI on 64 bit is very simple
if there are no quirks,I have already posted that [2] some time back.

ACPI on x86 also has a 32 bit and a 64 bit version
(arch/x86/pci/mmconfig_{32,64}.c}. The code there is a bit messed
up and it does not make sense to share or reuse that.

There has been suggestions earlier from Bjorn on sharing the
ECAM implementation[1], which was the starting point of
doing this patch.

Overall, this patch improves config window mapping for
pci-host-common.c based drivers on 64 bit and deletes
quite a bit of duplicated code. I would argue that this makes
sense even without ACPI.

JC.

[1] https://lkml.org/lkml/2016/3/3/921
[2] http://article.gmane.org/gmane.linux.kernel.pci/47753
Tomasz Nowicki April 21, 2016, 9:28 a.m. UTC | #11
On 19.04.2016 15:06, Arnd Bergmann wrote:
> On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
>>
>> Basically the whole content of pci-thunder-ecam.c and pci-thunder-pem.c.
>>
>> pci-thunder-ecam.c contains config space accessors. Similar for
>> pci-thunder-pem.c but it also has extra init call (it is now called
>> thunder_pem_init) which finds and maps related registers.
>
> They seem to do much more than just override the accessors, they actually
> change the contents of the config space as well. Is that really necessary
> on ACPI based systems as well?

Yes, the pci-thunder-ecam.c accessors are meant to emulate config space 
capabilities. They are necessary to synthesize EA capabilities (fixed 
PCI BARs), it wont work without this, for ACPI boot as well.

>
> Another idea: how about moving all of this logic into ACPI and calling
> some AML method to access the config space if the devices are that
> far out of spec.

Do you mean Linux specific way to call non-standard config space 
accessors? Then non-standard accessors are going to AML methods which 
are called from common code which handles quirks via unified API ?

Thanks,
Tomasz
Arnd Bergmann April 21, 2016, 9:36 a.m. UTC | #12
On Thursday 21 April 2016 11:28:15 Tomasz Nowicki wrote:
> On 19.04.2016 15:06, Arnd Bergmann wrote:
> > On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
> >>
> >> Basically the whole content of pci-thunder-ecam.c and pci-thunder-pem.c.
> >>
> >> pci-thunder-ecam.c contains config space accessors. Similar for
> >> pci-thunder-pem.c but it also has extra init call (it is now called
> >> thunder_pem_init) which finds and maps related registers.
> >
> > They seem to do much more than just override the accessors, they actually
> > change the contents of the config space as well. Is that really necessary
> > on ACPI based systems as well?
> 
> Yes, the pci-thunder-ecam.c accessors are meant to emulate config space 
> capabilities. They are necessary to synthesize EA capabilities (fixed 
> PCI BARs), it wont work without this, for ACPI boot as well.

Why is that? I thought the BARs never get reassigned when using ACPI,
so I'm surprised it's actually needed. Maybe I misunderstood what
you mean by fixed PCI BARs.

> > Another idea: how about moving all of this logic into ACPI and calling
> > some AML method to access the config space if the devices are that
> > far out of spec.
> 
> Do you mean Linux specific way to call non-standard config space 
> accessors? Then non-standard accessors are going to AML methods which 
> are called from common code which handles quirks via unified API ?

What I really meant was a standardized way to do handle hardware that
is in some way or another not compliant with PNP0A08: We could have
a different hardware ID for this and let all the first-generation
ARM servers and also anything else using ACPI with nonstandard PCI
use the same method across operating systems.

	Arnd
Tomasz Nowicki April 21, 2016, 10:08 a.m. UTC | #13
On 21.04.2016 11:36, Arnd Bergmann wrote:
> On Thursday 21 April 2016 11:28:15 Tomasz Nowicki wrote:
>> On 19.04.2016 15:06, Arnd Bergmann wrote:
>>> On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
>>>>
>>>> Basically the whole content of pci-thunder-ecam.c and pci-thunder-pem.c.
>>>>
>>>> pci-thunder-ecam.c contains config space accessors. Similar for
>>>> pci-thunder-pem.c but it also has extra init call (it is now called
>>>> thunder_pem_init) which finds and maps related registers.
>>>
>>> They seem to do much more than just override the accessors, they actually
>>> change the contents of the config space as well. Is that really necessary
>>> on ACPI based systems as well?
>>
>> Yes, the pci-thunder-ecam.c accessors are meant to emulate config space
>> capabilities. They are necessary to synthesize EA capabilities (fixed
>> PCI BARs), it wont work without this, for ACPI boot as well.
>
> Why is that? I thought the BARs never get reassigned when using ACPI,
> so I'm surprised it's actually needed. Maybe I misunderstood what
> you mean by fixed PCI BARs.

Yes, I meant something else. ThunderX has non-programmable PCI BAR 
addresses. So it uses PCI EA (Extended allocation) capabilities to get 
know PCI BARs addresses. But the early implementation (pass1.x) misses 
EA capabilities hence we need to emulate it in config space accessors.

Tomasz
Jon Masters April 22, 2016, 2:30 p.m. UTC | #14
On 04/21/2016 06:08 AM, Tomasz Nowicki wrote:
> On 21.04.2016 11:36, Arnd Bergmann wrote:
>> On Thursday 21 April 2016 11:28:15 Tomasz Nowicki wrote:
>>> On 19.04.2016 15:06, Arnd Bergmann wrote:
>>>> On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
>>>>>
>>>>> Basically the whole content of pci-thunder-ecam.c and
>>>>> pci-thunder-pem.c.
>>>>>
>>>>> pci-thunder-ecam.c contains config space accessors. Similar for
>>>>> pci-thunder-pem.c but it also has extra init call (it is now called
>>>>> thunder_pem_init) which finds and maps related registers.
>>>>
>>>> They seem to do much more than just override the accessors, they
>>>> actually
>>>> change the contents of the config space as well. Is that really
>>>> necessary
>>>> on ACPI based systems as well?
>>>
>>> Yes, the pci-thunder-ecam.c accessors are meant to emulate config space
>>> capabilities. They are necessary to synthesize EA capabilities (fixed
>>> PCI BARs), it wont work without this, for ACPI boot as well.
>>
>> Why is that? I thought the BARs never get reassigned when using ACPI,
>> so I'm surprised it's actually needed. Maybe I misunderstood what
>> you mean by fixed PCI BARs.
> 
> Yes, I meant something else. ThunderX has non-programmable PCI BAR
> addresses. So it uses PCI EA (Extended allocation) capabilities to get
> know PCI BARs addresses. But the early implementation (pass1.x) misses
> EA capabilities hence we need to emulate it in config space accessors.

Aside: In case it's helpful, at least one enterprise vendor I know of is
only supporting later silicon as a result of this. So IMO there's no
need to worry about this issue on the early preproduction chips.
David Daney April 22, 2016, 4 p.m. UTC | #15
On 04/22/2016 07:30 AM, Jon Masters wrote:
> On 04/21/2016 06:08 AM, Tomasz Nowicki wrote:
>> On 21.04.2016 11:36, Arnd Bergmann wrote:
>>> On Thursday 21 April 2016 11:28:15 Tomasz Nowicki wrote:
>>>> On 19.04.2016 15:06, Arnd Bergmann wrote:
>>>>> On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
>>>>>>
>>>>>> Basically the whole content of pci-thunder-ecam.c and
>>>>>> pci-thunder-pem.c.
>>>>>>
>>>>>> pci-thunder-ecam.c contains config space accessors. Similar for
>>>>>> pci-thunder-pem.c but it also has extra init call (it is now called
>>>>>> thunder_pem_init) which finds and maps related registers.
>>>>>
>>>>> They seem to do much more than just override the accessors, they
>>>>> actually
>>>>> change the contents of the config space as well. Is that really
>>>>> necessary
>>>>> on ACPI based systems as well?
>>>>
>>>> Yes, the pci-thunder-ecam.c accessors are meant to emulate config space
>>>> capabilities. They are necessary to synthesize EA capabilities (fixed
>>>> PCI BARs), it wont work without this, for ACPI boot as well.
>>>
>>> Why is that? I thought the BARs never get reassigned when using ACPI,
>>> so I'm surprised it's actually needed. Maybe I misunderstood what
>>> you mean by fixed PCI BARs.
>>
>> Yes, I meant something else. ThunderX has non-programmable PCI BAR
>> addresses. So it uses PCI EA (Extended allocation) capabilities to get
>> know PCI BARs addresses. But the early implementation (pass1.x) misses
>> EA capabilities hence we need to emulate it in config space accessors.
>
> Aside: In case it's helpful, at least one enterprise vendor I know of is
> only supporting later silicon as a result of this. So IMO there's no
> need to worry about this issue on the early preproduction chips.
>

There are two separate issues that make fixing up the ECAM space necessary:

1) As Jon mentioned, preproduction silicon lacks EA capabilities.

2) On 2-node NUMA systems, the EA capabilities of some devices may be 
incorrect even in production silicon.

In general, the strategy we use for dealing with both of these is to 
hook into the ECAM access methods, and supply corrected config space 
data.  For the case of device-tree provisioned ECAM access, the fix ups 
are done in pci-thunder-ecam.c.  This code is already present and seems 
to be working well.

As we consider ACPI support, supporting case #2 above will be desirable. 
  If we reuse the code in pci-thunder-ecam.c for this, we will probably 
get support for #1 for free.

David Daney
Bjorn Helgaas April 28, 2016, 8:14 p.m. UTC | #16
On Thu, Apr 21, 2016 at 11:36:53AM +0200, Arnd Bergmann wrote:
> On Thursday 21 April 2016 11:28:15 Tomasz Nowicki wrote:
> > On 19.04.2016 15:06, Arnd Bergmann wrote:
> > > On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
> > >>
> > >> Basically the whole content of pci-thunder-ecam.c and pci-thunder-pem.c.
> > >>
> > >> pci-thunder-ecam.c contains config space accessors. Similar for
> > >> pci-thunder-pem.c but it also has extra init call (it is now called
> > >> thunder_pem_init) which finds and maps related registers.
> > >
> > > They seem to do much more than just override the accessors, they actually
> > > change the contents of the config space as well. Is that really necessary
> > > on ACPI based systems as well?
> > 
> > Yes, the pci-thunder-ecam.c accessors are meant to emulate config space 
> > capabilities. They are necessary to synthesize EA capabilities (fixed 
> > PCI BARs), it wont work without this, for ACPI boot as well.
> 
> Why is that? I thought the BARs never get reassigned when using ACPI,

In general, there's no reason we can't reassign BARs, whether we're
using DT, ACPI, or whatever.  In many cases, systems with ACPI also
assign all the BARs in firmware, and Linux doesn't reassign them
unless it needs to.  But that's just a coincidence.  There's no
requirement that Linux leave BARs as firmware programmed them.
Arnd Bergmann April 28, 2016, 8:40 p.m. UTC | #17
On Thursday 28 April 2016 15:14:39 Bjorn Helgaas wrote:
> On Thu, Apr 21, 2016 at 11:36:53AM +0200, Arnd Bergmann wrote:
> > On Thursday 21 April 2016 11:28:15 Tomasz Nowicki wrote:
> > > On 19.04.2016 15:06, Arnd Bergmann wrote:
> > > > On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
> > > >>
> > > >> Basically the whole content of pci-thunder-ecam.c and pci-thunder-pem.c.
> > > >>
> > > >> pci-thunder-ecam.c contains config space accessors. Similar for
> > > >> pci-thunder-pem.c but it also has extra init call (it is now called
> > > >> thunder_pem_init) which finds and maps related registers.
> > > >
> > > > They seem to do much more than just override the accessors, they actually
> > > > change the contents of the config space as well. Is that really necessary
> > > > on ACPI based systems as well?
> > > 
> > > Yes, the pci-thunder-ecam.c accessors are meant to emulate config space 
> > > capabilities. They are necessary to synthesize EA capabilities (fixed 
> > > PCI BARs), it wont work without this, for ACPI boot as well.
> > 
> > Why is that? I thought the BARs never get reassigned when using ACPI,
> 
> In general, there's no reason we can't reassign BARs, whether we're
> using DT, ACPI, or whatever.  In many cases, systems with ACPI also
> assign all the BARs in firmware, and Linux doesn't reassign them
> unless it needs to.  But that's just a coincidence.  There's no
> requirement that Linux leave BARs as firmware programmed them.

I'm thought I've seen systems in which the ACPI BIOS assumes that
certain PCI devices never move around, because it pokes the registers
from AML, and changing them would require never using the same device
through ACPI. It's likely that this is against some standard, but that
won't help you if you have to deal with the system anyway.

	Arnd
Bjorn Helgaas April 28, 2016, 9:18 p.m. UTC | #18
On Thu, Apr 28, 2016 at 10:40:35PM +0200, Arnd Bergmann wrote:
> On Thursday 28 April 2016 15:14:39 Bjorn Helgaas wrote:
> > On Thu, Apr 21, 2016 at 11:36:53AM +0200, Arnd Bergmann wrote:
> > > On Thursday 21 April 2016 11:28:15 Tomasz Nowicki wrote:
> > > > On 19.04.2016 15:06, Arnd Bergmann wrote:
> > > > > On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
> > > > >>
> > > > >> Basically the whole content of pci-thunder-ecam.c and pci-thunder-pem.c.
> > > > >>
> > > > >> pci-thunder-ecam.c contains config space accessors. Similar for
> > > > >> pci-thunder-pem.c but it also has extra init call (it is now called
> > > > >> thunder_pem_init) which finds and maps related registers.
> > > > >
> > > > > They seem to do much more than just override the accessors, they actually
> > > > > change the contents of the config space as well. Is that really necessary
> > > > > on ACPI based systems as well?
> > > > 
> > > > Yes, the pci-thunder-ecam.c accessors are meant to emulate config space 
> > > > capabilities. They are necessary to synthesize EA capabilities (fixed 
> > > > PCI BARs), it wont work without this, for ACPI boot as well.
> > > 
> > > Why is that? I thought the BARs never get reassigned when using ACPI,
> > 
> > In general, there's no reason we can't reassign BARs, whether we're
> > using DT, ACPI, or whatever.  In many cases, systems with ACPI also
> > assign all the BARs in firmware, and Linux doesn't reassign them
> > unless it needs to.  But that's just a coincidence.  There's no
> > requirement that Linux leave BARs as firmware programmed them.
> 
> I'm thought I've seen systems in which the ACPI BIOS assumes that
> certain PCI devices never move around, because it pokes the registers
> from AML, and changing them would require never using the same device
> through ACPI. It's likely that this is against some standard, but that
> won't help you if you have to deal with the system anyway.

Yes, I'm pretty sure there are systems like that, e.g., I think SMM
code on some HP servers assumes the iLO address never changes.  I
think that is a firmware defect because I don't think there's any spec
that says firmware retains control over PCI BARs after handoff.  And
this particular case isn't really ACPI-specific.

But as you say, we have to deal with these systems anyway, even if we
consider that behavior broken.  My proposal has been to add quirks to
mark those devices as IORESOURCE_PCI_FIXED, but I don't think anybody
has gotten around to doing that.

Bjorn
Jon Masters April 28, 2016, 9:47 p.m. UTC | #19
Hi Bjorn, Arnd, all,

On 04/28/2016 05:18 PM, Bjorn Helgaas wrote:
> On Thu, Apr 28, 2016 at 10:40:35PM +0200, Arnd Bergmann wrote:
>> On Thursday 28 April 2016 15:14:39 Bjorn Helgaas wrote:
>>> On Thu, Apr 21, 2016 at 11:36:53AM +0200, Arnd Bergmann wrote:
>>>> On Thursday 21 April 2016 11:28:15 Tomasz Nowicki wrote:
>>>>> On 19.04.2016 15:06, Arnd Bergmann wrote:
>>>>>> On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
>>>>>>>
>>>>>>> Basically the whole content of pci-thunder-ecam.c and pci-thunder-pem.c.
>>>>>>>
>>>>>>> pci-thunder-ecam.c contains config space accessors. Similar for
>>>>>>> pci-thunder-pem.c but it also has extra init call (it is now called
>>>>>>> thunder_pem_init) which finds and maps related registers.
>>>>>>
>>>>>> They seem to do much more than just override the accessors, they actually
>>>>>> change the contents of the config space as well. Is that really necessary
>>>>>> on ACPI based systems as well?
>>>>>
>>>>> Yes, the pci-thunder-ecam.c accessors are meant to emulate config space 
>>>>> capabilities. They are necessary to synthesize EA capabilities (fixed 
>>>>> PCI BARs), it wont work without this, for ACPI boot as well.
>>>>
>>>> Why is that? I thought the BARs never get reassigned when using ACPI,

Just to specifically jump in here and clarify this piece, which only
pertains to the specific platform's special extra host driver (which
generally speaking I am encouraging all future platforms not to do). In
other words, the following has nothing to do with the rest of the patch
series and is entirely down to one specific SoC and its implementation.

ThunderX supports two different methods of PCIe configuration space for
on-chip devices: with EA and without EA (which is being phased out). EA
(Enhanced Allocation) is a fancy way of saying "read only BARs". Intel
did the spec change for that in PCI SIG, so it wasn't us folks in the
ARM community doing something weird. The good folks at Cavium desired a
means to express their on-SoC hardware using PCI so that it was nice and
enumerable, but without full boat PCI. EA fit the bill better than just
wiring BARs as write ignore or whatever. Again, it's happening in many
cases and there must be a reason Intel wanted to get it also.

I believe pci-thunder-ecam.c contains code to support the older devices
that don't do full EA by faking the EA capabilities, but they can
clarify. The point is, this is a specific and separate issue with the
way one vendor has chosen to implement on-SoC devices as PCIe
discoverable but using the newer PCI EA extension. And then the quirk is
to handle that not every device that's out there yet has real EA.

>>> In general, there's no reason we can't reassign BARs, whether we're
>>> using DT, ACPI, or whatever.  In many cases, systems with ACPI also
>>> assign all the BARs in firmware, and Linux doesn't reassign them
>>> unless it needs to.  But that's just a coincidence.  There's no
>>> requirement that Linux leave BARs as firmware programmed them.

There's no requirement, generally, that PCI compliant devices with ECAM
can't be programmed with different base addresses. There's this PCI
change called EA that is disjoint and some vendors have chosen to use
it. We didn't catch that early in the definition of the SBSA for ARM,
but just as an aside, I have already suggested we require future
generations of chips to not use EA and only support writeable BARs (even
for the decoders in the on-SoC platformish devices doing "PCI"). This
isn't Cavium's fault - they did the right thing with the data at hand
and nobody really considered the impact of PCI getting EA added. Again,
that's something that will likely happen on x86 at some point (maybe it
already is, I don't get any data about future Intel stuff).

On the rest of the quirks and hacks. Without going into too much detail,
some "concerned citizens" are chatting with various folks to ensure that
many of these common quirks aren't needed in future parts.

>> I'm thought I've seen systems in which the ACPI BIOS assumes that
>> certain PCI devices never move around, because it pokes the registers
>> from AML, and changing them would require never using the same device
>> through ACPI. It's likely that this is against some standard, but that
>> won't help you if you have to deal with the system anyway.

Right. This has happened, I think, and there you're no worse off on ARM
than you would be on x86 if you had AML poking at something underneath.

> Yes, I'm pretty sure there are systems like that, e.g., I think SMM
> code on some HP servers assumes the iLO address never changes.  I
> think that is a firmware defect because I don't think there's any spec
> that says firmware retains control over PCI BARs after handoff.  And
> this particular case isn't really ACPI-specific.

If you substitute SMM for EL3 on ARM we're bound to eventually have the
same kinds of things happening on some systems. It's just life.

> But as you say, we have to deal with these systems anyway, even if we
> consider that behavior broken.  My proposal has been to add quirks to
> mark those devices as IORESOURCE_PCI_FIXED, but I don't think anybody
> has gotten around to doing that.

Good to know.

Jon.
Lorenzo Pieralisi April 29, 2016, 9:41 a.m. UTC | #20
On Thu, Apr 28, 2016 at 05:47:15PM -0400, Jon Masters wrote:

[...]

> >>> In general, there's no reason we can't reassign BARs, whether we're
> >>> using DT, ACPI, or whatever.  In many cases, systems with ACPI also
> >>> assign all the BARs in firmware, and Linux doesn't reassign them
> >>> unless it needs to.  But that's just a coincidence.  There's no
> >>> requirement that Linux leave BARs as firmware programmed them.
> 
> There's no requirement, generally, that PCI compliant devices with ECAM
> can't be programmed with different base addresses. There's this PCI
> change called EA that is disjoint and some vendors have chosen to use
> it. We didn't catch that early in the definition of the SBSA for ARM,
> but just as an aside, I have already suggested we require future
> generations of chips to not use EA and only support writeable BARs (even
> for the decoders in the on-SoC platformish devices doing "PCI"). This
> isn't Cavium's fault - they did the right thing with the data at hand
> and nobody really considered the impact of PCI getting EA added. Again,
> that's something that will likely happen on x86 at some point (maybe it
> already is, I don't get any data about future Intel stuff).

PCI EA support in the kernel was implemented by Intel, for the records.

And I do not think anyone is questioning EA here (I mean implemented
through a real PCI capability, not config space quirks).

> On the rest of the quirks and hacks. Without going into too much detail,
> some "concerned citizens" are chatting with various folks to ensure that
> many of these common quirks aren't needed in future parts.
> 
> >> I'm thought I've seen systems in which the ACPI BIOS assumes that
> >> certain PCI devices never move around, because it pokes the registers
> >> from AML, and changing them would require never using the same device
> >> through ACPI. It's likely that this is against some standard, but that
> >> won't help you if you have to deal with the system anyway.
> 
> Right. This has happened, I think, and there you're no worse off on ARM
> than you would be on x86 if you had AML poking at something underneath.

Except that (if I read their code correctly - arch/x86/pci/i386.c,
see pcibios_resource_survey()) X86 claims the resources as
set-up by FW and thus does not reassign them, whereas on ARM we reassign
the whole PCI address space and we totally ignore the FW set-up (in DT
and ACPI alike), whether that's a problem or not time will tell,
as Bjorn mentioned I do not think that by the time FW hands over to
the OS there is any requirement whatsoever that prevents the OS
from reprogramming the PCI BARs set-up.

> > Yes, I'm pretty sure there are systems like that, e.g., I think SMM
> > code on some HP servers assumes the iLO address never changes.  I
> > think that is a firmware defect because I don't think there's any spec
> > that says firmware retains control over PCI BARs after handoff.  And
> > this particular case isn't really ACPI-specific.
> 
> If you substitute SMM for EL3 on ARM we're bound to eventually have the
> same kinds of things happening on some systems. It's just life.
> 
> > But as you say, we have to deal with these systems anyway, even if we
> > consider that behavior broken.  My proposal has been to add quirks to
> > mark those devices as IORESOURCE_PCI_FIXED, but I don't think anybody
> > has gotten around to doing that.

Well, that's going to be interesting. To me it is more something FW
should be able to communicate to the OS rather than a device specific
quirk, it is not that the device has fixed BARs, it is that the FW
expects them to be immutable (not saying that's the correct FW
behaviour - but it looks like a FW specific issue, not device specific).

I wonder whether this can be solved (at least in ACPI) through
a PCI BAR Target Operation Region (ACPI 6.0, 5.5.2.4.2), I will have
a look into that.

Lorenzo
diff mbox

Patch

diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
index 34c0aba..706621a 100644
--- a/drivers/pci/ecam.h
+++ b/drivers/pci/ecam.h
@@ -58,4 +58,9 @@  void __iomem *pci_generic_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
 /* default ECAM ops, bus shift 20, generic read and write */
 extern struct pci_generic_ecam_ops pci_generic_ecam_default_ops;
 
+#ifdef CONFIG_PCI_HOST_GENERIC
+/* for DT based pci controllers that support ECAM */
+int pci_host_common_probe(struct platform_device *pdev,
+			  struct pci_generic_ecam_ops *ops);
+#endif
 #endif
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 7a0780d..31d6eb5 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -82,6 +82,7 @@  config PCI_HOST_GENERIC
 	bool "Generic PCI host controller"
 	depends on (ARM || ARM64) && OF
 	select PCI_HOST_COMMON
+	select PCI_GENERIC_ECAM
 	help
 	  Say Y here if you want to support a simple generic PCI host
 	  controller, such as the one emulated by kvmtool.
diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
index e9f850f..99d99b3 100644
--- a/drivers/pci/host/pci-host-common.c
+++ b/drivers/pci/host/pci-host-common.c
@@ -22,27 +22,21 @@ 
 #include <linux/of_pci.h>
 #include <linux/platform_device.h>
 
-#include "pci-host-common.h"
+#include "../ecam.h"
 
-static void gen_pci_release_of_pci_ranges(struct gen_pci *pci)
-{
-	pci_free_resource_list(&pci->resources);
-}
-
-static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
+static int gen_pci_parse_request_of_pci_ranges(struct device *dev,
+		       struct list_head *resources, struct resource **bus_range)
 {
 	int err, res_valid = 0;
-	struct device *dev = pci->host.dev.parent;
 	struct device_node *np = dev->of_node;
 	resource_size_t iobase;
 	struct resource_entry *win;
 
-	err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pci->resources,
-					       &iobase);
+	err = of_pci_get_host_bridge_resources(np, 0, 0xff, resources, &iobase);
 	if (err)
 		return err;
 
-	resource_list_for_each_entry(win, &pci->resources) {
+	resource_list_for_each_entry(win, resources) {
 		struct resource *parent, *res = win->res;
 
 		switch (resource_type(res)) {
@@ -60,7 +54,7 @@  static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
 			res_valid |= !(res->flags & IORESOURCE_PREFETCH);
 			break;
 		case IORESOURCE_BUS:
-			pci->cfg.bus_range = res;
+			*bus_range = res;
 		default:
 			continue;
 		}
@@ -79,65 +73,67 @@  static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
 	return 0;
 
 out_release_res:
-	gen_pci_release_of_pci_ranges(pci);
 	return err;
 }
 
-static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
+static void gen_pci_generic_unmap_cfg(void *ptr)
+{
+	pci_generic_ecam_free((struct pci_config_window *)ptr);
+}
+
+static struct pci_config_window *gen_pci_init(struct device *dev,
+		struct list_head *resources, struct pci_generic_ecam_ops *ops)
 {
 	int err;
-	u8 bus_max;
-	resource_size_t busn;
-	struct resource *bus_range;
-	struct device *dev = pci->host.dev.parent;
-	struct device_node *np = dev->of_node;
-	u32 sz = 1 << pci->cfg.ops->bus_shift;
+	struct resource cfgres;
+	struct resource *bus_range = NULL;
+	struct pci_config_window *cfg;
+	unsigned int bus_shift = ops->bus_shift;
 
-	err = of_address_to_resource(np, 0, &pci->cfg.res);
+	/* Parse our PCI ranges and request their resources */
+	err = gen_pci_parse_request_of_pci_ranges(dev, resources, &bus_range);
+	if (err)
+		goto err_out;
+
+	err = of_address_to_resource(dev->of_node, 0, &cfgres);
 	if (err) {
 		dev_err(dev, "missing \"reg\" property\n");
-		return err;
+		goto err_out;
 	}
 
 	/* Limit the bus-range to fit within reg */
-	bus_max = pci->cfg.bus_range->start +
-		  (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1;
-	pci->cfg.bus_range->end = min_t(resource_size_t,
-					pci->cfg.bus_range->end, bus_max);
-
-	pci->cfg.win = devm_kcalloc(dev, resource_size(pci->cfg.bus_range),
-				    sizeof(*pci->cfg.win), GFP_KERNEL);
-	if (!pci->cfg.win)
-		return -ENOMEM;
-
-	/* Map our Configuration Space windows */
-	if (!devm_request_mem_region(dev, pci->cfg.res.start,
-				     resource_size(&pci->cfg.res),
-				     "Configuration Space"))
-		return -ENOMEM;
-
-	bus_range = pci->cfg.bus_range;
-	for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
-		u32 idx = busn - bus_range->start;
-
-		pci->cfg.win[idx] = devm_ioremap(dev,
-						 pci->cfg.res.start + idx * sz,
-						 sz);
-		if (!pci->cfg.win[idx])
-			return -ENOMEM;
+	bus_range->end = min(bus_range->end,
+		bus_range->start + (resource_size(&cfgres) >> bus_shift) - 1);
+
+	cfg = pci_generic_ecam_create(dev, cfgres.start, bus_range->start,
+				      bus_range->end, ops);
+	if (IS_ERR(cfg)) {
+		err = PTR_ERR(cfg);
+		goto err_out;
 	}
 
-	return 0;
+	err = devm_add_action(dev, gen_pci_generic_unmap_cfg, cfg);
+	if (err) {
+		gen_pci_generic_unmap_cfg(cfg);
+		goto err_out;
+	}
+	return cfg;
+
+err_out:
+	pci_free_resource_list(resources);
+	return ERR_PTR(err);
 }
 
 int pci_host_common_probe(struct platform_device *pdev,
-			  struct gen_pci *pci)
+			  struct pci_generic_ecam_ops *ops)
+
 {
-	int err;
 	const char *type;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct pci_bus *bus, *child;
+	struct pci_config_window *cfg;
+	struct list_head resources;
 
 	type = of_get_property(np, "device_type", NULL);
 	if (!type || strcmp(type, "pci")) {
@@ -147,29 +143,18 @@  int pci_host_common_probe(struct platform_device *pdev,
 
 	of_pci_check_probe_only();
 
-	pci->host.dev.parent = dev;
-	INIT_LIST_HEAD(&pci->host.windows);
-	INIT_LIST_HEAD(&pci->resources);
-
-	/* Parse our PCI ranges and request their resources */
-	err = gen_pci_parse_request_of_pci_ranges(pci);
-	if (err)
-		return err;
-
 	/* Parse and map our Configuration Space windows */
-	err = gen_pci_parse_map_cfg_windows(pci);
-	if (err) {
-		gen_pci_release_of_pci_ranges(pci);
-		return err;
-	}
+	INIT_LIST_HEAD(&resources);
+	cfg = gen_pci_init(dev, &resources, ops);
+	if (IS_ERR(cfg))
+		return PTR_ERR(cfg);
 
 	/* Do not reassign resources if probe only */
 	if (!pci_has_flag(PCI_PROBE_ONLY))
 		pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
 
-
-	bus = pci_scan_root_bus(dev, pci->cfg.bus_range->start,
-				&pci->cfg.ops->ops, pci, &pci->resources);
+	bus = pci_scan_root_bus(dev, cfg->bus_start, &ops->pci_ops, cfg,
+				&resources);
 	if (!bus) {
 		dev_err(dev, "Scanning rootbus failed");
 		return -ENODEV;
diff --git a/drivers/pci/host/pci-host-common.h b/drivers/pci/host/pci-host-common.h
deleted file mode 100644
index 09f3fa0..0000000
--- a/drivers/pci/host/pci-host-common.h
+++ /dev/null
@@ -1,47 +0,0 @@ 
-/*
- * 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) 2014 ARM Limited
- *
- * Author: Will Deacon <will.deacon@arm.com>
- */
-
-#ifndef _PCI_HOST_COMMON_H
-#define _PCI_HOST_COMMON_H
-
-#include <linux/kernel.h>
-#include <linux/platform_device.h>
-
-struct gen_pci_cfg_bus_ops {
-	u32 bus_shift;
-	struct pci_ops ops;
-};
-
-struct gen_pci_cfg_windows {
-	struct resource				res;
-	struct resource				*bus_range;
-	void __iomem				**win;
-
-	struct gen_pci_cfg_bus_ops		*ops;
-};
-
-struct gen_pci {
-	struct pci_host_bridge			host;
-	struct gen_pci_cfg_windows		cfg;
-	struct list_head			resources;
-};
-
-int pci_host_common_probe(struct platform_device *pdev,
-			  struct gen_pci *pci);
-
-#endif /* _PCI_HOST_COMMON_H */
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index e8aa78f..0150a62 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -25,41 +25,12 @@ 
 #include <linux/of_pci.h>
 #include <linux/platform_device.h>
 
-#include "pci-host-common.h"
+#include "../ecam.h"
 
-static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
-					     unsigned int devfn,
-					     int where)
-{
-	struct gen_pci *pci = bus->sysdata;
-	resource_size_t idx = bus->number - pci->cfg.bus_range->start;
-
-	return pci->cfg.win[idx] + ((devfn << 8) | where);
-}
-
-static struct gen_pci_cfg_bus_ops gen_pci_cfg_cam_bus_ops = {
+static struct pci_generic_ecam_ops gen_pci_cfg_cam_bus_ops = {
 	.bus_shift	= 16,
-	.ops		= {
-		.map_bus	= gen_pci_map_cfg_bus_cam,
-		.read		= pci_generic_config_read,
-		.write		= pci_generic_config_write,
-	}
-};
-
-static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
-					      unsigned int devfn,
-					      int where)
-{
-	struct gen_pci *pci = bus->sysdata;
-	resource_size_t idx = bus->number - pci->cfg.bus_range->start;
-
-	return pci->cfg.win[idx] + ((devfn << 12) | where);
-}
-
-static struct gen_pci_cfg_bus_ops gen_pci_cfg_ecam_bus_ops = {
-	.bus_shift	= 20,
-	.ops		= {
-		.map_bus	= gen_pci_map_cfg_bus_ecam,
+	.pci_ops	= {
+		.map_bus	= pci_generic_ecam_map_bus,
 		.read		= pci_generic_config_read,
 		.write		= pci_generic_config_write,
 	}
@@ -70,25 +41,22 @@  static const struct of_device_id gen_pci_of_match[] = {
 	  .data = &gen_pci_cfg_cam_bus_ops },
 
 	{ .compatible = "pci-host-ecam-generic",
-	  .data = &gen_pci_cfg_ecam_bus_ops },
+	  .data = &pci_generic_ecam_default_ops },
 
 	{ },
 };
+
 MODULE_DEVICE_TABLE(of, gen_pci_of_match);
 
 static int gen_pci_probe(struct platform_device *pdev)
 {
-	struct device *dev = &pdev->dev;
 	const struct of_device_id *of_id;
-	struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
-
-	if (!pci)
-		return -ENOMEM;
+	struct pci_generic_ecam_ops *ops;
 
-	of_id = of_match_node(gen_pci_of_match, dev->of_node);
-	pci->cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data;
+	of_id = of_match_node(gen_pci_of_match, pdev->dev.of_node);
+	ops = (struct pci_generic_ecam_ops *)of_id->data;
 
-	return pci_host_common_probe(pdev, pci);
+	return pci_host_common_probe(pdev, ops);
 }
 
 static struct platform_driver gen_pci_driver = {
diff --git a/drivers/pci/host/pci-thunder-ecam.c b/drivers/pci/host/pci-thunder-ecam.c
index d71935cb..f67c6d7 100644
--- a/drivers/pci/host/pci-thunder-ecam.c
+++ b/drivers/pci/host/pci-thunder-ecam.c
@@ -13,18 +13,7 @@ 
 #include <linux/of.h>
 #include <linux/platform_device.h>
 
-#include "pci-host-common.h"
-
-/* Mapping is standard ECAM */
-static void __iomem *thunder_ecam_map_bus(struct pci_bus *bus,
-					  unsigned int devfn,
-					  int where)
-{
-	struct gen_pci *pci = bus->sysdata;
-	resource_size_t idx = bus->number - pci->cfg.bus_range->start;
-
-	return pci->cfg.win[idx] + ((devfn << 12) | where);
-}
+#include "../ecam.h"
 
 static void set_val(u32 v, int where, int size, u32 *val)
 {
@@ -99,7 +88,7 @@  static int handle_ea_bar(u32 e0, int bar, struct pci_bus *bus,
 static int thunder_ecam_p2_config_read(struct pci_bus *bus, unsigned int devfn,
 				       int where, int size, u32 *val)
 {
-	struct gen_pci *pci = bus->sysdata;
+	struct pci_config_window *cfg = bus->sysdata;
 	int where_a = where & ~3;
 	void __iomem *addr;
 	u32 node_bits;
@@ -129,7 +118,7 @@  static int thunder_ecam_p2_config_read(struct pci_bus *bus, unsigned int devfn,
 	 * the config space access window.  Since we are working with
 	 * the high-order 32 bits, shift everything down by 32 bits.
 	 */
-	node_bits = (pci->cfg.res.start >> 32) & (1 << 12);
+	node_bits = (cfg->cfgaddr >> 32) & (1 << 12);
 
 	v |= node_bits;
 	set_val(v, where, size, val);
@@ -358,36 +347,24 @@  static int thunder_ecam_config_write(struct pci_bus *bus, unsigned int devfn,
 	return pci_generic_config_write(bus, devfn, where, size, val);
 }
 
-static struct gen_pci_cfg_bus_ops thunder_ecam_bus_ops = {
+static struct pci_generic_ecam_ops pci_thunder_ecam_ops = {
 	.bus_shift	= 20,
-	.ops		= {
-		.map_bus        = thunder_ecam_map_bus,
+	.pci_ops	= {
+		.map_bus        = pci_generic_ecam_map_bus,
 		.read           = thunder_ecam_config_read,
 		.write          = thunder_ecam_config_write,
 	}
 };
 
 static const struct of_device_id thunder_ecam_of_match[] = {
-	{ .compatible = "cavium,pci-host-thunder-ecam",
-	  .data = &thunder_ecam_bus_ops },
-
+	{ .compatible = "cavium,pci-host-thunder-ecam" },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, thunder_ecam_of_match);
 
 static int thunder_ecam_probe(struct platform_device *pdev)
 {
-	struct device *dev = &pdev->dev;
-	const struct of_device_id *of_id;
-	struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
-
-	if (!pci)
-		return -ENOMEM;
-
-	of_id = of_match_node(thunder_ecam_of_match, dev->of_node);
-	pci->cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data;
-
-	return pci_host_common_probe(pdev, pci);
+	return pci_host_common_probe(pdev, &pci_thunder_ecam_ops);
 }
 
 static struct platform_driver thunder_ecam_driver = {
diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
index cabb92a..91cfeb9 100644
--- a/drivers/pci/host/pci-thunder-pem.c
+++ b/drivers/pci/host/pci-thunder-pem.c
@@ -20,34 +20,22 @@ 
 #include <linux/of_pci.h>
 #include <linux/platform_device.h>
 
-#include "pci-host-common.h"
+#include "../ecam.h"
 
 #define PEM_CFG_WR 0x28
 #define PEM_CFG_RD 0x30
 
 struct thunder_pem_pci {
-	struct gen_pci	gen_pci;
 	u32		ea_entry[3];
 	void __iomem	*pem_reg_base;
 };
 
-static void __iomem *thunder_pem_map_bus(struct pci_bus *bus,
-					 unsigned int devfn, int where)
-{
-	struct gen_pci *pci = bus->sysdata;
-	resource_size_t idx = bus->number - pci->cfg.bus_range->start;
-
-	return pci->cfg.win[idx] + ((devfn << 16) | where);
-}
-
 static int thunder_pem_bridge_read(struct pci_bus *bus, unsigned int devfn,
 				   int where, int size, u32 *val)
 {
 	u64 read_val;
-	struct thunder_pem_pci *pem_pci;
-	struct gen_pci *pci = bus->sysdata;
-
-	pem_pci = container_of(pci, struct thunder_pem_pci, gen_pci);
+	struct pci_config_window *cfg = bus->sysdata;
+	struct thunder_pem_pci *pem_pci = (struct thunder_pem_pci *)cfg->priv;
 
 	if (devfn != 0 || where >= 2048) {
 		*val = ~0;
@@ -132,17 +120,17 @@  static int thunder_pem_bridge_read(struct pci_bus *bus, unsigned int devfn,
 static int thunder_pem_config_read(struct pci_bus *bus, unsigned int devfn,
 				   int where, int size, u32 *val)
 {
-	struct gen_pci *pci = bus->sysdata;
+	struct pci_config_window *cfg = bus->sysdata;
 
-	if (bus->number < pci->cfg.bus_range->start ||
-	    bus->number > pci->cfg.bus_range->end)
+	if (bus->number < cfg->bus_start ||
+	    bus->number > cfg->bus_end)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	/*
 	 * The first device on the bus is the PEM PCIe bridge.
 	 * Special case its config access.
 	 */
-	if (bus->number == pci->cfg.bus_range->start)
+	if (bus->number == cfg->bus_start)
 		return thunder_pem_bridge_read(bus, devfn, where, size, val);
 
 	return pci_generic_config_read(bus, devfn, where, size, val);
@@ -187,12 +175,11 @@  static u32 thunder_pem_bridge_w1c_bits(int where)
 static int thunder_pem_bridge_write(struct pci_bus *bus, unsigned int devfn,
 				    int where, int size, u32 val)
 {
-	struct gen_pci *pci = bus->sysdata;
-	struct thunder_pem_pci *pem_pci;
+	struct pci_config_window *cfg = bus->sysdata;
+	struct thunder_pem_pci *pem_pci = (struct thunder_pem_pci *)cfg->priv;
 	u64 write_val, read_val;
 	u32 mask = 0;
 
-	pem_pci = container_of(pci, struct thunder_pem_pci, gen_pci);
 
 	if (devfn != 0 || where >= 2048)
 		return PCIBIOS_DEVICE_NOT_FOUND;
@@ -256,53 +243,34 @@  static int thunder_pem_bridge_write(struct pci_bus *bus, unsigned int devfn,
 static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
 				    int where, int size, u32 val)
 {
-	struct gen_pci *pci = bus->sysdata;
+	struct pci_config_window *cfg = bus->sysdata;
 
-	if (bus->number < pci->cfg.bus_range->start ||
-	    bus->number > pci->cfg.bus_range->end)
+	if (bus->number < cfg->bus_start ||
+	    bus->number > cfg->bus_end)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	/*
 	 * The first device on the bus is the PEM PCIe bridge.
 	 * Special case its config access.
 	 */
-	if (bus->number == pci->cfg.bus_range->start)
+	if (bus->number == cfg->bus_start)
 		return thunder_pem_bridge_write(bus, devfn, where, size, val);
 
 
 	return pci_generic_config_write(bus, devfn, where, size, val);
 }
 
-static struct gen_pci_cfg_bus_ops thunder_pem_bus_ops = {
-	.bus_shift	= 24,
-	.ops		= {
-		.map_bus	= thunder_pem_map_bus,
-		.read		= thunder_pem_config_read,
-		.write		= thunder_pem_config_write,
-	}
-};
-
-static const struct of_device_id thunder_pem_of_match[] = {
-	{ .compatible = "cavium,pci-host-thunder-pem",
-	  .data = &thunder_pem_bus_ops },
-
-	{ },
-};
-MODULE_DEVICE_TABLE(of, thunder_pem_of_match);
-
-static int thunder_pem_probe(struct platform_device *pdev)
+static int thunder_pem_init(struct device *dev, struct pci_config_window *cfg)
 {
-	struct device *dev = &pdev->dev;
-	const struct of_device_id *of_id;
 	resource_size_t bar4_start;
 	struct resource *res_pem;
 	struct thunder_pem_pci *pem_pci;
+	struct platform_device *pdev;
 
 	pem_pci = devm_kzalloc(dev, sizeof(*pem_pci), GFP_KERNEL);
 	if (!pem_pci)
 		return -ENOMEM;
 
-	of_id = of_match_node(thunder_pem_of_match, dev->of_node);
-	pem_pci->gen_pci.cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data;
+	pdev = to_platform_device(dev);
 
 	/*
 	 * The second register range is the PEM bridge to the PCIe
@@ -330,7 +298,29 @@  static int thunder_pem_probe(struct platform_device *pdev)
 	pem_pci->ea_entry[1] = (u32)(res_pem->end - bar4_start) & ~3u;
 	pem_pci->ea_entry[2] = (u32)(bar4_start >> 32);
 
-	return pci_host_common_probe(pdev, &pem_pci->gen_pci);
+	cfg->priv = pem_pci;
+	return 0;
+}
+
+static struct pci_generic_ecam_ops pci_thunder_pem_ops = {
+	.bus_shift	= 24,
+	.init		= thunder_pem_init,
+	.pci_ops	= {
+		.map_bus	= pci_generic_ecam_map_bus,
+		.read		= thunder_pem_config_read,
+		.write		= thunder_pem_config_write,
+	}
+};
+
+static const struct of_device_id thunder_pem_of_match[] = {
+	{ .compatible = "cavium,pci-host-thunder-pem" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, thunder_pem_of_match);
+
+static int thunder_pem_probe(struct platform_device *pdev)
+{
+	return pci_host_common_probe(pdev, &pci_thunder_pem_ops);
 }
 
 static struct platform_driver thunder_pem_driver = {