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

[Abe Kohandel <abe.kohandel@intel.com>]

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