Re: [PATCH v1 1/5] xen/arm: Add NXP LINFlexD UART Driver

[Andrei Cherechesu <andrei.cherechesu@xxxxxxxxxxx>]

Hi, Julien, and thank you for the review!

On 11/09/2024 00:55, Julien Grall wrote:
> Hi,
>
> On 10/09/2024 15:34, Andrei Cherechesu (OSS) wrote:
>> From: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
>>
>> The LINFlexD UART is an UART controller available on NXP S32
>> processors family targeting automotive (for example: S32G2, S32G3,
>> S32R).
>>
>> S32G3 Reference Manual:
>> https://www.nxp.com/webapp/Download?colCode=RMS32G3.
>
> It looks like you need to create an account. Is there any public version of 
> the specification?
>
Unfortunately, the Reference Manual requires registration on NXP website, as 
per company policy.
The only public resource I can provide is the Data Sheet: 
https://www.nxp.com/docs/en/data-sheet/S32G3.pdf.
>>
>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
>> Signed-off-by: Peter van der Perk <peter.vander.perk@xxxxxxx>
>> ---
>>   xen/arch/arm/include/asm/linflex-uart.h |  62 ++++
>>   xen/drivers/char/Kconfig                |   8 +
>>   xen/drivers/char/Makefile               |   1 +
>>   xen/drivers/char/linflex-uart.c         | 365 ++++++++++++++++++++++++
>>   4 files changed, 436 insertions(+)
>>   create mode 100644 xen/arch/arm/include/asm/linflex-uart.h
>>   create mode 100644 xen/drivers/char/linflex-uart.c
>>
>> diff --git a/xen/arch/arm/include/asm/linflex-uart.h 
>> b/xen/arch/arm/include/asm/linflex-uart.h
>> new file mode 100644
>> index 0000000000..62dc54d155
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/linflex-uart.h
>> @@ -0,0 +1,62 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>
> The identifier GPL-2.0 was deprecated (see LICENSES/GPL-2.0). The new tag 
> should be GPL-2.0-only. The resulting license is the same.
Will fix in v2.
>
>> +/*
>> + * xen/arch/arm/include/asm/linflex-uart.h
>> + *
>> + * Common constant definition between early printk and the UART driver
>> + * for NXP LINFlexD UART.
>> + *
>> + * Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
>> + * Copyright 2018, 2021, 2024 NXP
>> + */
>> +
>> +#ifndef __ASM_ARM_LINFLEX_UART_H
>> +#define __ASM_ARM_LINFLEX_UART_H
>> +
>> +/* 32-bit register offsets */
>> +#define LINCR1          (0x0)
>> +#define LINIER          (0x4)
>> +#define LINSR           (0x8)
>> +#define UARTCR          (0x10)
>> +#define UARTSR          (0x14)
>> +#define LINFBRR         (0x24)
>> +#define LINIBRR         (0x28)
>> +#define BDRL            (0x38)
>> +#define BDRM            (0x3C)
>> +#define UARTPTO         (0x50)
>> +
>> +#define LINCR1_INIT     BIT(0, U)
>> +#define LINCR1_MME      BIT(4, U)
>> +#define LINCR1_BF       BIT(7, U)
>> +
>> +#define LINSR_LINS      GENMASK(15, 12)
>> +#define LINSR_LINS_INIT BIT(12, U)
>> +
>> +#define LINIER_DRIE     BIT(2, U)
>> +#define LINIER_DTIE     BIT(1, U)
>> +
>> +#define UARTCR_UART     BIT(0, U)
>> +#define UARTCR_WL0      BIT(1, U)
>> +#define UARTCR_PC0      BIT(3, U)
>> +#define UARTCR_TXEN     BIT(4, U)
>> +#define UARTCR_RXEN     BIT(5, U)
>> +#define UARTCR_PC1      BIT(6, U)
>> +#define UARTCR_TFBM     BIT(8, U)
>> +#define UARTCR_RFBM     BIT(9, U)
>> +#define UARTCR_RDFLRFC  GENMASK(12, 10)
>> +#define UARTCR_TDFLTFC  GENMASK(15, 13)
>> +#define UARTCR_ROSE     BIT(23, U)
>> +#define UARTCR_OSR      GENMASK(27, 24)
>> +
>> +#define UARTSR_DTFTFF   BIT(1, U)
>> +#define UARTSR_DRFRFE   BIT(2, U)
>> +
>> +#endif /* __ASM_ARM_LINFLEX_UART_H */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
>> index 3f836ab301..e175d07c02 100644
>> --- a/xen/drivers/char/Kconfig
>> +++ b/xen/drivers/char/Kconfig
>> @@ -13,6 +13,14 @@ config HAS_CADENCE_UART
>>         This selects the Xilinx Zynq Cadence UART. If you have a Xilinx Zynq
>>         based board, say Y.
>>   +config HAS_LINFLEX
>> +    bool "NXP LINFlexD UART driver"
>> +    default y
>> +    depends on ARM_64
>> +    help
>> +      This selects the NXP LINFlexD UART. If you have an NXP S32G or S32R
>> +      based board, say Y.
>> +
>>   config HAS_IMX_LPUART
>>       bool "i.MX LPUART driver"
>>       default y
>> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
>> index e7e374775d..d3b987da1d 100644
>> --- a/xen/drivers/char/Makefile
>> +++ b/xen/drivers/char/Makefile
>> @@ -10,6 +10,7 @@ obj-$(CONFIG_HAS_SCIF) += scif-uart.o
>>   obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
>>   obj-$(CONFIG_XHCI) += xhci-dbc.o
>>   obj-$(CONFIG_HAS_IMX_LPUART) += imx-lpuart.o
>> +obj-$(CONFIG_HAS_LINFLEX) += linflex-uart.o
>>   obj-$(CONFIG_ARM) += arm-uart.o
>>   obj-y += serial.o
>>   obj-$(CONFIG_XEN_GUEST) += xen_pv_console.o
>> diff --git a/xen/drivers/char/linflex-uart.c 
>> b/xen/drivers/char/linflex-uart.c
>> new file mode 100644
>> index 0000000000..4ca8f732ae
>> --- /dev/null
>> +++ b/xen/drivers/char/linflex-uart.c
>> @@ -0,0 +1,365 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>
> Ditto.
Will fix in v2.
>
>
>> +/*
>> + * xen/drivers/char/linflex-uart.c
>> + *
>> + * Driver for NXP LINFlexD UART.
>> + *
>> + * Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
>> + * Copyright 2018, 2021-2022, 2024 NXP
>> + */
>> +
>> +#include <xen/config.h>
>> +#include <xen/console.h>
>> +#include <xen/errno.h>
>> +#include <xen/serial.h>
>
> In Xen, we tend to order the include alphabetically within the same 
> directory. So this wants to be after xen/mm.h.
Right, will address in v2.
>
>
>> +#include <xen/init.h>
>> +#include <xen/irq.h>
>> +#include <xen/mm.h>
>> +#include <asm/device.h>
>> +#include <asm/io.h>
>> +#include <asm/linflex-uart.h>
>> +
>> +#define LINFLEX_CLK_FREQ        (125000000)
>> +#define LINFLEX_BAUDRATE        (115200)
>> +#define LINFLEX_LDIV_MULTIPLIER (16)
>> +
>> +static struct linflex_uart {
>> +    uint32_t baud, clock_hz;
>> +    uint32_t irq;
>> +    char __iomem *regs;
>> +    struct irqaction irqaction;
>> +    struct vuart_info vuart;
>> +} linflex_com;
>> +
>> +static uint32_t linflex_uart_readl(struct linflex_uart *uart, uint32_t off)
>> +{
>> +    return readl(uart->regs + off);
>> +}
>> +
>> +static void linflex_uart_writel(struct linflex_uart *uart, uint32_t off,
>> +                                uint32_t val)
>> +{
>> +    writel(val, uart->regs + off);
>> +}
>> +
>> +static void linflex_uart_writeb(struct linflex_uart *uart, uint32_t off,
>> +                                uint8_t val)
>> +{
>> +    writeb(val, uart->regs + off);
>> +}
>> +
>> +static uint32_t linflex_uart_get_osr(uint32_t uartcr)
>> +{
>> +    return (uartcr & UARTCR_OSR) >> 24;
>
> Please provide a define for 24. This would also make easier to correlate with 
> UARTCR_OSR.
Will address in v2.
>
>
>> +}
>> +
>> +static uint32_t linflex_uart_tx_fifo_mode(struct linflex_uart *uart)
>
> AFAICT, UARTCR_TFBM is one-bit. So should this return a bool?
Yes, it is one bit and I agree a bool would fit better. Will address in v2.
>
>
>> +{
>> +    return linflex_uart_readl(uart, UARTCR) & UARTCR_TFBM;
>> +}
>> +
>> +static uint32_t linflex_uart_rx_fifo_mode(struct linflex_uart *uart)
>
> Same here.
Will address in v2.
>
>> +{
>> +    return linflex_uart_readl(uart, UARTCR) & UARTCR_RFBM;
>> +}
>> +
>> +static uint32_t linflex_uart_ldiv_multiplier(struct linflex_uart *uart)
>> +{
>> +    uint32_t uartcr, mul = LINFLEX_LDIV_MULTIPLIER;
>> +
>> +    uartcr = linflex_uart_readl(uart, UARTCR);
>> +    if ( uartcr & UARTCR_ROSE )
>> +        mul = linflex_uart_get_osr(uartcr);
>> +
>> +    return mul;
>> +}
>> +
>> +static void linflex_uart_flush(struct serial_port *port)
>> +{
>> +    struct linflex_uart *uart = port->uart;
>
> Above, youa re using tab hard but above you use soft tab. Is the code 
> intended to follow Xen coding style? If so, you want to use soft tab.
Will fix in v2.
>
>
>> +
>> +    if ( linflex_uart_tx_fifo_mode(uart) )
>> +        while ( linflex_uart_readl(uart, UARTCR) & UARTCR_TDFLTFC );
>> +            cpu_relax();
>
> The indentation is really confusing here. It leads to think that cpu_relax() 
> should be part of while() but you are using ';'. I guess you really intended 
> to have cpu_relax() within the while loop?
Indeed, the intention was to have cpu_relax() called within the while loop, but 
functionally, this variant works almost the same. Thank you for spotting the 
mistake!
>
>
>> +
>> +    if ( linflex_uart_rx_fifo_mode(uart) )
>> +        while ( linflex_uart_readl(uart, UARTCR) & UARTCR_RDFLRFC );
>> +            cpu_relax();
>
> Same here.
>
>> +}
>> +
>> +static void __init linflex_uart_init_preirq(struct serial_port *port)
>> +{
>> +    struct linflex_uart *uart = port->uart;
>> +    uint32_t ibr, fbr, divisr, dividr, ctrl;
>> +
>> +    /* Disable RX/TX before init mode */
>> +    ctrl = linflex_uart_readl(uart, UARTCR);
>> +    ctrl &= ~(UARTCR_RXEN | UARTCR_TXEN);
>> +    linflex_uart_writel(uart, UARTCR, ctrl);
>> +
>> +    /*
>> +    * Smoothen the transition from early_printk by waiting
>> +    * for all pending characters to transmit
>> +    */
>
> The indentation for comment in Xen is:
>
> /*
>  * Foor
>  * Bar
>  */
>
Will fix in v2.
>> +    linflex_uart_flush(port);
>> +
>> +    /* Init mode */
>> +    ctrl = LINCR1_INIT;
>> +    linflex_uart_writel(uart, LINCR1, ctrl);
>> +
>> +    /* Waiting for init mode entry */
>> +    while ( (linflex_uart_readl(uart, LINSR) & LINSR_LINS) != 
>> LINSR_LINS_INIT )
>> +        cpu_relax();
>> +
>> +    /* Set Master Mode */
>> +    ctrl |= LINCR1_MME;
>> +    linflex_uart_writel(uart, LINCR1, ctrl);
>> +
>> +    /* Provide data bits, parity, stop bit, etc */
>> +    divisr = uart->clock_hz;
>> +    dividr = (uint32_t)(uart->baud * linflex_uart_ldiv_multiplier(uart));
>> +
>> +    ibr = (uint32_t)(divisr / dividr);
>> +    fbr = (uint32_t)((divisr % dividr) * 16 / dividr) & 0xF;
>
> On the 3 lines above, why do you need to cast to 32-bit? Is this because the 
> result is 64-bit? If so, why do you need to ignore the top bits?
>
Indeed, the casts are not needed. The maximum baud rate supported by LINFlexD 
is 2000000 bps, and the maximum value returned by 
linflex_uart_ldiv_multiplier() is 16.
Thus, "dividr" should never overflow (32000000 max value) and neither should 
the "fbr" computation. In v2, I will remove the casts and upper bound the baud 
rate to the maximum
supported value, if that's ok for you.
It would be also nice having a generic CONFIG_BAUDRATE (or similar) set by each 
platform, similar to U-Boot, to make this configurable.
>>
>> +    linflex_uart_writel(uart, LINIBRR, ibr);
>> +    linflex_uart_writel(uart, LINFBRR, fbr);
>> +
>> +    /* Set preset timeout register value */
>> +    linflex_uart_writel(uart, UARTPTO, 0xF);
>> +
>> +    /* Setting UARTCR[UART] bit is required for writing other bits in 
>> UARTCR */
>> +    linflex_uart_writel(uart, UARTCR, UARTCR_UART);
>> +
>> +    /* 8 bit data, no parity, UART mode, Buffer mode */
>> +    linflex_uart_writel(uart, UARTCR, UARTCR_PC1 | UARTCR_PC0 | UARTCR_WL0 |
>> +                        UARTCR_UART);
>> +
>> +    /* end init mode */
>> +    ctrl = linflex_uart_readl(uart, LINCR1);
>> +    ctrl &= ~LINCR1_INIT;
>> +    linflex_uart_writel(uart, LINCR1, ctrl);
>> +
>> +    /* Enable RX/TX after exiting init mode */
>> +    ctrl = linflex_uart_readl(uart, UARTCR);
>> +    ctrl |= UARTCR_RXEN | UARTCR_TXEN;
>> +    linflex_uart_writel(uart, UARTCR, ctrl);
>> +}
>> +
>> +static void linflex_uart_interrupt(int irq, void *data)
>> +{
>> +    struct serial_port *port = data;
>> +    struct linflex_uart *uart = port->uart;
>> +    uint32_t sts;
>> +
>> +    sts = linflex_uart_readl(uart, UARTSR);
>> +
>> +    if ( sts & UARTSR_DRFRFE )
>> +        serial_rx_interrupt(port);
>> +
>> +    if ( sts & UARTSR_DTFTFF )
>> +        serial_tx_interrupt(port);
>> +}
>> +
>> +static void __init linflex_uart_init_postirq(struct serial_port *port)
>> +{
>> +    struct linflex_uart *uart = port->uart;
>> +    uint32_t temp;
>> +
>> +    uart->irqaction.handler = linflex_uart_interrupt;
>> +    uart->irqaction.name = "linflex_uart";
>> +    uart->irqaction.dev_id = port;
>> +
>> +    if ( setup_irq(uart->irq, 0, &uart->irqaction) != 0 )
>> +    {
>> +        dprintk(XENLOG_ERR, "Failed to allocate linflex_uart IRQ %d\n",
>> +                uart->irq);
>
> NIT: This should only be called once during boot. So I would consider to use 
> printk() so it can be printed in production.
Will fix in v2.
>
>> +        return;
>> +    }
>> +
>> +    /* Enable interrupts */
>> +    temp = linflex_uart_readl(uart, LINIER);
>> +    temp |= (LINIER_DRIE | LINIER_DTIE);
>> +    linflex_uart_writel(uart, LINIER, temp);
>> +    dprintk(XENLOG_DEBUG, "IRQ %d enabled\n", uart->irq);
>
> Same here.
Will fix in v2.
>
>> +}
>> +
>> +static int linflex_uart_tx_ready(struct serial_port *port)
>> +{
>> +    struct linflex_uart *uart = port->uart;
>> +
>> +    if ( linflex_uart_tx_fifo_mode(uart) )
>> +        return (linflex_uart_readl(uart, UARTSR) & UARTSR_DTFTFF) == 0 ? 1 
>> : 0;
>> +
>> +    /*
>> +    * Buffer Mode => TX is waited to be ready after sending a char,
>> +    * so we can assume it is always ready before.
>> +    */
>
> Coding style. See above how it should be done for multi-line comments.
Will fix in v2.
>
>> +    return 1;
>> +}
>> +
>> +static void linflex_uart_putc(struct serial_port *port, char c)
>> +{
>> +    struct linflex_uart *uart = port->uart;
>> +    uint32_t uartsr;
>> +
>> +    if ( c == '\n' )
>> +        linflex_uart_putc(port, '\r');
>> +
>> +    linflex_uart_writeb(uart, BDRL, c);
>> +
>> +    /* Buffer Mode */
>> +    if ( !linflex_uart_tx_fifo_mode(uart) )
>> +    {
>> +        while ( (linflex_uart_readl(uart, UARTSR) & UARTSR_DTFTFF) == 0 )
>> +                cpu_relax();
>> +
>> +        uartsr = linflex_uart_readl(uart, UARTSR) | (UARTSR_DTFTFF);
>> +        linflex_uart_writel(uart, UARTSR, uartsr);
>> +    }
>> +}
>> +
>> +static int linflex_uart_getc(struct serial_port *port, char *pc)
>> +{
>> +    struct linflex_uart *uart = port->uart;
>> +    uint32_t ch, uartsr, rx_fifo_mode;
>> +
>> +    rx_fifo_mode = linflex_uart_rx_fifo_mode(uart);
>> +
>> +    if ( rx_fifo_mode )
>> +        while ( linflex_uart_readl(uart, UARTSR) & UARTSR_DRFRFE )
>> +            cpu_relax();
>> +    else
>> +        while ( !(linflex_uart_readl(uart, UARTSR) & UARTSR_DRFRFE) )
>> +            cpu_relax();
>> +
>> +    ch = linflex_uart_readl(uart, BDRM);
>> +    *pc = ch & 0xff;
>> +
>> +    if ( !rx_fifo_mode ) {
>> +        uartsr = linflex_uart_readl(uart, UARTSR) | UARTSR_DRFRFE;
>> +        linflex_uart_writel(uart, UARTSR, uartsr);
>> +    }
>> +
>> +    return 1;
>> +}
>> +
>> +static int __init linflex_uart_irq(struct serial_port *port)
>> +{
>> +    struct linflex_uart *uart = port->uart;
>> +
>> +    return ((uart->irq > 0) ? uart->irq : -1);
>> +}
>> +
>> +static const struct vuart_info *linflex_uart_vuart_info(
>> +                struct serial_port *port)
>> +{
>> +    struct linflex_uart *uart = port->uart;
>
> NIT: You are not modifying uart. So this coul be const.
Sure, but no other driver does this. Actually, this applies to most functions 
in this driver, right?
I can implement this in v2 if you agree to break the pattern.
>
>
>> +
>> +    return &uart->vuart;
>> +}
>> +
>> +static void linflex_uart_start_tx(struct serial_port *port)
>> +{
>> +    struct linflex_uart *uart = port->uart;
>> +    uint32_t temp;
>> +
>> +    temp = linflex_uart_readl(uart, LINIER);
>> +    linflex_uart_writel(uart, LINIER, temp | LINIER_DTIE);
>> +}
>> +
>> +static void linflex_uart_stop_tx(struct serial_port *port)
>> +{
>> +    struct linflex_uart *uart = port->uart;
>> +    uint32_t temp;
>> +
>> +    temp = linflex_uart_readl(uart, LINIER);
>> +    temp &= ~(LINIER_DTIE);
>> +    linflex_uart_writel(uart, LINIER, temp);
>> +}
>> +
>> +static struct uart_driver __read_mostly linflex_uart_driver = {
>> +    .init_preirq = linflex_uart_init_preirq,
>> +    .init_postirq = linflex_uart_init_postirq,
>> +    .tx_ready = linflex_uart_tx_ready,
>> +    .putc = linflex_uart_putc,
>> +    .flush = linflex_uart_flush,
>> +    .getc = linflex_uart_getc,
>> +    .irq = linflex_uart_irq,
>> +    .start_tx = linflex_uart_start_tx,
>> +    .stop_tx = linflex_uart_stop_tx,
>> +    .vuart_info = linflex_uart_vuart_info,
>> +};
>> +
>> +static int __init linflex_uart_init(struct dt_device_node *dev, const void 
>> *data)
>> +{
>> +    const char *config = data;
>> +    struct linflex_uart *uart;
>> +    paddr_t addr, size;
>> +    int res;
>> +
>> +    if ( strcmp(config, "") )
>> +        printk("WARNING: UART configuration is not supported\n");
>> +
>> +    uart = &linflex_com;
>> +
>> +    res = dt_device_get_paddr(dev, 0, &addr, &size);
>> +    if ( res )
>> +    {
>> +        printk("linflex-uart: Unable to retrieve the base address of the 
>> UART\n");
>> +        return res;
>> +    }
>> +
>> +    res = platform_get_irq(dev, 0);
>> +    if ( res < 0 )
>> +    {
>> +        printk("linflex-uart: Unable to retrieve the IRQ\n");
>> +        return -EINVAL;
>> +    }
>> +    uart->irq = res;
>> +
>> +    uart->regs = ioremap_nocache(addr, size);
>> +    if ( !uart->regs )
>> +    {
>> +        printk("linflex-uart: Unable to map the UART memory\n");
>> +        return -ENOMEM;
>> +    }
>> +
>> +    uart->clock_hz = LINFLEX_CLK_FREQ;
>> +    uart->baud = LINFLEX_BAUDRATE;
>> +
>> +    uart->vuart.base_addr = addr;
>> +    uart->vuart.size = size;
>> +    uart->vuart.data_off = BDRL;
>> +    uart->vuart.status_off = UARTSR;
>> +    uart->vuart.status = UARTSR_DTFTFF;
>> +
>> +    /* Register with generic serial driver */
>> +    serial_register_uart(SERHND_DTUART, &linflex_uart_driver, uart);
>> +
>> +    dt_device_set_used_by(dev, DOMID_XEN);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct dt_device_match linflex_uart_dt_compat[] __initconst =
>> +{
>> +    DT_MATCH_COMPATIBLE("nxp,s32g2-linflexuart"),
>> +    DT_MATCH_COMPATIBLE("nxp,s32g3-linflexuart"),
>> +    DT_MATCH_COMPATIBLE("fsl,s32v234-linflexuart"),
>> +    { /* sentinel */ },
>> +};
>> +
>> +DT_DEVICE_START(linflex_uart, "NXP LINFlexD UART", DEVICE_SERIAL)
>> +    .dt_match = linflex_uart_dt_compat,
>> +    .init = linflex_uart_init,
>> +DT_DEVICE_END
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>
> Cheers,
>
Regards,
Andrei Cherechesu