Re: [PATCH v3 3/5] spi: dw: Add support for master mode selection for DWC SSI controller

[Serge Semin <fancer.lancer@xxxxxxxxx>]

On Thu, Nov 11, 2021 at 03:14:26PM +0000, Mark Brown wrote:
> On Thu, Nov 11, 2021 at 05:52:46PM +0300, Serge Semin wrote:
> > On Thu, Nov 11, 2021 at 02:16:05PM +0000, Mark Brown wrote:
> > > On Thu, Nov 11, 2021 at 02:51:59PM +0800, nandhini.srikandan@xxxxxxxxx wrote:
> 
> > > > Add support to select the controller mode as master mode by setting
> > > > Bit 31 of CTRLR0 register. This feature is supported for controller
> > > > versions above v1.02.
> 
> > > Clearly older versions of the controller can also run in this mode...
> 
> > Yes, but the driver doesn't support the slave mode at the moment.
> > So always enabling the master mode seems natural. (see my next comment
> > also concerning this matter)
> 

> The commit message makes it sound like master mode is only supported for
> the newer versions.

I meant it doesn't really matter if the bit has been reserved before
and the driver doesn't support the Slave-mode of the controller
anyway.
Regarding the Master-mode feature availability. Originally Wan added
that flag setting for v1.01a here:
https://patchwork.kernel.org/project/spi-devel-general/patch/20200312113129.8198-8-wan.ahmad.zainie.wan.mohamad@xxxxxxxxx/
Nandhini said in v2 that both Keem Bay and Thunder Bay uses DWC SSI
v1.02a and the BIT[31] functionality is not Intel-specific, but
generic for DWC SSIs.  So version-wise it's either Wan or Nandhini
ware mistaken at some point.

> 
> > > This makes the configuration unconditional, it's not gated by controller
> > > version checks or any kind of quirk any more meaning that if anything
> 
> > We have already discussed this feature in v2:
> > https://patchwork.kernel.org/project/spi-devel-general/patch/20210824085856.12714-3-nandhini.srikandan@xxxxxxxxx/
> > Since that bit has been reserved before 1.02a but is no available for
> > any DWC SSI controller and the driver doesn't support the SPI-slave mode
> > at the moment I suggested to just always set that flag for the DWC SSI
> > code. Please see my reply to Nandhini here:
> 

> Given that people seem to frequently customise these IPs when
> integrating them I wouldn't trust people not to have added some other
> control into that reserved bit doing some magic stuff that's useful in
> their system.

In that case the corresponding platform code would have needed to have
that peculiarity properly handled and not to use a generic compatibles
like "snps,dwc-ssi-1.01a" or "snps,dw-apb-ssi", which are supposed to
be utilized for the default IP-core configs only. For the sake of the
code simplification I'd stick to setting that flag for each generic
DWC SSI-compatible device. That will be also helpful for DWC SSIs
which for some reason have the slave-mode enabled by default.

Alternatively the driver could read the IP-core version from the
DW_SPI_VERSION register, parse it (since it's in ASCII) and then use
it in the conditional Master mode activation here. But that could have
been a better solution in case if the older IP-cores would have used
that bit for something special, while Nandhini claims it was reserved.
So in this case I would stick with a simpler approach until we get to
face any problem in this matter, especially seeing we already pocking
the reserved bits of the CTRL0 register in this driver in the
spi_hw_init() method when it comes to the DFS field width detection.

-Sergey