Message-ID: <ZmJ7E305ow91ez2U@euler>
Date: Thu, 6 Jun 2024 22:14:27 -0500
From: Colin Foster <colin.foster@...advantage.com>
To: Mark Brown <broonie@...nel.org>, linux-spi@...r.kernel.org,
	linux-kernel@...r.kernel.org, miquel.raynal@...tlin.com
Subject: Re: omap2-mcspi multi mode

Hi Louis,

On Thu, Jun 06, 2024 at 10:06:07AM +0200, Louis Chauvet wrote:
> Le 04/06/24 - 22:04, Colin Foster a écrit :
> > Hi Louis,
> 
> Hi,
>  
> > I found that commit e64d3b6fc9a3 ("spi: omap2-mcpsi: Enable MULTI-mode
> > in more situations") caused a regression in the ocelot_mfd driver. It
> > essentially causes the boot to hang during probe of the SPI device.
> 
> I don't know what can cause this. My patch can "compact" few words into 
> only a bigger one, so the signal on the cable can change, but it follows 
> the SPI specification and the device should have the same behavior.
> 
> Instead of two very distinct words (for example two 8 bits words):
> 
>   <-- first word -->             <-- second word -->
>    _   _   _   _   _              _   _   _   _   _
> __| |_| |_| ... |_| |____________| |_| |_| ... |_| |_
> 
> The signal on the wire will be merged into one bigger (one 16 bits word):
> 
>   <-- first word -->  <-- second word -->
>    _   _   _   _   _   _   _   _   _   _
> __| |_| |_| ... |_| |_| |_| |_| ... |_| |_
> 
> > The following patch restores functionality. I can hook up a logic
> > analyzer tomorrow to get some more info, but I wanted to see if you had
> > any ideas.
>  
> I don't understand the link between the solution and my patch, can you 
> share the logic analyzer results?
> 
> Maybe the issue is the same as [1]? Does it solves the issue?
> 
> [1]: https://lore.kernel.org/all/20240506-fix-omap2-mcspi-v2-1-d9c77ba8b9c7@bootlin.com/

I took three measurements:

1. My patch added
2. No patches
3. The 'fix' patch applied from [1]

2 and 3 appear to behave the same for me. But CS is certainly the issue
I'm seeing. Here's a quick description:

A write on this chip is seven bytes - three bytes address and four bytes
data. Every write in 1, 2, and 3 starts with a CS assertion, 7 bytes,
and a CS de-assertion. Writes work.

A read is 8 bytes - three for address, one padding, and four data.
Writes in 1 start and end with CS asserting and de-asserting. Reads in
2 and 3 assert CS and combine multiple writes, which fails. Reads no
longer work as a result.

I thought maybe the lack of cs_change might be the culprit, but this
didn't resolve the issue either:

@@ -172,8 +175,13 @@ static int ocelot_spi_regmap_bus_write(void *context, const void *data, size_t c
 {
        struct device *dev = context;
        struct spi_device *spi = to_spi_device(dev);
+       struct spi_transfer     t = {
+                       .tx_buf         = data,
+                       .len            = count,
+                       .cs_change      = 1,
+               };

-       return spi_write(spi, data, count);
+       return spi_sync_transfer(spi, &t, 1);
 }


The relevant documentation on cs_change:

 * (ii) When the transfer is the last one in the message, the chip may
 * stay selected until the next transfer.  On multi-device SPI busses
 * with nothing blocking messages going to other devices, this is just
 * a performance hint; starting a message to another device deselects
 * this one.  But in other cases, this can be used to ensure correctness.
 * Some devices need protocol transactions to be built from a series of
 * spi_message submissions, where the content of one message is determined
 * by the results of previous messages and where the whole transaction
 * ends when the chipselect goes inactive.

And relevant code around cs_change:
https://elixir.bootlin.com/linux/v6.10-rc2/source/drivers/spi/spi.c#L1715


So I think the question I have is:

Should the CS line be de-asserted at the end of "spi_write"?

If yes, the multi mode patch seems to break this on 8-byte transfers.

If no, then how can I ensure this?


Thanks

Colin Foster

Powered by blists - more mailing lists