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

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

[Andre Schwarz]

The MVBC_P is a MPC5200B based camera system with Intel Gigabit ethernet
controller (using e1000) and custom Altera Cyclone-II FPGA on PCI.
Please see doc/README.mvbc_p for details.

Signed-off-by: Andre Schwarz <andre.schwarz@matrix-vision.de>
---



MATRIX VISION GmbH, Talstra?e 16, DE-71570 Oppenweiler  - Registergericht: Amtsgericht Stuttgart, HRB 271090
Gesch?ftsf?hrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patch_mvbc_p
Url: http://lists.denx.de/pipermail/u-boot/attachments/20080704/93ddfcee/attachment.txt 

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

[Wolfgang Grandegger]

Andre Schwarz wrote:
> The MVBC_P is a MPC5200B based camera system with Intel Gigabit ethernet
> controller (using e1000) and custom Altera Cyclone-II FPGA on PCI.
> Please see doc/README.mvbc_p for details.

One general remark. Please use the in_*() and out_*() accessor functions
to access I/O space. This will make most of your asm("sync");
instructions obsolete.

Wolfgang.

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

[Andre Schwarz]

Wolfgang,

thanks - I'll wait for further comments and send an update.

regards,
Andre


Wolfgang Grandegger schrieb:
> Andre Schwarz wrote:
>> The MVBC_P is a MPC5200B based camera system with Intel Gigabit ethernet
>> controller (using e1000) and custom Altera Cyclone-II FPGA on PCI.
>> Please see doc/README.mvbc_p for details.
>
> One general remark. Please use the in_*() and out_*() accessor functions
> to access I/O space. This will make most of your asm("sync");
> instructions obsolete.
>
> Wolfgang.


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

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

[Kim Phillips]

On Fri, 04 Jul 2008 09:42:24 +0200
Andre Schwarz <andre.schwarz@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.

> +	@$(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.

> +#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.

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

> +void hw_watchdog_reset(void)
> +{
> +	*(u8*) (0xff000005) = 0;

is this a magic number/needs a #define somewhere? 

> +#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).

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

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

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

[Andre Schwarz]

Kim,

I forgot to mention that both fpga.c are _not_ the same.
They are using different FPGA sizes _and_ different I/Os on different cpu.

It's not possible for me to use a common file for the short term.
Maybe later when the FPGA interface got mature ...

regards,
Andre

Kim Phillips schrieb:
> On Fri, 04 Jul 2008 09:42:24 +0200
> Andre Schwarz <andre.schwarz@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.
>
>   
>> +	@$(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.
>
>   
>> +#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.
>
>   
>> +	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.
>
>   
>> +void hw_watchdog_reset(void)
>> +{
>> +	*(u8*) (0xff000005) = 0;
>>     
>
> is this a magic number/needs a #define somewhere? 
>
>   
>> +#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).
>
>   
>> +
>> +#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
>   


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

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

[Kim Phillips]

On Tue, 08 Jul 2008 16:35:00 +0200
Andre Schwarz <andre.schwarz@matrix-vision.de> wrote:

> Kim,
> 
> I forgot to mention that both fpga.c are _not_ the same.
> They are using different FPGA sizes _and_ different I/Os on different cpu.

the size is a one-line thing that can be easily and understandably
#ifdeffed.  The gpio struct difference can be fixed like this:

static u32 get_dvo()
{
#ifdef CONFIG_MPC83XX
       volatile immap_t *im = (volatile immap_t *)CFG_IMMR;
       volatile gpio83xx_t *gpio = (volatile gpio83xx_t *)&im->gpio[0];
       return gpio->dat;
#else
#ifdef CONFIG_MPC5XXX
       struct mpc5xxx_gpio *gpio = (struct mpc5xxx_gpio*)MPC5XXX_GPIO;
       return gpio->simple_dvo;
#endif
#endif
}

...and subsequently use get_dvo() later in the file.

> It's not possible for me to use a common file for the short term.
> Maybe later when the FPGA interface got mature ...

I really think code maintenance will suffer if you don't do it now;
afaict, the file is functionally equal modulo the size one-liner.

Kim

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

[André Schwarz]

Kim Phillips wrote:
> On Tue, 08 Jul 2008 16:35:00 +0200
> Andre Schwarz <andre.schwarz@matrix-vision.de> wrote:
>
>   
>> Kim,
>>
>> I forgot to mention that both fpga.c are _not_ the same.
>> They are using different FPGA sizes _and_ different I/Os on different cpu.
>>     
>
> the size is a one-line thing that can be easily and understandably
> #ifdeffed.  The gpio struct difference can be fixed like this:
>
> static u32 get_dvo()
> {
> #ifdef CONFIG_MPC83XX
>        volatile immap_t *im = (volatile immap_t *)CFG_IMMR;
>        volatile gpio83xx_t *gpio = (volatile gpio83xx_t *)&im->gpio[0];
>        return gpio->dat;
> #else
> #ifdef CONFIG_MPC5XXX
>        struct mpc5xxx_gpio *gpio = (struct mpc5xxx_gpio*)MPC5XXX_GPIO;
>        return gpio->simple_dvo;
> #endif
> #endif
> }
>
> ...and subsequently use get_dvo() later in the file.
>
>   
ok - you're right.
Will fix it along with other out_* issues.
>> It's not possible for me to use a common file for the short term.
>> Maybe later when the FPGA interface got mature ...
>>     
>
> I really think code maintenance will suffer if you don't do it now;
> afaict, the file is functionally equal modulo the size one-liner.
>
> Kim
>   
yes - thanks for persuading :-)



MATRIX VISION GmbH, Talstra?e 16, DE-71570 Oppenweiler  - Registergericht: Amtsgericht Stuttgart, HRB 271090
Gesch?ftsf?hrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.denx.de/pipermail/u-boot/attachments/20080709/676d4809/attachment.htm