mbox series

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

Message ID 1587451187-6889-1-git-send-email-masonccyang@mxic.com.tw (mailing list archive)
Headers show
Series mtd: spi-nor: Add support for Octal 8D-8D-8D mode | expand

Message

Mason Yang April 21, 2020, 6:39 a.m. UTC
Hello,

This is repost of patchset from Boris Brezillon's
[RFC,00/18] mtd: spi-nor: Proposal for 8-8-8 mode support [1].

Background from cover letter for RFC[1].

The trend has been around Octal NOR Flash lately and the latest mainline
already supports 1-1-8 and 1-8-8 modes.

Boris opened a discussion on how we should support stateful modes (X-X-X
and XD-XD-XD, where X is the bus width and D means Double Transfer Rate).

JESD216C has defined specification for Octal 8S-8S-8S and 8D-8D-8D.
Based on JEDEC216C Basic Flash Parameter Table (BFPT) driver extract:
DWORD-18: command and command extension type.
DWORD-19: enable 8S-8S-8S/8D-8D-8D mode sequences by two instructions or
	  write CFG Reg 2.
DWORD-20: Maximum operation speed of device in Octal mode.

and xSPI profile 1.0 table:
DWORD-1: Read Fast command, the number of dummy cycles and address nbytes
	 for Read Status Register command.
DWORD-2: Read/Write volatile Register command for CFG Reg2.
DWORD-4 and DWORD-5: dummy cycles used for various frequencies.

The first set of patches is according to JESD216C adding Double Transfer
Rate(DTR) fields, extension command and command bytes number to the
spi_mem_op struct. This is from Boris patchset.

The second set of patches parse the xSPI profile 1.0 table for parameters
needed in Octal 8D-8D-8D mode. 

The third set of patches extract BFPT DWORD018,19,20 and define the 
relevant macros and enum in spi-nor layer for Octal 8S-8S-8S and 
8D-8D-8D mode operation. Parts of these are refer to Boris patchset but
we enable Octal 8D-8D-8D mode in spi_nor_late_init_params() rather than
Boris's adding a change_mode() call-back function.

The last set of patches in the series support Macronix mx25uw51245g
to tweak flash parameters a correct dummy cycles set for various frequency.

Also patched spi-mxic driver for testing on Macronix's Zynq PicoZed board
with Macronix's SPI controller (spi-mxic.c) and mx25uw51245g Octal flash.

[1] https://patchwork.ozlabs.org/cover/982926/


Summary of change log
---------------------
v2: 
Parse BFPT & xSPI table for Octal 8D-8D-8D mode parameters and enable Octal
mode in spi_nor_late_init_params().
Using Macros in spi_nor_spimem_read_data, spi_nor_spimem_write_data and
so on by Vignesh comments.

v1:
Without parsing BFPT & xSPI profile 1.0 table and enter Octal 8D-8D-8D
mode directly in spi_nor_fixups hooks.


thnaks for your time and review.
best regards,
Mason

Mason Yang (5):
  mtd: spi-nor: Add support for Octal 8D-8D-8D mode
  mtd: spi-nor: sfdp: Add support for xSPI profile 1.0 table
  mtd: spi-nor: Parse BFPT DWORD-18,19 and 20 for Octal 8D-8D-8D mode
  mtd: spi-nor: macronix: Add Octal 8D-8D-8D supports for Macronix
    mx25uw51245g
  spi: mxic: Patch for Octal 8D-8D-8D mode support

 drivers/mtd/spi-nor/core.c     | 220 ++++++++++++++++++++++++++++++++++++++--
 drivers/mtd/spi-nor/core.h     |  31 ++++++
 drivers/mtd/spi-nor/macronix.c |  41 ++++++++
 drivers/mtd/spi-nor/sfdp.c     | 222 ++++++++++++++++++++++++++++++++++++++++-
 drivers/mtd/spi-nor/sfdp.h     |  16 ++-
 drivers/spi/spi-mem.c          |   8 +-
 drivers/spi/spi-mxic.c         | 101 +++++++++++++------
 include/linux/mtd/spi-nor.h    |  51 +++++++++-
 include/linux/spi/spi-mem.h    |  13 +++
 9 files changed, 654 insertions(+), 49 deletions(-)

Comments

Boris Brezillon April 21, 2020, 7:23 a.m. UTC | #1
+Pratyush who's working on a similar patchet [1].

Hello Mason,

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.

Regards,

Boris

[1]https://lkml.org/lkml/2020/3/13/659
Vignesh Raghavendra April 21, 2020, 9:35 a.m. UTC | #2
On 21/04/20 12:53 pm, Boris Brezillon wrote:
> +Pratyush who's working on a similar patchet [1].
> 
> Hello Mason,
> 
> 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.
> 

[1] does soft reset on shutdown which should put it to reset default
state of 1S-1S-1S mode (if thats the POR default)

But, there is still one open question now that we are considering
supporting stateful modes:

