On Mon, Aug 27, 2007 at 10:31:36AM +0400, Vitaly Bordug wrote:
> 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:
[snip]
> > > +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.
You should probably actually test for that condition though, rather
than blithely truncating.
[snip]
> > > + /* 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,
I'm guessing that's because you didn't put the ((packed)) in: without
it, gcc will align the 64-bit quantity to a 64-bit boundary, inserting
32-bits of padding into the structure between pci_space and pci_addr,
so that it doesn't actually line up with the ranges property.
> 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.
!?! shouldn't be anything wrong with that arithmetic...
[snip]
> > > + 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...
Then we should fix of_translate_address() for 32-bit....
I don't think this patch is actually required for the 44x pci support,
yes? It might be better to leave this until later - it's only a tiny
piece of all the consolidations we should do between ppc32 and ppc64
PCI code. Currently there are a lot of silly, subtle differences
between them :(.
> > > + 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?
The attributes such as PREFETCH are encoded into cell 0 of the 3-cell
OF PCI address. So, ranges with different attributes won't appear as
contiguous in this space.
--
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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
