Re: [Qemu-devel] [PATCH v3 2/6] hw/arm/dyn_sysbus_devtree: helpers for sysbus device dynamic dt node generation

[Eric Auger]

On 09/09/2014 01:04 PM, Paolo Bonzini wrote:
> Il 09/09/2014 09:54, Eric Auger ha scritto:
>> This module will be used by ARM machine files to generate
>> device tree nodes of dynamically instantiated sysbus devices (ie.
>> those instantiated with -device option).
>>
>> Signed-off-by: Alexander Graf <address@hidden>
>> Signed-off-by: Eric Auger <address@hidden>
>>
>> ---
>>
>> v2 -> v3:
>> - add arm_ prefix
>> - arm_sysbus_device_create_devtree becomes static
>>
>> v1 -> v2:
>> - Code moved in an arch specific file to accomodate architecture
>>   dependent specificities.
>> - remove platform_bus_base from PlatformDevtreeData
>>
>> v1: code originally written by Alex Graf in e500.c and reused for ARM
>>     [Eric Auger]
>>     code originally moved in hw/misc/platform_devices and device itself
>> ---
>>  hw/arm/Makefile.objs                |  1 +
>>  hw/arm/dyn_sysbus_devtree.c         | 66 
>> +++++++++++++++++++++++++++++++++++++
> 
> File names in QEMU typically use a dash instead of an underscore.  Also,
> I see the "fdt" moniker used more often than "devtree" (ouch, I checked
> now and it's 7 vs. 851 uses :)).  So what about hw/arm/sysbus-fdt.c?

Sure I will correct _ and rename.
> 
>>  include/hw/arm/dyn_sysbus_devtree.h | 16 +++++++++
>>  3 files changed, 83 insertions(+)
>>  create mode 100644 hw/arm/dyn_sysbus_devtree.c
>>  create mode 100644 include/hw/arm/dyn_sysbus_devtree.h
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index 6088e53..bc5e014 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -3,6 +3,7 @@ obj-$(CONFIG_DIGIC) += digic_boards.o
>>  obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
>>  obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
>>  obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o
>> +obj-y += dyn_sysbus_devtree.o
>>  
>>  obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
>>  obj-$(CONFIG_DIGIC) += digic.o
>> diff --git a/hw/arm/dyn_sysbus_devtree.c b/hw/arm/dyn_sysbus_devtree.c
>> new file mode 100644
>> index 0000000..6375024
>> --- /dev/null
>> +++ b/hw/arm/dyn_sysbus_devtree.c
>> @@ -0,0 +1,66 @@
>> +#include "hw/arm/dyn_sysbus_devtree.h"
>> +#include "qemu/error-report.h"
>> +#include "sysemu/device_tree.h"
>> +
>> +static int arm_sysbus_device_create_devtree(Object *obj, void *opaque)
>> +{
>> +    PlatformDevtreeData *data = opaque;
>> +    Object *dev;
>> +    SysBusDevice *sbdev;
>> +    bool matched = false;
>> +
>> +    dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
>> +    sbdev = (SysBusDevice *)dev;
>> +
>> +    if (!sbdev) {
>> +        /* Container, traverse it for children */
>> +        return object_child_foreach(obj,
>> +                                    arm_sysbus_device_create_devtree, data);
>> +    }

When we add support for a dynamically instantiable device we add
something like

    if (object_dynamic_cast(obj, TYPE_ETSEC_COMMON)) {
        create_devtree_etsec(ETSEC_COMMON(dev), data);
        matched = true;
    }
>> +
>> +    if (!matched) {
> 
> Who is going to set "matched", since it doesn't escape?



> 
>> +        error_report("Device %s is not supported by this machine yet.",
>> +                     qdev_fw_name(DEVICE(dev)));
>> +        exit(1);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +void arm_platform_bus_create_devtree(DynSysbusParams *params,
>> +                                 void *fdt, const char *intc)
> 
> Let's just call this arm_create_fdt_dynamic.
OK
> 
>> +{
>> +    gchar *node = g_strdup_printf("/address@hidden"PRIx64,
>> +                                  params->platform_bus_base);
>> +    const char platcomp[] = "qemu,platform\0simple-bus";
>> +    PlatformDevtreeData data;
>> +    Object *container;
>> +    uint64_t addr = params->platform_bus_base;
>> +    uint64_t size = params->platform_bus_size;
>> +    int irq_start = params->platform_bus_first_irq;
>> +
>> +    /* Create a /platform node that we can put all devices into */
>> +    qemu_fdt_add_subnode(fdt, node);
>> +    qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
>> +
>> +    /* Our platform bus region is less than 32bit big, so 1 cell is enough 
>> for
>> +       address and size */
>> +    qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
>> +    qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
>> +    qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr, size);
>> +
>> +    qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", intc);
>> +
>> +    /* Loop through all devices and create nodes for known ones */
>> +    data.fdt = fdt;
>> +    data.intc = intc;
>> +    data.irq_start = irq_start;
>> +    data.node = node;
> 
> Why does arm_sysbus_device_create_devtree need intc and irq_start?

irq_start: needed because when the "interrupts" property is set for the
leaf component the irq number is irq_start +
object_property_get_int(obj, "irq[i]", NULL)
irq[i] being in [0, params->platform_bus_num_irqs]

intc: this was in case the leaf component would use "interrupt-parent"
prop. I miss experience on device trees and I don't know if it make
sense the leaf component uses a different interrupt controller than the
parent platform bus or if such property is mandatory in some cases.
Maybe not needed indeed.

Best Regards

Eric
> 
>> +
>> +    container = container_get(qdev_get_machine(), "/peripheral");
>> +    arm_sysbus_device_create_devtree(container, &data);
>> +    container = container_get(qdev_get_machine(), "/peripheral-anon");
>> +    arm_sysbus_device_create_devtree(container, &data);
>> +
>> +    g_free(node);
>> +}
>> diff --git a/include/hw/arm/dyn_sysbus_devtree.h 
>> b/include/hw/arm/dyn_sysbus_devtree.h
>> new file mode 100644
>> index 0000000..b072365
>> --- /dev/null
>> +++ b/include/hw/arm/dyn_sysbus_devtree.h
>> @@ -0,0 +1,16 @@
>> +#ifndef HW_ARM_DYN_SYSBUS_DEVTREE_H
>> +#define HW_ARM_DYN_SYSBUS_DEVTREE_H
>> +
>> +#include "hw/misc/dyn_sysbus_binding.h"
>> +
>> +typedef struct PlatformDevtreeData {
>> +    void *fdt;
>> +    const char *intc;
>> +    int irq_start;
>> +    const char *node;
>> +} PlatformDevtreeData;
>> +
>> +void arm_platform_bus_create_devtree(DynSysbusParams *params,
>> +                                    void *fdt, const char *intc);
>> +
>> +#endif
>>
>