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

[Michael Walle <michael@walle.cc>]

Hi,

>> >>>>>> 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?
>> 
>> The flash device is in octal dtr mode, which means it will expect the
>> opcode in octal dtr mode. But the data phase mode depends on the 
>> opcode
>> (and theoretically, the address phase, too). If you use the
>> (non-standard) RDID on this flash command has to be executed as 
>> 8d8d8s.
>> 
>> > 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.
>> 
>> I really don't have time right now. But something along:
>> 
>> spi_nor_read_id(.., proto) {
>>    bool emulated_8d8d8s;
>> 
>>    op = assemble_op(.., proto);
>> 
>>    /* Emulate 8d8d8s if the controller doesn't support it */
>>    if (!spi_mem_supports_op(op) && proto == 8d8d8s) {
>>       op = assemble_op(.., 8d8d8d);
>>       emulated_8d8d8s = true;
>>    }
>>    execute_op()
>>    if (emulated_8d8d8s) {
>>       /* discard every other byte of the response */
>>    }
>> }
> After checking with Macronix designer, a-a-b-b-c-c is the data 
> arrangement for
> read id operation of flash in 8D-8D-8D.

Could you please point to any specification? I doubt there is one
and every vendor will do it slightly differently. I mean we already
have some flashes which (apparently) reply to RDID in 8d8d8d.

For example, see the Semper flash datasheet:
https://www.infineon.com/dgdl/Infineon-S28HS256T_S28HS512T_S28HS01GT_S28HL256T_S28HL512T_S28HL01GT_256-Mb_(32-MB)_512-Mb_(64-MB)_1-Gb_(128-MB)_HS-T_(1.8-V)_HL-T_(3.0-V)_Semper_Flash_with_Octal_Interface-DataSheet-v03_00-EN.pdf?fileId=8ac78c8c7d0d8da4017d0ee6bca96f97&da=t

Have a look at Table 78 (or search for RDIDN_4_0) and Figure 28.

-michael

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