On Fri, Mar 04, 2016 at 08:08:36PM +0100, Patrick Wildt wrote:
> On Fri, Mar 04, 2016 at 09:38:18AM -0500, Brandon Mercer wrote:
> > On Fri, Mar 04, 2016 at 03:15:23PM +0100, Patrick Wildt wrote:
> > > Hi,
> > >
> > > this diff makes armv7 map the FDT, if available, and uses it to read
> > > information about the machine's available memory and bootargs.
> > >
> > > I'd like to get some opinions about the way I have implemented some
> > > stuff. For instance, I need the size of the FDT so I can properly
> > > copy it. Does it make sense to implement fdt_get_size()? Is there
> > > another, preferred way?
> > >
> > > Additionally, reading memory information, like where a node is mapped,
> > > is a bit more complicated than just reading two values. Especially if
> > > there are translation ranges on a node's parent. I also have working
> > > code or that, but I skipped "ranges" to not bloat the diff any further.
> >
> > If memory serves me correctly we need ranges right out of the gate for
> > the exynos boards I have, but those aren't important to anyone except
> > for me so I'm ok with waiting a short while.
>
> Yes, I stumbled upon that on the raspberry pi. The default device tree
> delivered by the raspberry pi foundation makes use of that.
>
> Excerpt:
>
> / {
> #address-cells = <0x1>;
> #size-cells = <0x1>;
> interrupt-parent = <0x1>;
> compatible = "brcm,bcm2710", "brcm,bcm2709";
> model = "Raspberry Pi 3 Model B";
> [...]
> soc {
> compatible = "simple-bus";
> #address-cells = <0x1>;
> #size-cells = <0x1>;
> ranges = <0x7e000000 0x3f000000 0x1000000>;
> [...]
> interrupt-controller@7e00b200 {
> compatible = "brcm,bcm2708-armctrl-ic";
> reg = <0x7e00b200 0x200>;
> interrupt-controller;
> #interrupt-cells = <0x2>;
> linux,phandle = <0x1>;
> phandle = <0x1>;
> };
Yes, this is needed as well. However I was commenting on reserved memory
regions. I don't want to see this turn into a bikeshed session so I
suggest we start with the initial diff, and add these additional
features soon after.
> diff --git sys/dev/ofw/fdt.c sys/dev/ofw/fdt.c
> index 18f6077..0993c92 100644
> --- sys/dev/ofw/fdt.c
> +++ sys/dev/ofw/fdt.c
> @@ -33,6 +33,7 @@ void *skip_node_name(u_int32_t *);
> void *fdt_parent_node_recurse(void *, void *);
> int fdt_node_property_int(void *, char *, int *);
> int fdt_node_property_ints(void *, char *, int *, int);
> +int fdt_translate_memory_address(void *, struct fdt_memory *);
> #ifdef DEBUG
> void fdt_print_node_recurse(void *, int);
> #endif
> @@ -361,10 +362,94 @@ fdt_parent_node(void *node)
> if (!tree_inited)
> return NULL;
>
> + if (node == pnode)
> + return NULL;
> +
> return fdt_parent_node_recurse(pnode, node);
> }
>
> /*
> + * Translate memory address depending on parent's range.
> + */
> +int
> +fdt_translate_memory_address(void *node, struct fdt_memory *mem)
> +{
> + void *parent;
> + int pac, psc, ac, sc, ret, rlen, rone, *range;
> + uint64_t from, to, size;
> +
> + if (node == NULL || mem == NULL)
> + return 1;
> +
> + parent = fdt_parent_node(node);
> + if (parent == NULL)
> + return 0;
> +
> + /* Empty ranges, 1:1 mapping. No ranges, translation barrier. */
> + rlen = fdt_node_property(node, "ranges", (char **)&range) / sizeof(int);
> + if (range == NULL)
> + return 0;
> + if (rlen <= 0)
> + return fdt_translate_memory_address(parent, mem);
> +
> + /* We only support 32-bit (1), and 64-bit (2) wide addresses here. */
> + ret = fdt_node_property_int(parent, "#address-cells", &pac);
> + if (ret != 1 || pac <= 0 || pac > 2)
> + return 1;
> +
> + /* We only support 32-bit (1), and 64-bit (2) wide sizes here. */
> + ret = fdt_node_property_int(parent, "#size-cells", &psc);
> + if (ret != 1 || psc <= 0 || psc > 2)
> + return 1;
> +
> + /* We only support 32-bit (1), and 64-bit (2) wide addresses here. */
> + ret = fdt_node_property_int(node, "#address-cells", &ac);
> + if (ret <= 0)
> + ac = pac;
> + else if (ret > 1 || ac <= 0 || ac > 2)
> + return 1;
> +
> + /* We only support 32-bit (1), and 64-bit (2) wide sizes here. */
> + ret = fdt_node_property_int(node, "#size-cells", &sc);
> + if (ret <= 0)
> + sc = psc;
> + else if (ret > 1 || sc <= 0 || sc > 2)
> + return 1;
> +
> + /* Must have at least one range. */
> + rone = pac + ac + sc;
> + if (rlen < rone)
> + return 1;
> +
> + /* For each range. */
> + for (; rlen >= rone; rlen -= rone, range += rone) {
> + /* Extract from and size, so we can see if we fit. */
> + from = betoh32(range[0]);
> + if (ac == 2)
> + from = (from << 32) + betoh32(range[1]);
> + size = betoh32(range[ac + pac]);
> + if (sc == 2)
> + size = (size << 32) + betoh32(range[ac + pac + 1]);
> +
> + /* Try next, if we're not in the range. */
> + if (mem->addr < from || (mem->addr + mem->size) > (from + size))
> + continue;
> +
> + /* All good, extract to address and translate. */
> + to = betoh32(range[ac]);
> + if (pac == 2)
> + to = (to << 32) + betoh32(range[ac + 1]);
> +
> + mem->addr -= from;
> + mem->addr += to;
> + return fdt_translate_memory_address(parent, mem);
> + }
> +
> + /* Range must have been in there. */
> + return 1;
> +}
> +
> +/*
> * Parse the memory address and size of a node.
> */
> int
> @@ -404,10 +489,7 @@ fdt_get_memory_address(void *node, int idx, struct
> fdt_memory *mem)
> if (sc == 2)
> mem->size = (mem->size << 32) + betoh32(in[off + ac + 1]);
>
> - /* TODO: translate memory address in ranges */
> - //return fdt_translate_memory_address(parent, mem);
> -
> - return 0;
> + return fdt_translate_memory_address(parent, mem);
> }
>
> #ifdef DEBUG
>
>
> > >
> > > I would like to extend the fdt_* API and add helpers to the code, so
> > > that it's easier to use the device tree. Is this the correct way?
> > > Should I rather implement some OF_* API?
> >
> > My initial thought is that it makes sense to extend fdt for this.
> >
> > I have not tested this diff, but I like the direction that this is
> > going.
> > Cheers,
> > bmercer
> > >
> > > Patrick
> > >
> > > diff --git sys/arch/armv7/armv7/armv7_machdep.c
> > > sys/arch/armv7/armv7/armv7_machdep.c
> > > index 11bea90..0a067c2 100644
> > > --- sys/arch/armv7/armv7/armv7_machdep.c
> > > +++ sys/arch/armv7/armv7/armv7_machdep.c
> > > @@ -130,6 +130,7 @@
> > > #include <armv7/armv7/armv7_machdep.h>
> > >
> > > #include <dev/cons.h>
> > > +#include <dev/ofw/fdt.h>
> > >
> > > #include <net/if.h>
> > >
> > > @@ -387,8 +388,10 @@ initarm(void *arg0, void *arg1, void *arg2)
> > > int loop, loop1, i, physsegs;
> > > u_int l1pagetable;
> > > pv_addr_t kernel_l1pt;
> > > + pv_addr_t fdt;
> > > paddr_t memstart;
> > > psize_t memsize;
> > > + void *config;
> > > extern uint32_t esym; /* &_end if no symbols are loaded */
> > >
> > > /* early bus_space_map support */
> > > @@ -423,6 +426,49 @@ initarm(void *arg0, void *arg1, void *arg2)
> > > armv7_a4x_bs_tag.bs_map = bootstrap_bs_map;
> > > tmp_bs_tag.bs_map = bootstrap_bs_map;
> > >
> > > + /*
> > > + * Now, map the bootconfig/FDT area.
> > > + *
> > > + * As we don't know the size of a possible FDT, map the size of a
> > > + * typical bootstrap bs map. The FDT might not be aligned, so this
> > > + * might take up to two L1_S_SIZEd mappings. In the unlikely case
> > > + * that the FDT is bigger than L1_S_SIZE (0x00100000), we need to
> > > + * remap it.
> > > + *
> > > + * XXX: There's (currently) no way to unmap a bootstrap mapping, so
> > > + * we might lose a bit of the bootstrap address space.
> > > + */
> > > + bootstrap_bs_map(NULL, (bus_addr_t)arg2, L1_S_SIZE, 0,
> > > + (bus_space_handle_t *)&config);
> > > + if (fdt_init(config) && fdt_get_size(config) != 0) {
> > > + uint32_t size = fdt_get_size(config);
> > > + if (size > L1_S_SIZE)
> > > + bootstrap_bs_map(NULL, (bus_addr_t)arg2, size, 0,
> > > + (bus_space_handle_t *)&config);
> > > + }
> > > +
> > > + if (fdt_init(config) && fdt_get_size(config) != 0) {
> > > + struct fdt_memory mem;
> > > + void *node;
> > > +
> > > + node = fdt_find_node("/memory");
> > > + if (node == NULL || fdt_get_memory_address(node, 0, &mem))
> > > + panic("initarm: no memory specificed");
> > > +
> > > + memstart = mem.addr;
> > > + memsize = mem.size;
> > > + physical_start = mem.addr;
> > > + physical_end = MIN(mem.addr + mem.size, (paddr_t)-PAGE_SIZE);
> > > +
> > > + node = fdt_find_node("/chosen");
> > > + if (node != NULL) {
> > > + char *bootargs;
> > > + if (fdt_node_property(node, "bootargs", &bootargs))
> > > + process_kernel_args(bootargs);
> > > + }
> > > + }
> > > +
> > > + /* XXX: Use FDT information. */
> > > platform_init();
> > > platform_disable_l2_if_needed();
> > >
> > > @@ -433,41 +479,36 @@ initarm(void *arg0, void *arg1, void *arg2)
> > > printf("\n%s booting ...\n", platform_boot_name());
> > >
> > > printf("arg0 %p arg1 %p arg2 %p\n", arg0, arg1, arg2);
> > > - parse_uboot_tags(arg2);
> > >
> > > - /*
> > > - * Examine the boot args string for options we need to know about
> > > - * now.
> > > - */
> > > - process_kernel_args(bootconfig.bootstring);
> > > + if (fdt_get_size(config) == 0) {
> > > + parse_uboot_tags(config);
> > > +
> > > + /*
> > > + * Examine the boot args string for options we need to know
> > > about
> > > + * now.
> > > + */
> > > + process_kernel_args(bootconfig.bootstring);
> > > +
> > > + /* normally u-boot will set up bootconfig.dramblocks */
> > > + bootconfig_dram(&bootconfig, &memstart, &memsize);
> > > +
> > > + /*
> > > + * Set up the variables that define the availablilty of
> > > + * physical memory.
> > > + *
> > > + * XXX pmap_bootstrap() needs an enema.
> > > + */
> > > + physical_start = bootconfig.dram[0].address;
> > > + physical_end = MIN((uint64_t)physical_start +
> > > + (bootconfig.dram[0].pages * PAGE_SIZE),
> > > (paddr_t)-PAGE_SIZE);
> > > + }
> > > +
> > > #ifdef RAMDISK_HOOKS
> > > - boothowto |= RB_DFLTROOT;
> > > + boothowto |= RB_DFLTROOT;
> > > #endif /* RAMDISK_HOOKS */
> > >
> > > - /* normally u-boot will set up bootconfig.dramblocks */
> > > - bootconfig_dram(&bootconfig, &memstart, &memsize);
> > > -
> > > - /*
> > > - * Set up the variables that define the availablilty of
> > > - * physical memory. For now, we're going to set
> > > - * physical_freestart to 0xa0200000 (where the kernel
> > > - * was loaded), and allocate the memory we need downwards.
> > > - * If we get too close to the page tables that RedBoot
> > > - * set up, we will panic. We will update physical_freestart
> > > - * and physical_freeend later to reflect what pmap_bootstrap()
> > > - * wants to see.
> > > - *
> > > - * XXX pmap_bootstrap() needs an enema.
> > > - */
> > > - physical_start = bootconfig.dram[0].address;
> > > - physical_end = MIN((uint64_t)physical_start +
> > > - (bootconfig.dram[0].pages * PAGE_SIZE), (paddr_t)-PAGE_SIZE);
> > > -
> > > - {
> > > - physical_freestart = (((unsigned long)esym - KERNEL_TEXT_BASE
> > > +0xfff) & ~0xfff) + memstart;
> > > - physical_freeend = MIN((uint64_t)memstart+memsize,
> > > - (paddr_t)-PAGE_SIZE);
> > > - }
> > > + physical_freestart = (((unsigned long)esym - KERNEL_TEXT_BASE +0xfff) &
> > > ~0xfff) + memstart;
> > > + physical_freeend = MIN((uint64_t)memstart+memsize, (paddr_t)-PAGE_SIZE);
> > >
> > > physmem = (physical_end - physical_start) / PAGE_SIZE;
> > >
> > > @@ -566,6 +607,15 @@ initarm(void *arg0, void *arg1, void *arg2)
> > > #endif
> > >
> > > /*
> > > + * Allocate pages for an FDT copy.
> > > + */
> > > + if (fdt_get_size(config) != 0) {
> > > + uint32_t size = fdt_get_size(config);
> > > + valloc_pages(fdt, round_page(size) / PAGE_SIZE);
> > > + memcpy((void *)fdt.pv_pa, config, size);
> > > + }
> > > +
> > > + /*
> > > * XXX Defer this to later so that we can reclaim the memory
> > > * XXX used by the RedBoot page tables.
> > > */
> > > @@ -656,6 +706,12 @@ initarm(void *arg0, void *arg1, void *arg2)
> > > pmap_map_entry(l1pagetable, vector_page, systempage.pv_pa,
> > > PROT_READ | PROT_WRITE | PROT_EXEC, PTE_CACHE);
> > >
> > > + /* Map the FDT. */
> > > + if (fdt.pv_va && fdt.pv_pa)
> > > + pmap_map_chunk(l1pagetable, fdt.pv_va, fdt.pv_pa,
> > > + round_page(fdt_get_size((void *)fdt.pv_pa)),
> > > + PROT_READ | PROT_WRITE, PTE_CACHE);
> > > +
> > > /*
> > > * map integrated peripherals at same address in l1pagetable
> > > * so that we can continue to use console.
> > > @@ -716,6 +772,10 @@ initarm(void *arg0, void *arg1, void *arg2)
> > > prefetch_abort_handler_address = (u_int)prefetch_abort_handler;
> > > undefined_handler_address = (u_int)undefinedinstruction_bounce;
> > >
> > > + /* Now we can reinit the FDT, using the virtual address. */
> > > + if (fdt.pv_va && fdt.pv_pa)
> > > + fdt_init((void *)fdt.pv_va);
> > > +
> > > /* Initialise the undefined instruction handlers */
> > > #ifdef VERBOSE_INIT_ARM
> > > printf("undefined ");
> > > diff --git sys/arch/armv7/conf/files.armv7 sys/arch/armv7/conf/files.armv7
> > > index 84c6064..3136ad0 100644
> > > --- sys/arch/armv7/conf/files.armv7
> > > +++ sys/arch/armv7/conf/files.armv7
> > > @@ -10,6 +10,7 @@ major {rd = 18}
> > >
> > > define fdt {}
> > > file arch/armv7/fdt/fdt_machdep.c fdt needs-flag
> > > +file dev/ofw/fdt.c
> > >
> > > file arch/arm/arm/conf.c
> > >
> > > diff --git sys/dev/ofw/fdt.c sys/dev/ofw/fdt.c
> > > index 6111faa..18f6077 100644
> > > --- sys/dev/ofw/fdt.c
> > > +++ sys/dev/ofw/fdt.c
> > > @@ -31,6 +31,8 @@ void *skip_property(u_int32_t *);
> > > void *skip_props(u_int32_t *);
> > > void *skip_node_name(u_int32_t *);
> > > void *fdt_parent_node_recurse(void *, void *);
> > > +int fdt_node_property_int(void *, char *, int *);
> > > +int fdt_node_property_ints(void *, char *, int *, int);
> > > #ifdef DEBUG
> > > void fdt_print_node_recurse(void *, int);
> > > #endif
> > > @@ -95,6 +97,21 @@ fdt_init(void *fdt)
> > > return version;
> > > }
> > >
> > > + /*
> > > + * Return the size of the FDT.
> > > + */
> > > +size_t
> > > +fdt_get_size(void *fdt)
> > > +{
> > > + if (!fdt)
> > > + return 0;
> > > +
> > > + if (!fdt_check_head(fdt))
> > > + return 0;
> > > +
> > > + return betoh32(((struct fdt_head *)fdt)->fh_size);
> > > +}
> > > +
> > > /*
> > > * Retrieve string pointer from strings table.
> > > */
> > > @@ -172,6 +189,35 @@ fdt_node_property(void *node, char *name, char **out)
> > > }
> > >
> > > /*
> > > + * Retrieves node property as integers and puts them in the given
> > > + * integer array.
> > > + */
> > > +int
> > > +fdt_node_property_ints(void *node, char *name, int *out, int outlen)
> > > +{
> > > + int *data;
> > > + int i, inlen;
> > > +
> > > + inlen = fdt_node_property(node, name, (char **)&data) / sizeof(int);
> > > + if (inlen <= 0)
> > > + return -1;
> > > +
> > > + for (i = 0; i < inlen && i < outlen; i++)
> > > + out[i] = betoh32(data[i]);
> > > +
> > > + return i;
> > > +}
> > > +
> > > +/*
> > > + * Retrieves node property as an integer.
> > > + */
> > > +int
> > > +fdt_node_property_int(void *node, char *name, int *out)
> > > +{
> > > + return fdt_node_property_ints(node, name, out, 1);
> > > +}
> > > +
> > > +/*
> > > * Retrieves next node, skipping all the children nodes of the pointed
> > > node
> > > * if passed 0 wil return first node of the tree (root)
> > > */
> > > @@ -318,6 +364,52 @@ fdt_parent_node(void *node)
> > > return fdt_parent_node_recurse(pnode, node);
> > > }
> > >
> > > +/*
> > > + * Parse the memory address and size of a node.
> > > + */
> > > +int
> > > +fdt_get_memory_address(void *node, int idx, struct fdt_memory *mem)
> > > +{
> > > + void *parent;
> > > + int ac, sc, off, ret, *in, inlen;
> > > +
> > > + if (node == NULL)
> > > + return 1;
> > > +
> > > + parent = fdt_parent_node(node);
> > > + if (parent == NULL)
> > > + return 1;
> > > +
> > > + /* We only support 32-bit (1), and 64-bit (2) wide addresses here. */
> > > + ret = fdt_node_property_int(parent, "#address-cells", &ac);
> > > + if (ret != 1 || ac <= 0 || ac > 2)
> > > + return 1;
> > > +
> > > + /* We only support 32-bit (1), and 64-bit (2) wide sizes here. */
> > > + ret = fdt_node_property_int(parent, "#size-cells", &sc);
> > > + if (ret != 1 || sc <= 0 || sc > 2)
> > > + return 1;
> > > +
> > > + inlen = fdt_node_property(node, "reg", (char **)&in) / sizeof(int);
> > > + if (inlen < ((idx + 1) * (ac + sc)))
> > > + return 1;
> > > +
> > > + off = idx * (ac + sc);
> > > +
> > > + mem->addr = betoh32(in[off]);
> > > + if (ac == 2)
> > > + mem->addr = (mem->addr << 32) + betoh32(in[off + 1]);
> > > +
> > > + mem->size = betoh32(in[off + ac]);
> > > + if (sc == 2)
> > > + mem->size = (mem->size << 32) + betoh32(in[off + ac + 1]);
> > > +
> > > + /* TODO: translate memory address in ranges */
> > > + //return fdt_translate_memory_address(parent, mem);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > #ifdef DEBUG
> > > /*
> > > * Debug methods for printing whole tree, particular odes and properies
> > > diff --git sys/dev/ofw/fdt.h sys/dev/ofw/fdt.h
> > > index 6a21239..ecb8bed 100644
> > > --- sys/dev/ofw/fdt.h
> > > +++ sys/dev/ofw/fdt.h
> > > @@ -38,6 +38,11 @@ struct fdt {
> > > int strings_size;
> > > };
> > >
> > > +struct fdt_memory {
> > > + uint64_t addr;
> > > + uint64_t size;
> > > +};
> > > +
> > > #define FDT_MAGIC 0xd00dfeed
> > > #define FDT_NODE_BEGIN 0x01
> > > #define FDT_NODE_END 0x02
> > > @@ -48,12 +53,14 @@ struct fdt {
> > > #define FDT_CODE_VERSION 0x11
> > >
> > > int fdt_init(void *);
> > > +size_t fdt_get_size(void *);
> > > void *fdt_next_node(void *);
> > > void *fdt_child_node(void *);
> > > char *fdt_node_name(void *);
> > > void *fdt_find_node(char *);
> > > int fdt_node_property(void *, char *, char **);
> > > void *fdt_parent_node(void *);
> > > +int fdt_get_memory_address(void *, int, struct fdt_memory *);
> > > #ifdef DEBUG
> > > void *fdt_print_property(void *, int);
> > > void fdt_print_node(void *, int);
> > >
> >
