| Message ID | 20230322102413.52886-1-akhilrajeev@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] i2c: tegra: Share same DMA channel for RX and TX | expand |
On 22/03/2023 10:24, 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> Just to confirm. This impacts all Tegra devices from Tegra20 to the latest. Does this work for all Tegra and the different DMA controllers that they have? Thanks Jon
> On 22/03/2023 10:24, 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> > > Just to confirm. This impacts all Tegra devices from Tegra20 to the > latest. Does this work for all Tegra and the different DMA controllers > that they have? > Yes, It should. I could see in the APB DMA driver that the same channel could be used for TX and RX and the direction is configured only during dma_prep_*() calls. I did not test it on a Tegra with APB DMA, but since it works very similar to GPC DMA there should not be any impact. Regards, Akhil -- nvpublic
On 22/03/2023 12:00, Akhil R wrote: >> On 22/03/2023 10:24, 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> >> >> Just to confirm. This impacts all Tegra devices from Tegra20 to the >> latest. Does this work for all Tegra and the different DMA controllers >> that they have? >> > Yes, It should. I could see in the APB DMA driver that the same channel > could be used for TX and RX and the direction is configured only during > dma_prep_*() calls. > I did not test it on a Tegra with APB DMA, but since it works very similar > to GPC DMA there should not be any impact. OK. BTW, this does not apply cleanly on top of -next. It appears that this is based on top "i2c: tegra: Fix PEC support for SMBUS block read" and that one needs to be applied first. This can be avoided if you send as a series. Jon
> On 22/03/2023 12:00, Akhil R wrote: > >> On 22/03/2023 10:24, 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> > >> > >> Just to confirm. This impacts all Tegra devices from Tegra20 to the > >> latest. Does this work for all Tegra and the different DMA controllers > >> that they have? > >> > > Yes, It should. I could see in the APB DMA driver that the same channel > > could be used for TX and RX and the direction is configured only during > > dma_prep_*() calls. > > I did not test it on a Tegra with APB DMA, but since it works very similar > > to GPC DMA there should not be any impact. > > > OK. BTW, this does not apply cleanly on top of -next. It appears that > this is based on top "i2c: tegra: Fix PEC support for SMBUS block read" > and that one needs to be applied first. This can be avoided if you send > as a series. > Oh. Okay. I used 'git am --3way' when I tried, and the conflict went unnoticed. Shall I send a new version on top of -next? The two patches were added in different contexts and that’s why I did not combine them as a series. Regards, Akhil
On Thu, Mar 23, 2023 at 09:26:00AM +0000, Akhil R wrote: > > On 22/03/2023 12:00, Akhil R wrote: > > >> On 22/03/2023 10:24, 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> > > >> > > >> Just to confirm. This impacts all Tegra devices from Tegra20 to the > > >> latest. Does this work for all Tegra and the different DMA controllers > > >> that they have? > > >> > > > Yes, It should. I could see in the APB DMA driver that the same channel > > > could be used for TX and RX and the direction is configured only during > > > dma_prep_*() calls. > > > I did not test it on a Tegra with APB DMA, but since it works very similar > > > to GPC DMA there should not be any impact. > > > > > > OK. BTW, this does not apply cleanly on top of -next. It appears that > > this is based on top "i2c: tegra: Fix PEC support for SMBUS block read" > > and that one needs to be applied first. This can be avoided if you send > > as a series. > > > Oh. Okay. I used 'git am --3way' when I tried, and the conflict went unnoticed. > Shall I send a new version on top of -next? > The two patches were added in different contexts and that’s why I did not > combine them as a series. It's usually best to combine them in a series even if they are in slightly different contexts. This is especially true if they cause conflicts between one another. If you send them as a series, you can resolve the conflicts yourself (you may not even have conflicts locally if you create the patches in the same branch), but if you send them separately the maintainer will end up having to resolve the conflicts (or apply in the right order). It's best if you resolve the conflicts because you know better than the maintainer (usually) or specify any dependencies to make it easier for the maintainer to do the right thing. But again, in the vast majority of cases, it's best to combine all the work on one driver in a single series before sending out. Thierry
, Mar 23, 2023 at 09:26:00AM +0000, Akhil R wrote: > > > On 22/03/2023 12:00, Akhil R wrote: > > > >> On 22/03/2023 10:24, 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> > > > >> > > > >> Just to confirm. This impacts all Tegra devices from Tegra20 to the > > > >> latest. Does this work for all Tegra and the different DMA controllers > > > >> that they have? > > > >> > > > > Yes, It should. I could see in the APB DMA driver that the same channel > > > > could be used for TX and RX and the direction is configured only during > > > > dma_prep_*() calls. > > > > I did not test it on a Tegra with APB DMA, but since it works very > similar > > > > to GPC DMA there should not be any impact. > > > > > > > > > OK. BTW, this does not apply cleanly on top of -next. It appears that > > > this is based on top "i2c: tegra: Fix PEC support for SMBUS block read" > > > and that one needs to be applied first. This can be avoided if you send > > > as a series. > > > > > Oh. Okay. I used 'git am --3way' when I tried, and the conflict went > unnoticed. > > Shall I send a new version on top of -next? > > The two patches were added in different contexts and that’s why I did not > > combine them as a series. > > It's usually best to combine them in a series even if they are in > slightly different contexts. This is especially true if they cause > conflicts between one another. If you send them as a series, you can > resolve the conflicts yourself (you may not even have conflicts locally > if you create the patches in the same branch), but if you send them > separately the maintainer will end up having to resolve the conflicts > (or apply in the right order). > > It's best if you resolve the conflicts because you know better than the > maintainer (usually) or specify any dependencies to make it easier for > the maintainer to do the right thing. > > But again, in the vast majority of cases, it's best to combine all the > work on one driver in a single series before sending out. > Okay. Got it. I shall send a new patchset with both the patches. Can I put the patchset as v1 or does it have to be something different? Because this patch is in v3 and "i2c: tegra: Fix PEC support for SMBUS block read" is v2 now. Regards, Akhil
On Thu, Mar 23, 2023 at 12:16:23PM +0000, Akhil R wrote: > , Mar 23, 2023 at 09:26:00AM +0000, Akhil R wrote: > > > > On 22/03/2023 12:00, Akhil R wrote: > > > > >> On 22/03/2023 10:24, 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> > > > > >> > > > > >> Just to confirm. This impacts all Tegra devices from Tegra20 to the > > > > >> latest. Does this work for all Tegra and the different DMA controllers > > > > >> that they have? > > > > >> > > > > > Yes, It should. I could see in the APB DMA driver that the same channel > > > > > could be used for TX and RX and the direction is configured only during > > > > > dma_prep_*() calls. > > > > > I did not test it on a Tegra with APB DMA, but since it works very > > similar > > > > > to GPC DMA there should not be any impact. > > > > > > > > > > > > OK. BTW, this does not apply cleanly on top of -next. It appears that > > > > this is based on top "i2c: tegra: Fix PEC support for SMBUS block read" > > > > and that one needs to be applied first. This can be avoided if you send > > > > as a series. > > > > > > > Oh. Okay. I used 'git am --3way' when I tried, and the conflict went > > unnoticed. > > > Shall I send a new version on top of -next? > > > The two patches were added in different contexts and that’s why I did not > > > combine them as a series. > > > > It's usually best to combine them in a series even if they are in > > slightly different contexts. This is especially true if they cause > > conflicts between one another. If you send them as a series, you can > > resolve the conflicts yourself (you may not even have conflicts locally > > if you create the patches in the same branch), but if you send them > > separately the maintainer will end up having to resolve the conflicts > > (or apply in the right order). > > > > It's best if you resolve the conflicts because you know better than the > > maintainer (usually) or specify any dependencies to make it easier for > > the maintainer to do the right thing. > > > > But again, in the vast majority of cases, it's best to combine all the > > work on one driver in a single series before sending out. > > > Okay. Got it. I shall send a new patchset with both the patches. > Can I put the patchset as v1 or does it have to be something different? > Because this patch is in v3 and "i2c: tegra: Fix PEC support for SMBUS > block read" is v2 now. Best to keep versioning. I'd go with making the combined series v4, which is probably the least confusing. You can technically also make a combined series where each patch is at a different version, but that would probably confuse people even more. Thierry
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 75250a46cf71..a4fc6b6ae24c 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 @@ -282,8 +281,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; @@ -392,16 +390,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) { @@ -414,7 +410,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; } @@ -427,20 +423,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; @@ -458,25 +448,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; @@ -975,11 +958,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); } @@ -993,7 +972,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) @@ -1010,7 +988,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; @@ -1022,7 +999,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; @@ -1036,7 +1012,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"); @@ -1346,13 +1322,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 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> --- v2->v3: Updated commit message and comment. Removed local variable for DMA channel. 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 | 69 ++++++++++------------------------ 1 file changed, 20 insertions(+), 49 deletions(-)