Re: [PATCH v17 5/7] mtd: spi-nor: core: Track flash's internal address mode

From: <Tudor.Ambarus@microchip.com>
To: <michael@walle.cc>
Cc: <p.yadav@ti.com>, <tkuw584924@gmail.com>,
	<Takahiro.Kuwano@infineon.com>, <linux-mtd@lists.infradead.org>,
	<miquel.raynal@bootlin.com>, <richard@nod.at>, <vigneshr@ti.com>,
	<Bacem.Daassi@infineon.com>
Subject: Re: [PATCH v17 5/7] mtd: spi-nor: core: Track flash's internal address mode
Date: Wed, 27 Jul 2022 12:51:42 +0000	[thread overview]
Message-ID: <4e940119-de23-4052-1a48-9190c8a1b300@microchip.com> (raw)
In-Reply-To: <97493d572d27d1b9d2102d0110cc7b70@walle.cc>

On 7/27/22 14:12, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-07-25 11:25, schrieb Tudor Ambarus:
>> We need to track the flash's internal address mode as there are flashes
>> that can operate with 4B opcodes but unfortunately do not have a 4B
>> opcode
>> correspondent for all the 3B opcodes. Such an example is the Infineon
>> Semper chips which provide 4B opcodes for read/program/erase but do not
>> provide 4B opcodes for Read/Write Any Register. These registers are
>> indexed by address and require the internal address mode of the flash
>> before Read/Write Any Register opcodes are issued.
>> 4B opcodes are preferred over changing the flash's address mode to
>> 4byte,
>> as set_4byte_addr_mode could be done in a non-volatile way and could
>> break
>> the boot sequence. Thus we need to track the flash's internal address
>> mode
>> so that we can use 4B opcodes together with opcodes that don't have a
>> 4B
>> opcode correspondent. Track flash's internal address mode.
>>
>> addr_mode_nbytes is discovered when parsing BFPT. For the
>> BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 case, one could introduce a method
>> that
>> queries the flash's internal address mode at run-time (works for
>> Winbond).
>> If a run-time querying can not be accomplished or if SFDP is not
>> defined
>> at all, but the address mode is volatile and resets to a default known
>> value at boot, one can change the default addr_mode_nbytes value of 3
>> by
>> introducing a flash_info flag. If the address mode can not be queried,
>> discovered and it is configured via a non-volatile register, we may
>> introduce a dt property, but it will harm the generic approach of the
>> jedec,spi-nor compatible. All this complexity is not needed now, so let
>> it
>> for future development.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi-nor/core.h | 5 +++++
>>  drivers/mtd/spi-nor/sfdp.c | 6 +++---
>>  2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 7dc4cda41db3..85b0cf254e97 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -341,6 +341,10 @@ struct spi_nor_otp {
>>   *                   ECC unit size for ECC-ed flashes.
>>   * @page_size:               the page size of the SPI NOR flash memory.
>>   * @addr_nbytes:     number of address bytes to send.
>> + * @addr_mode_nbytes:        number of address bytes of current address mode.
>> Useful
>> + *                   when the flash operates with 4B opcodes but needs the
>> + *                   internal address mode for opcodes that don't have a 4B
>> + *                   opcode correspondent.
>>   * @rdsr_dummy:              dummy cycles needed for Read Status Register command
>>   *                   in octal DTR mode.
>>   * @rdsr_addr_nbytes:        dummy address bytes needed for Read Status
>> Register
>> @@ -374,6 +378,7 @@ struct spi_nor_flash_parameter {
>>       u32                             writesize;
>>       u32                             page_size;
>>       u8                              addr_nbytes;
>> +     u8                              addr_mode_nbytes;
>>       u8                              rdsr_dummy;
>>       u8                              rdsr_addr_nbytes;
>>
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index 3a48173a2d78..c7973368f5dc 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -462,11 +462,11 @@ static int spi_nor_parse_bfpt(struct spi_nor
>> *nor,
>>       switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK)
>> {
>>       case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:
>>       case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
>> -             params->addr_nbytes = 3;
>> +             params->addr_mode_nbytes = params->addr_nbytes = 3;
> 
> I'd suggest to just use two lines here because you still can do a git
> blame and (maybe) see what the purpose of addr_nbytes was. But I
> don't have an objection here. You might or might not change it while
> committing.

will change, thanks for reviewing it!

> 
> Reviewed-by: Michael Walle <michael@walle.cc>
> 
>>               break;
>>
>>       case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
>> -             params->addr_nbytes = 4;
>> +             params->addr_mode_nbytes = params->addr_nbytes = 4;
>>               break;
>>
>>       default:
>> @@ -653,7 +653,7 @@ static u8 spi_nor_smpt_addr_nbytes(const struct
>> spi_nor *nor, const u32 settings
>>               return 4;
>>       case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
>>       default:
>> -             return nor->params->addr_nbytes;
>> +             return nor->params->addr_mode_nbytes;
>>       }
>>  }

-- 
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/