Nishanth Menon <n...@ti.com> writes: > Voltage values can get confusing in meaning with various SmartReflex > classes being active. Depending on the class used, the actual voltage > selected might be a variant. For example: > With SmartReflex AVS class 3: > a) If we don't have SR enabled, we will go to volt_nominal. > b) If have SR enabled, we go to volt_nominal, then enable SR and > expect it to adjust voltage. We don't really care about the > resultant voltage. > Essentially, when we ask voltage layer to scale to voltage x for OPP > y, it always means x is the nominal voltage for OPP y. > > Now, once we introduce SmartReflex AVS class 1.5: > a) If you are booting for the first time OR if you never enabled SR > before, we always go to volt_nominal. > b) If you enable SR and the OPP is calibrated, we will not enable SR > again when that OPP is accessed anymore, but when we set voltage for > an OPP, the voltage achieved will be volt_calibrated. > c) If recalibration timeout triggers or SR is disabled after a > calibration, the calibrated values are not valid anymore, at this point, > setting the voltage will mean volt_dynamic_nominal. > So, depending on which state the system is at, voltage for that OPP > we are setting has not 1 single value anymore, but 3 possible valid > values. > > For upper layers(DVFS/cpufreq OMAP SoC layers) to use voltage values, it > will need to know which type of voltage AVS strategy is being used and > the corresponding system state from voltage layer perspective. This > would replicate the role of voltage layer, SmartReflex AVS in the upper > layers and make the corresponding implementations complex. > > Since each voltage domain contains a set of volt_data structs representing > a voltage point that is supported for that domain, volt_data is a more > accurate representation of the voltage point we are interested in going to, > and the actual translation of this voltage point to the voltage value is > done inside the voltage layer. Doing this allows the users of the voltage > layer to be blissfully ignorant of any complexity of the underneath > layers and simplify the implementation of dependent layers. > > Signed-off-by: Nishanth Menon <n...@ti.com>
Mostly coding style and/or readability comments below... > --- > arch/arm/mach-omap2/pm.c | 3 +- > arch/arm/mach-omap2/smartreflex-class3.c | 3 +- > arch/arm/mach-omap2/voltage.c | 75 > +++++++++++++++++------------- > arch/arm/mach-omap2/voltage.h | 17 +++++-- > 4 files changed, 59 insertions(+), 39 deletions(-) > > diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c > index 2c3a253..2372744 100644 > --- a/arch/arm/mach-omap2/pm.c > +++ b/arch/arm/mach-omap2/pm.c > @@ -209,7 +209,8 @@ static int __init omap2_set_init_voltage(char *vdd_name, > char *clk_name, > goto exit; > } > > - omap_voltage_scale_vdd(voltdm, bootup_volt); > + omap_voltage_scale_vdd(voltdm, > + omap_voltage_get_voltdata(voltdm, bootup_volt)); coding style: to avoid wrapping (which IMO affects readabiliyt), assign volt_data to it's own variable then pass it to scale_vdd. > return 0; > > exit: > diff --git a/arch/arm/mach-omap2/smartreflex-class3.c > b/arch/arm/mach-omap2/smartreflex-class3.c > index f438cf4..2ee48af 100644 > --- a/arch/arm/mach-omap2/smartreflex-class3.c > +++ b/arch/arm/mach-omap2/smartreflex-class3.c > @@ -15,7 +15,8 @@ > > static int sr_class3_enable(struct voltagedomain *voltdm) > { > - unsigned long volt = omap_voltage_get_nom_volt(voltdm); > + unsigned long volt = omap_get_operation_voltage( > + omap_voltage_get_nom_volt(voltdm)); readability/wrapping Also, the name of the new function doesn't follow the naming convention of the rest of the voltage layer: e.g. omap_voltage_... > if (!volt) { > pr_warning("%s: Curr voltage unknown. Cannot enable sr_%s\n", > diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c > index 76bcaee..a12ac1e 100644 > --- a/arch/arm/mach-omap2/voltage.c > +++ b/arch/arm/mach-omap2/voltage.c > @@ -58,7 +58,7 @@ static struct dentry *voltage_dir; > > /* Init function pointers */ > static int vp_forceupdate_scale_voltage(struct omap_vdd_info *vdd, > - unsigned long target_volt); > + struct omap_volt_data *target_volt); > > static u32 omap3_voltage_read_reg(u16 mod, u8 offset) > { > @@ -164,13 +164,20 @@ static int vp_volt_debug_get(void *data, u64 *val) > static int nom_volt_debug_get(void *data, u64 *val) > { > struct omap_vdd_info *vdd = (struct omap_vdd_info *) data; > + struct omap_volt_data *volt_data; > > if (!vdd) { > pr_warning("Wrong paramater passed\n"); > return -EINVAL; > } > > - *val = omap_voltage_get_nom_volt(&vdd->voltdm); > + volt_data = omap_voltage_get_nom_volt(&vdd->voltdm); > + if (IS_ERR_OR_NULL(volt_data)) { > + pr_warning("%s: No voltage/domain?\n", __func__); > + return -ENODEV; > + } > + > + *val = volt_data->volt_nominal; > > return 0; > } > @@ -184,7 +191,8 @@ static void vp_latch_vsel(struct omap_vdd_info *vdd) > unsigned long uvdc; > char vsel; > > - uvdc = omap_voltage_get_nom_volt(&vdd->voltdm); > + uvdc = omap_get_operation_voltage( > + omap_voltage_get_nom_volt(&vdd->voltdm)); wrapping > if (!uvdc) { > pr_warning("%s: unable to find current voltage for vdd_%s\n", > __func__, vdd->voltdm.name); > @@ -302,13 +310,19 @@ static void __init vdd_debugfs_init(struct > omap_vdd_info *vdd) > > /* Voltage scale and accessory APIs */ > static int _pre_volt_scale(struct omap_vdd_info *vdd, > - unsigned long target_volt, u8 *target_vsel, u8 *current_vsel) > + struct omap_volt_data *target_volt, u8 *target_vsel, > + u8 *current_vsel) > { > - struct omap_volt_data *volt_data; > const struct omap_vc_common_data *vc_common; > const struct omap_vp_common_data *vp_common; > u32 vc_cmdval, vp_errgain_val; > > + if (IS_ERR_OR_NULL(target_volt) || IS_ERR_OR_NULL(vdd) || Checking for NULL on arguments passed in is fine, but not sure the error check here makes any sense. > + !target_vsel || !current_vsel) { > + pr_err("%s: invalid parms!\n", __func__); > + return -EINVAL; > + } > + > vc_common = vdd->vc_data->vc_common; > vp_common = vdd->vp_data->vp_common; > > @@ -332,12 +346,8 @@ static int _pre_volt_scale(struct omap_vdd_info *vdd, > return -EINVAL; > } > > - /* Get volt_data corresponding to target_volt */ > - volt_data = omap_voltage_get_voltdata(&vdd->voltdm, target_volt); > - if (IS_ERR(volt_data)) > - volt_data = NULL; > - > - *target_vsel = vdd->pmic_info->uv_to_vsel(target_volt); > + *target_vsel = vdd->pmic_info->uv_to_vsel( > + omap_get_operation_voltage(target_volt)); wrapping > *current_vsel = vdd->read_reg(prm_mod_offs, vdd->vp_data->voltage); > > /* Setting the ON voltage to the new target voltage */ > @@ -347,22 +357,21 @@ static int _pre_volt_scale(struct omap_vdd_info *vdd, > vdd->write_reg(vc_cmdval, prm_mod_offs, vdd->vc_data->cmdval_reg); > > /* Setting vp errorgain based on the voltage */ > - if (volt_data) { > - vp_errgain_val = vdd->read_reg(prm_mod_offs, > - vdd->vp_data->vpconfig); > - vdd->vp_rt_data.vpconfig_errorgain = volt_data->vp_errgain; > - vp_errgain_val &= ~vp_common->vpconfig_errorgain_mask; > - vp_errgain_val |= vdd->vp_rt_data.vpconfig_errorgain << > - vp_common->vpconfig_errorgain_shift; > - vdd->write_reg(vp_errgain_val, prm_mod_offs, > - vdd->vp_data->vpconfig); > - } > + vp_errgain_val = vdd->read_reg(prm_mod_offs, > + vdd->vp_data->vpconfig); > + vdd->vp_rt_data.vpconfig_errorgain = target_volt->vp_errgain; > + vp_errgain_val &= ~vp_common->vpconfig_errorgain_mask; > + vp_errgain_val |= vdd->vp_rt_data.vpconfig_errorgain << > + vp_common->vpconfig_errorgain_shift; > + vdd->write_reg(vp_errgain_val, prm_mod_offs, > + vdd->vp_data->vpconfig); > > return 0; > } > > static void _post_volt_scale(struct omap_vdd_info *vdd, > - unsigned long target_volt, u8 target_vsel, u8 current_vsel) > + struct omap_volt_data *target_volt, u8 target_vsel, > + u8 current_vsel) > { > u32 smps_steps = 0, smps_delay = 0; > > @@ -377,7 +386,7 @@ static void _post_volt_scale(struct omap_vdd_info *vdd, > > /* vc_bypass_scale_voltage - VC bypass method of voltage scaling */ > static int vc_bypass_scale_voltage(struct omap_vdd_info *vdd, > - unsigned long target_volt) > + struct omap_volt_data *target_volt) > { > u32 loop_cnt = 0, retries_cnt = 0; > u32 vc_valid, vc_bypass_val_reg, vc_bypass_value; > @@ -429,7 +438,7 @@ static int vc_bypass_scale_voltage(struct omap_vdd_info > *vdd, > > /* VP force update method of voltage scaling */ > static int vp_forceupdate_scale_voltage(struct omap_vdd_info *vdd, > - unsigned long target_volt) > + struct omap_volt_data *target_volt) > { > u32 vpconfig; > u8 target_vsel, current_vsel, prm_irqst_reg; > @@ -675,16 +684,15 @@ ovdc_out: > * omap_voltage_get_nom_volt() - Gets the current non-auto-compensated > voltage > * @voltdm: pointer to the VDD for which current voltage info is needed > * > - * API to get the current non-auto-compensated voltage for a VDD. > - * Returns 0 in case of error else returns the current voltage for the VDD. > + * API to get the current voltage data pointer for a VDD. > */ > -unsigned long omap_voltage_get_nom_volt(struct voltagedomain *voltdm) > +struct omap_volt_data *omap_voltage_get_nom_volt(struct voltagedomain > *voltdm) > { > struct omap_vdd_info *vdd; > > if (IS_ERR_OR_NULL(voltdm)) { > pr_warning("%s: VDD specified does not exist!\n", __func__); > - return 0; > + return ERR_PTR(-ENODATA); > } > > vdd = container_of(voltdm, struct omap_vdd_info, voltdm); > @@ -819,18 +827,19 @@ void omap_vp_disable(struct voltagedomain *voltdm) > * omap_voltage_scale_vdd() - API to scale voltage of a particular > * voltage domain. > * @voltdm: pointer to the VDD which is to be scaled. > - * @target_volt: The target voltage of the voltage domain > + * @target_volt: The target voltage data for the voltage domain > * > * This API should be called by the kernel to do the voltage scaling > * for a particular voltage domain during dvfs or any other situation. > */ > int omap_voltage_scale_vdd(struct voltagedomain *voltdm, > - unsigned long target_volt) > + struct omap_volt_data *target_volt) > { > struct omap_vdd_info *vdd; > > - if (IS_ERR_OR_NULL(voltdm)) { > - pr_warning("%s: VDD specified does not exist!\n", __func__); > + if (IS_ERR_OR_NULL(voltdm) || IS_ERR_OR_NULL(target_volt)) { checking for NULL should suffice > + pr_warning("%s: Bad Params vdm=%p tv=%p!\n", __func__, > + voltdm, target_volt); > return -EINVAL; > } > > @@ -856,7 +865,7 @@ int omap_voltage_scale_vdd(struct voltagedomain *voltdm, > */ > void omap_voltage_reset(struct voltagedomain *voltdm) > { > - unsigned long target_uvdc; > + struct omap_volt_data *target_uvdc; > > if (IS_ERR_OR_NULL(voltdm)) { > pr_warning("%s: VDD specified does not exist!\n", __func__); > diff --git a/arch/arm/mach-omap2/voltage.h b/arch/arm/mach-omap2/voltage.h > index e9f5408..6e9acd6 100644 > --- a/arch/arm/mach-omap2/voltage.h > +++ b/arch/arm/mach-omap2/voltage.h > @@ -131,25 +131,25 @@ struct omap_vdd_info { > const struct omap_vfsm_instance_data *vfsm; > struct voltagedomain voltdm; > struct dentry *debug_dir; > - u32 curr_volt; > + struct omap_volt_data *curr_volt; > bool vp_enabled; > u32 (*read_reg) (u16 mod, u8 offset); > void (*write_reg) (u32 val, u16 mod, u8 offset); > int (*volt_scale) (struct omap_vdd_info *vdd, > - unsigned long target_volt); > + struct omap_volt_data *target_volt); > }; > > unsigned long omap_vp_get_curr_volt(struct voltagedomain *voltdm); > void omap_vp_enable(struct voltagedomain *voltdm); > void omap_vp_disable(struct voltagedomain *voltdm); > int omap_voltage_scale_vdd(struct voltagedomain *voltdm, > - unsigned long target_volt); > + struct omap_volt_data *target_volt); > void omap_voltage_reset(struct voltagedomain *voltdm); > void omap_voltage_get_volttable(struct voltagedomain *voltdm, > struct omap_volt_data **volt_data); > struct omap_volt_data *omap_voltage_get_voltdata(struct voltagedomain > *voltdm, > unsigned long volt); > -unsigned long omap_voltage_get_nom_volt(struct voltagedomain *voltdm); > +struct omap_volt_data *omap_voltage_get_nom_volt(struct voltagedomain > *voltdm); > struct dentry *omap_voltage_get_dbgdir(struct voltagedomain *voltdm); > int __init omap_voltage_early_init(s16 prm_mod, s16 prm_irqst_mod, > struct omap_vdd_info *omap_vdd_array[], > @@ -181,4 +181,13 @@ static inline struct voltagedomain > *omap_voltage_domain_lookup(char *name) > } > #endif > > +/* convert volt data to the voltage for the voltage data */ > +static inline unsigned long omap_get_operation_voltage( > + struct omap_volt_data *vdata) > +{ > + if (IS_ERR_OR_NULL(vdata)) checking for NULL chould suffice > + return 0; > + return vdata->volt_nominal; > +} > + > #endif Also, this function name should follow the naming convention of the rest of the voltage layer. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html