[PATCH v2 0/2] Add support for Intel Thunder Bay SPI

From: Nandhini Srikandan <redacted>

Hi,

This patch set enables support for Designware SPI on the Intel Thunder Bay SoC.

Patch 1: SPI DT bindings for Intel Thunder Bay SoC.
Patch 2: Adds support for Designware SPI on Intel Thunderbay SoC.

The driver is tested on Keem Bay and Thunder Bay evaluation board

Changes from v1:
1) Designware CR0 specific macros are named in a generic way.
2) SPI CAP macros are named in generic way rather than naming project specific.
3) SPI KEEM BAY specific macros are replaced by generic macros.
4) Resued the existing SPI deassert API instead of adding another reset

Thanks & Regards,
Nandhini

Nandhini Srikandan (2):
  dt-bindings: spi: Add bindings for Intel Thunder Bay SoC
  spi: dw: Add support for Intel Thunder Bay SPI

 .../bindings/spi/snps,dw-apb-ssi.yaml         |  2 ++
 drivers/spi/spi-dw-core.c                     |  7 +++++--
 drivers/spi/spi-dw-mmio.c                     | 20 ++++++++++++++++++-
 drivers/spi/spi-dw.h                          | 12 ++++++++---
 4 files changed, 35 insertions(+), 6 deletions(-)

-- 
2.17.1

From: Nandhini Srikandan <redacted>

Add documentation for SPI controller in Intel Thunder Bay SoC.

Signed-off-by: Nandhini Srikandan <redacted>
---
 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 ca91201a9926..88532bf8ba85 100644
--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml

@@ -61,6 +61,8 @@ properties:
           - const: snps,dw-apb-ssi
       - description: Intel Keem Bay SPI Controller
         const: intel,keembay-ssi
+      - description: Intel Thunder Bay SPI Controller
+        const: intel,thunderbay-ssi
       - description: Baikal-T1 SPI Controller
         const: baikal,bt1-ssi
       - description: Baikal-T1 System Boot SPI Controller

-- 
2.17.1

From: Nandhini Srikandan <redacted>

Add support for Intel Thunder Bay SPI controller, which uses DesignWare
DWC_ssi core.
Bit 31 of CTRLR0 register is set for Thunder Bay, to
configure the device as a master or as a slave serial peripheral.
Bit 14(SSTE) of CTRLR0 register should be set(1) for Thunder Bay.

Signed-off-by: Nandhini Srikandan <redacted>
---
 drivers/spi/spi-dw-core.c |  7 +++++--
 drivers/spi/spi-dw-mmio.c | 20 +++++++++++++++++++-
 drivers/spi/spi-dw.h      | 12 +++++++++---
 3 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index a305074c482e..f7d45318db8a 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c

@@ -300,8 +300,11 @@ static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device *spi)
 		/* CTRLR0[13] Shift Register Loop */
 		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) << DWC_SSI_CTRLR0_SRL_OFFSET;
 
-		if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
-			cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;
+		if (dws->caps & DW_SPI_CAP_DWC_MST)
+			cr0 |= DWC_SSI_CTRLR0_MST;
+
+		if (dws->caps & DW_SPI_CAP_DWC_SSTE)
+			cr0 |= DWC_SSI_CTRLR0_SSTE;
 	}
 
 	return cr0;

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index 3379720cfcb8..2bd1dedd90b0 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c

@@ -217,7 +217,24 @@ static int dw_spi_dwc_ssi_init(struct platform_device *pdev,
 static int dw_spi_keembay_init(struct platform_device *pdev,
 			       struct dw_spi_mmio *dwsmmio)
 {
-	dwsmmio->dws.caps = DW_SPI_CAP_KEEMBAY_MST | DW_SPI_CAP_DWC_SSI;
+	/*
+	 * Set MST to make keem bay SPI as master.
+	 */
+	dwsmmio->dws.caps = DW_SPI_CAP_DWC_MST | DW_SPI_CAP_DWC_SSI;
+
+	return 0;
+}
+
+static int dw_spi_thunderbay_init(struct platform_device *pdev,
+				  struct dw_spi_mmio *dwsmmio)
+{
+	/*
+	 * Set MST to make thunder bay SPI as master.
+	 * Set SSTE to enable slave select toggle bit which is required
+	 * for the slave devices connected to the thunder bay SPI controller.
+	 */
+	dwsmmio->dws.caps = DW_SPI_CAP_DWC_MST | DW_SPI_CAP_DWC_SSTE |
+			    DW_SPI_CAP_DWC_SSI;
 
 	return 0;
 }

@@ -349,6 +366,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
 	{ .compatible = "renesas,rzn1-spi", .data = dw_spi_dw_apb_init},
 	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
 	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
+	{ .compatible = "intel,thunderbay-ssi", .data = dw_spi_thunderbay_init},
 	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
 	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
 	{ /* end of table */}

diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index b665e040862c..9fffe0a02f3a 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h

@@ -76,11 +76,16 @@
 #define DWC_SSI_CTRLR0_DFS_OFFSET	0
 
 /*
- * For Keem Bay, CTRLR0[31] is used to select controller mode.
+ * CTRLR0[31] is used to select controller mode.
  * 0: SSI is slave
  * 1: SSI is master
  */
-#define DWC_SSI_CTRLR0_KEEMBAY_MST	BIT(31)
+#define DWC_SSI_CTRLR0_MST		BIT(31)
+
+/*
+ * CTRLR0[14] is used to enable/disable Slave Select Toggle bit
+ */
+#define DWC_SSI_CTRLR0_SSTE		BIT(14)
 
 /* Bit fields in CTRLR1 */
 #define SPI_NDF_MASK			GENMASK(15, 0)

@@ -122,9 +127,10 @@ enum dw_ssi_type {
 
 /* DW SPI capabilities */
 #define DW_SPI_CAP_CS_OVERRIDE		BIT(0)
-#define DW_SPI_CAP_KEEMBAY_MST		BIT(1)
+#define DW_SPI_CAP_DWC_MST		BIT(1)
 #define DW_SPI_CAP_DWC_SSI		BIT(2)
 #define DW_SPI_CAP_DFS32		BIT(3)
+#define DW_SPI_CAP_DWC_SSTE		BIT(4)
 
 /* Slave spi_transfer/spi_mem_op related */
 struct dw_spi_cfg {

-- 
2.17.1

On Tue, 24 Aug 2021 16:58:55 +0800, nandhini.srikandan@intel.com wrote:

From: Nandhini Srikandan <redacted>

Add documentation for SPI controller in Intel Thunder Bay SoC.

Signed-off-by: Nandhini Srikandan <redacted>
---
 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
 1 file changed, 2 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Hello Nandhini

On Tue, Aug 24, 2021 at 04:58:54PM +0800, nandhini.srikandan@intel.com wrote:

From: Nandhini Srikandan <redacted>

Hi,

This patch set enables support for Designware SPI on the Intel Thunder Bay SoC.

Patch 1: SPI DT bindings for Intel Thunder Bay SoC.
Patch 2: Adds support for Designware SPI on Intel Thunderbay SoC.

The driver is tested on Keem Bay and Thunder Bay evaluation board

Changes from v1:
1) Designware CR0 specific macros are named in a generic way.
2) SPI CAP macros are named in generic way rather than naming project specific.
3) SPI KEEM BAY specific macros are replaced by generic macros.
4) Resued the existing SPI deassert API instead of adding another reset

