Re: [03/12,v3] pci: fsl: add PCI indirect access support

[Lian Minghuan-b31939 <b31939@freescale.com>]

HI Scott,

please see my comments inline.

On 01/04/2014 06:33 AM, Scott Wood wrote:
> On Wed, Oct 23, 2013 at 06:41:25PM +0800, Minghuan Lian wrote:
>> The patch adds PCI indirect read/write functions. The main code
>> is ported from arch/powerpc/sysdev/indirect_pci.c. We use general
>> IO API iowrite32be/ioread32be instead of out_be32/in_be32, and
>> use structure fsl_Pci instead of PowerPC's pci_controller.
>> The patch also provides fsl_pcie_check_link() to check PCI link.
>> The weak function fsl_arch_pci_exclude_device() is provided to
>> call ppc_md.pci_exclude_device() for PowerPC architecture.
>>
>> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
>>
>> ---
>> change log:
>> v1-v3:
>> Derived from http://patchwork.ozlabs.org/patch/278965/
>>
>> Based on upstream master.
>> Based on the discussion of RFC version here
>> http://patchwork.ozlabs.org/patch/274487/
>>
>>   drivers/pci/host/pci-fsl-common.c | 169 ++++++++++++++++++++++++++++++++------
>>   include/linux/fsl/pci-common.h    |   6 ++
>>   2 files changed, 151 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/pci/host/pci-fsl-common.c b/drivers/pci/host/pci-fsl-common.c
>> index 69d338b..8bc9a64 100644
>> --- a/drivers/pci/host/pci-fsl-common.c
>> +++ b/drivers/pci/host/pci-fsl-common.c
>> @@ -35,52 +35,173 @@
>>   #include <sysdev/fsl_soc.h>
>>   #include <sysdev/fsl_pci.h>
>>   
>> -static int fsl_pcie_check_link(struct pci_controller *hose)
>> +/* Indirect type */
>> +#define INDIRECT_TYPE_EXT_REG			0x00000002
>> +#define INDIRECT_TYPE_SURPRESS_PRIMARY_BUS	0x00000004
>> +#define INDIRECT_TYPE_NO_PCIE_LINK		0x00000008
>> +#define INDIRECT_TYPE_BIG_ENDIAN		0x00000010
>> +#define INDIRECT_TYPE_FSL_CFG_REG_LINK		0x00000040
> Why are these here rather than in the header, given that you have
> indirect_type in the struct in the header?
[Minghuan] It's better to define the type in the header file. I will fix it.
>
>> +int __weak fsl_arch_pci_exclude_device(struct fsl_pci *pci, u8 bus, u8 devfn)
>> +{
>> +	return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>> +static int fsl_pci_read_config(struct fsl_pci *pci, int bus, int devfn,
>> +				int offset, int len, u32 *val)
>> +{
>> +	u32 bus_no, reg, data;
>> +
>> +	if (pci->indirect_type & INDIRECT_TYPE_NO_PCIE_LINK) {
>> +		if (bus != pci->first_busno)
>> +			return PCIBIOS_DEVICE_NOT_FOUND;
>> +		if (devfn != 0)
>> +			return PCIBIOS_DEVICE_NOT_FOUND;
>> +	}
> A lot of this seems duplicated from arch/powerpc/sysdev/indirect_pci.c.
>
> How generally applicable is that file to non-PPC implementations?  At a
> minimum I see a similar file in arch/microblaze.  It should probably
> eventually be moved to common code, rather than duplicated again.  A
> prerequisite for that would be making common the dependencies it has on
> the rest of what is currently arch PCI infrastructure; until then, it's
> probably better to just have the common fsl-pci code know how to
> interface with the appropriate PPC/ARM code rather than trying to copy
> the infrastructure as well.
[Minghuan] Yes, This is a duplicate except it uses struct fsl_pci. But 
it is hard to be move to common code.
because every indirect read/write functions use different PCI controller 
structure which is very basic structure and ARM has no this structure.
If we can not establish a unified pci controller structure, we can only 
abstract out a simple structure which includes indirect access related 
fields,
and need a callback function to get the pointer like this: 
((powerpc/microblaze/mips/ pci_controller 
*)(pci_bus->sysdata))->indirect_struct.
Should we provide the common code for indirect access API or wait for 
the common PCI controller structure?
> -Scott