Re: [PATCH 4/4] xen/arm: pl011: Add SBSA UART device-tree support

From: Stefano Stabellini <sstabellini@kernel.org>
To: Michal Orzel <michal.orzel@amd.com>
Cc: xen-devel@lists.xenproject.org,
	 Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>,
	 Bertrand Marquis <bertrand.marquis@arm.com>,
	 Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH 4/4] xen/arm: pl011: Add SBSA UART device-tree support
Date: Wed, 14 Jun 2023 18:50:33 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2306141850250.897208@ubuntu-linux-20-04-desktop> (raw)
In-Reply-To: <20230607092727.19654-5-michal.orzel@amd.com>

On Wed, 7 Jun 2023, Michal Orzel wrote:
> We already have all the bits necessary in PL011 driver to support SBSA
> UART thanks to commit 032ea8c736d10f02672863c6e369338f948f7ed8 that
> enabled it for ACPI. Plumb in the remaining part for device-tree boot:
>  - add arm,sbsa-uart compatible to pl011_dt_match (no need for a separate
>    struct and DT_DEVICE_START as SBSA is a subset of PL011),
>  - from pl011_dt_uart_init(), check for SBSA UART compatible to determine
>    the UART type in use.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> After this series the last thing not to be in spec for newer UARTs (well,
> for rev1.5 introduced in 2007 I believe) is incorrect FIFO size. We hardcode it
> to 16 but in r1.5 it is 32. This requires checking the peripheral ID register
> or using arm,primecell-periphid dt property for overriding HW. Something to
> be done in the future (at least 16 is not harmful).
> ---
>  xen/drivers/char/pl011.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> index 403b1ac06551..f7bf3ad117af 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -286,7 +286,7 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
>      int res;
>      paddr_t addr, size;
>      uint32_t io_width;
> -    bool mmio32 = false;
> +    bool mmio32 = false, sbsa;
>  
>      if ( strcmp(config, "") )
>      {
> @@ -320,7 +320,9 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
>          }
>      }
>  
> -    res = pl011_uart_init(res, addr, size, false, mmio32);
> +    sbsa = dt_device_is_compatible(dev, "arm,sbsa-uart");
> +
> +    res = pl011_uart_init(res, addr, size, sbsa, mmio32);
>      if ( res < 0 )
>      {
>          printk("pl011: Unable to initialize\n");
> @@ -335,6 +337,8 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
>  static const struct dt_device_match pl011_dt_match[] __initconst =
>  {
>      DT_MATCH_COMPATIBLE("arm,pl011"),
> +    /* No need for a separate struct as SBSA UART is a subset of PL011 */
> +    DT_MATCH_COMPATIBLE("arm,sbsa-uart"),
>      { /* sentinel */ },
>  };
>  
> -- 
> 2.25.1
>