Message-ID: <aUlXWwoqnG7llPwA@debian-BULLSEYE-live-builder-AMD64>
Date: Mon, 22 Dec 2025 11:36:11 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Petre Rodan <petre.rodan@...dimension.ro>
Cc: Jonathan Cameron <jic23@...nel.org>,
	David Lechner <dlechner@...libre.com>,
	Nuno Sá <nuno.sa@...log.com>,
	Andy Shevchenko <andy@...nel.org>,
	Andreas Klinger <ak@...klinger.de>, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH 09/14] iio: pressure: mprls0025pa: mitigate SPI CS delay
 violation

On 12/20, Petre Rodan wrote:
> 
> Hello Marcelo,
> 
> thank you for the pointers.
> 
> On Sat, Dec 20, 2025 at 01:51:42AM -0300, Marcelo Schmitt wrote:
> > On 12/18, Petre Rodan wrote:
> > > Based on the sensor datasheet in chapter 7.6 SPI timing, Table 20,
> > > during the SPI transfer there is a minimum time interval between the
> > > CS being asserted and the first clock edge (tHDSS).
> > > This minimum interval of 2.5us is being violated if two consecutive SPI
> > > transfers are queued up, at least on my SPI controller (omap2_mcspi) [1]
> > > As you can see in the first package that only contains a NOP the interval
> > > is 0.75us (half a 800kHz clock cycle).
> > > 
> > > This patch mitigates the problem by implementing a different measurement
> > > technique that does not involve checking for the EOC indicator before
> > > reading the conversion, thus making sure SPI transfers are not queued up.
> > The correct way of fixing that is with protocol specific delay. Generic delays
> > like fsleep won't guarantee any delay between CS drop and fist SCLK edge.
> 
> sorry for the lack of detail in the commit log, I forgot to add it.
> 
> this patch changes the measurement sequence and as a consequence it also mitigates a problem where protocol-specific delays are not implemented in the SPI controller (for the corner case when multiple SPI transfers are queued up).
> 
> as per the datasheet pages 36 and 41, the measurement sequence can follow one of these two scenarios, for both bus implementations:
> 
> a) send measurement command and wait for the status byte to lose the busy flag.
> this means that we would need to keep sending NOP commands and read the first (status) byte in a loop until the busy flag goes away. this is how the driver was designed before this series of patches.
> 
> b) send measurement command and wait for 5ms for the data conversion to occur.
> same as a, but instead of polling the sensor for the status byte there's just a 5ms+ delay after which only one NOP command is sent.
> the read that follows will get the conversion data plus the status byte.
> depending on the status byte flags, either error out or pass the conversion along.
> 
> my patch replaces the measurement sequence defined by scenario a with scenario b and it only affects users that did not define the EoC interrupt pin in the device tree overlay.
> the datasheet contains sample code (pages 38 and 43) and they also happen to use scenario b, so no polling.
> in my testing the sensor always performed as expected (the busy flag was never set after the 5ms fsleep()).
> 
> > For SPI, we unfortunately we don't have any interface to set a pre-SCLK delay
> > so I suggest to make an spi_message with a dummy transfer to cause the initial
> > 2.5 µs delay. E.g.
> 
> for normal (non-queued) SPI messages the delay that exist between the CS assert and the first clock is ~4us, well above the 2.5us minimum.
> the problem only appears when the SPI controller gets multiple xfers without any delays between them - which happens in scenario a when two transfers get queued up (NOP that reads the status byte and the NOP that reads the status + conversions).

Sure, as you say, the probable reason to have enough first SCLK delay was
because the SPI controller was not initially dealing with many transfers.
Though, can't the MPR read transfer lack enough first SCLK delay if a different
device on the same bus bursts SPI transfers during MPR device read?
fsleep() may fix it for single-device use case, but I don't think it is the
proper way of getting transfer timings correctly. To ensure that first SCLK
delay, you got to use protocol specific delays so that the controller will
be able to properly set that delay.

> 
> this is just a hunch but I think that the CS timing is directly controlled by hardware when the issue happens.
> see chapter 24.3.2.8 page 4903 in the AM335x technical reference manual [1].
> I'm measuring exactly 0.5 clock cycles CS lead times which is the default for the TCS bit of the register MCSPI_CH(i)CONF.
> 
> [1] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf?ts=1766215152875
> 
> > /* dummy transfer with no data, just cause the delay */
> > xfers[0].delay.value = 2500 * NSEC_PER_SEC;
> > xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;
> > 
> > /* normal data transfer  */
> > xfer[1].tx_buf = data->tx_buf;
> > xfer[1].rx_buf = data->rx_buf;
> > ...

See spi.h for documentation of the delays available for SPI transfers. As I
mentioned, we unfortunately don't have a first SCLK delay for SPI but I think
you may get something equivalent with a dummy transfer like the example above.

> > 
> > Also, I don't see how the proposed change is 'implementing a different
> > measurement technique'. Consider updating the commit description or providing
> > better explanation.
> 
> I feel that using scenario b defined above we cure the problem before it happens.
> my patch provides a less complex implementation, it's recommended by the manufacturer in their sample code and provides less code duplication (status byte parsing) with no downsides afaict.
> 
> > > see Option 2 in Table 19 SPI output measurement command.
> > > Note that Honeywell's example code also follows this technique for both i2c
> > > and SPI.
> > > 
> > > This change only affects users that do not define the EOC interrupt in the
> > > device tree.
> > > 
> > > Remove defines that are no longer used.
> > > 
> > > [1] https://pasteboard.co/66WN38MRI1wc.png
> > > 
> > > Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf?download=false
> > > Signed-off-by: Petre Rodan <petre.rodan@...dimension.ro>
> 
> best regards,
> peter
> 
> -- 
> petre rodan

Powered by blists - more mailing lists