mbox series

[00/17] spi: dw: Add generic DW DMA controller support

Message ID 20200508132943.9826-1-Sergey.Semin@baikalelectronics.ru (mailing list archive)
Headers show
Series spi: dw: Add generic DW DMA controller support | expand

Message

Serge Semin May 8, 2020, 1:29 p.m. UTC
Baikal-T1 SoC provides DW DMA controller to perform low-speed peripherals
Mem-to-Dev and Dev-to-Mem transaction. This is also applicable to the DW
APB SSI devices embedded into the SoC. Currently this type DMA device is
supported by the DW APB SPI driver only as a middle layer code for Intel
MID PCI devices. Seeing the same code can be used for normal platform
DW DMAC device we introduced a set of patches to fix it within this
patchset.

First of all traditionally we replaced the legacy plain text-based dt-binding
file with yaml-based one. Then we unpinned the Intel MID specific code from
the generic DMA one and placed it into the spi-dw-pci.c driver, which was a
better place for it anyway. Then we introduced a set of naming cleanups since
the code was going to be used for generic DW DMAC device and DMAC usage
alterations to handle the controller functionality in a generic way by the
DW APB SSI MMIO driver as well. See the individual patches commit messages
for details.

In addition we fixed a problem in the native chip-select method, which despite
of multiple attempts to be fixed doesn't correctly perceive the SPI_CS_HIGH
flag and the enable-argument.

Finally as a cherry on a cake we replaced the manually written DebugFS
registers read method with a ready-to-use for the same purpose regset32
DebugFS interface usage.

This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4:
0e698dfa2822 ("Linux 5.7-rc4")
tag: v5.7-rc4

Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
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: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>
Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>
Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru>
Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Allison Randal <allison@lohutok.net>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Gareth Williams <gareth.williams.jx@renesas.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: linux-spi@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Serge Semin (17):
  dt-bindings: spi: Convert DW SPI binding to DT schema
  dt-bindings: spi: dw: Add DMA properties bindings
  spi: dw: Split up the generic DMA code and Intel MID driver
  spi: dw: Cleanup generic DW DMA code namings
  spi: dw: Discard static DW DMA slave structures
  spi: dw: Add DW SPI DMA/PCI/MMIO dependency on DW SPI core
  spi: dw: Add Tx/Rx finish wait methods to DMA
  spi: dw: Clear DMAC register when done or stopped
  spi: dw: Enable interrupts in accordance with DMA xfer mode
  spi: dw: Parameterize the DMA Rx/Tx burst length
  spi: dw: Fix native CS being unset
  spi: dw: Fix dma_slave_config used partly uninitialized
  spi: dw: Initialize paddr in DW SPI MMIO private data
  spi: dw: Add DMA support to the DW SPI MMIO driver
  spi: dw: Use DMA max burst to set the request thresholds
  spi: dw: Fix Rx-only DMA transfers
  spi: dw: Use regset32 DebugFS method to create a registers file

 .../bindings/spi/snps,dw-apb-ssi.txt          |  41 ---
 .../bindings/spi/snps,dw-apb-ssi.yaml         | 123 +++++++++
 .../devicetree/bindings/spi/spi-dw.txt        |  24 --
 drivers/spi/Kconfig                           |  16 +-
 drivers/spi/Makefile                          |   4 +-
 drivers/spi/{spi-dw-mid.c => spi-dw-dma.c}    | 237 ++++++++++++------
 drivers/spi/spi-dw-mmio.c                     |  15 +-
 drivers/spi/spi-dw-pci.c                      |  38 ++-
 drivers/spi/spi-dw.c                          |  89 +++----
 drivers/spi/spi-dw.h                          |  27 +-
 10 files changed, 402 insertions(+), 212 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
 create mode 100644 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
 delete mode 100644 Documentation/devicetree/bindings/spi/spi-dw.txt
 rename drivers/spi/{spi-dw-mid.c => spi-dw-dma.c} (53%)

Comments

Mark Brown May 8, 2020, 1:33 p.m. UTC | #1
On Fri, May 08, 2020 at 04:29:25PM +0300, Serge Semin wrote:

> Serge Semin (17):
>   dt-bindings: spi: Convert DW SPI binding to DT schema

Please don't make new feature development dependent on conversion to the
new schema format, there's quite a backlog of reviews of schema
conversions so it can slow things down.  It's good to do the conversions
but please do them after adding any new stuff to the binding rather than
before.
Mark Brown May 8, 2020, 5:30 p.m. UTC | #2
On Fri, May 08, 2020 at 04:29:32PM +0300, Serge Semin wrote:

> Since DMA transfers are performed asynchronously with actual SPI
> transaction, then even if DMA transfers are finished it doesn't mean
> all data is actually pushed to the SPI bus. Some data might still be

This looks like a bug fix so it should really have gone at the start of
the series so it can be sent to Linus as a bug fix rather than waiting
for the merge window.  This makes sense to me, a couple of nits below:

