| Message ID | 20230222102759.23165-1-akhilrajeev@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] i2c: tegra: Share same DMA channel for Rx and Tx | expand |
On Wed, Feb 22, 2023 at 03:57:59PM +0530, Akhil R wrote: > Allocate only one DMA channel for I2C and share it for both Tx and Rx. > Since I2C supports only half duplex, there is no impact on perf with > this. > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > --- > v1->v2: Remove WARN_ON for DMA channel mismatch. There is only one > channel in use with this change. > > drivers/i2c/busses/i2c-tegra.c | 54 ++++++++++------------------------ > 1 file changed, 15 insertions(+), 39 deletions(-) I'm a little confused by this. All device trees already list the very same reference for both TX and RX DMA channels in the I2C nodes, so these channels are already effectively shared, aren't they? So all this does is to get rid of the duplicated pointer? In practice, is the DMA channel pointer going to point to the exact same memory or are these separate objects that happen to point to the same hardware resource? In either case, I think the commit message should clarify that. Also, a few minor nits below... > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index 6aab84c8d22b..f52b835f1700 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -248,8 +248,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 > @@ -281,8 +280,7 @@ struct tegra_i2c_dev { > u8 *msg_buf; > > 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; > @@ -398,7 +396,7 @@ static int tegra_i2c_dma_submit(struct tegra_i2c_dev *i2c_dev, size_t 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; > + chan = i2c_dev->dma_chan; Is there any point in keeping around the local variable? It doesn't serve any purpose anymore. > > dma_desc = dmaengine_prep_slave_single(chan, i2c_dev->dma_phys, > len, dir, DMA_PREP_INTERRUPT | > @@ -426,14 +424,9 @@ 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; > } > } > > @@ -457,25 +450,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; > - > + /* The same channel will be used for both Rx and Tx. > + * Keeping the name as tx for backward compatibility with > + * existing devicetrees. > + */ Block comments should have the starting /* on a line of its own. Also, s/Rx/RX/ and s/Tx/TX/ for consistency. Looks like you also used "Rx" and "Tx" in the commit message. The driver uses "RX" and "TX" elsewhere, so better switch to that in both this comment and the commit message. Also, perhaps quote the "tx" reference above to highlight that it's the literal string in DT that you're referring to. Thierry
> > Allocate only one DMA channel for I2C and share it for both Tx and Rx. > > Since I2C supports only half duplex, there is no impact on perf with > > this. > > > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > > --- > > v1->v2: Remove WARN_ON for DMA channel mismatch. There is only one > > channel in use with this change. > > > > drivers/i2c/busses/i2c-tegra.c | 54 ++++++++++------------------------ > > 1 file changed, 15 insertions(+), 39 deletions(-) > > I'm a little confused by this. All device trees already list the very > same reference for both TX and RX DMA channels in the I2C nodes, so > these channels are already effectively shared, aren't they? The value given in DT refers to the slave-id of an I2C instance and not the channel number. Each I2C instance was in-effect using two different DMA channels with the same slave-id. This change is to free up the extra DMA channel since both will not be used simultaneously. > > So all this does is to get rid of the duplicated pointer? In practice, > is the DMA channel pointer going to point to the exact same memory or > are these separate objects that happen to point to the same hardware > resource? They are indeed separate hardware resource itself. As I described above, two separate DMA channels were in use for each I2C instance. > > In either case, I think the commit message should clarify that. Also, a > few minor nits below... Agreed with the other comments. Regards, Akhil
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 6aab84c8d22b..f52b835f1700 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -248,8 +248,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 @@ -281,8 +280,7 @@ struct tegra_i2c_dev { u8 *msg_buf; 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; @@ -398,7 +396,7 @@ static int tegra_i2c_dma_submit(struct tegra_i2c_dev *i2c_dev, size_t 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; + chan = i2c_dev->dma_chan; dma_desc = dmaengine_prep_slave_single(chan, i2c_dev->dma_phys, len, dir, DMA_PREP_INTERRUPT | @@ -426,14 +424,9 @@ 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; } } @@ -457,25 +450,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; - + /* The same channel will be used for both Rx and Tx. + * Keeping the name as tx for backward compatibility with + * existing devicetrees. + */ chan = dma_request_chan(i2c_dev->dev, "tx"); if (IS_ERR(chan)) { err = PTR_ERR(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_chan = chan; i2c_dev->dma_dev = chan->device->dev; - i2c_dev->dma_buf_size = i2c_dev->hw->quirks->max_write_len + I2C_PACKET_HEADER_SIZE; @@ -974,11 +960,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); } @@ -1008,8 +990,8 @@ static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev, else dma_burst = 8; + chan = i2c_dev->dma_chan; 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; @@ -1021,7 +1003,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; @@ -1333,13 +1314,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");
Allocate only one DMA channel for I2C and share it for both Tx and Rx. Since I2C supports only half duplex, there is no impact on perf with this. Signed-off-by: Akhil R <akhilrajeev@nvidia.com> --- v1->v2: Remove WARN_ON for DMA channel mismatch. There is only one channel in use with this change. drivers/i2c/busses/i2c-tegra.c | 54 ++++++++++------------------------ 1 file changed, 15 insertions(+), 39 deletions(-)