One more nitpick below.

On Thu, Jul 22, 2021 at 08:04:35PM +0300, Serge Semin wrote:

On Thu, Jul 22, 2021 at 01:33:58PM +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 added for Thunder Bay, to
configure the device as a master or as a slave serial peripheral.

quoted

Bit 14(SSTE) of CTRLR0 register should be set(1) for Thunder Bay.

Could you elaborate what this bit mean?

quoted

Added reset of SPI controller required for Thunder Bay.

If it's really required (is it?) then you were supposed to reflect
that in the code by returning a negative error if the driver fails to
retrieve the reset control handler. In accordance with that the
bindings should have been also updated so the dtbs_check procedure
would make sure the Thunder Bay SPI DT-node comply to the requirements
in that matter.

Anyway I've got a few comments regarding this part of your patch.
Please see them below.

quoted

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

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

@@ -302,6 +302,12 @@ static u32 dw_spi_prepare_cr0(struct dw_spi *dws, struct spi_device *spi)
 
 		if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
 			cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;
+

quoted

+		if (dws->caps & DW_SPI_CAP_THUNDERBAY_MST)
+			cr0 |= DWC_SSI_CTRLR0_THUNDERBAY_MST;

I guess that KeemBay and ThunderBay SPI controllers have been
synthesized based on the same IP-core with a few differences. Is that
true? Could you tell us what is the difference between them?

Anyway regarding this the Master/Slave part. Is the ThunderBay
implementation of the Master/Slave capability the same as it was
embedded in the KeemBay controller? If so then what do you think about
just renaming DW_SPI_CAP_KEEMBAY_MST to something like
DW_SPI_CAP_INTEL_MST and using it then for both Keembay and ThunderBay
versions of the SPI-controllers? (The similar renaming needs to be
provided for the DWC_SSI_CTRLR0_KEEMBAY_MST macro then.) You can
implement it as a preparation patch posted before this one in the
series.

quoted

+
+		if (dws->caps & DW_SPI_CAP_THUNDERBAY_SSTE)
+			cr0 |= DWC_SSI_CTRLR0_THUNDERBAY_SSTE;

Similar question regarding the SSTE bit. Is it something ThunderBay
specific only? Was the corresponding functionality embedded into the
KeemBay or any other Intel version of the DW SPI controller?

quoted

 	}
 
 	return cr0;

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

@@ -222,6 +222,15 @@ static int dw_spi_keembay_init(struct platform_device *pdev,
 	return 0;
 }
 
+static int dw_spi_thunderbay_init(struct platform_device *pdev,
+				  struct dw_spi_mmio *dwsmmio)
+{

quoted

+	dwsmmio->dws.caps = DW_SPI_CAP_THUNDERBAY_MST | DW_SPI_CAP_THUNDERBAY_RST |
+			    DW_SPI_CAP_THUNDERBAY_SSTE | DW_SPI_CAP_DWC_SSI;
+

Originally the DW_SPI_CAP-functionality was provided to modify the DW
SPI core driver behavior when it was required. For instance it was
mostly connected with the platform-specific CR0-register
configurations. So as I see it the reset part can be successfully
handled fully in the framework of the MMIO-platform glue-driver.
Instead of defining a new capability you could have just added the
next code in the ThunderBay init-method:

+	if (!dwsmmio->rstc) {
+		dev_err(&pdev->dev, "Reset control is missing\n");
+		return -EINVAL;
+	}
+
+	reset_control_assert(dwsmmio->rstc);

+	udelay(2);

Please, don't forget to add a header file with udelay() declaration to
this module.

-Sergey

+	reset_control_deassert(dwsmmio->rstc);
+

Thus you'd reuse the already implemented reset-controller handler
defined in the dw_spi_mmio structure with no need of implementing
a new capability.

quoted

+	return 0;
+}
+
 static int dw_spi_canaan_k210_init(struct platform_device *pdev,
 				   struct dw_spi_mmio *dwsmmio)
 {

@@ -243,6 +252,7 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 			 struct dw_spi_mmio *dwsmmio);
 	struct dw_spi_mmio *dwsmmio;
 	struct resource *mem;
+	struct reset_control *rst;
 	struct dw_spi *dws;
 	int ret;
 	int num_cs;

@@ -309,6 +319,15 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 			goto out;
 	}
 

quoted

+	if (dws->caps & DW_SPI_CAP_THUNDERBAY_RST) {
+		rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+		if (!IS_ERR(rst)) {
+			reset_control_assert(rst);
+			udelay(2);
+			reset_control_deassert(rst);
+		}
+	}
+

Please see my comment above. We don't need to have this code here if
you get to implement what I suggest there.

quoted

 	pm_runtime_enable(&pdev->dev);
 
 	ret = dw_spi_add_host(&pdev->dev, dws);

@@ -349,6 +368,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..bfe1d5edc25a 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h

@@ -82,6 +82,18 @@
  */
 #define DWC_SSI_CTRLR0_KEEMBAY_MST	BIT(31)
 

quoted

+/*
+ * For Thunder Bay, CTRLR0[14] should be set to 1.
+ */

Could you provide a bit more details what this bit has been
implemented for?

quoted

+#define DWC_SSI_CTRLR0_THUNDERBAY_SSTE	BIT(14)
+

quoted

+/*
+ * For Thunder Bay, CTRLR0[31] is used to select controller mode.
+ * 0: SSI is slave
+ * 1: SSI is master
+ */
+#define DWC_SSI_CTRLR0_THUNDERBAY_MST	BIT(31)

Please see my suggestion regarding the Master/Slave capability in one
of the comments above.

Regards
-Serge

quoted

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

@@ -125,6 +137,9 @@ enum dw_ssi_type {
 #define DW_SPI_CAP_KEEMBAY_MST		BIT(1)
 #define DW_SPI_CAP_DWC_SSI		BIT(2)
 #define DW_SPI_CAP_DFS32		BIT(3)
+#define DW_SPI_CAP_THUNDERBAY_MST	BIT(4)
+#define DW_SPI_CAP_THUNDERBAY_RST	BIT(5)
+#define DW_SPI_CAP_THUNDERBAY_SSTE	BIT(6)
 
 /* Slave spi_transfer/spi_mem_op related */
 struct dw_spi_cfg {

-- 
2.17.1