Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5133 bytes --]

On Fri, May 08, 2020 at 06:42:10PM +0300, Serge Semin wrote:
> On Fri, May 08, 2020 at 11:22:10AM +0100, Mark Brown wrote:

> > Can you be more specific about the issues?  From what you wrote it
> > sounded like the main thing was chip select handling.

> I thought it would be obvious from the patch itself. I've thoroughly described all
> the issues there. Here in cover-letter it's a summary of the main ones.

Bear in mind that the patch is a stand alone thing, there's not a copy
of the existing driver sitting with it and the stylistic changes make
comparisons even less obvious.

> 1) Registers mapping. The DW SSI registers are shifted by 0x100 with
> respect to the MMIO region start. The lowest 0x100 registers are
> responsible for the Baikal-T1 Boot Controller settings. There aren't much
> of them there though. Our code is interested only in a flag, which switches
> an accessibility of the DW APB SSI registers and direct SPI flash mapping.
> And this switchability is a reason of another peculiarity (see the next
> item for details).

That seems fairly easy to address, for example with a subdevice or
indirecting through ops for I/O that could add on an offset (what a
subdevice would end up accomplishing really).

> 2) SPI flash direct mapping. SPI flash direct mapping and DW APB SSI registers
> are mutual exclusive, so only one of them can be enabled at a time. In
> order to use the dirmap we have to switch the RDA bit off in the Boot
> Controller setup register. If DW APB SSI registers need to be accessed the
> RDA bit should be set. For this reason we have to make sure that dirmap
> operations, SPI operations and SPI-mem-ops operations are exclusive, since
> some of them need to interact with the DW APB SSI registers, while another

This exclusivity requirement is pretty standard for these flash memory
map controllers, the framework should ensure you don't get any overlap
between memory mapped and regular interactions.

> the directly mapped SPI flash MMIO (currently ctlr->io_mutex is responsible
> for this).

It only seemed to be referenced in the debugfs code?

> 3) A specific access to MMIO (concerning directly mapped SPI flash MMIO).
> The SoC interconnect is designed in a way so we can't use any instruction to
> read/write from/to the MMIO space. It has to be done by lw/sw with
> dword-aligned address passed. Though in this driver we only use a read
> operation from the directly mapped SPI flash memory.

That's a custom IP block for your systems so that'd be a separate
operation no matter what.

> 4) No direct handling of the CS. Though this is an issue of all DW SSI
> controllers, here with very small FIFO and no DMA/IRQ supported it mandates to
> workaround any preemption/interruption during a non-GPIO-CS-based transfer.
> For the same reason the driver doesn't support normal spi-messages based

As I said when reviewing the driver I think all you need here is support
in the core for linearizing messages into a single transfer and then
what you're left with is what should be a fairly small quirk for running
with interrupts disabled if there's no DMA or interrupts.  I'd expect
both bits of that to benefit some other users too, there's definitely
other controllers that have problems with automated chip select
handling but happen to get away with it a lot of the time.

> interface if no GPIO-CS supplied. In addition since FIFO is too small and most
> of our platforms don't have GPIO-CS support we had to create the SPI-mem-ops
> instead of generic SPI-callback.

As I also said in reviewing the driver that's just not a good idea
anyway, there is no way a driver should be open coding things like that
and there are much better ways to support this.  This is only there
because the driver isn't able to cope with normal messages, it's better
to solve that problem and use the generic flash code than to emulate the
generic flash code here.

In both these cases it looks like the majority of the reason the driver
is different is that you're trying to solve problems in the driver
without changing the core, some things are a lot easier to handle
further up the stack.

> 5) MMIO access race condition. As I described in the in-code comment it's a
> very tricky race happening during concurrent access from different cores to the
> APB bus. Due to this if SPI interface is working high frequency like

This looks like it should be a fairly small quirk?

> I am pretty sure I have forgotten something. Anyway it has been much easier
> to create a new driver instead of integrating all of these into a generic
> one. Integrating something like this in the current DW APB SSI driver would
> mean to have it completely overwritten (refactored if you want) which would
> bring us to a new driver anyway. I don't think it would be good to
> complicate the generic driver with so many peculiar things scattered around
> the code with various hooks or ifdef, especially seeing the current code has
> already become a bit messy.

It really does sound like other than the bits I don't think should be
implemented on a per-driver level these differences are relatively
small.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]