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

[Vitaly Bordug]

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 <vitb at kernel.crashing.org>
> > Signed-off-by: Stefan Roese <sr at denx.de>
> 
> 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