diff mbox series

[U-Boot,RFC,01/13] serial: s5p: rework Samsung UART driver to get rid of uart.h

Message ID 20171130012511.16333-2-andre.przywara@arm.com
State RFC
Delegated to: Tom Rini
Headers show
Series Nexell S5P6818 SoC support | expand

Commit Message

Andre Przywara Nov. 30, 2017, 1:24 a.m. UTC 
At the moment the serial_s5p driver takes care of both Exynos UARTs
as well as those from older Samsung SoCs (s3c/s5p series).
Looking more closely the only difference between those two groups is
how the fractional baud rate is programmed: via a "divslot" (s3c) or as
a proper fractional value (Exynos).
Instead of intricately expressing this via a special header file (which
is otherwise identical), let's use the blessings of DT to tackle this:
The S5P series of SoCs use their own compatible string, in line with
what the official DTs from the Linux kernel do. We then switch between
divslot and fractional value based on the compatible string used.
This allows us to get rid of the uart.h header files and make the
driver more flexible.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/dts/s5pc1xx-goni.dts             |  2 +-
 arch/arm/dts/s5pc1xx-smdkc100.dts         |  2 +-
 arch/arm/mach-exynos/include/mach/uart.h  | 44 ------------------------------
 arch/arm/mach-s5pc1xx/include/mach/uart.h | 44 ------------------------------
 drivers/serial/serial_s5p.c               | 45 +++++++++++++++++++++++++++----
 5 files changed, 42 insertions(+), 95 deletions(-)
 delete mode 100644 arch/arm/mach-exynos/include/mach/uart.h
 delete mode 100644 arch/arm/mach-s5pc1xx/include/mach/uart.h

Comments

