[U-Boot-Users] [PATCH] cfi-flash: Add CFG_FLASH_AUTOPROTECT_LIST

Matthias Fuchs

18 Apr 2008 18 Apr '08

4:29 p.m.

This patch adds a configurable flash auto protection list that can be used to make U-Boot protect flash regions in flash_init().

The idea has been discussed on the u-boot mailing list starting on Nov 18th, 2007.

Even this patch brings a new feature it is used as a bugfix for 4xx platforms where flash_init() does not completely protect the monitor's flash range in all situations.

U-Boot protects the flash range from CFG_MONITOR_BASE to (CFG_MONITOR_BASE + monitor_flash_len - 1) by default. This does not include the reset vector at 0xfffffffc.

Example: #define CFG_FLASH_AUTOPROTECT_LIST {{0xfff80000, 0x80000}}

This config option will auto protect the last 512k of flash that contains the bootloader on board like APC405 and PMC405.

Signed-off-by: Matthias Fuchs matthias.fuchs@esd-electronics.com --- drivers/mtd/cfi_flash.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index e3cfb8a..68ab55f 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -1873,6 +1873,12 @@ unsigned long flash_init (void) { unsigned long size = 0; int i; +#if defined(CFG_FLASH_AUTOPROTECT_LIST) + struct apl_s { + ulong start; + ulong size; + } apl[] = CFG_FLASH_AUTOPROTECT_LIST; +#endif

#ifdef CFG_FLASH_PROTECTION char *s = getenv("unlock"); @@ -1966,6 +1972,17 @@ unsigned long flash_init (void) CFG_ENV_ADDR_REDUND + CFG_ENV_SIZE_REDUND - 1, flash_get_info(CFG_ENV_ADDR_REDUND)); #endif + +#if defined(CFG_FLASH_AUTOPROTECT_LIST) + for (i = 0; i < (sizeof(apl) / sizeof(struct apl_s)); i++) { + debug("autoprotecting from %08x to %08x\n", + apl[i].start, apl[i].start + apl[i].size - 1); + flash_protect (FLAG_PROTECT_SET, + apl[i].start, + apl[i].start + apl[i].size - 1, + flash_get_info(apl[i].start)); + } +#endif return (size); }

-- 1.5.3

Show replies by date

Jean-Christophe PLAGNIOL-VILLARD

18 Apr 18 Apr

6:01 p.m.

New subject: [U-Boot-Users] [PATCH] cfi-flash: Add CFG_FLASH_AUTOPROTECT_LIST

On 16:29 Fri 18 Apr , Matthias Fuchs wrote:

...

This patch adds a configurable flash auto protection list that can be used to make U-Boot protect flash regions in flash_init().

The idea has been discussed on the u-boot mailing list starting on Nov 18th, 2007.

Even this patch brings a new feature it is used as a bugfix for 4xx platforms where flash_init() does not completely protect the monitor's flash range in all situations.

U-Boot protects the flash range from CFG_MONITOR_BASE to (CFG_MONITOR_BASE + monitor_flash_len - 1) by default. This does not include the reset vector at 0xfffffffc.

Example: #define CFG_FLASH_AUTOPROTECT_LIST {{0xfff80000, 0x80000}}

This config option will auto protect the last 512k of flash that contains the bootloader on board like APC405 and PMC405.

Signed-off-by: Matthias Fuchs matthias.fuchs@esd-electronics.com

drivers/mtd/cfi_flash.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index e3cfb8a..68ab55f 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -1873,6 +1873,12 @@ unsigned long flash_init (void) { unsigned long size = 0; int i; +#if defined(CFG_FLASH_AUTOPROTECT_LIST)
struct apl_s {
ulong start;
ulong size;
} apl[] = CFG_FLASH_AUTOPROTECT_LIST;
+#endif

I think it will be better to create a weak structure.

Best Regards, J.

Matthias Fuchs

19 Apr 19 Apr

1:30 p.m.

New subject: [U-Boot-Users] [PATCH] cfi-flash: Add CFG_FLASH_AUTOPROTECT_LIST

Hi Jean-Christophe,

is it possible to use weak structures? Or do you mean a weak function that initializes my autoprotect list?

Please give me an idea and I will implement it.

Matthias

On Friday 18 April 2008 18:01:57 Jean-Christophe PLAGNIOL-VILLARD wrote:

...

...
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index e3cfb8a..68ab55f 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -1873,6 +1873,12 @@ unsigned long flash_init (void) { unsigned long size = 0; int i; +#if defined(CFG_FLASH_AUTOPROTECT_LIST)
struct apl_s {
ulong start;
ulong size;
} apl[] = CFG_FLASH_AUTOPROTECT_LIST;
+#endif
I think it will be better to create a weak structure.

Best Regards, J.

Stefan Roese

2:02 p.m.

New subject: [U-Boot-Users] [PATCH] cfi-flash: Add CFG_FLASH_AUTOPROTECT_LIST

Hi Matthias,

On Saturday 19 April 2008, Matthias Fuchs wrote:

...

is it possible to use weak structures? Or do you mean a weak function that initializes my autoprotect list?

Please give me an idea and I will implement it.

I don't know if it's possible to implement a weak structure, but I would prefer something like this instead of your original implementation (no #ifdef's in the code):

struct apl_s { u32 start; u32 size; };

#if !defined(CFG_FLASH_AUTOPROTECT_LIST) struct apl_s apl[] = { }; #else struct apl_s apl[] = CFG_FLASH_AUTOPROTECT_LIST; #endif

And then later in the code:

+ for (i = 0; i < ARRAY_SIZE(apl); i++) { + debug("autoprotecting from %08x to %08x\n", + apl[i].start, apl[i].start + apl[i].size - 1); + flash_protect (FLAG_PROTECT_SET, + apl[i].start, + apl[i].start + apl[i].size - 1, + flash_get_info(apl[i].start)); + }

What do you think?

...

Matthias

On Friday 18 April 2008 18:01:57 Jean-Christophe PLAGNIOL-VILLARD wrote:

...
...
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index e3cfb8a..68ab55f 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -1873,6 +1873,12 @@ unsigned long flash_init (void) { unsigned long size = 0; int i; +#if defined(CFG_FLASH_AUTOPROTECT_LIST)
struct apl_s {
ulong start;
ulong size;
} apl[] = CFG_FLASH_AUTOPROTECT_LIST;
+#endif
I think it will be better to create a weak structure.

Best Regards, J.
!DSPAM:4809d80874783629025813!

-- Viele Grüße, Stefan ===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================

Matthias Fuchs

4:50 p.m.

New subject: [U-Boot-Users] [PATCH] cfi-flash: Add CFG_FLASH_AUTOPROTECT_LIST

Hi Stefan,

I could also life with your approach, but it will add code even to platforms that do not use the new option.

In this case I would prefer it against the weak implementation.

Matthias

On Saturday 19 April 2008 14:02:23 Stefan Roese wrote:

...

Hi Matthias,

On Saturday 19 April 2008, Matthias Fuchs wrote:

...
is it possible to use weak structures? Or do you mean a weak function that initializes my autoprotect list?

Please give me an idea and I will implement it.

I don't know if it's possible to implement a weak structure, but I would prefer something like this instead of your original implementation (no #ifdef's in the code):

struct apl_s { u32 start; u32 size; };

#if !defined(CFG_FLASH_AUTOPROTECT_LIST) struct apl_s apl[] = { }; #else struct apl_s apl[] = CFG_FLASH_AUTOPROTECT_LIST; #endif

And then later in the code:
  for (i = 0; i < ARRAY_SIZE(apl); i++) {
          debug("autoprotecting from %08x to %08x\n",
                apl[i].start, apl[i].start + apl[i].size - 1);
          flash_protect (FLAG_PROTECT_SET,
                         apl[i].start,
                         apl[i].start + apl[i].size - 1,
                         flash_get_info(apl[i].start));
  }
What do you think?

...
Matthias

On Friday 18 April 2008 18:01:57 Jean-Christophe PLAGNIOL-VILLARD wrote:

...
...
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index e3cfb8a..68ab55f 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -1873,6 +1873,12 @@ unsigned long flash_init (void) { unsigned long size = 0; int i; +#if defined(CFG_FLASH_AUTOPROTECT_LIST)
struct apl_s {
ulong start;
ulong size;
} apl[] = CFG_FLASH_AUTOPROTECT_LIST;
+#endif
I think it will be better to create a weak structure.

Best Regards, J.
!DSPAM:4809d80874783629025813!

-- ------------------------------------------------------------------------- Dipl.-Ing. Matthias Fuchs Head of System Design esd electronic system design gmbh Vahrenwalder Str. 207 - 30165 Hannover - GERMANY Phone: +49-511-37298-0 - Fax: +49-511-37298-68 Please visit our homepage http://www.esd.eu Quality Products - Made in Germany ------------------------------------------------------------------------- Geschäftsführer: Klaus Detering, Dr. Werner Schulze Amtsgericht Hannover HRB 51373 - VAT-ID DE 115672832 -------------------------------------------------------------------------

Jean-Christophe PLAGNIOL-VILLARD

5:33 p.m.

New subject: [U-Boot-Users] [PATCH] cfi-flash: Add?CFG_FLASH_AUTOPROTECT_LIST

On 16:50 Sat 19 Apr , Matthias Fuchs wrote:

...

Hi Stefan,

I could also life with your approach, but it will add code even to platforms that do not use the new option.

In this case I would prefer it against the weak implementation.

I've already answer about it to Timur in this e-mail http://article.gmane.org/gmane.comp.boot-loaders.u-boot/37814/match=weak

I'm preparing a path about adding this define

import compiler-gcc header from linux and add

#ifndef __weak_alias #ifndef __weak_alias(fct) __attribute__((weak,alias(#fct))) #endif

so just add __weak to your default stucture and overwrite in the board

Best Regards, J.

Matthias Fuchs

7:22 p.m.

New subject: [U-Boot-Users] [PATCH] cfi-flash: Add?CFG_FLASH_AUTOPROTECT_LIST

Hi,

I tried this in cfi_flash.c:

struct apl_s { ulong start; ulong size; }

struct apl_s apl[] __attribute__((weak)) = {};

I added a redeclaration of apl into my board code.

Now I ran into the problem that the ARRAY_SIZE macro on apl in cfi_flash.c does not take care of the redeclation. So it evaluated to 0 instead of the real size of the array from my board code.

Now I could

a) add a delimiter to the apl array (e.g. size=0 for the last entry) b) implement Stefan approach c) ???

Any idea?

Matthias

On Saturday 19 April 2008 17:33:43 Jean-Christophe PLAGNIOL-VILLARD wrote:

...

On 16:50 Sat 19 Apr , Matthias Fuchs wrote:

...
Hi Stefan,

I could also life with your approach, but it will add code even to platforms that do not use the new option.

In this case I would prefer it against the weak implementation.

I've already answer about it to Timur in this e-mail http://article.gmane.org/gmane.comp.boot-loaders.u-boot/37814/match=weak

I'm preparing a path about adding this define

import compiler-gcc header from linux and add

#ifndef __weak_alias #ifndef __weak_alias(fct) __attribute__((weak,alias(#fct))) #endif

so just add __weak to your default stucture and overwrite in the board

Best Regards, J.

This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2. http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/java one _______________________________________________ U-Boot-Users mailing list U-Boot-Users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users

Jean-Christophe PLAGNIOL-VILLARD

20 Apr 20 Apr

5:54 a.m.

New subject: [U-Boot-Users] [PATCH] cfi-flash:?Add?CFG_FLASH_AUTOPROTECT_LIST

On 19:22 Sat 19 Apr , Matthias Fuchs wrote:

...

Hi,

I tried this in cfi_flash.c:

struct apl_s { ulong start; ulong size; }

struct apl_s apl[] __attribute__((weak)) = {};

The problem with weak var is that the first declaration define the sizeof the var. and the other define the data;

As sugest Wolfgang we could do it like this

in cfi_flash.c

--- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -1873,6 +1873,12 @@ unsigned long flash_init (void) { unsigned long size = 0; int i; +if define(CONFIG_FLASH_AUTOPROTECT) +struct apl_s apl[] = init_flash_autoprotect(); +#endif

#ifdef CFG_FLASH_PROTECTION char *s = getenv("unlock"); @@ -1966,6 +1972,17 @@ unsigned long flash_init (void) CFG_ENV_ADDR_REDUND + CFG_ENV_SIZE_REDUND - 1, flash_get_info(CFG_ENV_ADDR_REDUND)); #endif + +#if defined(CONFIG_FLASH_AUTOPROTECT) + for (i = 0; i < (sizeof(apl) / sizeof(struct apl_s)); i++) { + debug("autoprotecting from %08x to %08x\n", + apl[i].start, apl[i].start + apl[i].size - 1); + flash_protect (FLAG_PROTECT_SET, + apl[i].start, + apl[i].start + apl[i].size - 1, + flash_get_info(apl[i].start)); + } +#endif

in config header

struct apl_s apl* inline init_flash_autoprotect() { struct apl_s a[] = { {1, 3}, {4, 5}, };

return a; } Best Regards, J.

Wolfgang Denk

21 Apr 21 Apr

1:44 a.m.

New subject: [U-Boot-Users] [PATCH] cfi-flash:?Add?CFG_FLASH_AUTOPROTECT_LIST

In message 20080420035435.GB27176@game.jcrosoft.org you wrote:

...

As sugest Wolfgang we could do it like this

I did not suggest to use weak at all here!

...

struct apl_s apl* inline init_flash_autoprotect() { struct apl_s a[] = { {1, 3}, {4, 5}, };

return a; }

Are you sure this works?

Best regards,

Wolfgang Denk

-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Remember that Beethoven wrote his first symphony in C ...

Wolfgang Denk

20 Apr 20 Apr

3:14 a.m.

In message 20080418160157.GB12486@game.jcrosoft.org you wrote:

...

...
+#if defined(CFG_FLASH_AUTOPROTECT_LIST)
struct apl_s {
ulong start;
ulong size;
} apl[] = CFG_FLASH_AUTOPROTECT_LIST;
+#endif
I think it will be better to create a weak structure.

In the interest of memory footprint I prefer to stick with the #ifdef here.

Best regards,

Wolfgang Denk

-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Motto of the Electrical Engineer: Working computer hardware is a lot like an erect penis: it stays up as long as you don't fuck with it.

Matthias Fuchs

3:22 p.m.

Hi Wolfgang,

even though I like the weak stuff, I agree with you. I also like Stefan's idea about removing the #ifdef from the code.

So I will update my patch in this direction.

The main idea behind my patch is to fix the bootloader autoprotection for 4xx. Do you (sr, J., others) think i is also a good idea to add this:

/* Monitor protection ON by default */ #if (CFG_MONITOR_BASE >= CFG_FLASH_BASE) flash_protect (FLAG_PROTECT_SET, CFG_MONITOR_BASE, +#if defined(CONFIG_4xx) 0xffffffff, +#else CFG_MONITOR_BASE + monitor_flash_len - 1, +#endif flash_get_info(CFG_MONITOR_BASE)); #endif

To be honest, I hope to get my autoprotection patch into 1.3.3 (as a bug fix). But the above three lines would be satisfactory for fixing the issue. Finally I would like to have both changes applied.

Matthias

On Sunday 20 April 2008 03:14:10 Wolfgang Denk wrote:

...

In message 20080418160157.GB12486@game.jcrosoft.org you wrote:

...
...
+#if defined(CFG_FLASH_AUTOPROTECT_LIST)
struct apl_s {
ulong start;
ulong size;
} apl[] = CFG_FLASH_AUTOPROTECT_LIST;
+#endif
I think it will be better to create a weak structure.
In the interest of memory footprint I prefer to stick with the #ifdef here.

Best regards,

Wolfgang Denk

Wolfgang Denk

11:52 p.m.

Dear Matthias,

in message 200804201522.40808.matthias.fuchs@esd-electronics.com you wrote:

...

even though I like the weak stuff, I agree with you. I also like Stefan's idea about removing the #ifdef from the code.

So I will update my patch in this direction.

Thanks.

...

The main idea behind my patch is to fix the bootloader autoprotection for 4xx. Do you (sr, J., others) think i is also a good idea to add this:
    /* Monitor protection ON by default */
#if (CFG_MONITOR_BASE >= CFG_FLASH_BASE) flash_protect (FLAG_PROTECT_SET, CFG_MONITOR_BASE, +#if defined(CONFIG_4xx) 0xffffffff, +#else CFG_MONITOR_BASE + monitor_flash_len - 1, +#endif flash_get_info(CFG_MONITOR_BASE)); #endif

No, I don't think this is a good idea.

First, I don't see what is so special about 4xx here. There are other processors which have the reset vector at the end of the address space as well, so why do it for 4xx and not - for example - for 85xx?

Also, what makes you think it is reasonable to protect everything from CFG_MONITOR_BASE to the end of memory? Yes, we usually pot U-Boot close to the end of the flash memory, but there is no guarantee that every board configuration works that way. We could put U-Boot at the beginning of the flash instead and just reserve a single sector at the end of the flash for the reset vector. OK, that would need adaption of the linker script as well, but ...

I don't think this hardwired, absolute constant makes sense to me. If 0xffffffff is really the value to use, then it should be identical to the "CFG_MONITOR_BASE + monitor_flash_len - 1" expression, and then we should have no need for the #ifdef at all.

...

To be honest, I hope to get my autoprotection patch into 1.3.3 (as a bug fix).

I see.

Best regards,

Wolfgang Denk

-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Microsoft Compatibility: your old Windows 3.11 application crash exactly as the new ones.

6227

Age (days ago)

6229

Last active (days ago)

List overview

Download

11 comments

4 participants

tags (0)

participants (4)

Jean-Christophe PLAGNIOL-VILLARD
Matthias Fuchs
Stefan Roese
Wolfgang Denk