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

[Serge Semin]

On Wed, Jun 07, 2023 at 09:53:41AM -0700, Abe Kohandel wrote:
> 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. 

Strictly speaking the "num-cs" property is supposed to be used for
generic DW APB SSI devices (compatible with the snps,dw* string) only
since the IP-core can be synthesized with up to 16 slave select lines
and originally it was considered as impossible to auto-detect the
number of lines (although it's actually possible just by test-writing
to the SER register - another idea for the driver improvement ;) ).
Meanwhile vendor-specific implementations of the DW APB/AHB SSI
controller are synthesized with the particular parameters values
including the number of slave-select signals. Thus the kernel driver
can easily infer all device parameters (like reg-io-width, num-cs,
etc) just based on the compatible string (that's what I did in the
spi-dw-bt1.o driver). The problem is that historically nobody cared
about too relaxed "num-cs" utilization and now we can't update the
driver and the bindings to strictly follow that constraint. The only
solution is to implement the SER-writability-based auto-detection
procedure but it isn't free of risks to break some older devices
(though this problem can be avoided by making it optional).

To sum it up and answering to your question, it's preferable to
directly initialize dw_spi.num_cs in driver if the value is known.

Note though that the DW APB SSI driver currently has a limitation of
GPIO-based chip-selects usage: a total number of chip-selects must not
exceed a number of the native chip select lines (except AMD Pensando
Elba device). It's because in order to have the controller performing
the SPI transfers at least one SER flag must be set. Since we can't
predict which native CS is free from being utilized on a platform
(SPI devices can be instantiated from user-space), the GPIO-based
lines are activated together with the corresponding SER flag.

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

Always welcome. Feel free to ask should any question arise on that
journey.

-Serge(y)

> 
> Thanks,
> Abe