Re: [PATCH 3/4] libxl: Allocate MMIO params for I2c device and update DT

[Oleksandr <olekstysh@xxxxxxxxx>]

On 05.05.22 10:33, Viresh Kumar wrote:

Hello Viresh


> This patch allocates Virtio MMIO params (IRQ and memory region) and pass
> them to the backend, also update Guest device-tree.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>   tools/libs/light/libxl_arm.c | 57 ++++++++++++++++++++++++++++++++++--
>   1 file changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 8132a47a72b6..ea633d6f91df 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -115,6 +115,26 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>           }
>       }
>   
> +    for (i = 0; i < d_config->num_i2cs; i++) {
> +        libxl_device_i2c *i2c = &d_config->i2cs[i];
> +
> +        i2c->base = alloc_virtio_mmio_base(gc);
> +        if (!i2c->base)
> +            return ERROR_FAIL;
> +
> +        i2c->irq = alloc_virtio_mmio_irq(gc);
> +        if (!i2c->irq)
> +            return ERROR_FAIL;
> +
> +        if (virtio_irq < i2c->irq)
> +            virtio_irq = i2c->irq;
> +
> +        virtio_enabled = true;
> +
> +        LOG(DEBUG, "Allocate Virtio MMIO params for I2C: IRQ %u BASE 
> 0x%"PRIx64,
> +            i2c->irq, i2c->base);
> +    }
> +
>       if (virtio_enabled)
>           nr_spis += (virtio_irq - 32) + 1;
>   
> @@ -874,7 +894,7 @@ static int make_vpci_node(libxl__gc *gc, void *fdt,
>   }
>   
>   
> -static int make_virtio_mmio_node(libxl__gc *gc, void *fdt,
> +static int _make_virtio_mmio_node(libxl__gc *gc, void *fdt,
>                                    uint64_t base, uint32_t irq)

NIT: I am not sure the leading underscore in the name would be welcome. 
I would probably rename it.
How about s/_make_virtio_mmio_node/make_virtio_mmio_node_common here and ...



>   {
>       int res;
> @@ -897,7 +917,35 @@ static int make_virtio_mmio_node(libxl__gc *gc, void *fdt,
>       res = fdt_property_interrupts(gc, fdt, &intr, 1);
>       if (res) return res;
>   
> -    res = fdt_property(fdt, "dma-coherent", NULL, 0);
> +    return fdt_property(fdt, "dma-coherent", NULL, 0);
> +}
> +
> +static int make_virtio_mmio_node(libxl__gc *gc, void *fdt,
> +                                 uint64_t base, uint32_t irq)
... s/make_virtio_mmio_node/make_virtio_mmio_node_simple here or 
something like that?

> +{
> +    int res;
> +
> +    res = _make_virtio_mmio_node(gc, fdt, base, irq);
> +    if (res) return res;
> +
> +    return fdt_end_node(fdt);
> +}
> +
> +static int make_virtio_mmio_node_i2c(libxl__gc *gc, void *fdt,
> +                                 uint64_t base, uint32_t irq)
> +{
> +    int res;
> +
> +    res = _make_virtio_mmio_node(gc, fdt, base, irq);
> +    if (res) return res;
> +
> +    res = fdt_begin_node(fdt, "i2c");
> +    if (res) return res;
> +
> +    res = fdt_property_compat(gc, fdt, 1, "virtio,device22");
Please provide a link (in the commit description) to the corresponding 
Linux device tree binding. I assume it is:
Documentation/devicetree/bindings/i2c/i2c-virtio.yaml


> +    if (res) return res;
> +
> +    res = fdt_end_node(fdt);
>       if (res) return res;
>   
>       res = fdt_end_node(fdt);
> @@ -1221,6 +1269,11 @@ static int libxl__prepare_dtb(libxl__gc *gc, 
> libxl_domain_config *d_config,
>                   FDT( make_virtio_mmio_node(gc, fdt, disk->base, disk->irq) );
>           }
>   
> +        for (i = 0; i < d_config->num_i2cs; i++) {
> +            libxl_device_i2c *i2c = &d_config->i2cs[i];
> +            FDT( make_virtio_mmio_node_i2c(gc, fdt, i2c->base, i2c->irq) );
> +        }
> +
>           if (pfdt)
>               FDT( copy_partial_fdt(gc, fdt, pfdt) );
>   
--
Regards,

Oleksandr Tyshchenko