Re: [PATCH v2 2/3] mtd: spi-nor: Altera ASMI Parallel II IP Core

[matthew . gerlach]

On Wed, 11 Oct 2017, Marek Vasut wrote:

> On 10/11/2017 07:00 PM, matthew.gerlach@xxxxxxxxxxxxxxx wrote:
> > On Tue, 10 Oct 2017, Marek Vasut wrote:
> >
> > > On 09/20/2017 08:28 PM, matthew.gerlach@xxxxxxxxxxxxxxx wrote:
> > > > From: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
> > > >
> > > > This patch adds support for a spi-nor, platform driver for the
> > > > Altera ASMI Parallel II IP Core. The intended use case is to be able
> > > > to update the flash used to load a FPGA at power up with mtd-utils.
> > > >
> > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
> > > > ---
> > > > v2:
> > > > ÂÂÂ minor checkpatch fixing by Wu Hao <hao.wu@xxxxxxxxx>
> > > > ÂÂÂ Use read_dummy value as suggested by Cyrille Pitchen.
> > > > ÂÂÂ Don't assume 4 byte addressing (Cryille Pichecn and Marek Vasut).
> > > > ÂÂÂ Fixed #define indenting as suggested by Marek Vasut.
> > > > ÂÂÂ Added units to timer values as suggested by Marek Vasut.
> > > > ÂÂÂ Use io(read|write)8_rep() as suggested by Marek Vasut.
> > > > ÂÂÂ Renamed function prefixed with __ as suggested by Marek Vasut.
> > >
> > > [...]
> > >
> > > > +#define QSPI_ACTION_REGÂÂÂÂÂÂÂÂÂÂÂ 0
> > > > +#define QSPI_ACTION_RSTÂÂÂÂÂÂÂÂÂÂÂ BIT(0)
> > > > +#define QSPI_ACTION_ENÂÂÂÂÂÂÂÂÂÂÂ BIT(1)
> > > > +#define QSPI_ACTION_SCÂÂÂÂÂÂÂÂÂÂÂ BIT(2)
> > > > +#define QSPI_ACTION_CHIP_SEL_SFTÂÂÂ 4
> > > > +#define QSPI_ACTION_DUMMY_SFTÂÂÂÂÂÂÂ 8
> > > > +#define QSPI_ACTION_READ_BACK_SFTÂÂÂ 16
> > > > +
> > > > +#define QSPI_FIFO_CNT_REGÂÂÂÂÂÂÂ 4
> > > > +#define QSPI_FIFO_DEPTHÂÂÂÂÂÂÂÂÂÂÂ 0x200
> > > > +#define QSPI_FIFO_CNT_MSKÂÂÂÂÂÂÂ 0x3ff
> > > > +#define QSPI_FIFO_CNT_RX_SFTÂÂÂÂÂÂÂ 0
> > > > +#define QSPI_FIFO_CNT_TX_SFTÂÂÂÂÂÂÂ 12
> > > > +
> > > > +#define QSPI_DATA_REGÂÂÂÂÂÂÂÂÂÂÂ 0x8
> > > > +
> > > > +#define QSPI_POLL_TIMEOUT_USÂÂÂÂÂÂÂ 10000000
> > >
> > > 10 s poll timeout ? :)
> >
> > Hi Marek,
> >
> > The 10s timeout is fairly arbitrary. In other words, I pulled it out of
> > thin air. Can you suggest a better timeout? From a practical
> > standpoint 10s seemed to be much better than no timeout when I was
> > debugging bad FPGA images. Without a timeout I was hanging the system
> > when the FPGA image failed. With this timeout, we get a nice message
> > and Linux keeps running happily.
> AFAIK the SPI subsystem has a timeout which is adaptive to the bus
> clock, maybe that's what you want to use here ?

Hi Marek,

I looked in spi-nor.c, and I see the following two macros:
#define DEFAULT_READY_WAIT_JIFFIES		(40UL * HZ)
#define CHIP_ERASE_2MB_READY_WAIT_JIFFIES	(40UL * HZ)

These timers values are used at the spi-nor layer for waiting for an
operation to complete.  It is the time spent waiting for Work In Progress
to clear.

The timer value I'm using, QSPI_POLL_TIMEOUT_US, is used at a lower layer.
This timer value is used for the time it takes to send the opcode and tx 
data and receive any bytes from the flash.  While 10 seconds may seem 
long, I am just trying to avoid a system hang when there is a catastrophic
failure with the flash controller in the FPGA or the flash part itself.

I certainly could be missing something with regards to the timeouts, but I 
think 10s for QSPI_POLL_TIMEOUT_US doesn't hurt, and it definately helps 
when something goes wrong.

Thanks for the feedback,
Matthew Gerlach

> --
> Best regards,
> Marek Vasut