Re: [PATCH 1/2] mtd: spi-nor: check 4-byte address support when parsing 4bait

[Yicong Yang <yangyicong@xxxxxxxxxxxxx>]

Hi Pratyush,

Thanks for the comments. Some replies below.

On 2021/1/29 19:16, Pratyush Yadav wrote:
> Hi Yicong,
> 
> On 27/01/21 05:40PM, Yicong Yang wrote:
>> The spi-nor core will convert the address mode to 4-btye,
>> without checking whether 4-byte address is supported or not.
>> For example, the 16M s25fs128s1 can work under both 3-byte
>> and 4-byte address and provides a 4bait table. The spi-nor
>> will drive the flash under 4-byte address mode after parsing
>> the 4bait and will cause it unusable on platforms doesn't
>> support 4-byte.
> 
> Another problem caused by 4BAIT parser prematurely selecting the address 
> width. See [0].
>  
> Let's fix this 4BAIT problem once and for all. Instead of setting 
> nor->addr_width to 4 in spi_nor_parse_4bait(), just set SNOR_F_HAS_4BAIT 
> (which is already being done). Then in spi_nor_default_setup(), use this 
> information when negotiating the read/write/program commands with the 
> controller to determine the correct address width to use.

The idea is great. ideally we should check the buswidth and commands in
spi_nor_default_setup() and finally set address width in
spi_nor_set_addr_width().

> 
> This refactor is easier said than done. We don't associate address width 
> information with a command. Just protocol, opcode, and dummy cycles 
> (only for read commands). A new mechanism needs to be added where we 
> associate a set of supported addresses with a command and then the 
> command negotiation can use all this information to arrive at the 
> optimal set of commands.
> 
> With this in mind, it would be great if you can come up with a patch to 
> add such a mechanism. But I would also be OK with this fix with the 
> condition that it is clearly marked as a temporary fix, and mentions 
> what should ideally be done.
> 

i'd like to get this issue fixed first to make the flash work properly,
if possible. then to do the refactor as it may take me sometime.

>> Add checking of 4-byte address support when parsing the 4bait
>> table, stop converting the address mode if it's not supported.
>>
>> Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
>> ---
>>  drivers/mtd/spi-nor/sfdp.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index 6ee7719..fdafc9b 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -940,6 +940,27 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,
>>  	return ret;
>>  }
>>  
>> +static int spi_nor_spimem_check_4byte_addr(struct spi_nor *nor,
>> +					   const struct spi_nor_read_command *read)
>> +{
>> +	struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(read->opcode, 1),
>> +					  SPI_MEM_OP_ADDR(4, 0, 1),
>> +					  SPI_MEM_OP_DUMMY(0, 1),
>> +					  SPI_MEM_OP_DATA_IN(0, NULL, 1));
> 
> Set buswidths to 0 here...

ok.

> 
>> +
>> +	op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(read->proto);
>> +	op.addr.buswidth = spi_nor_get_protocol_addr_nbits(read->proto);
>> +	op.data.buswidth = spi_nor_get_protocol_data_nbits(read->proto);
>> +	op.dummy.buswidth = op.addr.buswidth;
> 
> ... and use spi_nor_spimem_setup_op() to do this. It will also take care 
> of setting up DTR ops.

will change to the new function, i didn't notice it. :)

Thanks,
Yicong

> 
>> +	op.dummy.nbytes = (read->num_mode_clocks + read->num_wait_states) *
>> +			  op.dummy.buswidth / 8;
>> +
>> +	if (!spi_mem_supports_op(nor->spimem, &op))
>> +		return -EOPNOTSUPP;
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * spi_nor_parse_4bait() - parse the 4-Byte Address Instruction Table
>>   * @nor:		pointer to a 'struct spi_nor'.
>> @@ -1061,6 +1082,33 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
>>  		goto out;
>>  
>>  	/*
>> +	 * Check whether the 4-byte address is supported before converting
>> +	 * the instruction set to 4-byte.
>> +	 */
>> +	if (nor->spimem) {
>> +		bool support = false;
>> +
>> +		for (i = 0; i < SNOR_CMD_READ_MAX; i++) {
>> +			struct spi_nor_read_command read_cmd;
>> +
>> +			memcpy(&read_cmd, &params->reads[i], sizeof(read_cmd));
>> +
>> +			read_cmd.opcode = spi_nor_convert_3to4_read(read_cmd.opcode);
>> +			if (!spi_nor_spimem_check_4byte_addr(nor, &read_cmd)) {
>> +				support = true;
>> +				break;
>> +			}
>> +		}
>> +
>> +		/*
>> +		 * No supported 4-byte instruction is found, stop parsing the
>> +		 * 4bait table.
>> +		 */
>> +		if (!support)
>> +			goto out;
>> +	}
>> +
>> +	/*
>>  	 * Discard all operations from the 4-byte instruction set which are
>>  	 * not supported by this memory.
>>  	 */
> 
> [0] https://lore.kernel.org/linux-mtd/20201212115817.5122-1-vigneshr@xxxxxx/
>