[U-Boot] saveenv corrupts QSPI flash with latest commit U-Boot 2019.04-rc4-00047-gcfb3e102c4

[Ashish Kumar <ashish.kumar@nxp.com>]

> -----Original Message-----
> From: Vignesh Raghavendra <vigneshr@ti.com>
> Sent: Friday, March 29, 2019 12:17 PM
> To: Ashish Kumar <ashish.kumar@nxp.com>; Jagan Teki
> <jagan@openedev.com>; u-boot at lists.denx.de
> Cc: Kuldeep Singh <kuldeep.singh@nxp.com>
> Subject: Re: saveenv corrupts QSPI flash with latest commit U-Boot 2019.04-rc4-
> 00047-gcfb3e102c4
> 
> 
> 
> On 28/03/19 3:37 PM, Ashish Kumar wrote:
> >> -----Original Message-----
> >> From: Vignesh Raghavendra <vigneshr@ti.com>
> >> Sent: Wednesday, March 27, 2019 9:44 AM
> >> To: Ashish Kumar <ashish.kumar@nxp.com>; Jagan Teki
> >> <jagan@openedev.com>; u-boot at lists.denx.de; Tom Rini
> >> <trini@konsulko.com>
> >> Cc: Kuldeep Singh <kuldeep.singh@nxp.com>
> >> Subject: Re: saveenv corrupts QSPI flash with latest commit U-Boot
> >> 2019.04-rc4-
> >> 00047-gcfb3e102c4
> >>
> >>
> >>
> >> On 26/03/19 7:11 PM, Ashish Kumar wrote:
> >>>>
> >>>> On 26/03/19 10:36 AM, Ashish Kumar wrote:
> >>>>> Hello Maintainers,
> >>>>>
> >>>>>
> >>>>>
> >>>>> With latest commit I see that saveenv command corrupts QSPI-flash,
> >>>>> meaning if I read QSPI-flash at 0x0 offset RCW(reset configuration
> >>>>> word) is erased after saveenv command was executed.
> >>>>>
> >>>>> This is tested on LS1088ARDB, but it should not be limited to
> >>>>> LS1088ARDB rather it will valid for all LS Freescale ARM boards.(
> >>>>> like LS1088, LS2080/LS2085, LS1012, LS1043, LS1046).
> >>>>>
> >>>>
> >>>> Which is the SPI controller driver? Does the controller driver
> >>>> support reading/writing to flash using 4 byte addressing opcode?
> >>>
> >>> fsl_qspi.c. As per the migration it was not migrated to 4 byte.
> >>> Although it
> >> actually supports 4 byte for some SoC and 3 byte for older SoC that
> >> is the reason you see CONFIG_SPI_FLASH_BAR in code. My concerned SoC
> >> are all supporting
> >> 4 byte. I have not added any code modification in the current u-boot.
> >>>>
> >>
> >> OK, does normal read/write to offset 0 work fine? That is:
> >>
> >> sf probe
> >> sf erase 0x0 40000
> >> sf write <add1r> 0x0 0x40000 (write some random data) sf read <addr2>
> >> 0x0
> >> 0x40000 (read back) md.b <addr2>
> >>
> >> If this fails. Could you post full debug log (all debug prints
> >> enabled in
> >> spi_mem_exec_op()) to pastebin.ubuntu.com and provide a link here?
> >
> > Hi Vignesh,
> > This is working now(READ, WRITE), after some change in fsl_qspi driver, where
> I check for 4byte op codes now.
> 
> Hmm, I don't expect any 4 byte opcode to be used when SPI_FLASH_BAR is
> enabled.
> Could you what 4byte opcodes are being used and share the changes here?
Since SPI_FLASH_BAR and set_4byte are inversely related, I had disable this SPI_FLASH_BAR via defconfig, and 
Set this 4B_OP_CODE in SPANSION flash ids.
I will post my patch in upstream. Once I test the new flow of erase as suggested below.

> 
> > But now I see that erase is getting address as ZERO. Which in my
> > opinion is  because
> > spi_nor_erase_sector() call write_reg which has SPI_MEM_OP_NO_ADDR?
> >
> > /*
> >  * Initiate the erasure of a single sector  */ static int
> > spi_nor_erase_sector(struct spi_nor *nor, u32 addr) {
> >         u8 buf[SPI_NOR_MAX_ADDR_WIDTH];
> >         int i;
> >
> >         if (nor->erase)
> >                 return nor->erase(nor, addr);
> >
> >         /*
> >          * Default implementation, if driver doesn't have a specialized HW
> >          * control
> >          */
> >         for (i = nor->addr_width - 1; i >= 0; i--) {
> >                 buf[i] = addr & 0xff;
> >                 addr >>= 8;
> >         }
> >
> >         return nor->write_reg(nor, nor->erase_opcode, buf,
> > nor->addr_width); }
> >
> >
> > static int spi_nor_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
> > int len) {
> >         struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 1),
> >                                           SPI_MEM_OP_NO_ADDR,
> >                                           SPI_MEM_OP_NO_DUMMY,
> >                                           SPI_MEM_OP_DATA_OUT(len,
> > NULL, 1));
> >
> >                 return spi_nor_read_write_reg(nor, &op, buf); }
> >
> 
> The address of the sector to erased is in "buf".
Ok. So the address is in second xfer call, i.e. " SPI_XFER_END "

