[PATCH v4 0/2] mtd: spi-nor: Add possibility to disable chip erase

[PATCH v4 0/2] mtd: spi-nor: Add possibility to disable chip erase

[Marcin Krzeminski]

v4:
* Rebased to git://github.com/spi-nor/linux.git next branch.
* Typos corrected.
v3:
* Fix/reword commit messages.
v2:
* Rebased to git://github.com/spi-nor/linux.git

Marcin Krzeminski (2):
  mtd: spi-nor: Fix whole chip erasing for stacked chips.
  mtd: spi-nor: Disable chip erase for Micron n25q00.

 drivers/mtd/spi-nor/spi-nor.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.9.3

[PATCH v4 1/2] mtd: spi-nor: Fix whole chip erasing for stacked chips.

[Marcin Krzeminski]

Currently it is possible to disable chip erase for spi-nor driver.
Some modern stacked (multi die) flash chips do not support chip
erase opcode at all but spi-nor framework needs to cope with them too.
This commit extends existing functionality to allow disable
chip erase for a single flash chip.

Signed-off-by: Marcin Krzeminski <mar.krzeminski@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 2a643a1..595de54 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -85,6 +85,7 @@ struct flash_info {
 					 * Use dedicated 4byte address op codes
 					 * to support memory size above 128Mib.
 					 */
+#define NO_CHIP_ERASE		BIT(12) /* Chip does not support chip erase */
 };
 
 #define JEDEC_MFR(info)	((info)->id[0])
@@ -1621,6 +1622,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 		nor->flags |= SNOR_F_USE_FSR;
 	if (info->flags & SPI_NOR_HAS_TB)
 		nor->flags |= SNOR_F_HAS_SR_TB;
+	if (info->flags & NO_CHIP_ERASE)
+		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
 
 #ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
 	/* prefer "small sector" erase if possible */
-- 
2.9.3

Re: [PATCH v4 1/2] mtd: spi-nor: Fix whole chip erasing for stacked chips.

[Marek Vasut]

On 01/06/2017 06:19 PM, Marcin Krzeminski wrote:
> Currently it is possible to disable chip erase for spi-nor driver.
> Some modern stacked (multi die) flash chips do not support chip
> erase opcode at all but spi-nor framework needs to cope with them too.
> This commit extends existing functionality to allow disable
> chip erase for a single flash chip.

I wonder whether this should really be opt-out flag. Since we'll see
more multi-die chips (and chips with different die sizes), I'd say this
should rather be opt-in flag.

> Signed-off-by: Marcin Krzeminski <mar.krzeminski@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 2a643a1..595de54 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -85,6 +85,7 @@ struct flash_info {
>  					 * Use dedicated 4byte address op codes
>  					 * to support memory size above 128Mib.
>  					 */
> +#define NO_CHIP_ERASE		BIT(12) /* Chip does not support chip erase */
>  };
>  
>  #define JEDEC_MFR(info)	((info)->id[0])
> @@ -1621,6 +1622,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  		nor->flags |= SNOR_F_USE_FSR;
>  	if (info->flags & SPI_NOR_HAS_TB)
>  		nor->flags |= SNOR_F_HAS_SR_TB;
> +	if (info->flags & NO_CHIP_ERASE)
> +		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
>  
>  #ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
>  	/* prefer "small sector" erase if possible */
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH v4 1/2] mtd: spi-nor: Fix whole chip erasing for stacked chips.

[mar.krzeminski]

W dniu 07.01.2017 o 00:45, Marek Vasut pisze:
> On 01/06/2017 06:19 PM, Marcin Krzeminski wrote:
>> Currently it is possible to disable chip erase for spi-nor driver.
>> Some modern stacked (multi die) flash chips do not support chip
>> erase opcode at all but spi-nor framework needs to cope with them too.
>> This commit extends existing functionality to allow disable
>> chip erase for a single flash chip.
> I wonder whether this should really be opt-out flag. Since we'll see
> more multi-die chips (and chips with different die sizes), I'd say this
> should rather be opt-in flag.
Multi-die does not mean that chip will not support chip erase (we have 
already a multi-die
chips that still do chip erase). Please think about this patch more like 
a bug-fix not a feature.
More likely this will be flag kinda similar to eg. USE_FSR - for some 
manufacturers only.

We are far away for real die erase functionality in spi-nor framework 
(due to 4byte mode bootloaders issue).
Die erase could be covered in different series -  maybe with more advanced
erase procedure that takes biggest available erase size?

