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