Re: [PATCH 6/6] 44x/currituck: Add support for the new IBM currituck platform

[Tony Breeds <tony@bakeyournoodle.com>]

[-- Attachment #1: Type: text/plain, Size: 4483 bytes --]

On Wed, Nov 30, 2011 at 05:20:22PM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2011-11-30 at 16:24 +1100, Tony Breeds wrote:
> > +	plb {
> > +		compatible = "ibm,plb-4xx", "ibm,plb4"; /* Could be PLB6, doesn't matter */
> 
> Then make it plb6 and add it to the probe list. Might have to whack it's
> configuration registers one day etc...

Done.

> > +			 * XXX: 1 TB address space, do we really care past
> > +			 * 4 GB and should we expand cell width?
> > +			 */
> 
> For OPB probably not :-)

Fixed.

> That doesn't seem like a very useful pair of statements or useful debug
> message....

All the DBG cruft is gone.
 
> > +	for(i = 0; i < MAX_RANKS; i++){
> > +		reg = mfdcrx(DDR3_MR0CF + i);
> > +		printf("%s: reg=0x%08x\r\n", __func__, reg);
> 
> All that debug is pretty gross, keep it if you wish but make it a bit
> neater and/or wrap it in DBG

Yeah that was an oversight.
 
> > +		if (reg & 0x01) {
> 
> 	if (!(reg & 1))
> 		continue;
> 
> avoids too much indent

Done.

> > +	dt_fixup_memory(0x0ULL,  ibm_currituck_memsize);
> 
> Ok, I see why the global... I'd still prefer if the detect function just
> returned the value and the caller whacks the global.

Fixed.
 
> > +	while ((devp = find_node_by_devtype(devp, "pci"))) {
> > +		if (getprop(devp, "dma-ranges", &dma_ranges[0], sizeof(dma_ranges)) < 0) {
> 
> Can't you replace &dma_ranges[0] with just dma_ranges ?

Yes, Fixed.

> > +#define SPRN_PIR	0x11E	/* Processor Indentification Register */
> 
> That should go elsewhere along with the other SPR definitions.

There isn't a std. place in the bootwrapper.
> 
> > +void platform_init(void)
> > +{
> > +	/* Cap the zImage to 512MB */
> 
> Any reason ? If yes, please document it a bit more.

XXX

> > +	node = fdt_node_offset_by_prop_value(_dtb_start, -1, "device_type",
> > +	                                     "cpu", sizeof("cpu"));
> > +	if (!node)
> > +		fatal("Cannot find cpu node\n");
> 
> The above will return -a- CPU node... you have several and you don't
> know which one. You should probably iterate accross all of them.

I'm just trying to get the timebase-frequency, this willbe the same on
all CPUs (at least I hope so ;P) so I don't really care which CPU node I
get.
 
> > +	/* FIXME: Check this works */

Grr that comment shouldn't be there.  It does work :)

> > +#define PVR_476CURRITUCK	0x7ff50000
> 
> My understanding is that the currituck was the platform, not the chip,
> and that the chip was called something like 476FPE, am I wrong ?

No you're correct, I was confused about the boundry between 476fpe and
currituck.

> > +	cmplwi	cr0,r3,PVR_476CURRITUCK@h
> > +	beq	head_start_47x
> 
> So at some point, they gave us the magic foo to do with the PVR to
> identify any 476... I'll try to dig that out of my email archives.

Yeah I have that I'll work out the correct way to the the mask and
test.

> > +++ b/arch/powerpc/platforms/44x/ppc47x.c
> 
> Call the file currituck.c

Sure.
 
> > +static void __devinit quirk_ppc_currituck_usb_fixup(struct pci_dev *dev)
> > +{
> > +	pci_write_config_dword(dev, 0xe0, 0x0114231f);
> > +	pci_write_config_dword(dev, 0xe4, 0x00006c40);
> > +}
> 
> Pleae document better what you are doing here and also test
> that you are indeed on the right platform so you don't end up
> whacking bits on USB controllers on other platforms that happen
> to be compiled in the same binary.

Sure, no problem.
 
> Ok, I'll have to fixup that vs. Kyle patches but the good thing is that
> it will make things even simpler.

Thanks.
 
> Now pretty much everything in this platform file is generic I believe.
> We could move it all to ppc44x_simple.c. The only things that are not
> are the USB quirk and the interrupt fixup.
> 
> The USB quirk which should have a compatible test for the platform to
> make sure we don't run it on something else. For such a simple quirk, I
> think it's fine ot have it in ppc44x_simple.c or in drivers/pci/quirk.c
> 
> For the interrupt fixup, we can probably address it entirely in the
> device-tree, though that means exposing a bunch of on-board bridges
> which is only midly annoying.
> 
> Anyways, we can discuss that (or maybe an even better option)
> tomorrow :-) 

Okay I look forward to it :)
 
> Again, you are mixing the SoC with the board here. afaik, currituck is
> the board, not the SoC.

Fixed.

Yours Tony

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]