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:

quoted

On Sat, Aug 25, 2007 at 01:29:47PM +0400, Vitaly Bordug wrote:

[snip]

quoted

quoted

+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]

quoted

quoted

+	/* 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]

quoted

quoted

+			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 :(.

quoted

quoted

+			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).

quoted

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