[PATCH 05/32] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[PATCH 05/32] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Thomas Petazzoni]

The pcim_*() functions are used by the libata-sff subsystem, and this
subsystem is used for many SATA drivers on ARM platforms that do not
necessarily have I/O ports.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: linux-kernel at vger.kernel.org
---
 lib/devres.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/devres.c b/lib/devres.c
index 80b9c76..5639c3e 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -195,6 +195,7 @@ void devm_ioport_unmap(struct device *dev, void __iomem *addr)
 			       devm_ioport_map_match, (void *)addr));
 }
 EXPORT_SYMBOL(devm_ioport_unmap);
+#endif /* CONFIG_HAS_IOPORT */
 
 #ifdef CONFIG_PCI
 /*
@@ -400,4 +401,3 @@ void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
 }
 EXPORT_SYMBOL(pcim_iounmap_regions);
 #endif /* CONFIG_PCI */
-#endif /* CONFIG_HAS_IOPORT */
-- 
1.7.9.5

[PATCH 05/32] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Arnd Bergmann]

On Tuesday 12 February 2013, Thomas Petazzoni wrote:
> The pcim_*() functions are used by the libata-sff subsystem, and this
> subsystem is used for many SATA drivers on ARM platforms that do not
> necessarily have I/O ports.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: linux-kernel at vger.kernel.org

Sorry, but this patch is still incorrect. Any driver that requires a linear
mapping of I/O ports to __iomem pointers must depend CONFIG_HAS_IOPORT
with the current definition of that symbol (as mentioned before, we
should really rename that to CONFIG_HAS_IOPORT_MAP). Having these
functions not defined is a compile time check that is necessary to
ensure that all drivers have the correct annotation.

If a platform has no support for I/O ports at all, it should
probably not set CONFIG_NO_IOPORT at this point.

	Arnd

[PATCH 05/32] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 12 Feb 2013 18:00:48 +0000, Arnd Bergmann wrote:
> On Tuesday 12 February 2013, Thomas Petazzoni wrote:
> > The pcim_*() functions are used by the libata-sff subsystem, and
> > this subsystem is used for many SATA drivers on ARM platforms that
> > do not necessarily have I/O ports.
> > 
> > Signed-off-by: Thomas Petazzoni
> > <thomas.petazzoni@free-electrons.com> Cc: Paul Gortmaker
> > <paul.gortmaker@windriver.com> Cc: Jesse Barnes
> > <jbarnes@virtuousgeek.org> Cc: Yinghai Lu <yinghai@kernel.org>
> > Cc: linux-kernel at vger.kernel.org
> 
> Sorry, but this patch is still incorrect.

I know, but the discussion was so huge on the first posting that it was
basically impossible to draw a conclusion out of it.

> Any driver that requires a
> linear mapping of I/O ports to __iomem pointers must depend
> CONFIG_HAS_IOPORT with the current definition of that symbol (as
> mentioned before, we should really rename that to
> CONFIG_HAS_IOPORT_MAP). Having these functions not defined is a
> compile time check that is necessary to ensure that all drivers have
> the correct annotation.

I have the feeling that the problem is more complex than that. My
understanding is that the pcim_iomap_regions() function used by
drivers/ata/libata-sff.c can perfectly be used to map memory BARs, and
not necessarily I/O BARs. Therefore, this driver can perfectly be used
in an architecture where CONFIG_NO_IOPORT is selected.

The thing is that pcim_iomap_regions() transparently allows to remap an
I/O BAR is such a BAR is passed as argument, or a memory BAR if such a
BAR is passed as argument.

Therefore, I continue to believe that the pcim_*() functions are useful
even if the platform doesn't have CONFIG_HAS_IOPORT.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 05/32] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Arnd Bergmann]

On Tuesday 12 February 2013, Thomas Petazzoni wrote:
> > Any driver that requires a
> > linear mapping of I/O ports to __iomem pointers must depend
> > CONFIG_HAS_IOPORT with the current definition of that symbol (as
> > mentioned before, we should really rename that to
> > CONFIG_HAS_IOPORT_MAP). Having these functions not defined is a
> > compile time check that is necessary to ensure that all drivers have
> > the correct annotation.
> 
> I have the feeling that the problem is more complex than that. My
> understanding is that the pcim_iomap_regions() function used by
> drivers/ata/libata-sff.c can perfectly be used to map memory BARs, and
> not necessarily I/O BARs. Therefore, this driver can perfectly be used
> in an architecture where CONFIG_NO_IOPORT is selected.

That is correct.

> The thing is that pcim_iomap_regions() transparently allows to remap an
> I/O BAR is such a BAR is passed as argument, or a memory BAR if such a
> BAR is passed as argument.
> 
> Therefore, I continue to believe that the pcim_*() functions are useful
> even if the platform doesn't have CONFIG_HAS_IOPORT.

Yes, the pcim_ functions are useful in principle, but it falls back
to the __pci_ioport_map() for IORESOURCE_IO, and that needs to
return an error if CONFIG_HAS_IOPORT is not set.
I think it would be correct if you add this hunk:

diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c
index 0d83ea8..f9b6387 100644
--- a/lib/pci_iomap.c
+++ b/lib/pci_iomap.c
@@ -33,7 +33,7 @@ void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
                return NULL;
        if (maxlen && len > maxlen)
                len = maxlen;
