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

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

[Bin Meng]

Explicitly list supported read commands in the flash prarmas table
for SST25* flash parts.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---
 drivers/mtd/spi/sf_params.c | 20 ++++++++++----------
 include/spi_flash.h         |  3 ++-
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/spi/sf_params.c b/drivers/mtd/spi/sf_params.c
index 453edf0..d5f3597 100644
--- a/drivers/mtd/spi/sf_params.c
+++ b/drivers/mtd/spi/sf_params.c
@@ -88,16 +88,16 @@ const struct spi_flash_params spi_flash_params_table[] = {
 	{"N25Q1024A",	   0x20bb21, 0x0,       64 * 1024,  2048, RD_FULL, WR_QPP | E_FSR | SECT_4K},
 #endif
 #ifdef CONFIG_SPI_FLASH_SST		/* SST */
-	{"SST25VF040B",	   0xbf258d, 0x0,	64 * 1024,     8,	0,          SECT_4K | SST_WP},
-	{"SST25VF080B",	   0xbf258e, 0x0,	64 * 1024,    16,	0,	    SECT_4K | SST_WP},
-	{"SST25VF016B",	   0xbf2541, 0x0,	64 * 1024,    32,	0,	    SECT_4K | SST_WP},
-	{"SST25VF032B",	   0xbf254a, 0x0,	64 * 1024,    64,	0,	    SECT_4K | SST_WP},
-	{"SST25VF064C",	   0xbf254b, 0x0,	64 * 1024,   128,	0,		     SECT_4K},
-	{"SST25WF512",	   0xbf2501, 0x0,	64 * 1024,     1,	0,	    SECT_4K | SST_WP},
-	{"SST25WF010",	   0xbf2502, 0x0,	64 * 1024,     2,       0,          SECT_4K | SST_WP},
-	{"SST25WF020",	   0xbf2503, 0x0,	64 * 1024,     4,       0,	    SECT_4K | SST_WP},
-	{"SST25WF040",	   0xbf2504, 0x0,	64 * 1024,     8,       0,	    SECT_4K | SST_WP},
-	{"SST25WF080",	   0xbf2505, 0x0,	64 * 1024,    16,       0,	    SECT_4K | SST_WP},
+	{"SST25VF040B",	   0xbf258d, 0x0,	64 * 1024,     8, RD_SLOW,	    SECT_4K | SST_WP},
+	{"SST25VF080B",	   0xbf258e, 0x0,	64 * 1024,    16, RD_SLOW,	    SECT_4K | SST_WP},
+	{"SST25VF016B",	   0xbf2541, 0x0,	64 * 1024,    32, RD_SLOW,	    SECT_4K | SST_WP},
+	{"SST25VF032B",	   0xbf254a, 0x0,	64 * 1024,    64, RD_SLOW,	    SECT_4K | SST_WP},
+	{"SST25VF064C",	   0xbf254b, 0x0,	64 * 1024,   128, RD_EXTN,		     SECT_4K},
+	{"SST25WF512",	   0xbf2501, 0x0,	64 * 1024,     1, RD_SLOW,	    SECT_4K | SST_WP},
+	{"SST25WF010",	   0xbf2502, 0x0,	64 * 1024,     2, RD_SLOW,	    SECT_4K | SST_WP},
+	{"SST25WF020",	   0xbf2503, 0x0,	64 * 1024,     4, RD_SLOW,	    SECT_4K | SST_WP},
+	{"SST25WF040",	   0xbf2504, 0x0,	64 * 1024,     8, RD_SLOW,	    SECT_4K | SST_WP},
+	{"SST25WF080",	   0xbf2505, 0x0,	64 * 1024,    16, RD_SLOW,	    SECT_4K | SST_WP},
 #endif
 #ifdef CONFIG_SPI_FLASH_WINBOND		/* WINBOND */
 	{"W25P80",	   0xef2014, 0x0,	64 * 1024,    16,	0,		           0},
diff --git a/include/spi_flash.h b/include/spi_flash.h
index 408a5b4..a75e030 100644
--- a/include/spi_flash.h
+++ b/include/spi_flash.h
@@ -46,7 +46,8 @@ enum spi_read_cmds {
 	QUAD_OUTPUT_FAST = 1 << 3,
 	QUAD_IO_FAST = 1 << 4,
 };
-#define RD_EXTN		ARRAY_SLOW | DUAL_OUTPUT_FAST | DUAL_IO_FAST
+#define RD_SLOW		ARRAY_SLOW
+#define RD_EXTN		RD_SLOW | DUAL_OUTPUT_FAST | DUAL_IO_FAST
 #define RD_FULL		RD_EXTN | QUAD_OUTPUT_FAST | QUAD_IO_FAST
 
 /* Dual SPI flash memories */
-- 
1.8.2.1

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

[Simon Glass]

On 23 October 2014 07:37, Bin Meng <bmeng.cn@gmail.com> wrote:
> Explicitly list supported read commands in the flash prarmas table
> for SST25* flash parts.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>  drivers/mtd/spi/sf_params.c | 20 ++++++++++----------
>  include/spi_flash.h         |  3 ++-
>  2 files changed, 12 insertions(+), 11 deletions(-)

Acked-by: Simon Glass <sjg@chromium.org>

Tested on link (ich9)

Tested-by: Simon Glass <sjg@chromium.org>

(Again a few conflicts with the DM SPI series - will push to u-boot-x86/testing)

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

[Jagan Teki]

On 23 October 2014 19:07, Bin Meng <bmeng.cn@gmail.com> wrote:
> Explicitly list supported read commands in the flash prarmas table
> for SST25* flash parts.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>  drivers/mtd/spi/sf_params.c | 20 ++++++++++----------
>  include/spi_flash.h         |  3 ++-
>  2 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mtd/spi/sf_params.c b/drivers/mtd/spi/sf_params.c
> index 453edf0..d5f3597 100644
> --- a/drivers/mtd/spi/sf_params.c
> +++ b/drivers/mtd/spi/sf_params.c
> @@ -88,16 +88,16 @@ const struct spi_flash_params spi_flash_params_table[] = {
>         {"N25Q1024A",      0x20bb21, 0x0,       64 * 1024,  2048, RD_FULL, WR_QPP | E_FSR | SECT_4K},
>  #endif
>  #ifdef CONFIG_SPI_FLASH_SST            /* SST */
> -       {"SST25VF040B",    0xbf258d, 0x0,       64 * 1024,     8,       0,          SECT_4K | SST_WP},
> -       {"SST25VF080B",    0xbf258e, 0x0,       64 * 1024,    16,       0,          SECT_4K | SST_WP},
> -       {"SST25VF016B",    0xbf2541, 0x0,       64 * 1024,    32,       0,          SECT_4K | SST_WP},
> -       {"SST25VF032B",    0xbf254a, 0x0,       64 * 1024,    64,       0,          SECT_4K | SST_WP},
> -       {"SST25VF064C",    0xbf254b, 0x0,       64 * 1024,   128,       0,                   SECT_4K},
> -       {"SST25WF512",     0xbf2501, 0x0,       64 * 1024,     1,       0,          SECT_4K | SST_WP},
> -       {"SST25WF010",     0xbf2502, 0x0,       64 * 1024,     2,       0,          SECT_4K | SST_WP},
> -       {"SST25WF020",     0xbf2503, 0x0,       64 * 1024,     4,       0,          SECT_4K | SST_WP},
> -       {"SST25WF040",     0xbf2504, 0x0,       64 * 1024,     8,       0,          SECT_4K | SST_WP},
> -       {"SST25WF080",     0xbf2505, 0x0,       64 * 1024,    16,       0,          SECT_4K | SST_WP},
> +       {"SST25VF040B",    0xbf258d, 0x0,       64 * 1024,     8, RD_SLOW,          SECT_4K | SST_WP},
> +       {"SST25VF080B",    0xbf258e, 0x0,       64 * 1024,    16, RD_SLOW,          SECT_4K | SST_WP},
> +       {"SST25VF016B",    0xbf2541, 0x0,       64 * 1024,    32, RD_SLOW,          SECT_4K | SST_WP},
> +       {"SST25VF032B",    0xbf254a, 0x0,       64 * 1024,    64, RD_SLOW,          SECT_4K | SST_WP},
> +       {"SST25VF064C",    0xbf254b, 0x0,       64 * 1024,   128, RD_EXTN,                   SECT_4K},
> +       {"SST25WF512",     0xbf2501, 0x0,       64 * 1024,     1, RD_SLOW,          SECT_4K | SST_WP},
> +       {"SST25WF010",     0xbf2502, 0x0,       64 * 1024,     2, RD_SLOW,          SECT_4K | SST_WP},
> +       {"SST25WF020",     0xbf2503, 0x0,       64 * 1024,     4, RD_SLOW,          SECT_4K | SST_WP},
> +       {"SST25WF040",     0xbf2504, 0x0,       64 * 1024,     8, RD_SLOW,          SECT_4K | SST_WP},
> +       {"SST25WF080",     0xbf2505, 0x0,       64 * 1024,    16, RD_SLOW,          SECT_4K | SST_WP},
>  #endif
>  #ifdef CONFIG_SPI_FLASH_WINBOND                /* WINBOND */
>         {"W25P80",         0xef2014, 0x0,       64 * 1024,    16,       0,                         0},
> diff --git a/include/spi_flash.h b/include/spi_flash.h
> index 408a5b4..a75e030 100644
> --- a/include/spi_flash.h
> +++ b/include/spi_flash.h
> @@ -46,7 +46,8 @@ enum spi_read_cmds {
>         QUAD_OUTPUT_FAST = 1 << 3,
>         QUAD_IO_FAST = 1 << 4,
>  };
> -#define RD_EXTN                ARRAY_SLOW | DUAL_OUTPUT_FAST | DUAL_IO_FAST
> +#define RD_SLOW                ARRAY_SLOW
> +#define RD_EXTN                RD_SLOW | DUAL_OUTPUT_FAST | DUAL_IO_FAST
>  #define RD_FULL                RD_EXTN | QUAD_OUTPUT_FAST | QUAD_IO_FAST
>
>  /* Dual SPI flash memories */
> --
> 1.8.2.1
>

What is this, by defining RD_EXTN the fastest read cmd will pick based
on controller mode_rx
Why this RD_SLOW again? does this means the specific flash part will
only support slow read?

thanks!
-- 
Jagan.

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

[Bin Meng]

Hi Jegan,

On Tue, Oct 28, 2014 at 2:08 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> What is this, by defining RD_EXTN the fastest read cmd will pick based
> on controller mode_rx
> Why this RD_SLOW again? does this means the specific flash part will
> only support slow read?

Correct, the SST25* (except SST25VF064C) only supports the array read
(03h) and fast array read (0Bh) command.

Regards,
Bin

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

[Jagan Teki]

Hi Bin,

On 28 October 2014 06:52, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Jegan,
>
> On Tue, Oct 28, 2014 at 2:08 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>> What is this, by defining RD_EXTN the fastest read cmd will pick based
>> on controller mode_rx
>> Why this RD_SLOW again? does this means the specific flash part will
>> only support slow read?
>
> Correct, the SST25* (except SST25VF064C) only supports the array read
> (03h) and fast array read (0Bh) command.

Then make sure to add 0 for picking up fast read(default) and if you
need a slow read
to be operate all the ways just define RD_EXTN flash side and
SPI_OPM_RX_AS (but
which is not a recommended one as we have fast read supporting).

thanks!
-- 
Jagan.

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

[Bin Meng]

Hi Jagan,

On Tue, Oct 28, 2014 at 3:09 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> Hi Bin,
>
> On 28 October 2014 06:52, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Jegan,
>>
>> On Tue, Oct 28, 2014 at 2:08 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>> What is this, by defining RD_EXTN the fastest read cmd will pick based
>>> on controller mode_rx
>>> Why this RD_SLOW again? does this means the specific flash part will
>>> only support slow read?
>>
>> Correct, the SST25* (except SST25VF064C) only supports the array read
>> (03h) and fast array read (0Bh) command.
>
> Then make sure to add 0 for picking up fast read(default) and if you
> need a slow read
> to be operate all the ways just define RD_EXTN flash side and
> SPI_OPM_RX_AS (but
> which is not a recommended one as we have fast read supporting).

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

Regards,
Bin

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

[Jagan Teki]

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@gmail.com> wrote:
> Hi Jagan,
>
> On Tue, Oct 28, 2014 at 3:09 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>> Hi Bin,
>>
>> On 28 October 2014 06:52, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Jegan,
>>>
>>> On Tue, Oct 28, 2014 at 2:08 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>>> What is this, by defining RD_EXTN the fastest read cmd will pick based
>>>> on controller mode_rx
>>>> Why this RD_SLOW again? does this means the specific flash part will
>>>> only support slow read?
>>>
>>> Correct, the SST25* (except SST25VF064C) only supports the array read
>>> (03h) and fast array read (0Bh) command.
>>
>> Then make sure to add 0 for picking up fast read(default) and if you
>> need a slow read
>> to be operate all the ways just define RD_EXTN flash side and
>> SPI_OPM_RX_AS (but
>> which is not a recommended one as we have fast read supporting).
>
> 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.

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

Let me know if you still need to know more.

thanks!
-- 
Jagan.

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

[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@gmail.com> wrote:
> Hi Jagan,
>
> Thanks for the detailed explanation.
>
> On Tue, Oct 28, 2014 at 6:19 PM, Jagan Teki <jagannadh.teki@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@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.

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

[Bin Meng]

Hi Jagan,

On Wed, Oct 29, 2014 at 2:16 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> Hi Bin,
>
> 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.

It is not the flash but the controller only supports array slow read.
Yes, odd controller indeed, but supporting fast read needs a dummy
cycle which I believe requires the controller to know this fact.

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

It's no surprise. The spi controller you tested should support both
fast read and slow read.

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

OK, will do in v2 patch. Thanks for the comments.

Regards,
Bin