What to do with flashes that power up in 8D mode either due to factory
defaults or if 8D mode NV bit is set? Do we say SPI NOR framework won't
support such flashes?
Auto discovery of such flashes is quite difficult as different flashes
use different protocols for RDID cmd in 8D mode (address phase may or
may not be present, dummy cycles vary etc) is almost impossible w/o any
hint passed to the driver?


> Regards,
> 
> Boris
> 
> [1]https://lkml.org/lkml/2020/3/13/659
>
Boris Brezillon April 21, 2020, 12:17 p.m. UTC | #3
On Tue, 21 Apr 2020 15:05:08 +0530
Vignesh Raghavendra <vigneshr@ti.com> wrote:

> On 21/04/20 12:53 pm, Boris Brezillon wrote:
> > +Pratyush who's working on a similar patchet [1].
> > 
> > Hello Mason,
> > 
> > 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.
> >   
> 
> [1] does soft reset on shutdown which should put it to reset default
> state of 1S-1S-1S mode (if thats the POR default)

Oh ok, looks like I didn't read the patch series carefully enough.

> 
> But, there is still one open question now that we are considering
> supporting stateful modes:
> 
> What to do with flashes that power up in 8D mode either due to factory
> defaults or if 8D mode NV bit is set? Do we say SPI NOR framework won't
> support such flashes?
> Auto discovery of such flashes is quite difficult as different flashes
> use different protocols for RDID cmd in 8D mode (address phase may or
> may not be present, dummy cycles vary etc) is almost impossible w/o any
> hint passed to the driver?

I don't know yet. Looks like we'll have to pass the part-id and default
mode for those flashes (part-name being a part-specific compatible, and
boot-up mode being an extra property). But maybe we can ignore that for
now and focus on flashes booting in single SPI mode first :P.
Pratyush Yadav April 27, 2020, 5:55 p.m. UTC | #4
On 21/04/20 09:23AM, Boris Brezillon wrote:
> +Pratyush who's working on a similar patchet [1].
> 
> Hello Mason,
> 
> 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/
Mason Yang April 28, 2020, 6:14 a.m. UTC | #5
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 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.


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. 


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 ?


thanks & best regards,
Mason



CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Boris Brezillon April 28, 2020, 6:34 a.m. UTC | #6
On Tue, 28 Apr 2020 14:14:31 +0800
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 quickly went through your patches but can't reply them in each your 
> patches.

Why?!! Aren't you registered to the MTD mailing list? Sorry but having
reviews outside of the original thread is far from ideal. Please find a
way to reply to the original patchset.

> 
> i.e,.
> 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
> 
> -                                u8 opcode;
> +                                u16 opcode;
> 
> big/little Endian issue, right?

There's no endianness issue since it's SPI controller responsibility to
interpret this field.
 
> why not just u8 ext_opcode;

Because I see the ext_ext_code comimg, and it's also more consistent
with the addr field if we use an u16 and a number of cmd cycles.

> No any impact for exist code and actually only xSPI device use extension 
> command.

And extending the opcode field to an u16 has no impact either.
Pratyush Yadav April 28, 2020, 8:35 a.m. UTC | #7
On 28/04/20 08:34AM, Boris Brezillon wrote:
> On Tue, 28 Apr 2020 14:14:31 +0800
> masonccyang@mxic.com.tw wrote:
> > 
> > I quickly went through your patches but can't reply them in each 
> > your patches.
> 
> Why?!! Aren't you registered to the MTD mailing list? Sorry but having
> reviews outside of the original thread is far from ideal. Please find a
> way to reply to the original patchset.

Yes, inline replies to the original patchset would be nice.

FWIW, I'm not subscribed to the list either. I use the NNTP server at 
nntp.lore.kernel.org, and the newsgroup org.infradead.lists.linux-mtd to 
read and reply to the list. Most popular email clients should have NNTP 
support. I use neomutt, but AFAIK, Thunderbird and gnus also have NNTP 
support.
Pratyush Yadav April 28, 2020, 8:54 a.m. UTC | #8
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.
Mason Yang April 29, 2020, 5:59 a.m. UTC | #9
Hi Boris,

> > > > 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 quickly went through your patches but can't reply them in each your 
> > patches.
> 
> Why?!! Aren't you registered to the MTD mailing list? Sorry but having
> reviews outside of the original thread is far from ideal. Please find a
> way to reply to the original patchset.
> 
> > 
> > i.e,.
> > 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
> > 
> > -                                u8 opcode;
> > +                                u16 opcode;
> > 
> > big/little Endian issue, right?
> 
> There's no endianness issue since it's SPI controller responsibility to
> interpret this field.
> 
> > why not just u8 ext_opcode;
> 
> Because I see the ext_ext_code comimg, and it's also more consistent
> with the addr field if we use an u16 and a number of cmd cycles.
> 
> > No any impact for exist code and actually only xSPI device use 
extension 
> > command.
> 
> And extending the opcode field to an u16 has no impact either.
> 

okay, got your point.

thanks for your time & comments.
Mason


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Mason Yang April 29, 2020, 7:31 a.m. UTC | #10
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.

yes,sure!
let's work together to upstream the Octal 8D-8D-8D driver to mainline.

