Page MenuHomeFreeBSD
Differential D33140

[cpufreq] Support operating-mode-v2 tables with no voltages
ClosedPublic
Actions

Authored by adrian on Nov 27 2021, 3:05 AM.
Tags
None
Referenced Files
F171015980: D33140.id99073.diff
Tue, Sep 8, 5:10 AM
Unknown Object (File)
Sun, Sep 6, 3:36 AM
Unknown Object (File)
Sun, Sep 6, 3:31 AM
Unknown Object (File)
Sun, Sep 6, 1:37 AM
Unknown Object (File)
Sat, Sep 5, 4:20 PM
Unknown Object (File)
Sat, Sep 5, 1:28 PM
Unknown Object (File)
Sat, Sep 5, 10:45 AM
Unknown Object (File)
Fri, Sep 4, 6:02 PM
View All Files
Subscribers
imp
jrtc27

Details

Reviewers
manu
imp
mmel
Commits
rG05860ffdb428: cpufreq: Support operating-mode-v2 tables with no voltages
Summary

The linux device tree documentation for this states that
for v1 voltages are required, but for v2 voltages are optional.

So, handle that here - if there's no regulator/supply provided
for a v1 opmode then error out; but keep it optional for v2.
Then just don't both doing any regulator calls if it's not configured.

Test Plan
  • IPQ4018, with no voltage tables; the freq set is called appropriately.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 43165
Build 40053: arc lint + arc unit

Event Timeline

adrian created this revision.Nov 27 2021, 3:05 AM
Herald added a subscriber: imp. · View Herald TranscriptNov 27 2021, 3:05 AM
adrian requested review of this revision.Nov 27 2021, 3:05 AM
Harbormaster completed remote builds in B42993: Diff 99073.Nov 27 2021, 3:05 AM
adrian added reviewers: manu, imp.Nov 27 2021, 3:07 AM
manu added a comment.Nov 29 2021, 9:26 AM

I don't think that a new softc variable is needed.
Just validate that we have a regulator for v2 only and then if we have one just call the methods.

manu added a reviewer: mmel.Nov 29 2021, 9:26 AM
adrian added a comment.Nov 30 2021, 5:12 AM
In D33140#749351, @manu wrote:

I don't think that a new softc variable is needed.
Just validate that we have a regulator for v2 only and then if we have one just call the methods.

what would we compare sc->reg to in order to see if it's not set? Just '0' ?

adrian updated this revision to Diff 99207.Nov 30 2021, 5:13 AM

updates

Harbormaster completed remote builds in B43049: Diff 99207.Nov 30 2021, 5:13 AM
adrian updated this revision to Diff 99208.Nov 30 2021, 5:15 AM

more updates from jrtc27

Harbormaster completed remote builds in B43050: Diff 99208.Nov 30 2021, 5:15 AM
manu added a comment.Nov 30 2021, 7:34 AM
In D33140#749943, @adrian wrote:
In D33140#749351, @manu wrote:

I don't think that a new softc variable is needed.
Just validate that we have a regulator for v2 only and then if we have one just call the methods.

what would we compare sc->reg to in order to see if it's not set? Just '0' ?

regulator_t is a pointer so NULL

adrian updated this revision to Diff 99261.Nov 30 2021, 8:39 PM

comment from manu

Harbormaster completed remote builds in B43069: Diff 99261.Nov 30 2021, 8:39 PM
manu added a comment.Dec 1 2021, 9:39 AM

Small nit otherwise I think it's ok.

sys/dev/cpufreq/cpufreq_dt.c
76

Don't really see the point of this define but if you insist on having it it should be name CPUFREQ_DT_HAVE_REGULATOR.
But tbh the code will be clearer without it.

adrian added inline comments.Dec 1 2021, 4:46 PM
sys/dev/cpufreq/cpufreq_dt.c
76

ok, will change the name, thanks!

mmel added inline comments.Dec 1 2021, 7:13 PM
sys/dev/cpufreq/cpufreq_dt.c
209

Why?

218

Don't print voltage if is not used

238–241

Missing parenthesis ?

455

Why? opp table should not be touched is regulator is not required.

498

This is IMHO wrong. The 'opp-microvolt ' property should determine if we need a regulator setting. We should report a problem if the DT has 'opp-microvolt ' without the regulator property. Moreover, we should distinguish between ENOENT and other error codes. For example, this code silently ignores required regulator without driver.

jrtc27 added a subscriber: jrtc27.Dec 1 2021, 7:57 PM
jrtc27 added inline comments.
sys/dev/cpufreq/cpufreq_dt.c
221

Don't need parens on the RHS

245

Don't need parens on the RHS

507

Can't we just use sc->reg == NULL / != NULL in this function like everywhere else?

527

Don't need parens, and don't compare against false, just use !found_regulator

adrian marked 4 inline comments as done.Dec 5 2021, 2:05 AM

going to update the patch in a sec with the above stuff updated

sys/dev/cpufreq/cpufreq_dt.c
455

I really dislike uninitialised values in things!

adrian updated this revision to Diff 99468.Dec 5 2021, 2:15 AM

Updates from comments

Harbormaster completed remote builds in B43153: Diff 99468.Dec 5 2021, 2:15 AM
adrian added inline comments.Dec 5 2021, 2:18 AM
sys/dev/cpufreq/cpufreq_dt.c
498

I agree, and I think this'll be a good follow-up commit that I can do!

But I'd also really like to start landing the clock and GPIO stuff I have working here so I can start on porting bits of your pinmux stuff for apq8014 over and then make a start on the other drivers that I need.

Lemme see if I can rebase what I've got against the latest -HEAD and do the development against it there so I don't have to do a follow-up commit to address things.

adrian updated this revision to Diff 99511.Dec 6 2021, 3:07 AM

Address some feedback!

Harbormaster completed remote builds in B43165: Diff 99511.Dec 6 2021, 3:07 AM
adrian updated this revision to Diff 99512.Dec 6 2021, 3:07 AM

Fix commit msg

Harbormaster completed remote builds in B43166: Diff 99512.Dec 6 2021, 3:07 AM
adrian added inline comments.Dec 6 2021, 3:14 AM
sys/dev/cpufreq/cpufreq_dt.c
209

I dislike not explicitly initialising things!

This revision was not accepted when it landed; it landed in state Needs Review.Dec 14 2021, 5:51 PM
Closed by commit rG05860ffdb428: cpufreq: Support operating-mode-v2 tables with no voltages (authored by adrian). · Explain Why
This revision was automatically updated to reflect the committed changes.
adrian added a commit: rG05860ffdb428: cpufreq: Support operating-mode-v2 tables with no voltages.

Revision Contents
Changeset List

PathSize
sys/
dev/
cpufreq/
178 lines

Diff 99511

View Options

sys/dev/cpufreq/cpufreq_dt.c

Loading...