[PATCH] exynos: serial clear and set big endian fix

[PATCH] exynos: serial clear and set big endian fix

[Mark Brown]

From: Victor Kamensky <victor.kamensky@linaro.org>

The Samsung serial driver uses __set_bit and __clear_bit functions
directly with h/w registers but these functions do not account for any
difference between the endianess of the CPU and the hardware so if
running in big endian mode we need to byte swap.  Handle this by
wrapping these functions with open coded versions when configured for
big endian.

[Rewrote commit message -- broonie]

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Signed-off-by: Mark Brown <broonie@linaro.org>
---
 drivers/tty/serial/samsung.c | 43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index 8df6c173d6ed..7191adefe40e 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -78,6 +78,41 @@ static void dbg(const char *fmt, ...)
 #define S3C24XX_SERIAL_MAJOR	204
 #define S3C24XX_SERIAL_MINOR	64
 
+#ifndef CONFIG_CPU_BIG_ENDIAN /* little endian */
+static inline void __hw_set_bit(int nr, volatile unsigned long *addr)
+{
+	__set_bit(nr, addr);
+}
+
+static inline void __hw_clear_bit(int nr, volatile unsigned long *addr)
+{
+	__clear_bit(nr, addr);
+}
+#else
+static inline void __hw_set_bit(int nr, volatile unsigned long *addr)
+{
+	unsigned long mask = BIT_MASK(nr);
+	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+	unsigned long val = le32_to_cpu(*p);
+
+	val  |= mask;
+
+	*p = cpu_to_le32(val);
+}
+
+static inline void __hw_clear_bit(int nr, volatile unsigned long *addr)
+{
+	unsigned long mask = BIT_MASK(nr);
+	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+
+	unsigned long val = le32_to_cpu(*p);
+
+	val &= ~mask;
+
+	*p = cpu_to_le32(val);
+}
+#endif
+
 /* macros to change one thing to another */
 
 #define tx_enabled(port) ((port)->unused[0])
@@ -157,7 +192,7 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
 
 	if (tx_enabled(port)) {
 		if (s3c24xx_serial_has_interrupt_mask(port))
-			__set_bit(S3C64XX_UINTM_TXD,
+			__hw_set_bit(S3C64XX_UINTM_TXD,
 				portaddrl(port, S3C64XX_UINTM));
 		else
 			disable_irq_nosync(ourport->tx_irq);
@@ -176,7 +211,7 @@ static void s3c24xx_serial_start_tx(struct uart_port *port)
 			s3c24xx_serial_rx_disable(port);
 
 		if (s3c24xx_serial_has_interrupt_mask(port))
-			__clear_bit(S3C64XX_UINTM_TXD,
+			__hw_clear_bit(S3C64XX_UINTM_TXD,
 				portaddrl(port, S3C64XX_UINTM));
 		else
 			enable_irq(ourport->tx_irq);
@@ -191,7 +226,7 @@ static void s3c24xx_serial_stop_rx(struct uart_port *port)
 	if (rx_enabled(port)) {
 		dbg("s3c24xx_serial_stop_rx: port=%p\n", port);
 		if (s3c24xx_serial_has_interrupt_mask(port))
-			__set_bit(S3C64XX_UINTM_RXD,
+			__hw_set_bit(S3C64XX_UINTM_RXD,
 				portaddrl(port, S3C64XX_UINTM));
 		else
 			disable_irq_nosync(ourport->rx_irq);
@@ -548,7 +583,7 @@ static int s3c64xx_serial_startup(struct uart_port *port)
 	ourport->tx_claimed = 1;
 
 	/* Enable Rx Interrupt */
-	__clear_bit(S3C64XX_UINTM_RXD, portaddrl(port, S3C64XX_UINTM));
+	__hw_clear_bit(S3C64XX_UINTM_RXD, portaddrl(port, S3C64XX_UINTM));
 	dbg("s3c64xx_serial_startup ok\n");
 	return ret;
 }
-- 
2.0.1

Re: [PATCH] exynos: serial clear and set big endian fix

[Arnd Bergmann]

On Monday 21 July 2014 16:50:32 Mark Brown wrote:
> +#ifndef CONFIG_CPU_BIG_ENDIAN /* little endian */
> +static inline void __hw_set_bit(int nr, volatile unsigned long *addr)
> +{
> +       __set_bit(nr, addr);
> +}
> +
> +static inline void __hw_clear_bit(int nr, volatile unsigned long *addr)
> +{
> +       __clear_bit(nr, addr);
> +}
> +#else
> +static inline void __hw_set_bit(int nr, volatile unsigned long *addr)
> +{
> +       unsigned long mask = BIT_MASK(nr);
> +       unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
> +       unsigned long val = le32_to_cpu(*p);
> +
> +       val  |= mask;
> +
> +       *p = cpu_to_le32(val);
> +}

This is still wrong, because there is an associated bug: you must never
access pointers to MMIO registers from C code.

The correct way to do it is to use the readl()/writel() functions, or
readl_relaxed()/writel_relaxed() in case of drivers that don't need
to synchronize with DMA transfers.

I think what you want is something like

static inline void __hw_set_bit(int nr, unsigned long __iomem *addr)
{
       addr += BIT_WORD(nr);
       writel_relaxed(readl_relaxed(addr) | BIT_MASK(nr), addr);
}

which is also endian-safe.

	Arnd

Re: [PATCH] exynos: serial clear and set big endian fix

[Mark Brown]

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

On Mon, Jul 21, 2014 at 06:01:51PM +0200, Arnd Bergmann wrote:

> The correct way to do it is to use the readl()/writel() functions, or
> readl_relaxed()/writel_relaxed() in case of drivers that don't need
> to synchronize with DMA transfers.

And don't need to build on minority platforms like x86 either!

> I think what you want is something like

> static inline void __hw_set_bit(int nr, unsigned long __iomem *addr)
> {
>        addr += BIT_WORD(nr);
>        writel_relaxed(readl_relaxed(addr) | BIT_MASK(nr), addr);
> }

> which is also endian-safe.

Indeed, good point.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]