Re: [PATCH 1/2] mtd: spi-nor: Introduce erase_proto

[Tudor.Ambarus]

On 12/16/21 22:05, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Alexander,
> 
> On 09/12/21 11:08AM, Alexander A Sverdlin wrote:
>> From: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>
>>
>> I've been looking into non-working erase on mt25qu256a and pinpointed it to
>> be write_proto 1-4-4 selected from SFDP while the chip only supports 1-1-0
>> erase.
>>
>> For now just introduce the separate protocol without functional change and
>> leave the real fix for the following patch.
>>
>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>
>> ---
>>  drivers/mtd/spi-nor/core.c  | 9 ++++++---
>>  include/linux/mtd/spi-nor.h | 4 +++-
>>  2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 2e21d5a..dcd02ea 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -177,7 +177,7 @@ static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
>>
>>  static int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs)
>>  {
>> -     if (spi_nor_protocol_is_dtr(nor->write_proto))
>> +     if (spi_nor_protocol_is_dtr(nor->erase_proto))
>>               return -EOPNOTSUPP;
>>
>>       return nor->controller_ops->erase(nor, offs);
>> @@ -1186,7 +1186,7 @@ static int spi_nor_erase_chip(struct spi_nor *nor)
>>                                  SPI_MEM_OP_NO_DUMMY,
>>                                  SPI_MEM_OP_NO_DATA);
>>
>> -             spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
>> +             spi_nor_spimem_setup_op(nor, &op, nor->erase_proto);
>>
>>               ret = spi_mem_exec_op(nor->spimem, &op);
>>       } else {
>> @@ -1331,7 +1331,7 @@ int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>>                                  SPI_MEM_OP_NO_DUMMY,
>>                                  SPI_MEM_OP_NO_DATA);
>>
>> -             spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
>> +             spi_nor_spimem_setup_op(nor, &op, nor->erase_proto);
>>
>>               return spi_mem_exec_op(nor->spimem, &op);
>>       } else if (nor->controller_ops->erase) {
>> @@ -2727,6 +2727,9 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
>>        */
>>       if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops)
>>               spi_nor_init_default_locking_ops(nor);
>> +
>> +     if (!nor->erase_proto)
>> +             nor->erase_proto = nor->write_proto;
> 
> I get that you are trying to not break any existing flashes with this,
> but I don't quite like it. We should keep the same initialization flow
> with erase_proto as with write_proto, read_proto, etc. That is,
> initialize it to SNOR_PROTO_1_1_1 in spi_nor_scan() and then let the
> initialization procedure change it as needed.
> 
> The problem with this is of course that it could break some flashes by
> selecting the wrong erase. I would expect _most_ flashes to use
> erase_proto as 1-1-1 but I of course haven't went and looked at every
> single flash to point out the exceptions.
> 
> I would like to hear from others if they think it is okay to do this.
> 

Doesn't [1] solve Alexander's problem? Alexander, would you please test
Patrice's patch and provide a Tested-by tag if everything is ok?

Thanks,
ta

[1] https://patchwork.ozlabs.org/project/linux-mtd/patch/20220629133013.3382393-1-patrice.chotard@xxxxxxxxxxx/