[2/2] ns16550: Add compatible string for Raspberry Pi 4

Commit Message

Stewart Hildebrand July 24, 2019, 2:34 p.m. UTC

Signed-off-by: Stewart Hildebrand <[e-mail]>
---
xen/drivers/char/ns16550.c | 1 +
1 file changed, 1 insertion(+)

Comments

Julien Grall July 24, 2019, 4:11 p.m. UTC | #1

(+ Andre)

Hi Stewart,

On 24/07/2019 15:34, Stewart Hildebrand wrote:
> Signed-off-by: Stewart Hildebrand <[e-mail]>
> 
> ---
> 
> xen/drivers/char/ns16550.c | 1 +
> 
> 1 file changed, 1 insertion(+)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> 
> index e518f2d790..c8d7c9b710 100644
> 
> --- a/xen/drivers/char/ns16550.c
> 
> +++ b/xen/drivers/char/ns16550.c
> 
> @@ -1611,6 +1611,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("brcm,bcm2835-aux-uart"),

A different compatible usually means the UART behaves differently. So your 
commit message should at least explain what are the differences and why this is 
not necessary for Xen.

Looking at your repo xen-rpi4-builder, you are hacking the UART DT node. The new 
properties are not part of the binding for that compatible (see [1]).

While it is inevitable to hack device-tree for a first port, I don't want Xen to 
require unofficial binding.

Instead, those property should be set in the driver when matching 
brcm,bcm2835-aux-uart.

Cheers,

[1] Documentation/devicetree/bindings/serial/brcm,bcm2835-aux-uart.txt

> 
>       { /* sentinel */ },
> 
> };
> 
> -- 
> 
> 2.22.0
>

Andre Przywara July 24, 2019, 4:14 p.m. UTC | #2

On Wed, 24 Jul 2019 14:34:55 +0000
Stewart Hildebrand <[e-mail]> wrote:

Hi,

I am afraid this is not enough. In your repo you hack the DT to contain
the reg-shift and io-width properties, but those are not part of the
"brcm,bcm2835-aux-uart" binding. Using 32-bit accesses is an integral
property of this UART, and since it is only *somewhat* compatible to an
8250 or DW UART, it cannot use that binding. So I had the following
additional hunk in:

@@ -1572,6 +1572,12 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
     if ( uart->reg_width != 1 && uart->reg_width != 4 )
         return -EINVAL;
 
+    if ( dt_device_is_compatible(dev, "brcm,bcm2835-aux-uart") )
+    {
+        uart->reg_width = 4;
+        uart->reg_shift = 2;
+    }
+
     res = platform_get_irq(dev, 0);
     if ( ! res )
         return -EINVAL;

Feel free to use that snippet, that should work with the normal DT.

Cheers,
Andre.

> Signed-off-by: Stewart Hildebrand <[e-mail]>
> ---
> xen/drivers/char/ns16550.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index e518f2d790..c8d7c9b710 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1611,6 +1611,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("brcm,bcm2835-aux-uart"),
>      { /* sentinel */ },
> };
> --
> 2.22.0

Patch

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index e518f2d790..c8d7c9b710 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1611,6 +1611,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("brcm,bcm2835-aux-uart"),
     { /* sentinel */ },
};
--
2.22.0