| Message ID | 1656072327-13628-2-git-send-email-manikanta.guntupalli@xilinx.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: xiic: Added Standard mode and SMBus | expand |
W dniu 24.06.2022 o 14:05, Manikanta Guntupalli pisze: [...] > +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->repeated_start)) > + 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. [...] > @@ -579,31 +681,99 @@ static int xiic_busy(struct xiic_i2c *i2c) > static void xiic_start_recv(struct xiic_i2c *i2c) > { > 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)); > > - /* 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, (u8)(rx_watermark - 1)); 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. > + /* Disable Tx interrupts */ > + xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK | XIIC_INTR_TX_EMPTY_MASK); > > - 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 (i2c->dynamic) { > + u8 bytes; > + u16 val; > > - xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, > - msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0)); > + /* 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--; Again, do we really want to write 255 to RFD if msg->len == 0? > + > + 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 { > + 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? [...] Krzysztof
Hi Manikanta, W dniu 13.07.2022 o 09:54, Guntupalli, Manikanta pisze: > Hi Krzysztof, >> [...] >>> @@ -579,31 +681,99 @@ static int xiic_busy(struct xiic_i2c *i2c) >>> static void xiic_start_recv(struct xiic_i2c *i2c) >>> { >>> 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)); >>> >>> - /* 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, (u8)(rx_watermark - 1)); >> 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. >> > Here comparing and taking minimum value of msg->len and IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, while writing into register performed -1, so maximum value writing into RX_FIFO_PIRQ is 15. > I'm not sure you got my point - my point was that if the provided msg->len is 0, then rx_watermark will also be 0 and thus we will set the XIIC_RFD_REG_OFFSET to value 255 which seems illegal. >>> + /* Disable Tx interrupts */ >>> + xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK | >>> + XIIC_INTR_TX_EMPTY_MASK); >>> >>> - 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 (i2c->dynamic) { >>> + u8 bytes; >>> + u16 val; >>> >>> - xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, >>> - msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0)); >>> + /* 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--; >> Again, do we really want to write 255 to RFD if msg->len == 0? >> > Here comparing and taking minimum value of msg->len and IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, before writing into register performed decrement, so maximum value writing into RX_FIFO_PIRQ is 15. You are not correct - in case the msg->len is 0, the value written into the register will be 255 (if you decrement 0, you will get 255 in case of u8). Unless there is some check making sure msg->len is never 0, that I do not see. > >>> + >>> + 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. > For receive operation with byte count greater than 255, we will switch to standard mode else always in the dynamic mode only. > >> 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? > After deciding dynamic mode based on above logic, we need to set the DYN_START bit along with address in the TX FIFO to start the transfer. > >> 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? >> > I2C controller supports 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? >> > Below notes mentioned in I2C_M_NOSTART section of kernel documentation(link mentioned below), > " If you set the I2C_M_NOSTART variable for the first partial message, > we do not generate Addr, but we do generate the startbit S. This will > probably confuse all other clients on your bus, so don't try this." > > https://www.kernel.org/doc/Documentation/i2c/i2c-protocol > > So I2C_M_NOSTART flag need to use in second i2c_msg or in later i2c_msgs, but not in first i2c_msg. During first i2c_msg dynamic mode starts, so again no need to set start bit TX_FIFO. But what happens if somebody passes the I2C_M_NOSTART bit in the first message? There seems to be no check for that. I know the documentation says that this should be avioded but it also states what should happen (S bit is generated by no address is sent) and as far as I understand this is not what will happen in i2c-xiic. What will happen is that the address *will* be generated and it will be the first byte from the buf. > >> 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? > Since it supports I2C_M_NOSTART flag, need to keep code handling flag. But if it supports it, that should be annouced in xiic_func. > >>> + >>> + 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 { >>> + 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? > Here comparing and taking minimum value of msg->len and IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, before writing into register performed -1, so maximum value writing into RX_FIFO_PIRQ is 15. > Again, only if msg->len is not 0, which is not ensured. Krzysztof
Hi Krzysztof, > -----Original Message----- > From: Krzysztof Adamski <krzysztof.adamski@nokia.com> > Sent: Wednesday, July 20, 2022 3:56 PM > To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; Manikanta > Guntupalli <manikanta.guntupalli@xilinx.com>; michal.simek@xilinx.com; > Simek, Michal <michal.simek@amd.com>; linux-arm- > kernel@lists.infradead.org; linux-i2c@vger.kernel.org; linux- > kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com> > Cc: Raviteja Narayanam <raviteja.narayanam@xilinx.com> > Subject: Re: [PATCH 01/12] i2c: xiic: Add standard mode support for > 255 > byte read transfers > > Hi Manikanta, > > W dniu 13.07.2022 o 09:54, Guntupalli, Manikanta pisze: > > Hi Krzysztof, > >> [...] > >>> @@ -579,31 +681,99 @@ static int xiic_busy(struct xiic_i2c *i2c) > >>> static void xiic_start_recv(struct xiic_i2c *i2c) > >>> { > >>> 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)); > >>> > >>> - /* 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, (u8)(rx_watermark - 1)); > >> 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. > >> > > Here comparing and taking minimum value of msg->len and > IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, while writing > into register performed -1, so maximum value writing into RX_FIFO_PIRQ is > 15. > > > I'm not sure you got my point - my point was that if the provided > msg->len is 0, then rx_watermark will also be 0 and thus we will set the > XIIC_RFD_REG_OFFSET to value 255 which seems illegal. We will fix in V2. > >>> + /* Disable Tx interrupts */ > >>> + xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK | > >>> + XIIC_INTR_TX_EMPTY_MASK); > >>> > >>> - 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 (i2c->dynamic) { > >>> + u8 bytes; > >>> + u16 val; > >>> > >>> - xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, > >>> - msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0)); > >>> + /* 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--; > >> Again, do we really want to write 255 to RFD if msg->len == 0? > >> > > Here comparing and taking minimum value of msg->len and > IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, before writing > into register performed decrement, so maximum value writing into > RX_FIFO_PIRQ is 15. > You are not correct - in case the msg->len is 0, the value written into the > register will be 255 (if you decrement 0, you will get 255 in case of u8). Unless > there is some check making sure msg->len is never 0, that I do not see. Agreed, we will fix in V2. > > > >>> + > >>> + 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. > > For receive operation with byte count greater than 255, we will switch to > standard mode else always in the dynamic mode only. > > > >> 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? > > After deciding dynamic mode based on above logic, we need to set the > DYN_START bit along with address in the TX FIFO to start the transfer. > > > >> 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? > >> > > I2C controller supports 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? > >> > > Below notes mentioned in I2C_M_NOSTART section of kernel > > documentation(link mentioned below), " If you set the I2C_M_NOSTART > variable for the first partial message, > > we do not generate Addr, but we do generate the startbit S. This will > > probably confuse all other clients on your bus, so don't try this." > > > > https://www.kernel.org/doc/Documentation/i2c/i2c-protocol > > > > So I2C_M_NOSTART flag need to use in second i2c_msg or in later > i2c_msgs, but not in first i2c_msg. During first i2c_msg dynamic mode starts, > so again no need to set start bit TX_FIFO. > > But what happens if somebody passes the I2C_M_NOSTART bit in the first > message? There seems to be no check for that. I know the documentation > says that this should be avioded but it also states what should happen (S bit > is generated by no address is sent) and as far as I understand this is not what > will happen in i2c-xiic. What will happen is that the address *will* be > generated and it will be the first byte from the buf. > As per kernel documentation it's recommended to use I2C_M_NOSTART flag from second message onwards, but not mandatory. Dynamic mode won't start if someone passes I2C_M_NOSTART flag in first message. > > > >> 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? > > Since it supports I2C_M_NOSTART flag, need to keep code handling flag. > But if it supports it, that should be annouced in xiic_func. Since we are not announcing I2C_FUNC_NOSTART, we will remove I2C_M_NOSTART flag handling checks in V2. > > > >>> + > >>> + 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 { > >>> + 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? > > Here comparing and taking minimum value of msg->len and > IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, before writing > into register performed -1, so maximum value writing into RX_FIFO_PIRQ is > 15. > > > Again, only if msg->len is not 0, which is not ensured. > Agreed, we will fix in V2. > Krzysztof
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index 9a1c3f8b7048..fb2443623844 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 + * @repeated_start: Repeated start operation */ 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 repeated_start; }; #define XIIC_MSB_OFFSET 0 @@ -143,6 +147,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 @@ -316,13 +323,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)); @@ -330,13 +338,52 @@ 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); + } + + if (i2c->dynamic) { + u8 bytes; - 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); + /* 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) @@ -367,6 +414,56 @@ 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->repeated_start)) + 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; + + /* 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; @@ -401,7 +498,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) || @@ -480,7 +579,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) { @@ -579,31 +681,99 @@ static int xiic_busy(struct xiic_i2c *i2c) static void xiic_start_recv(struct xiic_i2c *i2c) { 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)); - /* 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, (u8)(rx_watermark - 1)); + /* Disable Tx interrupts */ + xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK | XIIC_INTR_TX_EMPTY_MASK); - 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 (i2c->dynamic) { + u8 bytes; + u16 val; - xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, - msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0)); + /* 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 { + 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) { + i2c->repeated_start = true; + /* 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)); + + /* Write to Control Register,to start transaction in Rx mode */ + if ((cr & XIIC_CR_MSMS_MASK) == 0) { + i2c->repeated_start = false; + xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, (cr | + XIIC_CR_MSMS_MASK) + & ~(XIIC_CR_DIR_IS_TX_MASK)); + } + 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 */ @@ -611,10 +781,15 @@ 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); } static void xiic_start_send(struct xiic_i2c *i2c) { + u8 cr = 0; + u16 data; struct i2c_msg *msg = i2c->tx_msg; dev_dbg(i2c->adap.dev.parent, "%s entry, msg: %p, len: %d", @@ -623,24 +798,61 @@ 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); + } + + /* 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->nmsgs > 1 || xiic_tx_space(i2c)) ? + XIIC_INTR_TX_HALF_MASK : 0)); + + xiic_fill_tx_fifo(i2c); + } else { + /* Check if RSTA should be set */ + cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET); + if (cr & XIIC_CR_MSMS_MASK) { + i2c->repeated_start = true; + /* 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); - } - /* 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->nmsgs > 1 || xiic_tx_space(i2c)) ? - XIIC_INTR_TX_HALF_MASK : 0)); + /* Fill fifo */ + xiic_std_fill_tx_fifo(i2c); + + if ((cr & XIIC_CR_MSMS_MASK) == 0) { + i2c->repeated_start = false; + + /* 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); + } - 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); + } } static void __xiic_start_xfer(struct xiic_i2c *i2c) @@ -701,6 +913,33 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) if (err < 0) return err; + /* Decide standard mode or Dynamic mode */ + i2c->dynamic = true; + + /* + * If number of messages is 1 and read length is > 255 bytes, + * enter standard mode + */ + + if (i2c->nmsgs == 1 && (i2c->tx_msg->flags & I2C_M_RD) && + i2c->tx_msg->len > MAX_READ_LENGTH_DYNAMIC) { + i2c->dynamic = false; + } else if (i2c->nmsgs > 1) { + int count; + + /* + * If number of messages is more than 1 and one of them is + * a read message, enter standard mode. Since repeated start + * operation in dynamic mode read is not happenning + */ + for (count = 0; count < i2c->nmsgs; count++) { + if (i2c->tx_msg[count].flags & I2C_M_RD) { + i2c->dynamic = false; + break; + } + } + } + err = xiic_start_xfer(i2c, msgs, num); if (err < 0) { dev_err(adap->dev.parent, "Error xiic_start_xfer\n"); @@ -737,15 +976,10 @@ 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, .class = I2C_CLASS_DEPRECATED, .algo = &xiic_algorithm, - .quirks = &xiic_quirks, }; static int xiic_i2c_probe(struct platform_device *pdev)