[Xen-devel,11/12,v3] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree

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

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---

Changes since v2:
- Currently device discovery using ACPI is not supported.
- Dropped the reviewed-by tag by Stefano as there were some IRQ related changes
  done later.

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

On Wed, May 10, 2017 at 08:05:17PM +0530, 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
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>

I will leave this to arm folks.

On Wed, 10 May 2017, 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
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> 
> Changes since v2:
> - Currently device discovery using ACPI is not supported.
> - Dropped the reviewed-by tag by Stefano as there were some IRQ related changes
>   done later.
> 
>  tools/libxl/libxl_arm.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index d842d88..f88ef0d 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -44,10 +44,23 @@ 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.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.vuart && (irq == GUEST_VPL011_SPI))
> +        {

code style for libxl is

   if () {

sorry about all the different code styles, it's a mess

Aside from that:

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


> +            LOG(ERROR, "Physical IRQ %u conflicting with pl011 SPI\n", irq);
> +            return ERROR_FAIL;
> +        }
> +
>          if (irq < 32)
>              continue;
>  
> @@ -130,9 +143,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 +604,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 +935,9 @@ next_resize:
>          FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) );
>          FDT( make_hypervisor_node(gc, fdt, vers) );
>  
> +        if (info->vuart)
> +            FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
> +
>          if (pfdt)
>              FDT( copy_partial_fdt(gc, fdt, pfdt) );
>  
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
>

Hi Bhupinder,

On 10/05/17 15:35, 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
>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>
> Changes since v2:
> - Currently device discovery using ACPI is not supported.
> - Dropped the reviewed-by tag by Stefano as there were some IRQ related changes
>   done later.
>
>  tools/libxl/libxl_arm.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index d842d88..f88ef0d 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -44,10 +44,23 @@ 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.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.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 +143,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 +604,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);

Again, please explain in the commit message why 115200. How this is 
going to affect the driver?

> +
> +    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 +935,9 @@ next_resize:
>          FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) );
>          FDT( make_hypervisor_node(gc, fdt, vers) );
>
> +        if (info->vuart)
> +            FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
> +
>          if (pfdt)
>              FDT( copy_partial_fdt(gc, fdt, pfdt) );
>
>

Cheers,