HI Peter

2015-03-24 19:23 GMT+01:00 Peter Hurley [off-list ref]:

Hi Maxime,

On 03/12/2015 05:55 PM, Maxime Coquelin wrote:

quoted

From: Maxime Coquelin <mcoquelin.stm32@gmail.com>

This drivers adds support to the STM32 USART controller, which is a
standard serial driver.

Comments below.

Thanks for the review, please find my answsers below

quoted

Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
---
 drivers/tty/serial/Kconfig       |  17 +
 drivers/tty/serial/Makefile      |   1 +
 drivers/tty/serial/stm32-usart.c | 695 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |   3 +
 4 files changed, 716 insertions(+)
 create mode 100644 drivers/tty/serial/stm32-usart.c

...

quoted

+static unsigned int stm32_tx_empty(struct uart_port *port)
+{
+     return readl_relaxed(port->membase + USART_SR) & USART_SR_TXE;
+}
+
+static void stm32_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+     /*
+      * This routine is used for seting signals of: DTR, DCD, CTS/RTS
+      * We use USART's hardware for CTS/RTS, so don't need any for that.

If this means that you're enabling autoflow control, then you still need
to respond to the state of TIOCM_RTS, so that both serial core and userspace
can halt input flow.

If the hardware doesn't support separate RTS enable/disable control,
then you need to disable autoRTS when TIOCM_RTS is clear, and re-enable
it when raised.

quoted

+      * Some boards have DTR and DCD implemented using PIO pins,
+      * code to do this should be hooked in here.
+      */
+}
+
+static unsigned int stm32_get_mctrl(struct uart_port *port)
+{
+     /*
+      * This routine is used for geting signals of: DTR, DCD, DSR, RI,
+      * and CTS/RTS
+      */
+     return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
+}
+

...

quoted

+
+static void stm32_set_termios(struct uart_port *port, struct ktermios *termios,
+                         struct ktermios *old)
+{
+     unsigned int baud;
+     u32 usardiv, mantissa, fraction;
+     tcflag_t cflag;
+     u32 cr1, cr2, cr3;
+     unsigned long flags;
+
+     baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk / 16);
+     cflag = termios->c_cflag;
+
+     spin_lock_irqsave(&port->lock, flags);
+
+     /* Stop serial port and reset value */
+     writel_relaxed(0, port->membase + USART_CR1);
+
+     cr1 = USART_CR1_TE | USART_CR1_RE | USART_CR1_UE | USART_CR1_RXNEIE;
+
+     if (cflag & CSTOPB)
+             cr2 = USART_CR2_STOP_2B;
+
+     if (cflag & PARENB) {
+             cr1 |= USART_CR1_PCE;
+             if ((cflag & CSIZE) == CS8)
+                     cr1 |= USART_CR1_M;
+     }
+
+     if (cflag & PARODD)
+             cr1 |= USART_CR1_PS;
+
+     if (cflag & CRTSCTS)
+             cr3 = USART_CR3_RTSE | USART_CR3_CTSE;

If this means autoflow control, then you need to define
throttle()/unthrottle() methods, otherwise the serial core won't
be able to throttle the remote when input buffers are about
to overflow.

And you should only enable the autoCTS and let the serial
core enable autoRTS through set_mctrl(TIOCM_RTS).

Just let me know if you need more info about how to do this.

Ok, let's see if I have well understood.

USART_CR3_RTSE should be set/cleared in set_mctrl(), depending on
TIOCM_RTS  value.
The throttle callback should disable the rx interrupt, and the
unthrottle enable it.
For CTS, it should be enabled in set_termios() if CRTSCTS, as done here.

Am I right?

quoted

