| Message ID | 1393948204-11555-2-git-send-email-Liviu.Dudau@arm.com |
|---|---|
| State | Deferred |
| Headers | show |
On Tuesday 04 March 2014, Liviu Dudau wrote: > +int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) > +{ > + return 0; > +} > + How about returning an error here? You don't actually register the range. 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 Tue, Mar 04, 2014 at 10:30:09PM +0000, Arnd Bergmann wrote: > On Tuesday 04 March 2014, Liviu Dudau wrote: > > +int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) > > +{ > > + return 0; > > +} > > + > > How about returning an error here? You don't actually register the range. That's not the intention here. I basically want a nop, as by default (read x86) we do nothing with the IO range. Best regards, Liviu > > Arnd >
On Thursday 06 March 2014, Liviu Dudau wrote: > On Tue, Mar 04, 2014 at 10:30:09PM +0000, Arnd Bergmann wrote: > > On Tuesday 04 March 2014, Liviu Dudau wrote: > > > +int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) > > > +{ > > > + return 0; > > > +} > > > + > > > > How about returning an error here? You don't actually register the range. > > That's not the intention here. I basically want a nop, as by default (read x86) > we do nothing with the IO range. I think x86 is a bad default though, because that is the exception rather than the rule. I also think that on x86, you shouldn't have an entry for the I/O space in the "ranges" property since there is no translation, and then we don't call this function. PCI devices described in DT on x86 would still be able to list their I/O BARs in DT, but you don't ever translate them into MMIO ranges. 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 Fri, Mar 07, 2014 at 01:24:12AM +0100, Arnd Bergmann wrote: > On Thursday 06 March 2014, Liviu Dudau wrote: > > On Tue, Mar 04, 2014 at 10:30:09PM +0000, Arnd Bergmann wrote: > > > On Tuesday 04 March 2014, Liviu Dudau wrote: > > > > +int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) > > > > +{ > > > > + return 0; > > > > +} > > > > + > > > > > > How about returning an error here? You don't actually register the range. > > > > That's not the intention here. I basically want a nop, as by default (read x86) > > we do nothing with the IO range. > > I think x86 is a bad default though, because that is the exception rather than > the rule. I also think that on x86, you shouldn't have an entry for the I/O > space in the "ranges" property since there is no translation, and then we don't > call this function. > > PCI devices described in DT on x86 would still be able to list their I/O BARs > in DT, but you don't ever translate them into MMIO ranges. So, if I understand you correctly, you would prefer to fail here and hence stop the parsing for the x86, rather than pretending everything is OK and going through the motions? That was not my original thinking when I've introduced this function here. The main purpose of the function is to help the correct translation of IO addresses in pci_address_to_pio(). As Jason has explained very nicely, we have 3 types of architectures here that we try to support: - the ones that have separate IO address space (x86) - the ones that have 1:1 mapping between physical IO addresses and logical ports - the architectures that memory map the IO addresses in virtual address space and then translate the logical addresses into virtual based on a given offset. For the first two types we don't want to do anything special. Architectures that fall in the last category will have to provide their own version of this function, with the arm64 version being generic enough to be used as de facto? But I can see your point of view as well. I just don't know if that is good enough for powerpc and microblaze. With the way things are in my patch, they should be able to switch to of_create_pci_host_bridge() easily*, with your suggestion they will have to provide their implementation for pci_register_io_range(). We really need to get another architecture converted. If there are no other takers I will make a stab once the current push towards upstreaming AArch64 hardware support slows down. * says the newby with confidence in his voice ;) Liviu > > 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 > -- 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 Fri, Mar 07, 2014 at 12:24:12AM +0000, Arnd Bergmann wrote: > On Thursday 06 March 2014, Liviu Dudau wrote: > > On Tue, Mar 04, 2014 at 10:30:09PM +0000, Arnd Bergmann wrote: > > > On Tuesday 04 March 2014, Liviu Dudau wrote: > > > > +int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) > > > > +{ > > > > + return 0; > > > > +} > > > > + > > > > > > How about returning an error here? You don't actually register the range. > > > > That's not the intention here. I basically want a nop, as by default (read x86) > > we do nothing with the IO range. > > I think x86 is a bad default though, because that is the exception rather than > the rule. I also think that on x86, you shouldn't have an entry for the I/O > space in the "ranges" property since there is no translation, and then we don't > call this function. > > PCI devices described in DT on x86 would still be able to list their I/O BARs > in DT, but you don't ever translate them into MMIO ranges. > > Arnd > So, if I understand you correctly, you would prefer to fail here and hence stop the parsing for the x86, rather than pretending everything is OK and going through the motions? That was not my original thinking when I've introduced this function here. The main purpose of the function is to help the correct translation of IO addresses in pci_address_to_pio(). As Jason has explained very nicely, we have 3 types of architectures here that we try to support: - the ones that have separate IO address space (x86) - the ones that have 1:1 mapping between physical IO addresses and logical ports - the architectures that memory map the IO addresses in virtual address space and then translate the logical addresses into virtual based on a given offset. For the first two types we don't want to do anything special. Architectures that fall in the last category will have to provide their own version of this function, with the arm64 version being generic enough to be used as de facto? But I can see your point of view as well. I just don't know if that is good enough for powerpc and microblaze. With the way things are in my patch, they should be able to switch to of_create_pci_host_bridge() easily*, with your suggestion they will have to provide their implementation for pci_register_io_range(). We really need to get another architecture converted. If there are no other takers I will make a stab once the current push towards upstreaming AArch64 hardware support slows down. * says the newby with confidence in his voice ;) Liviu
On Monday 10 March 2014 14:45:19 Liviu Dudau wrote: > > So, if I understand you correctly, you would prefer to fail here and hence stop the > parsing for the x86, rather than pretending everything is OK and going through the > motions? Yes, on x86 it is clearly a bug if we end up calling this function. > That was not my original thinking when I've introduced this function here. The main > purpose of the function is to help the correct translation of IO addresses in > pci_address_to_pio(). As Jason has explained very nicely, we have 3 types of > architectures here that we try to support: > - the ones that have separate IO address space (x86) > - the ones that have 1:1 mapping between physical IO addresses and logical ports Still not convinced this second one actually exists. > - the architectures that memory map the IO addresses in virtual address space > and then translate the logical addresses into virtual based on a given offset. > > For the first two types we don't want to do anything special. Architectures that > fall in the last category will have to provide their own version of this function, > with the arm64 version being generic enough to be used as de facto? The page flags might be different across architectures: arm32 currently uses MT_DEVICE, which only exists on arm32 and derived architectures (unicore32, arm64). > But I can see your point of view as well. I just don't know if that is good enough > for powerpc and microblaze. With the way things are in my patch, they should be able > to switch to of_create_pci_host_bridge() easily*, with your suggestion they will > have to provide their implementation for pci_register_io_range(). I think there would still be a lot to do have powerpc switch over to of_create_pci_host_bridge(), the more likely thing to happen is to have that architecture implement its own copy that calls the same internal helpers and does some more things that we may not want on other architectures. Microblaze can probably be changed to use of_create_pci_host_bridge() and need no custom code at all, it should need only a subset of what we need for arm64. > We really need to get another architecture converted. If there are no other takers I > will make a stab once the current push towards upstreaming AArch64 hardware support > slows down. Moving over microblaze I think would be a good start. It has rather specific requirements since there is only one host driver, but then again that PCI host implementation might be shared with zynq (or synthesizable there). I also wonder whether it's actually related to the X-gene PCI, since I know some of the ppc4xx use Xilinx PCI and X-gene is also based on those. 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
diff --git a/drivers/of/address.c b/drivers/of/address.c index 1a54f1f..d1bb30f 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -619,6 +619,11 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size, } EXPORT_SYMBOL(of_get_address); +int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) +{ + return 0; +} + unsigned long __weak pci_address_to_pio(phys_addr_t address) { if (address > IO_SPACE_LIMIT) diff --git a/include/linux/of_address.h b/include/linux/of_address.h index 5f6ed6b..40c418d 100644 --- a/include/linux/of_address.h +++ b/include/linux/of_address.h @@ -56,6 +56,7 @@ extern void __iomem *of_iomap(struct device_node *device, int index); extern const __be32 *of_get_address(struct device_node *dev, int index, u64 *size, unsigned int *flags); +extern int pci_register_io_range(phys_addr_t addr, resource_size_t size); extern unsigned long pci_address_to_pio(phys_addr_t addr); extern int of_pci_range_parser_init(struct of_pci_range_parser *parser,
Some architectures do not share x86 simple view of the I/O space and instead use a range of addresses that map to external devices. For PCI, these ranges can be expressed by OF bindings in a device tree file. Introduce a pci_register_io_range() helper function that can be used by the architecture code to keep track of the io ranges described by the PCI bindings. Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>