diff mbox

[v7,1/6] pci: Introduce pci_register_io_range() helper function.

Message ID CAL_JsqKCHf6VXR3FFcBSu1xuhP54dYsAJCZwT-X9p5iTZAOJfQ@mail.gmail.com
State Superseded
Headers show

Commit Message

Rob Herring June 27, 2014, 12:44 a.m. UTC
Adding Michael Simek...

On Thu, Jun 26, 2014 at 3:59 AM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> (sorry for replying to a months old thread)
>
> On Mon, Apr 07, 2014 at 06:58:24PM +0100, Bjorn Helgaas wrote:
>> On Mon, Apr 7, 2014 at 5:36 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> > I think migrating other architectures to use the same code should be
>> > a separate effort from adding a generic implementation that can be
>> > used by arm64. It's probably a good idea to have patches to convert
>> > arm32 and/or microblaze.
>>
>> Let me reiterate that I am 100% in favor of replacing arch-specific
>> code with more generic implementations.
>>
>> However, I am not 100% in favor of doing it as separate efforts
>> (although maybe I could be convinced).  The reasons I hesitate are
>> that (1) if only one architecture uses a new "generic" implementation,
>> we really don't know whether it is generic enough, (2) until I see the
>> patches to convert other architectures, I have to assume that I'm the
>> one who will write them, and (3) as soon as we add the code to
>> drivers/pci, it becomes partly my headache to maintain it, even if
>> only one arch benefits from it.
>
> I agree and understand your point.
>
>> Please don't think I'm questioning anyone's intent or good will.  It's
>> just that I understand the business pressures, and I know how hard it
>> can be to justify this sort of work to one's management, especially
>> after the immediate problem has been solved.
>
> But, unfortunately, that's something we failed to address in reasonable
> time (even though I was one of the proponents of the generic PCIe
> implementation). This work is very likely to slip further into the late
> part of this year and I am aware that several ARM partners are blocked
> on the (upstream) availability of PCIe support for the arm64 kernel.
>
> Although a bit late, I'm raising this now and hopefully we'll come to a
> conclusion soon. Delaying arm64 PCIe support even further is not a real
> option, which leaves us with:
>
> 1. Someone else (with enough PCIe knowledge) volunteering to take over
>    soon or

Well, I might have 2 months ago, but now I'm pretty booked up.

> 2. Dropping Liviu's work and going for an arm64-specific implementation
>    (most likely based on the arm32 implementation, see below)

3. Keeping Liviu's patches leaving some of the architecture specific
bits. I know Arnd and I both commented on it still needing more common
pieces, but compared to option 2 that would be way better.

Let's look at the patches in question:

3e71867 pci: Introduce pci_register_io_range() helper function.
6681dff pci: OF: Fix the conversion of IO ranges into IO resources.

Both OF patches. I'll happily merge them.


2d5dd85 pci: Create pci_host_bridge before its associated bus in
pci_create_root_bus.
f6f2854 pci: Introduce a domain number for pci_host_bridge.
524a9f5 pci: Export find_pci_host_bridge() function.

These don't seem to be too controversial.


fb75718 pci: of: Parse and map the IRQ when adding the PCI device.

6 LOC. Hardly controversial.


920a685 pci: Add support for creating a generic host_bridge from device tree

This function could be moved to drivers/of/of_pci.c if having it in
drivers/pci is too much maintenance burden. However, nearly the same
code is already being duplicated in every DT enabled ARM PCI host
driver and will continue as more PCI hosts are added. So this isn't
really a question of converting other architectures to common PCI host
infrastructure, but converting DT based PCI hosts to common
infrastructure. ARM is the only arch moving host drivers to
drivers/pci ATM. Until other architectures start doing that,
converting them is pointless.

bcf5c10 Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.

Seems like an independent fix that should be applied regardless.


7cfde80 arm64: Add architecture support for PCI

What is here is really just a function of which option we pick.


> First option is ideal but there is work to do as laid out by Arnd here:
>
> http://article.gmane.org/gmane.linux.kernel/1679304

I don't agree arm32 is harder than microblaze. Yes, converting ALL of
arm would be, but that is not necessary. With Liviu's latest branch
the hacks I previously needed are gone (thanks!), and this is all I
need to get Versatile PCI working (under QEMU):

 void pci_ioremap_set_mem_type(int mem_type);