+
+     usardiv = (port->uartclk * 25) / (baud * 4);
+     mantissa = (usardiv / 100) << USART_BRR_DIV_M_SHIFT;
+     fraction = DIV_ROUND_CLOSEST((usardiv % 100) * 16, 100);
+     if (fraction & ~USART_BRR_DIV_F_MASK) {
+             fraction = 0;
+             mantissa += (1 << USART_BRR_DIV_M_SHIFT);
+     }
+
+     writel_relaxed(mantissa | fraction, port->membase + USART_BRR);
+
+     uart_update_timeout(port, cflag, baud);
+
+     port->read_status_mask = USART_SR_ORE;
+     if (termios->c_iflag & INPCK)
+             port->read_status_mask |= USART_SR_PE | USART_SR_FE;
+     if (termios->c_iflag & (IGNBRK | BRKINT | PARMRK))
+             port->read_status_mask |= USART_SR_LBD;
+
+     /* Characters to ignore */
+     port->ignore_status_mask = 0;
+     if (termios->c_iflag & IGNPAR)
+             port->ignore_status_mask = USART_SR_PE | USART_SR_FE;
+     if (termios->c_iflag & IGNBRK) {
+             port->ignore_status_mask |= USART_SR_LBD;
+             /*
+              * If we're ignoring parity and break indicators,
+              * ignore overruns too (for real raw support).
+              */
+             if (termios->c_iflag & IGNPAR)
+                     port->ignore_status_mask |= USART_SR_ORE;
+     }
+
+     /*
+      * Ignore all characters if CREAD is not set.
+      */
+     if ((termios->c_cflag & CREAD) == 0)
+             port->ignore_status_mask |= USART_SR_DUMMY_RX;
+
+     writel_relaxed(cr3, port->membase + USART_CR3);
+     writel_relaxed(cr2, port->membase + USART_CR2);
+     writel_relaxed(cr1, port->membase + USART_CR1);
+
+     spin_unlock_irqrestore(&port->lock, flags);
+}
+

...

quoted

+static void stm32_config_port(struct uart_port *port, int flags)
+{
+     if ((flags & UART_CONFIG_TYPE))

Single parens.

Sure.

quoted

+             port->type = PORT_STM32;
+}
+

...

quoted

+
+static int stm32_init_port(struct stm32_port *stm32port,
+                       struct platform_device *pdev)
+{
+     struct uart_port *port = &stm32port->port;
+     struct resource *res;
+
+     port->iotype    = UPIO_MEM;
+     port->flags     = UPF_BOOT_AUTOCONF;
+     port->ops       = &stm32_uart_ops;
+     port->dev       = &pdev->dev;
+     port->irq       = platform_get_irq(pdev, 0);
+
+     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+     port->membase = devm_ioremap_resource(&pdev->dev, res);
+     if (IS_ERR(port->membase))
+             return PTR_ERR(port->membase);
+     port->mapbase = res->start;
+
+     spin_lock_init(&port->lock);
+
+     stm32port->clk = devm_clk_get(&pdev->dev, NULL);
+
+     if (WARN_ON(IS_ERR(stm32port->clk)))

Do you really need a stack trace here? Could this be dev_err() instead?

No I don't, dev_err() will be enough.

quoted

+             return -EINVAL;
+
+     /* ensure that clk rate is correct by enabling the clk */
+     clk_prepare_enable(stm32port->clk);
+     stm32port->port.uartclk = clk_get_rate(stm32port->clk);
+     WARN_ON(stm32port->port.uartclk == 0);

Or here?

Same here.

quoted

+     clk_disable_unprepare(stm32port->clk);
+
+     return 0;
+}
+
+static struct stm32_port *stm32_of_get_stm32_port(struct platform_device *pdev)
+{
+     struct device_node *np = pdev->dev.of_node;
+     int id;
+
+     if (!np)
+             return NULL;
+
+     id = of_alias_get_id(np, STM32_SERIAL_NAME);
+
+     if (id < 0)
+             id = 0;
+
+     if (WARN_ON(id >= STM32_MAX_PORTS))

Or here?

I will return PTR_ERR(-EINVAL); instead.

quoted

+             return NULL;
+
+     stm32_ports[id].port.line = id;
+     return &stm32_ports[id];
+}
+

...

quoted

diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index b212281..e22dee5 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h

@@ -258,4 +258,7 @@
 /* Cris v10 / v32 SoC */
 #define PORT_CRIS    112

+/* STM32 USART */
+#define PORT_STM32   110

Already taken.

You'll want to make sure v4 applies cleanly to Greg's tty-next branch.

Sure, I made a mistake during the rebase conflict resolution.

Thanks,
Maxime

Regards,
Peter Hurley

quoted

+
 #endif /* _UAPILINUX_SERIAL_CORE_H */