[PATCH 00/18] serial: samsung: gs101 updates and winter cleanup

[PATCH 00/18] serial: samsung: gs101 updates and winter cleanup

[Tudor Ambarus]

Hi,

The patch set is intended for v6.9 and is expected to be queued through
Greg's tty tree.

The patch set includes updates for GS101 so that we infer the IO type
from the compatible. This is because the GS101 Peripheral Blocks, which
include the serial, only allow 32-bit register accesses. So instead of
specifying the reg-io-width = 4 property everywhere, deduce the iotype
from the compatible. The GS101 patches were previously proposed at:
Link: https://lore.kernel.org/linux-arm-kernel/20240109125814.3691033-1-tudor.ambarus@linaro.org/

The patch set then includes some cleanup changes that started as a
consequence of trying to reduce the memory footprint of the
``struct s3c24xx_uart_info``. For arm32 the struct was not as bad
defined as for arm64, because all its members could fit in the same
cacheline. But for arm64 we started from:

struct s3c24xx_uart_info {
	const char  *              name;                 /*     0     8 */
	enum s3c24xx_port_type     type;                 /*     8     4 */
	unsigned int               port_type;            /*    12     4 */
	unsigned int               fifosize;             /*    16     4 */

	/* XXX 4 bytes hole, try to pack */

	long unsigned int          rx_fifomask;          /*    24     8 */
	long unsigned int          rx_fifoshift;         /*    32     8 */
	long unsigned int          rx_fifofull;          /*    40     8 */
	long unsigned int          tx_fifomask;          /*    48     8 */
	long unsigned int          tx_fifoshift;         /*    56     8 */
	/* --- cacheline 1 boundary (64 bytes) --- */
	long unsigned int          tx_fifofull;          /*    64     8 */
	unsigned int               def_clk_sel;          /*    72     4 */

	/* XXX 4 bytes hole, try to pack */

	long unsigned int          num_clks;             /*    80     8 */
	long unsigned int          clksel_mask;          /*    88     8 */
	long unsigned int          clksel_shift;         /*    96     8 */
	long unsigned int          ucon_mask;            /*   104     8 */
	unsigned int               has_divslot:1;        /*   112: 0  4 */

	/* size: 120, cachelines: 2, members: 16 */
	/* sum members: 104, holes: 2, sum holes: 8 */
	/* sum bitfield members: 1 bits (0 bytes) */
	/* padding: 4 */
	/* bit_padding: 31 bits */
	/* last cacheline: 56 bytes */
};

and after the cleaning we get to:

    struct s3c24xx_uart_info {
            const char  *              name;                 /*     0     8 */
            enum s3c24xx_port_type     type;                 /*     8     4 */
            unsigned int               port_type;            /*    12     4 */
            unsigned int               fifosize;             /*    16     4 */
            u32                        rx_fifomask;          /*    20     4 */
            u32                        rx_fifoshift;         /*    24     4 */
            u32                        rx_fifofull;          /*    28     4 */
            u32                        tx_fifomask;          /*    32     4 */
            u32                        tx_fifoshift;         /*    36     4 */
            u32                        tx_fifofull;          /*    40     4 */
            u32                        clksel_mask;          /*    44     4 */
            u32                        clksel_shift;         /*    48     4 */
            u32                        ucon_mask;            /*    52     4 */
            u8                         def_clk_sel;          /*    56     1 */
            u8                         num_clks;             /*    57     1 */
            u8                         iotype;               /*    58     1 */
            u8                         has_divslot:1;        /*    59: 0  1 */
    
            /* size: 64, cachelines: 1, members: 17 */
            /* padding: 4 */
            /* bit_padding: 7 bits */
    };

Also note that sorting the include files in alphabetic order in the
driver revealed some problems that were fixed with the following
patches:
Link: https://lore.kernel.org/linux-arm-kernel/20240110074007.4020016-1-tudor.ambarus@linaro.org/
Link: https://lore.kernel.org/linux-kernel/20240109141045.3704627-1-tudor.ambarus@linaro.org/

Cheers,
ta

Tudor Ambarus (18):
  tty: serial: samsung: prepare for different IO types
  tty: serial: samsung: set UPIO_MEM32 iotype for gs101
  tty: serial: samsung: add gs101 earlycon support
  tty: serial: samsung: sort headers alphabetically
  tty: serial: samsung: explicitly include <linux/types.h>
  tty: serial: samsung: use u32 for register interactions
  tty: serial: samsung: remove braces on single statement block
  tty: serial: samsung: move open brace '{' on the next line
  tty: serial: samsung: drop superfluous comment
  tty: serial: samsung: make max_count unsigned int
  tty: serial: samsung: don't compare with zero an if (bitwise
    expression)
  tty: serial: samsung: use TIOCSER_TEMT for tx_empty()
  tty: serial: samsung: return bool for s3c24xx_serial_txempty_nofifo()
  tty: serial: samsung: return bool for s3c24xx_serial_console_txrdy()
  tty: serial: samsung: change return type for
    s3c24xx_serial_rx_fifocnt()
  tty: serial: samsung: shrink the clock selection to 8 clocks
  tty: serial: samsung: shrink port feature flags to u8
  tty: serial: samsung: shrink memory footprint of ``struct
    s3c24xx_uart_info``

 drivers/tty/serial/samsung_tty.c | 239 ++++++++++++++++++-------------
 1 file changed, 136 insertions(+), 103 deletions(-)

-- 
2.43.0.472.g3155946c3a-goog

[PATCH 01/18] tty: serial: samsung: prepare for different IO types

[Tudor Ambarus]

GS101's Connectivity Peripheral blocks (peric0/1 blocks) which
include the I3C and USI (I2C, SPI, UART) only allow 32-bit
register accesses. If using 8-bit register accesses, a SError
Interrupt is raised causing the system unusable.

Instead of specifying the reg-io-width = 4 everywhere, for each node,
the requirement should be deduced from the compatible.

