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

[Serge Semin]

On Wed, Dec 08, 2021 at 11:03:03AM +0000, Srikandan, Nandhini wrote:
> 
> 
> > -----Original Message-----
> > From: Srikandan, Nandhini
> > Sent: Wednesday, November 17, 2021 5:29 PM
> > To: Serge Semin <fancer.lancer@xxxxxxxxx>; Mark Brown
> > <broonie@xxxxxxxxxx>
> > Cc: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>; robh+dt@xxxxxxxxxx;
> > linux-spi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; mgross@xxxxxxxxxxxxxxx; Pan, Kris
> > <kris.pan@xxxxxxxxx>; Demakkanavar, Kenchappa
> > <kenchappa.demakkanavar@xxxxxxxxx>; Zhou, Furong
> > <furong.zhou@xxxxxxxxx>; Sangannavar, Mallikarjunappa
> > <mallikarjunappa.sangannavar@xxxxxxxxx>; Vaidya, Mahesh R
> > <mahesh.r.vaidya@xxxxxxxxx>; A, Rashmi <Rashmi.A@xxxxxxxxx>
> > Subject: RE: [PATCH v3 3/5] spi: dw: Add support for master mode selection
> > for DWC SSI controller
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Serge Semin <fancer.lancer@xxxxxxxxx>
> > > Sent: Wednesday, November 17, 2021 12:46 AM
> > > To: Mark Brown <broonie@xxxxxxxxxx>; Srikandan, Nandhini
> > > <nandhini.srikandan@xxxxxxxxx>
> > > Cc: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>;
> > > robh+dt@xxxxxxxxxx; linux-spi@xxxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > > mgross@xxxxxxxxxxxxxxx; Pan, Kris <kris.pan@xxxxxxxxx>; Demakkanavar,
> > > Kenchappa <kenchappa.demakkanavar@xxxxxxxxx>; Zhou, Furong
> > > <furong.zhou@xxxxxxxxx>; Sangannavar, Mallikarjunappa
> > > <mallikarjunappa.sangannavar@xxxxxxxxx>; Vaidya, Mahesh R
> > > <mahesh.r.vaidya@xxxxxxxxx>; A, Rashmi <rashmi.a@xxxxxxxxx>
> > > 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@xxxxxxxxxxxxxxxxxxxx/
> > >
> > > 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
> > Sure, I will test this patch set on our hardware and then use the IP-core
> > version infrastructure support and make the changes as mentioned for MST
> > bit.
> > 
> > - Nandhini

> I have tested the patches on our hardware and it is working fine. When would these patches be available in mainline? So that I can start applying the changes on top of these patches and share for review.
> - Nandhini 

These changes have been merged into the Mark' repo in the spi-next
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/log/?h=for-next
The series together with the rest of the SPI-related changed will get
into the mainline during the merge window.
For the time being you can rebase your patches on top of the Mark' repo
and if it works well for you just post it out for review. If it looks
good you'll even be able to have your series merged into the kernel in
the next merge-window.

-Sergey

>