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.cherech...@nxp.com>
>>
>> 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.cherech...@nxp.com>
>> Signed-off-by: Peter van der Perk <peter.vander.p...@nxp.com>
>> ---
>> 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.cherech...@nxp.com>
>> + * 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.cherech...@nxp.com>
>> + * 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
