[PATCH 0/6] Cleanup AMBA PL011 driver

[PATCH 0/6] Cleanup AMBA PL011 driver

[Théo Lebrun]

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/202310050726.GDpZbMDO-lkp@intel.com/T/

Have a nice day,
Théo Lebrun

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
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 <theo.lebrun@bootlin.com>

[PATCH 1/6] tty: serial: amba: cleanup whitespace

[Théo Lebrun]

Fix whitespace in include/linux/amba/serial.h to match current kernel
coding standards. Fixes about:

 - CHECK: spaces preferred around that '|' (ctx:VxV)
 - ERROR: code indent should use tabs where possible
 - WARNING: Unnecessary space before function pointer arguments
 - WARNING: please, no spaces at the start of a line

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 include/linux/amba/serial.h | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/linux/amba/serial.h b/include/linux/amba/serial.h
index a1307b58cc2c..27003ec52114 100644
--- a/include/linux/amba/serial.h
+++ b/include/linux/amba/serial.h
@@ -75,10 +75,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 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
@@ -86,9 +86,9 @@
 #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 UART01x_FR_DCD		0x004
+#define UART01x_FR_DSR		0x002
+#define UART01x_FR_CTS		0x001
 #define UART01x_FR_TMSK		(UART01x_FR_TXFF + UART01x_FR_BUSY)
 
 /*
@@ -110,14 +110,14 @@
 #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_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 UART01x_LCRH_WLEN_8	0x60
 #define UART01x_LCRH_WLEN_7	0x40
@@ -203,8 +203,8 @@
 #define UART011_TXDMAE		(1 << 1)	/* enable transmit dma */
 #define UART011_RXDMAE		(1 << 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)
+#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)
 
 #ifndef __ASSEMBLY__
 struct amba_device; /* in uncompress this is included but amba/bus.h is not */
@@ -220,8 +220,8 @@ struct amba_pl011_data {
 	bool dma_rx_poll_enable;
 	unsigned int dma_rx_poll_rate;
 	unsigned int dma_rx_poll_timeout;
-        void (*init) (void);
-	void (*exit) (void);
+	void (*init)(void);
+	void (*exit)(void);
 };
 #endif
 

-- 
2.41.0

Re: [PATCH 1/6] tty: serial: amba: cleanup whitespace

[Linus Walleij]

On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Fix whitespace in include/linux/amba/serial.h to match current kernel
> coding standards. Fixes about:
>
>  - CHECK: spaces preferred around that '|' (ctx:VxV)
>  - ERROR: code indent should use tabs where possible
>  - WARNING: Unnecessary space before function pointer arguments
>  - WARNING: please, no spaces at the start of a line
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 2/6] tty: serial: amba: Use BIT() macro for constant declarations

[Théo Lebrun]

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 <theo.lebrun@bootlin.com>
---
 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

Re: [PATCH 2/6] tty: serial: amba: Use BIT() macro for constant declarations

[Linus Walleij]

On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <theo.lebrun@bootlin.com> 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 <theo.lebrun@bootlin.com>
> ---
>  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 <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 2/6] tty: serial: amba: Use BIT() macro for constant declarations

[Théo Lebrun]

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 <linus.walleij@linaro.org>

Clearly, thanks for that.

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 3/6] tty: serial: amba-pl011: cleanup driver

[Théo Lebrun]

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 <theo.lebrun@bootlin.com>
---
 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

Re: [PATCH 3/6] tty: serial: amba-pl011: cleanup driver

[Linus Walleij]

On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <theo.lebrun@bootlin.com> 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 <theo.lebrun@bootlin.com>

These look harmless enough to me.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 4/6] tty: serial: amba-pl011: replace TIOCMBIT macros by static functions

[Théo Lebrun]

The driver uses two TIOCMBIT macros inside pl011_{get,set}_mctrl to
simplify the logic. Those look scary to checkpatch because they contain
ifs without do-while loops.

Avoid the macros by creating small equivalent static functions; that
lets the compiler do its type checking & avoids checkpatch errors.

