[Qemu-devel] [PATCH v4 0/6] machvirt dynamic sysbus device instantiation

[Qemu-devel] [PATCH v4 0/6] machvirt dynamic sysbus device instantiation

[Eric Auger]

This patch series enables machvirt to dynamically instantiate sysbus
devices from command line (using -device option).

All those sysbus devices are plugged onto a platform bus. This latter
device is instantiated in machvirt and takes care of the binding of
children sysbus devices on a machine init done notifier. The device
tree node generation for children dynamic sysbus device also happens
on a subsequent notifier that must be executed after the above one.
machvirt registers that notifier before the platform bus creation to
make sure notifiers are executed in the right order: dt generation
after actual QOM binding.

Very few sysbus devices are supposed to be instantiated that way.
VFIO devices belong to them.

Node creation really is architecture specific. On ARM the dynamic
sysbus device node creation is implemented in a new C module,
hw/arm/sysbus-fdt.c and not in the machine file.

This series applies on top of Alex Graf's series
[PATCH v3 0/7] Dynamic sysbus device allocation support
http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg04860.html

Machvirt transformations and sysbus-fdt are largely inspired from Alex work.

The patch series can be found at:
http://git.linaro.org/people/eric.auger/qemu.git (branch vfio_integ_v7)

Best Regards

Eric

v3 -> v4:
- dyn_sysbus_binding removed since binding stuff now are implemented by
  the platform bus device
- due to a change in ARM load_dtb implementation using rom_add_blob_fixed,
  the dt no more is generated in a reset notifier but is generated on a
  machine init done notifier
- the augmented device tree is not generated from scratch anymore but is
  added using a modify_dtb function. This required some small change in
  boot.c
- the case where the user provides a dtb file now is handled
- some cleanup in virt additions
- implement a list of dyanmically instantiable devices in sysbus-fdt

v2 -> v3:
- patch now applies on top of Alex full patchset
- dyn_sysbus_devtree: add arm_prefix to emphasize the fact those
  functions are arm specific; arm_sysbus_device_create_devtree
  becomes static
- load_dtb renamed into arm_load_dtb
- add copyright in hw/arm/dyn_sysbus_devtree.c

v1 -> v2:
- device node generation no more in sysbus device but in
  dyn_sysbus_devtree
- VFIO region shrinked to 4MB and relocated in machvirt to avoid PCI
  shrink (dynamic vfio-mmio support might come latter)
- platform_bus_base removed from PlatformDevtreeData

Eric Auger (6):
  hw/arm/boot: load_dtb becomes non static arm_load_dtb
  hw/arm/boot: dtb start and limit moved in arm_boot_info
  hw/arm/boot: do not free VirtBoardInfo fdt in arm_load_dtb
  hw/arm: add a new modify_dtb_opaque field in arm_boot_info
  hw/arm/sysbus-fdt: helpers for platform bus nodes addition
  hw/arm/virt: add dynamic sysbus device support

 hw/arm/Makefile.objs        |   1 +
 hw/arm/boot.c               |  48 +++++++-----
 hw/arm/sysbus-fdt.c         | 181 ++++++++++++++++++++++++++++++++++++++++++++
 hw/arm/virt.c               |  59 +++++++++++++++
 include/hw/arm/arm.h        |   7 ++
 include/hw/arm/sysbus-fdt.h |  50 ++++++++++++
 6 files changed, 326 insertions(+), 20 deletions(-)
 create mode 100644 hw/arm/sysbus-fdt.c
 create mode 100644 include/hw/arm/sysbus-fdt.h

-- 
1.8.3.2

[Qemu-devel] [PATCH v4 1/6] hw/arm/boot: load_dtb becomes non static arm_load_dtb

[Eric Auger]

load_dtb is renamed into arm_load_dtb and becomes non static.
it will be used by machvirt for dynamic instantiation of
platform devices

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v2 -> v3:
load_dtb renamed into arm_load_dtb

Conflicts:
	hw/arm/boot.c
---
 hw/arm/boot.c        | 12 ++++++------
 include/hw/arm/arm.h |  2 ++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index bffbea5..f5714ea 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -313,7 +313,7 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
 }
 
 /**
- * load_dtb() - load a device tree binary image into memory
+ * arm_load_dtb() - load a device tree binary image into memory
  * @addr:       the address to load the image at
  * @binfo:      struct describing the boot environment
  * @addr_limit: upper limit of the available memory area at @addr
@@ -330,8 +330,8 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
  *          0 if the image size exceeds the limit,
  *          -1 on errors.
  */
-static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
-                    hwaddr addr_limit)
+int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
+                 hwaddr addr_limit)
 {
     void *fdt = NULL;
     int size, rc;
@@ -504,7 +504,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
             /* If we have a device tree blob, but no kernel to supply it to,
              * copy it to the base of RAM for a bootloader to pick up.
              */
-            if (load_dtb(info->loader_start, info, 0) < 0) {
+            if (arm_load_dtb(info->loader_start, info, 0) < 0) {
                 exit(1);
             }
         }
@@ -572,7 +572,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
             if (elf_low_addr < info->loader_start) {
                 elf_low_addr = 0;
             }
-            if (load_dtb(info->loader_start, info, elf_low_addr) < 0) {
+            if (arm_load_dtb(info->loader_start, info, elf_low_addr) < 0) {
                 exit(1);
             }
         }
@@ -637,7 +637,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
              */
             hwaddr dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size,
                                              4096);
-            if (load_dtb(dtb_start, info, 0) < 0) {
+            if (arm_load_dtb(dtb_start, info, 0) < 0) {
                 exit(1);
             }
             fixupcontext[FIXUP_ARGPTR] = dtb_start;
diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index cefc9e6..5fdae7b 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -68,6 +68,8 @@ struct arm_boot_info {
     hwaddr entry;
 };
 void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
+int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
+                 hwaddr addr_limit);
 
 /* Multiplication factor to convert from system clock ticks to qemu timer
    ticks.  */
-- 
1.8.3.2

[Qemu-devel] [PATCH v4 2/6] hw/arm/boot: dtb start and limit moved in arm_boot_info

[Eric Auger]

Two fields are added in arm_boot_info (dtb_start and dtb_limit). The
prototype of arm_load_kernel is changed to only use arm_boot_info.

The rationale behind introducing that change is when dealing with
dynamic sysbus devices, we need to upgrade the device tree with dynamic
device nodes after the dtb is already loaded. Storing those parameters
in arm_boot_info allows to avoid computing again dtb_start and
dtb_load, as done in arm_load_kernel.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 hw/arm/boot.c        | 38 +++++++++++++++++++++-----------------
 include/hw/arm/arm.h |  5 +++--
 2 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index f5714ea..9f0662e 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -314,24 +314,21 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
 
 /**
  * arm_load_dtb() - load a device tree binary image into memory
- * @addr:       the address to load the image at
  * @binfo:      struct describing the boot environment
- * @addr_limit: upper limit of the available memory area at @addr
  *
  * Load a device tree supplied by the machine or by the user  with the
- * '-dtb' command line option, and put it at offset @addr in target
- * memory.
+ * '-dtb' command line option, and put it at offset binfo->dtb_start in
+ * target memory.
  *
- * If @addr_limit contains a meaningful value (i.e., it is strictly greater
- * than @addr), the device tree is only loaded if its size does not exceed
- * the limit.
+ * If binfo->dtb_limit contains a meaningful value (i.e., it is strictly
+ * greater binfo->dtb_start, the device tree is only loaded if its size does
+ * not exceed this upper limit.
  *
  * Returns: the size of the device tree image on success,
  *          0 if the image size exceeds the limit,
  *          -1 on errors.
  */
-int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
-                 hwaddr addr_limit)
+int arm_load_dtb(const struct arm_boot_info *binfo)
 {
     void *fdt = NULL;
     int size, rc;
@@ -360,7 +357,8 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         }
     }
 
-    if (addr_limit > addr && size > (addr_limit - addr)) {
+    if (binfo->dtb_limit > binfo->dtb_start &&
+            size > (binfo->dtb_limit - binfo->dtb_start)) {
         /* Installing the device tree blob at addr would exceed addr_limit.
          * Whether this constitutes failure is up to the caller to decide,
          * so just return 0 as size, i.e., no error.
@@ -427,7 +425,7 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
     /* Put the DTB into the memory map as a ROM image: this will ensure
      * the DTB is copied again upon reset, even if addr points into RAM.
      */
-    rom_add_blob_fixed("dtb", fdt, size, addr);
+    rom_add_blob_fixed("dtb", fdt, size, binfo->dtb_start);
 
     g_free(fdt);
 
@@ -504,7 +502,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
             /* If we have a device tree blob, but no kernel to supply it to,
              * copy it to the base of RAM for a bootloader to pick up.
              */
-            if (arm_load_dtb(info->loader_start, info, 0) < 0) {
+            info->dtb_start = info->loader_start;
+            info->dtb_limit = 0;
+
+            if (arm_load_dtb(info) < 0) {
                 exit(1);
             }
         }
@@ -572,7 +573,9 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
             if (elf_low_addr < info->loader_start) {
                 elf_low_addr = 0;
             }
-            if (arm_load_dtb(info->loader_start, info, elf_low_addr) < 0) {
+            info->dtb_start = info->loader_start;
+            info->dtb_limit = elf_low_addr;
+            if (arm_load_dtb(info) < 0) {
                 exit(1);
             }
         }
@@ -635,12 +638,13 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
              * kernels will trash anything in the 4K page the initrd
              * ends in, so make sure the DTB isn't caught up in that.
              */
-            hwaddr dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size,
-                                             4096);
-            if (arm_load_dtb(dtb_start, info, 0) < 0) {
+            info->dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size,
+                                            4096);
+            info->dtb_limit = 0;
+            if (arm_load_dtb(info) < 0) {
                 exit(1);
             }
-            fixupcontext[FIXUP_ARGPTR] = dtb_start;
+            fixupcontext[FIXUP_ARGPTR] = info->dtb_start;
         } else {
             fixupcontext[FIXUP_ARGPTR] = info->loader_start + KERNEL_ARGS_ADDR;
             if (info->ram_size >= (1ULL << 32)) {
diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index 5fdae7b..5f1ecb7 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -65,11 +65,12 @@ struct arm_boot_info {
     int is_linux;
     hwaddr initrd_start;
     hwaddr initrd_size;
+    hwaddr dtb_start; /* start address of the dtb */
+    hwaddr dtb_limit; /* upper RAM limit the dtb cannot overshoot */
     hwaddr entry;
 };
 void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
-int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
-                 hwaddr addr_limit);
+int arm_load_dtb(const struct arm_boot_info *binfo);
 
 /* Multiplication factor to convert from system clock ticks to qemu timer
    ticks.  */
-- 
1.8.3.2

[Qemu-devel] [PATCH v4 3/6] hw/arm/boot: do not free VirtBoardInfo fdt in arm_load_dtb

[Eric Auger]

Currently arm_load_dtb frees the fdt handle whatever it is allocated
from load_device_tree or allocated externally.

When adding dynamic sysbus nodes after the first dtb load, we would like
to reuse the fdt used during the first load instead of re-creating the
whole device tree. If the fdt is destroyed, this is not possible.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 hw/arm/boot.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 9f0662e..0e4b078 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -427,12 +427,16 @@ int arm_load_dtb(const struct arm_boot_info *binfo)
      */
     rom_add_blob_fixed("dtb", fdt, size, binfo->dtb_start);
 
-    g_free(fdt);
+    if (binfo->dtb_filename) {
+        g_free(fdt);
+    }
 
     return size;
 
 fail:
-    g_free(fdt);
+    if (binfo->dtb_filename) {
+        g_free(fdt);
+    }
     return -1;
 }
 
-- 
1.8.3.2

[Qemu-devel] [PATCH v4 4/6] hw/arm: add a new modify_dtb_opaque field in arm_boot_info

[Eric Auger]