Prepare the samsung tty driver to allow IO types different than
UPIO_MEM. ``struct uart_port::iotype`` is an unsigned char where all
its 8 bits are exposed to uapi. We can't make NULL checks on it to
verify if it's set, thus always set it from the driver's data.
Use u8 for the ``iotype`` member of ``struct s3c24xx_uart_info`` to
emphasize that the iotype is an 8 bit mask.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 71d17d804fda..b8fe9df20202 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -84,6 +84,7 @@ struct s3c24xx_uart_info {
 	unsigned long		clksel_mask;
 	unsigned long		clksel_shift;
 	unsigned long		ucon_mask;
+	u8			iotype;
 
 	/* uart port features */
 
@@ -1742,7 +1743,6 @@ static void s3c24xx_serial_init_port_default(int index) {
 
 	spin_lock_init(&port->lock);
 
-	port->iotype = UPIO_MEM;
 	port->uartclk = 0;
 	port->fifosize = 16;
 	port->flags = UPF_BOOT_AUTOCONF;
@@ -1989,6 +1989,8 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 		break;
 	}
 
+	ourport->port.iotype = ourport->info->iotype;
+
 	if (np) {
 		of_property_read_u32(np,
 			"samsung,uart-fifosize", &ourport->port.fifosize);
@@ -2399,6 +2401,7 @@ static const struct s3c24xx_serial_drv_data s3c6400_serial_drv_data = {
 		.name		= "Samsung S3C6400 UART",
 		.type		= TYPE_S3C6400,
 		.port_type	= PORT_S3C6400,
+		.iotype		= UPIO_MEM,
 		.fifosize	= 64,
 		.has_divslot	= 1,
 		.rx_fifomask	= S3C2440_UFSTAT_RXMASK,
@@ -2428,6 +2431,7 @@ static const struct s3c24xx_serial_drv_data s5pv210_serial_drv_data = {
 		.name		= "Samsung S5PV210 UART",
 		.type		= TYPE_S3C6400,
 		.port_type	= PORT_S3C6400,
+		.iotype		= UPIO_MEM,
 		.has_divslot	= 1,
 		.rx_fifomask	= S5PV210_UFSTAT_RXMASK,
 		.rx_fifoshift	= S5PV210_UFSTAT_RXSHIFT,
@@ -2457,6 +2461,7 @@ static const struct s3c24xx_serial_drv_data s5pv210_serial_drv_data = {
 		.name		= "Samsung Exynos UART",	\
 		.type		= TYPE_S3C6400,			\
 		.port_type	= PORT_S3C6400,			\
+		.iotype		= UPIO_MEM,			\
 		.has_divslot	= 1,				\
 		.rx_fifomask	= S5PV210_UFSTAT_RXMASK,	\
 		.rx_fifoshift	= S5PV210_UFSTAT_RXSHIFT,	\
@@ -2517,6 +2522,7 @@ static const struct s3c24xx_serial_drv_data s5l_serial_drv_data = {
 		.name		= "Apple S5L UART",
 		.type		= TYPE_APPLE_S5L,
 		.port_type	= PORT_8250,
+		.iotype		= UPIO_MEM,
 		.fifosize	= 16,
 		.rx_fifomask	= S3C2410_UFSTAT_RXMASK,
 		.rx_fifoshift	= S3C2410_UFSTAT_RXSHIFT,
@@ -2546,6 +2552,7 @@ static const struct s3c24xx_serial_drv_data artpec8_serial_drv_data = {
 		.name		= "Axis ARTPEC-8 UART",
 		.type		= TYPE_S3C6400,
 		.port_type	= PORT_S3C6400,
+		.iotype		= UPIO_MEM,
 		.fifosize	= 64,
 		.has_divslot	= 1,
 		.rx_fifomask	= S5PV210_UFSTAT_RXMASK,
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 02/18] tty: serial: samsung: set UPIO_MEM32 iotype for gs101

[Tudor Ambarus]

GS101's Connectivity Peripheral blocks (peric0/1 blocks) which
include the I3C and USI (I2C, SPI, UART) only allow 32-bit
register accesses.

Instead of specifying the reg-io-width = 4 everywhere, for each node,
the requirement should be deduced from the compatible.

Infer UPIO_MEM32 iotype from the "google,gs101-uart" compatible.
Update the uart info name to be GS101 specific in order to
differentiate from the other exynos platforms. All the other settings
are not changed.

exynos_fifoszdt_serial_drv_data was replaced by gs101_serial_drv_data
because the iotype restriction is gs101 specific and there was no other
user of exynos_fifoszdt_serial_drv_data.

Reviewed-by: Peter Griffin <peter.griffin@linaro.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 38 +++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index b8fe9df20202..20ec6ef1a52f 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -2495,25 +2495,43 @@ static const struct s3c24xx_serial_drv_data exynos850_serial_drv_data = {
 	.fifosize = { 256, 64, 64, 64 },
 };
 
-/*
- * Common drv_data struct for platforms that specify samsung,uart-fifosize in
- * device tree.
- */
-static const struct s3c24xx_serial_drv_data exynos_fifoszdt_serial_drv_data = {
-	EXYNOS_COMMON_SERIAL_DRV_DATA(),
+static const struct s3c24xx_serial_drv_data gs101_serial_drv_data = {
+	.info = {
+		.name		= "Google GS101 UART",
+		.type		= TYPE_S3C6400,
+		.port_type	= PORT_S3C6400,
+		.iotype		= UPIO_MEM32,
+		.has_divslot	= 1,
+		.rx_fifomask	= S5PV210_UFSTAT_RXMASK,
+		.rx_fifoshift	= S5PV210_UFSTAT_RXSHIFT,
+		.rx_fifofull	= S5PV210_UFSTAT_RXFULL,
+		.tx_fifofull	= S5PV210_UFSTAT_TXFULL,
+		.tx_fifomask	= S5PV210_UFSTAT_TXMASK,
+		.tx_fifoshift	= S5PV210_UFSTAT_TXSHIFT,
+		.def_clk_sel	= S3C2410_UCON_CLKSEL0,
+		.num_clks	= 1,
+		.clksel_mask	= 0,
+		.clksel_shift	= 0,
+	},
+	.def_cfg = {
+		.ucon		= S5PV210_UCON_DEFAULT,
+		.ufcon		= S5PV210_UFCON_DEFAULT,
+		.has_fracval	= 1,
+	},
+	/* samsung,uart-fifosize must be specified in the device tree. */
 	.fifosize = { 0 },
 };
 
 #define EXYNOS4210_SERIAL_DRV_DATA (&exynos4210_serial_drv_data)
 #define EXYNOS5433_SERIAL_DRV_DATA (&exynos5433_serial_drv_data)
 #define EXYNOS850_SERIAL_DRV_DATA (&exynos850_serial_drv_data)
-#define EXYNOS_FIFOSZDT_DRV_DATA (&exynos_fifoszdt_serial_drv_data)
+#define GS101_SERIAL_DRV_DATA (&gs101_serial_drv_data)
 
 #else
 #define EXYNOS4210_SERIAL_DRV_DATA NULL
 #define EXYNOS5433_SERIAL_DRV_DATA NULL
 #define EXYNOS850_SERIAL_DRV_DATA NULL
-#define EXYNOS_FIFOSZDT_DRV_DATA NULL
+#define GS101_SERIAL_DRV_DATA NULL
 #endif
 
 #ifdef CONFIG_ARCH_APPLE
@@ -2601,7 +2619,7 @@ static const struct platform_device_id s3c24xx_serial_driver_ids[] = {
 		.driver_data	= (kernel_ulong_t)ARTPEC8_SERIAL_DRV_DATA,
 	}, {
 		.name		= "gs101-uart",
-		.driver_data	= (kernel_ulong_t)EXYNOS_FIFOSZDT_DRV_DATA,
+		.driver_data	= (kernel_ulong_t)GS101_SERIAL_DRV_DATA,
 	},
 	{ },
 };
@@ -2624,7 +2642,7 @@ static const struct of_device_id s3c24xx_uart_dt_match[] = {
 	{ .compatible = "axis,artpec8-uart",
 		.data = ARTPEC8_SERIAL_DRV_DATA },
 	{ .compatible = "google,gs101-uart",
-		.data = EXYNOS_FIFOSZDT_DRV_DATA },
+		.data = GS101_SERIAL_DRV_DATA },
 	{},
 };
 MODULE_DEVICE_TABLE(of, s3c24xx_uart_dt_match);
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 03/18] tty: serial: samsung: add gs101 earlycon support

[Tudor Ambarus]

The entire bus (PERIC) on which the GS101 serial resides only allows
32-bit register accesses. The reg-io-width dt property is disallowed
for the "google,gs101-uart" compatible and instead the iotype is
inferred from the compatible. Always set UPIO_MEM32 iotype for the
gs101 earlycon.

Reviewed-by: Peter Griffin <peter.griffin@linaro.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 20ec6ef1a52f..74bc5151dad4 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -2810,6 +2810,17 @@ OF_EARLYCON_DECLARE(exynos4210, "samsung,exynos4210-uart",
 OF_EARLYCON_DECLARE(artpec8, "axis,artpec8-uart",
 			s5pv210_early_console_setup);
 
+static int __init gs101_early_console_setup(struct earlycon_device *device,
+					    const char *opt)
+{
+	/* gs101 always expects MMIO32 register accesses. */
+	device->port.iotype = UPIO_MEM32;
+
+	return s5pv210_early_console_setup(device, opt);
+}
+
+OF_EARLYCON_DECLARE(gs101, "google,gs101-uart", gs101_early_console_setup);
+
 /* Apple S5L */
 static int __init apple_s5l_early_console_setup(struct earlycon_device *device,
 						const char *opt)
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 04/18] tty: serial: samsung: sort headers alphabetically

[Tudor Ambarus]

Sorting headers alphabetically helps locating duplicates,
and makes it easier to figure out where to insert new headers.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 74bc5151dad4..f37d6724bfe5 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -21,26 +21,27 @@
  * BJD, 04-Nov-2004
  */
 
-#include <linux/dmaengine.h>
+#include <linux/console.h>
+#include <linux/clk.h>
+#include <linux/cpufreq.h>
+#include <linux/delay.h>
 #include <linux/dma-mapping.h>
-#include <linux/slab.h>
+#include <linux/dmaengine.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
 #include <linux/math.h>
 #include <linux/module.h>
-#include <linux/ioport.h>
-#include <linux/io.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
-#include <linux/init.h>
+#include <linux/serial.h>
+#include <linux/serial_core.h>
+#include <linux/serial_s3c.h>
+#include <linux/slab.h>
 #include <linux/sysrq.h>
-#include <linux/console.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
-#include <linux/serial_core.h>
-#include <linux/serial.h>
-#include <linux/serial_s3c.h>
-#include <linux/delay.h>
-#include <linux/clk.h>
-#include <linux/cpufreq.h>
-#include <linux/of.h>
+
 #include <asm/irq.h>
 
 /* UART name and device definitions */
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 05/18] tty: serial: samsung: explicitly include <linux/types.h>

[Tudor Ambarus]

samsung_tty.c uses u32 and relies on <linux/console.h> to include
<linux/types.h>. Explicitly include <linux/types.h>. We shall aim to
have the driver self contained.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index f37d6724bfe5..b8b71a0109ea 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -41,6 +41,7 @@
 #include <linux/sysrq.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
+#include <linux/types.h>
 
 #include <asm/irq.h>
 
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 06/18] tty: serial: samsung: use u32 for register interactions

[Tudor Ambarus]

All registers of the IP have 32 bits. Use u32 variables when reading
or writing from/to the registers. The purpose of those variables becomes
clearer.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 79 ++++++++++++++++----------------
 1 file changed, 39 insertions(+), 40 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index b8b71a0109ea..d5f9bec24b8e 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -199,7 +199,7 @@ static void wr_reg(const struct uart_port *port, u32 reg, u32 val)
 /* Byte-order aware bit setting/clearing functions. */
 
 static inline void s3c24xx_set_bit(const struct uart_port *port, int idx,
-				   unsigned int reg)
+				   u32 reg)
 {
 	unsigned long flags;
 	u32 val;
@@ -212,7 +212,7 @@ static inline void s3c24xx_set_bit(const struct uart_port *port, int idx,
 }
 
 static inline void s3c24xx_clear_bit(const struct uart_port *port, int idx,
-				     unsigned int reg)
+				     u32 reg)
 {
 	unsigned long flags;
 	u32 val;
@@ -245,8 +245,8 @@ static void s3c24xx_serial_rx_enable(struct uart_port *port)
 {
 	struct s3c24xx_uart_port *ourport = to_ourport(port);
 	unsigned long flags;
-	unsigned int ucon, ufcon;
 	int count = 10000;
+	u32 ucon, ufcon;
 
 	uart_port_lock_irqsave(port, &flags);
 
@@ -269,7 +269,7 @@ static void s3c24xx_serial_rx_disable(struct uart_port *port)
 {
 	struct s3c24xx_uart_port *ourport = to_ourport(port);
 	unsigned long flags;
-	unsigned int ucon;
+	u32 ucon;
 
 	uart_port_lock_irqsave(port, &flags);
 
@@ -591,7 +591,7 @@ static inline const struct s3c2410_uartcfg
 }
 
 static int s3c24xx_serial_rx_fifocnt(const struct s3c24xx_uart_port *ourport,
-				     unsigned long ufstat)
+				     u32 ufstat)
 {
 	const struct s3c24xx_uart_info *info = ourport->info;
 
@@ -663,7 +663,7 @@ static void s3c64xx_start_rx_dma(struct s3c24xx_uart_port *ourport)
 static void enable_rx_dma(struct s3c24xx_uart_port *ourport)
 {
 	struct uart_port *port = &ourport->port;
-	unsigned int ucon;
+	u32 ucon;
 
 	/* set Rx mode to DMA mode */
 	ucon = rd_regl(port, S3C2410_UCON);
@@ -686,7 +686,7 @@ static void enable_rx_dma(struct s3c24xx_uart_port *ourport)
 static void enable_rx_pio(struct s3c24xx_uart_port *ourport)
 {
 	struct uart_port *port = &ourport->port;
-	unsigned int ucon;
+	u32 ucon;
 
 	/* set Rx mode to DMA mode */
 	ucon = rd_regl(port, S3C2410_UCON);
@@ -711,13 +711,14 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport);
 
 static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
 {
-	unsigned int utrstat, received;
 	struct s3c24xx_uart_port *ourport = dev_id;
 	struct uart_port *port = &ourport->port;
 	struct s3c24xx_uart_dma *dma = ourport->dma;
 	struct tty_struct *tty = tty_port_tty_get(&ourport->port.state->port);
 	struct tty_port *t = &port->state->port;
 	struct dma_tx_state state;
+	unsigned int received;
+	u32 utrstat;
 
 	utrstat = rd_regl(port, S3C2410_UTRSTAT);
 	rd_regl(port, S3C2410_UFSTAT);
@@ -759,9 +760,9 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
 static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
 {
 	struct uart_port *port = &ourport->port;
-	unsigned int ufcon, ufstat, uerstat;
 	unsigned int fifocnt = 0;
 	int max_count = port->fifosize;
+	u32 ufcon, ufstat, uerstat;
 	u8 ch, flag;
 
 	while (max_count-- > 0) {
@@ -945,7 +946,7 @@ static irqreturn_t s3c64xx_serial_handle_irq(int irq, void *id)
 {
 	const struct s3c24xx_uart_port *ourport = id;
 	const struct uart_port *port = &ourport->port;
-	unsigned int pend = rd_regl(port, S3C64XX_UINTP);
+	u32 pend = rd_regl(port, S3C64XX_UINTP);
 	irqreturn_t ret = IRQ_HANDLED;
 
 	if (pend & S3C64XX_UINTM_RXD_MSK) {
@@ -964,7 +965,7 @@ static irqreturn_t apple_serial_handle_irq(int irq, void *id)
 {
 	const struct s3c24xx_uart_port *ourport = id;
 	const struct uart_port *port = &ourport->port;
-	unsigned int pend = rd_regl(port, S3C2410_UTRSTAT);
+	u32 pend = rd_regl(port, S3C2410_UTRSTAT);
 	irqreturn_t ret = IRQ_NONE;
 
 	if (pend & (APPLE_S5L_UTRSTAT_RXTHRESH | APPLE_S5L_UTRSTAT_RXTO)) {
@@ -983,8 +984,8 @@ static irqreturn_t apple_serial_handle_irq(int irq, void *id)
 static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
 {
 	const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
-	unsigned long ufstat = rd_regl(port, S3C2410_UFSTAT);
-	unsigned long ufcon = rd_regl(port, S3C2410_UFCON);
+	u32 ufstat = rd_regl(port, S3C2410_UFSTAT);
+	u32 ufcon = rd_regl(port, S3C2410_UFCON);
 
 	if (ufcon & S3C2410_UFCON_FIFOMODE) {
 		if ((ufstat & info->tx_fifomask) != 0 ||
@@ -1000,7 +1001,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
 /* no modem control lines */
 static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
 {
-	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
+	u32 umstat = rd_reg(port, S3C2410_UMSTAT);
 
 	if (umstat & S3C2410_UMSTAT_CTS)
 		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
@@ -1010,8 +1011,8 @@ static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
 
 static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
-	unsigned int umcon = rd_regl(port, S3C2410_UMCON);
-	unsigned int ucon = rd_regl(port, S3C2410_UCON);
+	u32 umcon = rd_regl(port, S3C2410_UMCON);
+	u32 ucon = rd_regl(port, S3C2410_UCON);
 
 	if (mctrl & TIOCM_RTS)
 		umcon |= S3C2410_UMCOM_RTS_LOW;
@@ -1031,7 +1032,7 @@ static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
 static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state)
 {
 	unsigned long flags;
-	unsigned int ucon;
+	u32 ucon;
 
 	uart_port_lock_irqsave(port, &flags);
 
@@ -1189,7 +1190,7 @@ static void apple_s5l_serial_shutdown(struct uart_port *port)
 {
 	struct s3c24xx_uart_port *ourport = to_ourport(port);
 
-	unsigned int ucon;
+	u32 ucon;
 
 	ucon = rd_regl(port, S3C2410_UCON);
 	ucon &= ~(APPLE_S5L_UCON_TXTHRESH_ENA_MSK |
@@ -1215,7 +1216,7 @@ static int s3c64xx_serial_startup(struct uart_port *port)
 {
 	struct s3c24xx_uart_port *ourport = to_ourport(port);
 	unsigned long flags;
-	unsigned int ufcon;
+	u32 ufcon;
 	int ret;
 
 	wr_regl(port, S3C64XX_UINTM, 0xf);
@@ -1260,7 +1261,7 @@ static int apple_s5l_serial_startup(struct uart_port *port)
 {
 	struct s3c24xx_uart_port *ourport = to_ourport(port);
 	unsigned long flags;
-	unsigned int ufcon;
+	u32 ufcon;
 	int ret;
 
 	wr_regl(port, S3C2410_UTRSTAT, APPLE_S5L_UTRSTAT_ALL_FLAGS);
@@ -1345,7 +1346,7 @@ static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
 static inline int s3c24xx_serial_getsource(struct uart_port *port)
 {
 	const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
-	unsigned int ucon;
+	u32 ucon;
 
 	if (info->num_clks == 1)
 		return 0;
@@ -1359,7 +1360,7 @@ static void s3c24xx_serial_setsource(struct uart_port *port,
 			unsigned int clk_sel)
 {
 	const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
-	unsigned int ucon;
+	u32 ucon;
 
 	if (info->num_clks == 1)
 		return;
@@ -1476,9 +1477,8 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
 	struct clk *clk = ERR_PTR(-EINVAL);
 	unsigned long flags;
 	unsigned int baud, quot, clk_sel = 0;
-	unsigned int ulcon;
-	unsigned int umcon;
 	unsigned int udivslot = 0;
+	u32 ulcon, umcon;
 
 	/*
 	 * We don't support modem control lines.
@@ -1760,7 +1760,7 @@ static void s3c24xx_serial_resetport(struct uart_port *port,
 				     const struct s3c2410_uartcfg *cfg)
 {
 	const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
-	unsigned long ucon = rd_regl(port, S3C2410_UCON);
+	u32 ucon = rd_regl(port, S3C2410_UCON);
 
 	ucon &= (info->clksel_mask | info->ucon_mask);
 	wr_regl(port, S3C2410_UCON, ucon | cfg->ucon);
@@ -1906,7 +1906,7 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
 		wr_regl(port, S3C64XX_UINTSP, 0xf);
 		break;
 	case TYPE_APPLE_S5L: {
-		unsigned int ucon;
+		u32 ucon;
 
 		ucon = rd_regl(port, S3C2410_UCON);
 		ucon &= ~(APPLE_S5L_UCON_TXTHRESH_ENA_MSK |
@@ -2110,7 +2110,7 @@ static int s3c24xx_serial_resume_noirq(struct device *dev)
 		/* restore IRQ mask */
 		switch (ourport->info->type) {
 		case TYPE_S3C6400: {
-			unsigned int uintm = 0xf;
+			u32 uintm = 0xf;
 
 			if (ourport->tx_enabled)
 				uintm &= ~S3C64XX_UINTM_TXD_MSK;
@@ -2126,7 +2126,7 @@ static int s3c24xx_serial_resume_noirq(struct device *dev)
 			break;
 		}
 		case TYPE_APPLE_S5L: {
-			unsigned int ucon;
+			u32 ucon;
 			int ret;
 
 			ret = clk_prepare_enable(ourport->clk);
@@ -2188,10 +2188,10 @@ static const struct dev_pm_ops s3c24xx_serial_pm_ops = {
 static struct uart_port *cons_uart;
 
 static int
-s3c24xx_serial_console_txrdy(struct uart_port *port, unsigned int ufcon)
+s3c24xx_serial_console_txrdy(struct uart_port *port, u32 ufcon)
 {
 	const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
-	unsigned long ufstat, utrstat;
+	u32 ufstat, utrstat;
 
 	if (ufcon & S3C2410_UFCON_FIFOMODE) {
 		/* fifo mode - check amount of data in fifo registers... */
@@ -2207,7 +2207,7 @@ s3c24xx_serial_console_txrdy(struct uart_port *port, unsigned int ufcon)
 }
 
 static bool
-s3c24xx_port_configured(unsigned int ucon)
+s3c24xx_port_configured(u32 ucon)
 {
 	/* consider the serial port configured if the tx/rx mode set */
 	return (ucon & 0xf) != 0;
@@ -2222,7 +2222,7 @@ s3c24xx_port_configured(unsigned int ucon)
 static int s3c24xx_serial_get_poll_char(struct uart_port *port)
 {
 	const struct s3c24xx_uart_port *ourport = to_ourport(port);
-	unsigned int ufstat;
+	u32 ufstat;
 
 	ufstat = rd_regl(port, S3C2410_UFSTAT);
 	if (s3c24xx_serial_rx_fifocnt(ourport, ufstat) == 0)
@@ -2234,8 +2234,8 @@ static int s3c24xx_serial_get_poll_char(struct uart_port *port)
 static void s3c24xx_serial_put_poll_char(struct uart_port *port,
 		unsigned char c)
 {
-	unsigned int ufcon = rd_regl(port, S3C2410_UFCON);
-	unsigned int ucon = rd_regl(port, S3C2410_UCON);
+	u32 ufcon = rd_regl(port, S3C2410_UFCON);
+	u32 ucon = rd_regl(port, S3C2410_UCON);
 
 	/* not possible to xmit on unconfigured port */
 	if (!s3c24xx_port_configured(ucon))
@@ -2251,7 +2251,7 @@ static void s3c24xx_serial_put_poll_char(struct uart_port *port,
 static void
 s3c24xx_serial_console_putchar(struct uart_port *port, unsigned char ch)
 {
-	unsigned int ufcon = rd_regl(port, S3C2410_UFCON);
+	u32 ufcon = rd_regl(port, S3C2410_UFCON);
 
 	while (!s3c24xx_serial_console_txrdy(port, ufcon))
 		cpu_relax();
@@ -2262,7 +2262,7 @@ static void
 s3c24xx_serial_console_write(struct console *co, const char *s,
 			     unsigned int count)
 {
-	unsigned int ucon = rd_regl(cons_uart, S3C2410_UCON);
+	u32 ucon = rd_regl(cons_uart, S3C2410_UCON);
 	unsigned long flags;
 	bool locked = true;
 
@@ -2289,11 +2289,9 @@ s3c24xx_serial_get_options(struct uart_port *port, int *baud,
 			   int *parity, int *bits)
 {
 	struct clk *clk;
-	unsigned int ulcon;
-	unsigned int ucon;
-	unsigned int ubrdiv;
 	unsigned long rate;
 	unsigned int clk_sel;
+	u32 ulcon, ucon, ubrdiv;
 	char clk_name[MAX_CLK_NAME_LENGTH];
 
 	ulcon  = rd_regl(port, S3C2410_ULCON);
@@ -2743,7 +2741,8 @@ static int samsung_early_read(struct console *con, char *s, unsigned int n)
 {
 	struct earlycon_device *dev = con->data;
 	const struct samsung_early_console_data *data = dev->port.private_data;
-	int ch, ufstat, num_read = 0;
+	int num_read = 0;
+	u32 ch, ufstat;
 
 	while (num_read < n) {
 		ufstat = rd_regl(&dev->port, S3C2410_UFSTAT);
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 07/18] tty: serial: samsung: remove braces on single statement block

[Tudor Ambarus]

Braces {} are not necessary for single statement blocks.
Remove braces on single statement block.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index d5f9bec24b8e..11ae3a1dcdc3 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -2062,9 +2062,8 @@ static void s3c24xx_serial_remove(struct platform_device *dev)
 {
 	struct uart_port *port = s3c24xx_dev_to_port(&dev->dev);
 
-	if (port) {
+	if (port)
 		uart_remove_one_port(&s3c24xx_uart_drv, port);
-	}
 
 	uart_unregister_driver(&s3c24xx_uart_drv);
 }
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 08/18] tty: serial: samsung: move open brace '{' on the next line

[Tudor Ambarus]

Move open brace '{' following function definition on the next line.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 11ae3a1dcdc3..b9d1ef67468c 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -1740,7 +1740,8 @@ static struct uart_driver s3c24xx_uart_drv = {
 
 static struct s3c24xx_uart_port s3c24xx_serial_ports[UART_NR];
 
-static void s3c24xx_serial_init_port_default(int index) {
+static void s3c24xx_serial_init_port_default(int index)
+{
 	struct uart_port *port = &s3c24xx_serial_ports[index].port;
 
 	spin_lock_init(&port->lock);
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 09/18] tty: serial: samsung: drop superfluous comment

[Tudor Ambarus]

The comment brings no benefit as we can already see from the method's
name, ``s3c24xx_serial_pm``, that it deals with power management.
Drop the superfluous comment.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index b9d1ef67468c..90c49197efc7 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -1296,8 +1296,6 @@ static int apple_s5l_serial_startup(struct uart_port *port)
 	return ret;
 }
 
-/* power power management control */
-
 static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
 			      unsigned int old)
 {
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 10/18] tty: serial: samsung: make max_count unsigned int

[Tudor Ambarus]

``max_count`` negative values are not used. Since ``port->fifosize``
is an unsigned int, make ``max_count`` the same.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 90c49197efc7..dbbe6b8e3ceb 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -760,8 +760,8 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
 static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
 {
 	struct uart_port *port = &ourport->port;
+	unsigned int max_count = port->fifosize;
 	unsigned int fifocnt = 0;
-	int max_count = port->fifosize;
 	u32 ufcon, ufstat, uerstat;
 	u8 ch, flag;
 
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 11/18] tty: serial: samsung: don't compare with zero an if (bitwise expression)

[Tudor Ambarus]

Since an if tests the numeric value of an expression, certain coding
shortcuts can be used. The most obvious one is writing
    if (expression)
instead of
    if (expression != 0)

Since our case is a bitwise expression, it's more natural and clear to
use the ``if (expression)`` shortcut.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index dbbe6b8e3ceb..f2413da14b1d 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -988,8 +988,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
 	u32 ufcon = rd_regl(port, S3C2410_UFCON);
 
 	if (ufcon & S3C2410_UFCON_FIFOMODE) {
-		if ((ufstat & info->tx_fifomask) != 0 ||
-		    (ufstat & info->tx_fifofull))
+		if ((ufstat & info->tx_fifomask) || (ufstat & info->tx_fifofull))
 			return 0;
 
 		return 1;
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 12/18] tty: serial: samsung: use TIOCSER_TEMT for tx_empty()

[Tudor Ambarus]

The core expects for tx_empty() either TIOCSER_TEMT when the tx is
empty or 0 otherwise. Respect the core and use TIOCSER_TEMT.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index f2413da14b1d..46fba70f3d77 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -990,11 +990,10 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
 	if (ufcon & S3C2410_UFCON_FIFOMODE) {
 		if ((ufstat & info->tx_fifomask) || (ufstat & info->tx_fifofull))
 			return 0;
-
-		return 1;
+		return TIOCSER_TEMT;
 	}
 
-	return s3c24xx_serial_txempty_nofifo(port);
+	return s3c24xx_serial_txempty_nofifo(port) ? TIOCSER_TEMT : 0;
 }
 
 /* no modem control lines */
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 13/18] tty: serial: samsung: return bool for s3c24xx_serial_txempty_nofifo()

[Tudor Ambarus]

s3c24xx_serial_txempty_nofifo() returned either 0 or BIT(2), which is
counterintuitive. Make the method return bool, and return true when TX
is empty and false otherwise.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 46fba70f3d77..63e993bed296 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -236,9 +236,9 @@ static inline const char *s3c24xx_serial_portname(const struct uart_port *port)
 	return to_platform_device(port->dev)->name;
 }
 
-static int s3c24xx_serial_txempty_nofifo(const struct uart_port *port)
+static bool s3c24xx_serial_txempty_nofifo(const struct uart_port *port)
 {
-	return rd_regl(port, S3C2410_UTRSTAT) & S3C2410_UTRSTAT_TXE;
+	return !!(rd_regl(port, S3C2410_UTRSTAT) & S3C2410_UTRSTAT_TXE);
 }
 
 static void s3c24xx_serial_rx_enable(struct uart_port *port)
@@ -782,7 +782,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
 		ch = rd_reg(port, S3C2410_URXH);
 
 		if (port->flags & UPF_CONS_FLOW) {
-			int txe = s3c24xx_serial_txempty_nofifo(port);
+			bool txe = s3c24xx_serial_txempty_nofifo(port);
 
 			if (ourport->rx_enabled) {
 				if (!txe) {
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 14/18] tty: serial: samsung: return bool for s3c24xx_serial_console_txrdy()

[Tudor Ambarus]

s3c24xx_serial_console_txrdy() returned just 0 or 1 to indicate whether
the TX is empty or not. Change its return type to bool.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 63e993bed296..37c0ba2a122c 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -2183,7 +2183,7 @@ static const struct dev_pm_ops s3c24xx_serial_pm_ops = {
 
 static struct uart_port *cons_uart;
 
-static int
+static bool
 s3c24xx_serial_console_txrdy(struct uart_port *port, u32 ufcon)
 {
 	const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
@@ -2193,13 +2193,13 @@ s3c24xx_serial_console_txrdy(struct uart_port *port, u32 ufcon)
 		/* fifo mode - check amount of data in fifo registers... */
 
 		ufstat = rd_regl(port, S3C2410_UFSTAT);
-		return (ufstat & info->tx_fifofull) ? 0 : 1;
+		return !(ufstat & info->tx_fifofull);
 	}
 
 	/* in non-fifo mode, we go and use the tx buffer empty */
 
 	utrstat = rd_regl(port, S3C2410_UTRSTAT);
-	return (utrstat & S3C2410_UTRSTAT_TXE) ? 1 : 0;
+	return !!(utrstat & S3C2410_UTRSTAT_TXE);
 }
 
 static bool
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 15/18] tty: serial: samsung: change return type for s3c24xx_serial_rx_fifocnt()

[Tudor Ambarus]

Change the return type of the s3c24xx_serial_rx_fifocnt() method to
``unsigned int`` as the method only returns the fifo size and does not
handle error codes.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 37c0ba2a122c..436739cf9225 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -590,8 +590,8 @@ static inline const struct s3c2410_uartcfg
 	return ourport->cfg;
 }
 
-static int s3c24xx_serial_rx_fifocnt(const struct s3c24xx_uart_port *ourport,
-				     u32 ufstat)
+static unsigned int
+s3c24xx_serial_rx_fifocnt(const struct s3c24xx_uart_port *ourport, u32 ufstat)
 {
 	const struct s3c24xx_uart_info *info = ourport->info;
 
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 16/18] tty: serial: samsung: shrink the clock selection to 8 clocks

[Tudor Ambarus]

<linux/serial_s3c.h> provides a clock selection pool of maximum 4 clocks.
Update the driver to consider a pool selection of maximum 8 clocks. The
final scope is to reduce the memory footprint of
``struct s3c24xx_uart_info``.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 436739cf9225..5df2bcebf9fb 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -81,11 +81,11 @@ struct s3c24xx_uart_info {
 	unsigned long		tx_fifomask;
 	unsigned long		tx_fifoshift;
 	unsigned long		tx_fifofull;
-	unsigned int		def_clk_sel;
-	unsigned long		num_clks;
 	unsigned long		clksel_mask;
 	unsigned long		clksel_shift;
 	unsigned long		ucon_mask;
+	u8			def_clk_sel;
+	u8			num_clks;
 	u8			iotype;
 
 	/* uart port features */
@@ -1339,7 +1339,7 @@ static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
 
 #define MAX_CLK_NAME_LENGTH 15
 
-static inline int s3c24xx_serial_getsource(struct uart_port *port)
+static inline u8 s3c24xx_serial_getsource(struct uart_port *port)
 {
 	const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
 	u32 ucon;
@@ -1352,8 +1352,7 @@ static inline int s3c24xx_serial_getsource(struct uart_port *port)
 	return ucon >> info->clksel_shift;
 }
 
-static void s3c24xx_serial_setsource(struct uart_port *port,
-			unsigned int clk_sel)
+static void s3c24xx_serial_setsource(struct uart_port *port, u8 clk_sel)
 {
 	const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
 	u32 ucon;
@@ -1372,14 +1371,15 @@ static void s3c24xx_serial_setsource(struct uart_port *port,
 
 static unsigned int s3c24xx_serial_getclk(struct s3c24xx_uart_port *ourport,
 			unsigned int req_baud, struct clk **best_clk,
-			unsigned int *clk_num)
+			u8 *clk_num)
 {
 	const struct s3c24xx_uart_info *info = ourport->info;
 	struct clk *clk;
 	unsigned long rate;
-	unsigned int cnt, baud, quot, best_quot = 0;
+	unsigned int baud, quot, best_quot = 0;
 	char clkname[MAX_CLK_NAME_LENGTH];
 	int calc_deviation, deviation = (1 << 30) - 1;
+	u8 cnt;
 
 	for (cnt = 0; cnt < info->num_clks; cnt++) {
 		/* Keep selected clock if provided */
@@ -1472,9 +1472,10 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
 	struct s3c24xx_uart_port *ourport = to_ourport(port);
 	struct clk *clk = ERR_PTR(-EINVAL);
 	unsigned long flags;
-	unsigned int baud, quot, clk_sel = 0;
+	unsigned int baud, quot;
 	unsigned int udivslot = 0;
 	u32 ulcon, umcon;
+	u8 clk_sel = 0;
 
 	/*
 	 * We don't support modem control lines.
@@ -1775,10 +1776,9 @@ static int s3c24xx_serial_enable_baudclk(struct s3c24xx_uart_port *ourport)
 	struct device *dev = ourport->port.dev;
 	const struct s3c24xx_uart_info *info = ourport->info;
 	char clk_name[MAX_CLK_NAME_LENGTH];
-	unsigned int clk_sel;
 	struct clk *clk;
-	int clk_num;
 	int ret;
+	u8 clk_sel, clk_num;
 
 	clk_sel = ourport->cfg->clk_sel ? : info->def_clk_sel;
 	for (clk_num = 0; clk_num < info->num_clks; clk_num++) {
@@ -2286,9 +2286,9 @@ s3c24xx_serial_get_options(struct uart_port *port, int *baud,
 {
 	struct clk *clk;
 	unsigned long rate;
-	unsigned int clk_sel;
 	u32 ulcon, ucon, ubrdiv;
 	char clk_name[MAX_CLK_NAME_LENGTH];
+	u8 clk_sel;
 
 	ulcon  = rd_regl(port, S3C2410_ULCON);
 	ucon   = rd_regl(port, S3C2410_UCON);
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 17/18] tty: serial: samsung: shrink port feature flags to u8

[Tudor Ambarus]

There's a single flag defined as of now. Shrink the feature flags to u8
and aim for a better memory footprint for ``struct s3c24xx_uart_info``.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 5df2bcebf9fb..598d9fe7a492 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -90,7 +90,7 @@ struct s3c24xx_uart_info {
 
 	/* uart port features */
 
-	unsigned int		has_divslot:1;
+	u8			has_divslot:1;
 };
 
 struct s3c24xx_serial_drv_data {
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 18/18] tty: serial: samsung: shrink memory footprint of ``struct s3c24xx_uart_info``

[Tudor Ambarus]

Use u32 for the members of ``struct s3c24xx_uart_info`` that are used
for register interactions. The purpose of these members becomes clearer.

The greater benefit of this change is that it also reduces the memory
footprint of the struct, allowing 64-bit architectures to use a
single cacheline for the entire struct.

struct s3c24xx_uart_info {
	const char  *              name;                 /*     0     8 */
	enum s3c24xx_port_type     type;                 /*     8     4 */
	unsigned int               port_type;            /*    12     4 */
	unsigned int               fifosize;             /*    16     4 */
	u32                        rx_fifomask;          /*    20     4 */
	u32                        rx_fifoshift;         /*    24     4 */
	u32                        rx_fifofull;          /*    28     4 */
	u32                        tx_fifomask;          /*    32     4 */
	u32                        tx_fifoshift;         /*    36     4 */
	u32                        tx_fifofull;          /*    40     4 */
	u32                        clksel_mask;          /*    44     4 */
	u32                        clksel_shift;         /*    48     4 */
	u32                        ucon_mask;            /*    52     4 */
	u8                         def_clk_sel;          /*    56     1 */
	u8                         num_clks;             /*    57     1 */
	u8                         iotype;               /*    58     1 */
	u8                         has_divslot:1;        /*    59: 0  1 */

	/* size: 64, cachelines: 1, members: 17 */
	/* padding: 4 */
	/* bit_padding: 7 bits */
};

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 598d9fe7a492..40dceb41acb7 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -75,15 +75,15 @@ struct s3c24xx_uart_info {
 	enum s3c24xx_port_type	type;
 	unsigned int		port_type;
 	unsigned int		fifosize;
-	unsigned long		rx_fifomask;
-	unsigned long		rx_fifoshift;
-	unsigned long		rx_fifofull;
-	unsigned long		tx_fifomask;
-	unsigned long		tx_fifoshift;
-	unsigned long		tx_fifofull;
-	unsigned long		clksel_mask;
-	unsigned long		clksel_shift;
-	unsigned long		ucon_mask;
+	u32			rx_fifomask;
+	u32			rx_fifoshift;
+	u32			rx_fifofull;
+	u32			tx_fifomask;
+	u32			tx_fifoshift;
+	u32			tx_fifofull;
+	u32			clksel_mask;
+	u32			clksel_shift;
+	u32			ucon_mask;
 	u8			def_clk_sel;
 	u8			num_clks;
 	u8			iotype;
-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH 01/18] tty: serial: samsung: prepare for different IO types

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:21 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> GS101's Connectivity Peripheral blocks (peric0/1 blocks) which
> include the I3C and USI (I2C, SPI, UART) only allow 32-bit
> register accesses. If using 8-bit register accesses, a SError
> Interrupt is raised causing the system unusable.
>
> Instead of specifying the reg-io-width = 4 everywhere, for each node,
> the requirement should be deduced from the compatible.
>
> Prepare the samsung tty driver to allow IO types different than
> UPIO_MEM. ``struct uart_port::iotype`` is an unsigned char where all
> its 8 bits are exposed to uapi. We can't make NULL checks on it to
> verify if it's set, thus always set it from the driver's data.
> Use u8 for the ``iotype`` member of ``struct s3c24xx_uart_info`` to
> emphasize that the iotype is an 8 bit mask.
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/tty/serial/samsung_tty.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 71d17d804fda..b8fe9df20202 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -84,6 +84,7 @@ struct s3c24xx_uart_info {
>         unsigned long           clksel_mask;
>         unsigned long           clksel_shift;
>         unsigned long           ucon_mask;
> +       u8                      iotype;
>
>         /* uart port features */
>
> @@ -1742,7 +1743,6 @@ static void s3c24xx_serial_init_port_default(int index) {
>
>         spin_lock_init(&port->lock);
>
> -       port->iotype = UPIO_MEM;
>         port->uartclk = 0;
>         port->fifosize = 16;
>         port->flags = UPF_BOOT_AUTOCONF;
> @@ -1989,6 +1989,8 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
>                 break;
>         }
>
> +       ourport->port.iotype = ourport->info->iotype;
> +
>         if (np) {
>                 of_property_read_u32(np,
>                         "samsung,uart-fifosize", &ourport->port.fifosize);
> @@ -2399,6 +2401,7 @@ static const struct s3c24xx_serial_drv_data s3c6400_serial_drv_data = {
>                 .name           = "Samsung S3C6400 UART",
>                 .type           = TYPE_S3C6400,
>                 .port_type      = PORT_S3C6400,
> +               .iotype         = UPIO_MEM,
>                 .fifosize       = 64,
>                 .has_divslot    = 1,
>                 .rx_fifomask    = S3C2440_UFSTAT_RXMASK,
> @@ -2428,6 +2431,7 @@ static const struct s3c24xx_serial_drv_data s5pv210_serial_drv_data = {
>                 .name           = "Samsung S5PV210 UART",
>                 .type           = TYPE_S3C6400,
>                 .port_type      = PORT_S3C6400,
> +               .iotype         = UPIO_MEM,
>                 .has_divslot    = 1,
>                 .rx_fifomask    = S5PV210_UFSTAT_RXMASK,
>                 .rx_fifoshift   = S5PV210_UFSTAT_RXSHIFT,
> @@ -2457,6 +2461,7 @@ static const struct s3c24xx_serial_drv_data s5pv210_serial_drv_data = {
>                 .name           = "Samsung Exynos UART",        \
>                 .type           = TYPE_S3C6400,                 \
>                 .port_type      = PORT_S3C6400,                 \
> +               .iotype         = UPIO_MEM,                     \
>                 .has_divslot    = 1,                            \
>                 .rx_fifomask    = S5PV210_UFSTAT_RXMASK,        \
>                 .rx_fifoshift   = S5PV210_UFSTAT_RXSHIFT,       \
> @@ -2517,6 +2522,7 @@ static const struct s3c24xx_serial_drv_data s5l_serial_drv_data = {
>                 .name           = "Apple S5L UART",
>                 .type           = TYPE_APPLE_S5L,
>                 .port_type      = PORT_8250,
> +               .iotype         = UPIO_MEM,
>                 .fifosize       = 16,
>                 .rx_fifomask    = S3C2410_UFSTAT_RXMASK,
>                 .rx_fifoshift   = S3C2410_UFSTAT_RXSHIFT,
> @@ -2546,6 +2552,7 @@ static const struct s3c24xx_serial_drv_data artpec8_serial_drv_data = {
>                 .name           = "Axis ARTPEC-8 UART",
>                 .type           = TYPE_S3C6400,
>                 .port_type      = PORT_S3C6400,
> +               .iotype         = UPIO_MEM,
>                 .fifosize       = 64,
>                 .has_divslot    = 1,
>                 .rx_fifomask    = S5PV210_UFSTAT_RXMASK,
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 02/18] tty: serial: samsung: set UPIO_MEM32 iotype for gs101

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:21 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> GS101's Connectivity Peripheral blocks (peric0/1 blocks) which
> include the I3C and USI (I2C, SPI, UART) only allow 32-bit
> register accesses.
>
> Instead of specifying the reg-io-width = 4 everywhere, for each node,
> the requirement should be deduced from the compatible.
>
> Infer UPIO_MEM32 iotype from the "google,gs101-uart" compatible.
> Update the uart info name to be GS101 specific in order to
> differentiate from the other exynos platforms. All the other settings
> are not changed.
>
> exynos_fifoszdt_serial_drv_data was replaced by gs101_serial_drv_data
> because the iotype restriction is gs101 specific and there was no other
> user of exynos_fifoszdt_serial_drv_data.
>
> Reviewed-by: Peter Griffin <peter.griffin@linaro.org>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/tty/serial/samsung_tty.c | 38 +++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index b8fe9df20202..20ec6ef1a52f 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -2495,25 +2495,43 @@ static const struct s3c24xx_serial_drv_data exynos850_serial_drv_data = {
>         .fifosize = { 256, 64, 64, 64 },
>  };
>
> -/*
> - * Common drv_data struct for platforms that specify samsung,uart-fifosize in
> - * device tree.
> - */
> -static const struct s3c24xx_serial_drv_data exynos_fifoszdt_serial_drv_data = {
> -       EXYNOS_COMMON_SERIAL_DRV_DATA(),
> +static const struct s3c24xx_serial_drv_data gs101_serial_drv_data = {
> +       .info = {
> +               .name           = "Google GS101 UART",
> +               .type           = TYPE_S3C6400,
> +               .port_type      = PORT_S3C6400,
> +               .iotype         = UPIO_MEM32,
> +               .has_divslot    = 1,
> +               .rx_fifomask    = S5PV210_UFSTAT_RXMASK,
> +               .rx_fifoshift   = S5PV210_UFSTAT_RXSHIFT,
> +               .rx_fifofull    = S5PV210_UFSTAT_RXFULL,
> +               .tx_fifofull    = S5PV210_UFSTAT_TXFULL,
> +               .tx_fifomask    = S5PV210_UFSTAT_TXMASK,
> +               .tx_fifoshift   = S5PV210_UFSTAT_TXSHIFT,
> +               .def_clk_sel    = S3C2410_UCON_CLKSEL0,
> +               .num_clks       = 1,
> +               .clksel_mask    = 0,
> +               .clksel_shift   = 0,
> +       },
> +       .def_cfg = {
> +               .ucon           = S5PV210_UCON_DEFAULT,
> +               .ufcon          = S5PV210_UFCON_DEFAULT,
> +               .has_fracval    = 1,
> +       },
> +       /* samsung,uart-fifosize must be specified in the device tree. */
>         .fifosize = { 0 },
>  };
>
>  #define EXYNOS4210_SERIAL_DRV_DATA (&exynos4210_serial_drv_data)
>  #define EXYNOS5433_SERIAL_DRV_DATA (&exynos5433_serial_drv_data)
>  #define EXYNOS850_SERIAL_DRV_DATA (&exynos850_serial_drv_data)
> -#define EXYNOS_FIFOSZDT_DRV_DATA (&exynos_fifoszdt_serial_drv_data)
> +#define GS101_SERIAL_DRV_DATA (&gs101_serial_drv_data)
>
>  #else
>  #define EXYNOS4210_SERIAL_DRV_DATA NULL
>  #define EXYNOS5433_SERIAL_DRV_DATA NULL
>  #define EXYNOS850_SERIAL_DRV_DATA NULL
> -#define EXYNOS_FIFOSZDT_DRV_DATA NULL
> +#define GS101_SERIAL_DRV_DATA NULL
>  #endif
>
>  #ifdef CONFIG_ARCH_APPLE
> @@ -2601,7 +2619,7 @@ static const struct platform_device_id s3c24xx_serial_driver_ids[] = {
>                 .driver_data    = (kernel_ulong_t)ARTPEC8_SERIAL_DRV_DATA,
>         }, {
>                 .name           = "gs101-uart",
> -               .driver_data    = (kernel_ulong_t)EXYNOS_FIFOSZDT_DRV_DATA,
> +               .driver_data    = (kernel_ulong_t)GS101_SERIAL_DRV_DATA,
>         },
>         { },
>  };
> @@ -2624,7 +2642,7 @@ static const struct of_device_id s3c24xx_uart_dt_match[] = {
>         { .compatible = "axis,artpec8-uart",
>                 .data = ARTPEC8_SERIAL_DRV_DATA },
>         { .compatible = "google,gs101-uart",
> -               .data = EXYNOS_FIFOSZDT_DRV_DATA },
> +               .data = GS101_SERIAL_DRV_DATA },
>         {},
>  };
>  MODULE_DEVICE_TABLE(of, s3c24xx_uart_dt_match);
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 04/18] tty: serial: samsung: sort headers alphabetically

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:21 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Sorting headers alphabetically helps locating duplicates,
> and makes it easier to figure out where to insert new headers.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/tty/serial/samsung_tty.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 74bc5151dad4..f37d6724bfe5 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -21,26 +21,27 @@
>   * BJD, 04-Nov-2004
>   */
>
> -#include <linux/dmaengine.h>
> +#include <linux/console.h>
> +#include <linux/clk.h>
> +#include <linux/cpufreq.h>
> +#include <linux/delay.h>
>  #include <linux/dma-mapping.h>
> -#include <linux/slab.h>
> +#include <linux/dmaengine.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
>  #include <linux/math.h>
>  #include <linux/module.h>
> -#include <linux/ioport.h>
> -#include <linux/io.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
> -#include <linux/init.h>
> +#include <linux/serial.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial_s3c.h>
> +#include <linux/slab.h>
>  #include <linux/sysrq.h>
> -#include <linux/console.h>
>  #include <linux/tty.h>
>  #include <linux/tty_flip.h>
> -#include <linux/serial_core.h>
> -#include <linux/serial.h>
> -#include <linux/serial_s3c.h>
> -#include <linux/delay.h>
> -#include <linux/clk.h>
> -#include <linux/cpufreq.h>
> -#include <linux/of.h>
> +
>  #include <asm/irq.h>
>
>  /* UART name and device definitions */
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 03/18] tty: serial: samsung: add gs101 earlycon support

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:21 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> The entire bus (PERIC) on which the GS101 serial resides only allows
> 32-bit register accesses. The reg-io-width dt property is disallowed
> for the "google,gs101-uart" compatible and instead the iotype is
> inferred from the compatible. Always set UPIO_MEM32 iotype for the
> gs101 earlycon.
>
> Reviewed-by: Peter Griffin <peter.griffin@linaro.org>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/tty/serial/samsung_tty.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 20ec6ef1a52f..74bc5151dad4 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -2810,6 +2810,17 @@ OF_EARLYCON_DECLARE(exynos4210, "samsung,exynos4210-uart",
>  OF_EARLYCON_DECLARE(artpec8, "axis,artpec8-uart",
>                         s5pv210_early_console_setup);
>
> +static int __init gs101_early_console_setup(struct earlycon_device *device,
> +                                           const char *opt)
> +{
> +       /* gs101 always expects MMIO32 register accesses. */
> +       device->port.iotype = UPIO_MEM32;
> +
> +       return s5pv210_early_console_setup(device, opt);
> +}
> +
> +OF_EARLYCON_DECLARE(gs101, "google,gs101-uart", gs101_early_console_setup);
> +
>  /* Apple S5L */
>  static int __init apple_s5l_early_console_setup(struct earlycon_device *device,
>                                                 const char *opt)
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 05/18] tty: serial: samsung: explicitly include <linux/types.h>

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:22 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> samsung_tty.c uses u32 and relies on <linux/console.h> to include
> <linux/types.h>. Explicitly include <linux/types.h>. We shall aim to
> have the driver self contained.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/tty/serial/samsung_tty.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index f37d6724bfe5..b8b71a0109ea 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -41,6 +41,7 @@
>  #include <linux/sysrq.h>
>  #include <linux/tty.h>
>  #include <linux/tty_flip.h>
> +#include <linux/types.h>
>
>  #include <asm/irq.h>
>
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 06/18] tty: serial: samsung: use u32 for register interactions

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:22 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> All registers of the IP have 32 bits. Use u32 variables when reading
> or writing from/to the registers. The purpose of those variables becomes
> clearer.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/tty/serial/samsung_tty.c | 79 ++++++++++++++++----------------
>  1 file changed, 39 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index b8b71a0109ea..d5f9bec24b8e 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -199,7 +199,7 @@ static void wr_reg(const struct uart_port *port, u32 reg, u32 val)
>  /* Byte-order aware bit setting/clearing functions. */
>
>  static inline void s3c24xx_set_bit(const struct uart_port *port, int idx,
> -                                  unsigned int reg)
> +                                  u32 reg)
>  {
>         unsigned long flags;
>         u32 val;
> @@ -212,7 +212,7 @@ static inline void s3c24xx_set_bit(const struct uart_port *port, int idx,
>  }
>
>  static inline void s3c24xx_clear_bit(const struct uart_port *port, int idx,
> -                                    unsigned int reg)
> +                                    u32 reg)
>  {
>         unsigned long flags;
>         u32 val;
> @@ -245,8 +245,8 @@ static void s3c24xx_serial_rx_enable(struct uart_port *port)
>  {
>         struct s3c24xx_uart_port *ourport = to_ourport(port);
>         unsigned long flags;
> -       unsigned int ucon, ufcon;
>         int count = 10000;
> +       u32 ucon, ufcon;
>
>         uart_port_lock_irqsave(port, &flags);
>
> @@ -269,7 +269,7 @@ static void s3c24xx_serial_rx_disable(struct uart_port *port)
>  {
>         struct s3c24xx_uart_port *ourport = to_ourport(port);
>         unsigned long flags;
> -       unsigned int ucon;
> +       u32 ucon;
>
>         uart_port_lock_irqsave(port, &flags);
>
> @@ -591,7 +591,7 @@ static inline const struct s3c2410_uartcfg
>  }
>
>  static int s3c24xx_serial_rx_fifocnt(const struct s3c24xx_uart_port *ourport,
> -                                    unsigned long ufstat)
> +                                    u32 ufstat)
>  {
>         const struct s3c24xx_uart_info *info = ourport->info;
>
> @@ -663,7 +663,7 @@ static void s3c64xx_start_rx_dma(struct s3c24xx_uart_port *ourport)
>  static void enable_rx_dma(struct s3c24xx_uart_port *ourport)
>  {
>         struct uart_port *port = &ourport->port;
> -       unsigned int ucon;
> +       u32 ucon;
>
>         /* set Rx mode to DMA mode */
>         ucon = rd_regl(port, S3C2410_UCON);
> @@ -686,7 +686,7 @@ static void enable_rx_dma(struct s3c24xx_uart_port *ourport)
>  static void enable_rx_pio(struct s3c24xx_uart_port *ourport)
>  {
>         struct uart_port *port = &ourport->port;
> -       unsigned int ucon;
> +       u32 ucon;
>
>         /* set Rx mode to DMA mode */
>         ucon = rd_regl(port, S3C2410_UCON);
> @@ -711,13 +711,14 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport);
>
>  static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>  {
> -       unsigned int utrstat, received;
>         struct s3c24xx_uart_port *ourport = dev_id;
>         struct uart_port *port = &ourport->port;
>         struct s3c24xx_uart_dma *dma = ourport->dma;
>         struct tty_struct *tty = tty_port_tty_get(&ourport->port.state->port);
>         struct tty_port *t = &port->state->port;
>         struct dma_tx_state state;
> +       unsigned int received;
> +       u32 utrstat;
>
>         utrstat = rd_regl(port, S3C2410_UTRSTAT);
>         rd_regl(port, S3C2410_UFSTAT);
> @@ -759,9 +760,9 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>  static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>  {
>         struct uart_port *port = &ourport->port;
> -       unsigned int ufcon, ufstat, uerstat;
>         unsigned int fifocnt = 0;
>         int max_count = port->fifosize;
> +       u32 ufcon, ufstat, uerstat;
>         u8 ch, flag;
>
>         while (max_count-- > 0) {
> @@ -945,7 +946,7 @@ static irqreturn_t s3c64xx_serial_handle_irq(int irq, void *id)
>  {
>         const struct s3c24xx_uart_port *ourport = id;
>         const struct uart_port *port = &ourport->port;
> -       unsigned int pend = rd_regl(port, S3C64XX_UINTP);
> +       u32 pend = rd_regl(port, S3C64XX_UINTP);
>         irqreturn_t ret = IRQ_HANDLED;
>
>         if (pend & S3C64XX_UINTM_RXD_MSK) {
> @@ -964,7 +965,7 @@ static irqreturn_t apple_serial_handle_irq(int irq, void *id)
>  {
>         const struct s3c24xx_uart_port *ourport = id;
>         const struct uart_port *port = &ourport->port;
> -       unsigned int pend = rd_regl(port, S3C2410_UTRSTAT);
> +       u32 pend = rd_regl(port, S3C2410_UTRSTAT);
>         irqreturn_t ret = IRQ_NONE;
>
>         if (pend & (APPLE_S5L_UTRSTAT_RXTHRESH | APPLE_S5L_UTRSTAT_RXTO)) {
> @@ -983,8 +984,8 @@ static irqreturn_t apple_serial_handle_irq(int irq, void *id)
>  static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
>  {
>         const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
> -       unsigned long ufstat = rd_regl(port, S3C2410_UFSTAT);
> -       unsigned long ufcon = rd_regl(port, S3C2410_UFCON);
> +       u32 ufstat = rd_regl(port, S3C2410_UFSTAT);
> +       u32 ufcon = rd_regl(port, S3C2410_UFCON);
>
>         if (ufcon & S3C2410_UFCON_FIFOMODE) {
>                 if ((ufstat & info->tx_fifomask) != 0 ||
> @@ -1000,7 +1001,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
>  /* no modem control lines */
>  static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
>  {
> -       unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
> +       u32 umstat = rd_reg(port, S3C2410_UMSTAT);
>
>         if (umstat & S3C2410_UMSTAT_CTS)
>                 return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
> @@ -1010,8 +1011,8 @@ static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
>
>  static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  {
> -       unsigned int umcon = rd_regl(port, S3C2410_UMCON);
> -       unsigned int ucon = rd_regl(port, S3C2410_UCON);
> +       u32 umcon = rd_regl(port, S3C2410_UMCON);
> +       u32 ucon = rd_regl(port, S3C2410_UCON);
>
>         if (mctrl & TIOCM_RTS)
>                 umcon |= S3C2410_UMCOM_RTS_LOW;
> @@ -1031,7 +1032,7 @@ static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state)
>  {
>         unsigned long flags;
> -       unsigned int ucon;
> +       u32 ucon;
>
>         uart_port_lock_irqsave(port, &flags);
>
> @@ -1189,7 +1190,7 @@ static void apple_s5l_serial_shutdown(struct uart_port *port)
>  {
>         struct s3c24xx_uart_port *ourport = to_ourport(port);
>
> -       unsigned int ucon;
> +       u32 ucon;
>
>         ucon = rd_regl(port, S3C2410_UCON);
>         ucon &= ~(APPLE_S5L_UCON_TXTHRESH_ENA_MSK |
> @@ -1215,7 +1216,7 @@ static int s3c64xx_serial_startup(struct uart_port *port)
>  {
>         struct s3c24xx_uart_port *ourport = to_ourport(port);
>         unsigned long flags;
> -       unsigned int ufcon;
> +       u32 ufcon;
>         int ret;
>
>         wr_regl(port, S3C64XX_UINTM, 0xf);
> @@ -1260,7 +1261,7 @@ static int apple_s5l_serial_startup(struct uart_port *port)
>  {
>         struct s3c24xx_uart_port *ourport = to_ourport(port);
>         unsigned long flags;
> -       unsigned int ufcon;
> +       u32 ufcon;
>         int ret;
>
>         wr_regl(port, S3C2410_UTRSTAT, APPLE_S5L_UTRSTAT_ALL_FLAGS);
> @@ -1345,7 +1346,7 @@ static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
>  static inline int s3c24xx_serial_getsource(struct uart_port *port)
>  {
>         const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
> -       unsigned int ucon;
> +       u32 ucon;
>
>         if (info->num_clks == 1)
>                 return 0;
> @@ -1359,7 +1360,7 @@ static void s3c24xx_serial_setsource(struct uart_port *port,
>                         unsigned int clk_sel)
>  {
>         const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
> -       unsigned int ucon;
> +       u32 ucon;
>
>         if (info->num_clks == 1)
>                 return;
> @@ -1476,9 +1477,8 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
>         struct clk *clk = ERR_PTR(-EINVAL);
>         unsigned long flags;
>         unsigned int baud, quot, clk_sel = 0;
> -       unsigned int ulcon;
> -       unsigned int umcon;
>         unsigned int udivslot = 0;
> +       u32 ulcon, umcon;
>
>         /*
>          * We don't support modem control lines.
> @@ -1760,7 +1760,7 @@ static void s3c24xx_serial_resetport(struct uart_port *port,
>                                      const struct s3c2410_uartcfg *cfg)
>  {
>         const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
> -       unsigned long ucon = rd_regl(port, S3C2410_UCON);
> +       u32 ucon = rd_regl(port, S3C2410_UCON);
>
>         ucon &= (info->clksel_mask | info->ucon_mask);
>         wr_regl(port, S3C2410_UCON, ucon | cfg->ucon);
> @@ -1906,7 +1906,7 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
>                 wr_regl(port, S3C64XX_UINTSP, 0xf);
>                 break;
>         case TYPE_APPLE_S5L: {
> -               unsigned int ucon;
> +               u32 ucon;
>
>                 ucon = rd_regl(port, S3C2410_UCON);
>                 ucon &= ~(APPLE_S5L_UCON_TXTHRESH_ENA_MSK |
> @@ -2110,7 +2110,7 @@ static int s3c24xx_serial_resume_noirq(struct device *dev)
>                 /* restore IRQ mask */
>                 switch (ourport->info->type) {
>                 case TYPE_S3C6400: {
> -                       unsigned int uintm = 0xf;
> +                       u32 uintm = 0xf;
>
>                         if (ourport->tx_enabled)
>                                 uintm &= ~S3C64XX_UINTM_TXD_MSK;
> @@ -2126,7 +2126,7 @@ static int s3c24xx_serial_resume_noirq(struct device *dev)
>                         break;
>                 }
>                 case TYPE_APPLE_S5L: {
> -                       unsigned int ucon;
> +                       u32 ucon;
>                         int ret;
>
>                         ret = clk_prepare_enable(ourport->clk);
> @@ -2188,10 +2188,10 @@ static const struct dev_pm_ops s3c24xx_serial_pm_ops = {
>  static struct uart_port *cons_uart;
>
>  static int
> -s3c24xx_serial_console_txrdy(struct uart_port *port, unsigned int ufcon)
> +s3c24xx_serial_console_txrdy(struct uart_port *port, u32 ufcon)
>  {
>         const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
> -       unsigned long ufstat, utrstat;
> +       u32 ufstat, utrstat;
>
>         if (ufcon & S3C2410_UFCON_FIFOMODE) {
>                 /* fifo mode - check amount of data in fifo registers... */
> @@ -2207,7 +2207,7 @@ s3c24xx_serial_console_txrdy(struct uart_port *port, unsigned int ufcon)
>  }
>
>  static bool
> -s3c24xx_port_configured(unsigned int ucon)
> +s3c24xx_port_configured(u32 ucon)
>  {
>         /* consider the serial port configured if the tx/rx mode set */
>         return (ucon & 0xf) != 0;
> @@ -2222,7 +2222,7 @@ s3c24xx_port_configured(unsigned int ucon)
>  static int s3c24xx_serial_get_poll_char(struct uart_port *port)
>  {
>         const struct s3c24xx_uart_port *ourport = to_ourport(port);
> -       unsigned int ufstat;
> +       u32 ufstat;
>
>         ufstat = rd_regl(port, S3C2410_UFSTAT);
>         if (s3c24xx_serial_rx_fifocnt(ourport, ufstat) == 0)
> @@ -2234,8 +2234,8 @@ static int s3c24xx_serial_get_poll_char(struct uart_port *port)
>  static void s3c24xx_serial_put_poll_char(struct uart_port *port,
>                 unsigned char c)
>  {
> -       unsigned int ufcon = rd_regl(port, S3C2410_UFCON);
> -       unsigned int ucon = rd_regl(port, S3C2410_UCON);
> +       u32 ufcon = rd_regl(port, S3C2410_UFCON);
> +       u32 ucon = rd_regl(port, S3C2410_UCON);
>
>         /* not possible to xmit on unconfigured port */
>         if (!s3c24xx_port_configured(ucon))
> @@ -2251,7 +2251,7 @@ static void s3c24xx_serial_put_poll_char(struct uart_port *port,
>  static void
>  s3c24xx_serial_console_putchar(struct uart_port *port, unsigned char ch)
>  {
> -       unsigned int ufcon = rd_regl(port, S3C2410_UFCON);
> +       u32 ufcon = rd_regl(port, S3C2410_UFCON);
>
>         while (!s3c24xx_serial_console_txrdy(port, ufcon))
>                 cpu_relax();
> @@ -2262,7 +2262,7 @@ static void
>  s3c24xx_serial_console_write(struct console *co, const char *s,
>                              unsigned int count)
>  {
> -       unsigned int ucon = rd_regl(cons_uart, S3C2410_UCON);
> +       u32 ucon = rd_regl(cons_uart, S3C2410_UCON);
>         unsigned long flags;
>         bool locked = true;
>
> @@ -2289,11 +2289,9 @@ s3c24xx_serial_get_options(struct uart_port *port, int *baud,
>                            int *parity, int *bits)
>  {
>         struct clk *clk;
> -       unsigned int ulcon;
> -       unsigned int ucon;
> -       unsigned int ubrdiv;
>         unsigned long rate;
>         unsigned int clk_sel;
> +       u32 ulcon, ucon, ubrdiv;
>         char clk_name[MAX_CLK_NAME_LENGTH];
>
>         ulcon  = rd_regl(port, S3C2410_ULCON);
> @@ -2743,7 +2741,8 @@ static int samsung_early_read(struct console *con, char *s, unsigned int n)
>  {
>         struct earlycon_device *dev = con->data;
>         const struct samsung_early_console_data *data = dev->port.private_data;
> -       int ch, ufstat, num_read = 0;
> +       int num_read = 0;
> +       u32 ch, ufstat;
>
>         while (num_read < n) {
>                 ufstat = rd_regl(&dev->port, S3C2410_UFSTAT);
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 07/18] tty: serial: samsung: remove braces on single statement block

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:22 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Braces {} are not necessary for single statement blocks.
> Remove braces on single statement block.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/tty/serial/samsung_tty.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index d5f9bec24b8e..11ae3a1dcdc3 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -2062,9 +2062,8 @@ static void s3c24xx_serial_remove(struct platform_device *dev)
>  {
>         struct uart_port *port = s3c24xx_dev_to_port(&dev->dev);
>
> -       if (port) {
> +       if (port)
>                 uart_remove_one_port(&s3c24xx_uart_drv, port);
> -       }
>
>         uart_unregister_driver(&s3c24xx_uart_drv);
>  }
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 08/18] tty: serial: samsung: move open brace '{' on the next line

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:23 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Move open brace '{' following function definition on the next line.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/tty/serial/samsung_tty.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 11ae3a1dcdc3..b9d1ef67468c 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -1740,7 +1740,8 @@ static struct uart_driver s3c24xx_uart_drv = {
>
>  static struct s3c24xx_uart_port s3c24xx_serial_ports[UART_NR];
>
> -static void s3c24xx_serial_init_port_default(int index) {
> +static void s3c24xx_serial_init_port_default(int index)
> +{
>         struct uart_port *port = &s3c24xx_serial_ports[index].port;
>
>         spin_lock_init(&port->lock);
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 09/18] tty: serial: samsung: drop superfluous comment

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:23 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> The comment brings no benefit as we can already see from the method's
> name, ``s3c24xx_serial_pm``, that it deals with power management.
> Drop the superfluous comment.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/tty/serial/samsung_tty.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index b9d1ef67468c..90c49197efc7 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -1296,8 +1296,6 @@ static int apple_s5l_serial_startup(struct uart_port *port)
>         return ret;
>  }
>
> -/* power power management control */
> -
>  static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
>                               unsigned int old)
>  {
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 10/18] tty: serial: samsung: make max_count unsigned int

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:23 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> ``max_count`` negative values are not used. Since ``port->fifosize``
> is an unsigned int, make ``max_count`` the same.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/tty/serial/samsung_tty.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 90c49197efc7..dbbe6b8e3ceb 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -760,8 +760,8 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>  static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>  {
>         struct uart_port *port = &ourport->port;
> +       unsigned int max_count = port->fifosize;

What if port->fifosize is 0? Then this code below:

    while (max_count-- > 0) {

would cause int overflow, if max_count is unsigned?

>         unsigned int fifocnt = 0;
> -       int max_count = port->fifosize;
>         u32 ufcon, ufstat, uerstat;
>         u8 ch, flag;
>
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 11/18] tty: serial: samsung: don't compare with zero an if (bitwise expression)

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:24 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Since an if tests the numeric value of an expression, certain coding
> shortcuts can be used. The most obvious one is writing
>     if (expression)
> instead of
>     if (expression != 0)
>
> Since our case is a bitwise expression, it's more natural and clear to
> use the ``if (expression)`` shortcut.

Maybe the author of this code:

    (ufstat & info->tx_fifomask) != 0

just wanted to outline (logically) that the result of this bitwise
operation produces FIFO length, which he checks to have non-zero
length? Mechanically of course it doesn't matter much, and I guess
everyone can understand what's going on there even without '!= 0'
part. But it looks quite intentional to me, because in the same 'if'
block the author uses this as well:

    (ufstat & info->tx_fifofull)

without any comparison operators.

>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/tty/serial/samsung_tty.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index dbbe6b8e3ceb..f2413da14b1d 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -988,8 +988,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
>         u32 ufcon = rd_regl(port, S3C2410_UFCON);
>
>         if (ufcon & S3C2410_UFCON_FIFOMODE) {
> -               if ((ufstat & info->tx_fifomask) != 0 ||
> -                   (ufstat & info->tx_fifofull))
> +               if ((ufstat & info->tx_fifomask) || (ufstat & info->tx_fifofull))

Does this line fit into 80 characters? If no, please rework it so it
does. I guess it's also possible to get rid of superfluous braces
there, but then the code might look confusing, and I'm not sure if
checkpatch would be ok with that.

>                         return 0;
>
>                 return 1;
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 12/18] tty: serial: samsung: use TIOCSER_TEMT for tx_empty()

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:24 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> The core expects for tx_empty() either TIOCSER_TEMT when the tx is
> empty or 0 otherwise. Respect the core and use TIOCSER_TEMT.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/tty/serial/samsung_tty.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index f2413da14b1d..46fba70f3d77 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -990,11 +990,10 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
>         if (ufcon & S3C2410_UFCON_FIFOMODE) {
>                 if ((ufstat & info->tx_fifomask) || (ufstat & info->tx_fifofull))
>                         return 0;
> -
> -               return 1;
> +               return TIOCSER_TEMT;
>         }
>
> -       return s3c24xx_serial_txempty_nofifo(port);
> +       return s3c24xx_serial_txempty_nofifo(port) ? TIOCSER_TEMT : 0;

And because s3c24xx_serial_txempty_nofifo() might actually return 0x4,
and at least uart_get_lsr_info() tries to clear exactly 0x1 bit, this
brings functional change, which I think is in fact a fix. So a
"Fixed:" tag is needed here.

>  }
>
>  /* no modem control lines */
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 13/18] tty: serial: samsung: return bool for s3c24xx_serial_txempty_nofifo()

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:24 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> s3c24xx_serial_txempty_nofifo() returned either 0 or BIT(2), which is
> counterintuitive. Make the method return bool, and return true when TX
> is empty and false otherwise.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/tty/serial/samsung_tty.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 46fba70f3d77..63e993bed296 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -236,9 +236,9 @@ static inline const char *s3c24xx_serial_portname(const struct uart_port *port)
>         return to_platform_device(port->dev)->name;
>  }
>
> -static int s3c24xx_serial_txempty_nofifo(const struct uart_port *port)
> +static bool s3c24xx_serial_txempty_nofifo(const struct uart_port *port)
>  {
> -       return rd_regl(port, S3C2410_UTRSTAT) & S3C2410_UTRSTAT_TXE;
> +       return !!(rd_regl(port, S3C2410_UTRSTAT) & S3C2410_UTRSTAT_TXE);

If the function already returns bool, I'm not sure doing !! is
necessary. But I don't mind.

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  }
>
>  static void s3c24xx_serial_rx_enable(struct uart_port *port)
> @@ -782,7 +782,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>                 ch = rd_reg(port, S3C2410_URXH);
>
>                 if (port->flags & UPF_CONS_FLOW) {
> -                       int txe = s3c24xx_serial_txempty_nofifo(port);
> +                       bool txe = s3c24xx_serial_txempty_nofifo(port);
>
>                         if (ourport->rx_enabled) {
>                                 if (!txe) {
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 14/18] tty: serial: samsung: return bool for s3c24xx_serial_console_txrdy()

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> s3c24xx_serial_console_txrdy() returned just 0 or 1 to indicate whether
> the TX is empty or not. Change its return type to bool.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/tty/serial/samsung_tty.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 63e993bed296..37c0ba2a122c 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -2183,7 +2183,7 @@ static const struct dev_pm_ops s3c24xx_serial_pm_ops = {
>
>  static struct uart_port *cons_uart;
>
> -static int
> +static bool
>  s3c24xx_serial_console_txrdy(struct uart_port *port, u32 ufcon)
>  {
>         const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
> @@ -2193,13 +2193,13 @@ s3c24xx_serial_console_txrdy(struct uart_port *port, u32 ufcon)
>                 /* fifo mode - check amount of data in fifo registers... */
>
>                 ufstat = rd_regl(port, S3C2410_UFSTAT);
> -               return (ufstat & info->tx_fifofull) ? 0 : 1;
> +               return !(ufstat & info->tx_fifofull);
>         }
>
>         /* in non-fifo mode, we go and use the tx buffer empty */
>
>         utrstat = rd_regl(port, S3C2410_UTRSTAT);
> -       return (utrstat & S3C2410_UTRSTAT_TXE) ? 1 : 0;
> +       return !!(utrstat & S3C2410_UTRSTAT_TXE);

Again, personally I think !! is just clutters the code here, as the
function already returns bool. Other than that:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  }
>
>  static bool
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 15/18] tty: serial: samsung: change return type for s3c24xx_serial_rx_fifocnt()

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Change the return type of the s3c24xx_serial_rx_fifocnt() method to
> ``unsigned int`` as the method only returns the fifo size and does not
> handle error codes.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/tty/serial/samsung_tty.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 37c0ba2a122c..436739cf9225 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -590,8 +590,8 @@ static inline const struct s3c2410_uartcfg
>         return ourport->cfg;
>  }
>
> -static int s3c24xx_serial_rx_fifocnt(const struct s3c24xx_uart_port *ourport,
> -                                    u32 ufstat)
> +static unsigned int
> +s3c24xx_serial_rx_fifocnt(const struct s3c24xx_uart_port *ourport, u32 ufstat)
>  {
>         const struct s3c24xx_uart_info *info = ourport->info;
>
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 17/18] tty: serial: samsung: shrink port feature flags to u8

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> There's a single flag defined as of now. Shrink the feature flags to u8
> and aim for a better memory footprint for ``struct s3c24xx_uart_info``.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/tty/serial/samsung_tty.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 5df2bcebf9fb..598d9fe7a492 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -90,7 +90,7 @@ struct s3c24xx_uart_info {
>
>         /* uart port features */
>
> -       unsigned int            has_divslot:1;
> +       u8                      has_divslot:1;

But that's already a bit field. Why does it matter which type it is?

>  };
>
>  struct s3c24xx_serial_drv_data {
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 16/18] tty: serial: samsung: shrink the clock selection to 8 clocks

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> <linux/serial_s3c.h> provides a clock selection pool of maximum 4 clocks.

Then maybe it makes sense to turn those two field into 4-bit bit
fields? More importantly, what particular problem does this patch
solve, is this optimization really needed, and why? I'm not saying
it's not needed, just that commit message might've been more verbose
about this.

> Update the driver to consider a pool selection of maximum 8 clocks. The
> final scope is to reduce the memory footprint of
> ``struct s3c24xx_uart_info``.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/tty/serial/samsung_tty.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 436739cf9225..5df2bcebf9fb 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -81,11 +81,11 @@ struct s3c24xx_uart_info {
>         unsigned long           tx_fifomask;
>         unsigned long           tx_fifoshift;
>         unsigned long           tx_fifofull;
> -       unsigned int            def_clk_sel;
> -       unsigned long           num_clks;
>         unsigned long           clksel_mask;
>         unsigned long           clksel_shift;
>         unsigned long           ucon_mask;
> +       u8                      def_clk_sel;
> +       u8                      num_clks;
>         u8                      iotype;
>
>         /* uart port features */
> @@ -1339,7 +1339,7 @@ static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
>
>  #define MAX_CLK_NAME_LENGTH 15
>
> -static inline int s3c24xx_serial_getsource(struct uart_port *port)
> +static inline u8 s3c24xx_serial_getsource(struct uart_port *port)
>  {
>         const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
>         u32 ucon;
> @@ -1352,8 +1352,7 @@ static inline int s3c24xx_serial_getsource(struct uart_port *port)
>         return ucon >> info->clksel_shift;
>  }
>
> -static void s3c24xx_serial_setsource(struct uart_port *port,
> -                       unsigned int clk_sel)
> +static void s3c24xx_serial_setsource(struct uart_port *port, u8 clk_sel)
>  {
>         const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
>         u32 ucon;
> @@ -1372,14 +1371,15 @@ static void s3c24xx_serial_setsource(struct uart_port *port,
>
>  static unsigned int s3c24xx_serial_getclk(struct s3c24xx_uart_port *ourport,
>                         unsigned int req_baud, struct clk **best_clk,
> -                       unsigned int *clk_num)
> +                       u8 *clk_num)
>  {
>         const struct s3c24xx_uart_info *info = ourport->info;
>         struct clk *clk;
>         unsigned long rate;
> -       unsigned int cnt, baud, quot, best_quot = 0;
> +       unsigned int baud, quot, best_quot = 0;
>         char clkname[MAX_CLK_NAME_LENGTH];
>         int calc_deviation, deviation = (1 << 30) - 1;
> +       u8 cnt;
>
>         for (cnt = 0; cnt < info->num_clks; cnt++) {
>                 /* Keep selected clock if provided */
> @@ -1472,9 +1472,10 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
>         struct s3c24xx_uart_port *ourport = to_ourport(port);
>         struct clk *clk = ERR_PTR(-EINVAL);
>         unsigned long flags;
> -       unsigned int baud, quot, clk_sel = 0;
> +       unsigned int baud, quot;
>         unsigned int udivslot = 0;
>         u32 ulcon, umcon;
> +       u8 clk_sel = 0;
>
>         /*
>          * We don't support modem control lines.
> @@ -1775,10 +1776,9 @@ static int s3c24xx_serial_enable_baudclk(struct s3c24xx_uart_port *ourport)
>         struct device *dev = ourport->port.dev;
>         const struct s3c24xx_uart_info *info = ourport->info;
>         char clk_name[MAX_CLK_NAME_LENGTH];
> -       unsigned int clk_sel;
>         struct clk *clk;
> -       int clk_num;
>         int ret;
> +       u8 clk_sel, clk_num;
>
>         clk_sel = ourport->cfg->clk_sel ? : info->def_clk_sel;
>         for (clk_num = 0; clk_num < info->num_clks; clk_num++) {
> @@ -2286,9 +2286,9 @@ s3c24xx_serial_get_options(struct uart_port *port, int *baud,
>  {
>         struct clk *clk;
>         unsigned long rate;
> -       unsigned int clk_sel;
>         u32 ulcon, ucon, ubrdiv;
>         char clk_name[MAX_CLK_NAME_LENGTH];
> +       u8 clk_sel;
>
>         ulcon  = rd_regl(port, S3C2410_ULCON);
>         ucon   = rd_regl(port, S3C2410_UCON);
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 18/18] tty: serial: samsung: shrink memory footprint of ``struct s3c24xx_uart_info``

[Sam Protsenko]

On Wed, Jan 10, 2024 at 4:26 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Use u32 for the members of ``struct s3c24xx_uart_info`` that are used
> for register interactions. The purpose of these members becomes clearer.
>
> The greater benefit of this change is that it also reduces the memory
> footprint of the struct, allowing 64-bit architectures to use a
> single cacheline for the entire struct.
>
> struct s3c24xx_uart_info {
>         const char  *              name;                 /*     0     8 */
>         enum s3c24xx_port_type     type;                 /*     8     4 */
>         unsigned int               port_type;            /*    12     4 */
>         unsigned int               fifosize;             /*    16     4 */
>         u32                        rx_fifomask;          /*    20     4 */
>         u32                        rx_fifoshift;         /*    24     4 */
>         u32                        rx_fifofull;          /*    28     4 */
>         u32                        tx_fifomask;          /*    32     4 */
>         u32                        tx_fifoshift;         /*    36     4 */
>         u32                        tx_fifofull;          /*    40     4 */
>         u32                        clksel_mask;          /*    44     4 */
>         u32                        clksel_shift;         /*    48     4 */
>         u32                        ucon_mask;            /*    52     4 */
>         u8                         def_clk_sel;          /*    56     1 */
>         u8                         num_clks;             /*    57     1 */
>         u8                         iotype;               /*    58     1 */
>         u8                         has_divslot:1;        /*    59: 0  1 */
>
>         /* size: 64, cachelines: 1, members: 17 */
>         /* padding: 4 */
>         /* bit_padding: 7 bits */
> };
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/tty/serial/samsung_tty.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 598d9fe7a492..40dceb41acb7 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -75,15 +75,15 @@ struct s3c24xx_uart_info {
>         enum s3c24xx_port_type  type;
>         unsigned int            port_type;
>         unsigned int            fifosize;
> -       unsigned long           rx_fifomask;
> -       unsigned long           rx_fifoshift;
> -       unsigned long           rx_fifofull;
> -       unsigned long           tx_fifomask;
> -       unsigned long           tx_fifoshift;
> -       unsigned long           tx_fifofull;
> -       unsigned long           clksel_mask;
> -       unsigned long           clksel_shift;
> -       unsigned long           ucon_mask;
> +       u32                     rx_fifomask;
> +       u32                     rx_fifoshift;
> +       u32                     rx_fifofull;
> +       u32                     tx_fifomask;
> +       u32                     tx_fifoshift;
> +       u32                     tx_fifofull;
> +       u32                     clksel_mask;
> +       u32                     clksel_shift;
> +       u32                     ucon_mask;
>         u8                      def_clk_sel;
>         u8                      num_clks;
>         u8                      iotype;
> --
> 2.43.0.472.g3155946c3a-goog
>
>

Re: [PATCH 10/18] tty: serial: samsung: make max_count unsigned int

[Tudor Ambarus]

On 1/16/24 18:21, Sam Protsenko wrote:
> On Wed, Jan 10, 2024 at 4:23 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> ``max_count`` negative values are not used. Since ``port->fifosize``
>> is an unsigned int, make ``max_count`` the same.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>>  drivers/tty/serial/samsung_tty.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>> index 90c49197efc7..dbbe6b8e3ceb 100644
>> --- a/drivers/tty/serial/samsung_tty.c
>> +++ b/drivers/tty/serial/samsung_tty.c
>> @@ -760,8 +760,8 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>>  static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>>  {
>>         struct uart_port *port = &ourport->port;
>> +       unsigned int max_count = port->fifosize;
> 
> What if port->fifosize is 0? Then this code below:
> 
>     while (max_count-- > 0) {
> 
> would cause int overflow, if max_count is unsigned?
> 

good catch, Sam!

I'm thinking of amending this and add at the beginning of the method:

if (!max_count)
	return tty_flip_buffer_push(&port->state->port);

Thanks!
ta

Re: [PATCH 10/18] tty: serial: samsung: make max_count unsigned int

[André Draszik]

Hi,

On Wed, 2024-01-17 at 15:21 +0000, Tudor Ambarus wrote:
> 
> 
> On 1/16/24 18:21, Sam Protsenko wrote:
> > On Wed, Jan 10, 2024 at 4:23 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> > > 
> > > ``max_count`` negative values are not used. Since ``port->fifosize``
> > > is an unsigned int, make ``max_count`` the same.
> > > 
> > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> > > ---
> > >  drivers/tty/serial/samsung_tty.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> > > index 90c49197efc7..dbbe6b8e3ceb 100644
> > > --- a/drivers/tty/serial/samsung_tty.c
> > > +++ b/drivers/tty/serial/samsung_tty.c
> > > @@ -760,8 +760,8 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
> > >  static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
> > >  {
> > >         struct uart_port *port = &ourport->port;
> > > +       unsigned int max_count = port->fifosize;
> > 
> > What if port->fifosize is 0? Then this code below:
> > 
> >     while (max_count-- > 0) {
> > 
> > would cause int overflow, if max_count is unsigned?
> > 
> 
> good catch, Sam!

Does it matter, though? As this is a post-decrement, the test is done first, and the
decrement after. Therefore, it'll still bail out as expected.

> I'm thinking of amending this and add at the beginning of the method:
> 
> if (!max_count)
> 	return tty_flip_buffer_push(&port->state->port);

This will not help with overflow. It'll still have wrapped around after completing the
while() (always, no matter what start-value max_count had)

Cheers,
Andre'

Re: [PATCH 11/18] tty: serial: samsung: don't compare with zero an if (bitwise expression)

[Tudor Ambarus]

On 1/16/24 18:38, Sam Protsenko wrote:
> On Wed, Jan 10, 2024 at 4:24 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Since an if tests the numeric value of an expression, certain coding
>> shortcuts can be used. The most obvious one is writing
>>     if (expression)
>> instead of
>>     if (expression != 0)
>>
>> Since our case is a bitwise expression, it's more natural and clear to
>> use the ``if (expression)`` shortcut.
> 
> Maybe the author of this code:
> 
>     (ufstat & info->tx_fifomask) != 0
> 
> just wanted to outline (logically) that the result of this bitwise
> operation produces FIFO length, which he checks to have non-zero
> length? Mechanically of course it doesn't matter much, and I guess

that's a bitwise AND with the fifo mask to check if the fifo is empty or
not, it doesn't care about the length, just if the fifo is empty. IOW if
any of those bits are set, the fifo is not empty. I think not comparing
with zero explicitly is better. At the same time I'm fine dropping the
patch as well. So please tell me if you want me to reword the commit
message or drop the patch entirely.

> everyone can understand what's going on there even without '!= 0'
> part. But it looks quite intentional to me, because in the same 'if'
> block the author uses this as well:
> 
>     (ufstat & info->tx_fifofull)

tx_fifofull is just a bit in the register, in my case BIT(24). If that
bit is one, the fifo is full. Not comparing with zero is fine here, as
we're interested just in that bit/flag.

> 
> without any comparison operators.
> 
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>>  drivers/tty/serial/samsung_tty.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>> index dbbe6b8e3ceb..f2413da14b1d 100644
>> --- a/drivers/tty/serial/samsung_tty.c
>> +++ b/drivers/tty/serial/samsung_tty.c
>> @@ -988,8 +988,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
>>         u32 ufcon = rd_regl(port, S3C2410_UFCON);
>>
>>         if (ufcon & S3C2410_UFCON_FIFOMODE) {
>> -               if ((ufstat & info->tx_fifomask) != 0 ||
>> -                   (ufstat & info->tx_fifofull))
>> +               if ((ufstat & info->tx_fifomask) || (ufstat & info->tx_fifofull))
> 
> Does this line fit into 80 characters? If no, please rework it so it

it fits

> does. I guess it's also possible to get rid of superfluous braces
> there, but then the code might look confusing, and I'm not sure if
> checkpatch would be ok with that.
> 

I find it better with the braces.

Thanks!
ta

Re: [PATCH 10/18] tty: serial: samsung: make max_count unsigned int

[Tudor Ambarus]

On 1/17/24 15:38, André Draszik wrote:
>>>> +       unsigned int max_count = port->fifosize;
>>> What if port->fifosize is 0? Then this code below:
>>>
>>>     while (max_count-- > 0) {
>>>
>>> would cause int overflow, if max_count is unsigned?
>>>
>> good catch, Sam!
> Does it matter, though? As this is a post-decrement, the test is done first, and the
> decrement after. Therefore, it'll still bail out as expected.

Indeed, it doesn't. This reminds me of stop replying to emails at the
end of the day :)

Cheers Andre'!
ta

Re: [PATCH 14/18] tty: serial: samsung: return bool for s3c24xx_serial_console_txrdy()

[Tudor Ambarus]

On 1/16/24 18:54, Sam Protsenko wrote:
> On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> s3c24xx_serial_console_txrdy() returned just 0 or 1 to indicate whether
>> the TX is empty or not. Change its return type to bool.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>>  drivers/tty/serial/samsung_tty.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>> index 63e993bed296..37c0ba2a122c 100644
>> --- a/drivers/tty/serial/samsung_tty.c
>> +++ b/drivers/tty/serial/samsung_tty.c
>> @@ -2183,7 +2183,7 @@ static const struct dev_pm_ops s3c24xx_serial_pm_ops = {
>>
>>  static struct uart_port *cons_uart;
>>
>> -static int
>> +static bool
>>  s3c24xx_serial_console_txrdy(struct uart_port *port, u32 ufcon)
>>  {
>>         const struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
>> @@ -2193,13 +2193,13 @@ s3c24xx_serial_console_txrdy(struct uart_port *port, u32 ufcon)
>>                 /* fifo mode - check amount of data in fifo registers... */
>>
>>                 ufstat = rd_regl(port, S3C2410_UFSTAT);
>> -               return (ufstat & info->tx_fifofull) ? 0 : 1;
>> +               return !(ufstat & info->tx_fifofull);
>>         }
>>
>>         /* in non-fifo mode, we go and use the tx buffer empty */
>>
>>         utrstat = rd_regl(port, S3C2410_UTRSTAT);
>> -       return (utrstat & S3C2410_UTRSTAT_TXE) ? 1 : 0;
>> +       return !!(utrstat & S3C2410_UTRSTAT_TXE);
> 
> Again, personally I think !! is just clutters the code here, as the
> function already returns bool. Other than that:
> 

Indeed, I'll update. Thanks!
ta

Re: [PATCH 11/18] tty: serial: samsung: don't compare with zero an if (bitwise expression)

[Sam Protsenko]

On Wed, Jan 17, 2024 at 9:41 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 1/16/24 18:38, Sam Protsenko wrote:
> > On Wed, Jan 10, 2024 at 4:24 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >> Since an if tests the numeric value of an expression, certain coding
> >> shortcuts can be used. The most obvious one is writing
> >>     if (expression)
> >> instead of
> >>     if (expression != 0)
> >>
> >> Since our case is a bitwise expression, it's more natural and clear to
> >> use the ``if (expression)`` shortcut.
> >
> > Maybe the author of this code:
> >
> >     (ufstat & info->tx_fifomask) != 0
> >
> > just wanted to outline (logically) that the result of this bitwise
> > operation produces FIFO length, which he checks to have non-zero
> > length? Mechanically of course it doesn't matter much, and I guess
>
> that's a bitwise AND with the fifo mask to check if the fifo is empty or
> not, it doesn't care about the length, just if the fifo is empty. IOW if
> any of those bits are set, the fifo is not empty. I think not comparing
> with zero explicitly is better. At the same time I'm fine dropping the
> patch as well. So please tell me if you want me to reword the commit
> message or drop the patch entirely.
>

I'm not opposed to this patch, just don't have any preference in this
case. But the patch is ok with me.

> > everyone can understand what's going on there even without '!= 0'
> > part. But it looks quite intentional to me, because in the same 'if'
> > block the author uses this as well:
> >
> >     (ufstat & info->tx_fifofull)
>
> tx_fifofull is just a bit in the register, in my case BIT(24). If that
> bit is one, the fifo is full. Not comparing with zero is fine here, as
> we're interested just in that bit/flag.
>
> >
> > without any comparison operators.
> >
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> >> ---
> >>  drivers/tty/serial/samsung_tty.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> >> index dbbe6b8e3ceb..f2413da14b1d 100644
> >> --- a/drivers/tty/serial/samsung_tty.c
> >> +++ b/drivers/tty/serial/samsung_tty.c
> >> @@ -988,8 +988,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
> >>         u32 ufcon = rd_regl(port, S3C2410_UFCON);
> >>
> >>         if (ufcon & S3C2410_UFCON_FIFOMODE) {
> >> -               if ((ufstat & info->tx_fifomask) != 0 ||
> >> -                   (ufstat & info->tx_fifofull))
> >> +               if ((ufstat & info->tx_fifomask) || (ufstat & info->tx_fifofull))
> >
> > Does this line fit into 80 characters? If no, please rework it so it
>
> it fits
>

Just checked, and it's 1 character off (so it has length of 81
characters). I know it's not a strong rule in kernel anymore, but I
like it personally. If you are going to fix that, be free to add:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

> > does. I guess it's also possible to get rid of superfluous braces
> > there, but then the code might look confusing, and I'm not sure if
> > checkpatch would be ok with that.
> >
>
> I find it better with the braces.
>
> Thanks!
> ta

Re: [PATCH 10/18] tty: serial: samsung: make max_count unsigned int

[Sam Protsenko]

On Wed, Jan 17, 2024 at 9:38 AM André Draszik <andre.draszik@linaro.org> wrote:
>
> Hi,
>
> On Wed, 2024-01-17 at 15:21 +0000, Tudor Ambarus wrote:
> >
> >
> > On 1/16/24 18:21, Sam Protsenko wrote:
> > > On Wed, Jan 10, 2024 at 4:23 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> > > >
> > > > ``max_count`` negative values are not used. Since ``port->fifosize``
> > > > is an unsigned int, make ``max_count`` the same.
> > > >
> > > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> > > > ---
> > > >  drivers/tty/serial/samsung_tty.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> > > > index 90c49197efc7..dbbe6b8e3ceb 100644
> > > > --- a/drivers/tty/serial/samsung_tty.c
> > > > +++ b/drivers/tty/serial/samsung_tty.c
> > > > @@ -760,8 +760,8 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
> > > >  static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
> > > >  {
> > > >         struct uart_port *port = &ourport->port;
> > > > +       unsigned int max_count = port->fifosize;
> > >
> > > What if port->fifosize is 0? Then this code below:
> > >
> > >     while (max_count-- > 0) {
> > >
> > > would cause int overflow, if max_count is unsigned?
> > >
> >
> > good catch, Sam!
>
> Does it matter, though? As this is a post-decrement, the test is done first, and the
> decrement after. Therefore, it'll still bail out as expected.
>

Good catch on my good catch :)

> > I'm thinking of amending this and add at the beginning of the method:
> >
> > if (!max_count)
> >       return tty_flip_buffer_push(&port->state->port);
>
> This will not help with overflow. It'll still have wrapped around after completing the
> while() (always, no matter what start-value max_count had)
>
> Cheers,
> Andre'
>

Re: [PATCH 16/18] tty: serial: samsung: shrink the clock selection to 8 clocks

[Tudor Ambarus]

On 1/16/24 19:09, Sam Protsenko wrote:
> On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> <linux/serial_s3c.h> provides a clock selection pool of maximum 4 clocks.
> 
> Then maybe it makes sense to turn those two field into 4-bit bit
> fields? More importantly, what particular problem does this patch
> solve, is this optimization really needed, and why? I'm not saying
> it's not needed, just that commit message might've been more verbose
> about this.
> 

I guess I could have been more verbose in the phrase from below and said
that for arm64 ``struct s3c24xx_uart_info`` spans through 2 cachelines
and contains 2 holes, and with a bit of love it can fit a single
cacheline with no holes. The end goal is to reduce the memory footprint
of that struct.

I chose u8 and allowed a max of 8 clocks simple because it's large
enough to allow more clocks than are supported by the driver now, and
not too big to cause spanning of the structure through 2 cachelines.


>> Update the driver to consider a pool selection of maximum 8 clocks. The
>> final scope is to reduce the memory footprint of
>> ``struct s3c24xx_uart_info``.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>>  drivers/tty/serial/samsung_tty.c | 22 +++++++++++-----------
>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>> index 436739cf9225..5df2bcebf9fb 100644
>> --- a/drivers/tty/serial/samsung_tty.c
>> +++ b/drivers/tty/serial/samsung_tty.c
>> @@ -81,11 +81,11 @@ struct s3c24xx_uart_info {
>>         unsigned long           tx_fifomask;
>>         unsigned long           tx_fifoshift;
>>         unsigned long           tx_fifofull;
>> -       unsigned int            def_clk_sel;
>> -       unsigned long           num_clks;
>>         unsigned long           clksel_mask;
>>         unsigned long           clksel_shift;
>>         unsigned long           ucon_mask;
>> +       u8                      def_clk_sel;
>> +       u8                      num_clks;
>>         u8                      iotype;
>>

Re: [PATCH 10/18] tty: serial: samsung: make max_count unsigned int

[Sam Protsenko]

On Wed, Jan 17, 2024 at 9:54 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 1/17/24 15:38, André Draszik wrote:
> >>>> +       unsigned int max_count = port->fifosize;
> >>> What if port->fifosize is 0? Then this code below:
> >>>
> >>>     while (max_count-- > 0) {
> >>>
> >>> would cause int overflow, if max_count is unsigned?
> >>>
> >> good catch, Sam!
> > Does it matter, though? As this is a post-decrement, the test is done first, and the
> > decrement after. Therefore, it'll still bail out as expected.
>
> Indeed, it doesn't. This reminds me of stop replying to emails at the
> end of the day :)
>

And it reminds me to drink some coffee in the morning before doing any
reviews :) With above condition sorted, feel free to add:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

> Cheers Andre'!
> ta

Re: [PATCH 16/18] tty: serial: samsung: shrink the clock selection to 8 clocks

[Sam Protsenko]

On Wed, Jan 17, 2024 at 10:26 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 1/16/24 19:09, Sam Protsenko wrote:
> > On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >> <linux/serial_s3c.h> provides a clock selection pool of maximum 4 clocks.
> >
> > Then maybe it makes sense to turn those two field into 4-bit bit
> > fields? More importantly, what particular problem does this patch
> > solve, is this optimization really needed, and why? I'm not saying
> > it's not needed, just that commit message might've been more verbose
> > about this.
> >
>
> I guess I could have been more verbose in the phrase from below and said
> that for arm64 ``struct s3c24xx_uart_info`` spans through 2 cachelines
> and contains 2 holes, and with a bit of love it can fit a single
> cacheline with no holes. The end goal is to reduce the memory footprint
> of that struct.
>

Oh yeah, I actually like the cachelines part. Please add that bit to
the commit message if you don't mind.

> I chose u8 and allowed a max of 8 clocks simple because it's large
> enough to allow more clocks than are supported by the driver now, and
> not too big to cause spanning of the structure through 2 cachelines.
>

Gotcha. Maybe also add that reasoning to the commit message. Just a
thought. With above comments addressed, feel free to add:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>
> >> Update the driver to consider a pool selection of maximum 8 clocks. The
> >> final scope is to reduce the memory footprint of
> >> ``struct s3c24xx_uart_info``.
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> >> ---
> >>  drivers/tty/serial/samsung_tty.c | 22 +++++++++++-----------
> >>  1 file changed, 11 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> >> index 436739cf9225..5df2bcebf9fb 100644
> >> --- a/drivers/tty/serial/samsung_tty.c
> >> +++ b/drivers/tty/serial/samsung_tty.c
> >> @@ -81,11 +81,11 @@ struct s3c24xx_uart_info {
> >>         unsigned long           tx_fifomask;
> >>         unsigned long           tx_fifoshift;
> >>         unsigned long           tx_fifofull;
> >> -       unsigned int            def_clk_sel;
> >> -       unsigned long           num_clks;
> >>         unsigned long           clksel_mask;
> >>         unsigned long           clksel_shift;
> >>         unsigned long           ucon_mask;
> >> +       u8                      def_clk_sel;
> >> +       u8                      num_clks;
> >>         u8                      iotype;
> >>

Re: [PATCH 17/18] tty: serial: samsung: shrink port feature flags to u8

[Tudor Ambarus]

On 1/16/24 19:03, Sam Protsenko wrote:
> On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> There's a single flag defined as of now. Shrink the feature flags to u8
>> and aim for a better memory footprint for ``struct s3c24xx_uart_info``.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>>  drivers/tty/serial/samsung_tty.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>> index 5df2bcebf9fb..598d9fe7a492 100644
>> --- a/drivers/tty/serial/samsung_tty.c
>> +++ b/drivers/tty/serial/samsung_tty.c
>> @@ -90,7 +90,7 @@ struct s3c24xx_uart_info {
>>
>>         /* uart port features */
>>
>> -       unsigned int            has_divslot:1;
>> +       u8                      has_divslot:1;
> 
> But that's already a bit field. Why does it matter which type it is?
> 

If using unsigned int the bitfied is combined with the previous u8
fields, whereas if using u8 the bitfield will be independently defined.
So no benefit in terms of memory footprint, it's just a cosmetic change
to align the bitfield with the previous u8 fields. Allowing u32 for just
a bit can be misleading as one would ask itself where are the other
bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good
compromise.

I'll update the commit message with this explanation in v2 because I'd
keep the patch, it makes the struct look cleaner. At the same time I
don't have a strong opinion, so if you'd like to see the patch dropped,
tell there, I'll be fine with it.

Re: [PATCH 17/18] tty: serial: samsung: shrink port feature flags to u8

[Jiri Slaby]

On 19. 01. 24, 9:56, Tudor Ambarus wrote:
> 
> 
> On 1/16/24 19:03, Sam Protsenko wrote:
>> On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>
>>> There's a single flag defined as of now. Shrink the feature flags to u8
>>> and aim for a better memory footprint for ``struct s3c24xx_uart_info``.
>>>
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>>> ---
>>>   drivers/tty/serial/samsung_tty.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>>> index 5df2bcebf9fb..598d9fe7a492 100644
>>> --- a/drivers/tty/serial/samsung_tty.c
>>> +++ b/drivers/tty/serial/samsung_tty.c
>>> @@ -90,7 +90,7 @@ struct s3c24xx_uart_info {
>>>
>>>          /* uart port features */
>>>
>>> -       unsigned int            has_divslot:1;
>>> +       u8                      has_divslot:1;
>>
>> But that's already a bit field. Why does it matter which type it is?
>>
> 
> If using unsigned int the bitfied is combined with the previous u8
> fields, whereas if using u8 the bitfield will be independently defined.
> So no benefit in terms of memory footprint, it's just a cosmetic change
> to align the bitfield with the previous u8 fields. Allowing u32 for just
> a bit can be misleading as one would ask itself where are the other
> bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good
> compromise.

Why? What's wrong with bool? bitfields have terrible semantics wrt 
atomic writes for example.

-- 
js
suse labs

Re: [PATCH 17/18] tty: serial: samsung: shrink port feature flags to u8

[Tudor Ambarus]

On 1/19/24 09:07, Jiri Slaby wrote:

Hi, Jiri!

> On 19. 01. 24, 9:56, Tudor Ambarus wrote:
>>
>>
>> On 1/16/24 19:03, Sam Protsenko wrote:
>>> On Wed, Jan 10, 2024 at 4:25 AM Tudor Ambarus
>>> <tudor.ambarus@linaro.org> wrote:
>>>>
>>>> There's a single flag defined as of now. Shrink the feature flags to u8
>>>> and aim for a better memory footprint for ``struct s3c24xx_uart_info``.
>>>>
>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>>>> ---
>>>>   drivers/tty/serial/samsung_tty.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/tty/serial/samsung_tty.c
>>>> b/drivers/tty/serial/samsung_tty.c
>>>> index 5df2bcebf9fb..598d9fe7a492 100644
>>>> --- a/drivers/tty/serial/samsung_tty.c
>>>> +++ b/drivers/tty/serial/samsung_tty.c
>>>> @@ -90,7 +90,7 @@ struct s3c24xx_uart_info {
>>>>
>>>>          /* uart port features */
>>>>
>>>> -       unsigned int            has_divslot:1;
>>>> +       u8                      has_divslot:1;
>>>
>>> But that's already a bit field. Why does it matter which type it is?
>>>
>>
>> If using unsigned int the bitfied is combined with the previous u8
>> fields, whereas if using u8 the bitfield will be independently defined.
>> So no benefit in terms of memory footprint, it's just a cosmetic change
>> to align the bitfield with the previous u8 fields. Allowing u32 for just
>> a bit can be misleading as one would ask itself where are the other
>> bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good
>> compromise.
> 
> Why? What's wrong with bool? bitfields have terrible semantics wrt
> atomic writes for example.
> 

Bool occupies a byte and if more port features will ever be added we'll
occupy more bytes. Here's how the structure will look like with a bool:

struct s3c24xx_uart_info {
	const char  *              name;                 /*     0     8 */
	enum s3c24xx_port_type     type;                 /*     8     4 */
	unsigned int               port_type;            /*    12     4 */
	unsigned int               fifosize;             /*    16     4 */
	u32                        rx_fifomask;          /*    20     4 */
	u32                        rx_fifoshift;         /*    24     4 */
	u32                        rx_fifofull;          /*    28     4 */
	u32                        tx_fifomask;          /*    32     4 */
	u32                        tx_fifoshift;         /*    36     4 */
	u32                        tx_fifofull;          /*    40     4 */
	u32                        clksel_mask;          /*    44     4 */
	u32                        clksel_shift;         /*    48     4 */
	u32                        ucon_mask;            /*    52     4 */
	u8                         def_clk_sel;          /*    56     1 */
	u8                         num_clks;             /*    57     1 */
	u8                         iotype;               /*    58     1 */
	bool                       has_divslot;          /*    59     1 */

	/* size: 64, cachelines: 1, members: 17 */
	/* padding: 4 */
};

What's your preference?
Thanks,
ta

Re: [PATCH 17/18] tty: serial: samsung: shrink port feature flags to u8

[Jiri Slaby]

Hi,

On 19. 01. 24, 10:43, Tudor Ambarus wrote:
>>> If using unsigned int the bitfied is combined with the previous u8
>>> fields, whereas if using u8 the bitfield will be independently defined.
>>> So no benefit in terms of memory footprint, it's just a cosmetic change
>>> to align the bitfield with the previous u8 fields. Allowing u32 for just
>>> a bit can be misleading as one would ask itself where are the other
>>> bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good
>>> compromise.
>>
>> Why? What's wrong with bool? bitfields have terrible semantics wrt
>> atomic writes for example.
>>
> 
> Bool occupies a byte and if more port features will ever be added we'll
> occupy more bytes. Here's how the structure will look like with a bool:
> 
> struct s3c24xx_uart_info {
> 	const char  *              name;                 /*     0     8 */
> 	enum s3c24xx_port_type     type;                 /*     8     4 */
> 	unsigned int               port_type;            /*    12     4 */
> 	unsigned int               fifosize;             /*    16     4 */
> 	u32                        rx_fifomask;          /*    20     4 */
> 	u32                        rx_fifoshift;         /*    24     4 */
> 	u32                        rx_fifofull;          /*    28     4 */
> 	u32                        tx_fifomask;          /*    32     4 */
> 	u32                        tx_fifoshift;         /*    36     4 */
> 	u32                        tx_fifofull;          /*    40     4 */
> 	u32                        clksel_mask;          /*    44     4 */
> 	u32                        clksel_shift;         /*    48     4 */
> 	u32                        ucon_mask;            /*    52     4 */
> 	u8                         def_clk_sel;          /*    56     1 */
> 	u8                         num_clks;             /*    57     1 */
> 	u8                         iotype;               /*    58     1 */
> 	bool                       has_divslot;          /*    59     1 */
> 
> 	/* size: 64, cachelines: 1, members: 17 */
> 	/* padding: 4 */
> };
> 
> What's your preference?

bool :).

-- 
js
suse labs

Re: [PATCH 17/18] tty: serial: samsung: shrink port feature flags to u8

[Tudor Ambarus]

On 1/19/24 09:54, Jiri Slaby wrote:
> Hi,
> 
> On 19. 01. 24, 10:43, Tudor Ambarus wrote:
>>>> If using unsigned int the bitfied is combined with the previous u8
>>>> fields, whereas if using u8 the bitfield will be independently defined.
>>>> So no benefit in terms of memory footprint, it's just a cosmetic change
>>>> to align the bitfield with the previous u8 fields. Allowing u32 for
>>>> just
>>>> a bit can be misleading as one would ask itself where are the other
>>>> bits. Between a u32 bitfield and a bool a u8 bitfield seems like a good
>>>> compromise.
>>>
>>> Why? What's wrong with bool? bitfields have terrible semantics wrt
>>> atomic writes for example.
>>>
>>
>> Bool occupies a byte and if more port features will ever be added we'll
>> occupy more bytes. Here's how the structure will look like with a bool:
>>
>> struct s3c24xx_uart_info {
>>     const char  *              name;                 /*     0     8 */
>>     enum s3c24xx_port_type     type;                 /*     8     4 */
>>     unsigned int               port_type;            /*    12     4 */
>>     unsigned int               fifosize;             /*    16     4 */
>>     u32                        rx_fifomask;          /*    20     4 */
>>     u32                        rx_fifoshift;         /*    24     4 */
>>     u32                        rx_fifofull;          /*    28     4 */
>>     u32                        tx_fifomask;          /*    32     4 */
>>     u32                        tx_fifoshift;         /*    36     4 */
>>     u32                        tx_fifofull;          /*    40     4 */
>>     u32                        clksel_mask;          /*    44     4 */
>>     u32                        clksel_shift;         /*    48     4 */
>>     u32                        ucon_mask;            /*    52     4 */
>>     u8                         def_clk_sel;          /*    56     1 */
>>     u8                         num_clks;             /*    57     1 */
>>     u8                         iotype;               /*    58     1 */
>>     bool                       has_divslot;          /*    59     1 */
>>
>>     /* size: 64, cachelines: 1, members: 17 */
>>     /* padding: 4 */
>> };
>>
>> What's your preference?
> 
> bool :).
> 
I'm fine with a bool too as since the introduction of this driver we
have just this flag, it's unlikey to have 4 more soon to bypass the
first cacheline. Will change to bool.

Cheers,
ta