| Message ID | 20200416175729.5550-2-mani@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add CTS/RTS gpio support to STM32 UART | expand |
On 4/16/20 7:57 PM, mani@kernel.org wrote: > From: Manivannan Sadhasivam <mani@kernel.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@kernel.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 > + > /* 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. 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 |= ... Do you need modem control interrupts in your case ? 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 ? > + 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. > + 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]; >
Hi, On Fri, Apr 17, 2020 at 03:15:07PM +0200, Fabrice Gasnier wrote: > On 4/16/20 7:57 PM, mani@kernel.org wrote: > > From: Manivannan Sadhasivam <mani@kernel.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@kernel.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]; > >
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; + /* 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) +{ + 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; } + stm32port->gpios = mctrl_gpio_init(&stm32port->port, 0); + if (IS_ERR(stm32port->gpios)) + 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"); + 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];