Patchwork [v2,6/7] mtd: cfi_cmdset_0002: add CFI detection for SST 39VF{16, 32}xx chips

login
register
mail settings
Submitter Guillaume LECERF
Date March 30, 2010, 1:35 p.m.
Message ID <20100330133513.20107.72328.stgit@shiryu.yomgui.biz>
Download mbox | patch
Permalink /patch/48961/
State New
Headers show

Comments

Guillaume LECERF - March 30, 2010, 1:35 p.m.
SST 39VF{16,32}xx chips use the 0x0701 command set, fully compatible
with the AMD one. This patch adds support for detecting them in CFI
mode.

Based on a patch by Peter Turczaki [1].

[1] http://www.mail-archive.com/uclinux-dev@uclinux.org/msg05737.html

Signed-off-by: Guillaume LECERF <glecerf@gmail.com>
---
 drivers/mtd/chips/cfi_cmdset_0002.c |   41 +++++++++++++++++++++++++++++++++++
 drivers/mtd/chips/gen_probe.c       |    1 +
 2 files changed, 42 insertions(+), 0 deletions(-)
Wolfram Sang - April 8, 2010, 9:12 a.m.
On Tue, Mar 30, 2010 at 03:35:13PM +0200, Guillaume LECERF wrote:
> SST 39VF{16,32}xx chips use the 0x0701 command set, fully compatible
> with the AMD one. This patch adds support for detecting them in CFI
> mode.
> 
> Based on a patch by Peter Turczaki [1].
> 
> [1] http://www.mail-archive.com/uclinux-dev@uclinux.org/msg05737.html
> 
> Signed-off-by: Guillaume LECERF <glecerf@gmail.com>
> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c |   41 +++++++++++++++++++++++++++++++++++
>  drivers/mtd/chips/gen_probe.c       |    1 +
>  2 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index de1b4ba..e3e4a94 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -256,6 +256,36 @@ static void fixup_use_atmel_lock(struct mtd_info *mtd, void *param)
>  	mtd->flags |= MTD_POWERUP_LOCK;
>  }
>  
> +/** While reporting a mostly-correct CFI-Information block
> +  * the eraseblock-region information is severely damaged in SST
> +  * parts at least those of the 39VF{16,32,64}xxB series.
> +  **/

Kernel mulit line comments, please.