The main concern is where and how to enable xSPI octal mode?

Vignesh don't agree to enable it in fixup hooks and that's why I patched
it to spi_nor_late_init_params() and confirmed the device support xSPI 
Octal mode after BFPT DWORD-19 and xSPI pf 1.0 have been parsed.

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.

As JESD216F description on BFPT DOWRD 19th, only two way to enable 
xSPI Octal mode;
one is by two instruction: issue instruction 06h(WREN) and then E8h.
the other is issue instruction 06h, then issue instruction 72h (Write
CFG Reg2), address 0h and data 02h (8D-8D-8D).

Let our patches comply with this. you may refer to my patches
[v2,3/5] mtd: spi-nor: Parse BFPT DWORD-18, 19 and 20 for Octal 8D-8D-8D 
mode

                 /* Octal mode enable sequences. */
                 switch (bfpt.dwords[BFPT_DWORD(19)] & 
BFPT_DWORD19_OCTAL_SEQ_MASK) {
                 case BFPT_DWORD19_TWO_INST:
+       ----> to patch here.
                                 break;
                 case BFPT_DWORD19_CFG_REG2:
                                 params->xspi_enable = 
spi_nor_cfg_reg2_octal_enable;
                                 break;
                 default:
                                 break;
                 }


> 
> > 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.

yup, I got his point and please make sure CPU data access.

i.e,.
Fix endianness of the BFPT DWORDs and xSPI in sfdp.c

and your patch,
+                                ext = spi_nor_get_cmd_ext(nor, op);
+                                op->cmd.opcode = (op->cmd.opcode << 8) | 
ext;
+                                op->cmd.nbytes = 2;

I think maybe using u8 opcode[2] could avoid endianness.

Moreover, Vignesh think it's fine to use u8 ext_opcode in my v1 patches.
please check his comments on
https://patchwork.ozlabs.org/project/linux-mtd/patch/1573808288-19365-3-git-send-email-masonccyang@mxic.com.tw/ 



Let's open this discussion and maybe Vighesh and Tudor could have some 
comments on it.
thanks a lot.

> 
> > 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".

I mean this function just do xSPI profile 1.0 table parse, no read/pp 
setting
i.e,.

+                /*
+                 * Update the fast read settings. We set the default 
dummy cycles to 20
+                 * here. Flashes can change this value if they need to 
when enabling
+                 * octal mode.
+                 */
+                params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
+ spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
+                                                                  0, 20, 
opcode,
+ SNOR_PROTO_8_8_8_DTR);
+
+                /*
+                 * Since the flash supports xSPI DTR reads, it should 
also support DTR
+                 * Page Program opcodes.
+                 */
+                params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR;


> 
> > 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.
> 

as my concern above.

> -- 
> Regards,
> Pratyush Yadav

thanks & best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Boris Brezillon April 29, 2020, 8:37 a.m. UTC | #11
On Wed, 29 Apr 2020 15:31:35 +0800
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.  
> 
> yes,sure!
> let's work together to upstream the Octal 8D-8D-8D driver to mainline.
> 
> The main concern is where and how to enable xSPI octal mode?
> 
> Vignesh don't agree to enable it in fixup hooks and that's why I patched
> it to spi_nor_late_init_params() and confirmed the device support xSPI 
> Octal mode after BFPT DWORD-19 and xSPI pf 1.0 have been parsed.
> 
> 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.
> 
> As JESD216F description on BFPT DOWRD 19th, only two way to enable 
> xSPI Octal mode;
> one is by two instruction: issue instruction 06h(WREN) and then E8h.
> the other is issue instruction 06h, then issue instruction 72h (Write
> CFG Reg2), address 0h and data 02h (8D-8D-8D).
> 
> Let our patches comply with this. you may refer to my patches
> [v2,3/5] mtd: spi-nor: Parse BFPT DWORD-18, 19 and 20 for Octal 8D-8D-8D 
> mode
> 
>                  /* Octal mode enable sequences. */
>                  switch (bfpt.dwords[BFPT_DWORD(19)] & 
> BFPT_DWORD19_OCTAL_SEQ_MASK) {
>                  case BFPT_DWORD19_TWO_INST:
> +       ----> to patch here.
>                                  break;
>                  case BFPT_DWORD19_CFG_REG2:
>                                  params->xspi_enable = 
> spi_nor_cfg_reg2_octal_enable;
>                                  break;
>                  default:
>                                  break;
>                  }
> 
> 
> >   
> > > 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.  
> 
> yup, I got his point and please make sure CPU data access.
> 
> i.e,.
> Fix endianness of the BFPT DWORDs and xSPI in sfdp.c
> 
> and your patch,
> +                                ext = spi_nor_get_cmd_ext(nor, op);
> +                                op->cmd.opcode = (op->cmd.opcode << 8) | 
> ext;
> +                                op->cmd.nbytes = 2;
> 
> I think maybe using u8 opcode[2] could avoid endianness.
> 

Again, if there's an endianness issue it's in your SPI driver, not
here, and I suspect you'd have the same issue with the address cycles.
SPI mem protocols has been using big endian for everything, and I think
that should be applied to dual-byte opcodes too.

