| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4fe9ed3b-ea8f-cb53-8e23-e50bec6ebda0@nvidia.com>
Date: Fri, 17 Sep 2021 10:45:08 +0100
From: Jon Hunter <jonathanh@...dia.com>
To: Sameer Pujar <spujar@...dia.com>, <vkoul@...nel.org>,
<ldewangan@...dia.com>, <thierry.reding@...il.com>
CC: <dmaengine@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [RESEND PATCH 3/3] dmaengine: tegra210-adma: Override ADMA FIFO
size
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@...dia.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@...dia.com>
Cheers
Jon
--
nvpublic
Powered by blists - more mailing lists