All of lore.kernel.org
 help / color / mirror / Atom feed
From: Doug Anderson <dianders@chromium.org>
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>
Subject: Re: [PATCH 4/7] spi: spi-geni-qcom: Add support for GPI dma
Date: Tue, 12 Jan 2021 16:01:18 -0800	[thread overview]
Message-ID: <CAD=FV=XmfpQXhK_tKor-ta+5dqT-aq7OnV1e=VY-vMuXmUQEfQ@mail.gmail.com> (raw)
In-Reply-To: <20210111151651.1616813-5-vkoul@kernel.org>

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?

  parent reply	other threads:[~2021-01-13  0:49 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-11 15:16 [PATCH 0/7] Add and enable GPI DMA users Vinod Koul
2021-01-11 15:16 ` [PATCH 1/7] soc: qcom: geni: move GENI_IF_DISABLE_RO to common header Vinod Koul
2021-01-11 15:31   ` Bjorn Andersson
2021-01-11 15:16 ` [PATCH 2/7] soc: qcom: geni: move struct geni_wrapper to header Vinod Koul
2021-01-11 15:34   ` Bjorn Andersson
2021-01-11 17:43     ` Vinod Koul
2021-01-11 18:51       ` Bjorn Andersson
2021-01-11 15:16 ` [PATCH 3/7] soc: qcom: geni: Add support for gpi dma Vinod Koul
2021-01-11 15:40   ` Bjorn Andersson
2021-01-11 17:22     ` Vinod Koul
2021-01-13  0:01   ` Doug Anderson
2021-01-13  3:22     ` Vinod Koul
2021-01-11 15:16 ` [PATCH 4/7] spi: spi-geni-qcom: Add support for GPI dma Vinod Koul
2021-01-11 16:35   ` Mark Brown
2021-06-16  8:50     ` Vinod Koul
2021-06-16 11:35       ` Mark Brown
2021-06-16 12:02         ` Vinod Koul
2021-06-17  6:20           ` Vinod Koul
2021-01-11 17:19   ` Bjorn Andersson
2021-01-12  7:31   ` Lukas Wunner
2021-01-12 11:02   ` kernel test robot
2021-01-12 11:02     ` kernel test robot
2021-01-13  0:01   ` Doug Anderson [this message]
2021-01-13  3:24     ` Vinod Koul
2021-02-04 21:34   ` Dmitry Baryshkov
2021-01-11 15:16 ` [PATCH 5/7] i2c: qcom-geni: Add support for GPI DMA Vinod Koul
2021-01-11 18:14   ` Bjorn Andersson
2021-01-12  5:50     ` Vinod Koul
2021-01-11 15:16 ` [PATCH 6/7] arm64: dts: qcom: sdm845: Add gpi dma node Vinod Koul
2021-01-11 18:23   ` Bjorn Andersson
2021-01-12  4:21     ` Vinod Koul
2021-01-11 15:16 ` [PATCH 7/7] arm64: dts: qcom: sdm845: enable dma for spi Vinod Koul
2021-01-11 16:04   ` Konrad Dybcio
2021-01-11 17:46     ` Vinod Koul
2021-01-11 20:45       ` Konrad Dybcio
2021-01-12  4:19         ` Vinod Koul
2021-01-11 16:47   ` Doug Anderson
2021-01-11 17:56     ` Vinod Koul
2021-01-13  0:01 ` [PATCH 0/7] Add and enable GPI DMA users Doug Anderson
2021-01-13  3:03   ` Vinod Koul
2021-01-11 19:37 [PATCH 4/7] spi: spi-geni-qcom: Add support for GPI dma kernel test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAD=FV=XmfpQXhK_tKor-ta+5dqT-aq7OnV1e=VY-vMuXmUQEfQ@mail.gmail.com' \
    --to=dianders@chromium.org \
    --cc=agross@kernel.org \
    --cc=amit.pundir@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=broonie@kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=mka@chromium.org \
    --cc=sumit.semwal@linaro.org \
    --cc=vkoul@kernel.org \
    --cc=wsa@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.