[PATCH 0/3] DW apb_ssi V4 support for Kendryte K210 RISC-V SoC

[PATCH 0/3] DW apb_ssi V4 support for Kendryte K210 RISC-V SoC

[Damien Le Moal]

The Canaan Kendryte K210 RISC-V SoC includes a DesignWare apb_ssi V4
SPI controller implemented with a maximum data frame size of 32-bits
(SSI_MAX_XFER_SIZE=32 synthesis parameter).

This series of patches adds support for this SoC by adding implementing
changes necessary to support the 32-bits xfer size configuration. This
is done in patch 1.

Patch 2 introduces a workaround for a HW bug on this SoC which triggers
RX FIFO overrun errors when the RX FIFO fills up to its maximum detected
depth of 32. The patch manually reduces the fifo depth to 31.

Patch 3 documents the new compatible string "canaan,k210-spi" used to
identify this SoC.

Damien Le Moal (3):
  spi: dw: Add support for 32-bits max xfer size
  spi: dw: Add support for the Canaan K210 SoC SPI
  dt-bindings: Update Synopsis DW apb ssi bindings

 .../bindings/spi/snps,dw-apb-ssi.yaml         |  2 +
 drivers/spi/spi-dw-core.c                     | 40 +++++++++++++++----
 drivers/spi/spi-dw-mmio.c                     | 16 ++++++++
 drivers/spi/spi-dw.h                          |  8 ++++
 4 files changed, 59 insertions(+), 7 deletions(-)

-- 
2.28.0

[PATCH 1/3] spi: dw: Add support for 32-bits max xfer size

[Damien Le Moal]

Synopsis DesignWare DW_apb_ssi version 3.23 onward defines a 32-bits
maximum transfer size synthesis parameter (SSI_MAX_XFER_SIZE=32) in
addition to the legacy 16-bits configuration (SSI_MAX_XFER_SIZE=16) for
SPI controllers.  When SSI_MAX_XFER_SIZE=32, the layout of the ctrlr0
register changes and RX/TX FIFO words can be up to 32-bits.

The layout of ctrlr0 change for the 32-bits configuration moves the
data frame format field to bits 16..20 instead of bits 3..0
when the controller is configured with SSI_MAX_XFER_SIZE=16.

Introduce the DW SPI capability flag DW_SPI_CAP_DWC_APB_XFER32 to
indicate in that the controller is configured with SSI_MAX_XFER_SIZE=32.
Since the SSI_MAX_XFER_SIZE parameter is a controller synthesis
parameter not accessible through a register, the detection of this
parameter value is done in spi_hw_init() by writing and reading the
ctrlr0 register and testing the value of bits 20..16.

The DW_SPI_CAP_DWC_APB_XFER32 flag is used in dw_spi_update_config() to
set the data frame size field at the correct position and in
dw_spi_add_host() to extend bits_per_word_mask to 32-bits.
dw_reader() and dw_writer() are also modified to handle 32-bits FIFO
words.

