Re: [BUG] mtd: cfi_cmdset_0002: write regression since v4.17-rc1

From: Tokunori Ikegami
Date: Sun Mar 06 2022 - 10:54:23 EST

Next message: Sedat Dilek: "Re: [Linux-v5.17-rc6] Building tools/perf with perl v5.34"
Previous message: Thorsten Leemhuis: "Re: [REGRESSION] Too-low frequency limit for AMD GPU PCI-passed-through to Windows VM"
In reply to: Ahmad Fatoum: "Re: [BUG] mtd: cfi_cmdset_0002: write regression since v4.17-rc1"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 2022/03/04 20:11, Ahmad Fatoum wrote:

Hello Tokunori-san,

On 20.02.22 13:22, Tokunori Ikegami wrote:

Hi Ahmad-san,

Could you please try the version 2 patch attached for the error case?
This version is to check the DQ true data 0xFF by chip_good().

I had a similar patch locally as well at first. I just tested yours
and I can't reproduce the issue.

Thanks for your support.
Sorry if possible could you please retest the attached the patch again since this fixed the version 1 patch maintainer review comments?

But I am not sure if this works or not since the error is possible to be caused by Hi-Z 0xff on floating bus or etc.

That it works for me could be because of Hi-Z 0xff, which is why
decided against it.

I see.

What seems to work for me is checking if chip_good or chip_ready
and map_word is equal to 0xFF. I can't justify why this is ok though.
(Worst case bus is floating at this point of time and Hi-Z is read
as 0xff on CPU data lines...)

Sorry I am not sure about this.
I thought the chip_ready() itself is correct as implemented as the data sheet in the past.
But it did not work correctly so changed to use chip_good() instead as it is also correct.

What exactly in the datasheet makes you believe chip_good is not appropriate?

I just mentioned about the actual issue behaviors as not worked chip_good() on S29GL964N and not worked chip_ready() on MX29GL512FHT2I-11G before etc.
Anyway let me recheck the data sheet details as just checked it again quickly but needed more investigation to understand.

As far as I checked still both chip_good() and chip_ready() seem correct but still the root cause is unknown.
If as you mentioned the issue was cased by the DQ true data 0xFF I am not sure why the read work without any error after the write operation.
Also if the error was caused by the Hi-Z 0xff on floating bus as mentioned I am not sure why the read work without any error after the write operation with chip_ready().
Sorry anyway the root cause is also unknown when the write operation was changed to use chip_good() instead of chip_ready().

I've be ok with v1 then. Restores working behavior for me and shouldn't break others.

Noted but still I am thinking the version 2 patch to check 0xff seems better than to use chip_ready() so let me consider this again later.

Regards,
Ikegami

Cheers and thanks again,
Ahmad

Regards,
Ikegami

Regards,
Ikegami

Cheers,
Ahmad

From 306f7266cb2b6d07bbc5882b3b977264483ad128 Mon Sep 17 00:00:00 2001
From: Tokunori Ikegami <ikegami.t@xxxxxxxxx>
Date: Mon, 14 Feb 2022 01:08:02 +0900
Subject: [PATCH v2] mtd: cfi_cmdset_0002: Use chip_ready() for write on
S29GL064N

The regression issue has been caused on S29GL064N and reported it.
Also the change mentioned is to use chip_good() for buffered write.
So disable the change on S29GL064N and use chip_ready() as before.

Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
Signed-off-by: Tokunori Ikegami <ikegami.t@xxxxxxxxx>
Cc: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
Cc: Richard Weinberger <richard@xxxxxx>
Cc: Vignesh Raghavendra <vigneshr@xxxxxx>
Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx
Cc: stable@xxxxxxxxxxxxxxx
Link: https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@xxxxxxxxxxxxxx/
---
drivers/mtd/chips/cfi_cmdset_0002.c | 89 +++++++++++++++--------------
1 file changed, 47 insertions(+), 42 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index a761134fd3be..5e14b60e8638 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -48,6 +48,7 @@
#define SST49LF040B 0x0050
#define SST49LF008A 0x005a
#define AT49BV6416 0x00d6
+#define S29GL064N_MN12 0x0c01

