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
