diff mbox

[24/25,v6] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree

Message ID 1500296815-10243-25-git-send-email-bhupinder.thakur@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Bhupinder Thakur July 17, 2017, 1:06 p.m. UTC
The SBSA UART node format is as specified in
Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt and given below:

ARM SBSA defined generic UART
------------------------------
This UART uses a subset of the PL011 registers and consequently lives
in the PL011 driver. It's baudrate and other communication parameters
cannot be adjusted at runtime, so it lacks a clock specifier here.

Required properties:
- compatible: must be "arm,sbsa-uart"
- reg: exactly one register range
- interrupts: exactly one interrupt specifier
- current-speed: the (fixed) baud rate set by the firmware

Currently the baud rate of 115200 has been selected as a default value,
which is one of the valid baud rate setttings. Higher baud rate was
selected since an emulated pl011 can support any valid baud rate without
any limitation of the hardware.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

 tools/libxl/libxl_arm.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

Comments

Julien Grall July 18, 2017, 11:04 a.m. UTC | #1
Hi Bhupinder,

On 17/07/17 14:06, Bhupinder Thakur wrote:
> The SBSA UART node format is as specified in
> Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt and given below:
>
> ARM SBSA defined generic UART
> ------------------------------
> This UART uses a subset of the PL011 registers and consequently lives
> in the PL011 driver. It's baudrate and other communication parameters
> cannot be adjusted at runtime, so it lacks a clock specifier here.
>
> Required properties:
> - compatible: must be "arm,sbsa-uart"
> - reg: exactly one register range
> - interrupts: exactly one interrupt specifier
> - current-speed: the (fixed) baud rate set by the firmware
>
> Currently the baud rate of 115200 has been selected as a default value,
> which is one of the valid baud rate setttings. Higher baud rate was

s/setttings/settings/

> selected since an emulated pl011 can support any valid baud rate without
> any limitation of the hardware.
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> ---
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
>
>  tools/libxl/libxl_arm.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index e3e5791..9eee50c 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -44,10 +44,22 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>      uint32_t nr_spis = 0;
>      unsigned int i;
>
> +    /*
> +     * If pl011 vuart is enabled then increment the nr_spis to allow allocation
> +     * of SPI VIRQ for pl011.
> +     */
> +    if (d_config->b_info.arch_arm.vuart)

vuart is an enum. Please follow what we did for the gic_version, i.e 
using a switch or at least checking the value of vuart.

> +        nr_spis += (GUEST_VPL011_SPI - 32) + 1;
> +
>      for (i = 0; i < d_config->b_info.num_irqs; i++) {
>          uint32_t irq = d_config->b_info.irqs[i];
>          uint32_t spi;
>
> +        if (d_config->b_info.arch_arm.vuart && (irq == GUEST_VPL011_SPI)) {
> +            LOG(ERROR, "Physical IRQ %u conflicting with pl011 SPI\n", irq);
> +            return ERROR_FAIL;
> +        }

This limitation looks a bit random. Can we have a TODO in the code and 
the commit message to explain the reason of this limitation?

> +
>          if (irq < 32)
>              continue;
>
> @@ -130,9 +142,10 @@ static struct arch_info {
>      const char *guest_type;
>      const char *timer_compat;
>      const char *cpu_compat;
> +    const char *uart_compat;
>  } arch_info[] = {
> -    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15" },
> -    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" },
> +    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15", "arm,sbsa-uart" },
> +    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart" },
>  };
>
>  /*
> @@ -590,6 +603,38 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>      return 0;
>  }
>
> +static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
> +                                 const struct arch_info *ainfo,
> +                                 struct xc_dom_image *dom)
> +{
> +    int res;
> +    gic_interrupt intr;
> +
> +    res = fdt_begin_node(fdt, "sbsa-pl011");
> +    if (res) return res;
> +
> +    res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat);

NIT: uart_compat is exactly the same for AArch64 and AArch32. So you can 
directly use "arm,sbsa-uart" here.

> +    if (res) return res;
> +
> +    res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
> +                            1,
> +                            GUEST_PL011_BASE, GUEST_PL011_SIZE);
> +    if (res) return res;
> +
> +    set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> +
> +    res = fdt_property_interrupts(gc, fdt, &intr, 1);
> +    if (res) return res;
> +
> +    /* Use a default baud rate of 115200. */
> +    fdt_property_u32(fdt, "current-speed", 115200);
> +
> +    res = fdt_end_node(fdt);
> +    if (res) return res;
> +
> +    return 0;
> +}
> +
>  static const struct arch_info *get_arch_info(libxl__gc *gc,
>                                               const struct xc_dom_image *dom)
>  {
> @@ -889,6 +934,9 @@ next_resize:
>          FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) );
>          FDT( make_hypervisor_node(gc, fdt, vers) );
>
> +        if (info->arch_arm.vuart)
> +            FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
> +
>          if (pfdt)
>              FDT( copy_partial_fdt(gc, fdt, pfdt) );
>
>

Cheers,
Bhupinder Thakur July 21, 2017, 6:35 a.m. UTC | #2
Hi Julien,


