Hi,
While adding upstream support to a new platform (Mobileye EyeQ5[1]) that
uses the AMBA PL011 driver, I took some time to look at the PL011
driver and ended up with a few patches that cleanup parts of it. The
line-diff is big mostly because of the checkpatch-fixing commits.
The driver hadn't received any love for quite some time. A single commit
changes the code's behavior: see "tty: serial: amba-pl011: Parse bits
option as 5, 6, 7 or 8 in _get_options". See commit messages for more
information.
[1]: https://lore.kernel.org/all/[email protected]/T/
Have a nice day,
Théo Lebrun
Signed-off-by: Théo Lebrun <[email protected]>
---
Théo Lebrun (6):
tty: serial: amba: cleanup whitespace
tty: serial: amba: Use BIT() macro for constant declarations
tty: serial: amba-pl011: cleanup driver
tty: serial: amba-pl011: replace TIOCMBIT macros by static functions
tty: serial: amba-pl011: unindent pl011_console_get_options function body
tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options
drivers/tty/serial/amba-pl011.c | 238 ++++++++++++++++++++--------------------
include/linux/amba/serial.h | 192 ++++++++++++++++----------------
2 files changed, 214 insertions(+), 216 deletions(-)
---
base-commit: ad582615776e62e365ab2dfa7a7a3806ada28b30
change-id: 20231023-mbly-uart-afcacbb98f8b
Best regards,
--
Théo Lebrun <[email protected]>
pl011_console_get_options() gets called to retrieve currently configured
options from the registers. Previously, LCRH_TX.WLEN was being parsed
as either 7 or 8 (fallback). Hardware supports values from 5 to 8
inclusive, which pl011_set_termios() exploits for example.
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/tty/serial/amba-pl011.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 5774d48c7f16..b2062e4cbbab 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2384,10 +2384,7 @@ static void pl011_console_get_options(struct uart_amba_port *uap, int *baud,
*parity = 'o';
}
- if ((lcr_h & 0x60) == UART01x_LCRH_WLEN_7)
- *bits = 7;
- else
- *bits = 8;
+ *bits = FIELD_GET(0x60, lcr_h) + 5; /* from 5 to 8 inclusive */
ibrd = pl011_read(uap, REG_IBRD);
fbrd = pl011_read(uap, REG_FBRD);
--
2.41.0
The driver uses bit shifts and hexadecimal expressions to declare
constants. Replace that with the BIT() macro that clarifies intent.
Signed-off-by: Théo Lebrun <[email protected]>
---
include/linux/amba/serial.h | 182 ++++++++++++++++++++++----------------------
1 file changed, 91 insertions(+), 91 deletions(-)
diff --git a/include/linux/amba/serial.h b/include/linux/amba/serial.h
index 27003ec52114..d926980478ed 100644
--- a/include/linux/amba/serial.h
+++ b/include/linux/amba/serial.h
@@ -70,64 +70,64 @@
#define ZX_UART011_ICR 0x4c
#define ZX_UART011_DMACR 0x50
-#define UART011_DR_OE (1 << 11)
-#define UART011_DR_BE (1 << 10)
-#define UART011_DR_PE (1 << 9)
-#define UART011_DR_FE (1 << 8)
-
-#define UART01x_RSR_OE 0x08
-#define UART01x_RSR_BE 0x04
-#define UART01x_RSR_PE 0x02
-#define UART01x_RSR_FE 0x01
-
-#define UART011_FR_RI 0x100
-#define UART011_FR_TXFE 0x080
-#define UART011_FR_RXFF 0x040
-#define UART01x_FR_TXFF 0x020
-#define UART01x_FR_RXFE 0x010
-#define UART01x_FR_BUSY 0x008
-#define UART01x_FR_DCD 0x004
-#define UART01x_FR_DSR 0x002
-#define UART01x_FR_CTS 0x001
+#define UART011_DR_OE BIT(11)
+#define UART011_DR_BE BIT(10)
+#define UART011_DR_PE BIT(9)
+#define UART011_DR_FE BIT(8)
+
+#define UART01x_RSR_OE BIT(3)
+#define UART01x_RSR_BE BIT(2)
+#define UART01x_RSR_PE BIT(1)
+#define UART01x_RSR_FE BIT(0)
+
+#define UART011_FR_RI BIT(8)
+#define UART011_FR_TXFE BIT(7)
+#define UART011_FR_RXFF BIT(6)
+#define UART01x_FR_TXFF BIT(5)
+#define UART01x_FR_RXFE BIT(4)
+#define UART01x_FR_BUSY BIT(3)
+#define UART01x_FR_DCD BIT(2)
+#define UART01x_FR_DSR BIT(1)
+#define UART01x_FR_CTS BIT(0)
#define UART01x_FR_TMSK (UART01x_FR_TXFF + UART01x_FR_BUSY)
/*
* Some bits of Flag Register on ZTE device have different position from
* standard ones.
*/
-#define ZX_UART01x_FR_BUSY 0x100
-#define ZX_UART01x_FR_DSR 0x008
-#define ZX_UART01x_FR_CTS 0x002
-#define ZX_UART011_FR_RI 0x001
-
-#define UART011_CR_CTSEN 0x8000 /* CTS hardware flow control */
-#define UART011_CR_RTSEN 0x4000 /* RTS hardware flow control */
-#define UART011_CR_OUT2 0x2000 /* OUT2 */
-#define UART011_CR_OUT1 0x1000 /* OUT1 */
-#define UART011_CR_RTS 0x0800 /* RTS */
-#define UART011_CR_DTR 0x0400 /* DTR */
-#define UART011_CR_RXE 0x0200 /* receive enable */
-#define UART011_CR_TXE 0x0100 /* transmit enable */
-#define UART011_CR_LBE 0x0080 /* loopback enable */
-#define UART010_CR_RTIE 0x0040
-#define UART010_CR_TIE 0x0020
-#define UART010_CR_RIE 0x0010
-#define UART010_CR_MSIE 0x0008
-#define ST_UART011_CR_OVSFACT 0x0008 /* Oversampling factor */
-#define UART01x_CR_IIRLP 0x0004 /* SIR low power mode */
-#define UART01x_CR_SIREN 0x0002 /* SIR enable */
-#define UART01x_CR_UARTEN 0x0001 /* UART enable */
-
-#define UART011_LCRH_SPS 0x80
+#define ZX_UART01x_FR_BUSY BIT(8)
+#define ZX_UART01x_FR_DSR BIT(3)
+#define ZX_UART01x_FR_CTS BIT(1)
+#define ZX_UART011_FR_RI BIT(0)
+
+#define UART011_CR_CTSEN BIT(15) /* CTS hardware flow control */
+#define UART011_CR_RTSEN BIT(14) /* RTS hardware flow control */
+#define UART011_CR_OUT2 BIT(13) /* OUT2 */
+#define UART011_CR_OUT1 BIT(12) /* OUT1 */
+#define UART011_CR_RTS BIT(11) /* RTS */
+#define UART011_CR_DTR BIT(10) /* DTR */
+#define UART011_CR_RXE BIT(9) /* receive enable */
+#define UART011_CR_TXE BIT(8) /* transmit enable */
+#define UART011_CR_LBE BIT(7) /* loopback enable */
+#define UART010_CR_RTIE BIT(6)
+#define UART010_CR_TIE BIT(5)
+#define UART010_CR_RIE BIT(4)
+#define UART010_CR_MSIE BIT(3)
+#define ST_UART011_CR_OVSFACT BIT(3) /* Oversampling factor */
+#define UART01x_CR_IIRLP BIT(2) /* SIR low power mode */
+#define UART01x_CR_SIREN BIT(1) /* SIR enable */
+#define UART01x_CR_UARTEN BIT(0) /* UART enable */
+
+#define UART011_LCRH_SPS BIT(7)
#define UART01x_LCRH_WLEN_8 0x60
#define UART01x_LCRH_WLEN_7 0x40
#define UART01x_LCRH_WLEN_6 0x20
#define UART01x_LCRH_WLEN_5 0x00
-#define UART01x_LCRH_FEN 0x10
-#define UART01x_LCRH_STP2 0x08
-#define UART01x_LCRH_EPS 0x04
-#define UART01x_LCRH_PEN 0x02
-#define UART01x_LCRH_BRK 0x01
+#define UART01x_LCRH_FEN BIT(4)
+#define UART01x_LCRH_STP2 BIT(3)
+#define UART01x_LCRH_EPS BIT(2)
+#define UART01x_LCRH_PEN BIT(1)
+#define UART01x_LCRH_BRK BIT(0)
#define ST_UART011_DMAWM_RX_1 (0 << 3)
#define ST_UART011_DMAWM_RX_2 (1 << 3)
@@ -144,10 +144,10 @@
#define ST_UART011_DMAWM_TX_32 5
#define ST_UART011_DMAWM_TX_48 6
-#define UART010_IIR_RTIS 0x08
-#define UART010_IIR_TIS 0x04
-#define UART010_IIR_RIS 0x02
-#define UART010_IIR_MIS 0x01
+#define UART010_IIR_RTIS BIT(3)
+#define UART010_IIR_TIS BIT(2)
+#define UART010_IIR_RIS BIT(1)
+#define UART010_IIR_MIS BIT(0)
#define UART011_IFLS_RX1_8 (0 << 3)
#define UART011_IFLS_RX2_8 (1 << 3)
@@ -163,45 +163,45 @@
#define UART011_IFLS_RX_HALF (5 << 3)
#define UART011_IFLS_TX_HALF (5 << 0)
-#define UART011_OEIM (1 << 10) /* overrun error interrupt mask */
-#define UART011_BEIM (1 << 9) /* break error interrupt mask */
-#define UART011_PEIM (1 << 8) /* parity error interrupt mask */
-#define UART011_FEIM (1 << 7) /* framing error interrupt mask */
-#define UART011_RTIM (1 << 6) /* receive timeout interrupt mask */
-#define UART011_TXIM (1 << 5) /* transmit interrupt mask */
-#define UART011_RXIM (1 << 4) /* receive interrupt mask */
-#define UART011_DSRMIM (1 << 3) /* DSR interrupt mask */
-#define UART011_DCDMIM (1 << 2) /* DCD interrupt mask */
-#define UART011_CTSMIM (1 << 1) /* CTS interrupt mask */
-#define UART011_RIMIM (1 << 0) /* RI interrupt mask */
-
-#define UART011_OEIS (1 << 10) /* overrun error interrupt status */
-#define UART011_BEIS (1 << 9) /* break error interrupt status */
-#define UART011_PEIS (1 << 8) /* parity error interrupt status */
-#define UART011_FEIS (1 << 7) /* framing error interrupt status */
-#define UART011_RTIS (1 << 6) /* receive timeout interrupt status */
-#define UART011_TXIS (1 << 5) /* transmit interrupt status */
-#define UART011_RXIS (1 << 4) /* receive interrupt status */
-#define UART011_DSRMIS (1 << 3) /* DSR interrupt status */
-#define UART011_DCDMIS (1 << 2) /* DCD interrupt status */
-#define UART011_CTSMIS (1 << 1) /* CTS interrupt status */
-#define UART011_RIMIS (1 << 0) /* RI interrupt status */
-
-#define UART011_OEIC (1 << 10) /* overrun error interrupt clear */
-#define UART011_BEIC (1 << 9) /* break error interrupt clear */
-#define UART011_PEIC (1 << 8) /* parity error interrupt clear */
-#define UART011_FEIC (1 << 7) /* framing error interrupt clear */
-#define UART011_RTIC (1 << 6) /* receive timeout interrupt clear */
-#define UART011_TXIC (1 << 5) /* transmit interrupt clear */
-#define UART011_RXIC (1 << 4) /* receive interrupt clear */
-#define UART011_DSRMIC (1 << 3) /* DSR interrupt clear */
-#define UART011_DCDMIC (1 << 2) /* DCD interrupt clear */
-#define UART011_CTSMIC (1 << 1) /* CTS interrupt clear */
-#define UART011_RIMIC (1 << 0) /* RI interrupt clear */
-
-#define UART011_DMAONERR (1 << 2) /* disable dma on error */
-#define UART011_TXDMAE (1 << 1) /* enable transmit dma */
-#define UART011_RXDMAE (1 << 0) /* enable receive dma */
+#define UART011_OEIM BIT(10) /* overrun error interrupt mask */
+#define UART011_BEIM BIT(9) /* break error interrupt mask */
+#define UART011_PEIM BIT(8) /* parity error interrupt mask */
+#define UART011_FEIM BIT(7) /* framing error interrupt mask */
+#define UART011_RTIM BIT(6) /* receive timeout interrupt mask */
+#define UART011_TXIM BIT(5) /* transmit interrupt mask */
+#define UART011_RXIM BIT(4) /* receive interrupt mask */
+#define UART011_DSRMIM BIT(3) /* DSR interrupt mask */
+#define UART011_DCDMIM BIT(2) /* DCD interrupt mask */
+#define UART011_CTSMIM BIT(1) /* CTS interrupt mask */
+#define UART011_RIMIM BIT(0) /* RI interrupt mask */
+
+#define UART011_OEIS BIT(10) /* overrun error interrupt status */
+#define UART011_BEIS BIT(9) /* break error interrupt status */
+#define UART011_PEIS BIT(8) /* parity error interrupt status */
+#define UART011_FEIS BIT(7) /* framing error interrupt status */
+#define UART011_RTIS BIT(6) /* receive timeout interrupt status */
+#define UART011_TXIS BIT(5) /* transmit interrupt status */
+#define UART011_RXIS BIT(4) /* receive interrupt status */
+#define UART011_DSRMIS BIT(3) /* DSR interrupt status */
+#define UART011_DCDMIS BIT(2) /* DCD interrupt status */
+#define UART011_CTSMIS BIT(1) /* CTS interrupt status */
+#define UART011_RIMIS BIT(0) /* RI interrupt status */
+
+#define UART011_OEIC BIT(10) /* overrun error interrupt clear */
+#define UART011_BEIC BIT(9) /* break error interrupt clear */
+#define UART011_PEIC BIT(8) /* parity error interrupt clear */
+#define UART011_FEIC BIT(7) /* framing error interrupt clear */
+#define UART011_RTIC BIT(6) /* receive timeout interrupt clear */
+#define UART011_TXIC BIT(5) /* transmit interrupt clear */
+#define UART011_RXIC BIT(4) /* receive interrupt clear */
+#define UART011_DSRMIC BIT(3) /* DSR interrupt clear */
+#define UART011_DCDMIC BIT(2) /* DCD interrupt clear */
+#define UART011_CTSMIC BIT(1) /* CTS interrupt clear */
+#define UART011_RIMIC BIT(0) /* RI interrupt clear */
+
+#define UART011_DMAONERR BIT(2) /* disable dma on error */
+#define UART011_TXDMAE BIT(1) /* enable transmit dma */
+#define UART011_RXDMAE BIT(0) /* enable receive dma */
#define UART01x_RSR_ANY (UART01x_RSR_OE | UART01x_RSR_BE | UART01x_RSR_PE | UART01x_RSR_FE)
#define UART01x_FR_MODEM_ANY (UART01x_FR_DCD | UART01x_FR_DSR | UART01x_FR_CTS)
--
2.41.0
Follow recommandations from:
$ ./scripts/checkpatch.pl --strict --file \
drivers/tty/serial/amba-pl011.c
It does NOT fix alerts relative to TIOCMBIT which will be dealt with in
another patch. Fixes following alerts:
CHECK: Alignment should match open parenthesis
CHECK: Blank lines aren't necessary after an open brace '{'
CHECK: Comparison to NULL could be written [...]
CHECK: Lines should not end with a '('
CHECK: Please don't use multiple blank lines
CHECK: Please use a blank line after function/struct/union/enum declarations
CHECK: Prefer using the BIT macro
CHECK: Unbalanced braces around else statement
CHECK: Unnecessary parentheses around [...]
CHECK: braces {} should be used on all arms of this statement
CHECK: spaces preferred around that '/' (ctx:VxV)
CHECK: spaces preferred around that '|' (ctx:VxV)
ERROR: do not initialise statics to false
WARNING: Comparisons should place the constant on the right side of the test
WARNING: Possible unnecessary 'out of memory' message
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
WARNING: Prefer [subsystem eg: netdev]_info([subsystem]dev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
WARNING: quoted string split across lines
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/tty/serial/amba-pl011.c | 153 ++++++++++++++++++++--------------------
1 file changed, 76 insertions(+), 77 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 3dc9b0fcab1c..0d53973374de 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -50,8 +50,8 @@
#define AMBA_ISR_PASS_LIMIT 256
-#define UART_DR_ERROR (UART011_DR_OE|UART011_DR_BE|UART011_DR_PE|UART011_DR_FE)
-#define UART_DUMMY_DR_RX (1 << 16)
+#define UART_DR_ERROR (UART011_DR_OE | UART011_DR_BE | UART011_DR_PE | UART011_DR_FE)
+#define UART_DUMMY_DR_RX BIT(16)
enum {
REG_DR,
@@ -125,7 +125,7 @@ static unsigned int get_fifosize_arm(struct amba_device *dev)
static struct vendor_data vendor_arm = {
.reg_offset = pl011_std_offsets,
- .ifls = UART011_IFLS_RX4_8|UART011_IFLS_TX4_8,
+ .ifls = UART011_IFLS_RX4_8 | UART011_IFLS_TX4_8,
.fr_busy = UART01x_FR_BUSY,
.fr_dsr = UART01x_FR_DSR,
.fr_cts = UART01x_FR_CTS,
@@ -203,7 +203,7 @@ static unsigned int get_fifosize_st(struct amba_device *dev)
static struct vendor_data vendor_st = {
.reg_offset = pl011_st_offsets,
- .ifls = UART011_IFLS_RX_HALF|UART011_IFLS_TX_HALF,
+ .ifls = UART011_IFLS_RX_HALF | UART011_IFLS_TX_HALF,
.fr_busy = UART01x_FR_BUSY,
.fr_dsr = UART01x_FR_DSR,
.fr_cts = UART01x_FR_CTS,
@@ -275,13 +275,13 @@ struct uart_amba_port {
static unsigned int pl011_tx_empty(struct uart_port *port);
static unsigned int pl011_reg_to_offset(const struct uart_amba_port *uap,
- unsigned int reg)
+ unsigned int reg)
{
return uap->reg_offset[reg];
}
static unsigned int pl011_read(const struct uart_amba_port *uap,
- unsigned int reg)
+ unsigned int reg)
{
void __iomem *addr = uap->port.membase + pl011_reg_to_offset(uap, reg);
@@ -290,7 +290,7 @@ static unsigned int pl011_read(const struct uart_amba_port *uap,
}
static void pl011_write(unsigned int val, const struct uart_amba_port *uap,
- unsigned int reg)
+ unsigned int reg)
{
void __iomem *addr = uap->port.membase + pl011_reg_to_offset(uap, reg);
@@ -328,10 +328,12 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap)
uap->port.icount.brk++;
if (uart_handle_break(&uap->port))
continue;
- } else if (ch & UART011_DR_PE)
+ } else if (ch & UART011_DR_PE) {
uap->port.icount.parity++;
- else if (ch & UART011_DR_FE)
+ } else if (ch & UART011_DR_FE) {
uap->port.icount.frame++;
+ }
+
if (ch & UART011_DR_OE)
uap->port.icount.overrun++;
@@ -356,7 +358,6 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap)
return fifotaken;
}
-
/*
* All the DMA operation mode stuff goes inside this ifdef.
* This assumes that you have a generic DMA device interface,
@@ -367,18 +368,18 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap)
#define PL011_DMA_BUFFER_SIZE PAGE_SIZE
static int pl011_sgbuf_init(struct dma_chan *chan, struct pl011_sgbuf *sg,
- enum dma_data_direction dir)
+ enum dma_data_direction dir)
{
dma_addr_t dma_addr;
sg->buf = dma_alloc_coherent(chan->device->dev,
- PL011_DMA_BUFFER_SIZE, &dma_addr, GFP_KERNEL);
+ PL011_DMA_BUFFER_SIZE, &dma_addr, GFP_KERNEL);
if (!sg->buf)
return -ENOMEM;
sg_init_table(&sg->sg, 1);
sg_set_page(&sg->sg, phys_to_page(dma_addr),
- PL011_DMA_BUFFER_SIZE, offset_in_page(dma_addr));
+ PL011_DMA_BUFFER_SIZE, offset_in_page(dma_addr));
sg_dma_address(&sg->sg) = dma_addr;
sg_dma_len(&sg->sg) = PL011_DMA_BUFFER_SIZE;
@@ -386,12 +387,11 @@ static int pl011_sgbuf_init(struct dma_chan *chan, struct pl011_sgbuf *sg,
}
static void pl011_sgbuf_free(struct dma_chan *chan, struct pl011_sgbuf *sg,
- enum dma_data_direction dir)
+ enum dma_data_direction dir)
{
if (sg->buf) {
- dma_free_coherent(chan->device->dev,
- PL011_DMA_BUFFER_SIZE, sg->buf,
- sg_dma_address(&sg->sg));
+ dma_free_coherent(chan->device->dev, PL011_DMA_BUFFER_SIZE,
+ sg->buf, sg_dma_address(&sg->sg));
}
}
@@ -430,7 +430,7 @@ static void pl011_dma_probe(struct uart_amba_port *uap)
dma_cap_set(DMA_SLAVE, mask);
chan = dma_request_channel(mask, plat->dma_filter,
- plat->dma_tx_param);
+ plat->dma_tx_param);
if (!chan) {
dev_err(uap->port.dev, "no TX DMA channel!\n");
return;
@@ -471,12 +471,12 @@ static void pl011_dma_probe(struct uart_amba_port *uap)
* If the controller does, check for suitable residue processing
* otherwise assime all is well.
*/
- if (0 == dma_get_slave_caps(chan, &caps)) {
+ if (dma_get_slave_caps(chan, &caps) == 0) {
if (caps.residue_granularity ==
DMA_RESIDUE_GRANULARITY_DESCRIPTOR) {
dma_release_channel(chan);
dev_info(uap->port.dev,
- "RX DMA disabled - no residue processing\n");
+ "RX DMA disabled - no residue processing\n");
return;
}
}
@@ -505,18 +505,16 @@ static void pl011_dma_probe(struct uart_amba_port *uap)
else
uap->dmarx.poll_timeout = 3000;
} else if (!plat && dev->of_node) {
- uap->dmarx.auto_poll_rate = of_property_read_bool(
- dev->of_node, "auto-poll");
+ uap->dmarx.auto_poll_rate =
+ of_property_read_bool(dev->of_node, "auto-poll");
if (uap->dmarx.auto_poll_rate) {
u32 x;
- if (0 == of_property_read_u32(dev->of_node,
- "poll-rate-ms", &x))
+ if (of_property_read_u32(dev->of_node, "poll-rate-ms", &x) == 0)
uap->dmarx.poll_rate = x;
else
uap->dmarx.poll_rate = 100;
- if (0 == of_property_read_u32(dev->of_node,
- "poll-timeout-ms", &x))
+ if (of_property_read_u32(dev->of_node, "poll-timeout-ms", &x) == 0)
uap->dmarx.poll_timeout = x;
else
uap->dmarx.poll_timeout = 3000;
@@ -624,9 +622,9 @@ static int pl011_dma_tx_refill(struct uart_amba_port *uap)
if (count > PL011_DMA_BUFFER_SIZE)
count = PL011_DMA_BUFFER_SIZE;
- if (xmit->tail < xmit->head)
+ if (xmit->tail < xmit->head) {
memcpy(&dmatx->buf[0], &xmit->buf[xmit->tail], count);
- else {
+ } else {
size_t first = UART_XMIT_SIZE - xmit->tail;
size_t second;
@@ -648,7 +646,7 @@ static int pl011_dma_tx_refill(struct uart_amba_port *uap)
}
desc = dmaengine_prep_slave_sg(chan, &dmatx->sg, 1, DMA_MEM_TO_DEV,
- DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+ DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
if (!desc) {
dma_unmap_sg(dma_dev->dev, &dmatx->sg, 1, DMA_TO_DEVICE);
uap->dmatx.queued = false;
@@ -759,8 +757,9 @@ static inline bool pl011_dma_tx_start(struct uart_amba_port *uap)
if (pl011_dma_tx_refill(uap) > 0) {
uap->im &= ~UART011_TXIM;
pl011_write(uap->im, uap, REG_IMSC);
- } else
+ } else {
ret = false;
+ }
} else if (!(uap->dmacr & UART011_TXDMAE)) {
uap->dmacr |= UART011_TXDMAE;
pl011_write(uap->dmacr, uap, REG_DMACR);
@@ -836,9 +835,8 @@ static int pl011_dma_rx_trigger_dma(struct uart_amba_port *uap)
/* Start the RX DMA job */
sgbuf = uap->dmarx.use_buf_b ?
&uap->dmarx.sgbuf_b : &uap->dmarx.sgbuf_a;
- desc = dmaengine_prep_slave_sg(rxchan, &sgbuf->sg, 1,
- DMA_DEV_TO_MEM,
- DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+ desc = dmaengine_prep_slave_sg(rxchan, &sgbuf->sg, 1, DMA_DEV_TO_MEM,
+ DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
/*
* If the DMA engine is busy and cannot prepare a
* channel, no big deal, the driver will fall back
@@ -894,14 +892,13 @@ static void pl011_dma_rx_chars(struct uart_amba_port *uap,
/* Pick the remain data from the DMA */
if (pending) {
-
/*
* First take all chars in the DMA pipe, then look in the FIFO.
* Note that tty_insert_flip_buf() tries to take as many chars
* as it can.
*/
dma_count = tty_insert_flip_string(port, sgbuf->buf + dmataken,
- pending);
+ pending);
uap->port.icount.rx += dma_count;
if (dma_count < pending)
@@ -983,8 +980,8 @@ static void pl011_dma_rx_irq(struct uart_amba_port *uap)
/* Switch buffer & re-trigger DMA job */
dmarx->use_buf_b = !dmarx->use_buf_b;
if (pl011_dma_rx_trigger_dma(uap)) {
- dev_dbg(uap->port.dev, "could not retrigger RX DMA job "
- "fall back to interrupt mode\n");
+ dev_dbg(uap->port.dev,
+ "could not retrigger RX DMA job fall back to interrupt mode\n");
uap->im |= UART011_RXIM;
pl011_write(uap->im, uap, REG_IMSC);
}
@@ -1031,8 +1028,8 @@ static void pl011_dma_rx_callback(void *data)
* get some IRQ immediately from RX.
*/
if (ret) {
- dev_dbg(uap->port.dev, "could not retrigger RX DMA job "
- "fall back to interrupt mode\n");
+ dev_dbg(uap->port.dev,
+ "could not retrigger RX DMA job fall back to interrupt mode\n");
uap->im |= UART011_RXIM;
pl011_write(uap->im, uap, REG_IMSC);
}
@@ -1077,7 +1074,7 @@ static void pl011_dma_rx_poll(struct timer_list *t)
dmataken = sgbuf->sg.length - dmarx->last_residue;
size = dmarx->last_residue - state.residue;
dma_count = tty_insert_flip_string(port, sgbuf->buf + dmataken,
- size);
+ size);
if (dma_count == size)
dmarx->last_residue = state.residue;
dmarx->last_jiffies = jiffies;
@@ -1102,7 +1099,7 @@ static void pl011_dma_rx_poll(struct timer_list *t)
del_timer(&uap->dmarx.timer);
} else {
mod_timer(&uap->dmarx.timer,
- jiffies + msecs_to_jiffies(uap->dmarx.poll_rate));
+ jiffies + msecs_to_jiffies(uap->dmarx.poll_rate));
}
}
@@ -1118,7 +1115,6 @@ static void pl011_dma_startup(struct uart_amba_port *uap)
uap->dmatx.buf = kmalloc(PL011_DMA_BUFFER_SIZE, GFP_KERNEL | __GFP_DMA);
if (!uap->dmatx.buf) {
- dev_err(uap->port.dev, "no memory for DMA TX buffer\n");
uap->port.fifosize = uap->fifosize;
return;
}
@@ -1169,13 +1165,12 @@ static void pl011_dma_startup(struct uart_amba_port *uap)
if (uap->using_rx_dma) {
if (pl011_dma_rx_trigger_dma(uap))
- dev_dbg(uap->port.dev, "could not trigger initial "
- "RX DMA job, fall back to interrupt mode\n");
+ dev_dbg(uap->port.dev,
+ "could not trigger initial RX DMA job, fall back to interrupt mode\n");
if (uap->dmarx.poll_rate) {
timer_setup(&uap->dmarx.timer, pl011_dma_rx_poll, 0);
mod_timer(&uap->dmarx.timer,
- jiffies +
- msecs_to_jiffies(uap->dmarx.poll_rate));
+ jiffies + msecs_to_jiffies(uap->dmarx.poll_rate));
uap->dmarx.last_residue = PL011_DMA_BUFFER_SIZE;
uap->dmarx.last_jiffies = jiffies;
}
@@ -1363,8 +1358,8 @@ static void pl011_stop_rx(struct uart_port *port)
struct uart_amba_port *uap =
container_of(port, struct uart_amba_port, port);
- uap->im &= ~(UART011_RXIM|UART011_RTIM|UART011_FEIM|
- UART011_PEIM|UART011_BEIM|UART011_OEIM);
+ uap->im &= ~(UART011_RXIM | UART011_RTIM | UART011_FEIM |
+ UART011_PEIM | UART011_BEIM | UART011_OEIM);
pl011_write(uap->im, uap, REG_IMSC);
pl011_dma_rx_stop(uap);
@@ -1384,7 +1379,7 @@ static void pl011_enable_ms(struct uart_port *port)
struct uart_amba_port *uap =
container_of(port, struct uart_amba_port, port);
- uap->im |= UART011_RIMIM|UART011_CTSMIM|UART011_DCDMIM|UART011_DSRMIM;
+ uap->im |= UART011_RIMIM | UART011_CTSMIM | UART011_DCDMIM | UART011_DSRMIM;
pl011_write(uap->im, uap, REG_IMSC);
}
@@ -1402,8 +1397,8 @@ __acquires(&uap->port.lock)
*/
if (pl011_dma_rx_available(uap)) {
if (pl011_dma_rx_trigger_dma(uap)) {
- dev_dbg(uap->port.dev, "could not trigger RX DMA job "
- "fall back to interrupt mode again\n");
+ dev_dbg(uap->port.dev,
+ "could not trigger RX DMA job fall back to interrupt mode again\n");
uap->im |= UART011_RXIM;
pl011_write(uap->im, uap, REG_IMSC);
} else {
@@ -1413,8 +1408,7 @@ __acquires(&uap->port.lock)
uap->dmarx.last_jiffies = jiffies;
uap->dmarx.last_residue = PL011_DMA_BUFFER_SIZE;
mod_timer(&uap->dmarx.timer,
- jiffies +
- msecs_to_jiffies(uap->dmarx.poll_rate));
+ jiffies + msecs_to_jiffies(uap->dmarx.poll_rate));
}
#endif
}
@@ -1561,18 +1555,17 @@ static irqreturn_t pl011_int(int irq, void *dev_id)
do {
check_apply_cts_event_workaround(uap);
- pl011_write(status & ~(UART011_TXIS|UART011_RTIS|
- UART011_RXIS),
+ pl011_write(status & ~(UART011_TXIS | UART011_RTIS | UART011_RXIS),
uap, REG_ICR);
- if (status & (UART011_RTIS|UART011_RXIS)) {
+ if (status & (UART011_RTIS | UART011_RXIS)) {
if (pl011_dma_rx_running(uap))
pl011_dma_rx_irq(uap);
else
pl011_rx_chars(uap);
}
- if (status & (UART011_DSRMIS|UART011_DCDMIS|
- UART011_CTSMIS|UART011_RIMIS))
+ if (status & (UART011_DSRMIS | UART011_DCDMIS |
+ UART011_CTSMIS | UART011_RIMIS))
pl011_modem_status(uap);
if (status & UART011_TXIS)
pl011_tx_chars(uap, true);
@@ -1712,7 +1705,7 @@ static int pl011_get_poll_char(struct uart_port *port)
}
static void pl011_put_poll_char(struct uart_port *port,
- unsigned char ch)
+ unsigned char ch)
{
struct uart_amba_port *uap =
container_of(port, struct uart_amba_port, port);
@@ -1914,13 +1907,13 @@ static int sbsa_uart_startup(struct uart_port *port)
}
static void pl011_shutdown_channel(struct uart_amba_port *uap,
- unsigned int lcrh)
+ unsigned int lcrh)
{
- unsigned long val;
+ unsigned long val;
- val = pl011_read(uap, lcrh);
- val &= ~(UART01x_LCRH_BRK | UART01x_LCRH_FEN);
- pl011_write(val, uap, lcrh);
+ val = pl011_read(uap, lcrh);
+ val &= ~(UART01x_LCRH_BRK | UART01x_LCRH_FEN);
+ pl011_write(val, uap, lcrh);
}
/*
@@ -2069,7 +2062,7 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
uap->dmarx.poll_rate = DIV_ROUND_UP(10000000, baud);
#endif
- if (baud > port->uartclk/16)
+ if (baud > port->uartclk / 16)
quot = DIV_ROUND_CLOSEST(port->uartclk * 8, baud);
else
quot = DIV_ROUND_CLOSEST(port->uartclk * 4, baud);
@@ -2151,9 +2144,9 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
* else we see data corruption.
*/
if (uap->vendor->oversampling) {
- if ((baud >= 3000000) && (baud < 3250000) && (quot > 1))
+ if (baud >= 3000000 && baud < 3250000 && quot > 1)
quot -= 1;
- else if ((baud > 3250000) && (quot > 2))
+ else if (baud > 3250000 && quot > 2)
quot -= 2;
}
/* Set baud rate */
@@ -2222,13 +2215,14 @@ static void pl011_config_port(struct uart_port *port, int flags)
static int pl011_verify_port(struct uart_port *port, struct serial_struct *ser)
{
int ret = 0;
+
if (ser->type != PORT_UNKNOWN && ser->type != PORT_AMBA)
ret = -EINVAL;
if (ser->irq < 0 || ser->irq >= nr_irqs)
ret = -EINVAL;
if (ser->baud_base < 9600)
ret = -EINVAL;
- if (port->mapbase != (unsigned long) ser->iomem_base)
+ if (port->mapbase != (unsigned long)ser->iomem_base)
ret = -EINVAL;
return ret;
}
@@ -2537,7 +2531,7 @@ static void qdf2400_e44_putc(struct uart_port *port, unsigned char c)
cpu_relax();
}
-static void qdf2400_e44_early_write(struct console *con, const char *s, unsigned n)
+static void qdf2400_e44_early_write(struct console *con, const char *s, unsigned int n)
{
struct earlycon_device *dev = con->data;
@@ -2556,7 +2550,7 @@ static void pl011_putc(struct uart_port *port, unsigned char c)
cpu_relax();
}
-static void pl011_early_write(struct console *con, const char *s, unsigned n)
+static void pl011_early_write(struct console *con, const char *s, unsigned int n)
{
struct earlycon_device *dev = con->data;
@@ -2617,6 +2611,7 @@ static int __init pl011_early_console_setup(struct earlycon_device *device,
return 0;
}
+
OF_EARLYCON_DECLARE(pl011, "arm,pl011", pl011_early_console_setup);
OF_EARLYCON_DECLARE(pl011, "arm,sbsa-uart", pl011_early_console_setup);
@@ -2640,6 +2635,7 @@ qdf2400_e44_early_console_setup(struct earlycon_device *device,
device->con->write = qdf2400_e44_early_write;
return 0;
}
+
EARLYCON_DECLARE(qdf2400_e44, qdf2400_e44_early_console_setup);
#else
@@ -2659,8 +2655,8 @@ static struct uart_driver amba_reg = {
static int pl011_probe_dt_alias(int index, struct device *dev)
{
struct device_node *np;
- static bool seen_dev_with_alias = false;
- static bool seen_dev_without_alias = false;
+ static bool seen_dev_with_alias;
+ static bool seen_dev_without_alias;
int ret = index;
if (!IS_ENABLED(CONFIG_OF))
@@ -2676,7 +2672,7 @@ static int pl011_probe_dt_alias(int index, struct device *dev)
ret = index;
} else {
seen_dev_with_alias = true;
- if (ret >= ARRAY_SIZE(amba_ports) || amba_ports[ret] != NULL) {
+ if (ret >= ARRAY_SIZE(amba_ports) || amba_ports[ret]) {
dev_warn(dev, "requested serial port %d not available.\n", ret);
ret = index;
}
@@ -2710,7 +2706,7 @@ static int pl011_find_free_port(void)
int i;
for (i = 0; i < ARRAY_SIZE(amba_ports); i++)
- if (amba_ports[i] == NULL)
+ if (!amba_ports[i])
return i;
return -EBUSY;
@@ -2916,9 +2912,12 @@ static int sbsa_uart_probe(struct platform_device *pdev)
if (qdf2400_e44_present) {
dev_info(&pdev->dev, "working around QDF2400 SoC erratum 44\n");
uap->vendor = &vendor_qdt_qdf2400_e44;
- } else
-#endif
+ } else {
uap->vendor = &vendor_sbsa;
+ }
+#else
+ uap->vendor = &vendor_sbsa;
+#endif
uap->reg_offset = uap->vendor->reg_offset;
uap->fifosize = 32;
@@ -3002,7 +3001,7 @@ static struct amba_driver pl011_driver = {
static int __init pl011_init(void)
{
- printk(KERN_INFO "Serial: AMBA PL011 UART driver\n");
+ pr_info("Serial: AMBA PL011 UART driver\n");
if (platform_driver_register(&arm_sbsa_uart_platform_driver))
pr_warn("could not register SBSA UART platform driver\n");
--
2.41.0
On Thu, 26 Oct 2023, Théo Lebrun wrote:
> pl011_console_get_options() gets called to retrieve currently configured
> options from the registers. Previously, LCRH_TX.WLEN was being parsed
> as either 7 or 8 (fallback). Hardware supports values from 5 to 8
> inclusive, which pl011_set_termios() exploits for example.
>
> Signed-off-by: Théo Lebrun <[email protected]>
> ---
> drivers/tty/serial/amba-pl011.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 5774d48c7f16..b2062e4cbbab 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2384,10 +2384,7 @@ static void pl011_console_get_options(struct uart_amba_port *uap, int *baud,
> *parity = 'o';
> }
>
> - if ((lcr_h & 0x60) == UART01x_LCRH_WLEN_7)
> - *bits = 7;
> - else
> - *bits = 8;
> + *bits = FIELD_GET(0x60, lcr_h) + 5; /* from 5 to 8 inclusive */
0x60 needs to be replaced with a named define!
--
i.
Hi,
On Thu Oct 26, 2023 at 1:13 PM CEST, Ilpo Järvinen wrote:
> On Thu, 26 Oct 2023, Théo Lebrun wrote:
> >
> > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> > index 5774d48c7f16..b2062e4cbbab 100644
> > --- a/drivers/tty/serial/amba-pl011.c
> > +++ b/drivers/tty/serial/amba-pl011.c
> > @@ -2384,10 +2384,7 @@ static void pl011_console_get_options(struct uart_amba_port *uap, int *baud,
> > *parity = 'o';
> > }
> >
> > - if ((lcr_h & 0x60) == UART01x_LCRH_WLEN_7)
> > - *bits = 7;
> > - else
> > - *bits = 8;
> > + *bits = FIELD_GET(0x60, lcr_h) + 5; /* from 5 to 8 inclusive */
>
> 0x60 needs to be replaced with a named define!
Fixed locally for the next revision, thanks!
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <[email protected]> wrote:
> The driver uses bit shifts and hexadecimal expressions to declare
> constants. Replace that with the BIT() macro that clarifies intent.
>
> Signed-off-by: Théo Lebrun <[email protected]>
> ---
> include/linux/amba/serial.h | 182 ++++++++++++++++++++++----------------------
Since you don't know where this header may be included, you should
add #include <linux/bits.h> at the top of linux/amba/serial.h
With that fixed:
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <[email protected]> wrote:
> Follow recommandations from:
>
> $ ./scripts/checkpatch.pl --strict --file \
> drivers/tty/serial/amba-pl011.c
>
> It does NOT fix alerts relative to TIOCMBIT which will be dealt with in
> another patch. Fixes following alerts:
>
> CHECK: Alignment should match open parenthesis
> CHECK: Blank lines aren't necessary after an open brace '{'
> CHECK: Comparison to NULL could be written [...]
> CHECK: Lines should not end with a '('
> CHECK: Please don't use multiple blank lines
> CHECK: Please use a blank line after function/struct/union/enum declarations
> CHECK: Prefer using the BIT macro
> CHECK: Unbalanced braces around else statement
> CHECK: Unnecessary parentheses around [...]
> CHECK: braces {} should be used on all arms of this statement
> CHECK: spaces preferred around that '/' (ctx:VxV)
> CHECK: spaces preferred around that '|' (ctx:VxV)
> ERROR: do not initialise statics to false
> WARNING: Comparisons should place the constant on the right side of the test
> WARNING: Possible unnecessary 'out of memory' message
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> WARNING: Prefer [subsystem eg: netdev]_info([subsystem]dev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
> WARNING: quoted string split across lines
>
> Signed-off-by: Théo Lebrun <[email protected]>
These look harmless enough to me.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <[email protected]> wrote:
> pl011_console_get_options() gets called to retrieve currently configured
> options from the registers. Previously, LCRH_TX.WLEN was being parsed
> as either 7 or 8 (fallback). Hardware supports values from 5 to 8
> inclusive, which pl011_set_termios() exploits for example.
>
> Signed-off-by: Théo Lebrun <[email protected]>
With Ilpo's comment fixed:
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
Hello,
On Thu Oct 26, 2023 at 3:37 PM CEST, Linus Walleij wrote:
> Since you don't know where this header may be included, you should
> add #include <linux/bits.h> at the top of linux/amba/serial.h
>
> With that fixed:
> Reviewed-by: Linus Walleij <[email protected]>
Clearly, thanks for that.
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hello,
On Thu Oct 26, 2023 at 3:48 PM CEST, Linus Walleij wrote:
> On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <[email protected]> wrote:
>
> > pl011_console_get_options() gets called to retrieve currently configured
> > options from the registers. Previously, LCRH_TX.WLEN was being parsed
> > as either 7 or 8 (fallback). Hardware supports values from 5 to 8
> > inclusive, which pl011_set_termios() exploits for example.
> >
> > Signed-off-by: Théo Lebrun <[email protected]>
>
> With Ilpo's comment fixed:
> Reviewed-by: Linus Walleij <[email protected]>
It's been fixed locally. Thank you for your review Linus!
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Thu, 26 Oct 2023 12:41:23 +0200
Th?o Lebrun <[email protected]> wrote:
Hi,
I would change the commit title to better indicate that you add support
for bits 5 and 6, which was missing.
Maybe "Add support for 5 and 6 bits in..." ?
> pl011_console_get_options() gets called to retrieve currently configured
> options from the registers. Previously, LCRH_TX.WLEN was being parsed
It took me some time to understand your explanation :) Maybe change
to:
"Previously, only 7 or 8 bits were supported."
> as either 7 or 8 (fallback). Hardware supports values from 5 to 8
Add bits:
"5 to 8 bits..."
And indicate that this patch adds support for 5 and 6 bits.
> inclusive, which pl011_set_termios() exploits for example.
>
> Signed-off-by: Th?o Lebrun <[email protected]>
> ---
> drivers/tty/serial/amba-pl011.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 5774d48c7f16..b2062e4cbbab 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2384,10 +2384,7 @@ static void pl011_console_get_options(struct uart_amba_port *uap, int *baud,
> *parity = 'o';
> }
>
> - if ((lcr_h & 0x60) == UART01x_LCRH_WLEN_7)
> - *bits = 7;
> - else
> - *bits = 8;
> + *bits = FIELD_GET(0x60, lcr_h) + 5; /* from 5 to 8 inclusive */
Capital "F" -> "From...".
And add "bits" -> "From 5 to 8 bits..."
Hugo.
>
> ibrd = pl011_read(uap, REG_IBRD);
> fbrd = pl011_read(uap, REG_FBRD);
>
> --
> 2.41.0
>
Hello,
On Thu Oct 26, 2023 at 4:53 PM CEST, Hugo Villeneuve wrote:
> On Thu, 26 Oct 2023 12:41:23 +0200
> Théo Lebrun <[email protected]> wrote:
>
> Hi,
> I would change the commit title to better indicate that you add support
> for bits 5 and 6, which was missing.
>
> Maybe "Add support for 5 and 6 bits in..." ?
>
> > pl011_console_get_options() gets called to retrieve currently configured
> > options from the registers. Previously, LCRH_TX.WLEN was being parsed
>
> It took me some time to understand your explanation :) Maybe change
> to:
>
> "Previously, only 7 or 8 bits were supported."
>
> > as either 7 or 8 (fallback). Hardware supports values from 5 to 8
>
> Add bits:
>
> "5 to 8 bits..."
>
> And indicate that this patch adds support for 5 and 6 bits.
I agree the whole commit message is unclear. Let's rewrite it. What do
you think of the following:
tty: serial: amba-pl011: Allow parsing word length of 5/6 bits at console setup
If no options are given at console setup, we parse hardware register
LCRH_TX.WLEN for bits per word. We compare the value to the 7 bits
value (UART01x_LCRH_WLEN_7). If the hardware is configured for 5, 6
or 8 bits per word, we fallback to 8 bits.
Change that behavior to parse the whole range available: from 5 to 8
bits per word.
Note that we don't add support for 5/6 bits, we only update the parsing
of the regs (if no options are passed at setup) to reflect the current
hardware config. The behavior will be different only if the inherited
value (from reset/bootloader) is 5 or 6: previously we guessed 8 bits
word length, now we guess the right value.
What's your opinion on this new commit message?
Thanks!
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Oct 31, 2023 at 10:35:29AM +0100, Th?o Lebrun wrote:
> Hello,
>
> On Thu Oct 26, 2023 at 4:53 PM CEST, Hugo Villeneuve wrote:
> > On Thu, 26 Oct 2023 12:41:23 +0200
> > Th?o Lebrun <[email protected]> wrote:
> >
> > Hi,
> > I would change the commit title to better indicate that you add support
> > for bits 5 and 6, which was missing.
> >
> > Maybe "Add support for 5 and 6 bits in..." ?
> >
> > > pl011_console_get_options() gets called to retrieve currently configured
> > > options from the registers. Previously, LCRH_TX.WLEN was being parsed
> >
> > It took me some time to understand your explanation :) Maybe change
> > to:
> >
> > "Previously, only 7 or 8 bits were supported."
> >
> > > as either 7 or 8 (fallback). Hardware supports values from 5 to 8
> >
> > Add bits:
> >
> > "5 to 8 bits..."
> >
> > And indicate that this patch adds support for 5 and 6 bits.
>
> I agree the whole commit message is unclear. Let's rewrite it. What do
> you think of the following:
>
> tty: serial: amba-pl011: Allow parsing word length of 5/6 bits at console setup
>
> If no options are given at console setup, we parse hardware register
> LCRH_TX.WLEN for bits per word. We compare the value to the 7 bits
> value (UART01x_LCRH_WLEN_7). If the hardware is configured for 5, 6
> or 8 bits per word, we fallback to 8 bits.
>
> Change that behavior to parse the whole range available: from 5 to 8
> bits per word.
>
> Note that we don't add support for 5/6 bits, we only update the parsing
> of the regs (if no options are passed at setup) to reflect the current
> hardware config. The behavior will be different only if the inherited
> value (from reset/bootloader) is 5 or 6: previously we guessed 8 bits
> word length, now we guess the right value.
>
> What's your opinion on this new commit message?
There is no point in supporting 5 or 6 bits for console usage. Think
about it. What values are going to be sent over the console? It'll be
ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha
characters into control characters, punctuation and numbers. 5-bit
would be all control characters.
So there's no point trying to do anything with 5 or 6 bits per byte,
and I decided we might as well take that as an error (or maybe a
case that the hardware has not been setup) and default to 8 bits per
byte.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hello,
On Tue Oct 31, 2023 at 11:11 AM CET, Russell King (Oracle) wrote:
> There is no point in supporting 5 or 6 bits for console usage. Think
> about it. What values are going to be sent over the console? It'll be
> ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha
> characters into control characters, punctuation and numbers. 5-bit
> would be all control characters.
>
> So there's no point trying to do anything with 5 or 6 bits per byte,
> and I decided we might as well take that as an error (or maybe a
> case that the hardware has not been setup) and default to 8 bits per
> byte.
I see your point. Two things come to mind:
- I added this parsing of 5/6 bits to be symmetrical with
pl011_set_termios that handles 5/6 properly. Should pl011_set_termios
be modified then?
- If a value of 5 or 6 means the hardware has not been setup, shouldn't
we ignore all other parsed values?
If you decide to keep the current behavior, I'd be down to adding a
comment to explicit this choice in pl011_console_get_options.
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
------------------------------------------------------------------------
On Tue, Oct 31, 2023 at 12:04:11PM +0100, Th?o Lebrun wrote:
> Hello,
>
> On Tue Oct 31, 2023 at 11:11 AM CET, Russell King (Oracle) wrote:
> > There is no point in supporting 5 or 6 bits for console usage. Think
> > about it. What values are going to be sent over the console? It'll be
> > ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha
> > characters into control characters, punctuation and numbers. 5-bit
> > would be all control characters.
> >
> > So there's no point trying to do anything with 5 or 6 bits per byte,
> > and I decided we might as well take that as an error (or maybe a
> > case that the hardware has not been setup) and default to 8 bits per
> > byte.
>
> I see your point. Two things come to mind:
>
> - I added this parsing of 5/6 bits to be symmetrical with
> pl011_set_termios that handles 5/6 properly. Should pl011_set_termios
> be modified then?
Why should it? Note that I said above about _console_ usage which is
what you were referring to - the early code that sets up the console
by either reading the current settings (so that we can transparently
use the UART when its handed over already setup by a boot loader).
This is completely different to what happens once the kernel is running.
Userspace might very well have a reason to set 5 or 6 bits if it wants
to communicate with a device that uses those sizes.
However, such a device won't be a console for the reasons I outlined
above (it will truncate the ASCII characters turning console messages
into garbage.)
> If you decide to keep the current behavior, I'd be down to adding a
> comment to explicit this choice in pl011_console_get_options.
Well, honestly I don't think it needs a comment _if_ one thinks about
what these sizes mean for what is supposed to be a console displaying
ASCII characters. It feels to me like pointing out the obvious, and
would be on the level of teaching people how to suck eggs... but then
again, maybe there are times when people need to be taught how to
suck eggs...
So yes, add a comment if you think it's a good idea, but should that
comment be replicated in almost every driver or should it be documented
elsewhere?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Tue, 31 Oct 2023 10:35:29 +0100
Th?o Lebrun <[email protected]> wrote:
> Hello,
>
> On Thu Oct 26, 2023 at 4:53 PM CEST, Hugo Villeneuve wrote:
> > On Thu, 26 Oct 2023 12:41:23 +0200
> > Th?o Lebrun <[email protected]> wrote:
> >
> > Hi,
> > I would change the commit title to better indicate that you add support
> > for bits 5 and 6, which was missing.
> >
> > Maybe "Add support for 5 and 6 bits in..." ?
> >
> > > pl011_console_get_options() gets called to retrieve currently configured
> > > options from the registers. Previously, LCRH_TX.WLEN was being parsed
> >
> > It took me some time to understand your explanation :) Maybe change
> > to:
> >
> > "Previously, only 7 or 8 bits were supported."
> >
> > > as either 7 or 8 (fallback). Hardware supports values from 5 to 8
> >
> > Add bits:
> >
> > "5 to 8 bits..."
> >
> > And indicate that this patch adds support for 5 and 6 bits.
>
> I agree the whole commit message is unclear. Let's rewrite it. What do
> you think of the following:
>
> tty: serial: amba-pl011: Allow parsing word length of 5/6 bits at console setup
>
> If no options are given at console setup, we parse hardware register
> LCRH_TX.WLEN for bits per word. We compare the value to the 7 bits
> value (UART01x_LCRH_WLEN_7). If the hardware is configured for 5, 6
> or 8 bits per word, we fallback to 8 bits.
>
> Change that behavior to parse the whole range available: from 5 to 8
> bits per word.
>
> Note that we don't add support for 5/6 bits, we only update the parsing
> of the regs (if no options are passed at setup) to reflect the current
> hardware config. The behavior will be different only if the inherited
> value (from reset/bootloader) is 5 or 6: previously we guessed 8 bits
> word length, now we guess the right value.
>
> What's your opinion on this new commit message?
Hi,
that's fine with me.
Hugo.
On Tue Oct 31, 2023 at 12:22 PM CET, Russell King (Oracle) wrote:
> On Tue, Oct 31, 2023 at 12:04:11PM +0100, Théo Lebrun wrote:
> > On Tue Oct 31, 2023 at 11:11 AM CET, Russell King (Oracle) wrote:
> > > There is no point in supporting 5 or 6 bits for console usage. Think
> > > about it. What values are going to be sent over the console? It'll be
> > > ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha
> > > characters into control characters, punctuation and numbers. 5-bit
> > > would be all control characters.
> > >
> > > So there's no point trying to do anything with 5 or 6 bits per byte,
> > > and I decided we might as well take that as an error (or maybe a
> > > case that the hardware has not been setup) and default to 8 bits per
> > > byte.
> >
> > I see your point. Two things come to mind:
> >
> > - I added this parsing of 5/6 bits to be symmetrical with
> > pl011_set_termios that handles 5/6 properly. Should pl011_set_termios
> > be modified then?
>
> Why should it? Note that I said above about _console_ usage which is
> what you were referring to - the early code that sets up the console
> by either reading the current settings (so that we can transparently
> use the UART when its handed over already setup by a boot loader).
>
> This is completely different to what happens once the kernel is running.
> Userspace might very well have a reason to set 5 or 6 bits if it wants
> to communicate with a device that uses those sizes.
>
> However, such a device won't be a console for the reasons I outlined
> above (it will truncate the ASCII characters turning console messages
> into garbage.)
I'm not sure I get it. (1) We assume it is a console so it's ASCII so no
reason to set to 5 or 6 bits per word. But (2) there might be a reason
to set the UART to 5 or 6 bits, the userspace decides.
How do the two interact? Say we boot to Linux, userspace configures to 6
bits because reasons and we reset. At second probe we see a config of 6
bits per word but assume that can't be logical, even though it is.
What makes us suppose at probe that it must be a console?
I won't die on a hill for this topic; we'll go the way you prefer!
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Oct 31, 2023 at 02:51:45PM +0100, Th?o Lebrun wrote:
> On Tue Oct 31, 2023 at 12:22 PM CET, Russell King (Oracle) wrote:
> > On Tue, Oct 31, 2023 at 12:04:11PM +0100, Th?o Lebrun wrote:
> > > On Tue Oct 31, 2023 at 11:11 AM CET, Russell King (Oracle) wrote:
> > > > There is no point in supporting 5 or 6 bits for console usage. Think
> > > > about it. What values are going to be sent over the console? It'll be
> > > > ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha
> > > > characters into control characters, punctuation and numbers. 5-bit
> > > > would be all control characters.
> > > >
> > > > So there's no point trying to do anything with 5 or 6 bits per byte,
> > > > and I decided we might as well take that as an error (or maybe a
> > > > case that the hardware has not been setup) and default to 8 bits per
> > > > byte.
> > >
> > > I see your point. Two things come to mind:
> > >
> > > - I added this parsing of 5/6 bits to be symmetrical with
> > > pl011_set_termios that handles 5/6 properly. Should pl011_set_termios
> > > be modified then?
> >
> > Why should it? Note that I said above about _console_ usage which is
> > what you were referring to - the early code that sets up the console
> > by either reading the current settings (so that we can transparently
> > use the UART when its handed over already setup by a boot loader).
> >
> > This is completely different to what happens once the kernel is running.
> > Userspace might very well have a reason to set 5 or 6 bits if it wants
> > to communicate with a device that uses those sizes.
> >
> > However, such a device won't be a console for the reasons I outlined
> > above (it will truncate the ASCII characters turning console messages
> > into garbage.)
>
> I'm not sure I get it. (1) We assume it is a console so it's ASCII so no
> reason to set to 5 or 6 bits per word. But (2) there might be a reason
> to set the UART to 5 or 6 bits, the userspace decides.
Precisely.
> How do the two interact? Say we boot to Linux, userspace configures to 6
> bits because reasons and we reset. At second probe we see a config of 6
> bits per word but assume that can't be logical, even though it is.
I think you're conflating "serial console" with "serial port". A
"serial port" can support multiple different formats, and in this case,
such as 5, 6, 7, and 8 bits. 5 and 6 bits are likely to be a specialised
application which uses a binary protocol, not ASCII.
A "serial console" is one application of a "serial port" and a "serial
console" is used to send ASCII characters, not a binary protocol.
> What makes us suppose at probe that it must be a console?
Sorry, but no, we don't assume every serial port is a serial console.
Unless something has changed since I was involved with the serial
layer, we only read the parameters from a serial port _if_ and only
if that port is being used as a serial console.
TTYs under Linux have a standard initial set of parameters at boot -
9600 baud, 8 bits, etc. The exception to this is if a serial port *is
being used* as a serial console, where these settings are overriden by
the serial console configuration. The reason for that is... imagine
the chaos that would occur if:
- Your boot loader configures (through user configuration) the serial
port for 115200 baud.
- The boot loader loads the kernel and passes control to it, with
a command line specifying that this serial port is to be used for
the serial console, but not specifying any parameters.
- The kernel boots, and starts outputting messages at 115200 baud.
- Userspace starts running, opens /dev/console, and switches the port
to 9600 baud. Now you have utter garbage being received from the
serial console.
So, the serial console is special in that regard.
Now, when we configure the serial console, we attempt to:
1) parse the options provided on the console= line to set the serial
port appropriately, or
2) if there are no options, then we attempt to set the serial port to
something sane *for use as a serial console*, which uses ASCII protocol
not some random binary protocol. 5 and 6 bits make no sense here.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hello,
On Tue Oct 31, 2023 at 3:05 PM CET, Russell King (Oracle) wrote:
> On Tue, Oct 31, 2023 at 02:51:45PM +0100, Théo Lebrun wrote:
> > On Tue Oct 31, 2023 at 12:22 PM CET, Russell King (Oracle) wrote:
> > > On Tue, Oct 31, 2023 at 12:04:11PM +0100, Théo Lebrun wrote:
> > > > On Tue Oct 31, 2023 at 11:11 AM CET, Russell King (Oracle) wrote:
> > > > > There is no point in supporting 5 or 6 bits for console usage. Think
> > > > > about it. What values are going to be sent over the console? It'll be
> > > > > ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha
> > > > > characters into control characters, punctuation and numbers. 5-bit
> > > > > would be all control characters.
> > > > >
> > > > > So there's no point trying to do anything with 5 or 6 bits per byte,
> > > > > and I decided we might as well take that as an error (or maybe a
> > > > > case that the hardware has not been setup) and default to 8 bits per
> > > > > byte.
> > > >
> > > > I see your point. Two things come to mind:
> > > >
> > > > - I added this parsing of 5/6 bits to be symmetrical with
> > > > pl011_set_termios that handles 5/6 properly. Should pl011_set_termios
> > > > be modified then?
> > >
> > > Why should it? Note that I said above about _console_ usage which is
> > > what you were referring to - the early code that sets up the console
> > > by either reading the current settings (so that we can transparently
> > > use the UART when its handed over already setup by a boot loader).
> > >
> > > This is completely different to what happens once the kernel is running.
> > > Userspace might very well have a reason to set 5 or 6 bits if it wants
> > > to communicate with a device that uses those sizes.
> > >
> > > However, such a device won't be a console for the reasons I outlined
> > > above (it will truncate the ASCII characters turning console messages
> > > into garbage.)
> >
> > I'm not sure I get it. (1) We assume it is a console so it's ASCII so no
> > reason to set to 5 or 6 bits per word. But (2) there might be a reason
> > to set the UART to 5 or 6 bits, the userspace decides.
>
> Precisely.
>
> > How do the two interact? Say we boot to Linux, userspace configures to 6
> > bits because reasons and we reset. At second probe we see a config of 6
> > bits per word but assume that can't be logical, even though it is.
>
> I think you're conflating "serial console" with "serial port". A
> "serial port" can support multiple different formats, and in this case,
> such as 5, 6, 7, and 8 bits. 5 and 6 bits are likely to be a specialised
> application which uses a binary protocol, not ASCII.
>
> A "serial console" is one application of a "serial port" and a "serial
> console" is used to send ASCII characters, not a binary protocol.
That was all clear in my mind; I was missing the following bit:
> Sorry, but no, we don't assume every serial port is a serial console.
> Unless something has changed since I was involved with the serial
> layer, **we only read the parameters from a serial port _if_ and only
> if that port is being used as a serial console.**
Thank you for the time you took; I'll get rid of the patch and send a V2
fixing nits for other patches.
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com