Benoit, thanks.. overall good discussion that is nice to take in.. more comments follow: Cousson, Benoit said the following on 10/26/2009 08:05 AM: >> From: Nishanth Menon [mailto:menon.nishanth@xxxxxxxxx] >> >> 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! >> > > SR is a regular IP, with a dedicated PD, dedicated fclk and bound to the L4... > VP/VC is not, that why I was proposing to separated them. > I am not denying it, but VP/VC functions are already separated out where I could do it (note the seperation of function names), only they dont have a new file perse.. but that is not needed at the moment. > >>> 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? >> > > AFAIK, platform_device is there in order to deal with SoC devices??? > Considering that SR is an IP that can be in several SoC, it looks to me the perfect candidate for platform_device/platform_driver. > Am I missing something? > Everything abstracted is a perfect candidate for device-driver model, but that is beside the point. platform_device traditionally has been platform aka board specific info, not SOC specific info.. > >>> - 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. >> > > omap34xx_sr1, omap34xx_sr2, omap34xx_srlist... seem to be quite OMAP34xx specifics... > please read the code carefully next time, look at how it is copied based on cpu_is_ api in in __init?? yes, there are specific paramaters to SOC, you cannot avoid that. but you can make the handling independent of the SOC. that is what I have done.. let me know if you have a better idea.. :D.. > >>> - 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.. >> > > Fully agree, that was one of the proposals I had in mind, but since it will require some more works, it should be done in a second pass. > > >> 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. >> > > We had the case in several chips that unfortunately are not there anymore > :-( > The SR IP was done to handle several config, having SR tied to VP/VC is a chip dependant implementation. > lets bring in a proper voltage control infrastructure then worry about SR class2/class3 implementation. class2 is inferior to class 3 and the overhead of cpu intervention should be balanced with a proper latency heuristics, the infrastructure is sadly missing at this point of time.. > >> 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? >> > > It is not a matter or being better, it is just having HW vs. SW implementation. The point is Class 3 is not usable with non SR compliant power IC. In that case you should rely on a Class 2 implementation. > yep, I am aware of it.. but see my previous comment -> just thinking PMIC is not enough, there is a tradeoff involved for Class 2 operation - for continuous monitoring - hwmod etc might be a better option in that case.. but most of the folks do use PMICs and can benefit.. for proper values, you need NVALUES - dont forget that NOT all ICs have them burnt in.. and cant use SR anyways properly.. > >>> [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? >> > > We can define a struct with these methods. The struct will be implemented inside the PMIC code. The binding will be done in the board setup code. The SR driver will use the struct pointer to dereference the methods. > I like words, but prefer a patch instead ;).. This is not something I want to do over my lunch hour ;).. maybe we should take it up as a follow up patch? Regards, NM -- 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
