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