> +static void fixup_old_sst_eraseregion(struct mtd_info *mtd)
> +{
> +	struct map_info *map = mtd->priv;
> +	struct cfi_private *cfi = map->fldrv_priv;
> +
> +	/** Although the part claims to have two eraseblock-regions
> +	  * these refer to the same region within the flash-array.
> +	  * Because a really CFI-Compliant part may only return

s/Compliant/compliant/?

> +	  * one eraseblock-length per physical memory region
> +	  * we pretend the part said it had just one region ;)
> +	  **/
> +	cfi->cfiq->NumEraseRegions = 1;
> +	cfi->cfiq->EraseRegionInfo[0] = cfi->cfiq->EraseRegionInfo[1];

Why is this last line needed? The comment says they are the same?

> +}
> +
> +static void fixup_sst39vf(struct mtd_info *mtd, void *param)
> +{
> +	struct map_info *map = mtd->priv;
> +	struct cfi_private *cfi = map->fldrv_priv;
> +
> +	fixup_old_sst_eraseregion(mtd);
> +
> +	cfi->addr_unlock1 = 0x5555;
> +	cfi->addr_unlock2 = 0x2AAA;
> +}
> +
>  static void fixup_s29gl064n_sectors(struct mtd_info *mtd, void *param)
>  {
>  	struct map_info *map = mtd->priv;
> @@ -278,6 +308,16 @@ static void fixup_s29gl032n_sectors(struct mtd_info *mtd, void *param)
>  	}
>  }
>  
> +/* Used to fix CFI-Tables of chips without Extended Query Tables
> + */
> +static struct cfi_fixup cfi_nopri_fixup_table[] = {
> +	{ CFI_MFR_SST, 0x234A, fixup_sst39vf, NULL, }, // SST39VF1602
> +	{ CFI_MFR_SST, 0x234B, fixup_sst39vf, NULL, }, // SST39VF1601
> +	{ CFI_MFR_SST, 0x235A, fixup_sst39vf, NULL, }, // SST39VF3202
> +	{ CFI_MFR_SST, 0x235B, fixup_sst39vf, NULL, }, // SST39VF3201
> +	{ 0, 0, NULL, NULL }
> +};
> +
>  static struct cfi_fixup cfi_fixup_table[] = {
>  	{ CFI_MFR_ATMEL, CFI_ID_ANY, fixup_convert_atmel_pri, NULL },
>  #ifdef AMD_BOOTLOC_BUG
> @@ -408,6 +448,7 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
>  			cfi->addr_unlock1 = 0x555;
>  			cfi->addr_unlock2 = 0x2aa;
>  		}
> +		cfi_fixup(mtd, cfi_nopri_fixup_table);
>  
>  		if (!cfi->addr_unlock1 || !cfi->addr_unlock2) {
>  			kfree(mtd);
> diff --git a/drivers/mtd/chips/gen_probe.c b/drivers/mtd/chips/gen_probe.c
> index 991c457..599c259 100644
> --- a/drivers/mtd/chips/gen_probe.c
> +++ b/drivers/mtd/chips/gen_probe.c
> @@ -249,6 +249,7 @@ static struct mtd_info *check_cmd_set(struct map_info *map, int primary)
>  #endif
>  #ifdef CONFIG_MTD_CFI_AMDSTD
>  	case P_ID_AMD_STD:
> +	case P_ID_SST_OLD:
>  		return cfi_cmdset_0002(map, primary);
>  #endif
>  #ifdef CONFIG_MTD_CFI_STAA
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
Wolfram Sang - April 8, 2010, 3:21 p.m.
On Tue, Mar 30, 2010 at 03:35:13PM +0200, Guillaume LECERF wrote:
> SST 39VF{16,32}xx chips use the 0x0701 command set, fully compatible
> with the AMD one. This patch adds support for detecting them in CFI
> mode.
> 
> Based on a patch by Peter Turczaki [1].
> 
> [1] http://www.mail-archive.com/uclinux-dev@uclinux.org/msg05737.html
> 
> Signed-off-by: Guillaume LECERF <glecerf@gmail.com>
> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c |   41 +++++++++++++++++++++++++++++++++++
>  drivers/mtd/chips/gen_probe.c       |    1 +
>  2 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index de1b4ba..e3e4a94 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -256,6 +256,36 @@ static void fixup_use_atmel_lock(struct mtd_info *mtd, void *param)
>  	mtd->flags |= MTD_POWERUP_LOCK;
>  }
>  
> +/** While reporting a mostly-correct CFI-Information block
> +  * the eraseblock-region information is severely damaged in SST
> +  * parts at least those of the 39VF{16,32,64}xxB series.
> +  **/
> +static void fixup_old_sst_eraseregion(struct mtd_info *mtd)
> +{
> +	struct map_info *map = mtd->priv;
> +	struct cfi_private *cfi = map->fldrv_priv;
> +
> +	/** Although the part claims to have two eraseblock-regions
> +	  * these refer to the same region within the flash-array.
> +	  * Because a really CFI-Compliant part may only return
> +	  * one eraseblock-length per physical memory region
> +	  * we pretend the part said it had just one region ;)
> +	  **/
> +	cfi->cfiq->NumEraseRegions = 1;
> +	cfi->cfiq->EraseRegionInfo[0] = cfi->cfiq->EraseRegionInfo[1];
> +}
> +
> +static void fixup_sst39vf(struct mtd_info *mtd, void *param)
> +{
> +	struct map_info *map = mtd->priv;
> +	struct cfi_private *cfi = map->fldrv_priv;
> +
> +	fixup_old_sst_eraseregion(mtd);
> +
> +	cfi->addr_unlock1 = 0x5555;
> +	cfi->addr_unlock2 = 0x2AAA;
> +}
> +
>  static void fixup_s29gl064n_sectors(struct mtd_info *mtd, void *param)
>  {
>  	struct map_info *map = mtd->priv;
> @@ -278,6 +308,16 @@ static void fixup_s29gl032n_sectors(struct mtd_info *mtd, void *param)
>  	}
>  }
>  
> +/* Used to fix CFI-Tables of chips without Extended Query Tables
> + */
> +static struct cfi_fixup cfi_nopri_fixup_table[] = {
> +	{ CFI_MFR_SST, 0x234A, fixup_sst39vf, NULL, }, // SST39VF1602

I got a build error. CFI_MFR_SST is missing.

> +	{ CFI_MFR_SST, 0x234B, fixup_sst39vf, NULL, }, // SST39VF1601
> +	{ CFI_MFR_SST, 0x235A, fixup_sst39vf, NULL, }, // SST39VF3202
> +	{ CFI_MFR_SST, 0x235B, fixup_sst39vf, NULL, }, // SST39VF3201
> +	{ 0, 0, NULL, NULL }
> +};
> +
>  static struct cfi_fixup cfi_fixup_table[] = {
>  	{ CFI_MFR_ATMEL, CFI_ID_ANY, fixup_convert_atmel_pri, NULL },
>  #ifdef AMD_BOOTLOC_BUG
> @@ -408,6 +448,7 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
>  			cfi->addr_unlock1 = 0x555;
>  			cfi->addr_unlock2 = 0x2aa;
>  		}
> +		cfi_fixup(mtd, cfi_nopri_fixup_table);
>  
>  		if (!cfi->addr_unlock1 || !cfi->addr_unlock2) {
>  			kfree(mtd);
> diff --git a/drivers/mtd/chips/gen_probe.c b/drivers/mtd/chips/gen_probe.c
> index 991c457..599c259 100644
> --- a/drivers/mtd/chips/gen_probe.c
> +++ b/drivers/mtd/chips/gen_probe.c
> @@ -249,6 +249,7 @@ static struct mtd_info *check_cmd_set(struct map_info *map, int primary)
>  #endif
>  #ifdef CONFIG_MTD_CFI_AMDSTD
>  	case P_ID_AMD_STD:
> +	case P_ID_SST_OLD:
>  		return cfi_cmdset_0002(map, primary);
>  #endif
>  #ifdef CONFIG_MTD_CFI_STAA
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
Guillaume LECERF - April 8, 2010, 3:32 p.m.
2010/4/8 Wolfram Sang <w.sang@pengutronix.de>:
>> +/* Used to fix CFI-Tables of chips without Extended Query Tables
>> + */
>> +static struct cfi_fixup cfi_nopri_fixup_table[] = {
>> +     { CFI_MFR_SST, 0x234A, fixup_sst39vf, NULL, }, // SST39VF1602
>
> I got a build error. CFI_MFR_SST is missing.

This patch depends on this commit
http://git.infradead.org/mtd-2.6.git/commit/f3e69c6584be2db1ccd5292d6a1d7c566d265701.
Your personnal repo must be outdated.
Wolfram Sang - April 9, 2010, 8:55 a.m.
On Thu, Apr 08, 2010 at 05:32:08PM +0200, Guillaume LECERF wrote:
> 2010/4/8 Wolfram Sang <w.sang@pengutronix.de>:
> >> +/* Used to fix CFI-Tables of chips without Extended Query Tables
> >> + */
> >> +static struct cfi_fixup cfi_nopri_fixup_table[] = {
> >> +     { CFI_MFR_SST, 0x234A, fixup_sst39vf, NULL, }, // SST39VF1602
> >
> > I got a build error. CFI_MFR_SST is missing.
> 
> This patch depends on this commit
> http://git.infradead.org/mtd-2.6.git/commit/f3e69c6584be2db1ccd5292d6a1d7c566d265701.
> Your personnal repo must be outdated.

Thanks. I was unaware that there are pending patches below the v2.6.34-rc2 tag.
Wolfram Sang - April 12, 2010, 2:24 a.m.
On Thu, Apr 08, 2010 at 11:12:15AM +0200, Wolfram Sang wrote:
> On Tue, Mar 30, 2010 at 03:35:13PM +0200, Guillaume LECERF wrote:
> > SST 39VF{16,32}xx chips use the 0x0701 command set, fully compatible
> > with the AMD one. This patch adds support for detecting them in CFI
> > mode.
> > 
> > Based on a patch by Peter Turczaki [1].
> > 
> > [1] http://www.mail-archive.com/uclinux-dev@uclinux.org/msg05737.html
> > 
> > Signed-off-by: Guillaume LECERF <glecerf@gmail.com>
> > ---
> >  drivers/mtd/chips/cfi_cmdset_0002.c |   41 +++++++++++++++++++++++++++++++++++
> >  drivers/mtd/chips/gen_probe.c       |    1 +
> >  2 files changed, 42 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index de1b4ba..e3e4a94 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -256,6 +256,36 @@ static void fixup_use_atmel_lock(struct mtd_info *mtd, void *param)
> >  	mtd->flags |= MTD_POWERUP_LOCK;
> >  }
> >  
> > +/** While reporting a mostly-correct CFI-Information block
> > +  * the eraseblock-region information is severely damaged in SST
> > +  * parts at least those of the 39VF{16,32,64}xxB series.
> > +  **/
> 
> Kernel mulit line comments, please.
> 
> > +static void fixup_old_sst_eraseregion(struct mtd_info *mtd)
> > +{
> > +	struct map_info *map = mtd->priv;
> > +	struct cfi_private *cfi = map->fldrv_priv;
> > +
> > +	/** Although the part claims to have two eraseblock-regions
> > +	  * these refer to the same region within the flash-array.
> > +	  * Because a really CFI-Compliant part may only return
> 
> s/Compliant/compliant/?
> 
> > +	  * one eraseblock-length per physical memory region
> > +	  * we pretend the part said it had just one region ;)
> > +	  **/
> > +	cfi->cfiq->NumEraseRegions = 1;
> > +	cfi->cfiq->EraseRegionInfo[0] = cfi->cfiq->EraseRegionInfo[1];
> 
> Why is this last line needed? The comment says they are the same?

Okay, my remark was rubbish, yet the comment in the source was a bit confusing,
too. It is correct, though maybe the term 'region' is a bit overloaded. What
about replacing both comments with something a bit simpler like this:

/*
 * These flashes report two seperate eraseblock regions based on the
 * sector_erase-size and block_erase-size, although they both operate on the
 * same memory. This is not allowed according to CFI, so we just pick the
 * sector_erase-size.
 */

This is according to the datasheets. You pick the block-data size here
(ususally using command 0x50). Why is that? I tried sector_size on a
SST39WF1601 and it works fine so far, testing with mtd_stresstest. (Sidenote: I
have to use JEDEC-probing though, as my flashes don't report 0x701 but 0x002
(AMD standard) as their primary command set. But they still need their custom
unlock address :( )

> 
> > +}
> > +
> > +static void fixup_sst39vf(struct mtd_info *mtd, void *param)
> > +{
> > +	struct map_info *map = mtd->priv;
> > +	struct cfi_private *cfi = map->fldrv_priv;
> > +
> > +	fixup_old_sst_eraseregion(mtd);
> > +
> > +	cfi->addr_unlock1 = 0x5555;
> > +	cfi->addr_unlock2 = 0x2AAA;
> > +}
> > +
> >  static void fixup_s29gl064n_sectors(struct mtd_info *mtd, void *param)
> >  {
> >  	struct map_info *map = mtd->priv;
> > @@ -278,6 +308,16 @@ static void fixup_s29gl032n_sectors(struct mtd_info *mtd, void *param)
> >  	}
> >  }
> >  
> > +/* Used to fix CFI-Tables of chips without Extended Query Tables
> > + */
> > +static struct cfi_fixup cfi_nopri_fixup_table[] = {
> > +	{ CFI_MFR_SST, 0x234A, fixup_sst39vf, NULL, }, // SST39VF1602
> > +	{ CFI_MFR_SST, 0x234B, fixup_sst39vf, NULL, }, // SST39VF1601
> > +	{ CFI_MFR_SST, 0x235A, fixup_sst39vf, NULL, }, // SST39VF3202
> > +	{ CFI_MFR_SST, 0x235B, fixup_sst39vf, NULL, }, // SST39VF3201
> > +	{ 0, 0, NULL, NULL }
> > +};
> > +
> >  static struct cfi_fixup cfi_fixup_table[] = {
> >  	{ CFI_MFR_ATMEL, CFI_ID_ANY, fixup_convert_atmel_pri, NULL },
> >  #ifdef AMD_BOOTLOC_BUG
> > @@ -408,6 +448,7 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
> >  			cfi->addr_unlock1 = 0x555;
> >  			cfi->addr_unlock2 = 0x2aa;
> >  		}
> > +		cfi_fixup(mtd, cfi_nopri_fixup_table);
> >  
> >  		if (!cfi->addr_unlock1 || !cfi->addr_unlock2) {
> >  			kfree(mtd);
> > diff --git a/drivers/mtd/chips/gen_probe.c b/drivers/mtd/chips/gen_probe.c
> > index 991c457..599c259 100644
> > --- a/drivers/mtd/chips/gen_probe.c
> > +++ b/drivers/mtd/chips/gen_probe.c
> > @@ -249,6 +249,7 @@ static struct mtd_info *check_cmd_set(struct map_info *map, int primary)
> >  #endif
> >  #ifdef CONFIG_MTD_CFI_AMDSTD
> >  	case P_ID_AMD_STD:
> > +	case P_ID_SST_OLD:
> >  		return cfi_cmdset_0002(map, primary);
> >  #endif
> >  #ifdef CONFIG_MTD_CFI_STAA
> > 
> > 
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |



> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
Fabio Giovagnini - April 12, 2010, 4:28 p.m.
Hi guys,
I'm very interested to use such a patch; which kernel version Do I need to use 
for appling it successfully?

Regards


In data lunedì 12 aprile 2010 04:24:58, Wolfram Sang ha scritto:
: > On Thu, Apr 08, 2010 at 11:12:15AM +0200, Wolfram Sang wrote:
> > On Tue, Mar 30, 2010 at 03:35:13PM +0200, Guillaume LECERF wrote:
> > > SST 39VF{16,32}xx chips use the 0x0701 command set, fully compatible
> > > with the AMD one. This patch adds support for detecting them in CFI
> > > mode.
> > >
> > > Based on a patch by Peter Turczaki [1].
> > >
> > > [1] http://www.mail-archive.com/uclinux-dev@uclinux.org/msg05737.html
> > >
> > > Signed-off-by: Guillaume LECERF <glecerf@gmail.com>
> > > ---
> > >  drivers/mtd/chips/cfi_cmdset_0002.c |   41
> > > +++++++++++++++++++++++++++++++++++ drivers/mtd/chips/gen_probe.c      
> > > |    1 +
> > >  2 files changed, 42 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> > > b/drivers/mtd/chips/cfi_cmdset_0002.c index de1b4ba..e3e4a94 100644
> > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > > @@ -256,6 +256,36 @@ static void fixup_use_atmel_lock(struct mtd_info
> > > *mtd, void *param) mtd->flags |= MTD_POWERUP_LOCK;
> > >  }
> > >
> > > +/** While reporting a mostly-correct CFI-Information block
> > > +  * the eraseblock-region information is severely damaged in SST
> > > +  * parts at least those of the 39VF{16,32,64}xxB series.
> > > +  **/
> >
> > Kernel mulit line comments, please.
> >
> > > +static void fixup_old_sst_eraseregion(struct mtd_info *mtd)
> > > +{
> > > +	struct map_info *map = mtd->priv;
> > > +	struct cfi_private *cfi = map->fldrv_priv;
> > > +
> > > +	/** Although the part claims to have two eraseblock-regions
> > > +	  * these refer to the same region within the flash-array.
> > > +	  * Because a really CFI-Compliant part may only return
> >
> > s/Compliant/compliant/?
> >
> > > +	  * one eraseblock-length per physical memory region
> > > +	  * we pretend the part said it had just one region ;)
> > > +	  **/
> > > +	cfi->cfiq->NumEraseRegions = 1;
> > > +	cfi->cfiq->EraseRegionInfo[0] = cfi->cfiq->EraseRegionInfo[1];
> >
> > Why is this last line needed? The comment says they are the same?
> 
> Okay, my remark was rubbish, yet the comment in the source was a bit
>  confusing, too. It is correct, though maybe the term 'region' is a bit
>  overloaded. What about replacing both comments with something a bit
>  simpler like this:
> 
> /*
>  * These flashes report two seperate eraseblock regions based on the
>  * sector_erase-size and block_erase-size, although they both operate on
>  the * same memory. This is not allowed according to CFI, so we just pick
>  the * sector_erase-size.
>  */
> 
> This is according to the datasheets. You pick the block-data size here
> (ususally using command 0x50). Why is that? I tried sector_size on a
> SST39WF1601 and it works fine so far, testing with mtd_stresstest.
>  (Sidenote: I have to use JEDEC-probing though, as my flashes don't report
>  0x701 but 0x002 (AMD standard) as their primary command set. But they
>  still need their custom unlock address :( )
> 
> > > +}
> > > +
> > > +static void fixup_sst39vf(struct mtd_info *mtd, void *param)
> > > +{
> > > +	struct map_info *map = mtd->priv;
> > > +	struct cfi_private *cfi = map->fldrv_priv;
> > > +
> > > +	fixup_old_sst_eraseregion(mtd);
> > > +
> > > +	cfi->addr_unlock1 = 0x5555;
> > > +	cfi->addr_unlock2 = 0x2AAA;
> > > +}
> > > +
> > >  static void fixup_s29gl064n_sectors(struct mtd_info *mtd, void *param)
> > >  {
> > >  	struct map_info *map = mtd->priv;
> > > @@ -278,6 +308,16 @@ static void fixup_s29gl032n_sectors(struct
> > > mtd_info *mtd, void *param) }
> > >  }
> > >
> > > +/* Used to fix CFI-Tables of chips without Extended Query Tables
> > > + */
> > > +static struct cfi_fixup cfi_nopri_fixup_table[] = {
> > > +	{ CFI_MFR_SST, 0x234A, fixup_sst39vf, NULL, }, // SST39VF1602
> > > +	{ CFI_MFR_SST, 0x234B, fixup_sst39vf, NULL, }, // SST39VF1601
> > > +	{ CFI_MFR_SST, 0x235A, fixup_sst39vf, NULL, }, // SST39VF3202
> > > +	{ CFI_MFR_SST, 0x235B, fixup_sst39vf, NULL, }, // SST39VF3201
> > > +	{ 0, 0, NULL, NULL }
> > > +};
> > > +
> > >  static struct cfi_fixup cfi_fixup_table[] = {
> > >  	{ CFI_MFR_ATMEL, CFI_ID_ANY, fixup_convert_atmel_pri, NULL },
> > >  #ifdef AMD_BOOTLOC_BUG
> > > @@ -408,6 +448,7 @@ struct mtd_info *cfi_cmdset_0002(struct map_info
> > > *map, int primary) cfi->addr_unlock1 = 0x555;
> > >  			cfi->addr_unlock2 = 0x2aa;
> > >  		}
> > > +		cfi_fixup(mtd, cfi_nopri_fixup_table);
> > >
> > >  		if (!cfi->addr_unlock1 || !cfi->addr_unlock2) {
> > >  			kfree(mtd);
> > > diff --git a/drivers/mtd/chips/gen_probe.c
> > > b/drivers/mtd/chips/gen_probe.c index 991c457..599c259 100644
> > > --- a/drivers/mtd/chips/gen_probe.c
> > > +++ b/drivers/mtd/chips/gen_probe.c
> > > @@ -249,6 +249,7 @@ static struct mtd_info *check_cmd_set(struct
> > > map_info *map, int primary) #endif
> > >  #ifdef CONFIG_MTD_CFI_AMDSTD
> > >  	case P_ID_AMD_STD:
> > > +	case P_ID_SST_OLD:
> > >  		return cfi_cmdset_0002(map, primary);
> > >  #endif
> > >  #ifdef CONFIG_MTD_CFI_STAA
> > >
> > >
> > > ______________________________________________________
> > > Linux MTD discussion mailing list
> > > http://lists.infradead.org/mailman/listinfo/linux-mtd/
> >
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
Wolfram Sang - April 13, 2010, 12:27 a.m.
On Mon, Apr 12, 2010 at 06:28:38PM +0200, Fabio Giovagnini wrote:

> I'm very interested to use such a patch; which kernel version Do I need to use 
> for appling it successfully?

I tried the mtd-master-tree and 2.6.33.2 with commit
f3e69c6584be2db1ccd5292d6a1d7c566d265701. Worked with both.
Guillaume LECERF - April 16, 2010, 10:17 a.m.
2010/4/12 Wolfram Sang <w.sang@pengutronix.de>:
>> > +     * one eraseblock-length per physical memory region
>> > +     * we pretend the part said it had just one region ;)
>> > +     **/
>> > +   cfi->cfiq->NumEraseRegions = 1;
>> > +   cfi->cfiq->EraseRegionInfo[0] = cfi->cfiq->EraseRegionInfo[1];
>>
>> Why is this last line needed? The comment says they are the same?
>
> Okay, my remark was rubbish, yet the comment in the source was a bit confusing,
> too. It is correct, though maybe the term 'region' is a bit overloaded. What
> about replacing both comments with something a bit simpler like this:
>
> /*
>  * These flashes report two seperate eraseblock regions based on the
>  * sector_erase-size and block_erase-size, although they both operate on the
>  * same memory. This is not allowed according to CFI, so we just pick the
>  * sector_erase-size.
>  */
>
> This is according to the datasheets. You pick the block-data size here
> (ususally using command 0x50). Why is that? I tried sector_size on a
> SST39WF1601 and it works fine so far, testing with mtd_stresstest. (Sidenote: I
> have to use JEDEC-probing though, as my flashes don't report 0x701 but 0x002
> (AMD standard) as their primary command set. But they still need their custom
> unlock address :( )

The 39VF3201 chip I use is on a brcm63xx board, running OpenWRT. It
reports 2 erase regions (as indicated in the datasheet):

Number of Erase Block Regions: 2
  Erase Region #0: BlockSize 0x1000 bytes, 1024 blocks
  Erase Region #1: BlockSize 0x10000 bytes, 64 blocks

But the function parse_cfe_partitions() in
drivers/mtd/maps/bcm963xx-flash.c [1] tries to read the partition
table at $(master->erasesize) (cf. line 67).
If I use the sector_erase-size, parse_cfe_partitions() tries to read
at 0x1000, and fails because the partition table is actually at
0x10000 on my flash.


[1] https://dev.openwrt.org/browser/trunk/target/linux/brcm63xx/patches-2.6.32/040-bcm963xx_flashmap.patch
Wolfram Sang - April 18, 2010, 11:29 p.m.
Hi Guillaume,

> > Okay, my remark was rubbish, yet the comment in the source was a bit confusing,
> > too. It is correct, though maybe the term 'region' is a bit overloaded. What
> > about replacing both comments with something a bit simpler like this:
> >
> > /*
> >  * These flashes report two seperate eraseblock regions based on the
> >  * sector_erase-size and block_erase-size, although they both operate on the
> >  * same memory. This is not allowed according to CFI, so we just pick the
> >  * sector_erase-size.
> >  */
> >

Any opinion about this change?

> > This is according to the datasheets. You pick the block-data size here
> > (ususally using command 0x50). Why is that? I tried sector_size on a
> > SST39WF1601 and it works fine so far, testing with mtd_stresstest. (Sidenote: I
> > have to use JEDEC-probing though, as my flashes don't report 0x701 but 0x002
> > (AMD standard) as their primary command set. But they still need their custom
> > unlock address :( )
> 
> The 39VF3201 chip I use is on a brcm63xx board, running OpenWRT. It
> reports 2 erase regions (as indicated in the datasheet):
> 
> Number of Erase Block Regions: 2
>   Erase Region #0: BlockSize 0x1000 bytes, 1024 blocks
>   Erase Region #1: BlockSize 0x10000 bytes, 64 blocks

Yes, same here.

> But the function parse_cfe_partitions() in
> drivers/mtd/maps/bcm963xx-flash.c [1] tries to read the partition
> table at $(master->erasesize) (cf. line 67).
> If I use the sector_erase-size, parse_cfe_partitions() tries to read
> at 0x1000, and fails because the partition table is actually at
> 0x10000 on my flash.

I see the problem, but think it should be fixed differently (in the driver?).
cfi_cmdset_0002.c uses CMD 0x30 for erasing (line 1604 in mtd/master). The SST
datasheet now tells that the chip then expects the sector address (table 6 or
figure 11). Given that I wonder if erasing truly works unless I missed
something (at least it fails with mtd_stresstest for me if I change my
jedec_probe to use 0x10000 instead of 0x1000).

Kind regards,

   Wolfram
Guillaume LECERF - April 20, 2010, 9:44 a.m.
Hi Wolfram,

>> > Okay, my remark was rubbish, yet the comment in the source was a bit confusing,
>> > too. It is correct, though maybe the term 'region' is a bit overloaded. What
>> > about replacing both comments with something a bit simpler like this:
>> >
>> > /*
>> >  * These flashes report two seperate eraseblock regions based on the
>> >  * sector_erase-size and block_erase-size, although they both operate on the
>> >  * same memory. This is not allowed according to CFI, so we just pick the
>> >  * sector_erase-size.
>> >  */
>> >
>
> Any opinion about this change?

I'm of course ok with this change, I integrated it in my patch.

> > This is according to the datasheets. You pick the block-data size here
>> > (ususally using command 0x50). Why is that? I tried sector_size on a
>> > SST39WF1601 and it works fine so far, testing with mtd_stresstest. (Sidenote: I
>> > have to use JEDEC-probing though, as my flashes don't report 0x701 but 0x002
>> > (AMD standard) as their primary command set. But they still need their custom
>> > unlock address :( )
>>
>> The 39VF3201 chip I use is on a brcm63xx board, running OpenWRT. It
>> reports 2 erase regions (as indicated in the datasheet):
>>
>> Number of Erase Block Regions: 2
>>   Erase Region #0: BlockSize 0x1000 bytes, 1024 blocks
>>   Erase Region #1: BlockSize 0x10000 bytes, 64 blocks
>
> Yes, same here.
>
>> But the function parse_cfe_partitions() in
>> drivers/mtd/maps/bcm963xx-flash.c [1] tries to read the partition
>> table at $(master->erasesize) (cf. line 67).
>> If I use the sector_erase-size, parse_cfe_partitions() tries to read
>> at 0x1000, and fails because the partition table is actually at
>> 0x10000 on my flash.
>
> I see the problem, but think it should be fixed differently (in the driver?).
> cfi_cmdset_0002.c uses CMD 0x30 for erasing (line 1604 in mtd/master). The SST
> datasheet now tells that the chip then expects the sector address (table 6 or
> figure 11). Given that I wonder if erasing truly works unless I missed
> something (at least it fails with mtd_stresstest for me if I change my
> jedec_probe to use 0x10000 instead of 0x1000).

Ok, you were right, the problem is in the driver.
I tried to use the hardcoded value 0x10000 instead of mtd->erasesize,
and use the sector_erase-size.
mtd_stresstest now runs smoothly, so I need to ask bcm963xx-flash.c
authors why they use this variable.

I continue my tests and discussions with the bcm963xx-flash.c authors
and get back when I've got a clean solution.

Thanks for your investigations !

Regards.
Wolfram Sang - April 22, 2010, 6:13 a.m.
> I continue my tests and discussions with the bcm963xx-flash.c authors
> and get back when I've got a clean solution.
> 
> Thanks for your investigations !

You are welcome. Please CC me on the next round, so I can add my tags.

Patch

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index de1b4ba..e3e4a94 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -256,6 +256,36 @@  static void fixup_use_atmel_lock(struct mtd_info *mtd, void *param)
 	mtd->flags |= MTD_POWERUP_LOCK;
 }
 
+/** While reporting a mostly-correct CFI-Information block
+  * the eraseblock-region information is severely damaged in SST
+  * parts at least those of the 39VF{16,32,64}xxB series.
+  **/
+static void fixup_old_sst_eraseregion(struct mtd_info *mtd)
+{
+	struct map_info *map = mtd->priv;
+	struct cfi_private *cfi = map->fldrv_priv;
+
+	/** Although the part claims to have two eraseblock-regions
+	  * these refer to the same region within the flash-array.
+	  * Because a really CFI-Compliant part may only return
+	  * one eraseblock-length per physical memory region
+	  * we pretend the part said it had just one region ;)
+	  **/
+	cfi->cfiq->NumEraseRegions = 1;
+	cfi->cfiq->EraseRegionInfo[0] = cfi->cfiq->EraseRegionInfo[1];
+}
+
+static void fixup_sst39vf(struct mtd_info *mtd, void *param)
+{
+	struct map_info *map = mtd->priv;
+	struct cfi_private *cfi = map->fldrv_priv;
+
+	fixup_old_sst_eraseregion(mtd);
+
+	cfi->addr_unlock1 = 0x5555;
+	cfi->addr_unlock2 = 0x2AAA;
+}
+
 static void fixup_s29gl064n_sectors(struct mtd_info *mtd, void *param)
 {
 	struct map_info *map = mtd->priv;
@@ -278,6 +308,16 @@  static void fixup_s29gl032n_sectors(struct mtd_info *mtd, void *param)
 	}
 }
 
+/* Used to fix CFI-Tables of chips without Extended Query Tables
+ */
+static struct cfi_fixup cfi_nopri_fixup_table[] = {
+	{ CFI_MFR_SST, 0x234A, fixup_sst39vf, NULL, }, // SST39VF1602
+	{ CFI_MFR_SST, 0x234B, fixup_sst39vf, NULL, }, // SST39VF1601
+	{ CFI_MFR_SST, 0x235A, fixup_sst39vf, NULL, }, // SST39VF3202
+	{ CFI_MFR_SST, 0x235B, fixup_sst39vf, NULL, }, // SST39VF3201
+	{ 0, 0, NULL, NULL }
+};
+
 static struct cfi_fixup cfi_fixup_table[] = {
 	{ CFI_MFR_ATMEL, CFI_ID_ANY, fixup_convert_atmel_pri, NULL },
 #ifdef AMD_BOOTLOC_BUG
@@ -408,6 +448,7 @@  struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 			cfi->addr_unlock1 = 0x555;
 			cfi->addr_unlock2 = 0x2aa;
 		}
+		cfi_fixup(mtd, cfi_nopri_fixup_table);
 
 		if (!cfi->addr_unlock1 || !cfi->addr_unlock2) {
 			kfree(mtd);
diff --git a/drivers/mtd/chips/gen_probe.c b/drivers/mtd/chips/gen_probe.c
index 991c457..599c259 100644
--- a/drivers/mtd/chips/gen_probe.c
+++ b/drivers/mtd/chips/gen_probe.c
@@ -249,6 +249,7 @@  static struct mtd_info *check_cmd_set(struct map_info *map, int primary)
 #endif
 #ifdef CONFIG_MTD_CFI_AMDSTD
 	case P_ID_AMD_STD:
+	case P_ID_SST_OLD:
 		return cfi_cmdset_0002(map, primary);
 #endif
 #ifdef CONFIG_MTD_CFI_STAA