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

Pratyush Yadav
2020-05-15T06:55:30Z

Hi Mason,

On 15/05/20 10:26AM, masonccyang@mxic.com.tw wrote:
>
> Hi Pratyush,
>
> > > > > I can't apply your patches to enable xSPI Octal mode for
> > > > > mx25uw51245g because your patches set up Octal protocol first and
> > > > > then using Octal protocol to write Configuration Register 2(CFG
> > > > > Reg2). I think driver
> > > > > should write CFG Reg2 in SPI 1-1-1 mode (power on state) and make
> sure
> > > > > write CFG Reg 2 is success and then setup Octa protocol in the
> last.
> > > >
> > > > Register writes should work in 1S mode, because nor->reg_proto is
> only
> > > > set _after_ 8D mode is enabled (see spi_nor_octal_dtr_enable()). In
> > > > fact, both patch 15 and 16 in my series use register writes in 1S
> mode.
> > >
> > > but I didn't see driver roll back "nor->read/write_proto = 1"
> > > if xxx->octal_dtr_enable() return failed!
> >
> > I copied what spi_nor_quad_enable() did, and made failure fatal. So if
> > xxx->octal_dtr_enable() fails, the probe would fail and the flash would
> > be unusable. You can try your hand at a fallback system where you try
>
> IMHO, it's not a good for system booting from SPI-NOR,
> driver should still keep system alive in SPI 1-1-1 mode in case of
> enable Octal/Quad failed.

I agree.

> Therefore, my patches is to setup nor->read/write_proto = 8 in case
> driver enable Octal mode is success. And to enable Octal mode in
> spi_nor_late_init_params()rather than as spi_nor_quad_enable()did.

Like I mentioned before, spi_nor_late_init_params() is called _before_
we call spi_nor_spimem_adjust_hwcaps(). That call is needed to make sure
the controller also supports octal mode operations. Otherwise, you'd end
up enabling octal mode on a controller that doesn't support it with no
way of going back now.

But we can still have a fallback mechanism even in spi_nor_init() that
would switch to a "less preferred" mode (like 1-1-1 mode) if "more
preferred" ones like octal or quad fail.

That said, I think it would be a good idea to not keep tacking features
on this series. This makes it harder for reviewers because now they are
trying to shoot a moving target. Let basic 8D support stabilize and get
merged in, and then a fallback system can be submitted as a separate
patch series.

--
Regards,
Pratyush Yadav