> Moreover, Vignesh think it's fine to use u8 ext_opcode in my v1 patches.
> please check his comments on
> https://patchwork.ozlabs.org/project/linux-mtd/patch/1573808288-19365-3-git-send-email-masonccyang@mxic.com.tw/ 
> 
> 
> 
> Let's open this discussion and maybe Vighesh and Tudor could have some 
> comments on it.

Changing for opcode[2] would mean patching all spi-mem drivers. That's
doable, but given we already have the address field encoded in a u64, I
don't see a good reason to not apply that rule to the opcode.
Pratyush Yadav April 29, 2020, 6:18 p.m. UTC | #12
Hi Mason,

On 29/04/20 03:31PM, masonccyang@mxic.com.tw wrote:
> Hi Pratyush,
> > > > 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.
> 
> yes,sure!
> let's work together to upstream the Octal 8D-8D-8D driver to mainline.
> 
> The main concern is where and how to enable xSPI octal mode?
> 
> Vignesh don't agree to enable it in fixup hooks and that's why I patched
> it to spi_nor_late_init_params() and confirmed the device support xSPI 
> Octal mode after BFPT DWORD-19 and xSPI pf 1.0 have been parsed.

My series does it in a octal_dtr_enable() hook, which is called from 
spi_nor_init(). Just like how it is done for quad_enable(). So, the 
expectation is that you populate the octal DTR hook for your flash in a 
flash-specific hook (like the default_init() fixup hook). Example of 
this can be seen in patches 15 and 16 of my series in 
spi_nor_cypress_octal_enable() and spi_nor_micron_octal_dtr_enable().

An alternative for this would be a generic way to enable these flashes, 
like from BFPT DWORD 19. That doesn't work for either of the flashes I 
had, so I didn't implement it because I wouldn't be able to test it. If 
it works for you, please implement it. I don't mind doing it myself, but 
then you would need to help me test it.
 
> 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.
 
> As JESD216F description on BFPT DOWRD 19th, only two way to enable 
> xSPI Octal mode;
> one is by two instruction: issue instruction 06h(WREN) and then E8h.
> the other is issue instruction 06h, then issue instruction 72h (Write
> CFG Reg2), address 0h and data 02h (8D-8D-8D).
> 
> Let our patches comply with this. you may refer to my patches
> [v2,3/5] mtd: spi-nor: Parse BFPT DWORD-18, 19 and 20 for Octal 8D-8D-8D 
> mode

The Cypress Semper S28 flash family says that it does not have an Octal 
Enable bit (i.e, the Octal Enable Requirements field is 0), even though 
it does have an Octal Enable bit. That bit resides in CFG Reg 5. And the 
Micron MT35ABA family, doesn't have DWORD19 in their BFPT at all. So, 
the two flashes I need to support don't have this. At the very least, we 
need to provide a flash-specific way to enable Octal DTR mode, along 
with an xSPI compliant way.

So I suggest that we have two separate type of 8D enable functions. One 
type which is generic and works on any xSPI complint device, like the 
spi_nor_cfg_reg2_octal_enable() in your patch. The other would be 
flash-specific ones, which flashes can set in their fixup hooks.
 
>                  /* Octal mode enable sequences. */
>                  switch (bfpt.dwords[BFPT_DWORD(19)] & 
> BFPT_DWORD19_OCTAL_SEQ_MASK) {
>                  case BFPT_DWORD19_TWO_INST:
> +       ----> to patch here.
>                                  break;
>                  case BFPT_DWORD19_CFG_REG2:
>                                  params->xspi_enable = 
> spi_nor_cfg_reg2_octal_enable;
>                                  break;
>                  default:
>                                  break;
>                  }
> 
> 
> > 
> > > 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.
> 
> yup, I got his point and please make sure CPU data access.
> 
> i.e,.
> Fix endianness of the BFPT DWORDs and xSPI in sfdp.c
> 
> and your patch,
> +                                ext = spi_nor_get_cmd_ext(nor, op);
> +                                op->cmd.opcode = (op->cmd.opcode << 8) | 
> ext;
> +                                op->cmd.nbytes = 2;
> 
> I think maybe using u8 opcode[2] could avoid endianness.

Again, thanks Boris for answering this. FWIW, I don't see anything wrong 
with his suggestion.

To clarify a bit more, the idea is that we transmit the opcode MSB 
first, just we do for the address. Assume we want to issue the command 
0x05. In 1S mode, we set cmd.opcode to 0x05. Here cmd.nbytes == 1. Treat 
is as a 1-byte value, so the MSB is the same as the LSB. We directly 
send 0x5 on the bus.

If cmd.nbytes == 2, then the opcode would be 0x05FA (assuming extension 
is invert of command). So we send the MSB (0x05) first, and LSB (0xFA) 
next.

In all this, I don't see where endianness comes into the picture. While 
the _location_ of the MSB in memory may change because of the 
endianness, the MSB of the _number_ will always be 0x05. So, regardless 
of the endianness, the operation (opcode >> 8) should always give 0x05 
and (opcode & F) should always give 0xFA. Endianness is just how you 
represent these values in memory.
 
