Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFC PATCH] mtd: spi-nor: add support to non-uniform SPI NOR
 flash memories
To:     Tudor Ambarus <tudor.ambarus@microchip.com>,
        cyrille.pitchen@microchip.com, dwmw2@infradead.org,
        computersforpeace@gmail.com, boris.brezillon@bootlin.com,
        richard@nod.at
Cc:     linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
        nicolas.ferre@microchip.com, Cristian.Birsan@microchip.com
References: <20180518093233.24241-1-tudor.ambarus@microchip.com>
 <89d45190-95b0-b780-b219-e6c6adcb6147@gmail.com>
 <4cd7d47a-fd56-6b54-3b38-262adf46a97f@microchip.com>
From:   Marek Vasut <marek.vasut@gmail.com>
Message-ID: <bbb9387b-4404-e6a0-b8b5-f48bcebdd474@gmail.com>
Date:   Mon, 21 May 2018 18:59:04 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.7.0
MIME-Version: 1.0
In-Reply-To: <4cd7d47a-fd56-6b54-3b38-262adf46a97f@microchip.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 05/21/2018 06:42 PM, Tudor Ambarus wrote:
> Hi, Marek,

[...]

>>> This is a transitional patch: non-uniform erase maps will be used later
>>> when initialized based on the SFDP data.
>>
>> What about non-SFDP non-linear flashes ?
> 
> Non-SFDP non-uniform flashes support is not addressed with this
> proposal, I should have told this in the commit message, thanks. But we
> are backward compatible, if non-SFDP, the flashes are considered
> uniform.

OK. btw wall-of-text description of patch isn't my fav thing.

