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

From: Mark Brown <broonie@kernel.org>
To: Serge Semin <fancer.lancer@gmail.com>
Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>,
	nandhini.srikandan@intel.com, robh+dt@kernel.org,
	linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, mgross@linux.intel.com,
	kris.pan@intel.com, kenchappa.demakkanavar@intel.com,
	furong.zhou@intel.com, mallikarjunappa.sangannavar@intel.com,
	mahesh.r.vaidya@intel.com, rashmi.a@intel.com
Subject: Re: [PATCH v3 3/5] spi: dw: Add support for master mode selection for DWC SSI controller
Date: Thu, 11 Nov 2021 16:25:02 +0000	[thread overview]
Message-ID: <YY1D3tM4fg8h6mmj@sirena.org.uk> (raw)
In-Reply-To: <20211111160627.fcgrvj2k7x3lwtkp@mobilestation>

[-- Attachment #1: Type: text/plain, Size: 1946 bytes --]

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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]