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