[U-Boot] [PATCH 4/5] sf: Update SST25* flash params of supported read commands

[Jagan Teki]

Hi Bin,

On 28 October 2014 16:09, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Jagan,
>
> Thanks for the detailed explanation.
>
> On Tue, Oct 28, 2014 at 6:19 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>> Hi Bin,
>>
>> Your understanding is quite normal - like the way of adding supported read
>> command support, but here the sf logic to pick the read command is different.
>>
>> Please see below.
>>
>> On 28 October 2014 14:08, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> I am not sure if I understand it correctly. Do you mean adding ARRAY_FAST to:
>>>
>>> enum spi_read_cmds {
>>>         ARRAY_SLOW              = 1 << 0,
>>>         DUAL_OUTPUT_FAST        = 1 << 1,
>>>         DUAL_IO_FAST            = 1 << 2,
>>>         QUAD_OUTPUT_FAST        = 1 << 3,
>>>         QUAD_IO_FAST            = 1 << 4,
>>> +       ARRAY_FAST              = 1 << 5,
>>> };
>>>
>>> then changing the SST flash parameters like this:
>>>
>>> -       {"SST25WF080",     0xbf2505, 0x0,       64 * 1024,    16,
>>>  0,          SECT_4K | SST_WP},
>>> +       {"SST25VF040B",    0xbf258d, 0x0,       64 * 1024,     8,
>>> RD_SLOW | ARRAY_FAST,          SECT_4K | SST_WP},
>>>
>>> I originally intended to do this, however I am not sure whether I
>>> should give ARRAY_FAST to 1 << 5 (meaning the fastest read command
>>> than any others). Maybe it should be 1 << 3? Also I need update every
>>> spi controller driver to initialize spi->op_mode_rx to something
>>> meaningful. Currently only ti_qspi.c has such initialization
>>> (slave->op_mode_rx = 8;).
>>
>> Legacy spi-flash has a default support for fast reads, as new sf added e_rd_cmd
>> the way to support or extend the read commands.
>>
>> Suppose the flash support all different reads slow, fast, quad and
>> io's then sf_params
>> will define RD_FULL and then it's upto controller driver to handle
>> these rx transfers or not.
>> Say my driver will have only single wire (say array slow or fast) then
>> no need to define op_mode_rx
>> on driver as it's operates legacy and will pick the fast read by
>> defining e_rd_cmd as 0
>>
>> For the same flash above if the driver is able to work for 2 or 4 wire
>> then e_rd_cmd will
>> define as best supported rx transfer in ti_qspi case 8, Quad.
>>
>> The idea is clear we always get the fastest read by taking controller
>> driver [(q)spi controller]
>> as a priority irrespective of supporting flash. So in your case the
>> flash is supporting array slow
>> and fast and driver will able to do fast rx transfer, so there is no
>> requirements to define op_mode_rx
>> on driver and e_rd_cmd on flash - it simply take fast read.
>
> Yes, I know this. The issue is that with some controller (like the ich
> one) only array slow is supported. Thus the spi flash params should
> announce this capability when I set the spi controller op_mode_rx to
> array slow only otherwise the sf subsystem will choose fast read, just
> as you indicated. Right now my patches only updates the SST25* flash
> params as I have tested them but ideally I should update all flash
> params. Before that I still need cross check datasheets of all current
> supported spi flashes in sf_params.c to get to know whether it is true
> that when a flash supports array read it must support fast array read
> as well though.

This looks odd to me - supporting array slow without support on fast read.
Please check the datasheets, and as per as my knowledge/experience it
should support both.

Note: I have tested SST25WF080 with fast read couple of times.

>
>> See below code for more understanding:
>> /* Look for the fastest read cmd */
>> cmd = fls(params->e_rd_cmd & flash->spi->op_mode_rx);
>> if (cmd) {
>>           cmd = spi_read_cmds_array[cmd - 1];
>>           flash->read_cmd = cmd;
>> } else {
>>            /* Go for default supported read cmd */
>>            flash->read_cmd = CMD_READ_ARRAY_FAST;
>> }
>
> Yes, I understand the logic here as well. That's why I asked in my
> previous email
>
>> +ARRAY_FAST              = 1 << 5,
>
> Whether this one (fast array read) should become 1 << 5. Also I
> started to question whether it is OK to group these command together
> like below
>
> #define RD_EXTN (ARRAY_SLOW | DUAL_OUTPUT_FAST | DUAL_IO_FAST)
> #define RD_FULL (RD_EXTN | QUAD_OUTPUT_FAST | QUAD_IO_FAST)
>
> Again I need cross check flash datasheets.

Please do - for all [if possible]

thanks!
-- 
Jagan.