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

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

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

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

[Jean-Christophe PLAGNIOL-VILLARD]

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.

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

[Matthias Fuchs]

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.

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

[Stefan Roese]

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

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

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

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

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

[Jean-Christophe PLAGNIOL-VILLARD]

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.

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

[Matthias Fuchs]

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 at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/u-boot-users



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

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

[Jean-Christophe PLAGNIOL-VILLARD]

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.

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

[Wolfgang Denk]

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

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

[Wolfgang Denk]

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

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

[Matthias Fuchs]

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

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

[Wolfgang Denk]

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