On Mon, 27 Aug 2007 17:49:55 +1000
David Gibson wrote:
> 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.
>
ok, makes sense.
> [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.
>
will check, but sounds reasonable.
> > 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...
>
Wrong example, I'll have real snippet if alignment trick won't work (and I hope
it will)
> [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 :(.
Well, my point is:
- pci_32.c ranges parsing code is confusing and silly in many
ways
- there is no obvious way to enable 64bit phys addressed
handled correctly there.
possible approaches may bring more problems than solve.
- completely correct is going to be *hard* as David noted, but
we can consider effective merge(with code flow similar to existing
funcs) of existing
bits a good starting point. Else, when a lot of
stufpci_process_bridge_OF_rangesf would depend on
both functions, it would be too painful to rewrite.
(speaking about pci_process_bridge_OF_ranges here)
>
> > > > + 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.
>
ok, I think there's no problem to implement it in the new func then.
--
Sincerely, Vitaly
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
