[v3,2/6] hw/arm/dyn_sysbus_devtree: helpers for sysbus device dynamic dt node generation

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 <agraf@suse.de>
Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

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 +++++++++++++++++++++++++++++++++++++
 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

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 <agraf@suse.de>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> 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?

>  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);
> +    }
> +
> +    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.

> +{
> +    gchar *node = g_strdup_printf("/platform@%"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?

> +
> +    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
>

On Tue, Sep 9, 2014 at 9:04 PM, Paolo Bonzini <pbonzini@redhat.com> 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 <agraf@suse.de>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> 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?
>

Yes, "devtree" is legacy. Please use fdt or dtb as appropriate.

Regards,
Peter

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 <agraf@suse.de>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> 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("/platform@%"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
>>
>

On 9 September 2014 16:56, Eric Auger <eric.auger@linaro.org> wrote:
> 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.

Somewhat tangential, but for passthrough devices how
do we decide whether the device tree node needs to
mark the interrupt as edge or level triggered?
Presumably this is going to be a "depends on what
the passed through hardware is" thing...

-- PMM

Il 09/09/2014 17:56, Eric Auger ha scritto:
>>> >> +    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?
> 
> 

That's not part of this patch though, right?  So this code for now is
dead.  Please remove the dead code if it is not used in this series.

We really should make that an interface, so that the code can do just

    if (object_dynamic_cast(obj, TYPE_FDT_BUILDER)) {
        fdt_builder_create_fdt(FDT_BUILDER(dev), data);
    } else {
        ...
    }

(and so can the generic virt.c code) but that can come later.

>> 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]

Ah, it's passed to the not-yet-existing create_* functions.

> 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.

No idea, sorry.

Paolo

On 09/09/2014 06:00 PM, Peter Maydell wrote:
> On 9 September 2014 16:56, Eric Auger <eric.auger@linaro.org> wrote:
>> 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.
> 
> Somewhat tangential, but for passthrough devices how
> do we decide whether the device tree node needs to
> mark the interrupt as edge or level triggered?
> Presumably this is going to be a "depends on what
> the passed through hardware is" thing...

Hi Peter,

Yes I think this is one of the reasons why Alex insisted on having the
device node creation specialized for each device and not common to VFIO
devices.

Besides, I did not looked sufficiently at Antonios' patch "VFIO:
PLATFORM: Return device tree info for a platform device node" but I
guess this would enable to retrieve the property values.
https://www.mail-archive.com/kvm@vger.kernel.org/msg106282.html.

But I remember I read somewhere such interrupts properties were not
always correct in dt?

Best Regards

Eric
> 
> -- PMM
>

On 09/09/2014 06:03 PM, Paolo Bonzini wrote:
> Il 09/09/2014 17:56, Eric Auger ha scritto:
>>>>>> +    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?
>>
>>
> 
> That's not part of this patch though, right?

right
  So this code for now is
> dead.  Please remove the dead code if it is not used in this series.
> 
> We really should make that an interface, so that the code can do just
> 
>     if (object_dynamic_cast(obj, TYPE_FDT_BUILDER)) {
>         fdt_builder_create_fdt(FDT_BUILDER(dev), data);
>     } else {
>         ...
>     }
> 
> (and so can the generic virt.c code) but that can come later.

OK I will correct
> 
>>> 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]
> 
> Ah, it's passed to the not-yet-existing create_* functions.
yep
> 
>> 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.
> 
> No idea, sorry.
- Eric
> 
> Paolo
>