Thanks,
Marcin
>
>> Signed-off-by: Marcin Krzeminski <mar.krzeminski@gmail.com>
>> ---
>>   drivers/mtd/spi-nor/spi-nor.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 2a643a1..595de54 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -85,6 +85,7 @@ struct flash_info {
>>   					 * Use dedicated 4byte address op codes
>>   					 * to support memory size above 128Mib.
>>   					 */
>> +#define NO_CHIP_ERASE		BIT(12) /* Chip does not support chip erase */
>>   };
>>   
>>   #define JEDEC_MFR(info)	((info)->id[0])
>> @@ -1621,6 +1622,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>   		nor->flags |= SNOR_F_USE_FSR;
>>   	if (info->flags & SPI_NOR_HAS_TB)
>>   		nor->flags |= SNOR_F_HAS_SR_TB;
>> +	if (info->flags & NO_CHIP_ERASE)
>> +		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
>>   
>>   #ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
>>   	/* prefer "small sector" erase if possible */
>>
>

Re: [PATCH v4 1/2] mtd: spi-nor: Fix whole chip erasing for stacked chips.

[Cyrille Pitchen]

Hi Marek,

Le 07/01/2017 à 00:45, Marek Vasut a écrit :
> On 01/06/2017 06:19 PM, Marcin Krzeminski wrote:
>> Currently it is possible to disable chip erase for spi-nor driver.
>> Some modern stacked (multi die) flash chips do not support chip
>> erase opcode at all but spi-nor framework needs to cope with them too.
>> This commit extends existing functionality to allow disable
>> chip erase for a single flash chip.
> 
> I wonder whether this should really be opt-out flag. Since we'll see
> more multi-die chips (and chips with different die sizes), I'd say this
> should rather be opt-in flag.
>

Just to be sure I've understood what you propose: do you suggest something
like CAN_CHIP_ERASE instead of NO_CHIP_ERASE for the new info->flags bit?

If so, don't forget that info->flags value is tested for every single entry
of the spi_nor_ids[] array, including all legacy SPI memories such as AT25
or M25P80. Currently spi-nor.c calls erase_chip() from spi_nor_erase()
whenever the user tries to erase the whole memory (len == mtd->size),
whatever SPI memory is connected to the controller. This implementation is
not restricted to multi-die memory. Indeed, spi-nor.c is not currently
aware of multi-die memories.

Hence if we want an opt-in logic for the new flag and still want to be
backward compatible, we would have to add the new flag into each entry of
the spi_nor_ids[] array.

Also I asked Marcin to split his original patch into 2 parts because I
think there are 2 different topics:

1 - disabling chip erase command on memories which don't support this SPI
command. This is a bug fix: we can still provide this chip erase feature
using only many regular sector/block erase commands. For sure it is not
optimal but at least it works hence the bug is fixed.

Some multi-die memories are examples of devices concerned by the bug but
maybe they are not the only one. S3AN memories also suffer from this bug
and it has already been dealt by some Ricardo's patches few weeks ago.

2 - adding support to the die erase command: this one is an optimization so
for me, still useful but with a lower priority as we can still live without
it so this would let use more time to think about the proper way to
implement this feature. Also, as Marcin reported, we already have examples
of multi-die memories which do support the chip erase command.

Hence it shows us that (chip erase not supported != multi-die).

Best regards,

Cyrille

>> Signed-off-by: Marcin Krzeminski <mar.krzeminski@gmail.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 2a643a1..595de54 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -85,6 +85,7 @@ struct flash_info {
>>  					 * Use dedicated 4byte address op codes
>>  					 * to support memory size above 128Mib.
>>  					 */
>> +#define NO_CHIP_ERASE		BIT(12) /* Chip does not support chip erase */
>>  };
>>  
>>  #define JEDEC_MFR(info)	((info)->id[0])
>> @@ -1621,6 +1622,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>  		nor->flags |= SNOR_F_USE_FSR;
>>  	if (info->flags & SPI_NOR_HAS_TB)
>>  		nor->flags |= SNOR_F_HAS_SR_TB;
>> +	if (info->flags & NO_CHIP_ERASE)
>> +		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
>>  
>>  #ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
>>  	/* prefer "small sector" erase if possible */
>>
> 
> 

Re: [PATCH v4 1/2] mtd: spi-nor: Fix whole chip erasing for stacked chips.

[Marek Vasut]

