Re: [PATCH v8 1/5] mtd: cfi_cmdset_0002: Add support for polling status register
From: Tokunori Ikegami
Date: Tue Jun 25 2019 - 13:01:29 EST
Hi,
Thanks for the fix.
Reviewed-by: Tokunori Ikegami <ikegami.t@xxxxxxxxx>
I have just tested the patch quickly on my local environment that uses
the cfi_cmdset_0002 flash device but not HyperFlash family.
So tested as not affected by the change.
On 2019/06/25 16:57, Vignesh Raghavendra wrote:
HyperFlash devices are compliant with CFI AMD/Fujitsu Extended Command
Set (0x0002) for flash operations, therefore
drivers/mtd/chips/cfi_cmdset_0002.c can be used as is. But these devices
do not support DQ polling method of determining chip ready/good status.
These flashes provide Status Register whose bits can be polled to know
status of flash operation.
Cypress HyperFlash datasheet here[1], talks about CFI Amd/Fujitsu
Extended Query version 1.5. Bit 0 of "Software Features supported" field
of CFI Primary Vendor-Specific Extended Query table indicates
presence/absence of status register and Bit 1 indicates whether or not
DQ polling is supported. Using these bits, its possible to determine
whether flash supports DQ polling or need to use Status Register.
Add support for polling Status Register to know device ready/status of
erase/write operations when DQ polling is not supported.
Print error messages on erase/program failure by looking at related
Status Register bits.
[1] https://www.cypress.com/file/213346/download
Signed-off-by: Vignesh Raghavendra <vigneshr@xxxxxx>
---
v8:
Fix up status register polling to support banked flashes in patch 1/5.
drivers/mtd/chips/cfi_cmdset_0002.c | 130 ++++++++++++++++++++++++----
include/linux/mtd/cfi.h | 7 ++
2 files changed, 120 insertions(+), 17 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index c8fa5906bdf9..09f8aaf1e763 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -49,6 +49,16 @@
#define SST49LF008A 0x005a
#define AT49BV6416 0x00d6
+/*
+ * Status Register bit description. Used by flash devices that don't
+ * support DQ polling (e.g. HyperFlash)
+ */
+#define CFI_SR_DRB BIT(7)
+#define CFI_SR_ESB BIT(5)
+#define CFI_SR_PSB BIT(4)
+#define CFI_SR_WBASB BIT(3)
+#define CFI_SR_SLSB BIT(1)
+
static int cfi_amdstd_read (struct mtd_info *, loff_t, size_t, size_t *, u_char *);
static int cfi_amdstd_write_words(struct mtd_info *, loff_t, size_t, size_t *, const u_char *);
static int cfi_amdstd_write_buffers(struct mtd_info *, loff_t, size_t, size_t *, const u_char *);
@@ -97,6 +107,50 @@ static struct mtd_chip_driver cfi_amdstd_chipdrv = {
.module = THIS_MODULE
};
+/*
+ * Use status register to poll for Erase/write completion when DQ is not
+ * supported. This is indicated by Bit[1:0] of SoftwareFeatures field in
+ * CFI Primary Vendor-Specific Extended Query table 1.5
+ */
+static int cfi_use_status_reg(struct cfi_private *cfi)
+{
+ struct cfi_pri_amdstd *extp = cfi->cmdset_priv;
+ u8 poll_mask = CFI_POLL_STATUS_REG | CFI_POLL_DQ;
+
+ return extp->MinorVersion >= '5' &&
+ (extp->SoftwareFeatures & poll_mask) == CFI_POLL_STATUS_REG;
+}
+
+static void cfi_check_err_status(struct map_info *map, struct flchip *chip,
Just a minor comment.
I though that it is better to be called this function in chip_good()
instead of to be called separately.
But I can understand that the chip_good() is called repeatedly in
do_erase_chip(), do_erase_oneblock() and do_write_buffer() until timeout.
Also do_write_oneword() is also possible to be changed to call the
chip_good() repeatedly instead of chip_ready().
So additional logics is needed to avoid the repeated error messages in
the function above.
Anyway it is okay for the current implementation to me.
Regards,
Ikegami
+ unsigned long adr)
+{
+ struct cfi_private *cfi = map->fldrv_priv;
+ map_word status;
+
+ if (!cfi_use_status_reg(cfi))
+ return;
+
+ cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
+ cfi->device_type, NULL);
+ status = map_read(map, adr);
+
+ if (map_word_bitsset(map, status, CMD(0x3a))) {
+ unsigned long chipstatus = MERGESTATUS(status);
+
+ if (chipstatus & CFI_SR_ESB)
+ pr_err("%s erase operation failed, status %lx\n",
+ map->name, chipstatus);
+ if (chipstatus & CFI_SR_PSB)
+ pr_err("%s program operation failed, status %lx\n",
+ map->name, chipstatus);
+ if (chipstatus & CFI_SR_WBASB)
+ pr_err("%s buffer program command aborted, status %lx\n",
+ map->name, chipstatus);
+ if (chipstatus & CFI_SR_SLSB)
+ pr_err("%s sector write protected, status %lx\n",
+ map->name, chipstatus);
+ }
+}
/* #define DEBUG_CFI_FEATURES */
@@ -742,10 +796,25 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
* correctly and is therefore not done (particularly with interleaved chips
* as each chip must be checked independently of the others).
*/
-static int __xipram chip_ready(struct map_info *map, unsigned long addr)
+static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
+ unsigned long addr)
{
+ struct cfi_private *cfi = map->fldrv_priv;
map_word d, t;
+ if (cfi_use_status_reg(cfi)) {
+ map_word ready = CMD(CFI_SR_DRB);
+ /*
+ * For chips that support status register, check device
+ * ready bit
+ */
+ cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
+ cfi->device_type, NULL);
+ d = map_read(map, addr);
+
+ return map_word_andequal(map, d, ready, ready);
+ }
+
d = map_read(map, addr);
t = map_read(map, addr);
@@ -767,10 +836,30 @@ static int __xipram chip_ready(struct map_info *map, unsigned long addr)
* as each chip must be checked independently of the others).
*
*/
-static int __xipram chip_good(struct map_info *map, unsigned long addr, map_word expected)
+static int __xipram chip_good(struct map_info *map, struct flchip *chip,
+ unsigned long addr, map_word expected)
{
+ struct cfi_private *cfi = map->fldrv_priv;
map_word oldd, curd;
+ if (cfi_use_status_reg(cfi)) {
+ map_word ready = CMD(CFI_SR_DRB);
+ map_word err = CMD(CFI_SR_PSB | CFI_SR_ESB);
+ /*
+ * For chips that support status register, check device
+ * ready bit and Erase/Program status bit to know if
+ * operation succeeded.
+ */
+ cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
+ cfi->device_type, NULL);
+ curd = map_read(map, addr);
+
+ if (map_word_andequal(map, curd, ready, ready))
+ return !map_word_bitsset(map, curd, err);
+
+ return 0;
+ }
+
oldd = map_read(map, addr);
curd = map_read(map, addr);
@@ -792,7 +881,7 @@ static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr
case FL_STATUS:
for (;;) {
- if (chip_ready(map, adr))
+ if (chip_ready(map, chip, adr))
break;
if (time_after(jiffies, timeo)) {
@@ -830,7 +919,7 @@ static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr
chip->state = FL_ERASE_SUSPENDING;
chip->erase_suspended = 1;
for (;;) {
- if (chip_ready(map, adr))
+ if (chip_ready(map, chip, adr))
break;
if (time_after(jiffies, timeo)) {
@@ -1362,7 +1451,7 @@ static int do_otp_lock(struct map_info *map, struct flchip *chip, loff_t adr,
/* wait for chip to become ready */
timeo = jiffies + msecs_to_jiffies(2);
for (;;) {
- if (chip_ready(map, adr))
+ if (chip_ready(map, chip, adr))
break;
if (time_after(jiffies, timeo)) {
@@ -1628,22 +1717,24 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
continue;
}
- if (time_after(jiffies, timeo) && !chip_ready(map, adr)){
+ if (time_after(jiffies, timeo) &&
+ !chip_ready(map, chip, adr)) {
xip_enable(map, chip, adr);
printk(KERN_WARNING "MTD %s(): software timeout\n", __func__);
xip_disable(map, chip, adr);
break;
}
- if (chip_ready(map, adr))
+ if (chip_ready(map, chip, adr))
break;
/* Latency issues. Drop the lock, wait a while and retry */
UDELAY(map, chip, adr, 1);
}
/* Did we succeed? */
- if (!chip_good(map, adr, datum)) {
+ if (!chip_good(map, chip, adr, datum)) {
/* reset on all failures. */
+ cfi_check_err_status(map, chip, adr);
map_write(map, CMD(0xF0), chip->start);
/* FIXME - should have reset delay before continuing */
@@ -1881,10 +1972,11 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
* We check "time_after" and "!chip_good" before checking "chip_good" to avoid
* the failure due to scheduling.
*/
- if (time_after(jiffies, timeo) && !chip_good(map, adr, datum))
+ if (time_after(jiffies, timeo) &&
+ !chip_good(map, chip, adr, datum))
break;
- if (chip_good(map, adr, datum)) {
+ if (chip_good(map, chip, adr, datum)) {
xip_enable(map, chip, adr);
goto op_done;
}
@@ -1901,6 +1993,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
* See e.g.
* http://www.spansion.com/Support/Application%20Notes/MirrorBit_Write_Buffer_Prog_Page_Buffer_Read_AN.pdf
*/
+ cfi_check_err_status(map, chip, adr);
cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
cfi->device_type, NULL);
cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
@@ -2018,7 +2111,7 @@ static int cfi_amdstd_panic_wait(struct map_info *map, struct flchip *chip,
* If the driver thinks the chip is idle, and no toggle bits
* are changing, then the chip is actually idle for sure.
*/
- if (chip->state == FL_READY && chip_ready(map, adr))
+ if (chip->state == FL_READY && chip_ready(map, chip, adr))
return 0;
/*
@@ -2035,7 +2128,7 @@ static int cfi_amdstd_panic_wait(struct map_info *map, struct flchip *chip,
/* wait for the chip to become ready */
for (i = 0; i < jiffies_to_usecs(timeo); i++) {
- if (chip_ready(map, adr))
+ if (chip_ready(map, chip, adr))
return 0;
udelay(1);
@@ -2099,14 +2192,15 @@ static int do_panic_write_oneword(struct map_info *map, struct flchip *chip,
map_write(map, datum, adr);
for (i = 0; i < jiffies_to_usecs(uWriteTimeout); i++) {
- if (chip_ready(map, adr))
+ if (chip_ready(map, chip, adr))
break;
udelay(1);
}
- if (!chip_good(map, adr, datum)) {
+ if (!chip_good(map, chip, adr, datum)) {
/* reset on all failures. */
+ cfi_check_err_status(map, chip, adr);
map_write(map, CMD(0xF0), chip->start);
/* FIXME - should have reset delay before continuing */
@@ -2300,7 +2394,7 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip)
chip->erase_suspended = 0;
}
- if (chip_good(map, adr, map_word_ff(map)))
+ if (chip_good(map, chip, adr, map_word_ff(map)))
break;
if (time_after(jiffies, timeo)) {
@@ -2316,6 +2410,7 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip)
/* Did we succeed? */
if (ret) {
/* reset on all failures. */
+ cfi_check_err_status(map, chip, adr);
map_write(map, CMD(0xF0), chip->start);
/* FIXME - should have reset delay before continuing */
@@ -2396,7 +2491,7 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
chip->erase_suspended = 0;
}
- if (chip_good(map, adr, map_word_ff(map)))
+ if (chip_good(map, chip, adr, map_word_ff(map)))
break;
if (time_after(jiffies, timeo)) {
@@ -2412,6 +2507,7 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
/* Did we succeed? */
if (ret) {
/* reset on all failures. */
+ cfi_check_err_status(map, chip, adr);
map_write(map, CMD(0xF0), chip->start);
/* FIXME - should have reset delay before continuing */
@@ -2589,7 +2685,7 @@ static int __maybe_unused do_ppb_xxlock(struct map_info *map,
*/
timeo = jiffies + msecs_to_jiffies(2000); /* 2s max (un)locking */
for (;;) {
- if (chip_ready(map, adr))
+ if (chip_ready(map, chip, adr))
break;
if (time_after(jiffies, timeo)) {
diff --git a/include/linux/mtd/cfi.h b/include/linux/mtd/cfi.h
index 208c87cf2e3e..c98a21108688 100644
--- a/include/linux/mtd/cfi.h
+++ b/include/linux/mtd/cfi.h
@@ -219,6 +219,13 @@ struct cfi_pri_amdstd {
uint8_t VppMin;
uint8_t VppMax;
uint8_t TopBottom;
+ /* Below field are added from version 1.5 */
+ uint8_t ProgramSuspend;
+ uint8_t UnlockBypass;
+ uint8_t SecureSiliconSector;
+ uint8_t SoftwareFeatures;
+#define CFI_POLL_STATUS_REG BIT(0)
+#define CFI_POLL_DQ BIT(1)
} __packed;
/* Vendor-Specific PRI for Atmel chips (command set 0x0002) */