[PATCH 1/3] spi: qcom: geni: remove unused defines

[PATCH 1/3] spi: qcom: geni: remove unused defines

[Vinod Koul]

Commit b59c122484ec ("spi: spi-geni-qcom: Add support for GPI dma")
added GPI support but also added unused defines, so remove them

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/spi/spi-geni-qcom.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index e2affaee4e76..413fa1a7a936 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -71,10 +71,6 @@
 #define GSI_CPHA		BIT(4)
 #define GSI_CPOL		BIT(5)
 
-#define MAX_TX_SG		3
-#define NUM_SPI_XFER		8
-#define SPI_XFER_TIMEOUT_MS	250
-
 struct spi_geni_master {
 	struct geni_se se;
 	struct device *dev;
-- 
2.31.1

[PATCH 2/3] spi: qcom: geni: set the error code for gpi transfer

[Vinod Koul]

Before we invoke spi_finalize_current_transfer() in
spi_gsi_callback_result() we should set the spi->cur_msg->status as
appropriate (0 for success, error otherwise).

The helps to return error on transfer and not wait till it timesout on
error

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/spi/spi-geni-qcom.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 413fa1a7a936..b9769de1f5f0 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -346,17 +346,20 @@ spi_gsi_callback_result(void *cb, const struct dmaengine_result *result)
 {
 	struct spi_master *spi = cb;
 
+	spi->cur_msg->status = -EIO;
 	if (result->result != DMA_TRANS_NOERROR) {
 		dev_err(&spi->dev, "DMA txn failed: %d\n", result->result);
 		return;
 	}
 
 	if (!result->residue) {
+		spi->cur_msg->status = 0;
 		dev_dbg(&spi->dev, "DMA txn completed\n");
-		spi_finalize_current_transfer(spi);
 	} else {
 		dev_err(&spi->dev, "DMA xfer has pending: %d\n", result->residue);
 	}
+
+	spi_finalize_current_transfer(spi);
 }
 
 static int setup_gsi_xfer(struct spi_transfer *xfer, struct spi_geni_master *mas,
-- 
2.31.1

[PATCH 3/3] spi: qcom: geni: handle timeout for gpi mode

[Vinod Koul]

We missed adding handle_err for gpi mode, so add a new function
spi_geni_handle_err() which would call handle_fifo_timeout() or newly
added handle_gpi_timeout() based on mode

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/spi/spi-geni-qcom.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index b9769de1f5f0..5b6e9a6643d8 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -164,6 +164,30 @@ static void handle_fifo_timeout(struct spi_master *spi,
 	}
 }
 
+static void handle_gpi_timeout(struct spi_master *spi, struct spi_message *msg)
+{
+	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+
+	dmaengine_terminate_sync(mas->tx);
+	dmaengine_terminate_sync(mas->rx);
+}
+
+static void spi_geni_handle_err(struct spi_master *spi, struct spi_message *msg)
+{
+	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+
+	switch (mas->cur_xfer_mode) {
+	case GENI_SE_FIFO:
+		handle_fifo_timeout(spi, msg);
+		break;
+	case GENI_GPI_DMA:
+		handle_gpi_timeout(spi, msg);
+		break;
+	default:
+		dev_err(mas->dev, "Abort on Mode:%d not supported", mas->cur_xfer_mode);
+	}
+}
+
 static bool spi_geni_is_abort_still_pending(struct spi_geni_master *mas)
 {
 	struct geni_se *se = &mas->se;
@@ -921,7 +945,7 @@ static int spi_geni_probe(struct platform_device *pdev)
 	spi->can_dma = geni_can_dma;
 	spi->dma_map_dev = dev->parent;
 	spi->auto_runtime_pm = true;
-	spi->handle_err = handle_fifo_timeout;
+	spi->handle_err = spi_geni_handle_err;
 	spi->use_gpio_descriptors = true;
 
 	init_completion(&mas->cs_done);
-- 
2.31.1

Re: [PATCH 2/3] spi: qcom: geni: set the error code for gpi transfer

[Doug Anderson]

Hi,

On Wed, Nov 17, 2021 at 5:31 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> @@ -346,17 +346,20 @@ spi_gsi_callback_result(void *cb, const struct dmaengine_result *result)
>  {
>         struct spi_master *spi = cb;
>
> +       spi->cur_msg->status = -EIO;
>         if (result->result != DMA_TRANS_NOERROR) {
>                 dev_err(&spi->dev, "DMA txn failed: %d\n", result->result);
>                 return;
>         }

Don't you want to call spi_finalize_current_transfer() in the case of
a DMA error? Otherwise I think you're still going to wait for a
timeout? ...and then when you get the timeout then spi_transfer_wait()
will return -ETIMEDOUT and that will overwrite your -EIO, won't it?

-Doug

Re: [PATCH 3/3] spi: qcom: geni: handle timeout for gpi mode

[Doug Anderson]

Hi,

On Wed, Nov 17, 2021 at 5:31 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> We missed adding handle_err for gpi mode, so add a new function
> spi_geni_handle_err() which would call handle_fifo_timeout() or newly
> added handle_gpi_timeout() based on mode
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/spi/spi-geni-qcom.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)

Fixes: b59c122484ec ("spi: spi-geni-qcom: Add support for GPI dma")
Reported-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>


> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index b9769de1f5f0..5b6e9a6643d8 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -164,6 +164,30 @@ static void handle_fifo_timeout(struct spi_master *spi,
>         }
>  }
>
> +static void handle_gpi_timeout(struct spi_master *spi, struct spi_message *msg)
> +{
> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +
> +       dmaengine_terminate_sync(mas->tx);
> +       dmaengine_terminate_sync(mas->rx);
> +}
> +
> +static void spi_geni_handle_err(struct spi_master *spi, struct spi_message *msg)
> +{
> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +
> +       switch (mas->cur_xfer_mode) {
> +       case GENI_SE_FIFO:
> +               handle_fifo_timeout(spi, msg);
> +               break;
> +       case GENI_GPI_DMA:
> +               handle_gpi_timeout(spi, msg);

Slight nit that maybe you should call it handle_gpi_err() instead of
handle_gpi_timeout(). As I understand it this function will get called
for _all_ errors, including errors reported by
spi_gsi_callback_result(). So basically if you have residue then
you'll immediately finalize the transfer with -EIO in the status and
then spi_geni_handle_err() will get called. It seems a little strange
that it then goes and calls a function whose name makes it sound as if
it's only called for "timeout". (For the FIFO case we actually only
hit this for timeouts since we don't currently terminate transfers
early for errors, so the FIFO name is actually OK).

-Doug

Re: [PATCH 1/3] spi: qcom: geni: remove unused defines

[Doug Anderson]

Hi,

On Wed, Nov 17, 2021 at 5:31 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> Commit b59c122484ec ("spi: spi-geni-qcom: Add support for GPI dma")
> added GPI support but also added unused defines, so remove them
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/spi/spi-geni-qcom.c | 4 ----
>  1 file changed, 4 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 2/3] spi: qcom: geni: set the error code for gpi transfer

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 360 bytes --]

