[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 5/6] hw/arm/sysbus-fdt: helpers for platform
From: |
Shannon Zhao |
Subject: |
Re: [Qemu-devel] [PATCH v4 5/6] hw/arm/sysbus-fdt: helpers for platform bus nodes addition |
Date: |
Thu, 27 Nov 2014 20:56:23 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 |
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.
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