> Moreover, Vignesh think it's fine to use u8 ext_opcode in my v1 patches.
> please check his comments on
> https://patchwork.ozlabs.org/project/linux-mtd/patch/1573808288-19365-3-git-send-email-masonccyang@mxic.com.tw/ 

In fact, that's how I did it in the first version of my series as well, 
but refactored it on Boris's suggestion.
 
> Let's open this discussion and maybe Vighesh and Tudor could have some 
> comments on it.
> thanks a lot.
> 
> > 
> > > 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".
> 
> I mean this function just do xSPI profile 1.0 table parse, no read/pp 
> setting
> i.e,.
> 
> +                /*
> +                 * Update the fast read settings. We set the default 
> dummy cycles to 20
> +                 * here. Flashes can change this value if they need to 
> when enabling
> +                 * octal mode.
> +                 */
> +                params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
> + spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> +                                                                  0, 20, 
> opcode,
> + SNOR_PROTO_8_8_8_DTR);
> +
> +                /*
> +                 * Since the flash supports xSPI DTR reads, it should 
> also support DTR
> +                 * Page Program opcodes.
> +                 */
> +                params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR;

Ok. Fair enough.
 
> 
> > 
> > > 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.
> > 
> 
> as my concern above.

I hope my answer above clears this up.
Vignesh Raghavendra April 30, 2020, 8:21 a.m. UTC | #13
Hi Mason,

On 29/04/20 1:01 pm, masonccyang@mxic.com.tw wrote:
> 
> Hi Pratyush,
> 
>  
>>>>> On Tue, 21 Apr 2020 14:39:42 +0800
>>>>> Mason Yang <masonccyang@mxic.com.tw> wrote:
[...]
>>>
> 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.
> 
> yes,sure!
> let's work together to upstream the Octal 8D-8D-8D driver to mainline.
> 
> The main concern is where and how to enable xSPI octal mode?
> 
> Vignesh don't agree to enable it in fixup hooks and that's why I patched
> it to spi_nor_late_init_params() and confirmed the device support xSPI 
> Octal mode after BFPT DWORD-19 and xSPI pf 1.0 have been parsed.
> 

My suggestion was to use SFDP wherever possible.. E.g: it is possible to
get opcode extension type from BFPT...

But using BFPT DWORD-19 is not correct for switching to 8D-8D-8D mode:

Per JESD216D.01 Bits 22:20 of  19th DWORD of BFPT:

Octal Enable Requirements:

This field describes whether the device contains a Octal Enable bit used
to enable 1-1-8 and 1-
8-8 octal read or octal program operations.

So, this cannot be used for enabling 8D-8D-8D mode... Flashes that only
support 1S-1S-1S and 8D-8D-8D will set this field to 0.

There is a separate table to enable 8D mode called
"Command Sequences to Change to Octal DDR (8D-8D-8D) mode". But if flash
does not have the table or has bad data, fixup hook is the only way...

If mx25* supports above table, please build on top of Pratyush's series
to add support for parsing this table. Otherwise, macronix would have to
use a fixup hook too...

> 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.
> 
> As JESD216F description on BFPT DOWRD 19th, only two way to enable 
> xSPI Octal mode;

Where is JESD216F? Latest I can find is JESD216D.01

> one is by two instruction: issue instruction 06h(WREN) and then E8h.
> the other is issue instruction 06h, then issue instruction 72h (Write
> CFG Reg2), address 0h and data 02h (8D-8D-8D).
> 
> Let our patches comply with this. you may refer to my patches
> [v2,3/5] mtd: spi-nor: Parse BFPT DWORD-18, 19 and 20 for Octal 8D-8D-8D 
> mode

As I pointed out earlier using above DWORDS seems wrong for 8D-8D-8D,
they can be used for 1-1-8 and 1-
8-8

> 
>                  /* Octal mode enable sequences. */
>                  switch (bfpt.dwords[BFPT_DWORD(19)] & 
> BFPT_DWORD19_OCTAL_SEQ_MASK) {
>                  case BFPT_DWORD19_TWO_INST:
> +       ----> to patch here.
>                                  break;
>                  case BFPT_DWORD19_CFG_REG2:
>                                  params->xspi_enable = 
> spi_nor_cfg_reg2_octal_enable;
>                                  break;
>                  default:
>                                  break;
>                  }
> 
> 
>>
>>> 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? 

Is the big/little Endian issue a quirk of the flash or controller? If
its controller specific then it needs to handled in controller driver.

If this is a flash quirk, please point to the waveforms in the flash
datasheet...

