On Wed, Dec 17, 2014 at 2:32 PM, Eric Auger <eric.au...@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.rey...@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*/
> > };
> >
> >
>
>
