Skip to contentSkip to conversation

Add MVBC_P board

PATCH New Andre Schwarz 3 participants 8 messages Jul 4, 2008 – Jul 8, 2008

lore
Share & tools
lore

Andre Schwarz

author
#0 lore raw

PATCH 1/1 Add MVBC_P board

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 <[email protected]>

---

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

#1 lore raw

PATCH 1/1 Add MVBC_P board

Quoted 3 lines · 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.

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.

author
#2 lore raw

PATCH 1/1 Add MVBC_P board

Wolfgang,

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

regards,
Andre

Quoted 10 lines · Wolfgang Grandegger
Quoted 3 lines · 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.

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

#3 lore raw

PATCH 1/1 Add MVBC_P board

In reply to Andre Schwarz

On Fri, 04 Jul 2008 09:42:24 +0200
Andre Schwarz <[email protected]> wrote:

Hello Andre,

Quoted 2 lines
 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.

Review

Quoted 1 line
(quoted diff) +1 -0
+	@$(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.

Review

Quoted 3 lines
(quoted diff) +3 -0
+#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.

Quoted 11 lines
(quoted diff) +11 -0
+	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;

Review

Quoted 10 lines
(quoted diff) +10 -0
+
+	*(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.

Review

Quoted 3 lines
(quoted diff) +3 -0
+void hw_watchdog_reset(void)
+{
+	*(u8*) (0xff000005) = 0;

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

Review

Quoted 10 lines
(quoted diff) +10 -0
+#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).

Quoted 4 lines
(quoted diff) +4 -0
+
+#define CONFIG_SHOW_BOOT_PROGRESS 1
+
+#define MV_KERNEL_ADDR_RAM	"0x00100000"

Review

Quoted 10 lines
(quoted diff) +10 -0
+#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

author
#4 lore raw

PATCH 1/1 Add MVBC_P board

Kim,

thanks for the hints.

Quoted 6 lines · Kim Phillips

On Fri, 04 Jul 2008 09:42:24 +0200
Andre Schwarz <[email protected]> wrote:

Hello Andre,

Quoted 10 lines
Quoted 3 lines
 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.

Quoted 9 lines

Review

Quoted 2 lines
(quoted diff) +1 -0
+	@$(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:

(quoted diff) +1 -1
-	@$(MKCONFIG) -n $@ -a MVBC_P ppc mpc5xxx mvbc_p
+	@$(MKCONFIG) -n $@ -a MVBC_P ppc mpc5xxx mvbc_p matrix-vision
Quoted 3 lines

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

Quoted 10 lines

Review

Quoted 4 lines
(quoted diff) +3 -0
+#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.

Quoted 16 lines
Quoted 16 lines
(quoted diff) +16 -0
+	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;
Quoted 10 lines

Review

Quoted 6 lines
(quoted diff) +5 -0
+
+	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.

Quoted 7 lines

Review

Quoted 4 lines
(quoted diff) +3 -0
+void hw_watchdog_reset(void)
+{
+	*(u8*) (0xff000005) = 0;

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

ok.

Quoted 16 lines

Review

Quoted 12 lines
(quoted diff) +10 -0
+#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 ?

Quoted 19 lines
Quoted 5 lines
(quoted diff) +4 -0
+
+#define CONFIG_SHOW_BOOT_PROGRESS 1
+
+#define MV_KERNEL_ADDR_RAM	"0x00100000"

Review

Quoted 10 lines
(quoted diff) +10 -0
+#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!

Quoted 2 lines

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

author
#5 lore raw

PATCH 1/1 Add MVBC_P board

In reply to Kim Phillips

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

Quoted 106 lines · Kim Phillips

On Fri, 04 Jul 2008 09:42:24 +0200
Andre Schwarz <[email protected]> wrote:

Hello Andre,

Quoted 3 lines
 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.

Review

Quoted 2 lines
(quoted diff) +1 -0
+	@$(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:

(quoted diff) +1 -1
-	@$(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.

Review

Quoted 4 lines
(quoted diff) +3 -0
+#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.

Quoted 12 lines
(quoted diff) +11 -0
+	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;

Review

Quoted 10 lines
(quoted diff) +10 -0
+
+	*(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.

Review

Quoted 4 lines
(quoted diff) +3 -0
+void hw_watchdog_reset(void)
+{
+	*(u8*) (0xff000005) = 0;

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

Review

Quoted 11 lines
(quoted diff) +10 -0
+#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).

Quoted 5 lines
(quoted diff) +4 -0
+
+#define CONFIG_SHOW_BOOT_PROGRESS 1
+
+#define MV_KERNEL_ADDR_RAM	"0x00100000"

Review

Quoted 10 lines
(quoted diff) +10 -0
+#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!

Quoted 2 lines

Kim

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

#6 lore raw

PATCH 1/1 Add MVBC_P board

Quoted 4 lines · On Tue, 08 Jul 2008 16:35:00 +0200 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.

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.

Quoted 2 lines

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

author
#7 lore raw

PATCH 1/1 Add MVBC_P board

Quoted 23 lines · Kim Phillips
Quoted 5 lines · On Tue, 08 Jul 2008 16:35:00 +0200 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.

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;
Quoted 7 lines

#endif
#endif
}

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

ok - you're right.
Will fix it along with other out_* issues.

Quoted 9 lines
Quoted 3 lines

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