>>> 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.
> 
> yup, I got his point and please make sure CPU data access.
> 
> i.e,.
> Fix endianness of the BFPT DWORDs and xSPI in sfdp.c
> 
> and your patch,
> +                                ext = spi_nor_get_cmd_ext(nor, op);
> +                                op->cmd.opcode = (op->cmd.opcode << 8) | 
> ext;
> +                                op->cmd.nbytes = 2;
> 
> I think maybe using u8 opcode[2] could avoid endianness.
> 
> Moreover, Vignesh think it's fine to use u8 ext_opcode in my v1 patches.
> please check his comments on
> https://patchwork.ozlabs.org/project/linux-mtd/patch/1573808288-19365-3-git-send-email-masonccyang@mxic.com.tw/ 
> 
> 
> 
> Let's open this discussion and maybe Vighesh and Tudor could have some 
> comments on it.
> thanks a lot.
> 

Sorry , but others clearly see having single variable to store cmd +
extension is beneficial here. So, I take back my suggestion.

Regards
Vignesh
Mason Yang May 5, 2020, 9:31 a.m. UTC | #14
Hi Pratyush,

> > > > > 
> > > > > 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.
> > 
> > yes,sure!
> > let's work together to upstream the Octal 8D-8D-8D driver to mainline.
> > 
> > The main concern is where and how to enable xSPI octal mode?
> > 
> > Vignesh don't agree to enable it in fixup hooks and that's why I 
patched
> > it to spi_nor_late_init_params() and confirmed the device support xSPI 

> > Octal mode after BFPT DWORD-19 and xSPI pf 1.0 have been parsed.
> 
> My series does it in a octal_dtr_enable() hook, which is called from 
> spi_nor_init(). Just like how it is done for quad_enable(). So, the 
> expectation is that you populate the octal DTR hook for your flash in a 
> flash-specific hook (like the default_init() fixup hook). Example of 
> this can be seen in patches 15 and 16 of my series in 
> spi_nor_cypress_octal_enable() and spi_nor_micron_octal_dtr_enable().
> 
> An alternative for this would be a generic way to enable these flashes, 
> like from BFPT DWORD 19. That doesn't work for either of the flashes I 
> had, so I didn't implement it because I wouldn't be able to test it. If 
> it works for you, please implement it. I don't mind doing it myself, but 

> then you would need to help me test it.
> 
> > 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!

> 
> > As JESD216F description on BFPT DOWRD 19th, only two way to enable 
> > xSPI Octal mode;
> > one is by two instruction: issue instruction 06h(WREN) and then E8h.
> > the other is issue instruction 06h, then issue instruction 72h (Write
> > CFG Reg2), address 0h and data 02h (8D-8D-8D).
> > 
> > Let our patches comply with this. you may refer to my patches
> > [v2,3/5] mtd: spi-nor: Parse BFPT DWORD-18, 19 and 20 for Octal 
8D-8D-8D 
> > mode
> 
> The Cypress Semper S28 flash family says that it does not have an Octal 
> Enable bit (i.e, the Octal Enable Requirements field is 0), even though 
> it does have an Octal Enable bit. That bit resides in CFG Reg 5. And the 

> Micron MT35ABA family, doesn't have DWORD19 in their BFPT at all. So, 
> the two flashes I need to support don't have this. At the very least, we 

> need to provide a flash-specific way to enable Octal DTR mode, along 
> with an xSPI compliant way.
> 
> So I suggest that we have two separate type of 8D enable functions. One 
> type which is generic and works on any xSPI complint device, like the 
> spi_nor_cfg_reg2_octal_enable() in your patch. The other would be 
> flash-specific ones, which flashes can set in their fixup hooks.

okay, sure.

> 
> >                  /* Octal mode enable sequences. */
> >                  switch (bfpt.dwords[BFPT_DWORD(19)] & 
> > BFPT_DWORD19_OCTAL_SEQ_MASK) {
> >                  case BFPT_DWORD19_TWO_INST:
> > +       ----> to patch here.
> >                                  break;
> >                  case BFPT_DWORD19_CFG_REG2:
> >                                  params->xspi_enable = 
> > spi_nor_cfg_reg2_octal_enable;
> >                                  break;
> >                  default:
> >                                  break;
> >                  }
> > 
> > 
> > > 
> > > > 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.
> > 
> > yup, I got his point and please make sure CPU data access.
> > 
> > i.e,.
> > Fix endianness of the BFPT DWORDs and xSPI in sfdp.c
> > 
> > and your patch,
> > +                                ext = spi_nor_get_cmd_ext(nor, op);
> > +                                op->cmd.opcode = (op->cmd.opcode << 
8) | 
> > ext;
> > +                                op->cmd.nbytes = 2;
> > 
> > I think maybe using u8 opcode[2] could avoid endianness.
> 
> Again, thanks Boris for answering this. FWIW, I don't see anything wrong 

> with his suggestion.
> 
> To clarify a bit more, the idea is that we transmit the opcode MSB 
> first, just we do for the address. Assume we want to issue the command 
> 0x05. In 1S mode, we set cmd.opcode to 0x05. Here cmd.nbytes == 1. Treat 

> is as a 1-byte value, so the MSB is the same as the LSB. We directly 
> send 0x5 on the bus.

There are many SPI controllers driver use "op->cmd.opcode" directly,
so is spi-mxic.c.

i.e,.
ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, op->cmd.nbytes);

