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 <address@hidden>
>> Signed-off-by: Eric Auger <address@hidden>
>>
>> ---
>>
>> 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 <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"
>> +#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("/address@hidden"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 <address@hidden>
>> + *  Eric Auger <address@hidden>
>> + *
>> + * 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
>>
> 
>