Re: [PATCH v3 00/21] spi: dw: Add full Baikal-T1 SPI Controllers support

2020-10-02 Thread Andy Shevchenko
On Fri, Oct 2, 2020 at 3:56 PM Mark Brown  wrote:
> On Fri, Oct 02, 2020 at 01:24:44PM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 02, 2020 at 01:28:08AM +0300, Serge Semin wrote:
>
> > > the subject. Though some of them are mere cleanups or weakly related with
> > > the subject fixes, but we just couldn't leave the code as is at some
> > > places since we were working with the DW APB SSI driver anyway. Here is
> > > what we did to fix the original DW APB SSI driver, to make it less messy.
>
> > Maybe it's time to put your name into MAINTAINERS for this driver?
>
> Seems sensible to me - Andy, it probably makes sense to add you as well?

I have more than enough on my plate currently. Maybe in the future.

> Does one of you want to send a patch for this?

--
With Best Regards,
Andy Shevchenko


Re: [PATCH v3 00/21] spi: dw: Add full Baikal-T1 SPI Controllers support

2020-10-02 Thread Mark Brown
On Fri, Oct 02, 2020 at 01:24:44PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 02, 2020 at 01:28:08AM +0300, Serge Semin wrote:

> > the subject. Though some of them are mere cleanups or weakly related with
> > the subject fixes, but we just couldn't leave the code as is at some
> > places since we were working with the DW APB SSI driver anyway. Here is
> > what we did to fix the original DW APB SSI driver, to make it less messy.

> Maybe it's time to put your name into MAINTAINERS for this driver?

Seems sensible to me - Andy, it probably makes sense to add you as well?
Does one of you want to send a patch for this?


signature.asc
Description: PGP signature


Re: [PATCH v3 00/21] spi: dw: Add full Baikal-T1 SPI Controllers support

2020-10-02 Thread Andy Shevchenko
On Fri, Oct 02, 2020 at 01:28:08AM +0300, Serge Semin wrote:
> Originally I intended to merge a dedicated Baikal-T1 System Boot SPI
> Controller driver into the kernel and leave the DW APB SSI driver
> untouched. But after a long discussion (see the link at the bottom of the
> letter) Mark and Andy persuaded me to integrate what we developed there
> into the DW APB SSI core driver to be useful for another controllers,
> which may have got the same peculiarities/problems as ours:
> - No IRQ.
> - No DMA.
> - No GPIO CS, so a native CS is utilized.
> - small Tx/Rx FIFO depth.
> - Automatic CS assertion/de-assertion.
> - Slow system bus.
> All of them have been fixed in the framework of this patchset in some
> extent at least for the SPI memory operations. As I expected it wasn't
> that easy and the integration took that many patches as you can see from
> the subject. Though some of them are mere cleanups or weakly related with
> the subject fixes, but we just couldn't leave the code as is at some
> places since we were working with the DW APB SSI driver anyway. Here is
> what we did to fix the original DW APB SSI driver, to make it less messy.

Maybe it's time to put your name into MAINTAINERS for this driver?


-- 
With Best Regards,
Andy Shevchenko




[PATCH v3 00/21] spi: dw: Add full Baikal-T1 SPI Controllers support

2020-10-01 Thread Serge Semin
Originally I intended to merge a dedicated Baikal-T1 System Boot SPI
Controller driver into the kernel and leave the DW APB SSI driver
untouched. But after a long discussion (see the link at the bottom of the
letter) Mark and Andy persuaded me to integrate what we developed there
into the DW APB SSI core driver to be useful for another controllers,
which may have got the same peculiarities/problems as ours:
- No IRQ.
- No DMA.
- No GPIO CS, so a native CS is utilized.
- small Tx/Rx FIFO depth.
- Automatic CS assertion/de-assertion.
- Slow system bus.
All of them have been fixed in the framework of this patchset in some
extent at least for the SPI memory operations. As I expected it wasn't
that easy and the integration took that many patches as you can see from
the subject. Though some of them are mere cleanups or weakly related with
the subject fixes, but we just couldn't leave the code as is at some
places since we were working with the DW APB SSI driver anyway. Here is
what we did to fix the original DW APB SSI driver, to make it less messy.

First two patches are just cleanups to simplify the DW APB SSI device
initialization a bit. We suggest to discard the IRQ threshold macro as
unused and use a ternary operator to initialize the set_cs callback
instead of assigning-and-updating it.

Then we've discovered that the n_bytes field of the driver private data is
used by the DW APB SSI IRQ handler, which requires it to be initialized
before the SMP memory barrier and to be visible from another CPUs. Speaking
about the SMP memory barrier. Having one right after the shared resources
initialization is enough and there is no point in using the spin-lock to
protect the Tx/Rx buffer pointers. The protection functionality is
redundant there by the driver design. (Though I have a doubt whether the
SMP memory barrier is also required there because the normal IO-methods
like readl/writel implies a full memory barrier. So any memory operations
performed before them are supposed to be seen by devices and another CPUs.
See the patch log for details of my concern.)

Thirdly we've found out that there is some confusion in the IRQs
masking/unmasking/clearing in the SPI-transfer procedure. Multiple interrupts
are unmasked on the SPI-transfer initialization, but just TXEI is only
masked back on completion. Similarly IRQ status isn't cleared on the
controller reset, which actually makes the reset being not full and errors
prone in the controller probe procedure.

Another very important optimization is using the IO-relaxed accessors in
the dw_read_io_reg()/dw_write_io_reg() methods. Since the Tx/Rx FIFO data
registers are the most frequently accessible controller resource, using
relaxed accessors there will significantly improve the data read/write
performance. At least on Baikal-T1 SoC such modification opens up a way to
have the DW APB SSI controller working with higher SPI bus speeds, than
without it.

Fifthly we've made an effort to cleanup the code using the SPI-device
private data - chip_data. We suggest to remove the chip type from there
since it isn't used and isn't implemented right anyway. Then instead of
having a bus speed, clock divider, transfer mode preserved there, and
recalculating the CR0 fields of the SPI-device-specific phase, polarity
and frame format each time the SPI transfer is requested, we can save it
in the chip_data instance. By doing so we'll make that structure finally
used as it was supposed to by design (see the spi-fsl-dspi.c, spi-pl022.c,
spi-pxa2xx.c drivers for examples).

Sixthly instead of having the SPI-transfer specific CR0-update callback,
we suggest to implement the DW APB SSI controller capabilities approach.
By doing so we can now inject the vendor-specific peculiarities in
different parts of the DW APB SSI core driver (which is required to
implement both SPI-transfers and the SPI memory operations). This will
also make the code less confusing like defining a callback in the core
driver, setting it up in the glue layer, then calling it from the core
driver again. Seeing the small capabilities implementation embedded
in-situ is more readable than tracking the callbacks assignments. This
will concern the CS-override, Keembay master setup, DW SSI-specific CR0
registers layout capabilities.

Seventhly since there are going to be two types of the transfers
implemented in the DW APB SSI core driver, we need a common method to set
the controller configuration like, Tx/Rx-mode, bus speed, data frame size
and number of data frames to read in case of the memory operations. So we
just detached the corresponding code from the SPI-transfer-one method and
made it to be a part of the new dw_spi_update_config() function, which is
former update_cr0(). Note that the new method will be also useful for the
glue drivers, which due to the hardware design need to create their own
memory operations (for instance, for the dirmap-operations provided in the
Baikal-T System Boot SPI controller driver).

Eighthly