Simon Glass Dec. 2, 2017, 3:30 a.m. UTC | #1 
On 29 November 2017 at 18:24, Andre Przywara <andre.przywara@arm.com> wrote:
> At the moment the serial_s5p driver takes care of both Exynos UARTs
> as well as those from older Samsung SoCs (s3c/s5p series).
> Looking more closely the only difference between those two groups is
> how the fractional baud rate is programmed: via a "divslot" (s3c) or as
> a proper fractional value (Exynos).
> Instead of intricately expressing this via a special header file (which
> is otherwise identical), let's use the blessings of DT to tackle this:
> The S5P series of SoCs use their own compatible string, in line with
> what the official DTs from the Linux kernel do. We then switch between
> divslot and fractional value based on the compatible string used.
> This allows us to get rid of the uart.h header files and make the
> driver more flexible.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm/dts/s5pc1xx-goni.dts             |  2 +-
>  arch/arm/dts/s5pc1xx-smdkc100.dts         |  2 +-
>  arch/arm/mach-exynos/include/mach/uart.h  | 44 ------------------------------
>  arch/arm/mach-s5pc1xx/include/mach/uart.h | 44 ------------------------------
>  drivers/serial/serial_s5p.c               | 45 +++++++++++++++++++++++++++----
>  5 files changed, 42 insertions(+), 95 deletions(-)
>  delete mode 100644 arch/arm/mach-exynos/include/mach/uart.h
>  delete mode 100644 arch/arm/mach-s5pc1xx/include/mach/uart.h
>
> diff --git a/arch/arm/dts/s5pc1xx-goni.dts b/arch/arm/dts/s5pc1xx-goni.dts
> index 182325a091..964c7a6b67 100644
> --- a/arch/arm/dts/s5pc1xx-goni.dts
> +++ b/arch/arm/dts/s5pc1xx-goni.dts
> @@ -28,7 +28,7 @@
>         };
>
>         serial@e2900800 {
> -               compatible = "samsung,exynos4210-uart";
> +               compatible = "samsung,s5pv210-uart";

Does this match linux?

Apart from that:

Reviewed-by: Simon Glass <sjg@chromium.org>
Andre Przywara Dec. 4, 2017, 10:21 a.m. UTC | #2 
Hi Simon,

On 02/12/17 03:30, Simon Glass wrote:
> On 29 November 2017 at 18:24, Andre Przywara <andre.przywara@arm.com> wrote:
>> At the moment the serial_s5p driver takes care of both Exynos UARTs
>> as well as those from older Samsung SoCs (s3c/s5p series).
>> Looking more closely the only difference between those two groups is
>> how the fractional baud rate is programmed: via a "divslot" (s3c) or as
>> a proper fractional value (Exynos).
>> Instead of intricately expressing this via a special header file (which
>> is otherwise identical), let's use the blessings of DT to tackle this:
>> The S5P series of SoCs use their own compatible string, in line with
>> what the official DTs from the Linux kernel do. We then switch between
>> divslot and fractional value based on the compatible string used.
>> This allows us to get rid of the uart.h header files and make the
>> driver more flexible.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  arch/arm/dts/s5pc1xx-goni.dts             |  2 +-
>>  arch/arm/dts/s5pc1xx-smdkc100.dts         |  2 +-
>>  arch/arm/mach-exynos/include/mach/uart.h  | 44 ------------------------------
>>  arch/arm/mach-s5pc1xx/include/mach/uart.h | 44 ------------------------------
>>  drivers/serial/serial_s5p.c               | 45 +++++++++++++++++++++++++++----
>>  5 files changed, 42 insertions(+), 95 deletions(-)
>>  delete mode 100644 arch/arm/mach-exynos/include/mach/uart.h
>>  delete mode 100644 arch/arm/mach-s5pc1xx/include/mach/uart.h
>>
>> diff --git a/arch/arm/dts/s5pc1xx-goni.dts b/arch/arm/dts/s5pc1xx-goni.dts
>> index 182325a091..964c7a6b67 100644
>> --- a/arch/arm/dts/s5pc1xx-goni.dts
>> +++ b/arch/arm/dts/s5pc1xx-goni.dts
>> @@ -28,7 +28,7 @@
>>         };
>>
>>         serial@e2900800 {
>> -               compatible = "samsung,exynos4210-uart";
>> +               compatible = "samsung,s5pv210-uart";
> 
> Does this match linux?

Yes, this is where I got the compatible from:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/s5pv210.dtsi#n331

And s5pv210.dtsi is included by s5pv210-goni.dts.

> Apart from that:
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

Thanks a ton for the complete review! This is much appreciated!

Cheers,
Andre.
diff mbox series

Patch

diff --git a/arch/arm/dts/s5pc1xx-goni.dts b/arch/arm/dts/s5pc1xx-goni.dts
index 182325a091..964c7a6b67 100644
--- a/arch/arm/dts/s5pc1xx-goni.dts
+++ b/arch/arm/dts/s5pc1xx-goni.dts
@@ -28,7 +28,7 @@ 
 	};
 
 	serial@e2900800 {
-		compatible = "samsung,exynos4210-uart";
+		compatible = "samsung,s5pv210-uart";
 		reg = <0xe2900800 0x400>;
 		id = <2>;
 	};
