Message-ID: <20211116191542.vc42cxvflzn66ien@mobilestation>
Date:   Tue, 16 Nov 2021 22:15:42 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Mark Brown <broonie@...nel.org>, nandhini.srikandan@...el.com
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        robh+dt@...nel.org, linux-spi@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        mgross@...ux.intel.com, kris.pan@...el.com,
        kenchappa.demakkanavar@...el.com, furong.zhou@...el.com,
        mallikarjunappa.sangannavar@...el.com, mahesh.r.vaidya@...el.com,
        rashmi.a@...el.com
Subject: Re: [PATCH v3 3/5] spi: dw: Add support for master mode selection
 for DWC SSI controller

On Thu, Nov 11, 2021 at 04:25:02PM +0000, Mark Brown wrote:
> On Thu, Nov 11, 2021 at 07:06:27PM +0300, Serge Semin wrote:
> > On Thu, Nov 11, 2021 at 03:14:26PM +0000, Mark Brown wrote:
> 
> > > 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.
> 

> That's easier right up until the point where it explodes - I'd prefer to
> be more conservative here.  Fixing things up after the fact gets painful
> when people end up only finding the bug in released kernels, especially
> if it's distro end users or similar rather than developers.

Since IP-core and components versions is now supported that will easy
to implement. Thanks for merging the corresponding series in BTW.

> 
> > 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.
> 
> If the device has a version register checking that seems ideal - the
> infrastructure will most likely be useful in future anyway.  A bit of a
> shame that it's an ASCII string though.

That's what the patchset has been implemented for in the first place
https://lore.kernel.org/linux-spi/20211115181917.7521-1-Sergey.Semin@baikalelectronics.ru/

Nandhini, Mark has just merged in the series that adds the IP-core
versions infrastructure support to the DW SSI driver.  So now you can
easily convert this patch to be using that new interface like this:
-               if (dws->caps & DW_SPI_CAP_KEEMBAY_MST)
-                       cr0 |= DWC_SSI_CTRLR0_KEEMBAY_MST;
+               /* CTRLR0[31] MST */
+		if (dw_spi_ver_is_ge(dws, HSSI, 102A))
+       	        cr0 |= DWC_HSSI_CTRLR0_MST;

Please don't forget to convert the DWC_SSI_CTRLR0_KEEMBAY_MST name to
something like DWC_HSSI_CTRLR0_MST and place it at the top of the DWC
HSSI CTRLR0 register macros list.

-Sergey

Powered by blists - more mailing lists