| Message ID | 20210626102806.15402-3-raviteja.narayanam@xilinx.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: xiic: Add features, bug fixes. | expand |
Hi, I know this patch is quite old but we need the smbus_block_read functionality, which depends on this one, so I would really like this to be merged. See my comments below. Dnia Sat, Jun 26, 2021 at 03:57:58PM +0530, Raviteja Narayanam napisał(a): > In the current driver implementation, there is a limit of read > transfer size to 255 bytes as it is using AXI I2C dynamic mode. > But the IP supports this transfer through standard mode. > > So added AXI I2C standard mode support to enable read transfers > of size more than 255 bytes. The driver scans through the message > request from user space and selects AXI I2C standard mode if there > is a read request of more than 255 bytes. Then the whole message goes > through standard mode Tx and Rx paths. > > Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com> > --- > drivers/i2c/busses/i2c-xiic.c | 367 +++++++++++++++++++++++++++++----- > 1 file changed, 319 insertions(+), 48 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c > index b0cfd9d15467..004103267e9c 100644 > --- a/drivers/i2c/busses/i2c-xiic.c > +++ b/drivers/i2c/busses/i2c-xiic.c > @@ -60,6 +60,8 @@ enum xiic_endian { > * @clk: Pointer to AXI4-lite input clock > * @state: See STATE_ > * @singlemaster: Indicates bus is single master > + * @dynamic: Mode of controller > + * @prev_msg_tx: Previous message is Tx > */ > struct xiic_i2c { > struct device *dev; > @@ -76,6 +78,8 @@ struct xiic_i2c { > struct clk *clk; > enum xilinx_i2c_state state; > bool singlemaster; > + bool dynamic; > + bool prev_msg_tx; > }; > > > @@ -144,6 +148,9 @@ struct xiic_i2c { > #define XIIC_TX_DYN_START_MASK 0x0100 /* 1 = Set dynamic start */ > #define XIIC_TX_DYN_STOP_MASK 0x0200 /* 1 = Set dynamic stop */ > > +/* Dynamic mode constants */ > +#define MAX_READ_LENGTH_DYNAMIC 255 /* Max length for dynamic read */ > + > /* > * The following constants define the register offsets for the Interrupt > * registers. There are some holes in the memory map for reserved addresses > @@ -270,6 +277,24 @@ static int xiic_clear_rx_fifo(struct xiic_i2c *i2c) > return 0; > } > > +static int xiic_wait_tx_empty(struct xiic_i2c *i2c) > +{ > + u8 isr; > + unsigned long timeout; > + > + timeout = jiffies + XIIC_I2C_TIMEOUT; > + for (isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET); > + !(isr & XIIC_INTR_TX_EMPTY_MASK); > + isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET)) { > + if (time_after(jiffies, timeout)) { > + dev_err(i2c->dev, "Timeout waiting at Tx empty\n"); > + return -ETIMEDOUT; > + } > + } > + > + return 0; > +} > + > static int xiic_reinit(struct xiic_i2c *i2c) > { > int ret; > @@ -311,13 +336,14 @@ static void xiic_deinit(struct xiic_i2c *i2c) > > static void xiic_read_rx(struct xiic_i2c *i2c) > { > - u8 bytes_in_fifo; > + u8 bytes_in_fifo, cr = 0, bytes_to_read = 0; > + u32 bytes_rem = 0; > int i; > > bytes_in_fifo = xiic_getreg8(i2c, XIIC_RFO_REG_OFFSET) + 1; > > dev_dbg(i2c->adap.dev.parent, > - "%s entry, bytes in fifo: %d, msg: %d, SR: 0x%x, CR: 0x%x\n", > + "%s entry, bytes in fifo: %d, rem: %d, SR: 0x%x, CR: 0x%x\n", > __func__, bytes_in_fifo, xiic_rx_space(i2c), > xiic_getreg8(i2c, XIIC_SR_REG_OFFSET), > xiic_getreg8(i2c, XIIC_CR_REG_OFFSET)); > @@ -325,13 +351,53 @@ static void xiic_read_rx(struct xiic_i2c *i2c) > if (bytes_in_fifo > xiic_rx_space(i2c)) > bytes_in_fifo = xiic_rx_space(i2c); > > - for (i = 0; i < bytes_in_fifo; i++) > + bytes_to_read = bytes_in_fifo; > + > + if (!i2c->dynamic) { > + bytes_rem = xiic_rx_space(i2c) - bytes_in_fifo; > + > + if (bytes_rem > IIC_RX_FIFO_DEPTH) { > + bytes_to_read = bytes_in_fifo; > + } else if (bytes_rem > 1) { > + bytes_to_read = bytes_rem - 1; > + } else if (bytes_rem == 1) { > + bytes_to_read = 1; > + /* Set NACK in CR to indicate slave transmitter */ > + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); > + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr | > + XIIC_CR_NO_ACK_MASK); > + } else if (bytes_rem == 0) { > + bytes_to_read = bytes_in_fifo; > + > + /* Generate stop on the bus if it is last message */ > + if (i2c->nmsgs == 1) { > + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); > + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr & > + ~XIIC_CR_MSMS_MASK); > + } > + > + /* Make TXACK=0, clean up for next transaction */ > + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); > + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr & > + ~XIIC_CR_NO_ACK_MASK); > + } > + } > + > + /* Read the fifo */ > + for (i = 0; i < bytes_to_read; i++) { > i2c->rx_msg->buf[i2c->rx_pos++] = > xiic_getreg8(i2c, XIIC_DRR_REG_OFFSET); > + } > > - xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, > - (xiic_rx_space(i2c) > IIC_RX_FIFO_DEPTH) ? > - IIC_RX_FIFO_DEPTH - 1 : xiic_rx_space(i2c) - 1); > + if (i2c->dynamic) { > + u8 bytes; > + > + /* Receive remaining bytes if less than fifo depth */ > + bytes = min_t(u8, xiic_rx_space(i2c), IIC_RX_FIFO_DEPTH); > + bytes--; > + > + xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, bytes); > + } > } > > static int xiic_tx_fifo_space(struct xiic_i2c *i2c) > @@ -361,6 +427,62 @@ static void xiic_fill_tx_fifo(struct xiic_i2c *i2c) > } > } > > +static void xiic_std_fill_tx_fifo(struct xiic_i2c *i2c) > +{ > + u8 fifo_space = xiic_tx_fifo_space(i2c); > + u16 data = 0; > + int len = xiic_tx_space(i2c); > + > + dev_dbg(i2c->adap.dev.parent, "%s entry, len: %d, fifo space: %d\n", > + __func__, len, fifo_space); > + > + if (len > fifo_space) > + len = fifo_space; > + else if (len && !(i2c->nmsgs > 1)) > + len--; > + > + while (len--) { > + data = i2c->tx_msg->buf[i2c->tx_pos++]; > + xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data); > + } > +} This function looks very similar to the original xiic_fill_tx_fifo. The only difference is that it does not decrease the len in case of repeated_start (btw, why?), and it does not set the DYN_STOP bit. But this could be done conditionally based on i2c->dynamic, instead. No need for this duplication, in my opinion. > + > +static void xiic_send_tx(struct xiic_i2c *i2c) > +{ > + dev_dbg(i2c->adap.dev.parent, > + "%s entry, rem: %d, SR: 0x%x, CR: 0x%x\n", > + __func__, xiic_tx_space(i2c), > + xiic_getreg8(i2c, XIIC_SR_REG_OFFSET), > + xiic_getreg8(i2c, XIIC_CR_REG_OFFSET)); > + > + if (xiic_tx_space(i2c) > 1) { > + xiic_std_fill_tx_fifo(i2c); > + return; > + } > + > + if ((xiic_tx_space(i2c) == 1)) { > + u16 data; > + > + if (i2c->nmsgs == 1) { > + u8 cr; > + int status; > + > + /* Wait till FIFO is empty so STOP is sent last */ > + status = xiic_wait_tx_empty(i2c); > + if (status) > + return; > + > + /* Write to CR to stop */ > + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); > + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr & > + ~XIIC_CR_MSMS_MASK); > + } > + /* Send last byte */ > + data = i2c->tx_msg->buf[i2c->tx_pos++]; > + xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data); > + } > +} > + > static void xiic_wakeup(struct xiic_i2c *i2c, int code) > { > i2c->tx_msg = NULL; > @@ -391,7 +513,9 @@ static irqreturn_t xiic_process(int irq, void *dev_id) > dev_dbg(i2c->adap.dev.parent, "%s: SR: 0x%x, msg: %p, nmsgs: %d\n", > __func__, xiic_getreg8(i2c, XIIC_SR_REG_OFFSET), > i2c->tx_msg, i2c->nmsgs); > - > + dev_dbg(i2c->adap.dev.parent, "%s, ISR: 0x%x, CR: 0x%x\n", > + __func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET), > + xiic_getreg8(i2c, XIIC_CR_REG_OFFSET)); > > /* Service requesting interrupt */ > if ((pend & XIIC_INTR_ARB_LOST_MASK) || > @@ -465,7 +589,10 @@ static irqreturn_t xiic_process(int irq, void *dev_id) > goto out; > } > > - xiic_fill_tx_fifo(i2c); > + if (i2c->dynamic) > + xiic_fill_tx_fifo(i2c); > + else > + xiic_send_tx(i2c); > > /* current message sent and there is space in the fifo */ > if (!xiic_tx_space(i2c) && xiic_tx_fifo_space(i2c) >= 2) { > @@ -554,35 +681,113 @@ static int xiic_busy(struct xiic_i2c *i2c) > > static void xiic_start_recv(struct xiic_i2c *i2c) > { > - u8 rx_watermark; > + u16 rx_watermark; > + u8 cr = 0, rfd_set = 0; > struct i2c_msg *msg = i2c->rx_msg = i2c->tx_msg; > unsigned long flags; > > - /* Clear and enable Rx full interrupt. */ > - xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK | XIIC_INTR_TX_ERROR_MASK); > + dev_dbg(i2c->adap.dev.parent, "%s entry, ISR: 0x%x, CR: 0x%x\n", > + __func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET), > + xiic_getreg8(i2c, XIIC_CR_REG_OFFSET)); > + > + /* Disable Tx interrupts */ > + xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK | XIIC_INTR_TX_EMPTY_MASK); > > - /* we want to get all but last byte, because the TX_ERROR IRQ is used > - * to inidicate error ACK on the address, and negative ack on the last > - * received byte, so to not mix them receive all but last. > - * In the case where there is only one byte to receive > - * we can check if ERROR and RX full is set at the same time > - */ > - rx_watermark = msg->len; > - if (rx_watermark > IIC_RX_FIFO_DEPTH) > - rx_watermark = IIC_RX_FIFO_DEPTH; > - xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rx_watermark - 1); > - > - local_irq_save(flags); > - if (!(msg->flags & I2C_M_NOSTART)) > - /* write the address */ > + if (i2c->dynamic) { > + u8 bytes; > + u16 val; > + > + /* Clear and enable Rx full interrupt. */ > + xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK | > + XIIC_INTR_TX_ERROR_MASK); > + > + /* > + * We want to get all but last byte, because the TX_ERROR IRQ > + * is used to indicate error ACK on the address, and > + * negative ack on the last received byte, so to not mix > + * them receive all but last. > + * In the case where there is only one byte to receive > + * we can check if ERROR and RX full is set at the same time > + */ > + rx_watermark = msg->len; > + bytes = min_t(u8, rx_watermark, IIC_RX_FIFO_DEPTH); > + bytes--; Do we really want to write 255 to RFD if msg->len == 0? That will set the compare value in the RX_FIFO_PIRQ register to max value (15) but I don't understand why we would like to do this. Also, bits 31:4 are reserved so I think we should not try to touch them. > + > + xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, bytes); > + > + local_irq_save(flags); > + if (!(msg->flags & I2C_M_NOSTART)) > + /* write the address */ > + xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, > + i2c_8bit_addr_from_msg(msg) | > + XIIC_TX_DYN_START_MASK); When reviewing this patch, I tried to understand how the controller knows if it should work in dynamic or in stanard mode. My understanding is that in order to start the dynamic mode logic, we have to set the DYN_START bit in the TX FIFO when we write an address there. Is this correct? But we don't do that if I2C_M_NOSTART flag is set so how is this supposed to work with this flag? I mean, does the controller really supports doing I2C_M_NOSTART in dynamic mode? Or does it support it at all? After all, when we skip this, we will still write to the TX_FIFO register 5 lines below. How is the controller supposed to know that the len that we write there is *not* actually an address? That being said, we do not annouce the I2C_FUNC_NOSTART support so maybe we should not care at all and just remove the code handling the I2C_M_NOSTART flag? > + > + xiic_irq_clr_en(i2c, XIIC_INTR_BNB_MASK); > + > + /* If last message, include dynamic stop bit with length */ > + val = (i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0; > + val |= msg->len; > + > + xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, val); > + local_irq_restore(flags); > + } else { > + /* > + * If previous message is Tx, make sure that Tx FIFO is empty > + * before starting a new transfer as the repeated start in > + * standard mode can corrupt the transaction if there are > + * still bytes to be transmitted in FIFO > + */ > + if (i2c->prev_msg_tx) { > + int status; > + > + status = xiic_wait_tx_empty(i2c); > + if (status) > + return; > + } > + > + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); > + > + /* Set Receive fifo depth */ > + rx_watermark = msg->len; > + if (rx_watermark > IIC_RX_FIFO_DEPTH) { > + rfd_set = IIC_RX_FIFO_DEPTH - 1; > + } else if ((rx_watermark == 1) || (rx_watermark == 0)) { > + rfd_set = rx_watermark - 1; Again, do we really want to write 255 to RFD if msg->len == 0? > + /* Handle single byte transfer separately */ > + cr |= XIIC_CR_NO_ACK_MASK; > + } else { > + rfd_set = rx_watermark - 2; > + } > + > + /* Check if RSTA should be set */ > + if (cr & XIIC_CR_MSMS_MASK) { > + /* Already a master, RSTA should be set */ > + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr | > + XIIC_CR_REPEATED_START_MASK) & > + ~(XIIC_CR_DIR_IS_TX_MASK)); > + } > + > + xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rfd_set); > + > + /* Clear and enable Rx full and transmit complete interrupts */ > + xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK | > + XIIC_INTR_TX_ERROR_MASK); > + > + /* Write the address */ > xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, > - i2c_8bit_addr_from_msg(msg) | XIIC_TX_DYN_START_MASK); > + i2c_8bit_addr_from_msg(msg)); > > - xiic_irq_clr_en(i2c, XIIC_INTR_BNB_MASK); > + /* Write to Control Register,to start transaction in Rx mode */ > + if ((cr & XIIC_CR_MSMS_MASK) == 0) { > + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr | > + XIIC_CR_MSMS_MASK) > + & ~(XIIC_CR_DIR_IS_TX_MASK)); > + } > > - xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, > - msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0)); > - local_irq_restore(flags); > + dev_dbg(i2c->adap.dev.parent, "%s end, ISR: 0x%x, CR: 0x%x\n", > + __func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET), > + xiic_getreg8(i2c, XIIC_CR_REG_OFFSET)); > + } > > if (i2c->nmsgs == 1) > /* very last, enable bus not busy as well */ > @@ -590,10 +795,17 @@ static void xiic_start_recv(struct xiic_i2c *i2c) > > /* the message is tx:ed */ > i2c->tx_pos = msg->len; > + > + /* Enable interrupts */ > + xiic_setreg32(i2c, XIIC_DGIER_OFFSET, XIIC_GINTR_ENABLE_MASK); > + > + i2c->prev_msg_tx = false; > } > > static void xiic_start_send(struct xiic_i2c *i2c) > { > + u8 cr = 0; > + u16 data; > struct i2c_msg *msg = i2c->tx_msg; > > xiic_irq_clr(i2c, XIIC_INTR_TX_ERROR_MASK); > @@ -604,22 +816,71 @@ static void xiic_start_send(struct xiic_i2c *i2c) > __func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET), > xiic_getreg8(i2c, XIIC_CR_REG_OFFSET)); > > - if (!(msg->flags & I2C_M_NOSTART)) { > - /* write the address */ > - u16 data = i2c_8bit_addr_from_msg(msg) | > - XIIC_TX_DYN_START_MASK; > - if ((i2c->nmsgs == 1) && msg->len == 0) > - /* no data and last message -> add STOP */ > - data |= XIIC_TX_DYN_STOP_MASK; > + if (i2c->dynamic) { > + if (!(msg->flags & I2C_M_NOSTART)) { > + /* write the address */ > + data = i2c_8bit_addr_from_msg(msg) | > + XIIC_TX_DYN_START_MASK; > + > + if (i2c->nmsgs == 1 && msg->len == 0) > + /* no data and last message -> add STOP */ > + data |= XIIC_TX_DYN_STOP_MASK; > + > + xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data); > + } > + > + xiic_fill_tx_fifo(i2c); > + > + /* Clear any pending Tx empty, Tx Error and then enable them */ > + xiic_irq_clr_en(i2c, (XIIC_INTR_TX_EMPTY_MASK | > + XIIC_INTR_TX_ERROR_MASK | > + XIIC_INTR_BNB_MASK)); Before this patch, interrupts were cleared before the call to xiic_fill_tx_fifo(), now it is the other way around. Was this intentional? If so, why? > + } else { > + /* > + * If previous message is Tx, make sure that Tx FIFO is empty > + * before starting a new transfer as the repeated start in > + * standard mode can corrupt the transaction if there are > + * still bytes to be transmitted in FIFO > + */ > + if (i2c->prev_msg_tx) { > + int status; > + > + status = xiic_wait_tx_empty(i2c); > + if (status) > + return; > + } > > + /* Check if RSTA should be set */ > + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); > + if (cr & XIIC_CR_MSMS_MASK) { > + /* Already a master, RSTA should be set */ > + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr | > + XIIC_CR_REPEATED_START_MASK | > + XIIC_CR_DIR_IS_TX_MASK) & > + ~(XIIC_CR_NO_ACK_MASK)); > + } > + > + /* Write address to FIFO */ > + data = i2c_8bit_addr_from_msg(msg); > xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data); > - } > + /* Fill fifo */ > + xiic_std_fill_tx_fifo(i2c); > > - xiic_fill_tx_fifo(i2c); > + if ((cr & XIIC_CR_MSMS_MASK) == 0) { > > - /* Clear any pending Tx empty, Tx Error and then enable them. */ > - xiic_irq_clr_en(i2c, XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_ERROR_MASK | > - XIIC_INTR_BNB_MASK); > + /* Start Tx by writing to CR */ > + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); > + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr | > + XIIC_CR_MSMS_MASK | > + XIIC_CR_DIR_IS_TX_MASK); > + } > + > + /* Clear any pending Tx empty, Tx Error and then enable them */ > + xiic_irq_clr_en(i2c, XIIC_INTR_TX_EMPTY_MASK | > + XIIC_INTR_TX_ERROR_MASK | > + XIIC_INTR_BNB_MASK); > + } > + i2c->prev_msg_tx = true; > } > > static irqreturn_t xiic_isr(int irq, void *dev_id) > @@ -703,7 +964,7 @@ static int xiic_start_xfer(struct xiic_i2c *i2c) > static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) > { > struct xiic_i2c *i2c = i2c_get_adapdata(adap); > - int err; > + int err, count; > > dev_dbg(adap->dev.parent, "%s entry SR: 0x%x\n", __func__, > xiic_getreg8(i2c, XIIC_SR_REG_OFFSET)); > @@ -719,6 +980,21 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) > i2c->tx_msg = msgs; > i2c->nmsgs = num; > > + /* Decide standard mode or Dynamic mode */ > + i2c->dynamic = true; > + > + /* Initialize prev message type */ > + i2c->prev_msg_tx = false; > + > + /* Enter standard mode only when read length is > 255 bytes */ > + for (count = 0; count < i2c->nmsgs; count++) { > + if ((i2c->tx_msg[count].flags & I2C_M_RD) && > + i2c->tx_msg[count].len > MAX_READ_LENGTH_DYNAMIC) { > + i2c->dynamic = false; > + break; > + } > + } > + > err = xiic_start_xfer(i2c); > if (err < 0) { > dev_err(adap->dev.parent, "Error xiic_start_xfer\n"); > @@ -752,16 +1028,11 @@ static const struct i2c_algorithm xiic_algorithm = { > .functionality = xiic_func, > }; > > -static const struct i2c_adapter_quirks xiic_quirks = { > - .max_read_len = 255, > -}; > - > static const struct i2c_adapter xiic_adapter = { > .owner = THIS_MODULE, > .name = DRIVER_NAME, > .class = I2C_CLASS_DEPRECATED, > .algo = &xiic_algorithm, > - .quirks = &xiic_quirks, > }; > >
Hi, On 6/29/22 13:02, Krzysztof Adamski wrote: > > Hi, > > I know this patch is quite old but we need the smbus_block_read > functionality, which depends on this one, so I would really like this to > be merged. See my comments below. Please try this series and let us know if it is working for you or not. https://lore.kernel.org/all/1656072327-13628-1-git-send-email-manikanta.guntupalli@xilinx.com/ Thanks, Michal
Hi Michal, I did not realize there is a v3 to that, sorry. I have tested those patches untill the "Add smbus_block_read functionality" patch and it seems to be working fine for me. I will move my comments to this thread instead. Krzysztof W dniu 29.06.2022 o 13:39, Michal Simek pisze: > Hi, > > On 6/29/22 13:02, Krzysztof Adamski wrote: >> >> Hi, >> >> I know this patch is quite old but we need the smbus_block_read >> functionality, which depends on this one, so I would really like this to >> be merged. See my comments below. > > Please try this series and let us know if it is working for you or not. > > https://lore.kernel.org/all/1656072327-13628-1-git-send-email-manikanta.guntupalli@xilinx.com/ > > > Thanks, > Michal
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index b0cfd9d15467..004103267e9c 100644 --- a/drivers/i2c/busses/i2c-xiic.c +++ b/drivers/i2c/busses/i2c-xiic.c @@ -60,6 +60,8 @@ enum xiic_endian { * @clk: Pointer to AXI4-lite input clock * @state: See STATE_ * @singlemaster: Indicates bus is single master + * @dynamic: Mode of controller + * @prev_msg_tx: Previous message is Tx */ struct xiic_i2c { struct device *dev; @@ -76,6 +78,8 @@ struct xiic_i2c { struct clk *clk; enum xilinx_i2c_state state; bool singlemaster; + bool dynamic; + bool prev_msg_tx; }; @@ -144,6 +148,9 @@ struct xiic_i2c { #define XIIC_TX_DYN_START_MASK 0x0100 /* 1 = Set dynamic start */ #define XIIC_TX_DYN_STOP_MASK 0x0200 /* 1 = Set dynamic stop */ +/* Dynamic mode constants */ +#define MAX_READ_LENGTH_DYNAMIC 255 /* Max length for dynamic read */ + /* * The following constants define the register offsets for the Interrupt * registers. There are some holes in the memory map for reserved addresses @@ -270,6 +277,24 @@ static int xiic_clear_rx_fifo(struct xiic_i2c *i2c) return 0; } +static int xiic_wait_tx_empty(struct xiic_i2c *i2c) +{ + u8 isr; + unsigned long timeout; + + timeout = jiffies + XIIC_I2C_TIMEOUT; + for (isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET); + !(isr & XIIC_INTR_TX_EMPTY_MASK); + isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET)) { + if (time_after(jiffies, timeout)) { + dev_err(i2c->dev, "Timeout waiting at Tx empty\n"); + return -ETIMEDOUT; + } + } + + return 0; +} + static int xiic_reinit(struct xiic_i2c *i2c) { int ret; @@ -311,13 +336,14 @@ static void xiic_deinit(struct xiic_i2c *i2c) static void xiic_read_rx(struct xiic_i2c *i2c) { - u8 bytes_in_fifo; + u8 bytes_in_fifo, cr = 0, bytes_to_read = 0; + u32 bytes_rem = 0; int i; bytes_in_fifo = xiic_getreg8(i2c, XIIC_RFO_REG_OFFSET) + 1; dev_dbg(i2c->adap.dev.parent, - "%s entry, bytes in fifo: %d, msg: %d, SR: 0x%x, CR: 0x%x\n", + "%s entry, bytes in fifo: %d, rem: %d, SR: 0x%x, CR: 0x%x\n", __func__, bytes_in_fifo, xiic_rx_space(i2c), xiic_getreg8(i2c, XIIC_SR_REG_OFFSET), xiic_getreg8(i2c, XIIC_CR_REG_OFFSET)); @@ -325,13 +351,53 @@ static void xiic_read_rx(struct xiic_i2c *i2c) if (bytes_in_fifo > xiic_rx_space(i2c)) bytes_in_fifo = xiic_rx_space(i2c); - for (i = 0; i < bytes_in_fifo; i++) + bytes_to_read = bytes_in_fifo; + + if (!i2c->dynamic) { + bytes_rem = xiic_rx_space(i2c) - bytes_in_fifo; + + if (bytes_rem > IIC_RX_FIFO_DEPTH) { + bytes_to_read = bytes_in_fifo; + } else if (bytes_rem > 1) { + bytes_to_read = bytes_rem - 1; + } else if (bytes_rem == 1) { + bytes_to_read = 1; + /* Set NACK in CR to indicate slave transmitter */ + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr | + XIIC_CR_NO_ACK_MASK); + } else if (bytes_rem == 0) { + bytes_to_read = bytes_in_fifo; + + /* Generate stop on the bus if it is last message */ + if (i2c->nmsgs == 1) { + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr & + ~XIIC_CR_MSMS_MASK); + } + + /* Make TXACK=0, clean up for next transaction */ + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr & + ~XIIC_CR_NO_ACK_MASK); + } + } + + /* Read the fifo */ + for (i = 0; i < bytes_to_read; i++) { i2c->rx_msg->buf[i2c->rx_pos++] = xiic_getreg8(i2c, XIIC_DRR_REG_OFFSET); + } - xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, - (xiic_rx_space(i2c) > IIC_RX_FIFO_DEPTH) ? - IIC_RX_FIFO_DEPTH - 1 : xiic_rx_space(i2c) - 1); + if (i2c->dynamic) { + u8 bytes; + + /* Receive remaining bytes if less than fifo depth */ + bytes = min_t(u8, xiic_rx_space(i2c), IIC_RX_FIFO_DEPTH); + bytes--; + + xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, bytes); + } } static int xiic_tx_fifo_space(struct xiic_i2c *i2c) @@ -361,6 +427,62 @@ static void xiic_fill_tx_fifo(struct xiic_i2c *i2c) } } +static void xiic_std_fill_tx_fifo(struct xiic_i2c *i2c) +{ + u8 fifo_space = xiic_tx_fifo_space(i2c); + u16 data = 0; + int len = xiic_tx_space(i2c); + + dev_dbg(i2c->adap.dev.parent, "%s entry, len: %d, fifo space: %d\n", + __func__, len, fifo_space); + + if (len > fifo_space) + len = fifo_space; + else if (len && !(i2c->nmsgs > 1)) + len--; + + while (len--) { + data = i2c->tx_msg->buf[i2c->tx_pos++]; + xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data); + } +} + +static void xiic_send_tx(struct xiic_i2c *i2c) +{ + dev_dbg(i2c->adap.dev.parent, + "%s entry, rem: %d, SR: 0x%x, CR: 0x%x\n", + __func__, xiic_tx_space(i2c), + xiic_getreg8(i2c, XIIC_SR_REG_OFFSET), + xiic_getreg8(i2c, XIIC_CR_REG_OFFSET)); + + if (xiic_tx_space(i2c) > 1) { + xiic_std_fill_tx_fifo(i2c); + return; + } + + if ((xiic_tx_space(i2c) == 1)) { + u16 data; + + if (i2c->nmsgs == 1) { + u8 cr; + int status; + + /* Wait till FIFO is empty so STOP is sent last */ + status = xiic_wait_tx_empty(i2c); + if (status) + return; + + /* Write to CR to stop */ + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr & + ~XIIC_CR_MSMS_MASK); + } + /* Send last byte */ + data = i2c->tx_msg->buf[i2c->tx_pos++]; + xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data); + } +} + static void xiic_wakeup(struct xiic_i2c *i2c, int code) { i2c->tx_msg = NULL; @@ -391,7 +513,9 @@ static irqreturn_t xiic_process(int irq, void *dev_id) dev_dbg(i2c->adap.dev.parent, "%s: SR: 0x%x, msg: %p, nmsgs: %d\n", __func__, xiic_getreg8(i2c, XIIC_SR_REG_OFFSET), i2c->tx_msg, i2c->nmsgs); - + dev_dbg(i2c->adap.dev.parent, "%s, ISR: 0x%x, CR: 0x%x\n", + __func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET), + xiic_getreg8(i2c, XIIC_CR_REG_OFFSET)); /* Service requesting interrupt */ if ((pend & XIIC_INTR_ARB_LOST_MASK) || @@ -465,7 +589,10 @@ static irqreturn_t xiic_process(int irq, void *dev_id) goto out; } - xiic_fill_tx_fifo(i2c); + if (i2c->dynamic) + xiic_fill_tx_fifo(i2c); + else + xiic_send_tx(i2c); /* current message sent and there is space in the fifo */ if (!xiic_tx_space(i2c) && xiic_tx_fifo_space(i2c) >= 2) { @@ -554,35 +681,113 @@ static int xiic_busy(struct xiic_i2c *i2c) static void xiic_start_recv(struct xiic_i2c *i2c) { - u8 rx_watermark; + u16 rx_watermark; + u8 cr = 0, rfd_set = 0; struct i2c_msg *msg = i2c->rx_msg = i2c->tx_msg; unsigned long flags; - /* Clear and enable Rx full interrupt. */ - xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK | XIIC_INTR_TX_ERROR_MASK); + dev_dbg(i2c->adap.dev.parent, "%s entry, ISR: 0x%x, CR: 0x%x\n", + __func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET), + xiic_getreg8(i2c, XIIC_CR_REG_OFFSET)); + + /* Disable Tx interrupts */ + xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK | XIIC_INTR_TX_EMPTY_MASK); - /* we want to get all but last byte, because the TX_ERROR IRQ is used - * to inidicate error ACK on the address, and negative ack on the last - * received byte, so to not mix them receive all but last. - * In the case where there is only one byte to receive - * we can check if ERROR and RX full is set at the same time - */ - rx_watermark = msg->len; - if (rx_watermark > IIC_RX_FIFO_DEPTH) - rx_watermark = IIC_RX_FIFO_DEPTH; - xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rx_watermark - 1); - - local_irq_save(flags); - if (!(msg->flags & I2C_M_NOSTART)) - /* write the address */ + if (i2c->dynamic) { + u8 bytes; + u16 val; + + /* Clear and enable Rx full interrupt. */ + xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK | + XIIC_INTR_TX_ERROR_MASK); + + /* + * We want to get all but last byte, because the TX_ERROR IRQ + * is used to indicate error ACK on the address, and + * negative ack on the last received byte, so to not mix + * them receive all but last. + * In the case where there is only one byte to receive + * we can check if ERROR and RX full is set at the same time + */ + rx_watermark = msg->len; + bytes = min_t(u8, rx_watermark, IIC_RX_FIFO_DEPTH); + bytes--; + + xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, bytes); + + local_irq_save(flags); + if (!(msg->flags & I2C_M_NOSTART)) + /* write the address */ + xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, + i2c_8bit_addr_from_msg(msg) | + XIIC_TX_DYN_START_MASK); + + xiic_irq_clr_en(i2c, XIIC_INTR_BNB_MASK); + + /* If last message, include dynamic stop bit with length */ + val = (i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0; + val |= msg->len; + + xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, val); + local_irq_restore(flags); + } else { + /* + * If previous message is Tx, make sure that Tx FIFO is empty + * before starting a new transfer as the repeated start in + * standard mode can corrupt the transaction if there are + * still bytes to be transmitted in FIFO + */ + if (i2c->prev_msg_tx) { + int status; + + status = xiic_wait_tx_empty(i2c); + if (status) + return; + } + + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); + + /* Set Receive fifo depth */ + rx_watermark = msg->len; + if (rx_watermark > IIC_RX_FIFO_DEPTH) { + rfd_set = IIC_RX_FIFO_DEPTH - 1; + } else if ((rx_watermark == 1) || (rx_watermark == 0)) { + rfd_set = rx_watermark - 1; + /* Handle single byte transfer separately */ + cr |= XIIC_CR_NO_ACK_MASK; + } else { + rfd_set = rx_watermark - 2; + } + + /* Check if RSTA should be set */ + if (cr & XIIC_CR_MSMS_MASK) { + /* Already a master, RSTA should be set */ + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr | + XIIC_CR_REPEATED_START_MASK) & + ~(XIIC_CR_DIR_IS_TX_MASK)); + } + + xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rfd_set); + + /* Clear and enable Rx full and transmit complete interrupts */ + xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK | + XIIC_INTR_TX_ERROR_MASK); + + /* Write the address */ xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, - i2c_8bit_addr_from_msg(msg) | XIIC_TX_DYN_START_MASK); + i2c_8bit_addr_from_msg(msg)); - xiic_irq_clr_en(i2c, XIIC_INTR_BNB_MASK); + /* Write to Control Register,to start transaction in Rx mode */ + if ((cr & XIIC_CR_MSMS_MASK) == 0) { + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr | + XIIC_CR_MSMS_MASK) + & ~(XIIC_CR_DIR_IS_TX_MASK)); + } - xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, - msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0)); - local_irq_restore(flags); + dev_dbg(i2c->adap.dev.parent, "%s end, ISR: 0x%x, CR: 0x%x\n", + __func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET), + xiic_getreg8(i2c, XIIC_CR_REG_OFFSET)); + } if (i2c->nmsgs == 1) /* very last, enable bus not busy as well */ @@ -590,10 +795,17 @@ static void xiic_start_recv(struct xiic_i2c *i2c) /* the message is tx:ed */ i2c->tx_pos = msg->len; + + /* Enable interrupts */ + xiic_setreg32(i2c, XIIC_DGIER_OFFSET, XIIC_GINTR_ENABLE_MASK); + + i2c->prev_msg_tx = false; } static void xiic_start_send(struct xiic_i2c *i2c) { + u8 cr = 0; + u16 data; struct i2c_msg *msg = i2c->tx_msg; xiic_irq_clr(i2c, XIIC_INTR_TX_ERROR_MASK); @@ -604,22 +816,71 @@ static void xiic_start_send(struct xiic_i2c *i2c) __func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET), xiic_getreg8(i2c, XIIC_CR_REG_OFFSET)); - if (!(msg->flags & I2C_M_NOSTART)) { - /* write the address */ - u16 data = i2c_8bit_addr_from_msg(msg) | - XIIC_TX_DYN_START_MASK; - if ((i2c->nmsgs == 1) && msg->len == 0) - /* no data and last message -> add STOP */ - data |= XIIC_TX_DYN_STOP_MASK; + if (i2c->dynamic) { + if (!(msg->flags & I2C_M_NOSTART)) { + /* write the address */ + data = i2c_8bit_addr_from_msg(msg) | + XIIC_TX_DYN_START_MASK; + + if (i2c->nmsgs == 1 && msg->len == 0) + /* no data and last message -> add STOP */ + data |= XIIC_TX_DYN_STOP_MASK; + + xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data); + } + + xiic_fill_tx_fifo(i2c); + + /* Clear any pending Tx empty, Tx Error and then enable them */ + xiic_irq_clr_en(i2c, (XIIC_INTR_TX_EMPTY_MASK | + XIIC_INTR_TX_ERROR_MASK | + XIIC_INTR_BNB_MASK)); + } else { + /* + * If previous message is Tx, make sure that Tx FIFO is empty + * before starting a new transfer as the repeated start in + * standard mode can corrupt the transaction if there are + * still bytes to be transmitted in FIFO + */ + if (i2c->prev_msg_tx) { + int status; + + status = xiic_wait_tx_empty(i2c); + if (status) + return; + } + /* Check if RSTA should be set */ + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); + if (cr & XIIC_CR_MSMS_MASK) { + /* Already a master, RSTA should be set */ + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr | + XIIC_CR_REPEATED_START_MASK | + XIIC_CR_DIR_IS_TX_MASK) & + ~(XIIC_CR_NO_ACK_MASK)); + } + + /* Write address to FIFO */ + data = i2c_8bit_addr_from_msg(msg); xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data); - } + /* Fill fifo */ + xiic_std_fill_tx_fifo(i2c); - xiic_fill_tx_fifo(i2c); + if ((cr & XIIC_CR_MSMS_MASK) == 0) { - /* Clear any pending Tx empty, Tx Error and then enable them. */ - xiic_irq_clr_en(i2c, XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_ERROR_MASK | - XIIC_INTR_BNB_MASK); + /* Start Tx by writing to CR */ + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, cr | + XIIC_CR_MSMS_MASK | + XIIC_CR_DIR_IS_TX_MASK); + } + + /* Clear any pending Tx empty, Tx Error and then enable them */ + xiic_irq_clr_en(i2c, XIIC_INTR_TX_EMPTY_MASK | + XIIC_INTR_TX_ERROR_MASK | + XIIC_INTR_BNB_MASK); + } + i2c->prev_msg_tx = true; } static irqreturn_t xiic_isr(int irq, void *dev_id) @@ -703,7 +964,7 @@ static int xiic_start_xfer(struct xiic_i2c *i2c) static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) { struct xiic_i2c *i2c = i2c_get_adapdata(adap); - int err; + int err, count; dev_dbg(adap->dev.parent, "%s entry SR: 0x%x\n", __func__, xiic_getreg8(i2c, XIIC_SR_REG_OFFSET)); @@ -719,6 +980,21 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) i2c->tx_msg = msgs; i2c->nmsgs = num; + /* Decide standard mode or Dynamic mode */ + i2c->dynamic = true; + + /* Initialize prev message type */ + i2c->prev_msg_tx = false; + + /* Enter standard mode only when read length is > 255 bytes */ + for (count = 0; count < i2c->nmsgs; count++) { + if ((i2c->tx_msg[count].flags & I2C_M_RD) && + i2c->tx_msg[count].len > MAX_READ_LENGTH_DYNAMIC) { + i2c->dynamic = false; + break; + } + } + err = xiic_start_xfer(i2c); if (err < 0) { dev_err(adap->dev.parent, "Error xiic_start_xfer\n"); @@ -752,16 +1028,11 @@ static const struct i2c_algorithm xiic_algorithm = { .functionality = xiic_func, }; -static const struct i2c_adapter_quirks xiic_quirks = { - .max_read_len = 255, -}; - static const struct i2c_adapter xiic_adapter = { .owner = THIS_MODULE, .name = DRIVER_NAME, .class = I2C_CLASS_DEPRECATED, .algo = &xiic_algorithm, - .quirks = &xiic_quirks, };
In the current driver implementation, there is a limit of read transfer size to 255 bytes as it is using AXI I2C dynamic mode. But the IP supports this transfer through standard mode. So added AXI I2C standard mode support to enable read transfers of size more than 255 bytes. The driver scans through the message request from user space and selects AXI I2C standard mode if there is a read request of more than 255 bytes. Then the whole message goes through standard mode Tx and Rx paths. Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com> --- drivers/i2c/busses/i2c-xiic.c | 367 +++++++++++++++++++++++++++++----- 1 file changed, 319 insertions(+), 48 deletions(-)