Thanks for the update. I'll have a look at the series on the next
week.

Regards,
-Sergey

Thanks & Regards,
Nandhini

Nandhini Srikandan (2):
  dt-bindings: spi: Add bindings for Intel Thunder Bay SoC
  spi: dw: Add support for Intel Thunder Bay SPI

 .../bindings/spi/snps,dw-apb-ssi.yaml         |  2 ++
 drivers/spi/spi-dw-core.c                     |  7 +++++--
 drivers/spi/spi-dw-mmio.c                     | 20 ++++++++++++++++++-
 drivers/spi/spi-dw.h                          | 12 ++++++++---
 4 files changed, 35 insertions(+), 6 deletions(-)

-- 
2.17.1

Hello Nandhini

On Tue, Aug 24, 2021 at 04:58:55PM +0800, nandhini.srikandan@intel.com wrote:

From: Nandhini Srikandan <redacted>

Add documentation for SPI controller in Intel Thunder Bay SoC.

Signed-off-by: Nandhini Srikandan <redacted>
---
 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 2 ++
 1 file changed, 2 insertions(+)

Acked-by: Serge Semin <redacted>

-Sergey

Hi Nandhini

On Tue, Aug 24, 2021 at 04:58:56PM +0800, nandhini.srikandan@intel.com wrote:

From: Nandhini Srikandan <redacted>

Add support for Intel Thunder Bay SPI controller, which uses DesignWare
DWC_ssi core.
Bit 31 of CTRLR0 register is set for Thunder Bay, to
configure the device as a master or as a slave serial peripheral.
Bit 14(SSTE) of CTRLR0 register should be set(1) for Thunder Bay.

After reading your response to my v1 comments, I've got a better
notion of the features you are trying to implement here. Please see my
comments below.

Signed-off-by: Nandhini Srikandan <redacted>
---

Just to note for your future patchwork. Instead of having a single
general changelog text in the cover letter it is much more convenient
for reviewers to see both the summary changelog and a changelog of
individual patches here under '---' delimiter.

+		if (dws->caps & DW_SPI_CAP_DWC_MST)
+			cr0 |= DWC_SSI_CTRLR0_MST;

Since you've used a generic suffix here, are you sure the MST/SLV feature
toggled by the BIT(31) bit is generic for all DWC SSI controllers?
I am asking because I don't have DWC SSI IP manual, but there is a
CTRL0 register layout posted by your colleague Wan Ahmad Zainie a year
ago: https://patches.linaro.org/patch/214693/ . It doesn't have that bit
defined.

If you are and it's specific to all DWC SSI controllers of v1.01a and
newer, then why not to implement that flag setting up in the framework
of the "DW_SPI_CAP_DWC_SSI" capability? Thus we'd have all
"snps,dwc-ssi-1.01a"-compatible devices and devices with the
DW_SPI_CAP_DWC_SSI flag set working well if for some reason they have
got slave-mode enabled by default.

Regarding SSTE flag and feature implemented behind it. First of all
AFAICS from the Wan Ahmad Zainie post sited above it is indeed generic
for both DWC SSI and DW APB SSI IP-cores of the controllers. Thus we
don't need an additional DWC SSI capability flag defined for it, but
need to have it generically implemented in the DW SPI core driver.
Secondly as you said it two weeks ago it defines a slave-specific
protocol, the way the SSI and CLK signals are driven between consecutive
frames:

quoted

SSTE (Slave Select Toggle Enable)
When SSTE bit is set to 1, the slave select line will toggle between
 consecutive data frames, with the serial clock being held to its default
 value while slave select line is high.
When SSTE bit is set to 0, slave select line will stay low and clock will
 run continuously for the duration of the transfer.

In general DWC SSI/DW APB SSI controller can be connected to slave
devices with SSTE and normal communication protocol requirements at
the same time by using different CS-lanes. Therefore the SSTE feature
turns to be Slave/Peripheral-device specific rather than
controller-specific and needs to be enabled/disabled when it's
required by a slave device.

Thus here is what I'd suggest to implement the SSTE feature generically:
1) Add a new SPI-slave Synopsys-specific DT-property into the bindings
file like this:

--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml

@@ -143,6 +143,12 @@ patternProperties:
           is an optional feature of the designware controller, and the
           upper limit is also subject to controller configuration.
 
+      snps,sste:
+        description: Slave select line will toggle between consecutive
+          data frames, with the serial clock being held to its default
+          value while slave select line is high.
+        type: boolean
+
 unevaluatedProperties: false
 
 required:

Please do that in a separate preparation patch submitted before the
"dt-bindings: spi: Add bindings for Intel Thunder Bay SoC" patch
in this series.

2) Add that property support into the driver like this:

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index a305074c482e..5caa74b9aa74 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c

@@ -27,6 +27,7 @@
 struct chip_data {
 	u32 cr0;
 	u32 rx_sample_dly;	/* RX sample delay */
+	bool sste;		/* Slave Select Toggle flag */
 };
 
 #ifdef CONFIG_DEBUG_FS

@@ -269,6 +270,7 @@ static irqreturn_t dw_spi_irq(int irq, void *dev_id)
 
 static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device *spi)
 {
+	struct chip_data *chip = spi_get_ctldata(spi);
 	u32 cr0 = 0;
 
 	if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) {

@@ -285,6 +287,9 @@ static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device *spi)
 
 		/* CTRLR0[11] Shift Register Loop */
 		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) << SPI_SRL_OFFSET;
+
+		/* CTRLR0[24] Slave Select Toggle Enable */
+		cr0 |= chip->sste << SPI_SSTE_OFFSET;
 	} else {
 		/* CTRLR0[ 7: 6] Frame Format */
 		cr0 |= SSI_MOTO_SPI << DWC_SSI_CTRLR0_FRF_OFFSET;

@@ -300,6 +305,9 @@ static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device *spi)
 		/* CTRLR0[13] Shift Register Loop */
 		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) << DWC_SSI_CTRLR0_SRL_OFFSET;
 
