Hi,

On Mon, Jan 11, 2021 at 7:17 AM Vinod Koul [off-list ref] 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...

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.

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 ?

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.

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...

nit: check against != GPI mode?