[U-Boot] Reg. CFI flash_init and hardware write protected devices

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Frank Svendsbøe]

We have a board that feature NOR flash and hardware write protection
is handled by controlling the write enable pin. When write protection
is enabled, the nWE pin is forced high by external logic. The memory
controller and/or CFI logic is unaware of this, and since CFI uses
write enable as part of the CFI command set, a CFI probing will fail
when write protection is enabled.

The rationale for controlling nWE and not WP (write protection) is
that WP only protects the first sector of the device. In our case this
is less than the size of the bootloader (U-boot), since that occupies
two sectors. Due to this the built-in NOR write protection is rather
useless.

Our current solution based on controlling nWE is to hardcode flash
geometry in board code when flash protection is enabled. In order to
use CFI as intended when write protection is disabled, we call the
generic flash_init function as defined in
drivers/mtd/cfi_flash.c. When protection is enabled we hardcode the
geometry info in board code. In order separate our flash_init and the
generic flash_init, and be able to call both, we've introduced a new
ifdef to cfi_flash.c called CONFIG_CFI_FLASH_OVERRIDE_INIT.  Like
this:

----

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 6039e1f..772096e 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -176,6 +176,10 @@ u64 flash_read64(void *addr)__attribute__((weak,
alias("__flash_read64")));
 #define flash_read64   __flash_read64
 #endif

+#ifdef CONFIG_CFI_FLASH_OVERRIDE_INIT
+#define flash_init __flash_init
+#endif
+
 /*-----------------------------------------------------------------------
  */
 #if defined(CONFIG_ENV_IS_IN_FLASH) ||
defined(CONFIG_ENV_ADDR_REDUND) || (CONFIG_SYS_MONITOR_BASE >=

----

Now, in board code our redefined flash_init will be called. But if
write protection is off, we call the original function,
eg. __flash_init.

Using the preprocessor is often considered bad design. However, the
alternatives here such as adding a weak attribute to flash_init will
not make us able to call the generic/original function.  Therefore we
consider adding an ifdef as better design than making the function
weak, and it'll reduce the amount of redundant code in board code.

What do you think about this?

Best regards,
Frank E. Svendsb?e

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Mike Frysinger]

On Tuesday, May 31, 2011 04:35:17 Frank Svendsb?e wrote:
> Now, in board code our redefined flash_init will be called. But if
> write protection is off, we call the original function,
> eg. __flash_init.

if your code is simply setting the pin high at init time and then never 
bringing it back down, then just do that in your board_early_init_f func.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110531/581bb348/attachment.pgp 

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Frank Svendsbøe]

Hi Mike,

On Tue, May 31, 2011 at 2:49 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tuesday, May 31, 2011 04:35:17 Frank Svendsb?e wrote:
>> Now, in board code our redefined flash_init will be called. But if
>> write protection is off, we call the original function,
>> eg. __flash_init.
>
> if your code is simply setting the pin high at init time and then never
> bringing it back down, then just do that in your board_early_init_f func.
> -mike
>

No this is not possible, and I'm sorry for not explaining it better.

The pin is not controlled by me. It is implicitly set/cleared via the memory
controller, but it doesn't matter because we have external logic
forcing it high.

Having the possibility to control this pin in software wouldn't make it
"hardware write protection" would it?

The protection state is kept by an external AVR and in order to enable write
access both a jumper and a write protection flag must be set.

Best regards,
Frank

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Mike Frysinger]

On Tuesday, May 31, 2011 09:25:54 Frank Svendsb?e wrote:
> Having the possibility to control this pin in software wouldn't make it
> "hardware write protection" would it?

yes, it would.  "software write protection" is what u-boot offers today -- the 
software checks the addresses before allowing writes.  hardware write 
protection is where u-boot puts out the write command anyways the request is 
rejected by the hardware.  just because you have a knob to toggle the hardware 
write protection doesnt mean it is no longer protected in hardware.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110531/72c79c2c/attachment.pgp 

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Stefan Roese]

Hi Frank,

On Tuesday 31 May 2011 10:35:17 Frank Svendsb?e wrote:
> We have a board that feature NOR flash and hardware write protection
> is handled by controlling the write enable pin. When write protection
> is enabled, the nWE pin is forced high by external logic. The memory
> controller and/or CFI logic is unaware of this, and since CFI uses
> write enable as part of the CFI command set, a CFI probing will fail
> when write protection is enabled.
> 
> The rationale for controlling nWE and not WP (write protection) is
> that WP only protects the first sector of the device. In our case this
> is less than the size of the bootloader (U-boot), since that occupies
> two sectors. Due to this the built-in NOR write protection is rather
> useless.

Understood. But why don't you disable write-protection when you first call 
flash_init()? And enable the write-protection after the chip is correctly 
detected?
 
> Our current solution based on controlling nWE is to hardcode flash
> geometry in board code when flash protection is enabled. In order to
> use CFI as intended when write protection is disabled, we call the
> generic flash_init function as defined in
> drivers/mtd/cfi_flash.c.

How is write-protection enabled/disabled on your board?