+		/* CTRLR0[14] Slave Select Toggle Enable */
+		cr0 |= chip->sste << DWC_SSI_CTRLR0_SSTE_OFFSET;
+
 		if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
 			cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;
 	}

@@ -789,6 +797,9 @@ static int dw_spi_setup(struct spi_device *spi)
 		chip->rx_sample_dly = DIV_ROUND_CLOSEST(rx_sample_dly_ns,
 							NSEC_PER_SEC /
 							dws->max_freq);
+
+		/* Get slave select toggling feature requirement */
+		chip->sste = device_property_read_bool(&spi->dev, "snps,sste");
 	}
 
 	/*

diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index b665e040862c..2ee3f839de39 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h

@@ -65,8 +65,10 @@
 #define SPI_SLVOE_OFFSET		10
 #define SPI_SRL_OFFSET			11
 #define SPI_CFS_OFFSET			12
+#define SPI_SSTE_OFFSET			24
 
 /* Bit fields in CTRLR0 based on DWC_ssi_databook.pdf v1.01a */
+#define DWC_SSI_CTRLR0_SSTE_OFFSET	14
 #define DWC_SSI_CTRLR0_SRL_OFFSET	13
 #define DWC_SSI_CTRLR0_TMOD_OFFSET	10
 #define DWC_SSI_CTRLR0_TMOD_MASK	GENMASK(11, 10)

Please also do that in a separate preparation patch.

3) If MST BIT(31) feature is generic, then please discard the
DW_SPI_CAP_KEEMBAY_MST capability flag and set the MST bit for each
DWC SSI device with DW_SPI_CAP_DWC_SSI capability set. If it's
Intel-specific, then convert the DW_SPI_CAP_KEEMBAY_MST capability
macro name to DW_SPI_CAP_INTEL_MST.

Please also do that in a separate preparation patch.

4) After all of that you can add the "Thunder Bay SPI" controller
support into the DW SPI MMIO driver by placing the
"intel,thunderbay-ssi" compatibility string into the OF-device table.
Since both Thunder and Keembay SPIs are based on the same IP-core then
you can just reuse the dw_spi_keembay_init() for both of them after
renaming it to something like dw_spi_intel_init().

-Sergey

-----Original Message-----
From: Serge Semin <redacted>
Sent: Sunday, September 5, 2021 8:04 PM
To: Srikandan, Nandhini <redacted>
Cc: Serge Semin <redacted>; broonie@kernel.org;
robh+dt@kernel.org; linux-spi@vger.kernel.org; linux-
kernel@vger.kernel.org; devicetree@vger.kernel.org;
mgross@linux.intel.com; Pan, Kris [off-list ref]; Demakkanavar,
Kenchappa [off-list ref]; Zhou, Furong
[off-list ref]; Sangannavar, Mallikarjunappa
[off-list ref]; Vaidya, Mahesh R
[off-list ref]; A, Rashmi [off-list ref]
Subject: Re: [PATCH v2 2/2] spi: dw: Add support for Intel Thunder Bay SPI

Hi Nandhini

On Tue, Aug 24, 2021 at 04:58:56PM +0800, nandhini.srikandan@intel.com
wrote:

quoted

From: Nandhini Srikandan <redacted>

Add support for Intel Thunder Bay SPI controller, which uses
DesignWare DWC_ssi core.
Bit 31 of CTRLR0 register is set for Thunder Bay, to configure the
device as a master or as a slave serial peripheral.
Bit 14(SSTE) of CTRLR0 register should be set(1) for Thunder Bay.

After reading your response to my v1 comments, I've got a better notion of
the features you are trying to implement here. Please see my comments
below.

quoted

Signed-off-by: Nandhini Srikandan <redacted>
---

Just to note for your future patchwork. Instead of having a single general
changelog text in the cover letter it is much more convenient for reviewers to
see both the summary changelog and a changelog of individual patches here
under '---' delimiter.

Sure, I will add changelog for individual patches also.

quoted

 drivers/spi/spi-dw-core.c |  7 +++++--  drivers/spi/spi-dw-mmio.c |
20 +++++++++++++++++++-
 drivers/spi/spi-dw.h      | 12 +++++++++---
 3 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index a305074c482e..f7d45318db8a 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c

@@ -300,8 +300,11 @@ static u32 dw_spi_prepare_cr0(struct dw_spi

*dws, struct spi_device *spi)

quoted

 		/* CTRLR0[13] Shift Register Loop */
 		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) <<
DWC_SSI_CTRLR0_SRL_OFFSET;

-		if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
-			cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;

quoted

+		if (dws->caps & DW_SPI_CAP_DWC_MST)
+			cr0 |= DWC_SSI_CTRLR0_MST;

Since you've used a generic suffix here, are you sure the MST/SLV feature
toggled by the BIT(31) bit is generic for all DWC SSI controllers?
I am asking because I don't have DWC SSI IP manual, but there is a
CTRL0 register layout posted by your colleague Wan Ahmad Zainie a year
ago: https://patches.linaro.org/patch/214693/ . It doesn't have that bit
defined.

If you are and it's specific to all DWC SSI controllers of v1.01a and newer,
then why not to implement that flag setting up in the framework of the
"DW_SPI_CAP_DWC_SSI" capability? Thus we'd have all "snps,dwc-ssi-
1.01a"-compatible devices and devices with the DW_SPI_CAP_DWC_SSI flag
set working well if for some reason they have got slave-mode enabled by
default.

Intel Keem Bay and Thunder Bay uses v1.02a version of DWC SSI controller. According to v1.02a, BIT31 of CTRLR0 is used for selecting Master or slave mode. In earlier versions, it was reserved. Both Keem Bay and Thunder Bay has to work in master mode, so this bit is set. The dwc_ssi controller can either function in master or slave (default) mode as per the spec. The bit31 requirement is only for Keem Bay and Thunder bay and other controllers can have a requirement to function in slave mode as well. Hence the bit is set only for Keem Bay/Thunder Bay. Please let me know if it should be set default to master mode.
Wan Ahmed Zainie has posted that patch based on earlier version of the controller and later up streamed the DW_SPI_CAP_KEEMBAY_MST capability flag. This will become generic now.

Sure, will modify SSTE as DT-property and do the necessary changes in both code and in DT.

The feature is for the controller version v1.02a and above. The controller can function on master or slave mode, default being slave mode. So, it is modified to master only in Keem bay and Thunder bay. 
The difference between v1.01a and v1.02a w.r.t CTRLR0 is BIT31 selection of master/slave mode. Though the feature is generic but BIT31 is needed to be set only for bay, I will rename the macros to a generic name. 

