Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 9 Jan 2023 19:25:05 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Sudip Mukherjee <sudip.mukherjee@sifive.com>
Cc:     Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        jude.onyenegecha@sifive.com, ben.dooks@sifive.com,
        jeegar.lakhani@sifive.com, linux-spi@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 00/15] Add support for enhanced SPI for Designware SPI
 controllers
Message-ID: <20230109162505.o3clvmwu3eremlyd@mobilestation>
References: <20221212180732.79167-1-sudip.mukherjee@sifive.com>
 <20221218174523.cke7ubh6nycd247c@mobilestation>
 <20230104222036.h4ke6maxkdvuqtqc@mobilestation>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230104222036.h4ke6maxkdvuqtqc@mobilestation>
Precedence: bulk

Hello Sudip

On Thu, Jan 05, 2023 at 01:20:39AM +0300, Serge Semin wrote:
> Hi Sudip
> 
> On Sun, Dec 18, 2022 at 08:45:26PM +0300, Serge Semin wrote:
> > Hi Sudip
> > 
> > On Mon, Dec 12, 2022 at 06:07:17PM +0000, Sudip Mukherjee wrote:
> > > The is v2 of the patch series adding enhanced SPI support. Some Synopsys SSI
> > > controllers support enhanced SPI which includes Dual mode, Quad mode and
> > > Octal mode. DWC_ssi includes clock stretching feature in enhanced SPI modes
> > > which can be used to prevent FIFO underflow and overflow conditions while
> > > transmitting or receiving the data respectively.
> > > 
> > > This is almost a complete rework based on the review from Serge.
> > 
> > Thank you very much for the series. I'll have a look at it on the next
> > week.
> 
> Just so you know. I haven't forgot about the series. There are some
> problematic parts which I need to give more thinking than I originally
> expected. I'll submit my comments very soon. Sorry for the delay.
> 
> Good news is that I've got the HW-manual for the DW SSI v1.01a
> IP-core. So I'll no longer need to ask of you about that device
> implementation specifics.

Finally I managed to consolidate my thoughts regarding your patchset.
Here is the summary. Some specific comments will be sent in-reply to
the corresponding patches.

First of all there is a crucial difference between eSPI capability
available on DW APB SSI and DW AHB SSI controllers:
DW APB SSI 4.x:
+ Tx until FIFO is empty
+ No clock stretching at all
DW AHB SSI 1.x:
+ Tx until CTRLR1.NDF if clock stretching is available
+ If no clock stretching then Tx until FIFO is empty.
See the DW APB SSI IP-cores don't have the clock stretching feature.
That should be kept in mind while implementing the portable eSPI
support in the DW SSI driver. Your version of the eSPI support is only
applicable for the devices with the eSPI clock-stretching capability
which significantly narrows down its applicability. Anyway you should
convert your code to working only if the clock-stretching feature is
detected.

Moreover your implementation will only work on the platforms with the
native chip-selects. If the GPIO-based CS setup is detected the standard
SPI-messages/transfers-based kernel API will be utilized (see the
spi_mem_exec_op() method implementation) which currently imply the
single-laned SPI bus only. For the same reason the DMA capability won't
work in your eSPI implementation.

If you want that to be fixed then you'll need to update the standard
transfer (+DMA) procedure so one would take the
spi_transfer.{tx_nbits,rx_nbits} fields into account activating the
eSPI modes accordingly. The dw_spi_transfer_handler() method and its
DMA counterpart shall be altered too since the eSPI implies the
Tx-only or Rx-only modes.

Here are several notes applicable to all your patches:
1. Please use the dw_spi_enh* prefix for all eSPI-related methods. That
shall unify the eSPI-code a bit in the same way as it was done in the
DMA-modules (see it has the method defined with the dw_spi_dma_* prefix).

2. Please define the enhanced versions of the MEM-op methods below their
non-enhanced counterparts. Thus we'll have a clearer driver code.

3. It isn't normally welcome to add some code in one patch and fix it in
some following up patch.

4. I am pretty much sure you shouldn't touch the spi_controller data
internals if it isn't platform-specific settings on the controller
probe procedure. Mark won't bless such change. The
spi_controller.xfer_complete field is the internal completion handler
and should be used by the SPI core only.

Regarding the patchset in general. It's ok to provide the eSPI mode
support for the native chip-selects only. But since there are going to be
not a few requests to you to fix I'd suggest to refactor the series in the
next manner:

[PATCH 1] spi: dw: Add Enhanced capabilities flags
Just define the new capability flags
+ DW_SPI_CAP_ENH
+ DW_SPI_CAP_ENH_CLK_STR
Note the capabilities auto-detection will be added later in this patchset.
Yeah, I remember asking you to add the DW_SPI_CAP_EMODE macro, but adding
the _ENH suffix instead seems more readable and would refer to the
eSPI-related methods.

[PATCH 2] spi: dw: Update enhanced frame forward config
In this patch please fix the dw_spi_update_config() method so one would
perform both standard and enhanced config setups:
+ Add new field dw_spi_cfg.enh_frf.
+ Declare new structure struct dw_spi_enh_cfg with the wait_c, addr_l,
inst_l and trans_t fields.
+ Convert the dw_spi_update_config() method to accepting the optional
struct dw_spi_enh_cfg *ecfg pointer.
+ Update the CTRLR1.NDF field for Tx-only transfers if
DW_SPI_CAP_ENH_CLK_STR capability is available.

[PATCH 3] spi: dw: Reduce mem-ops init method indentations
+ This is a prerequisite patch before adding the eSPI-related
MEM-op methods to make the following up patches simpler and more
coherent. It implies updating the dw_spi_init_mem_ops()
function so one would return straight away if the platform-specific CS-methods
or MEM-ops methods are specified. Thus further function updates would be
performed in the more coherent patches with simpler changelog. (See my
comments to your patch "[PATCH v2 04/15] spi: dw: add check for support of enhanced spi".)

[PATCH 4] spi: dw: Add enhanced mem-op verification method
+ Almost the same as your patch "[PATCH v2 04/15] spi: dw: add check for support of enhanced spi"
except the code will have one less indentation due to the patch #3.
Some other issues shall be fixed too. See my comments to your patch for
details. (init if DW_SPI_CAP_ENH_CLK_STR, op->dummy.nbytes / op->dummy.buswidth >= 4)

[PATCH 5] spi: dw: Add enhanced mem-op size adjustment method
+ The same as your patch "[PATCH v2 11/15] spi: dw: adjust size of mem_op"

[PATCH 6] spi: dw: Mask out IRQs if no xfer data available
+ Instead of checking the master->cur_msg pointer to be non-NULL I guess
it would be ok to disable the IRQs if !dws->rx_len && !dws->tx_len in the
dw_spi_irq() method. Although I have doubts that after the commit
a1d5aa6f7f97 ("spi: dw: Disable all IRQs when controller is unused") there
is need in that conditional statement especially seeing the dw_reader()
and dw_writer() methods won't do anything if no data available to
transfer.

[PATCH 7] spi: dw: Move wait-function to the driver core
+ Instead of re-implementing the xfer completion wait function one more
time (as you've done in "[PATCH v2 09/15] spi: dw: use irq handler for
enhanced spi") just move the dw_spi_dma_wait() method from spi-dw-dma.c to
spi-dw-core.c, accordingly fix the prototype and implementation, rename
the dw_spi.dma_completion field to something like xfer_completion and use
the new method in the DW SSI core and DMA modules.

[PATCH 8] spi: dw: spi: dw: Add enhanced mem-op execution method
+ Just add the methods particularly implementing the mem-op execution
process like:
dw_spi_enh_irq_setup(): instead of updating the dw_spi_irq_setup() method
just create a new one which would initialize the IRQ-handler and unmask
IRQs accordingly (depending on the rx_len/rx_len values?).
dw_spi_enh_transfer_handler(): similar to your implementation of the
IRQ-handler except it will use the internal wait-for-completion infrastructure.
dw_spi_enh_write_then_read(): shall write the cmd+addr and activate the
CS line. After that the transfer shall begin.
dw_spi_enh_exec_mem_op(): similar to the dw_spi_exec_mem_op() method. It
shall update the controller config taking the enhanced part into account,
activate the IRQs using the dw_spi_enh_irq_setup() method, call the
dw_spi_enh_write_then_read() function and then wait until the IRQ-based
transfer is completed.

[PATCH 9] spi: dw: Add enhanced mem-op capability auto-detection
+ Just check whether the CTRLR0.SPI_FRF field is writable and values it
accepts. Based on that set the DW_SPI_CAP_ENH capability flag and update
spi_controller.mode_bits field. Similarly check whether the
SPI_CTRLR0.CLK_STRETCH_EN bit is writable and set the
DW_SPI_CAP_ENH_CLK_STR capability flag.
+ Note first you need to try detecting the eSPI capability and only
then the eSPI clock stretching capability.
+ Note the best place for that is the dw_spi_hw_init() method where all
the HW-init and auto-detection is done.

[PATCH 10] spi: dt-bindings: dw-apb-ssi: Add DW AHB SSI compatible string
+ The same as your "[PATCH v2 14/15] spi: dt-bindings: snps,dw-ahb-ssi: Add generic dw-ahb-ssi version"

[PATCH 11] spi: dw: Add DW AHB SSI compatible string
+ The same as your "[PATCH v2 15/15] spi: dw: initialize dwc-ssi controller"

Some implementation-specific comments I'll submit in-reply to the
corresponding patches.

-Serge(y)

> 
> -Serge(y)
> 
> > 
> > -Serge(y)
> > 
> > > 
> > > 
> > > -- 
> > > Regards
> > > Sudip
> > > 
> > > Sudip Mukherjee (15):
> > >   spi: dw: Introduce spi_frf and STD_SPI
> > >   spi: dw: update NDF while using enhanced spi mode
> > >   spi: dw: update SPI_CTRLR0 register
> > >   spi: dw: add check for support of enhanced spi
> > >   spi: dw: Introduce enhanced mem_op
> > >   spi: dw: Introduce dual/quad/octal spi
> > >   spi: dw: send cmd and addr to start the spi transfer
> > >   spi: dw: update irq setup to use multiple handler
> > >   spi: dw: use irq handler for enhanced spi
> > >   spi: dw: Calculate Receive FIFO Threshold Level
> > >   spi: dw: adjust size of mem_op
> > >   spi: dw: Add retry for enhanced spi mode
> > >   spi: dw: detect enhanced spi mode
> > >   spi: dt-bindings: snps,dw-ahb-ssi: Add generic dw-ahb-ssi version
> > >   spi: dw: initialize dwc-ssi controller
> > > 
> > >  .../bindings/spi/snps,dw-apb-ssi.yaml         |   1 +
> > >  drivers/spi/spi-dw-core.c                     | 347 +++++++++++++++++-
> > >  drivers/spi/spi-dw-mmio.c                     |   1 +
> > >  drivers/spi/spi-dw.h                          |  27 ++
> > >  4 files changed, 364 insertions(+), 12 deletions(-)
> > > 
> > > -- 
> > > 2.30.2
> > >