Suggested-by: Sean Anderson <seanga2@gmail.com>
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/spi/spi-dw-core.c | 40 ++++++++++++++++++++++++++++++++-------
 drivers/spi/spi-dw.h      |  8 ++++++++
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 2e50cc0a9291..4c16832b16fd 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -137,14 +137,16 @@ static inline u32 rx_max(struct dw_spi *dws)
 static void dw_writer(struct dw_spi *dws)
 {
 	u32 max = tx_max(dws);
-	u16 txw = 0;
+	u32 txw = 0;
 
 	while (max--) {
 		if (dws->tx) {
 			if (dws->n_bytes == 1)
 				txw = *(u8 *)(dws->tx);
-			else
+			else if (dws->n_bytes == 2)
 				txw = *(u16 *)(dws->tx);
+			else
+				txw = *(u32 *)(dws->tx);
 
 			dws->tx += dws->n_bytes;
 		}
@@ -156,15 +158,17 @@ static void dw_writer(struct dw_spi *dws)
 static void dw_reader(struct dw_spi *dws)
 {
 	u32 max = rx_max(dws);
-	u16 rxw;
+	u32 rxw;
 
 	while (max--) {
 		rxw = dw_read_io_reg(dws, DW_SPI_DR);
 		if (dws->rx) {
 			if (dws->n_bytes == 1)
 				*(u8 *)(dws->rx) = rxw;
-			else
+			else if (dws->n_bytes == 2)
 				*(u16 *)(dws->rx) = rxw;
+			else
+				*(u32 *)(dws->rx) = rxw;
 
 			dws->rx += dws->n_bytes;
 		}
@@ -311,8 +315,12 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
 	u32 speed_hz;
 	u16 clk_div;
 
-	/* CTRLR0[ 4/3: 0] Data Frame Size */
-	cr0 |= (cfg->dfs - 1);
+	if (!(dws->caps & DW_SPI_CAP_DWC_APB_XFER32))
+		/* CTRLR0[ 4/3: 0] Data Frame Size */
+		cr0 |= (cfg->dfs - 1);
+	else
+		/* CTRLR0[20: 16] Data Frame Size */
+		cr0 |= FIELD_PREP(DWC_APB_CTRLR0_DFS32_MASK, cfg->dfs - 1);
 
 	if (!(dws->caps & DW_SPI_CAP_DWC_SSI))
 		/* CTRLR0[ 9:8] Transfer Mode */
@@ -828,6 +836,21 @@ static void spi_hw_init(struct device *dev, struct dw_spi *dws)
 		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
 	}
 
+	if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) {
+		u32 cr0;
+
+		/*
+		 * Detect APB SSI CTRLR0 size by looking at the data
+		 * frame size field.
+		 */
+		dw_writel(dws, DW_SPI_CTRLR0, 0xffffffff);
+		cr0 = dw_readl(dws, DW_SPI_CTRLR0);
+		if (FIELD_GET(DWC_APB_CTRLR0_DFS32_MASK, cr0)) {
+			dev_dbg(dev, "Detected 32-bits max data frame size\n");
+			dws->caps |= DW_SPI_CAP_DWC_APB_XFER32;
+		}
+	}
+
 	/* enable HW fixup for explicit CS deselect for Amazon's alpine chip */
 	if (dws->caps & DW_SPI_CAP_CS_OVERRIDE)
 		dw_writel(dws, DW_SPI_CS_OVERRIDE, 0xF);
@@ -864,7 +887,10 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
 
 	master->use_gpio_descriptors = true;
 	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP;
-	master->bits_per_word_mask =  SPI_BPW_RANGE_MASK(4, 16);
+	if (dws->caps & DW_SPI_CAP_DWC_APB_XFER32)
+		master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
+	else
+		master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 16);
 	master->bus_num = dws->bus_num;
 	master->num_chipselect = dws->num_cs;
 	master->setup = dw_spi_setup;
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index faf40cb66498..cba5c1f8c456 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -9,6 +9,7 @@
 #include <linux/io.h>
 #include <linux/scatterlist.h>
 #include <linux/spi/spi-mem.h>
+#include <linux/bitfield.h>
 
 /* Register offsets */
 #define DW_SPI_CTRLR0			0x00
@@ -72,6 +73,12 @@
 #define DWC_SSI_CTRLR0_FRF_OFFSET	6
 #define DWC_SSI_CTRLR0_DFS_OFFSET	0
 
+/*
+ * Data frame size bits mask in CTRLR0 for DWC_apb_ssi v4 when the
+ * controller capability supports DW_SPI_CAP_DWC_APB_XFER32.
+ */
+#define DWC_APB_CTRLR0_DFS32_MASK	GENMASK(20, 16)
+
 /*
  * For Keem Bay, CTRLR0[31] is used to select controller mode.
  * 0: SSI is slave
@@ -121,6 +128,7 @@ enum dw_ssi_type {
 #define DW_SPI_CAP_CS_OVERRIDE		BIT(0)
 #define DW_SPI_CAP_KEEMBAY_MST		BIT(1)
 #define DW_SPI_CAP_DWC_SSI		BIT(2)
+#define DW_SPI_CAP_DWC_APB_XFER32	BIT(3)
 
 /* Slave spi_transfer/spi_mem_op related */
 struct dw_spi_cfg {
-- 
2.28.0

[PATCH 2/3] spi: dw: Add support for the Canaan K210 SoC SPI

[Damien Le Moal]

The Canaan Kendryte K210 RISC-V SoC includes a DW apb_ssi v4 controller
which is documented to have a 32 words deep TX and RX FIFO. The FIFO
length detection in spi_hw_init() correctly detects this value.
However, when the controller RX FIFO is filled up to 32 entries
(RXFLR = 32), an RX FIFO overrun error occurs. This likely due to a
hardware bug which can be avoided by force setting the fifo_len field of
struct dw_spi to 31.

Define the dw_spi_canaan_k210_init() function to force set fifo_len to
31 when the device node compatible string is "canaan,k210-spi".

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/spi/spi-dw-mmio.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index d0cc5bf4fa4e..17c06039a74d 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -222,6 +222,21 @@ static int dw_spi_keembay_init(struct platform_device *pdev,
 	return 0;
 }
 
+static int dw_spi_canaan_k210_init(struct platform_device *pdev,
+				   struct dw_spi_mmio *dwsmmio)
+{
+	/*
+	 * The Canaan Kendryte K210 SoC DW apb_ssi v4 spi controller is
+	 * documented to have a 32 word deep TX and RX FIFO, which
+	 * spi_hw_init() detects. However, when the RX FIFO is filled up to
+	 * 32 entries (RXFLR = 32), an RX FIFO overrun error occurs. Avoid this
+	 * problem by force setting fifo_len to 31.
+	 */
+	dwsmmio->dws.fifo_len = 31;
+
+	return 0;
+}
+
 static int dw_spi_mmio_probe(struct platform_device *pdev)
 {
 	int (*init_func)(struct platform_device *pdev,
@@ -335,6 +350,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
 	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
 	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
 	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
+	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
 	{ /* end of table */}
 };
 MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
-- 
2.28.0

[PATCH 3/3] dt-bindings: Update Synopsis DW apb ssi bindings

[Damien Le Moal]

Update the snps,dw-apb-ssi.yaml document to include the compatibility
string "canaan,k210-spi" compatible string for the Canaan Kendryte K210
RISC-V SoC DW apb_ssi V4 SPI controller.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
index 99ed9b416e94..4825157cd92e 100644
--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
@@ -65,6 +65,8 @@ properties:
         const: baikal,bt1-ssi
       - description: Baikal-T1 System Boot SPI Controller
         const: baikal,bt1-sys-ssi
+      - description: Canaan Kendryte K210 SoS SPI Controller
+        const: canaan,k210-spi
 
   reg:
     minItems: 1
-- 
2.28.0

Re: [PATCH 3/3] dt-bindings: Update Synopsis DW apb ssi bindings

[Serge Semin]

Hello Damien,

> [PATCH 3/3] dt-bindings: Update Synopsis DW apb ssi bindings

The patch subject is too uncertain. There is no subsystem name and
what is added to the bindings. I'd suggest something like:
"dt-bindings: spi: dw-apb-ssi: Add Canaan K210 SPI controller"

Then feel free to add
Acked-by: Serge Semin <fancer.lancer@gmail.com>

-Sergey

On Thu, Nov 19, 2020 at 09:02:53PM +0900, Damien Le Moal wrote:
> Update the snps,dw-apb-ssi.yaml document to include the compatibility
> string "canaan,k210-spi" compatible string for the Canaan Kendryte K210
> RISC-V SoC DW apb_ssi V4 SPI controller.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> index 99ed9b416e94..4825157cd92e 100644
> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> @@ -65,6 +65,8 @@ properties:
>          const: baikal,bt1-ssi
>        - description: Baikal-T1 System Boot SPI Controller
>          const: baikal,bt1-sys-ssi
> +      - description: Canaan Kendryte K210 SoS SPI Controller
> +        const: canaan,k210-spi
>  
>    reg:
>      minItems: 1
> -- 
> 2.28.0
> 

Re: [PATCH 2/3] spi: dw: Add support for the Canaan K210 SoC SPI

[Serge Semin]

On Thu, Nov 19, 2020 at 09:02:52PM +0900, Damien Le Moal wrote:
> The Canaan Kendryte K210 RISC-V SoC includes a DW apb_ssi v4 controller
> which is documented to have a 32 words deep TX and RX FIFO. The FIFO
> length detection in spi_hw_init() correctly detects this value.
> However, when the controller RX FIFO is filled up to 32 entries
> (RXFLR = 32), an RX FIFO overrun error occurs. This likely due to a
> hardware bug which can be avoided by force setting the fifo_len field of
> struct dw_spi to 31.
> 
> Define the dw_spi_canaan_k210_init() function to force set fifo_len to
> 31 when the device node compatible string is "canaan,k210-spi".

Looking good. Thanks.
Acked-by: Serge Semin <fancer.lancer@gmail.com>

> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/spi/spi-dw-mmio.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> index d0cc5bf4fa4e..17c06039a74d 100644
> --- a/drivers/spi/spi-dw-mmio.c
> +++ b/drivers/spi/spi-dw-mmio.c
> @@ -222,6 +222,21 @@ static int dw_spi_keembay_init(struct platform_device *pdev,
>  	return 0;
>  }
>  
> +static int dw_spi_canaan_k210_init(struct platform_device *pdev,
> +				   struct dw_spi_mmio *dwsmmio)
> +{
> +	/*
> +	 * The Canaan Kendryte K210 SoC DW apb_ssi v4 spi controller is
> +	 * documented to have a 32 word deep TX and RX FIFO, which
> +	 * spi_hw_init() detects. However, when the RX FIFO is filled up to
> +	 * 32 entries (RXFLR = 32), an RX FIFO overrun error occurs. Avoid this
> +	 * problem by force setting fifo_len to 31.
> +	 */
> +	dwsmmio->dws.fifo_len = 31;
> +
> +	return 0;
> +}
> +
>  static int dw_spi_mmio_probe(struct platform_device *pdev)
>  {
>  	int (*init_func)(struct platform_device *pdev,
> @@ -335,6 +350,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
>  	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
>  	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
>  	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
> +	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
>  	{ /* end of table */}
>  };
>  MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
> -- 
> 2.28.0
> 

Re: [PATCH 1/3] spi: dw: Add support for 32-bits max xfer size

[Serge Semin]

Damien,

Thanks for sending the patch. My comments are below.

On Thu, Nov 19, 2020 at 09:02:51PM +0900, Damien Le Moal wrote:
> Synopsis DesignWare DW_apb_ssi version 3.23 onward defines a 32-bits
> maximum transfer size synthesis parameter (SSI_MAX_XFER_SIZE=32) in
> addition to the legacy 16-bits configuration (SSI_MAX_XFER_SIZE=16) for
> SPI controllers.  When SSI_MAX_XFER_SIZE=32, the layout of the ctrlr0
> register changes and RX/TX FIFO words can be up to 32-bits.
> 
> The layout of ctrlr0 change for the 32-bits configuration moves the
> data frame format field to bits 16..20 instead of bits 3..0
> when the controller is configured with SSI_MAX_XFER_SIZE=16.
> 
> Introduce the DW SPI capability flag DW_SPI_CAP_DWC_APB_XFER32 to
> indicate in that the controller is configured with SSI_MAX_XFER_SIZE=32.
> Since the SSI_MAX_XFER_SIZE parameter is a controller synthesis
> parameter not accessible through a register, the detection of this
> parameter value is done in spi_hw_init() by writing and reading the
> ctrlr0 register and testing the value of bits 20..16.
> 
> The DW_SPI_CAP_DWC_APB_XFER32 flag is used in dw_spi_update_config() to
> set the data frame size field at the correct position and in
> dw_spi_add_host() to extend bits_per_word_mask to 32-bits.
> dw_reader() and dw_writer() are also modified to handle 32-bits FIFO
> words.
> 
> Suggested-by: Sean Anderson <seanga2@gmail.com>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/spi/spi-dw-core.c | 40 ++++++++++++++++++++++++++++++++-------
>  drivers/spi/spi-dw.h      |  8 ++++++++
>  2 files changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 2e50cc0a9291..4c16832b16fd 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -137,14 +137,16 @@ static inline u32 rx_max(struct dw_spi *dws)
>  static void dw_writer(struct dw_spi *dws)
>  {
>  	u32 max = tx_max(dws);
> -	u16 txw = 0;
> +	u32 txw = 0;
>  
>  	while (max--) {
>  		if (dws->tx) {
>  			if (dws->n_bytes == 1)
>  				txw = *(u8 *)(dws->tx);
> -			else
> +			else if (dws->n_bytes == 2)
>  				txw = *(u16 *)(dws->tx);
> +			else
> +				txw = *(u32 *)(dws->tx);
>  
>  			dws->tx += dws->n_bytes;
>  		}
> @@ -156,15 +158,17 @@ static void dw_writer(struct dw_spi *dws)
>  static void dw_reader(struct dw_spi *dws)
>  {
>  	u32 max = rx_max(dws);
> -	u16 rxw;
> +	u32 rxw;
>  
>  	while (max--) {
>  		rxw = dw_read_io_reg(dws, DW_SPI_DR);
>  		if (dws->rx) {
>  			if (dws->n_bytes == 1)
>  				*(u8 *)(dws->rx) = rxw;
> -			else
> +			else if (dws->n_bytes == 2)
>  				*(u16 *)(dws->rx) = rxw;
> +			else
> +				*(u32 *)(dws->rx) = rxw;
>  
>  			dws->rx += dws->n_bytes;
>  		}
> @@ -311,8 +315,12 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
>  	u32 speed_hz;
>  	u16 clk_div;
>  

> -	/* CTRLR0[ 4/3: 0] Data Frame Size */
> -	cr0 |= (cfg->dfs - 1);
> +	if (!(dws->caps & DW_SPI_CAP_DWC_APB_XFER32))
> +		/* CTRLR0[ 4/3: 0] Data Frame Size */
> +		cr0 |= (cfg->dfs - 1);
> +	else
> +		/* CTRLR0[20: 16] Data Frame Size */
> +		cr0 |= FIELD_PREP(DWC_APB_CTRLR0_DFS32_MASK, cfg->dfs - 1);

Instead of constantly checking the capability flag here I'd suggest to add
a new dfs_off member to the private data. Like this:
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -148,6 +148,7 @@ struct dw_spi {
        unsigned long           paddr;
        int                     irq;
        u32                     fifo_len;       /* depth of the FIFO buffer */
+       int                     dfs_off;        /* DFS field offset */
        u32                     max_mem_freq;   /* max mem-ops bus freq */
        u32                     max_freq;       /* max bus freq supported */

Then the only thing you'd need to add here is:
-	/* CTRLR0[ 4/3: 0] Data Frame Size */
-	cr0 |= (cfg->dfs - 1);
+	/* CTRLR0[ 4/3: 0]/CTRLR0[ 20: 16] Data Frame Size */
+	cr0 |= (cfg->dfs - 1) << dws->dfs_off;

* Note SPI core guarantees that bits_per_word is within the bits_per_word_mask
* initialized in the SPI controller descriptor.

>  
>  	if (!(dws->caps & DW_SPI_CAP_DWC_SSI))
>  		/* CTRLR0[ 9:8] Transfer Mode */
> @@ -828,6 +836,21 @@ static void spi_hw_init(struct device *dev, struct dw_spi *dws)
>  		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
>  	}
>  
> +	if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) {
> +		u32 cr0;
> +
> +		/*
> +		 * Detect APB SSI CTRLR0 size by looking at the data
> +		 * frame size field.
> +		 */

> +		dw_writel(dws, DW_SPI_CTRLR0, 0xffffffff);

This isn't going to work because you need to disable the SSI controller
first. Then please revert the change or cleanup the register and disable
the controller back after finishing the detection.

> +		cr0 = dw_readl(dws, DW_SPI_CTRLR0);

> +		if (FIELD_GET(DWC_APB_CTRLR0_DFS32_MASK, cr0)) {

Instead of reading the DFS_32 field state I'd suggest to read the normal
DFS bits as Sean does in the patch:
https://patchwork.ozlabs.org/project/uboot/patch/20201016225755.302659-10-seanga2@gmail.com/

That'd be better because we can't guarantee what some older versions of
the DW APB SSI controller preserve zeros in the upper part of CTRLR0, but
we are sure the DFS field will always have zeros if DW APB SSI is
synthesized with SSI_MAX_XFER_SIZE == 32.

So instead of having a new DWC_APB_CTRLR0_DFS32_MASK mask declared you'll
need to add the macros as following:
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -41,6 +41,7 @@
 
 /* Bit fields in CTRLR0 */
 #define SPI_DFS_OFFSET                 0
+#define SPI_DFS_MASK                   GENMASK(3, 0)
+#define SPI_DFS32_OFFSET		16
 
 #define SPI_FRF_OFFSET                 4
 #define SPI_FRF_SPI                    0x0

The SPI_DFS32_OFFSET macro will be needed to initialize the dfs_off member
if DFS32 capability is detected.

> +			dev_dbg(dev, "Detected 32-bits max data frame size\n");

> +			dws->caps |= DW_SPI_CAP_DWC_APB_XFER32;

I suggest to rename the flag to DW_SPI_CAP_DFS32, because it will be
set for both DW APB SSI v4 (in fact for v3.23a and newer (see the Sean'
patch log), but never mind) and DW SSI controllers. See the next comment
for details of implementation.

> +		}
> +	}

To sum up what I said above the detection procedure would look like this
(note it's untested...):

+	/*
+	 * Detect CTRLR0.DFS field size and offset by testing the lowest bits
+	 * writability. Note DW SSI controller also has the extended DFS, but
+	 * with zero offset.
+	 */
+	if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) {
+		u32 cr0, tmp = dw_readl(dws, DW_SPI_CTRLR0);
+
+		spi_enable_chip(dws, 0);
+		dw_writel(dws, DW_SPI_CTRLR0, 0xffffffff);
+		cr0 = dw_readl(dws, DW_SPI_CTRLR0);
+		dw_writel(dws, DW_SPI_CTRLR0, tmp);
+		spi_enable_chip(dws, 1);
+		if (!(cr0 & SPI_DFS_MASK)) {
+			dev_dbg(dev, "Detected 32-bits max data frame size\n");
+			dws->caps |= DW_SPI_CAP_DFS32;
*			dws->dfs_off = SPI_DFS32_OFFSET;
+		}
+	} else {
+		dws->caps |= DW_SPI_CAP_DFS32;
+	}

Note I've added the DW SSI fix here as it was suggested in a comment to
your previous patchset revision...

> +
>  	/* enable HW fixup for explicit CS deselect for Amazon's alpine chip */
>  	if (dws->caps & DW_SPI_CAP_CS_OVERRIDE)
>  		dw_writel(dws, DW_SPI_CS_OVERRIDE, 0xF);
> @@ -864,7 +887,10 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>  
>  	master->use_gpio_descriptors = true;
>  	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP;
> -	master->bits_per_word_mask =  SPI_BPW_RANGE_MASK(4, 16);

> +	if (dws->caps & DW_SPI_CAP_DWC_APB_XFER32)

Please use the DW_SPI_CAP_DFS32 capability flag here.

> +		master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> +	else
> +		master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 16);

So this part of your fix will work for all types of controllers:
- DW APB SSI v3.22a and older
- DW APB SSI v3.23a and newer with SSI_MAX_XFER_SIZE == 16
- DW APB SSI v3.23a and newer with SSI_MAX_XFER_SIZE == 32
- DW SSI v1.*

>  	master->bus_num = dws->bus_num;
>  	master->num_chipselect = dws->num_cs;
>  	master->setup = dw_spi_setup;
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index faf40cb66498..cba5c1f8c456 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -9,6 +9,7 @@
>  #include <linux/io.h>
>  #include <linux/scatterlist.h>
>  #include <linux/spi/spi-mem.h>
> +#include <linux/bitfield.h>
>  
>  /* Register offsets */
>  #define DW_SPI_CTRLR0			0x00
> @@ -72,6 +73,12 @@
>  #define DWC_SSI_CTRLR0_FRF_OFFSET	6
>  #define DWC_SSI_CTRLR0_DFS_OFFSET	0
> 
 
> +/*
> + * Data frame size bits mask in CTRLR0 for DWC_apb_ssi v4 when the
> + * controller capability supports DW_SPI_CAP_DWC_APB_XFER32.
> + */

> +#define DWC_APB_CTRLR0_DFS32_MASK	GENMASK(20, 16)

This won't be needed if you implement the fix as I suggest.

> +
>  /*
>   * For Keem Bay, CTRLR0[31] is used to select controller mode.
>   * 0: SSI is slave
> @@ -121,6 +128,7 @@ enum dw_ssi_type {
>  #define DW_SPI_CAP_CS_OVERRIDE		BIT(0)
>  #define DW_SPI_CAP_KEEMBAY_MST		BIT(1)
>  #define DW_SPI_CAP_DWC_SSI		BIT(2)

> +#define DW_SPI_CAP_DWC_APB_XFER32	BIT(3)

Please replace it with DW_SPI_CAP_DFS32, since the capability flag
will be used for the DW APB SSI v3.23a and newer and DW SSI controllers.

-Sergey

>  
>  /* Slave spi_transfer/spi_mem_op related */
>  struct dw_spi_cfg {
> -- 
> 2.28.0
> 

Re: [PATCH 1/3] spi: dw: Add support for 32-bits max xfer size

[Sean Anderson]

On 11/25/20 2:52 PM, Serge Semin wrote:
> Damien,
> 
> Thanks for sending the patch. My comments are below.
> 
> On Thu, Nov 19, 2020 at 09:02:51PM +0900, Damien Le Moal wrote:
>> Synopsis DesignWare DW_apb_ssi version 3.23 onward defines a 32-bits
>> maximum transfer size synthesis parameter (SSI_MAX_XFER_SIZE=32) in
>> addition to the legacy 16-bits configuration (SSI_MAX_XFER_SIZE=16) for
>> SPI controllers.  When SSI_MAX_XFER_SIZE=32, the layout of the ctrlr0
>> register changes and RX/TX FIFO words can be up to 32-bits.
>>
>> The layout of ctrlr0 change for the 32-bits configuration moves the
>> data frame format field to bits 16..20 instead of bits 3..0
>> when the controller is configured with SSI_MAX_XFER_SIZE=16.
>>
>> Introduce the DW SPI capability flag DW_SPI_CAP_DWC_APB_XFER32 to
>> indicate in that the controller is configured with SSI_MAX_XFER_SIZE=32.
>> Since the SSI_MAX_XFER_SIZE parameter is a controller synthesis
>> parameter not accessible through a register, the detection of this
>> parameter value is done in spi_hw_init() by writing and reading the
>> ctrlr0 register and testing the value of bits 20..16.
>>
>> The DW_SPI_CAP_DWC_APB_XFER32 flag is used in dw_spi_update_config() to
>> set the data frame size field at the correct position and in
>> dw_spi_add_host() to extend bits_per_word_mask to 32-bits.
>> dw_reader() and dw_writer() are also modified to handle 32-bits FIFO
>> words.
>>
>> Suggested-by: Sean Anderson <seanga2@gmail.com>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>>   drivers/spi/spi-dw-core.c | 40 ++++++++++++++++++++++++++++++++-------
>>   drivers/spi/spi-dw.h      |  8 ++++++++
>>   2 files changed, 41 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
>> index 2e50cc0a9291..4c16832b16fd 100644
>> --- a/drivers/spi/spi-dw-core.c
>> +++ b/drivers/spi/spi-dw-core.c
>> @@ -137,14 +137,16 @@ static inline u32 rx_max(struct dw_spi *dws)
>>   static void dw_writer(struct dw_spi *dws)
>>   {
>>   	u32 max = tx_max(dws);
>> -	u16 txw = 0;
>> +	u32 txw = 0;
>>   
>>   	while (max--) {
>>   		if (dws->tx) {
>>   			if (dws->n_bytes == 1)
>>   				txw = *(u8 *)(dws->tx);
>> -			else
>> +			else if (dws->n_bytes == 2)
>>   				txw = *(u16 *)(dws->tx);
>> +			else
>> +				txw = *(u32 *)(dws->tx);
>>   
>>   			dws->tx += dws->n_bytes;
>>   		}
>> @@ -156,15 +158,17 @@ static void dw_writer(struct dw_spi *dws)
>>   static void dw_reader(struct dw_spi *dws)
>>   {
>>   	u32 max = rx_max(dws);
>> -	u16 rxw;
>> +	u32 rxw;
>>   
>>   	while (max--) {
>>   		rxw = dw_read_io_reg(dws, DW_SPI_DR);
>>   		if (dws->rx) {
>>   			if (dws->n_bytes == 1)
>>   				*(u8 *)(dws->rx) = rxw;
>> -			else
>> +			else if (dws->n_bytes == 2)
>>   				*(u16 *)(dws->rx) = rxw;
>> +			else
>> +				*(u32 *)(dws->rx) = rxw;
>>   
>>   			dws->rx += dws->n_bytes;
>>   		}
>> @@ -311,8 +315,12 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
>>   	u32 speed_hz;
>>   	u16 clk_div;
>>   
> 
>> -	/* CTRLR0[ 4/3: 0] Data Frame Size */
>> -	cr0 |= (cfg->dfs - 1);
>> +	if (!(dws->caps & DW_SPI_CAP_DWC_APB_XFER32))
>> +		/* CTRLR0[ 4/3: 0] Data Frame Size */
>> +		cr0 |= (cfg->dfs - 1);
>> +	else
>> +		/* CTRLR0[20: 16] Data Frame Size */
>> +		cr0 |= FIELD_PREP(DWC_APB_CTRLR0_DFS32_MASK, cfg->dfs - 1);
> 
> Instead of constantly checking the capability flag here I'd suggest to add
> a new dfs_off member to the private data. Like this:
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -148,6 +148,7 @@ struct dw_spi {
>          unsigned long           paddr;
>          int                     irq;
>          u32                     fifo_len;       /* depth of the FIFO buffer */
> +       int                     dfs_off;        /* DFS field offset */
>          u32                     max_mem_freq;   /* max mem-ops bus freq */
>          u32                     max_freq;       /* max bus freq supported */
> 
> Then the only thing you'd need to add here is:
> -	/* CTRLR0[ 4/3: 0] Data Frame Size */
> -	cr0 |= (cfg->dfs - 1);
> +	/* CTRLR0[ 4/3: 0]/CTRLR0[ 20: 16] Data Frame Size */
> +	cr0 |= (cfg->dfs - 1) << dws->dfs_off;
> 
> * Note SPI core guarantees that bits_per_word is within the bits_per_word_mask
> * initialized in the SPI controller descriptor.
> 
>>   
>>   	if (!(dws->caps & DW_SPI_CAP_DWC_SSI))
>>   		/* CTRLR0[ 9:8] Transfer Mode */
>> @@ -828,6 +836,21 @@ static void spi_hw_init(struct device *dev, struct dw_spi *dws)
>>   		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
>>   	}
>>   
>> +	if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) {
>> +		u32 cr0;
>> +
>> +		/*
>> +		 * Detect APB SSI CTRLR0 size by looking at the data
>> +		 * frame size field.
>> +		 */
> 
>> +		dw_writel(dws, DW_SPI_CTRLR0, 0xffffffff);
> 
> This isn't going to work because you need to disable the SSI controller
> first. Then please revert the change or cleanup the register and disable
> the controller back after finishing the detection.

In U-Boot at least, ctrlr0 is written before every transfer, so there is
no need to restore it afterward. Same for enr. I don't know whether this
holds true for Linux.

--Sean

> 
>> +		cr0 = dw_readl(dws, DW_SPI_CTRLR0);
> 
>> +		if (FIELD_GET(DWC_APB_CTRLR0_DFS32_MASK, cr0)) {
> 
> Instead of reading the DFS_32 field state I'd suggest to read the normal
> DFS bits as Sean does in the patch:
> https://patchwork.ozlabs.org/project/uboot/patch/20201016225755.302659-10-seanga2@gmail.com/
> 
> That'd be better because we can't guarantee what some older versions of
> the DW APB SSI controller preserve zeros in the upper part of CTRLR0, but
> we are sure the DFS field will always have zeros if DW APB SSI is
> synthesized with SSI_MAX_XFER_SIZE == 32.
> 
> So instead of having a new DWC_APB_CTRLR0_DFS32_MASK mask declared you'll
> need to add the macros as following:
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -41,6 +41,7 @@
>   
>   /* Bit fields in CTRLR0 */
>   #define SPI_DFS_OFFSET                 0
> +#define SPI_DFS_MASK                   GENMASK(3, 0)
> +#define SPI_DFS32_OFFSET		16
>   
>   #define SPI_FRF_OFFSET                 4
>   #define SPI_FRF_SPI                    0x0
> 
> The SPI_DFS32_OFFSET macro will be needed to initialize the dfs_off member
> if DFS32 capability is detected.
> 
>> +			dev_dbg(dev, "Detected 32-bits max data frame size\n");
> 
>> +			dws->caps |= DW_SPI_CAP_DWC_APB_XFER32;
> 
> I suggest to rename the flag to DW_SPI_CAP_DFS32, because it will be
> set for both DW APB SSI v4 (in fact for v3.23a and newer (see the Sean'
> patch log), but never mind) and DW SSI controllers. See the next comment
> for details of implementation.
> 
>> +		}
>> +	}
> 
> To sum up what I said above the detection procedure would look like this
> (note it's untested...):
> 
> +	/*
> +	 * Detect CTRLR0.DFS field size and offset by testing the lowest bits
> +	 * writability. Note DW SSI controller also has the extended DFS, but
> +	 * with zero offset.
> +	 */
> +	if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) {
> +		u32 cr0, tmp = dw_readl(dws, DW_SPI_CTRLR0);
> +
> +		spi_enable_chip(dws, 0);
> +		dw_writel(dws, DW_SPI_CTRLR0, 0xffffffff);
> +		cr0 = dw_readl(dws, DW_SPI_CTRLR0);
> +		dw_writel(dws, DW_SPI_CTRLR0, tmp);
> +		spi_enable_chip(dws, 1);
> +		if (!(cr0 & SPI_DFS_MASK)) {
> +			dev_dbg(dev, "Detected 32-bits max data frame size\n");
> +			dws->caps |= DW_SPI_CAP_DFS32;
> *			dws->dfs_off = SPI_DFS32_OFFSET;
> +		}
> +	} else {
> +		dws->caps |= DW_SPI_CAP_DFS32;
> +	}
> 
> Note I've added the DW SSI fix here as it was suggested in a comment to
> your previous patchset revision...
> 
>> +
>>   	/* enable HW fixup for explicit CS deselect for Amazon's alpine chip */
>>   	if (dws->caps & DW_SPI_CAP_CS_OVERRIDE)
>>   		dw_writel(dws, DW_SPI_CS_OVERRIDE, 0xF);
>> @@ -864,7 +887,10 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>>   
>>   	master->use_gpio_descriptors = true;
>>   	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP;
>> -	master->bits_per_word_mask =  SPI_BPW_RANGE_MASK(4, 16);
> 
>> +	if (dws->caps & DW_SPI_CAP_DWC_APB_XFER32)
> 
> Please use the DW_SPI_CAP_DFS32 capability flag here.
> 
>> +		master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
>> +	else
>> +		master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 16);
> 
> So this part of your fix will work for all types of controllers:
> - DW APB SSI v3.22a and older
> - DW APB SSI v3.23a and newer with SSI_MAX_XFER_SIZE == 16
> - DW APB SSI v3.23a and newer with SSI_MAX_XFER_SIZE == 32
> - DW SSI v1.*
> 
>>   	master->bus_num = dws->bus_num;
>>   	master->num_chipselect = dws->num_cs;
>>   	master->setup = dw_spi_setup;
>> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
>> index faf40cb66498..cba5c1f8c456 100644
>> --- a/drivers/spi/spi-dw.h
>> +++ b/drivers/spi/spi-dw.h
>> @@ -9,6 +9,7 @@
>>   #include <linux/io.h>
>>   #include <linux/scatterlist.h>
>>   #include <linux/spi/spi-mem.h>
>> +#include <linux/bitfield.h>
>>   
>>   /* Register offsets */
>>   #define DW_SPI_CTRLR0			0x00
>> @@ -72,6 +73,12 @@
>>   #define DWC_SSI_CTRLR0_FRF_OFFSET	6
>>   #define DWC_SSI_CTRLR0_DFS_OFFSET	0
>>
>   
>> +/*
>> + * Data frame size bits mask in CTRLR0 for DWC_apb_ssi v4 when the
>> + * controller capability supports DW_SPI_CAP_DWC_APB_XFER32.
>> + */
> 
>> +#define DWC_APB_CTRLR0_DFS32_MASK	GENMASK(20, 16)
> 
> This won't be needed if you implement the fix as I suggest.
> 
>> +
>>   /*
>>    * For Keem Bay, CTRLR0[31] is used to select controller mode.
>>    * 0: SSI is slave
>> @@ -121,6 +128,7 @@ enum dw_ssi_type {
>>   #define DW_SPI_CAP_CS_OVERRIDE		BIT(0)
>>   #define DW_SPI_CAP_KEEMBAY_MST		BIT(1)
>>   #define DW_SPI_CAP_DWC_SSI		BIT(2)
> 
>> +#define DW_SPI_CAP_DWC_APB_XFER32	BIT(3)
> 
> Please replace it with DW_SPI_CAP_DFS32, since the capability flag
> will be used for the DW APB SSI v3.23a and newer and DW SSI controllers.
> 
> -Sergey
> 
>>   
>>   /* Slave spi_transfer/spi_mem_op related */
>>   struct dw_spi_cfg {
>> -- 
>> 2.28.0
>>

Re: [PATCH 1/3] spi: dw: Add support for 32-bits max xfer size

[Damien Le Moal]

On 2020/11/26 13:55, Sean Anderson wrote:
> On 11/25/20 2:52 PM, Serge Semin wrote:
[...]
>>> +	if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) {
>>> +		u32 cr0;
>>> +
>>> +		/*
>>> +		 * Detect APB SSI CTRLR0 size by looking at the data
>>> +		 * frame size field.
>>> +		 */
>>
>>> +		dw_writel(dws, DW_SPI_CTRLR0, 0xffffffff);
>>
>> This isn't going to work because you need to disable the SSI controller
>> first. Then please revert the change or cleanup the register and disable
>> the controller back after finishing the detection.
> 
> In U-Boot at least, ctrlr0 is written before every transfer, so there is
> no need to restore it afterward. Same for enr. I don't know whether this
> holds true for Linux.

Same in Linux too. The base value for cr0 is prepared and saved in chip->cr0
field when the controller is scanned/reset and that saved base value is used for
updating cr0 on every transfer. Serge point here was to have the DFS32 detection
not result in the initial cr0 value changing, as some older versions of the DW
controller may react to it. The cr0 save/restore in the detection does not
matter for the K210 though. Since this is only done on scan/reset and not in the
fast path, I think it is OK and definitely safer.


-- 
Damien Le Moal
Western Digital Research