| Message ID | 1502095997-31219-25-git-send-email-bhupinder.thakur@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | SBSA UART emulation support in Xen | expand |
On 07/08/17 09:53, 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 settings. Higher baud rate was > selected since an emulated pl011 can support any valid baud rate without > any limitation of the hardware. > > A check is added to ensure that user specified irq does not conflict with > the SPI assgined to vpl011. If there is a conflict then it flags an error. > > 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> > > Changes since v6: > - Added a comment explaining why user specified IRQ should not conflict with vpl011 > SPI. > - Checking the vuart type explicitly against vpl011 enum type. > - Removed uart-compat string and using "arm,sbsa-uart" string directly. > - I have retained the reviewed-by/acked-by tags as these are minor changes. > > tools/libxl/libxl_arm.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c > index a33d3c9..6629852 100644 > --- a/tools/libxl/libxl_arm.c > +++ b/tools/libxl/libxl_arm.c > @@ -43,11 +43,29 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, > { > uint32_t nr_spis = 0; > unsigned int i; > + uint32_t vuart_irq = 0; > + > + /* > + * 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 == LIBXL_VUART_TYPE_SBSA_UART) { > + nr_spis += (GUEST_VPL011_SPI - 32) + 1; > + vuart_irq = GUEST_VPL011_SPI; > + } > > for (i = 0; i < d_config->b_info.num_irqs; i++) { > uint32_t irq = d_config->b_info.irqs[i]; > uint32_t spi; > > + /* > + * The user specified irq should not conflict with the vpl011 irq. > + */ I am sorry but in the changelog you wrote: "Add a comment explaining why user specified IRQ should not conflict with vpl011 SPI". But you still don't explain why... And I still don't see the TODO requested on v6. > + if (irq == vuart_irq) { Hmmm if vUART is not enabled you would compare to 0. And I don't think we should make this assumption in the code. This would also give a random error to the user. It would be better if you introduce a local boolean vuart_enabled to know whether you need to check the conflict. > + LOG(ERROR, "Physical IRQ %u conflicting with pl011 SPI\n", irq); > + return ERROR_FAIL; > + } > + > if (irq < 32) > continue; > > @@ -590,6 +608,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, "arm,sbsa-uart"); > + 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 +939,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 == LIBXL_VUART_TYPE_SBSA_UART) > + FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) ); > + > if (pfdt) > FDT( copy_partial_fdt(gc, fdt, pfdt) ); > > Cheers,
Hi Julien, >> >> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c >> index a33d3c9..6629852 100644 >> --- a/tools/libxl/libxl_arm.c >> +++ b/tools/libxl/libxl_arm.c >> @@ -43,11 +43,29 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, >> { >> uint32_t nr_spis = 0; >> unsigned int i; >> + uint32_t vuart_irq = 0; >> + >> + /* >> + * 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 == LIBXL_VUART_TYPE_SBSA_UART) { >> + nr_spis += (GUEST_VPL011_SPI - 32) + 1; >> + vuart_irq = GUEST_VPL011_SPI; >> + } >> >> for (i = 0; i < d_config->b_info.num_irqs; i++) { >> uint32_t irq = d_config->b_info.irqs[i]; >> uint32_t spi; >> >> + /* >> + * The user specified irq should not conflict with the vpl011 >> irq. >> + */ > > > I am sorry but in the changelog you wrote: "Add a comment explaining why > user specified IRQ should not conflict with vpl011 SPI". But you still don't > explain why... And I still don't see the TODO requested on v6. ok. So should I mention that if there is a conflict then that interrupt would not be received correctly by the guest as user expects certain behaviour on receiving the specified IRQ? Can you please elaborate what is the TODO in this case? > >> + if (irq == vuart_irq) { > > > Hmmm if vUART is not enabled you would compare to 0. And I don't think we > should make this assumption in the code. This would also give a random error > to the user. > I thought that since this is an SPI interrupt it would start with at least 32. > It would be better if you introduce a local boolean vuart_enabled to know > whether you need to check the conflict. ok. I will add a boolean variable. Regards, Bhupinder
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index a33d3c9..6629852 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -43,11 +43,29 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, { uint32_t nr_spis = 0; unsigned int i; + uint32_t vuart_irq = 0; + + /* + * 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 == LIBXL_VUART_TYPE_SBSA_UART) { + nr_spis += (GUEST_VPL011_SPI - 32) + 1; + vuart_irq = GUEST_VPL011_SPI; + } for (i = 0; i < d_config->b_info.num_irqs; i++) { uint32_t irq = d_config->b_info.irqs[i]; uint32_t spi; + /* + * The user specified irq should not conflict with the vpl011 irq. + */ + if (irq == vuart_irq) { + LOG(ERROR, "Physical IRQ %u conflicting with pl011 SPI\n", irq); + return ERROR_FAIL; + } + if (irq < 32) continue; @@ -590,6 +608,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, "arm,sbsa-uart"); + 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 +939,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 == LIBXL_VUART_TYPE_SBSA_UART) + FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) ); + if (pfdt) FDT( copy_partial_fdt(gc, fdt, pfdt) );