> And rationale in using nor->write_reg() for erase is that, in case of controllers
> that support memory mapped access to flash, read/write is done via MMIO
> interface whereas erase is using done via register/FIFO interface.
Could you please explain meaning of register/FIFO interface ? how erase is different from write?

> 
> But, problem is at driver that tries to interpret spi_xfer() to decode messages into
> cmd+addr+dummy+data format. With moving to spi-mem layer, erase sector
> cmd+addr+dummy+address is
> now passed as part of message that has SPI_XFER_END (instead of
> SPI_XFER_BEGIN) flag.
> 
> fsl_qspi.c driver should be able to handle this similar to register write commands
> such as Bank Register write etc.
> 
> Something along the line of below diff(untested and just an indicative fix) this:
> 
> diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index
> 1598c4f69890..1fd12b8728d3 100644
> --- a/drivers/spi/fsl_qspi.c
> +++ b/drivers/spi/fsl_qspi.c
> @@ -783,18 +783,21 @@ int qspi_xfer(struct fsl_qspi_priv *priv, unsigned int
> bitlen,
>                 }
> 
>                 if (flags == SPI_XFER_END) {
> -                       priv->sf_addr = wr_sfaddr;
> -                       qspi_op_write(priv, (u8 *)dout, bytes);
> +                       if ((priv->cur_seqid == QSPI_CMD_SE) ||
> +                                       (priv->cur_seqid == QSPI_CMD_BE_4K)) {
> +                               /* DO: populate priv->sf_addr with addr from
> +                                * dout */
> +                               qspi_op_erase(priv);
> +                       } else {
> +                               priv->sf_addr = wr_sfaddr;
> +                               qspi_op_write(priv, (u8 *)dout, bytes);
> +                       }
Yes something similar should work.

>                         return 0;
>                 }
> 
>                 if (priv->cur_seqid == QSPI_CMD_FAST_READ ||
>                     priv->cur_seqid == QSPI_CMD_RDAR) {
>                         priv->sf_addr = swab32(txbuf) & OFFSET_BITS_MASK;
> -               } else if ((priv->cur_seqid == QSPI_CMD_SE) ||
> -                          (priv->cur_seqid == QSPI_CMD_BE_4K)) {
> -                       priv->sf_addr = swab32(txbuf) & OFFSET_BITS_MASK;
> -                       qspi_op_erase(priv);
>                 } else if (priv->cur_seqid == QSPI_CMD_PP ||
>                            priv->cur_seqid == QSPI_CMD_WRAR) {
>                         wr_sfaddr = swab32(txbuf) & OFFSET_BITS_MASK
> 
> 
> Honestly, fsl_qspi.c is almost a parallel spi-nor-core framework and should be
> moved to spi-mem as soon as possible.
> Otherwise any simple change to spi-nor-core will break the driver.
First I will post this these changes that will unblock fsl_qspi hopefully. Next step would be to migrated to mem_ops.

Regards
Ashish 
> 
> 
> Regards
> Vignesh
> 
> > Regards
> > Ashish
> >>
> >>>> Could you enable all the debug prints in spi_mem_exec_op()
> >>>> (especially those at the end)?
> >>> Logs are very verbose: I have commented two debug logs which are in
> >>> for loop Logs here are from the end, 0b 33 f3 43 | [59B in] [ret 0]
> >>
> >> I dont know the address from which saveenv is trying to read from.
> >> But does the address bytes match (33 f3 43)?
> >>
> >> [...]
> >>> 0b 33 ff f0 | [16B in] [ret 0]
> >>> Erasing SPI flash...06 | [0B -] [ret 0]
> >>> d8 | [3B out] [ret 0]
> >>
> >>
> >> This is the sector erase command but you haven't enabled log for the
> >> part where address of the sector is dumped. Could you provide that info?
> >>
> >> Also, the commit ID in the subject is not present in upstream tree.
> >> So I am not sure what exactly maybe broken. Does 2019.01 work fine?
> >> U-Boot had major sf layer refactoring in 2019.04-rc1. Could you see
> >> if
> >> 2019.04-rc1 works fine?
> >>
> >>
> >>
> >>
> >> --
> >> Regards
> >> Vignesh
> 
> --
> Regards
> Vignesh