Re: [PATCH v1 19/25] PCI: rcar Gen2: Request host bridge window resources

[Geert Uytterhoeven]

Hi Bjorn,

On Tue, Jun 21, 2016 at 6:49 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> On Tue, Jun 21, 2016 at 06:41:00PM +0300, Valentine Barshak wrote:
>> On Tue, Jun 21, 2016 at 09:26:23AM -0500, Bjorn Helgaas wrote:
>> > On Tue, Jun 21, 2016 at 12:41:31PM +0200, Geert Uytterhoeven wrote:
>> > > On Tue, Jun 7, 2016 at 1:07 AM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
>> > > > Request host bridge window resources so they appear in ioport_resource and
>> > > > iomem_resource and are reflected in /proc/ioports and /proc/iomem.
>> > > >
>> > > > Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>> > > > ---
>> > > >  drivers/pci/host/pci-rcar-gen2.c |    4 ++++
>> > > >  1 file changed, 4 insertions(+)
>> > > >
>> > > > diff --git a/drivers/pci/host/pci-rcar-gen2.c b/drivers/pci/host/pci-rcar-gen2.c
>> > > > index 9980a4b..617a6b2 100644
>> > > > --- a/drivers/pci/host/pci-rcar-gen2.c
>> > > > +++ b/drivers/pci/host/pci-rcar-gen2.c
>> > > > @@ -194,6 +194,7 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys)
>> > > >         struct rcar_pci_priv *priv = sys->private_data;
>> > > >         void __iomem *reg = priv->reg;
>> > > >         u32 val;
>> > > > +       int ret;
>> > > >
>> > > >         pm_runtime_enable(priv->dev);
>> > > >         pm_runtime_get_sync(priv->dev);
>> > > > @@ -275,6 +276,9 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys)
>> > > >         /* Add PCI resources */
>> > > >         pci_add_resource(&sys->resources, &priv->io_res);
>> > > >         pci_add_resource(&sys->resources, &priv->mem_res);
>> > > > +       ret = devm_request_pci_bus_resources(priv->dev, &sys->resources);
>> > > > +       if (ret < 0)
>> > > > +               return ret;
>> > > >
>> > > >         /* Setup bus number based on platform device id / of bus-range */
>> > > >         sys->busnr = priv->busnr;
>> > >
>> > > This patch (commit 1bd019707b7c9249d34c5d348f1ef75eb4d83e89 in pci/next)
>> > > broke PCI on r8a7791/koelsch. Dmesg differences are:
>> > >
>> > >  pci-rcar-gen2 ee090000.pci: PCI: bus0 revision 11
>> > > -pci-rcar-gen2 ee090000.pci: PCI host bridge to bus 0000:00
>> > > -pci_bus 0000:00: root bus resource [io  0xee080000-0xee0810ff]
>> > > -pci_bus 0000:00: root bus resource [mem 0xee080000-0xee0810ff]
>> >
>> > This is probably a result of this code in drivers/pci/host/pci-rcar-gen2.c:
>> >
>> >         /*
>> >          * The controller does not support/use port I/O,
>> >          * so setup a dummy port I/O region here.
>> >          */
>> >         priv->io_res.start = priv->mem_res.start;
>> >         priv->io_res.end = priv->mem_res.end;
>> >         priv->io_res.flags = IORESOURCE_IO;
>> >
>> > We try to avoid adding dummy regions like this, but maybe we missed
>> > this one.  I haven't found any email discussion about it yet, so I
>> > don't know what the reason for this one is.  Valentine, do you
>> > remember?
>>
>> I do not, but I think I've found something in my mailbox.
>> Below is the quote from the original conversation with:
>> (Re: [PATCH] pci: Add R-Car Gen2 internal PCI support)
>>
>> [quote starts]
>>  >>+    priv->mem_res = *mem_res;
>>  >>+    /*
>>  >>+     * The controller does not support/use port I/O,
>>  >>+     * so setup a dummy port I/O region here.
>>  >>+     */
>>  >>+    priv->io_res.start = priv->mem_res.start;
>>  >>+    priv->io_res.end = priv->mem_res.end;
>>  >>+    priv->io_res.flags = IORESOURCE_IO;
>>  >>
>>  > I don't understand this.  There's no requirement (at least as far as the
>>  > PCI core is concerned) to supply an I/O aperture at all, and I think it
>>  > would be better if you didn't.
>>  >
>>  > Oh, I see ... maybe pcibios_init_resources() forces you to have an
>>  > I/O resource to avoid having it give you a default one?  And I
>>  > suppose that since you have several host bridges, these dummy I/O
>>  > regions have to be distinct.  Ugh.  Well, I guess this is something
>>  > you'd have to fix here or in the ARM code, it's up to you what to do.
>>
>>  Exactly. This is to avoid assigning default I/O resources.
>> [quote ends]
>
> Oh, right, now I remember.  Thanks for digging that out.
>
> I propose the patches below to remove the requirement for having an
> I/O space.  Any chance one of you could test them?

Thanks!
PCI seems to be working again on r8a7791/koelsch using today's pci/next,
which includes these patches. Lspci shows the USB controllers again.

/proc/iomem gained a few entries:

 ec700000-ec70ffff : /dma-controller@ec700000
 ec720000-ec72ffff : /dma-controller@ec720000
 ec740000-ec7401ff : audmapp
+ee080000-ee0810ff : /pci@ee090000
+  ee080000-ee080fff : 0000:00:01.0
+  ee081000-ee0810ff : 0000:00:02.0
 ee090000-ee090bff : /pci@ee090000
+ee0c0000-ee0c10ff : /pci@ee0d0000
+  ee0c0000-ee0c0fff : 0001:01:01.0
+  ee0c1000-ee0c10ff : 0001:01:02.0
 ee0d0000-ee0d0bff : /pci@ee0d0000
 ee300000-ee301fff : /sata@ee300000
 ee700000-ee7003ff : /ethernet@ee700000

Hence
Tested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds