diff mbox series

spi: spi-synquacer: fix set_cs handling

Message ID 20210124221755.1587718-1-jassisinghbrar@gmail.com (mailing list archive)
State Superseded
Headers show
Series spi: spi-synquacer: fix set_cs handling | expand

Commit Message

Jassi Brar Jan. 24, 2021, 10:17 p.m. UTC 
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(-)

Comments

Mark Brown Jan. 25, 2021, 12:37 p.m. UTC | #1 
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.
Jassi Brar Feb. 1, 2021, 7:25 a.m. UTC | #2 
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.
diff mbox series

Patch

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,