Page MenuHomeFreeBSD

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

Authored by adrian on Nov 27 2021, 3:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 12 2024, 10:06 AM
Unknown Object (File)
Feb 12 2024, 10:06 AM
Unknown Object (File)
Feb 12 2024, 10:06 AM
Unknown Object (File)
Feb 12 2024, 10:06 AM
Unknown Object (File)
Feb 12 2024, 10:06 AM
Unknown Object (File)
Feb 12 2024, 10:06 AM
Unknown Object (File)
Jan 15 2024, 3:11 AM
Unknown Object (File)
Dec 23 2023, 1:21 AM
Subscribers

Details

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
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

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' ?

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

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.

sys/dev/cpufreq/cpufreq_dt.c
76

ok, will change the name, thanks!

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 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

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!

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.

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
This revision was automatically updated to reflect the committed changes.