linux-kernel - Re: [PATCH v4 1/6] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 20 Jun 2024 12:14:57 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Mark Brown <broonie@...nel.org>
Cc: David Lechner <dlechner@...libre.com>,
	Marcelo Schmitt <marcelo.schmitt@...log.com>, lars@...afoo.de,
	Michael.Hennerich@...log.com, jic23@...nel.org, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	nuno.sa@...log.com, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-spi@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/6] spi: Enable controllers to extend the SPI
 protocol with MOSI idle configuration

On 06/19, Mark Brown wrote:
> On Wed, Jun 19, 2024 at 03:58:00PM -0300, Marcelo Schmitt wrote:
> > On 06/19, David Lechner wrote:
> > > On 6/18/24 6:10 PM, Marcelo Schmitt wrote:
> 
> > > > +In this extension to the usual SPI protocol, the MOSI line state is specified to
> > > > +be kept high when CS is active but the controller is not clocking out data to
> 
> > > I think it would be less ambiguous to say "asserted" instead of "active".

ack, replaced "active" by "asserted" when describing CS state for v5.

> 
> > I'm not sure. IMHO, it looks less ambiguous to say a CS is active.
> > I think the most common for CS lines is to have a CS that is active low (i.e.
> > the line is at a low voltage level when the controller is selecting the device).
> > To me, "assert" sounds closer to the idea o setting something (like a bit to 1),
> > which is the opposite of active low CS.
> > Though, no strong opinion about it.
> > I go with what the maintainers prefer.
> 
> I think they're synonyms but asserted is the more common term for chip
> selects.
> 
> 
> > > > +#define SPI_CONTROLLER_MOSI_IDLE_LOW    BIT(8)  /* Can idle MOSI low */
> > > > +#define SPI_CONTROLLER_MOSI_IDLE_HIGH   BIT(9)  /* Can idle MOSI high */
> 
> > > I don't see where these are used anywhere else in the series. They
> > > seem redundant with SPI_MOSI_IDLE_LOW and SPI_MOSI_IDLE_HIGH.
> 
> > Good point.
> > They are currently not being used.
> > Comparing with what we have for SPI_CONTROLLER_MULTI_CS, I'm thinking it may be
> > handy to have dt properties to indicate controller MOSI idle capabilities.
> > Does that sound reasonable?
> 
> We shouldn't need DT properties, we should just know if the controller
> supports this based on knowing what controller is, and I'd not expect it
> to depend on board wiring.

Okay, though, I fail to see the need for 
#define SPI_CONTROLLER_MOSI_IDLE_LOW    BIT(8)  /* Can idle MOSI low */
#define SPI_CONTROLLER_MOSI_IDLE_HIGH   BIT(9)  /* Can idle MOSI high */

It looks like SPI_CONTROLLER bits are used to tweak controller operation in
various ways.
Right now, I'm not aware of any additional tweak needed to support the MOSI idle
feature. I have tested that on Raspberry Pi with bitbang/gpio controller and on
CoraZ7 with spi-engine and it did work fine in those setups.
Anyway, I'm prone to implement any additional changes to make this set better.

Thanks,
Marcelo