Re: [PATCHv3 06/11] omap3+: vc: use new vc_params and vp_params in parameter calculations

Kevin Hilman
2011-11-04T21:11:32Z

Tero Kristo <t-kristo@ti.com> writes:

> Signed-off-by: Tero Kristo <t-kristo@ti.com>

Missing descriptive changelog.

Some minor comments below...

Also, all of these timing calculations would benefit from a few more
pairs of eyes looking at this stuff, as I'm not the expert here. In
particular, I'd like to see a reviewed-by/acked-by on this stuff from
Nishanth Menon and anyone else who is familiar with the various timing
calculations.

> ---
> arch/arm/mach-omap2/vc.c | 166 ++++++++++++++++++++++++++++++++++++++--------
> 1 files changed, 138 insertions(+), 28 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/vc.c b/arch/arm/mach-omap2/vc.c
> index 16fa912..c50e598 100644
> --- a/arch/arm/mach-omap2/vc.c
> +++ b/arch/arm/mach-omap2/vc.c
> @@ -1,14 +1,18 @@
> #include <linux/kernel.h>
> #include <linux/delay.h>
> #include <linux/init.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
>
> #include <plat/cpu.h>
> +#include <plat/prcm.h>
>
> #include "voltage.h"
> #include "vc.h"
> #include "prm-regbits-34xx.h"
> #include "prm-regbits-44xx.h"
> #include "prm44xx.h"
> +#include "scrm44xx.h"
>
> /**
> * struct omap_vc_channel_cfg - describe the cfg_channel bitfield
> @@ -194,44 +198,156 @@ int omap_vc_bypass_scale(struct voltagedomain *voltdm,
> return 0;
> }
>
> -static void __init omap3_vfsm_init(struct voltagedomain *voltdm)
> +static void omap3_set_ret_timings(struct voltagedomain *voltdm)
> {
> - /*
> - * Voltage Manager FSM parameters init
> - * XXX This data should be passed in from the board file
> - */
> - voltdm->write(OMAP3_CLKSETUP, OMAP3_PRM_CLKSETUP_OFFSET);
> - voltdm->write(OMAP3_VOLTOFFSET, OMAP3_PRM_VOLTOFFSET_OFFSET);
> - voltdm->write(OMAP3_VOLTSETUP2, OMAP3_PRM_VOLTSETUP2_OFFSET);
> + unsigned long voltsetup1;
> + struct clk *sys_ck;
> + u32 sys_clk_rate;
> +
> + sys_ck = clk_get(NULL, "sys_ck");
> + if (IS_ERR(sys_ck)) {
> + pr_warning("%s: unable to get sys_ck to calculate "
> + "vdd_%s timings\n", __func__, voltdm->name);
> + return;
> + }
> + sys_clk_rate = clk_get_rate(sys_ck);
> + clk_put(sys_ck);

sys_clk_rate is already available in voltdm->sys_clk.rate, initialized
in late_init.

> + voltsetup1 = (voltdm->vc_param->on - voltdm->vc_param->ret) /
> + voltdm->pmic->slew_rate;
> +
> + voltsetup1 = voltsetup1 * sys_clk_rate / 8 / 1000000 + 1;
> + voltdm->rmw(voltdm->vfsm->voltsetup_mask,
> + voltsetup1 << __ffs(voltdm->vfsm->voltsetup_mask),
> + voltdm->vfsm->voltsetup_reg);
> +
> + /* set voltsetup 2 to 0 */

Comment is not helpful. Either describe why, or remove the comment
(preferably the former.)

