Page MenuHomeFreeBSD

Add support for Zen 4 in amdsmn and amdtemp
ClosedPublic

Authored by diizzy on Jul 16 2023, 12:11 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 10, 11:22 AM
Unknown Object (File)
Fri, Nov 1, 3:37 AM
Unknown Object (File)
Mon, Oct 28, 4:28 AM
Unknown Object (File)
Fri, Oct 25, 1:57 PM
Unknown Object (File)
Sun, Oct 20, 8:20 AM
Unknown Object (File)
Thu, Oct 17, 9:20 PM
Unknown Object (File)
Oct 3 2024, 5:52 AM
Unknown Object (File)
Oct 1 2024, 12:45 PM

Details

Summary

Author: Akio Morita
Source: http://jyurai.ddo.jp/~amorita/diary/?date=20221102#p01

Appears to work fine on my Ryzen 7900 CPU with Asus ProArt X670E-Creator WiFi motherboard

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This seems good to me. I'd be tempted to make the tmp base in the identification tables, but it may no tbe worth it (what's here is likely fine, but if we have more, then we'll want to look again).
Plus a possible formatting nit.

sys/dev/amdtemp/amdtemp.c
140

double check tab vs spaces here to make it consistent.

This revision is now accepted and ready to land.Jul 16 2023, 11:27 PM

I am not knowledgeable about this driver or hardware. The change is not easy to review without context.

I guess "appears to work fine" means that it attaches and returns sensible values in the dev.amdtemp and dev.cpu sysctl trees?

sys/dev/amdtemp/amdtemp.c
342

I suspect that there is a slightly better placement for this assignment, but I can't point it out without context.

diizzy marked an inline comment as done.

Fix tab vs spaces issue

This revision now requires review to proceed.Jul 18 2023, 8:11 PM

@mhorne
You're correct about sysctl entries

sys/dev/amdtemp/amdtemp.c
342

I've generated a new patch with -U99999 if that's what you were referring to?

LGTM! Just one thing which can be fixed before pushing.

sys/dev/amdtemp/amdtemp.c
342

Yes, thank you.

526–528

IMO sc->sc_temp_base = AMDTEMP_17H_CCD_TMP_BASE; should go here, where we subsequently probe the CCD sensors, and (maybe) overwrite its value in amdtemp_probe_ccd_sensors19h().

This revision is now accepted and ready to land.Jul 18 2023, 9:06 PM

Update with mhorne's suggestions

This revision now requires review to proceed.Jul 22 2023, 10:43 AM

The only issue I have is that I lack the authors email address, is there a way to work around that?

The only issue I have is that I lack the authors email address, is there a way to work around that?

I don't know, I have never had this problem. In this case the "Source" tag you have seems fine, but phrase it as "Obtained From". Did you find the change via search engine? Maybe just state that, unless there is some way to contact the author from their website.

I've left two comments (first 16th) asking for a reply but no response

@freebsd_igalic.co found a person named Akio Morita on https://docs.freebsd.org/en/articles/contributors/ which I think is likely be the same person

I managed to get hold of the author and a proper e-mail, since I'm not at a src committer (only ports) can someone approve it?

This revision is now accepted and ready to land.Aug 1 2023, 12:55 PM