Hello Valentine,
On Tue, 18 Sep 2007 20:07:54 +0400
Valentine Barshak wrote:
> Currently pci_32 doesn't support 64-bit physical addresses, while
> PowerPC440 platform has PCI space typically mapped above 4GB range.
> The patch adds 64-bit physical address support to 32-bit PCI code
> in order to bring-up PCI on 44x platform.
>
The approach looks interesting, and resource_size_t<>ulong apparently makes
sense,
but I would strongly disagree we need to add more nuts and bolts into ranges
parse func:
> Signed-off-by: Valentine Barshak <[EMAIL PROTECTED]>
[...]
> void __init
> pci_process_bridge_OF_ranges(struct pci_controller *hose,
> struct device_node *dev, int primary)
> {
> static unsigned int static_lc_ranges[256] __initdata;
> const unsigned int *dt_ranges;
> - unsigned int *lc_ranges, *ranges, *prev, size;
> + unsigned int *lc_ranges, *ranges, *prev;
> int rlen = 0, orig_rlen;
> int memno = 0;
> struct resource *res;
> + u32 prev_pci_space, pci_space;
> + u64 prev_pci_addr, pci_addr;
> + u64 prev_size, size;
> + phys_addr_t cpu_phys_addr;
> +
> int np, na = of_n_addr_cells(dev);
> np = na + 5;
>
> @@ -879,11 +891,18 @@
> prev = NULL;
> while ((rlen -= np * sizeof(unsigned int)) >= 0) {
> if (prev) {
> - if (prev[0] == ranges[0] && prev[1] == ranges[1] &&
> - (prev[2] + prev[na+4]) == ranges[2] &&
> - (prev[na+2] + prev[na+4]) == ranges[na+2]) {
> - prev[na+4] += ranges[na+4];
> + prev_pci_space = prev[0];
> + prev_pci_addr = pci_get_range64(&prev[1]);
> + prev_size = pci_get_range64(&prev[na+3]);
> + pci_space = ranges[0];
> + pci_addr = pci_get_range64(&ranges[1]);
> + if ((prev_pci_space == pci_space) &&
> + ((prev_pci_addr + prev_size) == pci_addr)) {
> + size = pci_get_range64(&ranges[na+3]);
> + prev_size += size;
> ranges[0] = 0;
> + prev[na+3] = (u32)((prev_size >> 32) &
> 0xffffffff);
> + prev[na+4] = (u32)(prev_size & 0xffffffff);
> ranges += np;
> continue;
I do think that ranges hacking (even on a copy) to cope with contiguous ranges
is not a good deed. And nobody would object the upper looks horrible from the
maintenance POV.
> }
> @@ -904,21 +923,22 @@
> rlen = orig_rlen;
> while (ranges && (rlen -= np * sizeof(unsigned int)) >= 0) {
> res = NULL;
> - size = ranges[na+4];
> - switch ((ranges[0] >> 24) & 0x3) {
> + size = pci_get_range64(&ranges[na+3]);
This is not correct - it should depend on #ac of the parent node.
But I'll stop right here - there is no deep mutual difference between 32 & 64
bit so that to keep 2 similar implementations, none of which (esp 32bit) really
comply the spec. It should work the same way, and, if there are differences,
they should be handled
explicitly, and, of course, reconsidered if they make sense (like isa_io_base,
absense of io_size in ppc32 and so on)
I am *not* telling here that my implementation is the only true way around. But
we need to improve and make code cleaner rather then just extend existing
error-prone approach.
--
Sincerely, Vitaly
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
