Re: [Xen-devel] [PATCH 06/11] xen/arm: vpl011: Add a new pl011 uart node in the guest DT in the toolstack

[Julien Grall <julien.grall@xxxxxxx>]

Hi Konrad,

On 03/03/2017 09:03 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Feb 21, 2017 at 04:56:03PM +0530, Bhupinder Thakur wrote:
> > Add a new pl011 uart node
> >     - Get the pl011 spi virq from Xen using a hvm call
> >     - Add a new device tree node in the guest DT for SBSA pl011 uart containing 
> > the IRQ
> >       (read above) and the MMIO address range to be used by the guest
> >
> > The format for the node is specified in 
> > Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt.
> Why don't you just copy-n-paste it in? It is only 10 linues.
>
> > Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
> > ---
> >  tools/libxl/libxl_arm.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 45 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> > index d842d88..34c7e39 100644
> > --- a/tools/libxl/libxl_arm.c
> > +++ b/tools/libxl/libxl_arm.c
> > @@ -130,9 +130,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 +591,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, uint64_t irq)
> Hm, mayube my editor is wrong but these arguments don't appear
> to be right..
>
> > +{
> > +    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;
> Shouldn't we free them?
libxl is allocating a big chunk of memory for the FDT blob (see 
libxl__prepare_dtb) and those helpers will only fill the buffer. If they 
fail is means the buffer is not big enough and the process will be 
restarted from the beginning.
So there is not need to worry to free anything here.

> > +
> > +    set_interrupt(intr, irq, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> 0xF looks like a good candidate for #define.
> > +
> > +    res = fdt_property_interrupts(gc, fdt, &intr, 1);
> > +    if (res) return res;
> Again, shouldn't we free it if we things go south?
> > +
> > +    fdt_property_u32(fdt, "current-speed", 115200);
> So perhaps a #define?
+1 and explaining why 115200. If it is a random value it should be 
explained.
Cheers,

--
Julien Grall

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