HI Peter
2015-03-24 19:23 GMT+01:00 Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>:
> Hi Maxime,
>
> On 03/12/2015 05:55 PM, Maxime Coquelin wrote:
>> From: Maxime Coquelin <mcoquelin.stm32@xxxxxxxxx>
>>
>> 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
>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@xxxxxxxxx>
>> ---
>> 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
>>
...
>> +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.
>
>
>> + * 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;
>> +}
>> +
...
>> +
>> +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?
>
>> +
>> + 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);
>> +}
>> +
...
>> +static void stm32_config_port(struct uart_port *port, int flags)
>> +{
>> + if ((flags & UART_CONFIG_TYPE))
>
> Single parens.
Sure.
>
>> + port->type = PORT_STM32;
>> +}
>> +
...
>> +
>> +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.
>
>> + 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.
>
>> + 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.
>
>> + return NULL;
>> +
>> + stm32_ports[id].port.line = id;
>> + return &stm32_ports[id];
>> +}
>> +
...
>> 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
>
>> +
>> #endif /* _UAPILINUX_SERIAL_CORE_H */
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
