[U-Boot] [PATCH v2 0/10] Update gdsys ppc4xx-based boards
From: Dirk Eibach dirk.eibach@gdsys.cc
This series depends on the "Bring in new I2C framework" series by Heiko Schocher.
Changes in v2: - fpga_state has been moved to arch_global_data - include cmd_fpgad in iocon - move cmd_fpgad to common and fix whitespace issues - update email account - use multibus soft-i2c in iocon
Dirk Eibach (10): powerpc/ppc4xx: Add generic accessor functions for gdsys FPGA powerpc/ppc4xx: Add gdsys mclink interface powerpc/ppc4xx: Add fpgad command for dumping gdsys fpga registers powerpc/ppc4xx: Use generic FPGA accessors in gdsys common code powerpc/ppc4xx: Support gdsys multichannel iocon hardware powerpc/ppc4xx: Use generic FPGA accessors on all gdsys 405ep/ex boards powerpc/ppc4xx: Fixup phy erratum on gdsys iocon hardware powerpc/ppc4xx: Increase timeout for gdsys mclink bus startup powerpc/ppc4xx: Consider gdsys FPGA OSD size powerpc/ppc4xx: Remove CONFIG_SYS_FLASH_PROTECTION from gdsys boards
board/gdsys/405ep/405ep.c | 12 +- board/gdsys/405ep/dlvision-10g.c | 20 ++- board/gdsys/405ep/io.c | 20 ++- board/gdsys/405ep/iocon.c | 467 ++++++++++++++++++++++++++++++++++---- board/gdsys/405ep/neo.c | 17 +- board/gdsys/405ex/405ex.c | 14 +- board/gdsys/405ex/io64.c | 38 ++-- board/gdsys/common/Makefile | 2 +- board/gdsys/common/mclink.c | 142 ++++++++++++ board/gdsys/common/mclink.h | 31 +++ board/gdsys/common/osd.c | 101 +++++---- common/Makefile | 1 + common/cmd_fpgad.c | 99 ++++++++ include/configs/dlvision-10g.h | 3 +- include/configs/dlvision.h | 3 +- include/configs/io.h | 3 +- include/configs/iocon.h | 50 ++++- include/configs/neo.h | 3 +- include/gdsys_fpga.h | 20 ++- 19 files changed, 892 insertions(+), 154 deletions(-) create mode 100644 board/gdsys/common/mclink.c create mode 100644 board/gdsys/common/mclink.h create mode 100644 common/cmd_fpgad.c
From: Dirk Eibach eibach@gdsys.de
A set of accessor functions was added to be able to access not only memory mapped FPGA in a generic way.
Signed-off-by: Dirk Eibach dirk.eibach@gdsys.cc --- Changes in v2: None
include/gdsys_fpga.h | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/include/gdsys_fpga.h b/include/gdsys_fpga.h index 1758d74..c12a31d 100644 --- a/include/gdsys_fpga.h +++ b/include/gdsys_fpga.h @@ -32,9 +32,14 @@ enum { FPGA_STATE_PLATFORM = 1 << 2, };
+#define REG(reg) offsetof(struct ihs_fpga, reg) + int get_fpga_state(unsigned dev); void print_fpga_state(unsigned dev);
+void fpga_set_reg(unsigned int fpga, u16 reg, u16 data); +u16 fpga_get_reg(unsigned int fpga, u16 reg); + struct ihs_gpio { u16 read; u16 clear;
Dear dirk.eibach@gdsys.cc,
In message 1367847325-21463-2-git-send-email-dirk.eibach@gdsys.cc you wrote:
From: Dirk Eibach eibach@gdsys.de
A set of accessor functions was added to be able to access not only memory mapped FPGA in a generic way.
Signed-off-by: Dirk Eibach dirk.eibach@gdsys.cc
Changes in v2: None
include/gdsys_fpga.h | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
This patch makes no sense. Please squash it with the patch that actually uses the new definition.
Best regards,
Wolfgang Denk
Dear dirk.eibach@gdsys.cc,
In message 1367847325-21463-2-git-send-email-dirk.eibach@gdsys.cc you wrote:
From: Dirk Eibach eibach@gdsys.de
A set of accessor functions was added to be able to access not only memory mapped FPGA in a generic way.
Please squash with patch that actually uses this define.
Best regards,
Wolfgang Denk
From: Dirk Eibach eibach@gdsys.de
mclink is a serial interface for communication between gdsys FPGA.
Signed-off-by: Dirk Eibach dirk.eibach@gdsys.cc
--- Changes in v2: - update email account
board/gdsys/common/Makefile | 2 +- board/gdsys/common/mclink.c | 140 +++++++++++++++++++++++++++++++++++++++++++ board/gdsys/common/mclink.h | 31 ++++++++++ 3 files changed, 172 insertions(+), 1 deletions(-) create mode 100644 board/gdsys/common/mclink.c create mode 100644 board/gdsys/common/mclink.h
diff --git a/board/gdsys/common/Makefile b/board/gdsys/common/Makefile index 05dd65d..6f09550 100644 --- a/board/gdsys/common/Makefile +++ b/board/gdsys/common/Makefile @@ -31,7 +31,7 @@ LIB = $(obj)lib$(VENDOR).o
COBJS-$(CONFIG_IO) += miiphybb.o COBJS-$(CONFIG_IO64) += miiphybb.o -COBJS-$(CONFIG_IOCON) += osd.o +COBJS-$(CONFIG_IOCON) += osd.o mclink.o COBJS-$(CONFIG_DLVISION_10G) += osd.o
COBJS := $(COBJS-y) diff --git a/board/gdsys/common/mclink.c b/board/gdsys/common/mclink.c new file mode 100644 index 0000000..9a4f3e9 --- /dev/null +++ b/board/gdsys/common/mclink.c @@ -0,0 +1,140 @@ +/* + * (C) Copyright 2012 + * Dirk Eibach, Guntermann & Drunck GmbH, dirk.eibach@gdsys.cc + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <common.h> +#include <asm/io.h> +#include <errno.h> + +#include <gdsys_fpga.h> + +enum { + MCINT_SLAVE_LINK_CHANGED_EV = 1 << 7, + MCINT_TX_ERROR_EV = 1 << 9, + MCINT_TX_BUFFER_FREE = 1 << 10, + MCINT_TX_PACKET_TRANSMITTED_EV = 1 << 11, + MCINT_RX_ERROR_EV = 1 << 13, + MCINT_RX_CONTENT_AVAILABLE = 1 << 14, + MCINT_RX_PACKET_RECEIVED_EV = 1 << 15, +}; + +int mclink_probe(void) +{ + unsigned int k; + unsigned int ctr = 0; + int slaves = 0; + + for (k = 0; k < CONFIG_SYS_MCLINK_MAX; ++k) { + int timeout = 0; + + if (!(fpga_get_reg(k, REG(mc_status)) & (1 << 15))) + break; + + fpga_set_reg(k, REG(mc_control), 0x8000); + + while (!(fpga_get_reg(k, REG(mc_status)) & (1 << 14))) { + udelay(100); + if (ctr++ > 3) { + timeout = 1; + break; + } + } + if (timeout) + break; + + slaves++; + } + + return slaves; +} + +int mclink_send(u8 slave, u16 addr, u16 data) +{ + unsigned int ctr = 0; + u16 int_status; + u16 rx_cmd_status; + u16 rx_cmd; + + /* reset interrupt status */ + int_status = fpga_get_reg(0, REG(mc_int)); + fpga_set_reg(0, REG(mc_int), int_status); + + /* send */ + fpga_set_reg(0, REG(mc_tx_address), addr); + fpga_set_reg(0, REG(mc_tx_data), data); + fpga_set_reg(0, REG(mc_tx_cmd), (slave & 0x03) << 14); + fpga_set_reg(0, REG(mc_control), 0x8001); + + /* wait for reply */ + while (!(fpga_get_reg(0, REG(mc_int)) & MCINT_RX_PACKET_RECEIVED_EV)) { + udelay(100); + if (ctr++ > 3) + return -ETIMEDOUT; + } + + rx_cmd_status = fpga_get_reg(0, REG(mc_rx_cmd_status)); + rx_cmd = (rx_cmd_status >> 12) & 0x03; + if (rx_cmd != 0) + printf("mclink_send: received cmd %d, expected %d\n", rx_cmd, + 0); + + return 0; +} + +int mclink_receive(u8 slave, u16 addr, u16 *data) +{ + u16 rx_cmd_status; + u16 rx_cmd; + unsigned int ctr = 0; + + /* send read request */ + fpga_set_reg(0, REG(mc_tx_address), addr); + fpga_set_reg(0, REG(mc_tx_cmd), + ((slave & 0x03) << 14) | (1 << 12) | (1 << 0)); + fpga_set_reg(0, REG(mc_control), 0x8001); + + /* wait for reply */ + while (!(fpga_get_reg(0, REG(mc_int)) & MCINT_RX_CONTENT_AVAILABLE)) { + udelay(100); + if (ctr++ > 3) + return -ETIMEDOUT; + } + + /* check reply */ + rx_cmd_status = fpga_get_reg(0, REG(mc_rx_cmd_status)); + if ((rx_cmd_status >> 14) != slave) { + printf("mclink_receive: reply from slave %d, expected %d\n", + rx_cmd_status >> 14, slave); + return -EINVAL; + } + + rx_cmd = (rx_cmd_status >> 12) & 0x03; + if (rx_cmd != 1) { + printf("mclink_send: received cmd %d, expected %d\n", + rx_cmd, 1); + return -EIO; + } + + *data = fpga_get_reg(0, REG(mc_rx_data)); + + return 0; +} diff --git a/board/gdsys/common/mclink.h b/board/gdsys/common/mclink.h new file mode 100644 index 0000000..47a62b4 --- /dev/null +++ b/board/gdsys/common/mclink.h @@ -0,0 +1,31 @@ +/* + * (C) Copyright 2012 + * Dirk Eibach, Guntermann & Drunck GmbH, dirk.eibach@gdsys.cc + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef _MCLINK_H_ +#define _MCLINK_H_ + +int mclink_probe(void); +int mclink_send(u8 slave, u16 addr, u16 data); +int mclink_receive(u8 slave, u16 addr, u16 *data); + +#endif
From: Dirk Eibach eibach@gdsys.de
Signed-off-by: Dirk Eibach dirk.eibach@gdsys.cc
--- Changes in v2: - move cmd_fpgad to common and fix whitespace issues - update email account
common/Makefile | 1 + common/cmd_fpgad.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 0 deletions(-) create mode 100644 common/cmd_fpgad.c
diff --git a/common/Makefile b/common/Makefile index 0e0fff1..edabbad 100644 --- a/common/Makefile +++ b/common/Makefile @@ -110,6 +110,7 @@ COBJS-$(CONFIG_CMD_FLASH) += cmd_flash.o ifdef CONFIG_FPGA COBJS-$(CONFIG_CMD_FPGA) += cmd_fpga.o endif +COBJS-$(CONFIG_CMD_FPGAD) += cmd_fpgad.o COBJS-$(CONFIG_CMD_FS_GENERIC) += cmd_fs.o COBJS-$(CONFIG_CMD_GETTIME) += cmd_gettime.o COBJS-$(CONFIG_CMD_GPIO) += cmd_gpio.o diff --git a/common/cmd_fpgad.c b/common/cmd_fpgad.c new file mode 100644 index 0000000..0100f83 --- /dev/null +++ b/common/cmd_fpgad.c @@ -0,0 +1,99 @@ +/* + * (C) Copyright 2013 + * Dirk Eibach, Guntermann & Drunck GmbH, dirk.eibach@gdsys.cc + * + * based on cmd_mem.c + * (C) Copyright 2000 + * Wolfgang Denk, DENX Software Engineering, wd@denx.de. + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <common.h> +#include <command.h> + +#include <gdsys_fpga.h> + +static uint dp_last_fpga; +static uint dp_last_addr; +static uint dp_last_length = 0x40; + +/* + * FPGA Memory Display + * + * Syntax: + * fpgad {fpga} {addr} {len} + */ +#define DISP_LINE_LEN 16 +int do_fpga_md(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + unsigned int k; + unsigned int fpga; + ulong addr, length; + int rc = 0; + u16 linebuf[DISP_LINE_LEN/sizeof(u16)]; + + /* + * We use the last specified parameters, unless new ones are + * entered. + */ + fpga = dp_last_fpga; + addr = dp_last_addr; + length = dp_last_length; + + if (argc < 3) + return CMD_RET_USAGE; + + if ((flag & CMD_FLAG_REPEAT) == 0) { + /* + * FPGA is specified since argc > 2 + */ + fpga = simple_strtoul(argv[1], NULL, 16); + + /* + * Address is specified since argc > 2 + */ + addr = simple_strtoul(argv[2], NULL, 16); + + /* + * If another parameter, it is the length to display. + * Length is the number of objects, not number of bytes. + */ + if (argc > 3) + length = simple_strtoul(argv[3], NULL, 16); + } + + /* Print the lines. */ + for (k = 0; k < DISP_LINE_LEN / sizeof(u16); ++k) + linebuf[k] = fpga_get_reg(fpga, addr + k * sizeof(u16)); + print_buffer(addr, (void *)linebuf, sizeof(u16), + length, DISP_LINE_LEN / sizeof(u16)); + addr += sizeof(u16)*length; + + dp_last_fpga = fpga; + dp_last_addr = addr; + dp_last_length = length; + return rc; +} + +U_BOOT_CMD( + fpgad, 4, 1, do_fpga_md, + "fpga register display", + "fpga address [# of objects]" +);
From: Dirk Eibach eibach@gdsys.de
Signed-off-by: Dirk Eibach dirk.eibach@gdsys.cc --- Changes in v2: None
board/gdsys/405ep/405ep.c | 12 +++---- board/gdsys/common/osd.c | 70 +++++++++++++++++++++----------------------- 2 files changed, 38 insertions(+), 44 deletions(-)
diff --git a/board/gdsys/405ep/405ep.c b/board/gdsys/405ep/405ep.c index 6221171..e5fbd2e 100644 --- a/board/gdsys/405ep/405ep.c +++ b/board/gdsys/405ep/405ep.c @@ -106,22 +106,20 @@ int board_early_init_r(void) gd405ep_set_fpga_reset(0);
for (k = 0; k < CONFIG_SYS_FPGA_COUNT; ++k) { - struct ihs_fpga *fpga = - (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(k); #ifdef CONFIG_SYS_FPGA_NO_RFL_HI - u16 *reflection_target = &fpga->reflection_low; + u16 reflection_target = REG(reflection_low); #else - u16 *reflection_target = &fpga->reflection_high; + u16 reflection_target = REG(reflection_high); #endif /* * wait for fpga out of reset */ ctr = 0; while (1) { - out_le16(&fpga->reflection_low, - REFLECTION_TESTPATTERN); + fpga_set_reg(k, REG(reflection_low), + REFLECTION_TESTPATTERN);
- if (in_le16(reflection_target) == + if (fpga_get_reg(k, reflection_target) == REFLECTION_TESTPATTERN_INV) break;
diff --git a/board/gdsys/common/osd.c b/board/gdsys/common/osd.c index a192c98..81c8136 100644 --- a/board/gdsys/common/osd.c +++ b/board/gdsys/common/osd.c @@ -22,7 +22,7 @@ */
#include <common.h> -#include <i2c.h> +#include "bb_i2c.h" #include <asm/io.h>
#include <gdsys_fpga.h> @@ -67,37 +67,34 @@ enum { CH7301_DSP = 0x56, /* DVI Sync polarity Register */ };
+unsigned int max_osd_screen = CONFIG_SYS_OSD_SCREENS - 1; + #if defined(CONFIG_SYS_ICS8N3QV01) || defined(CONFIG_SYS_SIL1178) static void fpga_iic_write(unsigned screen, u8 slave, u8 reg, u8 data) { - struct ihs_fpga *fpga = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(screen); - struct ihs_i2c *i2c = &fpga->i2c; - - while (in_le16(&fpga->extended_interrupt) & (1 << 12)) + while (fpga_get_reg(screen, REG(extended_interrupt)) & (1 << 12)) ; - out_le16(&i2c->write_mailbox_ext, reg | (data << 8)); - out_le16(&i2c->write_mailbox, 0xc400 | (slave << 1)); + fpga_set_reg(screen, REG(i2c.write_mailbox_ext), reg | (data << 8)); + fpga_set_reg(screen, REG(i2c.write_mailbox), 0xc400 | (slave << 1)); }
static u8 fpga_iic_read(unsigned screen, u8 slave, u8 reg) { - struct ihs_fpga *fpga = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(screen); - struct ihs_i2c *i2c = &fpga->i2c; unsigned int ctr = 0;
- while (in_le16(&fpga->extended_interrupt) & (1 << 12)) + while (fpga_get_reg(screen, REG(extended_interrupt)) & (1 << 12)) ; - out_le16(&fpga->extended_interrupt, 1 << 14); - out_le16(&i2c->write_mailbox_ext, reg); - out_le16(&i2c->write_mailbox, 0xc000 | (slave << 1)); - while (!(in_le16(&fpga->extended_interrupt) & (1 << 14))) { + fpga_set_reg(screen, REG(extended_interrupt), 1 << 14); + fpga_set_reg(screen, REG(i2c.write_mailbox_ext), reg); + fpga_set_reg(screen, REG(i2c.write_mailbox), 0xc000 | (slave << 1)); + while (!(fpga_get_reg(screen, REG(extended_interrupt)) & (1 << 14))) { udelay(100000); if (ctr++ > 5) { printf("iic receive timeout\n"); break; } } - return in_le16(&i2c->read_mailbox_ext) >> 8; + return fpga_get_reg(screen, REG(i2c.read_mailbox_ext)) >> 8; } #endif
@@ -129,7 +126,6 @@ static void mpc92469ac_calc_parameters(unsigned int fout,
static void mpc92469ac_set(unsigned screen, unsigned int fout) { - struct ihs_fpga *fpga = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(screen); unsigned int n; unsigned int m; unsigned int bitval = 0; @@ -150,7 +146,7 @@ static void mpc92469ac_set(unsigned screen, unsigned int fout) break; }
- out_le16(&fpga->mpc3w_control, (bitval << 9) | m); + fpga_set_reg(screen, REG(mpc3w_control), (bitval << 9) | m); } #endif
@@ -265,14 +261,13 @@ static void ics8n3qv01_set(unsigned screen, unsigned int fout) static int osd_write_videomem(unsigned screen, unsigned offset, u16 *data, size_t charcount) { - struct ihs_fpga *fpga = - (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(screen); unsigned int k;
for (k = 0; k < charcount; ++k) { if (offset + k >= BUFSIZE) return -1; - out_le16(&fpga->videomem + offset + k, data[k]); + fpga_set_reg(screen, REG(videomem) + sizeof(u16) * (offset + k), + data[k]); }
return charcount; @@ -282,7 +277,7 @@ static int osd_print(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { unsigned screen;
- for (screen = 0; screen < CONFIG_SYS_OSD_SCREENS; ++screen) { + for (screen = 0; screen <= max_osd_screen; ++screen) { unsigned x; unsigned y; unsigned charcount; @@ -318,10 +313,8 @@ static int osd_print(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
int osd_probe(unsigned screen) { - struct ihs_fpga *fpga = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(screen); - struct ihs_osd *osd = &fpga->osd; - u16 version = in_le16(&osd->version); - u16 features = in_le16(&osd->features); + u16 version = fpga_get_reg(screen, REG(osd.version)); + u16 features = fpga_get_reg(screen, REG(osd.features)); unsigned width; unsigned height; u8 value; @@ -333,16 +326,16 @@ int osd_probe(unsigned screen) screen, version/100, version%100, width, height);
#ifdef CONFIG_SYS_CH7301 - value = i2c_reg_read(CH7301_I2C_ADDR, CH7301_DID); + value = bb_i2c_reg_read(screen, CH7301_I2C_ADDR, CH7301_DID); if (value != 0x17) { printf(" Probing CH7301 failed, DID %02x\n", value); return -1; } - i2c_reg_write(CH7301_I2C_ADDR, CH7301_TPCP, 0x08); - i2c_reg_write(CH7301_I2C_ADDR, CH7301_TPD, 0x16); - i2c_reg_write(CH7301_I2C_ADDR, CH7301_TPF, 0x60); - i2c_reg_write(CH7301_I2C_ADDR, CH7301_DC, 0x09); - i2c_reg_write(CH7301_I2C_ADDR, CH7301_PM, 0xc0); + bb_i2c_reg_write(screen, CH7301_I2C_ADDR, CH7301_TPCP, 0x08); + bb_i2c_reg_write(screen, CH7301_I2C_ADDR, CH7301_TPD, 0x16); + bb_i2c_reg_write(screen, CH7301_I2C_ADDR, CH7301_TPF, 0x60); + bb_i2c_reg_write(screen, CH7301_I2C_ADDR, CH7301_DC, 0x09); + bb_i2c_reg_write(screen, CH7301_I2C_ADDR, CH7301_PM, 0xc0); #endif
#ifdef CONFIG_SYS_MPC92469AC @@ -372,12 +365,15 @@ int osd_probe(unsigned screen) fpga_iic_write(screen, SIL1178_MASTER_I2C_ADDRESS, 0x08, 0x37); #endif
- out_le16(&fpga->videocontrol, 0x0002); - out_le16(&osd->control, 0x0049); + fpga_set_reg(screen, REG(videocontrol), 0x0002); + fpga_set_reg(screen, REG(osd.control), 0x0049); + + fpga_set_reg(screen, REG(osd.xy_size), ((32 - 1) << 8) | (16 - 1)); + fpga_set_reg(screen, REG(osd.x_pos), 0x007f); + fpga_set_reg(screen, REG(osd.y_pos), 0x005f);
- out_le16(&osd->xy_size, ((32 - 1) << 8) | (16 - 1)); - out_le16(&osd->x_pos, 0x007f); - out_le16(&osd->y_pos, 0x005f); + if (screen > max_osd_screen) + max_osd_screen = screen;
return 0; } @@ -386,7 +382,7 @@ int osd_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { unsigned screen;
- for (screen = 0; screen < CONFIG_SYS_OSD_SCREENS; ++screen) { + for (screen = 0; screen <= max_osd_screen; ++screen) { unsigned x; unsigned y; unsigned k;
Dear dirk.eibach@gdsys.cc,
In message 1367847325-21463-5-git-send-email-dirk.eibach@gdsys.cc you wrote:
...
fpga_set_reg(k, REG(reflection_low),REFLECTION_TESTPATTERN);
This breaks bisecting. You reference functions which you only add in a later patch.
Please fix this.
Best regards,
Wolfgang Denk
On Mon, May 06, 2013 at 04:07:15PM +0200, Wolfgang Denk wrote:
Dear dirk.eibach@gdsys.cc,
In message 1367847325-21463-5-git-send-email-dirk.eibach@gdsys.cc you wrote:
...
fpga_set_reg(k, REG(reflection_low),REFLECTION_TESTPATTERN);This breaks bisecting. You reference functions which you only add in a later patch.
Note that 'git rebase -i' and the exec keyword can help you test bisectability.
Hi Tom,
On Mon, May 06, 2013 at 04:07:15PM +0200, Wolfgang Denk wrote:
Dear dirk.eibach@gdsys.cc,
In message 1367847325-21463-5-git-send-email-dirk.eibach@gdsys.cc you wrote:
...
fpga_set_reg(k, REG(reflection_low),REFLECTION_TESTPATTERN);This breaks bisecting. You reference functions which you only add in a later patch.
Note that 'git rebase -i' and the exec keyword can help you test bisectability.
I know 'git rebase -i', but what is the exec keyword? It seems there is some room for improvement on patman :)
Cheers Dirk
On Mon, May 06, 2013 at 05:02:12PM +0200, Dirk Eibach wrote:
Hi Tom,
On Mon, May 06, 2013 at 04:07:15PM +0200, Wolfgang Denk wrote:
Dear dirk.eibach@gdsys.cc,
In message 1367847325-21463-5-git-send-email-dirk.eibach@gdsys.cc you wrote:
...
fpga_set_reg(k, REG(reflection_low),REFLECTION_TESTPATTERN);This breaks bisecting. You reference functions which you only add in a later patch.
Note that 'git rebase -i' and the exec keyword can help you test bisectability.
I know 'git rebase -i', but what is the exec keyword? It seems there is some room for improvement on patman :)
Well, it's a rebase command. In addition to pick/edit/reword/squash/fixup you can do 'exec make sandbox', or more likely some script that builds a bunch of things and returns non-zero on failure. Git will then run that command at that point in the rebase (say, pick the first commit, exec) and if the exec fails, you get dropped to a command prompt and can edit the previous commit, etc.
Hi Tom,
On Mon, May 06, 2013 at 05:02:12PM +0200, Dirk Eibach wrote:
Hi Tom,
On Mon, May 06, 2013 at 04:07:15PM +0200, Wolfgang Denk wrote:
Dear dirk.eibach@gdsys.cc,
In message 1367847325-21463-5-git-send-email-dirk.eibach@gdsys.cc you wrote:
...
fpga_set_reg(k, REG(reflection_low),REFLECTION_TESTPATTERN);This breaks bisecting. You reference functions which you only add in a later patch.
Note that 'git rebase -i' and the exec keyword can help you test bisectability.
I know 'git rebase -i', but what is the exec keyword? It seems there is some room for improvement on patman :)
Well, it's a rebase command. In addition to pick/edit/reword/squash/fixup you can do 'exec make sandbox', or more likely some script that builds a bunch of things and returns non-zero on failure. Git will then run that command at that point in the rebase (say, pick the first commit, exec) and if the exec fails, you get dropped to a command prompt and can edit the previous commit, etc.
Wow, that's a very nice tool. I will pick it into my workflow.
Cheers Dirk
From: Dirk Eibach eibach@gdsys.de
Signed-off-by: Dirk Eibach dirk.eibach@gdsys.cc
--- Changes in v2: - fpga_state has been moved to arch_global_data - include cmd_fpgad in iocon - use multibus soft-i2c in iocon
board/gdsys/405ep/iocon.c | 253 ++++++++++++++++++++++++++++++++++++++------- board/gdsys/common/osd.c | 25 +++-- include/configs/iocon.h | 44 ++++++-- include/gdsys_fpga.h | 15 +++- 4 files changed, 280 insertions(+), 57 deletions(-)
diff --git a/board/gdsys/405ep/iocon.c b/board/gdsys/405ep/iocon.c index 0fc7db2..0ab8772 100644 --- a/board/gdsys/405ep/iocon.c +++ b/board/gdsys/405ep/iocon.c @@ -23,6 +23,7 @@
#include <common.h> #include <command.h> +#include <errno.h> #include <asm/processor.h> #include <asm/io.h> #include <asm/ppc4xx-gpio.h> @@ -31,6 +32,13 @@ #include <gdsys_fpga.h>
#include "../common/osd.h" +#include "../common/mclink.h" + +#include <i2c.h> +#include <pca953x.h> +#include <pca9698.h> + +DECLARE_GLOBAL_DATA_PTR;
#define LATCH0_BASE (CONFIG_SYS_LATCH_BASE) #define LATCH1_BASE (CONFIG_SYS_LATCH_BASE + 0x100) @@ -47,11 +55,20 @@ enum { HWVER_100 = 0, HWVER_104 = 1, HWVER_110 = 2, + HWVER_120 = 3, + HWVER_200 = 4, + HWVER_210 = 5, +}; + +enum { + FPGA_HWVER_200 = 0, + FPGA_HWVER_210 = 1, };
enum { COMPRESSION_NONE = 0, - COMPRESSION_TYPE1_DELTA, + COMPRESSION_TYPE1_DELTA = 1, + COMPRESSION_TYPE1_TYPE2_DELTA = 3, };
enum { @@ -67,8 +84,56 @@ enum {
enum { RAM_DDR2_32 = 0, + RAM_DDR3_32 = 1, +}; + +enum { + MCFPGA_DONE = 1 << 0, + MCFPGA_INIT_N = 1 << 1, + MCFPGA_PROGRAM_N = 1 << 2, + MCFPGA_UPDATE_ENABLE_N = 1 << 3, + MCFPGA_RESET_N = 1 << 4, };
+unsigned int mclink_fpgacount; + +void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) +{ + int res; + struct ihs_fpga *fpga_0 = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(0); + + switch (fpga) { + case 0: + out_le16((u16 *)fpga_0 + reg / sizeof(u16), data); + break; + default: + res = mclink_send(fpga - 1, reg, data); + if (res < 0) + printf("mclink_send reg %02x data %04x returned %d\n", + reg, data, res); + break; + } +} + +u16 fpga_get_reg(unsigned int fpga, u16 reg) +{ + int res; + u16 data; + struct ihs_fpga *fpga_0 = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(0); + + switch (fpga) { + case 0: + return in_le16((u16 *)fpga_0 + reg / sizeof(u16)); + default: + if (fpga > mclink_fpgacount) + return -EINVAL; + res = mclink_receive(fpga - 1, reg, &data); + if (res < 0) + printf("mclink_receive returned %d\n", res); + return data; + } +} + /* * Check Board Identity: */ @@ -90,12 +155,11 @@ int checkboard(void) return 0; }
-static void print_fpga_info(void) +static void print_fpga_info(unsigned int fpga) { - struct ihs_fpga *fpga = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(0); - u16 versions = in_le16(&fpga->versions); - u16 fpga_version = in_le16(&fpga->fpga_version); - u16 fpga_features = in_le16(&fpga->fpga_features); + u16 versions = fpga_get_reg(fpga, REG(versions)); + u16 fpga_version = fpga_get_reg(fpga, REG(fpga_version)); + u16 fpga_features = fpga_get_reg(fpga, REG(fpga_features)); unsigned unit_type; unsigned hardware_version; unsigned feature_compression; @@ -105,9 +169,9 @@ static void print_fpga_info(void) unsigned feature_ramconfig; unsigned feature_carriers; unsigned feature_video_channels; + int legacy = get_fpga_state(0) & FPGA_STATE_PLATFORM;
unit_type = (versions & 0xf000) >> 12; - hardware_version = versions & 0x000f; feature_compression = (fpga_features & 0xe000) >> 13; feature_osd = fpga_features & (1<<11); feature_audio = (fpga_features & 0x0600) >> 9; @@ -116,6 +180,9 @@ static void print_fpga_info(void) feature_carriers = (fpga_features & 0x000c) >> 2; feature_video_channels = fpga_features & 0x0003;
+ if (legacy) + printf("legacy "); + switch (unit_type) { case UNITTYPE_MAIN_USER: printf("Mainchannel"); @@ -130,27 +197,68 @@ static void print_fpga_info(void) break; }
- switch (hardware_version) { - case HWVER_100: - printf(" HW-Ver 1.00\n"); - break; - - case HWVER_104: - printf(" HW-Ver 1.04\n"); - break; - - case HWVER_110: - printf(" HW-Ver 1.10\n"); - break; + if (unit_type == UNITTYPE_MAIN_USER) { + if (legacy) + hardware_version = + (in_le16((void *)LATCH2_BASE)>>8) & 0x0f; + else + hardware_version = + (!!pca9698_get_value(0x20, 24) << 0) + | (!!pca9698_get_value(0x20, 25) << 1) + | (!!pca9698_get_value(0x20, 26) << 2) + | (!!pca9698_get_value(0x20, 27) << 3); + switch (hardware_version) { + case HWVER_100: + printf(" HW-Ver 1.00,"); + break; + + case HWVER_104: + printf(" HW-Ver 1.04,"); + break; + + case HWVER_110: + printf(" HW-Ver 1.10,"); + break; + + case HWVER_120: + printf(" HW-Ver 1.20-1.21,"); + break; + + case HWVER_200: + printf(" HW-Ver 2.00,"); + break; + + case HWVER_210: + printf(" HW-Ver 2.10,"); + break; + + default: + printf(" HW-Ver %d(not supported),", + hardware_version); + break; + } + }
- default: - printf(" HW-Ver %d(not supported)\n", - hardware_version); - break; + if (unit_type == UNITTYPE_VIDEO_USER) { + hardware_version = versions & 0x000f; + switch (hardware_version) { + case FPGA_HWVER_200: + printf(" HW-Ver 2.00,"); + break; + + case FPGA_HWVER_210: + printf(" HW-Ver 2.10,"); + break; + + default: + printf(" HW-Ver %d(not supported),", + hardware_version); + break; + } }
- printf(" FPGA V %d.%02d, features:", - fpga_version / 100, fpga_version % 100); + printf(" FPGA V %d.%02d\n features:", + fpga_version / 100, fpga_version % 100);
switch (feature_compression) { @@ -162,6 +270,10 @@ static void print_fpga_info(void) printf(" type1-deltacompression"); break;
+ case COMPRESSION_TYPE1_TYPE2_DELTA: + printf(" type1-deltacompression, type2-inlinecompression"); + break; + default: printf(" compression %d(not supported)", feature_compression); break; @@ -208,6 +320,10 @@ static void print_fpga_info(void) printf(", RAM 32 bit DDR2"); break;
+ case RAM_DDR3_32: + printf(", RAM 32 bit DDR3"); + break; + default: printf(", RAM %d(not supported)", feature_ramconfig); break; @@ -220,41 +336,94 @@ static void print_fpga_info(void)
int last_stage_init(void) { - print_fpga_info(); + int slaves; + unsigned int k; + unsigned char mclink_controllers[] = { 0x24, 0x25, 0x26 }; + + print_fpga_info(0); + osd_probe(0); + + /* wait for FPGA done */ + for (k = 0; k < ARRAY_SIZE(mclink_controllers); ++k) { + unsigned int ctr = 0; + + if (i2c_probe(mclink_controllers[k])) + continue; + + while (!(pca953x_get_val(mclink_controllers[k]) + & MCFPGA_DONE)) { + udelay(100000); + if (ctr++ > 5) { + printf("no done for mclink_controller %d\n", k); + break; + } + } + } + + /* wait for slave-PLLs to be up and running */ + udelay(500000); + + mclink_fpgacount = CONFIG_SYS_MCLINK_MAX; + slaves = mclink_probe(); + mclink_fpgacount = 0; + + if (slaves <= 0) + return 0; + + mclink_fpgacount = slaves; + + for (k = 1; k <= slaves; ++k) { + print_fpga_info(k); + osd_probe(k); + }
- return osd_probe(0); + return 0; }
/* * provide access to fpga gpios (for I2C bitbang) */ -void fpga_gpio_set(int pin) +void fpga_gpio_set(unsigned int bus, int pin) { - out_le16((void *)(CONFIG_SYS_FPGA0_BASE + 0x18), pin); + fpga_set_reg(bus, REG(gpio.set), pin); }
-void fpga_gpio_clear(int pin) +void fpga_gpio_clear(unsigned int bus, int pin) { - out_le16((void *)(CONFIG_SYS_FPGA0_BASE + 0x16), pin); + fpga_set_reg(bus, REG(gpio.clear), pin); }
-int fpga_gpio_get(int pin) +int fpga_gpio_get(unsigned int bus, int pin) { - return in_le16((void *)(CONFIG_SYS_FPGA0_BASE + 0x14)) & pin; + return fpga_get_reg(bus, REG(gpio.read)) & pin; }
void gd405ep_init(void) { + unsigned int k; + + if (i2c_probe(0x20)) { /* i2c_probe returns 0 on success */ + for (k = 0; k < CONFIG_SYS_FPGA_COUNT; ++k) + gd->arch.fpga_state[k] |= FPGA_STATE_PLATFORM; + } else { + pca9698_direction_output(0x20, 4, 1); + } }
void gd405ep_set_fpga_reset(unsigned state) { - if (state) { - out_le16((void *)LATCH0_BASE, CONFIG_SYS_LATCH0_RESET); - out_le16((void *)LATCH1_BASE, CONFIG_SYS_LATCH1_RESET); + int legacy = get_fpga_state(0) & FPGA_STATE_PLATFORM; + + if (legacy) { + if (state) { + out_le16((void *)LATCH0_BASE, CONFIG_SYS_LATCH0_RESET); + out_le16((void *)LATCH1_BASE, CONFIG_SYS_LATCH1_RESET); + } else { + out_le16((void *)LATCH0_BASE, CONFIG_SYS_LATCH0_BOOT); + out_le16((void *)LATCH1_BASE, CONFIG_SYS_LATCH1_BOOT); + } } else { - out_le16((void *)LATCH0_BASE, CONFIG_SYS_LATCH0_BOOT); - out_le16((void *)LATCH1_BASE, CONFIG_SYS_LATCH1_BOOT); + pca9698_set_value(0x20, 4, state ? 0 : 1); } }
@@ -269,5 +438,11 @@ void gd405ep_setup_hw(void)
int gd405ep_get_fpga_done(unsigned fpga) { - return in_le16((void *)LATCH2_BASE) & CONFIG_SYS_FPGA_DONE(fpga); + int legacy = get_fpga_state(0) & FPGA_STATE_PLATFORM; + + if (legacy) + return in_le16((void *)LATCH2_BASE) + & CONFIG_SYS_FPGA_DONE(fpga); + else + return pca9698_get_value(0x20, 20); } diff --git a/board/gdsys/common/osd.c b/board/gdsys/common/osd.c index 81c8136..2cfbe58 100644 --- a/board/gdsys/common/osd.c +++ b/board/gdsys/common/osd.c @@ -22,7 +22,8 @@ */
#include <common.h> -#include "bb_i2c.h" +#include <i2c.h> +#include <malloc.h> #include <asm/io.h>
#include <gdsys_fpga.h> @@ -69,6 +70,10 @@ enum {
unsigned int max_osd_screen = CONFIG_SYS_OSD_SCREENS - 1;
+#ifdef CONFIG_SYS_CH7301 +int ch7301_i2c[] = CONFIG_SYS_CH7301_I2C; +#endif + #if defined(CONFIG_SYS_ICS8N3QV01) || defined(CONFIG_SYS_SIL1178) static void fpga_iic_write(unsigned screen, u8 slave, u8 reg, u8 data) { @@ -318,6 +323,9 @@ int osd_probe(unsigned screen) unsigned width; unsigned height; u8 value; +#ifdef CONFIG_SYS_CH7301 + int old_bus = i2c_get_bus_num(); +#endif
width = ((features & 0x3f00) >> 8) + 1; height = (features & 0x001f) + 1; @@ -326,16 +334,19 @@ int osd_probe(unsigned screen) screen, version/100, version%100, width, height);
#ifdef CONFIG_SYS_CH7301 - value = bb_i2c_reg_read(screen, CH7301_I2C_ADDR, CH7301_DID); + i2c_set_bus_num(ch7301_i2c[screen]); + value = i2c_reg_read(CH7301_I2C_ADDR, CH7301_DID); if (value != 0x17) { printf(" Probing CH7301 failed, DID %02x\n", value); + i2c_set_bus_num(old_bus); return -1; } - bb_i2c_reg_write(screen, CH7301_I2C_ADDR, CH7301_TPCP, 0x08); - bb_i2c_reg_write(screen, CH7301_I2C_ADDR, CH7301_TPD, 0x16); - bb_i2c_reg_write(screen, CH7301_I2C_ADDR, CH7301_TPF, 0x60); - bb_i2c_reg_write(screen, CH7301_I2C_ADDR, CH7301_DC, 0x09); - bb_i2c_reg_write(screen, CH7301_I2C_ADDR, CH7301_PM, 0xc0); + i2c_reg_write(CH7301_I2C_ADDR, CH7301_TPCP, 0x08); + i2c_reg_write(CH7301_I2C_ADDR, CH7301_TPD, 0x16); + i2c_reg_write(CH7301_I2C_ADDR, CH7301_TPF, 0x60); + i2c_reg_write(CH7301_I2C_ADDR, CH7301_DC, 0x09); + i2c_reg_write(CH7301_I2C_ADDR, CH7301_PM, 0xc0); + i2c_set_bus_num(old_bus); #endif
#ifdef CONFIG_SYS_MPC92469AC diff --git a/include/configs/iocon.h b/include/configs/iocon.h index 5972711..921ac05 100644 --- a/include/configs/iocon.h +++ b/include/configs/iocon.h @@ -79,6 +79,7 @@ * Commands additional to the ones defined in amcc-common.h */ #define CONFIG_CMD_CACHE +#define CONFIG_CMD_FPGAD #undef CONFIG_CMD_EEPROM
/* @@ -116,23 +117,46 @@ #define CONFIG_SYS_I2C_PPC4XX_SPEED_0 400000 #define CONFIG_SYS_I2C_PPC4XX_SLAVE_0 0x7F
+#define CONFIG_SYS_I2C_SPEED 400000 + +#define CONFIG_PCA953X /* NXP PCA9554 */ +#define CONFIG_PCA9698 /* NXP PCA9698 */ + /* * Software (bit-bang) I2C driver configuration */ +#define CONFIG_SYS_I2C_SOFT +#define CONFIG_SYS_I2C_SOFT_SPEED 50000 +#define CONFIG_SYS_I2C_SOFT_SLAVE 0x7F +#define I2C_SOFT_DECLARATIONS2 +#define CONFIG_SYS_I2C_SOFT_SPEED_2 50000 +#define CONFIG_SYS_I2C_SOFT_SLAVE_2 0x7F +#define I2C_SOFT_DECLARATIONS3 +#define CONFIG_SYS_I2C_SOFT_SPEED_3 50000 +#define CONFIG_SYS_I2C_SOFT_SLAVE_3 0x7F +#define I2C_SOFT_DECLARATIONS4 +#define CONFIG_SYS_I2C_SOFT_SPEED_4 50000 +#define CONFIG_SYS_I2C_SOFT_SLAVE_4 0x7F + +#define CONFIG_SYS_CH7301_I2C {1, 2, 3, 4}
#ifndef __ASSEMBLY__ -void fpga_gpio_set(int pin); -void fpga_gpio_clear(int pin); -int fpga_gpio_get(int pin); +void fpga_gpio_set(unsigned int bus, int pin); +void fpga_gpio_clear(unsigned int bus, int pin); +int fpga_gpio_get(unsigned int bus, int pin); #endif
#define I2C_ACTIVE { } #define I2C_TRISTATE { } -#define I2C_READ fpga_gpio_get(0x0040) ? 1 : 0 -#define I2C_SDA(bit) if (bit) fpga_gpio_set(0x0040); \ - else fpga_gpio_clear(0x0040) -#define I2C_SCL(bit) if (bit) fpga_gpio_set(0x0020); \ - else fpga_gpio_clear(0x0020) +#define I2C_READ fpga_gpio_get(I2C_ADAP_HWNR, 0x0040) ? 1 : 0 +#define I2C_SDA(bit) if (bit) \ + fpga_gpio_set(I2C_ADAP_HWNR, 0x0040); \ + else \ + fpga_gpio_clear(I2C_ADAP_HWNR, 0x0040) +#define I2C_SCL(bit) if (bit) \ + fpga_gpio_set(I2C_ADAP_HWNR, 0x0020); \ + else \ + fpga_gpio_clear(I2C_ADAP_HWNR, 0x0020) #define I2C_DELAY udelay(25) /* 1/4 I2C clock duration */
/* @@ -252,6 +276,8 @@ int fpga_gpio_get(int pin);
#define CONFIG_SYS_FPGA_COUNT 1
+#define CONFIG_SYS_MCLINK_MAX 3 + /* Memory Bank 3 (Latches) initialization */ #define CONFIG_SYS_LATCH_BASE 0x7f200000 #define CONFIG_SYS_EBC_PB3AP 0x02025080 @@ -267,6 +293,6 @@ int fpga_gpio_get(int pin); */ #define CONFIG_SYS_MPC92469AC #define CONFIG_SYS_CH7301 -#define CONFIG_SYS_OSD_SCREENS CONFIG_SYS_FPGA_COUNT +#define CONFIG_SYS_OSD_SCREENS 1
#endif /* __CONFIG_H */ diff --git a/include/gdsys_fpga.h b/include/gdsys_fpga.h index c12a31d..b246f2f 100644 --- a/include/gdsys_fpga.h +++ b/include/gdsys_fpga.h @@ -117,10 +117,21 @@ struct ihs_fpga { u16 mpc3w_control; /* 0x001a */ u16 reserved_1[19]; /* 0x001c */ u16 videocontrol; /* 0x0042 */ - u16 reserved_2[93]; /* 0x0044 */ + u16 reserved_2[14]; /* 0x0044 */ + u16 mc_int; /* 0x0060 */ + u16 mc_int_en; /* 0x0062 */ + u16 mc_status; /* 0x0064 */ + u16 mc_control; /* 0x0066 */ + u16 mc_tx_data; /* 0x0068 */ + u16 mc_tx_address; /* 0x006a */ + u16 mc_tx_cmd; /* 0x006c */ + u16 mc_res; /* 0x006e */ + u16 mc_rx_cmd_status; /* 0x0070 */ + u16 mc_rx_data; /* 0x0072 */ + u16 reserved_3[69]; /* 0x0074 */ u16 reflection_high; /* 0x00fe */ struct ihs_osd osd; /* 0x0100 */ - u16 reserved_3[889]; /* 0x010e */ + u16 reserved_4[889]; /* 0x010e */ u16 videomem; /* 0x0800 */ }; #endif
Dear dirk.eibach@gdsys.cc,
In message 1367847325-21463-6-git-send-email-dirk.eibach@gdsys.cc you wrote:
From: Dirk Eibach eibach@gdsys.de
Signed-off-by: Dirk Eibach dirk.eibach@gdsys.cc
Changes in v2:
- fpga_state has been moved to arch_global_data
- include cmd_fpgad in iocon
- use multibus soft-i2c in iocon
Please fix the checkpatch errors.
Best regards,
Wolfgang Denk
Dear Wolfgang,
Dear dirk.eibach@gdsys.cc,
In message 1367847325-21463-6-git-send-email-dirk.eibach@gdsys.cc you wrote:
From: Dirk Eibach eibach@gdsys.de
Signed-off-by: Dirk Eibach dirk.eibach@gdsys.cc
Changes in v2:
- fpga_state has been moved to arch_global_data
- include cmd_fpgad in iocon
- use multibus soft-i2c in iocon
Please fix the checkpatch errors.
if we are talking about:
CHECK: Blank lines aren't necessary before a close brace '}' #491: FILE: include/configs/iocon.h:141: + +#define CONFIG_SYS_CH7301_I2C {1, 2, 3, 4}
This seems to be a false positive.
Cheers Dirk
Dear Dirk Eibach,
In message 49d89b84e14cf375d9e113fcc15ce289@gdsys.cc you wrote:
Please fix the checkpatch errors.
if we are talking about:
CHECK: Blank lines aren't necessary before a close brace '}' #491: FILE: include/configs/iocon.h:141:
+#define CONFIG_SYS_CH7301_I2C {1, 2, 3, 4}
This seems to be a false positive.
No, I'm referring to these:
ERROR: Macros with complex values should be enclosed in parenthesis #595: FILE: include/configs/iocon.h:151: +#define I2C_READ fpga_gpio_get(I2C_ADAP_HWNR, 0x0040) ? 1 : 0
ERROR: Macros with multiple statements should be enclosed in a do - while loop #596: FILE: include/configs/iocon.h:152: +#define I2C_SDA(bit) if (bit) \ + fpga_gpio_set(I2C_ADAP_HWNR, 0x0040); \ + else \ + fpga_gpio_clear(I2C_ADAP_HWNR, 0x0040)
ERROR: Macros with multiple statements should be enclosed in a do - while loop #600: FILE: include/configs/iocon.h:156: +#define I2C_SCL(bit) if (bit) \ + fpga_gpio_set(I2C_ADAP_HWNR, 0x0020); \ + else \ + fpga_gpio_clear(I2C_ADAP_HWNR, 0x0020)
total: 3 errors, 0 warnings, 509 lines checked
Best regards,
Wolfgang Denk
Hi Wolfgang,
Dear Dirk Eibach,
In message 49d89b84e14cf375d9e113fcc15ce289@gdsys.cc you wrote:
Please fix the checkpatch errors.
if we are talking about:
CHECK: Blank lines aren't necessary before a close brace '}' #491: FILE: include/configs/iocon.h:141:
+#define CONFIG_SYS_CH7301_I2C {1, 2, 3, 4}
This seems to be a false positive.
No, I'm referring to these:
ERROR: Macros with complex values should be enclosed in parenthesis #595: FILE: include/configs/iocon.h:151: +#define I2C_READ fpga_gpio_get(I2C_ADAP_HWNR, 0x0040) ? 1 : 0
ERROR: Macros with multiple statements should be enclosed in a do - while loop #596: FILE: include/configs/iocon.h:152: +#define I2C_SDA(bit) if (bit) \
fpga_gpio_set(I2C_ADAP_HWNR, 0x0040); \else \fpga_gpio_clear(I2C_ADAP_HWNR, 0x0040)ERROR: Macros with multiple statements should be enclosed in a do - while loop #600: FILE: include/configs/iocon.h:156: +#define I2C_SCL(bit) if (bit) \
fpga_gpio_set(I2C_ADAP_HWNR, 0x0020); \else \fpga_gpio_clear(I2C_ADAP_HWNR, 0x0020)total: 3 errors, 0 warnings, 509 lines checked
OK, thanks, that makes sense. Which version of checkpatch are you using? Latest u-boot master checkpatch/patman does not show these (at least not for me).
Cheers Dirk
Dear Dirk,
In message 3628fded6de7e6b4286d3444a7e5e3ec@gdsys.cc you wrote:
total: 3 errors, 0 warnings, 509 lines checked
OK, thanks, that makes sense. Which version of checkpatch are you using? Latest u-boot master checkpatch/patman does not show these (at least not for me).
It does for me, interestingly ONLY if the current directory is NOT in the U-Boot tree.
Apparently both the COMPLEX_MACRO and MULTISTATEMENT_MACRO_USE_DO_WHILE option should better be enabled, or we will miss such issues.
Added Joe to Cc: - Joe, what do you think? I'd rather remove the silencing for these from .checkpatch.conf ?
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Mon, May 6, 2013 at 2:15 PM, Wolfgang Denk wd@denx.de wrote:
Dear Dirk,
In message 3628fded6de7e6b4286d3444a7e5e3ec@gdsys.cc you wrote:
total: 3 errors, 0 warnings, 509 lines checked
OK, thanks, that makes sense. Which version of checkpatch are you using? Latest u-boot master checkpatch/patman does not show these (at least not for me).
It does for me, interestingly ONLY if the current directory is NOT in the U-Boot tree.
Apparently both the COMPLEX_MACRO and MULTISTATEMENT_MACRO_USE_DO_WHILE option should better be enabled, or we will miss such issues.
Added Joe to Cc: - Joe, what do you think? I'd rather remove the silencing for these from .checkpatch.conf ?
Back in October 2011 checkpatch was generating false positives on COMPLEX_MACRO. If this has been fixed and copied from Linux, then we can certainly stop ignoring it. I reported the issue at that time.
As the .checkpatch.conf comments say, CONFIG_SYS_I2C_NOPROBES definitions were causing false positives with MULTISTATEMENT_MACRO_USE_DO_WHILE. I agree that we can remove ignoring MULTISTATEMENT_MACRO_USE_DO_WHILE as long as the false positives are allowed when identified.
Cheers, -Joe
From: Dirk Eibach eibach@gdsys.de
Signed-off-by: Dirk Eibach dirk.eibach@gdsys.cc --- Changes in v2: None
board/gdsys/405ep/dlvision-10g.c | 20 ++++++++++++++------ board/gdsys/405ep/io.c | 20 ++++++++++++++------ board/gdsys/405ep/neo.c | 17 +++++++++++++---- board/gdsys/405ex/405ex.c | 14 ++++++-------- board/gdsys/405ex/io64.c | 38 ++++++++++++++++++++++---------------- 5 files changed, 69 insertions(+), 40 deletions(-)
diff --git a/board/gdsys/405ep/dlvision-10g.c b/board/gdsys/405ep/dlvision-10g.c index 644493b..4d1a02e 100644 --- a/board/gdsys/405ep/dlvision-10g.c +++ b/board/gdsys/405ep/dlvision-10g.c @@ -71,6 +71,16 @@ enum { RAM_DDR2_64 = 2, };
+void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) +{ + out_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16), data); +} + +u16 fpga_get_reg(unsigned int fpga, u16 reg) +{ + return in_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16)); +} + int misc_init_r(void) { /* startup fans */ @@ -95,10 +105,9 @@ static unsigned int get_mc2_present(void)
static void print_fpga_info(unsigned dev) { - struct ihs_fpga *fpga = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(dev); - u16 versions = in_le16(&fpga->versions); - u16 fpga_version = in_le16(&fpga->fpga_version); - u16 fpga_features = in_le16(&fpga->fpga_features); + u16 versions = fpga_get_reg(dev, REG(versions)); + u16 fpga_version = fpga_get_reg(dev, REG(fpga_version)); + u16 fpga_features = fpga_get_reg(dev, REG(fpga_features)); unsigned unit_type; unsigned hardware_version; unsigned feature_rs232; @@ -263,8 +272,7 @@ int checkboard(void)
int last_stage_init(void) { - struct ihs_fpga *fpga = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(0); - u16 versions = in_le16(&fpga->versions); + u16 versions = fpga_get_reg(0, REG(versions));
print_fpga_info(0); if (get_mc2_present()) diff --git a/board/gdsys/405ep/io.c b/board/gdsys/405ep/io.c index 070dcbb..dbaf9d6 100644 --- a/board/gdsys/405ep/io.c +++ b/board/gdsys/405ep/io.c @@ -53,6 +53,16 @@ enum { HWVER_122 = 3, };
+void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) +{ + out_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16), data); +} + +u16 fpga_get_reg(unsigned int fpga, u16 reg) +{ + return in_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16)); +} + int misc_init_r(void) { /* startup fans */ @@ -117,10 +127,9 @@ int checkboard(void)
static void print_fpga_info(void) { - struct ihs_fpga *fpga = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(0); - u16 versions = in_le16(&fpga->versions); - u16 fpga_version = in_le16(&fpga->fpga_version); - u16 fpga_features = in_le16(&fpga->fpga_features); + u16 versions = fpga_get_reg(0, REG(versions)); + u16 fpga_version = fpga_get_reg(0, REG(fpga_version)); + u16 fpga_features = fpga_get_reg(0, REG(fpga_features)); unsigned unit_type; unsigned hardware_version; unsigned feature_channels; @@ -179,7 +188,6 @@ static void print_fpga_info(void) */ int last_stage_init(void) { - struct ihs_fpga *fpga = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(0); unsigned int k;
print_fpga_info(); @@ -191,7 +199,7 @@ int last_stage_init(void) configure_gbit_phy(k);
/* take fpga serdes blocks out of reset */ - out_le16(&fpga->quad_serdes_reset, 0); + fpga_set_reg(0, REG(quad_serdes_reset), 0);
return 0; } diff --git a/board/gdsys/405ep/neo.c b/board/gdsys/405ep/neo.c index d336e55..f06a280 100644 --- a/board/gdsys/405ep/neo.c +++ b/board/gdsys/405ep/neo.c @@ -44,6 +44,16 @@ enum { HWVER_300 = 3, };
+void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) +{ + out_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16), data); +} + +u16 fpga_get_reg(unsigned int fpga, u16 reg) +{ + return in_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16)); +} + int misc_init_r(void) { /* startup fans */ @@ -70,10 +80,9 @@ int checkboard(void)
static void print_fpga_info(void) { - struct ihs_fpga *fpga = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(0); - u16 versions = in_le16(&fpga->versions); - u16 fpga_version = in_le16(&fpga->fpga_version); - u16 fpga_features = in_le16(&fpga->fpga_features); + u16 versions = fpga_get_reg(0, REG(versions)); + u16 fpga_version = fpga_get_reg(0, REG(fpga_version)); + u16 fpga_features = fpga_get_reg(0, REG(fpga_features)); int fpga_state = get_fpga_state(0); unsigned unit_type; unsigned hardware_version; diff --git a/board/gdsys/405ex/405ex.c b/board/gdsys/405ex/405ex.c index 32e24c0..b7e9802 100644 --- a/board/gdsys/405ex/405ex.c +++ b/board/gdsys/405ex/405ex.c @@ -220,23 +220,21 @@ int board_early_init_r(void) gd405ex_set_fpga_reset(0);
for (k = 0; k < CONFIG_SYS_FPGA_COUNT; ++k) { - struct ihs_fpga *fpga = - (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(k); #ifdef CONFIG_SYS_FPGA_NO_RFL_HI - u16 *reflection_target = &fpga->reflection_low; + u16 reflection_target = REG(reflection_low); #else - u16 *reflection_target = &fpga->reflection_high; + u16 reflection_target = REG(reflection_high); #endif /* * wait for fpga out of reset */ ctr = 0; while (1) { - out_le16(&fpga->reflection_low, - REFLECTION_TESTPATTERN); + fpga_set_reg(k, REG(reflection_low), + REFLECTION_TESTPATTERN);
- if (in_le16(reflection_target) == - REFLECTION_TESTPATTERN_INV) + if (fpga_get_reg(k, reflection_target) == + REFLECTION_TESTPATTERN_INV) break;
udelay(100000); diff --git a/board/gdsys/405ex/io64.c b/board/gdsys/405ex/io64.c index 7d2899d..f0f4241 100644 --- a/board/gdsys/405ex/io64.c +++ b/board/gdsys/405ex/io64.c @@ -67,6 +67,16 @@ enum { HWVER_110 = 1, };
+void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) +{ + out_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16), data); +} + +u16 fpga_get_reg(unsigned int fpga, u16 reg) +{ + return in_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16)); +} + static inline void blank_string(int size) { int i; @@ -100,10 +110,9 @@ int misc_init_r(void)
static void print_fpga_info(unsigned dev) { - struct ihs_fpga *fpga = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(dev); - u16 versions = in_le16(&fpga->versions); - u16 fpga_version = in_le16(&fpga->fpga_version); - u16 fpga_features = in_le16(&fpga->fpga_features); + u16 versions = fpga_get_reg(dev, REG(versions)); + u16 fpga_version = fpga_get_reg(dev, REG(fpga_version)); + u16 fpga_features = fpga_get_reg(dev, REG(fpga_features)); int fpga_state = get_fpga_state(dev);
unsigned unit_type; @@ -242,8 +251,6 @@ int last_stage_init(void) { unsigned int k; unsigned int fpga; - struct ihs_fpga *fpga0 = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(0); - struct ihs_fpga *fpga1 = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(1); int failed = 0; char str_phys[] = "Setup PHYs -"; char str_serdes[] = "Start SERDES blocks"; @@ -281,17 +288,16 @@ int last_stage_init(void) /* take fpga serdes blocks out of reset */ puts(str_serdes); udelay(500000); - out_le16(&fpga0->quad_serdes_reset, 0); - out_le16(&fpga1->quad_serdes_reset, 0); + fpga_set_reg(0, REG(quad_serdes_reset), 0); + fpga_set_reg(1, REG(quad_serdes_reset), 0); blank_string(strlen(str_serdes));
/* take channels out of reset */ puts(str_channels); udelay(500000); for (fpga = 0; fpga < 2; ++fpga) { - u16 *ch0_config_int = &(fpga ? fpga1 : fpga0)->ch0_config_int; for (k = 0; k < 32; ++k) - out_le16(ch0_config_int + 4 * k, 0); + fpga_set_reg(fpga, REG(ch0_config_int) + 8 * k, 0); } blank_string(strlen(str_channels));
@@ -299,16 +305,16 @@ int last_stage_init(void) puts(str_locks); udelay(500000); for (fpga = 0; fpga < 2; ++fpga) { - u16 *ch0_status_int = &(fpga ? fpga1 : fpga0)->ch0_status_int; for (k = 0; k < 32; ++k) { - u16 status = in_le16(ch0_status_int + 4*k); + u16 status = + fpga_get_reg(fpga, REG(ch0_status_int) + 8 * k); if (!(status & (1 << 4))) { failed = 1; printf("fpga %d channel %d: no serdes lock\n", fpga, k); } /* reset events */ - out_le16(ch0_status_int + 4*k, status); + fpga_set_reg(fpga, REG(ch0_status_int) + 8 * k, 0); } } blank_string(strlen(str_locks)); @@ -316,14 +322,14 @@ int last_stage_init(void) /* verify hicb_status */ puts(str_hicb); for (fpga = 0; fpga < 2; ++fpga) { - u16 *ch0_hicb_status_int = &(fpga ? fpga1 : fpga0)->ch0_hicb_status_int; for (k = 0; k < 32; ++k) { - u16 status = in_le16(ch0_hicb_status_int + 4*k); + u16 status = + fpga_get_reg(fpga, REG(ch0_status_int) + 8 * k); if (status) printf("fpga %d hicb %d: hicb status %04x\n", fpga, k, status); /* reset events */ - out_le16(ch0_hicb_status_int + 4*k, status); + fpga_set_reg(fpga, REG(ch0_status_int) + 8 * k, 0); } } blank_string(strlen(str_hicb));
Dear dirk.eibach@gdsys.cc,
In message 1367847325-21463-7-git-send-email-dirk.eibach@gdsys.cc you wrote:
From: Dirk Eibach eibach@gdsys.de
Signed-off-by: Dirk Eibach dirk.eibach@gdsys.cc
Changes in v2: None
board/gdsys/405ep/dlvision-10g.c | 20 ++++++++++++++------ board/gdsys/405ep/io.c | 20 ++++++++++++++------ board/gdsys/405ep/neo.c | 17 +++++++++++++---- board/gdsys/405ex/405ex.c | 14 ++++++-------- board/gdsys/405ex/io64.c | 38 ++++++++++++++++++++++---------------- 5 files changed, 69 insertions(+), 40 deletions(-)
diff --git a/board/gdsys/405ep/dlvision-10g.c b/board/gdsys/405ep/dlvision-10g.c index 644493b..4d1a02e 100644 --- a/board/gdsys/405ep/dlvision-10g.c +++ b/board/gdsys/405ep/dlvision-10g.c @@ -71,6 +71,16 @@ enum { RAM_DDR2_64 = 2, };
+void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) +{
- out_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16), data);
+}
+u16 fpga_get_reg(unsigned int fpga, u16 reg) +{
- return in_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16));
+}
We strictly discourage using a notation of base address plus offset, and require to use proper C structs instead, especially because this allows type checking by the compiler.
You had the this, and now attempt to throw it away. This makes no sense.
NAK.
Best regards,
Wolfgang Denk
Hi Wolfgang,
...
We strictly discourage using a notation of base address plus offset, and require to use proper C structs instead, especially because this allows type checking by the compiler.
I am very well aware of that.
You had the this, and now attempt to throw it away. This makes no sense.
In fact it does. We have FPGAs that are memory mapped and others that are not. They must be accessed by the same drivers. So the alternative would be to create FPGA instances at address NULL and getting the register offesets by casting pointers to u16. Not very nice either.
Cheers Dirk
Dear Dirk Eibach,
In message e8fccb4d422d619744521268691e5a9b@gdsys.cc you wrote:
You had the this, and now attempt to throw it away. This makes no sense.
In fact it does. We have FPGAs that are memory mapped and others that are not. They must be accessed by the same drivers. So the alternative would be to create FPGA instances at address NULL and getting the register offesets by casting pointers to u16. Not very nice either.
Your new code still boils down to using the same standard I/O accessors.
So your FPGA registers must be mapped somehow to I/O memory.
When you can do something like
+u16 fpga_get_reg(unsigned int fpga, u16 reg) +{ + return in_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16)); +}
why would you not be able to continue using in_le16() directly?
Sorry, I don't get it.
Best regards,
Wolfgang Denk
Hi Wolfgang,
Am 06.05.2013 17:22, schrieb Wolfgang Denk:
Dear Dirk Eibach,
In message e8fccb4d422d619744521268691e5a9b@gdsys.cc you wrote:
You had the this, and now attempt to throw it away. This makes no sense.
In fact it does. We have FPGAs that are memory mapped and others that are not. They must be accessed by the same drivers. So the alternative would be to create FPGA instances at address NULL and getting the register offesets by casting pointers to u16. Not very nice either.
Your new code still boils down to using the same standard I/O accessors.
So your FPGA registers must be mapped somehow to I/O memory.
When you can do something like
+u16 fpga_get_reg(unsigned int fpga, u16 reg) +{
- return in_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg /
sizeof(u16)); +}
why would you not be able to continue using in_le16() directly?
Sorry, I don't get it.
Read the source, Luke :)
On our iocon platform you find:
+void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) +{ + int res; + struct ihs_fpga *fpga_0 = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(0); + + switch (fpga) { + case 0: + out_le16((u16 *)fpga_0 + reg / sizeof(u16), data); + break; + default: + res = mclink_send(fpga - 1, reg, data); + if (res < 0) + printf("mclink_send reg %02x data %04x returned %d\n", + reg, data, res); + break; + } +}
So no memory mapping here. That's the reason for all this fuzz.
And sorry for the messed up series. Sometimes rebase can make things worse :)
Cheers Dirk
Dear Dirk Eibach,
In message 449c45a6a9978c55e84d3fe7efe6f0ac@gdsys.cc you wrote:
Read the source, Luke :)
It was a bit difficult to spot in this unordered mix of patches ;-)
+void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) +{
- int res;
- struct ihs_fpga *fpga_0 = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(0);
- switch (fpga) {
- case 0:
out_le16((u16 *)fpga_0 + reg / sizeof(u16), data);break;- default:
res = mclink_send(fpga - 1, reg, data);if (res < 0)printf("mclink_send reg %02x data %04x returned %d\n",reg, data, res);break;- }
+}
So no memory mapping here. That's the reason for all this fuzz.
OK - but I don't see why mclink_send() could not be used in the same way, i. e. taking a struct member as argument?
Apparently all your FPGA accesses are for u16 data only? Then you could rewrite fpga_set_reg() similar to this:
int fpga_set_reg(u32 fpga, u16 *addr, u16 data)
And voila, no need to move away from the C structs.
Note 1: You print an error message if mclink_send() returns an error; I think this error should propagate, i. e. this function should not return void.
Note 2: I changed the type of the first arg into "u32" so it is consistent with the other args. Mixing native types (unsigned int) here and defined types (as u16 instead of unsigned short) looks weird to me.
Best regards,
Wolfgang Denk
Hi Wolfgang,
+void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) +{
- int res;
- struct ihs_fpga *fpga_0 = (struct ihs_fpga
*)CONFIG_SYS_FPGA_BASE(0);
- switch (fpga) {
- case 0:
out_le16((u16 *)fpga_0 + reg / sizeof(u16), data);break;- default:
res = mclink_send(fpga - 1, reg, data);if (res < 0)printf("mclink_send reg %02x data %04x returned %d\n",reg, data, res);break;- }
+}
So no memory mapping here. That's the reason for all this fuzz.
OK - but I don't see why mclink_send() could not be used in the same way, i. e. taking a struct member as argument?
Certainly it *can* be used using *pointers* to struct members as argument.
What I stated yesterday was: "We have FPGAs that are memory mapped and others that are not. They must be accessed by the same drivers. So the alternative would be to create FPGA instances at address NULL and getting the register offesets by casting pointers to u16. Not very nice either."
Apparently all your FPGA accesses are for u16 data only? Then you could rewrite fpga_set_reg() similar to this:
int fpga_set_reg(u32 fpga, u16 *addr, u16 data)
With struct ihs_fpga { u16 reflection_low; /* 0x0000 */ u16 versions; /* 0x0002 */ ... }; and void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) the call looks like fpga_set_reg(k, REG(reflection_low), REFLECTION_TESTPATTERN);
To use int fpga_set_reg(u32 fpga, u16 *addr, u16 data) we need something like struct ihs_fpga system_fpgas[] = { (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(0), (struct ihs_fpga *)NULL, (struct ihs_fpga *)NULL, (struct ihs_fpga *)NULL, }; and fpga_set_reg(k, system_fpgas[k].reflection_low, REFLECTION_TESTPATTERN);
In fpga_set_reg() it would look like mclink_send(fpga - 1, (u16)addr, data);
I personally dislike all this NULL-pointer passing and casting. But YMMV. If that's the u-boot-way I will be happy to change it.
And voila, no need to move away from the C structs.
Note 1: You print an error message if mclink_send() returns an error; I think this error should propagate, i. e. this function should not return void.
Yep. Happy were the days when accessing FPGA registers could not fail :(
Note 2: I changed the type of the first arg into "u32" so it is consistent with the other args. Mixing native types (unsigned int) here and defined types (as u16 instead of unsigned short) looks weird to me.
I wanted to express the 16 bit io-implementation of our address/data busses, while the fpga index is simply a number. Maybe that is information overkill :)
Cheers Dirk
Dear Dirk Eibach,
In message cf2913653766edc89705f49e08758df7@gdsys.cc you wrote:
OK - but I don't see why mclink_send() could not be used in the same way, i. e. taking a struct member as argument?
Certainly it *can* be used using *pointers* to struct members as argument.
What I stated yesterday was: "We have FPGAs that are memory mapped and others that are not. They must be accessed by the same drivers. So the alternative would be to create FPGA instances at address NULL and getting the register offesets by casting pointers to u16. Not very nice either."
Yes, you wrote that. I did not understand it then, nor do I understand it now. What do you mean by "create FPGA instances at address NULL"? in any case, "getting the register offsets by casting pointers to u16" makes no sense, as this is exactly what we try to avoid. By referencing a C struct you do NOT use any offsets. You use references to struct elements; where needed, you let the compiler to the address calculations (and the type checking).
No offsets. No casts.
could rewrite fpga_set_reg() similar to this:
int fpga_set_reg(u32 fpga, u16 *addr, u16 data)
With struct ihs_fpga { u16 reflection_low; /* 0x0000 */ u16 versions; /* 0x0002 */ ... }; and void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) the call looks like fpga_set_reg(k, REG(reflection_low), REFLECTION_TESTPATTERN);
Mo. It should look like that:
int fpga_set_reg(u32 fpga, u16 *addr, u16 data)
rc = fpga_set_reg(k, &ptr->reflection_low, REFLECTION_TESTPATTERN); if (rc) ...
To use int fpga_set_reg(u32 fpga, u16 *addr, u16 data) we need something like struct ihs_fpga system_fpgas[] = { (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(0), (struct ihs_fpga *)NULL, (struct ihs_fpga *)NULL, (struct ihs_fpga *)NULL, };
Is this not redundant? If you have an array of pointers (addresses), then you can identify the FPGA by using the pointer, and don't need to pass both the pointer _and_ the index to the fpga_{g,s}et_reg() functions?
I personally dislike all this NULL-pointer passing and casting. But YMMV. If that's the u-boot-way I will be happy to change it.
Why would you need casts? The whole purpose of this is to _avoid_ casts, so that the compiler has a chance to make sure all accesses are performed using the correct alignment and size of the respective data types.
Note 2: I changed the type of the first arg into "u32" so it is consistent with the other args. Mixing native types (unsigned int) here and defined types (as u16 instead of unsigned short) looks weird to me.
I wanted to express the 16 bit io-implementation of our address/data busses, while the fpga index is simply a number. Maybe that is information overkill :)
Eventually you can drop the index alltogether when you just pass a pointer?
Best regards,
Wolfgang Denk
Am 07.05.2013 13:36, schrieb Wolfgang Denk:
Dear Dirk Eibach,
In message cf2913653766edc89705f49e08758df7@gdsys.cc you wrote:
OK - but I don't see why mclink_send() could not be used in the same way, i. e. taking a struct member as argument?
Certainly it *can* be used using *pointers* to struct members as argument.
What I stated yesterday was: "We have FPGAs that are memory mapped and others that are not. They must be accessed by the same drivers. So the alternative would be to create FPGA instances at address NULL and getting the register offesets by casting pointers to u16. Not very nice either."
Yes, you wrote that. I did not understand it then, nor do I understand it now. What do you mean by "create FPGA instances at address NULL"? in any case, "getting the register offsets by casting pointers to u16" makes no sense, as this is exactly what we try to avoid. By referencing a C struct you do NOT use any offsets. You use references to struct elements; where needed, you let the compiler to the address calculations (and the type checking).
No offsets. No casts.
OK. Once more. 3 of our 4 FPGAs are *not* memory mapped. There is no base address. This is what I want to show by:
struct ihs_fpga system_fpgas[] = { (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(0), (struct ihs_fpga *)NULL, (struct ihs_fpga *)NULL, (struct ihs_fpga *)NULL, };
For accessing registers of those (not memory mapped) FPGAs I need an u16 register-index. That is what I want to show by:
mclink_send(fpga - 1, (u16)addr, data);
This is the cast I am talking about.
This is the reason why I still need the fpga index parameter.
Cheers Dirk
Dear Dirk Eibach,
In message ba0b32bf657f5f3ffb6056cc6bd0452e@gdsys.cc you wrote:
OK. Once more. 3 of our 4 FPGAs are *not* memory mapped. There is no base address. This is what I want to show by:
struct ihs_fpga system_fpgas[] = { (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(0), (struct ihs_fpga *)NULL, (struct ihs_fpga *)NULL, (struct ihs_fpga *)NULL, };
For accessing registers of those (not memory mapped) FPGAs I need an u16 register-index. That is what I want to show by:
mclink_send(fpga - 1, (u16)addr, data);
Can you please provide a bit more context? What is "addr", and why does it need a cast?
This is the cast I am talking about.
This is the reason why I still need the fpga index parameter.
From these code snippets I can't see what you mean.
Best regards,
Wolfgang Denk
Hi Wolfgang,
in my example we have four FPGAs. One is memory mapped while the other three are not. They all have the same register interface which is mapped by struct ihs_fpga { u16 reflection_low; u16 versions; ... u16 mc_tx_address; u16 mc_tx_data; u16 mc_tx_cmd; ... };
To have instances of those FPGA structs, we might create an array like this: struct ihs_fpga system_fpgas[] = { (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE, (struct ihs_fpga *)NULL, (struct ihs_fpga *)NULL, (struct ihs_fpga *)NULL, }; The first element ist our memory mapped FPGA with base address CONFIG_SYS_FPGA_BASE. For the other three I use NULL as base address because I don't have any better idea what else to choose.
To probe those FPGAs we might have to do something like: for (k=0; k < 4; ++k) fpga_set_reg(k, &system_fpgas[k].reflection_low, REFLECTION_TESTPATTERN);
The implementation of fpga_set_reg() might look like:
void fpga_set_reg(unsigned int fpga, u16 *reg, u16 data) { switch (fpga) { case 0: out_le16(reg, data); break; default: mclink_send(fpga - 1, (u16)reg, data); break; } }
where mclink_send() is the routine to access the non memory mapped FPGAs through the memory mapped FPGA: int mclink_send(u8 slave, u16 addr, u16 data) { ... fpga_set_reg(0, &system_fpgas[0].mc_tx_address, addr); fpga_set_reg(0, &system_fpgas[0].mc_tx_data, data); fpga_set_reg(0, &system_fpgas[0].mc_tx_cmd, (slave & 0x03) << 14); }
The cast to u16 I am talking about happens, when mclink_send() is called.
Cheers Dirk
Dear Dirk,
In message 58ff5023adcbf08481bc2707b0a70f50@gdsys.cc you wrote:
in my example we have four FPGAs. One is memory mapped while the other three are not. They all have the same register interface which is mapped by struct ihs_fpga { u16 reflection_low; u16 versions; ... u16 mc_tx_address; u16 mc_tx_data; u16 mc_tx_cmd; ... };
To have instances of those FPGA structs, we might create an array like this: struct ihs_fpga system_fpgas[] = { (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE, (struct ihs_fpga *)NULL, (struct ihs_fpga *)NULL, (struct ihs_fpga *)NULL, }; The first element ist our memory mapped FPGA with base address CONFIG_SYS_FPGA_BASE. For the other three I use NULL as base address because I don't have any better idea what else to choose.
To probe those FPGAs we might have to do something like: for (k=0; k < 4; ++k) fpga_set_reg(k, &system_fpgas[k].reflection_low,
I think using index notation everywhere makes the code hard to read. Use a pointer, for example like that:
struct ihs_fpga *fpgap = system_fpgas[k];
fpga_set_reg(k, &fpgap->reflection_low, ...
The implementation of fpga_set_reg() might look like:
void fpga_set_reg(unsigned int fpga, u16 *reg, u16 data) { switch (fpga) { case 0: out_le16(reg, data); break; default: mclink_send(fpga - 1, (u16)reg, data); break; } }
The problem here comes from your decision to use the same interface for two different, incompatible things. There are two ugly parts in this code: first is the need for a cast, second (and even worse) is the use of a pointer value of 0 to get to the offset of a field. Actually this is not even standard conformng, as you initialize the pointer as NULL, and there is no real guarantee that
NULL == (struct ihs_fpga *)0
It appears you need both the element address and the offset in your fpga_set_reg() code, so you should pass this information, like that [note that we no longer need an array of addresses]:
struct ihs_fpga fpga_ptr = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE; ...
void fpga_set_reg(u32 fpga, u16 *reg, off_t regoff, u16 data) { if (fpga) return mclink_send(fpga - 1, regoff, data);
return out_le16(reg, data); }
where mclink_send() is the routine to access the non memory mapped FPGAs through the memory mapped FPGA: int mclink_send(u8 slave, u16 addr, u16 data) { ... fpga_set_reg(0, &system_fpgas[0].mc_tx_address, addr); fpga_set_reg(0, &system_fpgas[0].mc_tx_data, data); fpga_set_reg(0, &system_fpgas[0].mc_tx_cmd, (slave & 0x03) << 14); }
And this becomes:
fpga_set_reg(0, &fpga_ptr->mc_tx_address, offsetof(struct ihs_fpga, mc_tx_address), addr);
etc. Yes, this is long and ugly to write, so you may want to define a helper macro like:
#define FPGA_SET_REG(ix,fld,val) \ fpga_set_reg((ix), \ &fpga_ptr->fld, \ offsetof(struct ihs_fpga, fld), \ val)
so this turns into a plain and simple
FPGA_SET_REG(0, mc_tx_address, addr); FPGA_SET_REG(0, mc_tx_data, data); FPGA_SET_REG(0, mc_tx_cmd, (slave & 0x03) << 14);
What do you think?
Best regards,
Wolfgang Denk
Hi Wolfgang,
thanks for investing so much time into this. It is really appreciated.
Am 08.05.2013 12:13, schrieb Wolfgang Denk:
Dear Dirk,
In message 58ff5023adcbf08481bc2707b0a70f50@gdsys.cc you wrote:
in my example we have four FPGAs. One is memory mapped while the other three are not. They all have the same register interface which is mapped by struct ihs_fpga { u16 reflection_low; u16 versions; ... u16 mc_tx_address; u16 mc_tx_data; u16 mc_tx_cmd; ... };
To have instances of those FPGA structs, we might create an array like this: struct ihs_fpga system_fpgas[] = { (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE, (struct ihs_fpga *)NULL, (struct ihs_fpga *)NULL, (struct ihs_fpga *)NULL, }; The first element ist our memory mapped FPGA with base address CONFIG_SYS_FPGA_BASE. For the other three I use NULL as base address because I don't have any better idea what else to choose.
To probe those FPGAs we might have to do something like: for (k=0; k < 4; ++k) fpga_set_reg(k, &system_fpgas[k].reflection_low,
I think using index notation everywhere makes the code hard to read. Use a pointer, for example like that:
struct ihs_fpga *fpgap = system_fpgas[k];
fpga_set_reg(k, &fpgap->reflection_low, ...
The implementation of fpga_set_reg() might look like:
void fpga_set_reg(unsigned int fpga, u16 *reg, u16 data) { switch (fpga) { case 0: out_le16(reg, data); break; default: mclink_send(fpga - 1, (u16)reg, data); break; } }
The problem here comes from your decision to use the same interface for two different, incompatible things. There are two ugly parts in this code: first is the need for a cast, second (and even worse) is the use of a pointer value of 0 to get to the offset of a field. Actually this is not even standard conformng, as you initialize the pointer as NULL, and there is no real guarantee that
NULL == (struct ihs_fpga *)0
You are right. This is *exactly* what I meant when I wrote: "We have FPGAs that are memory mapped and others that are not. They must be accessed by the same drivers. So the alternative would be to create FPGA instances at address NULL and getting the register offesets by casting pointers to u16. Not very nice either." on monday.
It appears you need both the element address and the offset in your fpga_set_reg() code, so you should pass this information, like that [note that we no longer need an array of addresses]:
struct ihs_fpga fpga_ptr = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE; ...
void fpga_set_reg(u32 fpga, u16 *reg, off_t regoff, u16 data) { if (fpga) return mclink_send(fpga - 1, regoff, data);
return out_le16(reg, data); }
where mclink_send() is the routine to access the non memory mapped FPGAs through the memory mapped FPGA: int mclink_send(u8 slave, u16 addr, u16 data) { ... fpga_set_reg(0, &system_fpgas[0].mc_tx_address, addr); fpga_set_reg(0, &system_fpgas[0].mc_tx_data, data); fpga_set_reg(0, &system_fpgas[0].mc_tx_cmd, (slave & 0x03) << 14); }
And this becomes:
fpga_set_reg(0, &fpga_ptr->mc_tx_address, offsetof(struct ihs_fpga, mc_tx_address), addr);
etc. Yes, this is long and ugly to write, so you may want to define a helper macro like:
#define FPGA_SET_REG(ix,fld,val) \ fpga_set_reg((ix), \ &fpga_ptr->fld, \ offsetof(struct ihs_fpga, fld), \ val)
so this turns into a plain and simple
FPGA_SET_REG(0, mc_tx_address, addr); FPGA_SET_REG(0, mc_tx_data, data); FPGA_SET_REG(0, mc_tx_cmd, (slave & 0x03) << 14);
What do you think?
Very nice. I think this looks supringsingly similar to my original implementation in the patch series. There we have: #define REG(reg) offsetof(struct ihs_fpga, reg) and fpga_set_reg(k, REG(reflection_low), REFLECTION_TESTPATTERN); and void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) { int res; struct ihs_fpga *fpga_0 = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(0);
switch (fpga) { case 0: out_le16((u16 *)fpga_0 + reg / sizeof(u16), data); break; default: res = mclink_send(fpga - 1, reg, data); if (res < 0) printf("mclink_send reg %02x data %04x returned %d\n", reg, data, res); break; } }
Sorry, I'm a little confused here. Is your approach an improvement anyway?
Cheers Dirk
Dear Dirk,
In message 913a7e8badaaed06605fdf68faacedc8@gdsys.cc you wrote:
thanks for investing so much time into this. It is really appreciated.
Well, just rejecting a patch without being able to give recommendations _how_ to improve it is not exactly nice. Sometimes it happens (like due to lack of time and/or better knowledge), but I try to avoid that.
NULL == (struct ihs_fpga *)0
You are right. This is *exactly* what I meant when I wrote: "We have FPGAs that are memory mapped and others that are not. They must be accessed by the same drivers. So the alternative would be to create FPGA instances at address NULL and getting the register offesets by casting pointers to u16. Not very nice either." on monday.
You don't get what I mean. There is no guarantee by the C standard that the "value" of NULL actually resolves to 0; it could be something else. So assuming it does is not only ugly, but non-conforming to the C standard, and thus error prone.
What do you think?
Very nice. I think this looks supringsingly similar to my original implementation in the patch series. There we have:
Yes, except that now we still have 1) the full type checking when we access struct elements in the standard I/O accessors and 2) no need for casts when using offsets, and 3) no need to assume that NULL == 0.
Sorry, I'm a little confused here. Is your approach an improvement anyway?
Yes, definitely. At least I'm convinced of that.
Best regards,
Wolfgang Denk
Hello Wolfgang,
sorry for the delay, I had to clean some other things on my ToDoList.
I cleaned up my patchseries and wanted to start implementing your proposal. And ran into a problem. Remember, the basic reason for the the generic FPGA accessors was, that we have a certain amount of basically identical FPGAs which are only connected to the CPU in different ways. The drivers accessing those FPGAs should be agnostic of that and simply be able to access those FPGAs by index.
It appears you need both the element address and the offset in your fpga_set_reg() code, so you should pass this information, like that [note that we no longer need an array of addresses]:
struct ihs_fpga fpga_ptr = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE; ...
void fpga_set_reg(u32 fpga, u16 *reg, off_t regoff, u16 data) { if (fpga) return mclink_send(fpga - 1, regoff, data);
return out_le16(reg, data); }
where mclink_send() is the routine to access the non memory mapped FPGAs through the memory mapped FPGA: int mclink_send(u8 slave, u16 addr, u16 data) { ... fpga_set_reg(0, &system_fpgas[0].mc_tx_address, addr); fpga_set_reg(0, &system_fpgas[0].mc_tx_data, data); fpga_set_reg(0, &system_fpgas[0].mc_tx_cmd, (slave & 0x03) << 14); }
And this becomes:
fpga_set_reg(0, &fpga_ptr->mc_tx_address, offsetof(struct ihs_fpga, mc_tx_address), addr);
etc. Yes, this is long and ugly to write, so you may want to define a helper macro like:
#define FPGA_SET_REG(ix,fld,val) \ fpga_set_reg((ix), \ &fpga_ptr->fld, \ offsetof(struct ihs_fpga, fld), \ val)
so this turns into a plain and simple
FPGA_SET_REG(0, mc_tx_address, addr); FPGA_SET_REG(0, mc_tx_data, data); FPGA_SET_REG(0, mc_tx_cmd, (slave & 0x03) << 14);
This works nicely for our memory mapped FPGA. But how about the other FPGAs? I don't have a fpga_ptr for them. Setting it NULL would make FPGA_SET_REG dereference a NULL pointer. Certainly I might call fpga_set_reg(1, NULL, offsetof(struct ihs_fpga, mc_tx_address), addr); but that would not be generic any more.
Do you have any idea how to solve this?
Cheers Dirk
Hello Wolfgang,
would you please have a look at this? I need some feedback to get this finally sorted.
Cheers Dirk
2013/5/15 Dirk Eibach dirk.eibach@gdsys.cc:
Hello Wolfgang,
sorry for the delay, I had to clean some other things on my ToDoList.
I cleaned up my patchseries and wanted to start implementing your proposal. And ran into a problem. Remember, the basic reason for the the generic FPGA accessors was, that we have a certain amount of basically identical FPGAs which are only connected to the CPU in different ways. The drivers accessing those FPGAs should be agnostic of that and simply be able to access those FPGAs by index.
It appears you need both the element address and the offset in your fpga_set_reg() code, so you should pass this information, like that [note that we no longer need an array of addresses]:
struct ihs_fpga fpga_ptr = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE; ...
void fpga_set_reg(u32 fpga, u16 *reg, off_t regoff, u16 data) { if (fpga) return mclink_send(fpga - 1, regoff, data);
return out_le16(reg, data);}
where mclink_send() is the routine to access the non memory mapped FPGAs through the memory mapped FPGA: int mclink_send(u8 slave, u16 addr, u16 data) { ... fpga_set_reg(0, &system_fpgas[0].mc_tx_address, addr); fpga_set_reg(0, &system_fpgas[0].mc_tx_data, data); fpga_set_reg(0, &system_fpgas[0].mc_tx_cmd, (slave & 0x03) << 14); }
And this becomes:
fpga_set_reg(0, &fpga_ptr->mc_tx_address, offsetof(struct ihs_fpga, mc_tx_address), addr);etc. Yes, this is long and ugly to write, so you may want to define a helper macro like:
#define FPGA_SET_REG(ix,fld,val) \ fpga_set_reg((ix), \ &fpga_ptr->fld, \ offsetof(struct ihs_fpga, fld), \ val)
so this turns into a plain and simple
FPGA_SET_REG(0, mc_tx_address, addr); FPGA_SET_REG(0, mc_tx_data, data); FPGA_SET_REG(0, mc_tx_cmd, (slave & 0x03) << 14);This works nicely for our memory mapped FPGA. But how about the other FPGAs? I don't have a fpga_ptr for them. Setting it NULL would make FPGA_SET_REG dereference a NULL pointer. Certainly I might call fpga_set_reg(1, NULL, offsetof(struct ihs_fpga, mc_tx_address), addr); but that would not be generic any more.
Do you have any idea how to solve this?
Cheers Dirk _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hello Wolfgang,
would you please have a look at this? I need some feedback to get this finally sorted.
Cheers Dirk
2013/5/15 Dirk Eibach dirk.eibach@gdsys.cc:
Hello Wolfgang,
sorry for the delay, I had to clean some other things on my ToDoList.
I cleaned up my patchseries and wanted to start implementing your proposal. And ran into a problem. Remember, the basic reason for the the generic FPGA accessors was, that we have a certain amount of basically identical FPGAs which are only connected to the CPU in different ways. The drivers accessing those FPGAs should be agnostic of that and simply be able to access those FPGAs by index.
It appears you need both the element address and the offset in your fpga_set_reg() code, so you should pass this information, like that [note that we no longer need an array of addresses]:
struct ihs_fpga fpga_ptr = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE; ...
void fpga_set_reg(u32 fpga, u16 *reg, off_t regoff, u16 data) { if (fpga) return mclink_send(fpga - 1, regoff, data);
return out_le16(reg, data);}
where mclink_send() is the routine to access the non memory mapped FPGAs through the memory mapped FPGA: int mclink_send(u8 slave, u16 addr, u16 data) { ... fpga_set_reg(0, &system_fpgas[0].mc_tx_address, addr); fpga_set_reg(0, &system_fpgas[0].mc_tx_data, data); fpga_set_reg(0, &system_fpgas[0].mc_tx_cmd, (slave & 0x03) << 14); }
And this becomes:
fpga_set_reg(0, &fpga_ptr->mc_tx_address, offsetof(struct ihs_fpga, mc_tx_address), addr);etc. Yes, this is long and ugly to write, so you may want to define a helper macro like:
#define FPGA_SET_REG(ix,fld,val) \ fpga_set_reg((ix), \ &fpga_ptr->fld, \ offsetof(struct ihs_fpga, fld), \ val)
so this turns into a plain and simple
FPGA_SET_REG(0, mc_tx_address, addr); FPGA_SET_REG(0, mc_tx_data, data); FPGA_SET_REG(0, mc_tx_cmd, (slave & 0x03) << 14);This works nicely for our memory mapped FPGA. But how about the other FPGAs? I don't have a fpga_ptr for them. Setting it NULL would make FPGA_SET_REG dereference a NULL pointer. Certainly I might call fpga_set_reg(1, NULL, offsetof(struct ihs_fpga, mc_tx_address), addr); but that would not be generic any more.
Do you have any idea how to solve this?
Cheers Dirk _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Dear Dirk,
In message 0e4604dcfff89a1bd2b7f144a5b3ceae@gdsys.cc you wrote:
sorry for the delay, I had to clean some other things on my ToDoList.
Ditto :-(
void fpga_set_reg(u32 fpga, u16 *reg, off_t regoff, u16 data) { if (fpga) return mclink_send(fpga - 1, regoff, data);
return out_le16(reg, data); }
...
#define FPGA_SET_REG(ix,fld,val) \ fpga_set_reg((ix), \ &fpga_ptr->fld, \ offsetof(struct ihs_fpga, fld), \ val)
...
FPGA_SET_REG(0, mc_tx_address, addr); FPGA_SET_REG(0, mc_tx_data, data); FPGA_SET_REG(0, mc_tx_cmd, (slave & 0x03) << 14);
This works nicely for our memory mapped FPGA. But how about the other FPGAs? I don't have a fpga_ptr for them. Setting it NULL would make FPGA_SET_REG dereference a NULL pointer.
No, it does not.
In your setup you have a single memory mapped FPGA, addressed by the globally visible pointer fpga_ptr.
When accessing this memory mapped FPGA (index is zero), then this pointer gets used, which is OK.
For the other devices (index is not zero), then the pointer is ignored, and only the offset is used.
In case you had more memory mapped devices (not only index 0; say, something like index < N) you can still use this, and pass any (properly aligned) pointer for the non-memory mapped case. Note that the pointer will NOT be dereferenced - only the address of field "fld" will be calculated (and even this only in theory, because this code will not be executed for the non-memory mapped case).
Certainly I might call fpga_set_reg(1, NULL, offsetof(struct ihs_fpga, mc_tx_address), addr); but that would not be generic any more.
You don't call fpga_set_reg() any more, you just use the FPGA_SET_REG() macro described above.
Do you have any idea how to solve this?
I think this needs no new solution, because it shoul just work :-)
Best regards,
Wolfgang Denk
From: Dirk Eibach eibach@gdsys.de
Marvell 88E1518 has an erratum that requires fixing up. This patch checks for presence of this phy and adds code for fixup.
Signed-off-by: Dirk Eibach dirk.eibach@gdsys.cc --- Changes in v2: None
board/gdsys/405ep/iocon.c | 214 +++++++++++++++++++++++++++++++++++++++++++++ include/configs/iocon.h | 3 + 2 files changed, 217 insertions(+), 0 deletions(-)
diff --git a/board/gdsys/405ep/iocon.c b/board/gdsys/405ep/iocon.c index 0ab8772..bf84ae8 100644 --- a/board/gdsys/405ep/iocon.c +++ b/board/gdsys/405ep/iocon.c @@ -38,6 +38,8 @@ #include <pca953x.h> #include <pca9698.h>
+#include <miiphy.h> + DECLARE_GLOBAL_DATA_PTR;
#define LATCH0_BASE (CONFIG_SYS_LATCH_BASE) @@ -95,8 +97,16 @@ enum { MCFPGA_RESET_N = 1 << 4, };
+enum { + GPIO_MDC = 1 << 14, + GPIO_MDIO = 1 << 15, +}; + unsigned int mclink_fpgacount;
+static int setup_88e1518(const char *bus, unsigned char addr); +static int verify_88e1518(const char *bus, unsigned char addr); + void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) { int res; @@ -339,6 +349,7 @@ int last_stage_init(void) int slaves; unsigned int k; unsigned char mclink_controllers[] = { 0x24, 0x25, 0x26 }; + int legacy = get_fpga_state(0) & FPGA_STATE_PLATFORM;
print_fpga_info(0); osd_probe(0); @@ -360,6 +371,16 @@ int last_stage_init(void) } }
+ if (!legacy) { + miiphy_register(bb_miiphy_buses[0].name, bb_miiphy_read, + bb_miiphy_write); + if (!verify_88e1518(bb_miiphy_buses[0].name, 0)) { + printf("Fixup 88e1518 erratum on %s\n", + bb_miiphy_buses[0].name); + setup_88e1518(bb_miiphy_buses[0].name, 0); + } + } + /* wait for slave-PLLs to be up and running */ udelay(500000);
@@ -375,6 +396,13 @@ int last_stage_init(void) for (k = 1; k <= slaves; ++k) { print_fpga_info(k); osd_probe(k); + miiphy_register(bb_miiphy_buses[k].name, + bb_miiphy_read, bb_miiphy_write); + if (!verify_88e1518(bb_miiphy_buses[k].name, 0)) { + printf("Fixup 88e1518 erratum on %s\n", + bb_miiphy_buses[k].name); + setup_88e1518(bb_miiphy_buses[k].name, 0); + } }
return 0; @@ -446,3 +474,189 @@ int gd405ep_get_fpga_done(unsigned fpga) else return pca9698_get_value(0x20, 20); } + +/* + * FPGA MII bitbang implementation + */ + +struct fpga_mii { + unsigned fpga; + int mdio; +} fpga_mii[] = { + { 0, 1}, + { 1, 1}, + { 2, 1}, + { 3, 1}, +}; + +static int mii_dummy_init(struct bb_miiphy_bus *bus) +{ + return 0; +} + +static int mii_mdio_active(struct bb_miiphy_bus *bus) +{ + struct fpga_mii *fpga_mii = bus->priv; + + if (fpga_mii->mdio) + fpga_set_reg(fpga_mii->fpga, REG(gpio.set), GPIO_MDIO); + else + fpga_set_reg(fpga_mii->fpga, REG(gpio.clear), GPIO_MDIO); + + return 0; +} + +static int mii_mdio_tristate(struct bb_miiphy_bus *bus) +{ + struct fpga_mii *fpga_mii = bus->priv; + + fpga_set_reg(fpga_mii->fpga, REG(gpio.set), GPIO_MDIO); + + return 0; +} + +static int mii_set_mdio(struct bb_miiphy_bus *bus, int v) +{ + struct fpga_mii *fpga_mii = bus->priv; + + if (v) + fpga_set_reg(fpga_mii->fpga, REG(gpio.set), GPIO_MDIO); + else + fpga_set_reg(fpga_mii->fpga, REG(gpio.clear), GPIO_MDIO); + + fpga_mii->mdio = v; + + return 0; +} + +static int mii_get_mdio(struct bb_miiphy_bus *bus, int *v) +{ + struct fpga_mii *fpga_mii = bus->priv; + + *v = ((fpga_get_reg(fpga_mii->fpga, REG(gpio.read)) & GPIO_MDIO) != 0); + + return 0; +} + +static int mii_set_mdc(struct bb_miiphy_bus *bus, int v) +{ + struct fpga_mii *fpga_mii = bus->priv; + + if (v) + fpga_set_reg(fpga_mii->fpga, REG(gpio.set), GPIO_MDC); + else + fpga_set_reg(fpga_mii->fpga, REG(gpio.clear), GPIO_MDC); + + return 0; +} + +static int mii_delay(struct bb_miiphy_bus *bus) +{ + udelay(1); + + return 0; +} + +struct bb_miiphy_bus bb_miiphy_buses[] = { + { + .name = "trans1", + .init = mii_dummy_init, + .mdio_active = mii_mdio_active, + .mdio_tristate = mii_mdio_tristate, + .set_mdio = mii_set_mdio, + .get_mdio = mii_get_mdio, + .set_mdc = mii_set_mdc, + .delay = mii_delay, + .priv = &fpga_mii[0], + }, + { + .name = "trans2", + .init = mii_dummy_init, + .mdio_active = mii_mdio_active, + .mdio_tristate = mii_mdio_tristate, + .set_mdio = mii_set_mdio, + .get_mdio = mii_get_mdio, + .set_mdc = mii_set_mdc, + .delay = mii_delay, + .priv = &fpga_mii[1], + }, + { + .name = "trans3", + .init = mii_dummy_init, + .mdio_active = mii_mdio_active, + .mdio_tristate = mii_mdio_tristate, + .set_mdio = mii_set_mdio, + .get_mdio = mii_get_mdio, + .set_mdc = mii_set_mdc, + .delay = mii_delay, + .priv = &fpga_mii[2], + }, + { + .name = "trans4", + .init = mii_dummy_init, + .mdio_active = mii_mdio_active, + .mdio_tristate = mii_mdio_tristate, + .set_mdio = mii_set_mdio, + .get_mdio = mii_get_mdio, + .set_mdc = mii_set_mdc, + .delay = mii_delay, + .priv = &fpga_mii[3], + }, +}; + +int bb_miiphy_buses_num = sizeof(bb_miiphy_buses) / + sizeof(bb_miiphy_buses[0]); + +/* + * Workaround for erratum mentioned in 88E1518 release notes + */ + +static int verify_88e1518(const char *bus, unsigned char addr) +{ + u16 phy_id1, phy_id2; + + if (miiphy_read(bus, addr, 2, &phy_id1) || + miiphy_read(bus, addr, 3, &phy_id2)) { + printf("Error reading from the PHY addr=%02x\n", addr); + return -EIO; + } + + if ((phy_id1 != 0x0141) || ((phy_id2 & 0xfff0) != 0x0dd0)) + return -EINVAL; + + return 0; +} + +struct regfix_88e1518 { + u8 reg; + u16 data; +} regfix_88e1518[] = { + { 22, 0x00ff }, + { 17, 0x214b }, + { 16, 0x2144 }, + { 17, 0x0c28 }, + { 16, 0x2146 }, + { 17, 0xb233 }, + { 16, 0x214d }, + { 17, 0xcc0c }, + { 16, 0x2159 }, + { 22, 0x00fb }, + { 7, 0xc00d }, + { 22, 0x0000 }, +}; + +static int setup_88e1518(const char *bus, unsigned char addr) +{ + unsigned int k; + + for (k = 0; k < ARRAY_SIZE(regfix_88e1518); ++k) { + if (miiphy_write(bus, addr, + regfix_88e1518[k].reg, + regfix_88e1518[k].data)) { + printf("Error writing to the PHY addr=%02x\n", addr); + return -1; + } + } + + return 0; +} diff --git a/include/configs/iocon.h b/include/configs/iocon.h index 921ac05..d5f48d3 100644 --- a/include/configs/iocon.h +++ b/include/configs/iocon.h @@ -295,4 +295,7 @@ int fpga_gpio_get(unsigned int bus, int pin); #define CONFIG_SYS_CH7301 #define CONFIG_SYS_OSD_SCREENS 1
+#define CONFIG_BITBANGMII /* bit-bang MII PHY management */ +#define CONFIG_BITBANGMII_MULTI + #endif /* __CONFIG_H */
From: Dirk Eibach eibach@gdsys.de
Signed-off-by: Dirk Eibach dirk.eibach@gdsys.cc --- Changes in v2: None
board/gdsys/common/mclink.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/board/gdsys/common/mclink.c b/board/gdsys/common/mclink.c index 9a4f3e9..d2983bb 100644 --- a/board/gdsys/common/mclink.c +++ b/board/gdsys/common/mclink.c @@ -40,11 +40,11 @@ enum { int mclink_probe(void) { unsigned int k; - unsigned int ctr = 0; int slaves = 0;
for (k = 0; k < CONFIG_SYS_MCLINK_MAX; ++k) { int timeout = 0; + unsigned int ctr = 0;
if (!(fpga_get_reg(k, REG(mc_status)) & (1 << 15))) break; @@ -53,7 +53,7 @@ int mclink_probe(void)
while (!(fpga_get_reg(k, REG(mc_status)) & (1 << 14))) { udelay(100); - if (ctr++ > 3) { + if (ctr++ > 500) { timeout = 1; break; } @@ -61,6 +61,8 @@ int mclink_probe(void) if (timeout) break;
+ printf("waited %d us for mclink %d to come up\n", ctr * 100, k); + slaves++; }
From: Dirk Eibach eibach@gdsys.de
OSD size was constant 32x16 characters. Now the size is set as announced by the FPGA.
Signed-off-by: Dirk Eibach dirk.eibach@gdsys.cc --- Changes in v2: None
board/gdsys/common/osd.c | 34 ++++++++++++++++++---------------- 1 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/board/gdsys/common/osd.c b/board/gdsys/common/osd.c index 2cfbe58..9ce51cf 100644 --- a/board/gdsys/common/osd.c +++ b/board/gdsys/common/osd.c @@ -43,10 +43,6 @@
#define PIXCLK_640_480_60 25180000
-#define BASE_WIDTH 32 -#define BASE_HEIGHT 16 -#define BUFSIZE (BASE_WIDTH * BASE_HEIGHT) - enum { CH7301_CM = 0x1c, /* Clock Mode Register */ CH7301_IC = 0x1d, /* Input Clock Register */ @@ -68,6 +64,11 @@ enum { CH7301_DSP = 0x56, /* DVI Sync polarity Register */ };
+unsigned int base_width; +unsigned int base_height; +size_t bufsize; +u16 *buf; + unsigned int max_osd_screen = CONFIG_SYS_OSD_SCREENS - 1;
#ifdef CONFIG_SYS_CH7301 @@ -269,7 +270,7 @@ static int osd_write_videomem(unsigned screen, unsigned offset, unsigned int k;
for (k = 0; k < charcount; ++k) { - if (offset + k >= BUFSIZE) + if (offset + k >= bufsize) return -1; fpga_set_reg(screen, REG(videomem) + sizeof(u16) * (offset + k), data[k]); @@ -289,7 +290,6 @@ static int osd_print(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) unsigned len; u8 color; unsigned int k; - u16 buf[BUFSIZE]; char *text; int res;
@@ -303,12 +303,12 @@ static int osd_print(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) color = simple_strtoul(argv[3], NULL, 16); text = argv[4]; charcount = strlen(text); - len = (charcount > BUFSIZE) ? BUFSIZE : charcount; + len = (charcount > bufsize) ? bufsize : charcount;
for (k = 0; k < len; ++k) buf[k] = (text[k] << 8) | color;
- res = osd_write_videomem(screen, y * BASE_WIDTH + x, buf, len); + res = osd_write_videomem(screen, y * base_width + x, buf, len); if (res < 0) return res; } @@ -320,18 +320,20 @@ int osd_probe(unsigned screen) { u16 version = fpga_get_reg(screen, REG(osd.version)); u16 features = fpga_get_reg(screen, REG(osd.features)); - unsigned width; - unsigned height; u8 value; #ifdef CONFIG_SYS_CH7301 int old_bus = i2c_get_bus_num(); #endif
- width = ((features & 0x3f00) >> 8) + 1; - height = (features & 0x001f) + 1; + base_width = ((features & 0x3f00) >> 8) + 1; + base_height = (features & 0x001f) + 1; + bufsize = base_width * base_height; + buf = malloc(sizeof(u16) * bufsize); + if (!buf) + return -1;
printf("OSD%d: Digital-OSD version %01d.%02d, %d" "x%d characters\n", - screen, version/100, version%100, width, height); + screen, version/100, version%100, base_width, base_height);
#ifdef CONFIG_SYS_CH7301 i2c_set_bus_num(ch7301_i2c[screen]); @@ -397,7 +399,7 @@ int osd_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) unsigned x; unsigned y; unsigned k; - u16 buffer[BASE_WIDTH]; + u16 buffer[base_width]; char *rp; u16 *wp = buffer; unsigned count = (argc > 4) ? @@ -422,13 +424,13 @@ int osd_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
rp += 4; wp++; - if (wp - buffer > BASE_WIDTH) + if (wp - buffer > base_width) break; }
for (k = 0; k < count; ++k) { unsigned offset = - y * BASE_WIDTH + x + k * (wp - buffer); + y * base_width + x + k * (wp - buffer); osd_write_videomem(screen, offset, buffer, wp - buffer); }
From: Dirk Eibach eibach@gdsys.de
CONFIG_SYS_FLASH_PROTECTION was active on most gdsys boards by default, while hardware flash protection was not implemented. Hardware support was added recently and we get into trouble because backward compatibility is broken (u-boot can't unprotect the protected flash after a downgrade). So we decided to disable hardware flash protection for all our boards.
Signed-off-by: Dirk Eibach dirk.eibach@gdsys.cc
--- Changes in v2: None
include/configs/dlvision-10g.h | 3 +-- include/configs/dlvision.h | 3 +-- include/configs/io.h | 3 +-- include/configs/iocon.h | 3 +-- include/configs/neo.h | 3 +-- 5 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/include/configs/dlvision-10g.h b/include/configs/dlvision-10g.h index 5d6308f..6528055 100644 --- a/include/configs/dlvision-10g.h +++ b/include/configs/dlvision-10g.h @@ -34,7 +34,7 @@ * Include common defines/options for all AMCC eval boards */ #define CONFIG_HOSTNAME dlvsion-10g -#define CONFIG_IDENT_STRING " dlvision-10g 0.03" +#define CONFIG_IDENT_STRING " dlvision-10g 0.04" #include "amcc-common.h"
#define CONFIG_BOARD_EARLY_INIT_F @@ -161,7 +161,6 @@ #define CONFIG_SYS_FLASH_WRITE_TOUT 500 /* Timeout for Flash Write/ms */
#define CONFIG_SYS_FLASH_USE_BUFFER_WRITE 1 /* use buff'd writes */ -#define CONFIG_SYS_FLASH_PROTECTION 1 /* use hardware flash protect */
#define CONFIG_SYS_FLASH_EMPTY_INFO /* 'E' for empty sector on flinfo */ #define CONFIG_SYS_FLASH_QUIET_TEST 1 /* no warn upon unknown flash */ diff --git a/include/configs/dlvision.h b/include/configs/dlvision.h index 24c9fa4..948930e 100644 --- a/include/configs/dlvision.h +++ b/include/configs/dlvision.h @@ -34,7 +34,7 @@ * Include common defines/options for all AMCC eval boards */ #define CONFIG_HOSTNAME dlvision -#define CONFIG_IDENT_STRING " dlvision 0.01" +#define CONFIG_IDENT_STRING " dlvision 0.02" #include "amcc-common.h"
#define CONFIG_BOARD_EARLY_INIT_F /* call board_early_init_f */ @@ -125,7 +125,6 @@ #define CONFIG_SYS_FLASH_WRITE_TOUT 500 /* Timeout for Flash Write/ms */
#define CONFIG_SYS_FLASH_USE_BUFFER_WRITE 1 /* use buff'd writes */ -#define CONFIG_SYS_FLASH_PROTECTION 1 /* use hardware flash protect */
#define CONFIG_SYS_FLASH_EMPTY_INFO /* 'E' for empty sector on flinfo */ #define CONFIG_SYS_FLASH_QUIET_TEST 1 /* no warn upon unknown flash */ diff --git a/include/configs/io.h b/include/configs/io.h index 4950ad7..0985cf0 100644 --- a/include/configs/io.h +++ b/include/configs/io.h @@ -34,7 +34,7 @@ * Include common defines/options for all AMCC eval boards */ #define CONFIG_HOSTNAME io -#define CONFIG_IDENT_STRING " io 0.05" +#define CONFIG_IDENT_STRING " io 0.06" #include "amcc-common.h"
#define CONFIG_BOARD_EARLY_INIT_F @@ -139,7 +139,6 @@ #define CONFIG_SYS_FLASH_WRITE_TOUT 500 /* Timeout for Flash Write/ms */
#define CONFIG_SYS_FLASH_USE_BUFFER_WRITE 1 /* use buff'd writes */ -#define CONFIG_SYS_FLASH_PROTECTION 1 /* use hardware flash protect */
#define CONFIG_SYS_FLASH_EMPTY_INFO /* 'E' for empty sector on flinfo */ #define CONFIG_SYS_FLASH_QUIET_TEST 1 /* no warn upon unknown flash */ diff --git a/include/configs/iocon.h b/include/configs/iocon.h index d5f48d3..4ac785a 100644 --- a/include/configs/iocon.h +++ b/include/configs/iocon.h @@ -34,7 +34,7 @@ * Include common defines/options for all AMCC eval boards */ #define CONFIG_HOSTNAME iocon -#define CONFIG_IDENT_STRING " iocon 0.04" +#define CONFIG_IDENT_STRING " iocon 0.05" #include "amcc-common.h"
#define CONFIG_BOARD_EARLY_INIT_F @@ -181,7 +181,6 @@ int fpga_gpio_get(unsigned int bus, int pin); #define CONFIG_SYS_FLASH_WRITE_TOUT 500 /* Timeout for Flash Write/ms */
#define CONFIG_SYS_FLASH_USE_BUFFER_WRITE 1 /* use buff'd writes */ -#define CONFIG_SYS_FLASH_PROTECTION 1 /* use hardware flash protect */
#define CONFIG_SYS_FLASH_EMPTY_INFO /* 'E' for empty sector on flinfo */ #define CONFIG_SYS_FLASH_QUIET_TEST 1 /* no warn upon unknown flash */ diff --git a/include/configs/neo.h b/include/configs/neo.h index be1c9ff..e1b832a 100644 --- a/include/configs/neo.h +++ b/include/configs/neo.h @@ -35,7 +35,7 @@ * Include common defines/options for all AMCC eval boards */ #define CONFIG_HOSTNAME neo -#define CONFIG_IDENT_STRING " neo 0.01" +#define CONFIG_IDENT_STRING " neo 0.02" #include "amcc-common.h"
#define CONFIG_BOARD_EARLY_INIT_F @@ -146,7 +146,6 @@ #define CONFIG_SYS_FLASH_WRITE_TOUT 500 /* Timeout for Flash Write (in ms) */
#define CONFIG_SYS_FLASH_USE_BUFFER_WRITE 1 /* use buffered writes (20x faster) */ -#define CONFIG_SYS_FLASH_PROTECTION 1 /* use hardware flash protection */
#define CONFIG_SYS_FLASH_EMPTY_INFO /* print 'E' for empty sector on flinfo */ #define CONFIG_SYS_FLASH_QUIET_TEST 1 /* don't warn upon unknown flash */
Dear dirk.eibach@gdsys.cc,
In message 1367847325-21463-1-git-send-email-dirk.eibach@gdsys.cc you wrote:
powerpc/ppc4xx: Add generic accessor functions for gdsys FPGA powerpc/ppc4xx: Add gdsys mclink interface powerpc/ppc4xx: Add fpgad command for dumping gdsys fpga registers powerpc/ppc4xx: Use generic FPGA accessors in gdsys common code powerpc/ppc4xx: Support gdsys multichannel iocon hardware powerpc/ppc4xx: Use generic FPGA accessors on all gdsys 405ep/ex boards powerpc/ppc4xx: Fixup phy erratum on gdsys iocon hardware powerpc/ppc4xx: Increase timeout for gdsys mclink bus startup powerpc/ppc4xx: Consider gdsys FPGA OSD size powerpc/ppc4xx: Remove CONFIG_SYS_FLASH_PROTECTION from gdsys boards
This sequence of patches is not logically ordered, and split into patches at random. Please group stuff the belongs together, and squash patches theat define things with those that actually use it.
Best regards,
Wolfgang Denk
participants (6)
-
Dirk Eibach -
Dirk Eibach -
dirk.eibach＠gdsys.cc
-
Joe Hershberger -
Tom Rini -
Wolfgang Denk