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

[Baptiste Reynal <b.reynal@virtualopensystems.com>]

[-- Attachment #1: Type: text/plain, Size: 4261 bytes --]

On Wed, Dec 17, 2014 at 2:32 PM, Eric Auger <eric.auger@linaro.org> 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
?

>
> >
> > Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> > ---
> >  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/%s@%" 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*/
> >  };
> >
> >
>
>

[-- Attachment #2: Type: text/html, Size: 5836 bytes --]