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.

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

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

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

> +             printf("i2c_write cmmd failed\n");
> +             I2C_SET_BUS(old_bus);
> +             return 0xfe;

ditto

> +     }
> +     i2c_read(0x78, 0x40, 1, (unsigned char *)cmmd, 1);

ditto

> +     cmmd[0] = 0x00;
> +     for (l = 1; (l < timeout) && (cmmd[0] == 0); l++) {
> +             if (i2c_read(0x78, 0x40, 1, (unsigned char *)cmmd, 1) != 0) {

ditto..you get the idea

> +static uint8_t aduc_execute(uint8_t par0, uint8_t par1, uint8_t cmd)
> +{
> +     return aduc_execute_long(par0, par1, cmd, 600);
> +}

don't see the purpose of this - rename aduc_execute_long() to
aduc_execute(), after having swallowed the default timeout value?

> +int aduc_download_block(unsigned long addr, unsigned long len)
> +{
> +     unsigned char buf[10];
> +     unsigned long m;
> +
> +     m      = addr;

m = addr;

add a newline if you feel that it 'looks misaligned' (I don't).

> +     for (m = 0; m < 50; m++) {
> +             if (i2c_read(0x78, 0x40, 1, buf, 1) != 0) {
> +                     printf("i2c_read status failed\n");
> +                     return 3;
> +             }
> +             if (buf[0] == 0xff)
> +                     udelay(100000);
> +             else if (buf[0] == 0)
> +                     return 0;
> +             else
> +                     return 4;
> +     }
> +     return 5;

not reached?  wait, does that loop ever execute more than once?  please
clean this up.

> +int do_show_aduc(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +     uchar buf[32];
> +     int   old_bus;
> +
> +     old_bus = I2C_GET_BUS();

can add assignment to declaration above

> +     I2C_SET_BUS(1);
> +
> +     if (i2c_read(0x78, 0, 1, buf, 32) != 0) {
> +             printf("Error reading from ADUC\n");

no error return?

> +int do_cmd_aduc(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +     uint8_t par0, par1, cmd, stat;
> +
> +     if (argc < 4) {
> +             puts("Missing parameter\n");
> +             return 1;
> +     }
> +
> +     par0 = simple_strtoul(argv[1], NULL, 16);
> +     par1 = simple_strtoul(argv[2], NULL, 16);
> +     cmd  = simple_strtoul(argv[3], NULL, 16);
> +
> +     printf("%02x %02x %02x\n", par0, par1, cmd);
> +
> +     old_bus = I2C_GET_BUS();
> +     I2C_SET_BUS(1);
> +
> +     stat = aduc_execute(par0, par1, cmd);
> +
> +     printf("\n");
> +     if (stat != 0x01)
> +             printf("Got status %02x\n", stat);
> +     I2C_SET_BUS(old_bus);
> +     return 0;

blank line before the return please.

did you also want to return stat?

> +}
> +
> +

take one from here :)

> +int do_fpga_aduc(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +     uint8_t              stat;
> +     uint8_t              buffer[512 + 16];
> +     unsigned long        addr;
> +     unsigned long        size;
> +     const unsigned char *fpgadata;
> +     int                  i, index, len;

please don't align declarations like this - if a type is added with a
longer name, all lines have to be modified, reducing patch
reviewability.  Some vars can be regrouped on the same line here, too.

> +     fpgadata = (const unsigned char *)addr;
> +/* display infos on fpgaimage */

alignment.  fyi, "infos" is franglais.  In English, it's just "info".

> +     index = 15;
> +     for (i = 0; i < 4; i++) {
> +             len = fpgadata[index];
> +             printf("FPGA: %s\n", &(fpgadata[index + 1]));
> +             index += len + 3;
> +     }
> +
> +/* search for preamble 0xFFFFFFFF */

alignment

> +     I2C_SET_BUS(old_bus);
> +     return 0;

blank line before return please

> +int do_image_aduc(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])

there is a lot of code this function has in common with do_fpga_aduc()
- please refactor.

> +int do_load_aduc(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +     int                  slot;
> +     uint8_t              stat;
> +
> +     slot = simple_strtoul(argv[1], NULL, 16);
> +
> +     old_bus = I2C_GET_BUS();
> +     I2C_SET_BUS(1);
> +
> +/* Load slot from FLASH to FPGA */

alignment

> +     stat = aduc_execute_long(slot, 0x00, 0x04, 30000);
> +     printf("\n");
> +     if (stat != 0x01) {
> +             printf("Got status at LOAD_SLOT:%02x\n", stat);
> +             I2C_SET_BUS(old_bus);
> +             return 1;
> +     }
> +     I2C_SET_BUS(old_bus);

insert blank line here please

> +     return 0;

if aduc_execute returns 0 on success, just make the stat check do the
printf, set the bus to the old bus once, and return stat.


> 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)?

> +                     result[0] = 0;
> +                     result[1] = 0;
> +                     result[2] = 0;
> +                     result[3] = 0;
> +                     result[4] = 0;

memset?

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

> +++ b/board/esd/vme8349/config.mk
> @@ -0,0 +1,27 @@
> +#
> +# Copyright (c) 2009 esd gmbh hannover germany.

please respect the original copyright holder(s).

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

> 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

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

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

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

> +/* 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).

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

Thanks,

Kim
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to