4) After all of that you can add the "Thunder Bay SPI" controller support into
the DW SPI MMIO driver by placing the "intel,thunderbay-ssi" compatibility
string into the OF-device table.
Since both Thunder and Keembay SPIs are based on the same IP-core then
you can just reuse the dw_spi_keembay_init() for both of them after
renaming it to something like dw_spi_intel_init().

Sure, will do the same.

Regards,
Nandhini

quoted

 	}

 	return cr0;

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index 3379720cfcb8..2bd1dedd90b0 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c

@@ -217,7 +217,24 @@ static int dw_spi_dwc_ssi_init(struct

platform_device *pdev,  static int dw_spi_keembay_init(struct

platform_device *pdev,

quoted

 			       struct dw_spi_mmio *dwsmmio)  {
-	dwsmmio->dws.caps = DW_SPI_CAP_KEEMBAY_MST |

DW_SPI_CAP_DWC_SSI;

quoted

+	/*
+	 * Set MST to make keem bay SPI as master.
+	 */
+	dwsmmio->dws.caps = DW_SPI_CAP_DWC_MST |

DW_SPI_CAP_DWC_SSI;

quoted

+
+	return 0;
+}
+
+static int dw_spi_thunderbay_init(struct platform_device *pdev,
+				  struct dw_spi_mmio *dwsmmio)
+{
+	/*
+	 * Set MST to make thunder bay SPI as master.
+	 * Set SSTE to enable slave select toggle bit which is required
+	 * for the slave devices connected to the thunder bay SPI controller.
+	 */
+	dwsmmio->dws.caps = DW_SPI_CAP_DWC_MST |

DW_SPI_CAP_DWC_SSTE |

quoted

+			    DW_SPI_CAP_DWC_SSI;

 	return 0;
 }

@@ -349,6 +366,7 @@ static const struct of_device_id

dw_spi_mmio_of_match[] = {

quoted

 	{ .compatible = "renesas,rzn1-spi", .data = dw_spi_dw_apb_init},
 	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
 	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
+	{ .compatible = "intel,thunderbay-ssi", .data =
+dw_spi_thunderbay_init},
 	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
 	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
 	{ /* end of table */}

diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h index

b665e040862c..9fffe0a02f3a 100644

--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h

@@ -76,11 +76,16 @@
 #define DWC_SSI_CTRLR0_DFS_OFFSET	0

 /*
- * For Keem Bay, CTRLR0[31] is used to select controller mode.
+ * CTRLR0[31] is used to select controller mode.
  * 0: SSI is slave
  * 1: SSI is master
  */
-#define DWC_SSI_CTRLR0_KEEMBAY_MST	BIT(31)
+#define DWC_SSI_CTRLR0_MST		BIT(31)
+
+/*
+ * CTRLR0[14] is used to enable/disable Slave Select Toggle bit  */
+#define DWC_SSI_CTRLR0_SSTE		BIT(14)

 /* Bit fields in CTRLR1 */
 #define SPI_NDF_MASK			GENMASK(15, 0)

@@ -122,9 +127,10 @@ enum dw_ssi_type {

 /* DW SPI capabilities */
 #define DW_SPI_CAP_CS_OVERRIDE		BIT(0)
-#define DW_SPI_CAP_KEEMBAY_MST		BIT(1)
+#define DW_SPI_CAP_DWC_MST		BIT(1)
 #define DW_SPI_CAP_DWC_SSI		BIT(2)
 #define DW_SPI_CAP_DFS32		BIT(3)
+#define DW_SPI_CAP_DWC_SSTE		BIT(4)

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

2.17.1

On Tue, Sep 07, 2021 at 10:54:10AM +0000, Srikandan, Nandhini wrote:

quoted

-----Original Message-----
From: Serge Semin <redacted>
Sent: Sunday, September 5, 2021 8:04 PM
To: Srikandan, Nandhini <redacted>
Cc: Serge Semin <redacted>; broonie@kernel.org;
robh+dt@kernel.org; linux-spi@vger.kernel.org; linux-
kernel@vger.kernel.org; devicetree@vger.kernel.org;
mgross@linux.intel.com; Pan, Kris [off-list ref]; Demakkanavar,
Kenchappa [off-list ref]; Zhou, Furong
[off-list ref]; Sangannavar, Mallikarjunappa
[off-list ref]; Vaidya, Mahesh R
[off-list ref]; A, Rashmi [off-list ref]
Subject: Re: [PATCH v2 2/2] spi: dw: Add support for Intel Thunder Bay SPI

Hi Nandhini

On Tue, Aug 24, 2021 at 04:58:56PM +0800, nandhini.srikandan@intel.com
wrote:

quoted

From: Nandhini Srikandan <redacted>

Add support for Intel Thunder Bay SPI controller, which uses
DesignWare DWC_ssi core.
Bit 31 of CTRLR0 register is set for Thunder Bay, to configure the
device as a master or as a slave serial peripheral.
Bit 14(SSTE) of CTRLR0 register should be set(1) for Thunder Bay.

After reading your response to my v1 comments, I've got a better notion of
the features you are trying to implement here. Please see my comments
below.

quoted

Signed-off-by: Nandhini Srikandan <redacted>
---

Just to note for your future patchwork. Instead of having a single general
changelog text in the cover letter it is much more convenient for reviewers to
see both the summary changelog and a changelog of individual patches here
under '---' delimiter.

Sure, I will add changelog for individual patches also.

quoted

quoted

 drivers/spi/spi-dw-core.c |  7 +++++--  drivers/spi/spi-dw-mmio.c |
20 +++++++++++++++++++-
 drivers/spi/spi-dw.h      | 12 +++++++++---
 3 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index a305074c482e..f7d45318db8a 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c

@@ -300,8 +300,11 @@ static u32 dw_spi_prepare_cr0(struct dw_spi

*dws, struct spi_device *spi)

quoted

 		/* CTRLR0[13] Shift Register Loop */
 		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) <<
DWC_SSI_CTRLR0_SRL_OFFSET;

-		if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
-			cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;

quoted

+		if (dws->caps & DW_SPI_CAP_DWC_MST)
+			cr0 |= DWC_SSI_CTRLR0_MST;

Since you've used a generic suffix here, are you sure the MST/SLV feature
toggled by the BIT(31) bit is generic for all DWC SSI controllers?
I am asking because I don't have DWC SSI IP manual, but there is a
CTRL0 register layout posted by your colleague Wan Ahmad Zainie a year
ago: https://patches.linaro.org/patch/214693/ . It doesn't have that bit
defined.

If you are and it's specific to all DWC SSI controllers of v1.01a and newer,
then why not to implement that flag setting up in the framework of the
"DW_SPI_CAP_DWC_SSI" capability? Thus we'd have all "snps,dwc-ssi-
1.01a"-compatible devices and devices with the DW_SPI_CAP_DWC_SSI flag
set working well if for some reason they have got slave-mode enabled by
default.

Intel Keem Bay and Thunder Bay uses v1.02a version of DWC SSI controller. According to v1.02a, BIT31 of CTRLR0 is used for selecting Master or slave mode. In earlier versions, it was reserved. Both Keem Bay and Thunder Bay has to work in master mode, so this bit is set. The dwc_ssi controller can either function in master or slave (default) mode as per the spec. The bit31 requirement is only for Keem Bay and Thunder bay and other controllers can have a requirement to function in slave mode as well. Hence the bit is set only for Keem Bay/Thunder Bay. Please let me know if it should be set default to master mode.
Wan Ahmed Zainie has posted that patch based on earlier version of the controller and later up streamed the DW_SPI_CAP_KEEMBAY_MST capability flag. This will become generic now.

I see. Thanks for clarification. IIUC BIT(31) is indeed specific to
all DWC SSI (not only Keem/Thunder Bay SPI IPs) and indeed determines
the Master/Slave mode of the controller. Then I don't really
understand why Wan Ahmed didn't make it set generically in CR0 for all
DWC SSI v1.01a instead of marking it as "intel,keembay-ssi"-specific
seeing he provided a generic "snps,dwc-ssi-1.01a" compatible code in
that same patchset.

That decision might have been caused by having different default
states of CTRLR0.31 bit in generic DWC SSI and Keem/Thunder Bay SSI...
Anyway I believe it won't hurt to set that bit for each DWC SSI
especially seeing the DW APB SSI driver doesn't support the SPI slave
mode at the moment. So please do that in a dedicated patch by converting
the DWC_SSI_CTRLR0_KEEMBAY_MST macro to a generic DWC_SSI_CTRLR0_MST and
applying it for CTRLR0.31 for each DW_SPI_CAP_DWC_SSI controller.

quoted

quoted

+
+		if (dws->caps & DW_SPI_CAP_DWC_SSTE)
+			cr0 |= DWC_SSI_CTRLR0_SSTE;

Regarding SSTE flag and feature implemented behind it. First of all AFAICS
from the Wan Ahmad Zainie post sited above it is indeed generic for both
DWC SSI and DW APB SSI IP-cores of the controllers. Thus we don't need an
additional DWC SSI capability flag defined for it, but need to have it
generically implemented in the DW SPI core driver.
Secondly as you said it two weeks ago it defines a slave-specific protocol, the
way the SSI and CLK signals are driven between consecutive
frames:

quoted

quoted

SSTE (Slave Select Toggle Enable)
When SSTE bit is set to 1, the slave select line will toggle between
consecutive data frames, with the serial clock being held to its
default  value while slave select line is high.
When SSTE bit is set to 0, slave select line will stay low and clock
will  run continuously for the duration of the transfer.

In general DWC SSI/DW APB SSI controller can be connected to slave devices
with SSTE and normal communication protocol requirements at the same
time by using different CS-lanes. Therefore the SSTE feature turns to be
Slave/Peripheral-device specific rather than controller-specific and needs to
be enabled/disabled when it's required by a slave device.

Thus here is what I'd suggest to implement the SSTE feature generically:
1) Add a new SPI-slave Synopsys-specific DT-property into the bindings file
like this:

--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml

@@ -143,6 +143,12 @@ patternProperties:
           is an optional feature of the designware controller, and the
           upper limit is also subject to controller configuration.

+      snps,sste:
+        description: Slave select line will toggle between consecutive
+          data frames, with the serial clock being held to its default
+          value while slave select line is high.
+        type: boolean
+
 unevaluatedProperties: false

 required:

Please do that in a separate preparation patch submitted before the
"dt-bindings: spi: Add bindings for Intel Thunder Bay SoC" patch in this
series.

Sure, will modify SSTE as DT-property and do the necessary changes in both code and in DT.

quoted

2) Add that property support into the driver like this:

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c index