For the second instance __assign_bit is not usable because it deals with
unsigned long pointers whereas we have an unsigned int in
pl011_set_mctrl.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/tty/serial/amba-pl011.c | 46 +++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 0d53973374de..bb3082c4d35c 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1087,7 +1087,6 @@ static void pl011_dma_rx_poll(struct timer_list *t)
 	 */
 	if (jiffies_to_msecs(jiffies - dmarx->last_jiffies)
 			> uap->dmarx.poll_timeout) {
-
 		spin_lock_irqsave(&uap->port.lock, flags);
 		pl011_dma_rx_stop(uap);
 		uap->im |= UART011_RXIM;
@@ -1595,6 +1594,12 @@ static unsigned int pl011_tx_empty(struct uart_port *port)
 							0 : TIOCSER_TEMT;
 }
 
+static void pl011_maybe_set_bit(bool cond, unsigned int *ptr, unsigned int mask)
+{
+	if (cond)
+		*ptr |= mask;
+}
+
 static unsigned int pl011_get_mctrl(struct uart_port *port)
 {
 	struct uart_amba_port *uap =
@@ -1602,18 +1607,22 @@ static unsigned int pl011_get_mctrl(struct uart_port *port)
 	unsigned int result = 0;
 	unsigned int status = pl011_read(uap, REG_FR);
 
-#define TIOCMBIT(uartbit, tiocmbit)	\
-	if (status & uartbit)		\
-		result |= tiocmbit
+	pl011_maybe_set_bit(status & UART01x_FR_DCD, &result, TIOCM_CAR);
+	pl011_maybe_set_bit(status & uap->vendor->fr_dsr, &result, TIOCM_DSR);
+	pl011_maybe_set_bit(status & uap->vendor->fr_cts, &result, TIOCM_CTS);
+	pl011_maybe_set_bit(status & uap->vendor->fr_ri, &result, TIOCM_RNG);
 
-	TIOCMBIT(UART01x_FR_DCD, TIOCM_CAR);
-	TIOCMBIT(uap->vendor->fr_dsr, TIOCM_DSR);
-	TIOCMBIT(uap->vendor->fr_cts, TIOCM_CTS);
-	TIOCMBIT(uap->vendor->fr_ri, TIOCM_RNG);
-#undef TIOCMBIT
 	return result;
 }
 
+static void pl011_assign_bit(bool cond, unsigned int *ptr, unsigned int mask)
+{
+	if (cond)
+		*ptr |= mask;
+	else
+		*ptr &= ~mask;
+}
+
 static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
 	struct uart_amba_port *uap =
@@ -1622,23 +1631,16 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
 
 	cr = pl011_read(uap, REG_CR);
 
-#define	TIOCMBIT(tiocmbit, uartbit)		\
-	if (mctrl & tiocmbit)		\
-		cr |= uartbit;		\
-	else				\
-		cr &= ~uartbit
-
-	TIOCMBIT(TIOCM_RTS, UART011_CR_RTS);
-	TIOCMBIT(TIOCM_DTR, UART011_CR_DTR);
-	TIOCMBIT(TIOCM_OUT1, UART011_CR_OUT1);
-	TIOCMBIT(TIOCM_OUT2, UART011_CR_OUT2);
-	TIOCMBIT(TIOCM_LOOP, UART011_CR_LBE);
+	pl011_assign_bit(mctrl & TIOCM_RTS, &cr, UART011_CR_RTS);
+	pl011_assign_bit(mctrl & TIOCM_DTR, &cr, UART011_CR_DTR);
+	pl011_assign_bit(mctrl & TIOCM_OUT1, &cr, UART011_CR_OUT1);
+	pl011_assign_bit(mctrl & TIOCM_OUT2, &cr, UART011_CR_OUT2);
+	pl011_assign_bit(mctrl & TIOCM_LOOP, &cr, UART011_CR_LBE);
 
 	if (port->status & UPSTAT_AUTORTS) {
 		/* We need to disable auto-RTS if we want to turn RTS off */
-		TIOCMBIT(TIOCM_RTS, UART011_CR_RTSEN);
+		pl011_assign_bit(mctrl & TIOCM_RTS, &cr, UART011_CR_RTSEN);
 	}
