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

[Marek Vasut <marek.vasut@gmail.com>]

On 10/13/2017 09:24 PM, matthew.gerlach@linux.intel.com wrote:

[ culling the ridiculous cc list ]

> On Wed, 11 Oct 2017, Marek Vasut wrote:
> 
>> On 10/11/2017 07:00 PM, matthew.gerlach@linux.intel.com wrote:
>>>
>>>
>>> On Tue, 10 Oct 2017, Marek Vasut wrote:
>>>
>>>> On 09/20/2017 08:28 PM, matthew.gerlach@linux.intel.com wrote:
>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>
>>>>> 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@linux.intel.com>
>>>>> ---
>>>>> v2:
>>>>>     minor checkpatch fixing by Wu Hao <hao.wu@intel.com>
>>>>>     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.

It takes 10s to detect catastrophic failure ? I'd expect you can scale
that timeout value by the bus clock speed.

We have ie. this in drivers/spi/spi.c:

9                                 ret = 0;
1040                                 ms = 8LL * 1000LL * xfer->len;
1041                                 do_div(ms, xfer->speed_hz);
1042                                 ms += ms + 200; /* some tolerance */
1043
1044                                 if (ms > UINT_MAX)
1045                                         ms = UINT_MAX;
1046
1047                                 ms =
wait_for_completion_timeout(&master->xfer_completion,
1048
msecs_to_jiffies(ms));

> 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
>>


-- 
Best regards,
Marek Vasut