* [PATCH v4 0/2] Tegra I2C DMA and SMBus blockread updates @ 2023-03-24 11:59 Akhil R 2023-03-24 11:59 ` [PATCH v4 1/2] i2c: tegra: Fix PEC support for SMBUS block read Akhil R 2023-03-24 11:59 ` [PATCH v4 2/2] i2c: tegra: Share same DMA channel for RX and TX Akhil R 0 siblings, 2 replies; 8+ messages in thread From: Akhil R @ 2023-03-24 11:59 UTC (permalink / raw) To: christian.koenig, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, sumit.semwal, thierry.reding, wsa Cc: akhilrajeev The patchset contains two independent updates to tegra-i2c driver. * Update I2C DMA channel usage to use single DMA channel instead of two different hardware channels. * Fix PEC byte issue during SMBus block read caused by mismanaged msg->len property in i2c_msg struct. v3->v4: Combine the two patches to one series to avoid merge conflicts. Adding the latest independent version links below. https://lkml.kernel.org/lkml/ZBxZlNOhLyUZi1B+@orome/T/#mb41b90701e70d9663d54455c7e014a528c589284 https://lkml.kernel.org/lkml/20230322131037.53805-1-akhilrajeev@nvidia.com/T/#u Akhil R (2): i2c: tegra: Fix PEC support for SMBUS block read i2c: tegra: Share same DMA channel for RX and TX drivers/i2c/busses/i2c-tegra.c | 107 ++++++++++++++------------------- 1 file changed, 46 insertions(+), 61 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 1/2] i2c: tegra: Fix PEC support for SMBUS block read 2023-03-24 11:59 [PATCH v4 0/2] Tegra I2C DMA and SMBus blockread updates Akhil R @ 2023-03-24 11:59 ` Akhil R 2023-04-05 12:25 ` Thierry Reding 2023-04-13 9:40 ` Dmitry Osipenko 2023-03-24 11:59 ` [PATCH v4 2/2] i2c: tegra: Share same DMA channel for RX and TX Akhil R 1 sibling, 2 replies; 8+ messages in thread From: Akhil R @ 2023-03-24 11:59 UTC (permalink / raw) To: christian.koenig, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, sumit.semwal, thierry.reding, wsa Cc: akhilrajeev Update the msg->len value correctly for SMBUS block read. The discrepancy went unnoticed as msg->len is used in SMBUS transfers only when a PEC byte is added. Fixes: d7583c8a5748 ("i2c: tegra: Add SMBus block read function") Signed-off-by: Akhil R <akhilrajeev@nvidia.com> --- drivers/i2c/busses/i2c-tegra.c | 38 +++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 6aab84c8d22b..83e74b8baf67 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -245,6 +245,7 @@ struct tegra_i2c_hw_feature { * @msg_err: error code for completed message * @msg_buf: pointer to current message data * @msg_buf_remaining: size of unsent data in the message buffer + * @msg_len: length of message in current transfer * @msg_read: indicates that the transfer is a read access * @timings: i2c timings information like bus frequency * @multimaster_mode: indicates that I2C controller is in multi-master mode @@ -279,6 +280,7 @@ struct tegra_i2c_dev { size_t msg_buf_remaining; int msg_err; u8 *msg_buf; + __u16 msg_len; struct completion dma_complete; struct dma_chan *tx_dma_chan; @@ -1169,7 +1171,7 @@ static void tegra_i2c_push_packet_header(struct tegra_i2c_dev *i2c_dev, else i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO); - packet_header = msg->len - 1; + packet_header = i2c_dev->msg_len - 1; if (i2c_dev->dma_mode && !i2c_dev->msg_read) *dma_buf++ = packet_header; @@ -1242,20 +1244,32 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, return err; i2c_dev->msg_buf = msg->buf; + i2c_dev->msg_len = msg->len; - /* The condition true implies smbus block read and len is already read */ - if (msg->flags & I2C_M_RECV_LEN && end_state != MSG_END_CONTINUE) - i2c_dev->msg_buf = msg->buf + 1; - - i2c_dev->msg_buf_remaining = msg->len; i2c_dev->msg_err = I2C_ERR_NONE; i2c_dev->msg_read = !!(msg->flags & I2C_M_RD); reinit_completion(&i2c_dev->msg_complete); + /* * + * For SMBUS block read command, read only 1 byte in the first transfer. + * Adjust that 1 byte for the next transfer in the msg buffer and msg + * length. + */ + if (msg->flags & I2C_M_RECV_LEN) { + if (end_state == MSG_END_CONTINUE) { + i2c_dev->msg_len = 1; + } else { + i2c_dev->msg_buf += 1; + i2c_dev->msg_len -= 1; + } + } + + i2c_dev->msg_buf_remaining = i2c_dev->msg_len; + if (i2c_dev->msg_read) - xfer_size = msg->len; + xfer_size = i2c_dev->msg_len; else - xfer_size = msg->len + I2C_PACKET_HEADER_SIZE; + xfer_size = i2c_dev->msg_len + I2C_PACKET_HEADER_SIZE; xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD); @@ -1295,7 +1309,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, if (!i2c_dev->msg_read) { if (i2c_dev->dma_mode) { memcpy(i2c_dev->dma_buf + I2C_PACKET_HEADER_SIZE, - msg->buf, msg->len); + msg->buf, i2c_dev->msg_len); dma_sync_single_for_device(i2c_dev->dma_dev, i2c_dev->dma_phys, @@ -1352,7 +1366,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, i2c_dev->dma_phys, xfer_size, DMA_FROM_DEVICE); - memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf, msg->len); + memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf, i2c_dev->msg_len); } } @@ -1408,8 +1422,8 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], ret = tegra_i2c_xfer_msg(i2c_dev, &msgs[i], MSG_END_CONTINUE); if (ret) break; - /* Set the read byte as msg len */ - msgs[i].len = msgs[i].buf[0]; + /* Set the msg length from first byte */ + msgs[i].len += msgs[i].buf[0]; dev_dbg(i2c_dev->dev, "reading %d bytes\n", msgs[i].len); } ret = tegra_i2c_xfer_msg(i2c_dev, &msgs[i], end_type); -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v4 1/2] i2c: tegra: Fix PEC support for SMBUS block read 2023-03-24 11:59 ` [PATCH v4 1/2] i2c: tegra: Fix PEC support for SMBUS block read Akhil R @ 2023-04-05 12:25 ` Thierry Reding 2023-04-05 16:11 ` Akhil R 2023-04-13 9:40 ` Dmitry Osipenko 1 sibling, 1 reply; 8+ messages in thread From: Thierry Reding @ 2023-04-05 12:25 UTC (permalink / raw) To: Akhil R Cc: christian.koenig, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, sumit.semwal, wsa [-- Attachment #1: Type: text/plain, Size: 4557 bytes --] On Fri, Mar 24, 2023 at 05:29:23PM +0530, Akhil R wrote: > Update the msg->len value correctly for SMBUS block read. The discrepancy > went unnoticed as msg->len is used in SMBUS transfers only when a PEC > byte is added. > > Fixes: d7583c8a5748 ("i2c: tegra: Add SMBus block read function") > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > --- > drivers/i2c/busses/i2c-tegra.c | 38 +++++++++++++++++++++++----------- > 1 file changed, 26 insertions(+), 12 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index 6aab84c8d22b..83e74b8baf67 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -245,6 +245,7 @@ struct tegra_i2c_hw_feature { > * @msg_err: error code for completed message > * @msg_buf: pointer to current message data > * @msg_buf_remaining: size of unsent data in the message buffer > + * @msg_len: length of message in current transfer > * @msg_read: indicates that the transfer is a read access > * @timings: i2c timings information like bus frequency > * @multimaster_mode: indicates that I2C controller is in multi-master mode > @@ -279,6 +280,7 @@ struct tegra_i2c_dev { > size_t msg_buf_remaining; > int msg_err; > u8 *msg_buf; > + __u16 msg_len; > > struct completion dma_complete; > struct dma_chan *tx_dma_chan; > @@ -1169,7 +1171,7 @@ static void tegra_i2c_push_packet_header(struct tegra_i2c_dev *i2c_dev, > else > i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO); > > - packet_header = msg->len - 1; > + packet_header = i2c_dev->msg_len - 1; > > if (i2c_dev->dma_mode && !i2c_dev->msg_read) > *dma_buf++ = packet_header; > @@ -1242,20 +1244,32 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, > return err; > > i2c_dev->msg_buf = msg->buf; > + i2c_dev->msg_len = msg->len; > > - /* The condition true implies smbus block read and len is already read */ > - if (msg->flags & I2C_M_RECV_LEN && end_state != MSG_END_CONTINUE) > - i2c_dev->msg_buf = msg->buf + 1; > - > - i2c_dev->msg_buf_remaining = msg->len; > i2c_dev->msg_err = I2C_ERR_NONE; > i2c_dev->msg_read = !!(msg->flags & I2C_M_RD); > reinit_completion(&i2c_dev->msg_complete); > > + /* * > + * For SMBUS block read command, read only 1 byte in the first transfer. > + * Adjust that 1 byte for the next transfer in the msg buffer and msg > + * length. > + */ > + if (msg->flags & I2C_M_RECV_LEN) { > + if (end_state == MSG_END_CONTINUE) { > + i2c_dev->msg_len = 1; > + } else { > + i2c_dev->msg_buf += 1; > + i2c_dev->msg_len -= 1; > + } > + } > + > + i2c_dev->msg_buf_remaining = i2c_dev->msg_len; > + > if (i2c_dev->msg_read) > - xfer_size = msg->len; > + xfer_size = i2c_dev->msg_len; > else > - xfer_size = msg->len + I2C_PACKET_HEADER_SIZE; > + xfer_size = i2c_dev->msg_len + I2C_PACKET_HEADER_SIZE; > > xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD); > > @@ -1295,7 +1309,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, > if (!i2c_dev->msg_read) { > if (i2c_dev->dma_mode) { > memcpy(i2c_dev->dma_buf + I2C_PACKET_HEADER_SIZE, > - msg->buf, msg->len); > + msg->buf, i2c_dev->msg_len); > > dma_sync_single_for_device(i2c_dev->dma_dev, > i2c_dev->dma_phys, > @@ -1352,7 +1366,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, > i2c_dev->dma_phys, > xfer_size, DMA_FROM_DEVICE); > > - memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf, msg->len); > + memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf, i2c_dev->msg_len); > } > } > > @@ -1408,8 +1422,8 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], > ret = tegra_i2c_xfer_msg(i2c_dev, &msgs[i], MSG_END_CONTINUE); > if (ret) > break; > - /* Set the read byte as msg len */ > - msgs[i].len = msgs[i].buf[0]; > + /* Set the msg length from first byte */ > + msgs[i].len += msgs[i].buf[0]; I'm having trouble understanding why this change is needed. msg->len is never changed in tegra_i2c_xfer_msg(), as far as I can tell, so it would contain whatever was in it for the previous transfer. But since we want to read the message length from the first byte, wouldn't the assignment (i.e. the old code) be the right way to do that? If we add the length from the first byte, we could potentially be reading more than whan the first byte indicated. What am I missing? Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v4 1/2] i2c: tegra: Fix PEC support for SMBUS block read 2023-04-05 12:25 ` Thierry Reding @ 2023-04-05 16:11 ` Akhil R 2023-04-13 8:27 ` Thierry Reding 0 siblings, 1 reply; 8+ messages in thread From: Akhil R @ 2023-04-05 16:11 UTC (permalink / raw) To: Thierry Reding Cc: christian.koenig, digetx, Jonathan Hunter, Laxman Dewangan, linux-i2c, linux-kernel, linux-tegra, sumit.semwal, wsa > On Fri, Mar 24, 2023 at 05:29:23PM +0530, Akhil R wrote: > > Update the msg->len value correctly for SMBUS block read. The discrepancy > > went unnoticed as msg->len is used in SMBUS transfers only when a PEC > > byte is added. > > > > Fixes: d7583c8a5748 ("i2c: tegra: Add SMBus block read function") > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > > --- > > drivers/i2c/busses/i2c-tegra.c | 38 +++++++++++++++++++++++----------- > > 1 file changed, 26 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > > index 6aab84c8d22b..83e74b8baf67 100644 > > --- a/drivers/i2c/busses/i2c-tegra.c > > +++ b/drivers/i2c/busses/i2c-tegra.c > > @@ -245,6 +245,7 @@ struct tegra_i2c_hw_feature { > > * @msg_err: error code for completed message > > * @msg_buf: pointer to current message data > > * @msg_buf_remaining: size of unsent data in the message buffer > > + * @msg_len: length of message in current transfer > > * @msg_read: indicates that the transfer is a read access > > * @timings: i2c timings information like bus frequency > > * @multimaster_mode: indicates that I2C controller is in multi-master > mode > > @@ -279,6 +280,7 @@ struct tegra_i2c_dev { > > size_t msg_buf_remaining; > > int msg_err; > > u8 *msg_buf; > > + __u16 msg_len; > > > > struct completion dma_complete; > > struct dma_chan *tx_dma_chan; > > @@ -1169,7 +1171,7 @@ static void tegra_i2c_push_packet_header(struct > tegra_i2c_dev *i2c_dev, > > else > > i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO); > > > > - packet_header = msg->len - 1; > > + packet_header = i2c_dev->msg_len - 1; > > > > if (i2c_dev->dma_mode && !i2c_dev->msg_read) > > *dma_buf++ = packet_header; > > @@ -1242,20 +1244,32 @@ static int tegra_i2c_xfer_msg(struct > tegra_i2c_dev *i2c_dev, > > return err; > > > > i2c_dev->msg_buf = msg->buf; > > + i2c_dev->msg_len = msg->len; > > > > - /* The condition true implies smbus block read and len is already > read */ > > - if (msg->flags & I2C_M_RECV_LEN && end_state != > MSG_END_CONTINUE) > > - i2c_dev->msg_buf = msg->buf + 1; > > - > > - i2c_dev->msg_buf_remaining = msg->len; > > i2c_dev->msg_err = I2C_ERR_NONE; > > i2c_dev->msg_read = !!(msg->flags & I2C_M_RD); > > reinit_completion(&i2c_dev->msg_complete); > > > > + /* * > > + * For SMBUS block read command, read only 1 byte in the first > transfer. > > + * Adjust that 1 byte for the next transfer in the msg buffer and msg > > + * length. > > + */ > > + if (msg->flags & I2C_M_RECV_LEN) { > > + if (end_state == MSG_END_CONTINUE) { > > + i2c_dev->msg_len = 1; > > + } else { > > + i2c_dev->msg_buf += 1; > > + i2c_dev->msg_len -= 1; > > + } > > + } > > + > > + i2c_dev->msg_buf_remaining = i2c_dev->msg_len; > > + > > if (i2c_dev->msg_read) > > - xfer_size = msg->len; > > + xfer_size = i2c_dev->msg_len; > > else > > - xfer_size = msg->len + I2C_PACKET_HEADER_SIZE; > > + xfer_size = i2c_dev->msg_len + I2C_PACKET_HEADER_SIZE; > > > > xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD); > > > > @@ -1295,7 +1309,7 @@ static int tegra_i2c_xfer_msg(struct > tegra_i2c_dev *i2c_dev, > > if (!i2c_dev->msg_read) { > > if (i2c_dev->dma_mode) { > > memcpy(i2c_dev->dma_buf + > I2C_PACKET_HEADER_SIZE, > > - msg->buf, msg->len); > > + msg->buf, i2c_dev->msg_len); > > > > dma_sync_single_for_device(i2c_dev->dma_dev, > > i2c_dev->dma_phys, > > @@ -1352,7 +1366,7 @@ static int tegra_i2c_xfer_msg(struct > tegra_i2c_dev *i2c_dev, > > i2c_dev->dma_phys, > > xfer_size, > DMA_FROM_DEVICE); > > > > - memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf, msg- > >len); > > + memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf, > i2c_dev->msg_len); > > } > > } > > > > @@ -1408,8 +1422,8 @@ static int tegra_i2c_xfer(struct i2c_adapter > *adap, struct i2c_msg msgs[], > > ret = tegra_i2c_xfer_msg(i2c_dev, &msgs[i], > MSG_END_CONTINUE); > > if (ret) > > break; > > - /* Set the read byte as msg len */ > > - msgs[i].len = msgs[i].buf[0]; > > + /* Set the msg length from first byte */ > > + msgs[i].len += msgs[i].buf[0]; > > I'm having trouble understanding why this change is needed. msg->len is > never changed in tegra_i2c_xfer_msg(), as far as I can tell, so it would > contain whatever was in it for the previous transfer. But since we want > to read the message length from the first byte, wouldn't the assignment > (i.e. the old code) be the right way to do that? If we add the length > from the first byte, we could potentially be reading more than whan the > first byte indicated. > > What am I missing? > The value in the first byte will contain only the number of bytes to read further. The value excludes the first byte as well as the PEC byte. The function i2c_smbus_xfer_emulated(), in file i2c-core-smbus.c, increments msg->len based on 'wants_pec'. Other functions, specifically the function i2c_smbus_check_pec() expects msg->len to be the number of bytes of data + first length byte + PEC byte. To avoid reading more bytes, I added another parameter ' i2c_dev->msg_len' which will contain the exact number of bytes to read in the current xfer_msg(). Regards, Akhil ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 1/2] i2c: tegra: Fix PEC support for SMBUS block read 2023-04-05 16:11 ` Akhil R @ 2023-04-13 8:27 ` Thierry Reding 0 siblings, 0 replies; 8+ messages in thread From: Thierry Reding @ 2023-04-13 8:27 UTC (permalink / raw) To: Akhil R Cc: christian.koenig, digetx, Jonathan Hunter, Laxman Dewangan, linux-i2c, linux-kernel, linux-tegra, sumit.semwal, wsa [-- Attachment #1: Type: text/plain, Size: 5776 bytes --] On Wed, Apr 05, 2023 at 04:11:31PM +0000, Akhil R wrote: > > On Fri, Mar 24, 2023 at 05:29:23PM +0530, Akhil R wrote: > > > Update the msg->len value correctly for SMBUS block read. The discrepancy > > > went unnoticed as msg->len is used in SMBUS transfers only when a PEC > > > byte is added. > > > > > > Fixes: d7583c8a5748 ("i2c: tegra: Add SMBus block read function") > > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > > > --- > > > drivers/i2c/busses/i2c-tegra.c | 38 +++++++++++++++++++++++----------- > > > 1 file changed, 26 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > > > index 6aab84c8d22b..83e74b8baf67 100644 > > > --- a/drivers/i2c/busses/i2c-tegra.c > > > +++ b/drivers/i2c/busses/i2c-tegra.c > > > @@ -245,6 +245,7 @@ struct tegra_i2c_hw_feature { > > > * @msg_err: error code for completed message > > > * @msg_buf: pointer to current message data > > > * @msg_buf_remaining: size of unsent data in the message buffer > > > + * @msg_len: length of message in current transfer > > > * @msg_read: indicates that the transfer is a read access > > > * @timings: i2c timings information like bus frequency > > > * @multimaster_mode: indicates that I2C controller is in multi-master > > mode > > > @@ -279,6 +280,7 @@ struct tegra_i2c_dev { > > > size_t msg_buf_remaining; > > > int msg_err; > > > u8 *msg_buf; > > > + __u16 msg_len; > > > > > > struct completion dma_complete; > > > struct dma_chan *tx_dma_chan; > > > @@ -1169,7 +1171,7 @@ static void tegra_i2c_push_packet_header(struct > > tegra_i2c_dev *i2c_dev, > > > else > > > i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO); > > > > > > - packet_header = msg->len - 1; > > > + packet_header = i2c_dev->msg_len - 1; > > > > > > if (i2c_dev->dma_mode && !i2c_dev->msg_read) > > > *dma_buf++ = packet_header; > > > @@ -1242,20 +1244,32 @@ static int tegra_i2c_xfer_msg(struct > > tegra_i2c_dev *i2c_dev, > > > return err; > > > > > > i2c_dev->msg_buf = msg->buf; > > > + i2c_dev->msg_len = msg->len; > > > > > > - /* The condition true implies smbus block read and len is already > > read */ > > > - if (msg->flags & I2C_M_RECV_LEN && end_state != > > MSG_END_CONTINUE) > > > - i2c_dev->msg_buf = msg->buf + 1; > > > - > > > - i2c_dev->msg_buf_remaining = msg->len; > > > i2c_dev->msg_err = I2C_ERR_NONE; > > > i2c_dev->msg_read = !!(msg->flags & I2C_M_RD); > > > reinit_completion(&i2c_dev->msg_complete); > > > > > > + /* * > > > + * For SMBUS block read command, read only 1 byte in the first > > transfer. > > > + * Adjust that 1 byte for the next transfer in the msg buffer and msg > > > + * length. > > > + */ > > > + if (msg->flags & I2C_M_RECV_LEN) { > > > + if (end_state == MSG_END_CONTINUE) { > > > + i2c_dev->msg_len = 1; > > > + } else { > > > + i2c_dev->msg_buf += 1; > > > + i2c_dev->msg_len -= 1; > > > + } > > > + } > > > + > > > + i2c_dev->msg_buf_remaining = i2c_dev->msg_len; > > > + > > > if (i2c_dev->msg_read) > > > - xfer_size = msg->len; > > > + xfer_size = i2c_dev->msg_len; > > > else > > > - xfer_size = msg->len + I2C_PACKET_HEADER_SIZE; > > > + xfer_size = i2c_dev->msg_len + I2C_PACKET_HEADER_SIZE; > > > > > > xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD); > > > > > > @@ -1295,7 +1309,7 @@ static int tegra_i2c_xfer_msg(struct > > tegra_i2c_dev *i2c_dev, > > > if (!i2c_dev->msg_read) { > > > if (i2c_dev->dma_mode) { > > > memcpy(i2c_dev->dma_buf + > > I2C_PACKET_HEADER_SIZE, > > > - msg->buf, msg->len); > > > + msg->buf, i2c_dev->msg_len); > > > > > > dma_sync_single_for_device(i2c_dev->dma_dev, > > > i2c_dev->dma_phys, > > > @@ -1352,7 +1366,7 @@ static int tegra_i2c_xfer_msg(struct > > tegra_i2c_dev *i2c_dev, > > > i2c_dev->dma_phys, > > > xfer_size, > > DMA_FROM_DEVICE); > > > > > > - memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf, msg- > > >len); > > > + memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf, > > i2c_dev->msg_len); > > > } > > > } > > > > > > @@ -1408,8 +1422,8 @@ static int tegra_i2c_xfer(struct i2c_adapter > > *adap, struct i2c_msg msgs[], > > > ret = tegra_i2c_xfer_msg(i2c_dev, &msgs[i], > > MSG_END_CONTINUE); > > > if (ret) > > > break; > > > - /* Set the read byte as msg len */ > > > - msgs[i].len = msgs[i].buf[0]; > > > + /* Set the msg length from first byte */ > > > + msgs[i].len += msgs[i].buf[0]; > > > > I'm having trouble understanding why this change is needed. msg->len is > > never changed in tegra_i2c_xfer_msg(), as far as I can tell, so it would > > contain whatever was in it for the previous transfer. But since we want > > to read the message length from the first byte, wouldn't the assignment > > (i.e. the old code) be the right way to do that? If we add the length > > from the first byte, we could potentially be reading more than whan the > > first byte indicated. > > > > What am I missing? > > > The value in the first byte will contain only the number of bytes to read further. > The value excludes the first byte as well as the PEC byte. > The function i2c_smbus_xfer_emulated(), in file i2c-core-smbus.c, increments > msg->len based on 'wants_pec'. Other functions, specifically the function > i2c_smbus_check_pec() expects msg->len to be the number of bytes of data + > first length byte + PEC byte. > > To avoid reading more bytes, I added another parameter ' i2c_dev->msg_len' > which will contain the exact number of bytes to read in the current xfer_msg(). Okay, sounds good: Acked-by: Thierry Reding <treding@nvidia.com> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 1/2] i2c: tegra: Fix PEC support for SMBUS block read 2023-03-24 11:59 ` [PATCH v4 1/2] i2c: tegra: Fix PEC support for SMBUS block read Akhil R 2023-04-05 12:25 ` Thierry Reding @ 2023-04-13 9:40 ` Dmitry Osipenko 1 sibling, 0 replies; 8+ messages in thread From: Dmitry Osipenko @ 2023-04-13 9:40 UTC (permalink / raw) To: Akhil R, christian.koenig, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, sumit.semwal, thierry.reding, wsa On 3/24/23 14:59, Akhil R wrote: ... > @@ -279,6 +280,7 @@ struct tegra_i2c_dev { > size_t msg_buf_remaining; > int msg_err; > u8 *msg_buf; > + __u16 msg_len; __u16 is for UAPI headers, please use unsigned int. Also keep variables sorted by string length. > struct completion dma_complete; > struct dma_chan *tx_dma_chan; > @@ -1169,7 +1171,7 @@ static void tegra_i2c_push_packet_header(struct tegra_i2c_dev *i2c_dev, > else > i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO); > > - packet_header = msg->len - 1; > + packet_header = i2c_dev->msg_len - 1; > > if (i2c_dev->dma_mode && !i2c_dev->msg_read) > *dma_buf++ = packet_header; > @@ -1242,20 +1244,32 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, > return err; > > i2c_dev->msg_buf = msg->buf; > + i2c_dev->msg_len = msg->len; > > - /* The condition true implies smbus block read and len is already read */ > - if (msg->flags & I2C_M_RECV_LEN && end_state != MSG_END_CONTINUE) > - i2c_dev->msg_buf = msg->buf + 1; > - > - i2c_dev->msg_buf_remaining = msg->len; > i2c_dev->msg_err = I2C_ERR_NONE; > i2c_dev->msg_read = !!(msg->flags & I2C_M_RD); > reinit_completion(&i2c_dev->msg_complete); > > + /* * Please correct the comment style > + * For SMBUS block read command, read only 1 byte in the first transfer. > + * Adjust that 1 byte for the next transfer in the msg buffer and msg > + * length. > + */ -- Best regards, Dmitry ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 2/2] i2c: tegra: Share same DMA channel for RX and TX 2023-03-24 11:59 [PATCH v4 0/2] Tegra I2C DMA and SMBus blockread updates Akhil R 2023-03-24 11:59 ` [PATCH v4 1/2] i2c: tegra: Fix PEC support for SMBUS block read Akhil R @ 2023-03-24 11:59 ` Akhil R 2023-04-05 12:26 ` Thierry Reding 1 sibling, 1 reply; 8+ messages in thread From: Akhil R @ 2023-03-24 11:59 UTC (permalink / raw) To: christian.koenig, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, sumit.semwal, thierry.reding, wsa Cc: akhilrajeev Allocate only one DMA channel for I2C and share it for both TX and RX instead of using two different DMA hardware channels with the same slave ID. Since I2C supports only half duplex, there is no impact on perf with this. Signed-off-by: Akhil R <akhilrajeev@nvidia.com> --- drivers/i2c/busses/i2c-tegra.c | 69 ++++++++++------------------------ 1 file changed, 20 insertions(+), 49 deletions(-) diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 83e74b8baf67..b15829ff2aab 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -249,8 +249,7 @@ struct tegra_i2c_hw_feature { * @msg_read: indicates that the transfer is a read access * @timings: i2c timings information like bus frequency * @multimaster_mode: indicates that I2C controller is in multi-master mode - * @tx_dma_chan: DMA transmit channel - * @rx_dma_chan: DMA receive channel + * @dma_chan: DMA channel * @dma_phys: handle to DMA resources * @dma_buf: pointer to allocated DMA buffer * @dma_buf_size: DMA buffer size @@ -283,8 +282,7 @@ struct tegra_i2c_dev { __u16 msg_len; struct completion dma_complete; - struct dma_chan *tx_dma_chan; - struct dma_chan *rx_dma_chan; + struct dma_chan *dma_chan; unsigned int dma_buf_size; struct device *dma_dev; dma_addr_t dma_phys; @@ -393,16 +391,14 @@ static int tegra_i2c_dma_submit(struct tegra_i2c_dev *i2c_dev, size_t len) { struct dma_async_tx_descriptor *dma_desc; enum dma_transfer_direction dir; - struct dma_chan *chan; dev_dbg(i2c_dev->dev, "starting DMA for length: %zu\n", len); reinit_completion(&i2c_dev->dma_complete); dir = i2c_dev->msg_read ? DMA_DEV_TO_MEM : DMA_MEM_TO_DEV; - chan = i2c_dev->msg_read ? i2c_dev->rx_dma_chan : i2c_dev->tx_dma_chan; - dma_desc = dmaengine_prep_slave_single(chan, i2c_dev->dma_phys, + dma_desc = dmaengine_prep_slave_single(i2c_dev->dma_chan, i2c_dev->dma_phys, len, dir, DMA_PREP_INTERRUPT | DMA_CTRL_ACK); if (!dma_desc) { @@ -415,7 +411,7 @@ static int tegra_i2c_dma_submit(struct tegra_i2c_dev *i2c_dev, size_t len) dma_desc->callback_param = i2c_dev; dmaengine_submit(dma_desc); - dma_async_issue_pending(chan); + dma_async_issue_pending(i2c_dev->dma_chan); return 0; } @@ -428,20 +424,14 @@ static void tegra_i2c_release_dma(struct tegra_i2c_dev *i2c_dev) i2c_dev->dma_buf = NULL; } - if (i2c_dev->tx_dma_chan) { - dma_release_channel(i2c_dev->tx_dma_chan); - i2c_dev->tx_dma_chan = NULL; - } - - if (i2c_dev->rx_dma_chan) { - dma_release_channel(i2c_dev->rx_dma_chan); - i2c_dev->rx_dma_chan = NULL; + if (i2c_dev->dma_chan) { + dma_release_channel(i2c_dev->dma_chan); + i2c_dev->dma_chan = NULL; } } static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev) { - struct dma_chan *chan; dma_addr_t dma_phys; u32 *dma_buf; int err; @@ -459,25 +449,18 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev) return 0; } - chan = dma_request_chan(i2c_dev->dev, "rx"); - if (IS_ERR(chan)) { - err = PTR_ERR(chan); - goto err_out; - } - - i2c_dev->rx_dma_chan = chan; - - chan = dma_request_chan(i2c_dev->dev, "tx"); - if (IS_ERR(chan)) { - err = PTR_ERR(chan); + /* + * The same channel will be used for both RX and TX. + * Keeping the name as "tx" for backward compatibility + * with existing devicetrees. + */ + i2c_dev->dma_chan = dma_request_chan(i2c_dev->dev, "tx"); + if (IS_ERR(i2c_dev->dma_chan)) { + err = PTR_ERR(i2c_dev->dma_chan); goto err_out; } - i2c_dev->tx_dma_chan = chan; - - WARN_ON(i2c_dev->tx_dma_chan->device != i2c_dev->rx_dma_chan->device); - i2c_dev->dma_dev = chan->device->dev; - + i2c_dev->dma_dev = i2c_dev->dma_chan->device->dev; i2c_dev->dma_buf_size = i2c_dev->hw->quirks->max_write_len + I2C_PACKET_HEADER_SIZE; @@ -976,11 +959,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id) dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS); if (i2c_dev->dma_mode) { - if (i2c_dev->msg_read) - dmaengine_terminate_async(i2c_dev->rx_dma_chan); - else - dmaengine_terminate_async(i2c_dev->tx_dma_chan); - + dmaengine_terminate_async(i2c_dev->dma_chan); complete(&i2c_dev->dma_complete); } @@ -994,7 +973,6 @@ static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev, { struct dma_slave_config slv_config = {0}; u32 val, reg, dma_burst, reg_offset; - struct dma_chan *chan; int err; if (i2c_dev->hw->has_mst_fifo) @@ -1011,7 +989,6 @@ static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev, dma_burst = 8; if (i2c_dev->msg_read) { - chan = i2c_dev->rx_dma_chan; reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_RX_FIFO); slv_config.src_addr = i2c_dev->base_phys + reg_offset; @@ -1023,7 +1000,6 @@ static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev, else val = I2C_FIFO_CONTROL_RX_TRIG(dma_burst); } else { - chan = i2c_dev->tx_dma_chan; reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_TX_FIFO); slv_config.dst_addr = i2c_dev->base_phys + reg_offset; @@ -1037,7 +1013,7 @@ static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev, } slv_config.device_fc = true; - err = dmaengine_slave_config(chan, &slv_config); + err = dmaengine_slave_config(i2c_dev->dma_chan, &slv_config); if (err) { dev_err(i2c_dev->dev, "DMA config failed: %d\n", err); dev_err(i2c_dev->dev, "falling back to PIO\n"); @@ -1347,13 +1323,8 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, * performs synchronization after the transfer's termination * and we want to get a completion if transfer succeeded. */ - dmaengine_synchronize(i2c_dev->msg_read ? - i2c_dev->rx_dma_chan : - i2c_dev->tx_dma_chan); - - dmaengine_terminate_sync(i2c_dev->msg_read ? - i2c_dev->rx_dma_chan : - i2c_dev->tx_dma_chan); + dmaengine_synchronize(i2c_dev->dma_chan); + dmaengine_terminate_sync(i2c_dev->dma_chan); if (!time_left && !completion_done(&i2c_dev->dma_complete)) { dev_err(i2c_dev->dev, "DMA transfer timed out\n"); -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/2] i2c: tegra: Share same DMA channel for RX and TX 2023-03-24 11:59 ` [PATCH v4 2/2] i2c: tegra: Share same DMA channel for RX and TX Akhil R @ 2023-04-05 12:26 ` Thierry Reding 0 siblings, 0 replies; 8+ messages in thread From: Thierry Reding @ 2023-04-05 12:26 UTC (permalink / raw) To: Akhil R Cc: christian.koenig, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, sumit.semwal, wsa [-- Attachment #1: Type: text/plain, Size: 547 bytes --] On Fri, Mar 24, 2023 at 05:29:24PM +0530, Akhil R wrote: > Allocate only one DMA channel for I2C and share it for both TX and RX > instead of using two different DMA hardware channels with the same > slave ID. Since I2C supports only half duplex, there is no impact on > perf with this. > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > --- > drivers/i2c/busses/i2c-tegra.c | 69 ++++++++++------------------------ > 1 file changed, 20 insertions(+), 49 deletions(-) Looks good: Acked-by: Thierry Reding <treding@nvidia.com> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-04-13 9:40 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-24 11:59 [PATCH v4 0/2] Tegra I2C DMA and SMBus blockread updates Akhil R 2023-03-24 11:59 ` [PATCH v4 1/2] i2c: tegra: Fix PEC support for SMBUS block read Akhil R 2023-04-05 12:25 ` Thierry Reding 2023-04-05 16:11 ` Akhil R 2023-04-13 8:27 ` Thierry Reding 2023-04-13 9:40 ` Dmitry Osipenko 2023-03-24 11:59 ` [PATCH v4 2/2] i2c: tegra: Share same DMA channel for RX and TX Akhil R 2023-04-05 12:26 ` Thierry Reding
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).