> 
> If cmd.nbytes == 2, then the opcode would be 0x05FA (assuming extension 
> is invert of command). So we send the MSB (0x05) first, and LSB (0xFA) 
> next.

My platform is Xilinx Zynq platform which CPU is ARMv7 processor.

In 1-1-1 mode, it's OK to send 1 byte command by u16 opcode but
in 8D-8D-8D mode, I need to patch

i.e.,
op->cmd.opcode = op->cmd.opcode | (ext << 8);

rather than your patch.


> 
> In all this, I don't see where endianness comes into the picture. While 
> the _location_ of the MSB in memory may change because of the 
> endianness, the MSB of the _number_ will always be 0x05. So, regardless 
> of the endianness, the operation (opcode >> 8) should always give 0x05 
> and (opcode & F) should always give 0xFA. Endianness is just how you 
> represent these values in memory.
> 

thanks & best regards,
Mason


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Boris Brezillon May 5, 2020, 9:44 a.m. UTC | #15
On Tue, 5 May 2020 17:31:45 +0800
masonccyang@mxic.com.tw wrote:


> > > > > 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.  
> > > 
> > > yup, I got his point and please make sure CPU data access.
> > > 
> > > i.e,.
> > > Fix endianness of the BFPT DWORDs and xSPI in sfdp.c
> > > 
> > > and your patch,
> > > +                                ext = spi_nor_get_cmd_ext(nor, op);
> > > +                                op->cmd.opcode = (op->cmd.opcode <<   
> 8) | 
> > > ext;
> > > +                                op->cmd.nbytes = 2;
> > > 
> > > I think maybe using u8 opcode[2] could avoid endianness.  
> > 
> > Again, thanks Boris for answering this. FWIW, I don't see anything wrong   
> 
> > with his suggestion.
> > 
> > To clarify a bit more, the idea is that we transmit the opcode MSB 
> > first, just we do for the address. Assume we want to issue the command 
> > 0x05. In 1S mode, we set cmd.opcode to 0x05. Here cmd.nbytes == 1. Treat   
> 
> > is as a 1-byte value, so the MSB is the same as the LSB. We directly 
> > send 0x5 on the bus.  
> 
> There are many SPI controllers driver use "op->cmd.opcode" directly,
> so is spi-mxic.c.
> 
> i.e,.
> ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, op->cmd.nbytes);

Just because you do it doesn't mean it's right. And most controllers use
the opcode value, they don't dereference the pointer as you do here.

> 
> > 
> > If cmd.nbytes == 2, then the opcode would be 0x05FA (assuming extension 
> > is invert of command). So we send the MSB (0x05) first, and LSB (0xFA) 
> > next.  
> 
> My platform is Xilinx Zynq platform which CPU is ARMv7 processor.
> 
> In 1-1-1 mode, it's OK to send 1 byte command by u16 opcode but
> in 8D-8D-8D mode, I need to patch
> 
> i.e.,
> op->cmd.opcode = op->cmd.opcode | (ext << 8);
> 
> rather than your patch.

Seriously, how hard is it to extract each byte from the u16 if your
controller needs to pass things in a different order? I mean, that's
already how it's done for the address cycle, so why is it a problem
here? This sounds like bikeshedding to me. If the order is properly
documented in the kernel doc, I see no problem having it grouped in one
u16, with the first cmd cycle placed in the MSB and the second one in
the LSB.
Boris Brezillon May 5, 2020, 10:01 a.m. UTC | #16
On Tue, 5 May 2020 11:44:43 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Tue, 5 May 2020 17:31:45 +0800
> masonccyang@mxic.com.tw wrote:
> 
> 
> > > > > > 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.    
> > > > 
> > > > yup, I got his point and please make sure CPU data access.
> > > > 
> > > > i.e,.
> > > > Fix endianness of the BFPT DWORDs and xSPI in sfdp.c
> > > > 
> > > > and your patch,
> > > > +                                ext = spi_nor_get_cmd_ext(nor, op);
> > > > +                                op->cmd.opcode = (op->cmd.opcode <<     
> > 8) |   
> > > > ext;
> > > > +                                op->cmd.nbytes = 2;
> > > > 
> > > > I think maybe using u8 opcode[2] could avoid endianness.    
> > > 
> > > Again, thanks Boris for answering this. FWIW, I don't see anything wrong     
> >   
> > > with his suggestion.
> > > 
> > > To clarify a bit more, the idea is that we transmit the opcode MSB 
> > > first, just we do for the address. Assume we want to issue the command 
> > > 0x05. In 1S mode, we set cmd.opcode to 0x05. Here cmd.nbytes == 1. Treat     
> >   
> > > is as a 1-byte value, so the MSB is the same as the LSB. We directly 
> > > send 0x5 on the bus.    
> > 
> > There are many SPI controllers driver use "op->cmd.opcode" directly,
> > so is spi-mxic.c.
> > 
> > i.e,.
> > ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, op->cmd.nbytes);  
> 
> Just because you do it doesn't mean it's right. And most controllers use
> the opcode value, they don't dereference the pointer as you do here.
> 
> >   
> > > 
> > > If cmd.nbytes == 2, then the opcode would be 0x05FA (assuming extension 
> > > is invert of command). So we send the MSB (0x05) first, and LSB (0xFA) 
> > > next.    
> > 
> > My platform is Xilinx Zynq platform which CPU is ARMv7 processor.
> > 
> > In 1-1-1 mode, it's OK to send 1 byte command by u16 opcode but
> > in 8D-8D-8D mode, I need to patch
> > 
> > i.e.,
> > op->cmd.opcode = op->cmd.opcode | (ext << 8);
> > 
> > rather than your patch.  
> 
> Seriously, how hard is it to extract each byte from the u16 if your
> controller needs to pass things in a different order? I mean, that's
> already how it's done for the address cycle, so why is it a problem
> here? This sounds like bikeshedding to me. If the order is properly
> documented in the kernel doc, I see no problem having it grouped in one
> u16, with the first cmd cycle placed in the MSB and the second one in
> the LSB.

