[PATCH v2 00/10] spi: dw: Add support for DUAL/QUAD/OCTAL modes

[Sean Anderson]

On 4/19/21 3:06 AM, Jagan Teki wrote:
> On Sat, Apr 3, 2021 at 4:37 AM Sean Anderson <seanga2 at gmail.com> wrote:
>>
>> On 4/2/21 7:05 PM, Sean Anderson wrote:
>>> This series adds support for enhanced SPI modes. It was tested on a K210 (DWC
>>> SSI with QSPI flash).
>>>
>>> If anyone has a designware device with QSPI flash attached (especially a DW SSI
>>> APB device), I'd greatly appreciate them testing out this patch series. Given
>>> that there has been no testing of v2 over the past month, I don't think lack of
>>> testing should hold up this series.
>>>
>>> Changes in v3:
>>> - Dropped merged patches
>>> - Rebased on u-boot/master
>>>
>>> Changes in v2:
>>> - Add more information to exec_op debug message
>>> - Actually mask interrupts
>>> - Merge CAP_{DUAL,QUAD,OCTAL} into CAP_ENHANCED
>>> - Fix some inconsistencies in register naming and usage
>>> - Moved some hunks between commits so things make more sense
>>>
>>> Sean Anderson (10):
>>>     mtd: spi-mem: Export spi_mem_default_supports_op
>>>     spi: spi-mem: Add debug message for spi-mem ops
>>>     spi: dw: Log status register on timeout
>>>     spi: dw: Actually mask interrupts
>>>     spi: dw: Switch to capabilities
>>>     spi: dw: Rewrite poll_transfer logic
>>>     spi: dw: Add ENHANCED cap
>>>     spi: dw: Define registers for enhanced mode
>>>     spi: dw: Support enhanced SPI
>>>     spi: dw: Support clock stretching
>>>
>>>    drivers/spi/designware_spi.c | 647 ++++++++++++++++++++++++-----------
>>>    drivers/spi/spi-mem.c        |   7 +
>>>    include/spi-mem.h            |   3 +
>>>    3 files changed, 451 insertions(+), 206 deletions(-)
>>>
>>
>> Looks like I forgot to bump the version. This should be v3. I can resend if necessary.
> 
> Yes, Please.
> 
> There are few comments,
> 
> 1. Patch "spi: spi-mem: Add debug message for spi-mem ops"
> 
> As we discussed in the previous version, drop the unnecessary debug
> statements after ops execution as this patch trying to add more
> verbose to before ops execution.

 From last time:

>>> +       dev_dbg(slave->dev,
>>> +               "exec %02Xh %u-%u-%u addr=%llx dummy cycles=%u data bytes=%u\n",
>>> +               op->cmd.opcode, op->cmd.buswidth, op->addr.buswidth,
>>> +               op->data.buswidth, op->addr.val,
>>> +               op->dummy.buswidth ? op->dummy.nbytes * 8 / op->dummy.buswidth : 0,
>>> +               op->data.nbytes);
>>
>> This looks unnecessary to me, we have debug prints at the end of the
>> function, which indeed sufficient.
>>
> 
> This is insufficient. First, that information is printed after the op
> is executed. This is too late if one is trying to debug (e.g.) a hang in
> the spi driver. It's also missing opcode, widths, address, and dummy
> bytes. The only duplicated info here is nbytes. Since this is debugging
> information, I think we should print what is most useful for people
> debugging this subsystem.

I would like if you could respond to my response before I remove this.

> 
> 2. Comments in Patch v2,06/10

I will look into that before resending.

--Sean

> 
> Jagan.
>