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/[email protected]/
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/[email protected]/
Link: https://lore.kernel.org/linux-kernel/[email protected]/
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
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 <[email protected]>
Signed-off-by: Tudor Ambarus <[email protected]>
---
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
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 <[email protected]>
---
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
Move open brace '{' following function definition on the next line.
Signed-off-by: Tudor Ambarus <[email protected]>
---
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
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 <[email protected]>
---
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
On Wed, Jan 10, 2024 at 4:21 AM Tudor Ambarus <tudor.ambarus@linaroorg> 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 <[email protected]>
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
Reviewed-by: Sam Protsenko <[email protected]>
> 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
>
>
On Wed, Jan 10, 2024 at 4:23 AM Tudor Ambarus <tudor.ambarus@linaroorg> wrote:
>
> Move open brace '{' following function definition on the next line.
>
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
Reviewed-by: Sam Protsenko <[email protected]>
> 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
>
>
On Wed, Jan 10, 2024 at 4:24 AM Tudor Ambarus <tudor.ambarus@linaroorg> 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 <[email protected]>
> ---
> 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
>
>