Hum, so I guess now I've converted ARM...

Here's a branch with my changes[1]. And BTW, it also has
multi-platform support for Versatile as moving the PCI host to DT (and
drivers/pci/host) is about the last remaining obstacle.

Rob

[1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
versatile-pci-v2
--
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

Comments

Arnd Bergmann June 27, 2014, 11:03 a.m. UTC | #1
On Thursday 26 June 2014 19:44:21 Rob Herring wrote:
> > Although a bit late, I'm raising this now and hopefully we'll come to a
> > conclusion soon. Delaying arm64 PCIe support even further is not a real
> > option, which leaves us with:
> >
> > 1. Someone else (with enough PCIe knowledge) volunteering to take over
> >    soon or
> 
> Well, I might have 2 months ago, but now I'm pretty booked up.
> 
> > 2. Dropping Liviu's work and going for an arm64-specific implementation
> >    (most likely based on the arm32 implementation, see below)
> 
> 3. Keeping Liviu's patches leaving some of the architecture specific
> bits. I know Arnd and I both commented on it still needing more common
> pieces, but compared to option 2 that would be way better.

Agreed.
 
> 920a685 pci: Add support for creating a generic host_bridge from device tree
> 
> This function could be moved to drivers/of/of_pci.c if having it in
> drivers/pci is too much maintenance burden. However, nearly the same
> code is already being duplicated in every DT enabled ARM PCI host
> driver and will continue as more PCI hosts are added. So this isn't
> really a question of converting other architectures to common PCI host
> infrastructure, but converting DT based PCI hosts to common
> infrastructure. ARM is the only arch moving host drivers to
> drivers/pci ATM. Until other architectures start doing that,
> converting them is pointless.

This is the most important part in my mind. Every implementation gets
this code wrong at least initially, and we have to provide some generic
helper to get the broken code out of host drivers. I don't care whether
that helper is part of the PCI core or part of the OF core.

> 7cfde80 arm64: Add architecture support for PCI
> 
> What is here is really just a function of which option we pick.
> 
> 
> > First option is ideal but there is work to do as laid out by Arnd here:
> >
> > http://article.gmane.org/gmane.linux.kernel/1679304
> 
> I don't agree arm32 is harder than microblaze. Yes, converting ALL of
> arm would be, but that is not necessary. With Liviu's latest branch
> the hacks I previously needed are gone (thanks!), and this is all I
> need to get Versatile PCI working (under QEMU):

I meant converting all of arm32 would be harder, but I agree we don't
have to do it. It would be nice to convert all of the drivers/pci/host
drivers though, iow all multiplatform-enabled ones, and leaving the
current arm32 pci implementation for the platforms we don't want to
convert to multiplatform anyway (footbridge, iop, ixp4xx, ks8695 (?),
pxa, sa1100).

	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
Will Deacon June 27, 2014, 12:49 p.m. UTC | #2
On Fri, Jun 27, 2014 at 12:03:34PM +0100, Arnd Bergmann wrote:
> On Thursday 26 June 2014 19:44:21 Rob Herring wrote:
> > I don't agree arm32 is harder than microblaze. Yes, converting ALL of
> > arm would be, but that is not necessary. With Liviu's latest branch
> > the hacks I previously needed are gone (thanks!), and this is all I
> > need to get Versatile PCI working (under QEMU):
> 
> I meant converting all of arm32 would be harder, but I agree we don't
> have to do it. It would be nice to convert all of the drivers/pci/host
> drivers though, iow all multiplatform-enabled ones, and leaving the
> current arm32 pci implementation for the platforms we don't want to
> convert to multiplatform anyway (footbridge, iop, ixp4xx, ks8695 (?),
> pxa, sa1100).

I'm more than happy to convert the generic host controller we merged
recently, but I'd probably want the core changes merged first so that I
know I'm not wasting my time!

Will
--
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
Arnd Bergmann June 27, 2014, 1:16 p.m. UTC | #3
On Friday 27 June 2014 13:49:49 Will Deacon wrote:
> On Fri, Jun 27, 2014 at 12:03:34PM +0100, Arnd Bergmann wrote:
> > On Thursday 26 June 2014 19:44:21 Rob Herring wrote:
> > > I don't agree arm32 is harder than microblaze. Yes, converting ALL of
> > > arm would be, but that is not necessary. With Liviu's latest branch
> > > the hacks I previously needed are gone (thanks!), and this is all I
> > > need to get Versatile PCI working (under QEMU):
> > 
> > I meant converting all of arm32 would be harder, but I agree we don't
> > have to do it. It would be nice to convert all of the drivers/pci/host
> > drivers though, iow all multiplatform-enabled ones, and leaving the
> > current arm32 pci implementation for the platforms we don't want to
> > convert to multiplatform anyway (footbridge, iop, ixp4xx, ks8695 (?),
> > pxa, sa1100).
> 
> I'm more than happy to convert the generic host controller we merged
> recently, but I'd probably want the core changes merged first so that I
> know I'm not wasting my time!

That is definitely fine with me, but it's Bjorn's decision.

	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
Catalin Marinas June 27, 2014, 1:38 p.m. UTC | #4
On Fri, Jun 27, 2014 at 02:16:28PM +0100, Arnd Bergmann wrote:
> On Friday 27 June 2014 13:49:49 Will Deacon wrote:
> > On Fri, Jun 27, 2014 at 12:03:34PM +0100, Arnd Bergmann wrote:
> > > On Thursday 26 June 2014 19:44:21 Rob Herring wrote:
> > > > I don't agree arm32 is harder than microblaze. Yes, converting ALL of
> > > > arm would be, but that is not necessary. With Liviu's latest branch
> > > > the hacks I previously needed are gone (thanks!), and this is all I
> > > > need to get Versatile PCI working (under QEMU):
> > > 
> > > I meant converting all of arm32 would be harder, but I agree we don't
> > > have to do it. It would be nice to convert all of the drivers/pci/host
> > > drivers though, iow all multiplatform-enabled ones, and leaving the
> > > current arm32 pci implementation for the platforms we don't want to
> > > convert to multiplatform anyway (footbridge, iop, ixp4xx, ks8695 (?),
> > > pxa, sa1100).
> > 
> > I'm more than happy to convert the generic host controller we merged
> > recently, but I'd probably want the core changes merged first so that I
> > know I'm not wasting my time!
> 
> That is definitely fine with me, but it's Bjorn's decision.

Or the changes to the generic host controller can be part of Liviu's
patch series (maybe together with other PCI host drivers like
designware, as time allows).
Catalin Marinas June 27, 2014, 2:14 p.m. UTC | #5
On Fri, Jun 27, 2014 at 01:44:21AM +0100, Rob Herring wrote:
> On Thu, Jun 26, 2014 at 3:59 AM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > Although a bit late, I'm raising this now and hopefully we'll come to a
> > conclusion soon. Delaying arm64 PCIe support even further is not a real
> > option, which leaves us with:
> >
> > 1. Someone else (with enough PCIe knowledge) volunteering to take over
> >    soon or
> > 2. Dropping Liviu's work and going for an arm64-specific implementation
> >    (most likely based on the arm32 implementation, see below)
> 
> 3. Keeping Liviu's patches leaving some of the architecture specific
> bits. I know Arnd and I both commented on it still needing more common
> pieces, but compared to option 2 that would be way better.
> 
> Let's look at the patches in question:
> 
> 3e71867 pci: Introduce pci_register_io_range() helper function.
> 6681dff pci: OF: Fix the conversion of IO ranges into IO resources.
> 
> Both OF patches. I'll happily merge them.

We just need to make sure they don't break other users of
of_pci_range_to_resource() that the second patch introduces.

> 2d5dd85 pci: Create pci_host_bridge before its associated bus in
> pci_create_root_bus.
> f6f2854 pci: Introduce a domain number for pci_host_bridge.
> 524a9f5 pci: Export find_pci_host_bridge() function.
> 
> These don't seem to be too controversial.

I think here there were discussions around introducing domain_nr to
pci_host_bridge, particularly to the pci_create_root_bus_in_domain() API
change. I don't think we reached any conclusion.

> fb75718 pci: of: Parse and map the IRQ when adding the PCI device.
> 
> 6 LOC. Hardly controversial.

I agree.

> 920a685 pci: Add support for creating a generic host_bridge from device tree
> 
> This function could be moved to drivers/of/of_pci.c if having it in
> drivers/pci is too much maintenance burden.

I think it makes sense. Currently drivers/pci/host-bridge.c doesn't have
anything OF related, so of_pci.c looks more appropriate.

> However, nearly the same
> code is already being duplicated in every DT enabled ARM PCI host
> driver and will continue as more PCI hosts are added. So this isn't
> really a question of converting other architectures to common PCI host
> infrastructure, but converting DT based PCI hosts to common
> infrastructure. ARM is the only arch moving host drivers to
> drivers/pci ATM. Until other architectures start doing that,
> converting them is pointless.

I agree. It's probably more important to convert some of the
drivers/pci/host implementations to using the common parsing rather than
a new architecture (this way we avoid even more code duplication).

> bcf5c10 Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.
> 
> Seems like an independent fix that should be applied regardless.

Indeed. But it got stuck at the top of this series and hasn't been
pushed upstream.

> 7cfde80 arm64: Add architecture support for PCI
> 
> What is here is really just a function of which option we pick.

With Liviu's latest version (not posted) and with
of_create_pci_host_bridge() function moved to of_pci.c, I don't think
there is much new functionality added to drivers/pci/. What I think we
need is clarifying the domain_nr patch (and API change) and more users
of the new generic code. As you said, it doesn't need to be a separate
architecture but rather existing pci host drivers under drivers/pci. Of
course, other arch conversion should follow shortly as well but even
without an immediate conversion, I don't see too much additional
maintenance burden for the core PCIe code (and code sharing between new
PCIe host drivers is even more beneficial).
Bjorn Helgaas June 27, 2014, 2:55 p.m. UTC | #6
On Fri, Jun 27, 2014 at 8:14 AM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> ...
> With Liviu's latest version (not posted) and with
> of_create_pci_host_bridge() function moved to of_pci.c, I don't think
> there is much new functionality added to drivers/pci/. What I think we
> need is clarifying the domain_nr patch (and API change) and more users
> of the new generic code. As you said, it doesn't need to be a separate
> architecture but rather existing pci host drivers under drivers/pci. Of
> course, other arch conversion should follow shortly as well but even
> without an immediate conversion, I don't see too much additional
> maintenance burden for the core PCIe code (and code sharing between new
> PCIe host drivers is even more beneficial).

Sorry, I haven't had time to follow this.  It sounds like there are
several pieces we could get out of the way easily.  How about posting
the actual patches again?  Maybe re-order them so the easy pieces are
first so they can get applied even if there are issues with later
ones?

Bjorn
--
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
Liviu Dudau June 27, 2014, 3:18 p.m. UTC | #7
On Fri, Jun 27, 2014 at 03:55:04PM +0100, Bjorn Helgaas wrote:
> On Fri, Jun 27, 2014 at 8:14 AM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > ...
> > With Liviu's latest version (not posted) and with
> > of_create_pci_host_bridge() function moved to of_pci.c, I don't think
> > there is much new functionality added to drivers/pci/. What I think we
> > need is clarifying the domain_nr patch (and API change) and more users
> > of the new generic code. As you said, it doesn't need to be a separate
> > architecture but rather existing pci host drivers under drivers/pci. Of
> > course, other arch conversion should follow shortly as well but even
> > without an immediate conversion, I don't see too much additional
> > maintenance burden for the core PCIe code (and code sharing between new
> > PCIe host drivers is even more beneficial).
> 
> Sorry, I haven't had time to follow this.  It sounds like there are
> several pieces we could get out of the way easily.  How about posting
> the actual patches again?  Maybe re-order them so the easy pieces are
> first so they can get applied even if there are issues with later
> ones?

OK, I will post a new series on Monday.

Thanks,
Liviu

> 
> Bjorn
>
diff mbox

Patch

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 3d23418..22b7529 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -178,6 +178,7 @@  static inline void __iomem *__typesafe_io(unsigned
long addr)

 /* PCI fixed i/o mapping */
 #define PCI_IO_VIRT_BASE       0xfee00000
+#define PCI_IOBASE             PCI_IO_VIRT_BASE

 #if defined(CONFIG_PCI)