Re: [PATCH] spi: dw: Remove misleading comment for Mount Evans SoC

From: Serge Semin <fancer.lancer@gmail.com>
To: Abe Kohandel <abe.kohandel@intel.com>
Cc: linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org,
	Mark Brown <broonie@kernel.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: Re: [PATCH] spi: dw: Remove misleading comment for Mount Evans SoC
Date: Wed, 7 Jun 2023 18:28:28 +0300	[thread overview]
Message-ID: <20230607152828.4nxefvimokamhgvu@mobilestation> (raw)
In-Reply-To: <ZICboAIZAcnYzyJr@ekohande-desk2>

On Wed, Jun 07, 2023 at 08:00:48AM -0700, Abe Kohandel wrote:
> On 23/06/07 02:27PM, Serge Semin wrote:
> > On Tue, Jun 06, 2023 at 04:18:44PM -0700, Abe Kohandel wrote:
> > > Remove a misleading comment about the DMA operations of the Intel Mount
> > > Evans SoC's SPI Controller as requested by Serge.
> > > 
> > 
> > > Signed-off-by: Abe Kohandel <abe.kohandel@intel.com>
> > > Link: https://lore.kernel.org/linux-spi/20230606191333.247ucbf7h3tlooxf@mobilestation/
> > > Fixes: 0760d5d0e9f0 ("spi: dw: Add compatible for Intel Mount Evans SoC")
> > 
> > Note Fixes tag normally goes first. In this case it seems redundant
> > though.
> > 
> 
> Thanks, will do this in the future.
> 
> > > - * 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.
> > 
> > I had nothing against this part of the comment but only about the
> > second chunk of the text.
> 

> Thinking about it a bit more there is nothing precluding this controller from
> being used for other purposes in the future. It is configured with two chip
> selects, only one of which is used today. I removed it to so it wouldn't become
> inaccurate if that happens.

Ok. Regarding the number of chip-selects. You could have overwritten
the dw_spi.num_cs field with value 2 then in the dw_spi_mountevans_imc_init()
method. Thus having a bit safer driver for your platform.

> 
> > > + * DMA-based mem ops are not configured for this device and are not tested.
> > 
> > * Note mem-ops is just a feature of the DW APB/AHB SSI controllers
> > * which provides a way to perform write-then-read and write-only
> > * transfers (see Transmit only and EEPROM read transfer modes in the
> > * hw manual). It works irrespective of whether your controller has a
> > * DMA-engine connected or doesn't have. Modern DW SSI controllers
> > * support Enhanced SPI modes with the extended SPI-bus width
> > * capability. But it's a whole another story and such modes aren't
> > * currently supported by the driver.
> > 
> > Just a question for the sake of the discussion history. Does your
> > platform have a DMA-engine synthesized to work with this DW SSI
> > controller? That is does your controller has the DMA Controller
> > Interface (handshake signals) connected to any DMA-engine on your
> > platform? I am asking because if there is no such DMA-engine then
> > the last part of your statement is just redundant since you can't test
> > something which isn't supported by design.
> 

> The platform does have a DMA-engine synthesized but I have been having some
> challenges with getting it to work which may require some further quirks added
> to the DMA driver. 

The main question is whether that DMA-engine has the handshake signals
connected to the DW SSI controller. If it doesn't then adding such
engine support would be a great deal of challenge indeed because a
software-based handshaking interface would need to be added to the
DMA-engine subsystem first. Then the DW SSI driver would need to be
fixed to work with that interface. Taking a FIFO-size into account and
an amount of IRQs to handle on each handshaking round, the resultant
performance might get to be not worth all the efforts so a simple
IRQ-based transfers implementation may work better.

> One example being the system uses 40-bit addressing but the
> DMA-engine is only synthesized with 32-bit address capability and is meant to
> only target a specific region of memory where it "knowns" the upper byte of the
> address.

That's a pretty much well known problem. The kernel has a solution for
it: DMA-mask set for the DMA-engine device (see dma_set_mask() and
dma_set_mask_and_coherent()) and SWIOTLB (formerly known as bounce
buffers).

Alternatively modern CPUs are normally equipped with a thing like
IOMMU, which can be used to remap the limited device address space to
any CPU/RAM address.

-Serge(y)

> 
> Anyhow, I hope to work through some of those challenges and enable this in the
> future.
> 
> Thanks,
> Abe