Re: [PATCH 1/3] [POWERPC] Merge 32 and 64 bit pci_process_bridge_OF_ranges() instances

[David Gibson <david@gibson.dropbear.id.au>]

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