| Message ID | 20211111065201.10249-2-nandhini.srikandan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for Intel Thunder Bay SPI controller | expand |
On Thu, Nov 11, 2021 at 02:51:57PM +0800, nandhini.srikandan@intel.com wrote: > + snps,sste: > + description: Slave select line will toggle between consecutive > + data frames, with the serial clock being held to its default > + value while slave select line is high. > + type: boolean This is not something that should be configured in the DT, it needs to be controlled by the client driver. Changing this without involving the client driver will lead to data corruption.
Hello Nandhini, Mark On Thu, Nov 11, 2021 at 02:51:57PM +0800, nandhini.srikandan@intel.com wrote: > From: Nandhini Srikandan <nandhini.srikandan@intel.com> > > Add Slave Select Toggle Enable(SSTE) support for DWC SSI controller. Nandhini, as Mark said this is no need in this new property since that feature is supposed to be enabled by the client drivers by means of setting the SPI_CS_WORD flag in the spi_device->mode field. (See its usage for reference.) BTW Mark, why not to have a generic DT-property which would set that flag automatically by the SPI-core subsystem seeing it's indeed a client device-property? For instance there can be some property like "spi-cs-toggle" DT-property which when specified for the particular SPI-client DT-node will make the SPI-core subsystem to set the SPI_CS_WORD flag of the device mode? Like it has already been done for "spi-cs-high"/"spi-lsb-first"/etc. In this case Nandhini would need to just convert this patch a bit so to be fixing the Documentation/devicetree/bindings/spi/spi-controller.yaml bindings instead. -Sergey > > Signed-off-by: Nandhini Srikandan <nandhini.srikandan@intel.com> > --- > Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml > index ca91201a9926..866416d01e94 100644 > --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml > +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml > @@ -149,6 +149,12 @@ patternProperties: > is an optional feature of the designware controller, and the > upper limit is also subject to controller configuration. > > + snps,sste: > + description: Slave select line will toggle between consecutive > + data frames, with the serial clock being held to its default > + value while slave select line is high. > + type: boolean > + > unevaluatedProperties: false > > required: > -- > 2.17.1 >
On Thu, Nov 11, 2021 at 05:31:08PM +0300, Serge Semin wrote: > BTW Mark, why not to have a generic DT-property which would set that > flag automatically by the SPI-core subsystem seeing it's indeed a > client device-property? For instance there can be some property like > "spi-cs-toggle" DT-property which when specified for the particular > SPI-client DT-node will make the SPI-core subsystem to set the Anything like this is fundamentally part of the wire protocol for the device, there's no need for an extra property on top of the compatible for the device and the driver really, really needs to know what's going on to avoid data corruption. You could also use this feature together with varying the word size as an optimisation at runtime (eg, do long sequences of register writes in a single hardware operation by setting an appropriate word length to cause the controller to bounce chip select between writes). > SPI_CS_WORD flag of the device mode? Like it has already been done for > "spi-cs-high"/"spi-lsb-first"/etc. I don't think either of those properties was a good idea, there's a bunch of stuff in the older SPI bindings that don't make much sense.
On Thu, Nov 11, 2021 at 03:01:12PM +0000, Mark Brown wrote: > On Thu, Nov 11, 2021 at 05:31:08PM +0300, Serge Semin wrote: > > > BTW Mark, why not to have a generic DT-property which would set that > > flag automatically by the SPI-core subsystem seeing it's indeed a > > client device-property? For instance there can be some property like > > "spi-cs-toggle" DT-property which when specified for the particular > > SPI-client DT-node will make the SPI-core subsystem to set the > > Anything like this is fundamentally part of the wire protocol for the > device, there's no need for an extra property on top of the compatible > for the device and the driver really, really needs to know what's going > on to avoid data corruption. You could also use this feature together > with varying the word size as an optimisation at runtime (eg, do long > sequences of register writes in a single hardware operation by setting > an appropriate word length to cause the controller to bounce chip > select between writes). > > > SPI_CS_WORD flag of the device mode? Like it has already been done for > > "spi-cs-high"/"spi-lsb-first"/etc. > > I don't think either of those properties was a good idea, there's a > bunch of stuff in the older SPI bindings that don't make much sense. Ok. Thanks for clarification. No new DT-property then. Nandhini, could you please drop this patch in v4? One more time I'm sorry for misleading you on v2. -Sergey
> -----Original Message----- > From: Serge Semin <fancer.lancer@gmail.com> > Sent: Thursday, November 11, 2021 8:37 PM > To: Mark Brown <broonie@kernel.org>; Srikandan, Nandhini > <nandhini.srikandan@intel.com> > Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>; robh+dt@kernel.org; > linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; mgross@linux.intel.com; Pan, Kris > <kris.pan@intel.com>; Demakkanavar, Kenchappa > <kenchappa.demakkanavar@intel.com>; Zhou, Furong > <furong.zhou@intel.com>; Sangannavar, Mallikarjunappa > <mallikarjunappa.sangannavar@intel.com>; Vaidya, Mahesh R > <mahesh.r.vaidya@intel.com>; A, Rashmi <rashmi.a@intel.com> > Subject: Re: [PATCH v3 1/5] dt-bindings: spi: Add SSTE support for DWC SSI > controller > > On Thu, Nov 11, 2021 at 03:01:12PM +0000, Mark Brown wrote: > > On Thu, Nov 11, 2021 at 05:31:08PM +0300, Serge Semin wrote: > > > > > BTW Mark, why not to have a generic DT-property which would set that > > > flag automatically by the SPI-core subsystem seeing it's indeed a > > > client device-property? For instance there can be some property like > > > "spi-cs-toggle" DT-property which when specified for the particular > > > SPI-client DT-node will make the SPI-core subsystem to set the > > > > Anything like this is fundamentally part of the wire protocol for the > > device, there's no need for an extra property on top of the compatible > > for the device and the driver really, really needs to know what's > > going on to avoid data corruption. You could also use this feature > > together with varying the word size as an optimisation at runtime (eg, > > do long sequences of register writes in a single hardware operation by > > setting an appropriate word length to cause the controller to bounce > > chip select between writes). > > > > > SPI_CS_WORD flag of the device mode? Like it has already been done > > > for "spi-cs-high"/"spi-lsb-first"/etc. > > > > I don't think either of those properties was a good idea, there's a > > bunch of stuff in the older SPI bindings that don't make much sense. > > Ok. Thanks for clarification. No new DT-property then. > > Nandhini, could you please drop this patch in v4? One more time I'm sorry > for misleading you on v2. Sure, I will make use of SPI_CS_WORD flag and drop this patch in next version. > > -Sergey
diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml index ca91201a9926..866416d01e94 100644 --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml @@ -149,6 +149,12 @@ patternProperties: is an optional feature of the designware controller, and the upper limit is also subject to controller configuration. + snps,sste: + description: Slave select line will toggle between consecutive + data frames, with the serial clock being held to its default + value while slave select line is high. + type: boolean + unevaluatedProperties: false required: