amdsmu: Initial work on a driver for the AMD SMU

[obiwac]

Summary

Start work on a driver for the AMD SMU (system management unit), which will eventually be used for getting S0ix statistics (e.g. how long the CPU has spent in the deepest - S0i3 - sleep state during the last sleep) as well as letting PMFW (power management firmware, running on the SMU) know when we intend to enter and exit sleep. It is what's responsible for turning off the VDD line to the CPU.

Currently, amdsmu is just able to get the SMU's firmware version on AMD Rembrandt, Phoenix, and Strix Point CPUs. Future patches will come for stats and sleep, and will hopefully help with the EC wake event "noisiness" issue described in D48387.

This is the equivalent to amd-pmc on Linux.

Test Plan

I have tested this on the Framework 13 AMD Ryzen 7040 series (Phoenix), with SMU firmware version 76.70.0:

amdsmu0 on hostb0
amdsmu0: SMU version: 76.70.0 (program 0)

[cem]

sys/dev/amdsmu/amdsmu.c

104

nit: I think a blank first line is no longer style(9). Ditto elsewhere.

128–129

Ditto later diff -- it's been a while, but I think the whitespace is excessive for style(9).

135–141

These constants work out to a full second of waiting. Do we really need that long?

244–248

Do we need to unmap smu_space in this failure path?

[emaste]

sys/dev/amdsmu/amdsmu.c

104

Yeah, IIRC we first said the blank line was not necessary, but style(9) now says it should not be present, and also should be removed if there's significant changes to a file.

[obiwac]

Respond to review comments; align closer with style(9), unmap bus spaces.

[obiwac]

Thanks for the review @cem!

sys/dev/amdsmu/amdsmu.c

135–141

Mirroring the Linux constants for these here, but I don't know what bounds are reasonable in practice (SMU documentation is sparse). Probably you're right and 1 sec is excessive.

[kib]

So is there any documentation about SMU?

sys/dev/amdsmu/amdsmu.c

20

I would add a header like amdsmu_reg.h and put hw-defined constants there.

86

I do not understand this (I generally do not understand newbus, but this case might be even more special).

You are looking for pci device, and then you add an synthetic child. Why not attach the driver directly to the (pseudo-)pci device?

151

Not too useful comment(s)

156

I find it helpful to wrap the bus_space_read/write into driver-specific helpers. You might look at sys/x86/iommu/dmar.h and amdiommu.h for examples.

159

Esp. this one

[obiwac]

Add amdsmu.h and amdsmu_reg.h header files as well as bus space read/write wrappers as suggested by kib@.

[obiwac]

Thanks for the review @kib :)

So is there any documentation about SMU?

Basically not, no, except for the SMU index address/data which are just used to get the SMU memory space (and is the same thing as amdsmn).

It might make sense for amdsmu to use amdsmn's amdsmn_read, though its not much extra work for amdsmu. @cem thoughts on this?

Otherwise most of this is gleaned from the Linux driver.

sys/dev/amdsmu/amdsmu.c

86

The PCI device we look for is the root for other drivers such as amdsmn and amdtemp (and is already claimed by hostb). amdsmn and amdtemp do the same thing.

151

Mainly added this because I don't think it's obvious that we're waiting for a result because this is how to wait for the SMU to be ready to receive commands.

[obiwac]

sys/dev/amdsmu/amdsmu.h

24–26

Perhaps CPUID_* is not the clearest name and I should call this PCI_DEVICE_ID_AMD_*_ROOT like in sys/dev/amdsmn/amdsmn.c.

[cem]

In D48683#1176553, @obiwac wrote:

It might make sense for amdsmu to use amdsmn's amdsmn_read, though its not much extra work for amdsmu. @cem thoughts on this?

I don't have any strong opinions about this.

[emaste]

sys/dev/amdsmu/amdsmu.h

24–26

I think so yes

[kib]

sys/dev/amdsmu/amdsmu.c

81

sys/dev/amdsmu/amdsmu.h

21

There is sys/x86/include/cputypes.h which might be the adequate place.

That file already provides CPU_VENDOR_AMD which is identical to VENDORID_AMD.

sys/dev/amdsmu/amdsmu_reg.h

36

Are these parameters hw-defined, or they are some choices by the driver?

[obiwac]

Rename PCI device ID constants to be clearer, use CPU_VENDOR_AMD instead of defining ourselves, and style(9).

[obiwac]

sys/dev/amdsmu/amdsmu_reg.h

36

These constants come from the Linux driver. Fairly sure they are just choices by the driver, I doubt the SMU would ever take 1 full second to respond. But perhaps its best just to keep in line with what AMD are doing on Linux.

[kib]

sys/dev/amdsmu/amdsmu.h

26

Move them to _reg.h

sys/dev/amdsmu/amdsmu_reg.h

36

If they are not something describing hardware, but providing a parameters for the driver, the _reg.h is not the right place for them. Move to amdsmu.h.

[obiwac]

Move defines between headers.

[mckusick]

Review process looks good and complete.
Adding my accept as your mentor.