RE: [PATCH] mtd: spi-nor: Add support to exit 4-byte mode

["Bhumkar, Tejas Arvind" <tejas.arvind.bhumkar@amd.com>]

[AMD Official Use Only - General]

Hi Greg,

> -----Original Message-----
> From: Greg Malysa <greg.malysa@timesys.com>
> Sent: Friday, March 29, 2024 6:15 AM
> To: Bhumkar, Tejas Arvind <tejas.arvind.bhumkar@amd.com>
> Cc: u-boot@lists.denx.de; jagan@amarulasolutions.com; vigneshr@ti.com;
> Simek, Michal <michal.simek@amd.com>; Abbarapu, Venkatesh
> <venkatesh.abbarapu@amd.com>; git (AMD-Xilinx) <git@amd.com>; Ian Roberts
> <ian.roberts@timesys.com>
> Subject: Re: [PATCH] mtd: spi-nor: Add support to exit 4-byte mode
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Tejas,
>
> + Ian Roberts, my coworker has the following comments:
>
> I do not think it is appropriate to put what appears to be use-case specific logic
> into the core functionality.
>
> Your problem statement sounds like the chip is stuck in a stateful mode after a
> reset. As alternatives, I would suggest:
> 1) Toggle the hardware reset line for the chip.

[Tejas ] : Apologies for the delayed response.
         There are no HW reset lines.

> 2) If one does not exist, use SPI_FLASH_SOFT_RESET_ON_BOOT

[Tejas] : For Kria boards, we use QSPI flash and in NOR framework, the soft reset involves an 8_8_8 protocol in mem-ops.

> 3) Alter your recovery application set the chip back to 3B mode on exit.

[Tejas] :
This would be good, but as per datasheet :
Table 35: ENTER and EXIT 4-BYTE ADDRESS MODE Operations
The effect of the command is immediate. The default address mode is three bytes,
and the device returns to the default upon exiting the 4-byte address mode.
Since we're in the default 3B mode, we need to transition out of the 4B mode.

>
> As is, I also think this would cause problems for chips with existing support.
> set_4byte() sends commands to the chip, which may be unrecognized or overlap
> with a manufacturer's custom command and cause unintended side effects.

[Tejas] : If we examine the "set_4byte" function, we'll find that it offers nearly universal support across all chips for enabling or disabling 4B mode. Could you provide more details about your custom command point? I'd like to understand it better.

>
> On Thu, Mar 28, 2024 at 11:37 AM Tejas Bhumkar
> <tejas.arvind.bhumkar@amd.com> wrote:
> >
> > The Kria board features a recovery application that activates when the
> > FW_EN button is pressed.
> > Upon power-up flash operates in 3B mode, However, the recovery
> > application changes it back to 4B mode.
> > Following a reset, u-boot activates the CONFIG_SPI_FLASH_BAR and
> > expects the flash to be in 3B mode. However, there's no code to handle
> > this configuration. to address this issue, changes were made to
> > disable the 4B mode when the CONFIG_SPI_FLASH_BAR is enabled.
> >
> > Additionally, spi_nor_wait_till_ready() was included because there is
> > operation that places the device in a busy state before performing a
> > nor read.
> >
> > Signed-off-by: Tejas Bhumkar <tejas.arvind.bhumkar@amd.com>
> > ---
> >  drivers/mtd/spi/spi-nor-core.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/mtd/spi/spi-nor-core.c
> > b/drivers/mtd/spi/spi-nor-core.c index f86003ca8c..47f65a4f5e 100644
> > --- a/drivers/mtd/spi/spi-nor-core.c
> > +++ b/drivers/mtd/spi/spi-nor-core.c
> > @@ -1464,6 +1464,9 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t
> from, size_t len,
> >                 else
> >                         read_len = remain_len;  #endif
> > +               ret = spi_nor_wait_till_ready(nor);
> > +               if (ret)
> > +                       goto read_err;
>
> Can you elaborate on the purpose of this wait_till_ready?

[Tejas] : This step serves to ensure that the flash is ready before proceeding with the read operation.

Thank You,
Tejas.
>
> >
> >                 ret = nor->read(nor, addr, read_len, buf);
> >                 if (ret == 0) {
> > @@ -4161,6 +4164,7 @@ int spi_nor_scan(struct spi_nor *nor)  #else
> >         /* Configure the BAR - discover bank cmds and read current bank */
> >         nor->addr_width = 3;
> > +       set_4byte(nor, info, 0);
> >         ret = read_bar(nor, info);
> >         if (ret < 0)
> >                 return ret;
> > --
> > 2.37.6
> >
>
> Thanks,
> Greg
>
> --
> Greg Malysa
> Timesys Corporation