| Message ID | 1394811258-1500-4-git-send-email-Liviu.Dudau@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote: > --- /dev/null > +++ b/arch/arm64/kernel/pci.c [...] > +int pci_register_io_range(phys_addr_t address, resource_size_t size) [...] > +unsigned long pci_address_to_pio(phys_addr_t address) [...] > +void pcibios_fixup_bus(struct pci_bus *bus) [ actually most of this file ] Maybe it was raised before already but can we have __weak generic definitions of these functions? They don't seem to be arm64 specific in any way.
On Friday 14 March 2014, Catalin Marinas wrote: > On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote: > > --- /dev/null > > +++ b/arch/arm64/kernel/pci.c > [...] > > +int pci_register_io_range(phys_addr_t address, resource_size_t size) > [...] > > +unsigned long pci_address_to_pio(phys_addr_t address) > [...] > > +void pcibios_fixup_bus(struct pci_bus *bus) > [ actually most of this file ] > > Maybe it was raised before already but can we have __weak generic > definitions of these functions? They don't seem to be arm64 specific in > any way. > I would definitely prefer that. Arnd
On Fri, Mar 14, 2014 at 05:38:08PM +0000, Arnd Bergmann wrote: > On Friday 14 March 2014, Catalin Marinas wrote: > > On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote: > > > --- /dev/null > > > +++ b/arch/arm64/kernel/pci.c > > [...] > > > +int pci_register_io_range(phys_addr_t address, resource_size_t size) > > [...] > > > +unsigned long pci_address_to_pio(phys_addr_t address) > > [...] > > > +void pcibios_fixup_bus(struct pci_bus *bus) > > [ actually most of this file ] > > > > Maybe it was raised before already but can we have __weak generic > > definitions of these functions? They don't seem to be arm64 specific in > > any way. > > > > I would definitely prefer that. > > Arnd > I haven't seen any reaction from Bjorn on this, so I threaded carefully on that subject. I'm new to this so I don't know how to handle this. To my mind, and looking at the way every architecture has been setup, the pcibios_* function are intended to be provided by the architecture. No matter how much wishful thinking we are going to put in here, it will not change the fact that the non-arm64 specific version of pcibios_fixup_bus() that I wrote is not shared by anyone else and it will remain "for arm64 use only" regardless to where it is placed until the next architecture comes into the kernel. And even then its adoption is questionable. If we are looking for simple and common implementations of this function, maybe we should look at why microblaze and powerpc versions, that are identical, are not being made the default __weak implementation. As for the other two functions, I've no special attachment to where they are present and I'm happy to move them into drivers/pci on the condition that the patchset doesn't double in size. The reason why I'm weary of touching other architectures in a significant way is the current lack of engineering bandwidth and way of testing all the architectures. My low friction approach has been to introduce them in arm64 and then slowly move them into core (and yes, I know about good intentions and the road to hell.) Catalin, if you are happy to ask for ACKs from all arch maintainers that might get affected by our custom version of pci_address_to_pio() before you can pull PCI support for arm64 then I can propose a new patchset. Best regards, Liviu
On Friday 14 March 2014, Liviu Dudau wrote: > > > > I haven't seen any reaction from Bjorn on this, so I threaded carefully on that > subject. I'm new to this so I don't know how to handle this. > > To my mind, and looking at the way every architecture has been setup, the pcibios_* > function are intended to be provided by the architecture. That is definitely correct. > No matter how much wishful > thinking we are going to put in here, it will not change the fact that the non-arm64 > specific version of pcibios_fixup_bus() that I wrote is not shared by anyone else > and it will remain "for arm64 use only" regardless to where it is placed until the > next architecture comes into the kernel. And even then its adoption is questionable. Agreed as well. > If we are looking for simple and common implementations of this function, maybe we > should look at why microblaze and powerpc versions, that are identical, are not being > made the default __weak implementation. Microblaze could most likely just be moved over to your version. The only reason it is shared with the powerpc implementation is that they were anticipating the code to become shared again and that it was known to be working with flattened device tree. > As for the other two functions, I've no special attachment to where they are present > and I'm happy to move them into drivers/pci on the condition that the patchset doesn't > double in size. The reason why I'm weary of touching other architectures in a significant > way is the current lack of engineering bandwidth and way of testing all the architectures. > My low friction approach has been to introduce them in arm64 and then slowly move them > into core (and yes, I know about good intentions and the road to hell.) I think everyone working on PCI is fed up with having arch-specific implementations of all these, and Bjorn has been very supportive of generic infrastructure in the past. Even just adding a generic infrastructure in a common place that is used only by one architecture in my mind would be a significant improvement. Arnd
On Fri, 2014-03-14 at 20:10 +0100, Arnd Bergmann wrote: > > As for the other two functions, I've no special attachment to where they are present > > and I'm happy to move them into drivers/pci on the condition that the patchset doesn't > > double in size. The reason why I'm weary of touching other architectures in a significant > > way is the current lack of engineering bandwidth and way of testing all the architectures. > > My low friction approach has been to introduce them in arm64 and then slowly move them > > into core (and yes, I know about good intentions and the road to hell.) > > I think everyone working on PCI is fed up with having arch-specific implementations > of all these, and Bjorn has been very supportive of generic infrastructure in the > past. Even just adding a generic infrastructure in a common place that is used > only by one architecture in my mind would be a significant improvement. I agree, it's a reasonable approach and microblaze which is simple and just "copied" powerpc originally would be a good one to move over as well. powerpc itself has many historical quirks and while I'm interested in a common implementation, it will take me a bit of spare time to get through things and figure out what can be done there and what "hooks" might still be necessary. At this point, it's mostly a matter of: - I'm the one who knows the most about the powerpc PCI code as I wrote large chunks of it - I'm very very very busy with some other things at the moment So don't take my silence on these matters as a lack of interest, I think it's definitely all going in the right direction, I just don't have much bandwidth to consider the move of powerpc over just yet. Cheers, Ben.
On Fri, Mar 14, 2014 at 10:34 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > Use the generic host bridge functions to provide support for > PCI Express on arm64. There is no support for ISA memory. > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > Tested-by: Tanmay Inamdar <tinamdar@apm.com> > --- > arch/arm64/Kconfig | 19 +++- > arch/arm64/include/asm/Kbuild | 1 + > arch/arm64/include/asm/io.h | 3 +- > arch/arm64/include/asm/pci.h | 51 ++++++++++ > arch/arm64/kernel/Makefile | 1 + > arch/arm64/kernel/pci.c | 173 ++++++++++++++++++++++++++++++++ > 6 files changed, 246 insertions(+), 2 deletions(-) > create mode 100644 arch/arm64/include/asm/pci.h > create mode 100644 arch/arm64/kernel/pci.c [snip] > +#endif > + > +extern unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr); Can we at least align the function definition across architectures if not the implementation. > +int pci_register_io_range(phys_addr_t address, resource_size_t size) > +{ > + struct ioresource *res; > + resource_size_t allocated_size = 0; > + > + /* find if the range has not been already allocated */ > + list_for_each_entry(res, &io_list, list) { > + if (address >= res->start && > + address + size <= res->start + size) > + return 0; > + allocated_size += res->size; > + } > + > + /* range not already registered, check for space */ > + if (allocated_size + size > IO_SPACE_LIMIT) I believe this needs to be "allocated_size + size - 1". > + return -E2BIG; > +
On Mon, Mar 17, 2014 at 04:05:38PM +0000, Rob Herring wrote: > On Fri, Mar 14, 2014 at 10:34 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > > Use the generic host bridge functions to provide support for > > PCI Express on arm64. There is no support for ISA memory. > > > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > > Tested-by: Tanmay Inamdar <tinamdar@apm.com> > > --- > > arch/arm64/Kconfig | 19 +++- > > arch/arm64/include/asm/Kbuild | 1 + > > arch/arm64/include/asm/io.h | 3 +- > > arch/arm64/include/asm/pci.h | 51 ++++++++++ > > arch/arm64/kernel/Makefile | 1 + > > arch/arm64/kernel/pci.c | 173 ++++++++++++++++++++++++++++++++ > > 6 files changed, 246 insertions(+), 2 deletions(-) > > create mode 100644 arch/arm64/include/asm/pci.h > > create mode 100644 arch/arm64/kernel/pci.c > > [snip] > > > +#endif > > + > > +extern unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr); > > Can we at least align the function definition across architectures if > not the implementation. The choice of names is unfortunate. We are trying to follow the spirit of the arch/arm function, not the implementation, and for RFC that served the purpose. I'll come up with a better name as I don't intend to share the implementation with arch/arm here. Best regards, Liviu > > > > +int pci_register_io_range(phys_addr_t address, resource_size_t size) > > +{ > > + struct ioresource *res; > > + resource_size_t allocated_size = 0; > > + > > + /* find if the range has not been already allocated */ > > + list_for_each_entry(res, &io_list, list) { > > + if (address >= res->start && > > + address + size <= res->start + size) > > + return 0; > > + allocated_size += res->size; > > + } > > + > > + /* range not already registered, check for space */ > > + if (allocated_size + size > IO_SPACE_LIMIT) > > I believe this needs to be "allocated_size + size - 1". > > > + return -E2BIG; > > + >
On Fri, Mar 14, 2014 at 06:05:27PM +0000, Liviu Dudau wrote: > On Fri, Mar 14, 2014 at 05:38:08PM +0000, Arnd Bergmann wrote: > > On Friday 14 March 2014, Catalin Marinas wrote: > > > On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote: > > > > --- /dev/null > > > > +++ b/arch/arm64/kernel/pci.c > > > [...] > > > > +int pci_register_io_range(phys_addr_t address, resource_size_t size) > > > [...] > > > > +unsigned long pci_address_to_pio(phys_addr_t address) > > > [...] > > > > +void pcibios_fixup_bus(struct pci_bus *bus) > > > [ actually most of this file ] > > > > > > Maybe it was raised before already but can we have __weak generic > > > definitions of these functions? They don't seem to be arm64 specific in > > > any way. [...] > Catalin, if you are happy to ask for ACKs from all arch maintainers that might get > affected by our custom version of pci_address_to_pio() before you can pull PCI support > for arm64 then I can propose a new patchset. You don't need to change the other architectures, that's the point of a __weak definition, it will be automatically overridden. If you want, you can even place a GENERIC_PCI or whatever config option that is only selected by arm64 for the time being.
On Mon, Mar 17, 2014 at 05:38:21PM +0000, Catalin Marinas wrote: > On Fri, Mar 14, 2014 at 06:05:27PM +0000, Liviu Dudau wrote: > > On Fri, Mar 14, 2014 at 05:38:08PM +0000, Arnd Bergmann wrote: > > > On Friday 14 March 2014, Catalin Marinas wrote: > > > > On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote: > > > > > --- /dev/null > > > > > +++ b/arch/arm64/kernel/pci.c > > > > [...] > > > > > +int pci_register_io_range(phys_addr_t address, resource_size_t size) > > > > [...] > > > > > +unsigned long pci_address_to_pio(phys_addr_t address) > > > > [...] > > > > > +void pcibios_fixup_bus(struct pci_bus *bus) > > > > [ actually most of this file ] > > > > > > > > Maybe it was raised before already but can we have __weak generic > > > > definitions of these functions? They don't seem to be arm64 specific in > > > > any way. > [...] > > Catalin, if you are happy to ask for ACKs from all arch maintainers that might get > > affected by our custom version of pci_address_to_pio() before you can pull PCI support > > for arm64 then I can propose a new patchset. > > You don't need to change the other architectures, that's the point of a > __weak definition, it will be automatically overridden. If you want, you > can even place a GENERIC_PCI or whatever config option that is only > selected by arm64 for the time being. pci_address_to_pio() is alread __weak. My patch was adding the arm64 version of it. Adding an #ifdef GENERIC_PCI to the __weak implementation is not just a temporary solution. We would still have to keep some sort of #ifdef around in the function as x86 will never pickup our version. Regardless, the list of people that would need to ACK that change will increase, right? Best regards, Liviu > > -- > Catalin > -- > 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 Mon, Mar 17, 2014 at 06:05:25PM +0000, Liviu Dudau wrote: > On Mon, Mar 17, 2014 at 05:38:21PM +0000, Catalin Marinas wrote: > > On Fri, Mar 14, 2014 at 06:05:27PM +0000, Liviu Dudau wrote: > > > On Fri, Mar 14, 2014 at 05:38:08PM +0000, Arnd Bergmann wrote: > > > > On Friday 14 March 2014, Catalin Marinas wrote: > > > > > On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote: > > > > > > --- /dev/null > > > > > > +++ b/arch/arm64/kernel/pci.c > > > > > [...] > > > > > > +int pci_register_io_range(phys_addr_t address, resource_size_t size) > > > > > [...] > > > > > > +unsigned long pci_address_to_pio(phys_addr_t address) > > > > > [...] > > > > > > +void pcibios_fixup_bus(struct pci_bus *bus) > > > > > [ actually most of this file ] > > > > > > > > > > Maybe it was raised before already but can we have __weak generic > > > > > definitions of these functions? They don't seem to be arm64 specific in > > > > > any way. > > [...] > > > Catalin, if you are happy to ask for ACKs from all arch maintainers that might get > > > affected by our custom version of pci_address_to_pio() before you can pull PCI support > > > for arm64 then I can propose a new patchset. > > > > You don't need to change the other architectures, that's the point of a > > __weak definition, it will be automatically overridden. If you want, you > > can even place a GENERIC_PCI or whatever config option that is only > > selected by arm64 for the time being. > > pci_address_to_pio() is alread __weak. My patch was adding the arm64 version of it. Adding > an #ifdef GENERIC_PCI to the __weak implementation is not just a temporary solution. Ah, I start to understand what you mean, pci_address_to_pio() is already defined as __weak in drivers/of/address.c. So the reason we redefine it on arm64 is that it uses the io_list resources which are populated by pci_register_io_range(). Do you see any other architecture using a similar logic (that could be shared)? Any other functions in this file that could be shared (and are not __weak already)?
On Wed, Mar 19, 2014 at 01:56:19PM +0000, Catalin Marinas wrote: > On Mon, Mar 17, 2014 at 06:05:25PM +0000, Liviu Dudau wrote: > > On Mon, Mar 17, 2014 at 05:38:21PM +0000, Catalin Marinas wrote: > > > On Fri, Mar 14, 2014 at 06:05:27PM +0000, Liviu Dudau wrote: > > > > On Fri, Mar 14, 2014 at 05:38:08PM +0000, Arnd Bergmann wrote: > > > > > On Friday 14 March 2014, Catalin Marinas wrote: > > > > > > On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote: > > > > > > > --- /dev/null > > > > > > > +++ b/arch/arm64/kernel/pci.c > > > > > > [...] > > > > > > > +int pci_register_io_range(phys_addr_t address, resource_size_t size) > > > > > > [...] > > > > > > > +unsigned long pci_address_to_pio(phys_addr_t address) > > > > > > [...] > > > > > > > +void pcibios_fixup_bus(struct pci_bus *bus) > > > > > > [ actually most of this file ] > > > > > > > > > > > > Maybe it was raised before already but can we have __weak generic > > > > > > definitions of these functions? They don't seem to be arm64 specific in > > > > > > any way. > > > [...] > > > > Catalin, if you are happy to ask for ACKs from all arch maintainers that might get > > > > affected by our custom version of pci_address_to_pio() before you can pull PCI support > > > > for arm64 then I can propose a new patchset. > > > > > > You don't need to change the other architectures, that's the point of a > > > __weak definition, it will be automatically overridden. If you want, you > > > can even place a GENERIC_PCI or whatever config option that is only > > > selected by arm64 for the time being. > > > > pci_address_to_pio() is alread __weak. My patch was adding the arm64 version of it. Adding > > an #ifdef GENERIC_PCI to the __weak implementation is not just a temporary solution. > > Ah, I start to understand what you mean, pci_address_to_pio() is already > defined as __weak in drivers/of/address.c. So the reason we redefine it > on arm64 is that it uses the io_list resources which are populated by > pci_register_io_range(). Do you see any other architecture using a > similar logic (that could be shared)? All architectures that memory map the PCI IO range should be supported by my version of pci_address_to_pio(). But that still leaves the x86 and those architectures that have separate IO space or map it 1:1 into CPU address space to carry a different version (which the current "generic" weak version catters for). > > Any other functions in this file that could be shared (and are not > __weak already)? A version of the pci_register_io_range() that uses part of pci_ioremap_io() (the calculation of io_offset part). My ultimate point is that no matter how long we argue about the shape of the functions that I've added into arch/arm64/kernel/pci.c I don't think we can get away without having that file, or at least not in the first phase if we want speedy integration into mainline. Best regards, Liviu > > -- > Catalin
On Wed, Mar 19, 2014 at 12:21 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > On Wed, Mar 19, 2014 at 01:56:19PM +0000, Catalin Marinas wrote: >> On Mon, Mar 17, 2014 at 06:05:25PM +0000, Liviu Dudau wrote: >> > On Mon, Mar 17, 2014 at 05:38:21PM +0000, Catalin Marinas wrote: >> > > On Fri, Mar 14, 2014 at 06:05:27PM +0000, Liviu Dudau wrote: >> > > > On Fri, Mar 14, 2014 at 05:38:08PM +0000, Arnd Bergmann wrote: >> > > > > On Friday 14 March 2014, Catalin Marinas wrote: >> > > > > > On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote: >> > > > > > > --- /dev/null >> > > > > > > +++ b/arch/arm64/kernel/pci.c >> > > > > > [...] >> > > > > > > +int pci_register_io_range(phys_addr_t address, resource_size_t size) >> > > > > > [...] >> > > > > > > +unsigned long pci_address_to_pio(phys_addr_t address) >> > > > > > [...] >> > > > > > > +void pcibios_fixup_bus(struct pci_bus *bus) >> > > > > > [ actually most of this file ] >> > > > > > >> > > > > > Maybe it was raised before already but can we have __weak generic >> > > > > > definitions of these functions? They don't seem to be arm64 specific in >> > > > > > any way. >> > > [...] >> > > > Catalin, if you are happy to ask for ACKs from all arch maintainers that might get >> > > > affected by our custom version of pci_address_to_pio() before you can pull PCI support >> > > > for arm64 then I can propose a new patchset. >> > > >> > > You don't need to change the other architectures, that's the point of a >> > > __weak definition, it will be automatically overridden. If you want, you >> > > can even place a GENERIC_PCI or whatever config option that is only >> > > selected by arm64 for the time being. >> > >> > pci_address_to_pio() is alread __weak. My patch was adding the arm64 version of it. Adding >> > an #ifdef GENERIC_PCI to the __weak implementation is not just a temporary solution. >> >> Ah, I start to understand what you mean, pci_address_to_pio() is already >> defined as __weak in drivers/of/address.c. So the reason we redefine it >> on arm64 is that it uses the io_list resources which are populated by >> pci_register_io_range(). Do you see any other architecture using a >> similar logic (that could be shared)? > > All architectures that memory map the PCI IO range should be supported by my version of > pci_address_to_pio(). But that still leaves the x86 and those architectures that have > separate IO space or map it 1:1 into CPU address space to carry a different version (which > the current "generic" weak version catters for). > >> >> Any other functions in this file that could be shared (and are not >> __weak already)? > > A version of the pci_register_io_range() that uses part of pci_ioremap_io() (the calculation > of io_offset part). > > My ultimate point is that no matter how long we argue about the shape of the functions that > I've added into arch/arm64/kernel/pci.c I don't think we can get away without having that > file, or at least not in the first phase if we want speedy integration into mainline. "Speedy integration into mainline" is another way to say "leave it for others to deal with and clean up later." I've successfully used the preceeding series on ARM converting the Versatile PCI support to use it. The main part of the process was just copying this arm64 code to arm which tells me the code is in the wrong place. There are still some differences in pcibios_fixup_bus, and it is not clear to me whether the arm version should be doing all the things the arm64 version does and vice-versa. Certainly that should be sorted out. The other issues I see is pci_ioremap_io still has to be called by the driver and is not aligned at least for the prototype as I pointed out. Seems all these issues could be fixed with a simple CONFIG_ARCH_HAS_MMIO_IOSPACE kconfig option. Rob
On Wednesday 19 March 2014 12:53:11 Rob Herring wrote: > I've successfully used the preceeding series on ARM converting the > Versatile PCI support to use it. The main part of the process was just > copying this arm64 code to arm which tells me the code is in the wrong > place. There are still some differences in pcibios_fixup_bus, and it > is not clear to me whether the arm version should be doing all the > things the arm64 version does and vice-versa. Certainly that should be > sorted out. The other issues I see is pci_ioremap_io still has to be > called by the driver and is not aligned at least for the prototype as > I pointed out. Seems all these issues could be fixed with a simple > CONFIG_ARCH_HAS_MMIO_IOSPACE kconfig option. I don't even think we need a CONFIG option: Any architecture that wants to provide pci_ioremap_io (or whatever we end up calling the new version) needs to pass the virtual base address to the common code, and the easiest way to do that is using a macro (e.g. PCI_IO_VIRT_BASE). What I think we should end up with is a generic implementation like /* * architectures with special needs may provide their own version, * but most should be able to use this one. */ unsigned long __weak pci_ioremap_iospace(struct resource *bus, unsigned long cpu) { #ifdef PCI_IO_VIRT_BASE /* find free space, ioremap the bus address, return the offset */ ... #endif /* this architecture doesn't have memory mapped I/O space, so this function should never be called */ WARN_ON(1); return -ENODEV; } Arnd
On Wednesday 19 March 2014 17:21:41 Liviu Dudau wrote: > > My ultimate point is that no matter how long we argue about the shape of the functions that > I've added into arch/arm64/kernel/pci.c I don't think we can get away without having that > file, or at least not in the first phase if we want speedy integration into mainline. Let me simplify the discussion here: NAK to adding yet another architecture specific implementation. Arnd
On Wed, Mar 19, 2014 at 06:37:51PM +0000, Arnd Bergmann wrote: > On Wednesday 19 March 2014 17:21:41 Liviu Dudau wrote: > > > > My ultimate point is that no matter how long we argue about the shape of the functions that > > I've added into arch/arm64/kernel/pci.c I don't think we can get away without having that > > file, or at least not in the first phase if we want speedy integration into mainline. > > Let me simplify the discussion here: > > NAK to adding yet another architecture specific implementation. So what would be your approach for handling pci_address_to_pio() in a non-arch specific way? unsigned long __weak pci_address_to_pio(phys_addr_t address) { #ifdef ARCH_HAS_IOSPACE if (address > IO_SPACE_LIMIT) return (unsigned long)-1; return (unsigned long) address; #else struct ioresource *res; list_for_each_entry(res, &io_list, list) { if (address >= res->start && address < res->start + res->size) { return res->start - address; } } return (unsigned long)-1; #endif } Either that, or you have more magic rabbits than me. Best regards, Liviu > > Arnd > >
On Thursday 20 March 2014, Liviu Dudau wrote: > On Wed, Mar 19, 2014 at 06:37:51PM +0000, Arnd Bergmann wrote: > > On Wednesday 19 March 2014 17:21:41 Liviu Dudau wrote: > > > > > > My ultimate point is that no matter how long we argue about the shape of the functions that > > > I've added into arch/arm64/kernel/pci.c I don't think we can get away without having that > > > file, or at least not in the first phase if we want speedy integration into mainline. > > > > Let me simplify the discussion here: > > > > NAK to adding yet another architecture specific implementation. > > So what would be your approach for handling pci_address_to_pio() in a non-arch specific way? > > unsigned long __weak pci_address_to_pio(phys_addr_t address) > { > #ifdef ARCH_HAS_IOSPACE > if (address > IO_SPACE_LIMIT) > return (unsigned long)-1; > > return (unsigned long) address; > #else > struct ioresource *res; > > list_for_each_entry(res, &io_list, list) { > if (address >= res->start && > address < res->start + res->size) { > return res->start - address; > } > } > > return (unsigned long)-1; > #endif > } > > > Either that, or you have more magic rabbits than me. I don't even understand why you want to create a generic pci_address_to_pio implementation, when we don't need that for arm64 at all. Unless I'm missing something important, that function is only called in case of PCI_PROBE_DEVTREE with pci_of_scan on PowerPC. I don't expect any architecture to do the same thing, and the only other architecture that needs something like it (sparc) has a different implementation. The regular (non-DEVTREE) PCI bus scan should just be able to translate the BUS I/O addresses into Linux I/O port numbers using io_offset, without going through an intermediate step of translating into a CPU physical address first, so the entire lookup method won't get used. The reason why PowerPC needs it is that they traditionally don't use PCI config space access to assign or look at resources, but instead rely on the firmware to have set it up in advance and then put matching information into DT and the BARs. For Solaris and AIX, it's probably easier to use the information from DT, but in Linux, we already need to implement the manual bus scan, e.g. to do PCI device hotplugging if nothing else. Arnd
On Thu, Mar 20, 2014 at 11:17:22AM +0000, Arnd Bergmann wrote: > On Thursday 20 March 2014, Liviu Dudau wrote: > > On Wed, Mar 19, 2014 at 06:37:51PM +0000, Arnd Bergmann wrote: > > > On Wednesday 19 March 2014 17:21:41 Liviu Dudau wrote: > > > > > > > > My ultimate point is that no matter how long we argue about the shape of the functions that > > > > I've added into arch/arm64/kernel/pci.c I don't think we can get away without having that > > > > file, or at least not in the first phase if we want speedy integration into mainline. > > > > > > Let me simplify the discussion here: > > > > > > NAK to adding yet another architecture specific implementation. > > > > So what would be your approach for handling pci_address_to_pio() in a non-arch specific way? > > > > unsigned long __weak pci_address_to_pio(phys_addr_t address) > > { > > #ifdef ARCH_HAS_IOSPACE > > if (address > IO_SPACE_LIMIT) > > return (unsigned long)-1; > > > > return (unsigned long) address; > > #else > > struct ioresource *res; > > > > list_for_each_entry(res, &io_list, list) { > > if (address >= res->start && > > address < res->start + res->size) { > > return res->start - address; > > } > > } > > > > return (unsigned long)-1; > > #endif > > } > > > > > > Either that, or you have more magic rabbits than me. > > I don't even understand why you want to create a generic pci_address_to_pio > implementation, when we don't need that for arm64 at all. Unless I'm > missing something important, that function is only called in case of > PCI_PROBE_DEVTREE with pci_of_scan on PowerPC. I don't expect any > architecture to do the same thing, and the only other architecture that > needs something like it (sparc) has a different implementation. Because in my [v7 2/6]* patch for the generic host bridge support I start using pci_address_to_pio to fix the conversion of PCI ranges to resources. That requires an arm64 (or more correctly, an arch with memory mapped IO specific) version of pci_address_to_pio(). Best regards, Liviu > > The regular (non-DEVTREE) PCI bus scan should just be able to translate > the BUS I/O addresses into Linux I/O port numbers using io_offset, > without going through an intermediate step of translating into a CPU > physical address first, so the entire lookup method won't get used. > > The reason why PowerPC needs it is that they traditionally don't > use PCI config space access to assign or look at resources, but > instead rely on the firmware to have set it up in advance and then > put matching information into DT and the BARs. For Solaris and AIX, > it's probably easier to use the information from DT, but in Linux, > we already need to implement the manual bus scan, e.g. to do > PCI device hotplugging if nothing else. > > 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 Thursday 20 March 2014, Liviu Dudau wrote: > > I don't even understand why you want to create a generic pci_address_to_pio > > implementation, when we don't need that for arm64 at all. Unless I'm > > missing something important, that function is only called in case of > > PCI_PROBE_DEVTREE with pci_of_scan on PowerPC. I don't expect any > > architecture to do the same thing, and the only other architecture that > > needs something like it (sparc) has a different implementation. > > Because in my [v7 2/6]* patch for the generic host bridge support I start > using pci_address_to_pio to fix the conversion of PCI ranges to resources. > That requires an arm64 (or more correctly, an arch with memory mapped IO > specific) version of pci_address_to_pio(). Yes, but why do you use it there? Arnd
On Thu, Mar 20, 2014 at 12:26:02PM +0000, Arnd Bergmann wrote: > On Thursday 20 March 2014, Liviu Dudau wrote: > > > I don't even understand why you want to create a generic pci_address_to_pio > > > implementation, when we don't need that for arm64 at all. Unless I'm > > > missing something important, that function is only called in case of > > > PCI_PROBE_DEVTREE with pci_of_scan on PowerPC. I don't expect any > > > architecture to do the same thing, and the only other architecture that > > > needs something like it (sparc) has a different implementation. > > > > Because in my [v7 2/6]* patch for the generic host bridge support I start > > using pci_address_to_pio to fix the conversion of PCI ranges to resources. > > That requires an arm64 (or more correctly, an arch with memory mapped IO > > specific) version of pci_address_to_pio(). > > Yes, but why do you use it there? I'm parsing the device tree and have reached the PCI IO range for the host bridge. There is no host bridge created yet for that and hence no mapping. I need to take the DT range declaration that gives me CPU start .. SIZE --> BUS start and convert it into Logical IO port start .. Logical IO port end. While I could've come up with (yet) another function that does the conversion from CPU address to port IO address, I thought it would be nice to use the existing pci_address_to_pio() function and adapt it to my needs. It has a __weak implementation anyway so I could overwrite it in the architecture code. To me it looked like a good plan and we had some discussions around my initial goofy mistake of using pci_addr as physical address for translation. I never thought it was considered a bad idea. Best regards, Liviu > > Arnd >
On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote: > Use the generic host bridge functions to provide support for > PCI Express on arm64. There is no support for ISA memory. > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > Tested-by: Tanmay Inamdar <tinamdar@apm.com> > --- > arch/arm64/Kconfig | 19 +++- > arch/arm64/include/asm/Kbuild | 1 + > arch/arm64/include/asm/io.h | 3 +- > arch/arm64/include/asm/pci.h | 51 ++++++++++ > arch/arm64/kernel/Makefile | 1 + > arch/arm64/kernel/pci.c | 173 ++++++++++++++++++++++++++++++++ > 6 files changed, 246 insertions(+), 2 deletions(-) > create mode 100644 arch/arm64/include/asm/pci.h > create mode 100644 arch/arm64/kernel/pci.c > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 27bbcfc..d1c8568 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -62,7 +62,7 @@ config MMU > def_bool y > > config NO_IOPORT > - def_bool y > + def_bool y if !PCI > > config STACKTRACE_SUPPORT > def_bool y > @@ -134,6 +134,23 @@ menu "Bus support" > config ARM_AMBA > bool > > +config PCI > + bool "PCI support" > + help > + This feature enables support for PCIe bus system. If you say Y > + here, the kernel will include drivers and infrastructure code > + to support PCIe bus devices. > + > +config PCI_DOMAINS > + def_bool PCI > + > +config PCI_SYSCALL > + def_bool PCI > + > +source "drivers/pci/Kconfig" > +source "drivers/pci/pcie/Kconfig" > +source "drivers/pci/hotplug/Kconfig" > + > endmenu > > menu "Kernel Features" > diff --git a/arch/arm64/include/asm/Kbuild b/arch/arm64/include/asm/Kbuild > index 71c53ec..46924bc 100644 > --- a/arch/arm64/include/asm/Kbuild > +++ b/arch/arm64/include/asm/Kbuild > @@ -26,6 +26,7 @@ generic-y += mman.h > generic-y += msgbuf.h > generic-y += mutex.h > generic-y += pci.h > +generic-y += pci-bridge.h > generic-y += poll.h > generic-y += posix_types.h > generic-y += resource.h > diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h > index 7846a6b..67463a5 100644 > --- a/arch/arm64/include/asm/io.h > +++ b/arch/arm64/include/asm/io.h > @@ -120,7 +120,8 @@ static inline u64 __raw_readq(const volatile void __iomem *addr) > /* > * I/O port access primitives. > */ > -#define IO_SPACE_LIMIT 0xffff > +#define arch_has_dev_port() (1) > +#define IO_SPACE_LIMIT 0x1ffffff > #define PCI_IOBASE ((void __iomem *)(MODULES_VADDR - SZ_32M)) > > static inline u8 inb(unsigned long addr) > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h > new file mode 100644 > index 0000000..d93576f > --- /dev/null > +++ b/arch/arm64/include/asm/pci.h > @@ -0,0 +1,51 @@ > +#ifndef __ASM_PCI_H > +#define __ASM_PCI_H > +#ifdef __KERNEL__ > + > +#include <linux/types.h> > +#include <linux/slab.h> > +#include <linux/dma-mapping.h> > + > +#include <asm/io.h> > +#include <asm-generic/pci-bridge.h> > +#include <asm-generic/pci-dma-compat.h> > + > +#define PCIBIOS_MIN_IO 0x1000 > +#define PCIBIOS_MIN_MEM 0 > + > +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus); > + > +/* > + * Set to 1 if the kernel should re-assign all PCI bus numbers > + */ > +#define pcibios_assign_all_busses() \ > + (pci_has_flag(PCI_REASSIGN_ALL_BUS)) > + > +/* > + * PCI address space differs from physical memory address space > + */ > +#define PCI_DMA_BUS_IS_PHYS (0) > + > +extern int isa_dma_bridge_buggy; > + > +#ifdef CONFIG_PCI > +static inline int pci_domain_nr(struct pci_bus *bus) > +{ > + struct pci_host_bridge *bridge = find_pci_host_bridge(bus); > + > + if (bridge) > + return bridge->domain_nr; > + > + return 0; > +} > + > +static inline int pci_proc_domain(struct pci_bus *bus) > +{ > + return 1; > +} > +#endif > + > +extern unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr); > + > +#endif /* __KERNEL__ */ > +#endif /* __ASM_PCI_H */ > diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile > index 2d4554b..64fc479 100644 > --- a/arch/arm64/kernel/Makefile > +++ b/arch/arm64/kernel/Makefile > @@ -20,6 +20,7 @@ arm64-obj-$(CONFIG_HAVE_HW_BREAKPOINT)+= hw_breakpoint.o > arm64-obj-$(CONFIG_EARLY_PRINTK) += early_printk.o > arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND) += sleep.o suspend.o > arm64-obj-$(CONFIG_JUMP_LABEL) += jump_label.o > +arm64-obj-$(CONFIG_PCI) += pci.o > > obj-y += $(arm64-obj-y) vdso/ > obj-m += $(arm64-obj-m) > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > new file mode 100644 > index 0000000..9f29c9a > --- /dev/null > +++ b/arch/arm64/kernel/pci.c > @@ -0,0 +1,173 @@ > +/* > + * Code borrowed from powerpc/kernel/pci-common.c > + * > + * Copyright (C) 2003 Anton Blanchard <anton@au.ibm.com>, IBM > + * Copyright (C) 2014 ARM Ltd. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * version 2 as published by the Free Software Foundation. > + * > + */ > + > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/mm.h> > +#include <linux/of_pci.h> > +#include <linux/of_platform.h> > +#include <linux/slab.h> > + > +#include <asm/pci-bridge.h> > + > +struct ioresource { > + struct list_head list; > + phys_addr_t start; > + resource_size_t size; > +}; > + > +static LIST_HEAD(io_list); > + > +int pci_register_io_range(phys_addr_t address, resource_size_t size) > +{ > + struct ioresource *res; > + resource_size_t allocated_size = 0; > + > + /* find if the range has not been already allocated */ > + list_for_each_entry(res, &io_list, list) { > + if (address >= res->start && > + address + size <= res->start + size) > + return 0; > + allocated_size += res->size; > + } > + > + /* range not already registered, check for space */ > + if (allocated_size + size > IO_SPACE_LIMIT) > + return -E2BIG; > + > + /* add the range in the list */ > + res = kzalloc(sizeof(*res), GFP_KERNEL); > + if (!res) > + return -ENOMEM; > + res->start = address; > + res->size = size; > + > + list_add_tail(&res->list, &io_list); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(pci_register_io_range); > + > +unsigned long pci_address_to_pio(phys_addr_t address) > +{ > + struct ioresource *res; > + > + list_for_each_entry(res, &io_list, list) { > + if (address >= res->start && > + address < res->start + res->size) { > + return res->start - address; > + } > + } > + > + return (unsigned long)-1; > +} > +EXPORT_SYMBOL_GPL(pci_address_to_pio); > + > +/* > + * Called after each bus is probed, but before its children are examined > + */ > +void pcibios_fixup_bus(struct pci_bus *bus) > +{ > + struct pci_dev *dev; > + struct resource *res; > + int i; > + > + if (!pci_is_root_bus(bus)) { > + pci_read_bridge_bases(bus); > + > + pci_bus_for_each_resource(bus, res, i) { > + if (!res || !res->flags || res->parent) > + continue; > + > + /* > + * If we are going to reassign everything, we can > + * shrink the P2P resource to have zero size to > + * save space > + */ > + if (pci_has_flag(PCI_REASSIGN_ALL_RSRC)) { > + res->flags |= IORESOURCE_UNSET; > + res->start = 0; > + res->end = -1; > + continue; > + } > + } > + } Is there any other place we can put this? I'm trying to nuke pcibios_fixup_bus() because things like hotplug work better if we can do them in a per-device way rather than a per-bus way. > + > + list_for_each_entry(dev, &bus->devices, bus_list) { > + /* Ignore fully discovered devices */ > + if (dev->is_added) > + continue; > + > + set_dev_node(&dev->dev, pcibus_to_node(dev->bus)); Do you really need this? pci_device_add() already contains the same line. > + > + /* Read default IRQs and fixup if necessary */ > + dev->irq = of_irq_parse_and_map_pci(dev, 0, 0); Could this be done in pcibios_add_device() or someplace similar? > + } > +} > +EXPORT_SYMBOL(pcibios_fixup_bus); > + > +/* > + * We don't have to worry about legacy ISA devices, so nothing to do here > + */ > +resource_size_t pcibios_align_resource(void *data, const struct resource *res, > + resource_size_t size, resource_size_t align) > +{ > + return res->start; > +} > + > +int pcibios_enable_device(struct pci_dev *dev, int mask) > +{ > + return pci_enable_resources(dev, mask); > +} There is already a weak default implementation of pcibios_enable_device() that does this, so you should be able to just drop this. > +#define IO_SPACE_PAGES ((IO_SPACE_LIMIT + 1) / PAGE_SIZE) > +static DECLARE_BITMAP(pci_iospace, IO_SPACE_PAGES); > + > +unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr) > +{ > + unsigned long start, len, virt_start; > + int err; > + > + if (res->end > IO_SPACE_LIMIT) > + return -EINVAL; > + > + /* > + * try finding free space for the whole size first, > + * fall back to 64K if not available > + */ > + len = resource_size(res); > + start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES, > + res->start / PAGE_SIZE, len / PAGE_SIZE, 0); > + if (start == IO_SPACE_PAGES && len > SZ_64K) { > + len = SZ_64K; > + start = 0; > + start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES, > + start, len / PAGE_SIZE, 0); > + } > + > + /* no 64K area found */ > + if (start == IO_SPACE_PAGES) > + return -ENOMEM; > + > + /* ioremap physical aperture to virtual aperture */ > + virt_start = start * PAGE_SIZE + (unsigned long)PCI_IOBASE; > + err = ioremap_page_range(virt_start, virt_start + len, > + phys_addr, __pgprot(PROT_DEVICE_nGnRE)); > + if (err) > + return err; > + > + bitmap_set(pci_iospace, start, len / PAGE_SIZE); > + > + /* return io_offset */ > + return start * PAGE_SIZE - res->start; > +} > -- > 1.9.0 >
On Tue, Apr 08, 2014 at 12:58:30AM +0100, Bjorn Helgaas wrote: > On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote: > > Use the generic host bridge functions to provide support for > > PCI Express on arm64. There is no support for ISA memory. > > > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > > Tested-by: Tanmay Inamdar <tinamdar@apm.com> > > --- > > arch/arm64/Kconfig | 19 +++- > > arch/arm64/include/asm/Kbuild | 1 + > > arch/arm64/include/asm/io.h | 3 +- > > arch/arm64/include/asm/pci.h | 51 ++++++++++ > > arch/arm64/kernel/Makefile | 1 + > > arch/arm64/kernel/pci.c | 173 ++++++++++++++++++++++++++++++++ > > 6 files changed, 246 insertions(+), 2 deletions(-) > > create mode 100644 arch/arm64/include/asm/pci.h > > create mode 100644 arch/arm64/kernel/pci.c > > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > > index 27bbcfc..d1c8568 100644 > > --- a/arch/arm64/Kconfig > > +++ b/arch/arm64/Kconfig > > @@ -62,7 +62,7 @@ config MMU > > def_bool y > > > > config NO_IOPORT > > - def_bool y > > + def_bool y if !PCI > > > > config STACKTRACE_SUPPORT > > def_bool y > > @@ -134,6 +134,23 @@ menu "Bus support" > > config ARM_AMBA > > bool > > > > +config PCI > > + bool "PCI support" > > + help > > + This feature enables support for PCIe bus system. If you say Y > > + here, the kernel will include drivers and infrastructure code > > + to support PCIe bus devices. > > + > > +config PCI_DOMAINS > > + def_bool PCI > > + > > +config PCI_SYSCALL > > + def_bool PCI > > + > > +source "drivers/pci/Kconfig" > > +source "drivers/pci/pcie/Kconfig" > > +source "drivers/pci/hotplug/Kconfig" > > + > > endmenu > > > > menu "Kernel Features" > > diff --git a/arch/arm64/include/asm/Kbuild b/arch/arm64/include/asm/Kbuild > > index 71c53ec..46924bc 100644 > > --- a/arch/arm64/include/asm/Kbuild > > +++ b/arch/arm64/include/asm/Kbuild > > @@ -26,6 +26,7 @@ generic-y += mman.h > > generic-y += msgbuf.h > > generic-y += mutex.h > > generic-y += pci.h > > +generic-y += pci-bridge.h > > generic-y += poll.h > > generic-y += posix_types.h > > generic-y += resource.h > > diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h > > index 7846a6b..67463a5 100644 > > --- a/arch/arm64/include/asm/io.h > > +++ b/arch/arm64/include/asm/io.h > > @@ -120,7 +120,8 @@ static inline u64 __raw_readq(const volatile void __iomem *addr) > > /* > > * I/O port access primitives. > > */ > > -#define IO_SPACE_LIMIT 0xffff > > +#define arch_has_dev_port() (1) > > +#define IO_SPACE_LIMIT 0x1ffffff > > #define PCI_IOBASE ((void __iomem *)(MODULES_VADDR - SZ_32M)) > > > > static inline u8 inb(unsigned long addr) > > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h > > new file mode 100644 > > index 0000000..d93576f > > --- /dev/null > > +++ b/arch/arm64/include/asm/pci.h > > @@ -0,0 +1,51 @@ > > +#ifndef __ASM_PCI_H > > +#define __ASM_PCI_H > > +#ifdef __KERNEL__ > > + > > +#include <linux/types.h> > > +#include <linux/slab.h> > > +#include <linux/dma-mapping.h> > > + > > +#include <asm/io.h> > > +#include <asm-generic/pci-bridge.h> > > +#include <asm-generic/pci-dma-compat.h> > > + > > +#define PCIBIOS_MIN_IO 0x1000 > > +#define PCIBIOS_MIN_MEM 0 > > + > > +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus); > > + > > +/* > > + * Set to 1 if the kernel should re-assign all PCI bus numbers > > + */ > > +#define pcibios_assign_all_busses() \ > > + (pci_has_flag(PCI_REASSIGN_ALL_BUS)) > > + > > +/* > > + * PCI address space differs from physical memory address space > > + */ > > +#define PCI_DMA_BUS_IS_PHYS (0) > > + > > +extern int isa_dma_bridge_buggy; > > + > > +#ifdef CONFIG_PCI > > +static inline int pci_domain_nr(struct pci_bus *bus) > > +{ > > + struct pci_host_bridge *bridge = find_pci_host_bridge(bus); > > + > > + if (bridge) > > + return bridge->domain_nr; > > + > > + return 0; > > +} > > + > > +static inline int pci_proc_domain(struct pci_bus *bus) > > +{ > > + return 1; > > +} > > +#endif > > + > > +extern unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr); > > + > > +#endif /* __KERNEL__ */ > > +#endif /* __ASM_PCI_H */ > > diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile > > index 2d4554b..64fc479 100644 > > --- a/arch/arm64/kernel/Makefile > > +++ b/arch/arm64/kernel/Makefile > > @@ -20,6 +20,7 @@ arm64-obj-$(CONFIG_HAVE_HW_BREAKPOINT)+= hw_breakpoint.o > > arm64-obj-$(CONFIG_EARLY_PRINTK) += early_printk.o > > arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND) += sleep.o suspend.o > > arm64-obj-$(CONFIG_JUMP_LABEL) += jump_label.o > > +arm64-obj-$(CONFIG_PCI) += pci.o > > > > obj-y += $(arm64-obj-y) vdso/ > > obj-m += $(arm64-obj-m) > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > > new file mode 100644 > > index 0000000..9f29c9a > > --- /dev/null > > +++ b/arch/arm64/kernel/pci.c > > @@ -0,0 +1,173 @@ > > +/* > > + * Code borrowed from powerpc/kernel/pci-common.c > > + * > > + * Copyright (C) 2003 Anton Blanchard <anton@au.ibm.com>, IBM > > + * Copyright (C) 2014 ARM Ltd. > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License > > + * version 2 as published by the Free Software Foundation. > > + * > > + */ > > + > > +#include <linux/init.h> > > +#include <linux/io.h> > > +#include <linux/kernel.h> > > +#include <linux/mm.h> > > +#include <linux/of_pci.h> > > +#include <linux/of_platform.h> > > +#include <linux/slab.h> > > + > > +#include <asm/pci-bridge.h> > > + > > +struct ioresource { > > + struct list_head list; > > + phys_addr_t start; > > + resource_size_t size; > > +}; > > + > > +static LIST_HEAD(io_list); > > + > > +int pci_register_io_range(phys_addr_t address, resource_size_t size) > > +{ > > + struct ioresource *res; > > + resource_size_t allocated_size = 0; > > + > > + /* find if the range has not been already allocated */ > > + list_for_each_entry(res, &io_list, list) { > > + if (address >= res->start && > > + address + size <= res->start + size) > > + return 0; > > + allocated_size += res->size; > > + } > > + > > + /* range not already registered, check for space */ > > + if (allocated_size + size > IO_SPACE_LIMIT) > > + return -E2BIG; > > + > > + /* add the range in the list */ > > + res = kzalloc(sizeof(*res), GFP_KERNEL); > > + if (!res) > > + return -ENOMEM; > > + res->start = address; > > + res->size = size; > > + > > + list_add_tail(&res->list, &io_list); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(pci_register_io_range); > > + > > +unsigned long pci_address_to_pio(phys_addr_t address) > > +{ > > + struct ioresource *res; > > + > > + list_for_each_entry(res, &io_list, list) { > > + if (address >= res->start && > > + address < res->start + res->size) { > > + return res->start - address; > > + } > > + } > > + > > + return (unsigned long)-1; > > +} > > +EXPORT_SYMBOL_GPL(pci_address_to_pio); > > + > > +/* > > + * Called after each bus is probed, but before its children are examined > > + */ > > +void pcibios_fixup_bus(struct pci_bus *bus) > > +{ > > + struct pci_dev *dev; > > + struct resource *res; > > + int i; > > + > > + if (!pci_is_root_bus(bus)) { > > + pci_read_bridge_bases(bus); > > + > > + pci_bus_for_each_resource(bus, res, i) { > > + if (!res || !res->flags || res->parent) > > + continue; > > + > > + /* > > + * If we are going to reassign everything, we can > > + * shrink the P2P resource to have zero size to > > + * save space > > + */ > > + if (pci_has_flag(PCI_REASSIGN_ALL_RSRC)) { > > + res->flags |= IORESOURCE_UNSET; > > + res->start = 0; > > + res->end = -1; > > + continue; > > + } > > + } > > + } > > Is there any other place we can put this? I'm trying to nuke > pcibios_fixup_bus() because things like hotplug work better if we can do > them in a per-device way rather than a per-bus way. That's good. I'm all for removing this one, will give it a go. > > > + > > + list_for_each_entry(dev, &bus->devices, bus_list) { > > + /* Ignore fully discovered devices */ > > + if (dev->is_added) > > + continue; > > + > > + set_dev_node(&dev->dev, pcibus_to_node(dev->bus)); > > Do you really need this? pci_device_add() already contains the same line. OK. > > > + > > + /* Read default IRQs and fixup if necessary */ > > + dev->irq = of_irq_parse_and_map_pci(dev, 0, 0); > > Could this be done in pcibios_add_device() or someplace similar? Will do. > > > + } > > +} > > +EXPORT_SYMBOL(pcibios_fixup_bus); > > + > > +/* > > + * We don't have to worry about legacy ISA devices, so nothing to do here > > + */ > > +resource_size_t pcibios_align_resource(void *data, const struct resource *res, > > + resource_size_t size, resource_size_t align) > > +{ > > + return res->start; > > +} > > + > > +int pcibios_enable_device(struct pci_dev *dev, int mask) > > +{ > > + return pci_enable_resources(dev, mask); > > +} > > There is already a weak default implementation of pcibios_enable_device() > that does this, so you should be able to just drop this. OK. Thanks for reviewing the series! Liviu > > > +#define IO_SPACE_PAGES ((IO_SPACE_LIMIT + 1) / PAGE_SIZE) > > +static DECLARE_BITMAP(pci_iospace, IO_SPACE_PAGES); > > + > > +unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr) > > +{ > > + unsigned long start, len, virt_start; > > + int err; > > + > > + if (res->end > IO_SPACE_LIMIT) > > + return -EINVAL; > > + > > + /* > > + * try finding free space for the whole size first, > > + * fall back to 64K if not available > > + */ > > + len = resource_size(res); > > + start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES, > > + res->start / PAGE_SIZE, len / PAGE_SIZE, 0); > > + if (start == IO_SPACE_PAGES && len > SZ_64K) { > > + len = SZ_64K; > > + start = 0; > > + start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES, > > + start, len / PAGE_SIZE, 0); > > + } > > + > > + /* no 64K area found */ > > + if (start == IO_SPACE_PAGES) > > + return -ENOMEM; > > + > > + /* ioremap physical aperture to virtual aperture */ > > + virt_start = start * PAGE_SIZE + (unsigned long)PCI_IOBASE; > > + err = ioremap_page_range(virt_start, virt_start + len, > > + phys_addr, __pgprot(PROT_DEVICE_nGnRE)); > > + if (err) > > + return err; > > + > > + bitmap_set(pci_iospace, start, len / PAGE_SIZE); > > + > > + /* return io_offset */ > > + return start * PAGE_SIZE - res->start; > > +} > > -- > > 1.9.0 > > >
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 27bbcfc..d1c8568 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -62,7 +62,7 @@ config MMU def_bool y config NO_IOPORT - def_bool y + def_bool y if !PCI config STACKTRACE_SUPPORT def_bool y @@ -134,6 +134,23 @@ menu "Bus support" config ARM_AMBA bool +config PCI + bool "PCI support" + help + This feature enables support for PCIe bus system. If you say Y + here, the kernel will include drivers and infrastructure code + to support PCIe bus devices. + +config PCI_DOMAINS + def_bool PCI + +config PCI_SYSCALL + def_bool PCI + +source "drivers/pci/Kconfig" +source "drivers/pci/pcie/Kconfig" +source "drivers/pci/hotplug/Kconfig" + endmenu menu "Kernel Features" diff --git a/arch/arm64/include/asm/Kbuild b/arch/arm64/include/asm/Kbuild index 71c53ec..46924bc 100644 --- a/arch/arm64/include/asm/Kbuild +++ b/arch/arm64/include/asm/Kbuild @@ -26,6 +26,7 @@ generic-y += mman.h generic-y += msgbuf.h generic-y += mutex.h generic-y += pci.h +generic-y += pci-bridge.h generic-y += poll.h generic-y += posix_types.h generic-y += resource.h diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h index 7846a6b..67463a5 100644 --- a/arch/arm64/include/asm/io.h +++ b/arch/arm64/include/asm/io.h @@ -120,7 +120,8 @@ static inline u64 __raw_readq(const volatile void __iomem *addr) /* * I/O port access primitives. */ -#define IO_SPACE_LIMIT 0xffff +#define arch_has_dev_port() (1) +#define IO_SPACE_LIMIT 0x1ffffff #define PCI_IOBASE ((void __iomem *)(MODULES_VADDR - SZ_32M)) static inline u8 inb(unsigned long addr) diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h new file mode 100644 index 0000000..d93576f --- /dev/null +++ b/arch/arm64/include/asm/pci.h @@ -0,0 +1,51 @@ +#ifndef __ASM_PCI_H +#define __ASM_PCI_H +#ifdef __KERNEL__ + +#include <linux/types.h> +#include <linux/slab.h> +#include <linux/dma-mapping.h> + +#include <asm/io.h> +#include <asm-generic/pci-bridge.h> +#include <asm-generic/pci-dma-compat.h> + +#define PCIBIOS_MIN_IO 0x1000 +#define PCIBIOS_MIN_MEM 0 + +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus); + +/* + * Set to 1 if the kernel should re-assign all PCI bus numbers + */ +#define pcibios_assign_all_busses() \ + (pci_has_flag(PCI_REASSIGN_ALL_BUS)) + +/* + * PCI address space differs from physical memory address space + */ +#define PCI_DMA_BUS_IS_PHYS (0) + +extern int isa_dma_bridge_buggy; + +#ifdef CONFIG_PCI +static inline int pci_domain_nr(struct pci_bus *bus) +{ + struct pci_host_bridge *bridge = find_pci_host_bridge(bus); + + if (bridge) + return bridge->domain_nr; + + return 0; +} + +static inline int pci_proc_domain(struct pci_bus *bus) +{ + return 1; +} +#endif + +extern unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr); + +#endif /* __KERNEL__ */ +#endif /* __ASM_PCI_H */ diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index 2d4554b..64fc479 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -20,6 +20,7 @@ arm64-obj-$(CONFIG_HAVE_HW_BREAKPOINT)+= hw_breakpoint.o arm64-obj-$(CONFIG_EARLY_PRINTK) += early_printk.o arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND) += sleep.o suspend.o arm64-obj-$(CONFIG_JUMP_LABEL) += jump_label.o +arm64-obj-$(CONFIG_PCI) += pci.o obj-y += $(arm64-obj-y) vdso/ obj-m += $(arm64-obj-m) diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c new file mode 100644 index 0000000..9f29c9a --- /dev/null +++ b/arch/arm64/kernel/pci.c @@ -0,0 +1,173 @@ +/* + * Code borrowed from powerpc/kernel/pci-common.c + * + * Copyright (C) 2003 Anton Blanchard <anton@au.ibm.com>, IBM + * Copyright (C) 2014 ARM Ltd. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + */ + +#include <linux/init.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/mm.h> +#include <linux/of_pci.h> +#include <linux/of_platform.h> +#include <linux/slab.h> + +#include <asm/pci-bridge.h> + +struct ioresource { + struct list_head list; + phys_addr_t start; + resource_size_t size; +}; + +static LIST_HEAD(io_list); + +int pci_register_io_range(phys_addr_t address, resource_size_t size) +{ + struct ioresource *res; + resource_size_t allocated_size = 0; + + /* find if the range has not been already allocated */ + list_for_each_entry(res, &io_list, list) { + if (address >= res->start && + address + size <= res->start + size) + return 0; + allocated_size += res->size; + } + + /* range not already registered, check for space */ + if (allocated_size + size > IO_SPACE_LIMIT) + return -E2BIG; + + /* add the range in the list */ + res = kzalloc(sizeof(*res), GFP_KERNEL); + if (!res) + return -ENOMEM; + res->start = address; + res->size = size; + + list_add_tail(&res->list, &io_list); + + return 0; +} +EXPORT_SYMBOL_GPL(pci_register_io_range); + +unsigned long pci_address_to_pio(phys_addr_t address) +{ + struct ioresource *res; + + list_for_each_entry(res, &io_list, list) { + if (address >= res->start && + address < res->start + res->size) { + return res->start - address; + } + } + + return (unsigned long)-1; +} +EXPORT_SYMBOL_GPL(pci_address_to_pio); + +/* + * Called after each bus is probed, but before its children are examined + */ +void pcibios_fixup_bus(struct pci_bus *bus) +{ + struct pci_dev *dev; + struct resource *res; + int i; + + if (!pci_is_root_bus(bus)) { + pci_read_bridge_bases(bus); + + pci_bus_for_each_resource(bus, res, i) { + if (!res || !res->flags || res->parent) + continue; + + /* + * If we are going to reassign everything, we can + * shrink the P2P resource to have zero size to + * save space + */ + if (pci_has_flag(PCI_REASSIGN_ALL_RSRC)) { + res->flags |= IORESOURCE_UNSET; + res->start = 0; + res->end = -1; + continue; + } + } + } + + list_for_each_entry(dev, &bus->devices, bus_list) { + /* Ignore fully discovered devices */ + if (dev->is_added) + continue; + + set_dev_node(&dev->dev, pcibus_to_node(dev->bus)); + + /* Read default IRQs and fixup if necessary */ + dev->irq = of_irq_parse_and_map_pci(dev, 0, 0); + } +} +EXPORT_SYMBOL(pcibios_fixup_bus); + +/* + * We don't have to worry about legacy ISA devices, so nothing to do here + */ +resource_size_t pcibios_align_resource(void *data, const struct resource *res, + resource_size_t size, resource_size_t align) +{ + return res->start; +} + +int pcibios_enable_device(struct pci_dev *dev, int mask) +{ + return pci_enable_resources(dev, mask); +} + +#define IO_SPACE_PAGES ((IO_SPACE_LIMIT + 1) / PAGE_SIZE) +static DECLARE_BITMAP(pci_iospace, IO_SPACE_PAGES); + +unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr) +{ + unsigned long start, len, virt_start; + int err; + + if (res->end > IO_SPACE_LIMIT) + return -EINVAL; + + /* + * try finding free space for the whole size first, + * fall back to 64K if not available + */ + len = resource_size(res); + start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES, + res->start / PAGE_SIZE, len / PAGE_SIZE, 0); + if (start == IO_SPACE_PAGES && len > SZ_64K) { + len = SZ_64K; + start = 0; + start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES, + start, len / PAGE_SIZE, 0); + } + + /* no 64K area found */ + if (start == IO_SPACE_PAGES) + return -ENOMEM; + + /* ioremap physical aperture to virtual aperture */ + virt_start = start * PAGE_SIZE + (unsigned long)PCI_IOBASE; + err = ioremap_page_range(virt_start, virt_start + len, + phys_addr, __pgprot(PROT_DEVICE_nGnRE)); + if (err) + return err; + + bitmap_set(pci_iospace, start, len / PAGE_SIZE); + + /* return io_offset */ + return start * PAGE_SIZE - res->start; +}