Re: spi-nor: maxronix MX25L12835F support

[Pratyush Yadav <p.yadav@ti.com>]

On 01/03/21 11:11AM, Tudor.Ambarus@microchip.com wrote:
> On 3/1/21 12:52 PM, Vignesh Raghavendra wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Hi,
> > 
> > On 2/28/21 3:22 AM, Heiko Thiery wrote:
> > 
> > [...]
> >>>>> +#define SPI_NOR_AIM_SFDP       BIT(23) /* Try to parse SFDP. Used by flashes
> >>>>> +                                        * that share the same JEDEC-ID, but
> >>>>> +                                        * where a flash defines the SFDP tables
> >>>>> +                                        * and the other doesn't.
> >>>>> +                                        */
> >>>>>
> >>>>>         /* Part specific fixup hooks. */
> >>>>>         const struct spi_nor_fixups *fixups;
> >>>>> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> >>>>> index 9203abaac229..1ebce775eae4 100644
> >>>>> --- a/drivers/mtd/spi-nor/macronix.c
> >>>>> +++ b/drivers/mtd/spi-nor/macronix.c
> >>>>> @@ -50,7 +50,8 @@ static const struct flash_info macronix_parts[] = {
> >>>>>         { "mx25u4035",   INFO(0xc22533, 0, 64 * 1024,   8, SECT_4K) },
> >>>>>         { "mx25u8035",   INFO(0xc22534, 0, 64 * 1024,  16, SECT_4K) },
> >>>>>         { "mx25u6435f",  INFO(0xc22537, 0, 64 * 1024, 128, SECT_4K) },
> >>>>> -       { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SECT_4K) },
> >>>>> +       { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256,
> >>>>> +                             SECT_4K | SPI_NOR_AIM_SFDP) },
> >>>>>         { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
> >>>>>         { "mx25r1635f",  INFO(0xc22815, 0, 64 * 1024,  32,
> >>>>>                               SECT_4K | SPI_NOR_DUAL_READ |
> >>>>
> >>>> I tried your patch and it works like expected. I can now read the
> >>>> whole flash in ~2sec while without that it was ~6sec.
> >>>>
> >>>> # time dd if=/dev/mtd0 of=dump.bin
> >>>> 32768+0 records in
> >>>> 32768+0 records out
> >>>> real 0m 2.08s
> >>>> user 0m 0.01s
> >>>> sys 0m 2.06s
> >>>>
> >>>> vs.
> >>>>
> >>>> # time dd if=/dev/mtd0 of=dump.bin
> >>>> 32768+0 records in
> >>>> 32768+0 records out
> >>>> real 0m 6.16s
> >>>> user 0m 0.05s
> >>>> sys 0m 6.09s
> >>>>
> >>>>
> >>>
> >>> Great, thanks!
> >>>
> >>>> Should I prepare a patch with that change or will you do?
> >>>
> >>> Let's wait for a few days, so others can intervene. I'd like to
> >>> clarify what's happening on mx66l51235l too.
> >>
> >> Since a few days have passed and no one has commented, I would like to
> >> bring up the subject again.
> >>
> >> I can send a patch for the changes you suggested. What do you think?
> >>
> > 
> > Why not have a single entry for mx66l51235l/mx25l12805d with superset
> > capabilities declared. And then use info->fixups->post_sfdp() to fixup
> > capabilities for mx66l51235l based on absence of SFDP tables?

I think printing the correct flash name is somewhat important. Other 
than the handful of people who are reading this thread, few would know 
that SPI NOR calls mx25l12835f as mx25l12805d or vice versa. This can 
cause a lot of confusion among people trying to debug any issues.

So my vote goes for having separate entries for both the flashes and 
then adding a fixup hook to select the correct one by checking if SFDP 
read works.

> 
> do you mean to add SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ flags in order to
> trigger the SFDP parsing and then to undo the read init that is done in
> spi_nor_info_init_params()? Too much hustle.
> 
> > 
> > SPI_NOR_AIM_SFDP seems redundant to me. SPI NOR Framework should anyway
> > be using SFDP for detecting flash capabilities and away from flash_info
> > based static data.
> 
> Yeah, that's what I suggested a bit earlier in the thread. To first try to
> detect the caps by parsing SFDP and then if SFDP not supported then to do
> the static init via the flash flags. I'll have to check the implications,
> it will impact every flash.

I don't think it is a proper fix for this situation because of the 
reasons mentioned above.

I do think it is a good change in general. It makes sense to ask the 
device what it supports before falling back to the hard-coded values. My 
biggest concern is all the things that SFDP _can't_ tell us. We need to 
look at all the flash_info flags and see which ones can be detected from 
SFDP. If there are too many flashes using flags that can't be detected 
via SFDP then all those will have to be activated via fixup hooks. This 
can easily turn into a big maintenance burden.

A quick look at spi_nor_info_init_params() doesn't reveal anything too 
problematic though. There is SPI_NOR_NO_FR but IIUC it is for legacy 
flashes so they won't have SFDP anyway.

There are also SPI_NOR_OCTAL_READ and SPI_NOR_OCTAL_DTR_READ. The former 
can be detected via BFPT dword 17 but the BFPT parser does not support 
it yet. The latter is a bit more complicated. BFPT does not advertise 
it. The presence of the Profile 1.0 table signals this command is 
supported. Not every 8D-8D-8D capable flash needs to have this table but 
then a fixup would be needed anyway to specify the opcode, dummy, etc. 
In short, it should not be too hard to add support for detecting these 
flags.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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