[U-Boot] [PATCH] mpc83xx: Add esd VME8349 board support
Kim Phillips
kim.phillips at freescale.com
Thu Jun 11 17:15:48 CEST 2009
On Wed, 10 Jun 2009 19:09:48 +0200
Stefan Roese <sr at denx.de> wrote:
> From: Reinhard Arlt <reinhard.arlt at esd-electronics.com>
>
> From: Reinhard Arlt <reinhard.arlt at 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.arlt at esd-electronics.com>
> Signed-off-by: Stefan Roese <sr at 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.arlt at 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.arlt at 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
More information about the U-Boot
mailing list