This field can be used by any modify_dtb() function to pass
additional arguments requested to build the modified dtb. This
is needed for creating the platform bus dynamic sysbus nodes.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 include/hw/arm/arm.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index 5f1ecb7..ff776fa 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -68,6 +68,10 @@ struct arm_boot_info {
     hwaddr dtb_start; /* start address of the dtb */
     hwaddr dtb_limit; /* upper RAM limit the dtb cannot overshoot */
     hwaddr entry;
+    /* in case modify_dtb requires additional parameters to create the
+     * the new nodes, use following opaque
+     */
+    void *modify_dtb_opaque;
 };
 void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
 int arm_load_dtb(const struct arm_boot_info *binfo);
-- 
1.8.3.2

[Qemu-devel] [PATCH v4 5/6] hw/arm/sysbus-fdt: helpers for platform bus nodes addition

[Eric Auger]

This new C module will be used by ARM machine files to generate
platform bus node and their dynamic sysbus device tree nodes.

Dynamic sysbus device node addition is done in a machine init
done notifier. arm_register_platform_bus_fdt_creator does the
registration of this latter and is supposed to be called by
ARM machine files that support platform bus and their dynamic
sysbus. Addition of dynamic sysbus nodes is done only if the
user did not provide any dtb.

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v3 -> v4:
- dyn_sysbus_devtree.c renamed into sysbus-fdt.c
- use new PlatformBusDevice object
- the dtb upgrade is done through modify_dtb. Before the fdt
  was recreated from scratch. When the user provided a dtb this
  latter was overwritten which was not correct.
- an array contains the association between device type names
  and their node creation function
- I must aknowledge I did not find any cleaner way to implement
  a FDT_BUILDER interface, as suggested by Paolo. The class method
  would need to be initialized somewhere and since it cannot
  happen in the device itself - according to Alex & Peter comments -,
  I don't see when I shall associate the device type and its
  interface implementation.

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]
---
 hw/arm/Makefile.objs        |   1 +
 hw/arm/sysbus-fdt.c         | 181 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/arm/sysbus-fdt.h |  50 ++++++++++++
 3 files changed, 232 insertions(+)
 create mode 100644 hw/arm/sysbus-fdt.c
 create mode 100644 include/hw/arm/sysbus-fdt.h

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 6088e53..0cc63e1 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 += sysbus-fdt.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/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
new file mode 100644
index 0000000..d5476f1
--- /dev/null
+++ b/hw/arm/sysbus-fdt.c
@@ -0,0 +1,181 @@
+/*
+ * ARM Platform Bus device tree generation helpers
+ *
+ * Copyright (c) 2014 Linaro Limited
+ *
+ * Authors:
+ *  Alex Graf <agraf@suse.de>
+ *  Eric Auger <eric.auger@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "hw/arm/sysbus-fdt.h"
+#include "qemu/error-report.h"
+#include "sysemu/device_tree.h"
+#include "hw/platform-bus.h"
+#include "sysemu/sysemu.h"
+#include "hw/platform-bus.h"
+
+/*
+ * internal struct that contains the information to create dynamic
+ * sysbus device node
+ */
+typedef struct PlatformBusFdtData {
+    void *fdt; /* device tree handle */
+    int irq_start; /* index of the first IRQ usable by platform bus devices */
+    const char *pbus_node_name; /* name of the platform bus node */
+    PlatformBusDevice *pbus;
+} PlatformBusFdtData;
+
+/*
+ * struct used when calling the machine init done notifier
+ * that constructs the fdt nodes of platform bus devices
+ */
+typedef struct PlatformBusFdtNotifierParams {
+    ARMPlatformBusFdtParams *fdt_params;
+    Notifier notifier;
+} PlatformBusFdtNotifierParams;
+
+/* struct that associates a device type name and a node creation function */
+typedef struct NodeCreationPair {
+    const char *typename;
+    int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
+} NodeCreationPair;
+
+/* list of supported dynamic sysbus devices */
+NodeCreationPair add_fdt_node_functions[] = {
+        {"", NULL}, /*last element*/
+};
+
+/**
+ * add_fdt_node - add the device tree node of a dynamic sysbus device
+ *
+ * @sbdev: handle to the sysbus device
+ * @opaque: handle to the PlatformBusFdtData
+ *
+ * Checks the sysbus type belongs to the list of device types that
+ * are dynamically instantiable and in the positive call the node
+ * creation function.
+ */
+static int add_fdt_node(SysBusDevice *sbdev, void *opaque)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(add_fdt_node_functions); i++) {
+        if (!strcmp(object_get_typename(OBJECT(sbdev)),
+                    add_fdt_node_functions[i].typename)) {
+            add_fdt_node_functions[i].add_fdt_node_fn(sbdev, opaque);
+            return 0;
+        }
+    }
+    error_report("Device %s can not be dynamically instantiated",
+                     qdev_fw_name(DEVICE(sbdev)));
+    return 1;
+}
+
+/**
+ * add_all_platform_bus_fdt_nodes - create all the platform bus nodes
+ *
+ * builds the parent platform bus node and all the nodes of dynamic
+ * sysbus devices attached * to it. It is a modify_dtb() function, ie.
+ * called * by arm_load_dtb()
+ */
+static void add_all_platform_bus_fdt_nodes(const struct arm_boot_info *info,
+                                           void *fdt)
+{
+    const char platcomp[] = "qemu,platform\0simple-bus";
+    ARMPlatformBusSystemParams *system_params;
+    ARMPlatformBusFdtParams *fdt_params = info->modify_dtb_opaque;
+    PlatformBusDevice *pbus;
+    DeviceState *dev;
+    gchar *node;
+    uint64_t addr, size;
+    int irq_start;
+
+    if (!fdt_params) {
+        return;
+    }
+
+    system_params = fdt_params->system_params;
+    node = g_strdup_printf("/platform@%"PRIx64,
+                               system_params->platform_bus_base);
+    addr = system_params->platform_bus_base;
+    size = system_params->platform_bus_size;
+    irq_start = system_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", fdt_params->intc);
+
+    dev = qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE);
+    pbus = PLATFORM_BUS_DEVICE(dev);
+
+    /* We can only create dt nodes for dynamic devices when they're ready */
+    if (pbus->done_gathering) {
+        PlatformBusFdtData data = {
+            .fdt = fdt,
+            .irq_start = irq_start,
+            .pbus_node_name = node,
+            .pbus = pbus,
+        };
+
+        /* Loop through all dynamic sysbus devices and create their node */
+        foreach_dynamic_sysbus_device(add_fdt_node, &data);
+    }
+
+    g_free(node);
+}
+
+static void upgrade_dtb(ARMPlatformBusFdtParams *params)
+{
+    struct arm_boot_info *binfo = params->binfo;
+
+    /*
+     * In case the user provided a dtb, we assume he already integrated the
+     * dynamic sysbus nodes. This corresponds to a use case where the dynamic
+     * sysbus nodes are complex and their generation is not yet supported. In
+     * case the use can take in charge the guest dt while qemu takes in charge
+     * the qom stuff.
+     */
+    if (!binfo->dtb_filename) {
+        binfo->modify_dtb_opaque = params;
+        binfo->modify_dtb = add_all_platform_bus_fdt_nodes;
+        arm_load_dtb(binfo);
+    }
+}
+
+static void platform_bus_fdt_notify(Notifier *notifier, void *data)
+{
+    PlatformBusFdtNotifierParams *p =
+        container_of(notifier, PlatformBusFdtNotifierParams, notifier);
+    upgrade_dtb(p->fdt_params);
+}
+
+void arm_register_platform_bus_fdt_creator(ARMPlatformBusFdtParams *fdt_params)
+{
+    PlatformBusFdtNotifierParams *p = g_new(PlatformBusFdtNotifierParams, 1);
+
+    p->fdt_params = fdt_params;
+    p->notifier.notify = platform_bus_fdt_notify;
+    qemu_add_machine_init_done_notifier(&p->notifier);
+}
diff --git a/include/hw/arm/sysbus-fdt.h b/include/hw/arm/sysbus-fdt.h
new file mode 100644
index 0000000..b9602f1
--- /dev/null
+++ b/include/hw/arm/sysbus-fdt.h
@@ -0,0 +1,50 @@
+/*
+ * Dynamic sysbus device tree node generation API
+ *
+ * Copyright Linaro Limited, 2014
+ *
+ * Authors:
+ *  Alex Graf <agraf@suse.de>
+ *  Eric Auger <eric.auger@linaro.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef HW_ARM_SYSBUS_FDT_H
+#define HW_ARM_SYSBUS_FDT_H
+
+#include "hw/arm/arm.h"
+#include "qemu-common.h"
+#include "hw/sysbus.h"
+
+/*
+ * struct that contains dimensioning parameters of the platform bus
+ */
+typedef struct {
+    hwaddr platform_bus_base; /* start address of the bus */
+    hwaddr platform_bus_size; /* size of the bus */
+    int platform_bus_first_irq; /* first hwirq assigned to the bus */
+    int platform_bus_num_irqs; /* number of hwirq assigned to the bus */
+} ARMPlatformBusSystemParams;
+
+/*
+ * struct that contains all relevant info to build the fdt nodes of
+ * platform bus and attached dynamic sysbus devices
+ * in the future might be augmented with additional info
+ * such as PHY, CLK handles ...
+ */
+typedef struct {
+    ARMPlatformBusSystemParams *system_params;
+    struct arm_boot_info *binfo;
+    const char *intc; /* parent interrupt controller name */
+} ARMPlatformBusFdtParams;
+
+/**
+ * arm_register_platform_bus_fdt_creator - register a machine init done
+ * notifier that creates the device tree nodes of the platform bus and
+ * associated dynamic sysbus devices
+ */
+void arm_register_platform_bus_fdt_creator(ARMPlatformBusFdtParams *fdt_params);
+
+#endif
-- 
1.8.3.2

[Qemu-devel] [PATCH v4 6/6] hw/arm/virt: add dynamic sysbus device support

[Eric Auger]

Allows sysbus devices to be instantiated from command line by
using -device option. Machvirt creates a platform bus at init.
The dynamic sysbus devices are attached to a platform bus device.

The platform bus device registers a machine init done notifier
whose role will be to bind the dynamic sysbus devices. Indeed
dynamic sysbus devices are created after machine init.

machvirt also registers a notifier that will start the VFIO
dynamic device IRQ handling.

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Eric Auger <eric.auger@linaro.org>

---
v3 -> v4:
- use platform bus object, instantiated in create_platform_bus
- device tree generation for platform bus and children dynamic
  sysbus devices is no more handled at reset but in a
  machine_init_done_notifier (due to the change in implementaion
  of ARM load dtb using rom_add_blob_fixed).
- device tree enhancement now takes into account the case of
  user provided dtb. Before the user dtb was overwritten which
  was wrong. However in case the dtb is provided by the user,
  dynamic sysbus nodes are not added there.
- renaming of MACHVIRT_PLATFORM defines
- MACHVIRT_PLATFORM_PAGE_SHIFT and SIZE_PAGES not needed anymore,
  hence removed.
- DynSysbusParams struct renamed into ARMPlatformBusSystemParams
  and above params removed.
- separation of dt creation and QEMU binding is not mandated anymore
  since the device tree is not created from scratch anymore. Instead
  the modify_dtb function is used.
- create_platform_bus registers another machine init done notifier
  to start VFIO IRQ handling. This latter executes after the
  dynamic sysbus device binding.

v2 -> v3:
- renaming of arm_platform_bus_create_devtree and arm_load_dtb
- add copyright in hw/arm/dyn_sysbus_devtree.c

v1 -> v2:
- remove useless vfio-platform.h include file
- s/MACHVIRT_PLATFORM_HOLE/MACHVIRT_PLATFORM_SIZE
- use dyn_sysbus_binding and dyn_sysbus_devtree
- dynamic sysbus platform buse size shrinked to 4MB and
  moved between RTC and MMIO

v1:

Inspired from what Alex Graf did in ppc e500
https://lists.gnu.org/archive/html/qemu-ppc/2014-07/msg00012.html

Conflicts:
	hw/arm/sysbus-fdt.c
---
 hw/arm/virt.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 78f618d..3a09d58 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -42,6 +42,8 @@
 #include "exec/address-spaces.h"
 #include "qemu/bitops.h"
 #include "qemu/error-report.h"
