[PATCH] spi: sun6i: Set SPI mode in prepare_message

[PATCH] spi: sun6i: Set SPI mode in prepare_message

[Kevin Mehall]

With a GPIO chip select, CS is asserted before entering transfer_one.
The spi-sun6i driver previously configured the SPI mode (including clock
polarity) and enabled the bus in transfer_one, which can cause an
extraneous SCK transition with CS asserted, corrupting the transferred
data.

This patch moves the SPI mode configuration and bus enable to the
spi_prepare_message callback, ensuring that SCK is driven to the correct
level prior to asserting CS.

A previous fix for a related issue (0d7993b234c9f) was incomplete in that
it only avoided driving SCK at the wrong level when resuming from
autosuspend, but didn't help if switching CPOL modes between chip selects
while active, or if SCK floats to the opposite level when suspended.

Fixes: 0d7993b234c9 ("spi: spi-sun6i: Fix chipselect/clock bug")
Signed-off-by: Kevin Mehall <km@kevinmehall.net>
---
 drivers/spi/spi-sun6i.c | 76 +++++++++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 29 deletions(-)

diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 240e46f84f7b..85395f1385bc 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -201,6 +201,50 @@ static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
 	return SUN6I_MAX_XFER_SIZE - 1;
 }
 
+static int sun6i_spi_prepare_message(struct spi_controller *ctlr,
+				     struct spi_message *msg)
+{
+	struct sun6i_spi *sspi = spi_controller_get_devdata(ctlr);
+	struct spi_device *spi = msg->spi;
+	u32 reg;
+
+	/*
+	 * Set up the transfer control register: Chip Select,
+	 * polarities, etc.
+	 */
+	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
+
+	if (spi->mode & SPI_CPOL)
+		reg |= SUN6I_TFR_CTL_CPOL;
+	else
+		reg &= ~SUN6I_TFR_CTL_CPOL;
+
+	if (spi->mode & SPI_CPHA)
+		reg |= SUN6I_TFR_CTL_CPHA;
+	else
+		reg &= ~SUN6I_TFR_CTL_CPHA;
+
+	if (spi->mode & SPI_LSB_FIRST)
+		reg |= SUN6I_TFR_CTL_FBS;
+	else
+		reg &= ~SUN6I_TFR_CTL_FBS;
+
+	/* We want to control the chip select manually */
+	reg |= SUN6I_TFR_CTL_CS_MANUAL;
+
+	sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
+
+	/*
+	 * Now that the clock polarity is configured, enable the bus if the
+	 * controller was previously suspended.
+	 */
+	reg = sun6i_spi_read(sspi, SUN6I_GBL_CTL_REG);
+	reg |= SUN6I_GBL_CTL_BUS_ENABLE;
+	sun6i_spi_write(sspi, SUN6I_GBL_CTL_REG, reg);
+
+	return 0;
+}
+
 static void sun6i_spi_dma_rx_cb(void *param)
 {
 	struct sun6i_spi *sspi = param;
@@ -332,31 +376,12 @@ static int sun6i_spi_transfer_one(struct spi_controller *host,
 
 	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG, reg);
 
-	/*
-	 * Setup the transfer control register: Chip Select,
-	 * polarities, etc.
-	 */
-	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
-
-	if (spi->mode & SPI_CPOL)
-		reg |= SUN6I_TFR_CTL_CPOL;
-	else
-		reg &= ~SUN6I_TFR_CTL_CPOL;
-
-	if (spi->mode & SPI_CPHA)
-		reg |= SUN6I_TFR_CTL_CPHA;
-	else
-		reg &= ~SUN6I_TFR_CTL_CPHA;
-
-	if (spi->mode & SPI_LSB_FIRST)
-		reg |= SUN6I_TFR_CTL_FBS;
-	else
-		reg &= ~SUN6I_TFR_CTL_FBS;
-
 	/*
 	 * If it's a TX only transfer, we don't want to fill the RX
 	 * FIFO with bogus data
 	 */
+	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
+
 	if (sspi->rx_buf) {
 		reg &= ~SUN6I_TFR_CTL_DHB;
 		rx_len = tfr->len;
@@ -364,9 +389,6 @@ static int sun6i_spi_transfer_one(struct spi_controller *host,
 		reg |= SUN6I_TFR_CTL_DHB;
 	}
 
-	/* We want to control the chip select manually */
-	reg |= SUN6I_TFR_CTL_CS_MANUAL;
-
 	sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
 
 	if (sspi->cfg->has_clk_ctl) {
@@ -428,11 +450,6 @@ static int sun6i_spi_transfer_one(struct spi_controller *host,
 		sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
 	}
 
-	/* Finally enable the bus - doing so before might raise SCK to HIGH */
-	reg = sun6i_spi_read(sspi, SUN6I_GBL_CTL_REG);
-	reg |= SUN6I_GBL_CTL_BUS_ENABLE;
-	sun6i_spi_write(sspi, SUN6I_GBL_CTL_REG, reg);
-
 	/* Setup the transfer now... */
 	if (sspi->tx_buf) {
 		tx_len = tfr->len;
@@ -667,6 +684,7 @@ static int sun6i_spi_probe(struct platform_device *pdev)
 	host->max_speed_hz = 100 * 1000 * 1000;
 	host->min_speed_hz = 3 * 1000;
 	host->use_gpio_descriptors = true;
+	host->prepare_message = sun6i_spi_prepare_message;
 	host->set_cs = sun6i_spi_set_cs;
 	host->transfer_one = sun6i_spi_transfer_one;
 	host->num_chipselect = 4;
-- 
2.53.0

Re: [PATCH] spi: sun6i: Set SPI mode in prepare_message

[Mark Brown]

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

On Mon, Apr 20, 2026 at 10:46:04AM -0600, Kevin Mehall wrote:

> With a GPIO chip select, CS is asserted before entering transfer_one.
> The spi-sun6i driver previously configured the SPI mode (including clock
> polarity) and enabled the bus in transfer_one, which can cause an
> extraneous SCK transition with CS asserted, corrupting the transferred
> data.

> This patch moves the SPI mode configuration and bus enable to the
> spi_prepare_message callback, ensuring that SCK is driven to the correct
> level prior to asserting CS.

> +	/* We want to control the chip select manually */
> +	reg |= SUN6I_TFR_CTL_CS_MANUAL;
> +
> +	sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);

Might this cause the native chip select to get asserted, we didn't set
up values so it'll have defaults if it wasn't previously configured?

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

Re: [PATCH] spi: sun6i: Set SPI mode in prepare_message

[Kevin Mehall]

On Wed, Apr 22, 2026, at 8:57 AM, Mark Brown wrote:

> Might this cause the native chip select to get asserted, we didn't set
> up values so it'll have defaults if it wasn't previously configured?

Per the H616 datasheet, the SUN6I_TFR_CTL_CS_MANUAL bit (which it calls
SS_OWNER) is documented as:

> Usually, controller sends SS signal automatically with data together. When
this bit is set to 1, software must manually write SPI_CTL_REG.SS_LEVEL to 1 or
0 to control the level of SS signal.

SS_LEVEL (aka SUN6I_TFR_CTL_CS_LEVEL) resets to 0x1 (CS high = inactive)
and is also written in sun6i_spi_set_cs(), which is called in spi_setup() via
spi_set_cs(). Thus it should be initialized before we get here the first time.
Once SUN6I_TFR_CTL_CS_MANUAL is set, it is never cleared elsewhere in the
driver, so in any case, this can only affect the first transfer.

I believe this is actually a bugfix in that case: having SUN6I_TFR_CTL_CS_MANUAL
set earlier means that the write to SUN6I_TFR_CTL_CS_LEVEL in sun6i_spi_set_cs()
takes effect immediately, whereas previously that CS falling edge would have
been deferred until sun6i_spi_transfer_one() set SUN6I_TFR_CTL_CS_MANUAL. As any
configured cs_setup delay happens between those two steps, the configured delay
would have effectively been ignored on the very first transfer, and this change
makes the first transfer work like subsequent ones.

However, what's not clear to me is why SUN6I_TFR_CTL_CS_MANUAL was in
sun6i_spi_transfer_one() in the first place. sun6i_spi_set_cs() is writing to
the same register, and it seems to me that setting that bit there would be a
more logical place to do it, though I don't think there is any functional change
vs what I have here. Let me know if you'd like me to move it to
sun6i_spi_set_cs() instead and how that should be submitted (same patch?
separate patch before the rest of this in a series? standalone patch to be
applied after this one?).

Re: [PATCH] spi: sun6i: Set SPI mode in prepare_message

[Mark Brown]

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

On Wed, Apr 22, 2026 at 01:01:11PM -0600, Kevin Mehall wrote:

> SS_LEVEL (aka SUN6I_TFR_CTL_CS_LEVEL) resets to 0x1 (CS high = inactive)
> and is also written in sun6i_spi_set_cs(), which is called in spi_setup() via
> spi_set_cs(). Thus it should be initialized before we get here the first time.
> Once SUN6I_TFR_CTL_CS_MANUAL is set, it is never cleared elsewhere in the
> driver, so in any case, this can only affect the first transfer.

I do see that the driver uses a reset controller over suspend, are you
sure that setup() will be called again on resume?

> However, what's not clear to me is why SUN6I_TFR_CTL_CS_MANUAL was in
> sun6i_spi_transfer_one() in the first place. sun6i_spi_set_cs() is writing to
> the same register, and it seems to me that setting that bit there would be a
> more logical place to do it, though I don't think there is any functional change
> vs what I have here. Let me know if you'd like me to move it to
> sun6i_spi_set_cs() instead and how that should be submitted (same patch?
> separate patch before the rest of this in a series? standalone patch to be
> applied after this one?).

Smaller patches are generally better so splitting is good.  It does seem
sensible to group the chip select operations together, so long as we
ensure that everything else is stable before the chip select gets
asserted we should be good.

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

Re: [PATCH] spi: sun6i: Set SPI mode in prepare_message

[Kevin Mehall]

I tested with `spi-cs-setup-delay-ns = <1000000>;` and a hardware CS in the
device tree, and confirmed the suspected second bug: without this patch, the
first transfer after autosuspend ignores the setup delay.

> I do see that the driver uses a reset controller over suspend, are you
> sure that setup() will be called again on resume?

SUN6I_TFR_CTL_REG is indeed being reset after autosuspend because the above bug
reoccurs after a few seconds of inactivity. I am not sure if setup() is running on
resume because the datasheet reset value of SUN6I_TFR_CTL_CS_LEVEL is the same
as what would be written. Either way, it's moot if we set all CS-related bits in
the same register write.

I've moved the line that sets SUN6I_TFR_CTL_CS_MANUAL into sun6i_spi_set_cs()
rather than into sun6i_spi_prepare_message() as a separated commit in a new patch
series:
https://lore.kernel.org/linux-spi/20260423174001.2797797-1-km@kevinmehall.net/