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

[Tudor Ambarus]

On 10/5/23 14:27, Michael Walle wrote:
> Am 2023-10-05 15:19, schrieb Tudor Ambarus:
>> On 10/5/23 12:43, Michael Walle wrote:
>>> Hi,
>>>
>> hey
>>
>>>>> 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
>>>
>>> I've looked at the xspi spec. There is no RDID specified. So I'd argue,
>>> the only pseudo standard is that RDID was only ever used with 1s1s1s.
>>> But
>>> we added spi_nor_read_id() with parameters suited for the "unusual"
>>> 8d8d8d
>>> case with additional address and dummy cycles. Just for checking whether
>>> the
>>> octal-dtr switch was successful. Therefore, we've already added
>>> parameters to
>>> spi_nor_read_id() which are not standard. Then we can just add one more.
>>> It's
>>> just how macronix is doing it. Again there is no standard.
>>> If we'd only put standard (or for the 9F pseudo standard) things in the
>>> core,
>>> then spi_nor_read_id() would need to check whether the flash is in
>>> 1s1s1s
>>> mode. And no I wouldn't prefer that ;)
>>>
>>
>> If we really want to be catholic, we should switch to 8d-8d-8s mode and
>> then issue the read id and the core will handle the readid correctly.
>> But there is no such a thing, because macronix considers that it is in
>> 8d-8d-8d mode at the time of issuing 8d-8d-8s read id. That's why I say
>> it's a bug on their side. The core is meant to be generic, I don't want
>> to pollute the core with manufacturer specific fixes.
> 
> Then why does spi_nor_read_id() have the following parameters:
> 
>  * @naddr:      number of address bytes to send. Can be zero if the
> operation
>  *              does not need to send an address.
>  * @ndummy:     number of dummy bytes to send after an opcode or
> address. Can
>  *              be zero if the operation does not require dummy bytes.
>  * @proto:      the SPI protocol for register operation.
> 
> They aren't standard either. It's just the way spansion and micron
> transfers
> the ID via RDID in octal DTR mode. And now there's macronix who does it
> slightly
> different. But *neither* of them are standard. Why should one be in the
> core and
> one shouldn't.
> 
> spi_nor_read_id() should just handle proto == SNOR_PROTO_8D_8D_8S.

Yes, it should, but mx is in 8d-8d-8d at the time it issues the read id,
thus it passes the core proto == SNOR_PROTO_8D_8D_8D, isn't it? If you
care about this, please send a patch addressing it, it's better to talk
on code. I don't see yet how you will handle it, but I'm open to review
some code.