> When protection is enabled we hardcode the
> geometry info in board code. In order separate our flash_init and the
> generic flash_init, and be able to call both, we've introduced a new
> ifdef to cfi_flash.c called CONFIG_CFI_FLASH_OVERRIDE_INIT.  Like
> this:
> 
> ----
> 
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index 6039e1f..772096e 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -176,6 +176,10 @@ u64 flash_read64(void *addr)__attribute__((weak,
> alias("__flash_read64")));
>  #define flash_read64   __flash_read64
>  #endif
> 
> +#ifdef CONFIG_CFI_FLASH_OVERRIDE_INIT
> +#define flash_init __flash_init
> +#endif
> +
>  /*-----------------------------------------------------------------------
>   */
>  #if defined(CONFIG_ENV_IS_IN_FLASH) ||
> defined(CONFIG_ENV_ADDR_REDUND) || (CONFIG_SYS_MONITOR_BASE >=
> 
> ----
> 
> Now, in board code our redefined flash_init will be called. But if
> write protection is off, we call the original function,
> eg. __flash_init.
> 
> Using the preprocessor is often considered bad design. However, the
> alternatives here such as adding a weak attribute to flash_init will
> not make us able to call the generic/original function.  Therefore we
> consider adding an ifdef as better design than making the function
> weak, and it'll reduce the amount of redundant code in board code.

Why don't you think that you can't access the original function if it's 
defined as a weak default? This should work just fine, see for example 
ft_board_setup() in arch/powerpc/cpu/ppc4xx/fdt.c:

void __ft_board_setup(void *blob, bd_t *bd)
{
	...
}
void ft_board_setup(void *blob, bd_t *bd) __attribute__((weak, 
alias("__ft_board_setup")));


And then this weak default is overridden and still referenced in 
board/amcc/canyonlands/canyonlands.c:

void ft_board_setup(void *blob, bd_t *bd)
{
        __ft_board_setup(blob, bd);
	...


So no need for this ifdef in the common CFI driver. Or am I missing something 
here?

Best regards,
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] Reg. CFI flash_init and hardware write protected devices

[Frank Svendsbøe]

Hi Stefan,

On Tue, May 31, 2011 at 3:10 PM, Stefan Roese <sr@denx.de> wrote:
> Hi Frank,
>
> On Tuesday 31 May 2011 10:35:17 Frank Svendsb?e wrote:
>> We have a board that feature NOR flash and hardware write protection
>> is handled by controlling the write enable pin. When write protection
>> is enabled, the nWE pin is forced high by external logic. The memory
>> controller and/or CFI logic is unaware of this, and since CFI uses
>> write enable as part of the CFI command set, a CFI probing will fail
>> when write protection is enabled.
>>
>> The rationale for controlling nWE and not WP (write protection) is
>> that WP only protects the first sector of the device. In our case this
>> is less than the size of the bootloader (U-boot), since that occupies
>> two sectors. Due to this the built-in NOR write protection is rather
>> useless.
>
> Understood. But why don't you disable write-protection when you first call
> flash_init()? And enable the write-protection after the chip is correctly
> detected?
>

Simply because disabling write-protection is not impossible after installation.
Our device will be located 3000m below sea level.

As I explained Mike Frysinger, the write-protection settings is not controlled
by the PPC device running U-Boot. We can enable write-protection in the
lab (by setting a jumper), but not write software.

The whole purpose of this is to keep it "impossible" to destroy a factory
default version. For "mutable" software, we utilize another flash.

>> Our current solution based on controlling nWE is to hardcode flash
>> geometry in board code when flash protection is enabled. In order to
>> use CFI as intended when write protection is disabled, we call the
>> generic flash_init function as defined in
>> drivers/mtd/cfi_flash.c.
>
> How is write-protection enabled/disabled on your board?
>

Two ways/levels: 1) A hardware jumper on the factory default flash. 2)
On the non-factory default flash, write protection is enabled/disabled
by an FPGA and implicitly and AVR. To make it short, we cannot
change protection scheme from U-Boot (but we can via an SPI driver I
wrote for Linux).

>> When protection is enabled we hardcode the
>> geometry info in board code. In order separate our flash_init and the
>> generic flash_init, and be able to call both, we've introduced a new
>> ifdef to cfi_flash.c called CONFIG_CFI_FLASH_OVERRIDE_INIT. ?Like
>> this:
>>
>> ----
>>
>> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
>> index 6039e1f..772096e 100644
>> --- a/drivers/mtd/cfi_flash.c
>> +++ b/drivers/mtd/cfi_flash.c
>> @@ -176,6 +176,10 @@ u64 flash_read64(void *addr)__attribute__((weak,
>> alias("__flash_read64")));
>> ?#define flash_read64 ? __flash_read64
>> ?#endif
>>
>> +#ifdef CONFIG_CFI_FLASH_OVERRIDE_INIT
>> +#define flash_init __flash_init
>> +#endif
>> +
>> ?/*-----------------------------------------------------------------------
>> ? */
>> ?#if defined(CONFIG_ENV_IS_IN_FLASH) ||
>> defined(CONFIG_ENV_ADDR_REDUND) || (CONFIG_SYS_MONITOR_BASE >=
>>
>> ----
>>
>> Now, in board code our redefined flash_init will be called. But if
>> write protection is off, we call the original function,
>> eg. __flash_init.
>>
>> Using the preprocessor is often considered bad design. However, the
>> alternatives here such as adding a weak attribute to flash_init will
>> not make us able to call the generic/original function. ?Therefore we
>> consider adding an ifdef as better design than making the function
>> weak, and it'll reduce the amount of redundant code in board code.
>
> Why don't you think that you can't access the original function if it's
> defined as a weak default? This should work just fine, see for example
> ft_board_setup() in arch/powerpc/cpu/ppc4xx/fdt.c:
>
> void __ft_board_setup(void *blob, bd_t *bd)
> {
> ? ? ? ?...
> }
> void ft_board_setup(void *blob, bd_t *bd) __attribute__((weak,
> alias("__ft_board_setup")));
>
>
> And then this weak default is overridden and still referenced in
> board/amcc/canyonlands/canyonlands.c:
>
> void ft_board_setup(void *blob, bd_t *bd)
> {
> ? ? ? ?__ft_board_setup(blob, bd);
> ? ? ? ?...
>
>
> So no need for this ifdef in the common CFI driver. Or am I missing something
> here?
>

Oh. I didn't knew I could access the function that was overridden by the
weak attribute. I guess that's the alias is for right? If both can be called,
I'm happy to remove the ifdef.

I'll test that tomorrow and provide a patch if it works.

Best regards,
Frank

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Stefan Roese]

Hi Frank,

On Tuesday 31 May 2011 15:55:56 Frank Svendsb?e wrote:
> > Understood. But why don't you disable write-protection when you first
> > call flash_init()? And enable the write-protection after the chip is
> > correctly detected?
> 
> Simply because disabling write-protection is not impossible after
> installation. Our device will be located 3000m below sea level.

I see.
 
> As I explained Mike Frysinger, the write-protection settings is not
> controlled by the PPC device running U-Boot. We can enable
> write-protection in the lab (by setting a jumper), but not write software.
> 
> The whole purpose of this is to keep it "impossible" to destroy a factory
> default version. For "mutable" software, we utilize another flash.
> 
> >> Our current solution based on controlling nWE is to hardcode flash
> >> geometry in board code when flash protection is enabled. In order to
> >> use CFI as intended when write protection is disabled, we call the
> >> generic flash_init function as defined in
> >> drivers/mtd/cfi_flash.c.
> > 
> > How is write-protection enabled/disabled on your board?
> 
> Two ways/levels: 1) A hardware jumper on the factory default flash. 2)
> On the non-factory default flash, write protection is enabled/disabled
> by an FPGA and implicitly and AVR. To make it short, we cannot
> change protection scheme from U-Boot (but we can via an SPI driver I
> wrote for Linux).

