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