Odp.: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic
Krzeminski, Marcin (Nokia - PL/Wroclaw)
marcin.krzeminski at nokia.com
Wed Dec 21 08:28:59 PST 2016
>
> Le 16/12/2016 à 14:36, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> >
> >
> >> -----Original Message-----
> >> From: Cyrille Pitchen [mailto:cyrille.pitchen at atmel.com]
> >> Sent: Friday, December 16, 2016 11:52 AM
> >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >> <marcin.krzeminski at nokia.com>; linux-mtd at lists.infradead.org
> >> Cc: rfsw-patches at mlist.nokia.com; dwmw2 at infradead.org;
> >> computersforpeace at gmail.com; marek.vasut at gmail.com
> >> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic
> >>
> >> Hi Marcin,
> >>
> >> Le 16/12/2016 à 07:38, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> >>> Cyrille,
> >>>
> >>> Sorry for late answer.
> >>>
> >>>> -----Original Message-----
> >>>> From: Cyrille Pitchen [mailto:cyrille.pitchen at atmel.com]
> >>>> Sent: Friday, December 09, 2016 6:23 PM
> >>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >>>> <marcin.krzeminski at nokia.com>; linux-mtd at lists.infradead.org
> >>>> Cc: rfsw-patches at mlist.nokia.com; dwmw2 at infradead.org;
> >>>> computersforpeace at gmail.com; marek.vasut at gmail.com
> >>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command
> >>>> logic
> >>>>
> >>>> Le 30/11/2016 à 19:18, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Cyrille Pitchen [mailto:cyrille.pitchen at atmel.com]
> >>>>>> Sent: Wednesday, November 30, 2016 6:20 PM
> >>>>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >>>>>> <marcin.krzeminski at nokia.com>; linux-mtd at lists.infradead.org
> >>>>>> Cc: rfsw-patches at mlist.nokia.com; dwmw2 at infradead.org;
> >>>>>> computersforpeace at gmail.com; marek.vasut at gmail.com
> >>>>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command
> >>>>>> logic
> >>>>>>
> >>>>>> Le 30/11/2016 à 17:44, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> >>>>>>>
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Cyrille Pitchen [mailto:cyrille.pitchen at atmel.com]
> >>>>>>>> Sent: Tuesday, November 29, 2016 5:47 PM
> >>>>>>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >>>>>>>> <marcin.krzeminski at nokia.com>; linux-mtd at lists.infradead.org
> >>>>>>>> Cc: rfsw-patches at mlist.nokia.com; dwmw2 at infradead.org;
> >>>>>>>> computersforpeace at gmail.com; marek.vasut at gmail.com
> >>>>>>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase
> >>>>>>>> command logic
> >>>>>>>>
> >>>>>>>> Hi Marcin,
> >>>>>>>>
> >>>>>>>> I know you have already answered some of my questions of IRC but
> >>>>>>>> I ask them again here on the mailing list so everybody can
> >>>>>>>> benefit from your answers.
> >>>>>>>>
> >>>>>>>> Le 24/10/2016 à 15:03, marcin.krzeminski at nokia.com a écrit :
> >>>>>>>>> From: Marcin Krzeminski <marcin.krzeminski at nokia.com>
> >>>>>>>>>
> >>>>>>>>> This commit implements die erase logic.
> >>>>>>>>> Sector at a time procedure is moved to function, then erase
> >>>>>>>>> algorithm will try to use die erase cmd if size and address
> >>>>>>>>> cover one or more dies.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> I'm reading your v2 series but indeed I try to understand the
> >>>>>>>> purpose of the series simply because I don't know what does die
> >>>>>>>> erase do as compared to chip erase of sector erase.
> >>>>>>>> Is you series a bug fix or an optmization, maybe both?
> >>>>>>>
> >>>>>>> In current spi-nor framework when user want to erase whole flash
> >> (eg.
> >>>>>>> mtd_debug erase /dev/mtdX 0 0x8000000) framework issue chip
> >> erase
> >>>>>> cmd.
> >>>>>>> N25Q00 does not support chip erase, instead there was introduced
> >>>>>>> die erase command that can erase whole die (N25Q00 is a mulit-die
> >>>>>>> NOR
> >>>> chip).
> >>>>>>> So for this case this patch is a bug fix.
> >>>>>>> Since I have die erase command implemented it was tempting to use
> >>>>>>> die erase also in case where erase area cover one or more dies to
> >>>>>>> speed up erase process, especially when user has enabled 4KiB
> >>>>>>> sector. That is a small
> >>>>>> optimization.
> >>>>>>>
> >>>>>>> I will rewrite commit message and add those informations.
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Marcin
> >>>>>>>
> >>>>>>
> >>>>>> OK, then I think your patch should be split into two different patches.
> >>>>>>
> >>>>>> 1 - bug fix:
> >>>>>>
> >>>>>> The first patch introducing an explicit flag such as
> >>>>>> SPI_NOR_NO_CHIP_ERASE, which would be set in the relevant entries
> >>>>>> of the spi_nor_ids[] (see already existing flags such as
> >>>>>> SPI_NOR_NO_FR,
> >>>> SPI_NOR_NO_ERASE, ...).
> >>>>>>
> >>>>>> In spi_nor_scan():
> >>>>>>
> >>>>>> if (info->flags & USE_FSR)
> >>>>>> nor->flags |= SNOR_F_USE_FSR;
> >>>>>> if (info->flags & SPI_NOR_HAS_TB)
> >>>>>> nor->flags |= SNOR_F_HAS_SR_TB;
> >>>>>> + if (info->flags & SPI_NOR_NO_CHIP_ERASE)
> >>>>>> + nor->flags |= SNOR_F_NO_CHIP_ERASE;
> >>>>>>
> >>>>>> Then in spi_nor_erase():
> >>>>>>
> >>>>>> /* whole-chip erase? */
> >>>>>> - if (len == mtd->size) {
> >>>>>> + if (len == mtd->size && !(nor->flags & SNOR_F_NO_CHIP_ERASE)) {
> >>>>>> unsigned long timeout;
> >>>>>>
> >>>>>> So you fall into the sector erase case, no optimization yet.
> >>>>>>
> >>>>>> 2 - optimization of spi_nor_erase()
> >>>>>>
> >>>>>> In the "sector"-at-a-time erase branch, try to optimize when
> >>>>>> possible using die erase instead of sector erase.
> >>>>>>
> >>>>>> Each commit message describing the purpose of the patch.
> >>>>>
> >>>>>
> >>>>> OK, I will split patches in this way.
> >>>>>
> >>>>>>
> >>>>>> I would like to also handle the cases where the memory supports:
> >>>>>> - both chip and die erases: if so, no reason not to use chip erase on
> >> such
> >>>>>> multi-die memory.
> >>>>> I do not think that there are chips supporting both.
> >>>>> In this case two flags are needed one for disable chip erase and
> >>>>> another one for enabling die erase. Is that OK?
> >>>>
> >>>> I think it might be better to have 2 separated flags, at least for
> >>>> semantic
> >>>> reason: "the memory supports Die-Erase hence we don't try to use
> >>>> Chip- Erase" sounds a little bit odd to me.
> >>>
> >>> Let's do it with two flags then.
> >>>
> >>>>
> >>>> This is what I understand when I read:
> >>>> /* whole-chip erase? */
> >>>> - if (len == mtd->size) {
> >>>> + if (len == mtd->size && !die_erase) {
> >>>>
> >>>> Why do we try Chip-Erase only if Die-Erase is not supported?
> >>>
> >>> I assumed that there is no flash that support both.
> >>> Do you know one that has chip erase and die erase?
> >>>
> >>
> >> No, I don't but I didn't focus on erase operations so maybe some already
> >> exist or might exit in the future, who knows? :)
> >>
> >> Also, please have a look at Ricardo's patch:
> >> [v9] mtd: spi-nor: Add supported for S3AN spi-nor devices
> >> http://patchwork.ozlabs.org/patch/701935/
> >>
> >> This patch has recently been merged into git://github.com/spi-nor/linux.git
> >> so you will have to rebase your work on that tree.
> >
> > Thanks for information, so git://github.com/spi-nor/linux.git should be used
> > As base to send spi-nor patches?
>
> Indeed yes, as described in MAINTAINERS this is currently the official git
> tree for the spi-nor subsystem. However, we will move soon to infradead.org
> and so the nand subsystem is likely to do. Hence all MTD trees will be
> hosted by infradead.org.
>
> >>
> >> Ricardo's patch already introduces a SNOR_F_NO_OP_CHIP_ERASE flag used
> >> from spi_nor_erase(). The only missing part is to set this flag in nor->flag
> >> when another dedicated flag is set in info->flags.
> >> Currently this SNOR_F_NO_OP_CHIP_ERASE is only set from
> >> s3an_nor_scan() but it should be also set from spi_nor_scan() for the
> >> relevant memories.
> >
> > Ok.
> >>
> >>>>
> >>>> Reading something like "we try Chip-Erase unless Chip-Erase is not
> >>>> supported" is more logic, isn't it?
> >>>>
> >>>> I want to avoid the implicit assumption "Die Erase supported => Chip
> >>>> Erase not supported", even if it *might* be true, because it is
> >>>> strange when we read the code.
> >>>
> >>> Ok.
> >>>>
> >>>>>> - neither chip nor die erases.
> >>>>> This is supported DIE_ERASE flasg set and die_cnt == 0
> >>>>
> >>>> DIE_ERASE flag set and die_cnt == 0 meaning Chip-Erase is not supported?
> >>>> IHMO, semantically, it's not logical.
> >>>
> >>> Just to save yet another flag. Having so many flags make codes
> >>> complicated, but I agree the it I at least logical.
> >>>>
> >>>> Also discussing another topic on #mtd, it seems that some memories
> >>>> don't have a 4-byte address version of the Die Erase op code.
> >>>
> >>> Yes. Could be workarounded by enabling 4byte address mode just before
> >>> sending die erase and then disable it back.
> >>> The more important question is if you really one to have in upstream die
> >> erase?
> >>> It is about 30% faster than sector at time erase on some flash
> >>> (especially in case 4KiB sectors). From the other hand I see chips
> >>> that it was not faster at all (it also applies on chip erase).
> >>> My first version of fixing this problem was to add only the flag of disable
> >> chip erase.
> >>> See here:
> >>> http://lists.infradead.org/pipermail/linux-mtd/2016-October/069888.htm
> >>> l Maybe for now it is better to take above?
> >>>
> >>>>
> >>>> If so, it's preferred to use a 4-byte address Sector/Block Erase op
> >>>> code than to enter/exit the statefull 4-byte address mode then use
> >>>> the Die Erase op
> >>>> code: this statefull mode should be avoided as much as possible
> >>>> because many bootloaders expect the SPI memory to be in 3byte address
> >>>> mode and if a spurious reset occurs on the CPU side (but not on the
> >>>> memory side) those bootloaders will fail to read data from the SPI
> >> memory.
> >>>
> >>> Yeap, moreover similar problem you will face when you will try to add
> >>> QPi modes (444) to framework.
> >>>
> >>
> >> Entering the 4-byte address mode might be accepted as a workaround to
> >> allow using the Die Erase op code even on > 16MiB memory as long as the
> >> user can choose to disable this feature and the feature is not enabled by
> >> default.
> >> This way we don't prevent end users from using Die Erase if they want but
> >> we don't force them to use it if they don't want to.
> >>
> >> Indeed, Die Erase is an optimization but it is not mandatory to erase the
> >> memory: the slower Sector/Block Erase commands can still be used.
> >> However entering the 4-byte address mode to allow using the Die Erase
> >> command has side effects, as explained above, that might introduce
> >> regression.
> >>
> >> Let's imagine the case of a 2-die memory used to store 2 firmware images.
> >> The firmware update process could alternate between dies when writing a
> >> new firmware image so the current/older firmware image is still available if
> >> the update process fails to write the new image.
> >
> > At least for Micron it is possible to do such sequence:
> > 1. Enter 4 byte address mode.
> > 2. Send die erase.
> > 3. Exit 4 byte address mode.
> > 4. Start to poll if erase has been done.
> >
>
> If a spurious CPU reset occurs between the end of step 1 and the beginning
> of step 3, the whole memory would be stuck in its 4byte address mode and
> the bootloader would fail to read data. This is precisely what some
> products want to avoid.
Yes, I just had impression that you are thinking this could take whole erase.
Unfortunately I do not have better solution than this. Any ideas are welcome :)
>
> > Anyway this prove that upstreaming die erase impl. Is not good idea for now.
> > Let's wait for your JESD218 work to be merged.
> >
>
> Your use case is interesting, I don't say we should not add it, I just say
> a mechanism is needed to disable the feature and let it disabled by
> default. On some products, it might be risky hence unacceptable to enter
> the 4 byte mode but on other products it could be safe then the
> optimization you propose is interesting.
I am giving up this series for now. I'll wait for JESD216B merge since it will
completely brake this functionality. Instead I am going to re-spin removal
of chip erase as it is a clear bug in spi-nor framework at lest for n25q00.
Thanks,
Marcin
>
> >>
> >> Then, if you make the whole memory enter its 4-byte address mode to erase
> >> one single die and a spurious reboot occurs, many bootloaders would fail to
> >> read the old firmware from the other die just because the whole memory
> >> expects 4-byte addresses.
> >>
> >> About the QPI mode (SPI 4-4-4), I agree with you, it has the very same
> >> annoying side effect. Hence I've removed support of the SPI 2-2-2 and SPI
> >> 4-4-4 protocols at the memory side from my SFDP series for the moment.
> >
> > And that's remained me I have a comment about that :)
> >
> > Thanks,
> > Marcin
> >
> >> Besides, on the controller side, by changing the 3rd argument parameter of
> >> spi_nor_scan(), one of my patch allows the SPI controller driver to claim "I
> >> can't/don't want to use SPI 4-4-4 but I support SPI 1-1-4 and SPI 1-4-4
> >> protocols". So the SPI 4-4-4 protocol can be disabled if needed.
> >>
> >> Best regards,
> >>
> >> Cyrille
> >>
> >>
> >>> Thanks,
> >>> Marcin
> >>>
> >>>>
> >>>>>>
> >>>>>> Maybe I'm wrong but the assumption (multi-die => chip erase not
> >>>>>> supported) might be false.
> >>>>> It is false, but all multi-die chip I know support die erase or chip erase.
> >>>>> Never both.
> >>>>>
> >>>>> Thanks,
> >>>>> Marcin
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> Best regards,
> >>>>>>
> >>>>>> Cyrille
> >>>>>>
> >>>>>>
> >>>>>>>>
> >>>>>>>> Best regards,
> >>>>>>>>
> >>>>>>>> Cyrille
> >>>>>>>>
> >>>>>>>>> Signed-off-by: Marcin Krzeminski <marcin.krzeminski at nokia.com>
> >>>>>>>>> ---
> >>>>>>>>> drivers/mtd/spi-nor/spi-nor.c | 90
> >>>>>>>>> +++++++++++++++++++++++++++++++++++++------
> >>>>>>>>> 1 file changed, 78 insertions(+), 12 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
> >>>>>>>>> b/drivers/mtd/spi-nor/spi-nor.c index 3afe207..17bbec0 100644
> >>>>>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
> >>>>>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >>>>>>>>> @@ -360,6 +360,29 @@ static int spi_nor_erase_sector(struct
> >>>>>>>>> spi_nor
> >>>>>>>> *nor, u32 addr)
> >>>>>>>>> return nor->write_reg(nor, nor->erase_opcode, buf, nor-
> >>>>>>>>> addr_width); }
> >>>>>>>>>
> >>>>>>>>> +static inline int spi_nor_sector_at_time_erase(struct mtd_info
> >>>>>>>>> +*mtd,
> >>>>>>>>> +u32 addr, u32 len) {
> >>>>>>>>> + struct spi_nor *nor = mtd_to_spi_nor(mtd);
> >>>>>>>>> + int ret = 0;
> >>>>>>>>> +
> >>>>>>>>> + while (len) {
> >>>>>>>>> + write_enable(nor);
> >>>>>>>>> +
> >>>>>>>>> + ret = spi_nor_erase_sector(nor, addr);
> >>>>>>>>> + if (ret)
> >>>>>>>>> + return ret;
> >>>>>>>>> +
> >>>>>>>>> + addr += mtd->erasesize;
> >>>>>>>>> + len -= mtd->erasesize;
> >>>>>>>>> +
> >>>>>>>>> + ret = spi_nor_wait_till_ready(nor);
> >>>>>>>>> + if (ret)
> >>>>>>>>> + return ret;
> >>>>>>>>> + }
> >>>>>>>>> +
> >>>>>>>>> + return ret;
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>> /*
> >>>>>>>>> * Erase an address range on the nor chip. The address range
> >>>>>>>>> may
> >>>>>> extend
> >>>>>>>>> * one or more erase sectors. Return an error is there is a
> >>>>>>>>> problem
> >>>>>> erasing.
> >>>>>>>>> @@ -367,9 +390,11 @@ static int spi_nor_erase_sector(struct
> >>>>>>>>> spi_nor *nor, u32 addr) static int spi_nor_erase(struct
> >>>>>>>>> mtd_info *mtd, struct erase_info *instr) {
> >>>>>>>>> struct spi_nor *nor = mtd_to_spi_nor(mtd);
> >>>>>>>>> - u32 addr, len;
> >>>>>>>>> + u32 addr, len, die_no, die_size;
> >>>>>>>>> uint32_t rem;
> >>>>>>>>> int ret;
> >>>>>>>>> + unsigned long timeout;
> >>>>>>>>> + u8 die_erase = nor->die_cnt && SNOR_F_DIE_ERASE;
> >>>>>>>>>
> >>>>>>>>> dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr-
> >>> addr,
> >>>>>>>>> (long long)instr->len);
> >>>>>>>>> @@ -386,7 +411,7 @@ static int spi_nor_erase(struct mtd_info
> >>>>>>>>> *mtd,
> >>>>>>>> struct erase_info *instr)
> >>>>>>>>> return ret;
> >>>>>>>>>
> >>>>>>>>> /* whole-chip erase? */
> >>>>>>>>> - if (len == mtd->size) {
> >>>>>>>>> + if (len == mtd->size && !die_erase) {
> >>>>>>>>> unsigned long timeout;
> >>>>>>>>>
> >>>>>>>>> write_enable(nor);
> >>>>>>>>> @@ -416,17 +441,58 @@ static int spi_nor_erase(struct mtd_info
> >>>>>>>>> *mtd, struct erase_info *instr)
> >>>>>>>>>
> >>>>>>>>> /* "sector"-at-a-time erase */
> >>>>>>>>> } else {
> >>>>>>>>> - while (len) {
> >>>>>>>>> - write_enable(nor);
> >>>>>>>>> -
> >>>>>>>>> - ret = spi_nor_erase_sector(nor, addr);
> >>>>>>>>> - if (ret)
> >>>>>>>>> + if (die_erase) {
> >>>>>>>>> + die_size = div_u64_rem(mtd->size, nor-
> >>>>> die_cnt,
> >>>>>>>> &rem);
> >>>>>>>>> + if (rem) {
> >>>>>>>>> + ret = -EINVAL;
> >>>>>>>>> goto erase_err;
> >>>>>>>>> -
> >>>>>>>>> - addr += mtd->erasesize;
> >>>>>>>>> - len -= mtd->erasesize;
> >>>>>>>>> -
> >>>>>>>>> - ret = spi_nor_wait_till_ready(nor);
> >>>>>>>>> + }
> >>>>>>>>> + while (len) {
> >>>>>>>>> + die_no = div_u64_rem(addr,
> >>>> die_size,
> >>>>>>>> &rem);
> >>>>>>>>> +
> >>>>>>>>> + /* Check if address is aligned to die
> >>>> begin*/
> >>>>>>>>> + if (!rem) {
> >>>>>>>>> + /* die erase? */
> >>>>>>>>> + if (len >= die_size) {
> >>>>>>>>> + ret =
> >>>> spi_nor_die_erase(nor,
> >>>>>>>> addr);
> >>>>>>>>> + if (ret)
> >>>>>>>>> + goto
> >>>> erase_err;
> >>>>>>>>> +
> >>>>>>>>> + len -= die_size;
> >>>>>>>>> + addr += die_size;
> >>>>>>>>> +
> >>>>>>>>> + timeout =
> >>>>>>>> max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
> >>>>>>>>> +
> >>>>>>>> CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
> >>>>>>>>> +
> >>>> (unsigned
> >>>>>>>> long)(die_size / SZ_2M));
> >>>>>>>>> + ret =
> >>>>>>>> spi_nor_wait_till_ready_with_timeout(nor, timeout);
> >>>>>>>>> + if (ret)
> >>>>>>>>> + goto
> >>>> erase_err;
> >>>>>>>>> + } else {
> >>>>>>>>> + ret =
> >>>>>>>> spi_nor_sector_at_time_erase(mtd, addr, len);
> >>>>>>>>> + if (ret)
> >>>>>>>>> + goto
> >>>> erase_err;
> >>>>>>>>> + len = 0;
> >>>>>>>>> + }
> >>>>>>>>> + } else {
> >>>>>>>>> + /* check if end address cover
> >>>> at least
> >>>>>>>> one die */
> >>>>>>>>> + if (div64_ul(addr + len,
> >>>> die_size) >
> >>>>>>>> ++die_no) {
> >>>>>>>>> + /* align to next die */
> >>>>>>>>> + rem = die_size - rem;
> >>>>>>>>> + ret =
> >>>>>>>> spi_nor_sector_at_time_erase(mtd, addr, rem);
> >>>>>>>>> + if (ret)
> >>>>>>>>> + goto
> >>>> erase_err;
> >>>>>>>>> + len -= rem;
> >>>>>>>>> + addr += rem;
> >>>>>>>>> + } else {
> >>>>>>>>> + ret =
> >>>>>>>> spi_nor_sector_at_time_erase(mtd, addr, len);
> >>>>>>>>> + if (ret)
> >>>>>>>>> + goto
> >>>> erase_err;
> >>>>>>>>> + len = 0;
> >>>>>>>>> + }
> >>>>>>>>> + }
> >>>>>>>>> + }
> >>>>>>>>> + } else {
> >>>>>>>>> + ret = spi_nor_sector_at_time_erase(mtd,
> >>>> addr, len);
> >>>>>>>>> if (ret)
> >>>>>>>>> goto erase_err;
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >
> >
>
More information about the linux-mtd
mailing list