Message ID | BANLkTinUbHgHmXkMfNOVJofr9nJB3B2Phw@mail.gmail.com |
---|---|
State | Not Applicable |
Headers | show |
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
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
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
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
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
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 --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 + /*----------------------------------------------------------------------- */