| Message ID | 20100330133513.20107.72328.stgit@shiryu.yomgui.biz |
|---|---|
| State | New, archived |
| Headers | show |
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/
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/
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.
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.
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/
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/ >
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.
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
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
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.
> 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.
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
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(-)