Re: [Qemu-devel] [PATCH v8 1/3] hw/arm/sysbus-fdt: helpers for platform bus nodes addition

[Peter Maydell]

On 5 January 2015 at 16:14, Eric Auger <address@hidden> 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 <address@hidden>
> Signed-off-by: Eric Auger <address@hidden>
> Reviewed-by: Shannon Zhao <address@hidden>
> Reviewed-by: Alexander Graf <address@hidden>
>
> ---
> v7 -> v8:
> add Reviewed-by from Alex and Shannon
>
> v6 -> v7:
> - revert indentation in add_fdt_node_functions
>
> v5 -> v6:
> - add_all_platform_bus_fdt_nodes is not a modify_dtb function anymore
> - it now takes a handle to an ARMPlatformBusFdtParams.
> - fdt pointer is checked in case this notifier is executed after the
>   one that executes the load_dtb (this latter deallocates the fdt pointer)
> - check of fdt_filename moved in here.
> - upgrade_dtb is removed
> - copyright aligned between .h and .c
>
> v4 -> v5:
> - change indentation in add_fdt_node_functions. Also becomes a
>   static const.
> - ARMPlatformBusFdtParams.system_params becomes a pointer to
>   a const ARMPlatformBusSystemParams
> - removes platform-bus.h second inclusion
>
> 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         | 173 
> ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/sysbus-fdt.h |  60 +++++++++++++++
>  3 files changed, 234 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..a1dc0aa
> --- /dev/null
> +++ b/hw/arm/sysbus-fdt.c
> @@ -0,0 +1,173 @@
> +/*
> + * ARM Platform Bus device tree generation helpers
> + *
> + * Copyright (c) 2014 Linaro Limited
> + *
> + * Authors:
> + *  Alex Graf <address@hidden>
> + *  Eric Auger <address@hidden>
> + *
> + * 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"
> +
> +/*
> + * internal struct that contains the information to create dynamic
> + * sysbus device node
> + */
> +typedef struct PlatformBusFdtData {

FDT is an acronym, so "PlatformBusFDTData" would be nicer.
(Similar remarks apply for some other struct type names.)

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

Weird ordering again. If you put notifier first then you can do

   PlatformBusFdtNotifier *p = DO_UPCAST(PlatformBusFdtNotifier,
                                         notifier, notifier);

rather than raw container_of calls. (This is of course assuming
you see this as a type-of-notifier, which seems reasonable to
me...hence the tweak to the struct name.)

> +} 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 */
> +static const NodeCreationPair add_fdt_node_functions[] = {
> +    {"", NULL}, /*last element*/

Missing spaces: /* 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

s/and in the positive/and if so/

> + * 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.
> + */
> +static void add_all_platform_bus_fdt_nodes(ARMPlatformBusFdtParams 
> *fdt_params)
> +{
> +    const char platcomp[] = "qemu,platform\0simple-bus";
> +    PlatformBusDevice *pbus;
> +    DeviceState *dev;
> +    gchar *node;
> +    uint64_t addr, size;
> +    int irq_start, dtb_size;
> +    struct arm_boot_info *info = fdt_params->binfo;
> +    const ARMPlatformBusSystemParams *params = fdt_params->system_params;
> +    const char *intc = fdt_params->intc;
> +    void *fdt = info->get_dtb(info, &dtb_size);
> +
> +    /*
> +     * In case the user provided a dtb, we assume he already integrated the

s/In case/If/. s/he/they/.

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

"can take charge of".

> +     */
> +    if (info->dtb_filename) {
> +        return;
> +    }
> +
> +    if (!fdt) {
> +        fprintf(stderr,
> +                "fdt NULL pointer: platform bus nodes cannot be added\n") ;
> +        return;
> +    }

When can this happen? If it can happen due to user command line error
or similar, the error message should be more useful to the user.
If it can only happen due to a bug in the board model (which I think
is the case), then just assert(fdt);

> +
> +    node = g_strdup_printf("/address@hidden"PRIx64, 
> params->platform_bus_base);
> +    addr = params->platform_bus_base;
> +    size = params->platform_bus_size;
> +    irq_start = params->platform_bus_first_irq;
> +
> +    /* Create a /platform node that we can put all devices into */
> +    qemu_fdt_add_subnode(fdt, node);
> +    qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
> +
> +    /* Our platform bus region is less than 32bit big, so 1 cell is enough 
> for
> +       address and size */

"32 bits".
 /* multiline comments
  * should look like this
  */

> +    qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
> +    qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
> +    qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr, size);
> +
> +    qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", intc);
> +
> +    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) {

Isn't it a bug if we get here with done_gathering false? If not,
then what arranges for us to get called again later?

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

Rest looks OK I think.

-- PMM