> +static void dw_spi_dma_wait_tx_done(struct dw_spi *dws)
> +{
> +	int retry = WAIT_RETRIES;
> +	unsigned long ns;
> +
> +	ns = (NSEC_PER_SEC / spi_get_clk(dws)) * dws->n_bytes * BITS_PER_BYTE;
> +	ns *= dw_readl(dws, DW_SPI_TXFLR);
> +
> +	while (dw_spi_dma_tx_busy(dws) && retry--)
> +		ndelay(ns);

How deep can the FIFO be with this IP - could we end up ndelay()ing for
non-trivial amounts of time?

> +static inline u32 spi_get_clk(struct dw_spi *dws)
> +{
> +	u32 div = dw_readl(dws, DW_SPI_BAUDR);
> +
> +	return div ? dws->max_freq / div : 0;

Please write normal conditional statements rather than using the ternery
operator - it helps with legibility.
Mark Brown May 8, 2020, 5:36 p.m. UTC | #3
On Fri, May 08, 2020 at 04:29:25PM +0300, Serge Semin wrote:
> Baikal-T1 SoC provides DW DMA controller to perform low-speed peripherals
> Mem-to-Dev and Dev-to-Mem transaction. This is also applicable to the DW
> APB SSI devices embedded into the SoC. Currently this type DMA device is

This basically all looks good to me (without any hardware specific
knowledge), I had a few comments but they were mostly procedural ones -
mainly getting these bug fixes you've done merged as such.  Nice work.
Andy Shevchenko May 8, 2020, 7:16 p.m. UTC | #4
On Fri, May 08, 2020 at 06:36:09PM +0100, Mark Brown wrote:
> On Fri, May 08, 2020 at 04:29:25PM +0300, Serge Semin wrote:
> > Baikal-T1 SoC provides DW DMA controller to perform low-speed peripherals
> > Mem-to-Dev and Dev-to-Mem transaction. This is also applicable to the DW
> > APB SSI devices embedded into the SoC. Currently this type DMA device is
> 
> This basically all looks good to me (without any hardware specific
> knowledge), I had a few comments but they were mostly procedural ones -
> mainly getting these bug fixes you've done merged as such.  Nice work.

Agree that is nice work!
Though, can you please give us chance to review next version of the series and test on our hardware?

Serge, I think you should base it on spi/for-next rather than vanilla.
Andy Shevchenko May 8, 2020, 7:23 p.m. UTC | #5
On Fri, May 08, 2020 at 04:29:39PM +0300, Serge Semin wrote:
> Since the common code in the spi-dw-dma.c driver is ready to be used
> by the MMIO driver, we just need to initialize the DW SPI private data
> fields with Tx/Rx DMA channel handlers and setup the DW SPI DMA
> callbacks. This commit introduces such alterations to the DW SPI
> DMA/MMIO code. A dedicated DW SPI MMIO DMA initializer performs the
> DMA channels request from the parental device (which is supposed to be
> a DW SPI platform device). Then it checks whether the retrieved DW DMA
> channels support LLP. If they don't currently we print a warning, that
> further DMA usage may be finished with DW SPI inbound FIFO overflow
> (nothing else we can do about it at the moment). Finally the DW SPI
> MMIO DMA initializer sets the DMA-inited flag.
> 
> Currently the DMA will be initialized only for a generic DW SPI MMIO
> device declared with "snps,dw-apb-ssi" compatible string if SPI_DW_DMA
> kernel config is enabled.

Already in spi/for-next (see for generic suffix in spi-dw-mid.c).
Andy Shevchenko May 8, 2020, 7:27 p.m. UTC | #6
On Fri, May 08, 2020 at 04:29:41PM +0300, Serge Semin wrote:
> Tx-only DMA transfers are working perfectly fine since in this case
> the code just ignores the Rx FIFO overflow interrupts. But it turns
> out the SPI Rx-only transfers are broken since nothing pushing any
> data to the shift registers, so the Rx FIFO is left empty and the
> SPI core subsystems just returns a timeout error. Since DW DMAC
> driver doesn't support something like cyclic write operations of
> a single byte to a device register, the only way to support the
> Rx-only SPI transfers is to fake it by using a dummy Tx-buffer.
> This is what we intend to fix in this commit by setting the
> SPI_CONTROLLER_MUST_TX flag for DMA-capable platform.

Hmm... If Mark consider this a right thing to do, then it's fine.
I didn't investigate what this flag may produce as a side effect.

> --- a/drivers/spi/spi-dw.c
> +++ b/drivers/spi/spi-dw.c
> @@ -524,6 +524,7 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>  			dws->dma_inited = 0;
>  		} else {
>  			master->can_dma = dws->dma_ops->can_dma;
> +			master->flags |= SPI_CONTROLLER_MUST_TX;
>  		}
>  	}
>  
> -- 
> 2.25.1
>
Andy Shevchenko May 8, 2020, 7:30 p.m. UTC | #7
On Fri, May 08, 2020 at 04:29:42PM +0300, Serge Semin wrote:
> DebugFS kernel interface provides a dedicated method to create the
> registers dump file. Use it instead of creating a generic DebugFS
> file with manually written read callback function.

Of course ideal would be use something like regmap API which delivers this as a side effect.
But this also good clean up!
Comments below.

> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> Cc: Ramil Zaripov <Ramil.Zaripov@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: Arnd Bergmann <arnd@arndb.de>
> Cc: Allison Randal <allison@lohutok.net>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Gareth Williams <gareth.williams.jx@renesas.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> ---
>  drivers/spi/spi-dw.c | 85 +++++++++++++-------------------------------
>  drivers/spi/spi-dw.h |  2 ++
>  2 files changed, 27 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> index 897c998af578..625a5f152e1a 100644
> --- a/drivers/spi/spi-dw.c
> +++ b/drivers/spi/spi-dw.c
> @@ -32,66 +32,28 @@ struct chip_data {
>  };
>  
>  #ifdef CONFIG_DEBUG_FS
> -#define SPI_REGS_BUFSIZE	1024
> -static ssize_t dw_spi_show_regs(struct file *file, char __user *user_buf,
> -		size_t count, loff_t *ppos)
> -{
> -	struct dw_spi *dws = file->private_data;
> -	char *buf;
> -	u32 len = 0;
> -	ssize_t ret;
> -
> -	buf = kzalloc(SPI_REGS_BUFSIZE, GFP_KERNEL);
> -	if (!buf)
> -		return 0;
> -
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"%s registers:\n", dev_name(&dws->master->dev));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"=================================\n");
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"CTRL0: \t\t0x%08x\n", dw_readl(dws, DW_SPI_CTRL0));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"CTRL1: \t\t0x%08x\n", dw_readl(dws, DW_SPI_CTRL1));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"SSIENR: \t0x%08x\n", dw_readl(dws, DW_SPI_SSIENR));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"SER: \t\t0x%08x\n", dw_readl(dws, DW_SPI_SER));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"BAUDR: \t\t0x%08x\n", dw_readl(dws, DW_SPI_BAUDR));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"TXFTLR: \t0x%08x\n", dw_readl(dws, DW_SPI_TXFLTR));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"RXFTLR: \t0x%08x\n", dw_readl(dws, DW_SPI_RXFLTR));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"TXFLR: \t\t0x%08x\n", dw_readl(dws, DW_SPI_TXFLR));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"RXFLR: \t\t0x%08x\n", dw_readl(dws, DW_SPI_RXFLR));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"SR: \t\t0x%08x\n", dw_readl(dws, DW_SPI_SR));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"IMR: \t\t0x%08x\n", dw_readl(dws, DW_SPI_IMR));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"ISR: \t\t0x%08x\n", dw_readl(dws, DW_SPI_ISR));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"DMACR: \t\t0x%08x\n", dw_readl(dws, DW_SPI_DMACR));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"DMATDLR: \t0x%08x\n", dw_readl(dws, DW_SPI_DMATDLR));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"DMARDLR: \t0x%08x\n", dw_readl(dws, DW_SPI_DMARDLR));
> -	len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
> -			"=================================\n");
> -
> -	ret = simple_read_from_buffer(user_buf, count, ppos, buf, len);
> -	kfree(buf);
> -	return ret;
> +#define DW_SPI_DBGFS_REG(_name, _off)	\
> +{					\
> +	.name = _name,			\
> +	.offset = _off			\

Leave comma here.

>  }
>  
> -static const struct file_operations dw_spi_regs_ops = {
> -	.owner		= THIS_MODULE,
> -	.open		= simple_open,
> -	.read		= dw_spi_show_regs,
> -	.llseek		= default_llseek,
> +static const struct debugfs_reg32 dw_spi_dbgfs_regs[] = {
> +	DW_SPI_DBGFS_REG("CTRL0", DW_SPI_CTRL0),
> +	DW_SPI_DBGFS_REG("CTRL1", DW_SPI_CTRL1),
> +	DW_SPI_DBGFS_REG("SSIENR", DW_SPI_SSIENR),
> +	DW_SPI_DBGFS_REG("SER", DW_SPI_SER),
> +	DW_SPI_DBGFS_REG("BAUDR", DW_SPI_BAUDR),
> +	DW_SPI_DBGFS_REG("TXFTLR", DW_SPI_TXFLTR),
> +	DW_SPI_DBGFS_REG("RXFTLR", DW_SPI_RXFLTR),
> +	DW_SPI_DBGFS_REG("TXFLR", DW_SPI_TXFLR),
> +	DW_SPI_DBGFS_REG("RXFLR", DW_SPI_RXFLR),
> +	DW_SPI_DBGFS_REG("SR", DW_SPI_SR),
> +	DW_SPI_DBGFS_REG("IMR", DW_SPI_IMR),
> +	DW_SPI_DBGFS_REG("ISR", DW_SPI_ISR),
> +	DW_SPI_DBGFS_REG("DMACR", DW_SPI_DMACR),
> +	DW_SPI_DBGFS_REG("DMATDLR", DW_SPI_DMATDLR),
> +	DW_SPI_DBGFS_REG("DMARDLR", DW_SPI_DMARDLR)
>  };
>  
>  static int dw_spi_debugfs_init(struct dw_spi *dws)
> @@ -103,8 +65,11 @@ static int dw_spi_debugfs_init(struct dw_spi *dws)
>  	if (!dws->debugfs)
>  		return -ENOMEM;
>  
> -	debugfs_create_file("registers", S_IFREG | S_IRUGO,
> -		dws->debugfs, (void *)dws, &dw_spi_regs_ops);
> +	dws->regset.regs = dw_spi_dbgfs_regs;
> +	dws->regset.nregs = ARRAY_SIZE(dw_spi_dbgfs_regs);
> +	dws->regset.base = dws->regs;
> +	debugfs_create_regset32("registers", 0400, dws->debugfs, &dws->regset);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index a6c03ea9013c..bad0eca2efe6 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -4,6 +4,7 @@
>  
>  #include <linux/io.h>
>  #include <linux/scatterlist.h>
> +#include <linux/debugfs.h>

Perhaps keep ordered?

>  
>  /* Register offsets */
>  #define DW_SPI_CTRL0			0x00
> @@ -142,6 +143,7 @@ struct dw_spi {
>  	void			*priv;
>  #ifdef CONFIG_DEBUG_FS
>  	struct dentry *debugfs;

> +	struct debugfs_regset32 regset;

I'm wondering why we need it here and not simple on the stack?

>  #endif
>  };
>  
> -- 
> 2.25.1
>
Andy Shevchenko May 8, 2020, 7:33 p.m. UTC | #8
On Fri, May 08, 2020 at 04:29:25PM +0300, Serge Semin wrote:
> Baikal-T1 SoC provides DW DMA controller to perform low-speed peripherals
> Mem-to-Dev and Dev-to-Mem transaction. This is also applicable to the DW
> APB SSI devices embedded into the SoC. Currently this type DMA device is
> supported by the DW APB SPI driver only as a middle layer code for Intel
> MID PCI devices. Seeing the same code can be used for normal platform
> DW DMAC device we introduced a set of patches to fix it within this
> patchset.
> 
> First of all traditionally we replaced the legacy plain text-based dt-binding
> file with yaml-based one. Then we unpinned the Intel MID specific code from
> the generic DMA one and placed it into the spi-dw-pci.c driver, which was a
> better place for it anyway. Then we introduced a set of naming cleanups since
> the code was going to be used for generic DW DMAC device and DMAC usage
> alterations to handle the controller functionality in a generic way by the
> DW APB SSI MMIO driver as well. See the individual patches commit messages
> for details.
> 
> In addition we fixed a problem in the native chip-select method, which despite
> of multiple attempts to be fixed doesn't correctly perceive the SPI_CS_HIGH
> flag and the enable-argument.
> 
> Finally as a cherry on a cake we replaced the manually written DebugFS
> registers read method with a ready-to-use for the same purpose regset32
> DebugFS interface usage.

Thanks! Appreciate the series (some of the things I would like to have done
myself, but lack of time and no specific request from our hardware).

Though, I will wait for v2 rebased on top of spi/for-next and I will review the
rest of the patches (mostly those I haven't commented out).

> This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4:
> 0e698dfa2822 ("Linux 5.7-rc4")
> tag: v5.7-rc4

Perhaps --base will do the trick?

> 
> Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> 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: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>
> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>
> Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru>
> Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Allison Randal <allison@lohutok.net>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Gareth Williams <gareth.williams.jx@renesas.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: linux-spi@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> Serge Semin (17):
>   dt-bindings: spi: Convert DW SPI binding to DT schema
>   dt-bindings: spi: dw: Add DMA properties bindings
>   spi: dw: Split up the generic DMA code and Intel MID driver
>   spi: dw: Cleanup generic DW DMA code namings
>   spi: dw: Discard static DW DMA slave structures
>   spi: dw: Add DW SPI DMA/PCI/MMIO dependency on DW SPI core
>   spi: dw: Add Tx/Rx finish wait methods to DMA
>   spi: dw: Clear DMAC register when done or stopped
>   spi: dw: Enable interrupts in accordance with DMA xfer mode
>   spi: dw: Parameterize the DMA Rx/Tx burst length
>   spi: dw: Fix native CS being unset
>   spi: dw: Fix dma_slave_config used partly uninitialized
>   spi: dw: Initialize paddr in DW SPI MMIO private data
>   spi: dw: Add DMA support to the DW SPI MMIO driver
>   spi: dw: Use DMA max burst to set the request thresholds
>   spi: dw: Fix Rx-only DMA transfers
>   spi: dw: Use regset32 DebugFS method to create a registers file
> 
>  .../bindings/spi/snps,dw-apb-ssi.txt          |  41 ---
>  .../bindings/spi/snps,dw-apb-ssi.yaml         | 123 +++++++++
>  .../devicetree/bindings/spi/spi-dw.txt        |  24 --
>  drivers/spi/Kconfig                           |  16 +-
>  drivers/spi/Makefile                          |   4 +-
>  drivers/spi/{spi-dw-mid.c => spi-dw-dma.c}    | 237 ++++++++++++------
>  drivers/spi/spi-dw-mmio.c                     |  15 +-
>  drivers/spi/spi-dw-pci.c                      |  38 ++-
>  drivers/spi/spi-dw.c                          |  89 +++----
>  drivers/spi/spi-dw.h                          |  27 +-
>  10 files changed, 402 insertions(+), 212 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
>  create mode 100644 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>  delete mode 100644 Documentation/devicetree/bindings/spi/spi-dw.txt
>  rename drivers/spi/{spi-dw-mid.c => spi-dw-dma.c} (53%)
> 
> -- 
> 2.25.1
>
Linus Walleij May 8, 2020, 7:39 p.m. UTC | #9
On Fri, May 8, 2020 at 3:31 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:

> Commit 6e0a32d6f376 ("spi: dw: Fix default polarity of native
> chipselect") attempted to fix the problem when GPIO active-high
> chip-select is utilized to communicate with some SPI slave. It fixed
> the problem, but broke the normal native CS support. At the same time
> the reversion commit ada9e3fcc175 ("spi: dw: Correct handling of native
> chipselect") didn't solve the problem either, since it just inverted
> the set_cs() polarity perception without taking into account that
> CS-high might be applicable. Here is what is done to finally fix the
> problem.

I'm not sure this is the whole story.

I think Charles' fix made it work, and then commit
3e5ec1db8bfee845d9f8560d1c64aeaccd586398
"spi: Fix SPI_CS_HIGH setting when using native and GPIO CS"
fixed it broken again.

This commit will make sure only set SPI_CS_HIGH on a
spi_device if it is using a GPIO as CS. Before this change,
the core would set that on everything, and expect the
.set_cs() callback to cope.

I think we fixed that and that fix should have been undone
when applying commit 3e5ec1db8bfe.

So possibly Fixes: should be set only to this commit, so
that the fix is not backported to kernels without it.

> DW SPI controller demands any native CS being set in order to proceed
> with data transfer. So in order to activate the SPI communications we
> must set any bit in the Slave Select DW SPI controller register no
> matter whether the platform requests the GPIO- or native CS.

Ah-ha! Maybe we should even add a comment explaining that.
And that is why SPI_MASTER_GPIO_SS is set.

I suppose my naive understanding was:
"bit set to 1" = CS asserted (driven low)
"bit set to 0" = CS de-asserted  (driven high)
So that is not how this register works at all.

> This commit fixes the problem for all described cases. So no matter
> whether an SPI slave needs GPIO- or native-based CS with active-high
> or low signal the corresponding bit will be set in SER.

Makes sense.

>         struct dw_spi *dws = spi_controller_get_devdata(spi->controller);
>         struct chip_data *chip = spi_get_ctldata(spi);
> +       bool cs_high = !!(spi->mode & SPI_CS_HIGH);
>
>         /* Chip select logic is inverted from spi_set_cs() */
>         if (chip && chip->cs_control)
>                 chip->cs_control(!enable);
>
> -       if (!enable)
> +       if (cs_high == enable)
>                 dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));

This is the correct fix now but I an afraid not correct before
commit 3e5ec1db8bfe.

What I can't help but asking is: can the native chip select even
handle active high chip select if not backed by a GPIO?
Which register would set that polarity?

Yours,
Linus Walleij
Andy Shevchenko May 8, 2020, 7:41 p.m. UTC | #10
On Fri, May 08, 2020 at 04:29:28PM +0300, Serge Semin wrote:
> This is an initial preparation patch before adding the DW DMA support
> into the DW SPI MMIO driver. We need to unpin the DMA-specific code
> from the code intended to be used for Intel MID. This isn't that hard,
> since most part of the spi-dw-mid.c driver in fact implements a generic
> DW DMA interface for the DW SPI controller driver. The only Intel MID
> specifics concern getting the max frequency from the MRST Clock
> Control Unit and fetching the DMA controller channels from
> corresponding PCIe DMA controller. Since first one is related with the
> SPI interface configuration we moved it' implementation into the
> DW PCIe-SPI driver object. While seeing there is no other than Medfield
> board with DW DMA controller currently supported we left the DMA
> channels search procedure in the DW SPI DMA module. After being
> cleaned up of the Intel MID specifics former spi-dw-mid.c module
> can be just renamed to be the DW SPI DMA driver.

And I guess this already been done in spi/for-next in less invasive way.

> 
> Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> 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: Rob Herring <robh+dt@kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Allison Randal <allison@lohutok.net>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Gareth Williams <gareth.williams.jx@renesas.com>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> ---
>  drivers/spi/Kconfig                        |  8 ++---
>  drivers/spi/Makefile                       |  4 +--
>  drivers/spi/{spi-dw-mid.c => spi-dw-dma.c} | 36 ++------------------
>  drivers/spi/spi-dw-pci.c                   | 38 ++++++++++++++++++++--
>  drivers/spi/spi-dw.h                       | 12 +++++--
>  5 files changed, 55 insertions(+), 43 deletions(-)
>  rename drivers/spi/{spi-dw-mid.c => spi-dw-dma.c} (88%)
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 741b9140992a..9653c7f271e9 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -226,14 +226,14 @@ config SPI_DESIGNWARE
>  	help
>  	  general driver for SPI controller core from DesignWare
>  
> +config SPI_DW_DMA
> +	tristate "DMA support for DW SPI controller"
> +	depends on SPI_DESIGNWARE && DW_DMAC_PCI
> +
>  config SPI_DW_PCI
>  	tristate "PCI interface driver for DW SPI core"
>  	depends on SPI_DESIGNWARE && PCI
>  
> -config SPI_DW_MID_DMA
> -	bool "DMA support for DW SPI controller on Intel MID platform"
> -	depends on SPI_DW_PCI && DW_DMAC_PCI
> -
>  config SPI_DW_MMIO
>  	tristate "Memory-mapped io interface driver for DW SPI core"
>  	depends on SPI_DESIGNWARE
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 28f601327f8c..15eb760412a9 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -36,9 +36,9 @@ obj-$(CONFIG_SPI_COLDFIRE_QSPI)		+= spi-coldfire-qspi.o
>  obj-$(CONFIG_SPI_DAVINCI)		+= spi-davinci.o
>  obj-$(CONFIG_SPI_DLN2)			+= spi-dln2.o
>  obj-$(CONFIG_SPI_DESIGNWARE)		+= spi-dw.o
> +obj-$(CONFIG_SPI_DW_DMA)		+= spi-dw-dma.o
>  obj-$(CONFIG_SPI_DW_MMIO)		+= spi-dw-mmio.o
> -obj-$(CONFIG_SPI_DW_PCI)		+= spi-dw-midpci.o
> -spi-dw-midpci-objs			:= spi-dw-pci.o spi-dw-mid.o
> +obj-$(CONFIG_SPI_DW_PCI)		+= spi-dw-pci.o
>  obj-$(CONFIG_SPI_EFM32)			+= spi-efm32.o
>  obj-$(CONFIG_SPI_EP93XX)		+= spi-ep93xx.o
>  obj-$(CONFIG_SPI_FALCON)		+= spi-falcon.o
> diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-dma.c
> similarity index 88%
> rename from drivers/spi/spi-dw-mid.c
> rename to drivers/spi/spi-dw-dma.c
> index 0d86c37e0aeb..0230b4252611 100644
> --- a/drivers/spi/spi-dw-mid.c
> +++ b/drivers/spi/spi-dw-dma.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Special handling for DW core on Intel MID platform
> + * Special handling for DW DMA core
>   *
>   * Copyright (c) 2009, 2014 Intel Corporation.
>   */
> @@ -14,7 +14,6 @@
>  
>  #include "spi-dw.h"
>  
> -#ifdef CONFIG_SPI_DW_MID_DMA
>  #include <linux/pci.h>
>  #include <linux/platform_data/dma-dw.h>
>  
> @@ -283,40 +282,11 @@ static const struct dw_spi_dma_ops mid_dma_ops = {
>  	.dma_transfer	= mid_spi_dma_transfer,
>  	.dma_stop	= mid_spi_dma_stop,
>  };
> -#endif
>  
> -/* Some specific info for SPI0 controller on Intel MID */
> -
> -/* HW info for MRST Clk Control Unit, 32b reg per controller */
> -#define MRST_SPI_CLK_BASE	100000000	/* 100m */
> -#define MRST_CLK_SPI_REG	0xff11d86c
> -#define CLK_SPI_BDIV_OFFSET	0
> -#define CLK_SPI_BDIV_MASK	0x00000007
> -#define CLK_SPI_CDIV_OFFSET	9
> -#define CLK_SPI_CDIV_MASK	0x00000e00
> -#define CLK_SPI_DISABLE_OFFSET	8
> -
> -int dw_spi_mid_init(struct dw_spi *dws)
> +void dw_spi_pci_dma_setup(struct dw_spi *dws)
>  {
> -	void __iomem *clk_reg;
> -	u32 clk_cdiv;
> -
> -	clk_reg = ioremap(MRST_CLK_SPI_REG, 16);
> -	if (!clk_reg)
> -		return -ENOMEM;
> -
> -	/* Get SPI controller operating freq info */
> -	clk_cdiv = readl(clk_reg + dws->bus_num * sizeof(u32));
> -	clk_cdiv &= CLK_SPI_CDIV_MASK;
> -	clk_cdiv >>= CLK_SPI_CDIV_OFFSET;
> -	dws->max_freq = MRST_SPI_CLK_BASE / (clk_cdiv + 1);
> -
> -	iounmap(clk_reg);
> -
> -#ifdef CONFIG_SPI_DW_MID_DMA
>  	dws->dma_tx = &mid_dma_tx;
>  	dws->dma_rx = &mid_dma_rx;
>  	dws->dma_ops = &mid_dma_ops;
> -#endif
> -	return 0;
>  }
> +EXPORT_SYMBOL_GPL(dw_spi_pci_dma_setup);
> diff --git a/drivers/spi/spi-dw-pci.c b/drivers/spi/spi-dw-pci.c
> index 12c131b5fb4e..068f6897b903 100644
> --- a/drivers/spi/spi-dw-pci.c
> +++ b/drivers/spi/spi-dw-pci.c
> @@ -16,6 +16,17 @@
>  
>  #define DRIVER_NAME "dw_spi_pci"
>  
> +/* HW info for MRST Clk Control Unit, 32b reg per controller */
> +#define MRST_SPI_CLK_BASE	100000000	/* 100m */
> +#define MRST_CLK_SPI_REG	0xff11d86c
> +#define CLK_SPI_BDIV_OFFSET	0
> +#define CLK_SPI_BDIV_MASK	0x00000007
> +#define CLK_SPI_CDIV_OFFSET	9
> +#define CLK_SPI_CDIV_MASK	0x00000e00
> +#define CLK_SPI_DISABLE_OFFSET	8
> +
> +static int spi_mid_init(struct dw_spi *dws);
> +
>  struct spi_pci_desc {
>  	int	(*setup)(struct dw_spi *);
>  	u16	num_cs;
> @@ -24,13 +35,13 @@ struct spi_pci_desc {
>  };
>  
>  static struct spi_pci_desc spi_pci_mid_desc_1 = {
> -	.setup = dw_spi_mid_init,
> +	.setup = spi_mid_init,
>  	.num_cs = 5,
>  	.bus_num = 0,
>  };
>  
>  static struct spi_pci_desc spi_pci_mid_desc_2 = {
> -	.setup = dw_spi_mid_init,
> +	.setup = spi_mid_init,
>  	.num_cs = 2,
>  	.bus_num = 1,
>  };
> @@ -41,6 +52,29 @@ static struct spi_pci_desc spi_pci_ehl_desc = {
>  	.max_freq = 100000000,
>  };
>  
> +/* Some specific info for SPI0 controller on Intel MID */
> +static int spi_mid_init(struct dw_spi *dws)
> +{
> +	void __iomem *clk_reg;
> +	u32 clk_cdiv;
> +
> +	clk_reg = ioremap(MRST_CLK_SPI_REG, 16);
> +	if (!clk_reg)
> +		return -ENOMEM;
> +
> +	/* Get SPI controller operating freq info */
> +	clk_cdiv = readl(clk_reg + dws->bus_num * sizeof(u32));
> +	clk_cdiv &= CLK_SPI_CDIV_MASK;
> +	clk_cdiv >>= CLK_SPI_CDIV_OFFSET;
> +	dws->max_freq = MRST_SPI_CLK_BASE / (clk_cdiv + 1);
> +
> +	iounmap(clk_reg);
> +
> +	dw_spi_pci_dma_setup(dws);
> +
> +	return 0;
> +}
> +
>  static int spi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	struct dw_spi *dws;
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 1bf5713e047d..0a4e0890ef85 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -253,6 +253,14 @@ extern void dw_spi_remove_host(struct dw_spi *dws);
>  extern int dw_spi_suspend_host(struct dw_spi *dws);
>  extern int dw_spi_resume_host(struct dw_spi *dws);
>  
> -/* platform related setup */
> -extern int dw_spi_mid_init(struct dw_spi *dws); /* Intel MID platforms */
> +#ifdef CONFIG_SPI_DW_DMA
> +
> +extern void dw_spi_pci_dma_setup(struct dw_spi *dws);
> +
> +#else /* !CONFIG_SPI_DW_DMA */
> +
> +static inline void dw_spi_pci_dma_setup(struct dw_spi *dws) {}
> +
> +#endif /* !CONFIG_SPI_DW_DMA */
> +
>  #endif /* DW_SPI_HEADER_H */
> -- 
> 2.25.1
>
Andy Shevchenko May 8, 2020, 7:44 p.m. UTC | #11
On Fri, May 08, 2020 at 04:29:30PM +0300, Serge Semin wrote:
> Having them declared is redundant since each struct dw_dma_chan has
> the same structure embedded and the structure from the passed dma_chan
> private pointer will be copied there as a result of the next calls
> chain:
> dma_request_channel() -> find_candidate() -> dma_chan_get() ->
> device_alloc_chan_resources() = dwc_alloc_chan_resources() ->
> dw_dma_filter().
> So just remove the static dw_dma_chan structures and use a locally
> declared data instance with dst_id/src_id set to the same values as
> the static copies used to have.

I'm not against it, but you may leave if for the future (see spi/for-next).
Serge Semin May 12, 2020, 8:07 p.m. UTC | #12
On Fri, May 08, 2020 at 02:33:36PM +0100, Mark Brown wrote:
> On Fri, May 08, 2020 at 04:29:25PM +0300, Serge Semin wrote:
> 
> > Serge Semin (17):
> >   dt-bindings: spi: Convert DW SPI binding to DT schema
> 
> Please don't make new feature development dependent on conversion to the
> new schema format, there's quite a backlog of reviews of schema
> conversions so it can slow things down.  It's good to do the conversions
> but please do them after adding any new stuff to the binding rather than
> before.

So by saying this do you want me to revert an order of the first two patches
in the series, right? So the series would first add the DMA properties support
to the binding, then would convert the binding file to DT schema.

-Sergey
Serge Semin May 12, 2020, 8:30 p.m. UTC | #13
On Fri, May 08, 2020 at 06:36:09PM +0100, Mark Brown wrote:
> On Fri, May 08, 2020 at 04:29:25PM +0300, Serge Semin wrote:
> > Baikal-T1 SoC provides DW DMA controller to perform low-speed peripherals
> > Mem-to-Dev and Dev-to-Mem transaction. This is also applicable to the DW
> > APB SSI devices embedded into the SoC. Currently this type DMA device is
> 
> This basically all looks good to me (without any hardware specific
> knowledge), I had a few comments but they were mostly procedural ones -
> mainly getting these bug fixes you've done merged as such.  Nice work.

Thanks. I'll address the Andy's comments and send v2 after that.

-Sergey
Serge Semin May 12, 2020, 8:34 p.m. UTC | #14
On Fri, May 08, 2020 at 10:16:17PM +0300, Andy Shevchenko wrote:
> On Fri, May 08, 2020 at 06:36:09PM +0100, Mark Brown wrote:
> > On Fri, May 08, 2020 at 04:29:25PM +0300, Serge Semin wrote:
> > > Baikal-T1 SoC provides DW DMA controller to perform low-speed peripherals
> > > Mem-to-Dev and Dev-to-Mem transaction. This is also applicable to the DW
> > > APB SSI devices embedded into the SoC. Currently this type DMA device is
> > 
> > This basically all looks good to me (without any hardware specific
> > knowledge), I had a few comments but they were mostly procedural ones -
> > mainly getting these bug fixes you've done merged as such.  Nice work.
> 
> Agree that is nice work!
> Though, can you please give us chance to review next version of the series and test on our hardware?
> 
> Serge, I think you should base it on spi/for-next rather than vanilla.

Yeah, seeing there are potentially merge-conflict changes in the for-next
branch I have to do the rebase. Will be done in v2.

-Sergey

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Mark Brown May 13, 2020, 10:23 a.m. UTC | #15
On Tue, May 12, 2020 at 11:07:33PM +0300, Serge Semin wrote:
> On Fri, May 08, 2020 at 02:33:36PM +0100, Mark Brown wrote:

> > Please don't make new feature development dependent on conversion to the
> > new schema format, there's quite a backlog of reviews of schema
> > conversions so it can slow things down.  It's good to do the conversions
> > but please do them after adding any new stuff to the binding rather than
> > before.

> So by saying this do you want me to revert an order of the first two patches
> in the series, right? So the series would first add the DMA properties support
> to the binding, then would convert the binding file to DT schema.

The conversion to YAML format should be the very last thing in the patch
series, and as Andy says there's another patch in flight also doing this
conversion which you should coordinate with.
Serge Semin May 13, 2020, 11:04 a.m. UTC | #16
On Wed, May 13, 2020 at 11:23:24AM +0100, Mark Brown wrote:
> On Tue, May 12, 2020 at 11:07:33PM +0300, Serge Semin wrote:
> > On Fri, May 08, 2020 at 02:33:36PM +0100, Mark Brown wrote:
> 
> > > Please don't make new feature development dependent on conversion to the
> > > new schema format, there's quite a backlog of reviews of schema
> > > conversions so it can slow things down.  It's good to do the conversions
> > > but please do them after adding any new stuff to the binding rather than
> > > before.
> 
> > So by saying this do you want me to revert an order of the first two patches
> > in the series, right? So the series would first add the DMA properties support
> > to the binding, then would convert the binding file to DT schema.
> 
> The conversion to YAML format should be the very last thing in the patch
> series,

Hm, haven't heard about this requirement. Could you point me out to a doc or
some discussion concerning this for future reference? It's not a first DT
conversion patch I've submitted and non of them were addressed with such
request. I do understand that the order of DT concerning patches can be
important and agree to fix it by updating the original legacy binding first,
then perform a conversion. But placing the conversion in a tail of the series
just seems unnecessary. The patch can be dropped from any place of the series
if for some reason Rob would be late with review.

Personally I prefer placing all DT changes in the head of the series, so Rob
wouldn't need to search through the whole patchset looking for the DT-related
patches.

-Sergey

> and as Andy says there's another patch in flight also doing this
> conversion which you should coordinate with.
Mark Brown May 13, 2020, 11:21 a.m. UTC | #17
On Wed, May 13, 2020 at 02:04:07PM +0300, Serge Semin wrote:
> On Wed, May 13, 2020 at 11:23:24AM +0100, Mark Brown wrote:

> > The conversion to YAML format should be the very last thing in the patch
> > series,

> Hm, haven't heard about this requirement. Could you point me out to a doc or
> some discussion concerning this for future reference? It's not a first DT
> conversion patch I've submitted and non of them were addressed with such
> request. I do understand that the order of DT concerning patches can be
> important and agree to fix it by updating the original legacy binding first,
> then perform a conversion. But placing the conversion in a tail of the series
> just seems unnecessary. The patch can be dropped from any place of the series
> if for some reason Rob would be late with review.

This is a practical observation based on the fact that there is a huge
backlog of reviews of DT binding conversions and that those conversions
typically go through several review cycles and that not everyone who's
sending patches to the kernel is fully up to speed on processes or has
strong English.  By telling people (including other people who find
instructions on the list) to put the conversion right at the end of the
series I am avoiding any ambiguity or confusion about ordering with
regard to any other patches to the DT, including any new patches that
get added to the series.

> Personally I prefer placing all DT changes in the head of the series, so Rob
> wouldn't need to search through the whole patchset looking for the DT-related
> patches.

Ideally the YAML conversions would be done entirely separately to other
development rather than as part of a bigger series, they're pretty much
orthogonal anyway.  Sadly there's obvious content collisions with any
new development that adds DT stuff so that's not always the most
practical thing.
Mark Brown May 13, 2020, 11:36 a.m. UTC | #18
On Wed, May 13, 2020 at 02:35:55PM +0300, Serge Semin wrote:
> On Fri, May 08, 2020 at 06:30:23PM +0100, Mark Brown wrote:

> > > +	while (dw_spi_dma_tx_busy(dws) && retry--)
> > > +		ndelay(ns);

> > How deep can the FIFO be with this IP - could we end up ndelay()ing for
> > non-trivial amounts of time?

> According to the DW APB SSI db it can be up to 256 transfer words. So the delay
> should be trivial.

Yes, that should be fine.
Serge Semin May 13, 2020, 11:42 a.m. UTC | #19
On Wed, May 13, 2020 at 12:21:16PM +0100, Mark Brown wrote:
> On Wed, May 13, 2020 at 02:04:07PM +0300, Serge Semin wrote:
> > On Wed, May 13, 2020 at 11:23:24AM +0100, Mark Brown wrote:
> 
> > > The conversion to YAML format should be the very last thing in the patch
> > > series,
> 
> > Hm, haven't heard about this requirement. Could you point me out to a doc or
> > some discussion concerning this for future reference? It's not a first DT
> > conversion patch I've submitted and non of them were addressed with such
> > request. I do understand that the order of DT concerning patches can be
> > important and agree to fix it by updating the original legacy binding first,
> > then perform a conversion. But placing the conversion in a tail of the series
> > just seems unnecessary. The patch can be dropped from any place of the series
> > if for some reason Rob would be late with review.
> 
> This is a practical observation based on the fact that there is a huge
> backlog of reviews of DT binding conversions and that those conversions
> typically go through several review cycles and that not everyone who's
> sending patches to the kernel is fully up to speed on processes or has
> strong English.  By telling people (including other people who find
> instructions on the list) to put the conversion right at the end of the
> series I am avoiding any ambiguity or confusion about ordering with
> regard to any other patches to the DT, including any new patches that
> get added to the series.
> 
> > Personally I prefer placing all DT changes in the head of the series, so Rob
> > wouldn't need to search through the whole patchset looking for the DT-related
> > patches.
> 
> Ideally the YAML conversions would be done entirely separately to other
> development rather than as part of a bigger series, they're pretty much
> orthogonal anyway.  Sadly there's obvious content collisions with any
> new development that adds DT stuff so that's not always the most
> practical thing.

Ok. I see your point. I'll move the conversion patch to the tail of the series
after rebasing the patchset on top of the spi/for-next branch. Thanks for
clarification.

-Sergey
Andy Shevchenko May 13, 2020, 2:41 p.m. UTC | #20
On Wed, May 13, 2020 at 03:44:22PM +0300, Serge Semin wrote:
> On Fri, May 08, 2020 at 10:30:27PM +0300, Andy Shevchenko wrote:
> > On Fri, May 08, 2020 at 04:29:42PM +0300, Serge Semin wrote:
> > > DebugFS kernel interface provides a dedicated method to create the
> > > registers dump file. Use it instead of creating a generic DebugFS
> > > file with manually written read callback function.

> > > +#define DW_SPI_DBGFS_REG(_name, _off)	\
> > > +{					\
> > > +	.name = _name,			\
> > > +	.offset = _off			\
> > 
> > Leave comma here.
> 
> don't see a point.

It will help in case if this getting extended. Also slightly better to
distinguish between terminator type of members vs. data structures.

> > >  }

> > > +	struct debugfs_regset32 regset;
> > 
> > I'm wondering why we need it here and not simple on the stack?
> 
> Please see the way the DebugFS regset work. A prompt: how does the DebugFS
> core get to know what is a base address of the registers? =)

If they have a member in the struct which passed thru private pointer of inode.
But I see your point.
Linus Walleij May 14, 2020, 8:31 a.m. UTC | #21
On Wed, May 13, 2020 at 2:13 AM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:

> > This is the correct fix now but I an afraid not correct before
> > commit 3e5ec1db8bfe.
>
> Sorry, but that's "enable" flag propagation from basic spi_set_cs() to the HW CS
> setting callback is a nightmare. In Russia there is a common saying for such
> cases, which can be translated as "you can't figure it out without a bottle of
> vodka".)
>
> Actually the fix is correct no matter whether commit 3e5ec1db8bfe is applied or
> not. At least I don't see a connection between them.

OK that seems to hold given the resoning below so:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

> > What I can't help but asking is: can the native chip select even
> > handle active high chip select if not backed by a GPIO?
> > Which register would set that polarity?
>
> No. DW APB SSI doesn't support active-high mode of the native CS's.

We had some related discussion what to do with this case
when a controller can support active high CS if and only if
it is using a GPIO instead of the native CS. We didn't really
figure it out, I suppose ideally we should use two flags in the
master but that exercise is for another day.

Yours.
Linus Walleij
Andy Shevchenko May 14, 2020, 12:22 p.m. UTC | #22
On Thu, May 14, 2020 at 02:55:58PM +0300, Serge Semin wrote:
> On Thu, May 14, 2020 at 10:31:13AM +0200, Linus Walleij wrote:
> > On Wed, May 13, 2020 at 2:13 AM Serge Semin
> > <Sergey.Semin@baikalelectronics.ru> wrote:

> BTW I experience a problem with vger.kernel.org. For some reason a few days ago
> it started bouncing my emails back sent to the GPIO/MIPS/SPI/kernel mailing lists.
> I've sent multiple backward messages to the postmaster (postmaster (dog) vger.kernel.org)
> with the bounce text, but still with no response. Could you tell me who should I
> bother with this problem to get a help with its solution? 

Perhaps, helpdesk@kernel.org ?
Mark Brown May 14, 2020, 1:03 p.m. UTC | #23
On Thu, May 14, 2020 at 03:22:14PM +0300, Andy Shevchenko wrote:
> On Thu, May 14, 2020 at 02:55:58PM +0300, Serge Semin wrote:

> > BTW I experience a problem with vger.kernel.org. For some reason a few days ago
> > it started bouncing my emails back sent to the GPIO/MIPS/SPI/kernel mailing lists.
> > I've sent multiple backward messages to the postmaster (postmaster (dog) vger.kernel.org)
> > with the bounce text, but still with no response. Could you tell me who should I
> > bother with this problem to get a help with its solution? 

> Perhaps, helpdesk@kernel.org ?

vger is completely separate to kernel.org in terms of admin.  Usually
the bounce messages from vger say why things are being bounced, there's
a good chance that either mail to you is bouncing and Dave got annoyed
with you and is trying to get your attention or something you're doing
is looking like spam.
Mark Brown May 14, 2020, 2:35 p.m. UTC | #24
On Thu, May 14, 2020 at 02:55:58PM +0300, Serge Semin wrote:
> On Thu, May 14, 2020 at 10:31:13AM +0200, Linus Walleij wrote:

> > We had some related discussion what to do with this case
> > when a controller can support active high CS if and only if
> > it is using a GPIO instead of the native CS. We didn't really
> > figure it out, I suppose ideally we should use two flags in the
> > master but that exercise is for another day.

> Even though it might be painful, but as I see it the best way to generically fix
> this problem would be to change the controller->set_cs() callback
> semantics. SPI core should pass a CS activation flag to the set_cs()
> callback instead of the CS pin logical level (just propagate the enable argument
> passed to the spi_set_cs() SPI core method). So if an SPI controller supports
> the Active-high native CS, during the set_cs() callback invocation it would
> analyze the spi_device flags state to figure out whether the slave needs the

The idea with set_cs() is to support controllers that allow the chip
select to be directly managed.  If the controller is interpreting or
automatically managing chip select at all then set_cs() is not likely to
be a good fit, if the controller does support unfiltered management then
anything else is going to result in there being a bunch of duplicate
code between drivers.