So, I gave it a try, and we're talking about something as simple as the
below diff. And yes, the mxic controller needs to be patched before
extending the cmd.opcode field, but we're talking about one driver here
(all other drivers should be fine). Oh, and if you look a few lines below
the changed lines, you'll notice that we do exactly the same for the
address.

--->8---
diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index 69491f3a515d..c3f4136a7c1d 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -356,6 +356,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
        int nio = 1, i, ret;
        u32 ss_ctrl;
        u8 addr[8];
+       u8 cmd[2];
 
        ret = mxic_spi_set_freq(mxic, mem->spi->max_speed_hz);
        if (ret)
@@ -393,7 +394,10 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
        writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
               mxic->regs + HC_CFG);
 
-       ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, 1);
+       for (i = 0; i < op->cmd.nbytes; i++)
+               cmd[i] = op->cmd.opcode >> (8 * (op->cmd.nbytes - i - 1));
+
+       ret = mxic_spi_data_xfer(mxic, cmd, NULL, op->cmd.nbytes);
        if (ret)
                goto out;
Pratyush Yadav May 6, 2020, 9:40 a.m. UTC | #17
On 05/05/20 05:31PM, 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 
all possible protocols available, but I think that should be a different 
patchset.
Mason Yang May 11, 2020, 3:23 a.m. UTC | #18
Hi Vignesh,

> >>>
> >>> 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.
> > 
> > yes,sure!
> > let's work together to upstream the Octal 8D-8D-8D driver to mainline.
> > 
> > The main concern is where and how to enable xSPI octal mode?
> > 
> > Vignesh don't agree to enable it in fixup hooks and that's why I 
patched
> > it to spi_nor_late_init_params() and confirmed the device support xSPI 

> > Octal mode after BFPT DWORD-19 and xSPI pf 1.0 have been parsed.
> > 
> 
> My suggestion was to use SFDP wherever possible.. E.g: it is possible to
> get opcode extension type from BFPT...
> 
> But using BFPT DWORD-19 is not correct for switching to 8D-8D-8D mode:
> 
> Per JESD216D.01 Bits 22:20 of  19th DWORD of BFPT:
> 
> Octal Enable Requirements:
> 
> This field describes whether the device contains a Octal Enable bit used
> to enable 1-1-8 and 1-
> 8-8 octal read or octal program operations.
> 
> So, this cannot be used for enabling 8D-8D-8D mode... Flashes that only
> support 1S-1S-1S and 8D-8D-8D will set this field to 0.

yes, you are right, the bits 22~20 your mentioned are for 1-1-8 and 1-8-8 
mode enable requirements and they are zero if Flash only supports 
1S-1S-1S,
8S-8S-8S and 8D-8D-8D, just like mx25xx series.

There are bits 8~4 for 8S-8S-8S and 8D-8D-8D mode enable sequences and
I have patched these in this patches. 

By bits 8~4 in 19 th DWORD of BFPT, driver will know enable 8S-8S-8S or
8D-8D-8D by either issue two instruction (06h and E8h) or 
by Write CFG Reg 2.

mx25xx series supports enable Octal 8S-8S-8S/8D-8D-8D mode by Write CFG 
Reg 2.


> 
> There is a separate table to enable 8D mode called
> "Command Sequences to Change to Octal DDR (8D-8D-8D) mode". But if flash
> does not have the table or has bad data, fixup hook is the only way...
> 
> If mx25* supports above table, please build on top of Pratyush's series
> to add support for parsing this table. Otherwise, macronix would have to
> use a fixup hook too...

mx25xx series also supports "Command Sequences to Change to Octal DDR 
(8D-8D-8D) mode" for sure. I will patch them in next version.

For mx25* series, a fixup hook will only setup specific dummy cycles to 
device for various frequency after xSPI 1.0 table has been parsed.


thanks for your time & comments.
Mason


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Mason Yang May 15, 2020, 2:26 a.m. UTC | #19
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.

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.

> all possible protocols available, but I think that should be a different 

> patchset.
> 
> -- 
> Regards,
> Pratyush Yadav

thanks & best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Pratyush Yadav May 15, 2020, 6:55 a.m. UTC | #20
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.