Message ID | 1398514631-23956-11-git-send-email-marc.zyngier@arm.com |
---|---|
State | Superseded |
Headers | show |
I'm a U-Boot newbie so please feel free to correct how I'm reporting this issue.. I recently downloaded the 2014.04-rc3 snapshot to build U-Boot for my custom DA850-based board. The only change was to add a new target "dav850evm_nand" in boards.cfg with the added parameter "USE_NAND". The resulting AIS file was programmed into EVM-compatible NAND using standard sfh_OMAP-L138 method. The board failed to boot, and stayed in a loop printing the SPL console message repeatedly. After some debugging with CCS 5.5 and an XDS100v2, I found that incorrect code was being loaded into the 0xc108000 RAM destination. The da850evm.h file defines CONFIG_SYS_NAND_U_BOOT_OFFS as 0x28000, which corresponds to an AIS offset of 0x8000 but the u-boot header did not appear there in the AIS file. A search revealed that the Makefile catenated u-boot immediately after the SPL without any padding. Further investigation revealed that the target Makefile needs CONFIG_SPL_MAX_SIZE to be defined as 0x8000 in order for the padding to be performed properly; however, this constant was apparently deleted during a series of changes in April, 2013 to accommodate separate code and BSS size limits for another target. In its place, CONFIG_SPL_MAX_FOOTPRINT was defined as 32768. Unfortunately, the da850evm Makefile does not refer to this constant. To solve the problem, I added the following 2 lines in my custom-modified da850evm.h: #define CONFIG_SPL_PAD_TO 0x8000 #define CONFIG_SPL_MAX_SIZE 0x8000 although the first line may not be strictly required. This solved the problem and allowed the board to boot. Doesn't this mean that other similar targets may be broken? How can I assist with contributing the patch to fix this? One problem I have is that I only have an EXP, not an EVM board. My custom target board shares some features with the EVM such as NOR and NAND memory, but other things are different like the use of a fixed PHY. This makes it impossible for me to completely verify a da850evm build. I would like to see more target configurations available for the DA850EVM but it seems like development for this has stopped. NAND boot was a simple change, but adding USB support required me to copy & paste code from the da830evm target. Are there any future plans to do this, or will I need to do this development for my custom board only? Tom Taylor
On Sat, Apr 26, 2014 at 7:17 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > diff --git a/arch/arm/cpu/armv7/virt-dt.c b/arch/arm/cpu/armv7/virt-dt.c > new file mode 100644 > index 0000000..0b0d6a7 > --- /dev/null > +++ b/arch/arm/cpu/armv7/virt-dt.c > + > +static int fdt_psci(void *fdt) > +{ > +#ifdef CONFIG_ARMV7_PSCI > + int nodeoff; > + int tmp; > + > + nodeoff = fdt_path_offset(fdt, "/cpus"); > + if (nodeoff < 0) { > + printf("couldn't find /cpus\n"); > + return nodeoff; > + } > + > + /* add 'enable-method = "psci"' to each cpu node */ > + for (tmp = fdt_first_subnode(fdt, nodeoff); > + tmp >= 0; > + tmp = fdt_next_subnode(fdt, tmp)) { > + const struct fdt_property *prop; > + int len; > + > + prop = fdt_get_property(fdt, tmp, "device_type", &len); > + if (!prop) > + continue; > + if (len < 4) > + continue; > + if (strcmp(prop->data, "cpu")) > + continue; > + > + fdt_setprop_string(fdt, tmp, "enable-method", "psci"); > + } > + > + nodeoff = fdt_path_offset(fdt, "/psci"); > + if (nodeoff < 0) { > + nodeoff = fdt_path_offset(fdt, "/"); > + if (nodeoff < 0) > + return nodeoff; > + > + nodeoff = fdt_add_subnode(fdt, nodeoff, "psci"); > + if (nodeoff < 0) > + return nodeoff; > + } > + > + tmp = fdt_setprop_string(fdt, nodeoff, "compatible", "arm,psci"); > + if (tmp) > + return tmp; > + tmp = fdt_setprop_string(fdt, nodeoff, "method", "smc"); > + if (tmp) > + return tmp; > + tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend", ARM_PSCI_FN_CPU_SUSPEND); > + if (tmp) > + return tmp; > + tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off", ARM_PSCI_FN_CPU_OFF); > + if (tmp) > + return tmp; > + tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on", ARM_PSCI_FN_CPU_ON); > + if (tmp) > + return tmp; > + tmp = fdt_setprop_u32(fdt, nodeoff, "migrate", ARM_PSCI_FN_MIGRATE); > + if (tmp) > + return tmp; > +#endif > + return 0; > +} > So, I wonder if it would be better to be a bit more selective or cautious about adding these nodes and properties. Specifically, if they are already present in the device tree itself, perhaps they should be honored and left alone? I understand that U-Boot gets to define what it implements, and that if the secure monitor code doesn't actually implement something, or for that matter *does* implement it, it makes sense for U-Boot to be able to state those facts in a device tree. However, the DTS may also be stating what it has implemented or willing to honor on the Linux side as well. So, yeah, there has to be agreement here. But who gets to make the final adjustment to the device tree? U-boot with this code, or the DTS author who may have hand coded specific wishes and loaded a specific device tree? HTH, jdl
On Sat, Apr 26, 2014 at 01:34:58PM -0400, Tom Taylor wrote: > I'm a U-Boot newbie so please feel free to correct how I'm reporting > this issue.. > > I recently downloaded the 2014.04-rc3 snapshot to build U-Boot for > my custom DA850-based board. The only change was to add a new > target "dav850evm_nand" in boards.cfg with the added parameter > "USE_NAND". > > The resulting AIS file was programmed into EVM-compatible NAND using > standard sfh_OMAP-L138 method. > > The board failed to boot, and stayed in a loop printing the SPL > console message repeatedly. > > After some debugging with CCS 5.5 and an XDS100v2, I found that > incorrect code was being loaded into the 0xc108000 RAM destination. > The da850evm.h file defines CONFIG_SYS_NAND_U_BOOT_OFFS as 0x28000, > which corresponds to an AIS offset of 0x8000 but the u-boot header > did not appear there in the AIS file. A search revealed that the > Makefile catenated u-boot immediately after the SPL without any > padding. > > Further investigation revealed that the target Makefile needs > CONFIG_SPL_MAX_SIZE to be defined as 0x8000 in order for the padding > to be performed properly; however, this constant was apparently > deleted during a series of changes in April, 2013 to accommodate > separate code and BSS size limits for another target. In its place, > CONFIG_SPL_MAX_FOOTPRINT was defined as 32768. Unfortunately, the > da850evm Makefile does not refer to this constant. > > To solve the problem, I added the following 2 lines in my > custom-modified da850evm.h: > #define CONFIG_SPL_PAD_TO 0x8000 > #define CONFIG_SPL_MAX_SIZE 0x8000 Thanks for looking into this. I saw a similar issue on my am18xx EVM but was 5 bugs deep and didn't have time to bisect it then. I think the answer here is to post a patch fixing the boards listed in: $ git grep -l ARM926EJS include/configs/ | xargs grep -l DAVINCI As they all share that set of constraints. Thanks for digging into this!
On Fri, May 02 2014 at 9:13:08 pm BST, Jon Loeliger <loeliger@gmail.com> wrote: > On Sat, Apr 26, 2014 at 7:17 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > >> diff --git a/arch/arm/cpu/armv7/virt-dt.c b/arch/arm/cpu/armv7/virt-dt.c >> new file mode 100644 >> index 0000000..0b0d6a7 >> --- /dev/null >> +++ b/arch/arm/cpu/armv7/virt-dt.c > >> + >> +static int fdt_psci(void *fdt) >> +{ >> +#ifdef CONFIG_ARMV7_PSCI >> + int nodeoff; >> + int tmp; >> + >> + nodeoff = fdt_path_offset(fdt, "/cpus"); >> + if (nodeoff < 0) { >> + printf("couldn't find /cpus\n"); >> + return nodeoff; >> + } >> + >> + /* add 'enable-method = "psci"' to each cpu node */ >> + for (tmp = fdt_first_subnode(fdt, nodeoff); >> + tmp >= 0; >> + tmp = fdt_next_subnode(fdt, tmp)) { >> + const struct fdt_property *prop; >> + int len; >> + >> + prop = fdt_get_property(fdt, tmp, "device_type", &len); >> + if (!prop) >> + continue; >> + if (len < 4) >> + continue; >> + if (strcmp(prop->data, "cpu")) >> + continue; >> + >> + fdt_setprop_string(fdt, tmp, "enable-method", "psci"); >> + } >> + >> + nodeoff = fdt_path_offset(fdt, "/psci"); >> + if (nodeoff < 0) { >> + nodeoff = fdt_path_offset(fdt, "/"); >> + if (nodeoff < 0) >> + return nodeoff; >> + >> + nodeoff = fdt_add_subnode(fdt, nodeoff, "psci"); >> + if (nodeoff < 0) >> + return nodeoff; >> + } >> + >> + tmp = fdt_setprop_string(fdt, nodeoff, "compatible", "arm,psci"); >> + if (tmp) >> + return tmp; >> + tmp = fdt_setprop_string(fdt, nodeoff, "method", "smc"); >> + if (tmp) >> + return tmp; >> + tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend", >> ARM_PSCI_FN_CPU_SUSPEND); >> + if (tmp) >> + return tmp; >> + tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off", ARM_PSCI_FN_CPU_OFF); >> + if (tmp) >> + return tmp; >> + tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on", ARM_PSCI_FN_CPU_ON); >> + if (tmp) >> + return tmp; >> + tmp = fdt_setprop_u32(fdt, nodeoff, "migrate", ARM_PSCI_FN_MIGRATE); >> + if (tmp) >> + return tmp; >> +#endif >> + return 0; >> +} >> Hi Jon, > So, I wonder if it would be better to be a bit more selective or > cautious about adding these nodes and properties. Specifically, if > they are already present in the device tree itself, perhaps they > should be honored and left alone? Well, we have exactly two possibilities: - PSCI is provided by the platform's firmware, and we'd better not touch the DT at all. - PSCI is provided U-Boot, and we *own* the PSCI related nodes. I don't think there is an alternative to that, because either U-Boot or the firmware will install its own secure vectors. The DT manipulation code just reflect this situation. > I understand that U-Boot gets to define what it implements, and that if > the secure monitor code doesn't actually implement something, or for > that matter *does* implement it, it makes sense for U-Boot to be able > to state those facts in a device tree. However, the DTS may also be > stating what it has implemented or willing to honor on the Linux side > as well. So, yeah, there has to be agreement here. > > But who gets to make the final adjustment to the device tree? U-boot > with this code, or the DTS author who may have hand coded specific > wishes and loaded a specific device tree? We could have an environment variable named "i_know_this_looks_silly_but_nevertheless_please_pretty_please_leave_my_cpu_nodes_alone"? M.
diff --git a/arch/arm/cpu/armv7/Makefile b/arch/arm/cpu/armv7/Makefile index de1cc1a..44329cd 100644 --- a/arch/arm/cpu/armv7/Makefile +++ b/arch/arm/cpu/armv7/Makefile @@ -21,6 +21,7 @@ endif ifneq ($(CONFIG_ARMV7_NONSEC)$(CONFIG_ARMV7_VIRT),) obj-y += nonsec_virt.o obj-y += virt-v7.o +obj-y += virt-dt.o endif ifneq ($(CONFIG_ARMV7_PSCI),) diff --git a/arch/arm/cpu/armv7/virt-dt.c b/arch/arm/cpu/armv7/virt-dt.c new file mode 100644 index 0000000..0b0d6a7 --- /dev/null +++ b/arch/arm/cpu/armv7/virt-dt.c @@ -0,0 +1,100 @@ +/* + * Copyright (C) 2013 - ARM Ltd + * Author: Marc Zyngier <marc.zyngier@arm.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <common.h> +#include <stdio_dev.h> +#include <linux/ctype.h> +#include <linux/types.h> +#include <asm/global_data.h> +#include <libfdt.h> +#include <fdt_support.h> +#include <asm/armv7.h> +#include <asm/psci.h> + +static int fdt_psci(void *fdt) +{ +#ifdef CONFIG_ARMV7_PSCI + int nodeoff; + int tmp; + + nodeoff = fdt_path_offset(fdt, "/cpus"); + if (nodeoff < 0) { + printf("couldn't find /cpus\n"); + return nodeoff; + } + + /* add 'enable-method = "psci"' to each cpu node */ + for (tmp = fdt_first_subnode(fdt, nodeoff); + tmp >= 0; + tmp = fdt_next_subnode(fdt, tmp)) { + const struct fdt_property *prop; + int len; + + prop = fdt_get_property(fdt, tmp, "device_type", &len); + if (!prop) + continue; + if (len < 4) + continue; + if (strcmp(prop->data, "cpu")) + continue; + + fdt_setprop_string(fdt, tmp, "enable-method", "psci"); + } + + nodeoff = fdt_path_offset(fdt, "/psci"); + if (nodeoff < 0) { + nodeoff = fdt_path_offset(fdt, "/"); + if (nodeoff < 0) + return nodeoff; + + nodeoff = fdt_add_subnode(fdt, nodeoff, "psci"); + if (nodeoff < 0) + return nodeoff; + } + + tmp = fdt_setprop_string(fdt, nodeoff, "compatible", "arm,psci"); + if (tmp) + return tmp; + tmp = fdt_setprop_string(fdt, nodeoff, "method", "smc"); + if (tmp) + return tmp; + tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend", ARM_PSCI_FN_CPU_SUSPEND); + if (tmp) + return tmp; + tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off", ARM_PSCI_FN_CPU_OFF); + if (tmp) + return tmp; + tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on", ARM_PSCI_FN_CPU_ON); + if (tmp) + return tmp; + tmp = fdt_setprop_u32(fdt, nodeoff, "migrate", ARM_PSCI_FN_MIGRATE); + if (tmp) + return tmp; +#endif + return 0; +} + +int armv7_update_dt(void *fdt) +{ +#ifndef CONFIG_ARMV7_SECURE_BASE + /* secure code lives in RAM, keep it alive */ + fdt_add_mem_rsv(fdt, (unsigned long)__secure_start, + __secure_end - __secure_start); +#endif + + return fdt_psci(fdt); +} diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h index 11476dd..323f282 100644 --- a/arch/arm/include/asm/armv7.h +++ b/arch/arm/include/asm/armv7.h @@ -79,6 +79,7 @@ void v7_outer_cache_inval_range(u32 start, u32 end); #if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT) int armv7_init_nonsec(void); +int armv7_update_dt(void *fdt); /* defined in assembly file */ unsigned int _nonsec_init(void); diff --git a/arch/arm/lib/bootm-fdt.c b/arch/arm/lib/bootm-fdt.c index 8394e15..d4f1578 100644 --- a/arch/arm/lib/bootm-fdt.c +++ b/arch/arm/lib/bootm-fdt.c @@ -17,13 +17,14 @@ #include <common.h> #include <fdt_support.h> +#include <asm/armv7.h> DECLARE_GLOBAL_DATA_PTR; int arch_fixup_fdt(void *blob) { bd_t *bd = gd->bd; - int bank; + int bank, ret; u64 start[CONFIG_NR_DRAM_BANKS]; u64 size[CONFIG_NR_DRAM_BANKS]; @@ -32,5 +33,12 @@ int arch_fixup_fdt(void *blob) size[bank] = bd->bi_dram[bank].size; } - return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS); + ret = fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS); +#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT) + if (ret) + return ret; + + ret = armv7_update_dt(blob); +#endif + return ret; }
Generate the PSCI node in the device tree. Also add a reserve section for the "secure" code that lives in in normal RAM, so that the kernel knows it'd better not trip on it. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm/cpu/armv7/Makefile | 1 + arch/arm/cpu/armv7/virt-dt.c | 100 +++++++++++++++++++++++++++++++++++++++++++ arch/arm/include/asm/armv7.h | 1 + arch/arm/lib/bootm-fdt.c | 12 +++++- 4 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 arch/arm/cpu/armv7/virt-dt.c