Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221001122148.9158-1-kyarlagadda@nvidia.com> <20221001122148.9158-5-kyarlagadda@nvidia.com>
In-Reply-To: <20221001122148.9158-5-kyarlagadda@nvidia.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Sun, 2 Oct 2022 12:37:15 +0200
Message-ID: <CAMuHMdUoABDeLrdudfY09jGmCcnFpJmFGf0OieVBG6OuEpk7ZA@mail.gmail.com>
Subject: Re: [PATCH 5/5] spi: tegra210-quad: native dma support
To:     Krishna Yarlagadda <kyarlagadda@nvidia.com>
Cc:     broonie@kernel.org, thierry.reding@gmail.com, jonathanh@nvidia.com,
        linux-spi@vger.kernel.org, linux-tegra@vger.kernel.org,
        skomatineni@nvidia.com, ldewangan@nvidia.com,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Krishna,

On Sat, Oct 1, 2022 at 2:26 PM Krishna Yarlagadda
<kyarlagadda@nvidia.com> wrote:
> Enable Native DMA support for Tegra23 & Tegra24
>
> Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>

Thanks for your patch!

> --- a/drivers/spi/spi-tegra210-quad.c
> +++ b/drivers/spi/spi-tegra210-quad.c

> @@ -163,7 +169,7 @@
>  #define DATA_TRANSFER                          3
>
>  struct tegra_qspi_soc_data {
> -       bool has_dma;
> +       int has_dma;

unsigned int

Please rename the variable to e.g. "dma_mode", as "has_<foo>" suggests
it is a boolean flag.

>         bool cmb_xfer_capable;
>         unsigned int cs_count;
>  };

> @@ -629,23 +640,35 @@ static int tegra_qspi_start_dma_based_transfer(struct tegra_qspi *tqspi, struct
>                 len = tqspi->curr_dma_words * 4;
>
>         /* set attention level based on length of transfer */
> -       val = 0;
> -       if (len & 0xf) {
> -               val |= QSPI_TX_TRIG_1 | QSPI_RX_TRIG_1;
> -               dma_burst = 1;
> -       } else if (((len) >> 4) & 0x1) {
> -               val |= QSPI_TX_TRIG_4 | QSPI_RX_TRIG_4;
> -               dma_burst = 4;
> -       } else {
> -               val |= QSPI_TX_TRIG_8 | QSPI_RX_TRIG_8;
> -               dma_burst = 8;
> +       if (has_ext_dma) {
> +               val = 0;
> +               if (len & 0xf) {
> +                       val |= QSPI_TX_TRIG_1 | QSPI_RX_TRIG_1;
> +                       dma_burst = 1;
> +               } else if (((len) >> 4) & 0x1) {
> +                       val |= QSPI_TX_TRIG_4 | QSPI_RX_TRIG_4;
> +                       dma_burst = 4;
> +               } else {
> +                       val |= QSPI_TX_TRIG_8 | QSPI_RX_TRIG_8;
> +                       dma_burst = 8;
> +               }
>         }
>
>         tegra_qspi_writel(tqspi, val, QSPI_DMA_CTL);
>         tqspi->dma_control_reg = val;
>
>         dma_sconfig.device_fc = true;
> -       if (tqspi->cur_direction & DATA_DIR_TX) {
> +       if ((tqspi->cur_direction & DATA_DIR_TX) && !has_ext_dma) {
> +               if (tqspi->is_packed)
> +                       tx_dma_phys = t->tx_dma;
> +               else
> +                       tx_dma_phys = tqspi->tx_dma_phys;
> +               tegra_qspi_copy_client_txbuf_to_qspi_txbuf(tqspi, t);
> +               tegra_qspi_writel(tqspi, (tx_dma_phys & 0xffffffff),

lower_32_bits(), for consistency with below.

> +                                 QSPI_DMA_MEM_ADDRESS_REG);
> +               tegra_qspi_writel(tqspi, ((tx_dma_phys >> 32) & 0xff),

upper_32_bits(), to fix the build failures reported by 0-day
("warning: shift count >= width of type").

> +                                 QSPI_DMA_HI_ADDRESS_REG);
> +       } else if ((tqspi->cur_direction & DATA_DIR_TX) && has_ext_dma) {
>                 dma_sconfig.dst_addr = tqspi->phys + QSPI_TX_FIFO;
>                 dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>                 dma_sconfig.dst_maxburst = dma_burst;

> @@ -1045,6 +1085,8 @@ static int tegra_qspi_combined_seq_xfer(struct tegra_qspi *tqspi,
>                                         struct spi_message *msg)
>  {
>         bool is_first_msg = true;
> +       bool has_ext_dma = (tqspi->soc_data->has_dma &
> +                           QSPI_DMA_EXT) ? true : false;

No need for the "? true : false" (everywhere)

>         struct spi_transfer *xfer;
>         struct spi_device *spi = msg->spi;
>         u8 transfer_phase = 0;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds