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

[Jagan Teki]

On Mon, Apr 19, 2021 at 6:49 PM Sean Anderson <seanga2 at gmail.com> wrote:
>
> 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.

Yes, I have commented based on your previous version comments. What
I'm saying here is to adjust even the exiting debug statements in
order to not duplicate if your patch changes.

Jagan.