Theoretically also possible with U-Boot. But I understand that you don't want 
to do this.

<snip>
 
> > Why don't you think that you can't access the original function if it's
> > defined as a weak default? This should work just fine, see for example
> > ft_board_setup() in arch/powerpc/cpu/ppc4xx/fdt.c:
> > 
> > void __ft_board_setup(void *blob, bd_t *bd)
> > {
> >        ...
> > }
> > void ft_board_setup(void *blob, bd_t *bd) __attribute__((weak,
> > alias("__ft_board_setup")));
> > 
> > 
> > And then this weak default is overridden and still referenced in
> > board/amcc/canyonlands/canyonlands.c:
> > 
> > void ft_board_setup(void *blob, bd_t *bd)
> > {
> >        __ft_board_setup(blob, bd);
> >        ...
> > 
> > 
> > So no need for this ifdef in the common CFI driver. Or am I missing
> > something here?
> 
> Oh. I didn't knew I could access the function that was overridden by the
> weak attribute. I guess that's the alias is for right?

Yep.

> If both can be
> called, I'm happy to remove the ifdef.
> 
> I'll test that tomorrow and provide a patch if it works.

Good luck...

Best regards,
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] Reg. CFI flash_init and hardware write protected devices

[Frank Svendsbøe]

Hi Stefan,

On Tue, May 31, 2011 at 4:37 PM, Stefan Roese <sr@denx.de> wrote:
> Hi Frank,
>
> On Tuesday 31 May 2011 15:55:56 Frank Svendsb?e wrote:
>> > Understood. But why don't you disable write-protection when you first
>> > call flash_init()? And enable the write-protection after the chip is
>> > correctly detected?
>>
>> Simply because disabling write-protection is not impossible after
>> installation. Our device will be located 3000m below sea level.
>
> I see.
>

Hmm.. then you read my mind :) I meant to say "is not possible" and not "is not
impossible" :)

>> As I explained Mike Frysinger, the write-protection settings is not
>> controlled by the PPC device running U-Boot. We can enable
>> write-protection in the lab (by setting a jumper), but not write software.
>>

.. and here I meant to say "explained to", and "but not via software".


>> Two ways/levels: 1) A hardware jumper on the factory default flash. 2)
>> On the non-factory default flash, write protection is enabled/disabled
>> by an FPGA and implicitly and AVR. To make it short, we cannot
>> change protection scheme from U-Boot (but we can via an SPI driver I
>> wrote for Linux).
>

.. and yet another self-correction: s/and implicitly and/and implicitly an/
.. guess I had too much coffee yesterday :)

> Theoretically also possible with U-Boot. But I understand that you don't want
> to do this.
>

True. We could write a driver for U-Boot that could access the AVR and
command the FPGA to
disable write protection. But in the case for the "default factory
flash" where the flash
write protection is enabled by a demounted jumper, and cannot be
modified after installation,
CFI probing wouldn't work anyway. So I think the use for such a driver
would be limited.

Note that for normal operation, our system is running from a
non-hardware protected flash. But
even in this mode, we must command the AVR to enable write access
before we're permitted
to write to it. This is one of the features supported by the SPI
driver running in GNU/Linux.

In addition to this, we have the usual CFI/MTD protection scheme where
the filesystems
are mounted as read-only, etc.

