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 <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/).
>>>
>> 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