>> +    /*
>> +     * If pl011 vuart is enabled then increment the nr_spis to allow
>> allocation
>> +     * of SPI VIRQ for pl011.
>> +     */
>> +    if (d_config->b_info.arch_arm.vuart)
>
>
> vuart is an enum. Please follow what we did for the gic_version, i.e using a
> switch or at least checking the value of vuart.

ok. I will check the value against the specific enum value.

>
>> +        nr_spis += (GUEST_VPL011_SPI - 32) + 1;
>> +
>>      for (i = 0; i < d_config->b_info.num_irqs; i++) {
>>          uint32_t irq = d_config->b_info.irqs[i];
>>          uint32_t spi;
>>
>> +        if (d_config->b_info.arch_arm.vuart && (irq == GUEST_VPL011_SPI))
>> {
>> +            LOG(ERROR, "Physical IRQ %u conflicting with pl011 SPI\n",
>> irq);
>> +            return ERROR_FAIL;
>> +        }
>
>
> This limitation looks a bit random. Can we have a TODO in the code and the
> commit message to explain the reason of this limitation?

This check was added to make sure that the user specified irqs do not
conflict with the vpl011 irq since it takes up
one irq number in the SPI range.
>
>
>> +
>>          if (irq < 32)
>>              continue;
>>
>> @@ -130,9 +142,10 @@ static struct arch_info {
>>      const char *guest_type;
>>      const char *timer_compat;
>>      const char *cpu_compat;
>> +    const char *uart_compat;
>>  } arch_info[] = {
>> -    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15" },
>> -    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" },
>> +    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15",
>> "arm,sbsa-uart" },
>> +    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart"
>> },
>>  };
>>
>>  /*
>> @@ -590,6 +603,38 @@ static int make_hypervisor_node(libxl__gc *gc, void
>> *fdt,
>>      return 0;
>>  }
>>
>> +static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
>> +                                 const struct arch_info *ainfo,
>> +                                 struct xc_dom_image *dom)
>> +{
>> +    int res;
>> +    gic_interrupt intr;
>> +
>> +    res = fdt_begin_node(fdt, "sbsa-pl011");
>> +    if (res) return res;
>> +
>> +    res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat);
>
>
> NIT: uart_compat is exactly the same for AArch64 and AArch32. So you can
> directly use "arm,sbsa-uart" here.

ok. I will use the string directly and remove uart_compat field.

Regards,
Bhupinder
diff mbox

Patch

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index e3e5791..9eee50c 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -44,10 +44,22 @@  int libxl__arch_domain_prepare_config(libxl__gc *gc,
     uint32_t nr_spis = 0;
     unsigned int i;
 
+    /*
+     * If pl011 vuart is enabled then increment the nr_spis to allow allocation
+     * of SPI VIRQ for pl011.
+     */
+    if (d_config->b_info.arch_arm.vuart)
+        nr_spis += (GUEST_VPL011_SPI - 32) + 1;
+
     for (i = 0; i < d_config->b_info.num_irqs; i++) {
         uint32_t irq = d_config->b_info.irqs[i];
         uint32_t spi;
 
+        if (d_config->b_info.arch_arm.vuart && (irq == GUEST_VPL011_SPI)) {
+            LOG(ERROR, "Physical IRQ %u conflicting with pl011 SPI\n", irq);
+            return ERROR_FAIL;
+        }
+
         if (irq < 32)
             continue;
 
@@ -130,9 +142,10 @@  static struct arch_info {
     const char *guest_type;
     const char *timer_compat;
     const char *cpu_compat;
+    const char *uart_compat;
 } arch_info[] = {
-    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15" },
-    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" },
+    {"xen-3.0-armv7l",  "arm,armv7-timer", "arm,cortex-a15", "arm,sbsa-uart" },
+    {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart" },
 };
 
 /*
@@ -590,6 +603,38 @@  static int make_hypervisor_node(libxl__gc *gc, void *fdt,
     return 0;
 }
 
+static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
+                                 const struct arch_info *ainfo,
+                                 struct xc_dom_image *dom)
+{
+    int res;
+    gic_interrupt intr;
+
+    res = fdt_begin_node(fdt, "sbsa-pl011");
+    if (res) return res;
+
+    res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat);
+    if (res) return res;
+
+    res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
+                            1,
+                            GUEST_PL011_BASE, GUEST_PL011_SIZE);
+    if (res) return res;
+
+    set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
+
+    res = fdt_property_interrupts(gc, fdt, &intr, 1);
+    if (res) return res;
+
+    /* Use a default baud rate of 115200. */
+    fdt_property_u32(fdt, "current-speed", 115200);
+
+    res = fdt_end_node(fdt);
+    if (res) return res;
+
+    return 0;
+}
+
 static const struct arch_info *get_arch_info(libxl__gc *gc,
                                              const struct xc_dom_image *dom)
 {
@@ -889,6 +934,9 @@  next_resize:
         FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) );
         FDT( make_hypervisor_node(gc, fdt, vers) );
 
+        if (info->arch_arm.vuart)
+            FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
+
         if (pfdt)
             FDT( copy_partial_fdt(gc, fdt, pfdt) );