> <snip>
>
>> > Why don't you think that you can't access the original function if it's
>> > defined as a weak default? This should work just fine, see for example
>> > ft_board_setup() in arch/powerpc/cpu/ppc4xx/fdt.c:
>> >
>> > void __ft_board_setup(void *blob, bd_t *bd)
>> > {
>> > ? ? ? ?...
>> > }
>> > void ft_board_setup(void *blob, bd_t *bd) __attribute__((weak,
>> > alias("__ft_board_setup")));
>> >
>> >
>> > And then this weak default is overridden and still referenced in
>> > board/amcc/canyonlands/canyonlands.c:
>> >
>> > void ft_board_setup(void *blob, bd_t *bd)
>> > {
>> > ? ? ? ?__ft_board_setup(blob, bd);
>> > ? ? ? ?...
>> >
>> >
>> > So no need for this ifdef in the common CFI driver. Or am I missing
>> > something here?
>>
>> Oh. I didn't knew I could access the function that was overridden by the
>> weak attribute. I guess that's the alias is for right?
>
> Yep.
>
>> If both can be
>> called, I'm happy to remove the ifdef.
>>
>> I'll test that tomorrow and provide a patch if it works.
>
> Good luck...
>

Thanks, this worked for me. Is it ok for you too?

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 6039e1f..99360af 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -176,6 +176,7 @@ u64 flash_read64(void *addr)__attribute__((weak,
alias("__flash_read64")));
 #define flash_read64	__flash_read64
 #endif

+
 /*-----------------------------------------------------------------------
  */
 #if defined(CONFIG_ENV_IS_IN_FLASH) ||
defined(CONFIG_ENV_ADDR_REDUND) || (CONFIG_SYS_MONITOR_BASE >=
CONFIG_SYS_FLASH_BASE)
@@ -2151,7 +2152,8 @@ void flash_protect_default(void)
 #endif
 }

-unsigned long flash_init (void)
+unsigned long flash_init(void) __attribute__((weak, alias("__flash_init")));
+unsigned long __flash_init (void)
 {
 	unsigned long size = 0;
 	int i;

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Stefan Roese]

Hi Frank,

On Wednesday 01 June 2011 16:33:14 Frank Svendsb?e wrote:
> >> Simply because disabling write-protection is not impossible after
> >> installation. Our device will be located 3000m below sea level.
> > 
> > I see.
> 
> Hmm.. then you read my mind :) I meant to say "is not possible" and not "is
> not impossible" :)

Yep. I read so fast, that I didn't catch the misspelled words. ;)
 
> Thanks, this worked for me. Is it ok for you too?
> 
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index 6039e1f..99360af 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -176,6 +176,7 @@ u64 flash_read64(void *addr)__attribute__((weak,
> alias("__flash_read64")));
>  #define flash_read64	__flash_read64
>  #endif
> 
> +

Don't add this empty line.

>  /*-----------------------------------------------------------------------
>   */
>  #if defined(CONFIG_ENV_IS_IN_FLASH) ||
> defined(CONFIG_ENV_ADDR_REDUND) || (CONFIG_SYS_MONITOR_BASE >=
> CONFIG_SYS_FLASH_BASE)
> @@ -2151,7 +2152,8 @@ void flash_protect_default(void)
>  #endif
>  }
> 
> -unsigned long flash_init (void)
> +unsigned long flash_init(void) __attribute__((weak,
> alias("__flash_init"))); +unsigned long __flash_init (void)
>  {
>  	unsigned long size = 0;
>  	int i;

Looks good. I have no objections. Please go ahead and send this as a "proper" 
patch.

Best regards,
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] Reg. CFI flash_init and hardware write protected devices

[Frank Svendsbøe]

On Wed, Jun 1, 2011 at 5:34 PM, Stefan Roese <sr@denx.de> wrote:
> Hi Frank,
>
> On Wednesday 01 June 2011 16:33:14 Frank Svendsb?e wrote:
>> >> Simply because disabling write-protection is not impossible after
>> >> installation. Our device will be located 3000m below sea level.
>> >
>> > I see.
>>
>> Hmm.. then you read my mind :) I meant to say "is not possible" and not "is
>> not impossible" :)
>
> Yep. I read so fast, that I didn't catch the misspelled words. ;)
>
>> Thanks, this worked for me. Is it ok for you too?
>>
>> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
>> index 6039e1f..99360af 100644
>> --- a/drivers/mtd/cfi_flash.c
>> +++ b/drivers/mtd/cfi_flash.c
>> @@ -176,6 +176,7 @@ u64 flash_read64(void *addr)__attribute__((weak,
>> alias("__flash_read64")));
>> ?#define flash_read64 __flash_read64
>> ?#endif
>>
>> +
>
> Don't add this empty line.
>

Thanks for spotting this. I'll remove this of course.

>> ?/*-----------------------------------------------------------------------
>> ? */
>> ?#if defined(CONFIG_ENV_IS_IN_FLASH) ||
>> defined(CONFIG_ENV_ADDR_REDUND) || (CONFIG_SYS_MONITOR_BASE >=
>> CONFIG_SYS_FLASH_BASE)
>> @@ -2151,7 +2152,8 @@ void flash_protect_default(void)
>> ?#endif
>> ?}
>>
>> -unsigned long flash_init (void)
>> +unsigned long flash_init(void) __attribute__((weak,
>> alias("__flash_init"))); +unsigned long __flash_init (void)
>> ?{
>> ? ? ? unsigned long size = 0;
>> ? ? ? int i;
>
> Looks good. I have no objections. Please go ahead and send this as a "proper"
> patch.
>

Will do (as soon as I get back to work).

Best regards,
Frank.

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Frank Svendsbøe]