On 01/10/2017 06:24 PM, Cyrille Pitchen wrote:
> Hi Marek,
> 
> Le 07/01/2017 à 00:45, Marek Vasut a écrit :
>> On 01/06/2017 06:19 PM, Marcin Krzeminski wrote:
>>> Currently it is possible to disable chip erase for spi-nor driver.
>>> Some modern stacked (multi die) flash chips do not support chip
>>> erase opcode at all but spi-nor framework needs to cope with them too.
>>> This commit extends existing functionality to allow disable
>>> chip erase for a single flash chip.
>>
>> I wonder whether this should really be opt-out flag. Since we'll see
>> more multi-die chips (and chips with different die sizes), I'd say this
>> should rather be opt-in flag.
>>
> 
> Just to be sure I've understood what you propose: do you suggest something
> like CAN_CHIP_ERASE instead of NO_CHIP_ERASE for the new info->flags bit?

Yes

> If so, don't forget that info->flags value is tested for every single entry
> of the spi_nor_ids[] array, including all legacy SPI memories such as AT25
> or M25P80. Currently spi-nor.c calls erase_chip() from spi_nor_erase()
> whenever the user tries to erase the whole memory (len == mtd->size),
> whatever SPI memory is connected to the controller. This implementation is
> not restricted to multi-die memory. Indeed, spi-nor.c is not currently
> aware of multi-die memories.
> 
> Hence if we want an opt-in logic for the new flag and still want to be
> backward compatible, we would have to add the new flag into each entry of
> the spi_nor_ids[] array.

Yeah

> Also I asked Marcin to split his original patch into 2 parts because I
> think there are 2 different topics:
> 
> 1 - disabling chip erase command on memories which don't support this SPI
> command. This is a bug fix: we can still provide this chip erase feature
> using only many regular sector/block erase commands. For sure it is not
> optimal but at least it works hence the bug is fixed.
> 
> Some multi-die memories are examples of devices concerned by the bug but
> maybe they are not the only one. S3AN memories also suffer from this bug
> and it has already been dealt by some Ricardo's patches few weeks ago.
> 
> 2 - adding support to the die erase command: this one is an optimization so
> for me, still useful but with a lower priority as we can still live without
> it so this would let use more time to think about the proper way to
> implement this feature. Also, as Marcin reported, we already have examples
> of multi-die memories which do support the chip erase command.
> 
> Hence it shows us that (chip erase not supported != multi-die).

I agree with this, my point was only about whether we should handle this
as opt-in or out-out .

> Best regards,
> 
> Cyrille
> 
>>> Signed-off-by: Marcin Krzeminski <mar.krzeminski@gmail.com>
>>> ---
>>>  drivers/mtd/spi-nor/spi-nor.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>> index 2a643a1..595de54 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -85,6 +85,7 @@ struct flash_info {
>>>  					 * Use dedicated 4byte address op codes
>>>  					 * to support memory size above 128Mib.
>>>  					 */
>>> +#define NO_CHIP_ERASE		BIT(12) /* Chip does not support chip erase */
>>>  };
>>>  
>>>  #define JEDEC_MFR(info)	((info)->id[0])
>>> @@ -1621,6 +1622,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>>  		nor->flags |= SNOR_F_USE_FSR;
>>>  	if (info->flags & SPI_NOR_HAS_TB)
>>>  		nor->flags |= SNOR_F_HAS_SR_TB;
>>> +	if (info->flags & NO_CHIP_ERASE)
>>> +		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
>>>  
>>>  #ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
>>>  	/* prefer "small sector" erase if possible */
>>>
>>
>>
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH v4 1/2] mtd: spi-nor: Fix whole chip erasing for stacked chips.

[Cyrille Pitchen]

