Messages in this thread |  | | Subject | Re: [PATCH] tty: serial: amba-pl011: added RS485 support [v3] | From | Jiri Slaby <> | Date | Tue, 5 Jan 2021 11:38:39 +0100 |
| |
On 30. 12. 20, 4:16, Ivan Sistik wrote: > --- a/drivers/tty/serial/Kconfig > +++ b/drivers/tty/serial/Kconfig > @@ -75,6 +75,17 @@ config SERIAL_AMBA_PL011_CONSOLE > your boot loader (lilo or loadlin) about how to pass options to the > kernel at boot time.) > > +config SERIAL_AMBA_PL011_SOFT_RS485 > + bool "RS485 software direction switching for ARM AMBA PL011 serial" > + depends on SERIAL_AMBA_PL011=y > + help > + Enable RS485 software direction switching of driver enable (RTS pin) > + for ARM AMBA PL011 serial. AMBA PL011 does not have HW support for > + RS485. This driver use 2 hrtimers. One is used for rs485 delays.
"uses"
> + Second one is used for polling of TX FIFO. There is not TX FIFO
"The second one"
"There is no"
> + empty interrupt in PL011. Secondary timer is started by empty
"The secondary". But I am confused, 2 timers: one, second, and secondary -- three timers?
> + transmit buffer. > + > config SERIAL_EARLYCON_ARM_SEMIHOST > bool "Early console using ARM semihosting" > depends on ARM64 || ARM > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > index 16720c97a..6a40e5bc5 100644 > --- a/drivers/tty/serial/amba-pl011.c > +++ b/drivers/tty/serial/amba-pl011.c > @@ -46,6 +46,7 @@ > #include <linux/sizes.h> > #include <linux/io.h> > #include <linux/acpi.h> > +#include <linux/math64.h> > > #include "amba-pl011.h" > > @@ -60,6 +61,18 @@ > #define UART_DR_ERROR (UART011_DR_OE|UART011_DR_BE|UART011_DR_PE|UART011_DR_FE) > #define UART_DUMMY_DR_RX (1 << 16) > > +#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485 > +/* > + * Enum with current status > + */ > +enum rs485_status { > + rs485_receiving, > + rs485_delay_before_send, > + rs485_sending, > + rs485_delay_after_send
These are too generic names.
> +}; > +#endif
You don't have to add ifdeffery to declarations.
> + > static u16 pl011_std_offsets[REG_ARRAY_SIZE] = { > [REG_DR] = UART01x_DR, > [REG_FR] = UART01x_FR, > @@ -270,6 +283,16 @@ struct uart_amba_port { > unsigned int old_cr; /* state during shutdown */ > unsigned int fixed_baud; /* vendor-set fixed baud rate */ > char type[12]; > + > +#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485 > + enum rs485_status rs485_current_status; /* status used for RTS */ > + enum rs485_status rs485_next_status; /* this status after tick */ > + struct hrtimer rs485_delay_timer; > + struct hrtimer rs485_tx_empty_poll_timer; > + unsigned long send_char_time; /* send char (nanoseconds) */ > + bool rs485_last_char_sending; > +#endif > + > #ifdef CONFIG_DMA_ENGINE > /* DMA stuff */ > bool using_tx_dma; > @@ -306,6 +329,36 @@ static void pl011_write(unsigned int val, const struct uart_amba_port *uap, > writew_relaxed(val, addr); > } > > +#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485 > + > +static void pl011_rs485_start_rts_delay(struct uart_amba_port *uap); > + > +static void rs485_set_rts_signal(struct uart_amba_port *uap, bool value) > +{ > + unsigned int rts_temp_cr; > + > + rts_temp_cr = pl011_read(uap, REG_CR); > + > + if (!value) > + rts_temp_cr |= UART011_CR_RTS; > + else > + rts_temp_cr &= ~UART011_CR_RTS; > + > + pl011_write(rts_temp_cr, uap, REG_CR); > +} > + > +void rs485_cancel_timers(struct uart_amba_port *uap)
Why not static? And too generic name. The same above.
> +{ > + hrtimer_try_to_cancel(&(uap->rs485_delay_timer)); > + hrtimer_try_to_cancel(&(uap->rs485_tx_empty_poll_timer));
No need for the parentheses. They occur on many places in the patch. Have you run the patch through checkpatch?
> +} > + > +bool rs485_tx_fifo_empty(struct uart_amba_port *uap) > +{ > + return (pl011_read(uap, REG_FR) & UART011_FR_TXFE);
detto.
> +} > +#endif > + > /* > * Reads up to 256 characters from the FIFO or until it's empty and > * inserts them into the TTY layer. Returns the number of characters > @@ -1301,6 +1354,11 @@ static void pl011_stop_tx(struct uart_port *port) > uap->im &= ~UART011_TXIM; > pl011_write(uap->im, uap, REG_IMSC); > pl011_dma_tx_stop(uap); > + > +#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485 > + if (uap->port.rs485.flags & SER_RS485_ENABLED) > + pl011_rs485_start_rts_delay(uap);
Could you do the check inside pl011_rs485_start_rts_delay and remove the ifdef completely?
> +#endif > } > > static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq); > @@ -1319,8 +1377,113 @@ static void pl011_start_tx(struct uart_port *port) > struct uart_amba_port *uap = > container_of(port, struct uart_amba_port, port); > > - if (!pl011_dma_tx_start(uap)) > - pl011_start_tx_pio(uap); > +#define START_PL011_TX() \ > + do { \ > + if (!pl011_dma_tx_start(uap)) \ > + pl011_start_tx_pio(uap); \ > + } while (0)
Defining a macro inside a function? No. This should be a separate function as well as the code below anyway.
> +#ifndef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485 > + START_PL011_TX(); > +#else > + > + if (uap->port.rs485.flags & SER_RS485_ENABLED) { > + ktime_t ktime; > + > + switch (uap->rs485_current_status) { > + case rs485_delay_after_send: > + > + rs485_cancel_timers(uap); > + > + /* check if timer expired */ > + if (uap->rs485_current_status > + != rs485_delay_after_send) { > + /* Timer expired and RTS is in wrong state.*/ > + uap->rs485_current_status > + = rs485_delay_before_send; > + uap->rs485_next_status = rs485_sending; > + > + rs485_set_rts_signal(uap, > + uap->port.rs485.flags > + & SER_RS485_RTS_ON_SEND); > + > + ktime = ktime_set(0, > + uap->port.rs485 > + .delay_rts_before_send > + * 1000000L); > + > + hrtimer_start( > + &(uap->rs485_delay_timer), > + ktime, > + HRTIMER_MODE_REL);
SPaghetti code. It will be eliminated by moving to a separate function suggested above.
> + return; > + } > + > + /* timer was stopped and driver can continue sending */ > + uap->rs485_current_status = rs485_sending; > + uap->rs485_next_status = rs485_sending; > + > + /* driver is already in sending state */ > + START_PL011_TX(); > + break; > + > + > + case rs485_sending: > + /* stop old timer. There can be running timer */ > + /* which is checking TX FIFO empty flag */ > + rs485_cancel_timers(uap); > + > + /* driver is already in sending state */ > + START_PL011_TX(); > + break; > + > + case rs485_receiving: > + default: > + /* stop old timer. There can be running timer */ > + /* which is checking TX FIFO empty flag */ > + rs485_cancel_timers(uap); > + > + /* Set RTS */ > + rs485_set_rts_signal(uap, > + uap->port.rs485.flags > + & SER_RS485_RTS_ON_SEND); > + > + if (uap->port.rs485.delay_rts_before_send == 0) { > + /* Change state */ > + uap->rs485_current_status > + = rs485_sending; > + uap->rs485_next_status > + = rs485_sending; > + > + /* driver is in sending state */ > + START_PL011_TX(); > + break; > + } > + > + /* Change state */ > + uap->rs485_current_status > + = rs485_delay_before_send; > + uap->rs485_next_status = rs485_sending; > + > + /* Start timer */ > + ktime = ktime_set(0, > + uap->port.rs485.delay_rts_before_send > + * 1000000L);
This is overcomplicated ms_to_ktime().
> + hrtimer_start(&(uap->rs485_delay_timer), > + ktime, > + HRTIMER_MODE_REL); > + break; > + > + case rs485_delay_before_send: > + /* do nothing because delay timer should be running */ > + break; > + } > + } else { > + START_PL011_TX(); > + } > +#endif > + > +#undef START_PL011_TX > } > > static void pl011_stop_rx(struct uart_port *port) > @@ -1476,6 +1639,166 @@ static void check_apply_cts_event_workaround(struct uart_amba_port *uap) > dummy_read = pl011_read(uap, REG_ICR); > } > > +#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485 > + > +/* > + * Change state according to pending delay > + * Locking: port is locked in this function > + */ > +static enum hrtimer_restart > +pl011_rs485_tx_poll_timer(struct hrtimer *timer) > +{ > + unsigned long flags; > + ktime_t ktime; > + > + struct uart_amba_port *uap = > + container_of(timer, struct uart_amba_port, > + rs485_tx_empty_poll_timer); > + > + spin_lock_irqsave(&uap->port.lock, flags); > + > + if (!(uart_circ_empty(&uap->port.state->xmit))) { > + spin_unlock_irqrestore(&uap->port.lock, flags); > + return HRTIMER_NORESTART;
Create a label at the end and goto from here.
> + } > + > + if (!rs485_tx_fifo_empty(uap) || !uap->rs485_last_char_sending) { > + /* > + * FIFO is empty but there is last char in transmit shift > + * register so we need one more tick > + */ > + uap->rs485_last_char_sending = rs485_tx_fifo_empty(uap); > + > + hrtimer_forward_now(timer, ktime_set(0, uap->send_char_time));
ns_to_ktime()
> + > + spin_unlock_irqrestore(&uap->port.lock, flags); > + return HRTIMER_RESTART; > + } > + > + /* Check if delay after send is set*/ > + if (uap->port.rs485.delay_rts_after_send == 0) { > + /* Change state */ > + uap->rs485_current_status = rs485_receiving; > + uap->rs485_next_status = rs485_receiving; > + > + /* if there is no delay after send change RTS value*/ > + rs485_set_rts_signal(uap, > + uap->port.rs485.flags > + & SER_RS485_RTS_AFTER_SEND); > + > + spin_unlock_irqrestore(&uap->port.lock, flags); > + return HRTIMER_NORESTART; > + } > + > + /* Change state */ > + uap->rs485_current_status = rs485_delay_after_send; > + uap->rs485_next_status = rs485_receiving; > + > + /* RTS will be set in timer handler */ > + > + /* Start delay timer */ > + ktime = ktime_set(0, (uap->port.rs485.delay_rts_after_send > + * 1000000L));
ms_to_ktime().
> + hrtimer_start(&(uap->rs485_delay_timer), ktime, HRTIMER_MODE_REL); > + > + spin_unlock_irqrestore(&uap->port.lock, flags); > + return HRTIMER_NORESTART; > +} ... > +static void pl011_rs485_start_rts_delay(struct uart_amba_port *uap) > +{ > + ktime_t ktime; > + > + if (uap->rs485_current_status == rs485_receiving) > + return; > + > + /* if there is timeout in progress cancel it and start new */ > + hrtimer_try_to_cancel(&(uap->rs485_delay_timer)); > + hrtimer_try_to_cancel(&(uap->rs485_tx_empty_poll_timer)); > + > + > + if (!rs485_tx_fifo_empty(uap) > + || uap->port.rs485.delay_rts_after_send == 0) { > + /* > + * Schedule validation timer if there is data in TX FIFO > + * because there is not TX FIFO empty interrupt > + */ > + > + uap->rs485_current_status = rs485_sending; > + uap->rs485_next_status = rs485_sending; > + > + uap->rs485_last_char_sending = false; > + > + ktime = ktime_set(0, uap->send_char_time);
ns_to_ktime()
> + hrtimer_start(&(uap->rs485_tx_empty_poll_timer), > + ktime, > + HRTIMER_MODE_REL); > + return; > + } > + > + /* Change state */ > + uap->rs485_current_status = rs485_delay_after_send; > + uap->rs485_next_status = rs485_receiving; > + > + /* RTS will be set in timer handler */ > + > + /* Start timer */ > + ktime = ktime_set(0, (uap->port.rs485.delay_rts_after_send > + * 1000000L));
ms_to_ktime().
> + > + hrtimer_start(&(uap->rs485_delay_timer), > + ktime, > + HRTIMER_MODE_REL); > +} > +#endif > + > static irqreturn_t pl011_int(int irq, void *dev_id) > { > struct uart_amba_port *uap = dev_id; > @@ -1618,6 +1941,11 @@ static void pl011_quiesce_irqs(struct uart_port *port) > */ > pl011_write(pl011_read(uap, REG_IMSC) & ~UART011_TXIM, uap, > REG_IMSC); > + > +#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485 > + if (uap->port.rs485.flags & SER_RS485_ENABLED) > + pl011_rs485_start_rts_delay(uap); > +#endif
The same as above.
> } > > static int pl011_get_poll_char(struct uart_port *port) > @@ -1690,6 +2018,27 @@ static int pl011_hwinit(struct uart_port *port) > if (plat->init) > plat->init(); > } > + > +#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
Do this in a separate function and eliminate such ifdefs in the code.
> + /* > + * Initialize timers used for RS485 > + */ > + hrtimer_init(&(uap->rs485_delay_timer), > + CLOCK_MONOTONIC, > + HRTIMER_MODE_REL); > + > + uap->rs485_delay_timer.function = &pl011_rs485_timer; > + > + hrtimer_init(&(uap->rs485_tx_empty_poll_timer), > + CLOCK_MONOTONIC, > + HRTIMER_MODE_REL); > + > + uap->rs485_tx_empty_poll_timer.function = &pl011_rs485_tx_poll_timer; > + > + uap->rs485_current_status = rs485_receiving; > + rs485_set_rts_signal(uap, false); > +#endif > + > return 0; > } > > @@ -1873,6 +2222,16 @@ static void pl011_shutdown(struct uart_port *port) > struct uart_amba_port *uap = > container_of(port, struct uart_amba_port, port); > > +#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485 > + if (uap->port.rs485.flags & SER_RS485_ENABLED) { > + hrtimer_try_to_cancel(&(uap->rs485_delay_timer)); > + hrtimer_try_to_cancel(&(uap->rs485_tx_empty_poll_timer)); > + > + uap->rs485_current_status = rs485_receiving; > + rs485_set_rts_signal(uap, true); > + } > +#endif
Separate function + no ifdef.
> + > pl011_disable_interrupts(uap); > > pl011_dma_shutdown(uap); > @@ -1955,6 +2314,24 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios, > unsigned long flags; > unsigned int baud, quot, clkdiv; > > +#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485 > + unsigned int transfer_bit_count; > + unsigned long char_transfer_time; > + > + /* > + * Calculate bit count which will be send
"sent"
> + * by UART. It is used for calculation of > + * time required to start timer until TX FIFO (HW) is empty
Dot at the end.
> + * There is not interrupt for FIFO empty in PL011.
"There is not an" or "There is no"
> + * There is only FIFO empty flag in REG_FR. > + */ > + transfer_bit_count = 0; > + > +#define ADD_DATA_BITS(bits) (transfer_bit_count += bits)
This is ugly. 1) it's a macro. 2) it hides uses of transfer_bit_count from the reader.
> +#else > +#define ADD_DATA_BITS(bits) > +#endif > + > if (uap->vendor->oversampling) > clkdiv = 8; > else > @@ -1981,29 +2358,53 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios, > switch (termios->c_cflag & CSIZE) { > case CS5: > lcr_h = UART01x_LCRH_WLEN_5; > + ADD_DATA_BITS(7); > break; > case CS6: > lcr_h = UART01x_LCRH_WLEN_6; > + ADD_DATA_BITS(8); > break; > case CS7: > lcr_h = UART01x_LCRH_WLEN_7; > + ADD_DATA_BITS(9); > break; > default: // CS8 > lcr_h = UART01x_LCRH_WLEN_8; > + ADD_DATA_BITS(10); > break; > } > - if (termios->c_cflag & CSTOPB) > + > + if (termios->c_cflag & CSTOPB) { > lcr_h |= UART01x_LCRH_STP2; > + ADD_DATA_BITS(1); > + } > + > if (termios->c_cflag & PARENB) { > lcr_h |= UART01x_LCRH_PEN; > + ADD_DATA_BITS(1); > + > if (!(termios->c_cflag & PARODD)) > lcr_h |= UART01x_LCRH_EPS; > + > if (termios->c_cflag & CMSPAR) > lcr_h |= UART011_LCRH_SPS; > } > + > +#undef ADD_DATA_BITS > + > if (uap->fifosize > 1) > lcr_h |= UART01x_LCRH_FEN; > > +#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485 > + /* Calculate time required to send one char (nanoseconds) */ > + char_transfer_time = > + (unsigned long) div_u64( > + mul_u32_u32( > + (u32)transfer_bit_count, > + (u32)NSEC_PER_SEC), > + (u32)baud);
Wny do you cast all that?
> +#endif > + > spin_lock_irqsave(&port->lock, flags); > > /* > @@ -2020,6 +2421,11 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios, > old_cr = pl011_read(uap, REG_CR); > pl011_write(0, uap, REG_CR); > > +#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485 > + /* Update send_char_time in locked context */ > + uap->send_char_time = char_transfer_time; > +#endif > + > if (termios->c_cflag & CRTSCTS) { > if (old_cr & UART011_CR_RTS) > old_cr |= UART011_CR_RTSEN; > @@ -2122,6 +2528,47 @@ static void pl011_config_port(struct uart_port *port, int flags) > } > } > > +/* > + * Configure RS485 > + * Locking: called with port lock held and IRQs disabled > + */ > +#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485 > +static int pl011_config_rs485(struct uart_port *port, > + struct serial_rs485 *rs485) > +{ > + bool was_disabled; > + struct uart_amba_port *uap = > + container_of(port, struct uart_amba_port, port); > + > + was_disabled = !(port->rs485.flags & SER_RS485_ENABLED);
Where is this used?
> + port->rs485.flags = rs485->flags; > + port->rs485.delay_rts_after_send = rs485->delay_rts_after_send; > + port->rs485.delay_rts_before_send = rs485->delay_rts_before_send; > + > + if (port->rs485.flags & SER_RS485_ENABLED) { > + unsigned int cr; > + > + hrtimer_try_to_cancel(&(uap->rs485_delay_timer)); > + hrtimer_try_to_cancel(&(uap->rs485_tx_empty_poll_timer)); > + > + /* If RS485 is enabled, disable auto RTS */ > + cr = pl011_read(uap, REG_CR); > + cr &= ~UART011_CR_RTSEN; > + pl011_write(cr, uap, REG_CR); > + > + uap->rs485_current_status = rs485_receiving; > + rs485_set_rts_signal(uap, > + port->rs485.flags > + & SER_RS485_RTS_AFTER_SEND); > + } else { > + rs485_set_rts_signal(uap, true); > + } > + > + return 0; > +} > +#endif > + > /* > * verify the new serial_struct (for TIOCSSERIAL). > */ > @@ -2647,6 +3094,11 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id) > uap->port.irq = dev->irq[0]; > uap->port.ops = &amba_pl011_pops; > > +#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485 > + uap->port.rs485_config = &pl011_config_rs485; > + uap->port.rs485.flags = 0; /* RS485 is not enabled by default */ > +#endif > + > snprintf(uap->type, sizeof(uap->type), "PL011 rev%u", amba_rev(dev)); > > ret = pl011_setup_port(&dev->dev, uap, &dev->res, portnr); > @@ -2819,10 +3271,15 @@ static struct amba_driver pl011_driver = { > > static int __init pl011_init(void) > { > +#if IS_ENABLED(CONFIG_SERIAL_AMBA_PL011_SOFT_RS485) > + printk(KERN_INFO "Serial: AMBA PL011 UART driver with soft RS485 support\n"); > +#else > printk(KERN_INFO "Serial: AMBA PL011 UART driver\n"); > +#endif
Didn't Stefan already commented on this?
You can actually remove the print completely (in a separate patch).
> if (platform_driver_register(&arm_sbsa_uart_platform_driver)) > pr_warn("could not register SBSA UART platform driver\n"); > +
And I saw a comment about added whitespace too.
> return amba_driver_register(&pl011_driver); > } > >
thanks, -- js suse labs
|  |