On Wed, Jun 1, 2011 at 6:59 PM, Frank Svendsb?e
<frank.svendsboe@gmail.com> wrote:
> On Wed, Jun 1, 2011 at 5:34 PM, Stefan Roese <sr@denx.de> wrote:
>> Hi Frank,
>>
>> On Wednesday 01 June 2011 16:33:14 Frank Svendsb?e wrote:
>>> >> Simply because disabling write-protection is not impossible after
>>> >> installation. Our device will be located 3000m below sea level.
>>> >
>>> > I see.
>>>
>>> Hmm.. then you read my mind :) I meant to say "is not possible" and not "is
>>> not impossible" :)
>>
>> Yep. I read so fast, that I didn't catch the misspelled words. ;)
>>
>>> Thanks, this worked for me. Is it ok for you too?
>>>
>>> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
>>> index 6039e1f..99360af 100644
>>> --- a/drivers/mtd/cfi_flash.c
>>> +++ b/drivers/mtd/cfi_flash.c
>>> @@ -176,6 +176,7 @@ u64 flash_read64(void *addr)__attribute__((weak,
>>> alias("__flash_read64")));
>>> ?#define flash_read64 __flash_read64
>>> ?#endif
>>>
>>> +
>>
>> Don't add this empty line.
>>
>
> Thanks for spotting this. I'll remove this of course.
>
>>> ?/*-----------------------------------------------------------------------
>>> ? */
>>> ?#if defined(CONFIG_ENV_IS_IN_FLASH) ||
>>> defined(CONFIG_ENV_ADDR_REDUND) || (CONFIG_SYS_MONITOR_BASE >=
>>> CONFIG_SYS_FLASH_BASE)
>>> @@ -2151,7 +2152,8 @@ void flash_protect_default(void)
>>> ?#endif
>>> ?}
>>>
>>> -unsigned long flash_init (void)
>>> +unsigned long flash_init(void) __attribute__((weak,
>>> alias("__flash_init"))); +unsigned long __flash_init (void)
>>> ?{
>>> ? ? ? unsigned long size = 0;
>>> ? ? ? int i;
>>
>> Looks good. I have no objections. Please go ahead and send this as a "proper"
>> patch.
>>
>
> Will do (as soon as I get back to work).
>
> Best regards,
> Frank.
>

Hi Stefan,
A few weeks ago, I applied this change and everything worked fine in
U-Boot. However, I didn't succeed doing the same hack in Linux. I've
done some hardcoding experiments in drivers/mtd/chips/cfi_probe.c, but
so far no success. After reading some CFI manuals, it seems CFI
will/should use bus write operations even during CFI read operations.
Can you confirm this? Then my next question is how come U-Boot read
operations works by just hardcoding the flash geometry. It seems
U-Boot utilize just plain localbus accesses to read flash data, ie.
not the "CFI protocol". What did I miss here.. ?

Now I'm considering two options: 1) Either write a custom flash map
driver, and access the flash the same way U-Boot does, or 2) try to
convince the FPGA designers that our way of forcing write protection
is not according the the CFI specifications, and should be done
otherwise. What do you think is the best solution?

Is there anybody else out there supporting hardware write protection
on CFI devices?

Best regards,
Frank

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Wolfgang Denk]

Dear Frank,

In message <BANLkTinFMA7FN-BD7z2-ZYAT+-ubWn7S9Q@mail.gmail.com> you wrote:
>
> A few weeks ago, I applied this change and everything worked fine in
> U-Boot. However, I didn't succeed doing the same hack in Linux. I've
> done some hardcoding experiments in drivers/mtd/chips/cfi_probe.c, but
> so far no success. After reading some CFI manuals, it seems CFI
> will/should use bus write operations even during CFI read operations.

What exactly do you mean by "CFI read operations"?

> Can you confirm this? Then my next question is how come U-Boot read
> operations works by just hardcoding the flash geometry. It seems
> U-Boot utilize just plain localbus accesses to read flash data, ie.
> not the "CFI protocol". What did I miss here.. ?

It seems you misunderstand the different modes.  In command mode, you
will use the specific command sequences defined in the CFI protocol to
talk to the controller poart of the flash chip. In this mode, you can
read for example query information, flash geometry data, status
information, etc.  Yes, any such command sequence includes write
operations to the flash device, even when you actually want to read
some specific data.  In data mode, flash is working just as normal
real-only memory. No specific protocol is used, you just use plain
read accesses to read the user data programmed into the flash chip.

Any kind of probing, auto-identification etc. of the flash chip will
need to use CFI command sequences, and thus write access to the
device.  As long as you just want to read the data or execute code
from the flash no write accesses are needed.

> Now I'm considering two options: 1) Either write a custom flash map
> driver, and access the flash the same way U-Boot does, or 2) try to
> convince the FPGA designers that our way of forcing write protection
> is not according the the CFI specifications, and should be done
> otherwise. What do you think is the best solution?

Linux can also read from ROM memory. You just have to make sure that
you do not try to run the normal CFI flash drivers on your flash
devices - these will not work, because they will try to probe the
flash chips.

> Is there anybody else out there supporting hardware write protection
> on CFI devices?

CFI flash chips with the level of write protection you implemented
have to be dealt with in the same way as other ROM memory.  Normal CFI
driver are not appropriate 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
And now remains  That we find out the cause of this effect, Or rather
say, the cause of this defect...           -- Hamlet, Act II, Scene 2

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Frank Svendsbøe]

On Thu, Jun 23, 2011 at 5:21 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Frank,
>
> In message <BANLkTinFMA7FN-BD7z2-ZYAT+-ubWn7S9Q@mail.gmail.com> you wrote:
>>
>> A few weeks ago, I applied this change and everything worked fine in
>> U-Boot. However, I didn't succeed doing the same hack in Linux. I've
>> done some hardcoding experiments in drivers/mtd/chips/cfi_probe.c, but
>> so far no success. After reading some CFI manuals, it seems CFI
>> will/should use bus write operations even during CFI read operations.
>
> What exactly do you mean by "CFI read operations"?
>