+#include "hw/arm/sysbus-fdt.h"
+#include "hw/platform-bus.h"
 
 #define NUM_VIRTIO_TRANSPORTS 32
 
@@ -59,6 +61,11 @@
 #define GIC_FDT_IRQ_PPI_CPU_START 8
 #define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
 
+#define PLATFORM_BUS_BASE         0x9400000
+#define PLATFORM_BUS_SIZE         (4ULL * 1024 * 1024)
+#define PLATFORM_BUS_FIRST_IRQ    48
+#define PLATFORM_BUS_NUM_IRQS     20
+
 enum {
     VIRT_FLASH,
     VIRT_MEM,
@@ -68,6 +75,7 @@ enum {
     VIRT_UART,
     VIRT_MMIO,
     VIRT_RTC,
+    VIRT_PLATFORM_BUS,
 };
 
 typedef struct MemMapEntry {
@@ -107,6 +115,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
     [VIRT_UART] =       { 0x09000000, 0x00001000 },
     [VIRT_RTC] =        { 0x09010000, 0x00001000 },
+    [VIRT_PLATFORM_BUS] = {PLATFORM_BUS_BASE , PLATFORM_BUS_SIZE},
     [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     /* 0x10000000 .. 0x40000000 reserved for PCI */
@@ -117,6 +126,14 @@ static const int a15irqmap[] = {
     [VIRT_UART] = 1,
     [VIRT_RTC] = 2,
     [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
+    [VIRT_PLATFORM_BUS] = PLATFORM_BUS_FIRST_IRQ,
+};
+
+ARMPlatformBusSystemParams platform_bus_params = {
+    .platform_bus_base = PLATFORM_BUS_BASE,
+    .platform_bus_size = PLATFORM_BUS_SIZE,
+    .platform_bus_first_irq = PLATFORM_BUS_FIRST_IRQ,
+    .platform_bus_num_irqs = PLATFORM_BUS_NUM_IRQS,
 };
 
 static VirtBoardInfo machines[] = {
@@ -519,6 +536,45 @@ static void create_flash(const VirtBoardInfo *vbi)
     g_free(nodename);
 }
 
+static void create_platform_bus(VirtBoardInfo *vbi, qemu_irq *pic,
+                                ARMPlatformBusSystemParams *system_params)
+{
+    DeviceState *dev;
+    SysBusDevice *s;
+    int i;
+    ARMPlatformBusFdtParams *fdt_params = g_new(ARMPlatformBusFdtParams, 1);
+    MemoryRegion *sysmem = get_system_memory();
+
+    /*
+     * register the notifier that will update the device tree with
+     * the platform bus and device tree nodes. Must be done before
+     * the instantiation of the platform bus device that registers
+     * the notifier that instantiates the dynamic sysbus devices
+     */
+    fdt_params->system_params = system_params;
+    fdt_params->binfo = &vbi->bootinfo;
+    fdt_params->intc = "/intc";
+    arm_register_platform_bus_fdt_creator(fdt_params);
+
+    dev = qdev_create(NULL, TYPE_PLATFORM_BUS_DEVICE);
+    dev->id = TYPE_PLATFORM_BUS_DEVICE;
+    qdev_prop_set_uint32(dev, "num_irqs",
+        system_params->platform_bus_num_irqs);
+    qdev_prop_set_uint32(dev, "mmio_size",
+        system_params->platform_bus_size);
+    qdev_init_nofail(dev);
+    s = SYS_BUS_DEVICE(dev);
+
+    for (i = 0; i < system_params->platform_bus_num_irqs; i++) {
+        int irqn = system_params->platform_bus_first_irq + i;
+        sysbus_connect_irq(s, i, pic[irqn]);
+    }
+
+    memory_region_add_subregion(sysmem,
+                                system_params->platform_bus_base,
+                                sysbus_mmio_get_region(s, 0));
+}
+
 static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
 {
     const VirtBoardInfo *board = (const VirtBoardInfo *)binfo;
@@ -604,6 +660,8 @@ static void machvirt_init(MachineState *machine)
      */
     create_virtio_devices(vbi, pic);
 
+    create_platform_bus(vbi, pic, &platform_bus_params);
+
     vbi->bootinfo.ram_size = machine->ram_size;
     vbi->bootinfo.kernel_filename = machine->kernel_filename;
     vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
@@ -620,6 +678,7 @@ static QEMUMachine machvirt_a15_machine = {
     .desc = "ARM Virtual Machine",
     .init = machvirt_init,
     .max_cpus = 8,
+    .has_dynamic_sysbus = true,
 };
 
 static void machvirt_machine_init(void)
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v4 6/6] hw/arm/virt: add dynamic sysbus device support

[Alexander Graf]

On 31.10.14 14:53, Eric Auger wrote:
> Allows sysbus devices to be instantiated from command line by
> using -device option. Machvirt creates a platform bus at init.
> The dynamic sysbus devices are attached to a platform bus device.
> 
> The platform bus device registers a machine init done notifier
> whose role will be to bind the dynamic sysbus devices. Indeed
> dynamic sysbus devices are created after machine init.
> 
> machvirt also registers a notifier that will start the VFIO
> dynamic device IRQ handling.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> v3 -> v4:
> - use platform bus object, instantiated in create_platform_bus
> - device tree generation for platform bus and children dynamic
>   sysbus devices is no more handled at reset but in a
>   machine_init_done_notifier (due to the change in implementaion
>   of ARM load dtb using rom_add_blob_fixed).
> - device tree enhancement now takes into account the case of
>   user provided dtb. Before the user dtb was overwritten which
>   was wrong. However in case the dtb is provided by the user,
>   dynamic sysbus nodes are not added there.
> - renaming of MACHVIRT_PLATFORM defines
> - MACHVIRT_PLATFORM_PAGE_SHIFT and SIZE_PAGES not needed anymore,
>   hence removed.
> - DynSysbusParams struct renamed into ARMPlatformBusSystemParams
>   and above params removed.
> - separation of dt creation and QEMU binding is not mandated anymore
>   since the device tree is not created from scratch anymore. Instead
>   the modify_dtb function is used.
> - create_platform_bus registers another machine init done notifier
>   to start VFIO IRQ handling. This latter executes after the
>   dynamic sysbus device binding.
> 
> v2 -> v3:
> - renaming of arm_platform_bus_create_devtree and arm_load_dtb
> - add copyright in hw/arm/dyn_sysbus_devtree.c
> 
> v1 -> v2:
> - remove useless vfio-platform.h include file
> - s/MACHVIRT_PLATFORM_HOLE/MACHVIRT_PLATFORM_SIZE
> - use dyn_sysbus_binding and dyn_sysbus_devtree
> - dynamic sysbus platform buse size shrinked to 4MB and
>   moved between RTC and MMIO
> 
> v1:
> 
> Inspired from what Alex Graf did in ppc e500
> https://lists.gnu.org/archive/html/qemu-ppc/2014-07/msg00012.html
> 
> Conflicts:
> 	hw/arm/sysbus-fdt.c
> ---
>  hw/arm/virt.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 78f618d..3a09d58 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -42,6 +42,8 @@
>  #include "exec/address-spaces.h"
>  #include "qemu/bitops.h"
>  #include "qemu/error-report.h"
> +#include "hw/arm/sysbus-fdt.h"
> +#include "hw/platform-bus.h"
>  
>  #define NUM_VIRTIO_TRANSPORTS 32
>  
> @@ -59,6 +61,11 @@
>  #define GIC_FDT_IRQ_PPI_CPU_START 8
>  #define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
>  
> +#define PLATFORM_BUS_BASE         0x9400000
> +#define PLATFORM_BUS_SIZE         (4ULL * 1024 * 1024)
> +#define PLATFORM_BUS_FIRST_IRQ    48
> +#define PLATFORM_BUS_NUM_IRQS     20
> +
>  enum {
>      VIRT_FLASH,
>      VIRT_MEM,
> @@ -68,6 +75,7 @@ enum {
>      VIRT_UART,
>      VIRT_MMIO,
>      VIRT_RTC,
> +    VIRT_PLATFORM_BUS,
>  };
>  
>  typedef struct MemMapEntry {
> @@ -107,6 +115,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
>      [VIRT_UART] =       { 0x09000000, 0x00001000 },
>      [VIRT_RTC] =        { 0x09010000, 0x00001000 },
> +    [VIRT_PLATFORM_BUS] = {PLATFORM_BUS_BASE , PLATFORM_BUS_SIZE},
>      [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      /* 0x10000000 .. 0x40000000 reserved for PCI */
> @@ -117,6 +126,14 @@ static const int a15irqmap[] = {
>      [VIRT_UART] = 1,
>      [VIRT_RTC] = 2,
>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
> +    [VIRT_PLATFORM_BUS] = PLATFORM_BUS_FIRST_IRQ,
> +};
> +
> +ARMPlatformBusSystemParams platform_bus_params = {

static const?


Alex

Re: [Qemu-devel] [PATCH v4 5/6] hw/arm/sysbus-fdt: helpers for platform bus nodes addition

[Alexander Graf]

On 31.10.14 14:53, Eric Auger wrote:
> This new C module will be used by ARM machine files to generate
> platform bus node and their dynamic sysbus device tree nodes.
> 
> Dynamic sysbus device node addition is done in a machine init
> done notifier. arm_register_platform_bus_fdt_creator does the
> registration of this latter and is supposed to be called by
> ARM machine files that support platform bus and their dynamic
> sysbus. Addition of dynamic sysbus nodes is done only if the
> user did not provide any dtb.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v3 -> v4:
> - dyn_sysbus_devtree.c renamed into sysbus-fdt.c
> - use new PlatformBusDevice object
> - the dtb upgrade is done through modify_dtb. Before the fdt
>   was recreated from scratch. When the user provided a dtb this
>   latter was overwritten which was not correct.
> - an array contains the association between device type names
>   and their node creation function
> - I must aknowledge I did not find any cleaner way to implement
>   a FDT_BUILDER interface, as suggested by Paolo. The class method
>   would need to be initialized somewhere and since it cannot
>   happen in the device itself - according to Alex & Peter comments -,
>   I don't see when I shall associate the device type and its
>   interface implementation.
> 
> 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]
> ---
>  hw/arm/Makefile.objs        |   1 +
>  hw/arm/sysbus-fdt.c         | 181 ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/sysbus-fdt.h |  50 ++++++++++++
>  3 files changed, 232 insertions(+)
>  create mode 100644 hw/arm/sysbus-fdt.c
>  create mode 100644 include/hw/arm/sysbus-fdt.h
> 
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 6088e53..0cc63e1 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 += sysbus-fdt.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/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> new file mode 100644
> index 0000000..d5476f1
> --- /dev/null
> +++ b/hw/arm/sysbus-fdt.c
> @@ -0,0 +1,181 @@
> +/*
> + * ARM Platform Bus device tree generation helpers
> + *
> + * Copyright (c) 2014 Linaro Limited
> + *
> + * Authors:
> + *  Alex Graf <agraf@suse.de>
> + *  Eric Auger <eric.auger@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "hw/arm/sysbus-fdt.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/device_tree.h"
> +#include "hw/platform-bus.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/platform-bus.h"
> +
> +/*
> + * internal struct that contains the information to create dynamic
> + * sysbus device node
> + */
> +typedef struct PlatformBusFdtData {
> +    void *fdt; /* device tree handle */
> +    int irq_start; /* index of the first IRQ usable by platform bus devices */
> +    const char *pbus_node_name; /* name of the platform bus node */
> +    PlatformBusDevice *pbus;
> +} PlatformBusFdtData;
> +
> +/*
> + * struct used when calling the machine init done notifier
> + * that constructs the fdt nodes of platform bus devices
> + */
> +typedef struct PlatformBusFdtNotifierParams {
> +    ARMPlatformBusFdtParams *fdt_params;
> +    Notifier notifier;
> +} PlatformBusFdtNotifierParams;
> +
> +/* struct that associates a device type name and a node creation function */
> +typedef struct NodeCreationPair {
> +    const char *typename;
> +    int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
> +} NodeCreationPair;
> +
> +/* list of supported dynamic sysbus devices */
> +NodeCreationPair add_fdt_node_functions[] = {
> +        {"", NULL}, /*last element*/

s/    // :)


Alex

Re: [Qemu-devel] [PATCH v4 0/6] machvirt dynamic sysbus device instantiation

[Alexander Graf]

On 31.10.14 14:53, Eric Auger wrote:
> This patch series enables machvirt to dynamically instantiate sysbus
> devices from command line (using -device option).
> 
> All those sysbus devices are plugged onto a platform bus. This latter
> device is instantiated in machvirt and takes care of the binding of
> children sysbus devices on a machine init done notifier. The device
> tree node generation for children dynamic sysbus device also happens
> on a subsequent notifier that must be executed after the above one.
> machvirt registers that notifier before the platform bus creation to
> make sure notifiers are executed in the right order: dt generation
> after actual QOM binding.
> 
> Very few sysbus devices are supposed to be instantiated that way.
> VFIO devices belong to them.
> 
> Node creation really is architecture specific. On ARM the dynamic
> sysbus device node creation is implemented in a new C module,
> hw/arm/sysbus-fdt.c and not in the machine file.
> 
> This series applies on top of Alex Graf's series
> [PATCH v3 0/7] Dynamic sysbus device allocation support
> http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg04860.html
> 
> Machvirt transformations and sysbus-fdt are largely inspired from Alex work.
> 
> The patch series can be found at:
> http://git.linaro.org/people/eric.auger/qemu.git (branch vfio_integ_v7)

Overall the approach looks sane to me. I'm not 100% convinced it's a
good idea to make the fdt generation "arm generic", but I'll leave that
for Peter to decide. On PPC this definitely wouldn't fly with all the
different subarchs. I'm not sure whether ARM is more standardized on the
device tree generation, especially with different #address-size
properties and the likes.


Alex

Re: [Qemu-devel] [PATCH v4 1/6] hw/arm/boot: load_dtb becomes non static arm_load_dtb

[Shannon Zhao]

Hi Eric,

On 2014/10/31 21:53, Eric Auger wrote:
> load_dtb is renamed into arm_load_dtb and becomes non static.
> it will be used by machvirt for dynamic instantiation of
> platform devices
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v2 -> v3:
> load_dtb renamed into arm_load_dtb
> 
> Conflicts:
> 	hw/arm/boot.c
> ---
>  hw/arm/boot.c        | 12 ++++++------
>  include/hw/arm/arm.h |  2 ++
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index bffbea5..f5714ea 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -313,7 +313,7 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
>  }
>  
>  /**
> - * load_dtb() - load a device tree binary image into memory
> + * arm_load_dtb() - load a device tree binary image into memory
>   * @addr:       the address to load the image at
>   * @binfo:      struct describing the boot environment
>   * @addr_limit: upper limit of the available memory area at @addr
> @@ -330,8 +330,8 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
>   *          0 if the image size exceeds the limit,
>   *          -1 on errors.
>   */
> -static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> -                    hwaddr addr_limit)
> +int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> +                 hwaddr addr_limit)
>  {
>      void *fdt = NULL;
>      int size, rc;
> @@ -504,7 +504,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>              /* If we have a device tree blob, but no kernel to supply it to,
>               * copy it to the base of RAM for a bootloader to pick up.
>               */
> -            if (load_dtb(info->loader_start, info, 0) < 0) {
> +            if (arm_load_dtb(info->loader_start, info, 0) < 0) {
>                  exit(1);
>              }
>          }
> @@ -572,7 +572,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>              if (elf_low_addr < info->loader_start) {
>                  elf_low_addr = 0;
>              }
> -            if (load_dtb(info->loader_start, info, elf_low_addr) < 0) {
> +            if (arm_load_dtb(info->loader_start, info, elf_low_addr) < 0) {
>                  exit(1);
>              }
>          }

There is a "load_dtb" which is not updated.

            /* Pass elf_low_addr as address limit to load_dtb if it may be
             * pointing into RAM, otherwise pass '0' (no limit)
             */

Thanks,
Shannon

Re: [Qemu-devel] [PATCH v4 1/6] hw/arm/boot: load_dtb becomes non static arm_load_dtb

[Eric Auger]

On 11/27/2014 10:00 AM, Shannon Zhao wrote:
> Hi Eric,
> 
> On 2014/10/31 21:53, Eric Auger wrote:
>> load_dtb is renamed into arm_load_dtb and becomes non static.
>> it will be used by machvirt for dynamic instantiation of
>> platform devices
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v2 -> v3:
>> load_dtb renamed into arm_load_dtb
>>
>> Conflicts:
>> 	hw/arm/boot.c
>> ---
>>  hw/arm/boot.c        | 12 ++++++------
>>  include/hw/arm/arm.h |  2 ++
>>  2 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index bffbea5..f5714ea 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -313,7 +313,7 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
>>  }
>>  
>>  /**
>> - * load_dtb() - load a device tree binary image into memory
>> + * arm_load_dtb() - load a device tree binary image into memory
>>   * @addr:       the address to load the image at
>>   * @binfo:      struct describing the boot environment
>>   * @addr_limit: upper limit of the available memory area at @addr
>> @@ -330,8 +330,8 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
>>   *          0 if the image size exceeds the limit,
>>   *          -1 on errors.
>>   */
>> -static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>> -                    hwaddr addr_limit)
>> +int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>> +                 hwaddr addr_limit)
>>  {
>>      void *fdt = NULL;
>>      int size, rc;
>> @@ -504,7 +504,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>              /* If we have a device tree blob, but no kernel to supply it to,
>>               * copy it to the base of RAM for a bootloader to pick up.
>>               */
>> -            if (load_dtb(info->loader_start, info, 0) < 0) {
>> +            if (arm_load_dtb(info->loader_start, info, 0) < 0) {
>>                  exit(1);
>>              }
>>          }
>> @@ -572,7 +572,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>              if (elf_low_addr < info->loader_start) {
>>                  elf_low_addr = 0;
>>              }
>> -            if (load_dtb(info->loader_start, info, elf_low_addr) < 0) {
>> +            if (arm_load_dtb(info->loader_start, info, elf_low_addr) < 0) {
>>                  exit(1);
>>              }
>>          }
> 
> There is a "load_dtb" which is not updated.
Hi Shannon,

you mean in below comment, right?

Thanks

Eric
> 
>             /* Pass elf_low_addr as address limit to load_dtb if it may be
>              * pointing into RAM, otherwise pass '0' (no limit)
>              */
> 
> Thanks,
> Shannon
> 
> 

Re: [Qemu-devel] [PATCH v4 1/6] hw/arm/boot: load_dtb becomes non static arm_load_dtb

[Shannon Zhao]

On 2014/11/27 17:19, Eric Auger wrote:
> On 11/27/2014 10:00 AM, Shannon Zhao wrote:
>> Hi Eric,
>>
>> On 2014/10/31 21:53, Eric Auger wrote:
>>> load_dtb is renamed into arm_load_dtb and becomes non static.
>>> it will be used by machvirt for dynamic instantiation of
>>> platform devices
>>>
>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>
>>> ---
>>>
>>> v2 -> v3:
>>> load_dtb renamed into arm_load_dtb
>>>
>>> Conflicts:
>>> 	hw/arm/boot.c
>>> ---
>>>  hw/arm/boot.c        | 12 ++++++------
>>>  include/hw/arm/arm.h |  2 ++
>>>  2 files changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>>> index bffbea5..f5714ea 100644
>>> --- a/hw/arm/boot.c
>>> +++ b/hw/arm/boot.c
>>> @@ -313,7 +313,7 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
>>>  }
>>>  
>>>  /**
>>> - * load_dtb() - load a device tree binary image into memory
>>> + * arm_load_dtb() - load a device tree binary image into memory
>>>   * @addr:       the address to load the image at
>>>   * @binfo:      struct describing the boot environment
>>>   * @addr_limit: upper limit of the available memory area at @addr
>>> @@ -330,8 +330,8 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
>>>   *          0 if the image size exceeds the limit,
>>>   *          -1 on errors.
>>>   */
>>> -static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>> -                    hwaddr addr_limit)
>>> +int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>> +                 hwaddr addr_limit)
>>>  {
>>>      void *fdt = NULL;
>>>      int size, rc;
>>> @@ -504,7 +504,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>>              /* If we have a device tree blob, but no kernel to supply it to,
>>>               * copy it to the base of RAM for a bootloader to pick up.
>>>               */
>>> -            if (load_dtb(info->loader_start, info, 0) < 0) {
>>> +            if (arm_load_dtb(info->loader_start, info, 0) < 0) {
>>>                  exit(1);
>>>              }
>>>          }
>>> @@ -572,7 +572,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>>              if (elf_low_addr < info->loader_start) {
>>>                  elf_low_addr = 0;
>>>              }
>>> -            if (load_dtb(info->loader_start, info, elf_low_addr) < 0) {
>>> +            if (arm_load_dtb(info->loader_start, info, elf_low_addr) < 0) {
>>>                  exit(1);
>>>              }
>>>          }
>>
>> There is a "load_dtb" which is not updated.
> Hi Shannon,
> 
> you mean in below comment, right?
> 

Yes:-)

> Thanks
> 
> Eric
>>
>>             /* Pass elf_low_addr as address limit to load_dtb if it may be
>>              * pointing into RAM, otherwise pass '0' (no limit)
>>              */
>>
>> Thanks,
>> Shannon
>>
>>
> 
> 
> .
> 


-- 
Shannon

Re: [Qemu-devel] [PATCH v4 1/6] hw/arm/boot: load_dtb becomes non static arm_load_dtb

[Eric Auger]

On 11/27/2014 11:17 AM, Shannon Zhao wrote:
> On 2014/11/27 17:19, Eric Auger wrote:
>> On 11/27/2014 10:00 AM, Shannon Zhao wrote:
>>> Hi Eric,
>>>
>>> On 2014/10/31 21:53, Eric Auger wrote:
>>>> load_dtb is renamed into arm_load_dtb and becomes non static.
>>>> it will be used by machvirt for dynamic instantiation of
>>>> platform devices
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>>
>>>> ---
>>>>
>>>> v2 -> v3:
>>>> load_dtb renamed into arm_load_dtb
>>>>
>>>> Conflicts:
>>>> 	hw/arm/boot.c
>>>> ---
>>>>  hw/arm/boot.c        | 12 ++++++------
>>>>  include/hw/arm/arm.h |  2 ++
>>>>  2 files changed, 8 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>>>> index bffbea5..f5714ea 100644
>>>> --- a/hw/arm/boot.c
>>>> +++ b/hw/arm/boot.c
>>>> @@ -313,7 +313,7 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
>>>>  }
>>>>  
>>>>  /**
>>>> - * load_dtb() - load a device tree binary image into memory
>>>> + * arm_load_dtb() - load a device tree binary image into memory
>>>>   * @addr:       the address to load the image at
>>>>   * @binfo:      struct describing the boot environment
>>>>   * @addr_limit: upper limit of the available memory area at @addr
>>>> @@ -330,8 +330,8 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
>>>>   *          0 if the image size exceeds the limit,
>>>>   *          -1 on errors.
>>>>   */
>>>> -static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>>> -                    hwaddr addr_limit)
>>>> +int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>>> +                 hwaddr addr_limit)
>>>>  {
>>>>      void *fdt = NULL;
>>>>      int size, rc;
>>>> @@ -504,7 +504,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>>>              /* If we have a device tree blob, but no kernel to supply it to,
>>>>               * copy it to the base of RAM for a bootloader to pick up.
>>>>               */
>>>> -            if (load_dtb(info->loader_start, info, 0) < 0) {
>>>> +            if (arm_load_dtb(info->loader_start, info, 0) < 0) {
>>>>                  exit(1);
>>>>              }
>>>>          }
>>>> @@ -572,7 +572,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>>>              if (elf_low_addr < info->loader_start) {
>>>>                  elf_low_addr = 0;
>>>>              }
>>>> -            if (load_dtb(info->loader_start, info, elf_low_addr) < 0) {
>>>> +            if (arm_load_dtb(info->loader_start, info, elf_low_addr) < 0) {
>>>>                  exit(1);
>>>>              }
>>>>          }
>>>
>>> There is a "load_dtb" which is not updated.
>> Hi Shannon,
>>
>> you mean in below comment, right?

OK thanks for your time.

I will correct

Best Regards

Eric
>>
> 
> Yes:-)
> 
>> Thanks
>>
>> Eric
>>>
>>>             /* Pass elf_low_addr as address limit to load_dtb if it may be
>>>              * pointing into RAM, otherwise pass '0' (no limit)
>>>              */
>>>
>>> Thanks,
>>> Shannon
>>>
>>>
>>
>>
>> .
>>
> 
> 

