[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 5/6] hw/arm/sysbus-fdt: helpers for clock node g
From: |
Eric Auger |
Subject: |
Re: [Qemu-devel] [PATCH 5/6] hw/arm/sysbus-fdt: helpers for clock node generation |
Date: |
Tue, 5 Jan 2016 18:04:57 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 |
On 12/18/2015 03:54 PM, Peter Maydell wrote:
> On 17 December 2015 at 12:29, Eric Auger <address@hidden> wrote:
>> Some passthrough'ed devices depend on clock nodes. Those need to be
>> generated in the guest device tree. This patch introduces some helpers
>> to build a clock node from information retrieved in the host device tree.
>>
>> - inherit_properties copies properties from a host device tree node to
>> a guest device tree node
>> - fdt_build_clock_node builds a guest clock node and checks the host
>> fellow clock is a fixed one.
>>
>> fdt_build_clock_node will become static as soon as it gets used. A
>> dummy pre-declaration is needed for compilation of this patch.
>>
>> Signed-off-by: Eric Auger <address@hidden>
>>
>> ---
>>
>> RFC -> v1:
>> - use the new proto of qemu_fdt_getprop
>> - remove newline in error_report
>> - fix some style issues
>> ---
>> hw/arm/sysbus-fdt.c | 111
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 111 insertions(+)
>>
>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>> index 9d28797..c2d813b 100644
>> --- a/hw/arm/sysbus-fdt.c
>> +++ b/hw/arm/sysbus-fdt.c
>> @@ -21,6 +21,7 @@
>> *
>> */
>>
>> +#include <libfdt.h>
>> #include "hw/arm/sysbus-fdt.h"
>> #include "qemu/error-report.h"
>> #include "sysemu/device_tree.h"
>> @@ -56,6 +57,116 @@ typedef struct NodeCreationPair {
>> int (*add_fdt_node_fn)(SysBusDevice *sbdev, void *opaque);
>> } NodeCreationPair;
>>
>> +/* helpers */
>> +
>> +struct HostProperty {
>> + const char *name;
>> + bool optional;
>> +};
>> +
>> +typedef struct HostProperty HostProperty;
>
> You can combine the typedef and the struct declaration if you like.
ok
>
>> +
>> +/**
>> + * inherit_properties
>> + *
>> + * copies properties listed in an array from host device tree to
>> + * guest device tree. If a non optional property is not found, the
>> + * function self-asserts. An optional property is ignored if not found
>> + * in the host device tree.
>> + * @props: array of HostProperty to copy
>> + * @nb_props: number of properties in the array
>> + * @host_dt: host device tree blob
>> + * @guest_dt: guest device tree blob
>> + * @node_path: host dt node path where the property is supposed to be
>> + found
>> + * @nodename: guest node name the properties should be added to
>> + */
>> +static void inherit_properties(HostProperty *props, int nb_props,
>> + void *host_fdt, void *guest_fdt,
>> + char *node_path, char *nodename)
>> +{
>> + int i, prop_len;
>> + const void *r;
>> +
>> + for (i = 0; i < nb_props; i++) {
>> + r = qemu_fdt_getprop(host_fdt, node_path,
>> + props[i].name,
>> + &prop_len,
>> + props[i].optional ? NULL : &error_fatal);
>
> We'll get an error here if the host device tree doesn't match
> up correctly, right? Is the error message going to be sufficiently
> informative to the end user about what's gone wrong? (What does
> it end up looking like?)
hum you're right, in case of a mandated property there is auto-assert
with a good error message.
In case of an optional property, there is currently no message output to
the end-user and the host property is ignored/not propagated to the
guest, with potential functional consequences. I intend to use an error
object instead and print the error message in case the error is !=
FDT_ERR_NOTFOUND.
>
>> + if (r) {
>> + qemu_fdt_setprop(guest_fdt, nodename,
>> + props[i].name, r, prop_len);
>> + }
>> + }
>> +}
>> +
>> +/* clock properties whose values are copied/pasted from host */
>> +static HostProperty clock_inherited_properties[] = {
>> + {"compatible", 0},
>> + {"#clock-cells", 0},
>> + {"clock-frequency", 1},
>> + {"clock-output-names", 1},
>> +};
>> +
>> +/**
>> + * fdt_build_clock_node
>> + *
>> + * Build a guest clock node, used as a dependency from a passthrough'ed
>> + * device. Most information are retrieved from the host clock node.
>> + * Also check the host clock is a fixed one.
>> + *
>> + * @host_fdt: host device tree blob from which info are retrieved
>> + * @guest_fdt: guest device tree blob where the clock node is added
>> + * @host_phandle: phandle of the clock in host device tree
>> + * @guest_phandle: phandle to assign to the guest node
>> + */
>> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>> + uint32_t host_phandle,
>> + uint32_t guest_phandle);
>> +int fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>> + uint32_t host_phandle,
>> + uint32_t guest_phandle)
>> +{
>> + char node_path[256];
>
> Please don't use hardcoded fixed buffer lengths (see previous patch
> review comments).
OK
>
>> + char *nodename;
>> + const void *r;
>> + int ret, prop_len;
>> +
>> + ret = fdt_node_offset_by_phandle(host_fdt, host_phandle);
>> + if (ret <= 0) {
>> + error_report("not able to locate clock handle %d in host device
>> tree",
>> + host_phandle);
>> + goto out;
>> + }
>
> If we're going to return to the caller in the error case rather than just
> exiting here, I think we should return the error to the caller via an
> Error** rather than calling error_report() directly ourselves.
agreed
Thanks!
Eric
>
>> + ret = fdt_get_path(host_fdt, ret, node_path, 256);
>> + if (ret < 0) {
>> + error_report("not able to retrieve node path for clock handle %d",
>> + host_phandle);
>> + goto out;
>> + }
>> +
>> + r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len,
>> + &error_fatal);
>> + if (strcmp(r, "fixed-clock")) {
>> + error_report("clock handle %d is not a fixed clock", host_phandle);
>> + ret = -1;
>> + goto out;
>> + }
>> +
>> + nodename = strrchr(node_path, '/');
>> + qemu_fdt_add_subnode(guest_fdt, nodename);
>> +
>> + inherit_properties(clock_inherited_properties,
>> + ARRAY_SIZE(clock_inherited_properties),
>> + host_fdt, guest_fdt,
>> + node_path, nodename);
>> +
>> + qemu_fdt_setprop_cell(guest_fdt, nodename, "phandle", guest_phandle);
>> +
>> +out:
>> + return ret;
>> +}
>> +
>> /* Device Specific Code */
>
> thanks
> -- PMM
>
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH 5/6] hw/arm/sysbus-fdt: helpers for clock node generation,
Eric Auger <=