Re: [PATCH v4 1/6] mtd: spi-nor: Add manufacturer read id function

[Pratyush Yadav <pratyush@kernel.org>]

On Wed, Sep 20 2023, Tudor Ambarus wrote:

> Hi, Jaime,
>
> Thanks for the patch! I think we are getting closer to have this
> integrated, but I have some concerns that hopefully with your help we'll
> address and move forward.
>
> On 08.09.2023 09:42, Jaime Liao wrote:
>> From: JaimeLiao <jaimeliao@mxic.com.tw>
>> 
>> Add manufacturer read id function because of some flash
>> may change data format when read id in octal dtr mode.
>
> I'm not convinced such a method is really needed, would you please
> elaborate the explanation why it's needed?
>
> I'm looking at the mx66lm1g45g datasheet. From what I see in "Figure 13.
> Read Identification (RDID) Sequence (DTR-OPI Mode)" looks like even if
> the flash is operated in 8d-8d-8d mode, what the flash actually uses is
> a 8d-8d-8s mode for the read id. Each ID byte is sent twice on both
> rising and falling edge of the clock, thus behaving like a 8d-8d-8s
> protocol.
>
> I see this flash supports 1-1-1, 8-8-8, and 8d-8d-8d, there are no mixed
> modes supported, thus a 8d-8d-8s mode seems just like a hardware bug to
> me. So my proposal is to leave the core away and to handle the read id
> hack just in the macronix driver.

+1

[...]
>> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
>> index eb149e517c1f..8ab47691dfbb 100644
>> --- a/drivers/mtd/spi-nor/macronix.c
>> +++ b/drivers/mtd/spi-nor/macronix.c
>> @@ -118,9 +118,27 @@ static int macronix_nor_late_init(struct spi_nor *nor)
>>  	return 0;
>>  }
>>  
>> +static int macronix_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
>> +				enum spi_nor_protocol proto)
>> +{
>> +	int i, ret;
>> +
>> +	ret = spi_nor_default_read_id(nor, naddr, ndummy, id, proto);
>> +	/* Retrieve odd array and re-sort id because of read id format will be A-A-B-B-C-C
>> +	 * after enter into octal dtr mode for Macronix flashes.
>> +	 */
>> +	if (proto == SNOR_PROTO_8_8_8_DTR) {
>> +		for (i = 0; i < nor->info->id_len; i++)
>> +			id[i] = id[i*2];
>
> why do you overwrite the id? How about just checking that
> id[i] == id[i + 1]? why do you care if you print an a-a-b-b-c-c id?

Because id_len == 3 so you should only treat the ID as a-a-b. When you
memcmp() the ID after reading it, you would not get a match.

>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static const struct spi_nor_fixups macronix_nor_fixups = {
>>  	.default_init = macronix_nor_default_init,
>>  	.late_init = macronix_nor_late_init,
>> +	.read_id = macronix_nor_read_id,
>>  };
>>  
>>  const struct spi_nor_manufacturer spi_nor_macronix = {

-- 
Regards,
Pratyush Yadav

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