spi: dw: Add compatible for Intel Mount Evans SoC

The Intel Mount Evans SoC's Integrated Management Complex uses the SPI controller for access to a NOR SPI FLASH. However, the SoC doesn't provide a mechanism to override the native chip select signal.

This driver doesn't use DMA for memory operations when a chip select override is not provided due to the native chip select timing behavior. As a result no DMA configuration is done for the controller and this configuration is not tested.

The controller also has an errata where a full TX FIFO can result in data corruption. The suggested workaround is to never completely fill the FIFO. The TX FIFO has a size of 32 so the fifo_len is set to 31.

Signed-off-by Abe Kohandel <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

---
 drivers/spi/spi-dw-mmio.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

drivers/spi/spi-dw-mmio.c +0 -0

index 5f2aee69c1c1..c1d16157de61 100644
@@ -236,6 +236,31 @@ static int dw_spi_intel_init(struct platform_device *pdev,
 	return 0;
 }
 

drivers/spi/spi-dw-mmio.c +16 -0

+ */
+static int dw_spi_mountevans_imc_init(struct platform_device *pdev,
+				      struct dw_spi_mmio *dwsmmio)
+{
+	/*
+	 * The Intel Mount Evans SoC's Integrated Management Complex DW
+	 * apb_ssi_v4.02a controller has an errata where a full TX FIFO can
+	 * result in data corruption. The suggested workaround is to never
+	 * completely fill the FIFO. The TX FIFO has a size of 32 so the
+	 * fifo_len is set to 31.
+	 */
+	dwsmmio->dws.fifo_len = 31;
+
+	return 0;
+}
+
 static int dw_spi_canaan_k210_init(struct platform_device *pdev,
 				   struct dw_spi_mmio *dwsmmio)

