Merge 32 and 64 bit pci_process_bridge_OF_ranges() instances
RFC PATCH 1/1 Merge 32 and 64 bit pci_process_bridge_OF_ranges() instances
We are having 2 different instances of pci_process_bridge_OF_ranges(), which makes describing 64-bit physical addresses in non PPC64 case impossible.
This approach inherits pci space parsing, but has a new way to behave equally good in both 32bit and 64bit environments. This approach uses of_translate_address(), so implies proper ranges <> definition in devicetree, where PCI node has its reg on soc bus, and its ranges effectively belong to LAW addresses.
---
arch/powerpc/kernel/pci-common.c | 130 ++++++++++++++++++++++++++++++++++++++ arch/powerpc/kernel/pci_32.c | 114 --------------------------------- arch/powerpc/kernel/pci_64.c | 94 --------------------------- include/asm-powerpc/ppc-pci.h | 7 ++ 4 files changed, 137 insertions(+), 208 deletions(-)
arch/powerpc/kernel/pci-common.c +130 -0
@@ -478,3 +478,133 @@ void pci_resource_to_user(const struct pci_dev *dev, int bar, *start = rsrc->start - offset; *end = rsrc->end - offset; } + +void __devinit pci_process_bridge_OF_ranges (struct pci_controller *hose, +struct device_node *dev,int prim) +{ +const unsigned int *ranges; +unsigned int pci_space; +u64 size =0 , pci_addr; +int rlen =0 ; +int ranges_amnt, i, j, np; +int memno =0 ; +struct resource *res; +int na =of_n_addr_cells (dev); +struct ranges_pci *ranges64 =NULL ; +phys_addr_t cpu_phys_addr; + + np = na +5 ; + +/* From "PCI Binding to 1275" +* The ranges property is laid out as an array of elements, +* each of which comprises: +* cells 0 - 2: a PCI address +* cells 3 or 3+4: a CPU physical address +* (size depending on dev->n_addr_cells) +* cells 4+5 or 5+6: the size of the range +*/ + ranges =of_get_property (dev,"ranges" , &rlen); +if (!ranges) +return ; +/* Map ranges to struct according to spec. */ + ranges64 = (void *)ranges; + ranges_amnt = rlen /sizeof (*ranges64); + + hose->io_base_phys =0 ; +for (i =0 ; i < ranges_amnt; i++) { +u32 *addr_ptr; + res =NULL ; + +if (ranges64[i].pci_space ==0 ) +continue ; + + pci_space = ranges64[i].pci_space; + pci_addr = ranges64[i].pci_addr; + addr_ptr = (u32 *)(&ranges64[i].phys_addr); + cpu_phys_addr = + (phys_addr_t )of_translate_address (dev, addr_ptr); + size = ranges64[i].size; + +DBG ("Observed: pci %llx phys %llx size %llx\n" , pci_addr, + cpu_phys_addr, size); + +/* here we need to handle contiguous ranges */ +for (j = i+1 ; j < ranges_amnt; j++) { +/* no need to coalesce different region types */ +if (ranges64[j].pci_space != pci_space) +break ; +/* not contiguous. give up. */ +if (ranges64[j].pci_addr != pci_addr + size || + ranges64[j].phys_addr != cpu_phys_addr + size) +break ; + size += ranges64[j].size; + i = j;/* skip this one next turn */ + } +switch ((pci_space >>24 ) &0x3 ) { +case 1 :/* I/O space */ +#ifdef CONFIG_PPC32 +/* +* check from ppc32 pci implementation. +* This seems just wrong. -vitb +*/ +if (pci_addr !=0 ) +break ; +#endif +/* limit I/O space to 16MB */ +if (size >0x01000000 ) + size =0x01000000 ; + + hose->io_base_phys = cpu_phys_addr - pci_addr; +/* handle from 0 to top of I/O window */ +#ifdef CONFIG_PPC64 + hose->pci_io_size = pci_addr + size; +#endif + hose->io_base_virt =ioremap (hose->io_base_phys, size); +#ifdef CONFIG_PPC32 +if (prim) + isa_io_base = (unsigned long )hose->io_base_virt; +#endif + res = &hose->io_resource; + res->flags =IORESOURCE_IO ; + res->start = pci_addr; +DBG ("phb%d: IO 0x%llx -> 0x%llx\n" , hose->global_number, + (u64 ) res->start, + (u64) (res->start + size -1 )); +DBG ("IO phys %llx IO virt %p\n" , + (u64 ) hose->io_base_phys, hose->io_base_virt); +break ; +case 2 :/* memory space */ + memno =0 ; +#ifdef CONFIG_PPC32 +if ((pci_addr ==0 ) && (size <= (16 <<20 ))) { +/* 1st 16MB, i.e. ISA memory area */ +if (prim) + isa_mem_base = cpu_phys_addr; + memno =1 ; + } +#endif +while (memno <3 && hose->mem_resources[memno].flags) + ++memno; + +if (memno ==0 ) + hose->pci_mem_offset = cpu_phys_addr - pci_addr; +if (memno <3 ) { + res = &hose->mem_resources[memno]; + res->flags =IORESOURCE_MEM ; + res->start = cpu_phys_addr; +DBG ("phb%d: MEM 0x%llx -> 0x%llx\n" , + hose->global_number, res->start, + res->start + size -1 ); + } +break ; + } +if (res !=NULL ) { + res->name = dev->full_name; + res->end = res->start + size -1 ; + res->parent =NULL ; + res->sibling =NULL ; + res->child =NULL ; + } + } +} +
arch/powerpc/kernel/pci_32.c +0 -114
120 lines not on this page · raw message
arch/powerpc/kernel/pci_64.c +0 -94
100 lines not on this page · raw message
include/asm-powerpc/ppc-pci.h +7 -0
@@ -15,6 +15,13 @@#include <linux/pci.h> #include <asm/pci-bridge.h> +struct ranges_pci { +unsigned int pci_space; +u64 pci_addr; +phys_addr_t phys_addr; +u64 size; +} __attribute__((packed)); +extern unsigned long isa_io_base;extern void pci_setup_phb_io (struct pci_controller *hose,int primary);
Quoted 5 lines · Vitaly Bordug
We are having 2 different instances of
pci_process_bridge_OF_ranges(), which makes describing 64-bit physical addresses in non PPC64 case impossible.
Quoted 8 lines
This approach inherits pci space parsing, but has a new way to behave equally good in both 32bit and 64bit environments. This approach uses
of_translate_address(), so implies proper ranges <> definition in devicetree, where PCI node has its reg on soc bus, and its ranges effectively belong to LAW addresses.Signed-off-by Vitaly Bordug <[email protected]>Signed-off-by Stefan Roese <[email protected]>
The patch looks really good, but it's hard to review when you move the code around at the same as you change it.
Could you perhaps split the patch into two separate changesets, one that makes both functions identical in place, and one that merges them to live in a common location?
Quoted 5 lines
include/asm-powerpc/ppc-pci.h +0
-0
include/asm-powerpc/ppc-pci.h line 24
Quoted 10 lines
include/asm-powerpc/ppc-pci.h +7
-0@@ -15,6 +15,13 @@#include <linux/pci.h> #include <asm/pci-bridge.h> +struct ranges_pci { +unsigned int pci_space; +u64 pci_addr; +phys_addr_t phys_addr; +u64 size; +} __attribute__((packed)); +
This structure definition uses unaligned members because of the 'packed' attribute. Is that really what you intended?
Arnd <><
Quoted 12 lines · On Wed, 12 Sep 2007 00:57:17 +0200 Arnd Bergmann wrote:
Quoted 11 lines · Vitaly Bordug
We are having 2 different instances of
pci_process_bridge_OF_ranges(), which makes describing 64-bit physical addresses in non PPC64 case impossible.This approach inherits pci space parsing, but has a new way to behave equally good in both 32bit and 64bit environments. This approach uses
of_translate_address(), so implies proper ranges <> definition in devicetree, where PCI node has its reg on soc bus, and its ranges effectively belong to LAW addresses.
Quoted 6 lines
Quoted 2 lines
Signed-off-by Vitaly Bordug <[email protected]>Signed-off-by Stefan Roese <[email protected]>The patch looks really good, but it's hard to review when you move the code around at the same as you change it.
Well, it's more a rewrite than a move, based on 64-bit implementation.
Quoted 4 lines
Could you perhaps split the patch into two separate changesets, one that makes both functions identical in place, and one that merges them to live in a common location?
I'm not sure I'm following what you are requesting. What is a benefit of code duplication? I was thinking about,
if it will look good enough, to provide this function at generic level but changing its name a little, while leaving old stuff in place, and encouraging people to use it in favour of 32 or 64-bit-specific approaches. That way we won't kill many boards at once(in case, for example,odd dts with missed ranges for pci subnode).
Quoted 8 lines
Quoted 8 lines
(quoted diff) +0
-0
b/include/asm-powerpc/ppc-pci.hindex b847aa1..882b8bc 100644include/asm-powerpc/ppc-pci.h +0
-0@@ -15,6 +15,13 @@#include <linux/pci.h> #include <asm/pci-bridge.h>
Quoted 11 lines
include/asm-powerpc/ppc-pci.h line 24
Quoted 7 lines
include/asm-powerpc/ppc-pci.h +7
-0+struct ranges_pci { +unsigned int pci_space; +u64 pci_addr; +phys_addr_t phys_addr; +u64 size; +} __attribute__((packed)); +This structure definition uses unaligned members because of the 'packed' attribute. Is that really what you intended?
yes, exactly, because I'm mapping this struct on ranges extracted from the dts instead of juggling with ranges[foo] offsets.
Quoted 1 line
Arnd <><
Thanks for looking at it!
Signature · 1 line
Sincerely, Vitaly
Quoted 2 lines · Vitaly Bordug
Well, it's more a rewrite than a move, based on 64-bit implementation.
ok.
Quoted 11 lines
Quoted 4 lines
Could you perhaps split the patch into two separate changesets, one that makes both functions identical in place, and one that merges them to live in a common location?
I'm not sure I'm following what you are requesting. What is a benefit of code duplication? I was thinking about, if it will look good enough, to provide this function at generic level but changing its name a little, while leaving old stuff in place, and encouraging people to use it in favour of 32 or 64-bit-specific approaches. That way we won't kill many boards at
once(in case, for example,odd dts with missed ranges for pci subnode).
I wasn't suggesting to leave the duplicated code in, but rather to make the review easier by first modifying the code in place.
If you're taking the 64 bit code as a base, you can for instance make the first patch leave pci_32 alone, and modify the 64 bit pci_process_bridge_OF_ranges to look exactly like the merged version. That allows us to see what changed in the 64 bit case.
The second patch would then move the functions over, but leave the code identical to the result of the first patch.
Quoted 19 lines
Quoted 8 lines
Quoted 8 lines
(quoted diff) +0
-0
b/include/asm-powerpc/ppc-pci.hindex b847aa1..882b8bc 100644include/asm-powerpc/ppc-pci.h +0
-0@@ -15,6 +15,13 @@#include <linux/pci.h> #include <asm/pci-bridge.h> Quoted 11 lines
include/asm-powerpc/ppc-pci.h line 24
Quoted 7 lines
include/asm-powerpc/ppc-pci.h +7
-0+struct ranges_pci { +unsigned int pci_space; +u64 pci_addr; +phys_addr_t phys_addr; +u64 size; +} __attribute__((packed)); +This structure definition uses unaligned members because of the 'packed' attribute. Is that really what you intended?
Quoted 2 lines
yes, exactly, because I'm mapping this struct on ranges extracted from the dts instead of juggling with ranges[foo] offsets.
I see. It does however look wrong to me, because you are using a hardcoded phys_addr_t type. This breaks when phys_addr has a different size from what you expect, e.g. when booting a pure 32 bit kernel on a machine that has a 64 bit physical address space.
Arnd <><
Quoted 23 lines · On Wed, 12 Sep 2007 10:13:50 +0200 Arnd Bergmann wrote:
Quoted 3 lines · Vitaly Bordug
Well, it's more a rewrite than a move, based on 64-bit implementation.
ok.
Quoted 11 lines
Quoted 4 lines
Could you perhaps split the patch into two separate changesets, one that makes both functions identical in place, and one that merges them to live in a common location?
I'm not sure I'm following what you are requesting. What is a benefit of code duplication? I was thinking about, if it will look good enough, to provide this function at generic level but changing its name a little, while leaving old stuff in place, and encouraging people to use it in favour of 32 or 64-bit-specific approaches. That way we won't kill many boards at
once(in case, for example,odd dts with missed ranges for pci subnode).I wasn't suggesting to leave the duplicated code in, but rather to make the review easier by first modifying the code in place.
Quoted 8 lines
If you're taking the 64 bit code as a base, you can for instance make the first patch leave
pci_32alone, and modify the 64 bitpci_process_bridge_OF_rangesto look exactly like the merged version. That allows us to see what changed in the 64 bit case.The second patch would then move the functions over, but leave the code identical to the result of the first patch.
ok, makes sense, will do it that way.
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
Quoted 5 lines
(quoted diff) +0
-0
b/include/asm-powerpc/ppc-pci.hindex b847aa1..882b8bc 100644include/asm-powerpc/ppc-pci.h line 24
Quoted 10 lines
include/asm-powerpc/ppc-pci.h +7
-0@@ -15,6 +15,13 @@#include <linux/pci.h> #include <asm/pci-bridge.h> +struct ranges_pci { +unsigned int pci_space; +u64 pci_addr; +phys_addr_t phys_addr; +u64 size; +} __attribute__((packed)); +This structure definition uses unaligned members because of the 'packed' attribute. Is that really what you intended?
Quoted 8 lines
Quoted 2 lines
yes, exactly, because I'm mapping this struct on ranges extracted from the dts instead of juggling with ranges[foo] offsets.
I see. It does however look wrong to me, because you are using a hardcoded
phys_addr_ttype. This breaks whenphys_addrhas a different size from what you expect, e.g. when booting a pure 32 bit kernel on a machine that has a 64 bit physical address space.
I wondered around with "32 bit phys" and "64 bit phys" struct definitions first, but, well, it does not look good. In fact it already verified with alike case (on 4xx), and I thought it would be fair tradeoff to have 64 bit ranges definition.
otoh, there might be cases when phys_addr_t is u64 and pci stuff resides on some 32-bit SoC bus. I will try to address that next iteration.
Signature · 1 line
Sincerely, Vitaly
Quoted 63 lines · Vitaly Bordug
Quoted 23 lines · On Wed, 12 Sep 2007 10:13:50 +0200 Arnd Bergmann wrote:
Quoted 3 lines · Vitaly Bordug
Well, it's more a rewrite than a move, based on 64-bit implementation.
ok.
Quoted 11 lines
Quoted 4 lines
Could you perhaps split the patch into two separate changesets, one that makes both functions identical in place, and one that merges them to live in a common location?
I'm not sure I'm following what you are requesting. What is a benefit of code duplication? I was thinking about, if it will look good enough, to provide this function at generic level but changing its name a little, while leaving old stuff in place, and encouraging people to use it in favour of 32 or 64-bit-specific approaches. That way we won't kill many boards at
once(in case, for example,odd dts with missed ranges for pci subnode).I wasn't suggesting to leave the duplicated code in, but rather to make the review easier by first modifying the code in place.
Quoted 7 lines
If you're taking the 64 bit code as a base, you can for instance make the first patch leave
pci_32alone, and modify the 64 bitpci_process_bridge_OF_rangesto look exactly like the merged version. That allows us to see what changed in the 64 bit case.The second patch would then move the functions over, but leave the code identical to the result of the first patch.
ok, makes sense, will do it that way.
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
Quoted 5 lines
(quoted diff) +0
-0
b/include/asm-powerpc/ppc-pci.hindex b847aa1..882b8bc 100644include/asm-powerpc/ppc-pci.h line 24
Quoted 10 lines
include/asm-powerpc/ppc-pci.h +7
-0@@ -15,6 +15,13 @@#include <linux/pci.h> #include <asm/pci-bridge.h> +struct ranges_pci { +unsigned int pci_space; +u64 pci_addr; +phys_addr_t phys_addr; +u64 size; +} __attribute__((packed)); +This structure definition uses unaligned members because of the 'packed' attribute. Is that really what you intended?
Quoted 8 lines
Quoted 2 lines
yes, exactly, because I'm mapping this struct on ranges extracted from the dts instead of juggling with ranges[foo] offsets.
I see. It does however look wrong to me, because you are using a hardcoded
phys_addr_ttype. This breaks whenphys_addrhas a different size from what you expect, e.g. when booting a pure 32 bit kernel on a machine that has a 64 bit physical address space.
Quoted 8 lines
I wondered around with "32 bit phys" and "64 bit phys" struct definitions first, but, well, it does not look good. In fact it already verified with alike case (on 4xx), and I thought it would be fair tradeoff to have 64 bit ranges definition.
otoh, there might be cases when
phys_addr_tis u64 and pci stuff resides on some 32-bit SoC bus. I will try to address that next iteration.
Yes, I was going to point out this sort of case. I don't think including the parent-address in the structure is going to work.
Oh, also, since this structure is only used in this function, it should go in the .c file, not a .h file.
Signature · 4 lines
David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
include/asm-powerpc/ppc-pci.h line 24
Quoted 7 lines
include/asm-powerpc/ppc-pci.h +7
-0+struct ranges_pci { +unsigned int pci_space; +u64 pci_addr; +phys_addr_t phys_addr; +u64 size; +} __attribute__((packed)); +This structure definition uses unaligned members because of the 'packed' attribute. Is that really what you intended?
Quoted 7 lines
Quoted 2 lines
yes, exactly, because I'm mapping this struct on ranges extracted from the dts instead of juggling with ranges[foo] offsets.
I see. It does however look wrong to me, because you are using a hardcoded
phys_addr_ttype. This breaks whenphys_addrhas a different size from what you expect, e.g. when booting a pure 32 bit kernel on a machine that has a 64 bit physical address space.
More generally, you can even have a different size for the "phys_addr" for different nodes in the same device tree.
You really should look at the #address-cells in this node's parent, and translate that all the way up to the root node to get a CPU address.
Segher
Review
Quoted 2 lines · Vitaly Bordug
(quoted diff) +2
-0+ + hose->io_base_phys = cpu_phys_addr - pci_addr;
This is not gonna work on 32-bit platform (unless pci_addr == 0).
Should behose->io_base_phys = cpu_phys_addr;
The other way we should adjust io size like we do on 64-bit and rewrite resource fixup functions.
arch/powerpc/kernel/pci-common.c void pci_resource_to_user(const struct pci_dev *dev, int bar, · line 562
Quoted 5 lines
arch/powerpc/kernel/pci-common.c +5
-0+/* handle from 0 to top of I/O window */ +#ifdef CONFIG_PPC64 + hose->pci_io_size = pci_addr + size; +#endif + hose->io_base_virt =ioremap (hose->io_base_phys, size);
Do we need to ioremap on 64-bit? I think 64-bit uses different approach in handling io space.
arch/powerpc/kernel/pci-common.c void pci_resource_to_user(const struct pci_dev *dev, int bar, · line 566
Quoted 4 lines
arch/powerpc/kernel/pci-common.c +4
-0+#ifdef CONFIG_PPC32 +if (prim) + isa_io_base = (unsigned long )hose->io_base_virt; +#endif
What's exactly the point of merging this single function?
If this was intended to add 64-bit physical address support to 32-bit platforms (like ppc440), then it does not seem sufficient to make pci stuff work right. What about the 32-bit pcibios_fixup_resources(), pcibios_resource_to_bus(), pcibios_bus_to_resource() stuff?
There's still no 64-bit support. You'll get
"PCI Cannot allocate resource region" attempting to initialize resources at PCI start-up.
Thanks,
Valentine.
Hello Valentine,
Quoted 10 lines · On Tue, 18 Sep 2007 16:03:50 +0400 Valentine Barshak wrote:
Review
Quoted 2 lines · Vitaly Bordug
(quoted diff) +2
-0+ + hose->io_base_phys = cpu_phys_addr - pci_addr;This is not gonna work on 32-bit platform (unless
pci_addr== 0). Should behose->io_base_phys=cpu_phys_addr;
The other way we should adjust io size like we do on 64-bit and rewrite resource fixup functions.
I think the second sounds like a plan then. We should not "assume" where we can make things to behave properly.
Quoted 9 lines
arch/powerpc/kernel/pci-common.c void pci_resource_to_user(const struct pci_dev *dev, int bar, · line 562
Quoted 5 lines
arch/powerpc/kernel/pci-common.c +5
-0+/* handle from 0 to top of I/O window */ +#ifdef CONFIG_PPC64 + hose->pci_io_size = pci_addr + size; +#endif + hose->io_base_virt =ioremap (hose->io_base_phys, size);Do we need to ioremap on 64-bit? I think 64-bit uses different approach in handling io space.
Correct - 64 bit code does __get_vm_area() so the upper line is ppc32-specific.
Quoted 6 lines
arch/powerpc/kernel/pci-common.c void pci_resource_to_user(const struct pci_dev *dev, int bar, · line 566
Quoted 4 lines
arch/powerpc/kernel/pci-common.c +4
-0+#ifdef CONFIG_PPC32 +if (prim) + isa_io_base = (unsigned long )hose->io_base_virt; +#endif What's exactly the point of merging this single function?
Quoted 8 lines
If this was intended to add 64-bit physical address support to 32-bit platforms (like ppc440), then it does not seem sufficient to make pci stuff work right.
What about the 32-bitpcibios_fixup_resources(),pcibios_resource_to_bus(),pcibios_bus_to_resource()stuff? There's still no 64-bit support. You'll get
"PCI Cannot allocate resource region" attempting to initialize resources at PCI start-up.
Because this function is critical to get the stuff going. You can have some devices working even without PCI resources initted
(what in fact I had with mpc8272). I am in process of moving the mentioned to the common_... domain but I suspect there are other 32-bit-stick stuff still. So if you want to assist in cleaning this up, feel free to join the club...
Signature · 1 line
Sincerely, Vitaly
Quoted 32 lines · Vitaly Bordug
Hello Valentine,
Quoted 8 lines · On Tue, 18 Sep 2007 16:03:50 +0400 Valentine Barshak wrote:
Review
Quoted 2 lines · Vitaly Bordug
(quoted diff) +2
-0+ + hose->io_base_phys = cpu_phys_addr - pci_addr;This is not gonna work on 32-bit platform (unless
pci_addr== 0).
Should behose->io_base_phys=cpu_phys_addr;
The other way we should adjust io size like we do on 64-bit and rewrite resource fixup functions.I think the second sounds like a plan then. We should not "assume" where we can make things to behave properly.
Quoted 7 lines
arch/powerpc/kernel/pci-common.c void pci_resource_to_user(const struct pci_dev *dev, int bar, · line 562
Quoted 5 lines
arch/powerpc/kernel/pci-common.c +5
-0+/* handle from 0 to top of I/O window */ +#ifdef CONFIG_PPC64 + hose->pci_io_size = pci_addr + size; +#endif + hose->io_base_virt =ioremap (hose->io_base_phys, size);Do we need to ioremap on 64-bit? I think 64-bit uses different approach in handling io space.
Correct - 64 bit code does
__get_vm_area()so the upper line is ppc32-specific.Quoted 9 lines
arch/powerpc/kernel/pci-common.c void pci_resource_to_user(const struct pci_dev *dev, int bar, · line 566
Quoted 4 lines
arch/powerpc/kernel/pci-common.c +4
-0+#ifdef CONFIG_PPC32 +if (prim) + isa_io_base = (unsigned long )hose->io_base_virt; +#endif What's exactly the point of merging this single function?
If this was intended to add 64-bit physical address support to 32-bit platforms (like ppc440), then it does not seem sufficient to make pci stuff work right. What about the 32-bitpcibios_fixup_resources(),pcibios_resource_to_bus(),pcibios_bus_to_resource()stuff?
There's still no 64-bit support. You'll get
"PCI Cannot allocate resource region" attempting to initialize resources at PCI start-up.
Quoted 2 lines
Because this function is critical to get the stuff going. You can have some devices working even without PCI resources initted
(what in fact I had with mpc8272). I am in process of moving the mentioned to thecommon_... domain but I suspect there are other 32-bit-stick stuff still. So if you want to assist in cleaning this up, feel free to join the club...
Well, thanks for the invitation :)
I've been trying to work on the pci support here too. But I've used a bit different approach. I tried to add 64-bit phys addr support to pci_32.c. The patch seems to work on Sequoia. Actually, I thought merging was too risky at this point and might cause more problems breaking both 64 and 32-bit pci support. Thanks,
Valentine.
Hello Valentine,
Quoted 6 lines · On Tue, 18 Sep 2007 18:38:56 +0400 Valentine Barshak wrote:
Well, thanks for the invitation :)
I've been trying to work on the pci support here too. But I've used a bit different approach. I tried to add 64-bit phys addr support topci_32.c. The patch seems to work on Sequoia. Actually, I thought merging was too risky at this point and might cause more problems breaking both 64 and 32-bit pci support.
In order not to break, we just need same functionality and it is not that complex.
I began with the upper approach as well, but that seems more like a workaround, than a step forward.
Signature · 1 line
Sincerely, Vitaly
Quoted 3 lines · Valentine Barshak
Do we need to ioremap on 64-bit? I think 64-bit uses different approach
in handling io space.
Yup, we changed that recently and I haven't yet modified ppc32 to catch up (and may never do so, they have different constraints).
Ben.
Quoted 9 lines · Vitaly Bordug
We are having 2 different instances of
pci_process_bridge_OF_ranges(), which makes describing 64-bit physical addresses in non PPC64 case impossible.This approach inherits pci space parsing, but has a new way to behave equally good in both 32bit and 64bit environments. This approach uses
of_translate_address(), so implies proper ranges <> definition in devicetree, where PCI node has its reg on soc bus, and its ranges effectively belong to LAW addresses.
Sorry for the delay, I was travelling.
NAK the bits with struct ranges_pci. Don't map structures to those DT entries. Use a "cursor" pointer and some getter function, like prom_parse does (maybe export the ones in prom_parse).
Ben.
Oh and...
PowerMac -needs- the coalescing of ranges. Apple tends to split their ranges in bits in the device-tree and ends up with more than you have struct resource in the pci_bus.
Ben.