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

[Bjorn Helgaas <helgaas-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

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:
> > [+cc Valentine]
> > 
> 
> Hi Bjorn,
> 
> > Hi Geert,
> > 
> > Thanks a lot for testing this, and sorry for the breakage.
> > 
> > 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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > > > ---
> > > >  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?


commit 8387e687f72747b994c54e29e31861f0db07eb0c
Author: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Date:   Tue Jun 21 10:54:29 2016 -0500

    ARM: Make PCI I/O space optional
    
    For callers of pci_common_init_dev(), we previously always required a PCI
    I/O port resource.  If the caller's ->setup() function had added an I/O
    resource, we used that; otherwise, we added a default 64K I/O port space
    for it.
    
    There are PCI host bridges that do not support I/O port space, and we
    should not add fictitious spaces for them.
    
    If a caller sets struct hw_pci.io_optional, assume it is responsible for
    adding any I/O port resource it desires, and do not add any default I/O
    port space.
    
    Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
index 0070e85..2d88af5 100644
--- a/arch/arm/include/asm/mach/pci.h
+++ b/arch/arm/include/asm/mach/pci.h
@@ -22,6 +22,7 @@ struct hw_pci {
 	struct msi_controller *msi_ctrl;
 	struct pci_ops	*ops;
 	int		nr_controllers;
+	unsigned int	io_optional:1;
 	void		**private_data;
 	int		(*setup)(int nr, struct pci_sys_data *);
 	struct pci_bus *(*scan)(int nr, struct pci_sys_data *);
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 05e61a2..65f12ef 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -410,7 +410,8 @@ static int pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 	return irq;
 }
 
-static int pcibios_init_resources(int busnr, struct pci_sys_data *sys)
+static int pcibios_init_resource(int busnr, struct pci_sys_data *sys,
+				 int io_optional)
 {
 	int ret;
 	struct resource_entry *window;
@@ -420,6 +421,14 @@ static int pcibios_init_resources(int busnr, struct pci_sys_data *sys)
 			 &iomem_resource, sys->mem_offset);
 	}
 
+	/*
+	 * If a platform says I/O port support is optional, we don't add
+	 * the default I/O space.  The platform is responsible for adding
+	 * any I/O space it needs.
+	 */
+	if (io_optional)
+		return 0;
+
 	resource_list_for_each_entry(window, &sys->resources)
 		if (resource_type(window->res) == IORESOURCE_IO)
 			return 0;
@@ -466,7 +475,7 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 		if (ret > 0) {
 			struct pci_host_bridge *host_bridge;
 
-			ret = pcibios_init_resources(nr, sys);
+			ret = pcibios_init_resource(nr, sys, hw->io_optional);
 			if (ret)  {
 				kfree(sys);
 				break;

commit eeb4d6cdf8960c484c5a6eb9b310145b75a59ec1
Author: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Date:   Tue Jun 21 09:19:34 2016 -0500

    PCI: rcar: Drop gen2 dummy I/O port region
    
    Previously we added a dummy I/O port region even though the R-Car
    controller doesn't support PCI port I/O.  This resulted in bogus root bus
    resources like this:
    
      pci_bus 0000:00: root bus resource [io  0xee080000-0xee0810ff]
      pci_bus 0000:00: root bus resource [mem 0xee080000-0xee0810ff]
    
    Drop the unused dummy I/O port region and set struct hw_pci.io_optional so
    the ARM PCI code doesn't add a default one for us.
    
    Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/pci/host/pci-rcar-gen2.c b/drivers/pci/host/pci-rcar-gen2.c
index 9980a4b..ddf7765 100644
--- a/drivers/pci/host/pci-rcar-gen2.c
+++ b/drivers/pci/host/pci-rcar-gen2.c
@@ -97,7 +97,6 @@
 struct rcar_pci_priv {
 	struct device *dev;
 	void __iomem *reg;
-	struct resource io_res;
 	struct resource mem_res;
 	struct resource *cfg_res;
 	unsigned busnr;
@@ -273,7 +272,6 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys)
 		rcar_pci_setup_errirq(priv);
 
 	/* Add PCI resources */
-	pci_add_resource(&sys->resources, &priv->io_res);
 	pci_add_resource(&sys->resources, &priv->mem_res);
 
 	/* Setup bus number based on platform device id / of bus-range */
@@ -371,14 +369,6 @@ static int rcar_pci_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	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;
-
 	priv->cfg_res = cfg_res;
 
 	priv->irq = platform_get_irq(pdev, 0);
@@ -421,6 +411,7 @@ static int rcar_pci_probe(struct platform_device *pdev)
 	hw_private[0] = priv;
 	memset(&hw, 0, sizeof(hw));
 	hw.nr_controllers = ARRAY_SIZE(hw_private);
+	hw.io_optional = 1;
 	hw.private_data = hw_private;
 	hw.map_irq = rcar_pci_map_irq;
 	hw.ops = &rcar_pci_ops;