Page MenuHomeFreeBSD

intelspi: add PCI attachment (Lynx/Wildcat/Sunrise Point), fixup/cleanup
Needs ReviewPublic

Authored by greg_unrelenting.technology on Mar 13 2021, 3:31 PM.

Details

Reviewers
gonzo
wulf
Group Reviewers
Contributor Reviews (base)
Summary
  • apply the child's mode/speed
  • implement suspend/resume support
  • use RF_SHAREABLE interrupts
  • use bus_delayed_attach_children since the transfer can use interrupts
  • add support for newly added spibus features (cs_delay and flags)

Operation tested on Broadwell (Wildcat Point) MacBookPro12,1.
Attachment also tested on Kaby Lake (Sunrise Point) Pixelbook.


This is important because it's a prerequisite for supporting the input devices on modern Apple MacBooks.
(On the MacBookPro12,1, both USB and SPI are supported, but on other Broadwell-Skylake-Kaby Lake MacBooks, SPI is the *only* way to get keyboard and touchpad input. SPI is also used on "Apple Silicon" aarch64 MacBooks, so in the future when someone gets around to supporting that, my input driver would only have to gain FDT attachment. The generations between Kaby Lake and Apple Silicon — 2018-2020 "T2 chip" ones — are back to USB… except via a T2 virtual USB host controller really cursed thingy, if anyone wants to have some extra fun, go write a driver for that…)

Test Plan

Currently: buildkernel installkernel from here, boot on a MacBook with SPI inputs, kldload atopcase

(The input driver will be submitted here too "soon")

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