Le 10/01/2017 à 18:44, Marek Vasut a écrit :
> On 01/10/2017 06:24 PM, Cyrille Pitchen wrote:
>> Hi Marek,
>>
>> Le 07/01/2017 à 00:45, Marek Vasut a écrit :
>>> On 01/06/2017 06:19 PM, Marcin Krzeminski wrote:
>>>> Currently it is possible to disable chip erase for spi-nor driver.
>>>> Some modern stacked (multi die) flash chips do not support chip
>>>> erase opcode at all but spi-nor framework needs to cope with them too.
>>>> This commit extends existing functionality to allow disable
>>>> chip erase for a single flash chip.
>>>
>>> I wonder whether this should really be opt-out flag. Since we'll see
>>> more multi-die chips (and chips with different die sizes), I'd say this
>>> should rather be opt-in flag.
>>>
>>
>> Just to be sure I've understood what you propose: do you suggest something
>> like CAN_CHIP_ERASE instead of NO_CHIP_ERASE for the new info->flags bit?
> 
> Yes
> 
>> If so, don't forget that info->flags value is tested for every single entry
>> of the spi_nor_ids[] array, including all legacy SPI memories such as AT25
>> or M25P80. Currently spi-nor.c calls erase_chip() from spi_nor_erase()
>> whenever the user tries to erase the whole memory (len == mtd->size),
>> whatever SPI memory is connected to the controller. This implementation is
>> not restricted to multi-die memory. Indeed, spi-nor.c is not currently
>> aware of multi-die memories.
>>
>> Hence if we want an opt-in logic for the new flag and still want to be
>> backward compatible, we would have to add the new flag into each entry of
>> the spi_nor_ids[] array.
> 
> Yeah
> 
>> Also I asked Marcin to split his original patch into 2 parts because I
>> think there are 2 different topics:
>>
>> 1 - disabling chip erase command on memories which don't support this SPI
>> command. This is a bug fix: we can still provide this chip erase feature
>> using only many regular sector/block erase commands. For sure it is not
>> optimal but at least it works hence the bug is fixed.
>>
>> Some multi-die memories are examples of devices concerned by the bug but
>> maybe they are not the only one. S3AN memories also suffer from this bug
>> and it has already been dealt by some Ricardo's patches few weeks ago.
>>
>> 2 - adding support to the die erase command: this one is an optimization so
>> for me, still useful but with a lower priority as we can still live without
>> it so this would let use more time to think about the proper way to
>> implement this feature. Also, as Marcin reported, we already have examples
>> of multi-die memories which do support the chip erase command.
>>
>> Hence it shows us that (chip erase not supported != multi-die).
> 
> I agree with this, my point was only about whether we should handle this
> as opt-in or out-out .
> 

OK, then maybe we should check the datasheets of more multi-die memories
from different manufacturers and maybe even wait for more of them to be
released to find out whether most of them will support the chip erase
command or not.

My guess is that the majority will support that command because I expect
manufacturers to release new products backward compatible with older
products. Hence the case of Micron n25q00 should be only one a the few
exceptions to the rule. However I have absolutely no mean to be sure that
manufacturers will follow the Macronix example (chip erase supported)
rather than the Micron example (chip erase not supported). It's a bit hard
to define a global rule from only two different manufacturers.

That's said, I not really fond of adding a flag into 99.9% of the entries
of the spi_nor_ids[]. I would rather put a flag into the remaining 0.1%.

Let's think about the SPI_NOR_NO_FR (Fast Read 0Bh not supported, Read 03h
only): this is another opt-out flag because most SPI NOR memories support
Fast Read commands.

However, if manufacturers decide to break with the past and no longer
provide the chip erase command then I agree with you, it might be
interesting to study the opt-in approach. In such a case, I think we could
also define some new macro close to the existing INFO() macro: we would add
the opt-in flag directly inside the definition of the existing INFO() macro
since legacy memories support the chip erase command whereas this opt-in
flag would not be set in the other macro used add new entries for future
memories (single/multi-die or whatever) not supporting the chip erase.
Otherwise, the new entries of the spi_nor_ids[] array would be far too long.
Currently there are already many entries that don't respect the "80 column"
rule due to so numerous flags (SECT_4K, SPI_NOR_QUAD_READ, USE_FSR, ...) so
maybe we should avoid introducing a flag that would increase the length of
most entries even more.

I don't like that much hiding stuff into macro definitions but breaking
entries into many lines makes the spi_nor_ids[] array even bigger. At some
point maybe we should think about extracting this array from spi-nor.c then
putting it into a dedicated file, which would be then included in
spi-nor.c, just for a readiness purpose. However this is another topic not
related this series :)

