[U-Boot] [PATCH 1/3] ppc4xx: Consolidate pci_target_init() function

[Stefan Roese]

Hi Matthias,

On Sunday 15 November 2009 15:01:45 Matthias Fuchs wrote:
> > +	 * Make this region non-prefetchable.
> > +	 */
> > +	/* PMM0 Mask/Attribute - disabled b4 setting */
> > +	out_le32((void *)PCIL0_PMM0MA, 0x00000000);
> > +	/* PMM0 Local Address */
> > +	out_le32((void *)PCIL0_PMM0LA, CONFIG_SYS_PCI_MEMBASE);
> > +	/* PMM0 PCI Low Address */
> > +	out_le32((void *)PCIL0_PMM0PCILA, CONFIG_SYS_PCI_MEMBASE);
> > +	/* PMM0 PCI High Address */
> > +	out_le32((void *)PCIL0_PMM0PCIHA, 0x00000000);
> > +	/* 512M + No prefetching, and enable region */
> > +	out_le32((void *)PCIL0_PMM0MA, 0xE0000001);
> > +
> > +	/* PMM0 Mask/Attribute - disabled b4 setting */
> 
> Typo. PMM0<->PMM1 inside comments. 4x.

Thanks for spotting. Will fix.
 
> > +	out_le32((void *)PCIL0_PMM1MA, 0x00000000);
> > +	/* PMM0 Local Address */
> > +	out_le32((void *)PCIL0_PMM1LA, CONFIG_SYS_PCI_MEMBASE2);
> > +	/* PMM0 PCI Low Address */
> > +	out_le32((void *)PCIL0_PMM1PCILA, CONFIG_SYS_PCI_MEMBASE2);
> > +	/* PMM0 PCI High Address */
> > +	out_le32((void *)PCIL0_PMM1PCIHA, 0x00000000);
> > +	/* 512M + No prefetching, and enable region */
> > +	out_le32((void *)PCIL0_PMM1MA, 0xE0000001);
> 
> BTW, do you know why most 440 boards use PMM0+1 to map 1GB?
> PMC440 uses PMM0 for this only. I don't see a problem with this when
> CONFIG_SYS_PCI_MEMBASE is aligned to CONFIG_SYS_PCI_MEMBASE.

No, I don't see a problem with this either. We should probably address this in 
a follow-up patch.

<snip>

> > diff --git a/include/configs/PMC440.h b/include/configs/PMC440.h
> > index d6e2f6b..89be2ca 100644
> > --- a/include/configs/PMC440.h
> > +++ b/include/configs/PMC440.h
> > @@ -440,6 +440,7 @@
> >  #define CONFIG_SYS_PCI_SUBSYS_VENDORID 0x12FE	/* PCI Vendor ID: esd gmbh
> >      */ #define CONFIG_SYS_PCI_SUBSYS_ID_NONMONARCH 0x0441	/* PCI Device
> > ID: Non-Monarch */ #define CONFIG_SYS_PCI_SUBSYS_ID_MONARCH 0x0440	/* PCI
> > Device ID: Monarch */ +#define CONFIG_SYS_PCI_SUBSYS_ID	0x0440	/* for
> > weak __pci_target_init() */
> 
> I would prefer:
> #define CONFIG_SYS_PCI_SUBSYS_ID CONFIG_SYS_PCI_SUBSYS_ID_MONARCH /* for
>  weak __pci_target_init() */

OK, will change.
 
> On the other side I am also not very happy with defines that are required
>  just to satisfy some overwritten weak code. But in this case it probably
>  the easiest way to go.

Yes, I didn't see an easier way out of this, without introducing another 
#ifdef.
 
New patch will follow soon. Please feel free to send you Acked-by if you are 
ok with it.

Thanks.

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de