[U-Boot] [PATCH v2 8/8] lpc32xx: add support for board work_92105

[Albert ARIBAUD]

Hi Stefan,

Le Fri, 13 Feb 2015 10:36:52 +0100, Stefan Roese <sr at denx.de> a écrit :

> > +++ b/arch/arm/cpu/arm926ejs/lpc32xx/dram.c
> > @@ -0,0 +1,80 @@
> > +/*
> > + * LPC32xx dram init
> > + *
> > + * (C) Copyright 2014  DENX Software Engineering GmbH
> > + * Written-by: Albert ARIBAUD <albert.aribaud at 3adev.fr>
> > + *
> > + * This is called by SPL to gain access to the SDR DRAM.
> > + *
> > + * This code runs from SRAM.
> > + *
> > + * Actual CONFIG_LPC32XX_SDRAM_* parameters must be provided
> > + * by the board configuration file.
> > + *
> > + * SPDX-License-Identifier:	GPL-2.0+
> > + */
> > +
> > +#include <common.h>
> > +#include <netdev.h>
> > +#include <asm/arch/cpu.h>
> > +#include <asm/arch/clk.h>
> > +#include <asm/arch/wdt.h>
> > +#include <asm/arch/emc.h>
> > +#include <asm/io.h>
> > +
> > +static struct clk_pm_regs *clk = (struct clk_pm_regs *)CLK_PM_BASE;
> > +static struct emc_regs *emc = (struct emc_regs *)EMC_BASE;
> > +
> > +#if defined(CONFIG_SPL_BUILD)  
> 
> Why don't you move this compiler switch to the Makefiles instead?

Indeed, why? :)
Will do in v3.

> > +++ b/board/work-microwave/work_92105/README

> > +This board has SPL support, and uses the LPC32XX boot image format.
> > +Once the U-Boot target "work_92105" is built, the following two files
> > +can be flashed:
> > +
> > +	spl/lpc32xx-spl.bin at offset 0x0
> > +	u-boot.bin at offset 0x40000
> 
> Really u-boot.bin? Why don't you use u-boot.img with the header / CRC 
> instead? This is commonly used by other SPL platforms.

u-boot.bin (headerless) is what the old process used, so I went for the
least change path, but of course u-boot.img works too. I'll fix the
text to mention u-boot.img and just note that u-boot.bin would work too
but would not be checked for integrity.

(plus I need to fix the SPL image name anyway, since I changed it
when switching to mkimage.)

> And wouldn't it be nice to combine (concatenate) those two images into 
> one image that can be flashed to the board? There are already some 
> Makefile rules for doing this.

Actually it would have to concatenate two (redundant) copies of the SPL
image then one of U-Boot, bu yes, you're right. I'll look into it --
and add notes to this effect in the board README file.

> > +++ b/board/work-microwave/work_92105/work_92105.c
> > @@ -0,0 +1,86 @@
> > +/*
> > + * WORK Microwave work_92105 board support
> > + *
> > + * (C) Copyright 2014  DENX Software Engineering GmbH
> > + * Written-by: Albert ARIBAUD <albert.aribaud at 3adev.fr>
> > + *
> > + * SPDX-License-Identifier:	GPL-2.0+
> > + */
> > +
> > +#include <common.h>
> > +#include <asm/io.h>
> > +#include <asm/arch/sys_proto.h>
> > +#include <asm/arch/cpu.h>
> > +#include <asm/arch/emc.h>
> > +#include <asm/gpio.h>
> > +#include <spl.h>
> > +#include "work_92105_display.h"
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +#if defined(CONFIG_SPL_BUILD)
> 
> I would perfer to move those SPL related function into a seperate file 
> (e.g. spl.c) instead. And the Makefile can handle the conditional 
> compilation then.

Ok.

> > +++ b/include/configs/work_92105.h

> > +#define CONFIG_ETHADDR			00:12:B4:00:AF:FE
> > +#define	CONFIG_OVERWRITE_ETHADDR_ONCE
> 
> We usually don't allow MAC addresses to be set in the board config 
> files. Is this really needed?

If CONFIG_ETH.*ADDR are not to be allowed, then maybe ./README should
be updated to reflect this, and the more than 08 existing definitions
in include/configs should be fixed. :)

This is what was in the boards's default config initially. I'd rather
leave it there in case the actual board provisioning process assumes
the board to have this eth MAC address on very first boot. I can add a
big red blinking comment though.

> Thanks,
> Stefan

Thanks for your review!

Cordialement,
Albert ARIBAUD
3ADEV