Good question. When reading Intels CFI manual I was referring to
something called "read array" which used bus write commands. I guess
normal byte/word read access don't utilize this then.

>> Can you confirm this? Then my next question is how come U-Boot read
>> operations works by just hardcoding the flash geometry. It seems
>> U-Boot utilize just plain localbus accesses to read flash data, ie.
>> not the "CFI protocol". What did I miss here.. ?
>
> It seems you misunderstand the different modes. ?In command mode, you
> will use the specific command sequences defined in the CFI protocol to
> talk to the controller poart of the flash chip. In this mode, you can
> read for example query information, flash geometry data, status
> information, etc. ?Yes, any such command sequence includes write
> operations to the flash device, even when you actually want to read
> some specific data. ?In data mode, flash is working just as normal
> real-only memory. No specific protocol is used, you just use plain
> read accesses to read the user data programmed into the flash chip.
>

Right, and thanks for the explanation. What I've done to U-Boot is
bypass the probing which uses the command mode and thus avoid getting
problems with bus write accesses when flash protection is on.

> Any kind of probing, auto-identification etc. of the flash chip will
> need to use CFI command sequences, and thus write access to the
> device. ?As long as you just want to read the data or execute code
> from the flash no write accesses are needed.
>
>> Now I'm considering two options: 1) Either write a custom flash map
>> driver, and access the flash the same way U-Boot does, or 2) try to
>> convince the FPGA designers that our way of forcing write protection
>> is not according the the CFI specifications, and should be done
>> otherwise. What do you think is the best solution?
>
> Linux can also read from ROM memory. You just have to make sure that
> you do not try to run the normal CFI flash drivers on your flash
> devices - these will not work, because they will try to probe the
> flash chips.
>

Yes, well I tried to fool Linux's CFI driver too, but no luck so far.
It was after this I started reading about CFI and came to the (wrong)
conclusion that CFI uses bus write operations during read accesses
too.

>> Is there anybody else out there supporting hardware write protection
>> on CFI devices?
>
> CFI flash chips with the level of write protection you implemented
> have to be dealt with in the same way as other ROM memory. ?Normal CFI
> driver are not appropriate here.
>

Ok. So you recommend I stop "hacking" the CFI probing and instead
write a custom flash map driver?

Best regards,
Frank

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Wolfgang Denk]

Dear Frank,

In message <BANLkTi=p9gdOGOETWojRqkmrxXM7Do_tBg@mail.gmail.com> you wrote:
>
> > CFI flash chips with the level of write protection you implemented
> > have to be dealt with in the same way as other ROM memory. =A0Normal CFI
> > driver are not appropriate here.
> 
> Ok. So you recommend I stop "hacking" the CFI probing and instead
> write a custom flash map driver?

No.  Never write new drivers when you can use existing ones :-)

Describe the flash memory as "mtd-ram" (instead of "cfi-flash") in
the device tree and select the physmap driver in your kernel config.

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
The only way to get rid of a temptation is to yield to it.
                                                        - Oscar Wilde

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Frank Svendsbøe]

On Thu, Jun 23, 2011 at 7:55 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Frank,
>
> In message <BANLkTi=p9gdOGOETWojRqkmrxXM7Do_tBg@mail.gmail.com> you wrote:
>>
>> > CFI flash chips with the level of write protection you implemented
>> > have to be dealt with in the same way as other ROM memory. =A0Normal CFI
>> > driver are not appropriate here.
>>
>> Ok. So you recommend I stop "hacking" the CFI probing and instead
>> write a custom flash map driver?
>
> No. ?Never write new drivers when you can use existing ones :-)
>
> Describe the flash memory as "mtd-ram" (instead of "cfi-flash") in
> the device tree and select the physmap driver in your kernel config.
>

Oh. I will try that. Thanks a lot for this tip :)

.. and sorry for asking about Linux related topics on the U-Boot ML.

Best regards,
Frank

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Frank Svendsbøe]

>> Ok. So you recommend I stop "hacking" the CFI probing and instead
>> write a custom flash map driver?
>
> No. ?Never write new drivers when you can use existing ones :-)
>
> Describe the flash memory as "mtd-ram" (instead of "cfi-flash") in
> the device tree and select the physmap driver in your kernel config.
>

Hi Wolfgang, I did as you recommended and replaced cfi-flash with
mtd-ram in the device tree. I also defined CONFIG_MTD_PHYSMAP_OF,
CONFIG_MTD_MTDRAM, CONFIG_MTDRAM_TOTAL_SIZE according to our
specifications. The default erase-size was 128k, which is what we have
too, so I didn't touch that. Now when I boot the kernel recognizes all
the partitions I've defined in the dts. But, when mounting a
jffs2-filesystem, it ends with a jffs2_scan_eraseblock(): Magic
bitmask 0x1985 not found...

Do you have any other tips?

When working with this, I realised that if I could get it to work we'd
still might have a problem. You see, we need write access for one of
the flashes when upgrading software. We can't treat this as a simple
ROM. So do we need CFI working in order to set the device into
write-mode, erase flash sectors, etc.? Or do mtd-ram handle flash
write operations too?

In theory, I guess I could unmount the root fs, unload the mtd module,
insert the cfi-flash module, mount the filesystem, then write, etc..
But isn't that harder than write a custom map driver?

Best regards,
Frank

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Wolfgang Denk]

Dear =?ISO-8859-1?Q?Frank_Svendsb=F8e?=,

