| Message ID | 1587451187-6889-1-git-send-email-masonccyang@mxic.com.tw |
|---|---|
| Headers | show |
| Series | mtd: spi-nor: Add support for Octal 8D-8D-8D mode | expand |
+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
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 >
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.
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/
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. =====================================================================
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.
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.
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.
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. =====================================================================
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(¶ms->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. =====================================================================
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.
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(¶ms->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.
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
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. =====================================================================
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.
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;
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.
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. =====================================================================
Hi Boris, > > > > > > > > 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. yup, thanks again for your time & comments. > > --->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; > 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. =====================================================================
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. =====================================================================
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.
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(-)