-       if (flags & IORESOURCE_IO)
+       if (IS_ENABLED(CONFIG_HAS_IOPORT) && (flags & IORESOURCE_IO))
                return __pci_ioport_map(dev, start, len);
        if (flags & IORESOURCE_MEM) {
                if (flags & IORESOURCE_CACHEABLE)

in order to prevent a link error when CONFIG_HAS_IOPORT is unset.

	Arnd

[PATCH 05/32] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 12 Feb 2013 22:36:37 +0000, Arnd Bergmann wrote:

> > I have the feeling that the problem is more complex than that. My
> > understanding is that the pcim_iomap_regions() function used by
> > drivers/ata/libata-sff.c can perfectly be used to map memory BARs, and
> > not necessarily I/O BARs. Therefore, this driver can perfectly be used
> > in an architecture where CONFIG_NO_IOPORT is selected.
> 
> That is correct.
> 
> > The thing is that pcim_iomap_regions() transparently allows to remap an
> > I/O BAR is such a BAR is passed as argument, or a memory BAR if such a
> > BAR is passed as argument.
> > 
> > Therefore, I continue to believe that the pcim_*() functions are useful
> > even if the platform doesn't have CONFIG_HAS_IOPORT.
> 
> Yes, the pcim_ functions are useful in principle, but it falls back
> to the __pci_ioport_map() for IORESOURCE_IO, and that needs to
> return an error if CONFIG_HAS_IOPORT is not set.
> I think it would be correct if you add this hunk:
> 
> diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c
> index 0d83ea8..f9b6387 100644
> --- a/lib/pci_iomap.c
> +++ b/lib/pci_iomap.c
> @@ -33,7 +33,7 @@ void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
>                 return NULL;
>         if (maxlen && len > maxlen)
>                 len = maxlen;
> -       if (flags & IORESOURCE_IO)
> +       if (IS_ENABLED(CONFIG_HAS_IOPORT) && (flags & IORESOURCE_IO))
>                 return __pci_ioport_map(dev, start, len);
>         if (flags & IORESOURCE_MEM) {
>                 if (flags & IORESOURCE_CACHEABLE)
> 
> in order to prevent a link error when CONFIG_HAS_IOPORT is unset.

FWIW, a patch that is doing what I was initially proposing has been
merged for 3.9, and it doesn't contain the
IS_ENABLED(CONFIG_HAS_IOPORT) test you were proposing (and which I
think was correct). See:

commit 9ed8a30f3471347c1b763bd062fa78ae80f18eae
Author: Jingoo Han <jg1.han@samsung.com>
Date:   Wed Feb 27 17:02:42 2013 -0800

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 05/32] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Arnd Bergmann]

On Monday 04 March 2013, Thomas Petazzoni wrote:
> FWIW, a patch that is doing what I was initially proposing has been
> merged for 3.9, and it doesn't contain the
> IS_ENABLED(CONFIG_HAS_IOPORT) test you were proposing (and which I
> think was correct). See:
> 
> commit 9ed8a30f3471347c1b763bd062fa78ae80f18eae
> Author: Jingoo Han <jg1.han@samsung.com>
> Date:   Wed Feb 27 17:02:42 2013 -0800
> 

Sigh.

I'll take it as an additional incentive to finally clean up the logic behind
CONFIG_HAS_IOPORT by introducing a CONFIG_HAS_IOPORT_MAP symbol to replace it.

Thanks for the heads up.

	Arnd

[PATCH 05/32] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Thomas Petazzoni]

Dear Jingoo Han,

On Wed, 06 Mar 2013 06:28:08 +0000 (GMT), Jingoo Han wrote:

> Sorry, I did not know that you submitted the patch.

No problem, I'm happy to have one less patch to carry in my PCIe patch
set :)

> Like you, I am developing PCIe Host driver.

Just curious, do you already have some code? Thierry Reding and myself
have been looking at each other's PCIe host driver since a while in
order to make some consistent choices where possible. It would be good
to see your code as well.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 05/32] lib: devres: don't enclose pcim_*() functions in CONFIG_HAS_IOPORT

[Jason Gunthorpe]

On Wed, Mar 06, 2013 at 08:40:26AM +0000, Jingoo Han wrote:

> Yes, I am developing a PCIe driver for Samsung Exynos SoCs.
> Also, 2 days ago, I submitted the PCIe driver.
> 
> http://www.spinics.net/lists/linux-pci/msg20548.html
> http://www.spinics.net/lists/linux-pci/msg20549.html
> 
> If someone look at my code, it will be very helpful.

I took a quick peek at your DT bindings and I think they are way off
base. You need to read the thread(s) related to the new Armada and
Tegra drivers. Your bindings should look closer to them. 

You need to have DT bindings that follow the OF PCI requirements.

You need to determine if your hardware follows the PCI-SIG
requirements and includes a root port bridge config space, this will
guide if you need to use seperate PCI domains and DT stanzas, or if a
single domain and stanza should be used.

Please include lspci output in future commit messages

If you CC me the next time you post these patches I may be able to try
and help a bit, but lets start a new thread for that sort of
discussion. :)

Regards,
Jason