diff mbox

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

Message ID BANLkTinUbHgHmXkMfNOVJofr9nJB3B2Phw@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Frank Svendsbøe May 31, 2011, 8:35 a.m. UTC 
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:

----

 #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

Comments

Mike Frysinger May 31, 2011, 12:49 p.m. UTC | #1 
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
Stefan Roese May 31, 2011, 1:10 p.m. UTC | #2 
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@denx.de
Frank Svendsbøe May 31, 2011, 1:25 p.m. UTC | #3 
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
Frank Svendsbøe May 31, 2011, 1:55 p.m. UTC | #4 
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
Mike Frysinger May 31, 2011, 2:01 p.m. UTC | #5 
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
Stefan Roese May 31, 2011, 2:37 p.m. UTC | #6 
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@denx.de
diff mbox

Patch

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
+
 /*-----------------------------------------------------------------------
  */