| Message ID | 1631722025-19873-4-git-send-email-spujar@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 32de4745e20a639376735f198cccd0477f9aa396 |
| Headers | show |
| Series | Few Tegra210 ADMA fixes | expand |
On 15/09/2021 17:07, Sameer Pujar wrote: > ADMAIF FIFO uses a ring buffer and it is divided amongst the available > channels. The default FIFO size (in multiples of 16 words) of ADMAIF TX/RX > channels is as below: > * On Tegra210, > channel 1 to 2 : size = 3 > channel 3 to 10: size = 2 > * On Tegra186 and later, > channel 1 to 4 : size = 3 > channel 5 to 20: size = 2 > > As per recommendation from HW, FIFO size of ADMA channel should be same as > the corresponding ADMAIF channel it maps to. FIFO corruption is observed if > the sizes do not match. We are using the default FIFO sizes for ADMAIF and > there is no plan to support any custom values. > > Thus at runtime, override the ADMA channel FIFO size value depending on the > corresponding ADMAIF channel. > > Fixes: 9ab59bf5dd63 ("dmaengine: tegra210-adma: Fix channel FIFO configuration") > Signed-off-by: Sameer Pujar <spujar@nvidia.com> > --- > drivers/dma/tegra210-adma.c | 48 ++++++++++++++++++++++++++++++++------------- > 1 file changed, 34 insertions(+), 14 deletions(-) > > diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c > index 03f9776..911533c 100644 > --- a/drivers/dma/tegra210-adma.c > +++ b/drivers/dma/tegra210-adma.c > @@ -43,10 +43,8 @@ > #define TEGRA186_ADMA_CH_CONFIG_OUTSTANDING_REQS(reqs) (reqs << 4) > > #define ADMA_CH_FIFO_CTRL 0x2c > -#define TEGRA210_ADMA_CH_FIFO_CTRL_TXSIZE(val) (((val) & 0xf) << 8) > -#define TEGRA210_ADMA_CH_FIFO_CTRL_RXSIZE(val) ((val) & 0xf) > -#define TEGRA186_ADMA_CH_FIFO_CTRL_TXSIZE(val) (((val) & 0x1f) << 8) > -#define TEGRA186_ADMA_CH_FIFO_CTRL_RXSIZE(val) ((val) & 0x1f) > +#define ADMA_CH_TX_FIFO_SIZE_SHIFT 8 > +#define ADMA_CH_RX_FIFO_SIZE_SHIFT 0 > > #define ADMA_CH_LOWER_SRC_ADDR 0x34 > #define ADMA_CH_LOWER_TRG_ADDR 0x3c > @@ -61,12 +59,6 @@ > > #define TEGRA_ADMA_BURST_COMPLETE_TIME 20 > > -#define TEGRA210_FIFO_CTRL_DEFAULT (TEGRA210_ADMA_CH_FIFO_CTRL_TXSIZE(3) | \ > - TEGRA210_ADMA_CH_FIFO_CTRL_RXSIZE(3)) > - > -#define TEGRA186_FIFO_CTRL_DEFAULT (TEGRA186_ADMA_CH_FIFO_CTRL_TXSIZE(3) | \ > - TEGRA186_ADMA_CH_FIFO_CTRL_RXSIZE(3)) > - > #define ADMA_CH_REG_FIELD_VAL(val, mask, shift) (((val) & mask) << shift) > > struct tegra_adma; > @@ -84,6 +76,8 @@ struct tegra_adma; > * @ch_req_max: Maximum number of Tx or Rx channels available. > * @ch_reg_size: Size of DMA channel register space. > * @nr_channels: Number of DMA channels available. > + * @ch_fifo_size_mask: Mask for FIFO size field. > + * @sreq_index_offset: Slave channel index offset. > * @has_outstanding_reqs: If DMA channel can have outstanding requests. > */ > struct tegra_adma_chip_data { > @@ -98,6 +92,8 @@ struct tegra_adma_chip_data { > unsigned int ch_req_max; > unsigned int ch_reg_size; > unsigned int nr_channels; > + unsigned int ch_fifo_size_mask; > + unsigned int sreq_index_offset; > bool has_outstanding_reqs; > }; > > @@ -561,13 +557,14 @@ static int tegra_adma_set_xfer_params(struct tegra_adma_chan *tdc, > { > struct tegra_adma_chan_regs *ch_regs = &desc->ch_regs; > const struct tegra_adma_chip_data *cdata = tdc->tdma->cdata; > - unsigned int burst_size, adma_dir; > + unsigned int burst_size, adma_dir, fifo_size_shift; > > if (desc->num_periods > ADMA_CH_CONFIG_MAX_BUFS) > return -EINVAL; > > switch (direction) { > case DMA_MEM_TO_DEV: > + fifo_size_shift = ADMA_CH_TX_FIFO_SIZE_SHIFT; > adma_dir = ADMA_CH_CTRL_DIR_MEM2AHUB; > burst_size = tdc->sconfig.dst_maxburst; > ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(desc->num_periods - 1); > @@ -578,6 +575,7 @@ static int tegra_adma_set_xfer_params(struct tegra_adma_chan *tdc, > break; > > case DMA_DEV_TO_MEM: > + fifo_size_shift = ADMA_CH_RX_FIFO_SIZE_SHIFT; > adma_dir = ADMA_CH_CTRL_DIR_AHUB2MEM; > burst_size = tdc->sconfig.src_maxburst; > ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(desc->num_periods - 1); > @@ -599,7 +597,27 @@ static int tegra_adma_set_xfer_params(struct tegra_adma_chan *tdc, > ch_regs->config |= ADMA_CH_CONFIG_WEIGHT_FOR_WRR(1); > if (cdata->has_outstanding_reqs) > ch_regs->config |= TEGRA186_ADMA_CH_CONFIG_OUTSTANDING_REQS(8); > - ch_regs->fifo_ctrl = cdata->ch_fifo_ctrl; > + > + /* > + * 'sreq_index' represents the current ADMAIF channel number and as per > + * HW recommendation its FIFO size should match with the corresponding > + * ADMA channel. > + * > + * ADMA FIFO size is set as per below (based on default ADMAIF channel > + * FIFO sizes): > + * fifo_size = 0x2 (sreq_index > sreq_index_offset) > + * fifo_size = 0x3 (sreq_index <= sreq_index_offset) > + * > + */ > + if (tdc->sreq_index > cdata->sreq_index_offset) > + ch_regs->fifo_ctrl = > + ADMA_CH_REG_FIELD_VAL(2, cdata->ch_fifo_size_mask, > + fifo_size_shift); > + else > + ch_regs->fifo_ctrl = > + ADMA_CH_REG_FIELD_VAL(3, cdata->ch_fifo_size_mask, > + fifo_size_shift); > + > ch_regs->tc = desc->period_len & ADMA_CH_TC_COUNT_MASK; > > return tegra_adma_request_alloc(tdc, direction); > @@ -783,11 +801,12 @@ static const struct tegra_adma_chip_data tegra210_chip_data = { > .ch_req_tx_shift = 28, > .ch_req_rx_shift = 24, > .ch_base_offset = 0, > - .ch_fifo_ctrl = TEGRA210_FIFO_CTRL_DEFAULT, > .ch_req_mask = 0xf, > .ch_req_max = 10, > .ch_reg_size = 0x80, > .nr_channels = 22, > + .ch_fifo_size_mask = 0xf, > + .sreq_index_offset = 2, > .has_outstanding_reqs = false, > }; > > @@ -798,11 +817,12 @@ static const struct tegra_adma_chip_data tegra186_chip_data = { > .ch_req_tx_shift = 27, > .ch_req_rx_shift = 22, > .ch_base_offset = 0x10000, > - .ch_fifo_ctrl = TEGRA186_FIFO_CTRL_DEFAULT, > .ch_req_mask = 0x1f, > .ch_req_max = 20, > .ch_reg_size = 0x100, > .nr_channels = 32, > + .ch_fifo_size_mask = 0x1f, > + .sreq_index_offset = 4, > .has_outstanding_reqs = true, > }; Looks good to me. The only minor comment I have is that 'sreq_index_offset' is not very descriptive. Maybe fifo_size_sreq_index? It is not great either, so no big deal either way. Reviewed-by: Jon Hunter <jonathanh@nvidia.com> Cheers Jon
diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c index 03f9776..911533c 100644 --- a/drivers/dma/tegra210-adma.c +++ b/drivers/dma/tegra210-adma.c @@ -43,10 +43,8 @@ #define TEGRA186_ADMA_CH_CONFIG_OUTSTANDING_REQS(reqs) (reqs << 4) #define ADMA_CH_FIFO_CTRL 0x2c -#define TEGRA210_ADMA_CH_FIFO_CTRL_TXSIZE(val) (((val) & 0xf) << 8) -#define TEGRA210_ADMA_CH_FIFO_CTRL_RXSIZE(val) ((val) & 0xf) -#define TEGRA186_ADMA_CH_FIFO_CTRL_TXSIZE(val) (((val) & 0x1f) << 8) -#define TEGRA186_ADMA_CH_FIFO_CTRL_RXSIZE(val) ((val) & 0x1f) +#define ADMA_CH_TX_FIFO_SIZE_SHIFT 8 +#define ADMA_CH_RX_FIFO_SIZE_SHIFT 0 #define ADMA_CH_LOWER_SRC_ADDR 0x34 #define ADMA_CH_LOWER_TRG_ADDR 0x3c @@ -61,12 +59,6 @@ #define TEGRA_ADMA_BURST_COMPLETE_TIME 20 -#define TEGRA210_FIFO_CTRL_DEFAULT (TEGRA210_ADMA_CH_FIFO_CTRL_TXSIZE(3) | \ - TEGRA210_ADMA_CH_FIFO_CTRL_RXSIZE(3)) - -#define TEGRA186_FIFO_CTRL_DEFAULT (TEGRA186_ADMA_CH_FIFO_CTRL_TXSIZE(3) | \ - TEGRA186_ADMA_CH_FIFO_CTRL_RXSIZE(3)) - #define ADMA_CH_REG_FIELD_VAL(val, mask, shift) (((val) & mask) << shift) struct tegra_adma; @@ -84,6 +76,8 @@ struct tegra_adma; * @ch_req_max: Maximum number of Tx or Rx channels available. * @ch_reg_size: Size of DMA channel register space. * @nr_channels: Number of DMA channels available. + * @ch_fifo_size_mask: Mask for FIFO size field. + * @sreq_index_offset: Slave channel index offset. * @has_outstanding_reqs: If DMA channel can have outstanding requests. */ struct tegra_adma_chip_data { @@ -98,6 +92,8 @@ struct tegra_adma_chip_data { unsigned int ch_req_max; unsigned int ch_reg_size; unsigned int nr_channels; + unsigned int ch_fifo_size_mask; + unsigned int sreq_index_offset; bool has_outstanding_reqs; }; @@ -561,13 +557,14 @@ static int tegra_adma_set_xfer_params(struct tegra_adma_chan *tdc, { struct tegra_adma_chan_regs *ch_regs = &desc->ch_regs; const struct tegra_adma_chip_data *cdata = tdc->tdma->cdata; - unsigned int burst_size, adma_dir; + unsigned int burst_size, adma_dir, fifo_size_shift; if (desc->num_periods > ADMA_CH_CONFIG_MAX_BUFS) return -EINVAL; switch (direction) { case DMA_MEM_TO_DEV: + fifo_size_shift = ADMA_CH_TX_FIFO_SIZE_SHIFT; adma_dir = ADMA_CH_CTRL_DIR_MEM2AHUB; burst_size = tdc->sconfig.dst_maxburst; ch_regs->config = ADMA_CH_CONFIG_SRC_BUF(desc->num_periods - 1); @@ -578,6 +575,7 @@ static int tegra_adma_set_xfer_params(struct tegra_adma_chan *tdc, break; case DMA_DEV_TO_MEM: + fifo_size_shift = ADMA_CH_RX_FIFO_SIZE_SHIFT; adma_dir = ADMA_CH_CTRL_DIR_AHUB2MEM; burst_size = tdc->sconfig.src_maxburst; ch_regs->config = ADMA_CH_CONFIG_TRG_BUF(desc->num_periods - 1); @@ -599,7 +597,27 @@ static int tegra_adma_set_xfer_params(struct tegra_adma_chan *tdc, ch_regs->config |= ADMA_CH_CONFIG_WEIGHT_FOR_WRR(1); if (cdata->has_outstanding_reqs) ch_regs->config |= TEGRA186_ADMA_CH_CONFIG_OUTSTANDING_REQS(8); - ch_regs->fifo_ctrl = cdata->ch_fifo_ctrl; + + /* + * 'sreq_index' represents the current ADMAIF channel number and as per + * HW recommendation its FIFO size should match with the corresponding + * ADMA channel. + * + * ADMA FIFO size is set as per below (based on default ADMAIF channel + * FIFO sizes): + * fifo_size = 0x2 (sreq_index > sreq_index_offset) + * fifo_size = 0x3 (sreq_index <= sreq_index_offset) + * + */ + if (tdc->sreq_index > cdata->sreq_index_offset) + ch_regs->fifo_ctrl = + ADMA_CH_REG_FIELD_VAL(2, cdata->ch_fifo_size_mask, + fifo_size_shift); + else + ch_regs->fifo_ctrl = + ADMA_CH_REG_FIELD_VAL(3, cdata->ch_fifo_size_mask, + fifo_size_shift); + ch_regs->tc = desc->period_len & ADMA_CH_TC_COUNT_MASK; return tegra_adma_request_alloc(tdc, direction); @@ -783,11 +801,12 @@ static const struct tegra_adma_chip_data tegra210_chip_data = { .ch_req_tx_shift = 28, .ch_req_rx_shift = 24, .ch_base_offset = 0, - .ch_fifo_ctrl = TEGRA210_FIFO_CTRL_DEFAULT, .ch_req_mask = 0xf, .ch_req_max = 10, .ch_reg_size = 0x80, .nr_channels = 22, + .ch_fifo_size_mask = 0xf, + .sreq_index_offset = 2, .has_outstanding_reqs = false, }; @@ -798,11 +817,12 @@ static const struct tegra_adma_chip_data tegra186_chip_data = { .ch_req_tx_shift = 27, .ch_req_rx_shift = 22, .ch_base_offset = 0x10000, - .ch_fifo_ctrl = TEGRA186_FIFO_CTRL_DEFAULT, .ch_req_mask = 0x1f, .ch_req_max = 20, .ch_reg_size = 0x100, .nr_channels = 32, + .ch_fifo_size_mask = 0x1f, + .sreq_index_offset = 4, .has_outstanding_reqs = true, };
ADMAIF FIFO uses a ring buffer and it is divided amongst the available channels. The default FIFO size (in multiples of 16 words) of ADMAIF TX/RX channels is as below: * On Tegra210, channel 1 to 2 : size = 3 channel 3 to 10: size = 2 * On Tegra186 and later, channel 1 to 4 : size = 3 channel 5 to 20: size = 2 As per recommendation from HW, FIFO size of ADMA channel should be same as the corresponding ADMAIF channel it maps to. FIFO corruption is observed if the sizes do not match. We are using the default FIFO sizes for ADMAIF and there is no plan to support any custom values. Thus at runtime, override the ADMA channel FIFO size value depending on the corresponding ADMAIF channel. Fixes: 9ab59bf5dd63 ("dmaengine: tegra210-adma: Fix channel FIFO configuration") Signed-off-by: Sameer Pujar <spujar@nvidia.com> --- drivers/dma/tegra210-adma.c | 48 ++++++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 14 deletions(-)