Page MenuHomeFreeBSD

spibus: extend API: add cs_delay ivar, KEEP_CS and NO_SLEEP flags
Needs ReviewPublic

Authored by greg_unrelenting.technology on Apr 1 2021, 1:42 PM.

Details

Reviewers
manu
ian
Group Reviewers
Contributor Reviews (base)
Summary

These feature are required for an upcoming Apple MacBook topcase (HID over SPI) driver:

A delay after toggling CS is required to avoid anomalies like an extra junk byte in front of the message.

Keeping CS asserted is required to be able to read a status report after writing a command. (The device won't return the status if CS was deasserted.)

Sleep is not allowed in the interrupt context where the Apple input driver runs its transactions. Use a flag to tell the SPI driver to avoid mtx_sleep.


For reference, OpenBSD doing the delay and keep:
https://github.com/openbsd/src/commit/fe92277574533ee44bdcd49cfe822a8841b6a825


TODO: add support blindly to all SPI controller drivers?

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

That doesn't seems to be specific to the Apple hardware but to the intel controller.
Linux notes that after changing CS one should wait 2 clock cycles (spi controller clock).
Our driver doesn't seems to do anything about clock and just hardcode based on the one found on the MinowBoard.
Anyway, that seems more controller specific than SPI specific so I'm not sure that this is the correct approach.

In D29534#662122, @manu wrote:

That doesn't seems to be specific to the Apple hardware but to the intel controller.

The delay value is provided in ACPI for the input device.

Linux struct spi_transfer has a delay field, the Linux input driver uses it with the value it gets from ACPI.

Our driver doesn't seems to do anything about clock and just hardcode based on the one found on the MinowBoard.

You didn't notice the parent revision ;)

@manu OpenBSD just added this delay thing to their SPI API:

https://github.com/openbsd/src/commit/fe92277574533ee44bdcd49cfe822a8841b6a825

while working on Apple Silicon support (which is not using an Intel controller, obviously).

It *is* the SPI "client" chip on the Macbooks that requires this.

greg_unrelenting.technology retitled this revision from spibus: add cs_delay ivar to spibus: extend API with a cs_delay ivar and KEEP_CS flag.
greg_unrelenting.technology edited the summary of this revision. (Show Details)

We'll also need a flag to be able to keep CS asserted after a particular transaction.

greg_unrelenting.technology retitled this revision from spibus: extend API with a cs_delay ivar and KEEP_CS flag to spibus: extend API: cs_delay ivar, KEEP_CS and ALLOW_SLEEP flags.
greg_unrelenting.technology edited the summary of this revision. (Show Details)

Also add a polling mode to intelspi and guard it with an ALLOW_SLEEP transaction flag. (If any other driver does sleep, I'll add a check there too when I modify other drivers)

greg_unrelenting.technology retitled this revision from spibus: extend API: cs_delay ivar, KEEP_CS and ALLOW_SLEEP flags to spibus: extend API: add cs_delay ivar, KEEP_CS and NO_SLEEP flags.
greg_unrelenting.technology edited the summary of this revision. (Show Details)

Flip the flag: NO_SLEEP. Prevent the INTELSPI_BUSY sleep as well (I have not encountered it, but it's making me worried…)

Looking at other SPI drivers, it seems that quite a few also do this interrupt sleep thing. I can't test them because I don't have all the hardware, so I think I should just add if ((cmd->flags & SPI_FLAG_NO_SLEEP) == SPI_FLAG_NO_SLEEP) return (ENODEV) // TODO to any driver that does sleep?

Could you split the intel part and spibus part in two revs ?
I'm ok with landing the spibus part.