Page MenuHomeFreeBSD

Allow for PCI controllers with different register layout
Needs ReviewPublic

Authored by andrew on Jun 5 2020, 2:25 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 17, 11:34 AM
Unknown Object (File)
Tue, Oct 15, 9:27 PM
Unknown Object (File)
Tue, Oct 15, 1:41 PM
Unknown Object (File)
Sun, Oct 13, 6:28 PM
Unknown Object (File)
Sat, Oct 12, 6:55 PM
Unknown Object (File)
Sat, Oct 12, 5:30 AM
Unknown Object (File)
Sat, Oct 12, 1:05 AM
Unknown Object (File)
Sat, Oct 12, 1:04 AM

Details

Reviewers
None
Group Reviewers
arm64
ARM
riscv
Summary

Add an interface to allow child drivers to modify the register offset
calculation and the register access method. The former will also validate
the offset is valid for the device.

Use these in the n1sdp driver to remove duplicate code.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 31659
Build 29238: arc lint + arc unit

Event Timeline

andrew requested review of this revision.Jun 5 2020, 2:25 PM

Why not make it part of the pcib_interface?

sys/dev/pci/pci_host_generic_if.m
39

For some PCI-E controllers such as the one on the raspberry pi, the same register is used for all end point config, but we update another register first to tell the controller what end point we are interested in.

So it would be nice to be clear here that "get_offset" is not merely a side-effect-free calculation, but it is allowed to change the state of the controller. i.e., a driver cannot necessarily cache the result of several of these calls and use the offsets later.

It's not paart of the pcib interface because it is specific to a driver that used bus space. PCI on x86 doesn't use this as it uses ioports via {in,out}{b,w,l} in some cases.

sys/dev/pci/pci_host_generic_if.m
39

In that case we should also hold a lock to stop multiple threads trying to read or write the config simultaneously. I could rename this get_reg, and add a release_reg interface.

Add a way for more specific drivers to cleaan up after a read/write.
This renames get_offset to acquire_reg and adds release_reg when complete.
The latter is optional when there is no cleanup to perform.

sys/dev/pci/pci_host_generic.c
251

If we have the notion of acquire/release at the register level, then should the RELEASE_REG signature take the reg as well, for symmetry with ACQUIRE? Perhaps instead of the offset?

I don't have a strong opinion: it seems slightly counter intuitive, but maybe in all practical implementations, having the offset is more useful?

For my purposes, RELEASE would just call mtx_unlock() independent of all arguments except dev, so this is slightly academic.

sys/dev/pci/pci_host_generic.c
251

I expect in most cases the register details are unneeded, however they may be used when unmapping memory mapped in the acquire function.

If you have a better name suggestion I'm happy to change it.