| Message ID | 1422619129-23352-2-git-send-email-siarhei.siamashka@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Hans de Goede |
| Headers | show |
On Fri, 30 Jan 2015 13:58:46 +0200 Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote: > The commit f630974ccb3ce93e9607a3354e9acb266a8b7e95 > 'sunxi: Move SPL s_init() code to board_init_f()' > broke the FEL boot mode. > > This patch moves the DRAM initialization back to s_init() and > introduces an assembly entry point for FEL in order to provide > guaranteed initialization of the gdata pointer (r9). The assembly > entry point is also needed to ensure that the SPL code starts > executing in ARM mode. > > Because the sunxi board_init_f() does not contain anything that > is not already done by the default board_init_f(), it is removed > too. > > Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com> > --- > arch/arm/cpu/armv7/sunxi/Makefile | 1 + > arch/arm/cpu/armv7/sunxi/board.c | 26 ++++++++++---------------- > arch/arm/cpu/armv7/sunxi/start_fel.S | 16 ++++++++++++++++ > arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds | 3 ++- > 4 files changed, 29 insertions(+), 17 deletions(-) > create mode 100644 arch/arm/cpu/armv7/sunxi/start_fel.S [...] > +++ b/arch/arm/cpu/armv7/sunxi/start_fel.S > @@ -0,0 +1,16 @@ > +/* > + * Entry point of the FEL mode SPL. > + * > + * Copyright (c) 2015 Siarhei Siamashka <siarhei.siamashka@gmail.com> > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <asm-offsets.h> > +#include <config.h> > +#include <linux/linkage.h> > + > +ENTRY(_start_fel) > + ldr r9, =gdata > + b s_init > +ENDPROC(_start_fel) In fact, we probably need to save/restore the r9 register and do it as: push {r9, lr} ldr r9, =gdata bl s_init pop {r9, pc} And maybe save some other registers, depending on the calling conventions expected by the FEL code in BROM. As a side note, corrupting r9 mimics the old u-boot sunxi behaviour. And it used not to cause any visible problems so far, at least when working with the BROM code in the current Allwinner SoCs. Also as I see it, the ".bss" sections is supposed to be in DRAM, and cleared only after the DRAM is initialized. This violates the C standard a little bit and enforces some sort of u-boot specific coding tricks. Such as explicitly placing gdata in the ".data" section instead of ".bss". This is ugly, but probably justified. I'll submit a fixed v2 version of this patch later, but will first wait for additional comments from the other people.
Hi, On 30 January 2015 at 04:58, Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote: > The commit f630974ccb3ce93e9607a3354e9acb266a8b7e95 > 'sunxi: Move SPL s_init() code to board_init_f()' > broke the FEL boot mode. > > This patch moves the DRAM initialization back to s_init() and > introduces an assembly entry point for FEL in order to provide > guaranteed initialization of the gdata pointer (r9). The assembly > entry point is also needed to ensure that the SPL code starts > executing in ARM mode. > > Because the sunxi board_init_f() does not contain anything that > is not already done by the default board_init_f(), it is removed > too. > > Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com> > --- > arch/arm/cpu/armv7/sunxi/Makefile | 1 + > arch/arm/cpu/armv7/sunxi/board.c | 26 ++++++++++---------------- > arch/arm/cpu/armv7/sunxi/start_fel.S | 16 ++++++++++++++++ > arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds | 3 ++- > 4 files changed, 29 insertions(+), 17 deletions(-) > create mode 100644 arch/arm/cpu/armv7/sunxi/start_fel.S > > diff --git a/arch/arm/cpu/armv7/sunxi/Makefile b/arch/arm/cpu/armv7/sunxi/Makefile > index 48db744..e0d413d 100644 > --- a/arch/arm/cpu/armv7/sunxi/Makefile > +++ b/arch/arm/cpu/armv7/sunxi/Makefile > @@ -40,5 +40,6 @@ obj-$(CONFIG_MACH_SUN7I) += dram_sun4i.o > obj-$(CONFIG_MACH_SUN8I) += dram_sun8i.o > ifdef CONFIG_SPL_FEL > obj-y += start.o > +extra-y += start_fel.o > endif > endif > diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c > index 6e28bcd..ea6cb60 100644 > --- a/arch/arm/cpu/armv7/sunxi/board.c > +++ b/arch/arm/cpu/armv7/sunxi/board.c > @@ -85,6 +85,16 @@ void s_init(void) > timer_init(); > gpio_init(); > i2c_init_board(); > + > +#ifdef CONFIG_SPL_BUILD > + preloader_console_init(); s_init() is called before we have global_data, so you can't use a console. > + > +#ifdef CONFIG_SPL_I2C_SUPPORT > + /* Needed early by sunxi_board_init if PMU is enabled */ > + i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); > +#endif > + sunxi_board_init(); > +#endif Why do you need this code here? Can it not go in board_init_f()? > } > > #ifdef CONFIG_SPL_BUILD > @@ -103,22 +113,6 @@ u32 spl_boot_mode(void) > { > return MMCSD_MODE_RAW; > } > - > -void board_init_f(ulong dummy) > -{ > - preloader_console_init(); > - > -#ifdef CONFIG_SPL_I2C_SUPPORT > - /* Needed early by sunxi_board_init if PMU is enabled */ > - i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); > -#endif > - sunxi_board_init(); > - > - /* Clear the BSS. */ > - memset(__bss_start, 0, __bss_end - __bss_start); > - > - board_init_r(NULL, 0); > -} > #endif > > void reset_cpu(ulong addr) > diff --git a/arch/arm/cpu/armv7/sunxi/start_fel.S b/arch/arm/cpu/armv7/sunxi/start_fel.S > new file mode 100644 > index 0000000..e1c7cd4 > --- /dev/null > +++ b/arch/arm/cpu/armv7/sunxi/start_fel.S > @@ -0,0 +1,16 @@ > +/* > + * Entry point of the FEL mode SPL. > + * > + * Copyright (c) 2015 Siarhei Siamashka <siarhei.siamashka@gmail.com> > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <asm-offsets.h> > +#include <config.h> > +#include <linux/linkage.h> > + > +ENTRY(_start_fel) > + ldr r9, =gdata > + b s_init No we don't want global data here, and need to get rid of gdata so we can use driver model, etc. > +ENDPROC(_start_fel) > diff --git a/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds b/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds > index 928b7c1..beb8900 100644 > --- a/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds > +++ b/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds > @@ -6,7 +6,7 @@ > */ > OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm") > OUTPUT_ARCH(arm) > -ENTRY(s_init) > +ENTRY(_start_fel) > SECTIONS > { > . = 0x00002000; > @@ -14,6 +14,7 @@ SECTIONS > . = ALIGN(4); > .text : > { > + arch/arm/cpu/armv7/sunxi/start_fel.o (.text) > *(.text.s_init) Why does this have to jump to a special s_init? Can it not just start SPL normally as it does on Tegra, Exynos, etc? > *(.text*) > } There has to be a better way of making this work. Also do you have instructions on how I can try this out on a pcduino3 or other low-cost board? I understand that we need to fix this, but other archs deal with this within the existing framework, so I'd really like to get sunxi into the same state. Regards, Simon
On Sun, 1 Feb 2015 09:28:36 -0700 Simon Glass <sjg@chromium.org> wrote: > Hi, > > On 30 January 2015 at 04:58, Siarhei Siamashka > <siarhei.siamashka@gmail.com> wrote: > > The commit f630974ccb3ce93e9607a3354e9acb266a8b7e95 > > 'sunxi: Move SPL s_init() code to board_init_f()' > > broke the FEL boot mode. > > > > This patch moves the DRAM initialization back to s_init() and > > introduces an assembly entry point for FEL in order to provide > > guaranteed initialization of the gdata pointer (r9). The assembly > > entry point is also needed to ensure that the SPL code starts > > executing in ARM mode. > > > > Because the sunxi board_init_f() does not contain anything that > > is not already done by the default board_init_f(), it is removed > > too. > > > > Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com> > > --- > > arch/arm/cpu/armv7/sunxi/Makefile | 1 + > > arch/arm/cpu/armv7/sunxi/board.c | 26 ++++++++++---------------- > > arch/arm/cpu/armv7/sunxi/start_fel.S | 16 ++++++++++++++++ > > arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds | 3 ++- > > 4 files changed, 29 insertions(+), 17 deletions(-) > > create mode 100644 arch/arm/cpu/armv7/sunxi/start_fel.S > > > > diff --git a/arch/arm/cpu/armv7/sunxi/Makefile b/arch/arm/cpu/armv7/sunxi/Makefile > > index 48db744..e0d413d 100644 > > --- a/arch/arm/cpu/armv7/sunxi/Makefile > > +++ b/arch/arm/cpu/armv7/sunxi/Makefile > > @@ -40,5 +40,6 @@ obj-$(CONFIG_MACH_SUN7I) += dram_sun4i.o > > obj-$(CONFIG_MACH_SUN8I) += dram_sun8i.o > > ifdef CONFIG_SPL_FEL > > obj-y += start.o > > +extra-y += start_fel.o > > endif > > endif > > diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c > > index 6e28bcd..ea6cb60 100644 > > --- a/arch/arm/cpu/armv7/sunxi/board.c > > +++ b/arch/arm/cpu/armv7/sunxi/board.c > > @@ -85,6 +85,16 @@ void s_init(void) > > timer_init(); > > gpio_init(); > > i2c_init_board(); > > + > > +#ifdef CONFIG_SPL_BUILD > > + preloader_console_init(); > > s_init() is called before we have global_data, so you can't use a console. Oh, but somehow it just works for me. > > + > > +#ifdef CONFIG_SPL_I2C_SUPPORT > > + /* Needed early by sunxi_board_init if PMU is enabled */ > > + i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); > > +#endif > > + sunxi_board_init(); > > +#endif > > Why do you need this code here? The i2c_init() call is needed to initialize the PMIC as the comment says. The PMIC is needed to set correct voltages, necessary for the DRAM controller. And the sunxi_board_init() initializes DRAM. > Can it not go in board_init_f()? Then we probably would need a special stripped down FEL variant of board_init_f(), which makes the code a bit more messy. > > } > > > > #ifdef CONFIG_SPL_BUILD > > @@ -103,22 +113,6 @@ u32 spl_boot_mode(void) > > { > > return MMCSD_MODE_RAW; > > } > > - > > -void board_init_f(ulong dummy) > > -{ > > - preloader_console_init(); > > - > > -#ifdef CONFIG_SPL_I2C_SUPPORT > > - /* Needed early by sunxi_board_init if PMU is enabled */ > > - i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); > > -#endif > > - sunxi_board_init(); > > - > > - /* Clear the BSS. */ > > - memset(__bss_start, 0, __bss_end - __bss_start); > > - > > - board_init_r(NULL, 0); > > -} > > #endif > > > > void reset_cpu(ulong addr) > > diff --git a/arch/arm/cpu/armv7/sunxi/start_fel.S b/arch/arm/cpu/armv7/sunxi/start_fel.S > > new file mode 100644 > > index 0000000..e1c7cd4 > > --- /dev/null > > +++ b/arch/arm/cpu/armv7/sunxi/start_fel.S > > @@ -0,0 +1,16 @@ > > +/* > > + * Entry point of the FEL mode SPL. > > + * > > + * Copyright (c) 2015 Siarhei Siamashka <siarhei.siamashka@gmail.com> > > + * > > + * SPDX-License-Identifier: GPL-2.0+ > > + */ > > + > > +#include <asm-offsets.h> > > +#include <config.h> > > +#include <linux/linkage.h> > > + > > +ENTRY(_start_fel) > > + ldr r9, =gdata > > + b s_init > > No we don't want global data here, and need to get rid of gdata so we > can use driver model, etc. Appears that I need to educate myself on the global data vs. gdata differences. Using driver model in the sunxi SPL is a bit challenging because we don't have abundant amounts of SRAM there: http://linux-sunxi.org/SRAM_Controller Without relying on SoC-variant specific SRAM sections, we have 32 KiB for normal SPL, and only ~15 KiB for FEL SPL (the rest is used by the BROM FEL code). And SRAM is very much needed for the other important features. So far I can see that the pointer to gdata is stored in the r9 register. And gdata resides in the ".data" section, which means that it is initialized to 0 automatically. And this works now, unless I'm misunderstanding something. I would be more than happy to fix it in a future proof way. However it is very much desired to have a properly functioning FEL boot mode in u-boot 2015.04 and IMHO a quick hack relying on the legacy gdata feature might be not the worst possible option today. > > +ENDPROC(_start_fel) > > diff --git a/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds b/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds > > index 928b7c1..beb8900 100644 > > --- a/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds > > +++ b/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds > > @@ -6,7 +6,7 @@ > > */ > > OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm") > > OUTPUT_ARCH(arm) > > -ENTRY(s_init) > > +ENTRY(_start_fel) > > SECTIONS > > { > > . = 0x00002000; > > @@ -14,6 +14,7 @@ SECTIONS > > . = ALIGN(4); > > .text : > > { > > + arch/arm/cpu/armv7/sunxi/start_fel.o (.text) > > *(.text.s_init) > > Why does this have to jump to a special s_init? Can it not just start > SPL normally as it does on Tegra, Exynos, etc? > > > *(.text*) > > } > > There has to be a better way of making this work. Also do you have > instructions on how I can try this out on a pcduino3 or other low-cost > board? > > I understand that we need to fix this, but other archs deal with this > within the existing framework, so I'd really like to get sunxi into > the same state. For these parts, see my replies in: http://lists.denx.de/pipermail/u-boot/2015-February/203439.html http://lists.denx.de/pipermail/u-boot/2015-February/203485.html I'm afraid that we can't always fit the BROM code into the existing frameworks. But maybe some good solution exists for this particular case.
Hi Siarhei, On 1 February 2015 at 11:48, Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote: > On Sun, 1 Feb 2015 09:28:36 -0700 > Simon Glass <sjg@chromium.org> wrote: > >> Hi, >> >> On 30 January 2015 at 04:58, Siarhei Siamashka >> <siarhei.siamashka@gmail.com> wrote: >> > The commit f630974ccb3ce93e9607a3354e9acb266a8b7e95 >> > 'sunxi: Move SPL s_init() code to board_init_f()' >> > broke the FEL boot mode. >> > >> > This patch moves the DRAM initialization back to s_init() and >> > introduces an assembly entry point for FEL in order to provide >> > guaranteed initialization of the gdata pointer (r9). The assembly >> > entry point is also needed to ensure that the SPL code starts >> > executing in ARM mode. >> > >> > Because the sunxi board_init_f() does not contain anything that >> > is not already done by the default board_init_f(), it is removed >> > too. >> > >> > Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com> >> > --- >> > arch/arm/cpu/armv7/sunxi/Makefile | 1 + >> > arch/arm/cpu/armv7/sunxi/board.c | 26 ++++++++++---------------- >> > arch/arm/cpu/armv7/sunxi/start_fel.S | 16 ++++++++++++++++ >> > arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds | 3 ++- >> > 4 files changed, 29 insertions(+), 17 deletions(-) >> > create mode 100644 arch/arm/cpu/armv7/sunxi/start_fel.S >> > >> > diff --git a/arch/arm/cpu/armv7/sunxi/Makefile b/arch/arm/cpu/armv7/sunxi/Makefile >> > index 48db744..e0d413d 100644 >> > --- a/arch/arm/cpu/armv7/sunxi/Makefile >> > +++ b/arch/arm/cpu/armv7/sunxi/Makefile >> > @@ -40,5 +40,6 @@ obj-$(CONFIG_MACH_SUN7I) += dram_sun4i.o >> > obj-$(CONFIG_MACH_SUN8I) += dram_sun8i.o >> > ifdef CONFIG_SPL_FEL >> > obj-y += start.o >> > +extra-y += start_fel.o >> > endif >> > endif >> > diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c >> > index 6e28bcd..ea6cb60 100644 >> > --- a/arch/arm/cpu/armv7/sunxi/board.c >> > +++ b/arch/arm/cpu/armv7/sunxi/board.c >> > @@ -85,6 +85,16 @@ void s_init(void) >> > timer_init(); >> > gpio_init(); >> > i2c_init_board(); >> > + >> > +#ifdef CONFIG_SPL_BUILD >> > + preloader_console_init(); >> >> s_init() is called before we have global_data, so you can't use a console. > > Oh, but somehow it just works for me. I should have said that there *should* be no global_data at this point, i.e. we need to drop the hacks that add this. In fact global_data should be set up once in crt0.S and not before. > >> > + >> > +#ifdef CONFIG_SPL_I2C_SUPPORT >> > + /* Needed early by sunxi_board_init if PMU is enabled */ >> > + i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); >> > +#endif >> > + sunxi_board_init(); >> > +#endif >> >> Why do you need this code here? > > The i2c_init() call is needed to initialize the PMIC as the comment > says. The PMIC is needed to set correct voltages, necessary for > the DRAM controller. > > And the sunxi_board_init() initializes DRAM. > >> Can it not go in board_init_f()? > > Then we probably would need a special stripped down FEL variant of > board_init_f(), which makes the code a bit more messy. Yes I think it is well worth figuring out the really differences are between an SPL that boots from board storage and one that boots from FEL. For Exynos is it just a single switch() to select the boot source. For Tegra it essentially nothing. Exynos has a flag that tells SPL when it is booting from USB A-A. Does sunxi? > >> > } >> > >> > #ifdef CONFIG_SPL_BUILD >> > @@ -103,22 +113,6 @@ u32 spl_boot_mode(void) >> > { >> > return MMCSD_MODE_RAW; >> > } >> > - >> > -void board_init_f(ulong dummy) >> > -{ >> > - preloader_console_init(); >> > - >> > -#ifdef CONFIG_SPL_I2C_SUPPORT >> > - /* Needed early by sunxi_board_init if PMU is enabled */ >> > - i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); >> > -#endif >> > - sunxi_board_init(); >> > - >> > - /* Clear the BSS. */ >> > - memset(__bss_start, 0, __bss_end - __bss_start); >> > - >> > - board_init_r(NULL, 0); >> > -} >> > #endif >> > >> > void reset_cpu(ulong addr) >> > diff --git a/arch/arm/cpu/armv7/sunxi/start_fel.S b/arch/arm/cpu/armv7/sunxi/start_fel.S >> > new file mode 100644 >> > index 0000000..e1c7cd4 >> > --- /dev/null >> > +++ b/arch/arm/cpu/armv7/sunxi/start_fel.S >> > @@ -0,0 +1,16 @@ >> > +/* >> > + * Entry point of the FEL mode SPL. >> > + * >> > + * Copyright (c) 2015 Siarhei Siamashka <siarhei.siamashka@gmail.com> >> > + * >> > + * SPDX-License-Identifier: GPL-2.0+ >> > + */ >> > + >> > +#include <asm-offsets.h> >> > +#include <config.h> >> > +#include <linux/linkage.h> >> > + >> > +ENTRY(_start_fel) >> > + ldr r9, =gdata >> > + b s_init >> >> No we don't want global data here, and need to get rid of gdata so we >> can use driver model, etc. > > Appears that I need to educate myself on the global data vs. gdata > differences. > > Using driver model in the sunxi SPL is a bit challenging because > we don't have abundant amounts of SRAM there: > http://linux-sunxi.org/SRAM_Controller > Without relying on SoC-variant specific SRAM sections, we have 32 KiB > for normal SPL, and only ~15 KiB for FEL SPL (the rest is used by the > BROM FEL code). And SRAM is very much needed for the other important > features. > > So far I can see that the pointer to gdata is stored in the r9 register. > And gdata resides in the ".data" section, which means that it is > initialized to 0 automatically. And this works now, unless I'm > misunderstanding something. > > I would be more than happy to fix it in a future proof way. However it > is very much desired to have a properly functioning FEL boot mode in > u-boot 2015.04 and IMHO a quick hack relying on the legacy gdata feature > might be not the worst possible option today. > >> > +ENDPROC(_start_fel) >> > diff --git a/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds b/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds >> > index 928b7c1..beb8900 100644 >> > --- a/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds >> > +++ b/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds >> > @@ -6,7 +6,7 @@ >> > */ >> > OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm") >> > OUTPUT_ARCH(arm) >> > -ENTRY(s_init) >> > +ENTRY(_start_fel) >> > SECTIONS >> > { >> > . = 0x00002000; >> > @@ -14,6 +14,7 @@ SECTIONS >> > . = ALIGN(4); >> > .text : >> > { >> > + arch/arm/cpu/armv7/sunxi/start_fel.o (.text) >> > *(.text.s_init) >> >> Why does this have to jump to a special s_init? Can it not just start >> SPL normally as it does on Tegra, Exynos, etc? >> >> > *(.text*) >> > } >> >> There has to be a better way of making this work. Also do you have >> instructions on how I can try this out on a pcduino3 or other low-cost >> board? >> >> I understand that we need to fix this, but other archs deal with this >> within the existing framework, so I'd really like to get sunxi into >> the same state. > > For these parts, see my replies in: > http://lists.denx.de/pipermail/u-boot/2015-February/203439.html > http://lists.denx.de/pipermail/u-boot/2015-February/203485.html > > I'm afraid that we can't always fit the BROM code into the existing > frameworks. But maybe some good solution exists for this particular > case. > > -- > Best regards, > Siarhei Siamashka
On Sun, 1 Feb 2015 13:59:41 -0700 Simon Glass <sjg@chromium.org> wrote: > Hi Siarhei, > > On 1 February 2015 at 11:48, Siarhei Siamashka <siarhei.siamashka@gmail.com> > wrote: > > On Sun, 1 Feb 2015 09:28:36 -0700 > > Simon Glass <sjg@chromium.org> wrote: > > > >> Hi, > >> > >> On 30 January 2015 at 04:58, Siarhei Siamashka > >> <siarhei.siamashka@gmail.com> wrote: > >> > The commit f630974ccb3ce93e9607a3354e9acb266a8b7e95 > >> > 'sunxi: Move SPL s_init() code to board_init_f()' > >> > broke the FEL boot mode. > >> > > >> > This patch moves the DRAM initialization back to s_init() and > >> > introduces an assembly entry point for FEL in order to provide > >> > guaranteed initialization of the gdata pointer (r9). The assembly > >> > entry point is also needed to ensure that the SPL code starts > >> > executing in ARM mode. > >> > > >> > Because the sunxi board_init_f() does not contain anything that > >> > is not already done by the default board_init_f(), it is removed > >> > too. > >> > > >> > Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com> > >> > --- > >> > arch/arm/cpu/armv7/sunxi/Makefile | 1 + > >> > arch/arm/cpu/armv7/sunxi/board.c | 26 ++++++++++---------------- > >> > arch/arm/cpu/armv7/sunxi/start_fel.S | 16 ++++++++++++++++ > >> > arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds | 3 ++- > >> > 4 files changed, 29 insertions(+), 17 deletions(-) > >> > create mode 100644 arch/arm/cpu/armv7/sunxi/start_fel.S > >> > > >> > diff --git a/arch/arm/cpu/armv7/sunxi/Makefile > b/arch/arm/cpu/armv7/sunxi/Makefile > >> > index 48db744..e0d413d 100644 > >> > --- a/arch/arm/cpu/armv7/sunxi/Makefile > >> > +++ b/arch/arm/cpu/armv7/sunxi/Makefile > >> > @@ -40,5 +40,6 @@ obj-$(CONFIG_MACH_SUN7I) += dram_sun4i.o > >> > obj-$(CONFIG_MACH_SUN8I) += dram_sun8i.o > >> > ifdef CONFIG_SPL_FEL > >> > obj-y += start.o > >> > +extra-y += start_fel.o > >> > endif > >> > endif > >> > diff --git a/arch/arm/cpu/armv7/sunxi/board.c > b/arch/arm/cpu/armv7/sunxi/board.c > >> > index 6e28bcd..ea6cb60 100644 > >> > --- a/arch/arm/cpu/armv7/sunxi/board.c > >> > +++ b/arch/arm/cpu/armv7/sunxi/board.c > >> > @@ -85,6 +85,16 @@ void s_init(void) > >> > timer_init(); > >> > gpio_init(); > >> > i2c_init_board(); > >> > + > >> > +#ifdef CONFIG_SPL_BUILD > >> > + preloader_console_init(); > >> > >> s_init() is called before we have global_data, so you can't use a > console. > > > > Oh, but somehow it just works for me. > > I should have said that there *should* be no global_data at this point, > i.e. we need to drop the hacks that add this. In fact global_data should be > set up once in crt0.S and not before. OK, I understand. However crt0.S does not seem to be particularly compatible with FEL. It tries to override the stack pointer (in the FEL mode we need to use the original stack pointer provided to us by the BROM). And implies that 'board_init_f' needs to return control back to the BROM, however doing return from multiple nested levels of function calls is a tricky exercise, especially considering that the stack has been already moved elsewhere. On the other hand, I see that crt0.S just allocates global data on stack, clears it and then sets all the same r9 register. So what is the real difference compared to having global data defined in the ".data" section? I would say that right now an easy hack would be to remove gdata globally in u-boot (that's an admirable goal), but keep it with a different name under the CONFIG_SPL_FEL define just for sunxi. We might just rework this patch by providing the following FEL entry point: push {r9, lr} ldr r9, =sunxi_fel_gdata bl s_init bl board_init_f pop {r9, pc} Then hide the unneeded parts of board_init_f() under !defined(CONFIG_SPL_FEL) check, so that it just returns right after initializing DRAM. I see that the rationale for gdata removal is to allow having an early malloc pool for the driver model in SPL: http://lists.denx.de/pipermail/u-boot/2014-December/199528.html I think that you can keep experimenting with the driver model on sunxi with the regular SPL build, but just leave the FEL mode build alone for now. It is not like u-boot is going to ever switch to the driver model in the SPL on every platform (there are platforms where the SRAM size is extremely small, even smaller than on sunxi), so the sunxi FEL mode is not going to be alone doing this. > >> > + > >> > +#ifdef CONFIG_SPL_I2C_SUPPORT > >> > + /* Needed early by sunxi_board_init if PMU is enabled */ > >> > + i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); > >> > +#endif > >> > + sunxi_board_init(); > >> > +#endif > >> > >> Why do you need this code here? > > > > The i2c_init() call is needed to initialize the PMIC as the comment > > says. The PMIC is needed to set correct voltages, necessary for > > the DRAM controller. > > > > And the sunxi_board_init() initializes DRAM. > > > >> Can it not go in board_init_f()? > > > > Then we probably would need a special stripped down FEL variant of > > board_init_f(), which makes the code a bit more messy. > > Yes I think it is well worth figuring out the really differences are > between an SPL that boots from board storage and one that boots from FEL. > For Exynos is it just a single switch() to select the boot source. For > Tegra it essentially nothing. > > Exynos has a flag that tells SPL when it is booting from USB A-A. Does > sunxi? Currently sunxi has the CONFIG_SPL_FEL define. Even though I'm generally in favour of runtime detection, we can't do it in this case. This is only because the amount of available SRAM space is much smaller in the FEL mode and the normal SPL simply does not fit. If we could afford it, then surely having a single SPL binary (both bootable in the USB FEL mode and from the SD card) would be much preferable without separate '*_felconfig' and '*_defconfig' configs.
Hi Siarhei, On 1 February 2015 at 16:59, Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote: > > On Sun, 1 Feb 2015 13:59:41 -0700 > Simon Glass <sjg@chromium.org> wrote: > > > Hi Siarhei, > > > > On 1 February 2015 at 11:48, Siarhei Siamashka <siarhei.siamashka@gmail.com> > > wrote: > > > On Sun, 1 Feb 2015 09:28:36 -0700 > > > Simon Glass <sjg@chromium.org> wrote: > > > > > >> Hi, > > >> > > >> On 30 January 2015 at 04:58, Siarhei Siamashka > > >> <siarhei.siamashka@gmail.com> wrote: > > >> > The commit f630974ccb3ce93e9607a3354e9acb266a8b7e95 > > >> > 'sunxi: Move SPL s_init() code to board_init_f()' > > >> > broke the FEL boot mode. > > >> > > > >> > This patch moves the DRAM initialization back to s_init() and > > >> > introduces an assembly entry point for FEL in order to provide > > >> > guaranteed initialization of the gdata pointer (r9). The assembly > > >> > entry point is also needed to ensure that the SPL code starts > > >> > executing in ARM mode. > > >> > > > >> > Because the sunxi board_init_f() does not contain anything that > > >> > is not already done by the default board_init_f(), it is removed > > >> > too. > > >> > > > >> > Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com> > > >> > --- > > >> > arch/arm/cpu/armv7/sunxi/Makefile | 1 + > > >> > arch/arm/cpu/armv7/sunxi/board.c | 26 ++++++++++---------------- > > >> > arch/arm/cpu/armv7/sunxi/start_fel.S | 16 ++++++++++++++++ > > >> > arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds | 3 ++- > > >> > 4 files changed, 29 insertions(+), 17 deletions(-) > > >> > create mode 100644 arch/arm/cpu/armv7/sunxi/start_fel.S > > >> > > > >> > diff --git a/arch/arm/cpu/armv7/sunxi/Makefile > > b/arch/arm/cpu/armv7/sunxi/Makefile > > >> > index 48db744..e0d413d 100644 > > >> > --- a/arch/arm/cpu/armv7/sunxi/Makefile > > >> > +++ b/arch/arm/cpu/armv7/sunxi/Makefile > > >> > @@ -40,5 +40,6 @@ obj-$(CONFIG_MACH_SUN7I) += dram_sun4i.o > > >> > obj-$(CONFIG_MACH_SUN8I) += dram_sun8i.o > > >> > ifdef CONFIG_SPL_FEL > > >> > obj-y += start.o > > >> > +extra-y += start_fel.o > > >> > endif > > >> > endif > > >> > diff --git a/arch/arm/cpu/armv7/sunxi/board.c > > b/arch/arm/cpu/armv7/sunxi/board.c > > >> > index 6e28bcd..ea6cb60 100644 > > >> > --- a/arch/arm/cpu/armv7/sunxi/board.c > > >> > +++ b/arch/arm/cpu/armv7/sunxi/board.c > > >> > @@ -85,6 +85,16 @@ void s_init(void) > > >> > timer_init(); > > >> > gpio_init(); > > >> > i2c_init_board(); > > >> > + > > >> > +#ifdef CONFIG_SPL_BUILD > > >> > + preloader_console_init(); > > >> > > >> s_init() is called before we have global_data, so you can't use a > > console. > > > > > > Oh, but somehow it just works for me. > > > > I should have said that there *should* be no global_data at this point, > > i.e. we need to drop the hacks that add this. In fact global_data should be > > set up once in crt0.S and not before. > > OK, I understand. > > However crt0.S does not seem to be particularly compatible with FEL. > It tries to override the stack pointer (in the FEL mode we need to > use the original stack pointer provided to us by the BROM). And > implies that 'board_init_f' needs to return control back to the > BROM, however doing return from multiple nested levels of function > calls is a tricky exercise, especially considering that the stack > has been already moved elsewhere. How about we save sp and lr in other registers before they get changed. Then sunxi lowlevel_init can stash them in SRAM, or if we are clever, board_init_f() can do it. Then we can just restore the original stack and jump to lr when SPL is finished. I see no problem with using our own stack in SPL. In fact I'd prefer it. > > On the other hand, I see that crt0.S just allocates global data on > stack, clears it and then sets all the same r9 register. So what > is the real difference compared to having global data defined in > the ".data" section? > > I would say that right now an easy hack would be to remove gdata > globally in u-boot (that's an admirable goal), but keep it with a > different name under the CONFIG_SPL_FEL define just for sunxi. > We might just rework this patch by providing the following FEL > entry point: > > push {r9, lr} > ldr r9, =sunxi_fel_gdata > bl s_init > bl board_init_f > pop {r9, pc} No, let's just declare a locate data section variable for sunxi. It does not need to go in global_data. That just confuses things. > > Then hide the unneeded parts of board_init_f() under > !defined(CONFIG_SPL_FEL) check, so that it just returns > right after initializing DRAM. Can we not return in board_init_r() like SPL normally does? Even in FEL mode we might want to do something else. > > I see that the rationale for gdata removal is to allow having an > early malloc pool for the driver model in SPL: > http://lists.denx.de/pipermail/u-boot/2014-December/199528.html > > I think that you can keep experimenting with the driver model > on sunxi with the regular SPL build, but just leave the FEL mode > build alone for now. It is not like u-boot is going to ever switch > to the driver model in the SPL on every platform (there are platforms > where the SRAM size is extremely small, even smaller than on sunxi), > so the sunxi FEL mode is not going to be alone doing this. I believe that even FEL has enough SRAM for driver model, but I agree there will be cases where it is impossible (e.g. Atmel's 4KB seems really hard). > > > >> > + > > >> > +#ifdef CONFIG_SPL_I2C_SUPPORT > > >> > + /* Needed early by sunxi_board_init if PMU is enabled */ > > >> > + i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); > > >> > +#endif > > >> > + sunxi_board_init(); > > >> > +#endif > > >> > > >> Why do you need this code here? > > > > > > The i2c_init() call is needed to initialize the PMIC as the comment > > > says. The PMIC is needed to set correct voltages, necessary for > > > the DRAM controller. > > > > > > And the sunxi_board_init() initializes DRAM. > > > > > >> Can it not go in board_init_f()? > > > > > > Then we probably would need a special stripped down FEL variant of > > > board_init_f(), which makes the code a bit more messy. > > > > Yes I think it is well worth figuring out the really differences are > > between an SPL that boots from board storage and one that boots from FEL. > > For Exynos is it just a single switch() to select the boot source. For > > Tegra it essentially nothing. > > > > Exynos has a flag that tells SPL when it is booting from USB A-A. Does > > sunxi? > > Currently sunxi has the CONFIG_SPL_FEL define. > > Even though I'm generally in favour of runtime detection, we can't do > it in this case. This is only because the amount of available SRAM > space is much smaller in the FEL mode and the normal SPL simply does > not fit. If we could afford it, then surely having a single SPL > binary (both bootable in the USB FEL mode and from the SD card) would > be much preferable without separate '*_felconfig' and '*_defconfig' > configs. Oh dear that is awful! Perhaps the build should create two SPLs, one for FEL and one for normal? That avoids all the fiddling, although presumably FEL mode is mostly for U-Boot development? Why does FEL mode need so much SRAM? The model is wrong. It should not be calling into SPL - maybe someone should take this up with Allwinner for future chips. Regards, Simon
diff --git a/arch/arm/cpu/armv7/sunxi/Makefile b/arch/arm/cpu/armv7/sunxi/Makefile index 48db744..e0d413d 100644 --- a/arch/arm/cpu/armv7/sunxi/Makefile +++ b/arch/arm/cpu/armv7/sunxi/Makefile @@ -40,5 +40,6 @@ obj-$(CONFIG_MACH_SUN7I) += dram_sun4i.o obj-$(CONFIG_MACH_SUN8I) += dram_sun8i.o ifdef CONFIG_SPL_FEL obj-y += start.o +extra-y += start_fel.o endif endif diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c index 6e28bcd..ea6cb60 100644 --- a/arch/arm/cpu/armv7/sunxi/board.c +++ b/arch/arm/cpu/armv7/sunxi/board.c @@ -85,6 +85,16 @@ void s_init(void) timer_init(); gpio_init(); i2c_init_board(); + +#ifdef CONFIG_SPL_BUILD + preloader_console_init(); + +#ifdef CONFIG_SPL_I2C_SUPPORT + /* Needed early by sunxi_board_init if PMU is enabled */ + i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); +#endif + sunxi_board_init(); +#endif } #ifdef CONFIG_SPL_BUILD @@ -103,22 +113,6 @@ u32 spl_boot_mode(void) { return MMCSD_MODE_RAW; } - -void board_init_f(ulong dummy) -{ - preloader_console_init(); - -#ifdef CONFIG_SPL_I2C_SUPPORT - /* Needed early by sunxi_board_init if PMU is enabled */ - i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); -#endif - sunxi_board_init(); - - /* Clear the BSS. */ - memset(__bss_start, 0, __bss_end - __bss_start); - - board_init_r(NULL, 0); -} #endif void reset_cpu(ulong addr) diff --git a/arch/arm/cpu/armv7/sunxi/start_fel.S b/arch/arm/cpu/armv7/sunxi/start_fel.S new file mode 100644 index 0000000..e1c7cd4 --- /dev/null +++ b/arch/arm/cpu/armv7/sunxi/start_fel.S @@ -0,0 +1,16 @@ +/* + * Entry point of the FEL mode SPL. + * + * Copyright (c) 2015 Siarhei Siamashka <siarhei.siamashka@gmail.com> + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <asm-offsets.h> +#include <config.h> +#include <linux/linkage.h> + +ENTRY(_start_fel) + ldr r9, =gdata + b s_init +ENDPROC(_start_fel) diff --git a/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds b/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds index 928b7c1..beb8900 100644 --- a/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds +++ b/arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds @@ -6,7 +6,7 @@ */ OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm") OUTPUT_ARCH(arm) -ENTRY(s_init) +ENTRY(_start_fel) SECTIONS { . = 0x00002000; @@ -14,6 +14,7 @@ SECTIONS . = ALIGN(4); .text : { + arch/arm/cpu/armv7/sunxi/start_fel.o (.text) *(.text.s_init) *(.text*) }
The commit f630974ccb3ce93e9607a3354e9acb266a8b7e95 'sunxi: Move SPL s_init() code to board_init_f()' broke the FEL boot mode. This patch moves the DRAM initialization back to s_init() and introduces an assembly entry point for FEL in order to provide guaranteed initialization of the gdata pointer (r9). The assembly entry point is also needed to ensure that the SPL code starts executing in ARM mode. Because the sunxi board_init_f() does not contain anything that is not already done by the default board_init_f(), it is removed too. Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com> --- arch/arm/cpu/armv7/sunxi/Makefile | 1 + arch/arm/cpu/armv7/sunxi/board.c | 26 ++++++++++---------------- arch/arm/cpu/armv7/sunxi/start_fel.S | 16 ++++++++++++++++ arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds | 3 ++- 4 files changed, 29 insertions(+), 17 deletions(-) create mode 100644 arch/arm/cpu/armv7/sunxi/start_fel.S