Re: [PATCH] mtd: spi-nor: sfdp: Fix wrong addr length and dummy when perform SMPT detection command

[Yicong Yang <yangyicong@xxxxxxxxxxxxx>]

Hi Pratyush,


On 2020/4/21 19:44, Pratyush Yadav wrote:
> On 21/04/20 11:30AM, Yicong Yang wrote:
>> As per SFDP(JESD216D, Section 6.5.3) says of SMPT 1st DWORD, 11b of
>> bit[23:22] and 1111b of bit[19:16] represent variable
>> {address length, read latency}, which means "the {address length,
>> read latency} last set in memory device and this same value is used in the
>> configuration dectection command". Currently we use address length
>> and dummy byte of struct spi_nor in such conditions. But the value
>> are 0 as they are not set at the time, which will lead to
>> wrong perform of the dectection command.
>>
>> As the last command is read SFDP(1S-1S-1S, the only mode we use in kernel),
>> with 3-byte address and 8 dummy cycles, use the same values in
>> variable situations to perform correct dectection command.
>>
>> Fixes: b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
>> Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> FWIW, I posted a fix for the address width problem here [0], though in a 
> slightly different way. I'll re-roll the series soon, since it is based 
> on the code structure before the split.
>
>> ---
>>  drivers/mtd/spi-nor/sfdp.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index f6038d3..27a8faa 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -624,7 +624,7 @@ static u8 spi_nor_smpt_addr_width(const struct spi_nor *nor, const u32 settings)
>>  		return 4;
>>  	case SMPT_CMD_ADDRESS_LEN_USE_CURRENT:
>>  	default:
>> -		return nor->addr_width;
>> +		return 3;
> If in the future someone implements SFDP parsing in 8D-8D-8D mode, they 
> would want to use an address length of 4. Similarly, if someone has a 
> use-case where they have to configure their flash to a 4-byte addressing 
> mode in a non-volatile way, they would set nor->addr_width in a 
> flash-specific hook (like the post-BFPT hook) and would expect that 
> width to be used here as well.
>
> So I think instead of setting it in stone like this, we should instead 
> set the default nor->addr_width to 3 if it is configurable, and then let 
> flashes fix it up if they need to. This is what my patch [0] does.

As I mentioned in the commit, *currently* we use 1S-1S-1S mode to read SFDP, which is also
the last operation we did here. So I use the same address byte and dummy cycles here.
I think we won't meet the exception you mentioned, as it will fail when parsing SFDP
and won't come here.

I agree that currently we don't meet all the conditions. But your patch seems alike the way
here. AS you set address width when parsing BFPT, but it'll not be changed before parsing SMPT
here, we parse smpt once bfpt is parsed and don't do specific vendor operations, etc.


>
>>  	}
>>  }
>>
>> @@ -641,7 +641,7 @@ static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
>>  	u8 read_dummy = SMPT_CMD_READ_DUMMY(settings);
>>
>>  	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
>> -		return nor->read_dummy;
>> +		return 8;
> Same comment here. Set nor->read_dummy to a sane default (== 8) and then 
> let flash-specific hooks change it if they need to. This is tricky 
> though, since BFPT doesn't tell us the dummy cycles needed. Still, I 
> think if we set it in say spi_nor_parse_smpt(), it would be a bit more 
> flexible.

As I've mentioned above, it follows the behaviour in the current framework. What do you think of
introduce new field in spi_nor to provide more flexibility:

    struct spi_nor {
        ...
        sfdp_addr_width;
        sfdp_read_dummy;
        ...
    }

we can initialized it in the quite beginning(perhap in spi_nor_scan()) and let others to modify it.
It will also provide sfdp access and flash access with different parameter if necessary.


>>  	return read_dummy;
>>  }
> [0] https://lore.kernel.org/linux-mtd/20200313154645.29293-6-p.yadav@xxxxxx/
>


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