Re: [PATCH 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards.

[Julien Grall <julien.grall@arm.com>]

Hello,

Please include all the relevant maintainers for the code you modified.

On 06/04/2017 20:47, Chris Patterson wrote:
> From: Chris Patterson <pattersonc@ainfosec.com>
>
> Tegra boards feature a NS16550-compatible serial mapped into the MMIO
> space. Add support for its use both as a full-featured serial port and
> as an earlyprintk driver.
>
> This patch adds a quirk for platforms, such as the Tegra, which require
> require the NS16550 Rx timeout interrupt to be enabled for receive to
> function properly. The same quirk is applied in the eqvuialent Linux

s/eqvuialent/equivalent/

> driver [1].
>
> This quirk is selectively enabled for the platform using a new "hw_quirks"
> member with a corresponding set of bitmasks.  The existing quirk,
> dw_usr_bsy was updated to match this approach as well.

This patch would have benefit to be split in smaller ones as you do 
introduce distinct changes in the code.

>
> [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4539c24fe4f92c09ee668ef959d3e8180df619b9
>
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
> ---
>
> changes from rfc:
> - use bitmask for quirks in ns1660, including dw_usr_bsy
>
> ---
>  xen/arch/arm/Rules.mk       |  1 +
>  xen/drivers/char/ns16550.c  | 28 ++++++++++++++++++++++++----
>  xen/include/xen/8250-uart.h |  1 +
>  3 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index 569a0ba..43b32d0 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -44,6 +44,7 @@ EARLY_PRINTK_vexpress       := pl011,0x1c090000
>  EARLY_PRINTK_xgene-mcdivitt := 8250,0x1c021000,2
>  EARLY_PRINTK_xgene-storm    := 8250,0x1c020000,2
>  EARLY_PRINTK_zynqmp         := cadence,0xff000000
> +EARLY_PRINTK_tegra          := 8250,0x70006000,2

The EARLY_PRINTK_* are sorted alphabetically. So please introduce this 
one in the correct place.

Also, I was expecting to see this new earlyprintk documented in 
docs/misc/arm/early-printk.txt.

>
>  ifneq ($(EARLY_PRINTK_$(CONFIG_EARLY_PRINTK)),)
>  EARLY_PRINTK_CFG := $(subst $(comma), ,$(EARLY_PRINTK_$(CONFIG_EARLY_PRINTK)))
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index e4de3b4..1b75e89 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -62,7 +62,7 @@ static struct ns16550 {
>      struct timer resume_timer;
>      unsigned int timeout_ms;
>      bool_t intr_works;
> -    bool_t dw_usr_bsy;
> +    uint8_t hw_quirks;
>  #ifdef CONFIG_HAS_PCI
>      /* PCI card parameters. */
>      bool_t pb_bdf_enable;   /* if =1, pb-bdf effective, port behind bridge */
> @@ -414,6 +414,10 @@ static const struct ns16550_config __initconst uart_config[] =
>  };
>  #endif
>
> +/* Various hardware quirks/features that may be need be enabled per device */
> +#define HW_QUIRKS_DW_USR_BSY         (1<<0)
> +#define HW_QUIRKS_USE_RTOIE          (1<<1)
> +
>  static void ns16550_delayed_resume(void *data);
>
>  static u8 ns_read_reg(struct ns16550 *uart, unsigned int reg)
> @@ -578,7 +582,7 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
>      /* No interrupts. */
>      ns_write_reg(uart, UART_IER, 0);
>
> -    if ( uart->dw_usr_bsy &&
> +    if ( (uart->hw_quirks & HW_QUIRKS_DW_USR_BSY) &&
>           (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
>      {
>          /* DesignWare 8250 detects if LCR is written while the UART is
> @@ -651,12 +655,23 @@ static void ns16550_setup_postirq(struct ns16550 *uart)
>  {
>      if ( uart->irq > 0 )
>      {
> +        u8 ier_value = 0;
> +
>          /* Master interrupt enable; also keep DTR/RTS asserted. */
>          ns_write_reg(uart,
>                       UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS);
>
>          /* Enable receive interrupts. */
> -        ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
> +        ier_value = UART_IER_ERDAI;
> +
> +        /*
> +         * If we're on a platform that needs Rx timeouts enabled, also
> +         * set Rx TimeOut Interrupt Enable (RTOIE).
> +         */
> +        if ( uart->hw_quirks & HW_QUIRKS_USE_RTOIE )
> +          ier_value |= UART_IER_RTOIE;
> +
> +        ns_write_reg(uart, UART_IER, ier_value);
>      }
>
>      if ( uart->irq >= 0 )
> @@ -1271,7 +1286,11 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
>          return -EINVAL;
>      uart->irq = res;
>
> -    uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
> +    if ( dt_device_is_compatible(dev, "snps,dw-apb-uart") )
> +        uart->hw_quirks |= HW_QUIRKS_DW_USR_BSY;
> +
> +    if ( dt_device_is_compatible(dev, "nvidia,tegra20-uart") )
> +        uart->hw_quirks |= HW_QUIRKS_USE_RTOIE;
>
>      uart->vuart.base_addr = uart->io_base;
>      uart->vuart.size = uart->io_size;
> @@ -1292,6 +1311,7 @@ static const struct dt_device_match ns16550_dt_match[] __initconst =
>      DT_MATCH_COMPATIBLE("ns16550"),
>      DT_MATCH_COMPATIBLE("ns16550a"),
>      DT_MATCH_COMPATIBLE("snps,dw-apb-uart"),
> +    DT_MATCH_COMPATIBLE("nvidia,tegra20-uart"),
>      { /* sentinel */ },
>  };
>
> diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
> index c6b62c8..2ad0ee6 100644
> --- a/xen/include/xen/8250-uart.h
> +++ b/xen/include/xen/8250-uart.h
> @@ -41,6 +41,7 @@
>  #define UART_IER_ETHREI   0x02    /* tx reg. empty        */
>  #define UART_IER_ELSI     0x04    /* rx line status       */
>  #define UART_IER_EMSI     0x08    /* MODEM status         */
> +#define UART_IER_RTOIE    0x10    /* rx timeout           */
>
>  /* Interrupt Identificatiegister */
>  #define UART_IIR_NOINT    0x01    /* no interrupt pending */
>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel