Page MenuHomeFreeBSD
Differential D33140 Diff 100012 sys/dev/cpufreq/cpufreq_dt.c

Changeset View

Standalone View

View Options

sys/dev/cpufreq/cpufreq_dt.c

Show First 20 Lines • Show All 67 Lines • ▼ Show 20 Linesstruct cpufreq_dt_opp {
uint32_t uvolt_min; uint32_t uvolt_min;
uint32_t uvolt_max; uint32_t uvolt_max;
uint32_t uamps; uint32_t uamps;
uint32_t clk_latency; uint32_t clk_latency;
bool turbo_mode; bool turbo_mode;
bool opp_suspend; bool opp_suspend;
}; };
#define CPUFREQ_DT_HAVE_REGULATOR(sc) ((sc)->reg != NULL)
manuUnsubmitted
Done
Inline Actions

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.

manu: Don't really see the point of this define but if you insist on having it it should be name…
adrianAuthorUnsubmitted
Done
Inline Actions

ok, will change the name, thanks!

adrian: ok, will change the name, thanks!
struct cpufreq_dt_softc { struct cpufreq_dt_softc {
device_t dev; device_t dev;
clk_t clk; clk_t clk;
regulator_t reg; regulator_t reg;
struct cpufreq_dt_opp *opp; struct cpufreq_dt_opp *opp;
ssize_t nopp; ssize_t nopp;
▲ Show 20 Lines • Show All 92 Lines • ▼ Show 20 Lines if (!CPU_ISSET(sc->cpu, &sc->cpus)) {
DPRINTF(dev, "Not for this CPU\n"); DPRINTF(dev, "Not for this CPU\n");
return (0); return (0);
} }
if (clk_get_freq(sc->clk, &freq) != 0) { if (clk_get_freq(sc->clk, &freq) != 0) {
device_printf(dev, "Can't get current clk freq\n"); device_printf(dev, "Can't get current clk freq\n");
return (ENXIO); return (ENXIO);
} }
/*
* Only do the regulator work if it's required.
*/
if (CPUFREQ_DT_HAVE_REGULATOR(sc)) {
/* Try to get current valtage by using regulator first. */ /* Try to get current valtage by using regulator first. */
error = regulator_get_voltage(sc->reg, &uvolt); error = regulator_get_voltage(sc->reg, &uvolt);
if (error != 0) { if (error != 0) {
/* /*
* Try oppoints table as backup way. However, * Try oppoints table as backup way. However,
* this is insufficient because the actual processor * this is insufficient because the actual processor
* frequency may not be in the table. PLL frequency * frequency may not be in the table. PLL frequency
* granularity can be different that granularity of * granularity can be different that granularity of
* oppoint table. * oppoint table.
*/ */
copp = cpufreq_dt_find_opp(sc->dev, freq); copp = cpufreq_dt_find_opp(sc->dev, freq);
if (copp == NULL) { if (copp == NULL) {
device_printf(dev, device_printf(dev,
"Can't find the current freq in opp\n"); "Can't find the current freq in opp\n");
return (ENOENT); return (ENOENT);
} }
uvolt = copp->uvolt_target; uvolt = copp->uvolt_target;
} }
} else
mmelUnsubmitted
Not Done
Inline Actions

Why?

mmel: Why?
adrianAuthorUnsubmitted
Done
Inline Actions

I dislike not explicitly initialising things!

adrian: I dislike not explicitly initialising things!
uvolt = 0;
opp = cpufreq_dt_find_opp(sc->dev, set->freq * 1000000); opp = cpufreq_dt_find_opp(sc->dev, set->freq * 1000000);
if (opp == NULL) { if (opp == NULL) {
device_printf(dev, "Couldn't find an opp for this freq\n"); device_printf(dev, "Couldn't find an opp for this freq\n");
return (EINVAL); return (EINVAL);
} }
DPRINTF(sc->dev, "Current freq %ju, uvolt: %d\n", freq, uvolt); DPRINTF(sc->dev, "Current freq %ju, uvolt: %d\n", freq, uvolt);
DPRINTF(sc->dev, "Target freq %ju, , uvolt: %d\n", DPRINTF(sc->dev, "Target freq %ju, , uvolt: %d\n",
mmelUnsubmitted
Not Done
Inline Actions

Don't print voltage if is not used

mmel: Don't print voltage if is not used
opp->freq, opp->uvolt_target); opp->freq, opp->uvolt_target);
if (uvolt < opp->uvolt_target) { if (CPUFREQ_DT_HAVE_REGULATOR(sc) && (uvolt < opp->uvolt_target)) {
jrtc27Unsubmitted
Not Done
Inline Actions

Don't need parens on the RHS

jrtc27: Don't need parens on the RHS
DPRINTF(dev, "Changing regulator from %u to %u\n", DPRINTF(dev, "Changing regulator from %u to %u\n",
uvolt, opp->uvolt_target); uvolt, opp->uvolt_target);
error = regulator_set_voltage(sc->reg, error = regulator_set_voltage(sc->reg,
opp->uvolt_min, opp->uvolt_min,
opp->uvolt_max); opp->uvolt_max);
if (error != 0) { if (error != 0) {
DPRINTF(dev, "Failed, backout\n"); DPRINTF(dev, "Failed, backout\n");
return (ENXIO); return (ENXIO);
} }
} }
DPRINTF(dev, "Setting clk to %ju\n", opp->freq); DPRINTF(dev, "Setting clk to %ju\n", opp->freq);
error = clk_set_freq(sc->clk, opp->freq, CLK_SET_ROUND_DOWN); error = clk_set_freq(sc->clk, opp->freq, CLK_SET_ROUND_DOWN);
if (error != 0) { if (error != 0) {
DPRINTF(dev, "Failed, backout\n"); DPRINTF(dev, "Failed, backout\n");
/* Restore previous voltage (best effort) */ /* Restore previous voltage (best effort) */
if (CPUFREQ_DT_HAVE_REGULATOR(sc))
error = regulator_set_voltage(sc->reg, error = regulator_set_voltage(sc->reg,
copp->uvolt_min, copp->uvolt_min,
copp->uvolt_max); copp->uvolt_max);
mmelUnsubmitted
Done
Inline Actions

Missing parenthesis ?

mmel: Missing parenthesis ?
return (ENXIO); return (ENXIO);
} }
if (uvolt > opp->uvolt_target) { if (CPUFREQ_DT_HAVE_REGULATOR(sc) && (uvolt > opp->uvolt_target)) {
jrtc27Unsubmitted
Not Done
Inline Actions

Don't need parens on the RHS

jrtc27: Don't need parens on the RHS
DPRINTF(dev, "Changing regulator from %u to %u\n", DPRINTF(dev, "Changing regulator from %u to %u\n",
uvolt, opp->uvolt_target); uvolt, opp->uvolt_target);
error = regulator_set_voltage(sc->reg, error = regulator_set_voltage(sc->reg,
opp->uvolt_min, opp->uvolt_min,
opp->uvolt_max); opp->uvolt_max);
if (error != 0) { if (error != 0) {
DPRINTF(dev, "Failed to switch regulator to %d\n", DPRINTF(dev, "Failed to switch regulator to %d\n",
opp->uvolt_target); opp->uvolt_target);
▲ Show 20 Lines • Show All 48 Lines • ▼ Show 20 Lines
cpufreq_dt_identify(driver_t *driver, device_t parent) cpufreq_dt_identify(driver_t *driver, device_t parent)
{ {
phandle_t node; phandle_t node;
/* Properties must be listed under node /cpus/cpu@0 */ /* Properties must be listed under node /cpus/cpu@0 */
node = ofw_bus_get_node(parent); node = ofw_bus_get_node(parent);
/* The cpu@0 node must have the following properties */ /* The cpu@0 node must have the following properties */
if (!OF_hasprop(node, "clocks") || if (!OF_hasprop(node, "clocks"))
(!OF_hasprop(node, "cpu-supply") &&
!OF_hasprop(node, "cpu0-supply")))
return; return;
if (!OF_hasprop(node, "operating-points") && if (!OF_hasprop(node, "operating-points") &&
!OF_hasprop(node, "operating-points-v2")) !OF_hasprop(node, "operating-points-v2"))
return; return;
if (device_find_child(parent, "cpufreq_dt", -1) != NULL) if (device_find_child(parent, "cpufreq_dt", -1) != NULL)
return; return;
if (BUS_ADD_CHILD(parent, 0, "cpufreq_dt", device_get_unit(parent)) if (BUS_ADD_CHILD(parent, 0, "cpufreq_dt", device_get_unit(parent))
== NULL) == NULL)
device_printf(parent, "add cpufreq_dt child failed\n"); device_printf(parent, "add cpufreq_dt child failed\n");
} }
static int static int
cpufreq_dt_probe(device_t dev) cpufreq_dt_probe(device_t dev)
{ {
phandle_t node; phandle_t node;
node = ofw_bus_get_node(device_get_parent(dev)); node = ofw_bus_get_node(device_get_parent(dev));
if (!OF_hasprop(node, "clocks") || /*
(!OF_hasprop(node, "cpu-supply") && * Note - supply isn't required here for probe; we'll check
!OF_hasprop(node, "cpu0-supply"))) * it out in more detail during attach.
*/
if (!OF_hasprop(node, "clocks"))
return (ENXIO); return (ENXIO);
if (!OF_hasprop(node, "operating-points") && if (!OF_hasprop(node, "operating-points") &&
!OF_hasprop(node, "operating-points-v2")) !OF_hasprop(node, "operating-points-v2"))
return (ENXIO); return (ENXIO);
device_set_desc(dev, "Generic cpufreq driver"); device_set_desc(dev, "Generic cpufreq driver");
return (BUS_PROBE_GENERIC); return (BUS_PROBE_GENERIC);
Show All 36 Lines
static int static int
cpufreq_dt_oppv2_parse(struct cpufreq_dt_softc *sc, phandle_t node) cpufreq_dt_oppv2_parse(struct cpufreq_dt_softc *sc, phandle_t node)
{ {
phandle_t opp, opp_table, opp_xref; phandle_t opp, opp_table, opp_xref;
pcell_t cell[2]; pcell_t cell[2];
uint32_t *volts, lat; uint32_t *volts, lat;
int nvolt, i; int nvolt, i;
/*
* operating-points-v2 does not require the voltage entries
* and a regulator. So, it's OK if they're not there.
*/
if (OF_getencprop(node, "operating-points-v2", &opp_xref, if (OF_getencprop(node, "operating-points-v2", &opp_xref,
sizeof(opp_xref)) == -1) { sizeof(opp_xref)) == -1) {
device_printf(sc->dev, "Cannot get xref to oppv2 table\n"); device_printf(sc->dev, "Cannot get xref to oppv2 table\n");
return (ENXIO); return (ENXIO);
} }
opp_table = OF_node_from_xref(opp_xref); opp_table = OF_node_from_xref(opp_xref);
if (opp_table == opp_xref) if (opp_table == opp_xref)
Show All 26 Lines for (i = 0, opp_table = OF_child(opp_table); opp_table > 0;
else else
sc->opp[i].clk_latency = (int)lat; sc->opp[i].clk_latency = (int)lat;
if (OF_hasprop(opp_table, "turbo-mode")) if (OF_hasprop(opp_table, "turbo-mode"))
sc->opp[i].turbo_mode = true; sc->opp[i].turbo_mode = true;
if (OF_hasprop(opp_table, "opp-suspend")) if (OF_hasprop(opp_table, "opp-suspend"))
sc->opp[i].opp_suspend = true; sc->opp[i].opp_suspend = true;
nvolt = OF_getencprop_alloc_multi(opp_table, "opp-microvolt", if (CPUFREQ_DT_HAVE_REGULATOR(sc)) {
sizeof(*volts), (void **)&volts); nvolt = OF_getencprop_alloc_multi(opp_table,
"opp-microvolt", sizeof(*volts), (void **)&volts);
if (nvolt == 1) { if (nvolt == 1) {
sc->opp[i].uvolt_target = volts[0]; sc->opp[i].uvolt_target = volts[0];
sc->opp[i].uvolt_min = volts[0]; sc->opp[i].uvolt_min = volts[0];
sc->opp[i].uvolt_max = volts[0]; sc->opp[i].uvolt_max = volts[0];
} else if (nvolt == 3) { } else if (nvolt == 3) {
sc->opp[i].uvolt_target = volts[0]; sc->opp[i].uvolt_target = volts[0];
sc->opp[i].uvolt_min = volts[1]; sc->opp[i].uvolt_min = volts[1];
sc->opp[i].uvolt_max = volts[2]; sc->opp[i].uvolt_max = volts[2];
} else { } else {
device_printf(sc->dev, device_printf(sc->dev,
"Wrong count of opp-microvolt property\n"); "Wrong count of opp-microvolt property\n");
OF_prop_free(volts); OF_prop_free(volts);
free(sc->opp, M_DEVBUF); free(sc->opp, M_DEVBUF);
return (ENXIO); return (ENXIO);
} }
OF_prop_free(volts); OF_prop_free(volts);
} else {
/* No regulator required; don't add anything */
mmelUnsubmitted
Not Done
Inline Actions

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

mmel: Why? opp table should not be touched is regulator is not required.
adrianAuthorUnsubmitted
Done
Inline Actions

I really dislike uninitialised values in things!

adrian: I really dislike uninitialised values in things!
sc->opp[i].uvolt_target = 0;
sc->opp[i].uvolt_min = 0;
sc->opp[i].uvolt_max = 0;
}
if (bootverbose) if (bootverbose)
device_printf(sc->dev, "%ju.%03ju Mhz (%u uV)\n", device_printf(sc->dev, "%ju.%03ju Mhz (%u uV)\n",
sc->opp[i].freq / 1000000, sc->opp[i].freq / 1000000,
sc->opp[i].freq % 1000000, sc->opp[i].freq % 1000000,
sc->opp[i].uvolt_target); sc->opp[i].uvolt_target);
} }
return (0); return (0);
Show All 10 Linescpufreq_dt_attach(device_t dev)
int rv = 0; int rv = 0;
char device_type[16]; char device_type[16];
enum opp_version version; enum opp_version version;
sc = device_get_softc(dev); sc = device_get_softc(dev);
sc->dev = dev; sc->dev = dev;
node = ofw_bus_get_node(device_get_parent(dev)); node = ofw_bus_get_node(device_get_parent(dev));
sc->cpu = device_get_unit(device_get_parent(dev)); sc->cpu = device_get_unit(device_get_parent(dev));
sc->reg = NULL;
DPRINTF(dev, "cpu=%d\n", sc->cpu); DPRINTF(dev, "cpu=%d\n", sc->cpu);
if (sc->cpu >= mp_ncpus) { if (sc->cpu >= mp_ncpus) {
device_printf(dev, "Not attaching as cpu is not present\n"); device_printf(dev, "Not attaching as cpu is not present\n");
return (ENXIO); rv = ENXIO;
goto error;
} }
if (regulator_get_by_ofw_property(dev, node, /*
"cpu-supply", &sc->reg) != 0) { * Cache if we have the regulator supply but don't error out
if (regulator_get_by_ofw_property(dev, node, * quite yet. If it's operating-points-v2 then regulator
"cpu0-supply", &sc->reg) != 0) { * and voltage entries are optional.
mmelUnsubmitted
Not Done
Inline Actions

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.

mmel: This is IMHO wrong. The 'opp-microvolt ' property should determine if we need a regulator…
adrianAuthorUnsubmitted
Done
Inline Actions

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: I agree, and I think this'll be a good follow-up commit that I can do! But I'd also really…
*/
if (regulator_get_by_ofw_property(dev, node, "cpu-supply",
&sc->reg) == 0)
device_printf(dev, "Found cpu-supply\n");
else if (regulator_get_by_ofw_property(dev, node, "cpu0-supply",
&sc->reg) == 0)
device_printf(dev, "Found cpu0-supply\n");
/*
jrtc27Unsubmitted
Done
Inline Actions

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

jrtc27: Can't we just use sc->reg == NULL / != NULL in this function like everywhere else?
* Determine which operating mode we're in. Error out if we expect
* a regulator but we're not getting it.
*/
if (OF_hasprop(node, "operating-points"))
version = OPP_V1;
else if (OF_hasprop(node, "operating-points-v2"))
version = OPP_V2;
else {
device_printf(dev,
"didn't find a valid operating-points or v2 node\n");
rv = ENXIO;
goto error;
}
/*
* Now, we only enforce needing a regulator for v1.
*/
if ((version == OPP_V1) && !CPUFREQ_DT_HAVE_REGULATOR(sc)) {
device_printf(dev, "no regulator for %s\n", device_printf(dev, "no regulator for %s\n",
ofw_bus_get_name(device_get_parent(dev))); ofw_bus_get_name(device_get_parent(dev)));
jrtc27Unsubmitted
Done
Inline Actions

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

jrtc27: Don't need parens, and don't compare against false, just use !found_regulator
return (ENXIO); rv = ENXIO;
goto error;
} }
}
if (clk_get_by_ofw_index(dev, node, 0, &sc->clk) != 0) { if (clk_get_by_ofw_index(dev, node, 0, &sc->clk) != 0) {
device_printf(dev, "no clock for %s\n", device_printf(dev, "no clock for %s\n",
ofw_bus_get_name(device_get_parent(dev))); ofw_bus_get_name(device_get_parent(dev)));
regulator_release(sc->reg); rv = ENXIO;
return (ENXIO); goto error;
} }
if (OF_hasprop(node, "operating-points")) { if (version == OPP_V1) {
version = OPP_V1;
rv = cpufreq_dt_oppv1_parse(sc, node); rv = cpufreq_dt_oppv1_parse(sc, node);
if (rv != 0) { if (rv != 0) {
device_printf(dev, "Failed to parse opp-v1 table\n"); device_printf(dev, "Failed to parse opp-v1 table\n");
return (rv); goto error;
} }
OF_getencprop(node, "operating-points", &opp, OF_getencprop(node, "operating-points", &opp,
sizeof(opp)); sizeof(opp));
} else { } else if (version == OPP_V2) {
version = OPP_V2;
rv = cpufreq_dt_oppv2_parse(sc, node); rv = cpufreq_dt_oppv2_parse(sc, node);
if (rv != 0) { if (rv != 0) {
device_printf(dev, "Failed to parse opp-v2 table\n"); device_printf(dev, "Failed to parse opp-v2 table\n");
return (rv); goto error;
} }
OF_getencprop(node, "operating-points-v2", &opp, OF_getencprop(node, "operating-points-v2", &opp,
sizeof(opp)); sizeof(opp));
} else {
device_printf(dev, "operating points version is incorrect\n");
goto error;
} }
/* /*
* Find all CPUs that share the same opp table * Find all CPUs that share the same opp table
*/ */
CPU_ZERO(&sc->cpus); CPU_ZERO(&sc->cpus);
cnode = OF_parent(node); cnode = OF_parent(node);
for (cpu = 0, cnode = OF_child(cnode); cnode > 0; cnode = OF_peer(cnode)) { for (cpu = 0, cnode = OF_child(cnode); cnode > 0; cnode = OF_peer(cnode)) {
Show All 24 Linescpufreq_dt_attach(device_t dev)
} }
if (clk_get_freq(sc->clk, &freq) == 0) if (clk_get_freq(sc->clk, &freq) == 0)
cpufreq_dt_notify(dev, freq); cpufreq_dt_notify(dev, freq);
cpufreq_register(dev); cpufreq_register(dev);
return (0); return (0);
error:
if (CPUFREQ_DT_HAVE_REGULATOR(sc))
regulator_release(sc->reg);
return (rv);
} }
static device_method_t cpufreq_dt_methods[] = { static device_method_t cpufreq_dt_methods[] = {
/* Device interface */ /* Device interface */
DEVMETHOD(device_identify, cpufreq_dt_identify), DEVMETHOD(device_identify, cpufreq_dt_identify),
DEVMETHOD(device_probe, cpufreq_dt_probe), DEVMETHOD(device_probe, cpufreq_dt_probe),
DEVMETHOD(device_attach, cpufreq_dt_attach), DEVMETHOD(device_attach, cpufreq_dt_attach),
Show All 19 Lines