Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 6 Jun 2023 20:28:36 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Abe Kohandel <abe.kohandel@intel.com>
Cc:     linux-spi@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, Mark Brown <broonie@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: Re: [PATCH 1/2] spi: dw: Add compatible for Intel Mount Evans SoC
Message-ID: <20230606172836.rbvhaxala2voaany@mobilestation>
References: <20230606145402.474866-1-abe.kohandel@intel.com>
 <20230606145402.474866-2-abe.kohandel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230606145402.474866-2-abe.kohandel@intel.com>
Precedence: bulk

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
>