Hi Louis,
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.
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.
--- a/drivers/mfd/ocelot-spi.c
+++ b/drivers/mfd/ocelot-spi.c
@@ -225,6 +228,8 @@ static int ocelot_spi_probe(struct spi_device *spi)
}
spi->bits_per_word = 8;
+ spi->word_delay.value = 1;
+ spi->word_delay.unit = SPI_DELAY_UNIT_NSECS;
err = spi_setup(spi);
if (err)
Colin Foster
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/[email protected]/
> --- a/drivers/mfd/ocelot-spi.c
> +++ b/drivers/mfd/ocelot-spi.c
> @@ -225,6 +228,8 @@ static int ocelot_spi_probe(struct spi_device *spi)
> }
>
> spi->bits_per_word = 8;
> + spi->word_delay.value = 1;
> + spi->word_delay.unit = SPI_DELAY_UNIT_NSECS;
>
> err = spi_setup(spi);
> if (err)
>
>
> Colin Foster
>
>
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
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/[email protected]/
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
On Thu, Jun 06, 2024 at 10:14:27PM -0500, Colin Foster wrote:
> So I think the question I have is:
> Should the CS line be de-asserted at the end of "spi_write"?
Absent bodging with cs_change after any spi message the chip select
should be left deasserted.
Hi Louis,
On Fri, Jun 07, 2024 at 04:45:43PM +0100, Mark Brown wrote:
> On Thu, Jun 06, 2024 at 10:14:27PM -0500, Colin Foster wrote:
>
> > So I think the question I have is:
>
> > Should the CS line be de-asserted at the end of "spi_write"?
>
> Absent bodging with cs_change after any spi message the chip select
> should be left deasserted.
Do you have hardware to reproduce my results of two spi messages no
longer toggling the CS line and leaving the line at GND through the
transactions?