In message <BANLkTimt0LYXr-B5PAr1ONEaEoWKCyEHbw@mail.gmail.com> you wrote:
>
> Hi Wolfgang, I did as you recommended and replaced cfi-flash with
> mtd-ram in the device tree. I also defined CONFIG_MTD_PHYSMAP_OF,
> CONFIG_MTD_MTDRAM, CONFIG_MTDRAM_TOTAL_SIZE according to our
> specifications. The default erase-size was 128k, which is what we have
> too, so I didn't touch that. Now when I boot the kernel recognizes all
> the partitions I've defined in the dts. But, when mounting a
> jffs2-filesystem, it ends with a jffs2_scan_eraseblock(): Magic
> bitmask 0x1985 not found...
> 
> Do you have any other tips?

Difficult to speculate - I don't know your hardware (eventually you
have two 16 bit flash chips in parallel to build a 32 bit bus, and
have to double the chip's erase block size?), and I don;t know how you
created the JFFS2 file system.

Are you sure you want to use JFFS2?  UBIFS is considered to be a
better choice these days...

> When working with this, I realised that if I could get it to work we'd
> still might have a problem. You see, we need write access for one of
> the flashes when upgrading software. We can't treat this as a simple
> ROM. So do we need CFI working in order to set the device into
> write-mode, erase flash sectors, etc.? Or do mtd-ram handle flash
> write operations too?

mtd-ram provides a pure memory interface, i. e. you cannot use this to
erase or program any blocks in a CFI flash device.  To do so, you need
the CFI driver.

> In theory, I guess I could unmount the root fs, unload the mtd module,
> insert the cfi-flash module, mount the filesystem, then write, etc..

Yes, or you could start with the CFI driver in the first place.

> But isn't that harder than write a custom map driver?

I consider your chances to get such a customdriver into mainline to be
epsilon.  And you don't really want to use any out of tree drivers.


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
If you're out of tree, you don't exist.
     - David Woodhouse in <1304620350.2398.29.camel@i7.infradead.org>

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Frank Svendsbøe]

On Fri, Jun 24, 2011 at 4:26 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear =?ISO-8859-1?Q?Frank_Svendsb=F8e?=,
>
> In message <BANLkTimt0LYXr-B5PAr1ONEaEoWKCyEHbw@mail.gmail.com> you wrote:
>>
>> Hi Wolfgang, I did as you recommended and replaced cfi-flash with
>> mtd-ram in the device tree. I also defined CONFIG_MTD_PHYSMAP_OF,
>> CONFIG_MTD_MTDRAM, CONFIG_MTDRAM_TOTAL_SIZE according to our
>> specifications. The default erase-size was 128k, which is what we have
>> too, so I didn't touch that. Now when I boot the kernel recognizes all
>> the partitions I've defined in the dts. But, when mounting a
>> jffs2-filesystem, it ends with a jffs2_scan_eraseblock(): Magic
>> bitmask 0x1985 not found...
>>
>> Do you have any other tips?
>
> Difficult to speculate - I don't know your hardware (eventually you
> have two 16 bit flash chips in parallel to build a 32 bit bus, and
> have to double the chip's erase block size?), and I don;t know how you
> created the JFFS2 file system.
>

Yes I know, and I don't expect you to find the answer based on so
little data. FYI: We have two flash chips yes, but not to build a
32-bit bus, we have two for redundancy issues (one is storing a
factory default version). 16 bits databus and each sector is 64k
words, ie. 128k bytes. I created the jffs2 images using ELDKs
mkfs.jffs2. Thanks for providing me this tool :)

> Are you sure you want to use JFFS2? ?UBIFS is considered to be a
> better choice these days...
>

No, not sure. I've got two reasons for staying with JFFS2. 1) We
normally don't write to flash except when upgrading, so flash
performance/duration isn't very critical 2) I discovered UBIFS after I
had JFFS2 working and haven't seen the real need for it yet.

>> When working with this, I realised that if I could get it to work we'd
>> still might have a problem. You see, we need write access for one of
>> the flashes when upgrading software. We can't treat this as a simple
>> ROM. So do we need CFI working in order to set the device into
>> write-mode, erase flash sectors, etc.? Or do mtd-ram handle flash
>> write operations too?
>
> mtd-ram provides a pure memory interface, i. e. you cannot use this to
> erase or program any blocks in a CFI flash device. ?To do so, you need
> the CFI driver.
>

Right. mtd-ram is thus a dead-end solution for our purpose. We need to
be able to write in some specific cases.

>> In theory, I guess I could unmount the root fs, unload the mtd module,
>> insert the cfi-flash module, mount the filesystem, then write, etc..
>
> Yes, or you could start with the CFI driver in the first place.
>

That's what I did before you started interrupting me about mtd-ram...
No, just kidding :-)

Did you mean I should adapt the general CFI driver to support our
configuration, or... Based on what you say below I'm not sure what you
actually mean.

>> But isn't that harder than write a custom map driver?
>
> I consider your chances to get such a customdriver into mainline to be
> epsilon. ?And you don't really want to use any out of tree drivers.
>

Me too. Small chances. So we have two options I guess: 1) Ditch
hardware write protection and use CFI as it is supposed to be used, or
2) write a custom out-of-tree driver. What would you go for?

I'll consider if and how to get things into mainline once we have
things working as we want. If my port isn't accepted I will at least
learn why. Maybe the next will :)

Best regards,
Frank

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Wolfgang Denk]

Dear Frank,