>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@microchip.com>
>>>
>>> [tudor.ambarus@microchip.com:
>>> - add improvements on how the erase map is handled. The map is an array
>>> describing the boundaries of the erase regions. LSB bits of the region's
>>> offset are used to describe the supported erase types, to indicate if
>>> that specific region is the last region in the map and to mark if the
>>> region is overlaid or not. When one sends an addr and len to erase a
>>> chunk of memory, we identify in which region the address fits, we start
>>> erasing with the best fitted erase commands and when the region ends,
>>> continue to erase from the next region. The erase is optimal: identify
>>> the start offset (once), then erase with the best erase command,
>>> move forward and repeat.
>>
>> Is that like an R-tree ?
> 
> Not really. I find this RFC proposal faster and neat, but I'm open for
> suggestions and guidance.
> 
> One wants to erase a contiguous chunk of memory and sends us the
> starting address and the total length. The algorithm of finding the best
> sequence of erase commands can be summarized in four steps:
> 
> 1. Find in which region the address fits.
> This step is done only once, at the beginning. For the non-uniform
> SFDP-defined flashes, usually there are two or three regions defined.
> Nevertheless, in the worst case, the maximum number of regions that can
> be defined is on eight bits, so 255. Linear search for just 255 elements
> in the worst case looks good for me, especially that we do this search
> once.
> 
> 2. Find the *best* erase command that is defined in that region.
> Each region can define maximum 4 erase commands. *Best* is defined as
> the largest/biggest supported erase command with which the provided
> address is aligned and which does not erase more that what the user has
> asked for. In case of overlaid regions, alignment does not matter. The
> largest command will erase the remaining of the overlaid region without
> touching the region with which it overlaps (see S25FS512S). The
> supported erase commands are ordered by size with the biggest queried
> first. It is desirable to erase with large erase commands so that we
> erase as much as we can in one shoot, minimizing the erase() calls.
> 
> 3. Erase sector with the *best* erase command and move forward in a
> linear fashion.
> addr += cmd->size;
> len -= cmd->size;
> If the new address exceeds the end of this region, move to the next.
> 
> 4. While (len) goto step2.
> 
> That's all. Linearity is an advantage. We find the starting region and
> then we traverse each region in order without other queries.
> 
>>
>>> - order erase types by size, with the biggest erase type at BIT(0). With
>>> this, we can iterate from the biggest supported erase type to the
>>> smallest,
>>> and when find one that meets all the required conditions, break the
>>> loop.
>>> This saves time in determining the best erase cmd.
>>>
>>> - minimize the amount of erase() calls by using the best sequence of
>>> erase
>>> type commands depending on alignment.
>>
>> Nice, this was long overdue
>>
>>> - replace spi_nor_find_uniform_erase() with
>>> spi_nor_select_uniform_erase().
>>> Even for the SPI NOR memories with non-uniform erase types, we can
>>> determine
>>> at init if there are erase types that can erase the entire memory.
>>> Fill at
>>> init the uniform_erase_type bitmask, to encode the erase type
>>> commands that
>>> can erase the entire memory.
>>>
>>> - clarify support for overlaid regions. Considering one of the erase
>>> maps
>>> of the S25FS512S memory:
>>> Bottom: 8x 4KB sectors at bottom (only 4KB erase supported),
>>>          1x overlaid 224KB sector at bottom (only 256KB erase
>>> supported),
>>>          255x 256KB sectors (only 256KB erase supported)
>>> S25FS512S states that 'if a sector erase command is applied to a
>>> 256KB range
>>> that is overlaid by 4KB secors, the overlaid 4kB sectors are not
>>> affected by
>>> the erase'. When at init, the overlaid region size should be set to
>>> region->size = erase_size - count; in order to not miss chunks of data
>>> when traversing the regions.
>>>
>>> - backward compatibility test done on MX25L25673G.
>>>
>>> The 'erase with the best command, move forward and repeat' approach was
>>> suggested by Cristian Birsan in a brainstorm session, so:
>>> ]
>>> Suggested-by: Cristian Birsan <cristian.birsan@microchip.com>
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>> ---
>>>   drivers/mtd/spi-nor/spi-nor.c | 281
>>> +++++++++++++++++++++++++++++++++++++++---
>>>   include/linux/mtd/spi-nor.h   |  89 +++++++++++++
>>>   2 files changed, 356 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
>>> b/drivers/mtd/spi-nor/spi-nor.c
>>> index 494b7a2..bb70664 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -260,6 +260,17 @@ static void spi_nor_set_4byte_opcodes(struct
>>> spi_nor *nor,
>>>       nor->read_opcode = spi_nor_convert_3to4_read(nor->read_opcode);
>>>       nor->program_opcode =
>>> spi_nor_convert_3to4_program(nor->program_opcode);
>>>       nor->erase_opcode = spi_nor_convert_3to4_erase(nor->erase_opcode);
>>> +
>>> +    if (!spi_nor_has_uniform_erase(nor)) {
>>> +        struct spi_nor_erase_map *map = &nor->erase_map;
>>> +        struct spi_nor_erase_command *cmd;
>>> +        int i;
>>> +
>>> +        for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) {
>>> +            cmd = &map->commands[i];
>>> +            cmd->opcode = spi_nor_convert_3to4_erase(cmd->opcode);
>>> +        }
>>> +    }
>>>   }
>>>     /* Enable/disable 4-byte addressing mode. */
>>> @@ -497,6 +508,131 @@ static int spi_nor_erase_sector(struct spi_nor
>>> *nor, u32 addr)
>>>       return nor->write_reg(nor, nor->erase_opcode, buf,
>>> nor->addr_width);
>>>   }
>>>   +/* JEDEC JESD216B Standard imposes erase sizes to be power of 2. */
>>> +static inline u64
>>> +spi_nor_div_by_erase_size(const struct spi_nor_erase_command *cmd,
>>> +              u64 dividend, u32 *remainder)
>>> +{
>>> +    *remainder = (u32)dividend & cmd->size_mask;
>>> +    return dividend >> cmd->size_shift;
>>> +}
>>> +
>>> +static const struct spi_nor_erase_command *
>>> +spi_nor_find_best_erase_cmd(const struct spi_nor_erase_map *map,
>>> +                const struct spi_nor_erase_region *region, u64 addr,
>>> +                u32 len)
>>> +{
>>> +    const struct spi_nor_erase_command *cmd;
>>> +    u32 rem;
>>> +    int i;
>>> +    u8 cmd_mask = region->offset & SNOR_CMD_ERASE_MASK;
>>> +
>>> +    /*
>>> +     * Commands are ordered by size, with the biggest erase type at
>>> +     * index 0.
>>> +     */
>>> +    for (i = 0; i < SNOR_CMD_ERASE_MAX; i++) {
>>> +        /* Does the erase region support the tested erase command? */
>>> +        if (!(cmd_mask & BIT(i)))
>>> +            continue;
>>> +
>>> +        cmd = &map->commands[i];
>>> +
>>> +        /* Don't erase more than what the user has asked for. */
>>> +        if (cmd->size > len)
>>> +            continue;
>>
>> Are you sure checking for the full erase block length first and then
>> checking if you can sub-erase the block is OK ?
> 
> will respond in the next comment.
> 
>>
>>> +        if (!(region->offset & SNOR_OVERLAID_REGION)) {
>>> +            /* 'addr' must be aligned to the erase size. */
>>> +            spi_nor_div_by_erase_size(cmd, addr, &rem);
> 
> oh, I missed the if here, this should have been confusing.
> if (rem)
>     continue;
> else
>     return cmd;
> The else case can be merged with the one from below.
> 
> Returning to your previous question. I iterate from the biggest erase
> command to the smallest, because bigger is preferred, it will minimize
> the amount of erase() calls. The biggest erase command that doesn't
> erase more that what the user has asked for, will do. If the region is
> not-overlaid the address must also be aligned with the erase size.

You can have a flash with 4k sectors which also supports 64k erase and
try to erase ie. 128k at offset +4k. That means you need to first erase
small chunks, then big chunk, then small chunks again. So I don't think
you can start with large chunk to see if you can erase it, since on such
a setup the erase will degrade to massive amount of 4k erase ops.

[...]

>>> +    while (len) {
>>> +        cmd = spi_nor_find_best_erase_cmd(map, region, addr, len);
>>> +        if (!cmd)
>>> +            return -EINVAL;
>>
>> What would happen if you realize mid-way that you cannot erase some
>> sector , do you end up with partial erase ?
> 
> Is this possible? In non-overlaid regions, the address is aligned with
> at least one of the erase commands, else -EINVAL. For overlaid regions
> alignment doesn't matter. But yes, if this is possible, in this case,
> this proposal will do a partial erase.

Shouldn't we fail up front instead ?

-- 
Best regards,
Marek Vasut