On Wed, Nov 17, 2021 at 07:01:09PM +0530, Vinod Koul wrote:
> Before we invoke spi_finalize_current_transfer() in
> spi_gsi_callback_result() we should set the spi->cur_msg->status as
> appropriate (0 for success, error otherwise).

Fixes should come at the start of the patch series to make sure they can
be applied as fixes without pulling in anything else.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] spi: qcom: geni: remove unused defines

[Mark Brown]

On Wed, 17 Nov 2021 19:01:08 +0530, Vinod Koul wrote:
> Commit b59c122484ec ("spi: spi-geni-qcom: Add support for GPI dma")
> added GPI support but also added unused defines, so remove them
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/3] spi: qcom: geni: remove unused defines
      commit: 61f6e38ae8b6cbe140cfd320b3003a52147edef0
[2/3] spi: qcom: geni: set the error code for gpi transfer
      (no commit info)
[3/3] spi: qcom: geni: handle timeout for gpi mode
      (no commit info)

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH 2/3] spi: qcom: geni: set the error code for gpi transfer

[Vinod Koul]

On 17-11-21, 14:20, Doug Anderson wrote:
> Hi,
> 
> On Wed, Nov 17, 2021 at 5:31 AM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > @@ -346,17 +346,20 @@ spi_gsi_callback_result(void *cb, const struct dmaengine_result *result)
> >  {
> >         struct spi_master *spi = cb;
> >
> > +       spi->cur_msg->status = -EIO;
> >         if (result->result != DMA_TRANS_NOERROR) {
> >                 dev_err(&spi->dev, "DMA txn failed: %d\n", result->result);
> >                 return;
> >         }
> 
> Don't you want to call spi_finalize_current_transfer() in the case of
> a DMA error? Otherwise I think you're still going to wait for a
> timeout? ...and then when you get the timeout then spi_transfer_wait()
> will return -ETIMEDOUT and that will overwrite your -EIO, won't it?

Yes missed that and this reply :(

Fixed now and posting v2

-- 
~Vinod