On Wed, Apr 22, 2015 at 11:11:34AM +0800, Pi-Cheng Chen wrote:
[..]
> >> +config ARM_MT8173_CPUFREQ
> >> + bool "Mediatek MT8173 CPUFreq support"
> >> + depends on ARCH_MEDIATEK && REGULATOR
> >
> > I think you want to 'select REGULATOR' here; because REGULATOR isn't
> > a user-visible option.
>
> I am not sure but I need it to be "depends on" as other SoC cpufreq
> drivers. Please check
> ARM_S3C2416_CPUFREQ_VCORESCALE in drivers/cpufreq/Kconfig.arm
> By the way, I would like to know more details about the visibility of
> these configurable
> options, would you kindly point me out some documents about it?
Paul pointed out that I was wrong, so I'll defer to him. My knowledge
has likely been outdated.
[..]
> >> +/* OPP table for LITTLE cores of MT8173 */
> >> +struct mtk_cpu_opp mt8173_l_opp[] = {
> >
> > static const?
>
> Yes. I miss "static" here. But I need those two array to be non-const
> so that I could
> fix up the exact voltage values by querying the supported voltages of
> regulators.
> Please check the mt8173_cpufreq_cpu_opp_fixup() function below.
Indeed. Thanks.
[..]
> >> +static int mtk_cpufreq_voltage_trace(struct cpu_dvfs_info *info,
> >> + struct mtk_cpu_opp *opp)
> >> +{
> >> + struct regulator *proc_reg = info->proc_reg;
> >> + struct regulator *sram_reg = info->sram_reg;
> >> + int old_vproc, new_vproc, old_vsram, new_vsram, vsram, vproc, ret;
> >> +
> >> + old_vproc = regulator_get_voltage(proc_reg);
> >> + old_vsram = regulator_get_voltage(sram_reg);
> >> +
> >> + new_vproc = opp->vproc;
> >> + new_vsram = opp->vsram;
> >> +
> >> + /*
> >> + * In the case the voltage is going to be scaled up, Vsram and Vproc
> >> + * need to be scaled up step by step. In each step, Vsram needs to be
> >> + * set to (Vproc + 200mV) first, then Vproc is set to (Vsram - 100mV).
> >> + * Repeat the step until Vsram and Vproc are set to target voltage.
> >> + */
> >> + if (old_vproc < new_vproc) {
> >> +next_up_step:
> >> + old_vsram = regulator_get_voltage(sram_reg);
> >> +
> >> + vsram = (new_vsram - old_vproc < MAX_VOLT_SHIFT) ?
> >> + new_vsram : old_vproc + MAX_VOLT_SHIFT;
> >> + vsram = get_regulator_voltage_floor(sram_reg, vsram);
> >> +
> >> + ret = regulator_set_voltage(sram_reg, vsram, vsram);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + vproc = (new_vsram == vsram) ?
> >> + new_vproc : vsram - MIN_VOLT_SHIFT;
> >> + vproc = get_regulator_voltage_ceil(proc_reg, vproc);
> >> +
> >> + ret = regulator_set_voltage(proc_reg, vproc, vproc);
> >> + if (ret) {
> >> + regulator_set_voltage(sram_reg, old_vsram, old_vsram);
> >> + return ret;
> >> + }
> >> +
> >> + if (new_vproc == vproc && new_vsram == vsram)
> >> + return 0;
> >> +
> >> + old_vproc = vproc;
> >> + goto next_up_step;
> >
> > Perhaps a naive question: but, is this the correct place to do this? I
> > would expect this stepping behavior to be implemented in the driver
> > controlling the regulator you are consuming. It seems strange to do it
> > here.
>
> This was already discussed in the last round of this series of patches.
> Please check the discussion[1]. Any suggestion would be welcomed.
> Thanks.
Interesting, thanks. Sorry for rehashing already-covered territory!
Josh
Attachment:
pgpWwHEjQcym2.pgp
Description: PGP signature