> + voltdm->write(0, OMAP3_PRM_VOLTSETUP2_OFFSET);
> +}
> +
> +static void omap3_set_off_timings(struct voltagedomain *voltdm)
> +{
> + unsigned long clksetup;
> + unsigned long voltsetup2;
> +
> + clksetup = voltdm->read(OMAP3_PRM_CLKSETUP_OFFSET);
> +
> + /* voltsetup 2 in us */
> + voltsetup2 = voltdm->vc_param->on / voltdm->pmic->slew_rate;
> +
> + /* convert to 32k clk cycles */
> + voltsetup2 = voltsetup2 * 32768 / 1000000 + 1;
> +
> + voltdm->write(voltsetup2, OMAP3_PRM_VOLTSETUP2_OFFSET);
> +
> + /* set voltsetup1 to 0 */
> + voltdm->rmw(voltdm->vfsm->voltsetup_mask, 0,
> + voltdm->vfsm->voltsetup_reg);
> +
> + /* set voltoffset */

comment not helpful

> + voltdm->write(clksetup - voltsetup2, OMAP3_PRM_VOLTOFFSET_OFFSET);
> }
>
> static void __init omap3_vc_init_channel(struct voltagedomain *voltdm)
> {
> - static bool is_initialized;
> + omap3_set_off_timings(voltdm);
> +}
>
> - if (is_initialized)
> - return;
> +static u32 omap4_calc_volt_ramp(struct voltagedomain *voltdm, u32 voltage_diff,
> + u32 clk_rate)
> +{
> + u32 prescaler;
> + u32 cycles;
> + u32 time;
> +
> + time = voltage_diff / voltdm->pmic->slew_rate;
> +
> + cycles = clk_rate / 1000 * time;
> +
> + cycles /= 64;
> + prescaler = 0;
> +
> + /* shift to next prescaler until no overflow */
> +
> + /* scale for div 256 = 64 * 4 */
> + if (cycles > 63) {
> + cycles /= 4;
> + prescaler++;
> + }
> +
> + /* scale for div 512 = 256 * 2 */
> + if (cycles > 63) {
> + cycles /= 2;
> + prescaler++;
> + }
> +
> + /* scale for div 2048 = 512 * 4 */
> + if (cycles > 63) {
> + cycles /= 4;
> + prescaler++;
> + }
> +
> + /* check for overflow => invalid ramp time */
> + if (cycles > 63) {
> + pr_warning("%s: invalid setuptime for vdd_%s\n", __func__,
> + voltdm->name);
> + return 0;
> + }
>
> - omap3_vfsm_init(voltdm);
> + cycles++;
>
> - is_initialized = true;
> + return (prescaler << OMAP4430_RAMP_UP_PRESCAL_SHIFT) |
> + (cycles << OMAP4430_RAMP_UP_COUNT_SHIFT);
> }
>
> +static void omap4_set_timings(struct voltagedomain *voltdm, bool off_mode)
> +{
> + struct clk *sys_ck;
> + u32 sys_clk_rate;
> + u32 val;
> + u32 ramp;
> +
> + sys_ck = clk_get(NULL, "sys_clkin_ck");
> + if (IS_ERR(sys_ck)) {
> + pr_warning("%s: unable to get sys_ck to calculate "
> + "vdd_%s timings\n", __func__, voltdm->name);
> + return;
> + }
> + sys_clk_rate = clk_get_rate(sys_ck);
> + clk_put(sys_ck);
> +
> + /* configure the setup times */
> + val = voltdm->read(voltdm->vfsm->voltsetup_reg);
> +
> + if (off_mode)
> + ramp = omap4_calc_volt_ramp(voltdm,
> + voltdm->vc_param->on - voltdm->vc_param->off,
> + sys_clk_rate);
> + else
> + ramp = omap4_calc_volt_ramp(voltdm,
> + voltdm->vc_param->on - voltdm->vc_param->ret,
> + sys_clk_rate);
> +
> + if (!ramp)
> + return;
> +
> + val |= ramp << OMAP4430_RAMP_DOWN_COUNT_SHIFT;
> +
> + val |= ramp << OMAP4430_RAMP_UP_COUNT_SHIFT;
> +
> + voltdm->write(val, voltdm->vfsm->voltsetup_reg);
> +}
>
> /* OMAP4 specific voltage init functions */
> static void __init omap4_vc_init_channel(struct voltagedomain *voltdm)
> {
> - static bool is_initialized;
> u32 vc_val;
>
> - if (is_initialized)
> - return;
> + omap4_set_timings(voltdm, true);

Why the 2nd argument (off_mode)? Is it planned to change the timings
at runtime later?

> /* XXX These are magic numbers and do not belong! */
> vc_val = (0x60 << OMAP4430_SCLL_SHIFT | 0x26 << OMAP4430_SCLH_SHIFT);
> voltdm->write(vc_val, OMAP4_PRM_VC_CFG_I2C_CLK_OFFSET);
> -
> - is_initialized = true;
> }
>
> /**
> @@ -305,7 +421,6 @@ void __init omap_vc_init_channel(struct voltagedomain *voltdm)
> vc->i2c_slave_addr = voltdm->pmic->i2c_slave_addr;
> vc->volt_reg_addr = voltdm->pmic->volt_reg_addr;
> vc->cmd_reg_addr = voltdm->pmic->cmd_reg_addr;
> - vc->setup_time = voltdm->pmic->volt_setup_time;
>
> /* Configure the i2c slave address for this VC */
> voltdm->rmw(vc->smps_sa_mask,
> @@ -329,10 +444,10 @@ void __init omap_vc_init_channel(struct voltagedomain *voltdm)
> }
>
> /* Set up the on, inactive, retention and off voltage */
> - on_vsel = voltdm->pmic->uv_to_vsel(voltdm->pmic->on_volt);
> - onlp_vsel = voltdm->pmic->uv_to_vsel(voltdm->pmic->onlp_volt);
> - ret_vsel = voltdm->pmic->uv_to_vsel(voltdm->pmic->ret_volt);
> - off_vsel = voltdm->pmic->uv_to_vsel(voltdm->pmic->off_volt);
> + on_vsel = voltdm->pmic->uv_to_vsel(voltdm->vc_param->on);
> + onlp_vsel = voltdm->pmic->uv_to_vsel(voltdm->vc_param->onlp);
> + ret_vsel = voltdm->pmic->uv_to_vsel(voltdm->vc_param->ret);
> + off_vsel = voltdm->pmic->uv_to_vsel(voltdm->vc_param->off);
> val = ((on_vsel << vc->common->cmd_on_shift) |
> (onlp_vsel << vc->common->cmd_onlp_shift) |
> (ret_vsel << vc->common->cmd_ret_shift) |
> @@ -343,11 +458,6 @@ void __init omap_vc_init_channel(struct voltagedomain *voltdm)
> /* Channel configuration */
> omap_vc_config_channel(voltdm);
>
> - /* Configure the setup times */
> - voltdm->rmw(voltdm->vfsm->voltsetup_mask,
> - vc->setup_time << __ffs(voltdm->vfsm->voltsetup_mask),
> - voltdm->vfsm->voltsetup_reg);
> -
> omap_vc_i2c_init(voltdm);
>
> if (cpu_is_omap34xx())

Kevin