[U-Boot] [PATCH 1/3 v2] ppc4xx: Add AMCC Arches board support(dual 460GT)

[Victor Gallardo]

Dear Wolfgang,

Thanks for the feedback.

> ...
> > +int misc_init_r(void)
> > +{
> ...
> > +	/* reset all SGMII interfaces */
> > +	mfsdr(SDR0_SRST1,   reg);
> > +	reg |= (SDR0_SRST1_SGMII0 | SDR0_SRST1_SGMII1 | SDR0_SRST1_SGMII2);
> > +	mtsdr(SDR0_SRST1, reg);
> > +	mtsdr(SDR0_ETH_STS, 0xFFFFFFFF);
> > +	mtsdr(SDR0_SRST1,   0x00000000);
> > +
> > +	for (timeout = 60; timeout > 0; timeout--) {
> > +		udelay(1000);
> > +		mfsdr(SDR0_ETH_PLL, eth_pll);
> > +		if ((eth_pll & SDR0_ETH_PLL_PLLLOCK) != 0)
> > +			break;
> > +	}
>
> Where is the 60 ms timeout coming from?
>

This is a very high mark number we came up with when running some test
while waiting for SDR0_ETH_PLL_PLLLLOCK. Actually a timeout is really
not needed PLL LOCK is always achieved. No initialization of the
SDR0_ETH_PLL required. The SDR0_ETH_PLL is configured by default after
reset. The code should look more like this.

do {
	mfsdr(SDR0_ETH_PLL, eth_pll);
} while (!(eth_pll & SDR0_ETH_PLL_PLLLOCK));

> > --- a/include/configs/amcc-common.h
> > +++ b/include/configs/amcc-common.h
> > @@ -55,6 +55,13 @@
> >  #endif
> >  
> >  /*
> > + * Only very few boards have default netdev not set to eth0 (like Arches)
> > + */
> > +#if !defined(CONFIG_NETDEV)
> > +#define CONFIG_NETDEV		eth0
> > +#endif
> 
> Please don't add this to common code; please handle it locally in the
> arches configuration.

We do handle this locally in arches config section (canyonlands.h). The
above is the default. We need this because eth0 in Arches is used for
CPU-to-CPU Communcation via ethernet. Eth1 is used for normal RJ45
connection.


> > @@ -147,9 +154,11 @@
> >  /*
> >   * Booting and default environment
> >   */
> > +#if !defined(CONFIG_PREBOOT)
> >  #define CONFIG_PREBOOT	"echo;"	\
> >  	"echo Type \"run flash_nfs\" to mount root filesystem over NFS;" \
> >  	"echo"
> > +#endif
>
> Ditto. [Also: what's the reason for this change? ]
>

Once again. This is the default, we overwrite this in our arches
Config section (canyonlands.h). We modify CONFIG_PREBOOT to
Set the default ethact to ppc_4xx_eth1. See below

> > +	"net_self_load=tftp ${kernel_addr_r} ${bootfile};"		\
> > +		"tftp ${fdt_addr_r} ${fdt_file};"			\
> > +		"tftp ${ramdisk_addr_r} ${ramdisk_file};\0"		\
> 
> What is this needed for?

net_self_load is used by net_self to load linux, fdt blob, ramdisk
via tftp. this is very similar to net_nfs, flash_nfs and flash_self

> > diff --git a/include/configs/canyonlands.h b/include/configs/canyonlands.h
> > index 2f162e1..acad9b3 100644
> > --- a/include/configs/canyonlands.h
> > +++ b/include/configs/canyonlands.h
> ...
> > +#define CONFIG_PREBOOT \
> > +	"setenv ethact ppc_4xx_eth1;" \
> 
> This should not be done in the preboot command, but as part of the
> default config instead.

Yes, you are correct. I will fix this.

> > -#define CFG_AHB_BASE		0xE2000000	/* internal AHB peripherals	*/
> > +#define CFG_AHB_BASE		0xE2000000	/* internal AHB peripherals */
> 
> Is this change an improvement?

No

> > @@ -223,6 +268,7 @@
> >   * DDR SDRAM
> >   *----------------------------------------------------------------------------*/
> >  #if !defined(CONFIG_NAND_U_BOOT)
> > +#if !defined(CONFIG_ARCHES)
> >  /*
> >   * NAND booting U-Boot version uses a fixed initialization, since the whole
> >   * I2C SPD DIMM autodetection/calibration doesn't fit into the 4k of boot
> 
> Are you sure this is correct?

I will investigate.

> > +#if !defined(CONFIG_ARCHES)
> >  /* Only Canyonlands (460EX) has USB */
> >  #ifdef CONFIG_460EX
> 
> I think it would make more sense to move the "!defined(CONFIG_ARCHES)"
> _after_ the "#ifdef CONFIG_460EX".

Will double check

> > +#else	/* defined(CONFIG_ARCHES) */
> > +
> > +/* Arches board does not have USB connectivity */
> > +
> > +/*
> > + * Default environment variables
> > + */
> > +#define	CONFIG_EXTRA_ENV_SETTINGS					\
> 
> Ah... but these definitely do not belong into the "USB connectivity"
> block...

OK. Will fix.

> >   */
> > +#if !defined(CONFIG_ARCHES)
> >  #define CONFIG_CMD_DATE
> > -#define CONFIG_CMD_DTT
> >  #define CONFIG_CMD_NAND
> > +#define CONFIG_CMD_SNTP
> > +#endif
> > +#define CONFIG_CMD_DTT
> >  #define CONFIG_CMD_PCI
> >  #define CONFIG_CMD_SDRAM
> > -#define CONFIG_CMD_SNTP
> >  #ifdef CONFIG_460EX
> >  #define CONFIG_CMD_EXT2
> >  #define CONFIG_CMD_FAT
>
> I think we should separate these tlistrs of commands, so we can keep
> the lists sorted.

OK.

> > +/*
> > + * Arches has 32MBytes of NOR FLASH (Spansion 29GL256) so remap FLASH to
> > + * EBC address which accepts bigger regions:
> > + *   0xfe00.0000 -> 4.ce00.0000
> > + */
> 
> Cannot we do this automatically, dependent on the actual size of flash
> as determined by the code?
> 

Will investigate.

Best Regards,

Victor Gallardo