[PATCH] spi: Mediatek: fixup cpu_to_le32 incorrect usage

[PATCH] spi: Mediatek: fixup cpu_to_le32 incorrect usage

[Leilk Liu]

writel() already does a cpu_to_le32 conversion, so
remove cpu_to_le32().

Signed-off-by: Leilk Liu <leilk.liu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/spi/spi-mt65xx.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index ae645fa..519f50c 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -359,11 +359,9 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
 	struct mtk_spi *mdata = spi_master_get_devdata(master);
 
 	if (mdata->tx_sgl)
-		writel((__force u32)cpu_to_le32(xfer->tx_dma),
-		       mdata->base + SPI_TX_SRC_REG);
+		writel(xfer->tx_dma, mdata->base + SPI_TX_SRC_REG);
 	if (mdata->rx_sgl)
-		writel((__force u32)cpu_to_le32(xfer->rx_dma),
-		       mdata->base + SPI_RX_DST_REG);
+		writel(xfer->rx_dma, mdata->base + SPI_RX_DST_REG);
 }
 
 static int mtk_spi_fifo_transfer(struct spi_master *master,
-- 
1.8.1.1.dirty

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Applied "spi: Mediatek: fixup cpu_to_le32 incorrect usage" to the spi tree

[Mark Brown]

The patch

   spi: Mediatek: fixup cpu_to_le32 incorrect usage

has been applied to the spi tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

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

>From 39ba928f8b34720b60c68d4c1bb274ae219ab39e Mon Sep 17 00:00:00 2001
From: Leilk Liu <leilk.liu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Date: Thu, 13 Aug 2015 20:06:41 +0800
Subject: [PATCH] spi: Mediatek: fixup cpu_to_le32 incorrect usage

writel() already does a cpu_to_le32 conversion, so
remove cpu_to_le32().

Signed-off-by: Leilk Liu <leilk.liu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-mt65xx.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index 2c41dcf..62baaad 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -359,11 +359,9 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
 	struct mtk_spi *mdata = spi_master_get_devdata(master);
 
 	if (mdata->tx_sgl)
-		writel((__force u32)cpu_to_le32(xfer->tx_dma),
-		       mdata->base + SPI_TX_SRC_REG);
+		writel(xfer->tx_dma, mdata->base + SPI_TX_SRC_REG);
 	if (mdata->rx_sgl)
-		writel((__force u32)cpu_to_le32(xfer->rx_dma),
-		       mdata->base + SPI_RX_DST_REG);
+		writel(xfer->rx_dma, mdata->base + SPI_RX_DST_REG);
 }
 
 static int mtk_spi_fifo_transfer(struct spi_master *master,
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Mediatek: fixup cpu_to_le32 incorrect usage

[Arnd Bergmann]

On Thursday 13 August 2015 20:06:41 Leilk Liu wrote:
> diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
> index ae645fa..519f50c 100644
> --- a/drivers/spi/spi-mt65xx.c
> +++ b/drivers/spi/spi-mt65xx.c
> @@ -359,11 +359,9 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
>         struct mtk_spi *mdata = spi_master_get_devdata(master);
>  
>         if (mdata->tx_sgl)
> -               writel((__force u32)cpu_to_le32(xfer->tx_dma),
> -                      mdata->base + SPI_TX_SRC_REG);
> +               writel(xfer->tx_dma, mdata->base + SPI_TX_SRC_REG);
>         if (mdata->rx_sgl)
> -               writel((__force u32)cpu_to_le32(xfer->rx_dma),
> -                      mdata->base + SPI_RX_DST_REG);
> +               writel(xfer->rx_dma, mdata->base + SPI_RX_DST_REG);
>  }
>  
>  static int mtk_spi_fifo_transfer(struct spi_master *master,
> -- 
> 

This change looks good, but after I've briefly looked at the current driver,
I found at least one more location that looks like an endianess bug:


+static int mtk_spi_fifo_transfer(struct spi_master *master,
+                                struct spi_device *spi,
+                                struct spi_transfer *xfer)
+{
...
+
+       for (i = 0; i < cnt; i++)
+               writel(*((u32 *)xfer->tx_buf + i),
+                      mdata->base + SPI_TX_DATA_REG);
+
+       mtk_spi_enable_transfer(master);
+
+       return 1;
+}

Here, you write data into a FIFO register using an accessor that converts
to little-endian. In contrast, the DMA based accessor apparently has
no byte swap, so most likely one of the two is broken on big-endian
systems. My guess is that the look above is wrong, and you need to
use iowrite32_rep() instead, so you always write into the FIFO in
the byte stream order (first byte to last byte) rather than swapping
32-bit entities.

The other suspicious part is this:

+       reg_val |= (chip_config->tx_endian << SPI_CMD_TX_ENDIAN_OFFSET);
+       reg_val |= (chip_config->rx_endian << SPI_CMD_RX_ENDIAN_OFFSET);

It's not clear what the tx_endian/rx_endian bits refer to and why you
can't just always set the register interface to little-endian. If it's
set to big-endian, you probably need to add more byte swaps in the
driver.

	Arnd