[PATCH] spi: imx: correct handling of MXC_CSPIRXDATA value endianness

* [PATCH] spi: imx: correct handling of MXC_CSPIRXDATA value endianness
@ 2023-05-26 14:03 Simon Horman
  2023-05-26 14:09 ` Ahmad Fatoum
  0 siblings, 1 reply; 4+ messages in thread
From: Simon Horman @ 2023-05-26 14:03 UTC (permalink / raw)
  To: Mark Brown, Shawn Guo, Sascha Hauer
  Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	jiada wang, linux-spi, linux-arm-kernel, Simon Horman

The existing code seems be intended to handle MXC_CSPIRXDATA values
which are in big endian. However, it seems that this is only
handled correctly in the case where the host is little endian.

First, consider the read case.

	u32 val = be32_to_cpu(readl(...))

readl() will read a 32bit value and return it after applying le32_to_cpu().
On a little endian host le32_to_cpu() is a noop. So the raw value is
returned. This is then converted from big endian to host byte-order -
the value is byte-swapped - using be32_to_cpu(). Assuming the raw value
is big endian a host byte-order value is obtained. This seems correct.

However, on a big endian system, le32_to_cpu() will perform a byte-swap,
while be32_to_cpu() is a noop. Assuming the underlying value is big
endian this is incorrect, because it should not be byte-swapped to
obtain the value in host byte-order - big endian.

Surveying other kernel code it seems that a correct approach is:

	 be32_to_cpu((__force __be32)__raw_readl(...))

Here __raw_readl() does returns the raw value, without any calls
that can alter the byte-order. And be32_to_cpu() is called to correctly
either a) swaps the byte-order on a little endian host or b) does not
swap the byte-order on a big endian host.

Second, let us consider the write case:

	val = cpu_to_be32(val);
	...
	writel(val, ...);

writel() will write the 32bit value, passed as big endian, after applying
cpu_to_le32(). On a little endian system cpu_to_le32() is a noop and
thus the big endian value is stored. This seems correct.

However, on a big endian system cpu_to_le32() will byte-swap the value.
That is, converting it from big endian to little endian. The little
endian value is then stored. This seems incorrect.

Surveying other kernel code it seems that a correct approach is:

	__raw_writel((__force u32)cpu_to_be32(val), ...);

__raw_writel() will write the value with out applying any endian
conversion functions. Thus the big endian value is written.
This seems correct for the case at hand.

This patch adopts the __raw_readl() and __raw_writel() approaches described
above. It also avoids the following, which stores a big endian value in
a host byte-order variable.

	u32 val;
	...
	val = cpu_to_be32(val);

Reported by Sparse as:

  .../spi-imx.c:410:19: warning: cast to restricted __be32
  .../spi-imx.c:439:21: warning: incorrect type in assignment (different base types)
  .../spi-imx.c:439:21:    expected unsigned int [addressable] [usertype] val
  .../spi-imx.c:439:21:    got restricted __be32 [usertype]

Compile tested only.

Fixes: 71abd29057cb ("spi: imx: Add support for SPI Slave mode")
Signed-off-by: Simon Horman <horms@kernel.org>
---
 drivers/spi/spi-imx.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index bd6ddb142b13..99c1f76e073d 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -406,7 +406,10 @@ static void spi_imx_buf_tx_swap(struct spi_imx_data *spi_imx)
 
 static void mx53_ecspi_rx_slave(struct spi_imx_data *spi_imx)
 {
-	u32 val = be32_to_cpu(readl(spi_imx->base + MXC_CSPIRXDATA));
+	u32 val;
+
+	val = be32_to_cpu((__force __be32)__raw_readl(spi_imx->base +
+						      MXC_CSPIRXDATA));
 
 	if (spi_imx->rx_buf) {
 		int n_bytes = spi_imx->slave_burst % sizeof(val);
@@ -435,13 +438,13 @@ static void mx53_ecspi_tx_slave(struct spi_imx_data *spi_imx)
 	if (spi_imx->tx_buf) {
 		memcpy(((u8 *)&val) + sizeof(val) - n_bytes,
 		       spi_imx->tx_buf, n_bytes);
-		val = cpu_to_be32(val);
 		spi_imx->tx_buf += n_bytes;
 	}
 
 	spi_imx->count -= n_bytes;
 
-	writel(val, spi_imx->base + MXC_CSPITXDATA);
+	__raw_writel((__force u32)cpu_to_be32(val),
+		     spi_imx->base + MXC_CSPITXDATA);
 }
 
 /* MX51 eCSPI */


^ permalink raw reply related	[flat|nested] 4+ messages in thread