cfi-flash: Add CFG_FLASH_AUTOPROTECT_LIST

PATCH New Matthias Fuchs 4 participants 12 messages Apr 18, 2008 – Apr 20, 2008 Memory technology devices (MTD)

Share & tools

Matthias Fuchs

author

2008-04-18 14:29 UTC #0 lore raw

PATCH 1/1 cfi-flash: Add CFG_FLASH_AUTOPROTECT_LIST

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 <[email protected]>

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

drivers/mtd/cfi_flash.c +17 -0

index e3cfb8a..68ab55f 100644
@@ -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);
 }

Signature · 1 line

1.5.3

Jean-Christophe PLAGNIOL-VILLARD

2008-04-18 16:01 UTC #1 lore raw

PATCH 1/1 cfi-flash: Add CFG_FLASH_AUTOPROTECT_LIST

Quoted 30 lines · Matthias Fuchs

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 <[email protected]>
---
 drivers/mtd/cfi_flash.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)
drivers/mtd/cfi_flash.c +0 -0
index e3cfb8a..68ab55f 100644

drivers/mtd/cfi_flash.c unsigned long flash_init (void) · line 1881

Quoted 9 lines

drivers/mtd/cfi_flash.c +6 -0

@@ -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

author

2008-04-19 11:30 UTC #2 lore raw

PATCH 1/1 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

Quoted 19 lines · On Friday 18 April 2008 18:01:57 Jean-Christophe PLAGNIOL-VILLARD wrote:

Quoted 5 lines
drivers/mtd/cfi_flash.c +0 -0
index e3cfb8a..68ab55f 100644
drivers/mtd/cfi_flash.c unsigned long flash_init (void) · line 1881
Quoted 9 lines
drivers/mtd/cfi_flash.c +6 -0
@@ -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

2008-04-19 12:02 UTC #3 lore raw

PATCH 1/1 cfi-flash: Add CFG_FLASH_AUTOPROTECT_LIST

Hi Matthias,

Quoted 4 lines · Matthias Fuchs

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?

Quoted 24 lines

Matthias
Quoted 17 lines · On Friday 18 April 2008 18:01:57 Jean-Christophe PLAGNIOL-VILLARD wrote:
Quoted 5 lines
drivers/mtd/cfi_flash.c +0 -0
index e3cfb8a..68ab55f 100644
drivers/mtd/cfi_flash.c unsigned long flash_init (void) · line 1881
Quoted 9 lines
drivers/mtd/cfi_flash.c +6 -0
@@ -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.
Quoted 2 lines
Best Regards,
J.
!DSPAM:4809d80874783629025813!

Signature · 8 lines

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 at denx.de
=====================================================================

Matthias Fuchs

author

2008-04-19 14:50 UTC #4 lore raw

PATCH 1/1 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

Quoted 60 lines · On Saturday 19 April 2008 14:02:23 Stefan Roese wrote:

Hi Matthias,
Quoted 4 lines · Matthias Fuchs
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:
drivers/mtd/cfi_flash.c +8 -0
+       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?
Quoted 24 lines
Matthias
Quoted 17 lines · On Friday 18 April 2008 18:01:57 Jean-Christophe PLAGNIOL-VILLARD wrote:
Quoted 5 lines
drivers/mtd/cfi_flash.c +0 -0
index e3cfb8a..68ab55f 100644
drivers/mtd/cfi_flash.c unsigned long flash_init (void) · line 1881
Quoted 9 lines
drivers/mtd/cfi_flash.c +6 -0
@@ -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.
Quoted 2 lines
Best Regards,
J.
!DSPAM:4809d80874783629025813!

Signature · 13 lines

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

2008-04-19 15:33 UTC #5 lore raw

PATCH 1/1 cfi-flash: Add?CFG_FLASH_AUTOPROTECT_LIST

Quoted 6 lines · Matthias Fuchs

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

author

2008-04-19 17:22 UTC #6 lore raw

PATCH 1/1 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

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

Quoted 6 lines · Matthias Fuchs
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 at lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/u-boot-users

Signature · 13 lines

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

2008-04-20 03:54 UTC #7 lore raw

PATCH 1/1 cfi-flash:?Add?CFG_FLASH_AUTOPROTECT_LIST

Quoted 10 lines · Matthias Fuchs

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

drivers/mtd/cfi_flash.c +3 -0

@@ -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

2008-04-20 23:44 UTC #8 lore raw

PATCH 1/1 cfi-flash:?Add?CFG_FLASH_AUTOPROTECT_LIST

Quoted 2 lines · In message <[email protected]> you wrote:

As sugest Wolfgang we could do it like this

I did not suggest to use weak at all here!

Quoted 9 lines

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

Signature · 4 lines

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 at denx.de
Remember that Beethoven wrote his first symphony in C ...

Wolfgang Denk

2008-04-20 01:14 UTC #9 lore raw

PATCH 1/1 cfi-flash: Add CFG_FLASH_AUTOPROTECT_LIST

In reply to Jean-Christophe PLAGNIOL-VILLARD

Quoted 8 lines · In message <[email protected]> you wrote:

drivers/mtd/cfi_flash.c unsigned long flash_init (void) · line 1881
Quoted 7 lines
drivers/mtd/cfi_flash.c +6 -0
+#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

Signature · 5 lines

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 at 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

author

2008-04-20 13:22 UTC #10 lore raw

PATCH 1/1 cfi-flash: Add CFG_FLASH_AUTOPROTECT_LIST

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

Quoted 16 lines · On Sunday 20 April 2008 03:14:10 Wolfgang Denk wrote:

Quoted 8 lines · In message <[email protected]> you wrote:
drivers/mtd/cfi_flash.c unsigned long flash_init (void) · line 1881
Quoted 6 lines
drivers/mtd/cfi_flash.c +6 -0
+#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

2008-04-20 21:52 UTC #11 lore raw

PATCH 1/1 cfi-flash: Add CFG_FLASH_AUTOPROTECT_LIST

Dear Matthias,

Quoted 5 lines · in message <[email protected]> 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.

Quoted 13 lines

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,
(quoted diff) +2 -0
+#else
                       CFG_MONITOR_BASE + monitor_flash_len  - 1,
+#endif
                       flash_get_info(CFG_MONITOR_BASE));

Quoted 1 line

#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.

Quoted 1 line

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

I see.

Best regards,

Wolfgang Denk

Signature · 5 lines

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 at denx.de
Microsoft Compatibility:
     your old Windows 3.11 application crash exactly as the new ones.