[2/3] PCI: ARM: add support for virtual PCI host controller

On Tue, Feb 11, 2014 at 11:42:52AM +0100, Arnd Bergmann wrote:
> I looked briefly at the code and found that mach-kirkwood/pcie.c does
> both request_resource() and pci_add_resource_offset(), while
> drivers/pci/host/pci-mvebu.c only does the latter. Does the patch
> below restore the previous behavior?

It gets closer:

e0000000-f0000000 : <BAD>
  e0000000-e00fffff : PCI Bus 0000:01
    e0000000-e001ffff : 0000:01:00.0
      e0001000-e0001fff : /mbus/pex@e0000000/pcie@1,0/fpga@0/fpga_sysmon@1000

This patch:

specific structure ..

Jason
--
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

On Wednesday 12 February 2014 12:43:13 Jason Gunthorpe wrote:
> On Tue, Feb 11, 2014 at 11:42:52AM +0100, Arnd Bergmann wrote:
> 
> 
> Still missing release_region..
> 
> Thoughts on upstreamining these bits?

Upstreaming them would be great. Patches look correct by inspection as well.

> > Since the mvebu_pcie_setup() function seems very generic at this,
> > we should probably try to factor out that code into a common
> > helper, at least for arm64, but ideally shared with arm32
> > as well.
> 
> Yah, especially since people are not getting it completely right..
> 
> But some of the trouble here is a lack of a generic pci host driver
> structure, eg I have to pull the domain number out of the ARM32
> specific structure ..

I'm sure that Bjorn also has some plans of his own, but I think
we should have a generic host driver structure and gradually move
stuff into it from the architecture specific structs.

Now there is a 'struct pci_host_bridge' that is used on some
architectures already but not on ARM. It also contains a list
of resource windows plus offsets, and there is a set of functions
associated with it:

pci_create_root_bus()
pcibios_root_bridge_prepare()

etc.

Maybe all that's needed is to actually start using those on arm32?

	Arnd
--
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

On Wed, Feb 12, 2014 at 1:07 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 12 February 2014 12:43:13 Jason Gunthorpe wrote:
>> On Tue, Feb 11, 2014 at 11:42:52AM +0100, Arnd Bergmann wrote:
>>
>>
>> Still missing release_region..
>>
>> Thoughts on upstreamining these bits?
>
> Upstreaming them would be great. Patches look correct by inspection as well.
>
>> > Since the mvebu_pcie_setup() function seems very generic at this,
>> > we should probably try to factor out that code into a common
>> > helper, at least for arm64, but ideally shared with arm32
>> > as well.
>>
>> Yah, especially since people are not getting it completely right..
>>
>> But some of the trouble here is a lack of a generic pci host driver
>> structure, eg I have to pull the domain number out of the ARM32
>> specific structure ..
>
> I'm sure that Bjorn also has some plans of his own, but I think
> we should have a generic host driver structure and gradually move
> stuff into it from the architecture specific structs.

I don't have plans in this area right now, but I would definitely like
to migrate things like the domain, NUMA node, etc., into a generic
structure.

> Now there is a 'struct pci_host_bridge' that is used on some
> architectures already but not on ARM. It also contains a list
> of resource windows plus offsets, and there is a set of functions
> associated with it:
>
> pci_create_root_bus()
> pcibios_root_bridge_prepare()
>
> etc.
>
> Maybe all that's needed is to actually start using those on arm32?
>
>         Arnd
> --
> 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
--
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

Dear Jason Gunthorpe,

On Wed, 12 Feb 2014 12:43:13 -0700, Jason Gunthorpe wrote:

> diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
> index 2394e97..7fd54e9 100644
> --- a/drivers/bus/mvebu-mbus.c
> +++ b/drivers/bus/mvebu-mbus.c
> @@ -876,14 +876,14 @@ static void __init mvebu_mbus_get_pcie_resources(struct device_node *np,
>         ret = of_property_read_u32_array(np, "pcie-mem-aperture", reg, ARRAY_SIZE(reg));
>         if (!ret) {
>                 mem->start = reg[0];
> -               mem->end = mem->start + reg[1];
> +               mem->end = mem->start + reg[1] - 1;
>                 mem->flags = IORESOURCE_MEM;
>         }
>  
>         ret = of_property_read_u32_array(np, "pcie-io-aperture", reg, ARRAY_SIZE(reg));
>         if (!ret) {
>                 io->start = reg[0];
> -               io->end = io->start + reg[1];
> +               io->end = io->start + reg[1] - 1;
>                 io->flags = IORESOURCE_IO;
>         }
>  }

This certainly looks good.

> Fixes the wrong length (e0000000-f0000000 should be e0000000-efffffff)
> 
> And this fixes the <BAD>:
> 
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index ef8691a..fbb89cb 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -109,7 +109,9 @@ struct mvebu_pcie {
>         struct mvebu_pcie_port *ports;
>         struct msi_chip *msi;
>         struct resource io;
> +       char io_name[30];
>         struct resource realio;
> +       char mem_name[30];
>         struct resource mem;
>         struct resource busn;
>         int nports;
> @@ -681,10 +683,29 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
>  {
>         struct mvebu_pcie *pcie = sys_to_pcie(sys);
>         int i;
> +       int domain = 0;
>  
> +#ifdef CONFIG_PCI_DOMAINS
> +       domain = sys->domain;
> +#endif
> +
> +       snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x", domain);
> +       pcie->mem.name = pcie->mem_name;
> +
> +       snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", domain);
> +       pcie->realio.name = pcie->io_name;

I honestly don't know what PCI domains are, but this change looks
fairly harmless to me. I would however use kasprintf() instead maybe. I
can submit patches for those two changes if you want.

> Still missing release_region..

To match which request_region?

> > Since the mvebu_pcie_setup() function seems very generic at this,
> > we should probably try to factor out that code into a common
> > helper, at least for arm64, but ideally shared with arm32
> > as well.
> 
> Yah, especially since people are not getting it completely right..
> 
> But some of the trouble here is a lack of a generic pci host driver
> structure, eg I have to pull the domain number out of the ARM32
> specific structure ..

There is indeed a good deal of duplication of PCI related code in the
different architectures. The PCI_DOMAINS Kconfig option is for example
replicated in multiple architectures.

Best regards,

Thomas