From patchwork Sun Jan 29 20:36:06 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Grant Likely X-Patchwork-Id: 138513 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 9A1F71007D4 for ; Mon, 30 Jan 2012 15:11:50 +1100 (EST) Received: from localhost ([::1]:52224 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rrial-0000hK-C5 for incoming@patchwork.ozlabs.org; Sun, 29 Jan 2012 23:11:43 -0500 Received: from eggs.gnu.org ([140.186.70.92]:51321) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rriac-0000hF-PA for qemu-devel@nongnu.org; Sun, 29 Jan 2012 23:11:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rriaa-0007er-T1 for qemu-devel@nongnu.org; Sun, 29 Jan 2012 23:11:34 -0500 Received: from mail-gx0-f173.google.com ([209.85.161.173]:44673) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rriaa-0007ej-PK for qemu-devel@nongnu.org; Sun, 29 Jan 2012 23:11:32 -0500 Received: by ggnh1 with SMTP id h1so2128696ggn.4 for ; Sun, 29 Jan 2012 20:11:31 -0800 (PST) Received: by 10.101.180.19 with SMTP id h19mr6944640anp.74.1327896691162; Sun, 29 Jan 2012 20:11:31 -0800 (PST) Received: from localhost (S0106d8b37715ee14.cg.shawcable.net. [68.146.14.168]) by mx.google.com with ESMTPS id h29sm42824704ann.16.2012.01.29.20.11.28 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 29 Jan 2012 20:11:30 -0800 (PST) Received: by localhost (Postfix, from userid 1000) id E4A843E0665; Sun, 29 Jan 2012 13:36:06 -0700 (MST) Date: Sun, 29 Jan 2012 13:36:06 -0700 From: Grant Likely To: Peter Maydell Message-ID: <20120129203606.GB11695@ponder.secretlab.ca> References: <1327701190-24822-1-git-send-email-grant.likely@secretlab.ca> <201201272234.02700.paul@codesourcery.com> <20120128184837.GG2501@ponder.secretlab.ca> <201201291115.43916.paul@codesourcery.com> <20120129160105.GA10629@ponder.secretlab.ca> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.85.161.173 Cc: "Edgar E. Iglesias" , Jeremy Kerr , Paul Brook , Rob Herring , qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCH] arm: add device tree support X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Sun, Jan 29, 2012 at 07:13:55PM +0000, Peter Maydell wrote: > On 29 January 2012 16:01, Grant Likely wrote: > > diff --git a/configure b/configure > > index f69e08f..0c2deab 100755 > > --- a/configure > > +++ b/configure > > @@ -3411,6 +3411,9 @@ case "$target_arch2" in > >     gdb_xml_files="arm-core.xml arm-vfp.xml arm-vfp3.xml arm-neon.xml" > >     target_phys_bits=32 > >     target_llong_alignment=4 > > +    if test "$fdt" = "yes" ; then > > +      target_libs_softmmu="$fdt_libs" > > +    fi > > This doesn't need to be conditional -- compare the similar stanzas > for other fdt-using architectures. Okay. > >   ;; > >   cris) > >     target_nptl="yes" > > diff --git a/hw/arm_boot.c b/hw/arm_boot.c > > index 5f163fd..35bfa62 100644 > > --- a/hw/arm_boot.c > > +++ b/hw/arm_boot.c > > @@ -7,11 +7,14 @@ > >  * This code is licensed under the GPL. > >  */ > > > > +#include "config.h" > >  #include "hw.h" > >  #include "arm-misc.h" > >  #include "sysemu.h" > > +#include "boards.h" > >  #include "loader.h" > >  #include "elf.h" > > +#include "device_tree.h" > > > >  #define KERNEL_ARGS_ADDR 0x100 > >  #define KERNEL_LOAD_ADDR 0x00010000 > > @@ -207,6 +210,66 @@ static void set_kernel_args_old(const struct arm_boot_info *info, > >     } > >  } > > > > +static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info *binfo) > > +{ > > +#ifdef CONFIG_FDT > > +    uint32_t mem_reg_property[] = { cpu_to_be32(binfo->loader_start), > > +                                    cpu_to_be32(binfo->ram_size) }; > > +    void *fdt = NULL; > > +    char *filename; > > +    int size, rc; > > + > > +    if (!current_dtb_filename) > > +        return 0; > > scripts/checkpatch.pl complains about this and other style issues... > Fixed. > > + > > +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, current_dtb_filename); > > +    if (!filename) { > > +        fprintf(stderr, "Couldn't open dtb file %s\n", current_dtb_filename); > > +        return -1; > > +    } > > + > > +    fdt = load_device_tree(filename, &size); > > +    if (!fdt) { > > +        fprintf(stderr, "Couldn't open dtb file %s\n", filename); > > +        g_free(filename); > > +        return -1; > > +    } > > +    g_free(filename); > > + > > +    rc = qemu_devtree_setprop(fdt, "/memory", "reg", mem_reg_property, > > +                               sizeof(mem_reg_property)); > > +    if (rc < 0) > > +        fprintf(stderr, "couldn't set /memory/reg\n"); > > + > > +    rc = qemu_devtree_setprop_string(fdt, "/chosen", "bootargs", > > +                                      binfo->kernel_cmdline); > > +    if (rc < 0) > > +        fprintf(stderr, "couldn't set /chosen/bootargs\n"); > > This seems kind of weird -- if you're not trying to use 'rc' as > a running "something failed" flag to return to the caller then > why not just have "if (function() < 0) { fprintf(...); }" ? Mostly because it looked ugly when done that way. There is already a line break to deal with the long arguments, it looked worse to me when also put inside an if() block. If you prefer, then I will change it. > > > + > > +    if (binfo->initrd_size) { > > +        rc = qemu_devtree_setprop_cell(fdt, "/chosen", "linux,initrd-start", > > +                binfo->loader_start + INITRD_LOAD_ADDR); > > +        if (rc < 0) > > +            fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n"); > > + > > +        rc = qemu_devtree_setprop_cell(fdt, "/chosen", "linux,initrd-end", > > +                    binfo->loader_start +INITRD_LOAD_ADDR + > > +                    binfo->initrd_size); > > +        if (rc < 0) > > +            fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n"); > > +    } > > + > > +    cpu_physical_memory_write (addr, fdt, size); > > + > > +    return 0; > > + > > +#else > > +    fprintf(stderr, "Platform requested a device tree, " > > +                "but qemu was compiled without fdt support\n"); > > +    return -1; > > +#endif > > +} > > + > >  static void do_cpu_reset(void *opaque) > >  { > >     CPUState *env = opaque; > > @@ -221,12 +284,14 @@ static void do_cpu_reset(void *opaque) > >         } else { > >             if (env == first_cpu) { > >                 env->regs[15] = info->loader_start; > > -                if (old_param) { > > -                    set_kernel_args_old(info, info->initrd_size, > > +                if (!current_dtb_filename) { > > +                    if (old_param) { > > +                        set_kernel_args_old(info, info->initrd_size, > > +                                            info->loader_start); > > +                    } else { > > +                        set_kernel_args(info, info->initrd_size, > >                                         info->loader_start); > > -                } else { > > -                    set_kernel_args(info, info->initrd_size, > > -                                    info->loader_start); > > +                    } > >                 } > >             } else { > >                 info->secondary_cpu_reset_hook(env, info); > > @@ -239,10 +304,10 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info) > >  { > >     int kernel_size; > >     int initrd_size; > > -    int n; > > +    int n, rc; > >     int is_linux = 0; > >     uint64_t elf_entry; > > -    target_phys_addr_t entry; > > +    target_phys_addr_t entry, dtb_start; > >     int big_endian; > > > >     /* Load the kernel.  */ > > @@ -301,8 +366,22 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info) > >         } else { > >             initrd_size = 0; > >         } > > +        info->initrd_size = initrd_size; > > + > > +        /* Place the DTB after the initrd in memory */ > > +        dtb_start = TARGET_PAGE_ALIGN(info->loader_start + INITRD_LOAD_ADDR + > > +                                      initrd_size); > > +        rc = load_dtb(dtb_start, info); > > +        if (rc) > > +            exit(1); > > The rc variable is pretty obviously unnecessary here. Fixed > > > + > >         bootloader[4] = info->board_id; > > -        bootloader[5] = info->loader_start + KERNEL_ARGS_ADDR; > > +        /* for device tree boot, we pass the DTB directly in r2. Otherwise > > +         * we point to the kernel args */ > > +        if (current_dtb_filename) > > +            bootloader[5] = dtb_start; > > +        else > > +            bootloader[5] = info->loader_start + KERNEL_ARGS_ADDR; > >         bootloader[6] = entry; > >         for (n = 0; n < sizeof(bootloader) / 4; n++) { > >             bootloader[n] = tswap32(bootloader[n]); > > @@ -312,7 +391,6 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info) > >         if (info->nb_cpus > 1) { > >             info->write_secondary_boot(env, info); > >         } > > -        info->initrd_size = initrd_size; > >     } > >     info->is_linux = is_linux; > > > > diff --git a/hw/boards.h b/hw/boards.h > > index f6d3784..d06776c 100644 > > --- a/hw/boards.h > > +++ b/hw/boards.h > > @@ -34,5 +34,6 @@ typedef struct QEMUMachine { > >  int qemu_register_machine(QEMUMachine *m); > > > >  extern QEMUMachine *current_machine; > > +extern const char *current_dtb_filename; > > The suggestion on IRC for the long-term Right Thing for passing > dtb filename/kernel/etc to arm_boot is that the arm_boot code ought > to turn into a QOM object, and then the top level code can just > search the QOM tree the machine model instantiates for a QOM > object of the right type and pass filenames to it directly by > setting properties on it. So this new global is ok as it's > likely to go away again eventually. Okay, good. I had assumed that this would be a temporary solution until the needed infrastructure is in place. > > >  #endif > > diff --git a/qemu-options.hx b/qemu-options.hx > > index 3a07ae8..0a01baa 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -1964,6 +1964,15 @@ Use @var{file1} and @var{file2} as modules and pass arg=foo as parameter to the > >  first module. > >  ETEXI > > > > +DEF("dtb", HAS_ARG, QEMU_OPTION_dtb, \ > > +    "-dtb file use 'file' as a device tree image\n", QEMU_ARCH_ARM) > > Needs more spaces to make it line up right in -help output: > Linux/Multiboot boot specific: > -kernel bzImage use 'bzImage' as kernel image > -append cmdline use 'cmdline' as kernel command line > -initrd file use 'file' as initial ram disk > -dtb file use 'file' as a device tree image Fixed. New patch below... diff --git a/Makefile.target b/Makefile.target index 68481a3..5e465ec 100644 --- a/Makefile.target +++ b/Makefile.target @@ -363,6 +363,7 @@ obj-arm-y += vexpress.o obj-arm-y += strongarm.o obj-arm-y += collie.o obj-arm-y += pl041.o lm4549.o +obj-arm-$(CONFIG_FDT) += device_tree.o obj-sh4-y = shix.o r2d.o sh7750.o sh7750_regnames.o tc58128.o obj-sh4-y += sh_timer.o sh_serial.o sh_intc.o sh_pci.o sm501.o diff --git a/configure b/configure index f69e08f..2856897 100755 --- a/configure +++ b/configure @@ -3411,6 +3411,7 @@ case "$target_arch2" in gdb_xml_files="arm-core.xml arm-vfp.xml arm-vfp3.xml arm-neon.xml" target_phys_bits=32 target_llong_alignment=4 + target_libs_softmmu="$fdt_libs" ;; cris) target_nptl="yes" diff --git a/hw/arm_boot.c b/hw/arm_boot.c index 5f163fd..377d202 100644 --- a/hw/arm_boot.c +++ b/hw/arm_boot.c @@ -7,11 +7,14 @@ * This code is licensed under the GPL. */ +#include "config.h" #include "hw.h" #include "arm-misc.h" #include "sysemu.h" +#include "boards.h" #include "loader.h" #include "elf.h" +#include "device_tree.h" #define KERNEL_ARGS_ADDR 0x100 #define KERNEL_LOAD_ADDR 0x00010000 @@ -207,6 +210,71 @@ static void set_kernel_args_old(const struct arm_boot_info *info, } } +static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info *binfo) +{ +#ifdef CONFIG_FDT + uint32_t mem_reg_property[] = { cpu_to_be32(binfo->loader_start), + cpu_to_be32(binfo->ram_size) }; + void *fdt = NULL; + char *filename; + int size, rc; + + if (!current_dtb_filename) { + return 0; + } + + filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, current_dtb_filename); + if (!filename) { + fprintf(stderr, "Couldn't open dtb file %s\n", current_dtb_filename); + return -1; + } + + fdt = load_device_tree(filename, &size); + if (!fdt) { + fprintf(stderr, "Couldn't open dtb file %s\n", filename); + g_free(filename); + return -1; + } + g_free(filename); + + rc = qemu_devtree_setprop(fdt, "/memory", "reg", mem_reg_property, + sizeof(mem_reg_property)); + if (rc < 0) { + fprintf(stderr, "couldn't set /memory/reg\n"); + } + + rc = qemu_devtree_setprop_string(fdt, "/chosen", "bootargs", + binfo->kernel_cmdline); + if (rc < 0) { + fprintf(stderr, "couldn't set /chosen/bootargs\n"); + } + + if (binfo->initrd_size) { + rc = qemu_devtree_setprop_cell(fdt, "/chosen", "linux,initrd-start", + binfo->loader_start + INITRD_LOAD_ADDR); + if (rc < 0) { + fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n"); + } + + rc = qemu_devtree_setprop_cell(fdt, "/chosen", "linux,initrd-end", + binfo->loader_start + INITRD_LOAD_ADDR + + binfo->initrd_size); + if (rc < 0) { + fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n"); + } + } + + cpu_physical_memory_write(addr, fdt, size); + + return 0; + +#else + fprintf(stderr, "Platform requested a device tree, " + "but qemu was compiled without fdt support\n"); + return -1; +#endif +} + static void do_cpu_reset(void *opaque) { CPUState *env = opaque; @@ -221,12 +289,14 @@ static void do_cpu_reset(void *opaque) } else { if (env == first_cpu) { env->regs[15] = info->loader_start; - if (old_param) { - set_kernel_args_old(info, info->initrd_size, + if (!current_dtb_filename) { + if (old_param) { + set_kernel_args_old(info, info->initrd_size, + info->loader_start); + } else { + set_kernel_args(info, info->initrd_size, info->loader_start); - } else { - set_kernel_args(info, info->initrd_size, - info->loader_start); + } } } else { info->secondary_cpu_reset_hook(env, info); @@ -242,7 +312,7 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info) int n; int is_linux = 0; uint64_t elf_entry; - target_phys_addr_t entry; + target_phys_addr_t entry, dtb_start; int big_endian; /* Load the kernel. */ @@ -301,8 +371,23 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info) } else { initrd_size = 0; } + info->initrd_size = initrd_size; + + /* Place the DTB after the initrd in memory */ + dtb_start = TARGET_PAGE_ALIGN(info->loader_start + INITRD_LOAD_ADDR + + initrd_size); + if (load_dtb(dtb_start, info)) { + exit(1); + } + bootloader[4] = info->board_id; - bootloader[5] = info->loader_start + KERNEL_ARGS_ADDR; + /* for device tree boot, we pass the DTB directly in r2. Otherwise + * we point to the kernel args */ + if (current_dtb_filename) { + bootloader[5] = dtb_start; + } else { + bootloader[5] = info->loader_start + KERNEL_ARGS_ADDR; + } bootloader[6] = entry; for (n = 0; n < sizeof(bootloader) / 4; n++) { bootloader[n] = tswap32(bootloader[n]); @@ -312,7 +397,6 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info) if (info->nb_cpus > 1) { info->write_secondary_boot(env, info); } - info->initrd_size = initrd_size; } info->is_linux = is_linux; diff --git a/hw/boards.h b/hw/boards.h index f6d3784..d06776c 100644 --- a/hw/boards.h +++ b/hw/boards.h @@ -34,5 +34,6 @@ typedef struct QEMUMachine { int qemu_register_machine(QEMUMachine *m); extern QEMUMachine *current_machine; +extern const char *current_dtb_filename; #endif diff --git a/qemu-options.hx b/qemu-options.hx index 3a07ae8..309a403 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1964,6 +1964,15 @@ Use @var{file1} and @var{file2} as modules and pass arg=foo as parameter to the first module. ETEXI +DEF("dtb", HAS_ARG, QEMU_OPTION_dtb, \ + "-dtb file use 'file' as device tree image\n", QEMU_ARCH_ARM) +STEXI +@item -dtb @var{file} +@findex -dtb +Use @var{file} as a device tree binary (dtb) image and pass it to the kernel +on boot. +ETEXI + STEXI @end table ETEXI diff --git a/vl.c b/vl.c index d88a18c..b22eae3 100644 --- a/vl.c +++ b/vl.c @@ -1154,6 +1154,7 @@ void pcmcia_info(Monitor *mon) static QEMUMachine *first_machine = NULL; QEMUMachine *current_machine = NULL; +const char *current_dtb_filename = NULL; int qemu_register_machine(QEMUMachine *m) { @@ -2304,6 +2305,9 @@ int main(int argc, char **argv, char **envp) case QEMU_OPTION_initrd: initrd_filename = optarg; break; + case QEMU_OPTION_dtb: + current_dtb_filename = optarg; + break; case QEMU_OPTION_hda: { char buf[256]; @@ -3241,6 +3245,11 @@ int main(int argc, char **argv, char **envp) exit(1); } + if (!linux_boot && current_dtb_filename != NULL) { + fprintf(stderr, "-dtb only allowed with -kernel option\n"); + exit(1); + } + os_set_line_buffering(); if (init_timer_alarm() < 0) {