Re: [Xen-devel] [PATCH 09/10 v2] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi Julien,

> On 28/04/17 17:01, 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
>
>
> What is the plan for ACPI support?

We can take that up as a new feature after this patch.

>
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
>> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> ---
>>
>> Changes since v1:
>> - Modified the code to increment nr_spis based on the SPI value reserved
>> for
>>   vpl011.
>> - Added a check to verify that physical irq assigment is not conflicting
>> with
>>   vpl011 SPI.
>
>
> Those 2 changes are usually a call to drop reviewed-by or at least to check
> whether the reviewer (here Stefano) is happy with the changes made.

I will drop the reviewed-by tag in the next patch.

>
>> - Fixed minor indentation issues.
>>
>>  tools/libxl/libxl_arm.c | 54
>> +++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 52 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index d842d88..45a56a8 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -43,11 +43,25 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>  {
>>      uint32_t nr_spis = 0;
>>      unsigned int i;
>> +    bool vpl011_enabled = !strcmp(d_config->b_info.vuart, "pl011");
>> +
>> +    /*
>> +     * If pl011 vuart is enabled then increment the nr_spis to allow
>> allocation
>> +     * of SPI VIRQ for pl011.
>> +     */
>> +    if (vpl011_enabled)
>> +        nr_spis += (GUEST_VPL011_SPI - 32)+1;
>
>
> space before and after +.
>
ok.
>>
>>      for (i = 0; i < d_config->b_info.num_irqs; i++) {
>>          uint32_t irq = d_config->b_info.irqs[i];
>>          uint32_t spi;
>>
>> +        if (vpl011_enabled && irq == GUEST_VPL011_SPI)
>> +        {
>> +            LOG(ERROR, "Physical IRQ %d conflicting with pl011 SPI\n",
>> irq);
>
>
> s/%d/%u/
>
ok.
>
>> +            return ERROR_FAIL;
>> +        }
>> +
>>          if (irq < 32)
>>              continue;
>>
>> @@ -130,9 +144,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 +605,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);
>
>
> Please explain in the commit message why 115200.
>
ok.

Regards,
Bhupinder

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