Hi Greg, Thanks for reviewing.
On Fri, May 11, 2012 at 12:41 AM, Guyotte, Greg <[email protected]> wrote: > Hi Jean, > >> -----Original Message----- >> From: [email protected] [mailto:linux-omap- >> [email protected]] On Behalf Of J, KEERTHY >> Sent: Thursday, April 26, 2012 12:41 PM >> To: [email protected]; [email protected]; >> Hilman, Kevin; [email protected]; [email protected]; linux- >> [email protected] >> Cc: Pihet-XID, Jean; J, KEERTHY; Paul Walmsley; Gopinath, Thara; Menon, >> Nishanth >> Subject: [PATCH V3 07/10] ARM: OMAP2+: SmartReflex: Use per-OPP data >> structure >> >> From: Jean Pihet <[email protected]> >> >> The SmartReflex driver incorrectly treats some per-OPP data as data >> common to all OPPs (e.g., ERRMINLIMIT). Move this data into a per-OPP >> data structure. >> >> Furthermore, in order to make the SmartReflex implementation ready for >> the move to drivers/, remove the dependency from the SR driver code >> to the voltage layer by querying the data tables only from the SR device >> init code. >> >> Based on Paul's original code for the SmartReflex driver conversion. >> >> Signed-off-by: Jean Pihet <[email protected]> >> Signed-off-by: J Keerthy <[email protected]> >> Cc: Paul Walmsley <[email protected]> >> Cc: Thara Gopinath <[email protected]> >> Cc: Nishanth Menon <[email protected]> >> Cc: Kevin Hilman <[email protected]> >> --- >> arch/arm/mach-omap2/smartreflex.c | 38 +++++++++++++++++--------------- >> ---- >> arch/arm/mach-omap2/sr_device.c | 36 +++++++++++++++++++++++++++++--- >> -- >> include/linux/power/smartreflex.h | 8 +++++- >> 3 files changed, 54 insertions(+), 28 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach- >> omap2/smartreflex.c >> index acef08d..20075de 100644 >> --- a/arch/arm/mach-omap2/smartreflex.c >> +++ b/arch/arm/mach-omap2/smartreflex.c >> @@ -347,22 +347,23 @@ static void sr_v2_disable(struct omap_sr *sr) >> sr_write_reg(sr, IRQSTATUS, IRQSTATUS_MCUDISABLEACKINT); >> } >> >> -static u32 sr_retrieve_nvalue(struct omap_sr *sr, u32 efuse_offs) >> +static struct omap_sr_nvalue_table *sr_retrieve_nvalue_row( >> + struct omap_sr *sr, u32 efuse_offs) >> { >> int i; >> >> if (!sr->nvalue_table) { >> dev_warn(&sr->pdev->dev, "%s: Missing ntarget value table\n", >> __func__); >> - return 0; >> + return NULL; >> } >> >> for (i = 0; i < sr->nvalue_count; i++) { >> if (sr->nvalue_table[i].efuse_offs == efuse_offs) >> - return sr->nvalue_table[i].nvalue; >> + return &sr->nvalue_table[i]; >> } >> >> - return 0; >> + return NULL; >> } >> >> /* Public Functions */ >> @@ -586,7 +587,7 @@ int sr_enable(struct voltagedomain *voltdm, unsigned >> long volt) >> { >> struct omap_volt_data *volt_data; >> struct omap_sr *sr = _sr_lookup(voltdm); >> - u32 nvalue_reciprocal; >> + struct omap_sr_nvalue_table *nvalue_row; >> int ret; >> >> if (IS_ERR(sr)) { >> @@ -602,16 +603,16 @@ int sr_enable(struct voltagedomain *voltdm, unsigned >> long volt) >> return PTR_ERR(volt_data); >> } >> >> - nvalue_reciprocal = sr_retrieve_nvalue(sr, volt_data- >> >sr_efuse_offs); >> + nvalue_row = sr_retrieve_nvalue_row(sr, volt_data->sr_efuse_offs); >> >> - if (!nvalue_reciprocal) { >> - dev_warn(&sr->pdev->dev, "%s: NVALUE = 0 at voltage %ld\n", >> - __func__, volt); >> + if (!nvalue_row) { >> + dev_warn(&sr->pdev->dev, "%s: failure getting SR data for this >> voltage %ld\n", >> + __func__, volt); >> return -ENODATA; >> } >> >> /* errminlimit is opp dependent and hence linked to voltage */ >> - sr->err_minlimit = volt_data->sr_errminlimit; >> + sr->err_minlimit = nvalue_row->errminlimit; >> >> pm_runtime_get_sync(&sr->pdev->dev); >> >> @@ -624,7 +625,7 @@ int sr_enable(struct voltagedomain *voltdm, unsigned >> long volt) >> if (ret) >> return ret; >> >> - sr_write_reg(sr, NVALUERECIPROCAL, nvalue_reciprocal); >> + sr_write_reg(sr, NVALUERECIPROCAL, nvalue_row->nvalue); >> >> /* SRCONFIG - enable SR */ >> sr_modify_reg(sr, SRCONFIG, SRCONFIG_SRENABLE, SRCONFIG_SRENABLE); >> @@ -872,7 +873,6 @@ static int __init omap_sr_probe(struct platform_device >> *pdev) >> struct omap_sr_data *pdata = pdev->dev.platform_data; >> struct resource *mem, *irq; >> struct dentry *nvalue_dir; >> - struct omap_volt_data *volt_data; >> int i, ret = 0; >> >> sr_info = kzalloc(sizeof(struct omap_sr), GFP_KERNEL); >> @@ -990,12 +990,10 @@ static int __init omap_sr_probe(struct >> platform_device *pdev) >> goto err_debugfs; >> } >> >> - omap_voltage_get_volttable(sr_info->voltdm, &volt_data); >> - if (!volt_data) { >> - dev_warn(&pdev->dev, "%s: %s: No Voltage table for the" >> - " corresponding vdd. Cannot create debugfs" >> - "entries for n-values\n", >> - __func__, sr_info->name); >> + if (sr_info->nvalue_count == 0 || !sr_info->nvalue_table) { >> + dev_warn(&pdev->dev, "%s: %s: No Voltage table for the >> corresponding vdd. Cannot create debugfs entries for n-values\n", >> + __func__, sr_info->name); >> + >> ret = -ENODATA; >> goto err_debugfs; >> } >> @@ -1003,8 +1001,8 @@ static int __init omap_sr_probe(struct >> platform_device *pdev) >> for (i = 0; i < sr_info->nvalue_count; i++) { >> char name[NVALUE_NAME_LEN + 1]; >> >> - snprintf(name, sizeof(name), "volt_%d", >> - volt_data[i].volt_nominal); >> + snprintf(name, sizeof(name), "volt_%lu", >> + sr_info->nvalue_table[i].volt_nominal); >> (void) debugfs_create_x32(name, S_IRUGO | S_IWUSR, nvalue_dir, >> &(sr_info->nvalue_table[i].nvalue)); >> } >> diff --git a/arch/arm/mach-omap2/sr_device.c b/arch/arm/mach- >> omap2/sr_device.c >> index e081174..e107e39 100644 >> --- a/arch/arm/mach-omap2/sr_device.c >> +++ b/arch/arm/mach-omap2/sr_device.c >> @@ -36,7 +36,10 @@ static void __init sr_set_nvalues(struct omap_volt_data >> *volt_data, >> struct omap_sr_data *sr_data) >> { >> struct omap_sr_nvalue_table *nvalue_table; >> - int i, count = 0; >> + int i, j, count = 0; >> + >> + sr_data->nvalue_count = 0; >> + sr_data->nvalue_table = NULL; >> >> while (volt_data[count].volt_nominal) >> count++; >> @@ -44,8 +47,14 @@ static void __init sr_set_nvalues(struct omap_volt_data >> *volt_data, >> nvalue_table = kzalloc(sizeof(struct omap_sr_nvalue_table)*count, >> GFP_KERNEL); >> >> - for (i = 0; i < count; i++) { >> + if (!nvalue_table) { >> + pr_err("OMAP: SmartReflex: cannot allocate memory for n-value >> table\n"); >> + return; >> + } >> + >> + for (i = 0, j = 0; i < count; i++) { >> u32 v; >> + >> /* >> * In OMAP4 the efuse registers are 24 bit aligned. >> * A __raw_readl will fail for non-32 bit aligned address >> @@ -58,15 +67,30 @@ static void __init sr_set_nvalues(struct >> omap_volt_data *volt_data, >> omap_ctrl_readb(offset + 1) << 8 | >> omap_ctrl_readb(offset + 2) << 16; >> } else { >> - v = omap_ctrl_readl(volt_data[i].sr_efuse_offs); >> + v = omap_ctrl_readl(volt_data[i].sr_efuse_offs); >> } >> >> - nvalue_table[i].efuse_offs = volt_data[i].sr_efuse_offs; >> - nvalue_table[i].nvalue = v; >> + /* >> + * Many OMAP SoCs don't have the eFuse values set. >> + * For example, pretty much all OMAP3xxx before >> + * ES3.something. >> + * >> + * XXX There needs to be some way for board files or >> + * userspace to add these in. >> + */ >> + if (v == 0) >> + continue; >> + >> + nvalue_table[j].nvalue = v; >> + nvalue_table[j].efuse_offs = volt_data[i].sr_efuse_offs; >> + nvalue_table[j].errminlimit = volt_data[i].sr_errminlimit; >> + nvalue_table[j].volt_nominal = volt_data[i].volt_nominal; >> + >> + j++; >> } >> >> sr_data->nvalue_table = nvalue_table; >> - sr_data->nvalue_count = count; >> + sr_data->nvalue_count = j; >> } >> >> static int __init sr_dev_init(struct omap_hwmod *oh, void *user) >> diff --git a/include/linux/power/smartreflex.h >> b/include/linux/power/smartreflex.h >> index 78b795e..222f901 100644 >> --- a/include/linux/power/smartreflex.h >> +++ b/include/linux/power/smartreflex.h >> @@ -243,12 +243,16 @@ struct omap_sr_class_data { >> /** >> * struct omap_sr_nvalue_table - Smartreflex n-target value info >> * >> - * @efuse_offs: The offset of the efuse where n-target values are >> stored. >> - * @nvalue: The n-target value. >> + * @efuse_offs: The offset of the efuse where n-target values are >> stored. >> + * @nvalue: The n-target value. >> + * @errminlimit: The value of the ERRMINLIMIT bitfield for this n-target >> + * @volt_nominal: microvolts DC that the VDD is initially programmed to >> */ >> struct omap_sr_nvalue_table { >> u32 efuse_offs; >> u32 nvalue; >> + u32 errminlimit; >> + unsigned long volt_nominal; > > [GG] I would suggest that we also need to add errmaxlimit and errweight > fields to this structure. These are very much possible to change per OPP, > though it hasn't been common in implementations up to now. Ok. Since this patch set is focussed on minimal clean up and moving the code from mach-omap to drivers this is not addressed. I will sure take this up once this is moved. > >> }; >> >> /** >> -- >> 1.7.5.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-omap" in >> the body of a message to [email protected] >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- Regards and Thanks, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
