From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45489) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBn7c-0007JM-Ue for qemu-devel@nongnu.org; Thu, 15 Jan 2015 11:18:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YBn7Y-0001uc-4d for qemu-devel@nongnu.org; Thu, 15 Jan 2015 11:18:12 -0500 Received: from mail-wg0-f41.google.com ([74.125.82.41]:38917) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBn7X-0001uQ-Sa for qemu-devel@nongnu.org; Thu, 15 Jan 2015 11:18:08 -0500 Received: by mail-wg0-f41.google.com with SMTP id l18so15908797wgh.0 for ; Thu, 15 Jan 2015 08:18:07 -0800 (PST) Message-ID: <54B7E7D9.5070800@linaro.org> Date: Thu, 15 Jan 2015 17:16:25 +0100 From: Eric Auger MIME-Version: 1.0 References: <1421068903-8981-1-git-send-email-b.reynal@virtualopensystems.com> <1421068903-8981-5-git-send-email-b.reynal@virtualopensystems.com> In-Reply-To: <1421068903-8981-5-git-send-email-b.reynal@virtualopensystems.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH 4/4] hw/arm/sysbus-fdt: arm, pl330 vfio device property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Baptiste Reynal , qemu-devel@nongnu.org, kvmarm@lists.cs.columbia.edu Cc: Peter Maydell , tech@virtualopensystems.com Hi Baptiste, On 01/12/2015 02:21 PM, Baptiste Reynal wrote: > Adapt arm,pl330 function to use the vfio device property API. > > Clock apb-pclk is the default if a clock is needed by the device. > > Three optional parameters are taken into account : > - #dma-cells > - #dma-channels > - #dma-requests > > Signed-off-by: Baptiste Reynal > --- > hw/arm/sysbus-fdt.c | 120 ++++++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 103 insertions(+), 17 deletions(-) > > diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c > index 087e788..a0adc50 100644 > --- a/hw/arm/sysbus-fdt.c > +++ b/hw/arm/sysbus-fdt.c > @@ -96,13 +96,13 @@ static int set_interrupts_fdt_node(char *nodename, SysBusDevice *sbdev, > irq_attr[3*i+2] = cpu_to_be32(((__u32 *) irq_prop->data)[3*i+2]); > } > > - ret = qemu_fdt_setprop(fdt, nodename, "interrupts", > - irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t)); > + ret = qemu_fdt_setprop(fdt, nodename, "interrupts", > + irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t)); delta may be removed > > g_free(irq_attr); > g_free(irq_prop); > > - return ret; > + return ret; delta may be removed > } > > /** > @@ -140,6 +140,54 @@ static int set_regions_fdt_node(char *nodename, SysBusDevice *sbdev, > return ret; > } > may deserve a comment and/or pointer to Documentation/devicetree/bindings/arm/primecell.txt; don't know whether this is the reference doc, sorry. > +static int set_arm_primecell_node(char *nodename, SysBusDevice *sbdev, void > + *opaque) > +{ > + PlatformBusFdtData *data = opaque; > + void *fdt = data->fdt; > + int ret; > + VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev); > + VFIODevice *vbasedev = &vdev->vbasedev; > + struct vfio_dev_property *property; > + Wouldn't it make sense to handle the required compatible prop here? > + /* Optional properties */ > + property = vfio_get_dev_property(vbasedev->fd, "interrupts", > + VFIO_DEV_PROPERTY_TYPE_U32); > + if (property != NULL) { > + ret = set_interrupts_fdt_node(nodename, sbdev, opaque); > + if (ret < 0) { > + error_report("could not set interrupts property of node %s", > + nodename); > + goto fail; in case of fail the property is not released. > + } > + g_free(property); > + } > + > + property = vfio_get_dev_property(vbasedev->fd, "clock-names", > + VFIO_DEV_PROPERTY_TYPE_STRINGS); > + if (property != NULL) { > + /* If a clock is attached, we rely on apb-pclk */ > + /* Check clock existence */ > + ret = fdt_path_offset(fdt, "/apb-pclk"); > + > + if (ret < 0) { > + error_report("could not set clocks property of node %s", nodename); > + goto fail; release property > + } else { > + qemu_fdt_setprop_cells(fdt, nodename, "clocks", > + qemu_fdt_getprop_cell(fdt, "/apb-pclk", "phandle")); > + char clock_names[] = "apb_pclk"; > + qemu_fdt_setprop(fdt, nodename, "clock-names", clock_names, > + sizeof(clock_names)); > + } > + g_free(property); do you intend to support other optional properties? > + } > + > + return 0; > + > +fail: > + return -1; > +} > /** > * add_calxeda_midway_xgmac_fdt_node > * > @@ -213,6 +261,8 @@ static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque) > VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev); > VFIODevice *vbasedev = &vdev->vbasedev; > Object *obj = OBJECT(sbdev); > + struct vfio_dev_property *property; > + int propint; > > mmio_base = object_property_get_int(obj, "mmio[0]", NULL); > > @@ -222,6 +272,13 @@ static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque) > > qemu_fdt_add_subnode(fdt, nodename); > > + ret = set_arm_primecell_node(nodename, sbdev, opaque); > + if (ret < 0) { > + error_report("could not set arm,primecell properties of node %s", > + nodename); > + goto fail; > + } > + > /* > * Process compatible string to deal with multiple strings > * (; is replaced by \0) > @@ -237,20 +294,6 @@ static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque) > qemu_fdt_setprop(fdt, nodename, "compatible", > compat, compat_str_len); > > - /* Setup clocks for AMBA device */ > - /* Check clock existence */ > - ret = fdt_path_offset(fdt, "/apb-pclk"); > - > - if (ret < 0) { > - error_report("could not set clocks property of node %s", nodename); > - } else { > - qemu_fdt_setprop_cells(fdt, nodename, "clocks", > - qemu_fdt_getprop_cell(fdt, "/apb-pclk", "phandle")); > - char clock_names[] = "apb_pclk"; > - qemu_fdt_setprop(fdt, nodename, "clock-names", clock_names, > - sizeof(clock_names)); > - } > - > ret = set_regions_fdt_node(nodename, sbdev, opaque); > > if (ret < 0) { > @@ -266,6 +309,49 @@ static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque) > goto fail; > } > > + /* Optional properties */ > + property = vfio_get_dev_property(vbasedev->fd, "#dma-cells", > + VFIO_DEV_PROPERTY_TYPE_U32); > + if (property != NULL) { > + propint = cpu_to_be32(((__u32 *) property->data)[0]); > + ret = qemu_fdt_setprop(fdt, nodename, "#dma-cells", &propint, > + sizeof(int)); > + if (ret < 0) { > + error_report("could not set #dma-cells property of node %s", > + nodename); > + goto fail; property release > + } > + g_free(property); > + } > + > + property = vfio_get_dev_property(vbasedev->fd, "#dma-channels", > + VFIO_DEV_PROPERTY_TYPE_U32); > + if (property != NULL) { > + propint = cpu_to_be32(((__u32 *) property->data)[0]); > + ret = qemu_fdt_setprop(fdt, nodename, "#dma-cells", &propint, > + sizeof(int)); > + if (ret < 0) { > + error_report("could not set #dma-cells property of node %s", > + nodename); > + goto fail; > + } > + g_free(property); > + } > + > + property = vfio_get_dev_property(vbasedev->fd, "#dma-requests", > + VFIO_DEV_PROPERTY_TYPE_U32); > + if (property != NULL) { > + propint = cpu_to_be32(((__u32 *) property->data)[0]); > + ret = qemu_fdt_setprop(fdt, nodename, "#dma-cells", &propint, > + sizeof(int)); > + if (ret < 0) { > + error_report("could not set #dma-cells property of node %s", > + nodename); > + goto fail; > + } > + g_free(property); > + } > + > g_free(nodename); > > return 0; to me it looks a good and interesting illustration of the modality Best Regards Eric >