| 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
| ||
|
Date: Mon, 20 Apr 2020 23:21:32 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Fabrice Gasnier <fabrice.gasnier@...com>
Cc: gregkh@...uxfoundation.org, robh+dt@...nel.org,
mcoquelin.stm32@...il.com, alexandre.torgue@...com,
linux-serial@...r.kernel.org, devicetree@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
andy.shevchenko@...il.com, Erwan LE RAY <erwan.leray@...com>
Subject: Re: [PATCH v2 1/2] tty: serial: Add modem control gpio support for
STM32 UART
Hi,
On Fri, Apr 17, 2020 at 03:15:07PM +0200, Fabrice Gasnier wrote:
> On 4/16/20 7:57 PM, mani@...nel.org wrote:
> > From: Manivannan Sadhasivam <mani@...nel.org>
> >
> > STM32 UART controllers have the built in modem control support using
> > dedicated gpios which can be enabled using 'st,hw-flow-ctrl' flag in DT.
> > But there might be cases where the board design need to use different
> > gpios for modem control.
> >
> > For supporting such cases, this commit adds modem control gpio support
> > to STM32 UART controller using mctrl_gpio driver.
> >
> > Signed-off-by: Manivannan Sadhasivam <mani@...nel.org>
> > ---
> > drivers/tty/serial/Kconfig | 1 +
> > drivers/tty/serial/stm32-usart.c | 43 +++++++++++++++++++++++++++++++-
> > drivers/tty/serial/stm32-usart.h | 1 +
> > 3 files changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> > index 0aea76cd67ff..e7a6f2130684 100644
> > --- a/drivers/tty/serial/Kconfig
> > +++ b/drivers/tty/serial/Kconfig
> > @@ -1462,6 +1462,7 @@ config SERIAL_STM32
> > tristate "STMicroelectronics STM32 serial port support"
> > select SERIAL_CORE
> > depends on ARCH_STM32 || COMPILE_TEST
> > + select SERIAL_MCTRL_GPIO if GPIOLIB
> > help
> > This driver is for the on-chip Serial Controller on
> > STMicroelectronics STM32 MCUs.
> > diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> > index 5e93e8d40f59..026982259714 100644
> > --- a/drivers/tty/serial/stm32-usart.c
> > +++ b/drivers/tty/serial/stm32-usart.c
> > @@ -31,6 +31,7 @@
> > #include <linux/tty_flip.h>
> > #include <linux/tty.h>
> >
> > +#include "serial_mctrl_gpio.h"
> > #include "stm32-usart.h"
> >
> > static void stm32_stop_tx(struct uart_port *port);
> > @@ -510,12 +511,29 @@ static void stm32_set_mctrl(struct uart_port *port, unsigned int mctrl)
> > stm32_set_bits(port, ofs->cr3, USART_CR3_RTSE);
> > else
> > stm32_clr_bits(port, ofs->cr3, USART_CR3_RTSE);
> > +
> > + mctrl_gpio_set(stm32_port->gpios, mctrl);
> > }
> >
> > static unsigned int stm32_get_mctrl(struct uart_port *port)
> > {
> > + struct stm32_port *stm32_port = to_stm32_port(port);
> > + int ret;
>
> Hi Mani,
>
> Please find few minor remarks and a question from my side.
>
> 'ret' could be an unsigned int
>
Ok
> > +
> > /* This routine is used to get signals of: DCD, DSR, RI, and CTS */
> > - return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
> > + ret = TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
> > +
> > + return mctrl_gpio_get(stm32_port->gpios, &ret);
> > +}
> > +
> > +static void stm32_enable_ms(struct uart_port *port)
> > +{
>
> Just a question here: purpose of your patch is to handle the gpio case.
> So you may get modem control interrupts from gpios with this patch.
>
We will only get CTS gpio interrupt which will get handled by mctrl_gpio driver.
> In other drivers, I can see the implementation checks gpio usage (like
> in atmel_serial). When there's no gpio, the corresponding interrupt at
> the serial controller level is enabled, e.g. :
>
> if (!mctrl_gpio_to_gpiod(atmel_port->gpios, UART_GPIO_CTS))
> ier |= ...
>
Those drivers support both gpio and hardware modem control interrupts, so
they are checking to make sure either one of them is enabled at a time. But
on STM32 UART, modem interrupts are not enabled (USART_CR3_CTSIE etc...)
> Do you need modem control interrupts in your case ?
>
Nope. Since modem control interrupts in UART block are not enabled currently,
I don't bother about it.
> In case the Stinger96 board signals gets fixed in a future revision,
> would it be needed to enable modem control interrupts in the USART
> controller ?
>
I don't think so. The current driver supports hardware flow control without
using interrupts, so we are fine.
> > + mctrl_gpio_enable_ms(to_stm32_port(port)->gpios);
> > +}
> > +
> > +static void stm32_disable_ms(struct uart_port *port)
> > +{
> > + mctrl_gpio_disable_ms(to_stm32_port(port)->gpios);
> > }
> >
> > /* Transmit stop */
> > @@ -626,6 +644,9 @@ static void stm32_shutdown(struct uart_port *port)
> > u32 val, isr;
> > int ret;
> >
> > + /* Disable modem control interrupts */
> > + stm32_disable_ms(port);
> > +
> > val = USART_CR1_TXEIE | USART_CR1_TE;
> > val |= stm32_port->cr1_irq | USART_CR1_RE;
> > val |= BIT(cfg->uart_enable_bit);
> > @@ -764,6 +785,12 @@ static void stm32_set_termios(struct uart_port *port, struct ktermios *termios,
> > cr3 |= USART_CR3_CTSE | USART_CR3_RTSE;
> > }
> >
> > + /* Handle modem control interrupts */
> > + if (UART_ENABLE_MS(port, termios->c_cflag))
> > + stm32_enable_ms(port);
> > + else
> > + stm32_disable_ms(port);
> > +
> > usartdiv = DIV_ROUND_CLOSEST(port->uartclk, baud);
> >
> > /*
> > @@ -898,6 +925,7 @@ static const struct uart_ops stm32_uart_ops = {
> > .throttle = stm32_throttle,
> > .unthrottle = stm32_unthrottle,
> > .stop_rx = stm32_stop_rx,
> > + .enable_ms = stm32_enable_ms,
> > .break_ctl = stm32_break_ctl,
> > .startup = stm32_startup,
> > .shutdown = stm32_shutdown,
> > @@ -964,6 +992,19 @@ static int stm32_init_port(struct stm32_port *stm32port,
> > ret = -EINVAL;
>
> return -EINVAL;
>
> > }
> >
> > + stm32port->gpios = mctrl_gpio_init(&stm32port->port, 0);
> > + if (IS_ERR(stm32port->gpios))
>
> Please add error path: add a clk_disable_unprepare() here, before the
> return.
>
Ok.
Thanks,
Mani
> > + return PTR_ERR(stm32port->gpios);
> > +
> > + /* Both CTS/RTS gpios and "st,hw-flow-ctrl" should not be specified */
> > + if (stm32port->hw_flow_control) {
> > + if (mctrl_gpio_to_gpiod(stm32port->gpios, UART_GPIO_CTS) ||
> > + mctrl_gpio_to_gpiod(stm32port->gpios, UART_GPIO_RTS)) {
> > + dev_err(&pdev->dev, "Conflicting RTS/CTS config\n");
>
> same here
>
> Best Regards,
> Thanks,
> Fabrice
>
> > + return -EINVAL;
> > + }
> > + }
> > +
> > return ret;
> > }
> >
> > diff --git a/drivers/tty/serial/stm32-usart.h b/drivers/tty/serial/stm32-usart.h
> > index db8bf0d4982d..d4c916e78d40 100644
> > --- a/drivers/tty/serial/stm32-usart.h
> > +++ b/drivers/tty/serial/stm32-usart.h
> > @@ -274,6 +274,7 @@ struct stm32_port {
> > bool fifoen;
> > int wakeirq;
> > int rdr_mask; /* receive data register mask */
> > + struct mctrl_gpios *gpios; /* modem control gpios */
> > };
> >
> > static struct stm32_port stm32_ports[STM32_MAX_PORTS];
> >
Powered by blists - more mailing lists