-#undef TIOCMBIT
 
 	pl011_write(cr, uap, REG_CR);
 }

-- 
2.41.0

Re: [PATCH 4/6] tty: serial: amba-pl011: replace TIOCMBIT macros by static functions

[Linus Walleij]

On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> The driver uses two TIOCMBIT macros inside pl011_{get,set}_mctrl to
> simplify the logic. Those look scary to checkpatch because they contain
> ifs without do-while loops.
>
> Avoid the macros by creating small equivalent static functions; that
> lets the compiler do its type checking & avoids checkpatch errors.
>
> For the second instance __assign_bit is not usable because it deals with
> unsigned long pointers whereas we have an unsigned int in
> pl011_set_mctrl.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

The kernel looks better after this patch than before, so:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Since the eternal defines uses masks rather than bits we can't
use __set_bit() & friends in this case and that's life.

Yours,
Linus Walleij

Re: [PATCH 4/6] tty: serial: amba-pl011: replace TIOCMBIT macros by static functions

[Hugo Villeneuve]

On Thu, 26 Oct 2023 12:41:21 +0200
Théo Lebrun <theo.lebrun@bootlin.com> wrote:

Hi,

> The driver uses two TIOCMBIT macros inside pl011_{get,set}_mctrl to
> simplify the logic. Those look scary to checkpatch because they contain
> ifs without do-while loops.
> 
> Avoid the macros by creating small equivalent static functions; that
> lets the compiler do its type checking & avoids checkpatch errors.
> 
> For the second instance __assign_bit is not usable because it deals with
> unsigned long pointers whereas we have an unsigned int in
> pl011_set_mctrl.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/tty/serial/amba-pl011.c | 46 +++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 0d53973374de..bb3082c4d35c 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -1087,7 +1087,6 @@ static void pl011_dma_rx_poll(struct timer_list *t)
>  	 */
>  	if (jiffies_to_msecs(jiffies - dmarx->last_jiffies)
>  			> uap->dmarx.poll_timeout) {
> -

This should go into a separate patch, or simply be merged with one
of your other coding style/whitespace cleanup patches.

Hugo.


>  		spin_lock_irqsave(&uap->port.lock, flags);
>  		pl011_dma_rx_stop(uap);
>  		uap->im |= UART011_RXIM;
> @@ -1595,6 +1594,12 @@ static unsigned int pl011_tx_empty(struct uart_port *port)
>  							0 : TIOCSER_TEMT;
>  }
>  
> +static void pl011_maybe_set_bit(bool cond, unsigned int *ptr, unsigned int mask)
> +{
> +	if (cond)
> +		*ptr |= mask;
> +}
> +
>  static unsigned int pl011_get_mctrl(struct uart_port *port)
>  {
>  	struct uart_amba_port *uap =
> @@ -1602,18 +1607,22 @@ static unsigned int pl011_get_mctrl(struct uart_port *port)
>  	unsigned int result = 0;
>  	unsigned int status = pl011_read(uap, REG_FR);
>  
> -#define TIOCMBIT(uartbit, tiocmbit)	\
> -	if (status & uartbit)		\
> -		result |= tiocmbit
> +	pl011_maybe_set_bit(status & UART01x_FR_DCD, &result, TIOCM_CAR);
> +	pl011_maybe_set_bit(status & uap->vendor->fr_dsr, &result, TIOCM_DSR);
> +	pl011_maybe_set_bit(status & uap->vendor->fr_cts, &result, TIOCM_CTS);
> +	pl011_maybe_set_bit(status & uap->vendor->fr_ri, &result, TIOCM_RNG);
>  
> -	TIOCMBIT(UART01x_FR_DCD, TIOCM_CAR);
> -	TIOCMBIT(uap->vendor->fr_dsr, TIOCM_DSR);
> -	TIOCMBIT(uap->vendor->fr_cts, TIOCM_CTS);
> -	TIOCMBIT(uap->vendor->fr_ri, TIOCM_RNG);
> -#undef TIOCMBIT
>  	return result;
>  }
>  
> +static void pl011_assign_bit(bool cond, unsigned int *ptr, unsigned int mask)
> +{
> +	if (cond)
> +		*ptr |= mask;
> +	else
> +		*ptr &= ~mask;
> +}
> +
>  static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  {
>  	struct uart_amba_port *uap =
> @@ -1622,23 +1631,16 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  
>  	cr = pl011_read(uap, REG_CR);
>  
> -#define	TIOCMBIT(tiocmbit, uartbit)		\
> -	if (mctrl & tiocmbit)		\
> -		cr |= uartbit;		\
> -	else				\
> -		cr &= ~uartbit
> -
> -	TIOCMBIT(TIOCM_RTS, UART011_CR_RTS);
> -	TIOCMBIT(TIOCM_DTR, UART011_CR_DTR);
> -	TIOCMBIT(TIOCM_OUT1, UART011_CR_OUT1);
> -	TIOCMBIT(TIOCM_OUT2, UART011_CR_OUT2);
> -	TIOCMBIT(TIOCM_LOOP, UART011_CR_LBE);
> +	pl011_assign_bit(mctrl & TIOCM_RTS, &cr, UART011_CR_RTS);
> +	pl011_assign_bit(mctrl & TIOCM_DTR, &cr, UART011_CR_DTR);
> +	pl011_assign_bit(mctrl & TIOCM_OUT1, &cr, UART011_CR_OUT1);
> +	pl011_assign_bit(mctrl & TIOCM_OUT2, &cr, UART011_CR_OUT2);
> +	pl011_assign_bit(mctrl & TIOCM_LOOP, &cr, UART011_CR_LBE);
>  
>  	if (port->status & UPSTAT_AUTORTS) {
>  		/* We need to disable auto-RTS if we want to turn RTS off */
> -		TIOCMBIT(TIOCM_RTS, UART011_CR_RTSEN);
> +		pl011_assign_bit(mctrl & TIOCM_RTS, &cr, UART011_CR_RTSEN);
>  	}
> -#undef TIOCMBIT
>  
>  	pl011_write(cr, uap, REG_CR);
>  }
> 
> -- 
> 2.41.0
> 

