| Message ID | 20181010042630.6402-1-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [U-Boot,v4] arm: socfpga: fix SPL booting from fpga OnChip RAM | expand |
On 10/10/2018 06:26 AM, Simon Goldschmidt wrote: > This patch prevents disabling the FPGA bridges when > SPL or U-Boot is executed from FPGA onchip RAM. > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > --- > > Changes in v4: > - use an inline function in misc.h to check for the address > range instead of a macro in base_addr_ac5.h > > Changes in v3: > - use __image_copy_start to check if we are executing from FPGA > > Changes in v2: > - use less ifdefs and more C code for address checks > (but this gives a checkpatch warning because of comparing two > upper case constants) > - changed comments > > arch/arm/mach-socfpga/include/mach/base_addr_ac5.h | 1 + > arch/arm/mach-socfpga/include/mach/misc.h | 7 +++++++ > arch/arm/mach-socfpga/misc_gen5.c | 10 +++++++++- > arch/arm/mach-socfpga/spl_gen5.c | 10 +++++++--- > 4 files changed, 24 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h b/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h > index bb9e3faa29..2725e9fcc3 100644 > --- a/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h > +++ b/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h > @@ -6,6 +6,7 @@ > #ifndef _SOCFPGA_BASE_ADDRS_H_ > #define _SOCFPGA_BASE_ADDRS_H_ > > +#define SOCFPGA_FPGA_SLAVES_ADDRESS 0xc0000000 > #define SOCFPGA_STM_ADDRESS 0xfc000000 > #define SOCFPGA_DAP_ADDRESS 0xff000000 > #define SOCFPGA_EMAC0_ADDRESS 0xff700000 > diff --git a/arch/arm/mach-socfpga/include/mach/misc.h b/arch/arm/mach-socfpga/include/mach/misc.h > index 4fc9570a04..e78a86503e 100644 > --- a/arch/arm/mach-socfpga/include/mach/misc.h > +++ b/arch/arm/mach-socfpga/include/mach/misc.h > @@ -23,6 +23,13 @@ static inline void socfpga_fpga_add(void) {} > > #ifdef CONFIG_TARGET_SOCFPGA_GEN5 > void socfpga_sdram_remap_zero(void); > +static inline bool socfpga_is_fpga_slaves_addr(void *addr) > +{ Is the inline needed ? btw would it make sense to encode __image_copy_start here and just make it a function like ie. static bool socfpga_is_booting_from_fpga(void) {} ? [...]
On Wed, Oct 10, 2018 at 12:21 PM Marek Vasut <marex@denx.de> wrote: > > On 10/10/2018 06:26 AM, Simon Goldschmidt wrote: > > This patch prevents disabling the FPGA bridges when > > SPL or U-Boot is executed from FPGA onchip RAM. > > > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > > --- > > > > Changes in v4: > > - use an inline function in misc.h to check for the address > > range instead of a macro in base_addr_ac5.h > > > > Changes in v3: > > - use __image_copy_start to check if we are executing from FPGA > > > > Changes in v2: > > - use less ifdefs and more C code for address checks > > (but this gives a checkpatch warning because of comparing two > > upper case constants) > > - changed comments > > > > arch/arm/mach-socfpga/include/mach/base_addr_ac5.h | 1 + > > arch/arm/mach-socfpga/include/mach/misc.h | 7 +++++++ > > arch/arm/mach-socfpga/misc_gen5.c | 10 +++++++++- > > arch/arm/mach-socfpga/spl_gen5.c | 10 +++++++--- > > 4 files changed, 24 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h b/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h > > index bb9e3faa29..2725e9fcc3 100644 > > --- a/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h > > +++ b/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h > > @@ -6,6 +6,7 @@ > > #ifndef _SOCFPGA_BASE_ADDRS_H_ > > #define _SOCFPGA_BASE_ADDRS_H_ > > > > +#define SOCFPGA_FPGA_SLAVES_ADDRESS 0xc0000000 > > #define SOCFPGA_STM_ADDRESS 0xfc000000 > > #define SOCFPGA_DAP_ADDRESS 0xff000000 > > #define SOCFPGA_EMAC0_ADDRESS 0xff700000 > > diff --git a/arch/arm/mach-socfpga/include/mach/misc.h b/arch/arm/mach-socfpga/include/mach/misc.h > > index 4fc9570a04..e78a86503e 100644 > > --- a/arch/arm/mach-socfpga/include/mach/misc.h > > +++ b/arch/arm/mach-socfpga/include/mach/misc.h > > @@ -23,6 +23,13 @@ static inline void socfpga_fpga_add(void) {} > > > > #ifdef CONFIG_TARGET_SOCFPGA_GEN5 > > void socfpga_sdram_remap_zero(void); > > +static inline bool socfpga_is_fpga_slaves_addr(void *addr) > > +{ > > Is the inline needed ? If it's in this header, yes. As without the inline, you get this compiler warnings in files that include misc.h but don't call the function: warning: ‘socfpga_is_fpga_slaves_addr’ defined but not used [-Wunused-function] > btw would it make sense to encode __image_copy_start here and just make > it a function like ie. > > static bool socfpga_is_booting_from_fpga(void) {} ? Sure. Do you want me to move it into misc_gen5.c r keep the inline? Simon
On 10/10/2018 12:32 PM, Simon Goldschmidt wrote: > On Wed, Oct 10, 2018 at 12:21 PM Marek Vasut <marex@denx.de> wrote: >> >> On 10/10/2018 06:26 AM, Simon Goldschmidt wrote: >>> This patch prevents disabling the FPGA bridges when >>> SPL or U-Boot is executed from FPGA onchip RAM. >>> >>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>> --- >>> >>> Changes in v4: >>> - use an inline function in misc.h to check for the address >>> range instead of a macro in base_addr_ac5.h >>> >>> Changes in v3: >>> - use __image_copy_start to check if we are executing from FPGA >>> >>> Changes in v2: >>> - use less ifdefs and more C code for address checks >>> (but this gives a checkpatch warning because of comparing two >>> upper case constants) >>> - changed comments >>> >>> arch/arm/mach-socfpga/include/mach/base_addr_ac5.h | 1 + >>> arch/arm/mach-socfpga/include/mach/misc.h | 7 +++++++ >>> arch/arm/mach-socfpga/misc_gen5.c | 10 +++++++++- >>> arch/arm/mach-socfpga/spl_gen5.c | 10 +++++++--- >>> 4 files changed, 24 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h b/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h >>> index bb9e3faa29..2725e9fcc3 100644 >>> --- a/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h >>> +++ b/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h >>> @@ -6,6 +6,7 @@ >>> #ifndef _SOCFPGA_BASE_ADDRS_H_ >>> #define _SOCFPGA_BASE_ADDRS_H_ >>> >>> +#define SOCFPGA_FPGA_SLAVES_ADDRESS 0xc0000000 >>> #define SOCFPGA_STM_ADDRESS 0xfc000000 >>> #define SOCFPGA_DAP_ADDRESS 0xff000000 >>> #define SOCFPGA_EMAC0_ADDRESS 0xff700000 >>> diff --git a/arch/arm/mach-socfpga/include/mach/misc.h b/arch/arm/mach-socfpga/include/mach/misc.h >>> index 4fc9570a04..e78a86503e 100644 >>> --- a/arch/arm/mach-socfpga/include/mach/misc.h >>> +++ b/arch/arm/mach-socfpga/include/mach/misc.h >>> @@ -23,6 +23,13 @@ static inline void socfpga_fpga_add(void) {} >>> >>> #ifdef CONFIG_TARGET_SOCFPGA_GEN5 >>> void socfpga_sdram_remap_zero(void); >>> +static inline bool socfpga_is_fpga_slaves_addr(void *addr) >>> +{ >> >> Is the inline needed ? > > If it's in this header, yes. As without the inline, you get this compiler > warnings in files that include misc.h but don't call the function: > warning: ‘socfpga_is_fpga_slaves_addr’ defined but not used [-Wunused-function] Ah, got it. >> btw would it make sense to encode __image_copy_start here and just make >> it a function like ie. >> >> static bool socfpga_is_booting_from_fpga(void) {} ? > > Sure. Do you want me to move it into misc_gen5.c r keep the inline? Nope, just this. Sorry for pestering you so much.
On 10/10/2018 12:49 PM, Marek Vasut wrote: > On 10/10/2018 12:32 PM, Simon Goldschmidt wrote: >> On Wed, Oct 10, 2018 at 12:21 PM Marek Vasut <marex@denx.de> wrote: >>> >>> On 10/10/2018 06:26 AM, Simon Goldschmidt wrote: >>>> This patch prevents disabling the FPGA bridges when >>>> SPL or U-Boot is executed from FPGA onchip RAM. >>>> >>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>> --- >>>> >>>> Changes in v4: >>>> - use an inline function in misc.h to check for the address >>>> range instead of a macro in base_addr_ac5.h >>>> >>>> Changes in v3: >>>> - use __image_copy_start to check if we are executing from FPGA >>>> >>>> Changes in v2: >>>> - use less ifdefs and more C code for address checks >>>> (but this gives a checkpatch warning because of comparing two >>>> upper case constants) >>>> - changed comments >>>> >>>> arch/arm/mach-socfpga/include/mach/base_addr_ac5.h | 1 + >>>> arch/arm/mach-socfpga/include/mach/misc.h | 7 +++++++ >>>> arch/arm/mach-socfpga/misc_gen5.c | 10 +++++++++- >>>> arch/arm/mach-socfpga/spl_gen5.c | 10 +++++++--- >>>> 4 files changed, 24 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h b/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h >>>> index bb9e3faa29..2725e9fcc3 100644 >>>> --- a/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h >>>> +++ b/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h >>>> @@ -6,6 +6,7 @@ >>>> #ifndef _SOCFPGA_BASE_ADDRS_H_ >>>> #define _SOCFPGA_BASE_ADDRS_H_ >>>> >>>> +#define SOCFPGA_FPGA_SLAVES_ADDRESS 0xc0000000 >>>> #define SOCFPGA_STM_ADDRESS 0xfc000000 >>>> #define SOCFPGA_DAP_ADDRESS 0xff000000 >>>> #define SOCFPGA_EMAC0_ADDRESS 0xff700000 >>>> diff --git a/arch/arm/mach-socfpga/include/mach/misc.h b/arch/arm/mach-socfpga/include/mach/misc.h >>>> index 4fc9570a04..e78a86503e 100644 >>>> --- a/arch/arm/mach-socfpga/include/mach/misc.h >>>> +++ b/arch/arm/mach-socfpga/include/mach/misc.h >>>> @@ -23,6 +23,13 @@ static inline void socfpga_fpga_add(void) {} >>>> >>>> #ifdef CONFIG_TARGET_SOCFPGA_GEN5 >>>> void socfpga_sdram_remap_zero(void); >>>> +static inline bool socfpga_is_fpga_slaves_addr(void *addr) >>>> +{ >>> >>> Is the inline needed ? >> >> If it's in this header, yes. As without the inline, you get this compiler >> warnings in files that include misc.h but don't call the function: >> warning: ‘socfpga_is_fpga_slaves_addr’ defined but not used [-Wunused-function] > > Ah, got it. > >>> btw would it make sense to encode __image_copy_start here and just make >>> it a function like ie. >>> >>> static bool socfpga_is_booting_from_fpga(void) {} ? >> >> Sure. Do you want me to move it into misc_gen5.c r keep the inline? > > Nope, just this. Sorry for pestering you so much. And by just this I mean, just encode the __image_start_addr into the function and drop the argument, keep it where it is.
On Wed, Oct 10, 2018 at 12:50 PM Marek Vasut <marex@denx.de> wrote: > > On 10/10/2018 12:49 PM, Marek Vasut wrote: > > On 10/10/2018 12:32 PM, Simon Goldschmidt wrote: > >> On Wed, Oct 10, 2018 at 12:21 PM Marek Vasut <marex@denx.de> wrote: > >>> > >>> On 10/10/2018 06:26 AM, Simon Goldschmidt wrote: > >>>> This patch prevents disabling the FPGA bridges when > >>>> SPL or U-Boot is executed from FPGA onchip RAM. > >>>> > >>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > >>>> --- > >>>> > >>>> Changes in v4: > >>>> - use an inline function in misc.h to check for the address > >>>> range instead of a macro in base_addr_ac5.h > >>>> > >>>> Changes in v3: > >>>> - use __image_copy_start to check if we are executing from FPGA > >>>> > >>>> Changes in v2: > >>>> - use less ifdefs and more C code for address checks > >>>> (but this gives a checkpatch warning because of comparing two > >>>> upper case constants) > >>>> - changed comments > >>>> > >>>> arch/arm/mach-socfpga/include/mach/base_addr_ac5.h | 1 + > >>>> arch/arm/mach-socfpga/include/mach/misc.h | 7 +++++++ > >>>> arch/arm/mach-socfpga/misc_gen5.c | 10 +++++++++- > >>>> arch/arm/mach-socfpga/spl_gen5.c | 10 +++++++--- > >>>> 4 files changed, 24 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h b/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h > >>>> index bb9e3faa29..2725e9fcc3 100644 > >>>> --- a/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h > >>>> +++ b/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h > >>>> @@ -6,6 +6,7 @@ > >>>> #ifndef _SOCFPGA_BASE_ADDRS_H_ > >>>> #define _SOCFPGA_BASE_ADDRS_H_ > >>>> > >>>> +#define SOCFPGA_FPGA_SLAVES_ADDRESS 0xc0000000 > >>>> #define SOCFPGA_STM_ADDRESS 0xfc000000 > >>>> #define SOCFPGA_DAP_ADDRESS 0xff000000 > >>>> #define SOCFPGA_EMAC0_ADDRESS 0xff700000 > >>>> diff --git a/arch/arm/mach-socfpga/include/mach/misc.h b/arch/arm/mach-socfpga/include/mach/misc.h > >>>> index 4fc9570a04..e78a86503e 100644 > >>>> --- a/arch/arm/mach-socfpga/include/mach/misc.h > >>>> +++ b/arch/arm/mach-socfpga/include/mach/misc.h > >>>> @@ -23,6 +23,13 @@ static inline void socfpga_fpga_add(void) {} > >>>> > >>>> #ifdef CONFIG_TARGET_SOCFPGA_GEN5 > >>>> void socfpga_sdram_remap_zero(void); > >>>> +static inline bool socfpga_is_fpga_slaves_addr(void *addr) > >>>> +{ > >>> > >>> Is the inline needed ? > >> > >> If it's in this header, yes. As without the inline, you get this compiler > >> warnings in files that include misc.h but don't call the function: > >> warning: ‘socfpga_is_fpga_slaves_addr’ defined but not used [-Wunused-function] > > > > Ah, got it. > > > >>> btw would it make sense to encode __image_copy_start here and just make > >>> it a function like ie. > >>> > >>> static bool socfpga_is_booting_from_fpga(void) {} ? > >> > >> Sure. Do you want me to move it into misc_gen5.c r keep the inline? > > > > Nope, just this. Sorry for pestering you so much. No problem. Thanks to patman, sending new versions is not really much work :-) > > And by just this I mean, just encode the __image_start_addr into the > function and drop the argument, keep it where it is. Yeah, got it. Simon
diff --git a/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h b/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h index bb9e3faa29..2725e9fcc3 100644 --- a/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h +++ b/arch/arm/mach-socfpga/include/mach/base_addr_ac5.h @@ -6,6 +6,7 @@ #ifndef _SOCFPGA_BASE_ADDRS_H_ #define _SOCFPGA_BASE_ADDRS_H_ +#define SOCFPGA_FPGA_SLAVES_ADDRESS 0xc0000000 #define SOCFPGA_STM_ADDRESS 0xfc000000 #define SOCFPGA_DAP_ADDRESS 0xff000000 #define SOCFPGA_EMAC0_ADDRESS 0xff700000 diff --git a/arch/arm/mach-socfpga/include/mach/misc.h b/arch/arm/mach-socfpga/include/mach/misc.h index 4fc9570a04..e78a86503e 100644 --- a/arch/arm/mach-socfpga/include/mach/misc.h +++ b/arch/arm/mach-socfpga/include/mach/misc.h @@ -23,6 +23,13 @@ static inline void socfpga_fpga_add(void) {} #ifdef CONFIG_TARGET_SOCFPGA_GEN5 void socfpga_sdram_remap_zero(void); +static inline bool socfpga_is_fpga_slaves_addr(void *addr) +{ + if ((addr >= (void *)SOCFPGA_FPGA_SLAVES_ADDRESS) && + (addr < (void *)SOCFPGA_STM_ADDRESS)) + return true; + return false; +} #endif #ifdef CONFIG_TARGET_SOCFPGA_ARRIA10 diff --git a/arch/arm/mach-socfpga/misc_gen5.c b/arch/arm/mach-socfpga/misc_gen5.c index 429c3d6cd5..030f43d80a 100644 --- a/arch/arm/mach-socfpga/misc_gen5.c +++ b/arch/arm/mach-socfpga/misc_gen5.c @@ -20,6 +20,7 @@ #include <asm/arch/nic301.h> #include <asm/arch/scu.h> #include <asm/pl310.h> +#include <asm/sections.h> #include <dt-bindings/reset/altr,rst-mgr.h> @@ -177,6 +178,8 @@ static void socfpga_nic301_slave_ns(void) void socfpga_sdram_remap_zero(void) { + u32 remap; + socfpga_nic301_slave_ns(); /* @@ -187,7 +190,12 @@ void socfpga_sdram_remap_zero(void) setbits_le32(&scu_regs->sacr, 0xfff); /* Configure the L2 controller to make SDRAM start at 0 */ - writel(0x1, &nic301_regs->remap); /* remap.mpuzero */ + remap = 0x1; /* remap.mpuzero */ + /* Keep fpga bridge enabled when running from FPGA onchip RAM */ + if (socfpga_is_fpga_slaves_addr(__image_copy_start)) + remap |= 0x8; /* remap.hps2fpga */ + writel(remap, &nic301_regs->remap); + writel(0x1, &pl310->pl310_addr_filter_start); } diff --git a/arch/arm/mach-socfpga/spl_gen5.c b/arch/arm/mach-socfpga/spl_gen5.c index be318cc0d9..db1c4b722c 100644 --- a/arch/arm/mach-socfpga/spl_gen5.c +++ b/arch/arm/mach-socfpga/spl_gen5.c @@ -92,8 +92,11 @@ void board_init_f(ulong dummy) /* Put everything into reset but L4WD0. */ socfpga_per_reset_all(); - /* Put FPGA bridges into reset too. */ - socfpga_bridges_reset(1); + + if (!socfpga_is_fpga_slaves_addr(__image_copy_start)) { + /* Put FPGA bridges into reset (unless booting from FPGA). */ + socfpga_bridges_reset(1); + } socfpga_per_reset(SOCFPGA_RESET(SDR), 0); socfpga_per_reset(SOCFPGA_RESET(UART0), 0); @@ -163,5 +166,6 @@ void board_init_f(ulong dummy) hang(); } - socfpga_bridges_reset(1); + if (!socfpga_is_fpga_slaves_addr(__image_copy_start)) + socfpga_bridges_reset(1); }
This patch prevents disabling the FPGA bridges when SPL or U-Boot is executed from FPGA onchip RAM. Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> --- Changes in v4: - use an inline function in misc.h to check for the address range instead of a macro in base_addr_ac5.h Changes in v3: - use __image_copy_start to check if we are executing from FPGA Changes in v2: - use less ifdefs and more C code for address checks (but this gives a checkpatch warning because of comparing two upper case constants) - changed comments arch/arm/mach-socfpga/include/mach/base_addr_ac5.h | 1 + arch/arm/mach-socfpga/include/mach/misc.h | 7 +++++++ arch/arm/mach-socfpga/misc_gen5.c | 10 +++++++++- arch/arm/mach-socfpga/spl_gen5.c | 10 +++++++--- 4 files changed, 24 insertions(+), 4 deletions(-)