/*
* Status Register bit description. Used by flash devices that don't
@@ -462,7 +463,7 @@ static struct cfi_fixup cfi_fixup_table[] = {
{ CFI_MFR_AMD, 0x0056, fixup_use_secsi },
{ CFI_MFR_AMD, 0x005C, fixup_use_secsi },
{ CFI_MFR_AMD, 0x005F, fixup_use_secsi },
- { CFI_MFR_AMD, 0x0c01, fixup_s29gl064n_sectors },
+ { CFI_MFR_AMD, S29GL064N_MN12, fixup_s29gl064n_sectors },
{ CFI_MFR_AMD, 0x1301, fixup_s29gl064n_sectors },
{ CFI_MFR_AMD, 0x1a00, fixup_s29gl032n_sectors },
{ CFI_MFR_AMD, 0x1a01, fixup_s29gl032n_sectors },
@@ -801,22 +802,12 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
return NULL;
}

-/*
- * Return true if the chip is ready.
- *
- * Ready is one of: read mode, query mode, erase-suspend-read mode (in any
- * non-suspended sector) and is indicated by no toggle bits toggling.
- *
- * Note that anything more complicated than checking if no bits are toggling
- * (including checking DQ5 for an error status) is tricky to get working
- * 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, struct flchip *chip,
- unsigned long addr)
+static int __xipram chip_check(struct map_info *map, struct flchip *chip,
+ unsigned long addr, map_word *expected)
{
struct cfi_private *cfi = map->fldrv_priv;
- map_word d, t;
+ map_word oldd, curd;
+ int ret;

if (cfi_use_status_reg(cfi)) {
map_word ready = CMD(CFI_SR_DRB);
@@ -826,17 +817,35 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
*/
cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
cfi->device_type, NULL);
- d = map_read(map, addr);
+ curd = map_read(map, addr);

- return map_word_andequal(map, d, ready, ready);
+ return map_word_andequal(map, curd, ready, ready);
}

- d = map_read(map, addr);
- t = map_read(map, addr);
+ oldd = map_read(map, addr);
+ curd = map_read(map, addr);
+
+ ret = map_word_equal(map, oldd, curd);

- return map_word_equal(map, d, t);
+ if (!ret || !expected)
+ return ret;
+
+ return map_word_equal(map, curd, *expected);
}

+/*
+ * Return true if the chip is ready.
+ *
+ * Ready is one of: read mode, query mode, erase-suspend-read mode (in any
+ * non-suspended sector) and is indicated by no toggle bits toggling.
+ *
+ * Note that anything more complicated than checking if no bits are toggling
+ * (including checking DQ5 for an error status) is tricky to get working
+ * correctly and is therefore not done (particularly with interleaved chips
+ * as each chip must be checked independently of the others).
+ */
+#define chip_ready(map, chip, addr) chip_check(map, chip, addr, NULL)
+
/*
* Return true if the chip is ready and has the correct value.
*
@@ -855,28 +864,24 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
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);
+ return chip_check(map, chip, addr, &expected);
+}

- /*
- * 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);
- curd = map_read(map, addr);
+static bool cfi_use_chip_ready_for_write(struct map_info *map)
+{
+ struct cfi_private *cfi = map->fldrv_priv;

- return map_word_andequal(map, curd, ready, ready);
- }
+ return cfi->mfr == CFI_MFR_AMD && cfi->id == S29GL064N_MN12;
+}

- oldd = map_read(map, addr);
- curd = map_read(map, addr);
+static int __xipram chip_good_for_write(struct map_info *map,
+ struct flchip *chip, unsigned long addr,
+ map_word expected)
+{
+ if (cfi_use_chip_ready_for_write(map))
+ return chip_ready(map, chip, addr);

- return map_word_equal(map, oldd, curd) &&
- map_word_equal(map, curd, expected);
+ return chip_good(map, chip, addr, expected);
}

static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr, int mode)
@@ -1699,7 +1704,7 @@ static int __xipram do_write_oneword_once(struct map_info *map,
* "chip_good" to avoid the failure due to scheduling.
*/
if (time_after(jiffies, timeo) &&
- !chip_good(map, chip, adr, datum)) {
+ !chip_good_for_write(map, chip, adr, datum)) {
xip_enable(map, chip, adr);
printk(KERN_WARNING "MTD %s(): software timeout\n", __func__);
xip_disable(map, chip, adr);
@@ -1707,7 +1712,7 @@ static int __xipram do_write_oneword_once(struct map_info *map,
break;
}

- if (chip_good(map, chip, adr, datum)) {
+ if (chip_good_for_write(map, chip, adr, datum)) {
if (cfi_check_err_status(map, chip, adr))
ret = -EIO;
break;
@@ -1979,14 +1984,14 @@ static int __xipram do_write_buffer_wait(struct map_info *map,
* "chip_good" to avoid the failure due to scheduling.
*/
if (time_after(jiffies, timeo) &&
- !chip_good(map, chip, adr, datum)) {
+ !chip_good_for_write(map, chip, adr, datum)) {
pr_err("MTD %s(): software timeout, address:0x%.8lx.\n",
__func__, adr);
ret = -EIO;
break;
}

- if (chip_good(map, chip, adr, datum)) {
+ if (chip_good_for_write(map, chip, adr, datum)) {
if (cfi_check_err_status(map, chip, adr))
ret = -EIO;
break;
--
2.32.0

Next message: Sedat Dilek: "Re: [Linux-v5.17-rc6] Building tools/perf with perl v5.34"
Previous message: Thorsten Leemhuis: "Re: [REGRESSION] Too-low frequency limit for AMD GPU PCI-passed-through to Windows VM"
In reply to: Ahmad Fatoum: "Re: [BUG] mtd: cfi_cmdset_0002: write regression since v4.17-rc1"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]