[U-Boot] saveenv corrupts QSPI flash with latest commit U-Boot 2019.04-rc4-00047-gcfb3e102c4
Ashish Kumar
ashish.kumar at nxp.com
Fri Mar 29 11:48:28 UTC 2019
> -----Original Message-----
> From: Vignesh Raghavendra <vigneshr at ti.com>
> Sent: Friday, March 29, 2019 12:17 PM
> To: Ashish Kumar <ashish.kumar at nxp.com>; Jagan Teki
> <jagan at openedev.com>; u-boot at lists.denx.de
> Cc: Kuldeep Singh <kuldeep.singh at 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 at ti.com>
> >> Sent: Wednesday, March 27, 2019 9:44 AM
> >> To: Ashish Kumar <ashish.kumar at nxp.com>; Jagan Teki
> >> <jagan at openedev.com>; u-boot at lists.denx.de; Tom Rini
> >> <trini at konsulko.com>
> >> Cc: Kuldeep Singh <kuldeep.singh at 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
More information about the U-Boot
mailing list