diff --git a/arch/arm/dts/s5pc1xx-smdkc100.dts b/arch/arm/dts/s5pc1xx-smdkc100.dts
index 95f15ed48d..9f813eadbc 100644
--- a/arch/arm/dts/s5pc1xx-smdkc100.dts
+++ b/arch/arm/dts/s5pc1xx-smdkc100.dts
@@ -27,7 +27,7 @@ 
 	};
 
 	serial@ec000000 {
-		compatible = "samsung,exynos4210-uart";
+		compatible = "samsung,s5pv210-uart";
 		reg = <0xec000000 0x100>;
 		interrupts = <0 51 0>;
 		id = <0>;
diff --git a/arch/arm/mach-exynos/include/mach/uart.h b/arch/arm/mach-exynos/include/mach/uart.h
deleted file mode 100644
index 33d6ba3b64..0000000000
--- a/arch/arm/mach-exynos/include/mach/uart.h
+++ /dev/null
@@ -1,44 +0,0 @@ 
-/*
- * (C) Copyright 2009 Samsung Electronics
- * Minkyu Kang <mk7.kang@samsung.com>
- * Heungjun Kim <riverful.kim@samsung.com>
- *
- * SPDX-License-Identifier:	GPL-2.0+
- */
-
-#ifndef __ASM_ARCH_UART_H_
-#define __ASM_ARCH_UART_H_
-
-#ifndef __ASSEMBLY__
-/* baudrate rest value */
-union br_rest {
-	unsigned short	slot;		/* udivslot */
-	unsigned char	value;		/* ufracval */
-};
-
-struct s5p_uart {
-	unsigned int	ulcon;
-	unsigned int	ucon;
-	unsigned int	ufcon;
-	unsigned int	umcon;
-	unsigned int	utrstat;
-	unsigned int	uerstat;
-	unsigned int	ufstat;
-	unsigned int	umstat;
-	unsigned char	utxh;
-	unsigned char	res1[3];
-	unsigned char	urxh;
-	unsigned char	res2[3];
-	unsigned int	ubrdiv;
-	union br_rest	rest;
-	unsigned char	res3[0xffd0];
-};
-
-static inline int s5p_uart_divslot(void)
-{
-	return 0;
-}
-
-#endif	/* __ASSEMBLY__ */
-
-#endif
diff --git a/arch/arm/mach-s5pc1xx/include/mach/uart.h b/arch/arm/mach-s5pc1xx/include/mach/uart.h
deleted file mode 100644
index 26db098842..0000000000
--- a/arch/arm/mach-s5pc1xx/include/mach/uart.h
+++ /dev/null
@@ -1,44 +0,0 @@ 
-/*
- * (C) Copyright 2009 Samsung Electronics
- * Minkyu Kang <mk7.kang@samsung.com>
- * Heungjun Kim <riverful.kim@samsung.com>
- *
- * SPDX-License-Identifier:	GPL-2.0+
- */
-
-#ifndef __ASM_ARCH_UART_H_
-#define __ASM_ARCH_UART_H_
-
-#ifndef __ASSEMBLY__
-/* baudrate rest value */
-union br_rest {
-	unsigned short	slot;		/* udivslot */
-	unsigned char	value;		/* ufracval */
-};
-
-struct s5p_uart {
-	unsigned int	ulcon;
-	unsigned int	ucon;
-	unsigned int	ufcon;
-	unsigned int	umcon;
-	unsigned int	utrstat;
-	unsigned int	uerstat;
-	unsigned int	ufstat;
-	unsigned int	umstat;
-	unsigned char	utxh;
-	unsigned char	res1[3];
-	unsigned char	urxh;
-	unsigned char	res2[3];
-	unsigned int	ubrdiv;
-	union br_rest	rest;
-	unsigned char	res3[0x3d0];
-};
-
-static inline int s5p_uart_divslot(void)
-{
-	return 1;
-}
-
-#endif	/* __ASSEMBLY__ */
-
-#endif
diff --git a/drivers/serial/serial_s5p.c b/drivers/serial/serial_s5p.c
index a2f692bf05..8f46cd149e 100644
--- a/drivers/serial/serial_s5p.c
+++ b/drivers/serial/serial_s5p.c
@@ -15,12 +15,34 @@ 
 #include <linux/compiler.h>
 #include <asm/io.h>
 #include <asm/arch/clk.h>
-#include <asm/arch/uart.h>
 #include <serial.h>
 #include <clk.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
+/* baudrate rest value */
+union br_rest {
+	unsigned short	slot;		/* udivslot */
+	unsigned char	value;		/* ufracval */
+};
+
+struct s5p_uart {
+	unsigned int	ulcon;
+	unsigned int	ucon;
+	unsigned int	ufcon;
+	unsigned int	umcon;
+	unsigned int	utrstat;
+	unsigned int	uerstat;
+	unsigned int	ufstat;
+	unsigned int	umstat;
+	unsigned char	utxh;
+	unsigned char	res1[3];
+	unsigned char	urxh;
+	unsigned char	res2[3];
+	unsigned int	ubrdiv;
+	union br_rest	rest;
+};
+
 #define RX_FIFO_COUNT_SHIFT	0
 #define RX_FIFO_COUNT_MASK	(0xff << RX_FIFO_COUNT_SHIFT)
 #define RX_FIFO_FULL		(1 << 8)
@@ -28,10 +50,13 @@  DECLARE_GLOBAL_DATA_PTR;
 #define TX_FIFO_COUNT_MASK	(0xff << TX_FIFO_COUNT_SHIFT)
 #define TX_FIFO_FULL		(1 << 24)
 
+#define UART_HAS_DIVSLOT	1
+
 /* Information about a serial port */
 struct s5p_serial_platdata {
 	struct s5p_uart *reg;  /* address of registers in physical memory */
 	u8 port_id;     /* uart port number */
+	bool has_divslot;
 };
 
 /*
@@ -72,7 +97,7 @@  static void __maybe_unused s5p_serial_init(struct s5p_uart *uart)
 }
 
 static void __maybe_unused s5p_serial_baud(struct s5p_uart *uart, uint uclk,
-					   int baudrate)
+					   int baudrate, bool has_divslot)
 {
 	u32 val;
 
@@ -80,7 +105,7 @@  static void __maybe_unused s5p_serial_baud(struct s5p_uart *uart, uint uclk,
 
 	writel(val / 16 - 1, &uart->ubrdiv);
 
-	if (s5p_uart_divslot())
+	if (has_divslot)
 		writew(udivslot[val % 16], &uart->rest.slot);
 	else
 		writeb(val % 16, &uart->rest.value);
@@ -105,7 +130,7 @@  int s5p_serial_setbrg(struct udevice *dev, int baudrate)
 	uclk = get_uart_clk(plat->port_id);
 #endif
 
-	s5p_serial_baud(uart, uclk, baudrate);
+	s5p_serial_baud(uart, uclk, baudrate, plat->has_divslot);
 
 	return 0;
 }
@@ -180,6 +205,7 @@  static int s5p_serial_pending(struct udevice *dev, bool input)
 static int s5p_serial_ofdata_to_platdata(struct udevice *dev)
 {
 	struct s5p_serial_platdata *plat = dev->platdata;
+	unsigned long driver_data;
 	fdt_addr_t addr;
 
 	addr = devfdt_get_addr(dev);
@@ -189,6 +215,10 @@  static int s5p_serial_ofdata_to_platdata(struct udevice *dev)
 	plat->reg = (struct s5p_uart *)addr;
 	plat->port_id = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
 					"id", dev->seq);
+
+	driver_data = dev_get_driver_data(dev);
+	plat->has_divslot = driver_data & UART_HAS_DIVSLOT;
+
 	return 0;
 }
 
@@ -200,7 +230,12 @@  static const struct dm_serial_ops s5p_serial_ops = {
 };
 
 static const struct udevice_id s5p_serial_ids[] = {
-	{ .compatible = "samsung,exynos4210-uart" },
+	{ .compatible = "samsung,s3c2412-uart", .data = UART_HAS_DIVSLOT },
+	{ .compatible = "samsung,s3c2440-uart", .data = UART_HAS_DIVSLOT },
+	{ .compatible = "samsung,s3c6400-uart", .data = UART_HAS_DIVSLOT },
+	{ .compatible = "samsung,s5pv210-uart", .data = UART_HAS_DIVSLOT },
+	{ .compatible = "samsung,exynos4210-uart", .data = 0 },
+	{ .compatible = "samsung,exynos5433-uart", .data = 0 },
 	{ }
 };