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

[Alexander Graf <agraf@suse.de>]

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