| 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
| ||
|
Message-ID: <ZnMqOAPc3IXP-eHC@debian-BULLSEYE-live-builder-AMD64>
Date: Wed, 19 Jun 2024 15:58:00 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: David Lechner <dlechner@...libre.com>
Cc: Marcelo Schmitt <marcelo.schmitt@...log.com>, broonie@...nel.org,
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, David Lechner wrote:
> On 6/18/24 6:10 PM, Marcelo Schmitt wrote:
>
>
> > +
> > +MOSI idle state configuration
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +Common SPI protocol implementations don't specify any state or behavior for the
> > +MOSI line when the controller is not clocking out data. However, there do exist
> > +peripherals that require specific MOSI line state when data is not being clocked
> > +out. For example, if the peripheral expects the MOSI line to be high when the
> > +controller is not clocking out data (SPI_MOSI_IDLE_HIGH), then a transfer in SPI
> > +mode 0 would look like the following:
> > +
> > +::
> > +
> > + nCSx ___ ___
> > + \_________________________________________________________________/
> > + • •
> > + • •
> > + SCLK ___ ___ ___ ___ ___ ___ ___ ___
> > + _______/ \___/ \___/ \___/ \___/ \___/ \___/ \___/ \_____
> > + • : ; : ; : ; : ; : ; : ; : ; : ; •
> > + • : ; : ; : ; : ; : ; : ; : ; : ; •
> > + MOSI _____ _______ _______ _______________ ___
> > + 0x56 \_0_____/ 1 \_0_____/ 1 \_0_____/ 1 1 \_0_____/
> > + • ; ; ; ; ; ; ; ; •
> > + • ; ; ; ; ; ; ; ; •
> > + MISO XXX__________ _______________________ _______ XXX
> > + 0xBA XXX__/ 1 \_____0_/ 1 1 1 \_____0__/ 1 \____0__XXX
> > +
> > +Legend::
> > +
> > + • marks the start/end of transmission;
> > + : marks when data is clocked into the peripheral;
> > + ; marks when data is clocked into the controller;
> > + X marks when line states are not specified.
> > +
> > +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".
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.
>
> > +the peripheral and also when CS is inactive.
>
> As I mentioned in a previous review, I think the key detail here is that the
> MOSI line has to be in the required state during the CS line assertion
> (falling edge). I didn't really get that from the current wording. The current
> wording makes it sound like MOSI needs to be high indefinitely longer.
It may be that we only need MOSI high just before bringing CS low. Though,
I don't see that info in the datasheets. How much time would MOSI be required
to be high prior to bringing CS low? The timing diagrams for register access and
ADC sampling in "3-wire" mode all start and end with MOSI at logical 1 (high).
I think reg access work if MOSI is brought low after CS gets low, but sample
read definitely don't work.
>From the info available in datasheets, it looks like MOSI is indeed expected
to be high indefinitely amount of time. Except when the controller is clocking
out data to the peripheral.
Even if find out the amount of time MOSI would be required high prior to CS low,
then we would need some sort of MOSI high/low state set with a delay prior to
active CS. That might be enough to support the AD4000 series of devices but,
would it be worth the added complexity?
>
> > +
> > +Peripherals that require this extension must request it by setting the
> > +SPI_MOSI_IDLE_HIGH bit into the mode attribute of their struct spi_device and
>
> Could use inline code formatting for C code bits, e.g. ``struct spi_device``
> ``SPI_MOSI_IDLE_HIGH``, etc.
ok, updated those for v5.
>
> > +call spi_setup(). Controllers that support this extension should indicate it by> +setting SPI_MOSI_IDLE_HIGH in the mode_bits attribute of their struct
> > +spi_controller. The configuration to idle MOSI low is analogous but uses the
> > +SPI_MOSI_IDLE_LOW mode bit.
> > +
> > +
> > THANKS TO
> > ---------
> > Contributors to Linux-SPI discussions include (in alphabetical order,
>
> ...
>
> > index e8e1e798924f..8e50a8559225 100644
> > --- a/include/linux/spi/spi.h
> > +++ b/include/linux/spi/spi.h
> > @@ -599,6 +599,12 @@ struct spi_controller {
> > * assert/de-assert more than one chip select at once.
> > */
> > #define SPI_CONTROLLER_MULTI_CS BIT(7)
> > + /*
> > + * The spi-controller is capable of keeping the MOSI line low or high
> > + * when not clocking out data.
> > + */
> > +#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?
> >
> > /* Flag indicating if the allocation of this struct is devres-managed */
> > bool devm_allocated;
> > diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h
> > index ca56e477d161..ee4ac812b8f8 100644
> > --- a/include/uapi/linux/spi/spi.h
> > +++ b/include/uapi/linux/spi/spi.h
> > @@ -28,7 +28,8 @@
> > #define SPI_RX_OCTAL _BITUL(14) /* receive with 8 wires */
> > #define SPI_3WIRE_HIZ _BITUL(15) /* high impedance turnaround */
> > #define SPI_RX_CPHA_FLIP _BITUL(16) /* flip CPHA on Rx only xfer */
> > -#define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave mosi line low when idle */
> > +#define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave MOSI line low when idle */
> > +#define SPI_MOSI_IDLE_HIGH _BITUL(18) /* leave MOSI line high when idle */
> >
> > /*
> > * All the bits defined above should be covered by SPI_MODE_USER_MASK.
> > @@ -38,6 +39,6 @@
> > * These bits must not overlap. A static assert check should make sure of that.
> > * If adding extra bits, make sure to increase the bit index below as well.
> > */
> > -#define SPI_MODE_USER_MASK (_BITUL(18) - 1)
> > +#define SPI_MODE_USER_MASK (_BITUL(19) - 1)
> >
> > #endif /* _UAPI_SPI_H */
>
Powered by blists - more mailing lists