On Mon, 27 Aug 2007 11:15:16 +1000
David Gibson wrote:
> On Sat, Aug 25, 2007 at 01:29:47PM +0400, Vitaly Bordug wrote:
> >
> > 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. Currently
> > validated with 440EPx (sequoia) and mpc8349-eitx.
> >
> > Signed-off-by: Vitaly Bordug <[EMAIL PROTECTED]>
> > Signed-off-by: Stefan Roese <[EMAIL PROTECTED]>
>
> I like the idea, but I don't think this implementation is adequate
> yet.
OK, I'll try to address notes below, thanks for looking at it.
>
> > diff --git a/arch/powerpc/kernel/pci-common.c
> > b/arch/powerpc/kernel/pci-common.c index 083cfbd..57cd039 100644
> > --- a/arch/powerpc/kernel/pci-common.c
> > +++ b/arch/powerpc/kernel/pci-common.c
> > @@ -478,3 +478,162 @@ 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) +{
> > + static unsigned int static_lc_ranges[256];
> > + const unsigned int *ranges;
> > + unsigned int *lc_ranges;
> > + unsigned int pci_space;
> > + unsigned long size = 0;
>
> size can be 64-bit on 32-bit systems, at least in theory.
>
hm, well, but later, we'll call ioremap (at least for IO region) that has size
passed as ulong type. And, such a size could be consumed by the code,only if
resource_size_t is 64bit too. I'll look at it further.
> > + int rlen = 0;
> > + int orig_rlen, ranges_amnt, i;
> > + int memno = 0;
> > + struct resource *res;
> > + int np, na = of_n_addr_cells(dev);
> > + struct ranges_pci64_sz64 *ranges64 = NULL;
> > + struct ranges_pci32_sz64 *ranges32 = NULL;
> > + phys_addr_t pci_addr,
>
> This is wrong: the PCI binding defines the PCI addresses to be 64-bit,
> even if the CPU has 32-bit physical addresses.
>
hmm, ok
> cpu_phys_addr;
> > +
> > + np = na + 5;
> > +
> > + /* From "PCI Binding to 1275"
> z> + * The ranges property is laid out as an array of
> z> 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 == NULL)
> > + return;
>
> if (!ranges) would be the usual idiom here.
>
ok
> > + /* Sanity check, though hopefully that never happens */
> > + if (rlen > sizeof(static_lc_ranges)) {
> > + printk(KERN_WARNING "OF ranges property too
> > large !\n");
> > + rlen = sizeof(static_lc_ranges);
> > + }
> > +
> > + /* Let's work on a copy of the "ranges" property instead
> > + * of damaging the device-tree image in memory
> > + */
> > + lc_ranges = static_lc_ranges;
> > + memcpy(lc_ranges, ranges, rlen);
> > + orig_rlen = rlen;
> > +
> > + ranges = lc_ranges;
>
> You don't ever actually touch the ranges property in place, so there's
> no need for this copy stuff.
>
ok
> > + /* Map ranges to struct according to spec. */
> > + if (na >= 2) {
> > + ranges64 = (void *)ranges;
> > + ranges_amnt = rlen / sizeof(*ranges64);
> > + } else {
> > + ranges32 = (void *)ranges;
> > + ranges_amnt = rlen / sizeof(*ranges32);
> > + }
> > +
> > + hose->io_base_phys = 0;
> > + for (i = 0; i < ranges_amnt; i++) {
> > + res = NULL;
> > + if (ranges64) {
> > + if (ranges64[i].pci_space == 0)
> > + continue;
> > +
> > + pci_space = ranges64[i].pci_space;
> > + pci_addr =
> > + (u64) ranges64[i].pci_addr[0] << 32 |
> > ranges64[i].
> > + pci_addr[1];
>
> Why not just define the pci_addr field in your structures as u64? You
> would have to define the structure with attribute((packed)), I guess.
>
that was in the first approach, but it gets really interesting numbers (and
with contents nothing to do with real stuff),
on platforms that are pure 32bit. I mean,
u32 foo, foo1;
u64 foo64;
foo = bar[1];
foo1 = bar[2];
foo64 = ((u64)foo << 32) | foo1;
does not seem to work on 32bit board. I may need to write a clear testcase and
submit it here, maybe I'm missing something
very obvious.
> > + cpu_phys_addr =
> > + of_translate_address(dev,
> > ranges64[i].phys_addr);
> > + /*
> > + * I haven't observed 2 significant size
> > cells in kernel
> > + * code, so we're accounting only LSB of
> > size part
> > + * from ranges. -vitb
> > + */
> > + size = ranges64[i].size[1];
> > +#ifdef CONFIG_PPC64
> > + if (ranges64[i].size[0])
> > + size |= ranges64[i].size[0]<<32;
> > +#endif
> > + DBG("Observed: pci %llx phys %llx size
> > %x\n", pci_addr,
> > + cpu_phys_addr, size);
> > + } else {
> > + if (ranges32[i].pci_space == 0)
> > + continue;
> > +
> > + pci_space = (unsigned
> > int)ranges32[i].pci_space;
> > + pci_addr = (unsigned
> > int)ranges32[i].pci_addr[1];
> > + cpu_phys_addr = (unsigned
> > int)ranges32[i].phys_addr[0];
>
>
> We should really be using of_translate_address() in all cases - that's
> what it's for, after all.
>
being called, it gives 0 in 32bit branch of if () {. Since, iirc,
32bit range parsing does not have it in original, variant,
I have it put as is. worths a brief investigation...
> > + size = ranges32[i].size[1];
> > +
> > + DBG("Observed: pci %x phys %x size %x\n",
> > + (u32) pci_addr, (u32) cpu_phys_addr,
> > size);
> > + }
>
> You don't have any equivalent of the code that exists in both
> pre-existing versions to coalesce contiguous ranges. You probably
> want to use the 64-bit version, since that doesn't need a local copy
> of the ranges.
>
correct. yet, the whole aim of the upper seems to use summary size of
contiguous block and that's it.
How would we recognize IORESOURCE_PREFETCH then? (I know, my code is missing
that prefetch regions so far, but anyway).
>From the code, it seems to declare each resource that is part of contiguous
>block, with the size of entire block.
That's prolly from binding, but can you please elaborate the logic for better
understanding?
> > +
> > + switch ((pci_space >> 24) & 0x3) {
> > + case 1: /* I/O space */
> > +#ifdef CONFIG_PPC32
> > + /*
> > + * check from ppc32 pci implementation.
> > + * not sure if it is needed. -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); +
> > + if (prim)
> > + isa_io_base = (unsigned
> > long)hose->io_base_virt;
>
> The old 64-bit versions don't presently ioremap() or set isa_io_base.
> I'd be worried about changing this semantic, at least without a rather
> more widespread consolidation of the 32/64 bit PCI code.
>
Will ifdef it out.
> > +
> > + res = &hose->io_resource;
> > + res->flags = IORESOURCE_IO;
[snip]
--
Sincerely, Vitaly
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev
