Message-ID: <20200929162943.qbnzmzgxb75wdpyo@ti.com>
Date:   Tue, 29 Sep 2020 21:59:45 +0530
From:   Pratyush Yadav <p.yadav@...com>
To:     <Tudor.Ambarus@...rochip.com>
CC:     <me@...avpratyush.com>, <miquel.raynal@...tlin.com>,
        <richard@....at>, <vigneshr@...com>, <broonie@...nel.org>,
        <Nicolas.Ferre@...rochip.com>, <alexandre.belloni@...tlin.com>,
        <Ludovic.Desroches@...rochip.com>, <matthias.bgg@...il.com>,
        <michal.simek@...inx.com>, <linux-mtd@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-spi@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-mediatek@...ts.infradead.org>,
        <boris.brezillon@...labora.com>, <nsekhar@...com>
Subject: Re: [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol

On 29/09/20 03:42PM, Tudor.Ambarus@...rochip.com wrote:
> Hi, Pratyush,
> 
> I'm replying to v10 so that we continue the discussion, but this applies to v13 as well.
> 
> On 7/21/20 2:29 PM, Pratyush Yadav wrote:
> 
> >>> @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
> >>>         struct spi_nor_flash_parameter *params = nor->params;
> >>>         unsigned int cap;
> >>>
> >>> -       /* DTR modes are not supported yet, mask them all. */
> >>> -       *hwcaps &= ~SNOR_HWCAPS_DTR;
> >>> -
> >>>         /* X-X-X modes are not supported yet, mask them all. */
> >>>         *hwcaps &= ~SNOR_HWCAPS_X_X_X;
> >>>
> >>> +       /*
> >>> +        * If the reset line is broken, we do not want to enter a stateful
> >>> +        * mode.
> >>> +        */
> >>> +       if (nor->flags & SNOR_F_BROKEN_RESET)
> >>> +               *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);
> >>
> >> A dedicated reset line is not enough for flashes that keep their state
> >> in non-volatile bits. Since we can't protect from unexpected crashes in
> >> the non volatile state case, we should enter these modes only with an
> >> explicit request, i.e. an optional DT property: "update-nonvolatile-state",
> >> or something similar.
> > 
> > I wrote this patch with the assumption that we won't be supporting> non-volatile configuration as of now. In the previous discussions we
> 
> I think we have to take care of the stateful flashes now, otherwise we'll risk
> to end up with users that let their flashes in a mode from which they can't recover.
> I made some small RFC patches in reply to your v13, let me know what you think.

I haven't gone through them yet. Will check tomorrow.
 
> > came to the conclusion that it is not easy to detect the flash if it
> > boots in any mode other than 1S-1S-1S [0]. So if we update non-volatile
> > state, the flash would be useless after a reboot because we won't be
> > able to detect it in 8D mode. It doesn't matter if the reset line is
> > connected or not because it will reset the flash to the non-volatile
> > state, and we can't detect it from the non-volatile state.
> 
> correct, so a reset line for stateful modes doesn't help and the comment from the
> code should be updated. s/stateful/stateless

We are talking about two different kinds of "state" here. The state you 
are talking about is the persistent state of the flash configured via 
non-volatile registers. Yes, a reset line doesn't help in that case at 
all.

The other state is the non-persistent state we set on the flash. Using 
1S-1S-8D mode is stateless in the sense that we didn't change any state 
on the flash to be able to use this mode, and only had to use the 
correct opcode. If we execute a 1S-1S-1S command next it will also work 
because the flash is still interpreting opcodes in 1S mode. Using 
8D-8D-8D or 4S-4S-4S mode is stateful because we did have to configure 
some state on the flash (which can very well be volatile). Once 8D-8D-8D 
or 4S-4S-4S mode is entered, we cannot execute 1S-1S-1S commands until 
we reset the flash because now the flash is interpreting commands in 4S 
or 8D mode. This means we introduced some state on the flash.

Having a reset line will not help against the former but will help 
against the latter. If the flash is in a stateful mode like 8D-8D-8D 
without a reset line, an unexpected reset could leave bootloader unable 
to boot because it issues the commands in 1S-1S-1S mode that the flash 
cannot interpret. So even if the state we set is volatile, we still want 
to avoid doing it if there is no reset line.

So I think the code and comment should stay as they are.

