Paths

Table of Contentst

Differential D59478

CPU temperature sensor for jh7110
Needs ReviewPublic
Actions

Authored by bscott_bunyatech.com.au on Mon, Sep 7, 11:35 AM.

Details

Reviewers

mhorne

Group Reviewers

riscv

Summary

This patch creates a cpu temperature sensor for the jh7110 SoC.

The calibration numbers come from the openbsd driver but are reworked to produce a result in K rather than C.

The temperature is exposed as a sysctl, dev.jh7110_temp.0.temperature but I have also exposed it as dev.cpu.0.temperature because that's where you find it on a RaspberryPi so it's a lot more obvious. I think it should really only be in one place but don't know where that should be.

Test Plan

% sysctl -a dev.jh7110_temp.0.temperature dev.cpu.0.temperature
dev.jh7110_temp.0.temperature: 27.962C
dev.cpu.0.temperature: 27.962C

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

bscott_bunyatech.com.au created this revision.Mon, Sep 7, 11:35 AM

Herald added a subscriber: imp. · View Herald TranscriptMon, Sep 7, 11:35 AM

bscott_bunyatech.com.au requested review of this revision.Mon, Sep 7, 11:35 AM

Generally LGTM, with some minor tweaks needed.

sys/riscv/starfive/files.starfive
16
sys/riscv/starfive/jh7110_temp.c
1–8	SPDX identifier is supposed to come second now, don't ask me why. I am not sure that "Parts" has any legal precedent or meaning. It is probably safest to just list all three as copyright holders. (If the attribution to jsihv@gmx.com is only due to copying driver boilerplate, this can be dropped safely.)
10–11	Most source files should not include this directly.
68–71	I think we can add a simple sleepable mutex around the register access; this ensures it cannot be abused by userspace, and allows us to drop CTLFLAG_NEEDGIANT for the sysctls.
129	The sysctl node must be released as well.
145	Here and below.
219	We want to discourage new additions of `CTLFLAG_NEEDGIANT`. I think it can be dropped; see my comment above.
223–232	I think this is sensible. The `amdtemp(4)` driver also exports to each `dev.cpu.N.temperature` node. Obviously, this is SoC temperature, not quite the same, but unless there is some way to report per-CPU temperature, I believe this is a good choice.

bnovkov added a subscriber: bnovkov.Wed, Sep 9, 1:42 PM

bnovkov added inline comments.

sys/riscv/starfive/jh7110_temp.c
16	Where are you using this header? Same question for the `gpiobusvar.h` below.
68–71	+1.
73–78	The indentation here is a bit off.
80	Please turn these into `#define` constants.

New diff in a few minutes.

sys/riscv/starfive/jh7110_temp.c
1–8	Just feel bad not attributing things. Happy to reword it.
10–11	Just copying and have proven that it's not needed.
73–78	Moved up front to be with #defines for the constants so indentation is less of an issue.
129	OK, took a bit of searching for how to do it. It doesn't look that common but leaving a pointer to freed memory is very scary.

Made changes as suggested.

Also updated clocks for pwmdac although they aren't used anywhere (been playing with this in the background). They snuck in previously and I'm now just making them correct in case anybody else tries to use them. Should I have just removed them from this diff?

Revision Contents
Changeset List

Path

Size

sys/

dev/

clk/

starfive/

	jh7110_clk_sys.c
	jh7110_clk_sys.c.orig

14 lines

riscv/

starfive/

	files.starfive
	files.starfive.orig

2 lines

	jh7110_temp.c
	jh7110_temp.c.orig

247 lines

Diff 186120

View Options

sys/dev/clk/starfive/jh7110_clk_sys.c

View Options

sys/riscv/starfive/files.starfive

View Options

sys/riscv/starfive/jh7110_temp.c

CPU temperature sensor for jh7110Needs ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 186120

sys/dev/clk/starfive/jh7110_clk_sys.c

sys/riscv/starfive/files.starfive

sys/riscv/starfive/jh7110_temp.c

CPU temperature sensor for jh7110
Needs ReviewPublic
Actions

Revision Contents
Changeset List