Re: [Xen-devel] [PATCH 24/25 v6] xen/arm: vpl011: Add a pl011 uart DT node in the guest device tree

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

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

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