[U-Boot] [PATCH 4/4] sf: Add Quad-input Page Program(32h) instruction support

[Jagan Teki]

Hi Thomas,

On Tue, Dec 11, 2012 at 5:00 PM, Langer Thomas (LQDE RD ST PON SW)
<thomas.langer at lantiq.com> wrote:
> Hello Jagannadha,
>
> I have some remarks and questions, as I currently work on a hw platform, which also would allow
> to support dual- or quad-io accesses.

Planning to add reads too..

>
> So my first question: why is this restricted to write only? If you have a hardware, which is capable
> of supporting this, the read will definitely benefit from it, while the speedup for write depends on
> the internal programming time of the flash.

Yes I will try to add these in near feature.

>
> The other questions: On which hardware platform was this tested and for which flashes?
> Patches for both components are missing from this patch series.

Yes I am missing flash info..My plan will add the flash source.
Once this code is in a recommended position.

> And for both I have more remarks below.
>
> Jagannadha Sutradharudu Teki wrote on 2012-12-10:
>> This patch provides support to program a flash using
>> Quad-input Page Program(32h) instruction.
>>
>> This will effectively increases the data transfer rate
>> by up to four times, as compared to the Page Program(PP) instruction.
>>
>> Respective flash drivers need to use spi_flash_cmd_write_multi_qpp()
>> based on their usage.
>>
>
>
>> +#ifdef CONFIG_CMD_SF_QPP
>> +     else if (strcmp(argv[0], "write.qpp") == 0)
>> +             ret = spi_flash_write_qpp(flash, offset, len, buf);
>> +#endif
> Is it really necessary to have a dedicated command here? Wouldn't it be better, if the SF layer or
> the driver will use it automatically, if the hardware supports it and the driver has detected the
> feature of the flash?

But I couldn't see any way to detect the read/write modes though
dedicated flash register.
Appreciate if you give any suggestion.

>
>> +#ifdef CONFIG_CMD_SF_QPP
>> +int spi_flash_set_quad_enable_bit(struct spi_flash *flash)
>> +{
> [...]
>> +
>> +     if (data & 0x2) {
>> +             debug("SF: quad enable bit is already set.\n");
> Is this bit common for all flashes? Otherwise add some comments on tested flashed
> and/or TODO for an extension to provide this info from the flash driver.

OK, Thanks.

>
>> +             return ret;
>> +     } else {
>> +             debug("SF: need to set quad enable bit\n");
>> +
>> +             ret = spi_flash_cmd_write_config(flash, 0x0200);
> Same here. And why is this a 16-bit value here?

Yes WRR with 16-bit data.

WRR CMD+status reg+config reg.
8 + 8 + 8 = 8-bit CMD and 16-bit DATA.
>
>> +int spi_flash_cmd_write_multi_qpp(struct spi_flash *flash, u32 offset,
>> +             size_t len, const void *buf)
>> +{
>> +     unsigned long page_addr, byte_addr, page_size;
>> +     size_t chunk_len, actual;
>> +     int ret;
>> +     u8 cmd[4];
>> +
>> +     page_size = flash->page_size;
>> +     page_addr = offset / page_size;
>> +     byte_addr = offset % page_size;
>> +
>> +     ret = spi_claim_bus(flash->spi);
>> +     if (ret) {
>> +             debug("SF: unable to claim SPI bus\n");
>> +             return ret;
>> +     }
>> +
>> +     ret = spi_flash_set_quad_enable_bit(flash);
> I don't like this implicit setting here. And as far as I know, this bit is sticky/non-volatile. It is not
> necessary to write this each time.

I just check the config reg for quad bit if it is set already,  return.
otherwise set the bit and read it again.

> Maybe it make more sense to  have an interactive command to write this bit (enabled or disabled)
> to the flash?

yes i have the same thought, but thinking..this bit needs to set only
on QUAD modes.
Why this needs to be a separate commands.

> And then the flash probe function can check the bit and map to the appropriate read and write
> functions.

I am unclear.
Can you elaborate?

>
>> +     if (ret) {
>> +             debug("SF: set quad enable bit failed\n");
>> +             return ret;
>> +     }
>> +
>> +     cmd[0] = CMD_QUAD_PAGE_PROGRAM;
>> +     for (actual = 0; actual < len; actual += chunk_len) {
>> +             chunk_len = min(len - actual, page_size - byte_addr);
>> +
>> +             cmd[1] = page_addr >> 8;
>> +             cmd[2] = page_addr;
>> +             cmd[3] = byte_addr;
>> +
>> +             debug("PP: 0x%p => cmd = { 0x%02x 0x%02x%02x%02x }
>> chunk_len = %zu\n",
>> +                   buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len);
>> +
>> +             ret = spi_flash_cmd_write_enable(flash);
>> +             if (ret < 0) {
>> +                     debug("SF: enabling write failed\n");
>> +                     break;
>> +             }
>> +
>> +             ret = spi_flash_cmd_write(flash->spi, cmd, 4,
>> +                                       buf + actual, chunk_len);
>> +             if (ret < 0) {
>> +                     debug("SF: write failed\n");
>> +                     break;
>> +             }
> Except for the config bit and the different command code, I don't see any difference to the
> "regular" spi_flash_cmd_write_multi function. So the question is: How does the SPI framework

Yes, most of the code is redundant..
But my plan will short it up in near future a/f more testing. (not
touch the existing write call as of now)

> knows when to send 4 bits in parallel to 4 pins instead of the serialization to one signal pin?
>
> This extension to the SPI framework is completely missing!

Yes the entire bit logic is controller ed by specific controller driver.
MTD flash layer must send the respective commands.

Please add any inputs and let me know for any information.

Thanks,
Jagan.

>
>> diff --git a/include/spi_flash.h b/include/spi_flash.h
>> index 9da9062..61f0c19 100644
>> --- a/include/spi_flash.h
>> +++ b/include/spi_flash.h
>> @@ -43,6 +43,10 @@ struct spi_flash {
>>                               size_t len, void *buf);
>>       int             (*write)(struct spi_flash *flash, u32 offset,
>>                               size_t len, const void *buf);
>> +#ifdef CONFIG_CMD_SF_QPP
>> +     int             (*write_qpp)(struct spi_flash *flash, u32 offset,
>> +                             size_t len, const void *buf);
>
> The flash probe should detect, if QPP is possible, and then map the existing (*read) and (*write)
> pointers to the relevant functions. No new pointers required here.
>
>> +#endif
>>       int             (*erase)(struct spi_flash *flash, u32 offset,
>>                               size_t len);
>>  };
>
>> +#ifdef CONFIG_CMD_SF_QPP
>> +static inline int spi_flash_write_qpp(struct spi_flash *flash, u32 offset,
>> +             size_t len, const void *buf)
>> +{
>> +     return flash->write_qpp(flash, offset, len, buf);
>> +}
>> +#endif
> See above, this is also not required.
>
> I would be happy to support you by adapting and testing this extension on my platform.
>
> The plan on my side is to send the pieces to support my platform in the near future (not sure if
> my schedule will fit for the next merge window, but anyway..)
>
> Adding this feature on top would be very nice ;-)
>
> Best Regards,
> Thomas
>