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

[Bin Meng]

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.

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

Regards,
Bin