"Menon, Nishanth" <nm@xxxxxx> writes: > On Wed, Feb 23, 2011 at 14:29, Vishwanath Sripathy <vishwanath.bs@xxxxxx> wrote: >>> -----Original Message----- >>> From: Menon, Nishanth [mailto:nm@xxxxxx] >>> Sent: Wednesday, February 23, 2011 1:48 PM >>> To: Vishwanath Sripathy >>> Cc: linux-omap; Tony Lindgren; Kevin Hilman >>> Subject: Re: [PATCH 03/19] omap3+: voltage: remove initial voltage >>> >>> ÂWed, Feb 23, 2011 at 12:24, Vishwanath Sripathy >>> <vishwanath.bs@xxxxxx> wrote: >>> >> -----Original Message----- >>> >> From: Nishanth Menon [mailto:nm@xxxxxx] >>> >> Sent: Sunday, February 20, 2011 10:42 AM >>> >> To: Vishwanath Sripathy >>> >> Cc: linux-omap; Tony Lindgren; Kevin Hilman >>> >> Subject: Re: [PATCH 03/19] omap3+: voltage: remove initial voltage >>> >> >>> >> Vishwanath Sripathy wrote, on 02/19/2011 06:54 PM: >>> >> [..] >>> >> >> omap2_set_init_voltage should setup the curr_volt based on >>> which >>> >> OPP >>> >> >> the system is functioning at. Blindly setting a 1.2v setting in >> the >>> >> >> initial structure may not even match the default voltages stored >>> in >>> >> >> the voltage table which are supported for the domain. >>> >> >> >>> >> >> For example, OMAP3430 core domain does not use 1.2v and ends >>> up >>> >> >> generating a warning on the first transition. >>> >> >> >>> >> [..] >>> >> >> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach- >>> >> >> omap2/voltage.c >>> >> >> index 12be525..280ee12 100644 >>> >> >> --- a/arch/arm/mach-omap2/voltage.c >>> >> >> +++ b/arch/arm/mach-omap2/voltage.c >>> >> [..] >>> >> >> Â Â/* Generic voltage parameters */ >>> >> >> - Âvdd->curr_volt = 1200000; >>> >> > Where do you update this parameter upon initialization? Shouldn't >>> you >>> >> read >>> >> > the VP register and find the actual current voltage and update this >>> >> param? >>> >> >>> >> The sequence is as follows: >>> >> a) omapx_vdd_data configure is called as part of sr init sequence. >>> >> Â Â Â And the curr_volt with this patch is not updated at this stage. >>> >> b) somewhere down in the boot sequence, pm.c's >>> >> omap2_set_init_voltage >>> >> starts up. This looks up the current clk frequency from clock layer >> of >>> >> the parent device for the domain, picks up the nominal voltages >>> stored >>> >> in the opp layer, then does a omap_voltage_scale_vdd to that >>> voltage. In >>> >> omap_voltage_scale_vdd, The current voltage is merely picked off >>> the vp >>> >> (in _pre_volt_scale). the last step it does is to setup >> vdd->curr_volt. >>> >> This can then be used by dvfs layer etc to make appropriate >>> decisions. >>> >> >>> >> So, No, I dont think I need to update it here, it should happen as >>> part >>> >> of the pm init sequence. >>> >> >>> >> Could you explain what problem do you foresee by doing this? >>> > What if omap_voltage_scale_vdd fails when called from >>> > omap2_set_init_voltage? Or omap2_set_init_voltage returns error >>> even >>> > before calling omap_voltage_scale_vdd because it could not find the >>> > matching voltage for the frequency set by bootloader? >>> > >>> > Your assumption that curr_volt is always set by >>> omap2_set_init_voltage >>> > need not be true always. >>> >>> set_init_voltage's job is to set the initial voltage. if it is >>> incapable of doing it, I say fix that instead of hacking in some >>> random number as curr_volt. >> I never said to use random numbers. All I suggested was that instead of >> relying on some others function's behavior, read the VP register to fill >> in the right values. If set_init_voltage succeeds, it will anyway update >> with right voltage. > OK, lets take this argument for a moment. > Q: which voltage to set as curr_volt? > a) what if the update was done over vcbypass by bootloader? Cannot > trust the vp register for the value. > b) what if the voltage was updated over i2c1 to the PMIC by > bootloader? cant trust vp register again. > c) use some number like 1.2v - which we are aligned is wrong. > > in short, what should we do for a accurate curr_volt? vp register may > not be correct, instead, the right steps to do: > find my current freq, check the OPP table for the voltage needed, > setup the voltage for it, update curr_volt for it. this is trust > worthy, rest are not. > > This is what set_init_voltage does, replicating that logic in > voltage.c makes no sense again to me. Agreed. Please update the changelog with a summary of how curr_volt is properly updated so it's clear that you're not removing something that isn't updated elsewhere. Thanks, Kevin -- 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