>> Best regards,
>>
>> Cyrille
>>
>>>> Signed-off-by: Marcin Krzeminski <mar.krzeminski@gmail.com>
>>>> ---
>>>>  drivers/mtd/spi-nor/spi-nor.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>>> index 2a643a1..595de54 100644
>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>> @@ -85,6 +85,7 @@ struct flash_info {
>>>>  					 * Use dedicated 4byte address op codes
>>>>  					 * to support memory size above 128Mib.
>>>>  					 */
>>>> +#define NO_CHIP_ERASE		BIT(12) /* Chip does not support chip erase */
>>>>  };
>>>>  
>>>>  #define JEDEC_MFR(info)	((info)->id[0])
>>>> @@ -1621,6 +1622,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>>>  		nor->flags |= SNOR_F_USE_FSR;
>>>>  	if (info->flags & SPI_NOR_HAS_TB)
>>>>  		nor->flags |= SNOR_F_HAS_SR_TB;
>>>> +	if (info->flags & NO_CHIP_ERASE)
>>>> +		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
>>>>  
>>>>  #ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
>>>>  	/* prefer "small sector" erase if possible */
>>>>
>>>
>>>
>>
> 
> 

Re: [PATCH v4 1/2] mtd: spi-nor: Fix whole chip erasing for stacked chips.

[Marek Vasut]

On 01/11/2017 12:03 PM, Cyrille Pitchen wrote:
> Le 10/01/2017 à 18:44, Marek Vasut a écrit :
>> On 01/10/2017 06:24 PM, Cyrille Pitchen wrote:
>>> Hi Marek,
>>>
>>> Le 07/01/2017 à 00:45, Marek Vasut a écrit :
>>>> On 01/06/2017 06:19 PM, Marcin Krzeminski wrote:
>>>>> Currently it is possible to disable chip erase for spi-nor driver.
>>>>> Some modern stacked (multi die) flash chips do not support chip
>>>>> erase opcode at all but spi-nor framework needs to cope with them too.
>>>>> This commit extends existing functionality to allow disable
>>>>> chip erase for a single flash chip.
>>>>
>>>> I wonder whether this should really be opt-out flag. Since we'll see
>>>> more multi-die chips (and chips with different die sizes), I'd say this
>>>> should rather be opt-in flag.
>>>>
>>>
>>> Just to be sure I've understood what you propose: do you suggest something
>>> like CAN_CHIP_ERASE instead of NO_CHIP_ERASE for the new info->flags bit?
>>
>> Yes
>>
>>> If so, don't forget that info->flags value is tested for every single entry
>>> of the spi_nor_ids[] array, including all legacy SPI memories such as AT25
>>> or M25P80. Currently spi-nor.c calls erase_chip() from spi_nor_erase()
>>> whenever the user tries to erase the whole memory (len == mtd->size),
>>> whatever SPI memory is connected to the controller. This implementation is
>>> not restricted to multi-die memory. Indeed, spi-nor.c is not currently
>>> aware of multi-die memories.
>>>
>>> Hence if we want an opt-in logic for the new flag and still want to be
>>> backward compatible, we would have to add the new flag into each entry of
>>> the spi_nor_ids[] array.
>>
>> Yeah
>>
>>> Also I asked Marcin to split his original patch into 2 parts because I
>>> think there are 2 different topics:
>>>
>>> 1 - disabling chip erase command on memories which don't support this SPI
>>> command. This is a bug fix: we can still provide this chip erase feature
>>> using only many regular sector/block erase commands. For sure it is not
>>> optimal but at least it works hence the bug is fixed.
>>>
>>> Some multi-die memories are examples of devices concerned by the bug but
>>> maybe they are not the only one. S3AN memories also suffer from this bug
>>> and it has already been dealt by some Ricardo's patches few weeks ago.
>>>
>>> 2 - adding support to the die erase command: this one is an optimization so
>>> for me, still useful but with a lower priority as we can still live without
>>> it so this would let use more time to think about the proper way to
>>> implement this feature. Also, as Marcin reported, we already have examples
>>> of multi-die memories which do support the chip erase command.
>>>
>>> Hence it shows us that (chip erase not supported != multi-die).
>>
>> I agree with this, my point was only about whether we should handle this
>> as opt-in or out-out .
>>
> 
> OK, then maybe we should check the datasheets of more multi-die memories
> from different manufacturers and maybe even wait for more of them to be
> released to find out whether most of them will support the chip erase
> command or not.
> 
> My guess is that the majority will support that command because I expect
> manufacturers to release new products backward compatible with older
> products. Hence the case of Micron n25q00 should be only one a the few
> exceptions to the rule. However I have absolutely no mean to be sure that
> manufacturers will follow the Macronix example (chip erase supported)
> rather than the Micron example (chip erase not supported). It's a bit hard
> to define a global rule from only two different manufacturers.
> 
> That's said, I not really fond of adding a flag into 99.9% of the entries
> of the spi_nor_ids[]. I would rather put a flag into the remaining 0.1%.