{

drivers/spi/spi-dw-mmio.c +4 -0

@@ -405,6 +430,10 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
 	{ .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_hssi_init},
 	{ .compatible = "intel,keembay-ssi", .data = dw_spi_intel_init},
 	{ .compatible = "intel,thunderbay-ssi", .data = dw_spi_intel_init},
+	{
+		.compatible = "intel,mountevans-imc-ssi",
+		.data = dw_spi_mountevans_imc_init,
+	},
 	{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
 	{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
 	{ .compatible = "amd,pensando-elba-spi", .data = dw_spi_elba_init},

Signature · 1 line

2.40.1

Hi Abe

Quoted 27 lines · Abe Kohandel

The Intel Mount Evans SoC's Integrated Management Complex uses the SPI controller for access to a NOR SPI FLASH. However, the SoC doesn't provide a mechanism to override the native chip select signal.

This driver doesn't use DMA for memory operations when a chip select override is not provided due to the native chip select timing behavior. As a result no DMA configuration is done for the controller and this configuration is not tested.

The controller also has an errata where a full TX FIFO can result in data corruption. The suggested workaround is to never completely fill the FIFO. The TX FIFO has a size of 32 so the fifo_len is set to 31.

Signed-off-by Abe Kohandel <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

---
 drivers/spi/spi-dw-mmio.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

drivers/spi/spi-dw-mmio.c +0 -0

index 5f2aee69c1c1..c1d16157de61 100644
@@ -236,6 +236,31 @@ static int dw_spi_intel_init(struct platform_device *pdev,
 	return 0;
 }
 

drivers/spi/spi-dw-mmio.c static int dw_spi_intel_init(struct platform_device *pdev, · line 248

Quoted 10 lines

drivers/spi/spi-dw-mmio.c +9 -0

+/*
+ * The Intel Mount Evans SoC's Integrated Management Complex uses the
+ * SPI controller for access to a NOR SPI FLASH. However, the SoC doesn't
+ * provide a mechanism to override the native chip select signal.
+ *
 
+ * This driver doesn't use DMA for memory operations when a chip select
+ * override is not provided due to the native chip select timing behavior.
+ * As a result no DMA configuration is done for the controller and this
+ * configuration is not tested.

Based on what is written you didn't test the DMA-based memory operations on your hardware. Well, this driver doesn't use DMA for memory operations on the platforms with the native CS just because nobody has implemented that feature so far. AFAICS if DMA-based memory operations were supported by the driver I don't think that the native CS auto de-assertion would have been an issue except when there is no hw-accelerated LLPs list handling in the DMA controller (in the later case we could have fallen back to the IRQ-less implementation though). Moreover having the DMA-based memory ops implemented would have been even better than what the driver provides at the moment since it would have eliminated the mem-op transfers in the atomic context. So the comment seems misleading. Another problem is that it refers to a feature which may be added in future. So the comment will be wrong then. So I would suggest to either drop the comment or change to something that just states that the DMA-based mem ops weren't tested for this hardware.

Am I wrong in some aspects of understanding your comment? Did you mean something else than what I inferred from it?

-Serge(y)

Hi Serge,

Quoted 61 lines · Serge Semin

Hi Abe

Quoted 27 lines · Abe Kohandel

The Intel Mount Evans SoC's Integrated Management Complex uses the SPI controller for access to a NOR SPI FLASH. However, the SoC doesn't provide a mechanism to override the native chip select signal.

This driver doesn't use DMA for memory operations when a chip select override is not provided due to the native chip select timing behavior. As a result no DMA configuration is done for the controller and this configuration is not tested.

The controller also has an errata where a full TX FIFO can result in data corruption. The suggested workaround is to never completely fill the FIFO. The TX FIFO has a size of 32 so the fifo_len is set to 31.

Signed-off-by Abe Kohandel <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

---
 drivers/spi/spi-dw-mmio.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

drivers/spi/spi-dw-mmio.c +0 -0

index 5f2aee69c1c1..c1d16157de61 100644
@@ -236,6 +236,31 @@ static int dw_spi_intel_init(struct platform_device *pdev,
 	return 0;
 }
 

drivers/spi/spi-dw-mmio.c static int dw_spi_intel_init(struct platform_device *pdev, · line 248

Quoted 10 lines

drivers/spi/spi-dw-mmio.c +9 -0

+/*
+ * The Intel Mount Evans SoC's Integrated Management Complex uses the
+ * SPI controller for access to a NOR SPI FLASH. However, the SoC doesn't
+ * provide a mechanism to override the native chip select signal.
+ *
 
+ * This driver doesn't use DMA for memory operations when a chip select
+ * override is not provided due to the native chip select timing behavior.
+ * As a result no DMA configuration is done for the controller and this
+ * configuration is not tested.

Based on what is written you didn't test the DMA-based memory operations on your hardware. Well, this driver doesn't use DMA for memory operations on the platforms with the native CS just because nobody has implemented that feature so far. AFAICS if DMA-based memory operations were supported by the driver I don't think that the native CS auto de-assertion would have been an issue except when there is no hw-accelerated LLPs list handling in the DMA controller (in the later case we could have fallen back to the IRQ-less implementation though). Moreover having the DMA-based memory ops implemented would have been even better than what the driver provides at the moment since it would have eliminated the mem-op transfers in the atomic context. So the comment seems misleading. Another problem is that it refers to a feature which may be added in future. So the comment will be wrong then. So I would suggest to either drop the comment or change to something that just states that the DMA-based mem ops weren't tested for this hardware.

Am I wrong in some aspects of understanding your comment? Did you mean something else than what I inferred from it?

Quoted 2 lines

-Serge(y)

You have interpreted my comments correctly. I can see how the comment is misleading and can become obsolete in the future. I will shorten the comment to just indicated that no DMA-based mem ops are tested for this hardware.

Thanks,
Abe

Ok. Thanks. Could you submit a fixup patch then? Mark has already merged the series in as is:
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/commit/?id=0760d5d0e9f0c0e2200a0323a61d1995bb745dee

The Intel Mount Evans SoC's Integrated Management Complex has a DW apb_ssi_v4.02a controller. This series adds support for this controller.

No SoC level chip select override is provided and as such no DMA configuration is done for the controller.

Thanks,
Abe

[...]

Quoted 6 lines · Abe Kohandel

The Intel Mount Evans SoC's Integrated Management Complex has a DW apb_ssi_v4.02a controller. This series adds support for this controller.

No SoC level chip select override is provided and as such no DMA configuration is done for the controller.

Quoted 4 lines

Thanks,
Abe

[...]

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/2] spi: dw: Add compatible for Intel Mount Evans SoC

      commit: 0760d5d0e9f0c0e2200a0323a61d1995bb745dee
[2/2] dt-bindings: spi: snps,dw-apb-ssi: Add compatible for Intel Mount Evans SoC
      commit: 7bac98a338d63efb0b44ce4b79d53838491f00df

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Mark, next time please wait at least for a few days before applying. Give me a chance to review.)

* In this case I have had a question regarding the in-code comment which may have been needed to be fixed a bit.

Quoted 2 lines · Serge Semin

Mark, next time please wait at least for a few days before applying. Give me a chance to review.)

It's a trivial quirk for a platform, it seemed more hassle to wait TBH.

Quoted 2 lines

* In this case I have had a question regarding the in-code comment which may have been needed to be fixed a bit.

That can always be fixed incrementally if there's issues.