For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the
console write callback needs to enable/disable TX. It does this
by calling the rs485_start/stop_tx() callbacks. However, these
callbacks will disable/enable interrupts, which is a problem
for console write, as it must be responsible for
disabling/enabling interrupts.
Add an argument @in_con to the rs485_start/stop_tx() callbacks
to specify if they are being called from console write. If so,
the callbacks will not handle interrupt disabling/enabling.
For all call sites other than console write, there is no
functional change.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
drivers/tty/serial/8250/8250.h | 4 +--
drivers/tty/serial/8250/8250_bcm2835aux.c | 4 +--
drivers/tty/serial/8250/8250_omap.c | 2 +-
drivers/tty/serial/8250/8250_port.c | 34 +++++++++++++++--------
include/linux/serial_8250.h | 4 +--
5 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index e5310c65cf52..0d8717be0df7 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -231,8 +231,8 @@ void serial8250_rpm_put_tx(struct uart_8250_port *p);
int serial8250_em485_config(struct uart_port *port, struct ktermios *termios,
struct serial_rs485 *rs485);
-void serial8250_em485_start_tx(struct uart_8250_port *p);
-void serial8250_em485_stop_tx(struct uart_8250_port *p);
+void serial8250_em485_start_tx(struct uart_8250_port *p, bool in_con);
+void serial8250_em485_stop_tx(struct uart_8250_port *p, bool in_con);
void serial8250_em485_destroy(struct uart_8250_port *p);
extern struct serial_rs485 serial8250_em485_supported;
diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
index fdb53b54e99e..c9a106a86b56 100644
--- a/drivers/tty/serial/8250/8250_bcm2835aux.c
+++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
@@ -46,7 +46,7 @@ struct bcm2835aux_data {
u32 cntl;
};
-static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up)
+static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up, bool in_con)
{
if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
struct bcm2835aux_data *data = dev_get_drvdata(up->port.dev);
@@ -65,7 +65,7 @@ static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up)
serial8250_out_MCR(up, UART_MCR_RTS);
}
-static void bcm2835aux_rs485_stop_tx(struct uart_8250_port *up)
+static void bcm2835aux_rs485_stop_tx(struct uart_8250_port *up, bool in_con)
{
if (up->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
serial8250_out_MCR(up, 0);
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index b3be0fb184a3..fcbed7e98231 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -365,7 +365,7 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
if (up->port.rs485.flags & SER_RS485_ENABLED &&
up->port.rs485_config == serial8250_em485_config)
- serial8250_em485_stop_tx(up);
+ serial8250_em485_stop_tx(up, false);
}
/*
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 09ac521d232a..7c50387194ad 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -558,7 +558,7 @@ static int serial8250_em485_init(struct uart_8250_port *p)
deassert_rts:
if (p->em485->tx_stopped)
- p->rs485_stop_tx(p);
+ p->rs485_stop_tx(p, false);
return 0;
}
@@ -1398,10 +1398,11 @@ static void serial8250_stop_rx(struct uart_port *port)
/**
* serial8250_em485_stop_tx() - generic ->rs485_stop_tx() callback
* @p: uart 8250 port
+ * @in_con: true if called from console write, otherwise false
*
* Generic callback usable by 8250 uart drivers to stop rs485 transmission.
*/
-void serial8250_em485_stop_tx(struct uart_8250_port *p)
+void serial8250_em485_stop_tx(struct uart_8250_port *p, bool in_con)
{
unsigned char mcr = serial8250_in_MCR(p);
@@ -1419,7 +1420,9 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
serial8250_clear_and_reinit_fifos(p);
- __serial8250_start_rx_int(p);
+ /* In console context, caller handles interrupt enabling. */
+ if (!in_con)
+ __serial8250_start_rx_int(p);
}
}
EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
@@ -1434,7 +1437,7 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
serial8250_rpm_get(p);
uart_port_lock_irqsave(&p->port, &flags);
if (em485->active_timer == &em485->stop_tx_timer) {
- p->rs485_stop_tx(p);
+ p->rs485_stop_tx(p, false);
em485->active_timer = NULL;
em485->tx_stopped = true;
}
@@ -1466,7 +1469,7 @@ static void __stop_tx_rs485(struct uart_8250_port *p, u64 stop_delay)
em485->active_timer = &em485->stop_tx_timer;
hrtimer_start(&em485->stop_tx_timer, ns_to_ktime(stop_delay), HRTIMER_MODE_REL);
} else {
- p->rs485_stop_tx(p);
+ p->rs485_stop_tx(p, false);
em485->active_timer = NULL;
em485->tx_stopped = true;
}
@@ -1555,6 +1558,7 @@ static inline void __start_tx(struct uart_port *port)
/**
* serial8250_em485_start_tx() - generic ->rs485_start_tx() callback
* @up: uart 8250 port
+ * @in_con: true if called from console write, otherwise false
*
* Generic callback usable by 8250 uart drivers to start rs485 transmission.
* Assumes that setting the RTS bit in the MCR register means RTS is high.
@@ -1562,12 +1566,20 @@ static inline void __start_tx(struct uart_port *port)
* stoppable by disabling the UART_IER_RDI interrupt. (Some chips set the
* UART_LSR_DR bit even when UART_IER_RDI is disabled, foiling this approach.)
*/
-void serial8250_em485_start_tx(struct uart_8250_port *up)
+void serial8250_em485_start_tx(struct uart_8250_port *up, bool in_con)
{
unsigned char mcr = serial8250_in_MCR(up);
- if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
- serial8250_stop_rx(&up->port);
+ if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
+ /*
+ * In console context, caller handles interrupt disabling. So
+ * only LSR_DR masking is needed.
+ */
+ if (in_con)
+ __serial8250_stop_rx_mask_dr(&up->port);
+ else
+ serial8250_stop_rx(&up->port);
+ }
if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
mcr |= UART_MCR_RTS;
@@ -1600,7 +1612,7 @@ static bool start_tx_rs485(struct uart_port *port)
if (em485->tx_stopped) {
em485->tx_stopped = false;
- up->rs485_start_tx(up);
+ up->rs485_start_tx(up, false);
if (up->port.rs485.delay_rts_before_send > 0) {
em485->active_timer = &em485->start_tx_timer;
@@ -3402,7 +3414,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
if (em485) {
if (em485->tx_stopped)
- up->rs485_start_tx(up);
+ up->rs485_start_tx(up, true);
mdelay(port->rs485.delay_rts_before_send);
}
@@ -3440,7 +3452,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
if (em485) {
mdelay(port->rs485.delay_rts_after_send);
if (em485->tx_stopped)
- up->rs485_stop_tx(up);
+ up->rs485_stop_tx(up, true);
}
serial_port_out(port, UART_IER, ier);
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index e0717c8393d7..c25c026d173d 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -161,8 +161,8 @@ struct uart_8250_port {
void (*dl_write)(struct uart_8250_port *up, u32 value);
struct uart_8250_em485 *em485;
- void (*rs485_start_tx)(struct uart_8250_port *);
- void (*rs485_stop_tx)(struct uart_8250_port *);
+ void (*rs485_start_tx)(struct uart_8250_port *up, bool in_con);
+ void (*rs485_stop_tx)(struct uart_8250_port *up, bool in_con);
/* Serial port overrun backoff */
struct delayed_work overrun_backoff;
--
2.39.5On Fri 2024-10-25 13:03:26, John Ogness wrote: > For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the > console write callback needs to enable/disable TX. It does this > by calling the rs485_start/stop_tx() callbacks. However, these > callbacks will disable/enable interrupts, which is a problem > for console write, as it must be responsible for > disabling/enabling interrupts. It is not clear to me what exactly is the problem. Is the main problem calling pm_runtime*() API because it uses extra locks and can cause deadlocks? Or is it more complicated? IMHO, it would deserve some explanation. > Add an argument @in_con to the rs485_start/stop_tx() callbacks > to specify if they are being called from console write. If so, > the callbacks will not handle interrupt disabling/enabling. > > For all call sites other than console write, there is no > functional change. > > Signed-off-by: John Ogness <john.ogness@linutronix.de> It looks like the code does what the description says. And honestly, I do not have any idea how to improve the naming. I would keep it as is after reading John's answers in the thread. IMHO, one thing which makes things comlicated is that serial8250_em485_start_tx() and serial8250_em485_stop_tx() are not completely reversible operations. Especially, the change done by __serial8250_stop_rx_mask_dr() is not reverted in serial8250_em485_stop_tx(). It makes things look tricky. But I think that it is beyond the scope of this patchset to do anything about it. Just 2 my cents. Best Regaards, Petr
On 25. 10. 24, 12:57, John Ogness wrote:
> For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the
> console write callback needs to enable/disable TX. It does this
> by calling the rs485_start/stop_tx() callbacks. However, these
> callbacks will disable/enable interrupts, which is a problem
> for console write, as it must be responsible for
> disabling/enabling interrupts.
>
> Add an argument @in_con to the rs485_start/stop_tx() callbacks
> to specify if they are being called from console write. If so,
> the callbacks will not handle interrupt disabling/enabling.
>
> For all call sites other than console write, there is no
> functional change.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
...
> @@ -1562,12 +1566,20 @@ static inline void __start_tx(struct uart_port *port)
> * stoppable by disabling the UART_IER_RDI interrupt. (Some chips set the
> * UART_LSR_DR bit even when UART_IER_RDI is disabled, foiling this approach.)
> */
> -void serial8250_em485_start_tx(struct uart_8250_port *up)
> +void serial8250_em485_start_tx(struct uart_8250_port *up, bool in_con)
> {
> unsigned char mcr = serial8250_in_MCR(up);
>
> - if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> - serial8250_stop_rx(&up->port);
> + if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> + /*
> + * In console context, caller handles interrupt disabling. So
> + * only LSR_DR masking is needed.
> + */
> + if (in_con)
> + __serial8250_stop_rx_mask_dr(&up->port);
> + else
> + serial8250_stop_rx(&up->port);
Would it make sense to propagate in_con into serial8250_stop_rx() and do
the logic there? That would effectively eliminate patch 2/6.
thanks,
--
js
suse labs
On 2024-10-30, Jiri Slaby <jirislaby@kernel.org> wrote:
>> -void serial8250_em485_start_tx(struct uart_8250_port *up)
>> +void serial8250_em485_start_tx(struct uart_8250_port *up, bool in_con)
>> {
>> unsigned char mcr = serial8250_in_MCR(up);
>>
>> - if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
>> - serial8250_stop_rx(&up->port);
>> + if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
>> + /*
>> + * In console context, caller handles interrupt disabling. So
>> + * only LSR_DR masking is needed.
>> + */
>> + if (in_con)
>> + __serial8250_stop_rx_mask_dr(&up->port);
>> + else
>> + serial8250_stop_rx(&up->port);
>
> Would it make sense to propagate in_con into serial8250_stop_rx() and do
> the logic there? That would effectively eliminate patch 2/6.
I considered this, however:
1. The whole idea of stopping RX in order to do TX is an RS485
issue. Modifying the general ->stop_rx() callback for this purpose is
kind of out of place.
2. The ->stop_rx() callback is a general uart_ops callback. Changing its
prototype would literally affect all serial drivers. OTOH the
->rs485_start_tx() callback is specific to the 8250 driver. (It seems
each driver has implemented their own method for handling the RS485
hacks.)
So I would prefer to keep the necessary RS485 changes 8250-specific for
now.
John
On Fri, Oct 25, 2024 at 01:03:26PM +0206, John Ogness wrote: > For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the > console write callback needs to enable/disable TX. It does this > by calling the rs485_start/stop_tx() callbacks. However, these It would be nice if you be consistent across the commit messages and also provide the names of the callbacks in full, because I dunno if we have a local stop_tx() or rs485_start() or whatever the above means. If it is "the rs485_start_tx() / rs485_stop_tx() callbacks.", it's much clearer for the reader. > callbacks will disable/enable interrupts, which is a problem toggle? > for console write, as it must be responsible for > disabling/enabling interrupts. toggling ? > Add an argument @in_con to the rs485_start/stop_tx() callbacks As per above. > to specify if they are being called from console write. If so, > the callbacks will not handle interrupt disabling/enabling. toggling ? > For all call sites other than console write, there is no > functional change. So, why not call the parameter better to emphasize that it's about IRQ toggling? bool toggle_irq ? -- With Best Regards, Andy Shevchenko
On 2024-10-25, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >> Add an argument @in_con to the rs485_start/stop_tx() callbacks >> to specify if they are being called from console write. If so, >> the callbacks will not handle interrupt disabling/enabling. > > toggling ? > >> For all call sites other than console write, there is no >> functional change. > > So, why not call the parameter better to emphasize that it's about IRQ > toggling? bool toggle_irq ? Currently there are only 2 users: serial8250_em485_stop_tx() bcm2835aux_rs485_stop_tx() The first one toggles the IER bits, the second one does not. I figured it would make more sense to specify the context rather than what needs to be done and let the 8250-variant decide what it should do. But I have no problems renaming it to toggle_irq. It is an 8250-specific callback with few users. And really the IER bits is the only reason that the argument even needs to exist. John
On Fri, Oct 25, 2024 at 04:31:05PM +0206, John Ogness wrote: > On 2024-10-25, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> Add an argument @in_con to the rs485_start/stop_tx() callbacks > >> to specify if they are being called from console write. If so, > >> the callbacks will not handle interrupt disabling/enabling. > > > > toggling ? > > > >> For all call sites other than console write, there is no > >> functional change. > > > > So, why not call the parameter better to emphasize that it's about IRQ > > toggling? bool toggle_irq ? > > Currently there are only 2 users: > > serial8250_em485_stop_tx() > bcm2835aux_rs485_stop_tx() > > The first one toggles the IER bits, the second one does not. I figured > it would make more sense to specify the context rather than what needs > to be done and let the 8250-variant decide what it should do. > > But I have no problems renaming it to toggle_irq. It is an 8250-specific > callback with few users. And really the IER bits is the only reason that > the argument even needs to exist. Maybe toggle_ier will be better than? I haven't looked deeply into the implementations, so choose whichever describes better what's behind it. -- With Best Regards, Andy Shevchenko
© 2016 - 2024 Red Hat, Inc.