[PATCH 2/3] mtd: spi-nor: Implement die erase command logic
Cyrille Pitchen
cyrille.pitchen at atmel.com
Fri Dec 9 09:22:50 PST 2016
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.
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?
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.
>> - 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.
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.
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.
>>
>> 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