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

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

[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.

Thanks,
Abe

Abe Kohandel (2):
  spi: dw: Add compatible for Intel Mount Evans SoC
  dt-bindings: spi: snps,dw-apb-ssi: Add compatible for Intel Mount
    Evans SoC

 .../bindings/spi/snps,dw-apb-ssi.yaml         |  2 ++
 drivers/spi/spi-dw-mmio.c                     | 29 +++++++++++++++++++
 2 files changed, 31 insertions(+)

-- 
2.40.1

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

[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 <abe.kohandel@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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.
+ */
+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

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

[Serge Semin]

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 <abe.kohandel@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  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
> 

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

[Abe Kohandel]

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 <abe.kohandel@intel.com>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  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

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

[Serge Semin]

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 <abe.kohandel@intel.com>
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  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

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

[Abe Kohandel]

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/20230606231844.726272-1-abe.kohandel@intel.com/

Thanks,
Abe

[PATCH 2/2] dt-bindings: spi: snps,dw-apb-ssi: Add compatible for Intel Mount Evans SoC

[Abe Kohandel]

Document the DesignWare SSI controller compatible for Intel Mount Evans
Integrated Management Complex SoC.

Signed-off-by: Abe Kohandel <abe.kohandel@intel.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 12ca108864c6..a47cb144b09f 100644
--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
@@ -74,6 +74,8 @@ properties:
         const: intel,keembay-ssi
       - description: Intel Thunder Bay SPI Controller
         const: intel,thunderbay-ssi
+      - description: Intel Mount Evans Integrated Management Complex SPI Controller
+        const: intel,mountevans-imc-ssi
       - description: AMD Pensando Elba SoC SPI Controller
         const: amd,pensando-elba-spi
       - description: Baikal-T1 SPI Controller
-- 
2.40.1

Re: [PATCH 0/2] spi: dw: Add compatible for Intel Mount Evans SoC

[Mark Brown]

On Tue, 06 Jun 2023 07:54:00 -0700, Abe Kohandel wrote:
> 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
> 
> [...]

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

Re: [PATCH 0/2] spi: dw: Add compatible for Intel Mount Evans SoC

[Serge Semin]

On Tue, Jun 06, 2023 at 05:24:36PM +0100, Mark Brown wrote:
> On Tue, 06 Jun 2023 07:54:00 -0700, Abe Kohandel wrote:
> > 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
> > 
> > [...]
> 
> Applied to
> 
>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

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.

-Serge(y)

> 
> 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
> 

Re: [PATCH 0/2] spi: dw: Add compatible for Intel Mount Evans SoC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 414 bytes --]

On Tue, Jun 06, 2023 at 07:40:40PM +0300, Serge Semin wrote:

> 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.

> * 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] spi: dw: Add compatible for Intel Mount Evans SoC

[Serge Semin]

On Tue, Jun 06, 2023 at 05:47:40PM +0100, Mark Brown wrote:
> On Tue, Jun 06, 2023 at 07:40:40PM +0300, Serge Semin wrote:
> 
> > 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.
> 
> > * 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.

Ok. I've sent my comment in that regards. Let's see what the author
responds.

-Serge(y)