[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