Re: [Qemu-devel] [PATCH v4 5/6] hw/arm/sysbus-fdt: helpers for platform bus nodes addition

[Shannon Zhao]

On 2014/10/31 21:53, Eric Auger wrote:
> This new C module will be used by ARM machine files to generate
> platform bus node and their dynamic sysbus device tree nodes.
> 
> Dynamic sysbus device node addition is done in a machine init
> done notifier. arm_register_platform_bus_fdt_creator does the
> registration of this latter and is supposed to be called by
> ARM machine files that support platform bus and their dynamic
> sysbus. Addition of dynamic sysbus nodes is done only if the
> user did not provide any dtb.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v3 -> v4:
> - dyn_sysbus_devtree.c renamed into sysbus-fdt.c
> - use new PlatformBusDevice object
> - the dtb upgrade is done through modify_dtb. Before the fdt
>   was recreated from scratch. When the user provided a dtb this
>   latter was overwritten which was not correct.
> - an array contains the association between device type names
>   and their node creation function
> - I must aknowledge I did not find any cleaner way to implement
>   a FDT_BUILDER interface, as suggested by Paolo. The class method
>   would need to be initialized somewhere and since it cannot
>   happen in the device itself - according to Alex & Peter comments -,
>   I don't see when I shall associate the device type and its
>   interface implementation.
> 
> 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]
> ---
>  hw/arm/Makefile.objs        |   1 +
>  hw/arm/sysbus-fdt.c         | 181 ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/sysbus-fdt.h |  50 ++++++++++++
>  3 files changed, 232 insertions(+)
>  create mode 100644 hw/arm/sysbus-fdt.c
>  create mode 100644 include/hw/arm/sysbus-fdt.h
> 
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 6088e53..0cc63e1 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 += sysbus-fdt.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/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> new file mode 100644
> index 0000000..d5476f1
> --- /dev/null
> +++ b/hw/arm/sysbus-fdt.c
> @@ -0,0 +1,181 @@
> +/*
> + * ARM Platform Bus device tree generation helpers
> + *
> + * Copyright (c) 2014 Linaro Limited
> + *
> + * Authors:
> + *  Alex Graf <agraf@suse.de>
> + *  Eric Auger <eric.auger@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "hw/arm/sysbus-fdt.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/device_tree.h"
> +#include "hw/platform-bus.h"
[*]
> +#include "sysemu/sysemu.h"
> +#include "hw/platform-bus.h"
[*]
Duplicate include "hw/platform-bus.h"
> +
> +/*
> + * internal struct that contains the information to create dynamic
> + * sysbus device node
> + */
> +typedef struct PlatformBusFdtData {
> +    void *fdt; /* device tree handle */
> +    int irq_start; /* index of the first IRQ usable by platform bus devices */
> +    const char *pbus_node_name; /* name of the platform bus node */
> +    PlatformBusDevice *pbus;
> +} PlatformBusFdtData;
> +
> +/*
> + * struct used when calling the machine init done notifier
> + * that constructs the fdt nodes of platform bus devices
> + */
> +typedef struct PlatformBusFdtNotifierParams {
> +    ARMPlatformBusFdtParams *fdt_params;
> +    Notifier notifier;
> +} PlatformBusFdtNotifierParams;
> +
> +/* struct that associates a device type name and a node creation function */
> +typedef struct NodeCreationPair {
> +    const char *typename;
> +    int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
> +} NodeCreationPair;
> +
> +/* list of supported dynamic sysbus devices */
> +NodeCreationPair add_fdt_node_functions[] = {
> +        {"", NULL}, /*last element*/
> +};

