Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210111151651.1616813-1-vkoul@kernel.org> <20210111151651.1616813-5-vkoul@kernel.org>
In-Reply-To: <20210111151651.1616813-5-vkoul@kernel.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Tue, 12 Jan 2021 16:01:18 -0800
Message-ID: <CAD=FV=XmfpQXhK_tKor-ta+5dqT-aq7OnV1e=VY-vMuXmUQEfQ@mail.gmail.com>
Subject: Re: [PATCH 4/7] spi: spi-geni-qcom: Add support for GPI dma
To:     Vinod Koul <vkoul@kernel.org>
Cc:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Mark Brown <broonie@kernel.org>, Wolfram Sang <wsa@kernel.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Andy Gross <agross@kernel.org>,
        Matthias Kaehlcke <mka@chromium.org>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Amit Pundir <amit.pundir@linaro.org>,
        linux-spi <linux-spi@vger.kernel.org>, linux-i2c@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Mon, Jan 11, 2021 at 7:17 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> We can use GPI DMA for devices where it is enabled by firmware. Add
> support for this mode
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/spi/spi-geni-qcom.c | 395 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 384 insertions(+), 11 deletions(-)

I did a somewhat cursory review, mostly focusing on making sure that
the non-GPI/GSI stuff doesn't regress.  ;-)  I think you've already
got a bunch of feedback for v2 so I'll plan to look back when I see
the v2 and maybe will find time to look at some of the GSI/GPI stuff
too...


> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 512e925d5ea4..5bb0e2192734 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -2,6 +2,8 @@
>  // Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
>
>  #include <linux/clk.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/log2.h>
> @@ -10,6 +12,7 @@
>  #include <linux/pm_opp.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/qcom-geni-se.h>
> +#include <linux/dma/qcom-gpi-dma.h>

nit: sort ordering doesn't match other includes.  It seems like
existing includes in this file are sorted ignoring subdirs.


