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

[Abe Kohandel]

On 23/06/07 06:28PM, Serge Semin wrote:
> 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:

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

I am currently setting dw_spi.num_cs via the num-cs property in the device
tree. Is one preferred over the other? I guess setting the dw_spi.num_cs in
code is safer than using the device tree. 

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

Oh sorry, I wasn't explicit enough. The HW handshaking signals are connected to
the DW SSI controller so we should be able to take advantage of that
acceleration and not have to go through the challenging steps you have
outlined.

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

Thanks for all the advice Serge, much appreciated! Hopefully I can come back
with a patch to enable the DMA engine for this platform in the near future.

Thanks,
Abe