Eric, I have a question about how to use this.
If I want to dynamically add a device, I must add a member in above array, such as {TYPE_PFD, pfd_add_fdt_node}.
And the "pfd_add_fdt_node" is defined by myself which is used to dynamically generate the device fdt node.

Am I right ?

Thanks,
Shannon

> +
> +/**
> + * add_fdt_node - add the device tree node of a dynamic sysbus device
> + *
> + * @sbdev: handle to the sysbus device
> + * @opaque: handle to the PlatformBusFdtData
> + *
> + * Checks the sysbus type belongs to the list of device types that
> + * are dynamically instantiable and in the positive call the node
> + * creation function.
> + */
> +static int add_fdt_node(SysBusDevice *sbdev, void *opaque)
> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(add_fdt_node_functions); i++) {
> +        if (!strcmp(object_get_typename(OBJECT(sbdev)),
> +                    add_fdt_node_functions[i].typename)) {
> +            add_fdt_node_functions[i].add_fdt_node_fn(sbdev, opaque);
> +            return 0;
> +        }
> +    }
> +    error_report("Device %s can not be dynamically instantiated",
> +                     qdev_fw_name(DEVICE(sbdev)));
> +    return 1;
> +}
> +
> +/**
> + * add_all_platform_bus_fdt_nodes - create all the platform bus nodes
> + *
> + * builds the parent platform bus node and all the nodes of dynamic
> + * sysbus devices attached * to it. It is a modify_dtb() function, ie.
> + * called * by arm_load_dtb()
> + */
> +static void add_all_platform_bus_fdt_nodes(const struct arm_boot_info *info,
> +                                           void *fdt)
> +{
> +    const char platcomp[] = "qemu,platform\0simple-bus";
> +    ARMPlatformBusSystemParams *system_params;
> +    ARMPlatformBusFdtParams *fdt_params = info->modify_dtb_opaque;
> +    PlatformBusDevice *pbus;
> +    DeviceState *dev;
> +    gchar *node;
> +    uint64_t addr, size;
> +    int irq_start;
> +
> +    if (!fdt_params) {
> +        return;
> +    }
> +
> +    system_params = fdt_params->system_params;
> +    node = g_strdup_printf("/platform@%"PRIx64,
> +                               system_params->platform_bus_base);
> +    addr = system_params->platform_bus_base;
> +    size = system_params->platform_bus_size;
> +    irq_start = system_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", fdt_params->intc);
> +
> +    dev = qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE);
> +    pbus = PLATFORM_BUS_DEVICE(dev);
> +
> +    /* We can only create dt nodes for dynamic devices when they're ready */
> +    if (pbus->done_gathering) {
> +        PlatformBusFdtData data = {
> +            .fdt = fdt,
> +            .irq_start = irq_start,
> +            .pbus_node_name = node,
> +            .pbus = pbus,
> +        };
> +
> +        /* Loop through all dynamic sysbus devices and create their node */
> +        foreach_dynamic_sysbus_device(add_fdt_node, &data);
> +    }
> +
> +    g_free(node);
> +}
> +
> +static void upgrade_dtb(ARMPlatformBusFdtParams *params)
> +{
> +    struct arm_boot_info *binfo = params->binfo;
> +
> +    /*
> +     * In case the user provided a dtb, we assume he already integrated the
> +     * dynamic sysbus nodes. This corresponds to a use case where the dynamic
> +     * sysbus nodes are complex and their generation is not yet supported. In
> +     * case the use can take in charge the guest dt while qemu takes in charge
> +     * the qom stuff.
> +     */
> +    if (!binfo->dtb_filename) {
> +        binfo->modify_dtb_opaque = params;
> +        binfo->modify_dtb = add_all_platform_bus_fdt_nodes;
> +        arm_load_dtb(binfo);
> +    }
> +}
> +
> +static void platform_bus_fdt_notify(Notifier *notifier, void *data)
> +{
> +    PlatformBusFdtNotifierParams *p =
> +        container_of(notifier, PlatformBusFdtNotifierParams, notifier);
> +    upgrade_dtb(p->fdt_params);
> +}
> +
> +void arm_register_platform_bus_fdt_creator(ARMPlatformBusFdtParams *fdt_params)
> +{
> +    PlatformBusFdtNotifierParams *p = g_new(PlatformBusFdtNotifierParams, 1);
> +
> +    p->fdt_params = fdt_params;
> +    p->notifier.notify = platform_bus_fdt_notify;
> +    qemu_add_machine_init_done_notifier(&p->notifier);
> +}
> diff --git a/include/hw/arm/sysbus-fdt.h b/include/hw/arm/sysbus-fdt.h
> new file mode 100644
> index 0000000..b9602f1
> --- /dev/null
> +++ b/include/hw/arm/sysbus-fdt.h
> @@ -0,0 +1,50 @@
> +/*
> + * Dynamic sysbus device tree node generation API
> + *
> + * Copyright Linaro Limited, 2014
> + *
> + * Authors:
> + *  Alex Graf <agraf@suse.de>
> + *  Eric Auger <eric.auger@linaro.org>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef HW_ARM_SYSBUS_FDT_H
> +#define HW_ARM_SYSBUS_FDT_H
> +
> +#include "hw/arm/arm.h"
> +#include "qemu-common.h"
> +#include "hw/sysbus.h"
> +
> +/*
> + * struct that contains dimensioning parameters of the platform bus
> + */
> +typedef struct {
> +    hwaddr platform_bus_base; /* start address of the bus */
> +    hwaddr platform_bus_size; /* size of the bus */
> +    int platform_bus_first_irq; /* first hwirq assigned to the bus */
> +    int platform_bus_num_irqs; /* number of hwirq assigned to the bus */
> +} ARMPlatformBusSystemParams;
> +
> +/*
> + * struct that contains all relevant info to build the fdt nodes of
> + * platform bus and attached dynamic sysbus devices
> + * in the future might be augmented with additional info
> + * such as PHY, CLK handles ...
> + */
> +typedef struct {
> +    ARMPlatformBusSystemParams *system_params;
> +    struct arm_boot_info *binfo;
> +    const char *intc; /* parent interrupt controller name */
> +} ARMPlatformBusFdtParams;
> +
> +/**
> + * arm_register_platform_bus_fdt_creator - register a machine init done
> + * notifier that creates the device tree nodes of the platform bus and
> + * associated dynamic sysbus devices
> + */
> +void arm_register_platform_bus_fdt_creator(ARMPlatformBusFdtParams *fdt_params);
> +
> +#endif
> 


-- 
Shannon

Re: [Qemu-devel] [PATCH v4 5/6] hw/arm/sysbus-fdt: helpers for platform bus nodes addition

[Eric Auger]

On 11/27/2014 01:07 PM, Shannon Zhao wrote:
> On 2014/10/31 21:53, Eric Auger wrote:
>> This new C module will be used by ARM machine files to generate
>> platform bus node and their dynamic sysbus device tree nodes.
>>
>> Dynamic sysbus device node addition is done in a machine init
>> done notifier. arm_register_platform_bus_fdt_creator does the
>> registration of this latter and is supposed to be called by
>> ARM machine files that support platform bus and their dynamic
>> sysbus. Addition of dynamic sysbus nodes is done only if the
>> user did not provide any dtb.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v3 -> v4:
>> - dyn_sysbus_devtree.c renamed into sysbus-fdt.c
>> - use new PlatformBusDevice object
>> - the dtb upgrade is done through modify_dtb. Before the fdt
>>   was recreated from scratch. When the user provided a dtb this
>>   latter was overwritten which was not correct.
>> - an array contains the association between device type names
>>   and their node creation function
>> - I must aknowledge I did not find any cleaner way to implement
>>   a FDT_BUILDER interface, as suggested by Paolo. The class method
>>   would need to be initialized somewhere and since it cannot
>>   happen in the device itself - according to Alex & Peter comments -,
>>   I don't see when I shall associate the device type and its
>>   interface implementation.
>>
>> 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]
>> ---
>>  hw/arm/Makefile.objs        |   1 +
>>  hw/arm/sysbus-fdt.c         | 181 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/arm/sysbus-fdt.h |  50 ++++++++++++
>>  3 files changed, 232 insertions(+)
>>  create mode 100644 hw/arm/sysbus-fdt.c
>>  create mode 100644 include/hw/arm/sysbus-fdt.h
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index 6088e53..0cc63e1 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 += sysbus-fdt.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/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>> new file mode 100644
>> index 0000000..d5476f1
>> --- /dev/null
>> +++ b/hw/arm/sysbus-fdt.c
>> @@ -0,0 +1,181 @@
>> +/*
>> + * ARM Platform Bus device tree generation helpers
>> + *
>> + * Copyright (c) 2014 Linaro Limited
>> + *
>> + * Authors:
>> + *  Alex Graf <agraf@suse.de>
>> + *  Eric Auger <eric.auger@linaro.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2 or later, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>> + *
>> + */
>> +
>> +#include "hw/arm/sysbus-fdt.h"
>> +#include "qemu/error-report.h"
>> +#include "sysemu/device_tree.h"
>> +#include "hw/platform-bus.h"
> [*]
>> +#include "sysemu/sysemu.h"
>> +#include "hw/platform-bus.h"
> [*]
> Duplicate include "hw/platform-bus.h"
Hi Shannon,

