Re: [PATCH 2/3] mtd: spi-nor: use rdid-dummy-ncycles DT property

[Takahiro Kuwano <tkuw584924@gmail.com>]

On 3/21/2025 12:45 AM, Tudor Ambarus wrote:
> 
> 
> On 3/20/25 2:06 PM, Rob Herring wrote:
>> On Thu, Mar 20, 2025 at 2:44 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>
>>> Hi, Rob,
>>>
>>> On 3/19/25 11:30 PM, Rob Herring wrote:
>>>> On Wed, Mar 19, 2025 at 06:47:44PM +0900, Takahiro Kuwano wrote:
>>>>> There are infineon flashes [1] that require 8 dummy cycles for the
>>>>> 1-1-1 Read ID command. Since the command is not covered by JESD216
>>>>> or any other standard, get the number of dummy cycles from DT and use
>>>>> them to correctly identify the flash.
>>>>
>>>> If Read ID fails, then couldn't you just retry with dummy cycles? Or
>>>
>>> I think Read ID won't fail when the op requires 8 dummy cycles, it
>>> probably just reads garbage on the first 8 cycles, so we risk to wrongly
>>> match other flash IDs.
>>>
>>>> would unconditionally adding dummy cycles adversely affect other chips?
>>>
>>> Adding 8 dummy cycles to chips that don't need it, would mean ignoring
>>> the first byte of the flash ID, thus we again risk to wrongly match
>>> against other flash IDs.
>>>
>>>>
>>>> Otherwise, add a specific compatible to imply this requirement. Adding
>>>> quirk properties doesn't scale.
>>>
>>> Do you mean a flash name compatible, like "cyrs17b512,spi-nor"?
>>
>> Yes, but that's not the format of compatible strings.
>>
>>> The
>>> problem that I see with that is that we no longer bind against the
>>> generic jedec,spi-nor compatible, so people need to update their DT in
>>> case they use/plug-in a different flash on their board.
>>
>> This chip is clearly *not* compatible with a generic chip.
> 
> I think it is compatible. The chip defines the SFDP (serial flash
> discoverable parameters) tables. At probe time we parse those tables and
> initialize the flash based on them.
> 
> We don't even care about the chip ID, if all the flash parameters can be
> discovered via SFDP. Unfortunately these tables do not describe all the
> flash capabilities (block protection being one). Or worse, manufacturers
> mangle these tables.
> 
> So vendors need to identify chips to either fix those tables via some
> quirks after the parsing is done, or to specify support that's not
> covered by those tables.
> 
> For basic ops, flashes that get the SFDP tables right, don't even need a
> flash entry defined, we don't care about their ID, we just initialize
> the flash solely based on SFDP.
> 
> In this particular case, this flash needs identification to fix some
> wrong SFDP field, it corrects just the mode cycles for the FAST READ
> command. All the other commands seem fine according to patch 3/3.
> 
>>
>> You have the same problem with a property. Users have to add or remove
> 
> True. It's the same problem. Even if we specify the dummy cycles via a
> property, the next plugged-in flash will use those. We can of course
> fallback to the SFDP only init if the ID doesn't match any flash entry,
> but the problem is the same.
> 
>> the property if the flash changes. Anyone thinking they can use this
>> chip as a compatible 2nd source is SOL.
>>
> 
> I think the property vs compatible decision resumes at whether we
> consider that the dummy cycles requirement for Read ID is/will be
> generic or not.
> 
> I noticed that with higher frequencies or protocol modes (e.g, octal
> DTR), flashes tend to require more dummy cycles. I think with time,
> we'll have more flashes with such requirement. Takahiro can jump in and
> tell if it's already the case with IFX.
> 
Infineon SEMPER flash families (S25Hx-T/S28Hx-T) requires dummy cycles in
Read ID, depending on Configuration Register setting. That is to support
higher frequencies. By factory default dummy is 0 in 1S-1S-1S and it works
up to 133MHz. Users can change the setting to support higher frequencies.

> Thus instead of having lots of new compatibles for this, I lean towards
> having this property. I'm still open for the compatible idea, I just
> wanted to explain better where we are.
> 
> Thanks,
> ta
>