Hi Peter,
On 01/06/2015 07:40 PM, Peter Maydell wrote:
> On 5 January 2015 at 16:14, Eric Auger <eric.au...@linaro.org> 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 <ag...@suse.de>
>> Signed-off-by: Eric Auger <eric.au...@linaro.org>
>> Reviewed-by: Shannon Zhao <zhaoshengl...@huawei.com>
>> Reviewed-by: Alexander Graf <ag...@suse.de>
>>
>> ---
>> 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 <ag...@suse.de>
>> + * Eric Auger <eric.au...@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"
>> +
>> +/*
>> + * 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.)
Sure
>
>> + 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.)
Yes it is the case, I will correct the order.
>
>> +} 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 */
sure
>
>> +};
>> +
>> +/**
>> + * 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/
thanks
>
>> + * 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/.
OK
>
>> + * 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".
thanks
>
>> + */
>> + 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);
Indeed this was meant to warn about a bug in board model. I will use
assert then.
>
>> +
>> + node = g_strdup_printf("/platform@%"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
> */
OK
>
>> + 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?
Yes it would be bug in the board model. The "gathering" is done in the
notifier registered in platform_bus_realize while this function is
executed in the notifier registered in create_platform_bus, when calling
arm_register_platform_bus_fdt_creator. create_platform_bus takes care of
registering the fdt creator notifier first and then instantiating the
platform bus.
I will put an assert then.
Thanks for the review!
Best Regards
Eric
>
>> + 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
>
