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

masonccyang
2020-05-05T09:32:50Z

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.

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