In message <BANLkTindk4f2An2VqhSftPcGuvsEqLY7wQ@mail.gmail.com> you wrote:
>
> > Difficult to speculate - I don't know your hardware (eventually you
> > have two 16 bit flash chips in parallel to build a 32 bit bus, and
> > have to double the chip's erase block size?), and I don;t know how you
> > created the JFFS2 file system.
...
> factory default version). 16 bits databus and each sector is 64k
> words, ie. 128k bytes. I created the jffs2 images using ELDKs
> mkfs.jffs2. Thanks for providing me this tool :)

You need to provide the correct parameters to mkfs.jffs2...

> No, not sure. I've got two reasons for staying with JFFS2. 1) We
> normally don't write to flash except when upgrading, so flash
> performance/duration isn't very critical 2) I discovered UBIFS after I

Hm... JFFS2 is probaly not a good choice anyway.  On NOR, where you
don't have to deal with bad blocks, you might use ext2 or cramfs or
... - all of them are more efficient than JFFS2 then.

> had JFFS2 working and haven't seen the real need for it yet.

Mount time?

> Did you mean I should adapt the general CFI driver to support our
> configuration, or... Based on what you say below I'm not sure what you
> actually mean.

You have two different use cases that require (if we don't want to use
a proprietary driver) two different, mutually exclusive drivers.  I
would load the driver I need as module, and unload it if I need the
other one. This is pretty simple in principle, but a bit ore of a
challange in your case as you use this as a root file system
(including to load the modules from).

> Me too. Small chances. So we have two options I guess: 1) Ditch
> hardware write protection and use CFI as it is supposed to be used, or
> 2) write a custom out-of-tree driver. What would you go for?

If this is an option: 1)

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
Do not simplify the design of a program if a way can be found to make
it complex and wonderful.

[U-Boot] Reg. CFI flash_init and hardware write protected devices

[Frank Svendsbøe]

On Fri, Jun 24, 2011 at 10:26 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Frank,
>
> In message <BANLkTindk4f2An2VqhSftPcGuvsEqLY7wQ@mail.gmail.com> you wrote:
>>
>> > Difficult to speculate - I don't know your hardware (eventually you
>> > have two 16 bit flash chips in parallel to build a 32 bit bus, and
>> > have to double the chip's erase block size?), and I don;t know how you
>> > created the JFFS2 file system.
> ...
>> factory default version). 16 bits databus and each sector is 64k
>> words, ie. 128k bytes. I created the jffs2 images using ELDKs
>> mkfs.jffs2. Thanks for providing me this tool :)
>
> You need to provide the correct parameters to mkfs.jffs2...
>

Of course, but I don't see how changing from CFI to raw memory
accesses on target change the way the jffs2 image should be created.
What didn't I understand?

I'm currently using:
mkfs.jss2 -U -b -d /${input} -e 0x20000 -o ${output}

>> No, not sure. I've got two reasons for staying with JFFS2. 1) We
>> normally don't write to flash except when upgrading, so flash
>> performance/duration isn't very critical 2) I discovered UBIFS after I
>
> Hm... JFFS2 is probaly not a good choice anyway. ?On NOR, where you
> don't have to deal with bad blocks, you might use ext2 or cramfs or
> ... - all of them are more efficient than JFFS2 then.
>

Oh. Thanks for letting me know. I knew cramfs would be more efficient,
but surprised that ext2 would be too. Thought ext2 was "heavier".

>> had JFFS2 working and haven't seen the real need for it yet.
>
> Mount time?
>
Maybe 1-2 seconds. Haven't measured it yet. Maybe boot-time could've
been improved yes.

>> Did you mean I should adapt the general CFI driver to support our
>> configuration, or... Based on what you say below I'm not sure what you
>> actually mean.
>
> You have two different use cases that require (if we don't want to use
> a proprietary driver) two different, mutually exclusive drivers. ?I
> would load the driver I need as module, and unload it if I need the
> other one. This is pretty simple in principle, but a bit ore of a
> challange in your case as you use this as a root file system
> (including to load the modules from).
>

When thinking a bit more about this, I realize we don't need to
support writing to the current filesystem in use. I've partitioned our
device into three region, where each "region" consists of four
partitions (dtb, kernel, rootfs and "home"). When upgrading we write a
complete "region image" into another flash region, which is not part
of the region we're running. So maybe we could have both mtd-ram and
cfi-flash running at the same time, since we don't actually need CFI
for the flash we're currently booting from.

I currently use mtd-utils' flash_erase to erase, and 'dd if=/dev/mtdX'
to write. The mtds is created based on info in the dts.. Regarding
DTS: Is it possible to have different partitions within the same
physical flash using different drivers? Like this:

flash at 0,0 {
   compatible = "amd,s29gl512n", "cfi-flash",
   reg = <0x0 0x0 0x4000000>;
   partition at ...
}

... into something like this:

flash at 0,0 {
   compatible = "amd,s29gl512n", "cfi-flash",
   reg = <0x0, 0x0 0x2000000>;
   partition at ..
}

flash at 0,2000000 {
   compatible = "amd,s29gl512n", "mtd-ram",
   reg = <0x2000000, 0x0, 0x2000000>;
   partition@
}

In other words, partition a flash to use different drivers when
accessing different partitions? I think that could solve the problems.

>> Me too. Small chances. So we have two options I guess: 1) Ditch
>> hardware write protection and use CFI as it is supposed to be used, or
>> 2) write a custom out-of-tree driver. What would you go for?
>
> If this is an option: 1)
>

Good to know your opinion on this.

> 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
> Do not simplify the design of a program if a way can be found to make
> it complex and wonderful.
>

Hehe. Who wrote that?

Best regards,
Frank