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

[Serge Semin <fancer.lancer@gmail.com>]

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