Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim

From: Michael Walle <michael@walle.cc>
To: Tudor.Ambarus@microchip.com
Cc: richard@nod.at, miquel.raynal@bootlin.com,
	linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
	vigneshr@ti.com
Subject: Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim
Date: Sun, 19 Jan 2020 23:24:25 +0100	[thread overview]
Message-ID: <66c1ad8e74fb20a061f35f8b23a925ab@walle.cc> (raw)
In-Reply-To: <8021667.67K7kvUAe6@192.168.0.113>

Hi Tudor,

>> Am 2020-01-13 11:07, schrieb Michael Walle:
>> >>> Btw. is renaming the flashes also considered a backwards incomaptible
>> >>> change?
>> >>
>> >> No, we can fix the names.
>> >>
>> >>> And can there be two flashes with the same name? Because IMHO it
>> >>> would
>> >>> be
>> >>
>> >> I would prefer that we don't. Why would you have two different
>> >> jedec-ids with
>> >> the same name?
>> >
>> > Because as pointed out in the Winbond example you cannot distiguish
>> > between
>> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005
>> > and
>> > MX25L8006E. Thus my reasoning was to show only the common part, ie
>> > W25Q32
>> > or MX25L80 which should be the same for this particular ID. Like I
>> > said, I'd
>> > prefer showing an ambiguous name instead of a wrong one. But then you
>> > may
>> > have different IDs with the same ambiguous name.
>> 
>> Another solution would be to have the device tree provide a hint for 
>> the
>> actual flash chip. There would be multiple entries in the spi_nor_ids
>> with the
>> same flash id. By default the first one is used (keeping the current
>> behaviour). If there is for example
>> 
>>    compatible = "jedec,spi-nor", "w25q32jwq";
>> 
>> the flash_info for the w25q32jwq will be chosen.
> 
> This won't work for plug-able flashes. You will influence the name in 
> dt to be
> chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you will 
> end up
> with a wrong name for w25q32dw, thus the same problem.

No, because then the device tree is wrong and doesn't fit the hardware. 
You'd
have to some instance which could change the device tree node, like the
bootloader or some device tree overlay for plugable flashes. We should 
try to
solve the actual problem at hand first..

It is just not possible to autodetect the SPI flash, just because
the vendors reuse the same IDs for flashes with different features (and 
the
SFDP is likely not enough). Therefore, you need to have a hint in some 
place
to use the flash properly.

> If the flashes are identical but differ just in terms of name, we can 
> rename
> the flash to "w25q32jwq (w25q32dw)". I haven't studied the differences 
> between
> these flashes; if you want to fix them, send a patch and I'll try to 
> help.

It is not only the name, here are two examples which differ in 
functionality:
  (1) mx25l8005 doesn't support dual/quad mode. mx25l8006e supports 
dual/quad
      mode
  (2) mx25u3235f doesn't support TB bit, mx25u3232e has a TB bit.

well.. to repeat myself, the mx25l25635_post_bfpt_fixups is a third 
example.

-michael

> 
> Cheers,
> ta
> 
>> 
>> I know this will conflict with the new rule that there should only be
>> 
>>    compatible = "jedec,spi-nor";
>> 
>> without the actual flash chip. But it seems that it is not always
>> possible
>> to just use the jedec id to match the correct chip.
>> 
>> Also see for example mx25l25635_post_bfpt_fixups() which tries to 
>> figure
>> out different behaviour by looking at "some" SFDP data. In this case 
>> we
>> might have been lucky, but I fear that this won't work in all cases 
>> and
>> for older flashes it won't work at all.
>> 
>> BTW I do not suggest to add the strings to the the spi_nor_dev_ids[].
>> 
>> I guess that would be a less invasive way to fix different flashes 
>> with
>> same jedec ids.
>> 
>> -michael

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