a305074c482e..5caa74b9aa74 100644

--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c

@@ -27,6 +27,7 @@
 struct chip_data {
 	u32 cr0;
 	u32 rx_sample_dly;	/* RX sample delay */
+	bool sste;		/* Slave Select Toggle flag */
 };

 #ifdef CONFIG_DEBUG_FS

@@ -269,6 +270,7 @@ static irqreturn_t dw_spi_irq(int irq, void *dev_id)

 static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device *spi)  {
+	struct chip_data *chip = spi_get_ctldata(spi);
 	u32 cr0 = 0;

 	if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) { @@ -285,6 +287,9 @@

static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device *spi)

 		/* CTRLR0[11] Shift Register Loop */
 		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) << SPI_SRL_OFFSET;
+
+		/* CTRLR0[24] Slave Select Toggle Enable */
+		cr0 |= chip->sste << SPI_SSTE_OFFSET;
 	} else {
 		/* CTRLR0[ 7: 6] Frame Format */
 		cr0 |= SSI_MOTO_SPI << DWC_SSI_CTRLR0_FRF_OFFSET; @@
-300,6 +305,9 @@ static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct
spi_device *spi)
 		/* CTRLR0[13] Shift Register Loop */
 		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) <<
DWC_SSI_CTRLR0_SRL_OFFSET;

+		/* CTRLR0[14] Slave Select Toggle Enable */
+		cr0 |= chip->sste << DWC_SSI_CTRLR0_SSTE_OFFSET;
+
 		if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
 			cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;
 	}

@@ -789,6 +797,9 @@ static int dw_spi_setup(struct spi_device *spi)
 		chip->rx_sample_dly =

DIV_ROUND_CLOSEST(rx_sample_dly_ns,
 							NSEC_PER_SEC /
 							dws->max_freq);
+
+		/* Get slave select toggling feature requirement */
+		chip->sste = device_property_read_bool(&spi->dev,
"snps,sste");
 	}

 	/*

diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h index

b665e040862c..2ee3f839de39 100644

--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h

@@ -65,8 +65,10 @@
 #define SPI_SLVOE_OFFSET		10
 #define SPI_SRL_OFFSET			11
 #define SPI_CFS_OFFSET			12
+#define SPI_SSTE_OFFSET			24

 /* Bit fields in CTRLR0 based on DWC_ssi_databook.pdf v1.01a */
+#define DWC_SSI_CTRLR0_SSTE_OFFSET	14
 #define DWC_SSI_CTRLR0_SRL_OFFSET	13
 #define DWC_SSI_CTRLR0_TMOD_OFFSET	10
 #define DWC_SSI_CTRLR0_TMOD_MASK	GENMASK(11, 10)

Please also do that in a separate preparation patch.