> > 
> >> For the volatile state case, we can parse the SFDP SCCR map, save if we
> >> can enter stateful modes in a volatile way, and if yes allow the entering.
> > 
> > If we are not support volatile configurations, the reset line is enough
> > to take care of unexpected resets, no? I don't see any need to parse
> 
> the reset line is excellent for the stateless flashes, it guarantees that the
> volatile bits will return to their default state. Disabling the clock, waiting
> for a period and re-enabling it again should do the trick too, but probably
> a dedicated reset line is safer.
> 
> > SCCR map just for this.
> 
> This fits the RFC that I sent to your v13. We need to parse as much as we can
> from the SFDP tables so that we don't abuse the flash info flags.
> 
> > 
> >> Do the flashes that you played with define the SFDP SCCR map?
> > 
> > FWIW, the Cypress S28HS512T flash does but the Micron MT35XU512ABA does
> > not.
> > 
> >>> +
> >>>         for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) {
> >>>                 int rdidx, ppidx;
> >>>
> >>> @@ -2628,7 +2781,7 @@ static int spi_nor_default_setup(struct spi_nor *nor,
> >>>                  * controller directly implements the spi_nor interface.
> >>>                  * Yet another reason to switch to spi-mem.
> >>>                  */
> >>> -               ignored_mask = SNOR_HWCAPS_X_X_X;
> >>> +               ignored_mask = SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR;
> >>>                 if (shared_mask & ignored_mask) {
> >>>                         dev_dbg(nor->dev,
> >>>                                 "SPI n-n-n protocols are not supported.\n");
> >>> @@ -2774,11 +2927,25 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
> >>>                                           SNOR_PROTO_1_1_8);
> >>>         }
> >>>
> >>> +       if (info->flags & SPI_NOR_OCTAL_DTR_READ) {
> >>
> >> Why do we need this flag? Can't we determine if the flash supports
> >> octal DTR by parsing SFDP?
> > 
> > For Cypress S28HS512T, we can since it is xSPI compliant. We can't do
> > that for Micron MT35XU512ABA since it is not xSPI compliant.
> 
> Ok
> 
> > 
> >>> +               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, SPINOR_OP_READ_FAST,
> >>> +                                         SNOR_PROTO_8_8_8_DTR);
> >>> +       }
> >>> +
> >>>         /* Page Program settings. */
> >>>         params->hwcaps.mask |= SNOR_HWCAPS_PP;
> >>>         spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP],
> >>>                                 SPINOR_OP_PP, SNOR_PROTO_1_1_1);
> >>>
> >>> +       /*
> >>> +        * Since xSPI Page Program opcode is backward compatible with
> >>> +        * Legacy SPI, use Legacy SPI opcode there as well.
> >>> +        */
> >>> +       spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP_8_8_8_DTR],
> >>> +                               SPINOR_OP_PP, SNOR_PROTO_8_8_8_DTR);
> >>> +
> >>
> >> This looks fishy. You haven't updated the hwcaps.mask, these pp settings never
> >> get selected?
> > 
> > The problem here is that I don't see any field/table in SFDP that can
> > tell us {if,which} 8D-8D-8D program commands are supported. The xSPI
> > spec says that "The program commands provide SPI backward compatible
> > commands for programming data...".
> > 
> > So we populate the 8D page program opcodes here (and in 4bait parsing)
> > using the 1S opcodes. The flashes have to enable the hwcap in fixup
> > hooks.
> 
> I see. Would be good if you write this description as a comment, or in the commit
> message.
> 
> > 
> > As an alternative, maybe we can introduce the SPI_NOR_OCTAL_DTR_PP flag
> > that can enable the hwcap here? Thoughts?
> 
> Should be fine the way that you did. We can change this later on if needed.

I have already added it in v11 and later. Since it is already there I 
suppose it can stay.
 
> > 
> >>>         /*
> >>>          * Sector Erase settings. Sort Erase Types in ascending order, with the
> >>>          * smallest erase size starting at BIT(0).
> >>> @@ -2886,7 +3053,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
> >>>
> >>>         spi_nor_manufacturer_init_params(nor);
> >>>
> >>> -       if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
> >>> +       if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> >>> +                                SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&
> >>>             !(nor->info->flags & SPI_NOR_SKIP_SFDP))
> >>>                 spi_nor_sfdp_init_params(nor);
> >>>
> >>> @@ -2948,7 +3116,9 @@ static int spi_nor_init(struct spi_nor *nor)
> >>>                 return err;
> >>>         }
> >>>
> >>> -       if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
> >>> +       if (nor->addr_width == 4 &&
> >>> +           !(nor->info->flags & SPI_NOR_OCTAL_DTR_READ) &&
> >>
> >> Why is the Octal DTR read exempted?
> > 
> > It is based on the assumption explained below that 8D mode will always
> > use 4-byte addresses so we don't need to explicitly enable 8D mode.
> > Although I think maybe we should exempt all flashes that support DTR
> > mode?
> 
> 4-4-4-dtr can work with 3-byte addresses, check MX25L25673G. 2-2-2-dtr should work too.

Yes, I didn't catch this before.
 
> 
> > 
> >>> +           !(nor->flags & SNOR_F_4B_OPCODES)) {
> >>>                 /*
> >>>                  * If the RESET# pin isn't hooked up properly, or the system
> >>>                  * otherwise doesn't perform a reset command in the boot
> >>> @@ -3007,6 +3177,9 @@ static int spi_nor_set_addr_width(struct spi_nor *nor)
> >>>  {
> >>>         if (nor->addr_width) {
> >>>                 /* already configured from SFDP */
> >>> +       } else if (spi_nor_protocol_is_dtr(nor->read_proto)) {
> >>> +                /* Always use 4-byte addresses in DTR mode. */
> >>> +               nor->addr_width = 4;
> >>
> >> Why? DTR with 3 byte addr width should be possible too.
> > 
> > Should it be? What would happen to the half cycle left over? Do we then
> > start the dummy phase in the middle of the cycle? We would also have to
> > start the data phase in the middle of a cycle as well and end the
> > transaction with half a cycle left over.
> > 
> > AFAIK, the controller I tested with (Cadence QSPI) does not support
> > this. Similarly, the two flashes this series adds support for, Cypress
> > S28HS512T and Micron MT35XU512ABA, don't support 3-byte address in 8D
> > mode. I'm not sure if there are any flashes or controllers that do.
> 
> how about conditioning this only for 8-8-8-dtr?

Yes, it should only apply for 8D-8D-8D. Will fix.
 
> I'll now jump to v13 and continue the review there.

-- 
Regards,
Pratyush Yadav
Texas Instruments India

Powered by blists - more mailing lists