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

Commit Message

Abe Kohandel June 6, 2023, 2:54 p.m. UTC

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 <[e-mail]>
Reviewed-by: Andy Shevchenko <[e-mail]>
---
 drivers/spi/spi-dw-mmio.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Serge Semin June 6, 2023, 5:28 p.m. UTC | #1

Hi Abe

On Tue, Jun 06, 2023 at 07:54:01AM -0700, Abe Kohandel wrote:
> 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 <[e-mail]>
> Reviewed-by: Andy Shevchenko <[e-mail]>
> ---
>  drivers/spi/spi-dw-mmio.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> index 5f2aee69c1c1..c1d16157de61 100644
> --- a/drivers/spi/spi-dw-mmio.c
> +++ b/drivers/spi/spi-dw-mmio.c
> @@ -236,6 +236,31 @@ static int dw_spi_intel_init(struct platform_device *pdev,
>  	return 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)

> + */
> +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)
>  {
> @@ -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},
> -- 
> 2.40.1
>

Abe Kohandel June 6, 2023, 7:07 p.m. UTC | #2

Hi Serge,

On 23/06/06 08:28PM, Serge Semin wrote:
> Hi Abe
> 
> On Tue, Jun 06, 2023 at 07:54:01AM -0700, Abe Kohandel wrote:
> > 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 <[e-mail]>
> > Reviewed-by: Andy Shevchenko <[e-mail]>
> > ---
> >  drivers/spi/spi-dw-mmio.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> > index 5f2aee69c1c1..c1d16157de61 100644
> > --- a/drivers/spi/spi-dw-mmio.c
> > +++ b/drivers/spi/spi-dw-mmio.c
> > @@ -236,6 +236,31 @@ static int dw_spi_intel_init(struct platform_device *pdev,
> >  	return 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)
> 

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

Serge Semin June 6, 2023, 7:13 p.m. UTC | #3

On Tue, Jun 06, 2023 at 12:07:56PM -0700, Abe Kohandel wrote:
> Hi Serge,
> 
> On 23/06/06 08:28PM, Serge Semin wrote:
> > Hi Abe
> > 
> > On Tue, Jun 06, 2023 at 07:54:01AM -0700, Abe Kohandel wrote:
> > > 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 <[e-mail]>
> > > Reviewed-by: Andy Shevchenko <[e-mail]>
> > > ---
> > >  drivers/spi/spi-dw-mmio.c | 29 +++++++++++++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > > 
> > > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> > > index 5f2aee69c1c1..c1d16157de61 100644
> > > --- a/drivers/spi/spi-dw-mmio.c
> > > +++ b/drivers/spi/spi-dw-mmio.c
> > > @@ -236,6 +236,31 @@ static int dw_spi_intel_init(struct platform_device *pdev,
> > >  	return 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)
> > 
>
 
> 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.

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

-Serge(y)

> 
> Thanks,
> Abe

Abe Kohandel June 6, 2023, 11:21 p.m. UTC | #4

Hi Serge,

> 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

Patch submited in:
  https://lore.kernel.org/linux-spi/[e-mail]/

Thanks,
Abe

Patch

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index 5f2aee69c1c1..c1d16157de61 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -236,6 +236,31 @@  static int dw_spi_intel_init(struct platform_device *pdev,
 	return 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.
+ */
+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)
 {
@@ -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},