Re: [PATCH 4/6] tty: serial: amba-pl011: replace TIOCMBIT macros by static functions

[Théo Lebrun]

Hello,

On Thu Oct 26, 2023 at 4:24 PM CEST, Hugo Villeneuve wrote:
> On Thu, 26 Oct 2023 12:41:21 +0200
> Théo Lebrun <theo.lebrun@bootlin.com> wrote:
> > The driver uses two TIOCMBIT macros inside pl011_{get,set}_mctrl to
> > simplify the logic. Those look scary to checkpatch because they contain
> > ifs without do-while loops.
> > 
> > Avoid the macros by creating small equivalent static functions; that
> > lets the compiler do its type checking & avoids checkpatch errors.
> > 
> > For the second instance __assign_bit is not usable because it deals with
> > unsigned long pointers whereas we have an unsigned int in
> > pl011_set_mctrl.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  drivers/tty/serial/amba-pl011.c | 46 +++++++++++++++++++++--------------------
> >  1 file changed, 24 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> > index 0d53973374de..bb3082c4d35c 100644
> > --- a/drivers/tty/serial/amba-pl011.c
> > +++ b/drivers/tty/serial/amba-pl011.c
> > @@ -1087,7 +1087,6 @@ static void pl011_dma_rx_poll(struct timer_list *t)
> >  	 */
> >  	if (jiffies_to_msecs(jiffies - dmarx->last_jiffies)
> >  			> uap->dmarx.poll_timeout) {
> > -
>
> This should go into a separate patch, or simply be merged with one
> of your other coding style/whitespace cleanup patches.

Indeed, added to "tty: serial: amba-pl011: cleanup driver". Thanks.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 5/6] tty: serial: amba-pl011: unindent pl011_console_get_options function body

[Théo Lebrun]

The whole function body is encapsulated inside an if-condition. Reverse
the if logic and early return to remove one indentation level.