thanks for pointing this out
>> +
>> +/*
>> + * internal struct that contains the information to create dynamic
>> + * sysbus device node
>> + */
>> +typedef struct PlatformBusFdtData {
>> +    void *fdt; /* device tree handle */
>> +    int irq_start; /* index of the first IRQ usable by platform bus devices */
>> +    const char *pbus_node_name; /* name of the platform bus node */
>> +    PlatformBusDevice *pbus;
>> +} PlatformBusFdtData;
>> +
>> +/*
>> + * struct used when calling the machine init done notifier
>> + * that constructs the fdt nodes of platform bus devices
>> + */
>> +typedef struct PlatformBusFdtNotifierParams {
>> +    ARMPlatformBusFdtParams *fdt_params;
>> +    Notifier notifier;
>> +} PlatformBusFdtNotifierParams;
>> +
>> +/* struct that associates a device type name and a node creation function */
>> +typedef struct NodeCreationPair {
>> +    const char *typename;
>> +    int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>> +} NodeCreationPair;
>> +
>> +/* list of supported dynamic sysbus devices */
>> +NodeCreationPair add_fdt_node_functions[] = {
>> +        {"", NULL}, /*last element*/
>> +};
> 
> Eric, I have a question about how to use this.
> If I want to dynamically add a device, I must add a member in above array, such as {TYPE_PFD, pfd_add_fdt_node}.
> And the "pfd_add_fdt_node" is defined by myself which is used to dynamically generate the device fdt node.
> 
> Am I right ?
yes that's correct. You can find an example (Calxeda midway xgmac) in
[PATCH v7 12/16] hw/arm/sysbus-fdt: enable vfio-calxeda-xgmac dynamic
(https://patches.linaro.org/39910/).

I am currently reworking this according to Alex comments but the spirit
remains the same.

Please do not hesitate to come back to me if you have other questions.

Best Regards

Eric


> 
> Thanks,
> Shannon
> 
>> +
>> +/**
>> + * add_fdt_node - add the device tree node of a dynamic sysbus device
>> + *
>> + * @sbdev: handle to the sysbus device
>> + * @opaque: handle to the PlatformBusFdtData
>> + *
>> + * Checks the sysbus type belongs to the list of device types that
>> + * are dynamically instantiable and in the positive call the node
>> + * creation function.
>> + */
>> +static int add_fdt_node(SysBusDevice *sbdev, void *opaque)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(add_fdt_node_functions); i++) {
>> +        if (!strcmp(object_get_typename(OBJECT(sbdev)),
>> +                    add_fdt_node_functions[i].typename)) {
>> +            add_fdt_node_functions[i].add_fdt_node_fn(sbdev, opaque);
>> +            return 0;
>> +        }
>> +    }
>> +    error_report("Device %s can not be dynamically instantiated",
>> +                     qdev_fw_name(DEVICE(sbdev)));
>> +    return 1;
>> +}
>> +
>> +/**
>> + * add_all_platform_bus_fdt_nodes - create all the platform bus nodes
>> + *
>> + * builds the parent platform bus node and all the nodes of dynamic
>> + * sysbus devices attached * to it. It is a modify_dtb() function, ie.
>> + * called * by arm_load_dtb()
>> + */
>> +static void add_all_platform_bus_fdt_nodes(const struct arm_boot_info *info,
>> +                                           void *fdt)
>> +{
>> +    const char platcomp[] = "qemu,platform\0simple-bus";
>> +    ARMPlatformBusSystemParams *system_params;
>> +    ARMPlatformBusFdtParams *fdt_params = info->modify_dtb_opaque;
>> +    PlatformBusDevice *pbus;
>> +    DeviceState *dev;
>> +    gchar *node;
>> +    uint64_t addr, size;
>> +    int irq_start;
>> +
>> +    if (!fdt_params) {
>> +        return;
>> +    }
>> +
>> +    system_params = fdt_params->system_params;
>> +    node = g_strdup_printf("/platform@%"PRIx64,
>> +                               system_params->platform_bus_base);
>> +    addr = system_params->platform_bus_base;
>> +    size = system_params->platform_bus_size;
>> +    irq_start = system_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", fdt_params->intc);
>> +
>> +    dev = qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE);
>> +    pbus = PLATFORM_BUS_DEVICE(dev);
>> +
>> +    /* We can only create dt nodes for dynamic devices when they're ready */
>> +    if (pbus->done_gathering) {
>> +        PlatformBusFdtData data = {
>> +            .fdt = fdt,
>> +            .irq_start = irq_start,
>> +            .pbus_node_name = node,
>> +            .pbus = pbus,
>> +        };
>> +
>> +        /* Loop through all dynamic sysbus devices and create their node */
>> +        foreach_dynamic_sysbus_device(add_fdt_node, &data);
>> +    }
>> +
>> +    g_free(node);
>> +}
>> +
>> +static void upgrade_dtb(ARMPlatformBusFdtParams *params)
>> +{
>> +    struct arm_boot_info *binfo = params->binfo;
>> +
>> +    /*
>> +     * In case the user provided a dtb, we assume he already integrated the
>> +     * dynamic sysbus nodes. This corresponds to a use case where the dynamic
>> +     * sysbus nodes are complex and their generation is not yet supported. In
>> +     * case the use can take in charge the guest dt while qemu takes in charge
>> +     * the qom stuff.
>> +     */
>> +    if (!binfo->dtb_filename) {
>> +        binfo->modify_dtb_opaque = params;
>> +        binfo->modify_dtb = add_all_platform_bus_fdt_nodes;
>> +        arm_load_dtb(binfo);
>> +    }
>> +}
>> +
>> +static void platform_bus_fdt_notify(Notifier *notifier, void *data)
>> +{
>> +    PlatformBusFdtNotifierParams *p =
>> +        container_of(notifier, PlatformBusFdtNotifierParams, notifier);
>> +    upgrade_dtb(p->fdt_params);
>> +}
>> +
>> +void arm_register_platform_bus_fdt_creator(ARMPlatformBusFdtParams *fdt_params)
>> +{
>> +    PlatformBusFdtNotifierParams *p = g_new(PlatformBusFdtNotifierParams, 1);
>> +
>> +    p->fdt_params = fdt_params;
>> +    p->notifier.notify = platform_bus_fdt_notify;
>> +    qemu_add_machine_init_done_notifier(&p->notifier);
>> +}
>> diff --git a/include/hw/arm/sysbus-fdt.h b/include/hw/arm/sysbus-fdt.h
>> new file mode 100644
>> index 0000000..b9602f1
>> --- /dev/null
>> +++ b/include/hw/arm/sysbus-fdt.h
>> @@ -0,0 +1,50 @@
>> +/*
>> + * Dynamic sysbus device tree node generation API
>> + *
>> + * Copyright Linaro Limited, 2014
>> + *
>> + * Authors:
>> + *  Alex Graf <agraf@suse.de>
>> + *  Eric Auger <eric.auger@linaro.org>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef HW_ARM_SYSBUS_FDT_H
>> +#define HW_ARM_SYSBUS_FDT_H
>> +
>> +#include "hw/arm/arm.h"
>> +#include "qemu-common.h"
>> +#include "hw/sysbus.h"
>> +
>> +/*
>> + * struct that contains dimensioning parameters of the platform bus
>> + */
>> +typedef struct {
>> +    hwaddr platform_bus_base; /* start address of the bus */
>> +    hwaddr platform_bus_size; /* size of the bus */
>> +    int platform_bus_first_irq; /* first hwirq assigned to the bus */
>> +    int platform_bus_num_irqs; /* number of hwirq assigned to the bus */
>> +} ARMPlatformBusSystemParams;
>> +
>> +/*
>> + * struct that contains all relevant info to build the fdt nodes of
>> + * platform bus and attached dynamic sysbus devices
>> + * in the future might be augmented with additional info
>> + * such as PHY, CLK handles ...
>> + */
>> +typedef struct {
>> +    ARMPlatformBusSystemParams *system_params;
>> +    struct arm_boot_info *binfo;
>> +    const char *intc; /* parent interrupt controller name */
>> +} ARMPlatformBusFdtParams;
>> +
>> +/**
>> + * arm_register_platform_bus_fdt_creator - register a machine init done
>> + * notifier that creates the device tree nodes of the platform bus and
>> + * associated dynamic sysbus devices
>> + */
>> +void arm_register_platform_bus_fdt_creator(ARMPlatformBusFdtParams *fdt_params);
>> +
>> +#endif
>>
> 
> 

Re: [Qemu-devel] [PATCH v4 5/6] hw/arm/sysbus-fdt: helpers for platform bus nodes addition

[Shannon Zhao]

