diff mbox

arm: add device tree support

Message ID 20120129203606.GB11695@ponder.secretlab.ca
State New
Headers show

Commit Message

Grant Likely Jan. 29, 2012, 8:36 p.m. UTC
On Sun, Jan 29, 2012 at 07:13:55PM +0000, Peter Maydell wrote:
> On 29 January 2012 16:01, Grant Likely <grant.likely@secretlab.ca> 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...

---

Comments

Andreas Färber Jan. 30, 2012, 11:36 a.m. UTC | #1
Am 29.01.2012 21:36, schrieb Grant Likely:
> On Sun, Jan 29, 2012 at 07:13:55PM +0000, Peter Maydell wrote:
>> On 29 January 2012 16:01, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> +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
[snip]

Please always post patches using git-send-email and indicate the version
number as [PATCH vX] (opinions are divided whether as reply or as
top-level). The changes should be described below the commit message (or
in a cover letter).

http://wiki.qemu.org/Contribute/SubmitAPatch

Andreas
Grant Likely Jan. 30, 2012, 1:31 p.m. UTC | #2
On Mon, Jan 30, 2012 at 12:36:40PM +0100, Andreas Färber wrote:
> Am 29.01.2012 21:36, schrieb Grant Likely:
> > On Sun, Jan 29, 2012 at 07:13:55PM +0000, Peter Maydell wrote:
> >> On 29 January 2012 16:01, Grant Likely <grant.likely@secretlab.ca> wrote:
> >>> +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
> [snip]
> 
> Please always post patches using git-send-email and indicate the version
> number as [PATCH vX] (opinions are divided whether as reply or as
> top-level). The changes should be described below the commit message (or
> in a cover letter).
> 
> http://wiki.qemu.org/Contribute/SubmitAPatch
> 
> Andreas

I was really only including the new versions in my replies to shorten
up the review cycle.  I'll repost correctly when it looks like all
comments are resolved.

g.
diff mbox

Patch

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) {