>  static int spi_geni_prepare_message(struct spi_master *spi,
>                                         struct spi_message *spi_msg)
>  {
>         int ret;
>         struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +       struct geni_se *se = &mas->se;
> +
> +       mas->cur_xfer_mode = get_xfer_mode(spi);
> +
> +       if (mas->cur_xfer_mode == GENI_SE_FIFO) {
> +               geni_se_select_mode(se, GENI_SE_FIFO);

You don't need to do this over and over again.  We set up FIFO mode in
spi_geni_init() and it'll never change.


> +               reinit_completion(&mas->xfer_done);
> +               ret = setup_fifo_params(spi_msg->spi, spi);
> +               if (ret)
> +                       dev_err(mas->dev, "Couldn't select mode %d\n", ret);
> +
> +       } else if (mas->cur_xfer_mode == GENI_GPI_DMA) {
> +               mas->num_tx_eot = 0;
> +               mas->num_rx_eot = 0;
> +               mas->num_xfers = 0;
> +               reinit_completion(&mas->tx_cb);
> +               reinit_completion(&mas->rx_cb);
> +               memset(mas->gsi, 0, (sizeof(struct spi_geni_gsi) * NUM_SPI_XFER));
> +               geni_se_select_mode(se, GENI_GPI_DMA);
> +               ret = spi_geni_map_buf(mas, spi_msg);
> +

Extra blank line?

> +       } else {
> +               dev_err(mas->dev, "%s: Couldn't select mode %d", __func__, mas->cur_xfer_mode);

Please no __func__ in error messages unless you're doing a non-"dev"
print.  If you want to fill your log with function names you should
redefine the generic dev_xxx() functions to prefix "__func__" in your
own kernel.  You probably don't even need a printout here since
get_xfer_mode() already printed.


> +static int spi_geni_unprepare_message(struct spi_master *spi_mas, struct spi_message *spi_msg)
> +{
> +       struct spi_geni_master *mas = spi_master_get_devdata(spi_mas);
> +
> +       mas->cur_speed_hz = 0;
> +       mas->cur_bits_per_word = 0;

I think doing the above zeros will make the code a bunch slower for
FIFO mode.  Specifically we can avoid a whole bunch of (very slow)
interconnect code if the speed doesn't change between transfers and
the runtime PM auto power down hasn't hit.


> @@ -328,8 +609,34 @@ static int spi_geni_init(struct spi_geni_master *mas)
>         spi_tx_cfg &= ~CS_TOGGLE;
>         writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
>
> +       mas->tx = dma_request_slave_channel(mas->dev, "tx");
> +       if (IS_ERR_OR_NULL(mas->tx)) {

I didn't look too closely at this since I think Mark wanted you to
look into the core DMA support, but...

In general, don't you only need to do the DMA requests if you're in GPI mode?


> +               dev_err(mas->dev, "Failed to get tx DMA ch %ld", PTR_ERR(mas->tx));
> +               ret = PTR_ERR(mas->tx);
> +               goto out_pm;
> +       } else {

No need for else since last "if" ended up with goto".


> +               mas->rx = dma_request_slave_channel(mas->dev, "rx");
> +               if (IS_ERR_OR_NULL(mas->rx)) {
> +                       dev_err(mas->dev, "Failed to get rx DMA ch %ld", PTR_ERR(mas->rx));
> +                       dma_release_channel(mas->tx);
> +                       ret = PTR_ERR(mas->rx);
> +                       goto out_pm;
> +               }
> +
> +               gsi_sz = sizeof(struct spi_geni_gsi) * NUM_SPI_XFER;
> +               mas->gsi = devm_kzalloc(mas->dev, gsi_sz, GFP_KERNEL);
> +               if (IS_ERR_OR_NULL(mas->gsi)) {

Is it ever an error?  Just check against NULL?


> +                       dma_release_channel(mas->tx);
> +                       dma_release_channel(mas->rx);
> +                       mas->tx = NULL;
> +                       mas->rx = NULL;

ret = -ENOMEM ?


>  static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
> @@ -457,6 +765,11 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
>                 len = xfer->len / (mas->cur_bits_per_word / BITS_PER_BYTE + 1);
>         len &= TRANS_LEN_MSK;
>
> +       if (!xfer->cs_change) {
> +               if (!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))
> +                       m_param |= FRAGMENTATION;
> +       }

Why are you changing this?  It's for FIFO mode which works correctly
the way it is.  We _always_ want the FRAGMENTATION bit set because we
explicitly set the CS.  I haven't tried it, but I'd imagine this
change breaks stuff?  I'd expect all changes in setup_fifo_xfer() to
be removed from your patch.  If there's some reason you need them then
post a separate patch.


> @@ -494,13 +807,52 @@ static int spi_geni_transfer_one(struct spi_master *spi,
>                                 struct spi_transfer *xfer)
>  {
>         struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +       unsigned long timeout, jiffies;

Doesn't this shadow the global "jiffies"?


> +       int ret = 0i, i;
>
>         /* Terminate and return success for 0 byte length transfer */
>         if (!xfer->len)
> -               return 0;
> +               return ret;

It feels more documenting to just leave this as "return 0".


> +
> +       if (mas->cur_xfer_mode == GENI_SE_FIFO) {
> +               setup_fifo_xfer(xfer, mas, slv->mode, spi);

It's super important to return "1" in this case to tell the SPI core
that you left the transfer in progress.  You don't do that anymore, so
boom.


> +       } else {
> +               setup_gsi_xfer(xfer, mas, slv, spi);

This feels very non-symmetric.  In the FIFO case you just call a
function.  in the GSI case you have a whole pile of stuff inline.  Can
all the stuff below be stuck in setup_gsi_xfer() or maybe you can add
an extra wrapper function?  That means you don't need the weird goto
flow in this function...

> @@ -661,6 +1025,15 @@ static int spi_geni_probe(struct platform_device *pdev)
>         if (ret)
>                 goto spi_geni_probe_runtime_disable;
>
> +       /*
> +        * query the mode supported and set_cs for fifo mode only
> +        * for dma (gsi) mode, the gsi will set cs based on params passed in
> +        * TRE
> +        */
> +       mas->cur_xfer_mode = get_xfer_mode(spi);
> +       if (mas->cur_xfer_mode == GENI_SE_FIFO)

nit: check against != GPI mode?