[U-Boot] [PATCH 6/6] mpc5121: add support for PDM360NG board

[Anatolij Gustschin <agust@denx.de>]

Hi Detlev,

Detlev Zundel <dzu@denx.de> wrote:

> > Signed-off-by: Michael Weiss michael.weiss at ifm.com
> 
> Please fix the e-mail format.

Ok.

> > @@ -820,6 +820,9 @@ aria_config:	unconfig
> >  mecp5123_config:	unconfig
> >  	@$(MKCONFIG) -a mecp5123 ppc mpc512x mecp5123 esd
> >  
> > +pdm360ng_config:	unconfig
> > +	@$(MKCONFIG) -a pdm360ng ppc mpc512x pdm360ng
> > +
> >  mpc5121ads_config \
> >  mpc5121ads_rev2_config	\
> >  	: unconfig
> 
> Keep the list of targets sorted in the Makefile please.

Ok!

> > +/*
> > + * Co-Processor communication POST
> > + */
> > +#if defined(CONFIG_POST) && defined(CONFIG_SERIAL_MULTI)
> 
> Why is this depending on SERIAL_MULTI?  The POST system as such is
> orthogonal, no?

Currently there is only Coprocessor POST only which depends
on SERIAL_MULTI. I think I should better do something like:

#if defined(CONFIG_POST)
#if defined(CONFIG_SERIAL_MULTI)
/* Co-Processor POST code */
...
#endif
#endif

Even better would be to move POST code to separate file, it seems.

> > +
> > +#if defined(CONFIG_SYS_POST_WORD_ADDR)
> > +# define _POST_ADDR	(CONFIG_SYS_POST_WORD_ADDR)
> > +#else
> > +#error echo "No POST word address defined"
> > +#endif
> > +
> > +void post_word_store(ulong a)
> > +{
> > +	volatile void *save_addr = (volatile void *)(_POST_ADDR);
> > +
> > +	out_be32(save_addr, a);
> > +}
> > +
> > +ulong post_word_load(void)
> > +{
> > +	volatile void *save_addr = (volatile void *)(_POST_ADDR);
> > +
> > +	return in_be32(save_addr);
> > +}
> 
> Can't we put this into 512x shared code?  I really dislike putting this
> into board code.  I know that 5200 set a precedent here, but they all
> have identical code.  One should rather fix 5200 also.

I'll move it to cpu/mpc512x/commproc.c.

...
> > +	ret = read_port(cop_port, buf, sizeof(buf));
> > +	close_port(1);
> > +	if (ret <= 0) {
> > +		post_log("Error: Can't read WD Coprocessor port.\n");
> > +		return -1;
> > +	}
> > +
> > +	if (strcmp(buf, alive)) {
> > +		post_log("Error: WD-Cop. resp.: %s\n", buf);
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +#endif	/* CONFIG_POST */
> 
> Wouldn't it be a good idea to split the POST stuff into a separate file?

I'll move it to a separate file.

> > +/* Use SRAM for initial stack */
> > +#define CONFIG_SYS_INIT_RAM_ADDR	CONFIG_SYS_SRAM_BASE	/* Initial RAM address */
> > +#define CONFIG_SYS_INIT_RAM_END		CONFIG_SYS_SRAM_SIZE	/* End of used area in RAM */
> > +
> > +#define CONFIG_SYS_GBL_DATA_SIZE	0x100			/* num bytes initial data */
> > +#define CONFIG_SYS_GBL_DATA_OFFSET	(CONFIG_SYS_INIT_RAM_END - CONFIG_SYS_GBL_DATA_SIZE)
> > +#define CONFIG_SYS_POST_WORD_ADDR	(CONFIG_SYS_GBL_DATA_OFFSET - 0x4)
> > +#define CONFIG_SYS_INIT_SP_OFFSET	CONFIG_SYS_POST_WORD_ADDR
> 
> Isn't this shared among the 512x implementations so we can put it in a
> common file?

Yes, we should put it in a common file.

Thanks,
Anatolij

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