| Message ID | 1442066256-11835-5-git-send-email-otavio@ossystems.com.br |
|---|---|
| State | Rejected |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
On 12 September 2015 at 19:27, Otavio Salvador <otavio@ossystems.com.br> wrote: > The last 16 KiB of the SPI NOR contain some manufacturing information, which > should not be erased/overwritten. > > Configure the protection bits BP2, BP1 and BP0 so that this region is > protected. > > Signed-off-by: Otavio Salvador <otavio@ossystems.com.br> > --- > Changes since v1: > - Use CONFIG_SPI_NOR_PROTECTION_STM > > board/congatec/cgtqmx6eval/cgtqmx6eval.c | 25 +++++++++++++++++++++++++ > include/configs/cgtqmx6eval.h | 2 ++ > 2 files changed, 27 insertions(+) > > diff --git a/board/congatec/cgtqmx6eval/cgtqmx6eval.c b/board/congatec/cgtqmx6eval/cgtqmx6eval.c > index a9694d5..9aff08d 100644 > --- a/board/congatec/cgtqmx6eval/cgtqmx6eval.c > +++ b/board/congatec/cgtqmx6eval/cgtqmx6eval.c > @@ -31,6 +31,8 @@ > #include <miiphy.h> > #include <netdev.h> > #include <micrel.h> > +#include <spi_flash.h> > +#include <spi.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -399,6 +401,22 @@ void setup_spinor(void) > gpio_direction_output(IMX_GPIO_NR(3, 19), 0); > } > > +static void spinor_protect(void) > +{ > + struct spi_flash *spi; > + > + spi = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, > + CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); > + /* > + * Set BP2 BP1 BP0 to 001, so that the last 64 sectors > + * can be protected (0x3f0000 to 0x3fffff). The last sector (64*1024) area is being protected here, why? does this specific to your board? And also your taking an example of (0x3f0000 to 0x3fffff) that means the flash your using is 4MB is it? then this again becomes your board specific? is it? > + * > + * This area stores some manufacturing information that > + * should not be erased. > + */ > + spi_flash_cmd_write_status(spi, 1 << 2); > +} > + > #ifdef CONFIG_FSL_ESDHC > static struct fsl_esdhc_cfg usdhc_cfg[] = { > {USDHC2_BASE_ADDR}, > @@ -711,3 +729,10 @@ int misc_init_r(void) > #endif > return 0; > } > + > +int board_late_init(void) > +{ > + spinor_protect(); > + > + return 0; > +} > diff --git a/include/configs/cgtqmx6eval.h b/include/configs/cgtqmx6eval.h > index 66f81ec..8110605 100644 > --- a/include/configs/cgtqmx6eval.h > +++ b/include/configs/cgtqmx6eval.h > @@ -21,6 +21,7 @@ > #define CONFIG_SYS_MALLOC_LEN (10 * 1024 * 1024) > > #define CONFIG_BOARD_EARLY_INIT_F > +#define CONFIG_BOARD_LATE_INIT > #define CONFIG_MISC_INIT_R > > #define CONFIG_MXC_UART > @@ -34,6 +35,7 @@ > #define CONFIG_SPI_FLASH > #define CONFIG_SPI_FLASH_STMICRO > #define CONFIG_SPI_FLASH_SST > +#define CONFIG_SPI_NOR_PROTECTION_STM > #define CONFIG_MXC_SPI > #define CONFIG_SF_DEFAULT_BUS 0 > #define CONFIG_SF_DEFAULT_SPEED 20000000 > -- > 1.9.1 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
On Tue, Sep 22, 2015 at 4:11 PM, Jagan Teki <jteki@openedev.com> wrote: > On 12 September 2015 at 19:27, Otavio Salvador <otavio@ossystems.com.br> wrote: >> +static void spinor_protect(void) >> +{ >> + struct spi_flash *spi; >> + >> + spi = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, >> + CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); >> + /* >> + * Set BP2 BP1 BP0 to 001, so that the last 64 sectors >> + * can be protected (0x3f0000 to 0x3fffff). > > The last sector (64*1024) area is being protected here, why? does this > specific to your board? > And also your taking an example of (0x3f0000 to 0x3fffff) that means > the flash your using is 4MB is it? then this again becomes your board > specific? is it? Yes, the protected range is board specific. Congatec put some data in this area which are valuable and should not be erased. So we need to protect it.
On 23 September 2015 at 01:27, Otavio Salvador <otavio.salvador@ossystems.com.br> wrote: > On Tue, Sep 22, 2015 at 4:11 PM, Jagan Teki <jteki@openedev.com> wrote: >> On 12 September 2015 at 19:27, Otavio Salvador <otavio@ossystems.com.br> wrote: >>> +static void spinor_protect(void) >>> +{ >>> + struct spi_flash *spi; >>> + >>> + spi = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, >>> + CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); >>> + /* >>> + * Set BP2 BP1 BP0 to 001, so that the last 64 sectors >>> + * can be protected (0x3f0000 to 0x3fffff). >> >> The last sector (64*1024) area is being protected here, why? does this >> specific to your board? >> And also your taking an example of (0x3f0000 to 0x3fffff) that means >> the flash your using is 4MB is it? then this again becomes your board >> specific? is it? > > Yes, the protected range is board specific. Congatec put some data in > this area which are valuable and should not be erased. So we need to > protect it. Sorry, I didn't understand why protection range or bits are specific to board, is it different in Congetc because the flash area being protected by means of sector range which is obviously the flash offset. m25p32 BP2/BP1/BP0=protected sectors 000 = sector (none) 001 = sector (63) 010 = sector (63, 62) .... 111 = sector (all) I understand your concept of protecting sectors once you find the BP2, BP1 and BP0 bits on board and then you locked the particular sectors, is it? it's totally reverse way that you're trying to do is it? thanks!
On Wed, Sep 23, 2015 at 5:15 AM, Jagan Teki <jteki@openedev.com> wrote: > Sorry, I didn't understand why protection range or bits are specific > to board, is it different in Congetc because the flash area being > protected by means of sector range which is obviously the flash > offset. The protection range is board specific. For example: certain boards don't need any region to be protected at all, other boards may want the entire SPI flash to be protected. In Congatec's boards we only need to protect the last 16KiB sector because it contains data that are pre-programmed in the factory and we don't want the user to erase it. That's why we set in the congatec board code the BP bits to 001. > > m25p32 > > BP2/BP1/BP0=protected sectors > 000 = sector (none) > 001 = sector (63) > 010 = sector (63, 62) > .... > 111 = sector (all) > > I understand your concept of protecting sectors once you find the BP2, > BP1 and BP0 bits on board and then you locked the particular sectors, Yes, this is the idea. > is it? it's totally reverse way that you're trying to do is it? No, the logic is correct. In Congatec's board we only want the last sector to be protected, hence BP2 BP1 BP0 is set to 001.
On 23 September 2015 at 19:06, Otavio Salvador <otavio.salvador@ossystems.com.br> wrote: > On Wed, Sep 23, 2015 at 5:15 AM, Jagan Teki <jteki@openedev.com> wrote: > >> Sorry, I didn't understand why protection range or bits are specific >> to board, is it different in Congetc because the flash area being >> protected by means of sector range which is obviously the flash >> offset. > > The protection range is board specific. For example: certain boards > don't need any region to be protected at all, other boards may want > the entire SPI flash to be protected. In Congatec's boards we only > need to protect the last 16KiB sector because it contains data that > are pre-programmed in the factory and we don't want the user to erase > it. > > That's why we set in the congatec board code the BP bits to 001. > >> >> m25p32 >> >> BP2/BP1/BP0=protected sectors >> 000 = sector (none) >> 001 = sector (63) >> 010 = sector (63, 62) >> .... >> 111 = sector (all) >> >> I understand your concept of protecting sectors once you find the BP2, >> BP1 and BP0 bits on board and then you locked the particular sectors, > > Yes, this is the idea. > >> is it? it's totally reverse way that you're trying to do is it? > > No, the logic is correct. In Congatec's board we only want the last > sector to be protected, hence BP2 BP1 BP0 is set to 001. All looks fine as per your patches, but probing flash from board files isn't a good approach if one more board add similar approach. I have an idea similar to "cfi_flash" approach. "sf protect on off len" then based on the offset and len write the protected bits and skips the sectors which are protected by showing warning say "protected sectors will not be erased!" [1] Use the Linux approach[2] for more information, let me know for any more inputs. [1] http://www.denx.de/wiki/view/DULG/UBootCmdGroupFlash#Section_5.9.3.4. [2] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mtd/devices/m25p80.c?id=972e1b7b450a93589b2a4c709e68f68da059aa5c thanks!
Hello Jagan, On Wed, Sep 23, 2015 at 12:56 PM, Jagan Teki <jteki@openedev.com> wrote: > All looks fine as per your patches, but probing flash from board files > isn't a good approach if one more board add similar approach. > > I have an idea similar to "cfi_flash" approach. > > "sf protect on off len" then based on the offset and len write the > protected bits and skips the sectors which are protected by showing > warning say "protected sectors will not be erased!" [1] > > Use the Linux approach[2] for more information, let me know for any more inputs. > > [1] http://www.denx.de/wiki/view/DULG/UBootCmdGroupFlash#Section_5.9.3.4. > [2] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mtd/devices/m25p80.c?id=972e1b7b450a93589b2a4c709e68f68da059aa5c I think this is a good option however, can we include this one for this release and we can improve it for next?
On 23 September 2015 at 22:21, Otavio Salvador <otavio.salvador@ossystems.com.br> wrote: > Hello Jagan, > > On Wed, Sep 23, 2015 at 12:56 PM, Jagan Teki <jteki@openedev.com> wrote: >> All looks fine as per your patches, but probing flash from board files >> isn't a good approach if one more board add similar approach. >> >> I have an idea similar to "cfi_flash" approach. >> >> "sf protect on off len" then based on the offset and len write the >> protected bits and skips the sectors which are protected by showing >> warning say "protected sectors will not be erased!" [1] >> >> Use the Linux approach[2] for more information, let me know for any more inputs. >> >> [1] http://www.denx.de/wiki/view/DULG/UBootCmdGroupFlash#Section_5.9.3.4. >> [2] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mtd/devices/m25p80.c?id=972e1b7b450a93589b2a4c709e68f68da059aa5c > > I think this is a good option however, can we include this one for > this release and we can improve it for next? Do you have any defined schedule on coming release about this feature, because right now sf has lot of pending items to tune - I'm unable add again this on TODO list that become big task in future. If you have any time, please start the suggested approach I would help at any moment. thanks!
On Wed, Sep 23, 2015 at 2:15 PM, Jagan Teki <jteki@openedev.com> wrote: > On 23 September 2015 at 22:21, Otavio Salvador > <otavio.salvador@ossystems.com.br> wrote: >> Hello Jagan, >> >> On Wed, Sep 23, 2015 at 12:56 PM, Jagan Teki <jteki@openedev.com> wrote: >>> All looks fine as per your patches, but probing flash from board files >>> isn't a good approach if one more board add similar approach. >>> >>> I have an idea similar to "cfi_flash" approach. >>> >>> "sf protect on off len" then based on the offset and len write the >>> protected bits and skips the sectors which are protected by showing >>> warning say "protected sectors will not be erased!" [1] >>> >>> Use the Linux approach[2] for more information, let me know for any more inputs. >>> >>> [1] http://www.denx.de/wiki/view/DULG/UBootCmdGroupFlash#Section_5.9.3.4. >>> [2] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mtd/devices/m25p80.c?id=972e1b7b450a93589b2a4c709e68f68da059aa5c >> >> I think this is a good option however, can we include this one for >> this release and we can improve it for next? > > Do you have any defined schedule on coming release about this feature, > because right now sf has lot of pending items to tune - I'm unable add > again this on TODO list that become big task in future. > > If you have any time, please start the suggested approach I would help > at any moment. We are adding support for a number of different SoM part numbers from Congatec and the SPI protection is required for we be able to merge the SPL and 2015.10 to be usable for them. I can commit to work in this feature for 2016.01.
On 23 September 2015 at 22:51, Otavio Salvador <otavio.salvador@ossystems.com.br> wrote: > On Wed, Sep 23, 2015 at 2:15 PM, Jagan Teki <jteki@openedev.com> wrote: >> On 23 September 2015 at 22:21, Otavio Salvador >> <otavio.salvador@ossystems.com.br> wrote: >>> Hello Jagan, >>> >>> On Wed, Sep 23, 2015 at 12:56 PM, Jagan Teki <jteki@openedev.com> wrote: >>>> All looks fine as per your patches, but probing flash from board files >>>> isn't a good approach if one more board add similar approach. >>>> >>>> I have an idea similar to "cfi_flash" approach. >>>> >>>> "sf protect on off len" then based on the offset and len write the >>>> protected bits and skips the sectors which are protected by showing >>>> warning say "protected sectors will not be erased!" [1] >>>> >>>> Use the Linux approach[2] for more information, let me know for any more inputs. >>>> >>>> [1] http://www.denx.de/wiki/view/DULG/UBootCmdGroupFlash#Section_5.9.3.4. >>>> [2] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mtd/devices/m25p80.c?id=972e1b7b450a93589b2a4c709e68f68da059aa5c >>> >>> I think this is a good option however, can we include this one for >>> this release and we can improve it for next? >> >> Do you have any defined schedule on coming release about this feature, >> because right now sf has lot of pending items to tune - I'm unable add >> again this on TODO list that become big task in future. >> >> If you have any time, please start the suggested approach I would help >> at any moment. > > We are adding support for a number of different SoM part numbers from > Congatec and the SPI protection is required for we be able to merge > the SPL and 2015.10 to be usable for them. > > I can commit to work in this feature for 2016.01. Sorry, I understand your concern - but it's very difficult for me to maintain the drop_code (which should again removed later). Why can't you just add code as per my suggestion.. just a basic support as you aware probably will move the same in coming release if all set, because extending functionality is better approach rather than add it remove the same. Thanks for your commitment, let me know if you need any more help. thanks!
Hello Jagan, On Thu, Sep 24, 2015 at 5:03 PM, Jagan Teki <jteki@openedev.com> wrote: ... > "sf protect on off len" then based on the offset and len write the > protected bits and skips the sectors which are protected by showing > warning say "protected sectors will not be erased!" [1]" What about creating commands doing like this instead? "sf protect 000" ---> Write 000 to BP2 BP1 BP0 (do not protect any sector) "sf protect 001" ---> Write 001 to BP2 BP1 BP0 (protect the last 1/64 sectors) ... "sf protect 111" ---> Write 111 to BP2 BP1 BP0" (protect all sectors) Would this method be acceptable? It much simpler. I don't think that the proposed "sf protect on off len" would apply to the SPI NOR protection layout. Please advise.
On 25 September 2015 at 22:33, Otavio Salvador <otavio.salvador@ossystems.com.br> wrote: > Hello Jagan, > > On Thu, Sep 24, 2015 at 5:03 PM, Jagan Teki <jteki@openedev.com> wrote: > ... >> "sf protect on off len" then based on the offset and len write the >> protected bits and skips the sectors which are protected by showing >> warning say "protected sectors will not be erased!" [1]" > > What about creating commands doing like this instead? > > "sf protect 000" ---> Write 000 to BP2 BP1 BP0 (do not protect any sector) > "sf protect 001" ---> Write 001 to BP2 BP1 BP0 (protect the last 1/64 sectors) > ... > "sf protect 111" ---> Write 111 to BP2 BP1 BP0" (protect all sectors) This would rather un-obvious implementation, how can use controls register bits, from user point-of-view flash can be accessed in-terms of offset, size. > > Would this method be acceptable? It much simpler. > > I don't think that the proposed "sf protect on off len" would apply to > the SPI NOR protection layout. No, it would apply any flash-layout not only for SPI-NOR, in fact it is much known and acceptable way of implementation - see cfi flash for example(both in Linux, U-Boot) and same case with SPI-NOR on Linux[1] > > Please advise. [1] https://patchwork.ozlabs.org/patch/513041/ -- Jagan.
Hi Jagan,
On Sat, Sep 26, 2015 at 12:47 AM, Jagan Teki <jteki@openedev.com> wrote:
> [1] https://patchwork.ozlabs.org/patch/513041/
Thanks for your suggestion.
I have implemented the SPI NOR protection support based on Brian's
patch for the kernel as you suggested.
Please let me know your thoughts.
Thanks
diff --git a/board/congatec/cgtqmx6eval/cgtqmx6eval.c b/board/congatec/cgtqmx6eval/cgtqmx6eval.c index a9694d5..9aff08d 100644 --- a/board/congatec/cgtqmx6eval/cgtqmx6eval.c +++ b/board/congatec/cgtqmx6eval/cgtqmx6eval.c @@ -31,6 +31,8 @@ #include <miiphy.h> #include <netdev.h> #include <micrel.h> +#include <spi_flash.h> +#include <spi.h> DECLARE_GLOBAL_DATA_PTR; @@ -399,6 +401,22 @@ void setup_spinor(void) gpio_direction_output(IMX_GPIO_NR(3, 19), 0); } +static void spinor_protect(void) +{ + struct spi_flash *spi; + + spi = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, + CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); + /* + * Set BP2 BP1 BP0 to 001, so that the last 64 sectors + * can be protected (0x3f0000 to 0x3fffff). + * + * This area stores some manufacturing information that + * should not be erased. + */ + spi_flash_cmd_write_status(spi, 1 << 2); +} + #ifdef CONFIG_FSL_ESDHC static struct fsl_esdhc_cfg usdhc_cfg[] = { {USDHC2_BASE_ADDR}, @@ -711,3 +729,10 @@ int misc_init_r(void) #endif return 0; } + +int board_late_init(void) +{ + spinor_protect(); + + return 0; +} diff --git a/include/configs/cgtqmx6eval.h b/include/configs/cgtqmx6eval.h index 66f81ec..8110605 100644 --- a/include/configs/cgtqmx6eval.h +++ b/include/configs/cgtqmx6eval.h @@ -21,6 +21,7 @@ #define CONFIG_SYS_MALLOC_LEN (10 * 1024 * 1024) #define CONFIG_BOARD_EARLY_INIT_F +#define CONFIG_BOARD_LATE_INIT #define CONFIG_MISC_INIT_R #define CONFIG_MXC_UART @@ -34,6 +35,7 @@ #define CONFIG_SPI_FLASH #define CONFIG_SPI_FLASH_STMICRO #define CONFIG_SPI_FLASH_SST +#define CONFIG_SPI_NOR_PROTECTION_STM #define CONFIG_MXC_SPI #define CONFIG_SF_DEFAULT_BUS 0 #define CONFIG_SF_DEFAULT_SPEED 20000000
The last 16 KiB of the SPI NOR contain some manufacturing information, which should not be erased/overwritten. Configure the protection bits BP2, BP1 and BP0 so that this region is protected. Signed-off-by: Otavio Salvador <otavio@ossystems.com.br> --- Changes since v1: - Use CONFIG_SPI_NOR_PROTECTION_STM board/congatec/cgtqmx6eval/cgtqmx6eval.c | 25 +++++++++++++++++++++++++ include/configs/cgtqmx6eval.h | 2 ++ 2 files changed, 27 insertions(+)