Cousson, Benoit said the following on 10/25/2009 05:12 PM:
>> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-
>>
[...]
>> + },
>> + /* *INDENT-ON* */
>> +};
>> +
>> +/* SR list for 3430 */
>> +static __initdata struct omap_sr_list omap34xx_srlist = {
>> + .num_sr = 2,
>> + .sr_list = {&omap34xx_sr1, &omap34xx_sr2}
>> +};
>> +
>> +/* The final SR list */
>> +static struct omap_sr_list *omap_srlist;
>>
>
> I have a couple a suggestions regarding the code partitioning:
>
> - SmartReflex is one IP with several instances; it means that only the base address will change between SR1 and SR2. There is no need to duplicate the mask and shift defines per SR.
>
might have been easier with a standard OCP wrapper and standard
INT/INTSTATUS regs for SR instead of the current integration.... but
yeah, I had been thinking of that- if O4/beyond could have the same IP
wrapped in a different register mapping, i should handle it then, not
dream about it now.. I get your point here.. there are a few places we
can kick it out and some places your are stuck with them!
> Moreover, SR being an IP, I think we can encode the one IP / several instance in a platform_device / platform_driver code. It will allow the support of several drivers for the same devices in order to implement for example class3, class2 or class1 drivers. SR can even be represented by an HWMOD.
>
what is your intention in misusing platform_device for a SOC specific
code? I think you have an idea here that I am completely missing. can
you care to elaborate?
> - The smartreflex.c file contains 34xx specifics code; it should not be there, only SR specific code should be there.
>
read the TODO. which specific 3430 code does it have? other than using
macros which have 34xx prefix - that should be killed -yep.
> - If we want to go further, I think that the VP/VC code should not be in SR code either. It is located in the PRM, and can be used independently of SR.
> - In a SR class 2 mode, the smartreflex driver will not use the direct
> connection to the VP.
> - If we don't want SR we can still use the VP/VC for device voltage
> control.
>
- How about adding - Smart reflex should actually fit in a
voltage control infrastructure so that the system can manipulate and set
voltage with and without SR..
that is what is really missing in our code base today -> SR and VP comes
plugged in close as buddies in all silicons, so if your recommendation
has an silicon example where OMAP SR talks not to VP, let me know and I
will second splitting the code :D. till then sorry.. you dont have a case.
now, it is a different case where you want to use SR as a pure
monitoring engine -> that is not an implementation that is exactly
better than class 3 operation.. why support it at all?
> [snip]
>
>
>> +/****************** PMIC WEAK FUNCTIONS FOR TWL4030 derivatives
>> **********/
>> +
>> +/**
>> + * @brief pmic_srinit - Power management IC initialization
>> + * for smart reflex. The current code is written for TWL4030
>> + * derivatives, replace in board file if PMIC requires
>> + * a different sequence
>> + *
>> + * @return result of operation
>> + */
>> +int __weak __init omap_pmic_srinit(void)
>> +{
>> + int ret = -ENODEV;
>> +#ifdef CONFIG_TWL4030_CORE
>> + u8 reg;
>> + /* Enable SR on T2 */
>> + ret = twl4030_i2c_read_u8(TWL4030_MODULE_PM_RECEIVER, ®,
>> + R_DCDC_GLOBAL_CFG);
>> +
>> + reg |= DCDC_GLOBAL_CFG_ENABLE_SRFLX;
>> + ret |= twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, reg,
>> + R_DCDC_GLOBAL_CFG);
>> +#endif /* End of CONFIG_TWL4030_CORE */
>> + return ret;
>> +}
>> +
>> +/**
>> + * @brief omap_pmic_voltage_ramp_delay - how much should this pmic ramp
>> delay
>> + * Various PMICs have different ramp up and down delays. choose to
>> implement
>> + * in required pmic file to override this function.
>> + * On TWL4030 derivatives:
>> + * T2 SMPS slew rate (min) 4mV/uS, step size 12.5mV,
>> + * 2us added as buffer.
>> + *
>> + * @param srid - which SR is this for?
>> + * @param target_vsel - targetted voltage selction
>> + * @param current_vsel - current voltage selection
>> + *
>> + * @return delay in uSeconds
>> + */
>> +u32 __weak omap_pmic_voltage_ramp_delay(u8 srid, u8 target_vsel,
>> + u8 current_vsel)
>> +{
>> + u32 t2_smps_steps = abs(target_vsel - current_vsel);
>> + u32 t2_smps_delay = ((t2_smps_steps * 125) / 40) + 2;
>> + return t2_smps_delay;
>> +}
>> +
>> +#ifdef CONFIG_OMAP_VC_BYPASS_UPDATE
>> +/**
>> + * @brief omap_pmic_voltage_cmds - hook for pmic command sequence
>> + * to be send out which are specific to pmic to set a specific voltage.
>> + * this should inturn call vc_send_command with the required sequence
>> + * The current implementation is for TWL4030 derivatives
>> + *
>> + * @param srid - which SR is this for?
>> + * @param target_vsel - what voltage is desired to be set?
>> + *
>> + * @return specific value to set.
>> + */
>> +int __weak omap_pmic_voltage_cmds(u8 srid, u8 target_vsel)
>> +{
>> + u8 reg_addr = (srid == SR1) ? R_VDD1_SR_CONTROL : R_VDD2_SR_CONTROL;
>> + u16 timeout = COUNT_TIMEOUT_TWL4030_VCBYPASS;
>> + return vc_send_command(R_SRI2C_SLAVE_ADDR, reg_addr, target_vsel,
>> + &timeout);
>> +}
>> +#endif /* ifdef CONFIG_OMAP_VC_BYPASS_UPDATE */
>> +
>>
>
> The TWL4030 specific code should not be there even if this is the default PMIC, it should be in TWL4030 driver code.
> The usage of weak functions will prevent a multiple OMAP build with different PMIC to work.
> The mapping between omap and TWL4030 should be done in the board specific code.
>
hmm good point.. have a recommendation here?
> [snip]
>
>
>
>> +/*********************** DVFS Entry POINTS
>> **********************************/
>> +
>> +/**
>> + * @brief sr_vp_enable_both - enable both vp and sr
>> + *
>> + * @param target_opp - targetted op
>> + * @param current_opp - current opp
>> + *
>> + * @return 0 if ok, 1 if not ok
>> + */
>> +int sr_vp_enable_both(u32 target_opp, u32 current_opp)
>>
>
> It thinks that it not obvious from the name to understand what this API is doing, especially if we consider the other enable_smartreflex API.
> Maybe smartreflex_change_opp or sr_change_opp will more explicit. In any case the vp should be removed. The fact that VP is used internally is implementation dependent.
>
I dont see a reason why VP should need independent consideration as SR.
today SR talks to VP and there is no usage of SR sticking it's data else
where..
but on the naming -> yeah sr_vp_enable both --> explained in the comment
above - enable both SR and VP.. now, if you state DVFS entry point
should state stop_auto_voltage_change() you have a case there.. I
accept.. I also point you to my TODO above: a better voltage setting
infrastructure is needed than hacking our voltage code on top of current
code.
>
>> +{
>> + struct omap_sr *sr;
>> + u32 vdd, target_opp_no;
>> + int ret = 0;
>> +
>> + vdd = get_vdd(target_opp);
>> + target_opp_no = get_opp_no(target_opp);
>> + sr = get_sr_from_vdd(vdd);
>> +
>> + if (sr->is_autocomp_active && sr->is_sr_reset) {
>> + ret = srvp_enable(sr, target_opp_no);
>> + if (ret) {
>> + pr_err("SR[%d]:enableboth:"
>> + "failed enable SR\n", sr->srid);
>> + }
>> + }
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(sr_vp_enable_both);
>> +
>> +/**
>> + * @brief sr_vp_disable_both - disable both vp and sr
>> + *
>> + * @param target_opp - targetted opp
>> + * @param current_opp - current opp
>> + *
>> + * @return 0 if ok, 1 if not ok
>> + */
>> +int sr_vp_disable_both(u32 target_opp, u32 current_opp)
>> +{
>> + struct omap_sr *sr;
>> + u32 vdd;
>> + int ret = 0;
>> +
>> + vdd = get_vdd(target_opp);
>> + sr = get_sr_from_vdd(vdd);
>> +
>> + if (sr->is_autocomp_active && !sr->is_sr_reset) {
>> + ret = srvp_disable(sr);
>> + if (ret) {
>> + pr_err("SR[%d]:disableboth:"
>> + "failed disable SR\n", sr->srid);
>> + }
>> + }
>> +
>> + return ret;
>> +
>> +}
>> +EXPORT_SYMBOL(sr_vp_disable_both);
>> +
>> +/**
>> + * @brief sr_voltage_set - setup a voltage requested
>> + *
>> + * @param target_opp - targetted opp
>> + * @param current_opp - current opp
>> + * @param target_vsel - targeted voltage
>> + * @param current_vsel - current voltage
>> + *
>> + * @return - result of op -0 if ok, else value
>> + */
>> +int sr_voltage_set(u32 target_opp, u32 current_opp,
>> + u8 target_vsel, u8 current_vsel)
>> +{
>> + struct omap_sr *sr;
>> + u8 vdd, target_opp_no;
>> + int ret;
>> +
>> + vdd = get_vdd(target_opp);
>> + target_opp_no = get_opp_no(target_opp);
>> + sr = get_sr_from_vdd(vdd);
>> +
>> + ret =
>> + SR_CHOSEN_VOLTAGE_UPDATE_MECH(sr, target_opp_no, target_vsel,
>> + current_vsel);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(sr_voltage_set);
>> +
>> +/*********************** CPUIDLE ENTRY POINTS
>> *********************************/
>> +
>> +/**
>> + * @brief disable_smartreflex - disable SmartReflex before WFI
>> + *
>> + * @param srid SRID
>> + */
>> +void disable_smartreflex(u8 srid)
>> +{
>> + struct omap_sr *sr = NULL;
>> + int ret;
>> + u32 current_opp_no;
>> +
>> + /* I want to be in irq_disabled context..
>> + * else I will die.. find the rootcause and fix it instead
>> + */
>> + BUG_ON(!irqs_disabled());
>> +
>> + sr = get_sr(srid);
>> +
>> + current_opp_no = get_current_opp_number_from_sr(sr);
>> +
>> + if (sr->is_autocomp_active && !sr->is_sr_reset) {
>> + sr->req_opp_no = current_opp_no;
>> + ret = srvp_disable(sr);
>> + if (ret)
>> + pr_err("SR[%d]:disable_smartreflex:"
>> + "failed disable SR\n", sr->srid);
>> + }
>> +
>> + /* Reset the voltage for current OPP to nominal if SR was enabled */
>> + if (sr->is_autocomp_active)
>> + sr_vp_reset_voltage(srid);
>> +}
>> +EXPORT_SYMBOL(disable_smartreflex);
>> +
>> +/**
>> + * @brief enable_smartreflex - enable smart reflex after WFI is hit
>> + *
>> + * @param srid -SR ID to hit
>> + */
>> +void enable_smartreflex(u8 srid)
>>
>
> The naming is not coherent with previous API name sr_vp_enable_both. You should use the same denomination for smartreflex everywhere.
>
NAK - read the context please before you comment -> the call is from
cpu_idle perspective..
>
>> +{
>> + struct omap_sr *sr;
>> + int ret;
>> + u32 current_opp_no;
>> +
>> + /* I want to be in irq_disabled context..
>> + * else I will die.. find the rootcause and fix it instead
>> + */
>> + BUG_ON(!irqs_disabled());
>> +
>> + sr = get_sr(srid);
>> + current_opp_no = get_current_opp_number_from_sr(sr);
>> +
>> + if (sr->is_autocomp_active && sr->is_sr_reset) {
>> + ret = srvp_enable(sr, current_opp_no);
>> + if (ret)
>> + pr_err("SR[%d]:enable_smartreflex:"
>> + "failed enable SR\n", sr->srid);
>> + }
>> +}
>> +EXPORT_SYMBOL(enable_smartreflex);
>> +
>>
>
> [snip]
>
> For my point of view, this code should be dispatched into several files: smartreflex.c (pure SR IP driver), vp_vc.c (voltage processor and control), pmic driver, omap specific code and board initialization code.
>
come up with a clean proposal that can have code attached to it, or
better still a patch and I will gladly back my patch out.
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
