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

[liao jaime]

Hi,

>
> 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.
Flash transfer data while DQS raising and falling edge exactlly.
It is not a hardware bug or 8D-8D-8S.
So that I think we only need a method for specific comparison after
read id in 8d-8d-8d.

>
> Later (or even now), that could be moved down the callstack into
> the spimem core.
>
> -michael

Thanks
Jaime