Also turn two nested ifs into a single one at the end of the function.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/tty/serial/amba-pl011.c | 42 ++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index bb3082c4d35c..5774d48c7f16 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2369,34 +2369,34 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)
 static void pl011_console_get_options(struct uart_amba_port *uap, int *baud,
 				      int *parity, int *bits)
 {
-	if (pl011_read(uap, REG_CR) & UART01x_CR_UARTEN) {
-		unsigned int lcr_h, ibrd, fbrd;
+	unsigned int lcr_h, ibrd, fbrd;
 
-		lcr_h = pl011_read(uap, REG_LCRH_TX);
+	if (!(pl011_read(uap, REG_CR) & UART01x_CR_UARTEN))
+		return;
 
-		*parity = 'n';
-		if (lcr_h & UART01x_LCRH_PEN) {
-			if (lcr_h & UART01x_LCRH_EPS)
-				*parity = 'e';
-			else
-				*parity = 'o';
-		}
+	lcr_h = pl011_read(uap, REG_LCRH_TX);
 
-		if ((lcr_h & 0x60) == UART01x_LCRH_WLEN_7)
-			*bits = 7;
+	*parity = 'n';
+	if (lcr_h & UART01x_LCRH_PEN) {
+		if (lcr_h & UART01x_LCRH_EPS)
+			*parity = 'e';
 		else
-			*bits = 8;
+			*parity = 'o';
+	}
+
+	if ((lcr_h & 0x60) == UART01x_LCRH_WLEN_7)
+		*bits = 7;
+	else
+		*bits = 8;
 
-		ibrd = pl011_read(uap, REG_IBRD);
-		fbrd = pl011_read(uap, REG_FBRD);
+	ibrd = pl011_read(uap, REG_IBRD);
+	fbrd = pl011_read(uap, REG_FBRD);
 
-		*baud = uap->port.uartclk * 4 / (64 * ibrd + fbrd);
+	*baud = uap->port.uartclk * 4 / (64 * ibrd + fbrd);
 
-		if (uap->vendor->oversampling) {
-			if (pl011_read(uap, REG_CR)
-				  & ST_UART011_CR_OVSFACT)
-				*baud *= 2;
-		}
+	if (uap->vendor->oversampling &&
+	    (pl011_read(uap, REG_CR) & ST_UART011_CR_OVSFACT)) {
+		*baud *= 2;
 	}
 }
 

-- 
2.41.0

Re: [PATCH 5/6] tty: serial: amba-pl011: unindent pl011_console_get_options function body

[Linus Walleij]

On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> The whole function body is encapsulated inside an if-condition. Reverse
> the if logic and early return to remove one indentation level.
>
> Also turn two nested ifs into a single one at the end of the function.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

[Théo Lebrun]

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 <theo.lebrun@bootlin.com>
---
 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

Re: [PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

[Ilpo Järvinen]

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

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 <theo.lebrun@bootlin.com>
> ---
>  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.

Re: [PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

[Théo Lebrun]

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

Re: [PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

[Linus Walleij]

On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <theo.lebrun@bootlin.com> 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 <theo.lebrun@bootlin.com>

With Ilpo's comment fixed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

[Théo Lebrun]

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 <theo.lebrun@bootlin.com> 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 <theo.lebrun@bootlin.com>
>
> With Ilpo's comment fixed:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

It's been fixed locally. Thank you for your review Linus!

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

[Hugo Villeneuve]

On Thu, 26 Oct 2023 12:41:23 +0200
Théo Lebrun <theo.lebrun@bootlin.com> 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 <theo.lebrun@bootlin.com>
> ---
>  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
> 

Re: [PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

[Théo Lebrun]

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 <theo.lebrun@bootlin.com> 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

Re: [PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

[Russell King (Oracle)]

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 <theo.lebrun@bootlin.com> 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!

Re: [PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

[Théo Lebrun]

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

------------------------------------------------------------------------

Re: [PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

[Russell King (Oracle)]

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!

Re: [PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

[Théo Lebrun]

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

Re: [PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

[Russell King (Oracle)]

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!

Re: [PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

[Théo Lebrun]

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

Re: [PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

[Hugo Villeneuve]

On Tue, 31 Oct 2023 10:35:29 +0100
Théo Lebrun <theo.lebrun@bootlin.com> 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 <theo.lebrun@bootlin.com> 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.