Hello,
sorry to give feedback only for v6, it seems I missed the earlier
review rounds.
On Mon, Nov 12, 2018 at 02:34:33PM +0800, Zhang Ying-22455 wrote:
> From: Joshua Frkuska <joshua_frkuska@xxxxxxxxxx>
>
> This patch adds hardware supported slave-mode with master arbitration
> via the i2c generic slave interface. This allows master transactions
> to be supported while a slave-mode device is in idle.
>
> To enable this functionality enable i2c slave support.
>
> CONFIG_I2C_SLAVE=y
>
> If i2c-slave support is not enabled in the kernel config, we support
> master-mode only and the slave-mode support is not compiled in.
>
> A queue backed event-driven state machine is implemented in order to
> handle events in the order they occur in the hardware. The states
> for the most part follow the logic charts in the imx reference manual
>
> Signed-off-by: Maxim Syrchin <msyrchin@xxxxxxxxxxxxx>
> [joshua_frkuska@xxxxxxxxxx: Reworked patchset for upstream submission]
> Signed-off-by: Joshua Frkuska <joshua_frkuska@xxxxxxxxxx>
> Signed-off-by: Zhang Ying-22455 <ying.zhang22455@xxxxxxx>
> ---
> ---changed from v5:
> 1. Add STOP signal process in slave receive mode.
> 2. Add CONFIG_PPC_55xx support
> drivers/i2c/busses/i2c-imx.c | 764 +++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 721 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index d3ba08c..b53bee6 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -42,6 +42,7 @@
> #include <linux/interrupt.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> +#include <linux/kthread.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> @@ -58,6 +59,7 @@
> #include <linux/of.h>
> #include <linux/of_device.h>
> #include <linux/libata.h>
> +#include <uapi/linux/sched/types.h>
>
> /* This will be the driver name the kernel reports */
> #define DRIVER_NAME "imx-i2c"
> @@ -65,6 +67,13 @@
> /* Default value */
> #define IMX_I2C_BIT_RATE 100000 /* 100kHz */
>
> +/* Wait delays */
> +#define IMX_I2C_STATE_NO_DELAY 0
> +#define IMX_I2C_STATE_MIN_DELAY 1 /* 1 jiffy */
> +#define IMX_I2C_STATE_DELAY (HZ / 10)
Are these delays hardware imposed or chosen more or less arbitrarily? A
comment describing that would be nice.
> +#define MAX_EVENTS 16
The name of this define is too general, please use a driver specific
prefix.
> +
> /*
> * Enable DMA if transfer byte size is bigger than this threshold.
> * As the hardware request, it must bigger than 4 bytes.\
> @@ -122,6 +131,30 @@
>
> #define I2C_PM_TIMEOUT 10 /* ms */
>
> +enum imx_i2c_state {
> + STATE_IDLE,
> + STATE_SLAVE,
> + STATE_MASTER,
> + STATE_START_POLL,
> +};
> +
> +enum imx_i2c_events {
> + EVT_AL,
> + EVT_SI,
> + EVT_START,
> + EVT_STOP,
> + EVT_POLL,
> + EVT_INVALID,
> + EVT_ENTRY,
> +};
These enums don't match any hardware register values, right?
> +struct imx_i2c_evt_queue {
> + int count;
> + int head_idx;
> + int tail_idx;
> + enum imx_i2c_events evt[MAX_EVENTS];
> +};
> +
> enum pinmux_endian_type {
> BIG_ENDIAN,
> LITTLE_ENDIAN,
> @@ -246,6 +279,7 @@ struct imx_i2c_dma {
>
> struct imx_i2c_struct {
> struct i2c_adapter adapter;
> + struct i2c_client *slave;
> struct clk *clk;
> void __iomem *base;
> wait_queue_head_t queue;
> @@ -269,6 +303,17 @@ struct imx_i2c_struct {
> int pmuxcr_set;
> int pmuxcr_endian;
> void __iomem *pmuxcr_addr;
> +
> + enum imx_i2c_state state;
> + struct imx_i2c_evt_queue events;
> + int last_error;
> +
> + bool master_interrupt;
> + unsigned int start_retry_cnt;
> + unsigned int slave_poll_cnt;
> +
> + struct task_struct *slave_task;
> + wait_queue_head_t state_queue;
Here it would be sensible to group all slave mode related members
together and add a comment about that. Regarding the naming, I wonder if
some of these should have "slave" in their name that current don't.
> };
>
> static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
> @@ -322,6 +367,9 @@ struct imx_i2c_struct {
> };
> MODULE_DEVICE_TABLE(of, i2c_imx_dt_ids);
>
> +static void i2c_imx_clear_isr_bit(struct imx_i2c_struct *i2c_imx,
> + unsigned int status);
> +
This declaration doesn't seem to be needed.
> static inline int is_imx1_i2c(struct imx_i2c_struct *i2c_imx)
> {
> return i2c_imx->hwdata->devtype == IMX1_I2C;
> @@ -473,17 +521,28 @@ static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
> static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
> {
> unsigned long orig_jiffies = jiffies;
> - unsigned int temp;
> + unsigned int temp, ctl;
>
> dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>
> while (1) {
ctl can have a more narrow scope by moving it's declaration into the
while block. Would it make sense to call this variable "i2cr" or "cr"
instead? (The same applies to "temp" btw, which could be fixed in a
separate patch.)
> temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>
> - /* check for arbitration lost */
> - if (temp & I2SR_IAL) {
> - temp &= ~I2SR_IAL;
> - imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> + /*
> + * Check for arbitration lost. Datasheet recommends to
> + * clean IAL bit in interrupt handler before any other
> + * action.
> + *
> + * We can detect if controller resets MSTA bit, because
> + * hardware is switched to slave mode if arbitration was
> + * lost.
> + */
Please only use a space, not a tab between * and comment.
> + if (for_busy && !(ctl & I2CR_MSTA)) {
> + dev_dbg(&i2c_imx->adapter.dev,
> + "<%s> Lost arbitration (SR = %02x, CR = %02x)\n",
> + __func__, temp, ctl);
> return -EAGAIN;
> }
>
> @@ -502,16 +561,510 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
> return 0;
> }
>
> +#ifdef CONFIG_I2C_SLAVE
> +
I didn't check in detail, but if you mark the affected functions as
"maybe unused" instead of putting them in an ifdef-block you increase
compile coverage without affecting the compiled result.
> +static void i2c_imx_enable_i2c_controller(struct imx_i2c_struct *i2c_imx);
> +static void set_state(struct imx_i2c_struct *i2c_imx, unsigned int new);
> +static int i2c_imx_hw_start(struct imx_i2c_struct *i2c_imx);
> +static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx);
It would be great if you could resort the order of functions to get rid
of these declarations.
> +static inline int evt_find_next_idx(int *v)
> +{
> + return (*v)++ & (MAX_EVENTS - 1);
> +}
> +
> +static int i2c_imx_evt_put(struct imx_i2c_evt_queue *stk,
> + enum imx_i2c_events evt)
> +{
> + int count = stk->count++;
> + int idx;
> +
> + if (count < MAX_EVENTS) {
> + idx = evt_find_next_idx(&stk->tail_idx);
> + stk->evt[idx] = evt;
> + } else {
> + stk->count--;
> + return -EBUSY;
> + }
> +
> + return 0;
> +}
> +
> +static enum imx_i2c_events i2c_imx_evt_get(struct imx_i2c_evt_queue *stk)
> +{
> + int count = stk->count--;
> + int idx;
> +
> + if (count > 0) {
> + idx = evt_find_next_idx(&stk->head_idx);
> + } else {
> + stk->count++;
> + return EVT_INVALID;
> + }
> +
> + return stk->evt[idx];
> +}
> +
> +static bool evt_is_empty(struct imx_i2c_evt_queue *stk)
> +{
> + return (stk->count <= 0);
> +}
> +
> +static void evt_init(struct imx_i2c_evt_queue *stk)
> +{
> + stk->count = 0;
> + stk->tail_idx = -1;
> + stk->head_idx = -1;
> +}
Can you please use "i2c_imx_" as prefix for all new functions? This
helps interpreting stack dumps and ftrace filtering.
> +static void i2c_imx_clear_ial_bit(struct imx_i2c_struct *i2c_imx)
> +{
> + unsigned int status;
> +
> + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> +#ifdef CONFIG_PPC_55xx
> + status |= I2SR_IAL;
> +#else
> + status &= ~I2SR_IAL;
> +#endif
A comment describing why PPC is different here would be nice. Also
if (IS_ENABLED(CONFIG_PPC_55xx))
status |= ...
else
status &= ..
is nicer.
> + imx_i2c_write_reg(status, i2c_imx, IMX_I2C_I2SR);
> +}
> +
> [...]
> +static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
> +{
> + unsigned int temp = 0;
> + int result, cnt = 0;
> +
> + dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> +
> + if (i2c_imx->slave_task) {
this function is called in master mode, too, right. So maybe make that
if
if (IS_ENABLED(CONFIG_I2C_SLAVE_MODE) && i2c_imx->slave_task)
to make the compiler throw away dead code.
> + i2c_imx->last_error = -EINPROGRESS;
> +
> + result = i2c_imx_evt_put(&i2c_imx->events, EVT_START);
> + if (result) {
> + dev_err(&i2c_imx->adapter.dev,
> + "Event buffer overflow\n");
> + return result;
> + }
> +
> + wake_up(&i2c_imx->state_queue);
> +
> + result = i2c_imx->last_error;
> +
> + while (result == -EINPROGRESS) {
> + schedule();
> +
> + /* start hung monitoring */
> + cnt++;
> + if (cnt == 500000) {
> + dev_err(&i2c_imx->adapter.dev,
> + "Too many start loops\n");
> + temp = i2c_imx->hwdata->i2cr_ien_opcode
> + ^ I2CR_IEN;
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> + i2c_imx_enable_i2c_controller(i2c_imx);
> + temp = i2c_imx->hwdata->i2cr_ien_opcode
> + | I2CR_IIEN;
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> + return -ETIMEDOUT;
> + }
> + result = i2c_imx->last_error;
> + }
> + } else { /* i2c slave not used */
> +
> + result = i2c_imx_hw_start(i2c_imx);
> + if (result)
> + return result;
> +
> + /* Start I2C transaction */
> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + temp |= I2CR_MSTA;
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> + result = i2c_imx_bus_busy(i2c_imx, 1);
> + if (result)
> + return result;
> + i2c_imx->stopped = 0;
> +
> + temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
> + temp &= ~I2CR_DMAEN;
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> + }
> return result;
> }
>
> -static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
> +static void i2c_imx_hw_stop(struct imx_i2c_struct *i2c_imx)
> {
> unsigned int temp = 0;
>
> @@ -628,23 +1249,55 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
> }
>
> /* Disable I2C controller */
> - temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN,
> + temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN;
This change belongs into a separate patch.
> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> + clk_disable_unprepare(i2c_imx->clk);
This is a change that affects master mode. How did you test the patch?
> +}
> +
> +static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
> +{
> + if (!i2c_imx->slave) {
> + i2c_imx_hw_stop(i2c_imx);
> + } else {
> + dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> +
> + i2c_imx_evt_put(&i2c_imx->events, EVT_STOP);
> + wake_up(&i2c_imx->state_queue);
> + }
> +}
> +
> +static void i2c_imx_clear_isr_bit(struct imx_i2c_struct *i2c_imx,
> + unsigned int status)
> +{
> + status &= ~I2SR_IIF;
> + status |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IIF);
> + imx_i2c_write_reg(status, i2c_imx, IMX_I2C_I2SR);
> }
>
> static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
> {
> struct imx_i2c_struct *i2c_imx = dev_id;
> - unsigned int temp;
> + unsigned int status, ctl;
> +
> + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + if (status & I2SR_IIF) {
> + i2c_imx_clear_isr_bit(i2c_imx, status);
> +
> + if (ctl & I2CR_MSTA) {
> + dev_dbg(&i2c_imx->adapter.dev, "master interrupt");
> + i2c_imx->master_interrupt = true;
> + wake_up(&i2c_imx->queue);
> + } else if (status & I2SR_IAL) { /* arbitration lost */
> + i2c_imx_clear_ial_bit(i2c_imx);
> + i2c_imx_evt_put(&i2c_imx->events, EVT_AL);
> + } else {
> + dev_dbg(&i2c_imx->adapter.dev, "slave interrupt");
> + i2c_imx_evt_put(&i2c_imx->events, EVT_SI);
> + wake_up(&i2c_imx->state_queue);
> + }
>
> - temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> - if (temp & I2SR_IIF) {
> - /* save status register */
> - i2c_imx->i2csr = temp;
> - temp &= ~I2SR_IIF;
> - temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IIF);
> - imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> - wake_up(&i2c_imx->queue);
> return IRQ_HANDLED;
> }
>
> @@ -814,9 +1467,11 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> result = i2c_imx_trx_complete(i2c_imx);
> if (result)
> return result;
> +
> result = i2c_imx_acked(i2c_imx);
> if (result)
> return result;
> +
Such cosmetic patches shouldn't be mixed in a patch changing more than
other cosmetics.
> dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__);
>
> /* write data */
> @@ -828,6 +1483,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> result = i2c_imx_trx_complete(i2c_imx);
> if (result)
> return result;
> +
> result = i2c_imx_acked(i2c_imx);
> if (result)
> return result;
> @@ -842,24 +1498,24 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
> int block_data = msgs->flags & I2C_M_RECV_LEN;
>
> dev_dbg(&i2c_imx->adapter.dev,
> - "<%s> write slave address: addr=0x%x\n",
> + "<%s> read slave address: addr=0x%x\n",
> __func__, (msgs->addr << 1) | 0x01);
separate patch please
>
> /* write slave address */
> imx_i2c_write_reg((msgs->addr << 1) | 0x01, i2c_imx, IMX_I2C_I2DR);
> +
> result = i2c_imx_trx_complete(i2c_imx);
> if (result)
> return result;
> +
> result = i2c_imx_acked(i2c_imx);
> if (result)
> return result;
> -
> dev_dbg(&i2c_imx->adapter.dev, "<%s> setup bus\n", __func__);
>
> /* setup bus to read data */
> temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> temp &= ~I2CR_MTX;
> -
> /*
> * Reset the I2CR_TXAK flag initially for SMBus block read since the
> * length is unknown
> @@ -870,17 +1526,16 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
> imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
>
> dev_dbg(&i2c_imx->adapter.dev, "<%s> read data\n", __func__);
> -
> if (i2c_imx->dma && msgs->len >= DMA_THRESHOLD && !block_data)
> return i2c_imx_dma_read(i2c_imx, msgs, is_lastmsg);
>
> /* read data */
> for (i = 0; i < msgs->len; i++) {
> u8 len = 0;
> -
> result = i2c_imx_trx_complete(i2c_imx);
> if (result)
> return result;
> +
> /*
> * First byte is the length of remaining packet
> * in the SMBus block data read. Add it to
> @@ -905,6 +1560,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
> "<%s> clear MSTA\n", __func__);
> temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> temp &= ~(I2CR_MSTA | I2CR_MTX);
> +
> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> i2c_imx_bus_busy(i2c_imx, 0);
> i2c_imx->stopped = 1;
> @@ -931,6 +1587,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
> msgs->buf[0] = len;
> else
> msgs->buf[i] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> +
> dev_dbg(&i2c_imx->adapter.dev,
> "<%s> read byte: B%d=0x%X\n",
> __func__, i, msgs->buf[i]);
> @@ -1039,7 +1696,14 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
>
> /* Start I2C transfer */
> result = i2c_imx_start(i2c_imx);
> - if (result) {
> + if (result == -ETIMEDOUT) {
> + /*
> + * Recovery is not started on arbitration lost, since it can
> + * break slave transfer. But in case of "bus timeout" recovery
> + * it could be useful to bring controller out of a
> + * "strange state".
> + */
> + dev_dbg(&i2c_imx->adapter.dev, "call bus recovery");
> if (i2c_imx->adapter.bus_recovery_info) {
> i2c_recover_bus(&i2c_imx->adapter);
> result = i2c_imx_start(i2c_imx);
> @@ -1228,6 +1892,10 @@ static u32 i2c_imx_func(struct i2c_adapter *adapter)
> static const struct i2c_algorithm i2c_imx_algo = {
> .master_xfer = i2c_imx_xfer,
> .functionality = i2c_imx_func,
> +#ifdef CONFIG_I2C_SLAVE
> + .reg_slave = i2c_imx_reg_slave,
> + .unreg_slave = i2c_imx_unreg_slave,
> +#endif
> };
>
> static int i2c_imx_probe(struct platform_device *pdev)
> @@ -1301,6 +1969,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
> }
>
> /* Init queue */
> + init_waitqueue_head(&i2c_imx->state_queue);
> init_waitqueue_head(&i2c_imx->queue);
>
> /* Set up adapter data */
> @@ -1328,6 +1997,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
> /* Set up chip registers to defaults */
> imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN,
> i2c_imx, IMX_I2C_I2CR);
> +
> imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, IMX_I2C_I2SR);
>
> /* Init optional bus recovery function */
> @@ -1350,8 +2020,13 @@ static int i2c_imx_probe(struct platform_device *pdev)
> i2c_imx->adapter.name);
> dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
>
> +#ifdef CONIFG_LAYERSCAPE
> /* Init DMA config if supported */
> i2c_imx_dma_request(i2c_imx, phy_addr);
> +#endif
This looks unrelated to slave support, too.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