On 2014/11/27 20:25, Eric Auger wrote:
> On 11/27/2014 01:07 PM, Shannon Zhao wrote:
>> On 2014/10/31 21:53, Eric Auger wrote:
>>> This new C module will be used by ARM machine files to generate
>>> platform bus node and their dynamic sysbus device tree nodes.
>>>
>>> Dynamic sysbus device node addition is done in a machine init
>>> done notifier. arm_register_platform_bus_fdt_creator does the
>>> registration of this latter and is supposed to be called by
>>> ARM machine files that support platform bus and their dynamic
>>> sysbus. Addition of dynamic sysbus nodes is done only if the
>>> user did not provide any dtb.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>
>>> ---
>>>
>>> v3 -> v4:
>>> - dyn_sysbus_devtree.c renamed into sysbus-fdt.c
>>> - use new PlatformBusDevice object
>>> - the dtb upgrade is done through modify_dtb. Before the fdt
>>>   was recreated from scratch. When the user provided a dtb this
>>>   latter was overwritten which was not correct.
>>> - an array contains the association between device type names
>>>   and their node creation function
>>> - I must aknowledge I did not find any cleaner way to implement
>>>   a FDT_BUILDER interface, as suggested by Paolo. The class method
>>>   would need to be initialized somewhere and since it cannot
>>>   happen in the device itself - according to Alex & Peter comments -,
>>>   I don't see when I shall associate the device type and its
>>>   interface implementation.
>>>
>>> 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]
>>> ---
>>>  hw/arm/Makefile.objs        |   1 +
>>>  hw/arm/sysbus-fdt.c         | 181 ++++++++++++++++++++++++++++++++++++++++++++
>>>  include/hw/arm/sysbus-fdt.h |  50 ++++++++++++
>>>  3 files changed, 232 insertions(+)
>>>  create mode 100644 hw/arm/sysbus-fdt.c
>>>  create mode 100644 include/hw/arm/sysbus-fdt.h
>>>
>>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>>> index 6088e53..0cc63e1 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 += sysbus-fdt.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/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>>> new file mode 100644
>>> index 0000000..d5476f1
>>> --- /dev/null
>>> +++ b/hw/arm/sysbus-fdt.c
>>> @@ -0,0 +1,181 @@
>>> +/*
>>> + * ARM Platform Bus device tree generation helpers
>>> + *
>>> + * Copyright (c) 2014 Linaro Limited
>>> + *
>>> + * Authors:
>>> + *  Alex Graf <agraf@suse.de>
>>> + *  Eric Auger <eric.auger@linaro.org>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms and conditions of the GNU General Public License,
>>> + * version 2 or later, as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>>> + * more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License along with
>>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>>> + *
>>> + */
>>> +
>>> +#include "hw/arm/sysbus-fdt.h"
>>> +#include "qemu/error-report.h"
>>> +#include "sysemu/device_tree.h"
>>> +#include "hw/platform-bus.h"
>> [*]
>>> +#include "sysemu/sysemu.h"
>>> +#include "hw/platform-bus.h"
>> [*]
>> Duplicate include "hw/platform-bus.h"
> Hi Shannon,
> 
> thanks for pointing this out
>>> +
>>> +/*
>>> + * internal struct that contains the information to create dynamic
>>> + * sysbus device node
>>> + */
>>> +typedef struct PlatformBusFdtData {
>>> +    void *fdt; /* device tree handle */
>>> +    int irq_start; /* index of the first IRQ usable by platform bus devices */
>>> +    const char *pbus_node_name; /* name of the platform bus node */
>>> +    PlatformBusDevice *pbus;
>>> +} PlatformBusFdtData;
>>> +
>>> +/*
>>> + * struct used when calling the machine init done notifier
>>> + * that constructs the fdt nodes of platform bus devices
>>> + */
>>> +typedef struct PlatformBusFdtNotifierParams {
>>> +    ARMPlatformBusFdtParams *fdt_params;
>>> +    Notifier notifier;
>>> +} PlatformBusFdtNotifierParams;
>>> +
>>> +/* struct that associates a device type name and a node creation function */
>>> +typedef struct NodeCreationPair {
>>> +    const char *typename;
>>> +    int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>>> +} NodeCreationPair;
>>> +
>>> +/* list of supported dynamic sysbus devices */
>>> +NodeCreationPair add_fdt_node_functions[] = {
>>> +        {"", NULL}, /*last element*/
>>> +};
>>
>> Eric, I have a question about how to use this.
>> If I want to dynamically add a device, I must add a member in above array, such as {TYPE_PFD, pfd_add_fdt_node}.
>> And the "pfd_add_fdt_node" is defined by myself which is used to dynamically generate the device fdt node.
>>
>> Am I right ?
> yes that's correct. You can find an example (Calxeda midway xgmac) in
> [PATCH v7 12/16] hw/arm/sysbus-fdt: enable vfio-calxeda-xgmac dynamic
> (https://patches.linaro.org/39910/).
> 
Hi Eric,

Thanks for your reply.

Assuming that I want to add a device "TYPE_PFD". I create a pfd.c in hw/arm/.

As the "pfd_add_fdt_node" is associated with device "TYPE_PFD", so I just want to let it in pfd.c.
But the struct PlatformBusFdtData is an internal struct which means I have to move "pfd_add_fdt_node"
to sysbus-fdt.c. If so, I think it's a little interleaving. And the file sysbus-fdt.c is a little messy.

What do you think about moving the associated struct to sysbus-fdt.h ?

And I think the array add_fdt_node_functions is not convenient to add a new add_fdt_node_fn for a new device.
Maybe a global register function would be better.

Thanks,
Shannon

> I am currently reworking this according to Alex comments but the spirit
> remains the same.
> 
> Please do not hesitate to come back to me if you have other questions.
> 
> Best Regards
> 
> Eric
> 
> 


-- 
Shannon

Re: [Qemu-devel] [PATCH v4 5/6] hw/arm/sysbus-fdt: helpers for platform bus nodes addition

[Alexander Graf]

On 27.11.14 13:56, Shannon Zhao wrote:
> On 2014/11/27 20:25, Eric Auger wrote:
>> On 11/27/2014 01:07 PM, Shannon Zhao wrote:
>>> On 2014/10/31 21:53, Eric Auger wrote:
>>>> This new C module will be used by ARM machine files to generate
>>>> platform bus node and their dynamic sysbus device tree nodes.
>>>>
>>>> Dynamic sysbus device node addition is done in a machine init
>>>> done notifier. arm_register_platform_bus_fdt_creator does the
>>>> registration of this latter and is supposed to be called by
>>>> ARM machine files that support platform bus and their dynamic
>>>> sysbus. Addition of dynamic sysbus nodes is done only if the
>>>> user did not provide any dtb.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>>
>>>> ---
>>>>
>>>> v3 -> v4:
>>>> - dyn_sysbus_devtree.c renamed into sysbus-fdt.c
>>>> - use new PlatformBusDevice object
>>>> - the dtb upgrade is done through modify_dtb. Before the fdt
>>>>   was recreated from scratch. When the user provided a dtb this
>>>>   latter was overwritten which was not correct.
>>>> - an array contains the association between device type names
>>>>   and their node creation function
>>>> - I must aknowledge I did not find any cleaner way to implement
>>>>   a FDT_BUILDER interface, as suggested by Paolo. The class method
>>>>   would need to be initialized somewhere and since it cannot
>>>>   happen in the device itself - according to Alex & Peter comments -,
>>>>   I don't see when I shall associate the device type and its
>>>>   interface implementation.
>>>>
>>>> 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]
>>>> ---
>>>>  hw/arm/Makefile.objs        |   1 +
>>>>  hw/arm/sysbus-fdt.c         | 181 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/hw/arm/sysbus-fdt.h |  50 ++++++++++++
>>>>  3 files changed, 232 insertions(+)
>>>>  create mode 100644 hw/arm/sysbus-fdt.c
>>>>  create mode 100644 include/hw/arm/sysbus-fdt.h
>>>>
>>>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>>>> index 6088e53..0cc63e1 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 += sysbus-fdt.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/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>>>> new file mode 100644
>>>> index 0000000..d5476f1
>>>> --- /dev/null
>>>> +++ b/hw/arm/sysbus-fdt.c
>>>> @@ -0,0 +1,181 @@
>>>> +/*
>>>> + * ARM Platform Bus device tree generation helpers
>>>> + *
>>>> + * Copyright (c) 2014 Linaro Limited
>>>> + *
>>>> + * Authors:
>>>> + *  Alex Graf <agraf@suse.de>
>>>> + *  Eric Auger <eric.auger@linaro.org>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms and conditions of the GNU General Public License,
>>>> + * version 2 or later, as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>>>> + * more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License along with
>>>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>>>> + *
>>>> + */
>>>> +
>>>> +#include "hw/arm/sysbus-fdt.h"
>>>> +#include "qemu/error-report.h"
>>>> +#include "sysemu/device_tree.h"
>>>> +#include "hw/platform-bus.h"
>>> [*]
>>>> +#include "sysemu/sysemu.h"
>>>> +#include "hw/platform-bus.h"
>>> [*]
>>> Duplicate include "hw/platform-bus.h"
>> Hi Shannon,
>>
>> thanks for pointing this out
>>>> +
>>>> +/*
>>>> + * internal struct that contains the information to create dynamic
>>>> + * sysbus device node
>>>> + */
>>>> +typedef struct PlatformBusFdtData {
>>>> +    void *fdt; /* device tree handle */
>>>> +    int irq_start; /* index of the first IRQ usable by platform bus devices */
>>>> +    const char *pbus_node_name; /* name of the platform bus node */
>>>> +    PlatformBusDevice *pbus;
>>>> +} PlatformBusFdtData;
>>>> +
>>>> +/*
>>>> + * struct used when calling the machine init done notifier
>>>> + * that constructs the fdt nodes of platform bus devices
>>>> + */
>>>> +typedef struct PlatformBusFdtNotifierParams {
>>>> +    ARMPlatformBusFdtParams *fdt_params;
>>>> +    Notifier notifier;
>>>> +} PlatformBusFdtNotifierParams;
>>>> +
>>>> +/* struct that associates a device type name and a node creation function */
>>>> +typedef struct NodeCreationPair {
>>>> +    const char *typename;
>>>> +    int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>>>> +} NodeCreationPair;
>>>> +
>>>> +/* list of supported dynamic sysbus devices */
>>>> +NodeCreationPair add_fdt_node_functions[] = {
>>>> +        {"", NULL}, /*last element*/
>>>> +};
>>>
>>> Eric, I have a question about how to use this.
>>> If I want to dynamically add a device, I must add a member in above array, such as {TYPE_PFD, pfd_add_fdt_node}.
>>> And the "pfd_add_fdt_node" is defined by myself which is used to dynamically generate the device fdt node.
>>>
>>> Am I right ?
>> yes that's correct. You can find an example (Calxeda midway xgmac) in
>> [PATCH v7 12/16] hw/arm/sysbus-fdt: enable vfio-calxeda-xgmac dynamic
>> (https://patches.linaro.org/39910/).
>>
> Hi Eric,
> 
> Thanks for your reply.
> 
> Assuming that I want to add a device "TYPE_PFD". I create a pfd.c in hw/arm/.
> 
> As the "pfd_add_fdt_node" is associated with device "TYPE_PFD", so I just want to let it in pfd.c.

It's not associated with TYPE_PFD at all. The device tree node is 100%
machine owned and should be - only the machine knows about machine
specific device tree semantics.

If on top of that the machine needs to learn about device specific
semantics to create a device tree node, so be it :).


Alex

Re: [Qemu-devel] [PATCH v4 5/6] hw/arm/sysbus-fdt: helpers for platform bus nodes addition

[Eric Auger]

