Add IRQ support to the LiteX LiteUART serial interface
Changes from v4:
- picked up r/b Ilpo Järvinen on 12/14 and 13/14
- 12/14 ("add IRQ support for the RX path"): using dev_err() instead
of combining pr_err() and pr_fmt(); also, remove superfluous comment
> Changes from v3:
> - picked up r/b Ilpo Järvinen on select subset of commits
> - rebased entire series on top of tty-next tree
> - 2/14 ("use bit number macros"): explicitly include <linux/bits.h>
> - 3/14 ("remove unused uart_ops stubs"): don't add gratuitous comment
> removed later on in the series
> - 12/14 ("add IRQ support for the RX path"): add shadow irq register
> to support polling mode and avoid reading hardware mmio irq register
> to learn which irq flags are enabled
> - this also simplifies both liteuart_interrupt() and liteuart_startup()
> - 13/14 ("add IRQ support for the TX path"):
> - removed superfluous curly braces from liteuart_interrupt()
> - simplified [start|stop]_tx() by using shadow irq register from 12/14
> - simplified liteuart_tx_chars() by rebasing on top of tty-next tree
>
> Changes from v2:
> - further split out "separate RX loop from poll timer" into
> dedicated patches highlighting additional changes explicitly:
> - factoring out tty_flip_buffer_push() (6/14)
> - ack only RX events in RX loop (7/14)
> - pass constant flag to uart_insert_char() directly (8/14)
> - fix variable types in rx loop (9/14)
> - separating RX loop from poll timer (10/14)
> - added patch (11/14) to move function definitions to a more
> convenient location, making subsequent changes easier to read
> in diff patch form.
> - fixes and clarifications for RX path IRQ support patch (now 12/14):
> - only return IRQ_HANDLED for RX events
> - use pr_fmt() to improve style of irq setup error message
> - remove unnecessary locking from around single-register access
> - modify TX path to support both IRQ-mode and polling (13/14)
> - move polling-only liteuart_putchar() behind same conditional
> (CONFIG_SERIAL_LITEUART_CONSOLE) as the rest of the code that's
> still using it.
>
> Changes from v1:
> - split minor cosmetic changes out into individual patches
> (1/3 became 1..5/7)
> - patches 6/7 and 7/7 unchanged (used to be 2/3 and 3/3)
Gabriel Somlo (14):
serial: liteuart: use KBUILD_MODNAME as driver name
serial: liteuart: use bit number macros
serial: liteuart: remove unused uart_ops stubs
serial: liteuart: don't set unused port fields
serial: liteuart: minor style fix in liteuart_init()
serial: liteuart: move tty_flip_buffer_push() out of rx loop
serial: liteuart: rx loop should only ack rx events
serial: liteuart: simplify passing of uart_insert_char() flag
serial: liteuart: fix rx loop variable types
serial: liteuart: separate rx loop from poll timer
serial: liteuart: move function definitions
serial: liteuart: add IRQ support for the RX path
serial: liteuart: add IRQ support for the TX path
serial: liteuart: move polling putchar() function
drivers/tty/serial/liteuart.c | 210 ++++++++++++++++++++--------------
1 file changed, 126 insertions(+), 84 deletions(-)
--
2.38.1
Replace hard-coded instances of "liteuart" with KBUILD_MODNAME.
Signed-off-by: Gabriel Somlo <[email protected]>
---
drivers/tty/serial/liteuart.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 062812fe1b09..db898751ffe3 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -57,7 +57,7 @@ static struct console liteuart_console;
static struct uart_driver liteuart_driver = {
.owner = THIS_MODULE,
- .driver_name = "liteuart",
+ .driver_name = KBUILD_MODNAME,
.dev_name = "ttyLXU",
.major = 0,
.minor = 0,
@@ -321,7 +321,7 @@ static struct platform_driver liteuart_platform_driver = {
.probe = liteuart_probe,
.remove = liteuart_remove,
.driver = {
- .name = "liteuart",
+ .name = KBUILD_MODNAME,
.of_match_table = liteuart_of_match,
},
};
@@ -367,7 +367,7 @@ static int liteuart_console_setup(struct console *co, char *options)
}
static struct console liteuart_console = {
- .name = "liteuart",
+ .name = KBUILD_MODNAME,
.write = liteuart_console_write,
.device = uart_console_device,
.setup = liteuart_console_setup,
--
2.38.1
Add support for IRQ-driven RX. Support for the TX path will be added
in a separate commit.
Signed-off-by: Gabriel Somlo <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
Changes from v4:
- using dev_err() instead of a combo of pr_err() and pr_fmt()
- dropped "get irq" comment in probe()
> Changes from v3:
> - add shadow irq register to support polling mode and avoid reading
> hardware mmio irq register to learn which irq flags are enabled
> - this also simplifies both liteuart_interrupt() and liteuart_startup()
drivers/tty/serial/liteuart.c | 76 +++++++++++++++++++++++++++++++----
1 file changed, 69 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 8a6e176be08e..678c37c952cf 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -7,6 +7,7 @@
#include <linux/bits.h>
#include <linux/console.h>
+#include <linux/interrupt.h>
#include <linux/litex.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -46,6 +47,7 @@ struct liteuart_port {
struct uart_port port;
struct timer_list timer;
u32 id;
+ u8 irq_reg;
};
#define to_liteuart_port(port) container_of(port, struct liteuart_port, port)
@@ -76,6 +78,19 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch)
litex_write8(port->membase + OFF_RXTX, ch);
}
+static void liteuart_update_irq_reg(struct uart_port *port, bool set, u8 mask)
+{
+ struct liteuart_port *uart = to_liteuart_port(port);
+
+ if (set)
+ uart->irq_reg |= mask;
+ else
+ uart->irq_reg &= ~mask;
+
+ if (port->irq)
+ litex_write8(port->membase + OFF_EV_ENABLE, uart->irq_reg);
+}
+
static void liteuart_stop_tx(struct uart_port *port)
{
}
@@ -129,13 +144,27 @@ static void liteuart_rx_chars(struct uart_port *port)
tty_flip_buffer_push(&port->state->port);
}
+static irqreturn_t liteuart_interrupt(int irq, void *data)
+{
+ struct liteuart_port *uart = data;
+ struct uart_port *port = &uart->port;
+ u8 isr;
+
+ spin_lock(&port->lock);
+ isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
+ if (isr & EV_RX)
+ liteuart_rx_chars(port);
+ spin_unlock(&port->lock);
+
+ return IRQ_RETVAL(isr);
+}
+
static void liteuart_timer(struct timer_list *t)
{
struct liteuart_port *uart = from_timer(uart, t, timer);
struct uart_port *port = &uart->port;
- liteuart_rx_chars(port);
-
+ liteuart_interrupt(0, port);
mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
}
@@ -161,19 +190,46 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port)
static int liteuart_startup(struct uart_port *port)
{
struct liteuart_port *uart = to_liteuart_port(port);
+ unsigned long flags;
+ int ret;
- /* disable events */
- litex_write8(port->membase + OFF_EV_ENABLE, 0);
+ if (port->irq) {
+ ret = request_irq(port->irq, liteuart_interrupt, 0,
+ KBUILD_MODNAME, uart);
+ if (ret) {
+ dev_err(port->dev,
+ "line %d irq %d failed: switch to polling\n",
+ port->line, port->irq);
+ port->irq = 0;
+ }
+ }
- /* prepare timer for polling */
- timer_setup(&uart->timer, liteuart_timer, 0);
- mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
+ spin_lock_irqsave(&port->lock, flags);
+ /* only enabling rx irqs during startup */
+ liteuart_update_irq_reg(port, true, EV_RX);
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ if (!port->irq) {
+ timer_setup(&uart->timer, liteuart_timer, 0);
+ mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
+ }
return 0;
}
static void liteuart_shutdown(struct uart_port *port)
{
+ struct liteuart_port *uart = to_liteuart_port(port);
+ unsigned long flags;
+
+ spin_lock_irqsave(&port->lock, flags);
+ liteuart_update_irq_reg(port, false, EV_RX | EV_TX);
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ if (port->irq)
+ free_irq(port->irq, port);
+ else
+ del_timer_sync(&uart->timer);
}
static void liteuart_set_termios(struct uart_port *port, struct ktermios *new,
@@ -262,6 +318,12 @@ static int liteuart_probe(struct platform_device *pdev)
goto err_erase_id;
}
+ ret = platform_get_irq_optional(pdev, 0);
+ if (ret < 0 && ret != -ENXIO)
+ return ret;
+ if (ret > 0)
+ port->irq = ret;
+
/* values not from device tree */
port->dev = &pdev->dev;
port->iotype = UPIO_MEM;
--
2.38.1
Convert the rx loop into its own dedicated function, and (for now)
call it from the poll timer. This is in preparation for adding irq
support to the receive path.
Signed-off-by: Gabriel Somlo <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/liteuart.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 42ac9aee050a..76f8a09b82cd 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -68,10 +68,8 @@ static struct uart_driver liteuart_driver = {
#endif
};
-static void liteuart_timer(struct timer_list *t)
+static void liteuart_rx_chars(struct uart_port *port)
{
- struct liteuart_port *uart = from_timer(uart, t, timer);
- struct uart_port *port = &uart->port;
unsigned char __iomem *membase = port->membase;
unsigned int status, ch;
@@ -88,6 +86,14 @@ static void liteuart_timer(struct timer_list *t)
}
tty_flip_buffer_push(&port->state->port);
+}
+
+static void liteuart_timer(struct timer_list *t)
+{
+ struct liteuart_port *uart = from_timer(uart, t, timer);
+ struct uart_port *port = &uart->port;
+
+ liteuart_rx_chars(port);
mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
}
--
2.38.1
Switch the TX path to IRQ-driven operation, while maintaining support
for polling mode via the poll timer.
Signed-off-by: Gabriel Somlo <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/liteuart.c | 30 +++++++++++++-----------------
1 file changed, 13 insertions(+), 17 deletions(-)
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 678c37c952cf..850125870efb 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -93,27 +93,12 @@ static void liteuart_update_irq_reg(struct uart_port *port, bool set, u8 mask)
static void liteuart_stop_tx(struct uart_port *port)
{
+ liteuart_update_irq_reg(port, false, EV_TX);
}
static void liteuart_start_tx(struct uart_port *port)
{
- struct circ_buf *xmit = &port->state->xmit;
- unsigned char ch;
-
- if (unlikely(port->x_char)) {
- litex_write8(port->membase + OFF_RXTX, port->x_char);
- port->icount.tx++;
- port->x_char = 0;
- } else if (!uart_circ_empty(xmit)) {
- while (xmit->head != xmit->tail) {
- ch = xmit->buf[xmit->tail];
- uart_xmit_advance(port, 1);
- liteuart_putchar(port, ch);
- }
- }
-
- if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
- uart_write_wakeup(port);
+ liteuart_update_irq_reg(port, true, EV_TX);
}
static void liteuart_stop_rx(struct uart_port *port)
@@ -144,6 +129,15 @@ static void liteuart_rx_chars(struct uart_port *port)
tty_flip_buffer_push(&port->state->port);
}
+static void liteuart_tx_chars(struct uart_port *port)
+{
+ u8 ch;
+
+ uart_port_tx(port, ch,
+ !litex_read8(port->membase + OFF_TXFULL),
+ litex_write8(port->membase + OFF_RXTX, ch));
+}
+
static irqreturn_t liteuart_interrupt(int irq, void *data)
{
struct liteuart_port *uart = data;
@@ -154,6 +148,8 @@ static irqreturn_t liteuart_interrupt(int irq, void *data)
isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
if (isr & EV_RX)
liteuart_rx_chars(port);
+ if (isr & EV_TX)
+ liteuart_tx_chars(port);
spin_unlock(&port->lock);
return IRQ_RETVAL(isr);
--
2.38.1
While receiving characters, it is necessary to acknowledge each one
by writing to the EV_PENDING register's EV_RX bit. Ensure we do not
also gratuitously set the EV_TX bit in the process.
Signed-off-by: Gabriel Somlo <[email protected]>
---
drivers/tty/serial/liteuart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 81a86c5eb393..c90ab65fbdcf 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -82,7 +82,7 @@ static void liteuart_timer(struct timer_list *t)
port->icount.rx++;
/* necessary for RXEMPTY to refresh its value */
- litex_write8(membase + OFF_EV_PENDING, EV_TX | EV_RX);
+ litex_write8(membase + OFF_EV_PENDING, EV_RX);
/* no overflow bits in status */
if (!(uart_handle_sysrq_char(port, ch)))
--
2.38.1
Signed-off-by: Gabriel Somlo <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/liteuart.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index c6eb7eba5af8..1e3429bcc2ad 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -397,12 +397,10 @@ static int __init liteuart_init(void)
return res;
res = platform_driver_register(&liteuart_platform_driver);
- if (res) {
+ if (res)
uart_unregister_driver(&liteuart_driver);
- return res;
- }
- return 0;
+ return res;
}
static void __exit liteuart_exit(void)
--
2.38.1
Replace magic bit constants (e.g., 1, 2, 4) with BIT(x) expressions.
Signed-off-by: Gabriel Somlo <[email protected]>
---
drivers/tty/serial/liteuart.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index db898751ffe3..18c1eb315ee9 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -5,6 +5,7 @@
* Copyright (C) 2019-2020 Antmicro <http://www.antmicro.com>
*/
+#include <linux/bits.h>
#include <linux/console.h>
#include <linux/litex.h>
#include <linux/module.h>
@@ -38,8 +39,8 @@
#define OFF_EV_ENABLE 0x14
/* events */
-#define EV_TX 0x1
-#define EV_RX 0x2
+#define EV_TX BIT(0)
+#define EV_RX BIT(1)
struct liteuart_port {
struct uart_port port;
--
2.38.1
Calling tty_flip_buffer_push() for each individual received character
is overkill. Move it out of the rx loop, and only call it once per
set of characters received together.
Signed-off-by: Gabriel Somlo <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/liteuart.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 1e3429bcc2ad..81a86c5eb393 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -87,10 +87,10 @@ static void liteuart_timer(struct timer_list *t)
/* no overflow bits in status */
if (!(uart_handle_sysrq_char(port, ch)))
uart_insert_char(port, status, 0, ch, flg);
-
- tty_flip_buffer_push(&port->state->port);
}
+ tty_flip_buffer_push(&port->state->port);
+
mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
}
--
2.38.1
Update variable types to match the signature of uart_insert_char()
which consumes them.
Signed-off-by: Gabriel Somlo <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/liteuart.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index 81aa7c1da73c..42ac9aee050a 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -73,8 +73,7 @@ static void liteuart_timer(struct timer_list *t)
struct liteuart_port *uart = from_timer(uart, t, timer);
struct uart_port *port = &uart->port;
unsigned char __iomem *membase = port->membase;
- int ch;
- unsigned long status;
+ unsigned int status, ch;
while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
ch = litex_read8(membase + OFF_RXTX);
--
2.38.1
On 18. 11. 22, 15:55, Gabriel Somlo wrote:
> Update variable types to match the signature of uart_insert_char()
> which consumes them.
>
> Signed-off-by: Gabriel Somlo <[email protected]>
> Reviewed-by: Ilpo Järvinen <[email protected]>
> ---
> drivers/tty/serial/liteuart.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 81aa7c1da73c..42ac9aee050a 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -73,8 +73,7 @@ static void liteuart_timer(struct timer_list *t)
> struct liteuart_port *uart = from_timer(uart, t, timer);
> struct uart_port *port = &uart->port;
> unsigned char __iomem *membase = port->membase;
> - int ch;
> - unsigned long status;
> + unsigned int status, ch;
These should be u8 after all, right?
Wait, status is a bool in the end:
> while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
But why is it passed to uart_insert_char() as such? That's ugly. Maybe
drop all of this "status" and pass LSR_RXC directly. The while's
condition would simply look like (!litex_read8(membase + OFF_RXEMPTY)) then.
> ch = litex_read8(membase + OFF_RXTX);
thanks,
--
js
suse labs
On 21. 11. 22, 9:37, Jiri Slaby wrote:
> On 18. 11. 22, 15:55, Gabriel Somlo wrote:
>> Update variable types to match the signature of uart_insert_char()
>> which consumes them.
>>
>> Signed-off-by: Gabriel Somlo <[email protected]>
>> Reviewed-by: Ilpo Järvinen <[email protected]>
>> ---
>> drivers/tty/serial/liteuart.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/liteuart.c
>> b/drivers/tty/serial/liteuart.c
>> index 81aa7c1da73c..42ac9aee050a 100644
>> --- a/drivers/tty/serial/liteuart.c
>> +++ b/drivers/tty/serial/liteuart.c
>> @@ -73,8 +73,7 @@ static void liteuart_timer(struct timer_list *t)
>> struct liteuart_port *uart = from_timer(uart, t, timer);
>> struct uart_port *port = &uart->port;
>> unsigned char __iomem *membase = port->membase;
>> - int ch;
>> - unsigned long status;
>> + unsigned int status, ch;
>
> These should be u8 after all, right?
>
> Wait, status is a bool in the end:
>
>> while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
>
> But why is it passed to uart_insert_char() as such? That's ugly. Maybe
> drop all of this "status" and pass LSR_RXC directly.
Actually, even 0 directly, provided overrun parameter is always 0.
> The while's
> condition would simply look like (!litex_read8(membase + OFF_RXEMPTY))
> then.
>
>> ch = litex_read8(membase + OFF_RXTX);
>
> thanks,
--
js
On 21. 11. 22, 9:37, Jiri Slaby wrote:
> On 18. 11. 22, 15:55, Gabriel Somlo wrote:
>> Update variable types to match the signature of uart_insert_char()
>> which consumes them.
>>
>> Signed-off-by: Gabriel Somlo <[email protected]>
>> Reviewed-by: Ilpo Järvinen <[email protected]>
>> ---
>> drivers/tty/serial/liteuart.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/liteuart.c
>> b/drivers/tty/serial/liteuart.c
>> index 81aa7c1da73c..42ac9aee050a 100644
>> --- a/drivers/tty/serial/liteuart.c
>> +++ b/drivers/tty/serial/liteuart.c
>> @@ -73,8 +73,7 @@ static void liteuart_timer(struct timer_list *t)
>> struct liteuart_port *uart = from_timer(uart, t, timer);
>> struct uart_port *port = &uart->port;
>> unsigned char __iomem *membase = port->membase;
>> - int ch;
>> - unsigned long status;
>> + unsigned int status, ch;
>
> These should be u8 after all, right?
And can you change membase to u8 * too 8-)?
--
js
suse labs
On 18. 11. 22, 15:55, Gabriel Somlo wrote:
> Switch the TX path to IRQ-driven operation, while maintaining support
> for polling mode via the poll timer.
>
> Signed-off-by: Gabriel Somlo <[email protected]>
> Reviewed-by: Ilpo Järvinen <[email protected]>
...
> @@ -154,6 +148,8 @@ static irqreturn_t liteuart_interrupt(int irq, void *data)
> isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
> if (isr & EV_RX)
> liteuart_rx_chars(port);
> + if (isr & EV_TX)
> + liteuart_tx_chars(port);
Wait, how do you ensure the OFF_EV_PENDING reg contains EV_RX and/or
EV_TX in the polling mode?
thanks,
--
js
suse labs
On 18. 11. 22, 15:55, Gabriel Somlo wrote:
> Add support for IRQ-driven RX. Support for the TX path will be added
> in a separate commit.
>
> Signed-off-by: Gabriel Somlo <[email protected]>
> Reviewed-by: Ilpo Järvinen <[email protected]>
> ---
>
> Changes from v4:
> - using dev_err() instead of a combo of pr_err() and pr_fmt()
> - dropped "get irq" comment in probe()
>
>> Changes from v3:
>> - add shadow irq register to support polling mode and avoid reading
>> hardware mmio irq register to learn which irq flags are enabled
>> - this also simplifies both liteuart_interrupt() and liteuart_startup()
>
> drivers/tty/serial/liteuart.c | 76 +++++++++++++++++++++++++++++++----
> 1 file changed, 69 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index 8a6e176be08e..678c37c952cf 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -7,6 +7,7 @@
>
> #include <linux/bits.h>
> #include <linux/console.h>
> +#include <linux/interrupt.h>
> #include <linux/litex.h>
> #include <linux/module.h>
> #include <linux/of.h>
> @@ -46,6 +47,7 @@ struct liteuart_port {
> struct uart_port port;
> struct timer_list timer;
> u32 id;
> + u8 irq_reg;
> };
>
> #define to_liteuart_port(port) container_of(port, struct liteuart_port, port)
> @@ -76,6 +78,19 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch)
> litex_write8(port->membase + OFF_RXTX, ch);
> }
>
> +static void liteuart_update_irq_reg(struct uart_port *port, bool set, u8 mask)
> +{
> + struct liteuart_port *uart = to_liteuart_port(port);
> +
> + if (set)
> + uart->irq_reg |= mask;
> + else
> + uart->irq_reg &= ~mask;
> +
> + if (port->irq)
> + litex_write8(port->membase + OFF_EV_ENABLE, uart->irq_reg);
> +}
> +
> static void liteuart_stop_tx(struct uart_port *port)
> {
> }
> @@ -129,13 +144,27 @@ static void liteuart_rx_chars(struct uart_port *port)
> tty_flip_buffer_push(&port->state->port);
> }
>
> +static irqreturn_t liteuart_interrupt(int irq, void *data)
> +{
> + struct liteuart_port *uart = data;
> + struct uart_port *port = &uart->port;
> + u8 isr;
> +
> + spin_lock(&port->lock);
> + isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
> + if (isr & EV_RX)
> + liteuart_rx_chars(port);
> + spin_unlock(&port->lock);
> +
> + return IRQ_RETVAL(isr);
> +}
> +
> static void liteuart_timer(struct timer_list *t)
> {
> struct liteuart_port *uart = from_timer(uart, t, timer);
> struct uart_port *port = &uart->port;
>
> - liteuart_rx_chars(port);
> -
> + liteuart_interrupt(0, port);
Are you sure spin_lock() is safe from this path? I assume so, but have
you thought about it?
> mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> }
>
> @@ -161,19 +190,46 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port)
> static int liteuart_startup(struct uart_port *port)
> {
> struct liteuart_port *uart = to_liteuart_port(port);
> + unsigned long flags;
> + int ret;
>
> - /* disable events */
> - litex_write8(port->membase + OFF_EV_ENABLE, 0);
> + if (port->irq) {
> + ret = request_irq(port->irq, liteuart_interrupt, 0,
> + KBUILD_MODNAME, uart);
Just asking: cannot the irq be shared?
> + if (ret) {
> + dev_err(port->dev,
> + "line %d irq %d failed: switch to polling\n",
> + port->line, port->irq);
That is, it should be only dev_warn(), or?
> + port->irq = 0;
> + }
> + }
thanks,
--
js
suse labs
On Fri, Nov 18, 2022 at 3:56 PM Gabriel Somlo <[email protected]> wrote:
> Replace hard-coded instances of "liteuart" with KBUILD_MODNAME.
>
> Signed-off-by: Gabriel Somlo <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Fri, Nov 18, 2022 at 3:55 PM Gabriel Somlo <[email protected]> wrote:
> Replace magic bit constants (e.g., 1, 2, 4) with BIT(x) expressions.
>
> Signed-off-by: Gabriel Somlo <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Fri, Nov 18, 2022 at 3:56 PM Gabriel Somlo <[email protected]> wrote:
> Calling tty_flip_buffer_push() for each individual received character
> is overkill. Move it out of the rx loop, and only call it once per
> set of characters received together.
>
> Signed-off-by: Gabriel Somlo <[email protected]>
> Reviewed-by: Ilpo Järvinen <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Fri, Nov 18, 2022 at 3:56 PM Gabriel Somlo <[email protected]> wrote:
> Signed-off-by: Gabriel Somlo <[email protected]>
> Reviewed-by: Ilpo Järvinen <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Gabriel,
On Fri, Nov 18, 2022 at 3:57 PM Gabriel Somlo <[email protected]> wrote:
> Convert the rx loop into its own dedicated function, and (for now)
> call it from the poll timer. This is in preparation for adding irq
> support to the receive path.
>
> Signed-off-by: Gabriel Somlo <[email protected]>
> Reviewed-by: Ilpo Järvinen <[email protected]>
Thanks for your patch!
Reviewed-by: Geert Uytterhoeven <[email protected]>
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -68,10 +68,8 @@ static struct uart_driver liteuart_driver = {
> #endif
> };
>
> -static void liteuart_timer(struct timer_list *t)
> +static void liteuart_rx_chars(struct uart_port *port)
So first you spin this off into a separate function, so it can be
called from both the interrupt and polling paths.
Later, in "[PATCH v5 12/14] serial: liteuart: add IRQ support for the
RX path", you remove the call from the polling path...
> {
> - struct liteuart_port *uart = from_timer(uart, t, timer);
> - struct uart_port *port = &uart->port;
> unsigned char __iomem *membase = port->membase;
> unsigned int status, ch;
>
> @@ -88,6 +86,14 @@ static void liteuart_timer(struct timer_list *t)
> }
>
> tty_flip_buffer_push(&port->state->port);
> +}
> +
> +static void liteuart_timer(struct timer_list *t)
> +{
> + struct liteuart_port *uart = from_timer(uart, t, timer);
> + struct uart_port *port = &uart->port;
> +
> + liteuart_rx_chars(port);
>
> mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Jiri,
Thanks for the feedback!
On Mon, Nov 21, 2022 at 09:45:05AM +0100, Jiri Slaby wrote:
> On 21. 11. 22, 9:37, Jiri Slaby wrote:
> > On 18. 11. 22, 15:55, Gabriel Somlo wrote:
> > > Update variable types to match the signature of uart_insert_char()
> > > which consumes them.
> > >
> > > Signed-off-by: Gabriel Somlo <[email protected]>
> > > Reviewed-by: Ilpo J?rvinen <[email protected]>
> > > ---
> > > ? drivers/tty/serial/liteuart.c | 3 +--
> > > ? 1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/liteuart.c
> > > b/drivers/tty/serial/liteuart.c
> > > index 81aa7c1da73c..42ac9aee050a 100644
> > > --- a/drivers/tty/serial/liteuart.c
> > > +++ b/drivers/tty/serial/liteuart.c
> > > @@ -73,8 +73,7 @@ static void liteuart_timer(struct timer_list *t)
> > > ????? struct liteuart_port *uart = from_timer(uart, t, timer);
> > > ????? struct uart_port *port = &uart->port;
> > > ????? unsigned char __iomem *membase = port->membase;
> > > -??? int ch;
> > > -??? unsigned long status;
> > > +??? unsigned int status, ch;
> >
> > These should be u8 after all, right?
OK, so:
- I can hard-code `status` as `1`, like so:
while(!litex_read8(membase + OFF_RXEMPTY) {
...
if (!(uart_handle_sysrq_char(port, ch)))
uart_insert_char(port, 1, 0, ch, TTY_NORMAL);
... since `status` would always be `1` inside the loop. So I'm
basically going to get rid of it altogether.
- `ch` is indeed *produced* by `litex_read8()`, which would make it
`u8`. It is subsequently *consumed* by `uart_handle_sysrq_char()`
and `uart_insert_char()`, which both expect `unsigned int`.
If you think it's better to go with the type when the value is
produced (as opposed to when it's consumed), I'm OK with that for
the upcoming v6 of the series... :)
> And can you change membase to u8 * too 8-)?
Hmmm, in `struct uart_port` (in include/linux/serial_core.h), the
`membase` field is declared as:
unsigned char __iomem *membase;
which is why I'm thinking we should leave it as-is? Unless there are
plans (or a pending patch I'm unaware of) to switch the field in
include/linux/serial_core.h to `u8` as well? -- Please advise.
Thanks again,
--Gabriel
> --
> js
> suse labs
>
On Mon, Nov 21, 2022 at 09:58:41AM +0100, Jiri Slaby wrote:
> On 18. 11. 22, 15:55, Gabriel Somlo wrote:
> > Switch the TX path to IRQ-driven operation, while maintaining support
> > for polling mode via the poll timer.
> >
> > Signed-off-by: Gabriel Somlo <[email protected]>
> > Reviewed-by: Ilpo J?rvinen <[email protected]>
> ...
> > @@ -154,6 +148,8 @@ static irqreturn_t liteuart_interrupt(int irq, void *data)
> > isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
> > if (isr & EV_RX)
> > liteuart_rx_chars(port);
> > + if (isr & EV_TX)
> > + liteuart_tx_chars(port);
>
> Wait, how do you ensure the OFF_EV_PENDING reg contains EV_RX and/or EV_TX
> in the polling mode?
The hardware (well, *gateware*) is coded to populate the EV_PENDING
register regardless of whether IRQs are enabled via the EV_ENABLE
register (or indeed, whether IRQs are even "wired into" the design at
all). See
https://github.com/enjoy-digital/litex/blob/master/litex/soc/cores/uart.py
and
https://github.com/enjoy-digital/litex/blob/master/litex/soc/interconnect/csr_eventmanager.py
for a starting point on why it's OK to just assume that...
Thanks,
--Gabriel
On Mon, Nov 21, 2022 at 11:16:14AM +0100, Geert Uytterhoeven wrote:
> Hi Gabriel,
>
> On Fri, Nov 18, 2022 at 3:57 PM Gabriel Somlo <[email protected]> wrote:
> > Convert the rx loop into its own dedicated function, and (for now)
> > call it from the poll timer. This is in preparation for adding irq
> > support to the receive path.
> >
> > Signed-off-by: Gabriel Somlo <[email protected]>
> > Reviewed-by: Ilpo J?rvinen <[email protected]>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
>
> > --- a/drivers/tty/serial/liteuart.c
> > +++ b/drivers/tty/serial/liteuart.c
> > @@ -68,10 +68,8 @@ static struct uart_driver liteuart_driver = {
> > #endif
> > };
> >
> > -static void liteuart_timer(struct timer_list *t)
> > +static void liteuart_rx_chars(struct uart_port *port)
>
> So first you spin this off into a separate function, so it can be
> called from both the interrupt and polling paths.
> Later, in "[PATCH v5 12/14] serial: liteuart: add IRQ support for the
> RX path", you remove the call from the polling path...
That's right -- the polling path now calls the IRQ handler,
which, in turn, calls `liteuart_rx_chars()` (and later `*_tx_chars()`
as well).
The polling timer becomes just an alternative way to invoke the irq
handler, giving it a chance to take care of any pending transfers in
either/both directions.
Thanks,
--Gabriel
> > {
> > - struct liteuart_port *uart = from_timer(uart, t, timer);
> > - struct uart_port *port = &uart->port;
> > unsigned char __iomem *membase = port->membase;
> > unsigned int status, ch;
> >
> > @@ -88,6 +86,14 @@ static void liteuart_timer(struct timer_list *t)
> > }
> >
> > tty_flip_buffer_push(&port->state->port);
> > +}
> > +
> > +static void liteuart_timer(struct timer_list *t)
> > +{
> > + struct liteuart_port *uart = from_timer(uart, t, timer);
> > + struct uart_port *port = &uart->port;
> > +
> > + liteuart_rx_chars(port);
> >
> > mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > }
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
Hi Jiri,
On Mon, Nov 21, 2022 at 09:54:34AM +0100, Jiri Slaby wrote:
> On 18. 11. 22, 15:55, Gabriel Somlo wrote:
> > Add support for IRQ-driven RX. Support for the TX path will be added
> > in a separate commit.
> >
> > Signed-off-by: Gabriel Somlo <[email protected]>
> > Reviewed-by: Ilpo J?rvinen <[email protected]>
> > ---
> >
> > Changes from v4:
> > - using dev_err() instead of a combo of pr_err() and pr_fmt()
> > - dropped "get irq" comment in probe()
> >
> > > Changes from v3:
> > > - add shadow irq register to support polling mode and avoid reading
> > > hardware mmio irq register to learn which irq flags are enabled
> > > - this also simplifies both liteuart_interrupt() and liteuart_startup()
> >
> > drivers/tty/serial/liteuart.c | 76 +++++++++++++++++++++++++++++++----
> > 1 file changed, 69 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > index 8a6e176be08e..678c37c952cf 100644
> > --- a/drivers/tty/serial/liteuart.c
> > +++ b/drivers/tty/serial/liteuart.c
> > @@ -7,6 +7,7 @@
> > #include <linux/bits.h>
> > #include <linux/console.h>
> > +#include <linux/interrupt.h>
> > #include <linux/litex.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > @@ -46,6 +47,7 @@ struct liteuart_port {
> > struct uart_port port;
> > struct timer_list timer;
> > u32 id;
> > + u8 irq_reg;
> > };
> > #define to_liteuart_port(port) container_of(port, struct liteuart_port, port)
> > @@ -76,6 +78,19 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch)
> > litex_write8(port->membase + OFF_RXTX, ch);
> > }
> > +static void liteuart_update_irq_reg(struct uart_port *port, bool set, u8 mask)
> > +{
> > + struct liteuart_port *uart = to_liteuart_port(port);
> > +
> > + if (set)
> > + uart->irq_reg |= mask;
> > + else
> > + uart->irq_reg &= ~mask;
> > +
> > + if (port->irq)
> > + litex_write8(port->membase + OFF_EV_ENABLE, uart->irq_reg);
> > +}
> > +
> > static void liteuart_stop_tx(struct uart_port *port)
> > {
> > }
> > @@ -129,13 +144,27 @@ static void liteuart_rx_chars(struct uart_port *port)
> > tty_flip_buffer_push(&port->state->port);
> > }
> > +static irqreturn_t liteuart_interrupt(int irq, void *data)
> > +{
> > + struct liteuart_port *uart = data;
> > + struct uart_port *port = &uart->port;
> > + u8 isr;
> > +
> > + spin_lock(&port->lock);
> > + isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg;
> > + if (isr & EV_RX)
> > + liteuart_rx_chars(port);
> > + spin_unlock(&port->lock);
> > +
> > + return IRQ_RETVAL(isr);
> > +}
> > +
> > static void liteuart_timer(struct timer_list *t)
> > {
> > struct liteuart_port *uart = from_timer(uart, t, timer);
> > struct uart_port *port = &uart->port;
> > - liteuart_rx_chars(port);
> > -
> > + liteuart_interrupt(0, port);
>
> Are you sure spin_lock() is safe from this path? I assume so, but have you
> thought about it?
I checked and at that point `in_serving_softirq()` is true.
*However*, after studying spin_lock() and friends for a while, I'm
not quite clear on whether that unequivocally translates
to "yes, we're safe" :)
As such, I'm inclined to switch to `spin_lock_irqsave()` and
`spin_unlock_irqrestore()` even in the interrupt handler, which is
explicitly stated to be "safe from any context":
https://www.kernel.org/doc/html/v4.15/kernel-hacking/locking.html#cheat-sheet-for-locking
The alternative could be to set `TIMER_IRQSAFE` in `timer_setup()`,
but no other tty driver seems to be doing that, so I'd be a bit off
the beaten path there... :)
Please do let me know what you think about this, particularly if you
consider going the spin_lock_irqsave-everywhere-just-to-be-safe route
overkill... :)
> > mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > }
> > @@ -161,19 +190,46 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port)
> > static int liteuart_startup(struct uart_port *port)
> > {
> > struct liteuart_port *uart = to_liteuart_port(port);
> > + unsigned long flags;
> > + int ret;
> > - /* disable events */
> > - litex_write8(port->membase + OFF_EV_ENABLE, 0);
> > + if (port->irq) {
> > + ret = request_irq(port->irq, liteuart_interrupt, 0,
> > + KBUILD_MODNAME, uart);
>
> Just asking: cannot the irq be shared?
Given the way LiteX gateware is currently generated, each
irq-triggering device is given its own separate line. I don't think
setting the IRQF_SHARED flag actually *hurts* anything (no difference
in behavior while testing), but I don't think it's needed ATM.
> > + if (ret) {
> > + dev_err(port->dev,
> > + "line %d irq %d failed: switch to polling\n",
> > + port->line, port->irq);
>
> That is, it should be only dev_warn(), or?
Makes sense, will use dev_warn() in v6.
Please LMK what you think about spin_lock[_irqsave] (and IRQF_SHARED),
and I'll send out v6 with all the necessary chances right after that.
Thanks much,
--Gabriel
> > + port->irq = 0;
> > + }
> > + }
>
> thanks,
> --
> js
> suse labs
>
On 21. 11. 22, 14:55, Gabriel L. Somlo wrote:
> Hi Jiri,
>
> Thanks for the feedback!
>
> On Mon, Nov 21, 2022 at 09:45:05AM +0100, Jiri Slaby wrote:
>> On 21. 11. 22, 9:37, Jiri Slaby wrote:
>>> On 18. 11. 22, 15:55, Gabriel Somlo wrote:
>>>> Update variable types to match the signature of uart_insert_char()
>>>> which consumes them.
>>>>
>>>> Signed-off-by: Gabriel Somlo <[email protected]>
>>>> Reviewed-by: Ilpo Järvinen <[email protected]>
>>>> ---
>>>> drivers/tty/serial/liteuart.c | 3 +--
>>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/liteuart.c
>>>> b/drivers/tty/serial/liteuart.c
>>>> index 81aa7c1da73c..42ac9aee050a 100644
>>>> --- a/drivers/tty/serial/liteuart.c
>>>> +++ b/drivers/tty/serial/liteuart.c
>>>> @@ -73,8 +73,7 @@ static void liteuart_timer(struct timer_list *t)
>>>> struct liteuart_port *uart = from_timer(uart, t, timer);
>>>> struct uart_port *port = &uart->port;
>>>> unsigned char __iomem *membase = port->membase;
>>>> - int ch;
>>>> - unsigned long status;
>>>> + unsigned int status, ch;
>>>
>>> These should be u8 after all, right?
>
> OK, so:
>
> - I can hard-code `status` as `1`, like so:
>
> while(!litex_read8(membase + OFF_RXEMPTY) {
> ...
> if (!(uart_handle_sysrq_char(port, ch)))
> uart_insert_char(port, 1, 0, ch, TTY_NORMAL);
>
> ... since `status` would always be `1` inside the loop. So I'm
> basically going to get rid of it altogether.
Yes, I had that in my mind. Except passing 1 to uart_insert_char() when
overflow is hardwired to 0 makes no sense IMO :).
> - `ch` is indeed *produced* by `litex_read8()`, which would make it
> `u8`. It is subsequently *consumed* by `uart_handle_sysrq_char()`
> and `uart_insert_char()`, which both expect `unsigned int`.
Ignore uart_handle_sysrq_char and uart_insert_char. They should be fixed
one day. It should really be u8. All down the call chain (it magically
turns into int in the sysrq handlers, then char is expected).
> If you think it's better to go with the type when the value is
> produced (as opposed to when it's consumed), I'm OK with that for
> the upcoming v6 of the series... :)
Yes, please. We should slowly convert _all_ of them.
>> And can you change membase to u8 * too 8-)?
>
> Hmmm, in `struct uart_port` (in include/linux/serial_core.h), the
> `membase` field is declared as:
>
> unsigned char __iomem *membase;
>
> which is why I'm thinking we should leave it as-is? Unless there are
> plans (or a pending patch I'm unaware of) to switch the field in
> include/linux/serial_core.h to `u8` as well? -- Please advise.
Ah, then keep it. I somehow thought it's void *. And yes, even this
should be u8 __iomem *, eventually.
thanks,
--
js
suse labs
On 21. 11. 22, 19:50, Gabriel L. Somlo wrote:
>>> static void liteuart_timer(struct timer_list *t)
>>> {
>>> struct liteuart_port *uart = from_timer(uart, t, timer);
>>> struct uart_port *port = &uart->port;
>>> - liteuart_rx_chars(port);
>>> -
>>> + liteuart_interrupt(0, port);
>>
>> Are you sure spin_lock() is safe from this path? I assume so, but have you
>> thought about it?
>
> I checked and at that point `in_serving_softirq()` is true.
>
> *However*, after studying spin_lock() and friends for a while, I'm
> not quite clear on whether that unequivocally translates
> to "yes, we're safe" :)
Depends whether some hard irq context is grabbing the port lock too. If
it does, it will spin forever waiting for this soft irq to finish (never
happens as it was interrupted).
> As such, I'm inclined to switch to `spin_lock_irqsave()` and
> `spin_unlock_irqrestore()` even in the interrupt handler, which is
> explicitly stated to be "safe from any context":
> https://www.kernel.org/doc/html/v4.15/kernel-hacking/locking.html#cheat-sheet-for-locking
> The alternative could be to set `TIMER_IRQSAFE` in `timer_setup()`,
> but no other tty driver seems to be doing that, so I'd be a bit off
> the beaten path there... :)
Ah, no.
> Please do let me know what you think about this, particularly if you
> consider going the spin_lock_irqsave-everywhere-just-to-be-safe route
> overkill... :)
If you are unsure about the other contexts, irqsave/restore is the way
to go. It can be lifted later, if someone investigates harder.
thanks,
--
js
On Tue, Nov 22, 2022 at 8:44 AM Jiri Slaby <[email protected]> wrote:
> On 21. 11. 22, 19:50, Gabriel L. Somlo wrote:
> >>> static void liteuart_timer(struct timer_list *t)
> >>> {
> >>> struct liteuart_port *uart = from_timer(uart, t, timer);
> >>> struct uart_port *port = &uart->port;
> >>> - liteuart_rx_chars(port);
> >>> -
> >>> + liteuart_interrupt(0, port);
> >>
> >> Are you sure spin_lock() is safe from this path? I assume so, but have you
> >> thought about it?
> >
> > I checked and at that point `in_serving_softirq()` is true.
> >
> > *However*, after studying spin_lock() and friends for a while, I'm
> > not quite clear on whether that unequivocally translates
> > to "yes, we're safe" :)
>
> Depends whether some hard irq context is grabbing the port lock too. If
> it does, it will spin forever waiting for this soft irq to finish (never
> happens as it was interrupted).
>
> > As such, I'm inclined to switch to `spin_lock_irqsave()` and
> > `spin_unlock_irqrestore()` even in the interrupt handler, which is
> > explicitly stated to be "safe from any context":
> > https://www.kernel.org/doc/html/v4.15/kernel-hacking/locking.html#cheat-sheet-for-locking
>
>
>
> > The alternative could be to set `TIMER_IRQSAFE` in `timer_setup()`,
> > but no other tty driver seems to be doing that, so I'd be a bit off
> > the beaten path there... :)
>
> Ah, no.
>
> > Please do let me know what you think about this, particularly if you
> > consider going the spin_lock_irqsave-everywhere-just-to-be-safe route
> > overkill... :)
>
> If you are unsure about the other contexts, irqsave/restore is the way
> to go. It can be lifted later, if someone investigates harder.
Inside the interrupt handler, plain spin_lock() should be OK.
Inside the timer handler, I think you need to use spin_lock_irqsave(),
as e.g. liteuart_set_termios() (and lots of code in serial_core.c)
already uses spin_lock_irqsave().
Besides, on non-SMP, spin_lock() doesn't do anything, so you need
to rely on disabling the local interrupt.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, Nov 22, 2022 at 10:54:45AM +0100, Geert Uytterhoeven wrote:
> On Tue, Nov 22, 2022 at 8:44 AM Jiri Slaby <[email protected]> wrote:
> > On 21. 11. 22, 19:50, Gabriel L. Somlo wrote:
> > >>> static void liteuart_timer(struct timer_list *t)
> > >>> {
> > >>> struct liteuart_port *uart = from_timer(uart, t, timer);
> > >>> struct uart_port *port = &uart->port;
> > >>> - liteuart_rx_chars(port);
> > >>> -
> > >>> + liteuart_interrupt(0, port);
> > >>
> > >> Are you sure spin_lock() is safe from this path? I assume so, but have you
> > >> thought about it?
> > >
> > > I checked and at that point `in_serving_softirq()` is true.
> > >
> > > *However*, after studying spin_lock() and friends for a while, I'm
> > > not quite clear on whether that unequivocally translates
> > > to "yes, we're safe" :)
> >
> > Depends whether some hard irq context is grabbing the port lock too. If
> > it does, it will spin forever waiting for this soft irq to finish (never
> > happens as it was interrupted).
> >
> > > As such, I'm inclined to switch to `spin_lock_irqsave()` and
> > > `spin_unlock_irqrestore()` even in the interrupt handler, which is
> > > explicitly stated to be "safe from any context":
> > > https://www.kernel.org/doc/html/v4.15/kernel-hacking/locking.html#cheat-sheet-for-locking
> >
> >
> >
> > > The alternative could be to set `TIMER_IRQSAFE` in `timer_setup()`,
> > > but no other tty driver seems to be doing that, so I'd be a bit off
> > > the beaten path there... :)
> >
> > Ah, no.
> >
> > > Please do let me know what you think about this, particularly if you
> > > consider going the spin_lock_irqsave-everywhere-just-to-be-safe route
> > > overkill... :)
> >
> > If you are unsure about the other contexts, irqsave/restore is the way
> > to go. It can be lifted later, if someone investigates harder.
>
> Inside the interrupt handler, plain spin_lock() should be OK.
> Inside the timer handler, I think you need to use spin_lock_irqsave(),
> as e.g. liteuart_set_termios() (and lots of code in serial_core.c)
> already uses spin_lock_irqsave().
> Besides, on non-SMP, spin_lock() doesn't do anything, so you need
> to rely on disabling the local interrupt.
Thanks Geert for the clarification! I could write two wrappers around
the actual code doing the interrupt handler work, one with spin_lock()
for the actual irq handler and another with spin_lock_irqsave() for
the timer codepath. But to keep things simple I'm inclined to simply
use spin_lock_irqsave() and add a comment saying it's because we need
it when polling and it's not actually harmful when using IRQ.
Thanks,
--Gabriel
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
On Tue, Nov 22, 2022 at 08:37:58AM +0100, Jiri Slaby wrote:
> On 21. 11. 22, 14:55, Gabriel L. Somlo wrote:
> > Hi Jiri,
> >
> > Thanks for the feedback!
> >
> > On Mon, Nov 21, 2022 at 09:45:05AM +0100, Jiri Slaby wrote:
> > > On 21. 11. 22, 9:37, Jiri Slaby wrote:
> > > > On 18. 11. 22, 15:55, Gabriel Somlo wrote:
> > > > > Update variable types to match the signature of uart_insert_char()
> > > > > which consumes them.
> > > > >
> > > > > Signed-off-by: Gabriel Somlo <[email protected]>
> > > > > Reviewed-by: Ilpo J?rvinen <[email protected]>
> > > > > ---
> > > > > ? drivers/tty/serial/liteuart.c | 3 +--
> > > > > ? 1 file changed, 1 insertion(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tty/serial/liteuart.c
> > > > > b/drivers/tty/serial/liteuart.c
> > > > > index 81aa7c1da73c..42ac9aee050a 100644
> > > > > --- a/drivers/tty/serial/liteuart.c
> > > > > +++ b/drivers/tty/serial/liteuart.c
> > > > > @@ -73,8 +73,7 @@ static void liteuart_timer(struct timer_list *t)
> > > > > ????? struct liteuart_port *uart = from_timer(uart, t, timer);
> > > > > ????? struct uart_port *port = &uart->port;
> > > > > ????? unsigned char __iomem *membase = port->membase;
> > > > > -??? int ch;
> > > > > -??? unsigned long status;
> > > > > +??? unsigned int status, ch;
> > > >
> > > > These should be u8 after all, right?
> >
> > OK, so:
> >
> > - I can hard-code `status` as `1`, like so:
> >
> > while(!litex_read8(membase + OFF_RXEMPTY) {
> > ...
> > if (!(uart_handle_sysrq_char(port, ch)))
> > uart_insert_char(port, 1, 0, ch, TTY_NORMAL);
> >
> > ... since `status` would always be `1` inside the loop. So I'm
> > basically going to get rid of it altogether.
>
> Yes, I had that in my mind. Except passing 1 to uart_insert_char() when
> overflow is hardwired to 0 makes no sense IMO :).
So, looking at what uart_insert_char() does, I could simply do this
instead:
while(!litex_read8(membase + OFF_RXEMPTY) {
...
/* LiteUART does not provide overrun bits */
if (!(uart_handle_sysrq_char(port, ch) ||
tty_insert_flip_char(&port->state->port, ch, TTY_NORMAL)))
++port->icount.buf_overrun;
That is, `tty_insert_flip_char() is the portion of `uart_insert_char()`
that actually gets executed if status is 1 and overrun is 0...
I'm not quite confident about whether this is an improvement in legibility
and/or code quality, but please let me know what *you* think... :)
> > - `ch` is indeed *produced* by `litex_read8()`, which would make it
> > `u8`. It is subsequently *consumed* by `uart_handle_sysrq_char()`
> > and `uart_insert_char()`, which both expect `unsigned int`.
>
> Ignore uart_handle_sysrq_char and uart_insert_char. They should be fixed one
> day. It should really be u8. All down the call chain (it magically turns
> into int in the sysrq handlers, then char is expected).
>
> > If you think it's better to go with the type when the value is
> > produced (as opposed to when it's consumed), I'm OK with that for
> > the upcoming v6 of the series... :)
>
> Yes, please. We should slowly convert _all_ of them.
OK, u8 it is, then.
> > > And can you change membase to u8 * too 8-)?
> >
> > Hmmm, in `struct uart_port` (in include/linux/serial_core.h), the
> > `membase` field is declared as:
> >
> > unsigned char __iomem *membase;
> >
> > which is why I'm thinking we should leave it as-is? Unless there are
> > plans (or a pending patch I'm unaware of) to switch the field in
> > include/linux/serial_core.h to `u8` as well? -- Please advise.
>
> Ah, then keep it. I somehow thought it's void *. And yes, even this should
> be u8 __iomem *, eventually.
OK, it should/will get updated in bulk once that change is made across
the entire subsystem, leaving as-is for now.
Thanks again for all your help and advice!
--Gabriel
On 22. 11. 22, 22:05, Gabriel L. Somlo wrote:
> So, looking at what uart_insert_char() does, I could simply do this
> instead:
>
> while(!litex_read8(membase + OFF_RXEMPTY) {
> ...
> /* LiteUART does not provide overrun bits */
> if (!(uart_handle_sysrq_char(port, ch) ||
> tty_insert_flip_char(&port->state->port, ch, TTY_NORMAL)))
> ++port->icount.buf_overrun;
>
> That is, `tty_insert_flip_char() is the portion of `uart_insert_char()`
> that actually gets executed if status is 1 and overrun is 0...
>
> I'm not quite confident about whether this is an improvement in legibility
> and/or code quality,
It's not :). Keep the uart_ helper.
--
js
suse labs