* [PATCH v2] i2c: add Marvell 64xxx driver @ 2014-07-16 21:25 Antony Pavlov 2014-07-16 21:25 ` Antony Pavlov 0 siblings, 1 reply; 10+ messages in thread From: Antony Pavlov @ 2014-07-16 21:25 UTC (permalink / raw) To: barebox Changes since PATCH v1 (http://lists.infradead.org/pipermail/barebox/2014-June/019471.html): * check for timeout during mv64xxx_i2c_wait_for_completion(). Antony Pavlov (1): i2c: add Marvell 64xxx driver drivers/i2c/busses/Kconfig | 8 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-mv64xxx.c | 687 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 696 insertions(+) create mode 100644 drivers/i2c/busses/i2c-mv64xxx.c -- 2.0.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] i2c: add Marvell 64xxx driver 2014-07-16 21:25 [PATCH v2] i2c: add Marvell 64xxx driver Antony Pavlov @ 2014-07-16 21:25 ` Antony Pavlov 2014-07-17 9:33 ` Sebastian Hesselbarth 2014-07-22 8:51 ` Sebastian Hesselbarth 0 siblings, 2 replies; 10+ messages in thread From: Antony Pavlov @ 2014-07-16 21:25 UTC (permalink / raw) To: barebox This driver is also used for Allwinner SoCs I2C controllers. Ported from linux-3.15. The most notable barebox driver version changes: * drop message offloading support; * add reg-io-width parameter to use driver with byte-oriented controller versions. Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> --- drivers/i2c/busses/Kconfig | 8 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-mv64xxx.c | 687 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 696 insertions(+) diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 370abb0..851556f 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -16,6 +16,14 @@ config I2C_IMX bool "MPC85xx/i.MX I2C Master driver" depends on (ARCH_IMX && !ARCH_IMX1) || ARCH_MPC85XX +config I2C_MV64XXX + bool "Marvell mv64xxx I2C Controller" + depends on HAVE_CLK && OFDEVICE + help + If you say yes to this option, support will be included for the + built-in I2C interface on the Marvell 64xxx line of host bridges. + This driver is also used for Allwinner SoCs I2C controllers. + config I2C_OMAP bool "OMAP I2C Master driver" depends on ARCH_OMAP diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 9823d1b..4e4f6ba 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -1,5 +1,6 @@ obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o obj-$(CONFIG_I2C_IMX) += i2c-imx.o +obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o obj-$(CONFIG_I2C_OMAP) += i2c-omap.o obj-$(CONFIG_I2C_TEGRA) += i2c-tegra.o obj-$(CONFIG_I2C_VERSATILE) += i2c-versatile.o diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c new file mode 100644 index 0000000..324796a --- /dev/null +++ b/drivers/i2c/busses/i2c-mv64xxx.c @@ -0,0 +1,687 @@ +/* + * Driver for the i2c controller on the Marvell line of host bridges + * (e.g, gt642[46]0, mv643[46]0, mv644[46]0, and Orion SoC family). + * + * This code was ported from linux-3.15 kernel by Antony Pavlov. + * + * Author: Mark A. Greer <mgreer@mvista.com> + * + * 2005 (c) MontaVista, Software, Inc. This file is licensed under + * the terms of the GNU General Public License version 2. This program + * is licensed "as is" without any warranty of any kind, whether express + * or implied. + */ +#include <common.h> +#include <driver.h> +#include <init.h> +#include <of.h> +#include <malloc.h> +#include <types.h> +#include <xfuncs.h> +#include <clock.h> +#include <linux/clk.h> +#include <linux/err.h> + +#include <io.h> +#include <i2c/i2c.h> +#include <printk.h> + +#define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1) +#define MV64XXX_I2C_BAUD_DIV_N(val) (val & 0x7) +#define MV64XXX_I2C_BAUD_DIV_M(val) ((val & 0xf) << 3) + +#define MV64XXX_I2C_REG_CONTROL_ACK 0x00000004 +#define MV64XXX_I2C_REG_CONTROL_IFLG 0x00000008 +#define MV64XXX_I2C_REG_CONTROL_STOP 0x00000010 +#define MV64XXX_I2C_REG_CONTROL_START 0x00000020 +#define MV64XXX_I2C_REG_CONTROL_TWSIEN 0x00000040 +#define MV64XXX_I2C_REG_CONTROL_INTEN 0x00000080 + +/* Ctlr status values */ +#define MV64XXX_I2C_STATUS_BUS_ERR 0x00 +#define MV64XXX_I2C_STATUS_MAST_START 0x08 +#define MV64XXX_I2C_STATUS_MAST_REPEAT_START 0x10 +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK 0x18 +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK 0x20 +#define MV64XXX_I2C_STATUS_MAST_WR_ACK 0x28 +#define MV64XXX_I2C_STATUS_MAST_WR_NO_ACK 0x30 +#define MV64XXX_I2C_STATUS_MAST_LOST_ARB 0x38 +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK 0x40 +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK 0x48 +#define MV64XXX_I2C_STATUS_MAST_RD_DATA_ACK 0x50 +#define MV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK 0x58 +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK 0xd0 +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_NO_ACK 0xd8 +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK 0xe0 +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK 0xe8 +#define MV64XXX_I2C_STATUS_NO_STATUS 0xf8 + +/* Driver states */ +enum { + MV64XXX_I2C_STATE_INVALID, + MV64XXX_I2C_STATE_IDLE, + MV64XXX_I2C_STATE_WAITING_FOR_START_COND, + MV64XXX_I2C_STATE_WAITING_FOR_RESTART, + MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK, + MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK, + MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK, + MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA, +}; + +/* Driver actions */ +enum { + MV64XXX_I2C_ACTION_INVALID, + MV64XXX_I2C_ACTION_CONTINUE, + MV64XXX_I2C_ACTION_SEND_RESTART, + MV64XXX_I2C_ACTION_OFFLOAD_RESTART, + MV64XXX_I2C_ACTION_SEND_ADDR_1, + MV64XXX_I2C_ACTION_SEND_ADDR_2, + MV64XXX_I2C_ACTION_SEND_DATA, + MV64XXX_I2C_ACTION_RCV_DATA, + MV64XXX_I2C_ACTION_RCV_DATA_STOP, + MV64XXX_I2C_ACTION_SEND_STOP, + MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP, +}; + +struct mv64xxx_i2c_regs { + u8 addr; + u8 ext_addr; + u8 data; + u8 control; + u8 status; + u8 clock; + u8 soft_reset; +}; + +struct mv64xxx_i2c_data { + struct i2c_msg *msgs; + int num_msgs; + u32 state; + u32 action; + u32 aborting; + u32 cntl_bits; + void __iomem *reg_base; + struct mv64xxx_i2c_regs reg_offsets; + u32 addr1; + u32 addr2; + u32 bytes_left; + u32 byte_posn; + u32 send_stop; + u32 block; + int rc; + u32 freq_m; + u32 freq_n; + struct clk *clk; + struct i2c_msg *msg; + struct i2c_adapter adapter; +/* 5us delay in order to avoid repeated start timing violation */ + bool errata_delay; + struct reset_control *rstc; + bool irq_clear_inverted; + int reg_io_width; +}; + +static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = { + .addr = 0x00, + .ext_addr = 0x10, + .data = 0x04, + .control = 0x08, + .status = 0x0c, + .clock = 0x0c, + .soft_reset = 0x1c, +}; + +static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_sun4i = { + .addr = 0x00, + .ext_addr = 0x04, + .data = 0x08, + .control = 0x0c, + .status = 0x10, + .clock = 0x14, + .soft_reset = 0x18, +}; + +static inline void mv64xxx_write(struct mv64xxx_i2c_data *drv_data, u32 v, int reg) +{ + void *addr = drv_data->reg_base + reg; + + switch (drv_data->reg_io_width) { + case IORESOURCE_MEM_8BIT: + writeb((u8)v, addr); + break; + case IORESOURCE_MEM_32BIT: + writel(v, addr); + break; + default: + dev_err(&drv_data->adapter.dev, + "%s: wrong reg_io_width!\n", __func__); + } +} + +static inline u32 mv64xxx_read(struct mv64xxx_i2c_data *drv_data, int reg) +{ + void *addr = drv_data->reg_base + reg; + u32 r; + + switch (drv_data->reg_io_width) { + case IORESOURCE_MEM_8BIT: + r = readb(addr); + break; + + case IORESOURCE_MEM_32BIT: + r = readl(addr); + break; + + default: + dev_err(&drv_data->adapter.dev, + "%s: wrong reg_io_width!\n", __func__); + r = 0xffffffff; + } + + return r; +} + +static void +mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, + struct i2c_msg *msg) +{ + u32 dir = 0; + + drv_data->cntl_bits = MV64XXX_I2C_REG_CONTROL_ACK | + MV64XXX_I2C_REG_CONTROL_INTEN | MV64XXX_I2C_REG_CONTROL_TWSIEN; + + if (msg->flags & I2C_M_RD) + dir = 1; + + if (msg->flags & I2C_M_TEN) { + drv_data->addr1 = 0xf0 | (((u32)msg->addr & 0x300) >> 7) | dir; + drv_data->addr2 = (u32)msg->addr & 0xff; + } else { + drv_data->addr1 = MV64XXX_I2C_ADDR_ADDR((u32)msg->addr) | dir; + drv_data->addr2 = 0; + } +} + +/* + ***************************************************************************** + * + * Finite State Machine & Interrupt Routines + * + ***************************************************************************** + */ + +/* Reset hardware and initialize FSM */ +static void +mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data) +{ + mv64xxx_write(drv_data, 0, drv_data->reg_offsets.soft_reset); + mv64xxx_write(drv_data, MV64XXX_I2C_BAUD_DIV_M(drv_data->freq_m) | MV64XXX_I2C_BAUD_DIV_N(drv_data->freq_n), + drv_data->reg_offsets.clock); + mv64xxx_write(drv_data, 0, drv_data->reg_offsets.addr); + mv64xxx_write(drv_data, 0, drv_data->reg_offsets.ext_addr); + mv64xxx_write(drv_data, MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP, + drv_data->reg_offsets.control); + drv_data->state = MV64XXX_I2C_STATE_IDLE; +} + +static void +mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status) +{ + /* + * If state is idle, then this is likely the remnants of an old + * operation that driver has given up on or the user has killed. + * If so, issue the stop condition and go to idle. + */ + if (drv_data->state == MV64XXX_I2C_STATE_IDLE) { + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; + return; + } + + /* The status from the ctlr [mostly] tells us what to do next */ + switch (status) { + /* Start condition interrupt */ + case MV64XXX_I2C_STATUS_MAST_START: /* 0x08 */ + case MV64XXX_I2C_STATUS_MAST_REPEAT_START: /* 0x10 */ + drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_1; + drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK; + break; + + /* Performing a write */ + case MV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK: /* 0x18 */ + if (drv_data->msg->flags & I2C_M_TEN) { + drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_2; + drv_data->state = + MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK; + break; + } + /* FALLTHRU */ + case MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK: /* 0xd0 */ + case MV64XXX_I2C_STATUS_MAST_WR_ACK: /* 0x28 */ + if ((drv_data->bytes_left == 0) + || (drv_data->aborting + && (drv_data->byte_posn != 0))) { + if (drv_data->send_stop || drv_data->aborting) { + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; + drv_data->state = MV64XXX_I2C_STATE_IDLE; + } else { + drv_data->action = + MV64XXX_I2C_ACTION_SEND_RESTART; + drv_data->state = + MV64XXX_I2C_STATE_WAITING_FOR_RESTART; + } + } else { + drv_data->action = MV64XXX_I2C_ACTION_SEND_DATA; + drv_data->state = + MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK; + drv_data->bytes_left--; + } + break; + + /* Performing a read */ + case MV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK: /* 40 */ + if (drv_data->msg->flags & I2C_M_TEN) { + drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_2; + drv_data->state = + MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK; + break; + } + /* FALLTHRU */ + case MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK: /* 0xe0 */ + if (drv_data->bytes_left == 0) { + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; + drv_data->state = MV64XXX_I2C_STATE_IDLE; + break; + } + /* FALLTHRU */ + case MV64XXX_I2C_STATUS_MAST_RD_DATA_ACK: /* 0x50 */ + if (status != MV64XXX_I2C_STATUS_MAST_RD_DATA_ACK) + drv_data->action = MV64XXX_I2C_ACTION_CONTINUE; + else { + drv_data->action = MV64XXX_I2C_ACTION_RCV_DATA; + drv_data->bytes_left--; + } + drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA; + + if ((drv_data->bytes_left == 1) || drv_data->aborting) + drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_ACK; + break; + + case MV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK: /* 0x58 */ + drv_data->action = MV64XXX_I2C_ACTION_RCV_DATA_STOP; + drv_data->state = MV64XXX_I2C_STATE_IDLE; + break; + + case MV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK: /* 0x20 */ + case MV64XXX_I2C_STATUS_MAST_WR_NO_ACK: /* 30 */ + case MV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK: /* 48 */ + /* Doesn't seem to be a device at other end */ + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; + drv_data->state = MV64XXX_I2C_STATE_IDLE; + drv_data->rc = -ENXIO; + break; + + default: + dev_err(&drv_data->adapter.dev, + "mv64xxx_i2c_fsm: Ctlr Error -- state: 0x%x, " + "status: 0x%x, addr: 0x%x, flags: 0x%x\n", + drv_data->state, status, drv_data->msg->addr, + drv_data->msg->flags); + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; + mv64xxx_i2c_hw_init(drv_data); + drv_data->rc = -EIO; + } +} + +static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data) +{ + drv_data->msg = drv_data->msgs; + drv_data->byte_posn = 0; + drv_data->bytes_left = drv_data->msg->len; + drv_data->aborting = 0; + drv_data->rc = 0; + + mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs); + mv64xxx_write(drv_data, drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START, + drv_data->reg_offsets.control); +} + +static void +mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) +{ + switch (drv_data->action) { + case MV64XXX_I2C_ACTION_SEND_RESTART: + /* We should only get here if we have further messages */ + BUG_ON(drv_data->num_msgs == 0); + + drv_data->msgs++; + drv_data->num_msgs--; + mv64xxx_i2c_send_start(drv_data); + + if (drv_data->errata_delay) + udelay(5); + + /* + * We're never at the start of the message here, and by this + * time it's already too late to do any protocol mangling. + * Thankfully, do not advertise support for that feature. + */ + drv_data->send_stop = drv_data->num_msgs == 1; + break; + + case MV64XXX_I2C_ACTION_CONTINUE: + mv64xxx_write(drv_data, drv_data->cntl_bits, + drv_data->reg_offsets.control); + break; + + case MV64XXX_I2C_ACTION_SEND_ADDR_1: + mv64xxx_write(drv_data, drv_data->addr1, + drv_data->reg_offsets.data); + mv64xxx_write(drv_data, drv_data->cntl_bits, + drv_data->reg_offsets.control); + break; + + case MV64XXX_I2C_ACTION_SEND_ADDR_2: + mv64xxx_write(drv_data, drv_data->addr2, + drv_data->reg_offsets.data); + mv64xxx_write(drv_data, drv_data->cntl_bits, + drv_data->reg_offsets.control); + break; + + case MV64XXX_I2C_ACTION_SEND_DATA: + mv64xxx_write(drv_data, drv_data->msg->buf[drv_data->byte_posn++], + drv_data->reg_offsets.data); + mv64xxx_write(drv_data, drv_data->cntl_bits, + drv_data->reg_offsets.control); + break; + + case MV64XXX_I2C_ACTION_RCV_DATA: + drv_data->msg->buf[drv_data->byte_posn++] = + mv64xxx_read(drv_data, drv_data->reg_offsets.data); + mv64xxx_write(drv_data, drv_data->cntl_bits, + drv_data->reg_offsets.control); + break; + + case MV64XXX_I2C_ACTION_RCV_DATA_STOP: + drv_data->msg->buf[drv_data->byte_posn++] = + mv64xxx_read(drv_data, drv_data->reg_offsets.data); + drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; + mv64xxx_write(drv_data, drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, + drv_data->reg_offsets.control); + drv_data->block = 0; + if (drv_data->errata_delay) + udelay(5); + + break; + + case MV64XXX_I2C_ACTION_INVALID: + default: + dev_err(&drv_data->adapter.dev, + "mv64xxx_i2c_do_action: Invalid action: %d\n", + drv_data->action); + drv_data->rc = -EIO; + + /* FALLTHRU */ + case MV64XXX_I2C_ACTION_SEND_STOP: + drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; + mv64xxx_write(drv_data, drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, + drv_data->reg_offsets.control); + drv_data->block = 0; + break; + } +} + +static void mv64xxx_i2c_intr(struct mv64xxx_i2c_data *drv_data) +{ + u32 status; + uint64_t start; + + start = get_time_ns(); + + while (mv64xxx_read(drv_data, drv_data->reg_offsets.control) & + MV64XXX_I2C_REG_CONTROL_IFLG) { + status = mv64xxx_read(drv_data, drv_data->reg_offsets.status); + mv64xxx_i2c_fsm(drv_data, status); + mv64xxx_i2c_do_action(drv_data); + + if (drv_data->irq_clear_inverted) + mv64xxx_write(drv_data, drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_IFLG, + drv_data->reg_offsets.control); + + if (is_timeout_non_interruptible(start, 3 * SECOND)) { + drv_data->rc = -EIO; + break; + } + } +} + +/* + ***************************************************************************** + * + * I2C Msg Execution Routines + * + ***************************************************************************** + */ +static void +mv64xxx_i2c_wait_for_completion(struct mv64xxx_i2c_data *drv_data) +{ + do { + mv64xxx_i2c_intr(drv_data); + if (drv_data->rc) { + drv_data->state = MV64XXX_I2C_STATE_IDLE; + dev_err(&drv_data->adapter.dev, "I2C bus error\n"); + mv64xxx_i2c_hw_init(drv_data); + drv_data->block = 0; + } + } while (drv_data->block != 0); +} + +static int +mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg, + int is_last) +{ + drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND; + + drv_data->send_stop = is_last; + drv_data->block = 1; + mv64xxx_i2c_send_start(drv_data); + mv64xxx_i2c_wait_for_completion(drv_data); + + return drv_data->rc; +} + +/* + ***************************************************************************** + * + * I2C Core Support Routines (Interface to higher level I2C code) + * + ***************************************************************************** + */ +static int +mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) +{ + struct mv64xxx_i2c_data *drv_data = container_of(adap, struct mv64xxx_i2c_data, adapter); + int rc, ret = num; + + BUG_ON(drv_data->msgs != NULL); + drv_data->msgs = msgs; + drv_data->num_msgs = num; + + rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[0], num == 1); + if (rc < 0) + ret = rc; + + drv_data->num_msgs = 0; + drv_data->msgs = NULL; + + return ret; +} + +/* + ***************************************************************************** + * + * Driver Interface & Early Init Routines + * + ***************************************************************************** + */ +static struct of_device_id mv64xxx_i2c_of_match_table[] = { + { .compatible = "allwinner,sun4i-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_sun4i}, + { .compatible = "allwinner,sun6i-a31-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_sun4i}, + { .compatible = "marvell,mv64xxx-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_mv64xxx}, + { .compatible = "marvell,mv78230-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_mv64xxx}, + { .compatible = "marvell,mv78230-a0-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_mv64xxx}, + {} +}; + +static int +mv64xxx_calc_freq(const int tclk, const int n, const int m) +{ + return tclk / (10 * (m + 1) * (2 << n)); +} + +static bool +mv64xxx_find_baud_factors(const u32 req_freq, const u32 tclk, u32 *best_n, + u32 *best_m) +{ + int freq, delta, best_delta = INT_MAX; + int m, n; + + for (n = 0; n <= 7; n++) + for (m = 0; m <= 15; m++) { + freq = mv64xxx_calc_freq(tclk, n, m); + delta = req_freq - freq; + if (delta >= 0 && delta < best_delta) { + *best_m = m; + *best_n = n; + best_delta = delta; + } + if (best_delta == 0) + return true; + } + if (best_delta == INT_MAX) + return false; + return true; +} + +static int +mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, + struct device_d *pd) +{ + struct device_node *np = pd->device_node; + u32 bus_freq, tclk; + int rc = 0; + u32 prop; + struct mv64xxx_i2c_regs *mv64xxx_regs; + int freq; + + if (IS_ERR(drv_data->clk)) { + rc = -ENODEV; + goto out; + } + tclk = clk_get_rate(drv_data->clk); + + rc = of_property_read_u32(np, "clock-frequency", &bus_freq); + if (rc) + bus_freq = 100000; /* 100kHz by default */ + + if (!mv64xxx_find_baud_factors(bus_freq, tclk, + &drv_data->freq_n, &drv_data->freq_m)) { + rc = -EINVAL; + goto out; + } + + freq = mv64xxx_calc_freq(tclk, drv_data->freq_n, drv_data->freq_m); + dev_dbg(pd, "tclk=%d freq_n=%d freq_m=%d freq=%d\n", + tclk, drv_data->freq_n, drv_data->freq_m, freq); + + drv_data->reg_io_width = IORESOURCE_MEM_32BIT; + + if (of_property_read_u32(np, "reg-io-width", &prop) == 0) { + switch (prop) { + case 1: + drv_data->reg_io_width = IORESOURCE_MEM_8BIT; + break; + case 4: + drv_data->reg_io_width = IORESOURCE_MEM_32BIT; + break; + default: + dev_err(pd, "unsupported reg-io-width (%d)\n", prop); + rc = -EINVAL; + goto out; + } + } + + dev_get_drvdata(pd, (unsigned long *)&mv64xxx_regs); + memcpy(&drv_data->reg_offsets, mv64xxx_regs, + sizeof(drv_data->reg_offsets)); + + /* + * For controllers embedded in new SoCs activate the + * Transaction Generator support and the errata fix. + */ + if (of_device_is_compatible(np, "marvell,mv78230-i2c")) { + drv_data->errata_delay = true; + } + + if (of_device_is_compatible(np, "marvell,mv78230-a0-i2c")) { + drv_data->errata_delay = true; + } + + if (of_device_is_compatible(np, "allwinner,sun6i-a31-i2c")) + drv_data->irq_clear_inverted = true; + +out: + return rc; +} + +static int +mv64xxx_i2c_probe(struct device_d *pd) +{ + struct mv64xxx_i2c_data *drv_data; + int rc; + + if (!pd->device_node) + return -ENODEV; + + drv_data = xzalloc(sizeof(*drv_data)); + + drv_data->reg_base = dev_request_mem_region(pd, 0); + if (IS_ERR(drv_data->reg_base)) + return PTR_ERR(drv_data->reg_base); + + drv_data->clk = clk_get(pd, NULL); + if (IS_ERR(drv_data->clk)) + return PTR_ERR(drv_data->clk); + + clk_enable(drv_data->clk); + + rc = mv64xxx_of_config(drv_data, pd); + if (rc) + goto exit_clk; + + drv_data->adapter.master_xfer = mv64xxx_i2c_xfer; + drv_data->adapter.dev.parent = pd; + drv_data->adapter.nr = pd->id; + drv_data->adapter.dev.device_node = pd->device_node; + + mv64xxx_i2c_hw_init(drv_data); + + rc = i2c_add_numbered_adapter(&drv_data->adapter); + if (rc) { + dev_err(pd, "Failed to add I2C adapter\n"); + goto exit_clk; + } + + return 0; + +exit_clk: + clk_disable(drv_data->clk); + + return rc; +} + +static struct driver_d mv64xxx_i2c_driver = { + .probe = mv64xxx_i2c_probe, + .name = "mv64xxx_i2c", + .of_compatible = DRV_OF_COMPAT(mv64xxx_i2c_of_match_table), +}; +device_platform_driver(mv64xxx_i2c_driver); -- 2.0.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: add Marvell 64xxx driver 2014-07-16 21:25 ` Antony Pavlov @ 2014-07-17 9:33 ` Sebastian Hesselbarth 2014-07-17 11:59 ` Antony Pavlov 2014-07-22 8:51 ` Sebastian Hesselbarth 1 sibling, 1 reply; 10+ messages in thread From: Sebastian Hesselbarth @ 2014-07-17 9:33 UTC (permalink / raw) To: Antony Pavlov, barebox On 07/16/2014 11:25 PM, Antony Pavlov wrote: > This driver is also used for Allwinner SoCs I2C controllers. > > Ported from linux-3.15. > > The most notable barebox driver version changes: > > * drop message offloading support; > * add reg-io-width parameter to use driver with byte-oriented > controller versions. > > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> > --- [...] > diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c > new file mode 100644 > index 0000000..324796a > --- /dev/null > +++ b/drivers/i2c/busses/i2c-mv64xxx.c > @@ -0,0 +1,687 @@ > +/* > + * Driver for the i2c controller on the Marvell line of host bridges > + * (e.g, gt642[46]0, mv643[46]0, mv644[46]0, and Orion SoC family). > + * > + * This code was ported from linux-3.15 kernel by Antony Pavlov. > + * > + * Author: Mark A. Greer <mgreer@mvista.com> > + * > + * 2005 (c) MontaVista, Software, Inc. This file is licensed under > + * the terms of the GNU General Public License version 2. This program > + * is licensed "as is" without any warranty of any kind, whether express > + * or implied. > + */ > +#include <common.h> > +#include <driver.h> > +#include <init.h> > +#include <of.h> > +#include <malloc.h> > +#include <types.h> > +#include <xfuncs.h> > +#include <clock.h> > +#include <linux/clk.h> > +#include <linux/err.h> > + > +#include <io.h> > +#include <i2c/i2c.h> > +#include <printk.h> > + > +#define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1) > +#define MV64XXX_I2C_BAUD_DIV_N(val) (val & 0x7) > +#define MV64XXX_I2C_BAUD_DIV_M(val) ((val & 0xf) << 3) > + > +#define MV64XXX_I2C_REG_CONTROL_ACK 0x00000004 > +#define MV64XXX_I2C_REG_CONTROL_IFLG 0x00000008 > +#define MV64XXX_I2C_REG_CONTROL_STOP 0x00000010 > +#define MV64XXX_I2C_REG_CONTROL_START 0x00000020 > +#define MV64XXX_I2C_REG_CONTROL_TWSIEN 0x00000040 > +#define MV64XXX_I2C_REG_CONTROL_INTEN 0x00000080 Antony, I'll have a look at the driver itself later, but your patch is full of superfluous tabs like above between '#define' and 'MV64XXX_foo'. Isn't checkpatch.pl complaining about it? Anyway, no need to resend before I have tested the driver itself. Sebastian _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: add Marvell 64xxx driver 2014-07-17 9:33 ` Sebastian Hesselbarth @ 2014-07-17 11:59 ` Antony Pavlov 0 siblings, 0 replies; 10+ messages in thread From: Antony Pavlov @ 2014-07-17 11:59 UTC (permalink / raw) To: Sebastian Hesselbarth; +Cc: barebox On Thu, 17 Jul 2014 11:33:29 +0200 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > On 07/16/2014 11:25 PM, Antony Pavlov wrote: > > This driver is also used for Allwinner SoCs I2C controllers. > > > > Ported from linux-3.15. > > > > The most notable barebox driver version changes: > > > > * drop message offloading support; > > * add reg-io-width parameter to use driver with byte-oriented > > controller versions. > > > > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> > > --- > [...] > > diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c > > new file mode 100644 > > index 0000000..324796a > > --- /dev/null > > +++ b/drivers/i2c/busses/i2c-mv64xxx.c > > @@ -0,0 +1,687 @@ > > +/* > > + * Driver for the i2c controller on the Marvell line of host bridges > > + * (e.g, gt642[46]0, mv643[46]0, mv644[46]0, and Orion SoC family). > > + * > > + * This code was ported from linux-3.15 kernel by Antony Pavlov. > > + * > > + * Author: Mark A. Greer <mgreer@mvista.com> > > + * > > + * 2005 (c) MontaVista, Software, Inc. This file is licensed under > > + * the terms of the GNU General Public License version 2. This program > > + * is licensed "as is" without any warranty of any kind, whether express > > + * or implied. > > + */ > > +#include <common.h> > > +#include <driver.h> > > +#include <init.h> > > +#include <of.h> > > +#include <malloc.h> > > +#include <types.h> > > +#include <xfuncs.h> > > +#include <clock.h> > > +#include <linux/clk.h> > > +#include <linux/err.h> > > + > > +#include <io.h> > > +#include <i2c/i2c.h> > > +#include <printk.h> > > + > > +#define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1) > > +#define MV64XXX_I2C_BAUD_DIV_N(val) (val & 0x7) > > +#define MV64XXX_I2C_BAUD_DIV_M(val) ((val & 0xf) << 3) > > + > > +#define MV64XXX_I2C_REG_CONTROL_ACK 0x00000004 > > +#define MV64XXX_I2C_REG_CONTROL_IFLG 0x00000008 > > +#define MV64XXX_I2C_REG_CONTROL_STOP 0x00000010 > > +#define MV64XXX_I2C_REG_CONTROL_START 0x00000020 > > +#define MV64XXX_I2C_REG_CONTROL_TWSIEN 0x00000040 > > +#define MV64XXX_I2C_REG_CONTROL_INTEN 0x00000080 > > Antony, > > I'll have a look at the driver itself later, but your patch is full > of superfluous tabs like above between '#define' and 'MV64XXX_foo'. I have kept original linux driver formatting. Please see it with any interactive two panels diff tool (e.g. vimdiff): vimdiff barebox.git/drivers/i2c/busses/i2c-mv64xxx.c linux.git/drivers/i2c/busses/i2c-mv64xxx.c > > Isn't checkpatch.pl complaining about it? On the one hand I need to change all writel/readl to mv64xxx_write/mv64xxx_read, so some lines become 'over 80 characters'. But on the other hand I'm trying to keep formatting compatibility with original linux driver; I decided to keep original formatting even if lines become 'over 80 characters'. Moreover Sascha said that there is no problem with 'line over 80 characters' warnings :) see http://lists.infradead.org/pipermail/barebox/2014-June/019484.html > Anyway, no need to resend before I have tested the driver itself. Great! The driver needs addition tests. -- Best regards, Antony Pavlov _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: add Marvell 64xxx driver 2014-07-16 21:25 ` Antony Pavlov 2014-07-17 9:33 ` Sebastian Hesselbarth @ 2014-07-22 8:51 ` Sebastian Hesselbarth 2014-07-22 9:57 ` Antony Pavlov 2014-07-22 16:21 ` Antony Pavlov 1 sibling, 2 replies; 10+ messages in thread From: Sebastian Hesselbarth @ 2014-07-22 8:51 UTC (permalink / raw) To: Antony Pavlov, barebox On 07/16/2014 11:25 PM, Antony Pavlov wrote: > This driver is also used for Allwinner SoCs I2C controllers. > > Ported from linux-3.15. > > The most notable barebox driver version changes: > > * drop message offloading support; > * add reg-io-width parameter to use driver with byte-oriented > controller versions. > > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> Antony, I finally finished work on xHCI and PCI on Armada 370. Now I come back with the promised review of the i2c driver. I gave this driver a quick test on Mirabox, i2c_probe just gives I2C bus errors. What SoC did you test the driver on? I'll now have a closer look at why it fails, but I already have some comments below. > --- > drivers/i2c/busses/Kconfig | 8 + > drivers/i2c/busses/Makefile | 1 + > drivers/i2c/busses/i2c-mv64xxx.c | 687 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 696 insertions(+) > [...] > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile > index 9823d1b..4e4f6ba 100644 > --- a/drivers/i2c/busses/Makefile > +++ b/drivers/i2c/busses/Makefile > @@ -1,5 +1,6 @@ > obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o > obj-$(CONFIG_I2C_IMX) += i2c-imx.o > +obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o > obj-$(CONFIG_I2C_OMAP) += i2c-omap.o IMHO, you can also fixup the indention of i2c-imx.o and i2c-omap while you are at it. > obj-$(CONFIG_I2C_TEGRA) += i2c-tegra.o > obj-$(CONFIG_I2C_VERSATILE) += i2c-versatile.o > diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c > new file mode 100644 > index 0000000..324796a > --- /dev/null > +++ b/drivers/i2c/busses/i2c-mv64xxx.c > @@ -0,0 +1,687 @@ > +/* > + * Driver for the i2c controller on the Marvell line of host bridges > + * (e.g, gt642[46]0, mv643[46]0, mv644[46]0, and Orion SoC family). From above list, it is more than unlikely that we will see support for any of the mv643foo devices. How about to rename the driver to i2c-orion, get rid of mv643foo, and add Allwinner SoCs to the list above? > + * > + * This code was ported from linux-3.15 kernel by Antony Pavlov. > + * > + * Author: Mark A. Greer <mgreer@mvista.com> > + * > + * 2005 (c) MontaVista, Software, Inc. This file is licensed under > + * the terms of the GNU General Public License version 2. This program > + * is licensed "as is" without any warranty of any kind, whether express > + * or implied. > + */ > +#include <common.h> > +#include <driver.h> > +#include <init.h> > +#include <of.h> > +#include <malloc.h> > +#include <types.h> > +#include <xfuncs.h> > +#include <clock.h> > +#include <linux/clk.h> > +#include <linux/err.h> > + > +#include <io.h> > +#include <i2c/i2c.h> > +#include <printk.h> > + > +#define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1) > +#define MV64XXX_I2C_BAUD_DIV_N(val) (val & 0x7) > +#define MV64XXX_I2C_BAUD_DIV_M(val) ((val & 0xf) << 3) > + > +#define MV64XXX_I2C_REG_CONTROL_ACK 0x00000004 > +#define MV64XXX_I2C_REG_CONTROL_IFLG 0x00000008 > +#define MV64XXX_I2C_REG_CONTROL_STOP 0x00000010 > +#define MV64XXX_I2C_REG_CONTROL_START 0x00000020 > +#define MV64XXX_I2C_REG_CONTROL_TWSIEN 0x00000040 > +#define MV64XXX_I2C_REG_CONTROL_INTEN 0x00000080 As I said before, I see no point in tabs between #define and MV64XXX_FOO. It is not about the 80 column rule, but general style instead. Also, you can use BIT(x) for above defines. > + > +/* Ctlr status values */ > +#define MV64XXX_I2C_STATUS_BUS_ERR 0x00 > +#define MV64XXX_I2C_STATUS_MAST_START 0x08 > +#define MV64XXX_I2C_STATUS_MAST_REPEAT_START 0x10 > +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK 0x18 > +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK 0x20 > +#define MV64XXX_I2C_STATUS_MAST_WR_ACK 0x28 > +#define MV64XXX_I2C_STATUS_MAST_WR_NO_ACK 0x30 > +#define MV64XXX_I2C_STATUS_MAST_LOST_ARB 0x38 > +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK 0x40 > +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK 0x48 > +#define MV64XXX_I2C_STATUS_MAST_RD_DATA_ACK 0x50 > +#define MV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK 0x58 > +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK 0xd0 > +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_NO_ACK 0xd8 > +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK 0xe0 > +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK 0xe8 > +#define MV64XXX_I2C_STATUS_NO_STATUS 0xf8 > + > +/* Driver states */ > +enum { enum mv64xxx_state { and get rid of MV64XXX_I2C_ prefix below. > + MV64XXX_I2C_STATE_INVALID, > + MV64XXX_I2C_STATE_IDLE, > + MV64XXX_I2C_STATE_WAITING_FOR_START_COND, > + MV64XXX_I2C_STATE_WAITING_FOR_RESTART, > + MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK, > + MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK, > + MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK, > + MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA, > +}; > + > +/* Driver actions */ > +enum { enum mv64xxx_action { and get rid of MV64XXX_I2C_ prefix below. > + MV64XXX_I2C_ACTION_INVALID, > + MV64XXX_I2C_ACTION_CONTINUE, > + MV64XXX_I2C_ACTION_SEND_RESTART, > + MV64XXX_I2C_ACTION_OFFLOAD_RESTART, > + MV64XXX_I2C_ACTION_SEND_ADDR_1, > + MV64XXX_I2C_ACTION_SEND_ADDR_2, > + MV64XXX_I2C_ACTION_SEND_DATA, > + MV64XXX_I2C_ACTION_RCV_DATA, > + MV64XXX_I2C_ACTION_RCV_DATA_STOP, > + MV64XXX_I2C_ACTION_SEND_STOP, > + MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP, > +}; > + > +struct mv64xxx_i2c_regs { > + u8 addr; > + u8 ext_addr; > + u8 data; > + u8 control; > + u8 status; > + u8 clock; > + u8 soft_reset; > +}; > + > +struct mv64xxx_i2c_data { > + struct i2c_msg *msgs; > + int num_msgs; > + u32 state; > + u32 action; enum mv64xxx_state state; enum mv64xxx_action action; > + u32 aborting; You are never aborting, so get rid of it and the logic completely. > + u32 cntl_bits; u8 cntl_bits; Although Orion SoCs allow 32b access, the registers are always 8b. > + void __iomem *reg_base; If you struggle with long lines, '*regs' or '*base' is sufficient. > + struct mv64xxx_i2c_regs reg_offsets; ditto, just choose shorter names. > + u32 addr1; > + u32 addr2; If you want to keep the state machine and track all msg stuff, u8 is enough for both addrN above. > + u32 bytes_left; > + u32 byte_posn; ditto, IIRC i2c doesn't even allow you to send more than 255 bytes per message nor will you ever find a device that supports it. > + u32 send_stop; I wonder if you'll ever send a restart at all, that will leave the stop to the last message transferred. In any way, above should be bool. > + u32 block; Whatever block is for, remember that you will send each byte individually and you'll ever be in charge of the code, i.e. if you wait for completion, barebox will wait for it. There is no threading nor interrupts. > + int rc; > + u32 freq_m; > + u32 freq_n; You could just init the HW when you found the best dividers. No need to store them for eternity. > + struct clk *clk; > + struct i2c_msg *msg; > + struct i2c_adapter adapter; > +/* 5us delay in order to avoid repeated start timing violation */ > + bool errata_delay; > + struct reset_control *rstc; Unused. > + bool irq_clear_inverted; > + int reg_io_width; > +}; > + > +static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = { __maybe_unused and see below at compatibles table. > + .addr = 0x00, > + .ext_addr = 0x10, > + .data = 0x04, > + .control = 0x08, > + .status = 0x0c, > + .clock = 0x0c, > + .soft_reset = 0x1c, > +}; > + > +static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_sun4i = { ditto. > + .addr = 0x00, > + .ext_addr = 0x04, > + .data = 0x08, > + .control = 0x0c, > + .status = 0x10, > + .clock = 0x14, > + .soft_reset = 0x18, > +}; > + > +static inline void mv64xxx_write(struct mv64xxx_i2c_data *drv_data, u32 v, int reg) How about having a callback for this and _read below? You'd install the callback in _probe() and get rid of reg_io_width check for every read/write access, i.e static void mv64xxx_writel(...) { writel(v, addr); } static void mv64xxx_writeb(...) { writeb(v, addr); } static int mv64xxx_i2c_probe(...) { ... switch (reg_io_width) { case 1: drv_data->read = mv64xxx_readb; drv_data->write = mv64xxx_writeb; break; case 4: drv_data->read = mv64xxx_readl; drv_data->write = mv64xxx_writel; break; default: dev_err(pd, "unsupported reg-io-width (%d)\n", reg_io_width); rc = -EINVAL; goto out; } ... } > +{ > + void *addr = drv_data->reg_base + reg; > + > + switch (drv_data->reg_io_width) { > + case IORESOURCE_MEM_8BIT: > + writeb((u8)v, addr); > + break; > + case IORESOURCE_MEM_32BIT: > + writel(v, addr); > + break; > + default: > + dev_err(&drv_data->adapter.dev, > + "%s: wrong reg_io_width!\n", __func__); Beside the comment above, you already made sure reg_io_width will never be anything else than 8BIT or 32BIT. So the default case is not needed at all. > + } > +} > + > +static inline u32 mv64xxx_read(struct mv64xxx_i2c_data *drv_data, int reg) > +{ > + void *addr = drv_data->reg_base + reg; > + u32 r; > + > + switch (drv_data->reg_io_width) { > + case IORESOURCE_MEM_8BIT: > + r = readb(addr); > + break; > + > + case IORESOURCE_MEM_32BIT: > + r = readl(addr); > + break; > + > + default: > + dev_err(&drv_data->adapter.dev, > + "%s: wrong reg_io_width!\n", __func__); > + r = 0xffffffff; > + } > + > + return r; > +} > + > +static void > +mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, > + struct i2c_msg *msg) > +{ > + u32 dir = 0; > + > + drv_data->cntl_bits = MV64XXX_I2C_REG_CONTROL_ACK | > + MV64XXX_I2C_REG_CONTROL_INTEN | MV64XXX_I2C_REG_CONTROL_TWSIEN; > + > + if (msg->flags & I2C_M_RD) > + dir = 1; > + > + if (msg->flags & I2C_M_TEN) { > + drv_data->addr1 = 0xf0 | (((u32)msg->addr & 0x300) >> 7) | dir; > + drv_data->addr2 = (u32)msg->addr & 0xff; > + } else { > + drv_data->addr1 = MV64XXX_I2C_ADDR_ADDR((u32)msg->addr) | dir; > + drv_data->addr2 = 0; > + } > +} > + > +/* > + ***************************************************************************** > + * > + * Finite State Machine & Interrupt Routines > + * > + ***************************************************************************** > + */ > + > +/* Reset hardware and initialize FSM */ > +static void > +mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data) > +{ > + mv64xxx_write(drv_data, 0, drv_data->reg_offsets.soft_reset); > + mv64xxx_write(drv_data, MV64XXX_I2C_BAUD_DIV_M(drv_data->freq_m) | MV64XXX_I2C_BAUD_DIV_N(drv_data->freq_n), > + drv_data->reg_offsets.clock); > + mv64xxx_write(drv_data, 0, drv_data->reg_offsets.addr); > + mv64xxx_write(drv_data, 0, drv_data->reg_offsets.ext_addr); > + mv64xxx_write(drv_data, MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP, > + drv_data->reg_offsets.control); > + drv_data->state = MV64XXX_I2C_STATE_IDLE; > +} > + > +static void > +mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status) > +{ > + /* > + * If state is idle, then this is likely the remnants of an old > + * operation that driver has given up on or the user has killed. > + * If so, issue the stop condition and go to idle. > + */ > + if (drv_data->state == MV64XXX_I2C_STATE_IDLE) { > + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; > + return; > + } > + > + /* The status from the ctlr [mostly] tells us what to do next */ > + switch (status) { > + /* Start condition interrupt */ > + case MV64XXX_I2C_STATUS_MAST_START: /* 0x08 */ > + case MV64XXX_I2C_STATUS_MAST_REPEAT_START: /* 0x10 */ > + drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_1; > + drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK; > + break; > + > + /* Performing a write */ > + case MV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK: /* 0x18 */ > + if (drv_data->msg->flags & I2C_M_TEN) { > + drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_2; > + drv_data->state = > + MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK; > + break; > + } > + /* FALLTHRU */ > + case MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK: /* 0xd0 */ > + case MV64XXX_I2C_STATUS_MAST_WR_ACK: /* 0x28 */ > + if ((drv_data->bytes_left == 0) > + || (drv_data->aborting > + && (drv_data->byte_posn != 0))) { > + if (drv_data->send_stop || drv_data->aborting) { > + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; > + drv_data->state = MV64XXX_I2C_STATE_IDLE; > + } else { > + drv_data->action = > + MV64XXX_I2C_ACTION_SEND_RESTART; > + drv_data->state = > + MV64XXX_I2C_STATE_WAITING_FOR_RESTART; > + } > + } else { > + drv_data->action = MV64XXX_I2C_ACTION_SEND_DATA; > + drv_data->state = > + MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK; > + drv_data->bytes_left--; > + } > + break; > + > + /* Performing a read */ > + case MV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK: /* 40 */ > + if (drv_data->msg->flags & I2C_M_TEN) { > + drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_2; > + drv_data->state = > + MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK; > + break; > + } > + /* FALLTHRU */ > + case MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK: /* 0xe0 */ > + if (drv_data->bytes_left == 0) { > + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; > + drv_data->state = MV64XXX_I2C_STATE_IDLE; > + break; > + } > + /* FALLTHRU */ > + case MV64XXX_I2C_STATUS_MAST_RD_DATA_ACK: /* 0x50 */ > + if (status != MV64XXX_I2C_STATUS_MAST_RD_DATA_ACK) > + drv_data->action = MV64XXX_I2C_ACTION_CONTINUE; > + else { > + drv_data->action = MV64XXX_I2C_ACTION_RCV_DATA; > + drv_data->bytes_left--; > + } > + drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA; > + > + if ((drv_data->bytes_left == 1) || drv_data->aborting) > + drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_ACK; > + break; > + > + case MV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK: /* 0x58 */ > + drv_data->action = MV64XXX_I2C_ACTION_RCV_DATA_STOP; > + drv_data->state = MV64XXX_I2C_STATE_IDLE; > + break; > + > + case MV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK: /* 0x20 */ > + case MV64XXX_I2C_STATUS_MAST_WR_NO_ACK: /* 30 */ > + case MV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK: /* 48 */ > + /* Doesn't seem to be a device at other end */ > + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; > + drv_data->state = MV64XXX_I2C_STATE_IDLE; > + drv_data->rc = -ENXIO; > + break; > + > + default: > + dev_err(&drv_data->adapter.dev, > + "mv64xxx_i2c_fsm: Ctlr Error -- state: 0x%x, " > + "status: 0x%x, addr: 0x%x, flags: 0x%x\n", > + drv_data->state, status, drv_data->msg->addr, > + drv_data->msg->flags); > + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; > + mv64xxx_i2c_hw_init(drv_data); > + drv_data->rc = -EIO; > + } > +} > + > +static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data) > +{ > + drv_data->msg = drv_data->msgs; > + drv_data->byte_posn = 0; > + drv_data->bytes_left = drv_data->msg->len; > + drv_data->aborting = 0; > + drv_data->rc = 0; > + > + mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs); > + mv64xxx_write(drv_data, drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START, > + drv_data->reg_offsets.control); > +} > + > +static void > +mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) > +{ > + switch (drv_data->action) { > + case MV64XXX_I2C_ACTION_SEND_RESTART: > + /* We should only get here if we have further messages */ > + BUG_ON(drv_data->num_msgs == 0); > + > + drv_data->msgs++; > + drv_data->num_msgs--; > + mv64xxx_i2c_send_start(drv_data); > + > + if (drv_data->errata_delay) > + udelay(5); > + > + /* > + * We're never at the start of the message here, and by this > + * time it's already too late to do any protocol mangling. > + * Thankfully, do not advertise support for that feature. > + */ > + drv_data->send_stop = drv_data->num_msgs == 1; > + break; > + > + case MV64XXX_I2C_ACTION_CONTINUE: > + mv64xxx_write(drv_data, drv_data->cntl_bits, > + drv_data->reg_offsets.control); > + break; > + > + case MV64XXX_I2C_ACTION_SEND_ADDR_1: > + mv64xxx_write(drv_data, drv_data->addr1, > + drv_data->reg_offsets.data); > + mv64xxx_write(drv_data, drv_data->cntl_bits, > + drv_data->reg_offsets.control); > + break; > + > + case MV64XXX_I2C_ACTION_SEND_ADDR_2: > + mv64xxx_write(drv_data, drv_data->addr2, > + drv_data->reg_offsets.data); > + mv64xxx_write(drv_data, drv_data->cntl_bits, > + drv_data->reg_offsets.control); > + break; > + > + case MV64XXX_I2C_ACTION_SEND_DATA: > + mv64xxx_write(drv_data, drv_data->msg->buf[drv_data->byte_posn++], > + drv_data->reg_offsets.data); > + mv64xxx_write(drv_data, drv_data->cntl_bits, > + drv_data->reg_offsets.control); > + break; > + > + case MV64XXX_I2C_ACTION_RCV_DATA: > + drv_data->msg->buf[drv_data->byte_posn++] = > + mv64xxx_read(drv_data, drv_data->reg_offsets.data); > + mv64xxx_write(drv_data, drv_data->cntl_bits, > + drv_data->reg_offsets.control); > + break; > + > + case MV64XXX_I2C_ACTION_RCV_DATA_STOP: > + drv_data->msg->buf[drv_data->byte_posn++] = > + mv64xxx_read(drv_data, drv_data->reg_offsets.data); > + drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; > + mv64xxx_write(drv_data, drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, > + drv_data->reg_offsets.control); > + drv_data->block = 0; > + if (drv_data->errata_delay) > + udelay(5); > + > + break; > + > + case MV64XXX_I2C_ACTION_INVALID: > + default: > + dev_err(&drv_data->adapter.dev, > + "mv64xxx_i2c_do_action: Invalid action: %d\n", > + drv_data->action); > + drv_data->rc = -EIO; > + > + /* FALLTHRU */ > + case MV64XXX_I2C_ACTION_SEND_STOP: > + drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; > + mv64xxx_write(drv_data, drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, > + drv_data->reg_offsets.control); > + drv_data->block = 0; > + break; > + } > +} > + > +static void mv64xxx_i2c_intr(struct mv64xxx_i2c_data *drv_data) Is "intr" for interrupt? Barebox is running with irqs disabled, so maybe rename it to something like "mv64xxx_i2c_wait_for_done" ? > +{ > + u32 status; > + uint64_t start; > + > + start = get_time_ns(); > + > + while (mv64xxx_read(drv_data, drv_data->reg_offsets.control) & > + MV64XXX_I2C_REG_CONTROL_IFLG) { > + status = mv64xxx_read(drv_data, drv_data->reg_offsets.status); > + mv64xxx_i2c_fsm(drv_data, status); > + mv64xxx_i2c_do_action(drv_data); > + > + if (drv_data->irq_clear_inverted) > + mv64xxx_write(drv_data, drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_IFLG, > + drv_data->reg_offsets.control); > + > + if (is_timeout_non_interruptible(start, 3 * SECOND)) { > + drv_data->rc = -EIO; > + break; > + } > + } > +} > + > +/* > + ***************************************************************************** > + * > + * I2C Msg Execution Routines > + * > + ***************************************************************************** > + */ > +static void > +mv64xxx_i2c_wait_for_completion(struct mv64xxx_i2c_data *drv_data) > +{ > + do { > + mv64xxx_i2c_intr(drv_data); > + if (drv_data->rc) { > + drv_data->state = MV64XXX_I2C_STATE_IDLE; > + dev_err(&drv_data->adapter.dev, "I2C bus error\n"); > + mv64xxx_i2c_hw_init(drv_data); > + drv_data->block = 0; > + } > + } while (drv_data->block != 0); > +} > + > +static int > +mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg, > + int is_last) > +{ > + drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND; > + > + drv_data->send_stop = is_last; > + drv_data->block = 1; > + mv64xxx_i2c_send_start(drv_data); > + mv64xxx_i2c_wait_for_completion(drv_data); > + > + return drv_data->rc; > +} > + > +/* > + ***************************************************************************** > + * > + * I2C Core Support Routines (Interface to higher level I2C code) > + * > + ***************************************************************************** > + */ > +static int > +mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > +{ > + struct mv64xxx_i2c_data *drv_data = container_of(adap, struct mv64xxx_i2c_data, adapter); > + int rc, ret = num; > + > + BUG_ON(drv_data->msgs != NULL); > + drv_data->msgs = msgs; > + drv_data->num_msgs = num; > + > + rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[0], num == 1); > + if (rc < 0) > + ret = rc; > + > + drv_data->num_msgs = 0; > + drv_data->msgs = NULL; How about you loop over all passed msgs in here and get rid of the both drv_data variables completely? > + > + return ret; > +} > + > +/* > + ***************************************************************************** > + * > + * Driver Interface & Early Init Routines > + * > + ***************************************************************************** > + */ > +static struct of_device_id mv64xxx_i2c_of_match_table[] = { #if defined(CONFIG_SUN4I_FOO) > + { .compatible = "allwinner,sun4i-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_sun4i}, #endif #if defined(CONFIG_SUN6I_FOO) > + { .compatible = "allwinner,sun6i-a31-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_sun4i}, #endif #if defined(CONFIG_MACH_MVEBU) > + { .compatible = "marvell,mv64xxx-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_mv64xxx}, #endif #if defined(CONFIG_ARCH_ARMADA_XP) > + { .compatible = "marvell,mv78230-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_mv64xxx}, > + { .compatible = "marvell,mv78230-a0-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_mv64xxx}, #endif > + {} > +}; If you ifdef the compatibles, the compiler will be able to remove all structs that are not referenced. > + > +static int > +mv64xxx_calc_freq(const int tclk, const int n, const int m) inline? > +{ > + return tclk / (10 * (m + 1) * (2 << n)); > +} > + > +static bool > +mv64xxx_find_baud_factors(const u32 req_freq, const u32 tclk, u32 *best_n, > + u32 *best_m) > +{ > + int freq, delta, best_delta = INT_MAX; > + int m, n; > + > + for (n = 0; n <= 7; n++) > + for (m = 0; m <= 15; m++) { > + freq = mv64xxx_calc_freq(tclk, n, m); > + delta = req_freq - freq; > + if (delta >= 0 && delta < best_delta) { > + *best_m = m; > + *best_n = n; > + best_delta = delta; > + } > + if (best_delta == 0) > + return true; > + } > + if (best_delta == INT_MAX) > + return false; > + return true; > +} > + > +static int > +mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, > + struct device_d *pd) > +{ > + struct device_node *np = pd->device_node; > + u32 bus_freq, tclk; > + int rc = 0; > + u32 prop; > + struct mv64xxx_i2c_regs *mv64xxx_regs; > + int freq; > + > + if (IS_ERR(drv_data->clk)) { > + rc = -ENODEV; > + goto out; > + } > + tclk = clk_get_rate(drv_data->clk); > + > + rc = of_property_read_u32(np, "clock-frequency", &bus_freq); > + if (rc) > + bus_freq = 100000; /* 100kHz by default */ > + > + if (!mv64xxx_find_baud_factors(bus_freq, tclk, > + &drv_data->freq_n, &drv_data->freq_m)) { > + rc = -EINVAL; > + goto out; > + } > + > + freq = mv64xxx_calc_freq(tclk, drv_data->freq_n, drv_data->freq_m); > + dev_dbg(pd, "tclk=%d freq_n=%d freq_m=%d freq=%d\n", > + tclk, drv_data->freq_n, drv_data->freq_m, freq); > + > + drv_data->reg_io_width = IORESOURCE_MEM_32BIT; > + > + if (of_property_read_u32(np, "reg-io-width", &prop) == 0) { > + switch (prop) { > + case 1: > + drv_data->reg_io_width = IORESOURCE_MEM_8BIT; > + break; > + case 4: > + drv_data->reg_io_width = IORESOURCE_MEM_32BIT; > + break; > + default: > + dev_err(pd, "unsupported reg-io-width (%d)\n", prop); > + rc = -EINVAL; > + goto out; > + } > + } > + > + dev_get_drvdata(pd, (unsigned long *)&mv64xxx_regs); > + memcpy(&drv_data->reg_offsets, mv64xxx_regs, > + sizeof(drv_data->reg_offsets)); > + > + /* > + * For controllers embedded in new SoCs activate the > + * Transaction Generator support and the errata fix. > + */ > + if (of_device_is_compatible(np, "marvell,mv78230-i2c")) { > + drv_data->errata_delay = true; > + } You can get rid of the {}'s > + > + if (of_device_is_compatible(np, "marvell,mv78230-a0-i2c")) { > + drv_data->errata_delay = true; > + } > + > + if (of_device_is_compatible(np, "allwinner,sun6i-a31-i2c")) > + drv_data->irq_clear_inverted = true; > + > +out: > + return rc; > +} > + > +static int > +mv64xxx_i2c_probe(struct device_d *pd) > +{ > + struct mv64xxx_i2c_data *drv_data; > + int rc; > + > + if (!pd->device_node) > + return -ENODEV; > + > + drv_data = xzalloc(sizeof(*drv_data)); > + > + drv_data->reg_base = dev_request_mem_region(pd, 0); > + if (IS_ERR(drv_data->reg_base)) > + return PTR_ERR(drv_data->reg_base); > + > + drv_data->clk = clk_get(pd, NULL); > + if (IS_ERR(drv_data->clk)) > + return PTR_ERR(drv_data->clk); > + > + clk_enable(drv_data->clk); > + > + rc = mv64xxx_of_config(drv_data, pd); > + if (rc) > + goto exit_clk; > + > + drv_data->adapter.master_xfer = mv64xxx_i2c_xfer; > + drv_data->adapter.dev.parent = pd; > + drv_data->adapter.nr = pd->id; > + drv_data->adapter.dev.device_node = pd->device_node; > + > + mv64xxx_i2c_hw_init(drv_data); > + > + rc = i2c_add_numbered_adapter(&drv_data->adapter); > + if (rc) { > + dev_err(pd, "Failed to add I2C adapter\n"); > + goto exit_clk; > + } > + > + return 0; > + > +exit_clk: > + clk_disable(drv_data->clk); > + > + return rc; > +} > + > +static struct driver_d mv64xxx_i2c_driver = { > + .probe = mv64xxx_i2c_probe, > + .name = "mv64xxx_i2c", > + .of_compatible = DRV_OF_COMPAT(mv64xxx_i2c_of_match_table), > +}; > +device_platform_driver(mv64xxx_i2c_driver); > In general, I agree that picking the driver from Linux is a good idea because it is tested already. But IMHO there is often a huge amount of unnecessary abstraction included that should be considered again for a bootloader driver. I haven't looked at any detail of the FSM in this driver, but maybe it is a victim for cleanup? Sebastian _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: add Marvell 64xxx driver 2014-07-22 8:51 ` Sebastian Hesselbarth @ 2014-07-22 9:57 ` Antony Pavlov 2014-07-22 11:05 ` Sebastian Hesselbarth 2014-07-22 16:21 ` Antony Pavlov 1 sibling, 1 reply; 10+ messages in thread From: Antony Pavlov @ 2014-07-22 9:57 UTC (permalink / raw) To: Sebastian Hesselbarth; +Cc: barebox On Tue, 22 Jul 2014 10:51:10 +0200 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > On 07/16/2014 11:25 PM, Antony Pavlov wrote: > > This driver is also used for Allwinner SoCs I2C controllers. > > > > Ported from linux-3.15. > > > > The most notable barebox driver version changes: > > > > * drop message offloading support; > > * add reg-io-width parameter to use driver with byte-oriented > > controller versions. > > > > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> > > Antony, > > I finally finished work on xHCI and PCI on Armada 370. Now I come > back with the promised review of the i2c driver. > > I gave this driver a quick test on Mirabox, i2c_probe just gives I2C bus > errors. What SoC did you test the driver on? I test it on custom FPGA-based byte-oriented mv64xxx-style i2c controller. Linux 3.15 driver succesfully works with my controller. The only change is adding mv64xxx_write/mv64xxx_read functions for enabling byte registers access. Have you probed your Mirabox i2c bus under linux? > I'll now have a closer look at why it fails, but I already have some > comments below. > > > --- > > drivers/i2c/busses/Kconfig | 8 + > > drivers/i2c/busses/Makefile | 1 + > > drivers/i2c/busses/i2c-mv64xxx.c | 687 +++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 696 insertions(+) > > > [...] > > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile > > index 9823d1b..4e4f6ba 100644 > > --- a/drivers/i2c/busses/Makefile > > +++ b/drivers/i2c/busses/Makefile > > @@ -1,5 +1,6 @@ > > obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o > > obj-$(CONFIG_I2C_IMX) += i2c-imx.o > > +obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o > > obj-$(CONFIG_I2C_OMAP) += i2c-omap.o > > IMHO, you can also fixup the indention of i2c-imx.o and i2c-omap > while you are at it. I prefere using separate patch for fixing indentation. > > obj-$(CONFIG_I2C_TEGRA) += i2c-tegra.o > > obj-$(CONFIG_I2C_VERSATILE) += i2c-versatile.o > > diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c > > new file mode 100644 > > index 0000000..324796a > > --- /dev/null > > +++ b/drivers/i2c/busses/i2c-mv64xxx.c > > @@ -0,0 +1,687 @@ > > +/* > > + * Driver for the i2c controller on the Marvell line of host bridges > > + * (e.g, gt642[46]0, mv643[46]0, mv644[46]0, and Orion SoC family). > > From above list, it is more than unlikely that we will see support for > any of the mv643foo devices. How about to rename the driver to > i2c-orion, get rid of mv643foo, and add Allwinner SoCs to the list > above? I want to keep linux compatibility. This drivers has i2c-mv64xxx.c name in linux kernel! > > + * > > + * This code was ported from linux-3.15 kernel by Antony Pavlov. > > + * > > + * Author: Mark A. Greer <mgreer@mvista.com> > > + * > > + * 2005 (c) MontaVista, Software, Inc. This file is licensed under > > + * the terms of the GNU General Public License version 2. This program > > + * is licensed "as is" without any warranty of any kind, whether express > > + * or implied. > > + */ > > +#include <common.h> > > +#include <driver.h> > > +#include <init.h> > > +#include <of.h> > > +#include <malloc.h> > > +#include <types.h> > > +#include <xfuncs.h> > > +#include <clock.h> > > +#include <linux/clk.h> > > +#include <linux/err.h> > > + > > +#include <io.h> > > +#include <i2c/i2c.h> > > +#include <printk.h> > > + > > +#define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1) > > +#define MV64XXX_I2C_BAUD_DIV_N(val) (val & 0x7) > > +#define MV64XXX_I2C_BAUD_DIV_M(val) ((val & 0xf) << 3) > > + > > +#define MV64XXX_I2C_REG_CONTROL_ACK 0x00000004 > > +#define MV64XXX_I2C_REG_CONTROL_IFLG 0x00000008 > > +#define MV64XXX_I2C_REG_CONTROL_STOP 0x00000010 > > +#define MV64XXX_I2C_REG_CONTROL_START 0x00000020 > > +#define MV64XXX_I2C_REG_CONTROL_TWSIEN 0x00000040 > > +#define MV64XXX_I2C_REG_CONTROL_INTEN 0x00000080 > > As I said before, I see no point in tabs between #define and > MV64XXX_FOO. It is not about the 80 column rule, but general > style instead. > > Also, you can use BIT(x) for above defines. These are constants from linux kernel. I have only kept linux driver formatting. > > + > > +/* Ctlr status values */ > > +#define MV64XXX_I2C_STATUS_BUS_ERR 0x00 > > +#define MV64XXX_I2C_STATUS_MAST_START 0x08 > > +#define MV64XXX_I2C_STATUS_MAST_REPEAT_START 0x10 > > +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK 0x18 > > +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK 0x20 > > +#define MV64XXX_I2C_STATUS_MAST_WR_ACK 0x28 > > +#define MV64XXX_I2C_STATUS_MAST_WR_NO_ACK 0x30 > > +#define MV64XXX_I2C_STATUS_MAST_LOST_ARB 0x38 > > +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK 0x40 > > +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK 0x48 > > +#define MV64XXX_I2C_STATUS_MAST_RD_DATA_ACK 0x50 > > +#define MV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK 0x58 > > +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK 0xd0 > > +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_NO_ACK 0xd8 > > +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK 0xe0 > > +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK 0xe8 > > +#define MV64XXX_I2C_STATUS_NO_STATUS 0xf8 > > + > > +/* Driver states */ > > +enum { > > enum mv64xxx_state { > > and get rid of MV64XXX_I2C_ prefix below. > > > + MV64XXX_I2C_STATE_INVALID, > > + MV64XXX_I2C_STATE_IDLE, > > + MV64XXX_I2C_STATE_WAITING_FOR_START_COND, > > + MV64XXX_I2C_STATE_WAITING_FOR_RESTART, > > + MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK, > > + MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK, > > + MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK, > > + MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA, > > +}; > > + > > +/* Driver actions */ > > +enum { > > enum mv64xxx_action { > > and get rid of MV64XXX_I2C_ prefix below. > > > + MV64XXX_I2C_ACTION_INVALID, > > + MV64XXX_I2C_ACTION_CONTINUE, > > + MV64XXX_I2C_ACTION_SEND_RESTART, > > + MV64XXX_I2C_ACTION_OFFLOAD_RESTART, > > + MV64XXX_I2C_ACTION_SEND_ADDR_1, > > + MV64XXX_I2C_ACTION_SEND_ADDR_2, > > + MV64XXX_I2C_ACTION_SEND_DATA, > > + MV64XXX_I2C_ACTION_RCV_DATA, > > + MV64XXX_I2C_ACTION_RCV_DATA_STOP, > > + MV64XXX_I2C_ACTION_SEND_STOP, > > + MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP, > > +}; > > + > > +struct mv64xxx_i2c_regs { > > + u8 addr; > > + u8 ext_addr; > > + u8 data; > > + u8 control; > > + u8 status; > > + u8 clock; > > + u8 soft_reset; > > +}; > > + > > +struct mv64xxx_i2c_data { > > + struct i2c_msg *msgs; > > + int num_msgs; > > + u32 state; > > + u32 action; > > enum mv64xxx_state state; > enum mv64xxx_action action; Good idea! > > + u32 aborting; > > You are never aborting, so get rid of it and the logic completely. You are right, drv_data->aborting is always == 0. > > + u32 cntl_bits; > > u8 cntl_bits; > > Although Orion SoCs allow 32b access, the registers are always > 8b. That is why FPGA-based i2c-controller that I work with byte oriented register access is used! > > + void __iomem *reg_base; > > If you struggle with long lines, '*regs' or '*base' is sufficient. > > > + struct mv64xxx_i2c_regs reg_offsets; > > ditto, just choose shorter names. Linux kernel driver use this names. I prefer to use them too. > > + u32 addr1; > > + u32 addr2; > > If you want to keep the state machine and track all msg stuff, > u8 is enough for both addrN above. > > > + u32 bytes_left; > > + u32 byte_posn; > > ditto, IIRC i2c doesn't even allow you to send more than 255 bytes > per message nor will you ever find a device that supports it. > > > + u32 send_stop; > > I wonder if you'll ever send a restart at all, that will leave > the stop to the last message transferred. In any way, above should > be bool. > > > + u32 block; > > Whatever block is for, remember that you will send each byte > individually and you'll ever be in charge of the code, i.e. > if you wait for completion, barebox will wait for it. > There is no threading nor interrupts. > > > + int rc; > > + u32 freq_m; > > + u32 freq_n; > > You could just init the HW when you found the best dividers. > No need to store them for eternity. > > > + struct clk *clk; > > + struct i2c_msg *msg; > > + struct i2c_adapter adapter; > > +/* 5us delay in order to avoid repeated start timing violation */ > > + bool errata_delay; > > + struct reset_control *rstc; > > Unused. > > > + bool irq_clear_inverted; > > + int reg_io_width; > > +}; > > + > > +static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = { > > __maybe_unused and see below at compatibles table. > > > + .addr = 0x00, > > + .ext_addr = 0x10, > > + .data = 0x04, > > + .control = 0x08, > > + .status = 0x0c, > > + .clock = 0x0c, > > + .soft_reset = 0x1c, > > +}; > > + > > +static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_sun4i = { > > ditto. Agree. Also I think I can convert u32 to u8 as you have noted above. > > + .addr = 0x00, > > + .ext_addr = 0x04, > > + .data = 0x08, > > + .control = 0x0c, > > + .status = 0x10, > > + .clock = 0x14, > > + .soft_reset = 0x18, > > +}; > > + > > +static inline void mv64xxx_write(struct mv64xxx_i2c_data *drv_data, u32 v, int reg) > > How about having a callback for this and _read below? > > You'd install the callback in _probe() and get rid of reg_io_width > check for every read/write access, i.e > > static void mv64xxx_writel(...) > { > writel(v, addr); > } > > static void mv64xxx_writeb(...) > { > writeb(v, addr); > } > > static int mv64xxx_i2c_probe(...) > { > ... > > switch (reg_io_width) { > case 1: > drv_data->read = mv64xxx_readb; > drv_data->write = mv64xxx_writeb; > break; > case 4: > drv_data->read = mv64xxx_readl; > drv_data->write = mv64xxx_writel; > break; > default: > dev_err(pd, "unsupported reg-io-width (%d)\n", > reg_io_width); > rc = -EINVAL; > goto out; > } > > ... > } > > > +{ > > + void *addr = drv_data->reg_base + reg; > > + > > + switch (drv_data->reg_io_width) { > > + case IORESOURCE_MEM_8BIT: > > + writeb((u8)v, addr); > > + break; > > + case IORESOURCE_MEM_32BIT: > > + writel(v, addr); > > + break; > > + default: > > + dev_err(&drv_data->adapter.dev, > > + "%s: wrong reg_io_width!\n", __func__); > > Beside the comment above, you already made sure reg_io_width will > never be anything else than 8BIT or 32BIT. So the default case is > not needed at all. good I can change it to callback. I see a template in drivers/serial/serial_ns16550.c. > > + } > > +} > > + > > +static inline u32 mv64xxx_read(struct mv64xxx_i2c_data *drv_data, int reg) > > +{ > > + void *addr = drv_data->reg_base + reg; > > + u32 r; > > + > > + switch (drv_data->reg_io_width) { > > + case IORESOURCE_MEM_8BIT: > > + r = readb(addr); > > + break; > > + > > + case IORESOURCE_MEM_32BIT: > > + r = readl(addr); > > + break; > > + > > + default: > > + dev_err(&drv_data->adapter.dev, > > + "%s: wrong reg_io_width!\n", __func__); > > + r = 0xffffffff; > > + } > > + > > + return r; > > +} > > + > > +static void > > +mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, > > + struct i2c_msg *msg) > > +{ > > + u32 dir = 0; > > + > > + drv_data->cntl_bits = MV64XXX_I2C_REG_CONTROL_ACK | > > + MV64XXX_I2C_REG_CONTROL_INTEN | MV64XXX_I2C_REG_CONTROL_TWSIEN; > > + > > + if (msg->flags & I2C_M_RD) > > + dir = 1; > > + > > + if (msg->flags & I2C_M_TEN) { > > + drv_data->addr1 = 0xf0 | (((u32)msg->addr & 0x300) >> 7) | dir; > > + drv_data->addr2 = (u32)msg->addr & 0xff; > > + } else { > > + drv_data->addr1 = MV64XXX_I2C_ADDR_ADDR((u32)msg->addr) | dir; > > + drv_data->addr2 = 0; > > + } > > +} > > + > > +/* > > + ***************************************************************************** > > + * > > + * Finite State Machine & Interrupt Routines > > + * > > + ***************************************************************************** > > + */ > > + > > +/* Reset hardware and initialize FSM */ > > +static void > > +mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data) > > +{ > > + mv64xxx_write(drv_data, 0, drv_data->reg_offsets.soft_reset); > > + mv64xxx_write(drv_data, MV64XXX_I2C_BAUD_DIV_M(drv_data->freq_m) | MV64XXX_I2C_BAUD_DIV_N(drv_data->freq_n), > > + drv_data->reg_offsets.clock); > > + mv64xxx_write(drv_data, 0, drv_data->reg_offsets.addr); > > + mv64xxx_write(drv_data, 0, drv_data->reg_offsets.ext_addr); > > + mv64xxx_write(drv_data, MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP, > > + drv_data->reg_offsets.control); > > + drv_data->state = MV64XXX_I2C_STATE_IDLE; > > +} > > + > > +static void > > +mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status) > > +{ > > + /* > > + * If state is idle, then this is likely the remnants of an old > > + * operation that driver has given up on or the user has killed. > > + * If so, issue the stop condition and go to idle. > > + */ > > + if (drv_data->state == MV64XXX_I2C_STATE_IDLE) { > > + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; > > + return; > > + } > > + > > + /* The status from the ctlr [mostly] tells us what to do next */ > > + switch (status) { > > + /* Start condition interrupt */ > > + case MV64XXX_I2C_STATUS_MAST_START: /* 0x08 */ > > + case MV64XXX_I2C_STATUS_MAST_REPEAT_START: /* 0x10 */ > > + drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_1; > > + drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK; > > + break; > > + > > + /* Performing a write */ > > + case MV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK: /* 0x18 */ > > + if (drv_data->msg->flags & I2C_M_TEN) { > > + drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_2; > > + drv_data->state = > > + MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK; > > + break; > > + } > > + /* FALLTHRU */ > > + case MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK: /* 0xd0 */ > > + case MV64XXX_I2C_STATUS_MAST_WR_ACK: /* 0x28 */ > > + if ((drv_data->bytes_left == 0) > > + || (drv_data->aborting > > + && (drv_data->byte_posn != 0))) { > > + if (drv_data->send_stop || drv_data->aborting) { > > + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; > > + drv_data->state = MV64XXX_I2C_STATE_IDLE; > > + } else { > > + drv_data->action = > > + MV64XXX_I2C_ACTION_SEND_RESTART; > > + drv_data->state = > > + MV64XXX_I2C_STATE_WAITING_FOR_RESTART; > > + } > > + } else { > > + drv_data->action = MV64XXX_I2C_ACTION_SEND_DATA; > > + drv_data->state = > > + MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK; > > + drv_data->bytes_left--; > > + } > > + break; > > + > > + /* Performing a read */ > > + case MV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK: /* 40 */ > > + if (drv_data->msg->flags & I2C_M_TEN) { > > + drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_2; > > + drv_data->state = > > + MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK; > > + break; > > + } > > + /* FALLTHRU */ > > + case MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK: /* 0xe0 */ > > + if (drv_data->bytes_left == 0) { > > + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; > > + drv_data->state = MV64XXX_I2C_STATE_IDLE; > > + break; > > + } > > + /* FALLTHRU */ > > + case MV64XXX_I2C_STATUS_MAST_RD_DATA_ACK: /* 0x50 */ > > + if (status != MV64XXX_I2C_STATUS_MAST_RD_DATA_ACK) > > + drv_data->action = MV64XXX_I2C_ACTION_CONTINUE; > > + else { > > + drv_data->action = MV64XXX_I2C_ACTION_RCV_DATA; > > + drv_data->bytes_left--; > > + } > > + drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA; > > + > > + if ((drv_data->bytes_left == 1) || drv_data->aborting) > > + drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_ACK; > > + break; > > + > > + case MV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK: /* 0x58 */ > > + drv_data->action = MV64XXX_I2C_ACTION_RCV_DATA_STOP; > > + drv_data->state = MV64XXX_I2C_STATE_IDLE; > > + break; > > + > > + case MV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK: /* 0x20 */ > > + case MV64XXX_I2C_STATUS_MAST_WR_NO_ACK: /* 30 */ > > + case MV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK: /* 48 */ > > + /* Doesn't seem to be a device at other end */ > > + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; > > + drv_data->state = MV64XXX_I2C_STATE_IDLE; > > + drv_data->rc = -ENXIO; > > + break; > > + > > + default: > > + dev_err(&drv_data->adapter.dev, > > + "mv64xxx_i2c_fsm: Ctlr Error -- state: 0x%x, " > > + "status: 0x%x, addr: 0x%x, flags: 0x%x\n", > > + drv_data->state, status, drv_data->msg->addr, > > + drv_data->msg->flags); > > + drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP; > > + mv64xxx_i2c_hw_init(drv_data); > > + drv_data->rc = -EIO; > > + } > > +} > > + > > +static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data) > > +{ > > + drv_data->msg = drv_data->msgs; > > + drv_data->byte_posn = 0; > > + drv_data->bytes_left = drv_data->msg->len; > > + drv_data->aborting = 0; > > + drv_data->rc = 0; > > + > > + mv64xxx_i2c_prepare_for_io(drv_data, drv_data->msgs); > > + mv64xxx_write(drv_data, drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START, > > + drv_data->reg_offsets.control); > > +} > > + > > +static void > > +mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) > > +{ > > + switch (drv_data->action) { > > + case MV64XXX_I2C_ACTION_SEND_RESTART: > > + /* We should only get here if we have further messages */ > > + BUG_ON(drv_data->num_msgs == 0); > > + > > + drv_data->msgs++; > > + drv_data->num_msgs--; > > + mv64xxx_i2c_send_start(drv_data); > > + > > + if (drv_data->errata_delay) > > + udelay(5); > > + > > + /* > > + * We're never at the start of the message here, and by this > > + * time it's already too late to do any protocol mangling. > > + * Thankfully, do not advertise support for that feature. > > + */ > > + drv_data->send_stop = drv_data->num_msgs == 1; > > + break; > > + > > + case MV64XXX_I2C_ACTION_CONTINUE: > > + mv64xxx_write(drv_data, drv_data->cntl_bits, > > + drv_data->reg_offsets.control); > > + break; > > + > > + case MV64XXX_I2C_ACTION_SEND_ADDR_1: > > + mv64xxx_write(drv_data, drv_data->addr1, > > + drv_data->reg_offsets.data); > > + mv64xxx_write(drv_data, drv_data->cntl_bits, > > + drv_data->reg_offsets.control); > > + break; > > + > > + case MV64XXX_I2C_ACTION_SEND_ADDR_2: > > + mv64xxx_write(drv_data, drv_data->addr2, > > + drv_data->reg_offsets.data); > > + mv64xxx_write(drv_data, drv_data->cntl_bits, > > + drv_data->reg_offsets.control); > > + break; > > + > > + case MV64XXX_I2C_ACTION_SEND_DATA: > > + mv64xxx_write(drv_data, drv_data->msg->buf[drv_data->byte_posn++], > > + drv_data->reg_offsets.data); > > + mv64xxx_write(drv_data, drv_data->cntl_bits, > > + drv_data->reg_offsets.control); > > + break; > > + > > + case MV64XXX_I2C_ACTION_RCV_DATA: > > + drv_data->msg->buf[drv_data->byte_posn++] = > > + mv64xxx_read(drv_data, drv_data->reg_offsets.data); > > + mv64xxx_write(drv_data, drv_data->cntl_bits, > > + drv_data->reg_offsets.control); > > + break; > > + > > + case MV64XXX_I2C_ACTION_RCV_DATA_STOP: > > + drv_data->msg->buf[drv_data->byte_posn++] = > > + mv64xxx_read(drv_data, drv_data->reg_offsets.data); > > + drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; > > + mv64xxx_write(drv_data, drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, > > + drv_data->reg_offsets.control); > > + drv_data->block = 0; > > + if (drv_data->errata_delay) > > + udelay(5); > > + > > + break; > > + > > + case MV64XXX_I2C_ACTION_INVALID: > > + default: > > + dev_err(&drv_data->adapter.dev, > > + "mv64xxx_i2c_do_action: Invalid action: %d\n", > > + drv_data->action); > > + drv_data->rc = -EIO; > > + > > + /* FALLTHRU */ > > + case MV64XXX_I2C_ACTION_SEND_STOP: > > + drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; > > + mv64xxx_write(drv_data, drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, > > + drv_data->reg_offsets.control); > > + drv_data->block = 0; > > + break; > > + } > > +} > > + > > +static void mv64xxx_i2c_intr(struct mv64xxx_i2c_data *drv_data) > > Is "intr" for interrupt? Barebox is running with irqs disabled, so > maybe rename it to something like "mv64xxx_i2c_wait_for_done" ? This functions makes the work of the driver interrupt handler, so I prefere to keep linux kernel compartible naming here. > > +{ > > + u32 status; > > + uint64_t start; > > + > > + start = get_time_ns(); > > + > > + while (mv64xxx_read(drv_data, drv_data->reg_offsets.control) & > > + MV64XXX_I2C_REG_CONTROL_IFLG) { > > + status = mv64xxx_read(drv_data, drv_data->reg_offsets.status); > > + mv64xxx_i2c_fsm(drv_data, status); > > + mv64xxx_i2c_do_action(drv_data); > > + > > + if (drv_data->irq_clear_inverted) > > + mv64xxx_write(drv_data, drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_IFLG, > > + drv_data->reg_offsets.control); > > + > > + if (is_timeout_non_interruptible(start, 3 * SECOND)) { > > + drv_data->rc = -EIO; > > + break; > > + } > > + } > > +} > > + > > +/* > > + ***************************************************************************** > > + * > > + * I2C Msg Execution Routines > > + * > > + ***************************************************************************** > > + */ > > +static void > > +mv64xxx_i2c_wait_for_completion(struct mv64xxx_i2c_data *drv_data) > > +{ > > + do { > > + mv64xxx_i2c_intr(drv_data); > > + if (drv_data->rc) { > > + drv_data->state = MV64XXX_I2C_STATE_IDLE; > > + dev_err(&drv_data->adapter.dev, "I2C bus error\n"); > > + mv64xxx_i2c_hw_init(drv_data); > > + drv_data->block = 0; > > + } > > + } while (drv_data->block != 0); > > +} > > + > > +static int > > +mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg, > > + int is_last) > > +{ > > + drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND; > > + > > + drv_data->send_stop = is_last; > > + drv_data->block = 1; > > + mv64xxx_i2c_send_start(drv_data); > > + mv64xxx_i2c_wait_for_completion(drv_data); > > + > > + return drv_data->rc; > > +} > > + > > +/* > > + ***************************************************************************** > > + * > > + * I2C Core Support Routines (Interface to higher level I2C code) > > + * > > + ***************************************************************************** > > + */ > > +static int > > +mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > > +{ > > + struct mv64xxx_i2c_data *drv_data = container_of(adap, struct mv64xxx_i2c_data, adapter); > > + int rc, ret = num; > > + > > + BUG_ON(drv_data->msgs != NULL); > > + drv_data->msgs = msgs; > > + drv_data->num_msgs = num; > > + > > + rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[0], num == 1); > > + if (rc < 0) > > + ret = rc; > > + > > + drv_data->num_msgs = 0; > > + drv_data->msgs = NULL; > > How about you loop over all passed msgs in here and get rid of > the both drv_data variables completely? I don't want to change code from linux driver without very good reason. > > > + > > + return ret; > > +} > > + > > +/* > > + ***************************************************************************** > > + * > > + * Driver Interface & Early Init Routines > > + * > > + ***************************************************************************** > > + */ > > +static struct of_device_id mv64xxx_i2c_of_match_table[] = { > #if defined(CONFIG_SUN4I_FOO) > > + { .compatible = "allwinner,sun4i-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_sun4i}, > #endif > #if defined(CONFIG_SUN6I_FOO) > > + { .compatible = "allwinner,sun6i-a31-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_sun4i}, > #endif > #if defined(CONFIG_MACH_MVEBU) > > + { .compatible = "marvell,mv64xxx-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_mv64xxx}, > #endif > #if defined(CONFIG_ARCH_ARMADA_XP) > > + { .compatible = "marvell,mv78230-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_mv64xxx}, > > + { .compatible = "marvell,mv78230-a0-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_mv64xxx}, > #endif > > + {} > > +}; > > If you ifdef the compatibles, the compiler will be able to remove all > structs that are not referenced. > > > + > > +static int > > +mv64xxx_calc_freq(const int tclk, const int n, const int m) > > inline? Yes! > > +{ > > + return tclk / (10 * (m + 1) * (2 << n)); > > +} > > + > > +static bool > > +mv64xxx_find_baud_factors(const u32 req_freq, const u32 tclk, u32 *best_n, > > + u32 *best_m) > > +{ > > + int freq, delta, best_delta = INT_MAX; > > + int m, n; > > + > > + for (n = 0; n <= 7; n++) > > + for (m = 0; m <= 15; m++) { > > + freq = mv64xxx_calc_freq(tclk, n, m); > > + delta = req_freq - freq; > > + if (delta >= 0 && delta < best_delta) { > > + *best_m = m; > > + *best_n = n; > > + best_delta = delta; > > + } > > + if (best_delta == 0) > > + return true; > > + } > > + if (best_delta == INT_MAX) > > + return false; > > + return true; > > +} > > + > > +static int > > +mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, > > + struct device_d *pd) > > +{ > > + struct device_node *np = pd->device_node; > > + u32 bus_freq, tclk; > > + int rc = 0; > > + u32 prop; > > + struct mv64xxx_i2c_regs *mv64xxx_regs; > > + int freq; > > + > > + if (IS_ERR(drv_data->clk)) { > > + rc = -ENODEV; > > + goto out; > > + } > > + tclk = clk_get_rate(drv_data->clk); > > + > > + rc = of_property_read_u32(np, "clock-frequency", &bus_freq); > > + if (rc) > > + bus_freq = 100000; /* 100kHz by default */ > > + > > + if (!mv64xxx_find_baud_factors(bus_freq, tclk, > > + &drv_data->freq_n, &drv_data->freq_m)) { > > + rc = -EINVAL; > > + goto out; > > + } > > + > > + freq = mv64xxx_calc_freq(tclk, drv_data->freq_n, drv_data->freq_m); > > + dev_dbg(pd, "tclk=%d freq_n=%d freq_m=%d freq=%d\n", > > + tclk, drv_data->freq_n, drv_data->freq_m, freq); > > + > > + drv_data->reg_io_width = IORESOURCE_MEM_32BIT; > > + > > + if (of_property_read_u32(np, "reg-io-width", &prop) == 0) { > > + switch (prop) { > > + case 1: > > + drv_data->reg_io_width = IORESOURCE_MEM_8BIT; > > + break; > > + case 4: > > + drv_data->reg_io_width = IORESOURCE_MEM_32BIT; > > + break; > > + default: > > + dev_err(pd, "unsupported reg-io-width (%d)\n", prop); > > + rc = -EINVAL; > > + goto out; > > + } > > + } > > + > > + dev_get_drvdata(pd, (unsigned long *)&mv64xxx_regs); > > + memcpy(&drv_data->reg_offsets, mv64xxx_regs, > > + sizeof(drv_data->reg_offsets)); > > + > > + /* > > + * For controllers embedded in new SoCs activate the > > + * Transaction Generator support and the errata fix. > > + */ > > + if (of_device_is_compatible(np, "marvell,mv78230-i2c")) { > > + drv_data->errata_delay = true; > > + } > > You can get rid of the {}'s > > > + > > + if (of_device_is_compatible(np, "marvell,mv78230-a0-i2c")) { > > + drv_data->errata_delay = true; > > + } > > + > > + if (of_device_is_compatible(np, "allwinner,sun6i-a31-i2c")) > > + drv_data->irq_clear_inverted = true; > > + > > +out: > > + return rc; > > +} > > + > > +static int > > +mv64xxx_i2c_probe(struct device_d *pd) > > +{ > > + struct mv64xxx_i2c_data *drv_data; > > + int rc; > > + > > + if (!pd->device_node) > > + return -ENODEV; > > + > > + drv_data = xzalloc(sizeof(*drv_data)); > > + > > + drv_data->reg_base = dev_request_mem_region(pd, 0); > > + if (IS_ERR(drv_data->reg_base)) > > + return PTR_ERR(drv_data->reg_base); > > + > > + drv_data->clk = clk_get(pd, NULL); > > + if (IS_ERR(drv_data->clk)) > > + return PTR_ERR(drv_data->clk); > > + > > + clk_enable(drv_data->clk); > > + > > + rc = mv64xxx_of_config(drv_data, pd); > > + if (rc) > > + goto exit_clk; > > + > > + drv_data->adapter.master_xfer = mv64xxx_i2c_xfer; > > + drv_data->adapter.dev.parent = pd; > > + drv_data->adapter.nr = pd->id; > > + drv_data->adapter.dev.device_node = pd->device_node; > > + > > + mv64xxx_i2c_hw_init(drv_data); > > + > > + rc = i2c_add_numbered_adapter(&drv_data->adapter); > > + if (rc) { > > + dev_err(pd, "Failed to add I2C adapter\n"); > > + goto exit_clk; > > + } > > + > > + return 0; > > + > > +exit_clk: > > + clk_disable(drv_data->clk); > > + > > + return rc; > > +} > > + > > +static struct driver_d mv64xxx_i2c_driver = { > > + .probe = mv64xxx_i2c_probe, > > + .name = "mv64xxx_i2c", > > + .of_compatible = DRV_OF_COMPAT(mv64xxx_i2c_of_match_table), > > +}; > > +device_platform_driver(mv64xxx_i2c_driver); > > > > In general, I agree that picking the driver from Linux is a good idea > because it is tested already. But IMHO there is often a huge amount of > unnecessary abstraction included that should be considered again for a > bootloader driver. > > I haven't looked at any detail of the FSM in this driver, but maybe it > is a victim for cleanup? I'll try to fix most of your issues. I think that it is better to use the same driver in linux and barebox. There is no reason to fix barebox driver without fixing original linux driver. -- Best regards, Antony Pavlov _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: add Marvell 64xxx driver 2014-07-22 9:57 ` Antony Pavlov @ 2014-07-22 11:05 ` Sebastian Hesselbarth 2014-07-22 11:58 ` Antony Pavlov 0 siblings, 1 reply; 10+ messages in thread From: Sebastian Hesselbarth @ 2014-07-22 11:05 UTC (permalink / raw) To: Antony Pavlov; +Cc: barebox On 07/22/2014 11:57 AM, Antony Pavlov wrote: > On Tue, 22 Jul 2014 10:51:10 +0200 > Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: >> On 07/16/2014 11:25 PM, Antony Pavlov wrote: >>> This driver is also used for Allwinner SoCs I2C controllers. >>> >>> Ported from linux-3.15. >>> >>> The most notable barebox driver version changes: >>> >>> * drop message offloading support; >>> * add reg-io-width parameter to use driver with byte-oriented >>> controller versions. >>> >>> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> >> >> Antony, >> >> I finally finished work on xHCI and PCI on Armada 370. Now I come >> back with the promised review of the i2c driver. >> >> I gave this driver a quick test on Mirabox, i2c_probe just gives I2C bus >> errors. What SoC did you test the driver on? > > I test it on custom FPGA-based byte-oriented mv64xxx-style i2c controller. > > Linux 3.15 driver succesfully works with my controller. The only change is adding > mv64xxx_write/mv64xxx_read functions for enabling byte registers access. > > Have you probed your Mirabox i2c bus under linux? Actually, I switched to Dove CuBox after I realized that Mirabox has nothing on i2c on-board but only externally connected ;) Now you can add my Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> But I still have some comments.. >> [...] >>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile >>> index 9823d1b..4e4f6ba 100644 >>> --- a/drivers/i2c/busses/Makefile >>> +++ b/drivers/i2c/busses/Makefile >>> @@ -1,5 +1,6 @@ >>> obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o >>> obj-$(CONFIG_I2C_IMX) += i2c-imx.o >>> +obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o >>> obj-$(CONFIG_I2C_OMAP) += i2c-omap.o >> >> IMHO, you can also fixup the indention of i2c-imx.o and i2c-omap >> while you are at it. > > I prefere using separate patch for fixing indentation. Fine with me. >>> obj-$(CONFIG_I2C_TEGRA) += i2c-tegra.o >>> obj-$(CONFIG_I2C_VERSATILE) += i2c-versatile.o >>> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c >>> new file mode 100644 >>> index 0000000..324796a >>> --- /dev/null >>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c >>> @@ -0,0 +1,687 @@ >>> +/* >>> + * Driver for the i2c controller on the Marvell line of host bridges >>> + * (e.g, gt642[46]0, mv643[46]0, mv644[46]0, and Orion SoC family). >> >> From above list, it is more than unlikely that we will see support for >> any of the mv643foo devices. How about to rename the driver to >> i2c-orion, get rid of mv643foo, and add Allwinner SoCs to the list >> above? > > I want to keep linux compatibility. This drivers has i2c-mv64xxx.c name in linux kernel! Hmm, the driver name has nothing to do with linux compatibility. But ok, it is just a name. [...] >>> +#define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1) >>> +#define MV64XXX_I2C_BAUD_DIV_N(val) (val & 0x7) >>> +#define MV64XXX_I2C_BAUD_DIV_M(val) ((val & 0xf) << 3) >>> + >>> +#define MV64XXX_I2C_REG_CONTROL_ACK 0x00000004 >>> +#define MV64XXX_I2C_REG_CONTROL_IFLG 0x00000008 >>> +#define MV64XXX_I2C_REG_CONTROL_STOP 0x00000010 >>> +#define MV64XXX_I2C_REG_CONTROL_START 0x00000020 >>> +#define MV64XXX_I2C_REG_CONTROL_TWSIEN 0x00000040 >>> +#define MV64XXX_I2C_REG_CONTROL_INTEN 0x00000080 >> >> As I said before, I see no point in tabs between #define and >> MV64XXX_FOO. It is not about the 80 column rule, but general >> style instead. >> >> Also, you can use BIT(x) for above defines. > > These are constants from linux kernel. I have only kept linux driver formatting. Yeah, I got it that it isn't your fault. I'd suggest to rework them anyway ;) Also, you can get rid of MV64XXX_I2C_ prefix for the above defines, too. They are local to this file and should not collide with anything you include. IMHO it makes code much more readable if the defines aren't always prefixed. >>> +/* Ctlr status values */ >>> +#define MV64XXX_I2C_STATUS_BUS_ERR 0x00 >>> +#define MV64XXX_I2C_STATUS_MAST_START 0x08 >>> +#define MV64XXX_I2C_STATUS_MAST_REPEAT_START 0x10 >>> +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK 0x18 >>> +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK 0x20 >>> +#define MV64XXX_I2C_STATUS_MAST_WR_ACK 0x28 >>> +#define MV64XXX_I2C_STATUS_MAST_WR_NO_ACK 0x30 >>> +#define MV64XXX_I2C_STATUS_MAST_LOST_ARB 0x38 >>> +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK 0x40 >>> +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK 0x48 >>> +#define MV64XXX_I2C_STATUS_MAST_RD_DATA_ACK 0x50 >>> +#define MV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK 0x58 >>> +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK 0xd0 >>> +#define MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_NO_ACK 0xd8 >>> +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK 0xe0 >>> +#define MV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK 0xe8 >>> +#define MV64XXX_I2C_STATUS_NO_STATUS 0xf8 >>> + >>> +/* Driver states */ >>> +enum { >> >> enum mv64xxx_state { >> >> and get rid of MV64XXX_I2C_ prefix below. >> >>> + MV64XXX_I2C_STATE_INVALID, >>> + MV64XXX_I2C_STATE_IDLE, >>> + MV64XXX_I2C_STATE_WAITING_FOR_START_COND, >>> + MV64XXX_I2C_STATE_WAITING_FOR_RESTART, >>> + MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK, >>> + MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK, >>> + MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK, >>> + MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA, >>> +}; >>> + >>> +/* Driver actions */ >>> +enum { >> >> enum mv64xxx_action { >> >> and get rid of MV64XXX_I2C_ prefix below. >> >>> + MV64XXX_I2C_ACTION_INVALID, >>> + MV64XXX_I2C_ACTION_CONTINUE, >>> + MV64XXX_I2C_ACTION_SEND_RESTART, >>> + MV64XXX_I2C_ACTION_OFFLOAD_RESTART, >>> + MV64XXX_I2C_ACTION_SEND_ADDR_1, >>> + MV64XXX_I2C_ACTION_SEND_ADDR_2, >>> + MV64XXX_I2C_ACTION_SEND_DATA, >>> + MV64XXX_I2C_ACTION_RCV_DATA, >>> + MV64XXX_I2C_ACTION_RCV_DATA_STOP, >>> + MV64XXX_I2C_ACTION_SEND_STOP, >>> + MV64XXX_I2C_ACTION_OFFLOAD_SEND_STOP, >>> +}; >>> + >>> +struct mv64xxx_i2c_regs { >>> + u8 addr; >>> + u8 ext_addr; >>> + u8 data; >>> + u8 control; >>> + u8 status; >>> + u8 clock; >>> + u8 soft_reset; >>> +}; >>> + >>> +struct mv64xxx_i2c_data { >>> + struct i2c_msg *msgs; >>> + int num_msgs; >>> + u32 state; >>> + u32 action; >> >> enum mv64xxx_state state; >> enum mv64xxx_action action; > > Good idea! > >>> + u32 aborting; >> >> You are never aborting, so get rid of it and the logic completely. > > > You are right, drv_data->aborting is always == 0. > >>> + u32 cntl_bits; >> >> u8 cntl_bits; >> >> Although Orion SoCs allow 32b access, the registers are always >> 8b. > > That is why FPGA-based i2c-controller that I work with byte oriented register access is used! Yep, the registers are 8b because the i2c IP isn't even Marvell at all but something ancient. Anyway, no need for u32 above as all registers are 8b despite their access width. >>> + void __iomem *reg_base; >> >> If you struggle with long lines, '*regs' or '*base' is sufficient. >> >>> + struct mv64xxx_i2c_regs reg_offsets; >> >> ditto, just choose shorter names. > > Linux kernel driver use this names. I prefer to use them too. Again, names make no compatibility.. and again you are free to choose. >>> + u32 addr1; >>> + u32 addr2; >> >> If you want to keep the state machine and track all msg stuff, >> u8 is enough for both addrN above. >> >>> + u32 bytes_left; >>> + u32 byte_posn; >> >> ditto, IIRC i2c doesn't even allow you to send more than 255 bytes >> per message nor will you ever find a device that supports it. >> >>> + u32 send_stop; >> >> I wonder if you'll ever send a restart at all, that will leave >> the stop to the last message transferred. In any way, above should >> be bool. >> >>> + u32 block; >> >> Whatever block is for, remember that you will send each byte >> individually and you'll ever be in charge of the code, i.e. >> if you wait for completion, barebox will wait for it. >> There is no threading nor interrupts. >> >>> + int rc; >>> + u32 freq_m; >>> + u32 freq_n; >> >> You could just init the HW when you found the best dividers. >> No need to store them for eternity. >> >>> + struct clk *clk; >>> + struct i2c_msg *msg; >>> + struct i2c_adapter adapter; >>> +/* 5us delay in order to avoid repeated start timing violation */ >>> + bool errata_delay; >>> + struct reset_control *rstc; >> >> Unused. >> >>> + bool irq_clear_inverted; >>> + int reg_io_width; >>> +}; >>> + >>> +static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = { >> >> __maybe_unused and see below at compatibles table. >> >>> + .addr = 0x00, >>> + .ext_addr = 0x10, >>> + .data = 0x04, >>> + .control = 0x08, >>> + .status = 0x0c, >>> + .clock = 0x0c, >>> + .soft_reset = 0x1c, >>> +}; >>> + >>> +static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_sun4i = { >> >> ditto. > > Agree. Also I think I can convert u32 to u8 as you have noted above. Ok. >>> + .addr = 0x00, >>> + .ext_addr = 0x04, >>> + .data = 0x08, >>> + .control = 0x0c, >>> + .status = 0x10, >>> + .clock = 0x14, >>> + .soft_reset = 0x18, >>> +}; >>> + >>> +static inline void mv64xxx_write(struct mv64xxx_i2c_data *drv_data, u32 v, int reg) >> >> How about having a callback for this and _read below? >> >> You'd install the callback in _probe() and get rid of reg_io_width >> check for every read/write access, i.e >> >> static void mv64xxx_writel(...) >> { >> writel(v, addr); >> } >> >> static void mv64xxx_writeb(...) >> { >> writeb(v, addr); >> } >> >> static int mv64xxx_i2c_probe(...) >> { >> ... >> >> switch (reg_io_width) { >> case 1: >> drv_data->read = mv64xxx_readb; >> drv_data->write = mv64xxx_writeb; >> break; >> case 4: >> drv_data->read = mv64xxx_readl; >> drv_data->write = mv64xxx_writel; >> break; >> default: >> dev_err(pd, "unsupported reg-io-width (%d)\n", >> reg_io_width); >> rc = -EINVAL; >> goto out; >> } >> >> ... >> } >> >>> +{ >>> + void *addr = drv_data->reg_base + reg; >>> + >>> + switch (drv_data->reg_io_width) { >>> + case IORESOURCE_MEM_8BIT: >>> + writeb((u8)v, addr); >>> + break; >>> + case IORESOURCE_MEM_32BIT: >>> + writel(v, addr); >>> + break; >>> + default: >>> + dev_err(&drv_data->adapter.dev, >>> + "%s: wrong reg_io_width!\n", __func__); >> >> Beside the comment above, you already made sure reg_io_width will >> never be anything else than 8BIT or 32BIT. So the default case is >> not needed at all. > > good I can change it to callback. I see a template in drivers/serial/serial_ns16550.c. Ok. >>> + } >>> +} [...] >>> +static void mv64xxx_i2c_intr(struct mv64xxx_i2c_data *drv_data) >> >> Is "intr" for interrupt? Barebox is running with irqs disabled, so >> maybe rename it to something like "mv64xxx_i2c_wait_for_done" ? > > This functions makes the work of the driver interrupt handler, so I prefere to keep > linux kernel compartible naming here. Here I have to insist just because there is no interrupt. Don't make the driver look like the Linux driver. We do polling and we always know what response we expect at a certain time, i.e. you issue a byte write and you wait for it to complete. There will be no out-of-order errors or events you have to deal with in an interrupt context. >>> +{ >>> + u32 status; >>> + uint64_t start; >>> + >>> + start = get_time_ns(); >>> + >>> + while (mv64xxx_read(drv_data, drv_data->reg_offsets.control) & >>> + MV64XXX_I2C_REG_CONTROL_IFLG) { >>> + status = mv64xxx_read(drv_data, drv_data->reg_offsets.status); >>> + mv64xxx_i2c_fsm(drv_data, status); >>> + mv64xxx_i2c_do_action(drv_data); >>> + >>> + if (drv_data->irq_clear_inverted) >>> + mv64xxx_write(drv_data, drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_IFLG, >>> + drv_data->reg_offsets.control); >>> + >>> + if (is_timeout_non_interruptible(start, 3 * SECOND)) { >>> + drv_data->rc = -EIO; >>> + break; >>> + } >>> + } >>> +} >>> + >>> +/* >>> + ***************************************************************************** >>> + * >>> + * I2C Msg Execution Routines >>> + * >>> + ***************************************************************************** >>> + */ >>> +static void >>> +mv64xxx_i2c_wait_for_completion(struct mv64xxx_i2c_data *drv_data) >>> +{ >>> + do { >>> + mv64xxx_i2c_intr(drv_data); >>> + if (drv_data->rc) { >>> + drv_data->state = MV64XXX_I2C_STATE_IDLE; >>> + dev_err(&drv_data->adapter.dev, "I2C bus error\n"); This dev_err is killing me. I haven't checked return values but "No device responding at address FOO" is _not_ a bus error. >>> + mv64xxx_i2c_hw_init(drv_data); >>> + drv_data->block = 0; >>> + } >>> + } while (drv_data->block != 0); >>> +} >>> + >>> +static int >>> +mv64xxx_i2c_execute_msg(struct mv64xxx_i2c_data *drv_data, struct i2c_msg *msg, >>> + int is_last) >>> +{ >>> + drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_START_COND; >>> + >>> + drv_data->send_stop = is_last; >>> + drv_data->block = 1; >>> + mv64xxx_i2c_send_start(drv_data); >>> + mv64xxx_i2c_wait_for_completion(drv_data); >>> + >>> + return drv_data->rc; >>> +} >>> + >>> +/* >>> + ***************************************************************************** >>> + * >>> + * I2C Core Support Routines (Interface to higher level I2C code) >>> + * >>> + ***************************************************************************** >>> + */ >>> +static int >>> +mv64xxx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) >>> +{ >>> + struct mv64xxx_i2c_data *drv_data = container_of(adap, struct mv64xxx_i2c_data, adapter); >>> + int rc, ret = num; >>> + >>> + BUG_ON(drv_data->msgs != NULL); >>> + drv_data->msgs = msgs; >>> + drv_data->num_msgs = num; >>> + >>> + rc = mv64xxx_i2c_execute_msg(drv_data, &msgs[0], num == 1); >>> + if (rc < 0) >>> + ret = rc; >>> + >>> + drv_data->num_msgs = 0; >>> + drv_data->msgs = NULL; >> >> How about you loop over all passed msgs in here and get rid of >> the both drv_data variables completely? > > I don't want to change code from linux driver without very good reason. Oh, I guess you want to. Linux carries a whole bunch of software layers that are very unnecessary on Barebox. But if you feel better with it, I can live with it. >>> + >>> + return ret; >>> +} >>> + >>> +/* >>> + ***************************************************************************** >>> + * >>> + * Driver Interface & Early Init Routines >>> + * >>> + ***************************************************************************** >>> + */ >>> +static struct of_device_id mv64xxx_i2c_of_match_table[] = { >> #if defined(CONFIG_SUN4I_FOO) >>> + { .compatible = "allwinner,sun4i-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_sun4i}, >> #endif >> #if defined(CONFIG_SUN6I_FOO) >>> + { .compatible = "allwinner,sun6i-a31-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_sun4i}, >> #endif >> #if defined(CONFIG_MACH_MVEBU) >>> + { .compatible = "marvell,mv64xxx-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_mv64xxx}, >> #endif >> #if defined(CONFIG_ARCH_ARMADA_XP) >>> + { .compatible = "marvell,mv78230-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_mv64xxx}, >>> + { .compatible = "marvell,mv78230-a0-i2c", .data = (unsigned long)&mv64xxx_i2c_regs_mv64xxx}, >> #endif >>> + {} >>> +}; >> >> If you ifdef the compatibles, the compiler will be able to remove all >> structs that are not referenced. >> >>> + >>> +static int >>> +mv64xxx_calc_freq(const int tclk, const int n, const int m) >> >> inline? > > Yes! > [...] >> >> In general, I agree that picking the driver from Linux is a good idea >> because it is tested already. But IMHO there is often a huge amount of >> unnecessary abstraction included that should be considered again for a >> bootloader driver. >> >> I haven't looked at any detail of the FSM in this driver, but maybe it >> is a victim for cleanup? > > I'll try to fix most of your issues. > > I think that it is better to use the same driver in linux and barebox. > There is no reason to fix barebox driver without fixing original linux driver. It depends. You'll notice that there will be fixes/improvements for the Linux driver that will not make its way to the Barebox driver. While API may be somewhat compatible and we are also probing from DT, there is no need to carry interrupt/threading stuff and legacy code with this driver. IMHO, the driver (and its function names) should reflect what is used on barebox, no matter where the driver template came from. Sebastian _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: add Marvell 64xxx driver 2014-07-22 11:05 ` Sebastian Hesselbarth @ 2014-07-22 11:58 ` Antony Pavlov 0 siblings, 0 replies; 10+ messages in thread From: Antony Pavlov @ 2014-07-22 11:58 UTC (permalink / raw) To: Sebastian Hesselbarth; +Cc: barebox On Tue, 22 Jul 2014 13:05:56 +0200 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > On 07/22/2014 11:57 AM, Antony Pavlov wrote: > > On Tue, 22 Jul 2014 10:51:10 +0200 > > Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > >> On 07/16/2014 11:25 PM, Antony Pavlov wrote: > >>> This driver is also used for Allwinner SoCs I2C controllers. > >>> > >>> Ported from linux-3.15. > >>> > >>> The most notable barebox driver version changes: > >>> > >>> * drop message offloading support; > >>> * add reg-io-width parameter to use driver with byte-oriented > >>> controller versions. > >>> > >>> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> > >> > >> Antony, > >> > >> I finally finished work on xHCI and PCI on Armada 370. Now I come > >> back with the promised review of the i2c driver. > >> > >> I gave this driver a quick test on Mirabox, i2c_probe just gives I2C bus > >> errors. What SoC did you test the driver on? > > > > I test it on custom FPGA-based byte-oriented mv64xxx-style i2c controller. > > > > Linux 3.15 driver succesfully works with my controller. The only change is adding > > mv64xxx_write/mv64xxx_read functions for enabling byte registers access. > > > > Have you probed your Mirabox i2c bus under linux? > > Actually, I switched to Dove CuBox after I realized that Mirabox has > nothing on i2c on-board but only externally connected ;) > > Now you can add my > > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> Very good! I'll send PATCH v3 in several days. -- Best regards, Antony Pavlov _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: add Marvell 64xxx driver 2014-07-22 8:51 ` Sebastian Hesselbarth 2014-07-22 9:57 ` Antony Pavlov @ 2014-07-22 16:21 ` Antony Pavlov 2014-07-22 16:53 ` Sebastian Hesselbarth 1 sibling, 1 reply; 10+ messages in thread From: Antony Pavlov @ 2014-07-22 16:21 UTC (permalink / raw) To: Sebastian Hesselbarth; +Cc: barebox On Tue, 22 Jul 2014 10:51:10 +0200 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > On 07/16/2014 11:25 PM, Antony Pavlov wrote: > > This driver is also used for Allwinner SoCs I2C controllers. > > > > Ported from linux-3.15. > > > > The most notable barebox driver version changes: > > > > * drop message offloading support; > > * add reg-io-width parameter to use driver with byte-oriented > > controller versions. > > > > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com> > > Antony, > > I finally finished work on xHCI and PCI on Armada 370. Now I come Are you speaking about PCIe support for Armada 370 in barebox? -- Best regards, Antony Pavlov _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: add Marvell 64xxx driver 2014-07-22 16:21 ` Antony Pavlov @ 2014-07-22 16:53 ` Sebastian Hesselbarth 0 siblings, 0 replies; 10+ messages in thread From: Sebastian Hesselbarth @ 2014-07-22 16:53 UTC (permalink / raw) To: Antony Pavlov; +Cc: barebox On 07/22/2014 06:21 PM, Antony Pavlov wrote: > On Tue, 22 Jul 2014 10:51:10 +0200 > Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: >> I finally finished work on xHCI and PCI on Armada 370. Now I come > > Are you speaking about PCIe support for Armada 370 in barebox? Yes, but not only on Armada 370 but Orion SoCs in general. I am about to clean the PCIe driver now and see if I can give it a go on Dove or Kirkwood easily. This is what I got today with both PCIe driver and xHCI PCI driver: barebox 2014.07.0-00181-g2d9d176ef3c7-dirty #1081 Tue Jul 22 09:20:08 CEST 2014 Board: Globalscale Mirabox xHCI PCI pci0: USB xHCI 0.96 barebox:/ usb USB: scanning bus for devices... Using index 0 for the new disk Bus 001 Device 002: ID 0951:1653 DT 100 G2 Bus 001 Device 001: ID 0000:0000 USB 3.0 Root Hub 2 USB Device(s) found barebox:/ mount -a ext4 ext40: EXT2 rev 1, inode_size 256 none on / type ramfs none on /dev type devfs /dev/disk0.0 on /mnt/disk0.0 type ext4 barebox:/ ls /mnt/disk0.0/ . .. lost+found Sebastian _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-07-22 16:54 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-07-16 21:25 [PATCH v2] i2c: add Marvell 64xxx driver Antony Pavlov 2014-07-16 21:25 ` Antony Pavlov 2014-07-17 9:33 ` Sebastian Hesselbarth 2014-07-17 11:59 ` Antony Pavlov 2014-07-22 8:51 ` Sebastian Hesselbarth 2014-07-22 9:57 ` Antony Pavlov 2014-07-22 11:05 ` Sebastian Hesselbarth 2014-07-22 11:58 ` Antony Pavlov 2014-07-22 16:21 ` Antony Pavlov 2014-07-22 16:53 ` Sebastian Hesselbarth
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox