On Tue, Dec 17, 2013 at 11:52:57AM -0500, Anson Huang wrote:
> > > @@ -63,29 +68,48 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
> > > rcu_read_unlock();
> > > volt_old = regulator_get_voltage(arm_reg);
> > >
> > > - dev_dbg(cpu_dev, "%u MHz, %ld mV --> %u MHz, %ld mV\n",
> > > + /* Find the matching VDDSOC/VDDPU operating voltage */
> > > + while (soc_opp_index < soc_opp_count) {
> > > + if (new_freq == imx6_soc_opp[soc_opp_index].arm_freq)
> > > + break;
> > > + soc_opp_index++;
> > > + }
> >
> > I'm not comfortable with this lookup every time imx6q_set_target() is
> > called. I think we can use the 'index' variable to find VDDSOC/VDDPU
> > operating voltage from imx6_soc_opp table directly, if we sort the table
> > in the same order that freq_table is sorted.
> >
>
> yes, we can use the index passed from OPP, then I will add some comments that
> the freq/volt table of vddsoc should be sorted in same order.
It's not as easy as adding some comments. I think we need to ensure the
order in probe function, probably sorting them in code. Otherwise, it's
dangerous if we're running at a frequency with a wrong voltage.
Also, if we sort the table in the same order as freq_table, we do not
even need to have arm_freq in the soc_opp table.
<snip>
> > > + dev_dbg(cpu_dev, "%u MHz, arm %ld mV, soc-pu %d mV --> %u MHz, arm %ld mV, soc-pu %d mV\n",
> > > old_freq / 1000, volt_old / 1000,
> > > - new_freq / 1000, volt / 1000);
> > > + imx6_soc_opp[soc_opp_index].soc_volt / 1000,
> > > + new_freq / 1000, volt / 1000,
> > > + imx6_soc_opp[soc_opp_index].soc_volt / 1000);
> >
> > You print the same soc_volt for both old and new ones?
>
> this is my mistake, maybe I can leave the original code here to only
> print out vddarm's voltage? Otherwise, I have to add variable to keep
> old vddsoc/pu's voltage.
Yea, that's okay with leaving it as the existing code.
<snip>
> > > + /*
> > > + * Each OPP is a set of tuples consisting of frequency and
> > > + * voltage like <freq-kHz vol-uV>.
> > > + */
> > > + nr = prop->length / sizeof(u32);
> > > + if (nr % 2) {
> > > + dev_err(cpu_dev, "Invalid fsl-soc-operating-points list!\n");
> >
> > fsl,soc-operating-points
> >
> > and
> >
> > ret = -EINVAL;
>
> no need to free freq table here?
We're only populating the error code here, and will still goto
free_freq_table below.
> > > + goto free_freq_table;
> > > + }
<snip>
> > > + min_volt = max_volt = 0;
> > > + for (i = 0; i < nr / 2; i++) {
> > > + unsigned long freq = be32_to_cpup(val++);
> > > + unsigned long volt = be32_to_cpup(val++);
> > > +
> > > + if (i == 0)
> > > + min_volt = max_volt = volt;
> > > + if (volt < min_volt)
> > > + min_volt = volt;
> > > + if (volt > max_volt)
> > > + max_volt = volt;
> > > + opp = dev_pm_opp_find_freq_exact(cpu_dev, freq * 1000, true);
> > > + imx6_soc_opp[i].arm_freq = freq;
> > > + imx6_soc_opp[i].soc_volt = volt;
> > > + soc_opp_count++;
> > > + }
> > > + rcu_read_unlock();
> >
> > We may need another sanity check to see if soc_opp_count == num (arm opp
> > count) here.
>
> yes, will add it, need to make sure soc_opp_count is same as arm opp count,
> but since we did NOT have 1G2 check, so the soc_opp_count will ">=" arm opp
> count.
Hmm, this is another problem with the for-loop above. The soc_volt
should be added into imx6_soc_opp table and soc_opp_count increases,
only when dev_pm_opp_find_freq_exact() succeeds (IOW, the freq can be
found in arm opp table).
Shawn
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
