[2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
diff mbox series

Message ID 20200508093621.31619-3-Sergey.Semin@baikalelectronics.ru
State New
Headers show
Series
  • spi: Add Baikal-T1 System Boot SPI Controller driver
Related show

Commit Message

Serge Semin May 8, 2020, 9:36 a.m. UTC
This SPI-controller is a part of the Baikal-T1 System Controller and
is based on the DW APB SSI IP-core, but with very limited resources:
no IRQ, no DMA, only a single native chip-select and just 8 bytes Tx/Rx
FIFO available. In order to provide a transparent initial boot code
execution this controller is also utilized by an vendor-specific block,
which provides an CS0 SPI flash direct mapping interface. Since both
direct mapping and SPI controller normal utilization are mutual exclusive
only a one of these interfaces can be used to access an external SPI
slave device. Taking into account the peculiarities of the controller
registers and physically mapped SPI flash access, very limited resources
and seeing the normal usecase of the controller is to access an external
SPI-nor flash, we decided to create a dedicated SPI driver for it.

The driver provides callbacks for native messages-based SPI interface,
SPI-memory and direct mapping read operations. Due to not having any
asynchronous signaling interface provided by the core we have no choice
but to implement a polling-based data transmission/reception algorithm.
In addition to that in order to bypass the automatic native chip-select
toggle the driver disables the local interrupts during the memory-based
transfers if no complementary GPIO-based chip-select detected in the
platform.

Co-developed-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org
---
 drivers/spi/Kconfig       |  13 +
 drivers/spi/Makefile      |   1 +
 drivers/spi/spi-bt1-sys.c | 873 ++++++++++++++++++++++++++++++++++++++
 drivers/spi/spi-bt1-sys.h | 169 ++++++++
 4 files changed, 1056 insertions(+)
 create mode 100644 drivers/spi/spi-bt1-sys.c
 create mode 100644 drivers/spi/spi-bt1-sys.h

Comments

Andy Shevchenko May 8, 2020, 10:03 a.m. UTC | #1
On Fri, May 8, 2020 at 12:37 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
>
> This SPI-controller is a part of the Baikal-T1 System Controller and
> is based on the DW APB SSI IP-core, but with very limited resources:
> no IRQ, no DMA, only a single native chip-select and just 8 bytes Tx/Rx
> FIFO available. In order to provide a transparent initial boot code
> execution this controller is also utilized by an vendor-specific block,
> which provides an CS0 SPI flash direct mapping interface. Since both
> direct mapping and SPI controller normal utilization are mutual exclusive
> only a one of these interfaces can be used to access an external SPI
> slave device. Taking into account the peculiarities of the controller
> registers and physically mapped SPI flash access, very limited resources
> and seeing the normal usecase of the controller is to access an external
> SPI-nor flash, we decided to create a dedicated SPI driver for it.

It seems a lot of code.
Why can't you use spi-dw-mmio.c et al.?

> The driver provides callbacks for native messages-based SPI interface,
> SPI-memory and direct mapping read operations. Due to not having any
> asynchronous signaling interface provided by the core we have no choice
> but to implement a polling-based data transmission/reception algorithm.
> In addition to that in order to bypass the automatic native chip-select
> toggle the driver disables the local interrupts during the memory-based
> transfers if no complementary GPIO-based chip-select detected in the
> platform.
Serge Semin May 8, 2020, 10:15 a.m. UTC | #2
On Fri, May 08, 2020 at 01:03:11PM +0300, Andy Shevchenko wrote:
> On Fri, May 8, 2020 at 12:37 PM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
> >
> > This SPI-controller is a part of the Baikal-T1 System Controller and
> > is based on the DW APB SSI IP-core, but with very limited resources:
> > no IRQ, no DMA, only a single native chip-select and just 8 bytes Tx/Rx
> > FIFO available. In order to provide a transparent initial boot code
> > execution this controller is also utilized by an vendor-specific block,
> > which provides an CS0 SPI flash direct mapping interface. Since both
> > direct mapping and SPI controller normal utilization are mutual exclusive
> > only a one of these interfaces can be used to access an external SPI
> > slave device. Taking into account the peculiarities of the controller
> > registers and physically mapped SPI flash access, very limited resources
> > and seeing the normal usecase of the controller is to access an external
> > SPI-nor flash, we decided to create a dedicated SPI driver for it.
> 
> It seems a lot of code.
> Why can't you use spi-dw-mmio.c et al.?

I said above why. Even though the registers set is similar It's too specific
to be integrated into the generic DW SSI driver.

-Sergey

> 
> > The driver provides callbacks for native messages-based SPI interface,
> > SPI-memory and direct mapping read operations. Due to not having any
> > asynchronous signaling interface provided by the core we have no choice
> > but to implement a polling-based data transmission/reception algorithm.
> > In addition to that in order to bypass the automatic native chip-select
> > toggle the driver disables the local interrupts during the memory-based
> > transfers if no complementary GPIO-based chip-select detected in the
> > platform.
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
Mark Brown May 8, 2020, 10:22 a.m. UTC | #3
On Fri, May 08, 2020 at 01:15:41PM +0300, Serge Semin wrote:
> On Fri, May 08, 2020 at 01:03:11PM +0300, Andy Shevchenko wrote:

> > > slave device. Taking into account the peculiarities of the controller
> > > registers and physically mapped SPI flash access, very limited resources
> > > and seeing the normal usecase of the controller is to access an external
> > > SPI-nor flash, we decided to create a dedicated SPI driver for it.

> > It seems a lot of code.
> > Why can't you use spi-dw-mmio.c et al.?

> I said above why. Even though the registers set is similar It's too specific
> to be integrated into the generic DW SSI driver.

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

> > > The driver provides callbacks for native messages-based SPI interface,
> > > SPI-memory and direct mapping read operations. Due to not having any
> > > asynchronous signaling interface provided by the core we have no choice

What do you mean by "asynchronous signaling interface provided by the
core" here?
Andy Shevchenko May 8, 2020, 10:26 a.m. UTC | #4
On Fri, May 8, 2020 at 1:15 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
>
> On Fri, May 08, 2020 at 01:03:11PM +0300, Andy Shevchenko wrote:
> > On Fri, May 8, 2020 at 12:37 PM Serge Semin
> > <Sergey.Semin@baikalelectronics.ru> wrote:
> > >
> > > This SPI-controller is a part of the Baikal-T1 System Controller and
> > > is based on the DW APB SSI IP-core, but with very limited resources:
> > > no IRQ, no DMA, only a single native chip-select and just 8 bytes Tx/Rx
> > > FIFO available. In order to provide a transparent initial boot code
> > > execution this controller is also utilized by an vendor-specific block,
> > > which provides an CS0 SPI flash direct mapping interface. Since both
> > > direct mapping and SPI controller normal utilization are mutual exclusive
> > > only a one of these interfaces can be used to access an external SPI
> > > slave device. Taking into account the peculiarities of the controller
> > > registers and physically mapped SPI flash access, very limited resources
> > > and seeing the normal usecase of the controller is to access an external
> > > SPI-nor flash, we decided to create a dedicated SPI driver for it.
> >
> > It seems a lot of code.
> > Why can't you use spi-dw-mmio.c et al.?
>
> I said above why. Even though the registers set is similar It's too specific
> to be integrated into the generic DW SSI driver.

At least you may do at the beginning is to reuse header spi-dw.h and
put your stuff under
spi-dw-baikal.c or so. Then, look at the spi-dw.c and check what can
be reused (I think a lot).
Mark Brown May 8, 2020, 11:37 a.m. UTC | #5
On Fri, May 08, 2020 at 12:36:21PM +0300, Serge Semin wrote:

Your CC list on this series is *very* wide - are you sure that this is
relevant to everyone you're sending things to?  Kernel developers often
get a lot of mail meaning that extra mail can become a bit overwhelming
and cause things to get lost in the noise.

> This SPI-controller is a part of the Baikal-T1 System Controller and
> is based on the DW APB SSI IP-core, but with very limited resources:
> no IRQ, no DMA, only a single native chip-select and just 8 bytes Tx/Rx
> FIFO available. In order to provide a transparent initial boot code
> execution this controller is also utilized by an vendor-specific block,
> which provides an CS0 SPI flash direct mapping interface. Since both
> direct mapping and SPI controller normal utilization are mutual exclusive
> only a one of these interfaces can be used to access an external SPI
> slave device. Taking into account the peculiarities of the controller
> registers and physically mapped SPI flash access, very limited resources
> and seeing the normal usecase of the controller is to access an external
> SPI-nor flash, we decided to create a dedicated SPI driver for it.

My overall impression reading this is that there could be a lot more
reliance on both generic functionality and as Andy suggested the spi-dw
driver - the headers seem easy for example.  As far as I can tell the
main things this is adding compared to spi-dw are the dirmap code and
the IRQ disabling around the PIO on the FIFO both of which seem like
relatively small additions which it should be possible to accomodate
within spi-dw.  For example the driver could have multiple transfer
functions and pick a different one with the interrupt disabling when
running on this hardware.

> --- /dev/null
> +++ b/drivers/spi/spi-bt1-sys.c
> @@ -0,0 +1,873 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC

Please make the entire comment a C++ one so things look a bit cleaner
and more intentional.

> +	writel(BIT(req->cs), bs->regs + BC_SPI_SER);
> +	if (req->cs_gpiod) {
> +		gpiod_set_value_cansleep(req->cs_gpiod,
> +					 !!(bs->cfg.mode & SPI_CS_HIGH));

If you have a GPIO chip select you should just let the core manage it
through cs_gpiod rather than open coding.

> +	} else if (!req->dirmap) {
> +		local_irq_save(bs->cfg.flags);
> +		preempt_disable();
> +	}

This is obviously not great, especially for larger transfers.  It would
be a lot easier to review and manage this if the IRQ disabling was done
around the transfers in a single function rather than separately, that
way everything is next to each other and it's clearer that we're doing
this for the minimum time and didn't miss anything.  Right now it's hard
to tell if everything is joined up.

> +static void bs_update_cfg(struct bt1_spi *bs, struct spi_transfer *xfer)
> +{

This has exactly one caller, perhaps it could just be inlined there?  I
think this applies to some of the other internal functions.

> +static int bs_check_status(struct bt1_spi *bs)
> +{

It's not obvious from the name that this function will busy wait rather
than just reading some status registers.

> +	/*
> +	 * Spin around the BUSY-state bit waiting for the controller to finish
> +	 * all the requested IO-operations.
> +	 */
> +	do {
> +		data = readl(bs->regs + BC_SPI_SR);
> +	} while ((data & BC_SPI_SR_BUSY) || !(data & BC_SPI_SR_TFE));

We could use a timeout or something here in case something goes wrong,
as things stand we could end up spinning for ever.

It's also not clear to me why we only check for over/underrun before we
do the busy wait, especially a RX overrun could happen and cause us to
loose data while things are finishing up.

> +	while (txlen || rxlen) {
> +		cnt = BC_SPI_FIFO_LVL - __raw_readl(bs->regs + BC_SPI_TXFLR);
> +		cnt = min(cnt, txlen);
> +		txlen -= cnt;
> +		while (cnt--) {
> +			data = src ? *src++ : 0xFF;
> +			__raw_writel(data, bs->regs + BC_SPI_DR);
> +		}

It's more typical to write zeros when there's no data.

> +static int bs_exec_mem_op(struct spi_mem *mem,
> +			  const struct spi_mem_op *op)
> +{

It's not clear to me that this hardware actually supports spi_mem in
hardware?  

> +	if (!bs->cfg.cs_gpiod) {
> +		bs_push_bytes(bs, cmd, len);
> +
> +		if (op->data.dir == SPI_MEM_DATA_IN)
> +			ret = bs_pull_bytes(bs, op->data.buf.in,
> +					    op->data.nbytes);
> +	} else {
> +		bs_pp_bytes(bs, cmd, NULL, len);
> +
> +		if (op->data.dir == SPI_MEM_DATA_IN)
> +			bs_pp_bytes(bs, NULL, op->data.buf.in,
> +				    op->data.nbytes);
> +	}

The actual transfers here are done using the same PIO functions as
everything else as far as I can see which makes me wonder what this
spi_mem implementation is adding over the standard SPI flash support.
I've not checked but if we need this to get the dirmap support to kick
in then we should fix this by making the core not have that requirement.

> +static void bs_copy_from_map(void *to, void __iomem *from, size_t len)
> +{
> +	size_t shift, chunk;
> +	u32 data;

This feels like something that is likely to be suitable for a generic
implementation?  Indeed I'd kind of expect that the architecture
memcpy_to/fromio() would try to copy aligned blocks since it's usually
going to perform better (which I assume is why this function is doing
it).

> +	if (shift) {
> +		chunk = min_t(size_t, 4 - shift, len);
> +		data = readl_relaxed(from - shift);
> +		memcpy(to, &data + shift, chunk);

It's a little unclear what we're actually doing though - we read a data
address from the hardware?

> +static int bs_prepare_message(struct spi_controller *ctrl,
> +			      struct spi_message *msg)
> +{
> +	struct bt1_spi *bs = spi_controller_get_devdata(ctrl);
> +	struct spi_transfer *xfer;
> +	struct bt1_spi_cfg req;
> +
> +	/*
> +	 * Currently the driver doesn't support the generic messages-based
> +	 * SPI interface with pure native chip-select signal. This is due
> +	 * to the automatic native chip-select toggle peculiarity described
> +	 * in the comment of the bs_set_cfg() method. Alas we can't use the
> +	 * IRQ-disable-based boost approach here since the native queue-based
> +	 * SPI messages transfer method can sleep waiting for the
> +	 * transfers/CS-change delays.
> +	 */
> +	if (!msg->spi->cs_gpiod)
> +		return -ENOTSUPP;

This seems a bit much - we can validate that the message doesn't contain
any delays (which is going to be the overwhelming majority of devices)
or multiple transfers and reject just those transfers we can't support.
Multiple transfers are an issue with this hardware but you could
implement support in the core for coalescing them into single transfers,
this isn't the only hardware with automatic chip select handling that
can't cope with scatter/gather so it would be a useful thing to have.

> +	/*
> +	 * Collect the controller configuration common for all transfers and
> +	 * specific to the very first one.
> +	 */
> +	xfer = list_first_entry(&msg->transfers, typeof(*xfer), transfer_list);
> +	req.dirmap = false;
> +	req.cs = msg->spi->chip_select;
> +	req.cs_gpiod = msg->spi->cs_gpiod;
> +	req.flags = 0;
> +	req.mode = msg->spi->mode;
> +	req.tmode = BC_SPI_CTRL0_TMOD_TR;
> +	req.ndf = 0;
> +	req.dfs = xfer->bits_per_word;
> +	req.freq = xfer->speed_hz;
> +
> +	bs_set_cfg(bs, &req);

It's not clear to me what the benefit is in this indirection through
req?

> +static int bs_transfer_one(struct spi_controller *ctrl, struct spi_device *spi,
> +			   struct spi_transfer *xfer)
> +{
> +	struct bt1_spi *bs = spi_controller_get_devdata(ctrl);
> +
> +	bs_update_cfg(bs, xfer);
> +
> +	if (bs->cfg.dfs <= 8)
> +		bs_pp_bytes(bs, xfer->tx_buf, xfer->rx_buf, xfer->len);
> +	else
> +		bs_pp_words(bs, xfer->tx_buf, xfer->rx_buf, xfer->len / 2);

This will have issues with transfers with an odd number of bytes won't
it?

> +static struct bt1_spi *bs_create_data(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct bt1_spi *bs;

As with some of the other functions this has exactly one caller and
barely any content in it, just inline it.

> +	bs->dev = dev;
> +	bs->pdev = pdev;

Do you really need to separately save these (and is there a context
where you have the driver data but not the device anyway)?

> +#ifdef CONFIG_DEBUG_FS
> +
> +#define BC_SPI_DBGFS_REG(_name, _off)	\
> +{					\
> +	.name = _name,			\
> +	.offset = _off			\
> +}

Perhaps consider regmap_mmio?

> +static int bs_dbgfs_open_regs(struct inode *inode, struct file *file)
> +{
> +	struct bt1_spi *bs = inode->i_private;
> +	int ret;
> +
> +	ret = single_open(file, bs_dbgfs_show_regs, bs);
> +	if (!ret) {
> +		mutex_lock(&bs->ctrl->io_mutex);

Holding a mutex over the entire time that a file is open is not good,
it's also not clear to me what this mutex is supposed to be protecting
given that it's only referenced in these debugfs functions.

> +		writel(BC_CSR_SPI_RDA, bs->regs + BC_CSR);

Whatever this write is doing we never seem to undo it?
Serge Semin May 8, 2020, 3:42 p.m. UTC | #6
On Fri, May 08, 2020 at 11:22:10AM +0100, Mark Brown wrote:
> On Fri, May 08, 2020 at 01:15:41PM +0300, Serge Semin wrote:
> > On Fri, May 08, 2020 at 01:03:11PM +0300, Andy Shevchenko wrote:
> 
> > > > slave device. Taking into account the peculiarities of the controller
> > > > registers and physically mapped SPI flash access, very limited resources
> > > > and seeing the normal usecase of the controller is to access an external
> > > > SPI-nor flash, we decided to create a dedicated SPI driver for it.
> 
> > > It seems a lot of code.
> > > Why can't you use spi-dw-mmio.c et al.?
> 
> > I said above why. Even though the registers set is similar It's too specific
> > to be integrated into the generic DW SSI driver.
> 
> 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.
Anyway here are all collected at once:

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).

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
the directly mapped SPI flash MMIO (currently ctlr->io_mutex is responsible
for this).

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.

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
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.

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
12.5 - 25 MHz and there is some another code working with APB bus on another
core, the SPI data pushing function might not keep up with filling small FIFO
(8 bytes) on time, since the APB bus has got just 50 MHz frequency. Due to
this I had artificially limit the SPI bus frequency if there is more than one
CPU in the system.

6) A single CS. It's normally connected to an external SPI flash so the
driver is equipped with mem-ops and dirmap out of the box. BTW normal
SPI-operations are in fact unneeded on all the platforms currently
equipped with Baikal-T1 because all of them have got SPI flash attached to
this interface, and most likely it will be like in new platforms too.

7) No interrupts. Yes, this is another peculiarity. Since this DW APB core
is a part of the boot controller it just don't need IRQs. Boot controller
is responsible for the code loading from SPI flash. It has got a dedicated
RTL which provide a transparent access to the flash just by reading from a
corresponding MMIO range (see the SPI flash direct mapping described above).

8) No DMA. Yeah, and DMA isn't supported for the same reason.

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.

> 
> > > > The driver provides callbacks for native messages-based SPI interface,
> > > > SPI-memory and direct mapping read operations. Due to not having any
> > > > asynchronous signaling interface provided by the core we have no choice
> 
> What do you mean by "asynchronous signaling interface provided by the
> core" here?

By asynchronous signalling I meant DMA and IRQ. Non of them provided by the
controller.

-Sergey
Serge Semin May 8, 2020, 4:24 p.m. UTC | #7
On Fri, May 08, 2020 at 01:26:52PM +0300, Andy Shevchenko wrote:
> On Fri, May 8, 2020 at 1:15 PM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
> >
> > On Fri, May 08, 2020 at 01:03:11PM +0300, Andy Shevchenko wrote:
> > > On Fri, May 8, 2020 at 12:37 PM Serge Semin
> > > <Sergey.Semin@baikalelectronics.ru> wrote:
> > > >
> > > > This SPI-controller is a part of the Baikal-T1 System Controller and
> > > > is based on the DW APB SSI IP-core, but with very limited resources:
> > > > no IRQ, no DMA, only a single native chip-select and just 8 bytes Tx/Rx
> > > > FIFO available. In order to provide a transparent initial boot code
> > > > execution this controller is also utilized by an vendor-specific block,
> > > > which provides an CS0 SPI flash direct mapping interface. Since both
> > > > direct mapping and SPI controller normal utilization are mutual exclusive
> > > > only a one of these interfaces can be used to access an external SPI
> > > > slave device. Taking into account the peculiarities of the controller
> > > > registers and physically mapped SPI flash access, very limited resources
> > > > and seeing the normal usecase of the controller is to access an external
> > > > SPI-nor flash, we decided to create a dedicated SPI driver for it.
> > >
> > > It seems a lot of code.
> > > Why can't you use spi-dw-mmio.c et al.?
> >
> > I said above why. Even though the registers set is similar It's too specific
> > to be integrated into the generic DW SSI driver.
> 
> At least you may do at the beginning is to reuse header spi-dw.h and
> put your stuff under
> spi-dw-baikal.c or so. Then, look at the spi-dw.c and check what can
> be reused (I think a lot).

Nah, It's not a lot, barely a few things. What is useful is the registers map
(though it has to be shifted and most of the registers are unused), IO operations
(two small read-write methods with questionable design) and possibly a few lines
of config setting procedure.

You think I didn't had in mind to integrate this controller support into
the generic driver or resuse something from there? If it was worth a try I would
have done it.

-Sergey

> 
> -- 
> With Best Regards,
> Andy Shevchenko
Mark Brown May 8, 2020, 5:07 p.m. UTC | #8
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.
Serge Semin May 10, 2020, 12:20 a.m. UTC | #9
On Fri, May 08, 2020 at 12:37:51PM +0100, Mark Brown wrote:
> On Fri, May 08, 2020 at 12:36:21PM +0300, Serge Semin wrote:
> 
> Your CC list on this series is *very* wide - are you sure that this is
> relevant to everyone you're sending things to?  Kernel developers often
> get a lot of mail meaning that extra mail can become a bit overwhelming
> and cause things to get lost in the noise.

MIPS folks are here since Baikal-T1 is a MIPS-based chip and this patchset
is a part of the campaign of integrating the SoC support into the kernel.
With Lee and Miquel we discussed the dirmap support in the framework of another
patchset. Rob and devicetree-list are CC'ed due to having the bindings tree
updated. Then a series of folks who recently submitted the biggest updates into
the spi subsystems so might have valuable comments for this driver as well. So
yes, I am sure.

> 
> > This SPI-controller is a part of the Baikal-T1 System Controller and
> > is based on the DW APB SSI IP-core, but with very limited resources:
> > no IRQ, no DMA, only a single native chip-select and just 8 bytes Tx/Rx
> > FIFO available. In order to provide a transparent initial boot code
> > execution this controller is also utilized by an vendor-specific block,
> > which provides an CS0 SPI flash direct mapping interface. Since both
> > direct mapping and SPI controller normal utilization are mutual exclusive
> > only a one of these interfaces can be used to access an external SPI
> > slave device. Taking into account the peculiarities of the controller
> > registers and physically mapped SPI flash access, very limited resources
> > and seeing the normal usecase of the controller is to access an external
> > SPI-nor flash, we decided to create a dedicated SPI driver for it.
> 
> My overall impression reading this is that there could be a lot more
> reliance on both generic functionality and as Andy suggested the spi-dw
> driver - the headers seem easy for example.  As far as I can tell the
> main things this is adding compared to spi-dw are the dirmap code and
> the IRQ disabling around the PIO on the FIFO both of which seem like
> relatively small additions which it should be possible to accomodate
> within spi-dw.  For example the driver could have multiple transfer
> functions and pick a different one with the interrupt disabling when
> running on this hardware.

dirmap and IRQs disabling isn't all they have different. Please see my answer to
you in the cover-letter thread and the comments below in this email.

> 
> > --- /dev/null
> > +++ b/drivers/spi/spi-bt1-sys.c
> > @@ -0,0 +1,873 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
> 
> Please make the entire comment a C++ one so things look a bit cleaner
> and more intentional.

This has been unexpected. It's first time I see such a requirement.
Anyway ok. I'll fix it. Is it ok to have the C-style comments in the header
file?
* It isn't included by any assembly so from this point of view C++ style
* shall also work there.

> 
> > +	writel(BIT(req->cs), bs->regs + BC_SPI_SER);
> > +	if (req->cs_gpiod) {
> > +		gpiod_set_value_cansleep(req->cs_gpiod,
> > +					 !!(bs->cfg.mode & SPI_CS_HIGH));
> 
> If you have a GPIO chip select you should just let the core manage it
> through cs_gpiod rather than open coding.

Of course I know this, and normally I would have omitted the GPIO manual
assertion (hopefully soon my hands get to merging the AX99100 driver I've
developed some time ago). The thing is that this Baikal-T1 System SSI device
driver has been initially written before commit 05766050d5bd ("spi: spi-mem:
fallback to using transfers when CS gpios are used"). So asserting GPIO CS had
been required to initiate the SPI memory communications seeing the generic
spi_mem_exec_op() doesn't do this. Manual GPIO manipulation is indeed redundant
for the current SPI-mem op execution procedure.

Secondly the message of that commit states "Devices with chip selects driven
via GPIO are not compatible with the spi-mem operations." I find this statement
questionable, because for instance this device supports memory operations with
GPIO-driven CS. Though in current implementation the driver fallback to using normal
push-pull IO mode if GPIO CS is utilized as safer one. But even in this case
it's better than splitting the memory operations up into the transfers, which is
developed in the spi_mem_exec_op() method.

So in this matter my question is: how to modify the SPI-mem interface so the
SPI-memory operations would also work with GPIO driven CS? Some additional flag
might work...

Thirdly what about dirmap operations? If we got a GPIO driven CS then due to
lacking any CS manipulation in spi_mem_dirmap_read() method we wouldn't have
been able to make the direct mapping working without manual setting the GPIO.
So the same question here. How to work this around and justify your requirement?
Until the question is answered and we come up with reasonable solution in order
to have the SPI-mem/dirmap interface working together with GPIO CS support I have
to leave the manual GPIO manipulation.

> 
> > +	} else if (!req->dirmap) {
> > +		local_irq_save(bs->cfg.flags);
> > +		preempt_disable();
> > +	}
> 

> This is obviously not great, especially for larger transfers.

Don't tell me about this. But this is the only way to make the controller
working with all the weird stuff it has got.

> It would
> be a lot easier to review and manage this if the IRQ disabling was done
> around the transfers in a single function rather than separately, that
> way everything is next to each other and it's clearer that we're doing
> this for the minimum time and didn't miss anything.  Right now it's hard
> to tell if everything is joined up.

Hm, this is questionable. If it wasn't joined up I wouldn't have the driver
working. Anyway I see your point and from a different angle of view this might be
better to move this operations to the bs_exec_mem_op() method. But if you insist
on implementing the full SPI-message interface support even for native CS, I'd
leave it here to avoid the code duplication, since the IRQs disabling workaround
should be also used there.

> 
> > +static void bs_update_cfg(struct bt1_spi *bs, struct spi_transfer *xfer)
> > +{
> 
> This has exactly one caller, perhaps it could just be inlined there?  I
> think this applies to some of the other internal functions.

I don't want to inline methods just because they are used in a single place. This
might worsen readability and in this case it will. Currently the transfer_one()
method is consistent with self-explanatory methods: update config, push-pull
bytes/words, check status. This also applies to the rest of the code. I can
consider some improvements/optimizations in this matter though, but embedding
the functions isn't one of them. Moreover the compiler does the static
methods inlining automatically as soon as it sees they are called just once.

> 
> > +static int bs_check_status(struct bt1_spi *bs)
> > +{
> 
> It's not obvious from the name that this function will busy wait rather
> than just reading some status registers.

What do you suggest then? Renaming? Splitting up? If renaming, then what name do
you prefer? Something like bs_check_completion()?

> 
> > +	/*
> > +	 * Spin around the BUSY-state bit waiting for the controller to finish
> > +	 * all the requested IO-operations.
> > +	 */
> > +	do {
> > +		data = readl(bs->regs + BC_SPI_SR);
> > +	} while ((data & BC_SPI_SR_BUSY) || !(data & BC_SPI_SR_TFE));
> 
> We could use a timeout or something here in case something goes wrong,
> as things stand we could end up spinning for ever.

Ok. readl_poll_timeout_atomic() shall do the trick.

> 
> It's also not clear to me why we only check for over/underrun before we
> do the busy wait, especially a RX overrun could happen and cause us to
> loose data while things are finishing up.

In fact this busy-wait loop is relevant for the Tx-only operations. So here we
are waiting for the controller to push the rest of the data from FIFO to the
bus. This is necessary so the next IO operation wouldn't clear the FIFO up by
disabling the controller at the bs_set_cfg() method. The busy-wait loop shall
not cause any overruns (because Rx is turned off). If Tx/Rx overrun happened
it must have happened before and only If either the IO algorithm couldn't keep
up with data being pushed out/pulled in to/from the FIFOs (SPI bus is too fast
while MMIO operations are too slow or for some other case) or I messed
something up with the IO algorithm implementation.

Anyway in order to prevent such question from future code readers I'll move the
busy-wait loop in the beginning of the function.

> 
> > +	while (txlen || rxlen) {
> > +		cnt = BC_SPI_FIFO_LVL - __raw_readl(bs->regs + BC_SPI_TXFLR);
> > +		cnt = min(cnt, txlen);
> > +		txlen -= cnt;
> > +		while (cnt--) {
> > +			data = src ? *src++ : 0xFF;
> > +			__raw_writel(data, bs->regs + BC_SPI_DR);
> > +		}
> 
> It's more typical to write zeros when there's no data.

Ok.

> 
> > +static int bs_exec_mem_op(struct spi_mem *mem,
> > +			  const struct spi_mem_op *op)
> > +{
> 
> It's not clear to me that this hardware actually supports spi_mem in
> hardware?

SPI-mem operations are implemented by means of the EEPROM-read and Tx-only
modes of the controller.
 
> 
> > +	if (!bs->cfg.cs_gpiod) {
> > +		bs_push_bytes(bs, cmd, len);
> > +
> > +		if (op->data.dir == SPI_MEM_DATA_IN)
> > +			ret = bs_pull_bytes(bs, op->data.buf.in,
> > +					    op->data.nbytes);
> > +	} else {
> > +		bs_pp_bytes(bs, cmd, NULL, len);
> > +
> > +		if (op->data.dir == SPI_MEM_DATA_IN)
> > +			bs_pp_bytes(bs, NULL, op->data.buf.in,
> > +				    op->data.nbytes);
> > +	}
> 

> The actual transfers here are done using the same PIO functions as
> everything else as far as I can see which makes me wonder what this
> spi_mem implementation is adding over the standard SPI flash support.

No, if no GPIO CS is supplied the memory read operation is performed by means of
the EEPROM-read mode of the DW SSI controller (Write command is written to the
Tx FIFO and read data length - to the CTRLR1 register). The memory write
operation utilizes the Tx-only mode (just push data to the Tx FIFO, controller
discards all incoming data). See bs_set_cfg() for details.

If GPIO CS is utilized then we fallback to the normal push-pull methods because
first it's safer this way, second it's still better than handling multiple SPI
transfers.

BTW The EEPROM/Tx-only modes are required to be utilized for the controller on
our SoC in order to support the SPI bus operations with highest frequencies.
Due to relatively slow APB bus performance we have to minimize MMIO operation
per a single byte read/write. This is another reason why we had to create a
dedicated driver for this controller.

> I've not checked but if we need this to get the dirmap support to kick
> in then we should fix this by making the core not have that requirement.

No, dirmap is implemented by means of dedicated sub-block of the boot
controller, which does all of the IO by itself (though the controller config
still has to be accordingly setup including CS or GPIO CS). We just read a
ready SPI-flash data from a dedicated MMIO space.

> 
> > +static void bs_copy_from_map(void *to, void __iomem *from, size_t len)
> > +{
> > +	size_t shift, chunk;
> > +	u32 data;
> 
> This feels like something that is likely to be suitable for a generic
> implementation?  Indeed I'd kind of expect that the architecture
> memcpy_to/fromio() would try to copy aligned blocks since it's usually
> going to perform better (which I assume is why this function is doing
> it).

MIPS memcpy_to/fromio don't do this. They expect the MMIO space can be read in
a same way as normal memory space. It's our controller "feature", that MMIO can
be only accessible by lw/sw instructions. So I had no choice but to implement the
aligned read/write operations by myself here. AFAICS this won't be suitable for
the generic implementation due to the alignment restriction and the controller
setups, which are supposed to be performed before accessing the dirmap MMIO.

> 
> > +	if (shift) {
> > +		chunk = min_t(size_t, 4 - shift, len);
> > +		data = readl_relaxed(from - shift);
> > +		memcpy(to, &data + shift, chunk);
> 
> It's a little unclear what we're actually doing though - we read a data
> address from the hardware?

As I've written in the comment:
 * We split the copying up into the next three stages: unaligned head,
 * aligned body, unaligned tail.
here we do the unaligned head read operation. I read data from an aligned
address, then copy the requested part to the output buffer.

> 
> > +static int bs_prepare_message(struct spi_controller *ctrl,
> > +			      struct spi_message *msg)
> > +{
> > +	struct bt1_spi *bs = spi_controller_get_devdata(ctrl);
> > +	struct spi_transfer *xfer;
> > +	struct bt1_spi_cfg req;
> > +
> > +	/*
> > +	 * Currently the driver doesn't support the generic messages-based
> > +	 * SPI interface with pure native chip-select signal. This is due
> > +	 * to the automatic native chip-select toggle peculiarity described
> > +	 * in the comment of the bs_set_cfg() method. Alas we can't use the
> > +	 * IRQ-disable-based boost approach here since the native queue-based
> > +	 * SPI messages transfer method can sleep waiting for the
> > +	 * transfers/CS-change delays.
> > +	 */
> > +	if (!msg->spi->cs_gpiod)
> > +		return -ENOTSUPP;
> 
> This seems a bit much - we can validate that the message doesn't contain
> any delays (which is going to be the overwhelming majority of devices)
> or multiple transfers and reject just those transfers we can't support.
> Multiple transfers are an issue with this hardware but you could
> implement support in the core for coalescing them into single transfers,
> this isn't the only hardware with automatic chip select handling that
> can't cope with scatter/gather so it would be a useful thing to have.

As I said in the commit message to the binding this controller is nearly always
connected to an external SPI flash (I'd say always, because Baikal-T1 is fully
functional only when initially booted from an SPI flash). At least I don't know
any hardware using this interface differently (I've worked with all devices with
Baikal-T1 SoC on board). In current driver implementation the SPI-mem operations
covers the native CS case while the prepare_message()/transfer_one()/unprepare_message()
are used when GPIO-driven CS is available (due to the commit 05766050d5bd I had
to implement it). So I didn't want to over-complicate the driver just to support
some virtual hardware before it's really necessary.

> 
> > +	/*
> > +	 * Collect the controller configuration common for all transfers and
> > +	 * specific to the very first one.
> > +	 */
> > +	xfer = list_first_entry(&msg->transfers, typeof(*xfer), transfer_list);
> > +	req.dirmap = false;
> > +	req.cs = msg->spi->chip_select;
> > +	req.cs_gpiod = msg->spi->cs_gpiod;
> > +	req.flags = 0;
> > +	req.mode = msg->spi->mode;
> > +	req.tmode = BC_SPI_CTRL0_TMOD_TR;
> > +	req.ndf = 0;
> > +	req.dfs = xfer->bits_per_word;
> > +	req.freq = xfer->speed_hz;
> > +
> > +	bs_set_cfg(bs, &req);
> 
> It's not clear to me what the benefit is in this indirection through
> req?

Config request is useful to collect the controller configs in a single
structure from different objects like normal SPI transfers, SPI-mem
operations or dirmaps. The config structure is then uniformly translated
into the controller setting by the bs_set_cfg() method. 

> 
> > +static int bs_transfer_one(struct spi_controller *ctrl, struct spi_device *spi,
> > +			   struct spi_transfer *xfer)
> > +{
> > +	struct bt1_spi *bs = spi_controller_get_devdata(ctrl);
> > +
> > +	bs_update_cfg(bs, xfer);
> > +
> > +	if (bs->cfg.dfs <= 8)
> > +		bs_pp_bytes(bs, xfer->tx_buf, xfer->rx_buf, xfer->len);
> > +	else
> > +		bs_pp_words(bs, xfer->tx_buf, xfer->rx_buf, xfer->len / 2);
> 
> This will have issues with transfers with an odd number of bytes won't
> it?

No, it won't. Transfers with bits per word greater than 8 should have even
number of bytes seeing the len field is supposed to have the buffers length in
bytes:

 * In-memory data values are always in native CPU byte order, translated
 * from the wire byte order (big-endian except with SPI_LSB_FIRST).  So
 * for example when bits_per_word is sixteen, buffers are 2N bytes long
 * (@len = 2N) and hold N sixteen bit words in CPU byte order.
 *
 * When the word size of the SPI transfer is not a power-of-two multiple
 * of eight bits, those in-memory words include extra bits.  In-memory
 * words are always seen by protocol drivers as right-justified, so the
 * undefined (rx) or unused (tx) bits are always the most significant bits.

The controller expects exactly the same interpretation of right-justified data
when its length isn't not power of 2 including the extra bits.

> 
> > +static struct bt1_spi *bs_create_data(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct bt1_spi *bs;
> 
> As with some of the other functions this has exactly one caller and
> barely any content in it, just inline it.

As I said before. I don't see this being necessary.

> 
> > +	bs->dev = dev;
> > +	bs->pdev = pdev;
> 
> Do you really need to separately save these (and is there a context
> where you have the driver data but not the device anyway)?

Even if I don't, having a device pointer around in the private data is a
matter of convenience. It's either referencing the controller/mem pointers
cascade to get it, which may look bulky like &mem->controller->dev and having
an additional argument passed to the sub-functions and in some cases allocating
pointers on the stack; or to prevent all of these complications I just save the
device structure pointer in the private data, so to use it when I need to print
an error or allocate dev-res, etc.

Regarding the platform_device pointer. Well I've defined it in the private data
for the same reason. Though it's used much less than generic device pointer, so
I can discard it.

> 
> > +#ifdef CONFIG_DEBUG_FS
> > +
> > +#define BC_SPI_DBGFS_REG(_name, _off)	\
> > +{					\
> > +	.name = _name,			\
> > +	.offset = _off			\
> > +}
> 
> Perhaps consider regmap_mmio?

For multiple reasons no. First of all I need a direct access to the DR register
and I need to do this as fast as possible due to relatively slow APB
bus. The same concern is regarding the controller configs setting. Secondly
everywhere in the driver I use the normal read/write methods and there is no
need in even a basic regmap-update function. So an additional abstraction like
regmap would be unnecessary complication. Thirdly the access to the registers
is always done from a thread-safe context anyway (except DebugFS read operations).
Fourthly due to the dirmap access the DW SSI registers can be unavailable at the
moment of the dirmap read method invocation, that's why I need a mutual exclusive
control around the operations concerning the registers. Due to this the regmap_mmio
won't work here and I would have to implement my own accessors. There might be
another problems, so using regmap isn't a good idea.

> 
> > +static int bs_dbgfs_open_regs(struct inode *inode, struct file *file)
> > +{
> > +	struct bt1_spi *bs = inode->i_private;
> > +	int ret;
> > +
> > +	ret = single_open(file, bs_dbgfs_show_regs, bs);
> > +	if (!ret) {
> > +		mutex_lock(&bs->ctrl->io_mutex);
> 
> Holding a mutex over the entire time that a file is open is not good,
> it's also not clear to me what this mutex is supposed to be protecting
> given that it's only referenced in these debugfs functions.

Hm, it's io_mutex protecting the SPI IO operation to be thread-safe. See:
spi_mem_access_start()/spi_mem_access_end();
__spi_pump_messages();

By locking the mutex here I make sure the DebugFS registers dump operation won't
interfere the IO operations like dirmap, which make the registers unavailable
for normal MMIO-based access.

> 
> > +		writel(BC_CSR_SPI_RDA, bs->regs + BC_CSR);
> 
> Whatever this write is doing we never seem to undo it?

Here I switch on the DW APB SSI registers to be available for normal
readl/writel accesses. It's not a problem to leave the mode on, since
bs_set_cfg() will do a proper config anyway.


Wow, what a thorough review...

-Sergey
Chris Packham May 10, 2020, 10:17 p.m. UTC | #10
On 10/05/20 12:20 pm, Serge Semin wrote:
> On Fri, May 08, 2020 at 12:37:51PM +0100, Mark Brown wrote:
<snip>
>>> +	writel(BIT(req->cs), bs->regs + BC_SPI_SER);
>>> +	if (req->cs_gpiod) {
>>> +		gpiod_set_value_cansleep(req->cs_gpiod,
>>> +					 !!(bs->cfg.mode & SPI_CS_HIGH));
>> If you have a GPIO chip select you should just let the core manage it
>> through cs_gpiod rather than open coding.
> Of course I know this, and normally I would have omitted the GPIO manual
> assertion (hopefully soon my hands get to merging the AX99100 driver I've
> developed some time ago). The thing is that this Baikal-T1 System SSI device
> driver has been initially written before commit 05766050d5bd ("spi: spi-mem:
> fallback to using transfers when CS gpios are used"). So asserting GPIO CS had
> been required to initiate the SPI memory communications seeing the generic
> spi_mem_exec_op() doesn't do this. Manual GPIO manipulation is indeed redundant
> for the current SPI-mem op execution procedure.
>
> Secondly the message of that commit states "Devices with chip selects driven
> via GPIO are not compatible with the spi-mem operations." I find this statement
> questionable, because for instance this device supports memory operations with
> GPIO-driven CS. Though in current implementation the driver fallback to using normal
> push-pull IO mode if GPIO CS is utilized as safer one. But even in this case
> it's better than splitting the memory operations up into the transfers, which is
> developed in the spi_mem_exec_op() method.
On this specific bit. My use-case for 05766050d5bd was a SPI controller 
that supported direct mem accesses but a hardware design that required a 
GPIO CS. So yes I probably should have qualified it as _some_ devices.
> So in this matter my question is: how to modify the SPI-mem interface so the
> SPI-memory operations would also work with GPIO driven CS? Some additional flag
> might work...
Mark Brown May 11, 2020, 9:25 p.m. UTC | #11
On Sun, May 10, 2020 at 03:20:39AM +0300, Serge Semin wrote:
> On Fri, May 08, 2020 at 12:37:51PM +0100, Mark Brown wrote:

> > Your CC list on this series is *very* wide - are you sure that this is
> > relevant to everyone you're sending things to?  Kernel developers often
> > get a lot of mail meaning that extra mail can become a bit overwhelming
> > and cause things to get lost in the noise.

> MIPS folks are here since Baikal-T1 is a MIPS-based chip and this patchset
> is a part of the campaign of integrating the SoC support into the kernel.
> With Lee and Miquel we discussed the dirmap support in the framework of another
> patchset. Rob and devicetree-list are CC'ed due to having the bindings tree
> updated. Then a series of folks who recently submitted the biggest updates into
> the spi subsystems so might have valuable comments for this driver as well. So
> yes, I am sure.

I think you've got too many people who've been contributing to the
subsystem here at least - it looks like you picked up some people who
recently wrote drivers but haven't been doing a lot of review for
example for example.

> Anyway ok. I'll fix it. Is it ok to have the C-style comments in the header
> file?
> * It isn't included by any assembly so from this point of view C++ style
> * shall also work there.

The SPDX stuff requires C style comments in headers so yes.

> Secondly the message of that commit states "Devices with chip selects driven
> via GPIO are not compatible with the spi-mem operations." I find this statement
> questionable, because for instance this device supports memory operations with
> GPIO-driven CS. Though in current implementation the driver fallback to using normal
> push-pull IO mode if GPIO CS is utilized as safer one. But even in this case
> it's better than splitting the memory operations up into the transfers, which is
> developed in the spi_mem_exec_op() method.

> So in this matter my question is: how to modify the SPI-mem interface so the
> SPI-memory operations would also work with GPIO driven CS? Some additional flag
> might work...

Yes, some flags should work here - the issue was that at least some
controllers may end up trying to do multiple SPI operations for one
spi-mem thing which will break if the chip select doesn't get changed to
correspond with what's going on.

> Thirdly what about dirmap operations? If we got a GPIO driven CS then due to
> lacking any CS manipulation in spi_mem_dirmap_read() method we wouldn't have
> been able to make the direct mapping working without manual setting the GPIO.
> So the same question here. How to work this around and justify your requirement?
> Until the question is answered and we come up with reasonable solution in order
> to have the SPI-mem/dirmap interface working together with GPIO CS support I have
> to leave the manual GPIO manipulation.

If the issue with ensuring that chip select is managed appropriately for
transfers can be resolved then push that stuff up to the framework.  If
not then it's not clear that the open coded version can work well
with GPIO chip selects either.

> > > +static void bs_update_cfg(struct bt1_spi *bs, struct spi_transfer *xfer)
> > > +{

> > This has exactly one caller, perhaps it could just be inlined there?  I
> > think this applies to some of the other internal functions.

> I don't want to inline methods just because they are used in a single place. This
> might worsen readability and in this case it will. Currently the transfer_one()
> method is consistent with self-explanatory methods: update config, push-pull
> bytes/words, check status. This also applies to the rest of the code. I can
> consider some improvements/optimizations in this matter though, but embedding
> the functions isn't one of them. Moreover the compiler does the static
> methods inlining automatically as soon as it sees they are called just once.

One of the things that creating internal APIs like this does is that it
adds another layer of structure to the driver, making it a bit harder to
follow that it's following the usual structures of a Linux driver
(especially noticable here given the open coding).  It creates the
impression of the platform portability layers that people sometimes try
to build.

> > > +static int bs_check_status(struct bt1_spi *bs)
> > > +{

> > It's not obvious from the name that this function will busy wait rather
> > than just reading some status registers.

> What do you suggest then? Renaming? Splitting up? If renaming, then what name do
> you prefer? Something like bs_check_completion()?

wait_for_completion() or something else that mentions that it will wait
for completion rather than just looking to see if the operation has
completed.

> > > +static int bs_exec_mem_op(struct spi_mem *mem,
> > > +			  const struct spi_mem_op *op)
> > > +{

> > It's not clear to me that this hardware actually supports spi_mem in
> > hardware?

> SPI-mem operations are implemented by means of the EEPROM-read and Tx-only
> modes of the controller.

Sure, but those seem like normal SPI-level things rather than cases
where the hardware understands that it has a flash attached and is doing
flash specific things.  A very common case for this stuff is that
controllers have acceleration blocks for read and fall back on normal
SPI for writes and erases, that sounds like what's going on here.

> > > +	if (!bs->cfg.cs_gpiod) {
> > > +		bs_push_bytes(bs, cmd, len);
> > > +
> > > +		if (op->data.dir == SPI_MEM_DATA_IN)
> > > +			ret = bs_pull_bytes(bs, op->data.buf.in,
> > > +					    op->data.nbytes);
> > > +	} else {
> > > +		bs_pp_bytes(bs, cmd, NULL, len);
> > > +
> > > +		if (op->data.dir == SPI_MEM_DATA_IN)
> > > +			bs_pp_bytes(bs, NULL, op->data.buf.in,
> > > +				    op->data.nbytes);
> > > +	}
> > 

> > The actual transfers here are done using the same PIO functions as
> > everything else as far as I can see which makes me wonder what this
> > spi_mem implementation is adding over the standard SPI flash support.

> No, if no GPIO CS is supplied the memory read operation is performed by means of
> the EEPROM-read mode of the DW SSI controller (Write command is written to the
> Tx FIFO and read data length - to the CTRLR1 register). The memory write

So EEPROM read mode is a combined write/read - is that somehow less
performant or something than separate opeations so there's a reason not
to use it except in this special circumstance?

> operation utilizes the Tx-only mode (just push data to the Tx FIFO, controller
> discards all incoming data). See bs_set_cfg() for details.

That's not super illuminating on this issue?

> If GPIO CS is utilized then we fallback to the normal push-pull methods because
> first it's safer this way, second it's still better than handling multiple SPI
> transfers.

> BTW The EEPROM/Tx-only modes are required to be utilized for the controller on
> our SoC in order to support the SPI bus operations with highest frequencies.
> Due to relatively slow APB bus performance we have to minimize MMIO operation
> per a single byte read/write. This is another reason why we had to create a
> dedicated driver for this controller.

If the controller has support for TX only mode that seems like a good
thing to have in the generic driver - for TX only transfers that's less
bus I/O.  The EEPROM mode potentially also, I'm less clear on what
exactly it is but it sounds potentially useful for common cases with
register operations.  Perhaps other systems might have similarly slow
buses and get noticable performance benefits from using these modes.

> > > +static void bs_copy_from_map(void *to, void __iomem *from, size_t len)
> > > +{
> > > +	size_t shift, chunk;
> > > +	u32 data;

> > This feels like something that is likely to be suitable for a generic
> > implementation?  Indeed I'd kind of expect that the architecture
> > memcpy_to/fromio() would try to copy aligned blocks since it's usually
> > going to perform better (which I assume is why this function is doing
> > it).

> MIPS memcpy_to/fromio don't do this. They expect the MMIO space can be read in
> a same way as normal memory space. It's our controller "feature", that MMIO can
> be only accessible by lw/sw instructions. So I had no choice but to implement the
> aligned read/write operations by myself here. AFAICS this won't be suitable for
> the generic implementation due to the alignment restriction and the controller
> setups, which are supposed to be performed before accessing the dirmap MMIO.

You should document this in the driver so nobody comes along and does
the refactoring as a cleanup, it looks like duplicated or redundant code.

> > This seems a bit much - we can validate that the message doesn't contain
> > any delays (which is going to be the overwhelming majority of devices)
> > or multiple transfers and reject just those transfers we can't support.
> > Multiple transfers are an issue with this hardware but you could
> > implement support in the core for coalescing them into single transfers,
> > this isn't the only hardware with automatic chip select handling that
> > can't cope with scatter/gather so it would be a useful thing to have.

> As I said in the commit message to the binding this controller is nearly always
> connected to an external SPI flash (I'd say always, because Baikal-T1 is fully
> functional only when initially booted from an SPI flash). At least I don't know
> any hardware using this interface differently (I've worked with all devices with
> Baikal-T1 SoC on board). In current driver implementation the SPI-mem operations
> covers the native CS case while the prepare_message()/transfer_one()/unprepare_message()
> are used when GPIO-driven CS is available (due to the commit 05766050d5bd I had
> to implement it). So I didn't want to over-complicate the driver just to support
> some virtual hardware before it's really necessary.

Aside from the issue of tempting fate in board designs this circles back
to the open coding of the bits of the spi-mem stuff that aren't really
offloaded by the hardware - the driver would look a lot more standard
(and be closer to spi-dw I guess) if it only implemented the bits that
are offloaded and this interface was available.  Having the normal
SPI message interface would not only support non-flash hardware, it'd
also enable the use of more of the generic SPI flash code like other
similar controllers.

> > > +static int bs_transfer_one(struct spi_controller *ctrl, struct spi_device *spi,
> > > +			   struct spi_transfer *xfer)
> > > +{
> > > +	struct bt1_spi *bs = spi_controller_get_devdata(ctrl);
> > > +
> > > +	bs_update_cfg(bs, xfer);
> > > +
> > > +	if (bs->cfg.dfs <= 8)
> > > +		bs_pp_bytes(bs, xfer->tx_buf, xfer->rx_buf, xfer->len);
> > > +	else
> > > +		bs_pp_words(bs, xfer->tx_buf, xfer->rx_buf, xfer->len / 2);
> > 
> > This will have issues with transfers with an odd number of bytes won't
> > it?

> No, it won't. Transfers with bits per word greater than 8 should have even
> number of bytes seeing the len field is supposed to have the buffers length in
> bytes:

So dfs is bits per word?  This is one of those things where the internal
APIs make things harder to follow.

> > Perhaps consider regmap_mmio?

> For multiple reasons no. First of all I need a direct access to the DR register
> and I need to do this as fast as possible due to relatively slow APB
> bus. The same concern is regarding the controller configs setting. Secondly
> everywhere in the driver I use the normal read/write methods and there is no
> need in even a basic regmap-update function. So an additional abstraction like
> regmap would be unnecessary complication. Thirdly the access to the registers

That comment was more due to writing debug infrastructure than stuff
like update_bits - the main reason regmap-mmio exists is for the debug
and cache features.  If the performance is too marginal to allow any
overhead then fine.  There's nothing stopping you combining regmap and
non-regmap on the same device if you exclude the more sensitive
registers from regmap with the access operations but it's not exactly
nice.

> > > +	if (!ret) {
> > > +		mutex_lock(&bs->ctrl->io_mutex);

> > Holding a mutex over the entire time that a file is open is not good,
> > it's also not clear to me what this mutex is supposed to be protecting
> > given that it's only referenced in these debugfs functions.

> Hm, it's io_mutex protecting the SPI IO operation to be thread-safe. See:
> spi_mem_access_start()/spi_mem_access_end();
> __spi_pump_messages();

> By locking the mutex here I make sure the DebugFS registers dump operation won't
> interfere the IO operations like dirmap, which make the registers unavailable
> for normal MMIO-based access.

Oh, it's the SPI level mutex...  that is a bit icky, and it does let
userspace code block the bus fairly easily which could be problematic
but I guess it's debugfs so meh.

> > > +		writel(BC_CSR_SPI_RDA, bs->regs + BC_CSR);

> > Whatever this write is doing we never seem to undo it?

> Here I switch on the DW APB SSI registers to be available for normal
> readl/writel accesses. It's not a problem to leave the mode on, since
> bs_set_cfg() will do a proper config anyway.

A comment would help here, the fact that it's not undone looks like a
bug.
Serge Semin May 18, 2020, 12:05 a.m. UTC | #12
Mark,
I give up.) I'll try to integrate what I've done here in the generic DW APB SSI
driver framework. Then add some hooks so to support our specific DW APB SSI
controller. The I create a glue-layer for it. My answers to you comments are
below.

On Mon, May 11, 2020 at 10:25:06PM +0100, Mark Brown wrote:
> On Sun, May 10, 2020 at 03:20:39AM +0300, Serge Semin wrote:
> > On Fri, May 08, 2020 at 12:37:51PM +0100, Mark Brown wrote:
> 
> > > Your CC list on this series is *very* wide - are you sure that this is
> > > relevant to everyone you're sending things to?  Kernel developers often
> > > get a lot of mail meaning that extra mail can become a bit overwhelming
> > > and cause things to get lost in the noise.
> 
> > MIPS folks are here since Baikal-T1 is a MIPS-based chip and this patchset
> > is a part of the campaign of integrating the SoC support into the kernel.
> > With Lee and Miquel we discussed the dirmap support in the framework of another
> > patchset. Rob and devicetree-list are CC'ed due to having the bindings tree
> > updated. Then a series of folks who recently submitted the biggest updates into
> > the spi subsystems so might have valuable comments for this driver as well. So
> > yes, I am sure.
> 
> I think you've got too many people who've been contributing to the
> subsystem here at least - it looks like you picked up some people who
> recently wrote drivers but haven't been doing a lot of review for
> example for example.
> 
> > Anyway ok. I'll fix it. Is it ok to have the C-style comments in the header
> > file?
> > * It isn't included by any assembly so from this point of view C++ style
> > * shall also work there.
> 
> The SPDX stuff requires C style comments in headers so yes.
> 
> > Secondly the message of that commit states "Devices with chip selects driven
> > via GPIO are not compatible with the spi-mem operations." I find this statement
> > questionable, because for instance this device supports memory operations with
> > GPIO-driven CS. Though in current implementation the driver fallback to using normal
> > push-pull IO mode if GPIO CS is utilized as safer one. But even in this case
> > it's better than splitting the memory operations up into the transfers, which is
> > developed in the spi_mem_exec_op() method.
> 
> > So in this matter my question is: how to modify the SPI-mem interface so the
> > SPI-memory operations would also work with GPIO driven CS? Some additional flag
> > might work...
> 

> Yes, some flags should work here - the issue was that at least some
> controllers may end up trying to do multiple SPI operations for one
> spi-mem thing which will break if the chip select doesn't get changed to
> correspond with what's going on.

Ok. New SPI flag it is then. It will be something like this:
+ #define SPI_CONTROLLER_FLASH_SS		BIT(6)

Then we have to convert spi_set_cs() method to be non-static in the spi.c
to be used in the spi-mem.c to properly select slaves

Then the spi_mem_access_start() and spi_mem_access_stop() of spi-mem.c should be
updated in the following way:
--- spi_mem_access_start()
+++ spi_mem_access_start()
 
 	mutex_lock(&ctlr->bus_lock_mutex);
 	mutex_lock(&ctlr->io_mutex);
+
+	if (ctlr->flags & SPI_CONTROLLER_FLASH_SS)
+		spi_set_cs(mem->spi, true);
 
 	return 0;
 }
--- spi_mem_access_end()
+++ spi_mem_access_end()
 static void spi_mem_access_end(struct spi_mem *mem)
 {
 	struct spi_controller *ctlr = mem->spi->controller;
+
+	if (ctlr->flags & SPI_CONTROLLER_FLASH_SS)
+		spi_set_cs(mem->spi, false);
 
 	mutex_unlock(&ctlr->io_mutex);
 	mutex_unlock(&ctlr->bus_lock_mutex);
---

Method spi_mem_exec_op() shall also make sure that even if GPIO-driven CS is
utilized the normal mem-op is executed. Something like this should do this:
--- spi_mem_exec_op()
+++ spi_mem_exec_op()
 	if (!spi_mem_internal_supports_op(mem, op))
 		return -ENOTSUPP;
 
-	if (ctlr->mem_ops && !mem->spi->cs_gpiod) {
+	if (ctlr->mem_ops &&
+	    (!mem->spi->cs_gpiod || (ctlr->flags & SPI_CONTROLLER_FLASH_SS))) {
 		ret = spi_mem_access_start(mem);
 		if (ret)
 			return ret;
---

So, what do you think?

> 
> > Thirdly what about dirmap operations? If we got a GPIO driven CS then due to
> > lacking any CS manipulation in spi_mem_dirmap_read() method we wouldn't have
> > been able to make the direct mapping working without manual setting the GPIO.
> > So the same question here. How to work this around and justify your requirement?
> > Until the question is answered and we come up with reasonable solution in order
> > to have the SPI-mem/dirmap interface working together with GPIO CS support I have
> > to leave the manual GPIO manipulation.
> 
> If the issue with ensuring that chip select is managed appropriately for
> transfers can be resolved then push that stuff up to the framework.  If
> not then it's not clear that the open coded version can work well
> with GPIO chip selects either.

See the comment above.

> 
> > > > +static void bs_update_cfg(struct bt1_spi *bs, struct spi_transfer *xfer)
> > > > +{
> 
> > > This has exactly one caller, perhaps it could just be inlined there?  I
> > > think this applies to some of the other internal functions.
> 
> > I don't want to inline methods just because they are used in a single place. This
> > might worsen readability and in this case it will. Currently the transfer_one()
> > method is consistent with self-explanatory methods: update config, push-pull
> > bytes/words, check status. This also applies to the rest of the code. I can
> > consider some improvements/optimizations in this matter though, but embedding
> > the functions isn't one of them. Moreover the compiler does the static
> > methods inlining automatically as soon as it sees they are called just once.
> 
> One of the things that creating internal APIs like this does is that it
> adds another layer of structure to the driver, making it a bit harder to
> follow that it's following the usual structures of a Linux driver
> (especially noticable here given the open coding).  It creates the
> impression of the platform portability layers that people sometimes try
> to build.

Let's stop this discussion.) Since I am going to integrate this patch into the
generic DW APB SSI driver, I will have to follow that driver design anyway. So
most likely I won't implement a new handy abstraction.

> 
> > > > +static int bs_check_status(struct bt1_spi *bs)
> > > > +{
> 
> > > It's not obvious from the name that this function will busy wait rather
> > > than just reading some status registers.
> 
> > What do you suggest then? Renaming? Splitting up? If renaming, then what name do
> > you prefer? Something like bs_check_completion()?
> 
> wait_for_completion() or something else that mentions that it will wait
> for completion rather than just looking to see if the operation has
> completed.

Ok. If it will be still applicable.

> 
> > > > +static int bs_exec_mem_op(struct spi_mem *mem,
> > > > +			  const struct spi_mem_op *op)
> > > > +{
> 
> > > It's not clear to me that this hardware actually supports spi_mem in
> > > hardware?
> 
> > SPI-mem operations are implemented by means of the EEPROM-read and Tx-only
> > modes of the controller.
> 
> Sure, but those seem like normal SPI-level things rather than cases
> where the hardware understands that it has a flash attached and is doing
> flash specific things.

No, hardware can't detect whether the flash is attached. This must be defined by
the platform, like based on the DT sub-nodes.

> A very common case for this stuff is that
> controllers have acceleration blocks for read and fall back on normal
> SPI for writes and erases, that sounds like what's going on here.

Well, yeah, they do provide some acceleration. EEPROM-read provides automatic
write-cmd-dummy-data-then-read operations. But in this case the only thing we
have to push into the SPI Tx FIFO is command and dummy bytes. The read operation
will be performed automatically. So the only thing the driver has to do is to
keep up with Rx FIFO data pulling out before it's overflown. Similarly Tx-only
mode is used to perform the Flash write/erase commands. The driver has to keep up
with pushing data into the Tx FIFO before it gets emptied and CS is de-asserted.
Using these operations is mandatory in case if there is no GPIO-driven CS is
utilized, since native CS is automatically asserted during the whole EEPROM-read
or Tx-only mode executed.

As I also said such operations are very useful, since minimize IO operations
performed over the APB bus. By doing so we can support higher SPI bus
frequencies.

> 
> > > > +	if (!bs->cfg.cs_gpiod) {
> > > > +		bs_push_bytes(bs, cmd, len);
> > > > +
> > > > +		if (op->data.dir == SPI_MEM_DATA_IN)
> > > > +			ret = bs_pull_bytes(bs, op->data.buf.in,
> > > > +					    op->data.nbytes);
> > > > +	} else {
> > > > +		bs_pp_bytes(bs, cmd, NULL, len);
> > > > +
> > > > +		if (op->data.dir == SPI_MEM_DATA_IN)
> > > > +			bs_pp_bytes(bs, NULL, op->data.buf.in,
> > > > +				    op->data.nbytes);
> > > > +	}
> > > 
> 
> > > The actual transfers here are done using the same PIO functions as
> > > everything else as far as I can see which makes me wonder what this
> > > spi_mem implementation is adding over the standard SPI flash support.
> 
> > No, if no GPIO CS is supplied the memory read operation is performed by means of
> > the EEPROM-read mode of the DW SSI controller (Write command is written to the
> > Tx FIFO and read data length - to the CTRLR1 register). The memory write
> 
> So EEPROM read mode is a combined write/read - is that somehow less
> performant or something than separate opeations so there's a reason not
> to use it except in this special circumstance?

Actually It's more performant (less APB-bus MMIO operations required to perform
the transfers) and safer in case of Native CS-driven slave devices. In case of
GPIO-driven CS EEPROM and Tx-only are still more performant modes, but less safe
than normal synchronous SPI Tx/Rx mode.

I'll explain it one more time to logically continue this discussion. In general
the DW APB SSI controller may be utilized on two types of platform setups:

1) A slave device is selected by the DW APB SSI Native CS lane only.

You know, that Dw APB SSI native chip select is automatically asserted by the
SPI controller during a transfer being executed. As soon as there is no data in
the Tx FIFO to transfer (Tx-only or Tx-Rx modes) or there is no data left to
receive (EEPROM-read or Rx-only mode), the native CS will be automatically
de-asserted.

If we decided not to implement the mem-ops in the driver, then we'd have to:
for flash-write operation: analyze the passed message ahead, merge the Tx-only
operations and atomically execute them after activating Tx-only mode;
for flash-read operation: analyze the passed message ahead, merge Tx-only
operations, combine it with Rx-only tail part and atomically execute them all
after activating the EEPROM-read mode. Well, what we've done here is basically
a reverse operation performed by the spi_exec_mem_op(), which just split the
spi_mem_op structure up into a set of spi_transfer's. Why would we need to do
this, while we could just use the already provided perfectly suitable spi_mem_op
data structure for our needs? Especially seeing our controller supports the
operations like Tx-only and EEPROM-read specifically provided for the cases
of flash-device write/read operations. Isn't that a reason of claiming that it
supports mem-ops?

If we decided to implement the mem-ops in the driver (as it's done in this
patch), then the only thing we'd need to do is to enable the corresponding DW APB
SSI controller mode and push or push/pull spi_mem_op data on time. That's it.
No remapping, no data analyzing, no merging. Yes, the hardware doesn't provide
that much acceleration than normal Flash-capable SPI controllers, but still
it's better than handling standard transfers.

You'd say why remapping, analyzing and merging? Just use the normal SPI
synchronous Tx/Rx mode and push dummy zeros or ignore incoming data. The answer
is performance. Well, this might be good solution if the APB bus was fast enough.
If it's not, then we have to minimize the IO operations with Dw APB SSI
registers. This can be only done by using the EEPROM-read and Tx-only mode utilized. 

Note, all the operations described above MUST be executed in the atomic context,
so the data push/pull algorithms would keep up with keeping the Tx FIFO
non-empty and preventing the Rx FIFO overflown by automatically incoming data.
If either for a reason of IRQ or another task preemption or due to the slow APB
bus we get the Tx FIFO emptied, then the native CS will be de-asserted. If for
the same reason we get the Rx FIFO overflown, then we'll loose data.

2) A slave is selected by GPIO CS.

Well, TBH in this case it's still more performant to use the Tx-only and
EEPROM-read modes for communications (because we need less APB bus activity),
but these modes mandate the IRQs disabling since data is automatically
received. Since in this case the problem with automatic CS de-assertion doesn't
exist (we can manually set and clear GPIO CS), it is safe to use the normal
synchronous SPI Tx/Rx mode and avoid IRQs disabling. Though in case if better
performance is required then mem-ops should be utilized.


Note using Tx-only and EEPROM-read operations is more performant than
normal synchronous SPI Tx-Rx mode because it causes less APB bus activity.
In addition we need less data analyzing and mapping for spi-mem operations.
In our case both of these factors are very important due to slow APB bus.

(See further comment for details of what I can port from this driver to the
generic DW APB SSI core.)

> 
> > operation utilizes the Tx-only mode (just push data to the Tx FIFO, controller
> > discards all incoming data). See bs_set_cfg() for details.
> 
> That's not super illuminating on this issue?
> 
> > If GPIO CS is utilized then we fallback to the normal push-pull methods because
> > first it's safer this way, second it's still better than handling multiple SPI
> > transfers.
> 
> > BTW The EEPROM/Tx-only modes are required to be utilized for the controller on
> > our SoC in order to support the SPI bus operations with highest frequencies.
> > Due to relatively slow APB bus performance we have to minimize MMIO operation
> > per a single byte read/write. This is another reason why we had to create a
> > dedicated driver for this controller.
> 
> If the controller has support for TX only mode that seems like a good
> thing to have in the generic driver - for TX only transfers that's less
> bus I/O.  The EEPROM mode potentially also, I'm less clear on what
> exactly it is but it sounds potentially useful for common cases with
> register operations.  Perhaps other systems might have similarly slow
> buses and get noticable performance benefits from using these modes.

See my comments above. I gave up and will try to merge my driver into the
generic Dw APB SSI framework.

> 
> > > > +static void bs_copy_from_map(void *to, void __iomem *from, size_t len)
> > > > +{
> > > > +	size_t shift, chunk;
> > > > +	u32 data;
> 
> > > This feels like something that is likely to be suitable for a generic
> > > implementation?  Indeed I'd kind of expect that the architecture
> > > memcpy_to/fromio() would try to copy aligned blocks since it's usually
> > > going to perform better (which I assume is why this function is doing
> > > it).
> 
> > MIPS memcpy_to/fromio don't do this. They expect the MMIO space can be read in
> > a same way as normal memory space. It's our controller "feature", that MMIO can
> > be only accessible by lw/sw instructions. So I had no choice but to implement the
> > aligned read/write operations by myself here. AFAICS this won't be suitable for
> > the generic implementation due to the alignment restriction and the controller
> > setups, which are supposed to be performed before accessing the dirmap MMIO.
> 
> You should document this in the driver so nobody comes along and does
> the refactoring as a cleanup, it looks like duplicated or redundant code.

Ok.

> 
> > > This seems a bit much - we can validate that the message doesn't contain
> > > any delays (which is going to be the overwhelming majority of devices)
> > > or multiple transfers and reject just those transfers we can't support.
> > > Multiple transfers are an issue with this hardware but you could
> > > implement support in the core for coalescing them into single transfers,
> > > this isn't the only hardware with automatic chip select handling that
> > > can't cope with scatter/gather so it would be a useful thing to have.
> 
> > As I said in the commit message to the binding this controller is nearly always
> > connected to an external SPI flash (I'd say always, because Baikal-T1 is fully
> > functional only when initially booted from an SPI flash). At least I don't know
> > any hardware using this interface differently (I've worked with all devices with
> > Baikal-T1 SoC on board). In current driver implementation the SPI-mem operations
> > covers the native CS case while the prepare_message()/transfer_one()/unprepare_message()
> > are used when GPIO-driven CS is available (due to the commit 05766050d5bd I had
> > to implement it). So I didn't want to over-complicate the driver just to support
> > some virtual hardware before it's really necessary.
> 
> Aside from the issue of tempting fate in board designs this circles back
> to the open coding of the bits of the spi-mem stuff that aren't really
> offloaded by the hardware - the driver would look a lot more standard
> (and be closer to spi-dw I guess) if it only implemented the bits that
> are offloaded and this interface was available.  Having the normal
> SPI message interface would not only support non-flash hardware, it'd
> also enable the use of more of the generic SPI flash code like other
> similar controllers.

See my comment above, why it's necessary to have the spi-mem implemented for
the case of native CS. If GPIO-driven CS is utilized, then generic (synchronous
SPI Tx/Rx mode) but poll-based (since there is no IRQ provided for our controller)
algorithm can be utilized. If native CS is only available, then the safest way
is to have the atomic context by disabling the interrupts. This can be easily
done for the spi-mem interface, but nearly impossible for the generic
spi_transfer_one_message()-based IO. The only alternative is to implement the
device-specific transfer_one_message() callback, analyze SPI transfers,
merge them up, execute in the atomic context. Needless to say, that such approach
is only applicable for the IRQless controllers. If IRQ is available for the
controller, then using Native CS is still unsafe.

To sum up what approaches are better in what configurations if IRQ-less DW APB
SSI controller is available:
1) Flash/spi-mem + Native CS => EEPROM-read/Tx-only + atomic context - disable IRQs.
2) Flash/spi-mem + GPIO CS   => normal synchronous SPI Tx/Rx mode (currently this
                                will fallback to 4) because spi_mem_exec_op()
                                implementation, probably for good).
3) SPI msg + Native CS => reimplement transfer_one_message() + merge transfers
                          if possible otherwise use normal Tx+Rx mode + atomic
                          context - disabling IRQs during the whole message.
4) SPI msg + GPIO CS   => normal synchronous SPI Tx/Rx mode.

For the Dw APB SSI controller with IRQ the 2) and 4) items are the same. 1) and
3) can't be safely implemented due to possible concurrent IRQs.

Item 4) is already available in the current spi-dw.c driver, although the poll-mode
has been recently removed (I'll have to get it back). From this patch I can port the
items 1) and 2) to the generic version of the DW APB SSI driver. I won't spend
my time for 3) because first I've already spent a lot of it on implementing this
driver, second I don't need such functionality on our platforms. If someone ever
needs it, I'll assist with an advice of how to do this.

So, what do you think?

> 
> > > > +static int bs_transfer_one(struct spi_controller *ctrl, struct spi_device *spi,
> > > > +			   struct spi_transfer *xfer)
> > > > +{
> > > > +	struct bt1_spi *bs = spi_controller_get_devdata(ctrl);
> > > > +
> > > > +	bs_update_cfg(bs, xfer);
> > > > +
> > > > +	if (bs->cfg.dfs <= 8)
> > > > +		bs_pp_bytes(bs, xfer->tx_buf, xfer->rx_buf, xfer->len);
> > > > +	else
> > > > +		bs_pp_words(bs, xfer->tx_buf, xfer->rx_buf, xfer->len / 2);
> > > 
> > > This will have issues with transfers with an odd number of bytes won't
> > > it?
> 
> > No, it won't. Transfers with bits per word greater than 8 should have even
> > number of bytes seeing the len field is supposed to have the buffers length in
> > bytes:
> 
> So dfs is bits per word?  This is one of those things where the internal
> APIs make things harder to follow.
> 
> > > Perhaps consider regmap_mmio?
> 
> > For multiple reasons no. First of all I need a direct access to the DR register
> > and I need to do this as fast as possible due to relatively slow APB
> > bus. The same concern is regarding the controller configs setting. Secondly
> > everywhere in the driver I use the normal read/write methods and there is no
> > need in even a basic regmap-update function. So an additional abstraction like
> > regmap would be unnecessary complication. Thirdly the access to the registers
> 
> That comment was more due to writing debug infrastructure than stuff
> like update_bits - the main reason regmap-mmio exists is for the debug
> and cache features.  If the performance is too marginal to allow any
> overhead then fine.  There's nothing stopping you combining regmap and
> non-regmap on the same device if you exclude the more sensitive
> registers from regmap with the access operations but it's not exactly
> nice.
> 
> > > > +	if (!ret) {
> > > > +		mutex_lock(&bs->ctrl->io_mutex);
> 
> > > Holding a mutex over the entire time that a file is open is not good,
> > > it's also not clear to me what this mutex is supposed to be protecting
> > > given that it's only referenced in these debugfs functions.
> 
> > Hm, it's io_mutex protecting the SPI IO operation to be thread-safe. See:
> > spi_mem_access_start()/spi_mem_access_end();
> > __spi_pump_messages();
> 
> > By locking the mutex here I make sure the DebugFS registers dump operation won't
> > interfere the IO operations like dirmap, which make the registers unavailable
> > for normal MMIO-based access.
> 
> Oh, it's the SPI level mutex...  that is a bit icky, and it does let
> userspace code block the bus fairly easily which could be problematic
> but I guess it's debugfs so meh.

Ok.

> 
> > > > +		writel(BC_CSR_SPI_RDA, bs->regs + BC_CSR);
> 
> > > Whatever this write is doing we never seem to undo it?
> 
> > Here I switch on the DW APB SSI registers to be available for normal
> > readl/writel accesses. It's not a problem to leave the mode on, since
> > bs_set_cfg() will do a proper config anyway.
> 
> A comment would help here, the fact that it's not undone looks like a
> bug.

Ok.

-Sergey
Mark Brown May 18, 2020, 3:19 p.m. UTC | #13
On Mon, May 18, 2020 at 03:05:42AM +0300, Serge Semin wrote:
> On Mon, May 11, 2020 at 10:25:06PM +0100, Mark Brown wrote:

> > Yes, some flags should work here - the issue was that at least some
> > controllers may end up trying to do multiple SPI operations for one
> > spi-mem thing which will break if the chip select doesn't get changed to
> > correspond with what's going on.

> Ok. New SPI flag it is then. It will be something like this:
> + #define SPI_CONTROLLER_FLASH_SS		BIT(6)

I'd rather use CS than SS (it's more common in the code).

> So, what do you think?

Should be fine, controllers that have an issue implementing just
shouldn't set the flag.

> > > > It's not clear to me that this hardware actually supports spi_mem in
> > > > hardware?

> > > SPI-mem operations are implemented by means of the EEPROM-read and Tx-only
> > > modes of the controller.

> > Sure, but those seem like normal SPI-level things rather than cases
> > where the hardware understands that it has a flash attached and is doing
> > flash specific things.

> No, hardware can't detect whether the flash is attached. This must be defined by
> the platform, like based on the DT sub-nodes.

This isn't about autodetection, it's about the abstraction level the
hardware is operating on - some hardware is able to generate flash
operations by itself (possibly with some help programming the opcodes
that are needed by a given flash), some hardware just works at the
bytestream level.

> > A very common case for this stuff is that
> > controllers have acceleration blocks for read and fall back on normal
> > SPI for writes and erases, that sounds like what's going on here.
> 
> Well, yeah, they do provide some acceleration. EEPROM-read provides automatic
> write-cmd-dummy-data-then-read operations. But in this case the only thing we
> have to push into the SPI Tx FIFO is command and dummy bytes. The read operation

So it's a write then read but you have to program the write each time?
Serge Semin May 18, 2020, 9:17 p.m. UTC | #14
On Mon, May 18, 2020 at 04:19:47PM +0100, Mark Brown wrote:
> On Mon, May 18, 2020 at 03:05:42AM +0300, Serge Semin wrote:
> > On Mon, May 11, 2020 at 10:25:06PM +0100, Mark Brown wrote:
> 
> > > Yes, some flags should work here - the issue was that at least some
> > > controllers may end up trying to do multiple SPI operations for one
> > > spi-mem thing which will break if the chip select doesn't get changed to
> > > correspond with what's going on.
> 
> > Ok. New SPI flag it is then. It will be something like this:
> > + #define SPI_CONTROLLER_FLASH_SS		BIT(6)
> 
> I'd rather use CS than SS (it's more common in the code).

Ok.

> 
> > So, what do you think?
> 
> Should be fine, controllers that have an issue implementing just
> shouldn't set the flag.

Yes, exactly what I intended.

> 
> > > > > It's not clear to me that this hardware actually supports spi_mem in
> > > > > hardware?
> 
> > > > SPI-mem operations are implemented by means of the EEPROM-read and Tx-only
> > > > modes of the controller.
> 
> > > Sure, but those seem like normal SPI-level things rather than cases
> > > where the hardware understands that it has a flash attached and is doing
> > > flash specific things.
> 
> > No, hardware can't detect whether the flash is attached. This must be defined by
> > the platform, like based on the DT sub-nodes.
> 
> This isn't about autodetection, it's about the abstraction level the
> hardware is operating on - some hardware is able to generate flash
> operations by itself (possibly with some help programming the opcodes
> that are needed by a given flash), some hardware just works at the
> bytestream level.
> 
> > > A very common case for this stuff is that
> > > controllers have acceleration blocks for read and fall back on normal
> > > SPI for writes and erases, that sounds like what's going on here.
> > 
> > Well, yeah, they do provide some acceleration. EEPROM-read provides automatic
> > write-cmd-dummy-data-then-read operations. But in this case the only thing we
> > have to push into the SPI Tx FIFO is command and dummy bytes. The read operation
> 
> So it's a write then read but you have to program the write each time?

Here is what we need to do to perform the EEPROM-read operation:
1) Enable EEPROM-read mode.
2) Initialize a corresponding registers with a number of SPI transfer words
   (with bits-per-word taken into account) to read.
3) Push opcode + address + dummy bytes into the Tx FIFO. When it's done and
   the Tx FIFO is empty, the controller will proceed with read operations by
   pushing zeros (or ones, don't remember what level it's by default) to MOSI
   and pulling data from MISO into the RX FIFO.
4) Keep up with getting data from the Rx FIFO so one wouldn't get overflown.

Regarding programming write each time. Well, it's up to the driver implementation.
If opcode, address, dummy bytes and number of words to read are the same as before,
then re-programming isn't required.

-Sergey
Mark Brown May 19, 2020, 10:32 a.m. UTC | #15
On Tue, May 19, 2020 at 12:17:27AM +0300, Serge Semin wrote:

> Here is what we need to do to perform the EEPROM-read operation:
> 1) Enable EEPROM-read mode.
> 2) Initialize a corresponding registers with a number of SPI transfer words
>    (with bits-per-word taken into account) to read.
> 3) Push opcode + address + dummy bytes into the Tx FIFO. When it's done and
>    the Tx FIFO is empty, the controller will proceed with read operations by
>    pushing zeros (or ones, don't remember what level it's by default) to MOSI
>    and pulling data from MISO into the RX FIFO.
> 4) Keep up with getting data from the Rx FIFO so one wouldn't get overflown.

> Regarding programming write each time. Well, it's up to the driver implementation.
> If opcode, address, dummy bytes and number of words to read are the same as before,
> then re-programming isn't required.

Ah, nice.  This should be useful for far more than just flash - most
register reads will also be able to take advantage of this, they follow
a similar write then read pattern.

Patch
diff mbox series

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 741b9140992a..3b1815b6a904 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -171,6 +171,19 @@  config SPI_BCM_QSPI
 	  based platforms. This driver works for both SPI master for spi-nor
 	  flash device as well as MSPI device.
 
+config SPI_BT1_SYS
+	tristate "Baikal-T1 System Boot SPI Controller"
+	depends on (MIPS_BAIKAL_T1 && OF) || COMPILE_TEST
+	imply SPI_MEM
+	help
+	  SPI driver for Baikal-T1 System Boot SPI Controller. It's based on
+	  the DW APB SSI IP block, but with very limited resources available:
+	  no IRQ, no DMA, a single CS, FIFO of just 8 bytes depth. Its primary
+	  usecase is to boot the system from an external SPI-nor flash. So
+	  aside with the poll-based normal SPI interface the memory operations
+	  and read-only direct mapping are supported by the driver from
+	  scratch.
+
 config SPI_BITBANG
 	tristate "Utilities for Bitbanging SPI masters"
 	help
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 28f601327f8c..3c5d40a298e4 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -29,6 +29,7 @@  obj-$(CONFIG_SPI_BCM63XX)		+= spi-bcm63xx.o
 obj-$(CONFIG_SPI_BCM63XX_HSSPI)		+= spi-bcm63xx-hsspi.o
 obj-$(CONFIG_SPI_BCM_QSPI)		+= spi-iproc-qspi.o spi-brcmstb-qspi.o spi-bcm-qspi.o
 obj-$(CONFIG_SPI_BITBANG)		+= spi-bitbang.o
+obj-$(CONFIG_SPI_BT1_SYS)		+= spi-bt1-sys.o
 obj-$(CONFIG_SPI_BUTTERFLY)		+= spi-butterfly.o
 obj-$(CONFIG_SPI_CADENCE)		+= spi-cadence.o
 obj-$(CONFIG_SPI_CLPS711X)		+= spi-clps711x.o
diff --git a/drivers/spi/spi-bt1-sys.c b/drivers/spi/spi-bt1-sys.c
new file mode 100644
index 000000000000..f7ab0eb4c247
--- /dev/null
+++ b/drivers/spi/spi-bt1-sys.c
@@ -0,0 +1,873 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
+ *
+ * Authors:
+ *   Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
+ *   Serge Semin <Sergey.Semin@baikalelectronics.ru>
+ *
+ * Baikal-T1 System Boot SPI Controller driver
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/string.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
+#include <linux/of.h>
+#include <linux/clk.h>
+#include <linux/irqflags.h>
+#include <linux/preempt.h>
+#include <linux/debugfs.h>
+#include <linux/cpumask.h>
+#include <linux/seq_file.h>
+
+#include "spi-bt1-sys.h"
+
+static void bs_set_cfg(struct bt1_spi *bs, const struct bt1_spi_cfg *req)
+{
+	u32 ctrl0, ctrl1, baudr, min_sckdv, sckdv;
+
+	ctrl0 = FIELD_PREP(BC_SPI_CTRL0_DFS_MASK, req->dfs - 1) |
+		FIELD_PREP(BC_SPI_CTRL0_FRF_MASK, BC_SPI_CTRL0_FRF_SPI) |
+		(req->mode & SPI_CPHA ? BC_SPI_CTRL0_SCPH : 0) |
+		(req->mode & SPI_CPOL ? BC_SPI_CTRL0_SCPOL : 0) |
+		(req->mode & SPI_LOOP ? BC_SPI_CTRL0_SRL : 0) |
+		FIELD_PREP(BC_SPI_CTRL0_TMOD_MASK, req->tmode);
+	ctrl1 = FIELD_PREP(BC_SPI_CTRL1_NDF_MASK, req->ndf ? req->ndf - 1 : 0);
+
+	/*
+	 * There is a very tricky race condition hidden inside Baikal-T1 SoC
+	 * MMIO implementation. Simultaneous accesses to any registers via
+	 * the APB bus are serialized even if they are executed to different
+	 * addresses of even different controllers. This may cause
+	 * IO-operations delays (two, three, four, etc times the normal IO
+	 * operation latency), which in our case may lead to the data loss if
+	 * SPI-bus is fast enough and APB traffic is too heavy (either
+	 * generated by sibling CPU or DMA). So in order to fix this we have
+	 * no other way but to increase the SPI-bus clock divider lower limit.
+	 * Note this limitation isn't applicable to the SPI flash direct
+	 * mapping mode, since IO operations are performed by the Boot
+	 * Controller itself while CPU just reads the requested data.
+	 */
+	sckdv = DIV_ROUND_UP(bs->ref_freq, req->freq);
+	if (!req->dirmap && !req->cs_gpiod && num_possible_cpus() > 1)
+		min_sckdv = BC_SPI_MIN_SCKDV;
+	else
+		min_sckdv = 2;
+	sckdv = clamp_val(sckdv, min_sckdv, BC_SPI_BAUDR_SCKDV_MASK);
+	baudr = FIELD_PREP(BC_SPI_BAUDR_SCKDV_MASK, sckdv);
+
+	/*
+	 * Enable DW APB SSI controller registers access by means of the
+	 * RDA mode control bit. Only then we'll be able to read/write
+	 * from/to the controller memory mapped config-space.
+	 */
+	writel(BC_CSR_SPI_RDA, bs->regs + BC_CSR);
+
+	/*
+	 * Update the SPI controller configuration registers and enable
+	 * the requested slave for further communications.
+	 */
+	writel(0, bs->regs + BC_SPI_SSIENR);
+	writel(ctrl0, bs->regs + BC_SPI_CTRL0);
+	writel(ctrl1, bs->regs + BC_SPI_CTRL1);
+	writel(baudr, bs->regs + BC_SPI_BAUDR);
+	writel(BC_SPI_SSIENR_EN, bs->regs + BC_SPI_SSIENR);
+
+	bs->cfg = *req;
+
+	/*
+	 * DW APB SSI controller has a nasty peculiarity. It doesn't provide
+	 * a direct way to set and clear the native chip-select signal. Instead
+	 * the controller asserts the CS pin if Tx FIFO isn't empty and a
+	 * transmission is going on. When Tx FIFO doesn't have anything to be
+	 * pushed out the chip-select pin will be automatically de-asserted
+	 * back to the high level. Needless to say that the sudden CS
+	 * de-assertion in the middle of the message transfer most likely will
+	 * cause the data loss. So in order to make sure the executed transfers
+	 * are CS-atomic we have to either keep the controller FIFO non-empty
+	 * during the whole series of transfers or use GPIO-based chip-select
+	 * signal. The last case availability depends on the platform
+	 * configuration, while the former one can be achieved by making sure
+	 * nothing preempt the transfer execution context. It can be done by
+	 * disabling the local interrupts while the data being pushed and
+	 * pulled to/from the controller FIFOs. Note we don't have to worry
+	 * about this problem in case of SPI flash transparent access, because
+	 * IO operations are performed by the Boot Controller.
+	 */
+	writel(BIT(req->cs), bs->regs + BC_SPI_SER);
+	if (req->cs_gpiod) {
+		gpiod_set_value_cansleep(req->cs_gpiod,
+					 !!(bs->cfg.mode & SPI_CS_HIGH));
+	} else if (!req->dirmap) {
+		local_irq_save(bs->cfg.flags);
+		preempt_disable();
+	}
+
+	/*
+	 * If direct mapping is requested from now the DW APB SSI registers
+	 * are getting unavailable.
+	 */
+	if (req->dirmap)
+		writel(0, bs->regs + BC_CSR);
+}
+
+/*
+ * Note the next method is applicable only in non-direct mapping case. So
+ * we don't need to disable the transparent mode to access the DW APB SSI
+ * registers since they are already available after bs_set_cfg() actions.
+ */
+static void bs_update_cfg(struct bt1_spi *bs, struct spi_transfer *xfer)
+{
+	u32 ctrl0, baudr, min_sckdv, sckdv;
+
+	sckdv = DIV_ROUND_UP(bs->ref_freq, xfer->speed_hz);
+	if (!bs->cfg.cs_gpiod && num_possible_cpus() > 1)
+		min_sckdv = BC_SPI_MIN_SCKDV;
+	else
+		min_sckdv = 2;
+	sckdv = clamp_val(sckdv, min_sckdv, BC_SPI_BAUDR_SCKDV_MASK);
+	xfer->effective_speed_hz = bs->ref_freq / sckdv;
+
+	if (xfer->bits_per_word == bs->cfg.dfs &&
+	    xfer->speed_hz == bs->cfg.freq)
+		return;
+
+	ctrl0 = readl(bs->regs + BC_SPI_CTRL0) & ~BC_SPI_CTRL0_DFS_MASK;
+	ctrl0 |= FIELD_PREP(BC_SPI_CTRL0_DFS_MASK, xfer->bits_per_word - 1);
+	baudr = FIELD_PREP(BC_SPI_BAUDR_SCKDV_MASK, sckdv);
+
+	writel(0, bs->regs + BC_SPI_SSIENR);
+	writel(ctrl0, bs->regs + BC_SPI_CTRL0);
+	writel(baudr, bs->regs + BC_SPI_BAUDR);
+	writel(BC_SPI_SSIENR_EN, bs->regs + BC_SPI_SSIENR);
+
+	bs->cfg.dfs = xfer->bits_per_word;
+	bs->cfg.freq = xfer->speed_hz;
+}
+
+static void bs_clear_cfg(struct bt1_spi *bs)
+{
+	if (bs->cfg.cs_gpiod) {
+		gpiod_set_value_cansleep(bs->cfg.cs_gpiod,
+					 !(bs->cfg.mode & SPI_CS_HIGH));
+	} else if (!bs->cfg.dirmap) {
+		local_irq_restore(bs->cfg.flags);
+		preempt_enable();
+	}
+
+	/* Disable direct mapping if has been enabled in bs_set_cfg(). */
+	if (bs->cfg.dirmap)
+		writel(BC_CSR_SPI_RDA, bs->regs + BC_CSR);
+
+	writel(0, bs->regs + BC_SPI_SER);
+}
+
+static int bs_check_status(struct bt1_spi *bs)
+{
+	int ret = 0;
+	u32 data;
+
+	/* Check the bogus bits of the controller status register. */
+	data = readl(bs->regs + BC_SPI_RISR);
+	if (data & BC_SPI_RISR_RXO) {
+		dev_err(bs->dev, "RX FIFO overrun detected\n");
+		ret = -EIO;
+	}
+
+	if (data & BC_SPI_RISR_RXU) {
+		dev_err(bs->dev, "RX FIFO underrun detected\n");
+		ret = -EIO;
+	}
+
+	if (data & BC_SPI_RISR_TXO) {
+		dev_err(bs->dev, "TX FIFO overrun detected\n");
+		ret = -EIO;
+	}
+
+	readl(bs->regs + BC_SPI_ICR);
+
+	/*
+	 * Spin around the BUSY-state bit waiting for the controller to finish
+	 * all the requested IO-operations.
+	 */
+	do {
+		data = readl(bs->regs + BC_SPI_SR);
+	} while ((data & BC_SPI_SR_BUSY) || !(data & BC_SPI_SR_TFE));
+
+	return ret;
+}
+
+static int bs_pull_bytes(struct bt1_spi *bs, u8 *dst, unsigned int len)
+{
+	const u8 *end = dst + len;
+	const u8 *next = dst;
+	int retry = BC_SPI_RETRY;
+
+	/* Write dummy-data to the outbound FIFO to trigger the RO-transfer. */
+	if (bs->cfg.tmode == BC_SPI_CTRL0_TMOD_RO)
+		writel(GENMASK(bs->cfg.dfs - 1, 0), bs->regs + BC_SPI_DR);
+
+	while (dst < end) {
+		next += __raw_readl(bs->regs + BC_SPI_RXFLR);
+		if (dst < next) {
+			do {
+				*dst++ = __raw_readl(bs->regs + BC_SPI_DR);
+			} while (dst < next);
+			retry = BC_SPI_RETRY;
+		} else if (!retry--) {
+			return -EIO;
+		}
+	}
+
+	return 0;
+}
+
+static void bs_push_bytes(struct bt1_spi *bs, const u8 *src, unsigned int len)
+{
+	const u8 *end = src + len;
+	const u8 *next = src;
+
+	while (src < end) {
+		next += BC_SPI_FIFO_LEN - __raw_readl(bs->regs + BC_SPI_TXFLR);
+		if (src < next) {
+			if (next > end)
+				next = end;
+			do {
+				__raw_writel(*src++, bs->regs + BC_SPI_DR);
+			} while (src < next);
+		}
+	}
+}
+
+static void bs_pp_bytes(struct bt1_spi *bs, const u8 *src, u8 *dst,
+			unsigned int len)
+{
+	unsigned int txlen = len, rxlen = len;
+	u32 data, cnt;
+
+	/*
+	 * Note we have to keep the Tx FIFO only half full in order to prevent
+	 * the Rx FIFO overrun in case of the task rescheduling in the middle
+	 * of the loop.
+	 */
+	while (txlen || rxlen) {
+		cnt = BC_SPI_FIFO_LVL - __raw_readl(bs->regs + BC_SPI_TXFLR);
+		cnt = min(cnt, txlen);
+		txlen -= cnt;
+		while (cnt--) {
+			data = src ? *src++ : 0xFF;
+			__raw_writel(data, bs->regs + BC_SPI_DR);
+		}
+		cnt = __raw_readl(bs->regs + BC_SPI_RXFLR);
+		cnt = min(cnt, rxlen);
+		rxlen -= cnt;
+		while (cnt--) {
+			data = __raw_readl(bs->regs + BC_SPI_DR);
+			if (dst)
+				*dst++ = data;
+		}
+	}
+}
+
+static void bs_pp_words(struct bt1_spi *bs, const u16 *src, u16 *dst,
+			unsigned int len)
+{
+	unsigned int txlen = len, rxlen = len;
+	u32 data, cnt;
+
+	while (txlen || rxlen) {
+		cnt = BC_SPI_FIFO_LVL - __raw_readl(bs->regs + BC_SPI_TXFLR);
+		cnt = min(cnt, txlen);
+		txlen -= cnt;
+		while (cnt--) {
+			data = src ? *src++ : 0xFFFF;
+			__raw_writel(data, bs->regs + BC_SPI_DR);
+		}
+		cnt = __raw_readl(bs->regs + BC_SPI_RXFLR);
+		cnt = min(cnt, rxlen);
+		rxlen -= cnt;
+		while (cnt--) {
+			data = __raw_readl(bs->regs + BC_SPI_DR);
+			if (dst)
+				*dst++ = data;
+		}
+	}
+}
+
+static u8 *bs_create_cmd(struct bt1_spi *bs, const struct spi_mem_op *op,
+			 unsigned int *len)
+{
+	unsigned int i, j;
+	const u8 *data;
+	u8 *cmd;
+
+	/*
+	 * Calculate the total length of the EEPROM command transfer and
+	 * either use the pre-allocated buffer or create a temporary one.
+	 */
+	*len = 1 + op->addr.nbytes + op->dummy.nbytes;
+	if (op->data.dir == SPI_MEM_DATA_OUT)
+		*len += op->data.nbytes;
+
+	if (*len <= BC_SPI_BUF_SIZE) {
+		cmd = bs->buf;
+	} else {
+		cmd = kzalloc(*len, GFP_KERNEL);
+		if (!cmd)
+			return ERR_PTR(-ENOMEM);
+	}
+
+	/*
+	 * Collect the operation code, address and dummy bytes into the single
+	 * buffer.
+	 */
+	cmd[0] = op->cmd.opcode;
+	for (i = 1; i <= op->addr.nbytes; ++i) {
+		cmd[i] = op->addr.val >>
+			 (BITS_PER_BYTE * (op->addr.nbytes - i));
+	}
+	for (; i <= op->addr.nbytes + op->dummy.nbytes; ++i)
+		cmd[i] = 0xFF;
+
+	/*
+	 * If it's a transfer with data to be sent, also copy it into the
+	 * single buffer in order to speed up the push-data loop. Otherwise
+	 * in case of high-frequency SPI transfers we may have an occasional
+	 * output data loss.
+	 */
+	if (op->data.dir == SPI_MEM_DATA_OUT) {
+		data = op->data.buf.out;
+		for (j = 0; j < op->data.nbytes; ++i, ++j)
+			cmd[i] = data[j];
+	}
+
+	return cmd;
+}
+
+static void bs_clear_cmd(struct bt1_spi *bs, u8 *cmd)
+{
+	if (cmd != bs->buf)
+		kfree(cmd);
+}
+
+static int bs_adjust_mem_op_size(struct spi_mem *mem, struct spi_mem_op *op)
+{
+	if (op->data.dir == SPI_MEM_DATA_IN) {
+		if (op->data.nbytes > (BC_SPI_CTRL1_NDF_MASK + 1))
+			op->data.nbytes = BC_SPI_CTRL1_NDF_MASK + 1;
+	}
+
+	return 0;
+}
+
+static bool bs_supports_mem_op(struct spi_mem *mem,
+			       const struct spi_mem_op *op)
+{
+	if (op->data.buswidth > 1 || op->addr.buswidth > 1 ||
+	    op->dummy.buswidth > 1 || op->cmd.buswidth > 1)
+		return false;
+
+	return spi_mem_default_supports_op(mem, op);
+}
+
+static int bs_exec_mem_op(struct spi_mem *mem,
+			  const struct spi_mem_op *op)
+{
+	struct bt1_spi *bs = spi_controller_get_devdata(mem->spi->controller);
+	struct bt1_spi_cfg req;
+	unsigned int len;
+	int ret;
+	u8 *cmd;
+
+	/* Collect the controller configuration required by the operation. */
+	req.dirmap = false;
+	req.cs = mem->spi->chip_select;
+	req.cs_gpiod = mem->spi->cs_gpiod;
+	req.flags = 0;
+	req.mode = mem->spi->mode;
+	if (req.cs_gpiod) {
+		req.tmode = BC_SPI_CTRL0_TMOD_TR;
+		req.ndf = 0;
+	} else if (op->data.dir == SPI_MEM_DATA_IN) {
+		req.tmode = BC_SPI_CTRL0_TMOD_READ_EEPROM;
+		req.ndf = op->data.nbytes;
+	} else {
+		req.tmode = BC_SPI_CTRL0_TMOD_TO;
+		req.ndf = 0;
+	}
+	req.dfs = BITS_PER_BYTE;
+	req.freq = mem->spi->max_speed_hz;
+
+	/*
+	 * Collect the command data into a single output buffer to speed the
+	 * transmission up.
+	 */
+	cmd = bs_create_cmd(bs, op, &len);
+	if (IS_ERR(cmd))
+		return PTR_ERR(cmd);
+
+	bs_set_cfg(bs, &req);
+
+	/*
+	 * In the boosted mode we have to push and pull the data as soon as
+	 * possible otherwise it might be lost, due to either native CS
+	 * getting toggled or Rx FIFO getting overrun. In non-boosted mode we
+	 * can relax and push-pull data synchronously with no worries of losing
+	 * any data or breaking the transfers due to the native CS toggle.
+	 */
+	if (!bs->cfg.cs_gpiod) {
+		bs_push_bytes(bs, cmd, len);
+
+		if (op->data.dir == SPI_MEM_DATA_IN)
+			ret = bs_pull_bytes(bs, op->data.buf.in,
+					    op->data.nbytes);
+	} else {
+		bs_pp_bytes(bs, cmd, NULL, len);
+
+		if (op->data.dir == SPI_MEM_DATA_IN)
+			bs_pp_bytes(bs, NULL, op->data.buf.in,
+				    op->data.nbytes);
+	}
+
+	ret = bs_check_status(bs);
+
+	bs_clear_cfg(bs);
+
+	bs_clear_cmd(bs, cmd);
+
+	return ret;
+}
+
+static int bs_dirmap_create(struct spi_mem_dirmap_desc *desc)
+{
+	struct spi_controller *ctlr = desc->mem->spi->controller;
+	struct bt1_spi *bs = spi_controller_get_devdata(ctlr);
+
+	if (!bs->map || !bs_supports_mem_op(desc->mem, &desc->info.op_tmpl))
+		return -ENOTSUPP;
+
+	/*
+	 * Make sure the requested region doesn't go out of the physically
+	 * mapped flash memory bounds and the operation is read-only.
+	 */
+	if (desc->info.offset + desc->info.length > bs->map_len ||
+	    desc->info.op_tmpl.data.dir != SPI_MEM_DATA_IN)
+		return -ENOTSUPP;
+
+	return 0;
+}
+
+static void bs_copy_from_map(void *to, void __iomem *from, size_t len)
+{
+	size_t shift, chunk;
+	u32 data;
+
+	/*
+	 * We split the copying up into the next three stages: unaligned head,
+	 * aligned body, unaligned tail.
+	 */
+	shift = (size_t)from & 0x3;
+	if (shift) {
+		chunk = min_t(size_t, 4 - shift, len);
+		data = readl_relaxed(from - shift);
+		memcpy(to, &data + shift, chunk);
+		from += chunk;
+		to += chunk;
+		len -= chunk;
+	}
+
+	while (len >= 4) {
+		data = readl_relaxed(from);
+		memcpy(to, &data, 4);
+		from += 4;
+		to += 4;
+		len -= 4;
+	}
+
+	if (len) {
+		data = readl_relaxed(from);
+		memcpy(to, &data, len);
+	}
+}
+
+static ssize_t bs_dirmap_read(struct spi_mem_dirmap_desc *desc,
+			      u64 offs, size_t len, void *buf)
+{
+	struct spi_mem *mem = desc->mem;
+	struct bt1_spi *bs = spi_controller_get_devdata(mem->spi->controller);
+	struct bt1_spi_cfg req;
+
+	/*
+	 * Make sure the requested operation length is valid. Truncate the
+	 * length if it's greater than the length of the MMIO region.
+	 */
+	if (offs >= bs->map_len || !len)
+		return 0;
+
+	len = min_t(size_t, len, bs->map_len - offs);
+
+	/* Collect the controller configuration required by the operation. */
+	req.dirmap = true;
+	req.cs = mem->spi->chip_select;
+	req.cs_gpiod = mem->spi->cs_gpiod;
+	req.flags = 0;
+	req.mode = mem->spi->mode;
+	req.tmode = BC_SPI_CTRL0_TMOD_READ_EEPROM;
+	req.ndf = 4;
+	req.dfs = BITS_PER_BYTE;
+	req.freq = mem->spi->max_speed_hz;
+
+	bs_set_cfg(bs, &req);
+
+	bs_copy_from_map(buf, bs->map + offs, len);
+
+	bs_clear_cfg(bs);
+
+	return len;
+}
+
+static const struct spi_controller_mem_ops bs_mem_ops = {
+	.adjust_op_size = bs_adjust_mem_op_size,
+	.supports_op = bs_supports_mem_op,
+	.exec_op = bs_exec_mem_op,
+	.dirmap_create = bs_dirmap_create,
+	.dirmap_read = bs_dirmap_read
+};
+
+static int bs_prepare_message(struct spi_controller *ctrl,
+			      struct spi_message *msg)
+{
+	struct bt1_spi *bs = spi_controller_get_devdata(ctrl);
+	struct spi_transfer *xfer;
+	struct bt1_spi_cfg req;
+
+	/*
+	 * Currently the driver doesn't support the generic messages-based
+	 * SPI interface with pure native chip-select signal. This is due
+	 * to the automatic native chip-select toggle peculiarity described
+	 * in the comment of the bs_set_cfg() method. Alas we can't use the
+	 * IRQ-disable-based boost approach here since the native queue-based
+	 * SPI messages transfer method can sleep waiting for the
+	 * transfers/CS-change delays.
+	 */
+	if (!msg->spi->cs_gpiod)
+		return -ENOTSUPP;
+
+	/*
+	 * Collect the controller configuration common for all transfers and
+	 * specific to the very first one.
+	 */
+	xfer = list_first_entry(&msg->transfers, typeof(*xfer), transfer_list);
+	req.dirmap = false;
+	req.cs = msg->spi->chip_select;
+	req.cs_gpiod = msg->spi->cs_gpiod;
+	req.flags = 0;
+	req.mode = msg->spi->mode;
+	req.tmode = BC_SPI_CTRL0_TMOD_TR;
+	req.ndf = 0;
+	req.dfs = xfer->bits_per_word;
+	req.freq = xfer->speed_hz;
+
+	bs_set_cfg(bs, &req);
+
+	return 0;
+}
+
+static int bs_unprepare_message(struct spi_controller *ctrl,
+				struct spi_message *msg)
+{
+	struct bt1_spi *bs = spi_controller_get_devdata(ctrl);
+
+	bs_clear_cfg(bs);
+
+	return 0;
+}
+
+static int bs_transfer_one(struct spi_controller *ctrl, struct spi_device *spi,
+			   struct spi_transfer *xfer)
+{
+	struct bt1_spi *bs = spi_controller_get_devdata(ctrl);
+
+	bs_update_cfg(bs, xfer);
+
+	if (bs->cfg.dfs <= 8)
+		bs_pp_bytes(bs, xfer->tx_buf, xfer->rx_buf, xfer->len);
+	else
+		bs_pp_words(bs, xfer->tx_buf, xfer->rx_buf, xfer->len / 2);
+
+	return bs_check_status(bs);
+}
+
+static struct bt1_spi *bs_create_data(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct bt1_spi *bs;
+
+	bs = devm_kzalloc(dev, sizeof(*bs), GFP_KERNEL);
+	if (!bs)
+		return ERR_PTR(-ENOMEM);
+
+	bs->dev = dev;
+	bs->pdev = pdev;
+
+	return bs;
+}
+
+static int bs_request_regs(struct bt1_spi *bs)
+{
+	struct resource *res;
+
+	bs->regs = devm_platform_ioremap_resource_byname(bs->pdev, "config");
+	if (IS_ERR(bs->regs)) {
+		dev_err(bs->dev, "Couldn't get the controller registers\n");
+		return PTR_ERR(bs->regs);
+	}
+
+	/*
+	 * Direct mapping is optional feature, which in fact doesn't really
+	 * give much of the performance gain. But the driver supports it
+	 * anyway.
+	 */
+	bs->map = devm_platform_ioremap_resource_byname(bs->pdev, "map");
+	if (IS_ERR(bs->map)) {
+		bs->map = NULL;
+		bs->map_len = 0;
+		dev_warn(bs->dev, "No direct mapping memory detected\n");
+	} else {
+		res = platform_get_resource_byname(bs->pdev, IORESOURCE_MEM,
+						   "map");
+		bs->map_len = resource_size(res);
+	}
+
+	return 0;
+}
+
+static void bs_disable_clk(void *data)
+{
+	struct bt1_spi *bs = data;
+
+	clk_disable_unprepare(bs->ref);
+}
+
+static int bs_request_clks(struct bt1_spi *bs)
+{
+	int ret;
+
+	/*
+	 * SSI reference and APB clocks are synchronous so we only need a
+	 * single clock source requested and enabled.
+	 */
+	bs->ref = devm_clk_get(bs->dev, NULL);
+	if (IS_ERR(bs->ref)) {
+		dev_err(bs->dev, "Couldn't get the ref clock descriptor\n");
+		return PTR_ERR(bs->ref);
+	}
+
+	ret = clk_prepare_enable(bs->ref);
+	if (ret) {
+		dev_err(bs->dev, "Couldn't enable the reference clock\n");
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(bs->dev, bs_disable_clk, bs);
+	if (ret) {
+		dev_err(bs->dev, "Can't add the clocks disable action\n");
+		return ret;
+	}
+
+	bs->ref_freq = clk_get_rate(bs->ref);
+	if (!bs->ref_freq) {
+		dev_err(bs->dev, "Invalid reference clock frequency\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int bs_create_spi_controller(struct bt1_spi *bs)
+{
+	int ret;
+
+	bs->ctrl = spi_alloc_master(bs->dev, 0);
+	if (!bs->ctrl) {
+		dev_err(bs->dev, "No memory for SPI controller descriptor\n");
+		return -ENOMEM;
+	}
+
+	spi_controller_set_devdata(bs->ctrl, bs);
+
+	bs->ctrl->use_gpio_descriptors = true;
+	bs->ctrl->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP;
+	bs->ctrl->bits_per_word_mask =  SPI_BPW_RANGE_MASK(4, 16);
+	bs->ctrl->bus_num = bs->pdev->id;
+	bs->ctrl->prepare_message = bs_prepare_message;
+	bs->ctrl->unprepare_message = bs_unprepare_message;
+	bs->ctrl->transfer_one = bs_transfer_one;
+	bs->ctrl->mem_ops = &bs_mem_ops;
+	bs->ctrl->min_speed_hz = bs->ref_freq / (BC_SPI_BAUDR_SCKDV_MASK - 1);
+	bs->ctrl->max_speed_hz = bs->ref_freq / 2;
+	bs->ctrl->dev.of_node = bs->dev->of_node;
+	bs->ctrl->dev.fwnode = bs->dev->fwnode;
+	bs->ctrl->flags = SPI_MASTER_GPIO_SS;
+
+	ret = devm_spi_register_controller(bs->dev, bs->ctrl);
+	if (ret) {
+		spi_controller_put(bs->ctrl);
+		dev_err(bs->dev, "Failed to register the SPI controller\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_DEBUG_FS
+
+#define BC_SPI_DBGFS_REG(_name, _off)	\
+{					\
+	.name = _name,			\
+	.offset = _off			\
+}
+
+static const struct debugfs_reg32 bs_dbgfs_regs[] = {
+	BC_SPI_DBGFS_REG("CSR", BC_CSR),
+	BC_SPI_DBGFS_REG("MAR", BC_MAR),
+	BC_SPI_DBGFS_REG("DRID", BC_DRID),
+	BC_SPI_DBGFS_REG("VID", BC_VID),
+	BC_SPI_DBGFS_REG("CTRL0", BC_SPI_CTRL0),
+	BC_SPI_DBGFS_REG("CTRL1", BC_SPI_CTRL1),
+	BC_SPI_DBGFS_REG("SSIENR", BC_SPI_SSIENR),
+	BC_SPI_DBGFS_REG("SER", BC_SPI_SER),
+	BC_SPI_DBGFS_REG("BAUDR", BC_SPI_BAUDR),
+	BC_SPI_DBGFS_REG("TXFLR", BC_SPI_TXFLR),
+	BC_SPI_DBGFS_REG("RXFLR", BC_SPI_RXFLR),
+	BC_SPI_DBGFS_REG("SR", BC_SPI_SR),
+	BC_SPI_DBGFS_REG("RISR", BC_SPI_RISR),
+	BC_SPI_DBGFS_REG("IDR", BC_SPI_IDR),
+	BC_SPI_DBGFS_REG("VERSION", BC_SPI_VERSION)
+};
+
+static int bs_dbgfs_show_regs(struct seq_file *s, void *data)
+{
+	const struct debugfs_reg32 *regs = bs_dbgfs_regs;
+	struct bt1_spi *bs = s->private;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(bs_dbgfs_regs); i++, regs++) {
+		seq_printf(s, "%s = 0x%08x\n", regs->name,
+			   readl(bs->regs + regs->offset));
+		if (seq_has_overflowed(s))
+			break;
+	}
+
+	return 0;
+}
+
+static int bs_dbgfs_open_regs(struct inode *inode, struct file *file)
+{
+	struct bt1_spi *bs = inode->i_private;
+	int ret;
+
+	ret = single_open(file, bs_dbgfs_show_regs, bs);
+	if (!ret) {
+		mutex_lock(&bs->ctrl->io_mutex);
+		writel(BC_CSR_SPI_RDA, bs->regs + BC_CSR);
+	}
+
+	return ret;
+}
+
+static int bs_dbgfs_release_regs(struct inode *inode, struct file *file)
+{
+	struct bt1_spi *bs = inode->i_private;
+
+	mutex_unlock(&bs->ctrl->io_mutex);
+
+	return single_release(inode, file);
+}
+
+static const struct file_operations bs_fops_regs = {
+	.open = bs_dbgfs_open_regs,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = bs_dbgfs_release_regs
+};
+
+static void bs_dbgfs_remove(void *data)
+{
+	struct bt1_spi *bs = data;
+
+	debugfs_remove_recursive(bs->dbgfs);
+}
+
+static void bs_dbgfs_init(struct bt1_spi *bs)
+{
+	char name[32];
+	int ret;
+
+	snprintf(name, 32, "bt1-sys-ssi%d", bs->ctrl->bus_num);
+	bs->dbgfs = debugfs_create_dir(name, NULL);
+
+	ret = devm_add_action_or_reset(bs->dev, bs_dbgfs_remove, bs);
+	if (ret)
+		return;
+
+	debugfs_create_file("registers", 0400, bs->dbgfs, bs, &bs_fops_regs);
+}
+
+#else
+
+static inline int bs_dbgfs_init(struct bt1_spi *bs)
+{
+	return 0;
+}
+
+#endif /* CONFIG_DEBUG_FS */
+
+static int bs_probe(struct platform_device *pdev)
+{
+	struct bt1_spi *bs;
+	int ret;
+
+	bs = bs_create_data(pdev);
+	if (IS_ERR(bs))
+		return PTR_ERR(bs);
+
+	ret = bs_request_regs(bs);
+	if (ret)
+		return ret;
+
+	ret = bs_request_clks(bs);
+	if (ret)
+		return ret;
+
+	ret = bs_create_spi_controller(bs);
+	if (ret)
+		return ret;
+
+	bs_dbgfs_init(bs);
+
+	return 0;
+}
+
+static const struct of_device_id bs_of_match[] = {
+	{ .compatible = "baikal,bt1-sys-ssi" },
+	{ }
+};
+
+static struct platform_driver bs_driver = {
+	.probe = bs_probe,
+	.driver = {
+		.name = "bt1-sys-ssi",
+		.of_match_table = bs_of_match
+	}
+};
+module_platform_driver(bs_driver);
+
+MODULE_AUTHOR("Serge Semin <Sergey.Semin@baikalelectronics.ru>");
+MODULE_DESCRIPTION("Baikal-T1 System Boot SPI Controller driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/spi/spi-bt1-sys.h b/drivers/spi/spi-bt1-sys.h
new file mode 100644
index 000000000000..a45c0ef9ff4d
--- /dev/null
+++ b/drivers/spi/spi-bt1-sys.h
@@ -0,0 +1,169 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
+ *
+ * Baikal-T1 System Boot SPI Controller driver
+ */
+#ifndef __SPI_BT1_SYS_H__
+#define __SPI_BT1_SYS_H__
+
+#include <linux/kernel.h>
+#include <linux/bits.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/debugfs.h>
+#include <linux/gpio/consumer.h>
+#include <linux/spi/spi.h>
+
+/*
+ * Baikal-T1 System Boot SPI controller registers. In addition to the Vendor-
+ * specific settings these are normal Synopsys DW APB SSI controller registers.
+ * Exception is that the IP core is synthesized with no IRQ/DMA, with very
+ * small FIFO of just 8 words and have a specific IP block implementing
+ * physically mapped SPI flash found at CS0.
+ */
+#define BC_CSR				0x000
+#define BC_CSR_MODE_FLD			0
+#define BC_CSR_MODE_MASK		GENMASK(1, BC_CSR_MODE_FLD)
+#define BC_CSR_MODE_ROM			0
+#define BC_CSR_MODE_SRAM		1
+#define BC_CSR_MODE_FLASH		2
+#define BC_CSR_SPI_RDA			BIT(8)
+#define BC_MAR				0x004
+#define BC_MAR_BSAB			BIT(0)
+#define BC_DRID				0x008
+#define BC_VID				0x00C
+#define BC_SPI_CTRL0			0x100
+#define BC_SPI_CTRL0_DFS_FLD		0
+#define BC_SPI_CTRL0_DFS_MASK		GENMASK(3, BC_SPI_CTRL0_DFS_FLD)
+#define BC_SPI_CTRL0_FRF_FLD		4
+#define BC_SPI_CTRL0_FRF_MASK		GENMASK(5, BC_SPI_CTRL0_FRF_FLD)
+#define BC_SPI_CTRL0_FRF_SPI		0
+#define BC_SPI_CTRL0_FRF_SSP		1
+#define BC_SPI_CTRL0_FRF_MW		2
+#define BC_SPI_CTRL0_SCPH		BIT(6)
+#define BC_SPI_CTRL0_SCPOL		BIT(7)
+#define BC_SPI_CTRL0_TMOD_FLD		8
+#define BC_SPI_CTRL0_TMOD_MASK		GENMASK(9, BC_SPI_CTRL0_TMOD_FLD)
+#define BC_SPI_CTRL0_TMOD_TR		0
+#define BC_SPI_CTRL0_TMOD_TO		1
+#define BC_SPI_CTRL0_TMOD_RO		2
+#define BC_SPI_CTRL0_TMOD_READ_EEPROM	3
+#define BC_SPI_CTRL0_SRL		BIT(11)
+#define BC_SPI_CTRL1			0x104
+#define BC_SPI_CTRL1_NDF_FLD		0
+#define BC_SPI_CTRL1_NDF_MASK		GENMASK(15, BC_SPI_CTRL1_NDF_FLD)
+#define BC_SPI_SSIENR			0x108
+#define BC_SPI_SSIENR_EN		BIT(0)
+#define BC_SPI_SER			0x110
+#define BC_SPI_SER_0			BIT(0)
+#define BC_SPI_BAUDR			0x114
+#define BC_SPI_BAUDR_SCKDV_FLD		0
+#define BC_SPI_BAUDR_SCKDV_MASK		GENMASK(15, BC_SPI_BAUDR_SCKDV_FLD)
+#define BC_SPI_TXFTLR			0x118
+#define BC_SPI_RXFTLR			0x11C
+#define BC_SPI_TXFLR			0x120
+#define BC_SPI_TXFLR_TXTFL_FLD		0
+#define BC_SPI_TXFLR_TXTFL_MASK		GENMASK(2, BC_SPI_TXFLR_TXTFL_FLD)
+#define BC_SPI_RXFLR			0x124
+#define BC_SPI_RXFLR_RXTFL_FLD		0
+#define BC_SPI_RXFLR_RXTFL_MASK		GENMASK(2, BC_SPI_RXFLR_RXTFL_FLD)
+#define BC_SPI_SR			0x128
+#define BC_SPI_SR_BUSY			BIT(0)
+#define BC_SPI_SR_TFNF			BIT(1)
+#define BC_SPI_SR_TFE			BIT(2)
+#define BC_SPI_SR_RFNE			BIT(3)
+#define BC_SPI_SR_RFF			BIT(4)
+#define BC_SPI_RISR			0x134
+#define BC_SPI_RISR_TXE			BIT(0)
+#define BC_SPI_RISR_TXO			BIT(1)
+#define BC_SPI_RISR_RXU			BIT(2)
+#define BC_SPI_RISR_RXO			BIT(3)
+#define BC_SPI_RISR_RXF			BIT(4)
+#define BC_SPI_TXOICR			0x138
+#define BC_SPI_RXOICR			0x13C
+#define BC_SPI_RXUICR			0x140
+#define BC_SPI_ICR			0x148
+#define BC_SPI_ICR_STS			BIT(0)
+#define BC_SPI_IDR			0x158
+#define BC_SPI_VERSION			0x15C
+#define BC_SPI_DR			0x160
+#define BC_SPI_RX_DLY			0x1F0
+
+/*
+ * Boot controller specific parameters
+ * @BC_SPI_FIFO_LEN: DW APB SSI controller FIFO length.
+ * @BC_SPI_FIFO_LVL: Safe Tx FIFO level so not to afraid the xfer task being
+ *		     scheduled.
+ * @BC_SPI_NUM_CS: Number of hardware chip selects.
+ * @BC_SPI_MIN_SCKDV: Minimal clock divider supported when native CS is used.
+ * @BC_SPI_RETRY: Number of retries before give up in waiting new data.
+ *		  Timeout may happen if the controller detected Rx-overrun
+ *		  event.
+ * @BC_SPI_BUF_SIZE: Size of the cmd-buffer for Read/Write-EEPROM transfer. It
+ *		     includes the opcode, address, dummy and data bytes.
+ */
+#define BC_SPI_FIFO_LEN			8
+#define BC_SPI_FIFO_LVL			(BC_SPI_FIFO_LEN / 2)
+#define BC_SPI_NUM_CS			1
+#define BC_SPI_MIN_SCKDV		6
+#define BC_SPI_RETRY			1000
+#define BC_SPI_BUF_SIZE		(1 + BITS_PER_LONG_LONG / BITS_PER_BYTE + 512)
+
+/*
+ * struct bt1_spi_cfg - DW APB SPI controller configuration
+ * @dirmap: Flag of SPI flash direct mapping.
+ * @cs: Native chip select number.
+ * @cs_gpiod: Chip-select GPIO descriptor.
+ * @flags: IRQ flags saved in case of the boosted transfers.
+ * @mode: SPI transfer mode.
+ * @tmode: TR/TO/RO/EEPROM modes.
+ * @dfs: Data frame size.
+ * @ndf: Number of data frames for RO/EEPROM transfers.
+ * @freq: Transfer frequency.
+ */
+struct bt1_spi_cfg {
+	bool dirmap;
+	u8 cs;
+	struct gpio_desc *cs_gpiod;
+	unsigned long flags;
+	u32 mode;
+	u32 tmode;
+	u32 dfs;
+	u32 ndf;
+	u32 freq;
+};
+
+/*
+ * struct bt1_spi - Bailal-T1 System Boot SPI controller private data
+ * @dev: Pointer to the device structure.
+ * @pdev: Pointer to the platform device structure.
+ * @ctrl: Pointer to the SPI controller structure.
+ * @regs: Memory mapped controller registers.
+ * @map: Physically mapped SPI flash.
+ * @map_len: Length of the physically mapped ROM.
+ * @buf: Temporary buffer.
+ * @cfg: Current controller configuration.
+ * @ref_freq: Reference frequency.
+ * @ref: Reference clock source.
+ */
+struct bt1_spi {
+	struct device *dev;
+	struct platform_device *pdev;
+	struct spi_controller *ctrl;
+
+	void __iomem *regs;
+	void __iomem *map;
+	resource_size_t map_len;
+
+	u8 buf[BC_SPI_BUF_SIZE];
+	struct bt1_spi_cfg cfg;
+	unsigned long ref_freq;
+	struct clk *ref;
+
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *dbgfs;
+#endif
+};
+
+#endif /* __SPI_BT1_SYS_H__ */