3) If MST BIT(31) feature is generic, then please discard the
DW_SPI_CAP_KEEMBAY_MST capability flag and set the MST bit for each
DWC SSI device with DW_SPI_CAP_DWC_SSI capability set. If it's Intel-
specific, then convert the DW_SPI_CAP_KEEMBAY_MST capability macro
name to DW_SPI_CAP_INTEL_MST.

Please also do that in a separate preparation patch.

The feature is for the controller version v1.02a and above. The controller can function on master or slave mode, default being slave mode. So, it is modified to master only in Keem bay and Thunder bay. 
The difference between v1.01a and v1.02a w.r.t CTRLR0 is BIT31 selection of master/slave mode. Though the feature is generic but BIT31 is needed to be set only for bay, I will rename the macros to a generic name. 

Please, see my comment above. Let's set that bit for each DWC SSI
controller, so to have the driver protected from having the inverted
default state on any other vendor-specific controller.

quoted

4) After all of that you can add the "Thunder Bay SPI" controller support into
the DW SPI MMIO driver by placing the "intel,thunderbay-ssi" compatibility
string into the OF-device table.
Since both Thunder and Keembay SPIs are based on the same IP-core then
you can just reuse the dw_spi_keembay_init() for both of them after
renaming it to something like dw_spi_intel_init().

Sure, will do the same.

Thanks.

Regards,
-Sergey

Regards,
Nandhini

quoted

quoted

 	}

 	return cr0;

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index 3379720cfcb8..2bd1dedd90b0 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c