One interesting problem that I'm not sure how to properly solve: this device's description in ACPI (at least on Lynx/Wildcat) does have a _CID:

        // Scope (\_SB.PCI0)
            Device (SPI1)
            {
                Name (_ADR, 0x00150004)  // _ADR: Address
                Name (_CID, "INT33C1" /* Intel Serial I/O SPI Host Controller */)  // _CID: Compatible ID
                Name (_DDN, "Intel(R) Low Power Subsystem SPI Host Controller - 9CE6")  // _DDN: DOS Device Name
                Name (_UID, 0x02)  // _UID: Unique ID
                Name (CSST, 0x28)
                Name (CSHT, 0x0A)
                Name (RBUF, ResourceTemplate ()
                {
                    Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
                    {
                        0x00000015,
                    }
                })
                Method (_STA, 0, NotSerialized)  // _STA: Status
                {
                    Return (0x0F)
                }
.. more stuff

attaching via ACPI is useless though (can't allocate memory resource — because the resource is only available over PCI).

The kernel does match the ACPI handle to the PCI attachment — i.e. acpi_get_handle(dev) in intelspi_pci does successfully return \_SB.PCI0.SPI1 — but the opposite association does not register, because acpi_pci_update_device finds that an the attached data already exists! (Amusingly, comments there reference some retro Tablet PCs…)

So in my Apple input driver, which attaches to \_SB.PCI0.SPI1.SPIT by _HID, I can find the parent \_SB.PCI0.SPI1 handle, but its associated device_t (via acpi_get_device) is not the actual intelspi_pci, I guess it's just some dummy that would've been used for intelspi_acpi if it attached. As a temporary workaround I've patched acpi_pci_update_device to overwrite the AttachData, but that's not good.

Ideally the ACPI system would automagically attach the child node of this ACPI description to the spibus… :D but I'm not sure how it would do that.

greg_unrelenting.technology edited the summary of this revision. (Show Details)

Also add suspend-resume support (need to backup/restore private registers, also reinitialize).

I found this device on my machine:

none2@pci0:0:31:5: class=0x0c8000 rev=0x10 hdr=0x00 vendor=0x8086 device=0xa324 subvendor=0x1043 subdevice=0x8694

vendor     = 'Intel Corporation'
device     = 'Cannon Lake PCH SPI Controller'
class      = serial bus
bar   [10] = type Memory, range 32, base 0xfe010000, size 4096, enabled

Another device:

none4@pci0:0:31:5: class=0x0c8000 rev=0x00 hdr=0x00 vendor=0x8086 device=0x06a4 subvendor=0x17aa subdevice=0x3827

vendor     = 'Intel Corporation'
device     = 'Comet Lake PCH SPI Controller'
class      = serial bus

@dmitryluhtionov_gmail.com do you have a use case for touching SPI on these machines? Is there e.g. some flash memory connected to these controllers that you could try to dump?

@dmitryluhtionov_gmail.com do you have a use case for touching SPI on these machines? Is there e.g. some flash memory connected to these controllers that you could try to dump?

Hi
No. I can only experiment with this:

none2@pci0:0:31:5: class=0x0c8000 rev=0x10 hdr=0x00 vendor=0x8086 device=0xa324 subvendor=0x1043 subdevice=0x8694

vendor     = 'Intel Corporation'
device     = 'Cannon Lake PCH SPI Controller'
class      = serial bus
bar   [10] = type Memory, range 32, base 0xfe010000, size 4096, enabled

What should i do and what commands should i execute ?

sys/dev/intel/spi.c
459

You should set RF_SHAREABLE flag at least for PCI attachments

sys/dev/intel/spi.h
50

Use designated array initializers

sys/dev/intel/spi.c
476

Children attachment should be performed only after interrupts became available

sys/dev/intel/spi.c
459

Probably fine for ACPI attachment as well… would be nice to test all of this on a baytrail device but I don't currently have any

476

hmmm which interrupts? I don't think the SPI bus provides its own interrupts?

The interrupts used internally in this controller, well… turns out I actually need to — at least optionally — avoid using them.
For reasons you're familiar with :)

sys/dev/intel/spi.c
476

hmmm which interrupts?

controller interrupts. spibus_transfer does not work until PICs are enabled that happen very late in kernel boot sequence. If any child would request SPI transfer during probe or attach stage it would fail when intelspi is loaded from boot loader or compiled in to the kernel.

For reasons you're familiar with :)

ig4 has a special support for interrupt-less (polling) mode: https://github.com/freebsd/freebsd-src/blob/c441592a0e1591591665cd037a8a5e9b54675f99/sys/dev/ichiic/ig4_iic.c#L311 . It spins rather than sleeps while waiting for new data from IIC bus. It looks that intelspi does not support polling mode yet.

Just replace bus_generic_attach() with bus_delayed_attach_children() to fix it. Or implement before-mentioned mode.

sys/dev/intel/spi.c
476

Yep, that's what I mean — I've implemented polling mode because I need it to be able to do SPI transactions in an interrupt context.

Should I make it the default, with an "allow sleep" flag on the SPI message to use the old behavior? Or the other way around, add a "no sleep" flag?

Just replace bus_generic_attach() with bus_delayed_attach_children()

Sure, looks like a good idea anyway

greg_unrelenting.technology retitled this revision from intelspi: add PCI attachment (Lynx/Wildcat/Sunrise Point), apply child's mode/speed, cleanup to intelspi: add PCI attachment (Lynx/Wildcat/Sunrise Point), fixup/cleanup.
greg_unrelenting.technology edited the summary of this revision. (Show Details)
greg_unrelenting.technology edited the test plan for this revision. (Show Details)

Implement suggestions. Polling mode is in D29534 like other new SPI stack features

wulf added inline comments.
sys/dev/intel/spi.c
476

Should I make it the default, with an "allow sleep" flag on the SPI message to use the old behavior? Or the other way around, add a "no sleep" flag?

IMO Interrupt mode should be default and polled mode should be optional

This revision is now accepted and ready to land.Nov 5 2021, 8:19 AM
sys/dev/intel/spi.h
70

This line looks extraneous

Remove unnecessary array terminator, fix one tab character

This revision now requires review to proceed.Nov 5 2021, 12:49 PM
greg_unrelenting.technology edited the summary of this revision. (Show Details)
greg_unrelenting.technology edited the test plan for this revision. (Show Details)

Moved the new feature support here from D29534

sys/dev/intel/spi.c
389

Interrupt and poll timeouts are different. Why?

393

Set err value here

Make poll timeout equal to intr timeout (hopefully I calculated it correctly) and add poll timeout errno

LGTM

sys/dev/intel/spi.c
322

I suggest to use WAIT/DONTWAIT and INTR/NOINTR suffixes to be consistent with IIC/SMBUS. NO_SLEEP sounds ok too.

394

intelspi_transfer returns EIO on timeout in interrupt mode. Both err values must be equal.

This revision is now accepted and ready to land.Nov 7 2021, 6:12 PM
This revision now requires review to proceed.Nov 7 2021, 6:34 PM

Found one more device

none3@pci0:0:31:5: class=0x0c8000 rev=0x11 hdr=0x00 vendor=0x8086 device=0x7aa4 subvendor=0x1043 subdevice=0x8694

vendor     = 'Intel Corporation'
class      = serial bus
bar   [10] = type Memory, range 32, base 0xfe010000, size 4096, enabled

This is Alder Lake SPI bus