Re: [Qemu-devel] [RFC PATCH 1/3] hw/vfio/sysbus-fdt: generic add_generic_fdt_node

[Eric Auger]

On 12/17/2014 03:55 PM, Baptiste Reynal wrote:
> 
> 
> On Wed, Dec 17, 2014 at 2:32 PM, Eric Auger <address@hidden
> <mailto:address@hidden>> wrote:
> 
>     On 12/17/2014 11:09 AM, Baptiste Reynal wrote:
>     > Modify add_calxeda_midway_xgmac_fdt_node to make it more generic.
>     > Add multiple compatible strings support.
>     Hi Baptiste,
> 
>     Although I understand you may be tempted to transform the Calxeda basic
>     node creation function into something more generic, we came to the
>     current result after long discussions with Alex Graf. This generic
>     function was something released in the past but transformed into
>     something specific at the end.
> 
>     Typically the fact the IRQ is edge sensitive or level sensitive may
>     depend on the device and it would be arbitrary here to choose either in
>     a "generic" node creation.
> 
>     So the current trend for new devices is to add another entry in the
>     add_fdt_node_functions array; genericity target was argued in the past
>     and rejected.
> 
>     If you want to do something going in the direction of genericity I would
>     advise you to investigate using Antonios API ([RFC 0/4] VFIO: PLATFORM:
>     Return device tree info for a platform device node). This could
>     typically enable to get the egde/level sensitive characteritics.
> 
> 
> I'm not sure I totally understand your point, but creating a new 
> add_pl330_fdt_node function which will call to
> add_calxeda_midway_xgmac_fdt_node and perform amba specific process
> (multiple compatible string and clocks) seems to be a good solution for
> you ?
> 
No I had in mind the amba node creation function should duplicate the
code, have its own compat string manipulation if needed, add its own
clock properties ...

Maybe we could devise some helper function for building
the reg property and interrupt property?

BR

Eric
> 
>     >
>     > Signed-off-by: Baptiste Reynal <address@hidden
>     <mailto:address@hidden>>
>     > ---
>     >  hw/arm/sysbus-fdt.c | 21 ++++++++++++++++-----
>     >  1 file changed, 16 insertions(+), 5 deletions(-)
>     >
>     > diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>     > index 15bb50c..125ba37 100644
>     > --- a/hw/arm/sysbus-fdt.c
>     > +++ b/hw/arm/sysbus-fdt.c
>     > @@ -58,12 +58,12 @@ typedef struct NodeCreationPair {
>     >  /* Device Specific Code */
>     >
>     >  /**
>     > - * add_calxeda_midway_xgmac_fdt_node
>     > + * add_device_fdt_node
>     >   *
>     >   * Generates a very simple node with following properties:
>     >   * compatible string, regs, interrupts
>     >   */
>     > -static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev,
>     void *opaque)
>     > +static int add_generic_fdt_node(SysBusDevice *sbdev, void *opaque)
>     >  {
>     >      PlatformBusFdtData *data = opaque;
>     >      PlatformBusDevice *pbus = data->pbus;
>     > @@ -80,6 +80,18 @@ static int
>     add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
>     >      VFIODevice *vbasedev = &vdev->vbasedev;
>     >      Object *obj = OBJECT(sbdev);
>     >
>     > +    /*
>     > +     * Process compatible string to deal with multiple strings
>     > +     * (; is replaced by \0)
>     > +     */
>     > +    char *compat = g_strdup(vdev->compat);
>     > +    compat_str_len = strlen(compat) + 1;
>     > +
>     > +    char *semicolon = compat;
>     > +    while ((semicolon = strchr(semicolon, ';')) != NULL) {
>     > +        *semicolon = '\0';
>     > +    }
>     why can't you format the string directly in the corresponding VFIO AMBA
>     device? Formerly we had this problem of conversion since we passed the
>     format string in qemu command line.
> 
>     Best Regards
> 
>     Eric
> 
> 
> From my point of view, formatting the string directly in VFIO AMBA
> device will mess a lot with string function, but I can use an array of
> string in VFIOPlatformDevice to avoid it.
> 
> Best Regards,
> 
> Baptiste
> 
>     > +
>     >      mmio_base = object_property_get_int(obj, "mmio[0]", NULL);
>     >
>     >      nodename = g_strdup_printf("%s/address@hidden" PRIx64, parent_node,
>     > @@ -88,9 +100,8 @@ static int
>     add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
>     >
>     >      qemu_fdt_add_subnode(fdt, nodename);
>     >
>     > -    compat_str_len = strlen(vdev->compat) + 1;
>     >      qemu_fdt_setprop(fdt, nodename, "compatible",
>     > -                          vdev->compat, compat_str_len);
>     > +                          compat, compat_str_len);
>     >
>     >      reg_attr = g_new(uint64_t, vbasedev->num_regions*4);
>     >
>     > @@ -140,7 +151,7 @@ fail:
>     >
>     >  /* list of supported dynamic sysbus devices */
>     >  static const NodeCreationPair add_fdt_node_functions[] = {
>     > -{TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
>     > +{TYPE_VFIO_CALXEDA_XGMAC, add_generic_fdt_node},
>     >  {"", NULL}, /*last element*/
>     >  };
>     >
>     >
>