Yes, I agree on this one.

> Let's think about the SPI_NOR_NO_FR (Fast Read 0Bh not supported, Read 03h
> only): this is another opt-out flag because most SPI NOR memories support
> Fast Read commands.

The fast-read opcode is standard, while multi-die is only becoming a
thing nowadays. I don't think this is comparable.

> However, if manufacturers decide to break with the past and no longer
> provide the chip erase command then I agree with you, it might be
> interesting to study the opt-in approach. In such a case, I think we could
> also define some new macro close to the existing INFO() macro: we would add
> the opt-in flag directly inside the definition of the existing INFO() macro
> since legacy memories support the chip erase command whereas this opt-in
> flag would not be set in the other macro used add new entries for future
> memories (single/multi-die or whatever) not supporting the chip erase.
> Otherwise, the new entries of the spi_nor_ids[] array would be far too long.
> Currently there are already many entries that don't respect the "80 column"
> rule due to so numerous flags (SECT_4K, SPI_NOR_QUAD_READ, USE_FSR, ...) so
> maybe we should avoid introducing a flag that would increase the length of
> most entries even more.

There's another option, make it opt-out for now and wait how the
situation develops. If things tilt the other way later, we can always
change the flag to opt-in since it's an internal thing.

> I don't like that much hiding stuff into macro definitions but breaking
> entries into many lines makes the spi_nor_ids[] array even bigger. At some
> point maybe we should think about extracting this array from spi-nor.c then
> putting it into a dedicated file, which would be then included in
> spi-nor.c, just for a readiness purpose. However this is another topic not
> related this series :)

Agreed

>>> Best regards,
>>>
>>> Cyrille
>>>
>>>>> Signed-off-by: Marcin Krzeminski <mar.krzeminski@gmail.com>
>>>>> ---
>>>>>  drivers/mtd/spi-nor/spi-nor.c | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>>>> index 2a643a1..595de54 100644
>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>>> @@ -85,6 +85,7 @@ struct flash_info {
>>>>>  					 * Use dedicated 4byte address op codes
>>>>>  					 * to support memory size above 128Mib.
>>>>>  					 */
>>>>> +#define NO_CHIP_ERASE		BIT(12) /* Chip does not support chip erase */
>>>>>  };
>>>>>  
>>>>>  #define JEDEC_MFR(info)	((info)->id[0])
>>>>> @@ -1621,6 +1622,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>>>>  		nor->flags |= SNOR_F_USE_FSR;
>>>>>  	if (info->flags & SPI_NOR_HAS_TB)
>>>>>  		nor->flags |= SNOR_F_HAS_SR_TB;
>>>>> +	if (info->flags & NO_CHIP_ERASE)
>>>>> +		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
>>>>>  
>>>>>  #ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
>>>>>  	/* prefer "small sector" erase if possible */
>>>>>
>>>>
>>>>
>>>
>>
>>
> 


-- 
Best regards,
Marek Vasut

[PATCH v4 2/2] mtd: spi-nor: Disable chip erase for Micron n25q00.

[Marcin Krzeminski]

Micron n25q00 are stacked chips, thus do not support chip erase.
>From now spi-nor framework will not send chip erase command,
instead will use sector at time erase procedure.

Signed-off-by: Marcin Krzeminski <mar.krzeminski@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 595de54..0d013fd 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1026,8 +1026,8 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K | SPI_NOR_QUAD_READ) },
 	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
 	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
-	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
-	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
+	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
+	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
 
 	/* PMC */
 	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
-- 
2.9.3

Re: [PATCH v4 0/2] mtd: spi-nor: Add possibility to disable chip erase

[Cyrille Pitchen]

Le 06/01/2017 à 18:18, Marcin Krzeminski a écrit :
> v4:
> * Rebased to git://github.com/spi-nor/linux.git next branch.
> * Typos corrected.
> v3:
> * Fix/reword commit messages.
> v2:
> * Rebased to git://github.com/spi-nor/linux.git
> 
> Marcin Krzeminski (2):
>   mtd: spi-nor: Fix whole chip erasing for stacked chips.
>   mtd: spi-nor: Disable chip erase for Micron n25q00.
> 
>  drivers/mtd/spi-nor/spi-nor.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
series applied to github/spi-nor

Thanks!