| Message ID | 20241119093949.3014-1-SkyLake.Huang@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | [nand/next] mtd: nand: spi: Use write_cache first and then update_cache in write operation | expand |
Hello, On 19/11/2024 at 17:39:49 +08, Sky Huang <SkyLake.Huang@mediatek.com> wrote: > From: Sky Huang <skylake.huang@mediatek.com> > > According to discussion with Miquel Raynal <miquel.raynal@bootlin.com> > and Chuanhong Guo <gch981213@gmail.com> on FORESEE F35SQA002G patch, > Chuanhong recommmends that we can use the following sequence in > spinand_write_to_cache_op(): > > x1 mode: > 02H(program data load) -> 84H(random program data load) -> 84H ... > > x4 mode: > 32H(program data load x4) -> 34H(random program data load x4) -> 34H ... > > 02H or 32H commands will clear cache buffer on SPI-NAND and load > data to it. For those SPI controllers which can't finish transmission > in single step, 84H or 34H will be triggered for the rest data. > > We observe that some current SPI-NANDs, including FORESEE F35SQA001G and > F35SQA002G, must use 02H or 32H to reset cache buffer in flash before > using 84H or 34H. Or users may encounter program failure issue. This issue > is not always reproducible, but it may occur and cause system instability. > > This sequence should work on all SPI-NANDs nowadays. I also check with > Foresee that the sequence can solve the above program failure issue. > > On my test platform (MT7988), SPI driver is drivers/spi/spi-mt65xx.c. > And I limit MTK_SPI_IPM_PACKET_SIZE to SZ_1K to simulate lightweight SPI > controller which can only transmit 1024 bytes. > > The test step is the following: > - mtd erase /dev/mtd2 > - dd if=/dev/zero bs=2048 count=1 | tr '\0' '\xA5' > output.bin > - mtd write output.bin /dev/mtd2 > > Before applying this patch, write operation uses only 34H(update_cache): > [78.937720] OP code: 0x34, addr val: 0x0, data nbytes: 1020, data 1st byte: 0xa5 > [78.945297] OP code: 0x34, addr val: 0x3fc, data nbytes: 1020, data 1st byte: 0xa5 > [78.954251] OP code: 0x34, addr val: 0x7f8, data nbytes: 72, data 1st byte: 0xa5 > [78.962966] OP code: 0x10, addr val: 0x300 > [78.968816] OP code: 0x34, addr val: 0x0, data nbytes: 1020, data 1st byte: 0xff > [78.977233] OP code: 0x34, addr val: 0x3fc, data nbytes: 1020, data 1st byte: 0xff > [78.985124] OP code: 0x34, addr val: 0x7f8, data nbytes: 72, data 1st byte: 0xff > [78.992527] OP code: 0x10, addr val: 0x301 > [78.996981] OP code: 0x34, addr val: 0x0, data nbytes: 1020, data 1st byte: 0xff > [79.004416] OP code: 0x34, addr val: 0x3fc, data nbytes: 1020, data 1st byte: 0xff > [79.012031] OP code: 0x34, addr val: 0x7f8, data nbytes: 72, data 1st byte: 0xff > [79.019435] OP code: 0x10, addr val: 0x302 I am sorry but above you said that we should not perform: 0x32, 0x32, 0x32... because the second time it would clear the cache again. And here you tell us that actually the core already handles that by performing instead: 0x34, 0x34, 0x34... So what is the problem? Or maybe I misunderstood the issue, but I think Chuanhong raised an issue that is already solved? Isn't it? Thanks, Miquèl
Hello! 在 2024/12/5 23:32, Miquel Raynal 写道: > Hello, > > On 19/11/2024 at 17:39:49 +08, Sky Huang <SkyLake.Huang@mediatek.com> wrote: > >> From: Sky Huang <skylake.huang@mediatek.com> >> >> According to discussion with Miquel Raynal <miquel.raynal@bootlin.com> >> and Chuanhong Guo <gch981213@gmail.com> on FORESEE F35SQA002G patch, >> Chuanhong recommmends that we can use the following sequence in >> spinand_write_to_cache_op(): >> >> x1 mode: >> 02H(program data load) -> 84H(random program data load) -> 84H ... >> >> x4 mode: >> 32H(program data load x4) -> 34H(random program data load x4) -> 34H ... >> >> 02H or 32H commands will clear cache buffer on SPI-NAND and load >> data to it. For those SPI controllers which can't finish transmission >> in single step, 84H or 34H will be triggered for the rest data. >> >> We observe that some current SPI-NANDs, including FORESEE F35SQA001G and >> F35SQA002G, must use 02H or 32H to reset cache buffer in flash before >> using 84H or 34H. Or users may encounter program failure issue. This issue >> is not always reproducible, but it may occur and cause system instability. >> >> This sequence should work on all SPI-NANDs nowadays. I also check with >> Foresee that the sequence can solve the above program failure issue. >> >> On my test platform (MT7988), SPI driver is drivers/spi/spi-mt65xx.c. >> And I limit MTK_SPI_IPM_PACKET_SIZE to SZ_1K to simulate lightweight SPI >> controller which can only transmit 1024 bytes. >> >> The test step is the following: >> - mtd erase /dev/mtd2 >> - dd if=/dev/zero bs=2048 count=1 | tr '\0' '\xA5' > output.bin >> - mtd write output.bin /dev/mtd2 >> >> Before applying this patch, write operation uses only 34H(update_cache): >> [78.937720] OP code: 0x34, addr val: 0x0, data nbytes: 1020, data 1st byte: 0xa5 >> [78.945297] OP code: 0x34, addr val: 0x3fc, data nbytes: 1020, data 1st byte: 0xa5 >> [78.954251] OP code: 0x34, addr val: 0x7f8, data nbytes: 72, data 1st byte: 0xa5 >> [78.962966] OP code: 0x10, addr val: 0x300 >> [78.968816] OP code: 0x34, addr val: 0x0, data nbytes: 1020, data 1st byte: 0xff >> [78.977233] OP code: 0x34, addr val: 0x3fc, data nbytes: 1020, data 1st byte: 0xff >> [78.985124] OP code: 0x34, addr val: 0x7f8, data nbytes: 72, data 1st byte: 0xff >> [78.992527] OP code: 0x10, addr val: 0x301 >> [78.996981] OP code: 0x34, addr val: 0x0, data nbytes: 1020, data 1st byte: 0xff >> [79.004416] OP code: 0x34, addr val: 0x3fc, data nbytes: 1020, data 1st byte: 0xff >> [79.012031] OP code: 0x34, addr val: 0x7f8, data nbytes: 72, data 1st byte: 0xff >> [79.019435] OP code: 0x10, addr val: 0x302 > > I am sorry but above you said that we should not perform: > > 0x32, 0x32, 0x32... > > because the second time it would clear the cache again. And here > you tell us that actually the core already handles that by performing > instead: > > 0x34, 0x34, 0x34... > > So what is the problem? > > Or maybe I misunderstood the issue, but I think Chuanhong raised an > issue that is already solved? Isn't it? > The issue is that the FORESEE NANDs require the first cache writing instruction to be WRITE_CACHE instead of UPDATE_CACHE. i.e. it needs a command sequence of: 0x32, 0x34, 0x34, 0x34... This patch does exactly that, making the first instruction issued 0x32. It should be applied to fix the issue above. --- Regards, Chuanhong Guo
Hello, On 22/04/2025 at 09:38:26 +08, Chuanhong Guo <gch981213@gmail.com> wrote: > Hello! > 在 2024/12/5 23:32, Miquel Raynal 写道: >> Hello, >> On 19/11/2024 at 17:39:49 +08, Sky Huang <SkyLake.Huang@mediatek.com> >> wrote: >> >>> From: Sky Huang <skylake.huang@mediatek.com> >>> >>> According to discussion with Miquel Raynal <miquel.raynal@bootlin.com> >>> and Chuanhong Guo <gch981213@gmail.com> on FORESEE F35SQA002G patch, >>> Chuanhong recommmends that we can use the following sequence in >>> spinand_write_to_cache_op(): >>> >>> x1 mode: >>> 02H(program data load) -> 84H(random program data load) -> 84H ... >>> >>> x4 mode: >>> 32H(program data load x4) -> 34H(random program data load x4) -> 34H ... >>> >>> 02H or 32H commands will clear cache buffer on SPI-NAND and load >>> data to it. For those SPI controllers which can't finish transmission >>> in single step, 84H or 34H will be triggered for the rest data. >>> >>> We observe that some current SPI-NANDs, including FORESEE F35SQA001G and >>> F35SQA002G, must use 02H or 32H to reset cache buffer in flash before >>> using 84H or 34H. Or users may encounter program failure issue. This issue >>> is not always reproducible, but it may occur and cause system instability. >>> >>> This sequence should work on all SPI-NANDs nowadays. I also check with >>> Foresee that the sequence can solve the above program failure issue. >>> >>> On my test platform (MT7988), SPI driver is drivers/spi/spi-mt65xx.c. >>> And I limit MTK_SPI_IPM_PACKET_SIZE to SZ_1K to simulate lightweight SPI >>> controller which can only transmit 1024 bytes. >>> >>> The test step is the following: >>> - mtd erase /dev/mtd2 >>> - dd if=/dev/zero bs=2048 count=1 | tr '\0' '\xA5' > output.bin >>> - mtd write output.bin /dev/mtd2 >>> >>> Before applying this patch, write operation uses only 34H(update_cache): >>> [78.937720] OP code: 0x34, addr val: 0x0, data nbytes: 1020, data 1st byte: 0xa5 >>> [78.945297] OP code: 0x34, addr val: 0x3fc, data nbytes: 1020, data 1st byte: 0xa5 >>> [78.954251] OP code: 0x34, addr val: 0x7f8, data nbytes: 72, data 1st byte: 0xa5 >>> [78.962966] OP code: 0x10, addr val: 0x300 >>> [78.968816] OP code: 0x34, addr val: 0x0, data nbytes: 1020, data 1st byte: 0xff >>> [78.977233] OP code: 0x34, addr val: 0x3fc, data nbytes: 1020, data 1st byte: 0xff >>> [78.985124] OP code: 0x34, addr val: 0x7f8, data nbytes: 72, data 1st byte: 0xff >>> [78.992527] OP code: 0x10, addr val: 0x301 >>> [78.996981] OP code: 0x34, addr val: 0x0, data nbytes: 1020, data 1st byte: 0xff >>> [79.004416] OP code: 0x34, addr val: 0x3fc, data nbytes: 1020, data 1st byte: 0xff >>> [79.012031] OP code: 0x34, addr val: 0x7f8, data nbytes: 72, data 1st byte: 0xff >>> [79.019435] OP code: 0x10, addr val: 0x302 >> I am sorry but above you said that we should not perform: >> 0x32, 0x32, 0x32... >> because the second time it would clear the cache again. And here >> you tell us that actually the core already handles that by performing >> instead: >> 0x34, 0x34, 0x34... >> So what is the problem? >> Or maybe I misunderstood the issue, but I think Chuanhong raised an >> issue that is already solved? Isn't it? >> > > The issue is that the FORESEE NANDs require the first cache writing > instruction to be WRITE_CACHE instead of UPDATE_CACHE. i.e. it needs a > command sequence of: > 0x32, 0x34, 0x34, 0x34... So Foresee NANDs do not support update_cache, why are they advertised in the first place? Could you we have a less impacting solution for the other NANDs? > This patch does exactly that, making the first instruction issued 0x32. > It should be applied to fix the issue above. My understanding is that this is very specific to FORESEE NANDs and you are changing this for all NANDs. I have fears that it will break everywhere else. Overall I understand the problem, but I disagree with the fix. Could you propose something less impacting as hinted above? Thanks and sorry for the delay. Miquèl
Hello, Miquel! On Tue, Apr 29, 2025 at 4:15 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hello, > > On 22/04/2025 at 09:38:26 +08, Chuanhong Guo <gch981213@gmail.com> wrote: > > > Hello! > > 在 2024/12/5 23:32, Miquel Raynal 写道: > >> Hello, > >> On 19/11/2024 at 17:39:49 +08, Sky Huang <SkyLake.Huang@mediatek.com> > >> wrote: > >> > >>> From: Sky Huang <skylake.huang@mediatek.com> > >>> > >>> According to discussion with Miquel Raynal <miquel.raynal@bootlin.com> > >>> and Chuanhong Guo <gch981213@gmail.com> on FORESEE F35SQA002G patch, > >>> Chuanhong recommmends that we can use the following sequence in > >>> spinand_write_to_cache_op(): > >>> > >>> x1 mode: > >>> 02H(program data load) -> 84H(random program data load) -> 84H ... > >>> > >>> x4 mode: > >>> 32H(program data load x4) -> 34H(random program data load x4) -> 34H ... > >>> > >>> 02H or 32H commands will clear cache buffer on SPI-NAND and load > >>> data to it. For those SPI controllers which can't finish transmission > >>> in single step, 84H or 34H will be triggered for the rest data. > >>> > >>> We observe that some current SPI-NANDs, including FORESEE F35SQA001G and > >>> F35SQA002G, must use 02H or 32H to reset cache buffer in flash before > >>> using 84H or 34H. Or users may encounter program failure issue. This issue > >>> is not always reproducible, but it may occur and cause system instability. > >>> > >>> This sequence should work on all SPI-NANDs nowadays. I also check with > >>> Foresee that the sequence can solve the above program failure issue. > >>> > >>> On my test platform (MT7988), SPI driver is drivers/spi/spi-mt65xx.c. > >>> And I limit MTK_SPI_IPM_PACKET_SIZE to SZ_1K to simulate lightweight SPI > >>> controller which can only transmit 1024 bytes. > >>> > >>> The test step is the following: > >>> - mtd erase /dev/mtd2 > >>> - dd if=/dev/zero bs=2048 count=1 | tr '\0' '\xA5' > output.bin > >>> - mtd write output.bin /dev/mtd2 > >>> > >>> Before applying this patch, write operation uses only 34H(update_cache): > >>> [78.937720] OP code: 0x34, addr val: 0x0, data nbytes: 1020, data 1st byte: 0xa5 > >>> [78.945297] OP code: 0x34, addr val: 0x3fc, data nbytes: 1020, data 1st byte: 0xa5 > >>> [78.954251] OP code: 0x34, addr val: 0x7f8, data nbytes: 72, data 1st byte: 0xa5 > >>> [78.962966] OP code: 0x10, addr val: 0x300 > >>> [78.968816] OP code: 0x34, addr val: 0x0, data nbytes: 1020, data 1st byte: 0xff > >>> [78.977233] OP code: 0x34, addr val: 0x3fc, data nbytes: 1020, data 1st byte: 0xff > >>> [78.985124] OP code: 0x34, addr val: 0x7f8, data nbytes: 72, data 1st byte: 0xff > >>> [78.992527] OP code: 0x10, addr val: 0x301 > >>> [78.996981] OP code: 0x34, addr val: 0x0, data nbytes: 1020, data 1st byte: 0xff > >>> [79.004416] OP code: 0x34, addr val: 0x3fc, data nbytes: 1020, data 1st byte: 0xff > >>> [79.012031] OP code: 0x34, addr val: 0x7f8, data nbytes: 72, data 1st byte: 0xff > >>> [79.019435] OP code: 0x10, addr val: 0x302 > >> I am sorry but above you said that we should not perform: > >> 0x32, 0x32, 0x32... > >> because the second time it would clear the cache again. And here > >> you tell us that actually the core already handles that by performing > >> instead: > >> 0x34, 0x34, 0x34... > >> So what is the problem? > >> Or maybe I misunderstood the issue, but I think Chuanhong raised an > >> issue that is already solved? Isn't it? > >> > > > > The issue is that the FORESEE NANDs require the first cache writing > > instruction to be WRITE_CACHE instead of UPDATE_CACHE. i.e. it needs a > > command sequence of: > > 0x32, 0x34, 0x34, 0x34... > > So Foresee NANDs do not support update_cache, why are they advertised in > the first place? Could you we have a less impacting solution for the > other NANDs? > > > This patch does exactly that, making the first instruction issued 0x32. > > It should be applied to fix the issue above. > > My understanding is that this is very specific to FORESEE NANDs and you > are changing this for all NANDs. I have fears that it will break > everywhere else. I just checked a few datasheets of SPI-NANDs from Toshiba/Winbond/Etron/ESMT/GigaDevice/Macronix. All of them specify the programming sequence to be: write_enable->write_cache->update_cache if needed-> program_execute->poll status. Some of them mentions that the only difference between write_cache and update_cache is whether the page cache is cleared before writing (Winbond), while others don't specifically say that. The original sequence doesn't seem to be following any manufacturers' instructions. We just haven't run into any problems until this FORESEE one. > > Overall I understand the problem, but I disagree with the fix. Could you > propose something less impacting as hinted above? > > Thanks and sorry for the delay. > Miquèl
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c index b1df7f627161..351bd14d8e06 100644 --- a/drivers/mtd/nand/spi/core.c +++ b/drivers/mtd/nand/spi/core.c @@ -506,7 +506,10 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand, nbytes -= ret; column += ret; buf += ret; + + wdesc->info.op_tmpl = *spinand->op_templates.update_cache; } + wdesc->info.op_tmpl = *spinand->op_templates.write_cache; return 0; } @@ -1037,7 +1040,7 @@ static int spinand_create_dirmap(struct spinand_device *spinand, /* The plane number is passed in MSB just above the column address */ info.offset = plane << fls(nand->memorg.pagesize); - info.op_tmpl = *spinand->op_templates.update_cache; + info.op_tmpl = *spinand->op_templates.write_cache; desc = devm_spi_mem_dirmap_create(&spinand->spimem->spi->dev, spinand->spimem, &info); if (IS_ERR(desc)) @@ -1060,7 +1063,7 @@ static int spinand_create_dirmap(struct spinand_device *spinand, return 0; } - info.op_tmpl = *spinand->op_templates.update_cache; + info.op_tmpl = *spinand->op_templates.write_cache; info.op_tmpl.data.ecc = true; desc = devm_spi_mem_dirmap_create(&spinand->spimem->spi->dev, spinand->spimem, &info);