[U-Boot-Users] [PATCH] Add MVBC_P board

[Andre Schwarz]

Kim,

thanks for the hints.

Kim Phillips schrieb:
> On Fri, 04 Jul 2008 09:42:24 +0200
> Andre Schwarz <andre.schwarz at matrix-vision.de> wrote:
>
> Hello Andre,
>
>   
>>  board/mvbc_p/fpga.c            |  177 ++++++++++++++++++++
>>  board/mvbc_p/fpga.h            |   34 ++++
>>     
>
> couldn't help but notice this file is equal to board/mvblm7/fpga.c.
> Perhaps it's time to add your board/$(VENDOR)/common directory and put
> both this file and its header there.  This way you can make your code
> more maintainable by avoiding duplicating it all over the place.
>
>   
yes.
>> +	@$(MKCONFIG) -n $@ -a MVBC_P ppc mpc5xxx mvbc_p
>>     
>
> assuming $VENDOR == matrix-vision or something (your choice), you'd
> have to modify the above line like so:
>
> -	@$(MKCONFIG) -n $@ -a MVBC_P ppc mpc5xxx mvbc_p
> +	@$(MKCONFIG) -n $@ -a MVBC_P ppc mpc5xxx mvbc_p matrix-vision
>
> to enable building your (now a single copy) fpga.c.
>
>   
Of course you're right - but I'd like to add all boards first in order
_not_ to mix patches for different boards.
Maybe we can do this _after_ adding this board ?
If this is not ok I'll fix it right now ...
>> +#ifdef CONFIG_OF_LIBFDT
>> +#include <fdt_support.h>
>> +#endif
>>     
>
> it'd be nice to get rid of ifdeffing CONFIG_OF_LIBFDT all over the
> place, assuming, of course, you won't be supporting booting a
> non-fdt-aware OS.
>
>   
ok.
>> +	gpio->simple_ddr = SIMPLE_DDR;
>> +	gpio->simple_dvo = SIMPLE_DVO;
>> +	gpio->simple_ode = SIMPLE_ODE;
>> +	gpio->simple_gpioe = SIMPLE_GPIOEN;
>> +
>> +	gpio->sint_ode = SINT_ODE;
>> +	gpio->sint_ddr = SINT_DDR;
>> +	gpio->sint_dvo = SINT_DVO;
>> +	gpio->sint_inten = SINT_INTEN;
>> +	gpio->sint_itype = SINT_ITYPE;
>> +	gpio->sint_gpioe = SINT_GPIOEN;
>> +
>> +	*(vu_char *)MPC5XXX_WU_GPIO_ODE	= WKUP_ODE;
>> +	*(vu_char *)MPC5XXX_WU_GPIO_DIR	= WKUP_DIR;
>> +	*(vu_char *)MPC5XXX_WU_GPIO_DATA_O = WKUP_DO | ARB_X_EN;
>> +	*(vu_char *)MPC5XXX_WU_GPIO_ENABLE = WKUP_EN;
>> +
>> +	printf("simple_gpioe: 0x%08x\n", gpio->simple_gpioe);
>> +	printf("sint_gpioe  : 0x%08x\n", gpio->sint_gpioe);
>> +	__asm__ volatile ("sync");
>> +}
>>     
>
> same comment Wolfgang made; use in_* out_* accessor fns.
>
>   
yes - as already mentioned I'll wait for some more feedback before
re-submitting a modified patch.
>> +void hw_watchdog_reset(void)
>> +{
>> +	*(u8*) (0xff000005) = 0;
>>     
>
> is this a magic number/needs a #define somewhere? 
>   
ok.
>   
>> +#define MV_FPGA_DATA		"0xff860000"
>> +#define MV_FPGA_SIZE		"0x3c886"
>> +#define MV_KERNEL_ADDR		"0xffc00000"
>> +#define MV_INITRD_ADDR		"0xff900000"
>> +#define MV_INITRD_LENGTH	"0x00300000"
>> +#define MV_SCRATCH_ADDR		"0x00000000"
>> +#define MV_SCRATCH_LENGTH	MV_INITRD_LENGTH
>> +#define MV_AUTOSCR_ADDR		"0xff840000"
>> +#define MV_AUTOSCR_ADDR2	"0xff850000"
>> +#define MV_DTB_ADDR		"0xfffc0000"
>>     
>
> please use MK_STR (see other config files, e.g. MPC8313).
>
>   
hmm...
Is there a functional difference/advantage or simply coding style ?
>> +
>> +#define CONFIG_SHOW_BOOT_PROGRESS 1
>> +
>> +#define MV_KERNEL_ADDR_RAM	"0x00100000"
>> +#define MV_DTB_ADDR_RAM		"0x00600000"
>> +#define MV_INITRD_ADDR_RAM	"0x01000000"
>> +
>> +/* pass open firmware flat tree */
>> +#define CONFIG_OF_LIBFDT	1
>> +#define CONFIG_OF_BOARD_SETUP	1
>> +
>> +#define OF_CPU			"PowerPC,5200 at 0"
>> +#define OF_SOC			"soc5200 at f0000000"
>> +#define OF_TBCLK		(bd->bi_busfreq / 4)
>>     
>
> I thought we had done away with the above three (FLAT_TREE now
> obsolete).  Oh, I see now: 5xxx still uses it in LIBFDT code.  Bad 5xxx!
>
> Kim
>   
As always I have to rely on some recent board implementation since I
don't have time to dig into each line of code involved. Wouldn't it be a
good thing if the custodian removes all #defines as soon as it gets
obsolete ?


regards,
Andre



MATRIX VISION GmbH, Talstraße 16, DE-71570 Oppenweiler  - Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschäftsführer: Gerhard Thullner, Werner Armingeon, Uwe Furtner