| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cfb11ff3-e482-59fc-9f01-c19f9b23bc65@suse.com>
Date: Tue, 5 Jan 2021 11:38:39 +0100
From: Jiri Slaby <jslaby@...e.com>
To: Ivan Sistik <sistik@...olutions.sk>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Stefan Wahren <stefan.wahren@...e.com>
Cc: suravee.suthikulpanit@....com, sbranden@...adcom.com,
wahrenst@....net, linux-arm-kernel@...ts.infradead.org,
linux-rpi-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-serial@...r.kernel.org
Subject: Re: [PATCH] tty: serial: amba-pl011: added RS485 support [v3]
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
Powered by blists - more mailing lists