Kim,
On Thursday 11 June 2009 17:15:48 Kim Phillips wrote:
> On Wed, 10 Jun 2009 19:09:48 +0200
>
> Stefan Roese <s...@denx.de> wrote:
> > From: Reinhard Arlt <reinhard.a...@esd-electronics.com>
> >
> > From: Reinhard Arlt <reinhard.a...@esd-electronics.com>
> >
> > This patch adds support for the esd VME8349 board equipped with the
> > MPC8349. It's a VME PMC carrier board equipped with the Tundra
> > TSI148 VME-bridge.
> >
> > Signed-off-by: Reinhard Arlt <reinhard.a...@esd-electronics.com>
> > Signed-off-by: Stefan Roese <s...@denx.de>
> > ---
> > MAINTAINERS | 2 +
> > MAKEALL | 1 +
> > Makefile | 2 +
> > board/esd/vme8349/Makefile | 49 ++++
> > board/esd/vme8349/aduc.c | 522 +++++++++++++++++++++++++++++++++++
> > board/esd/vme8349/caddy.c | 203 ++++++++++++++
> > board/esd/vme8349/caddy.h | 78 ++++++
> > board/esd/vme8349/config.mk | 27 ++
> > board/esd/vme8349/pci.c | 409 +++++++++++++++++++++++++++
> > board/esd/vme8349/vme8349.c | 129 +++++++++
> > drivers/pci/pci_auto.c | 2 +
> > include/configs/vme8349.h | 638
> > +++++++++++++++++++++++++++++++++++++++++++
>
> might want to add a doc/README.vme8349 at some point.
Currently not planned. I have to admit that I usually don't add such a readme.
It's not an evaluation board after all.
> > +++ b/board/esd/vme8349/Makefile
> > @@ -0,0 +1,49 @@
> > +#
> > +# Copyright (c) 2009 esd gmbh hannover germany.
>
> I sincerely doubt esd wrote this file from scratch - please retain
> original copyrights.
OK, will do.
> > +++ b/board/esd/vme8349/aduc.c
> > @@ -0,0 +1,522 @@
> > +/*
> > + * aduc.c -- esd VME8349 board support for aduc848 monitor.
> > + * Copyright (c) 2008-2009 esd gmbh.
> > + *
> > + * Reinhard Arlt <reinhard.a...@esd-electronics.com>
> > + * Based on board/mpc8349emds/mpc8349emds.c (and previous 834x
> > releases.)
>
> this suggests a similar comment to mine above...and, er...there were no
> 'previous 834x releases' -- that was the first (it was renamed from
> ads). Now it'sunder board/freescale, btw.
This file written from scratch. Only the copyright header was copied. I'll
clean up the header in the next version.
> > +static uint8_t aduc_execute_long(uint8_t par0, uint8_t par1, uint8_t
> > cmd, uint32_t timeout) +{
> > + uint32_t l;
> > + unsigned char cmmd[8];
> > +
> > + cmmd[0] = 0;
> > + cmmd[1] = 3;
> > + cmmd[2] = par0;
> > + cmmd[3] = par1;
> > + cmmd[4] = cmd;
> > +
> > + if (i2c_write(0x78, 0x40, 1, (unsigned char *)cmmd, 5) != 0) {
>
> too many magic numbers...please use macros to define these constants.
All your comments to this file are valid remarks. Thanks. This file was work-
in-progress at the time I first published the patches. I'll remove this file
in the next version. It will be added when its finally finished.
<snip>
> > diff --git a/board/esd/vme8349/caddy.c b/board/esd/vme8349/caddy.c
> >
> > + while (ctrlc() == 0) {
> > + if (caddy_interface->cmd_in != caddy_interface->cmd_out) {
> > + CADDY_CMD *caddy_cmd;
> > + uint32_t result[5];
> > + uint16_t data16;
> > + uint8_t data8;
> > + uint32_t status;
> > + pci_dev_t dev;
>
> regroup, don't align.
>
> would it be easier to read if these were at the top level of the
> function (here and elsewhere)?
OK, will change.
> > + result[0] = 0;
> > + result[1] = 0;
> > + result[2] = 0;
> > + result[3] = 0;
> > + result[4] = 0;
>
> memset?
Yes.
> > +U_BOOT_CMD(
> > + caddy, 2, 0, do_caddy,
> > + "Start Caddy server.",
> > + "Start Caddy server with Data structure a given addr\n"
> > + );
> >
> >
> > diff --git a/board/esd/vme8349/caddy.h b/board/esd/vme8349/caddy.h
> >
> > +typedef enum {
> > + CADDY_CMD_IO_READ_8,
> > + CADDY_CMD_IO_READ_16,
> > + CADDY_CMD_IO_READ_32,
> > + CADDY_CMD_IO_WRITE_8,
> > + CADDY_CMD_IO_WRITE_16,
> > + CADDY_CMD_IO_WRITE_32,
> > + CADDY_CMD_CONFIG_READ_8,
> > + CADDY_CMD_CONFIG_READ_16,
> > + CADDY_CMD_CONFIG_READ_32,
> > + CADDY_CMD_CONFIG_WRITE_8,
> > + CADDY_CMD_CONFIG_WRITE_16,
> > + CADDY_CMD_CONFIG_WRITE_32,
> > +} CADDY_CMDS;
> > +
> > +
> > +typedef struct {
> > + uint32_t cmd;
> > + uint32_t issue;
> > + uint32_t addr;
> > + uint32_t par[5];
> > +} CADDY_CMD;
> > +
> > +typedef struct {
> > + uint32_t answer;
> > + uint32_t issue;
> > + uint32_t status;
> > + uint32_t par[5];
> > +} CADDY_ANSWER;
> > +
> > +typedef struct {
> > + uint8_t magic[16];
> > + uint32_t cmd_in;
> > + uint32_t cmd_out;
> > + uint32_t heartbeat;
> > + uint32_t reserved1;
> > + CADDY_CMD cmd[CMD_SIZE];
> > + uint32_t answer_in;
> > + uint32_t answer_out;
> > + uint32_t reserved2;
> > + uint32_t reserved3;
> > + CADDY_ANSWER answer[CMD_SIZE];
> > +} CADDY_INTERFACE;
>
> please remove all typedefs (see CodingStyle Ch. 5). Use 'struct xxx'
> in each instance instead.
We really would like to keep these typedefs. The reason for this is that
multiple customers already are using this header for their work. And
maintaining multiple versions of this file doesn't sound like a good idea.
> > +++ b/board/esd/vme8349/config.mk
> > @@ -0,0 +1,27 @@
> > +#
> > +# Copyright (c) 2009 esd gmbh hannover germany.
>
> please respect the original copyright holder(s).
OK.
> > diff --git a/board/esd/vme8349/pci.c b/board/esd/vme8349/pci.c
> >
> > +#ifdef CONFIG_PCI
> > +
> > +/* System RAM mapped to PCI space */
> > +#define CONFIG_PCI_SYS_MEM_BUS CONFIG_SYS_SDRAM_BASE
> > +#define CONFIG_PCI_SYS_MEM_PHYS CONFIG_SYS_SDRAM_BASE
> > +
> > +#ifndef CONFIG_PCI_PNP
> > +static struct pci_config_table pci_mpc8349emds_config_table[] = {
>
> please convert to use 83XX_GENERIC_PCI code.
OK, will try in next version.
> > diff --git a/board/esd/vme8349/vme8349.c b/board/esd/vme8349/vme8349.c
> > new file mode 100644
> > index 0000000..cd3d684
> > --- /dev/null
> > +++ b/board/esd/vme8349/vme8349.c
> > @@ -0,0 +1,129 @@
> > +/*
> > + * vme8349.c -- esd VME8349 board support.
> > + * Copyright (c) 2008-2009 esd gmbh.
>
> respect original copyright holders please
OK.
> > diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c
> > index c20b981..393e44d 100644
> > --- a/drivers/pci/pci_auto.c
> > +++ b/drivers/pci/pci_auto.c
> > @@ -403,6 +403,7 @@ int pciauto_config_device(struct pci_controller
> > *hose, pci_dev_t dev) PCI_DEV(dev));
> > break;
> > #endif
> > +#ifndef CONFIG_VME8349
> > #ifdef CONFIG_MPC834X
>
> #if defined(CONFIG_MPC834x) && !defined(CONFIG_VME8349)
>
> I don't know - this might need to be changed to ifdef
> CONFIG_MPC8349EMDS...
Should I change this to CONFIG_MPC8349EMDS? Or use
#if defined(CONFIG_MPC834x) && !defined(CONFIG_VME8349)
for now?
> > case PCI_CLASS_BRIDGE_OTHER:
> > /*
> > +++ b/include/configs/vme8349.h
> > @@ -0,0 +1,638 @@
> > +/*
> > + * esd vme8349 U-Boot configuration file.
> > + * Copyright (c) 2008, 2009 esd gmbh Hannover Germany
> > + *
> > + * reinhard.a...@esd-electronics.cd
> > + * Based on the MPC8349EMDS config.
>
> right, but you didn't carry the copyrights with the code :(.
Will fix.
> > +/*
> > + * High Level Configuration Options
> > + */
> > +#define CONFIG_E300 1 /* E300 Family */
> > +#define CONFIG_MPC83XX 1 /* MPC83XX family */
> > +#define CONFIG_MPC834X 1 /* MPC834X family */
>
> this is now CONFIG_MPC83xx. please rebase on top of u-boot-mpc83xx'
> next branch.
OK.
> > +/* System IO Config */
> > +#define CONFIG_SYS_SICRH SICRH_TSOBI1
>
> I'm guessing this is copied and should be 0 (see latest commit on
> mpc83xx' master).
I'm guessing as well. Will change.
> > +/*
> > + * Environment Configuration
> > + */
> > +#define CONFIG_ENV_OVERWRITE
> > +
> > +#if defined(CONFIG_TSEC_ENET)
> > +#define CONFIG_HAS_ETH0
> > +#define CONFIG_ETHADDR 00:a0:1e:a0:13:8d
> > +#define CONFIG_HAS_ETH1
> > +#define CONFIG_ETH1ADDR 00:a0:1e:a0:13:8e
> > +#endif
> > +
> > +#define CONFIG_IPADDR 192.168.1.234
> > +
>
> we don't hardcode mac & ip addresses any more - please remove.
Ups. Missed this. Will remove.
Thanks for your review Kim. I'll send a new version soon.
Best regards,
Stefan
=====================================================================
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: off...@denx.de
=====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
