[PATCH 2/3] mtd: spi-nor: Implement die erase command logic
Krzeminski, Marcin (Nokia - PL/Wroclaw)
marcin.krzeminski at nokia.com
Thu Dec 15 22:38:57 PST 2016
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?
>
> 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.html
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.
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