@@ -217,7 +217,24 @@ static int dw_spi_dwc_ssi_init(struct

platform_device *pdev,  static int dw_spi_keembay_init(struct

platform_device *pdev,

quoted

 			       struct dw_spi_mmio *dwsmmio)  {
-	dwsmmio->dws.caps = DW_SPI_CAP_KEEMBAY_MST |

DW_SPI_CAP_DWC_SSI;

quoted

+	/*
+	 * Set MST to make keem bay SPI as master.
+	 */
+	dwsmmio->dws.caps = DW_SPI_CAP_DWC_MST |

DW_SPI_CAP_DWC_SSI;

quoted

+
+	return 0;
+}
+
+static int dw_spi_thunderbay_init(struct platform_device *pdev,
+				  struct dw_spi_mmio *dwsmmio)
+{
+	/*
+	 * Set MST to make thunder bay SPI as master.
+	 * Set SSTE to enable slave select toggle bit which is required
+	 * for the slave devices connected to the thunder bay SPI controller.
+	 */
+	dwsmmio->dws.caps = DW_SPI_CAP_DWC_MST |

DW_SPI_CAP_DWC_SSTE |

quoted

+			    DW_SPI_CAP_DWC_SSI;

 	return 0;
 }

@@ -349,6 +366,7 @@ static const struct of_device_id

dw_spi_mmio_of_match[] = {

quoted

 	{ .compatible = "renesas,rzn1-spi", .data = dw_spi_dw_apb_init},
 	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
 	{ .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init},
+	{ .compatible = "intel,thunderbay-ssi", .data =
+dw_spi_thunderbay_init},
 	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
 	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
 	{ /* end of table */}

diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h index

b665e040862c..9fffe0a02f3a 100644

--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h

@@ -76,11 +76,16 @@
 #define DWC_SSI_CTRLR0_DFS_OFFSET	0

 /*
- * For Keem Bay, CTRLR0[31] is used to select controller mode.
+ * CTRLR0[31] is used to select controller mode.
  * 0: SSI is slave
  * 1: SSI is master
  */
-#define DWC_SSI_CTRLR0_KEEMBAY_MST	BIT(31)
+#define DWC_SSI_CTRLR0_MST		BIT(31)
+
+/*
+ * CTRLR0[14] is used to enable/disable Slave Select Toggle bit  */
+#define DWC_SSI_CTRLR0_SSTE		BIT(14)

 /* Bit fields in CTRLR1 */
 #define SPI_NDF_MASK			GENMASK(15, 0)

@@ -122,9 +127,10 @@ enum dw_ssi_type {

 /* DW SPI capabilities */
 #define DW_SPI_CAP_CS_OVERRIDE		BIT(0)
-#define DW_SPI_CAP_KEEMBAY_MST		BIT(1)
+#define DW_SPI_CAP_DWC_MST		BIT(1)
 #define DW_SPI_CAP_DWC_SSI		BIT(2)
 #define DW_SPI_CAP_DFS32		BIT(3)
+#define DW_SPI_CAP_DWC_SSTE		BIT(4)

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

2.17.1

-----Original Message-----
From: Serge Semin <redacted>
Sent: Wednesday, September 8, 2021 2:59 PM
To: Srikandan, Nandhini <redacted>
Cc: Serge Semin <redacted>; broonie@kernel.org;
robh+dt@kernel.org; linux-spi@vger.kernel.org; linux-
kernel@vger.kernel.org; devicetree@vger.kernel.org;
mgross@linux.intel.com; Pan, Kris [off-list ref]; Demakkanavar,
Kenchappa [off-list ref]; Zhou, Furong
[off-list ref]; Sangannavar, Mallikarjunappa
[off-list ref]; Vaidya, Mahesh R
[off-list ref]; A, Rashmi [off-list ref]
Subject: Re: [PATCH v2 2/2] spi: dw: Add support for Intel Thunder Bay SPI

On Tue, Sep 07, 2021 at 10:54:10AM +0000, Srikandan, Nandhini wrote:

quoted

quoted

-----Original Message-----
From: Serge Semin <redacted>
Sent: Sunday, September 5, 2021 8:04 PM
To: Srikandan, Nandhini <redacted>
Cc: Serge Semin <redacted>;
broonie@kernel.org;
robh+dt@kernel.org; linux-spi@vger.kernel.org; linux-
kernel@vger.kernel.org; devicetree@vger.kernel.org;
mgross@linux.intel.com; Pan, Kris [off-list ref];
Demakkanavar, Kenchappa [off-list ref];

Zhou,

quoted

quoted

Furong [off-list ref]; Sangannavar, Mallikarjunappa
[off-list ref]; Vaidya, Mahesh R
[off-list ref]; A, Rashmi [off-list ref]
Subject: Re: [PATCH v2 2/2] spi: dw: Add support for Intel Thunder
Bay SPI

Hi Nandhini

On Tue, Aug 24, 2021 at 04:58:56PM +0800,
nandhini.srikandan@intel.com
wrote:

quoted

From: Nandhini Srikandan <redacted>

Add support for Intel Thunder Bay SPI controller, which uses
DesignWare DWC_ssi core.
Bit 31 of CTRLR0 register is set for Thunder Bay, to configure the
device as a master or as a slave serial peripheral.
Bit 14(SSTE) of CTRLR0 register should be set(1) for Thunder Bay.

After reading your response to my v1 comments, I've got a better
notion of the features you are trying to implement here. Please see
my comments below.

quoted

Signed-off-by: Nandhini Srikandan <redacted>
---

Just to note for your future patchwork. Instead of having a single
general changelog text in the cover letter it is much more
convenient for reviewers to see both the summary changelog and a
changelog of individual patches here under '---' delimiter.

Sure, I will add changelog for individual patches also.

quoted

quoted

 drivers/spi/spi-dw-core.c |  7 +++++--  drivers/spi/spi-dw-mmio.c
|
20 +++++++++++++++++++-
 drivers/spi/spi-dw.h      | 12 +++++++++---
 3 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index a305074c482e..f7d45318db8a 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c

@@ -300,8 +300,11 @@ static u32 dw_spi_prepare_cr0(struct dw_spi

*dws, struct spi_device *spi)

quoted

 		/* CTRLR0[13] Shift Register Loop */
 		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) <<
DWC_SSI_CTRLR0_SRL_OFFSET;

-		if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
-			cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;

quoted

+		if (dws->caps & DW_SPI_CAP_DWC_MST)
+			cr0 |= DWC_SSI_CTRLR0_MST;

Since you've used a generic suffix here, are you sure the MST/SLV
feature toggled by the BIT(31) bit is generic for all DWC SSI controllers?
I am asking because I don't have DWC SSI IP manual, but there is a
CTRL0 register layout posted by your colleague Wan Ahmad Zainie a
year
ago: https://patches.linaro.org/patch/214693/ . It doesn't have that
bit defined.

If you are and it's specific to all DWC SSI controllers of v1.01a
and newer, then why not to implement that flag setting up in the
framework of the "DW_SPI_CAP_DWC_SSI" capability? Thus we'd have all
"snps,dwc-ssi- 1.01a"-compatible devices and devices with the
DW_SPI_CAP_DWC_SSI flag set working well if for some reason they
have got slave-mode enabled by default.

quoted

Intel Keem Bay and Thunder Bay uses v1.02a version of DWC SSI controller.

According to v1.02a, BIT31 of CTRLR0 is used for selecting Master or slave
mode. In earlier versions, it was reserved. Both Keem Bay and Thunder Bay
has to work in master mode, so this bit is set. The dwc_ssi controller can
either function in master or slave (default) mode as per the spec. The bit31
requirement is only for Keem Bay and Thunder bay and other controllers can
have a requirement to function in slave mode as well. Hence the bit is set
only for Keem Bay/Thunder Bay. Please let me know if it should be set
default to master mode.

quoted

Wan Ahmed Zainie has posted that patch based on earlier version of the

controller and later up streamed the DW_SPI_CAP_KEEMBAY_MST capability
flag. This will become generic now.

I see. Thanks for clarification. IIUC BIT(31) is indeed specific to all DWC SSI
(not only Keem/Thunder Bay SPI IPs) and indeed determines the
Master/Slave mode of the controller. Then I don't really understand why
Wan Ahmed didn't make it set generically in CR0 for all DWC SSI v1.01a
instead of marking it as "intel,keembay-ssi"-specific seeing he provided a
generic "snps,dwc-ssi-1.01a" compatible code in that same patchset.

That decision might have been caused by having different default states of
CTRLR0.31 bit in generic DWC SSI and Keem/Thunder Bay SSI...
Anyway I believe it won't hurt to set that bit for each DWC SSI especially
seeing the DW APB SSI driver doesn't support the SPI slave mode at the
moment. So please do that in a dedicated patch by converting the
DWC_SSI_CTRLR0_KEEMBAY_MST macro to a generic DWC_SSI_CTRLR0_MST
and applying it for CTRLR0.31 for each DW_SPI_CAP_DWC_SSI controller.

Sure, I will make the macro to a generic one and include it in the upcoming patchset.

quoted

quoted

quoted

+
+		if (dws->caps & DW_SPI_CAP_DWC_SSTE)
+			cr0 |= DWC_SSI_CTRLR0_SSTE;

Regarding SSTE flag and feature implemented behind it. First of all
AFAICS from the Wan Ahmad Zainie post sited above it is indeed
generic for both DWC SSI and DW APB SSI IP-cores of the controllers.
Thus we don't need an additional DWC SSI capability flag defined for
it, but need to have it generically implemented in the DW SPI core driver.
Secondly as you said it two weeks ago it defines a slave-specific
protocol, the way the SSI and CLK signals are driven between
consecutive
frames:

quoted

quoted

SSTE (Slave Select Toggle Enable) When SSTE bit is set to 1, the
slave select line will toggle between consecutive data frames,
with the serial clock being held to its default  value while
slave select line is high.
When SSTE bit is set to 0, slave select line will stay low and
clock will  run continuously for the duration of the transfer.

In general DWC SSI/DW APB SSI controller can be connected to slave
devices with SSTE and normal communication protocol requirements at
the same time by using different CS-lanes. Therefore the SSTE
feature turns to be Slave/Peripheral-device specific rather than
controller-specific and needs to be enabled/disabled when it's required

by a slave device.

quoted

quoted

Thus here is what I'd suggest to implement the SSTE feature generically:
1) Add a new SPI-slave Synopsys-specific DT-property into the
bindings file like this:

--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml

@@ -143,6 +143,12 @@ patternProperties:
           is an optional feature of the designware controller, and the
           upper limit is also subject to controller configuration.

+      snps,sste:
+        description: Slave select line will toggle between consecutive
+          data frames, with the serial clock being held to its default
+          value while slave select line is high.
+        type: boolean
+
 unevaluatedProperties: false

 required:

Please do that in a separate preparation patch submitted before the
"dt-bindings: spi: Add bindings for Intel Thunder Bay SoC" patch in
this series.

Sure, will modify SSTE as DT-property and do the necessary changes in both

code and in DT.

quoted

quoted

2) Add that property support into the driver like this:

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c

index
a305074c482e..5caa74b9aa74 100644

--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c

@@ -27,6 +27,7 @@
 struct chip_data {
 	u32 cr0;
 	u32 rx_sample_dly;	/* RX sample delay */
+	bool sste;		/* Slave Select Toggle flag */
 };

 #ifdef CONFIG_DEBUG_FS

@@ -269,6 +270,7 @@ static irqreturn_t dw_spi_irq(int irq, void

*dev_id)

 static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device
*spi)  {
+	struct chip_data *chip = spi_get_ctldata(spi);
 	u32 cr0 = 0;

 	if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) { @@ -285,6 +287,9 @@
static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device
*spi)

 		/* CTRLR0[11] Shift Register Loop */
 		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) << SPI_SRL_OFFSET;
