diff mbox series

omap2-mcspi multi mode

Message ID Zl/V0dU6SjAMkpLG@colin-ia-desktop (mailing list archive)
State New, archived
Headers show
Series omap2-mcspi multi mode | expand

Commit Message

Colin Foster June 5, 2024, 3:04 a.m. UTC 
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.



Colin Foster

Comments

Louis Chauvet June 6, 2024, 8:06 a.m. UTC | #1 
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/

> --- 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
> 
>
Colin Foster June 7, 2024, 3:14 a.m. UTC | #2 
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
Mark Brown June 7, 2024, 3:45 p.m. UTC | #3 
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.
Colin Foster June 11, 2024, 2:21 p.m. UTC | #4 
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?
Thorsten Leemhuis June 21, 2024, 8:21 a.m. UTC | #5 
On 11.06.24 16:21, Colin Foster wrote:
> 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?

Hmmm, I might have missed something, but it looks like nothing happened
since that exchange. Did this regression fall through the cracks or can
I consider the issue resolved for some reason?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke
Colin Foster June 21, 2024, 4:56 p.m. UTC | #6 
Hi Thorsten,

On Fri, Jun 21, 2024 at 10:21:26AM +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 11.06.24 16:21, Colin Foster wrote:
> > 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?
> 
> Hmmm, I might have missed something, but it looks like nothing happened
> since that exchange. Did this regression fall through the cracks or can
> I consider the issue resolved for some reason?
> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

No, you haven't missed anything. This still feels like a regression to
me, but historically "regressions" I've found end up being a
misconfiguration on my part. I'm travelling this week so it won't be
until next week / weekend that I can get to anything.

I'll plan to look into a fix if it is indeed an issue.

Colin Foster
diff mbox series

Patch

--- 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)