Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

Pratyush Yadav
2020-04-28T08:54:23Z

Hi Mason,

On 28/04/20 02:14PM, masonccyang@mxic.com.tw wrote:
>
> Hi Pratyush,
>
> > > On Tue, 21 Apr 2020 14:39:42 +0800
> > > Mason Yang <masonccyang@mxic.com.tw> wrote:
> > >
> > > > Hello,
> > > >
> > > > This is repost of patchset from Boris Brezillon's
> > > > [RFC,00/18] mtd: spi-nor: Proposal for 8-8-8 mode support [1].
> > >
> > > I only quickly went through the patches you sent and saying it's a
> > > repost of the RFC is a bit of a lie. You completely ignored the state
> > > tracking I was trying to do to avoid leaving the flash in 8D mode when
> > > suspending/resetting the board, and I think that part is crucial. If I
> > > remember correctly, we already had this discussion so I must say I'm a
> > > bit disappointed.
> > >
> > > Can you sync with Pratyush? I think his series [1] is better in that
> it
> > > tries to restore the flash in single-SPI mode before suspend (it's
> > > missing the shutdown case, but that can be easily added I think). Of
> > > course that'd be even better to have proper state tracking at the SPI
> > > NOR level.
> >
> > Hi Mason,
> >
> > I posted a re-roll of my series here [0]. Could you please base your
> > changes on top of it? Let me know if the series is missing something you
>
> > need.
> >
> > [0]
> https://lore.kernel.org/linux-mtd/20200424184410.8578-1-p.yadav@ti.com/
>
>
> Our mx25uw51245g supports BFPT DWORD-18,19 and 20 data and xSPI profile
> 1.0,
> and it comply with BFPT DWORD-19, octal mode enable sequences by write CFG
> Reg2
> with instruction 0x72. Therefore, I can't apply your patches.

I didn't mean apply my patches directly. I meant more along the lines of
edit your patches to work on top of my series. It should be as easy as
adding your flash's fixup hooks and its octal DTR enable hook, but if my
series is missing something you need (like complete Profile 1.0 parsing,
which I left out because I wanted to be conservative and didn't see any
immediate use-case for us), let me know, and we can work together to
address it.

> I quickly went through your patches but can't reply them in each your
> patches.
>
> i.e,.
> 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
>
> - u8 opcode;
> + u16 opcode;
>
> big/little Endian issue, right?
> why not just u8 ext_opcode;
> No any impact for exist code and actually only xSPI device use extension
> command.

Boris already explained the reasoning behind it.

> 2) [v4,08/16] mtd: spi-nor: parse xSPI Profile 1.0 table
>
> need extract more data from xSPI profile 1.0 table and no other specific
> setting.

Not sure what you mean by "no other specific setting".

> 3) [v4,11/16] mtd: spi-nor: enable octal DTR mode when possible
>
> +static int spi_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
> +{
> + int ret;
> +
> + if (!nor->params->octal_dtr_enable)
> + return 0;
> +
> + if (!(spi_nor_get_protocol_width(nor->read_proto) == 8 ||
> + spi_nor_get_protocol_width(nor->write_proto) == 8))
> + return 0;
> +
> + ret = nor->params->octal_dtr_enable(nor, enable);
> + if (ret)
> + return ret;
> +
> + if (enable)
> + nor->reg_proto = SNOR_PROTO_8_8_8_DTR;
> + else
> + nor->reg_proto = SNOR_PROTO_1_1_1;
> +
> + return 0;
> +}
> +
>
> it seems you enable device in Octal mode after SPI-NOR Framework is
> already
> in Octal protocol.
> Driver should set device by SPI 1-1-1 mode to enter Octal mode and then
> setup
> Read/PP command and protocol by spi_nor_set_read/pp_setting() for Octal
> mode,
> right ?

No. We need to setup Read and PP settings first. The overall flow is
that we first run spi_nor_setup(). This will perform a "negotiation"
between the controller and the flash to find out a common protocol they
both support, and then change to that protocol in spi_nor_init(). Even
if the flash supports octal DTR protocol, we can't use if if the
controller doesn't. That is why we want to first select the protocol in
the framework, and only then change the flash to that protocol.

In case the controller doesn't support 8D-8D-8D protocol, we would want
to use 1S-1S-1S protocol so the flash is at least usable. Changing to 8D
mode before finding this out would make the flash unusable.

--
Regards,
Pratyush Yadav