+
+		/* CTRLR0[24] Slave Select Toggle Enable */
+		cr0 |= chip->sste << SPI_SSTE_OFFSET;
 	} else {
 		/* CTRLR0[ 7: 6] Frame Format */
 		cr0 |= SSI_MOTO_SPI << DWC_SSI_CTRLR0_FRF_OFFSET; @@
-300,6 +305,9 @@ static u32 dw_spi_prepare_cr0(struct dw_spi *dws,
struct spi_device *spi)
 		/* CTRLR0[13] Shift Register Loop */
 		cr0 |= ((spi->mode & SPI_LOOP) ? 1 : 0) <<
DWC_SSI_CTRLR0_SRL_OFFSET;

+		/* CTRLR0[14] Slave Select Toggle Enable */
+		cr0 |= chip->sste << DWC_SSI_CTRLR0_SSTE_OFFSET;
+
 		if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
 			cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;
 	}

@@ -789,6 +797,9 @@ static int dw_spi_setup(struct spi_device *spi)
 		chip->rx_sample_dly =

DIV_ROUND_CLOSEST(rx_sample_dly_ns,
 							NSEC_PER_SEC /
 							dws->max_freq);
+
+		/* Get slave select toggling feature requirement */
+		chip->sste = device_property_read_bool(&spi->dev,
"snps,sste");
 	}

 	/*

diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h index

b665e040862c..2ee3f839de39 100644

--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h

@@ -65,8 +65,10 @@
 #define SPI_SLVOE_OFFSET		10
 #define SPI_SRL_OFFSET			11
 #define SPI_CFS_OFFSET			12
+#define SPI_SSTE_OFFSET			24

 /* Bit fields in CTRLR0 based on DWC_ssi_databook.pdf v1.01a */
+#define DWC_SSI_CTRLR0_SSTE_OFFSET	14
 #define DWC_SSI_CTRLR0_SRL_OFFSET	13
 #define DWC_SSI_CTRLR0_TMOD_OFFSET	10
 #define DWC_SSI_CTRLR0_TMOD_MASK	GENMASK(11, 10)

Please also do that in a separate preparation patch.

3) If MST BIT(31) feature is generic, then please discard the
DW_SPI_CAP_KEEMBAY_MST capability flag and set the MST bit for each
DWC SSI device with DW_SPI_CAP_DWC_SSI capability set. If it's
Intel- specific, then convert the DW_SPI_CAP_KEEMBAY_MST capability
macro name to DW_SPI_CAP_INTEL_MST.

Please also do that in a separate preparation patch.

quoted

The feature is for the controller version v1.02a and above. The controller

can function on master or slave mode, default being slave mode. So, it is
modified to master only in Keem bay and Thunder bay.

quoted

The difference between v1.01a and v1.02a w.r.t CTRLR0 is BIT31 selection

of master/slave mode. Though the feature is generic but BIT31 is needed to
be set only for bay, I will rename the macros to a generic name.

Please, see my comment above. Let's set that bit for each DWC SSI controller,
so to have the driver protected from having the inverted default state on any
other vendor-specific controller.

Sure, I will set the bit for each DWC SSI controller.

Regard,
Nandhini 

quoted

quoted

4) After all of that you can add the "Thunder Bay SPI" controller
support into the DW SPI MMIO driver by placing the
"intel,thunderbay-ssi" compatibility string into the OF-device table.
Since both Thunder and Keembay SPIs are based on the same IP-core
then you can just reuse the dw_spi_keembay_init() for both of them
after renaming it to something like dw_spi_intel_init().

quoted

Sure, will do the same.

Thanks.

Regards,
-Sergey

quoted

Regards,
Nandhini

quoted

quoted

 	}

 	return cr0;

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index 3379720cfcb8..2bd1dedd90b0 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c

@@ -217,7 +217,24 @@ static int dw_spi_dwc_ssi_init(struct

platform_device *pdev,  static int dw_spi_keembay_init(struct

platform_device *pdev,

quoted

 			       struct dw_spi_mmio *dwsmmio)  {
-	dwsmmio->dws.caps = DW_SPI_CAP_KEEMBAY_MST |

DW_SPI_CAP_DWC_SSI;

quoted

+	/*
+	 * Set MST to make keem bay SPI as master.
+	 */
+	dwsmmio->dws.caps = DW_SPI_CAP_DWC_MST |

DW_SPI_CAP_DWC_SSI;

quoted

+
+	return 0;
+}
+
+static int dw_spi_thunderbay_init(struct platform_device *pdev,
+				  struct dw_spi_mmio *dwsmmio) {
+	/*
+	 * Set MST to make thunder bay SPI as master.
+	 * Set SSTE to enable slave select toggle bit which is required
+	 * for the slave devices connected to the thunder bay SPI controller.
+	 */
+	dwsmmio->dws.caps = DW_SPI_CAP_DWC_MST |

DW_SPI_CAP_DWC_SSTE |

quoted

+			    DW_SPI_CAP_DWC_SSI;

 	return 0;
 }

@@ -349,6 +366,7 @@ static const struct of_device_id

dw_spi_mmio_of_match[] = {

quoted

 	{ .compatible = "renesas,rzn1-spi", .data = dw_spi_dw_apb_init},
 	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init},
 	{ .compatible = "intel,keembay-ssi", .data =
dw_spi_keembay_init},
+	{ .compatible = "intel,thunderbay-ssi", .data =
+dw_spi_thunderbay_init},
 	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
 	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
 	{ /* end of table */}

diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h index

b665e040862c..9fffe0a02f3a 100644

--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h

@@ -76,11 +76,16 @@
 #define DWC_SSI_CTRLR0_DFS_OFFSET	0

 /*
- * For Keem Bay, CTRLR0[31] is used to select controller mode.
+ * CTRLR0[31] is used to select controller mode.
  * 0: SSI is slave
  * 1: SSI is master
  */
-#define DWC_SSI_CTRLR0_KEEMBAY_MST	BIT(31)
+#define DWC_SSI_CTRLR0_MST		BIT(31)
+
+/*
+ * CTRLR0[14] is used to enable/disable Slave Select Toggle bit  */
+#define DWC_SSI_CTRLR0_SSTE		BIT(14)

 /* Bit fields in CTRLR1 */
 #define SPI_NDF_MASK			GENMASK(15, 0)

@@ -122,9 +127,10 @@ enum dw_ssi_type {

 /* DW SPI capabilities */
 #define DW_SPI_CAP_CS_OVERRIDE		BIT(0)
-#define DW_SPI_CAP_KEEMBAY_MST		BIT(1)
+#define DW_SPI_CAP_DWC_MST		BIT(1)
 #define DW_SPI_CAP_DWC_SSI		BIT(2)
 #define DW_SPI_CAP_DFS32		BIT(3)
+#define DW_SPI_CAP_DWC_SSTE		BIT(4)

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

2.17.1