spi: spi-synquacer: fix set_cs handling

spi: spi-synquacer: fix set_cs handling

[jassisinghbrar]

From: Jassi Brar <jaswinder.singh@linaro.org>

Respect the set_cs() request by actually flushing the FIFOs
and start/stop the SPI instance.

Fixes: b0823ee35cf9b ("spi: Add spi driver for Socionext SynQuacer platform")
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 drivers/spi/spi-synquacer.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

-- 
2.14.2

diff --git a/drivers/spi/spi-synquacer.c b/drivers/spi/spi-synquacer.c
index f99abd85c50a..3905d1e1dea6 100644
--- a/drivers/spi/spi-synquacer.c
+++ b/drivers/spi/spi-synquacer.c
@@ -365,11 +365,6 @@ static int synquacer_spi_transfer_one(struct spi_master *master,
 	val &= ~SYNQUACER_HSSPI_DMSTOP_STOP;
 	writel(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART);
 
-	val = readl(sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG);
-	val |= SYNQUACER_HSSPI_FIFOCFG_RX_FLUSH;
-	val |= SYNQUACER_HSSPI_FIFOCFG_TX_FLUSH;
-	writel(val, sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG);
-
 	/*
 	 * See if we can transfer 4-bytes as 1 word
 	 * to maximize the FIFO buffer efficiency.
@@ -463,10 +458,6 @@ static int synquacer_spi_transfer_one(struct spi_master *master,
 			msecs_to_jiffies(SYNQUACER_HSSPI_TRANSFER_TMOUT_MSEC));
 		writel(0, sspi->regs + SYNQUACER_HSSPI_REG_RXE);
 
-		/* stop RX and clean RXFIFO */
-		val = readl(sspi->regs + SYNQUACER_HSSPI_REG_DMSTART);
-		val |= SYNQUACER_HSSPI_DMSTOP_STOP;
-		writel(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART);
 		sspi->rx_buf = buf;
 		sspi->rx_words = SYNQUACER_HSSPI_FIFO_DEPTH;
 		read_fifo(sspi);
@@ -486,11 +477,25 @@ static void synquacer_spi_set_cs(struct spi_device *spi, bool enable)
 	struct synquacer_spi *sspi = spi_master_get_devdata(spi->master);
 	u32 val;
 
+	if (!(spi->mode & SPI_CS_HIGH))
+		enable = !enable;
+
 	val = readl(sspi->regs + SYNQUACER_HSSPI_REG_DMSTART);
 	val &= ~(SYNQUACER_HSSPI_DMPSEL_CS_MASK <<
 		 SYNQUACER_HSSPI_DMPSEL_CS_SHIFT);
 	val |= spi->chip_select << SYNQUACER_HSSPI_DMPSEL_CS_SHIFT;
-	writel(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART);
+
+	if (enable)
+		val &= ~SYNQUACER_HSSPI_DMSTOP_STOP;
+	else
+		val |= SYNQUACER_HSSPI_DMSTOP_STOP;
+
+	writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_DMSTART);
+
+	val = readl(sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG);
+	val |= SYNQUACER_HSSPI_FIFOCFG_RX_FLUSH;
+	val |= SYNQUACER_HSSPI_FIFOCFG_TX_FLUSH;
+	writel(val, sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG);
 }
 
 static int synquacer_spi_wait_status_update(struct synquacer_spi *sspi,

Re: spi: spi-synquacer: fix set_cs handling

[Mark Brown]

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

On Sun, Jan 24, 2021 at 04:17:55PM -0600, jassisinghbrar@gmail.com wrote:

> Respect the set_cs() request by actually flushing the FIFOs
> and start/stop the SPI instance.

set_cs() is a request to set the chip select not flush the FIFOs or
restart the hardware - what's the actual issue here?  Transfers should
happen in the transfer callback, the driver shouldn't be assuming there
is anything going on with chip select when completing transfers.

>  	struct synquacer_spi *sspi = spi_master_get_devdata(spi->master);
>  	u32 val;
>  
> +	if (!(spi->mode & SPI_CS_HIGH))
> +		enable = !enable;
> +

Let the core handle SET_CS_HIGH, this will double invert so is buggy.
It's also not called out in the changelog.

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

Re: spi: spi-synquacer: fix set_cs handling

[Jassi Brar]

On Mon, 25 Jan 2021 at 06:37, Mark Brown <broonie@kernel.org> wrote:
>
> On Sun, Jan 24, 2021 at 04:17:55PM -0600, jassisinghbrar@gmail.com wrote:
>
> > Respect the set_cs() request by actually flushing the FIFOs
> > and start/stop the SPI instance.
>
> set_cs() is a request to set the chip select not flush the FIFOs or
> restart the hardware - what's the actual issue here?  Transfers should
> happen in the transfer callback, the driver shouldn't be assuming there
> is anything going on with chip select when completing transfers.
>
The controller has one block for each slave-select, and we need to
actually stop the block to deassert the CS. At the minimum we need to
set the DMSTOP_STOP bit.
I will revise the patch to be much easier on the eyes.

thanks.