Re: [Qemu-devel] [PATCH v3 1/3] ARM: Allow boards to provide an fdt blob

[Peter Maydell]

(I've taken this work over from John so these review comments
are more in the nature of "notes to myself" than anything else ;-)

On 10 May 2013 07:58, John Rigby <address@hidden> wrote:
> If no fdt is provided on command line and the new field
> get_dtb in struct arm_boot_info is set then call it to
> get a device tree blob.
>
> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
> index 7b2b02d..4c56a1b 100644
> --- a/include/hw/arm/arm.h
> +++ b/include/hw/arm/arm.h
> @@ -31,6 +31,10 @@ struct arm_boot_info {
>      const char *kernel_cmdline;
>      const char *initrd_filename;
>      const char *dtb_filename;
> +    /* if a board is able to create a dtb without a dtb file then it
> +     * sets get_dtb.  This will only be used if no dtb file is provided.
> +     */
> +    void *(*get_dtb)(hwaddr addr, const struct arm_boot_info *binfo, int 
> *size);

The general idea here is OK, but it's not clear to me why
we need to pass in the addr to the hook function (certainly
the example in part 3 doesn't need it). I think we can just
move to
 void *(*get_dtb)(const struct arm_boot_info *binfo, int *size);

As with load_device_tree(), we should require (and document!)
that the returned blob is in memory freeable with g_free().

NB: we don't actually free the blob in load_dtb(), either
for load_device_tree() or for this; we should (separate patch).

>      hwaddr loader_start;
>      /* multicore boards that use the default secondary core boot functions
>       * need to put the address of the secondary boot code, the boot reg,
> @@ -59,6 +63,8 @@ struct arm_boot_info {
>      int is_linux;
>      hwaddr initrd_start;
>      hwaddr initrd_size;
> +    void *dtb_blob;
> +    int dtb_blob_size;

These appear to be relics from a previous version -- they're not used
and can be dropped.

thanks
-- PMM