On 11/27/2014 01:56 PM, Shannon Zhao wrote:
> On 2014/11/27 20:25, Eric Auger wrote:
>> On 11/27/2014 01:07 PM, Shannon Zhao wrote:
>>> On 2014/10/31 21:53, Eric Auger wrote:
>>>> This new C module will be used by ARM machine files to generate
>>>> platform bus node and their dynamic sysbus device tree nodes.
>>>>
>>>> Dynamic sysbus device node addition is done in a machine init
>>>> done notifier. arm_register_platform_bus_fdt_creator does the
>>>> registration of this latter and is supposed to be called by
>>>> ARM machine files that support platform bus and their dynamic
>>>> sysbus. Addition of dynamic sysbus nodes is done only if the
>>>> user did not provide any dtb.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>>
>>>> ---
>>>>
>>>> v3 -> v4:
>>>> - dyn_sysbus_devtree.c renamed into sysbus-fdt.c
>>>> - use new PlatformBusDevice object
>>>> - the dtb upgrade is done through modify_dtb. Before the fdt
>>>>   was recreated from scratch. When the user provided a dtb this
>>>>   latter was overwritten which was not correct.
>>>> - an array contains the association between device type names
>>>>   and their node creation function
>>>> - I must aknowledge I did not find any cleaner way to implement
>>>>   a FDT_BUILDER interface, as suggested by Paolo. The class method
>>>>   would need to be initialized somewhere and since it cannot
>>>>   happen in the device itself - according to Alex & Peter comments -,
>>>>   I don't see when I shall associate the device type and its
>>>>   interface implementation.
>>>>
>>>> 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]
>>>> ---
>>>>  hw/arm/Makefile.objs        |   1 +
>>>>  hw/arm/sysbus-fdt.c         | 181 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/hw/arm/sysbus-fdt.h |  50 ++++++++++++
>>>>  3 files changed, 232 insertions(+)
>>>>  create mode 100644 hw/arm/sysbus-fdt.c
>>>>  create mode 100644 include/hw/arm/sysbus-fdt.h
>>>>
>>>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>>>> index 6088e53..0cc63e1 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 += sysbus-fdt.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/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>>>> new file mode 100644
>>>> index 0000000..d5476f1
>>>> --- /dev/null
>>>> +++ b/hw/arm/sysbus-fdt.c
>>>> @@ -0,0 +1,181 @@
>>>> +/*
>>>> + * ARM Platform Bus device tree generation helpers
>>>> + *
>>>> + * Copyright (c) 2014 Linaro Limited
>>>> + *
>>>> + * Authors:
>>>> + *  Alex Graf <agraf@suse.de>
>>>> + *  Eric Auger <eric.auger@linaro.org>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms and conditions of the GNU General Public License,
>>>> + * version 2 or later, as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>>>> + * more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License along with
>>>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>>>> + *
>>>> + */
>>>> +
>>>> +#include "hw/arm/sysbus-fdt.h"
>>>> +#include "qemu/error-report.h"
>>>> +#include "sysemu/device_tree.h"
>>>> +#include "hw/platform-bus.h"
>>> [*]
>>>> +#include "sysemu/sysemu.h"
>>>> +#include "hw/platform-bus.h"
>>> [*]
>>> Duplicate include "hw/platform-bus.h"
>> Hi Shannon,
>>
>> thanks for pointing this out
>>>> +
>>>> +/*
>>>> + * internal struct that contains the information to create dynamic
>>>> + * sysbus device node
>>>> + */
>>>> +typedef struct PlatformBusFdtData {
>>>> +    void *fdt; /* device tree handle */
>>>> +    int irq_start; /* index of the first IRQ usable by platform bus devices */
>>>> +    const char *pbus_node_name; /* name of the platform bus node */
>>>> +    PlatformBusDevice *pbus;
>>>> +} PlatformBusFdtData;
>>>> +
>>>> +/*
>>>> + * struct used when calling the machine init done notifier
>>>> + * that constructs the fdt nodes of platform bus devices
>>>> + */
>>>> +typedef struct PlatformBusFdtNotifierParams {
>>>> +    ARMPlatformBusFdtParams *fdt_params;
>>>> +    Notifier notifier;
>>>> +} PlatformBusFdtNotifierParams;
>>>> +
>>>> +/* struct that associates a device type name and a node creation function */
>>>> +typedef struct NodeCreationPair {
>>>> +    const char *typename;
>>>> +    int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>>>> +} NodeCreationPair;
>>>> +
>>>> +/* list of supported dynamic sysbus devices */
>>>> +NodeCreationPair add_fdt_node_functions[] = {
>>>> +        {"", NULL}, /*last element*/
>>>> +};
>>>
>>> Eric, I have a question about how to use this.
>>> If I want to dynamically add a device, I must add a member in above array, such as {TYPE_PFD, pfd_add_fdt_node}.
>>> And the "pfd_add_fdt_node" is defined by myself which is used to dynamically generate the device fdt node.
>>>
>>> Am I right ?
>> yes that's correct. You can find an example (Calxeda midway xgmac) in
>> [PATCH v7 12/16] hw/arm/sysbus-fdt: enable vfio-calxeda-xgmac dynamic
>> (https://patches.linaro.org/39910/).
>>
> Hi Eric,
> 
> Thanks for your reply.
> 
> Assuming that I want to add a device "TYPE_PFD". I create a pfd.c in hw/arm/.
> 
> As the "pfd_add_fdt_node" is associated with device "TYPE_PFD", so I just want to let it in pfd.c.
> But the struct PlatformBusFdtData is an internal struct which means I have to move "pfd_add_fdt_node"
> to sysbus-fdt.c. If so, I think it's a little interleaving. And the file sysbus-fdt.c is a little messy.
> 
> What do you think about moving the associated struct to sysbus-fdt.h ?


> 
> And I think the array add_fdt_node_functions is not convenient to add a new add_fdt_node_fn for a new device.
> Maybe a global register function would be better.

Hi Shannon,

Sorry but I don't know what the pfd device correspond to. Please could
you elaborate on it or provide me with a link?

Besides I just wanted to warn you about the fact for the time being
quite few sysbus devices are supposed to be instantiated from command
line. We need to make sure this is the case for pfd. Also in the past we
discussed about the the relevance of putting the device node generation
in the (sysbus) device and I did some patch for that and Alex and Peter
convinced me this was a bad idea overall. Now we are talking about
devices that were generic (not arch specific), like vfio. In case of an
ARM specific device it might be worth to submit this point on the ML.
Besides the current philosophy was to put the device tree node creation
function in sysbus-fdt.c and not in the device file. Already there is
some uncertainty about relevance of putting this outside of the machine
file, according to Alex.

Eric


> 
> Thanks,
> Shannon
> 
>> I am currently reworking this according to Alex comments but the spirit
>> remains the same.
>>
>> Please do not hesitate to come back to me if you have other questions.
>>
>> Best Regards
>>
>> Eric
>>
>>
> 
> 

Re: [Qemu-devel] [PATCH v4 5/6] hw/arm/sysbus-fdt: helpers for platform bus nodes addition

[Shannon Zhao]

On 2014/11/27 21:16, Eric Auger wrote:
> On 11/27/2014 01:56 PM, Shannon Zhao wrote:
>> On 2014/11/27 20:25, Eric Auger wrote:
>>> On 11/27/2014 01:07 PM, Shannon Zhao wrote:
>>>> On 2014/10/31 21:53, Eric Auger wrote:
>>>>> This new C module will be used by ARM machine files to generate
>>>>> platform bus node and their dynamic sysbus device tree nodes.
>>>>>
>>>>> Dynamic sysbus device node addition is done in a machine init
>>>>> done notifier. arm_register_platform_bus_fdt_creator does the
>>>>> registration of this latter and is supposed to be called by
>>>>> ARM machine files that support platform bus and their dynamic
>>>>> sysbus. Addition of dynamic sysbus nodes is done only if the
>>>>> user did not provide any dtb.
>>>>>
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>>>
>>>>> ---
>>>>>
>>>>> v3 -> v4:
>>>>> - dyn_sysbus_devtree.c renamed into sysbus-fdt.c
>>>>> - use new PlatformBusDevice object
>>>>> - the dtb upgrade is done through modify_dtb. Before the fdt
>>>>>   was recreated from scratch. When the user provided a dtb this
>>>>>   latter was overwritten which was not correct.
>>>>> - an array contains the association between device type names
>>>>>   and their node creation function
>>>>> - I must aknowledge I did not find any cleaner way to implement
>>>>>   a FDT_BUILDER interface, as suggested by Paolo. The class method
>>>>>   would need to be initialized somewhere and since it cannot
>>>>>   happen in the device itself - according to Alex & Peter comments -,
>>>>>   I don't see when I shall associate the device type and its
>>>>>   interface implementation.
>>>>>
>>>>> 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]
>>>>> ---
>>>>>  hw/arm/Makefile.objs        |   1 +
>>>>>  hw/arm/sysbus-fdt.c         | 181 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>  include/hw/arm/sysbus-fdt.h |  50 ++++++++++++
>>>>>  3 files changed, 232 insertions(+)
>>>>>  create mode 100644 hw/arm/sysbus-fdt.c
>>>>>  create mode 100644 include/hw/arm/sysbus-fdt.h
>>>>>
>>>>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>>>>> index 6088e53..0cc63e1 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 += sysbus-fdt.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/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>>>>> new file mode 100644
>>>>> index 0000000..d5476f1
>>>>> --- /dev/null
>>>>> +++ b/hw/arm/sysbus-fdt.c
>>>>> @@ -0,0 +1,181 @@
>>>>> +/*
>>>>> + * ARM Platform Bus device tree generation helpers
>>>>> + *
>>>>> + * Copyright (c) 2014 Linaro Limited
>>>>> + *
>>>>> + * Authors:
>>>>> + *  Alex Graf <agraf@suse.de>
>>>>> + *  Eric Auger <eric.auger@linaro.org>
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify it
>>>>> + * under the terms and conditions of the GNU General Public License,
>>>>> + * version 2 or later, as published by the Free Software Foundation.
>>>>> + *
>>>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>>>>> + * more details.
>>>>> + *
>>>>> + * You should have received a copy of the GNU General Public License along with
>>>>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>>>>> + *
>>>>> + */
>>>>> +
>>>>> +#include "hw/arm/sysbus-fdt.h"
>>>>> +#include "qemu/error-report.h"
>>>>> +#include "sysemu/device_tree.h"
>>>>> +#include "hw/platform-bus.h"
>>>> [*]
>>>>> +#include "sysemu/sysemu.h"
>>>>> +#include "hw/platform-bus.h"
>>>> [*]
>>>> Duplicate include "hw/platform-bus.h"
>>> Hi Shannon,
>>>
>>> thanks for pointing this out
>>>>> +
>>>>> +/*
>>>>> + * internal struct that contains the information to create dynamic
>>>>> + * sysbus device node
>>>>> + */
>>>>> +typedef struct PlatformBusFdtData {
>>>>> +    void *fdt; /* device tree handle */
>>>>> +    int irq_start; /* index of the first IRQ usable by platform bus devices */
>>>>> +    const char *pbus_node_name; /* name of the platform bus node */
>>>>> +    PlatformBusDevice *pbus;
>>>>> +} PlatformBusFdtData;
>>>>> +
>>>>> +/*
>>>>> + * struct used when calling the machine init done notifier
>>>>> + * that constructs the fdt nodes of platform bus devices
>>>>> + */
>>>>> +typedef struct PlatformBusFdtNotifierParams {
>>>>> +    ARMPlatformBusFdtParams *fdt_params;
>>>>> +    Notifier notifier;
>>>>> +} PlatformBusFdtNotifierParams;
>>>>> +
>>>>> +/* struct that associates a device type name and a node creation function */
>>>>> +typedef struct NodeCreationPair {
>>>>> +    const char *typename;
>>>>> +    int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>>>>> +} NodeCreationPair;
>>>>> +
>>>>> +/* list of supported dynamic sysbus devices */
>>>>> +NodeCreationPair add_fdt_node_functions[] = {
>>>>> +        {"", NULL}, /*last element*/
>>>>> +};
>>>>
>>>> Eric, I have a question about how to use this.
>>>> If I want to dynamically add a device, I must add a member in above array, such as {TYPE_PFD, pfd_add_fdt_node}.
>>>> And the "pfd_add_fdt_node" is defined by myself which is used to dynamically generate the device fdt node.
>>>>
>>>> Am I right ?
>>> yes that's correct. You can find an example (Calxeda midway xgmac) in
>>> [PATCH v7 12/16] hw/arm/sysbus-fdt: enable vfio-calxeda-xgmac dynamic
>>> (https://patches.linaro.org/39910/).
>>>
>> Hi Eric,
>>
>> Thanks for your reply.
>>
>> Assuming that I want to add a device "TYPE_PFD". I create a pfd.c in hw/arm/.
>>
>> As the "pfd_add_fdt_node" is associated with device "TYPE_PFD", so I just want to let it in pfd.c.
>> But the struct PlatformBusFdtData is an internal struct which means I have to move "pfd_add_fdt_node"
>> to sysbus-fdt.c. If so, I think it's a little interleaving. And the file sysbus-fdt.c is a little messy.
>>
>> What do you think about moving the associated struct to sysbus-fdt.h ?
> 
> 
>>
>> And I think the array add_fdt_node_functions is not convenient to add a new add_fdt_node_fn for a new device.
>> Maybe a global register function would be better.
> 
> Hi Shannon,
> 
> Sorry but I don't know what the pfd device correspond to. Please could
> you elaborate on it or provide me with a link?
> 

Hi Eric,

The pfd device is not a real device, it's just an alias.

> Besides I just wanted to warn you about the fact for the time being
> quite few sysbus devices are supposed to be instantiated from command
> line. We need to make sure this is the case for pfd. Also in the past we
> discussed about the the relevance of putting the device node generation
> in the (sysbus) device and I did some patch for that and Alex and Peter
> convinced me this was a bad idea overall. Now we are talking about
> devices that were generic (not arch specific), like vfio. In case of an
> ARM specific device it might be worth to submit this point on the ML.
> Besides the current philosophy was to put the device tree node creation
> function in sysbus-fdt.c and not in the device file. Already there is
> some uncertainty about relevance of putting this outside of the machine
> file, according to Alex.
> 

Ok, I understand that. Thanks for your explaining :-)

Shannon
> Eric
> 
> 
>>
>> Thanks,
>> Shannon
>>
>>> I am currently reworking this according to Alex comments but the spirit
>>> remains the same.
>>>
>>> Please do not hesitate to come back to me if you have other questions.
>>>
>>> Best Regards
>>>
>>> Eric
>>>
>>>
>>
>>
> 
> 
> .
> 


-- 
Shannon