Re: [PATCHv4 02/15] omap3+: voltage: parameter segregation

[Tero Kristo <t-kristo@ti.com>]

On Wed, 2011-11-30 at 06:31 -0600, Menon, Nishanth wrote:
> On Wed, Nov 30, 2011 at 04:07, Tero Kristo <t-kristo@ti.com> wrote:
> > On Tue, 2011-11-29 at 12:26 -0600, Menon, Nishanth wrote:
> >> On Fri, Nov 25, 2011 at 09:49, Tero Kristo <t-kristo@ti.com> wrote:
> <more snip>
> >> > diff --git a/arch/arm/mach-omap2/opp3xxx_data.c b/arch/arm/mach-omap2/opp3xxx_data.c
> >> > index d95f3f9..1d44df5 100644
> >> > --- a/arch/arm/mach-omap2/opp3xxx_data.c
> >> > +++ b/arch/arm/mach-omap2/opp3xxx_data.c
> >> >
> >> >  /* 34xx */
> >> > +#define OMAP3_ON_VOLTAGE_UV            1200000
> >> > +#define OMAP3_ONLP_VOLTAGE_UV          1000000
> >> > +#define OMAP3_RET_VOLTAGE_UV           975000
> >> > +#define OMAP3_OFF_VOLTAGE_UV           600000
> >>
> >> this approach has a problem -> ON, ONLP and RET voltage should consider:
> Make that ON, ONLP, RET, OFF voltages! Sigh..
> 
> >> a) OMAP capabiltiy as above.
> >> b) PMIC capability which is being removed in this patch
> >>
> >> the framework should use the combination of both to make a decision.
> >
> > So, I think we should limit these voltages based on the PMIC vddmin /
> > vddmax values, right? I don't think PMIC has any other limitations, and
> Ideally speaking - this is what we want:
> PMIC says - on this rail - I can give max x V and min y V. and when
> you(kernel) gets control, expect Voltage = ON.
> 
> OMAP requirements - which ever wierd ones they might be - will be very
> specific to OMAP variants.
> some factors such as timing closures also come into play -> ON, ONLP,
> RET and OFF are OMAP reqs.
> 
> Now, we can have combinations like TPS+TWL+OMAP4430 (yep - those do
> exist) or something like custom PMIC+OMAP4460
> or many other combinations..
> 
> You could have additional complications as well - e.g. PMIC OFF path
> is definitely our favourite.
> writing 0x0 to TWL causes 0V and a 1 is 709..mV or 607..mV, 0x0 on TPS is 500mV!
> 
> > we shouldn't define the voltage levels for all of these modes for PMIC.
> > PMIC limits should also probably be changed from current values (based
> > on OMAP defines) and changed to actual values, like vddmin = 600mV
> > (vsel=0), or whatever is the minimum voltage for the corresponding PMIC.
> 
> my point being: framework cannot expect any assumption about the PMIC
> and the person writing the support for a new PMIC should be completely
> ignorant for which OMAP he/she is writing for.

Okay, so I will update the ON/ONLP/RET/OFF voltage level calculations to
use a PMIC specific function for the voltage level calculation, and use
a minimum value defined in vc_param, and select a voltage from PMIC
point that is at least this value, whatever weirdness would be added by
the PMIC. I can probably re-use something from the uv_to_vsel
implementations for example.

> 
> >> > +};
> >> > +
> >> >  #define OMAP4430_VDD_CORE_OPP50_UV             1025000
> >> >  #define OMAP4430_VDD_CORE_OPP100_UV            1200000
> >> >
> >> > @@ -64,6 +93,17 @@ struct omap_volt_data omap44xx_vdd_core_volt_data[] = {
> >> >        VOLT_DATA_DEFINE(0, 0, 0, 0),
> >> >  };
> >> >
> >> > +struct omap_vp_param omap44xx_core_vp_data = {
> >> > +       .vddmin                 = OMAP4_VP_CORE_VLIMITTO_VDDMIN,
> >> > +       .vddmax                 = OMAP4_VP_CORE_VLIMITTO_VDDMAX,
> >> > +};
> >> > +
> >> > +struct omap_vc_param omap44xx_core_vc_data = {
> >> > +       .on                     = OMAP4_ON_VOLTAGE_UV,
> >> > +       .onlp                   = OMAP4_ONLP_VOLTAGE_UV,
> >> > +       .ret                    = OMAP4_RET_VOLTAGE_UV,
> >> > +       .off                    = OMAP4_OFF_VOLTAGE_UV,
> >> > +};
> >>
> >> NOTE: we will be reaching all combinations ahead - in time to come
> >> ahead we will have 4470 as well - linking this to opp data seems wrong
> >> to me..
> >
> > They are not really that much linked to opp data, they are just defined
> > in this file. The actual attach to voltdm is done in
> > voltagedomainsxxxx_data.c file, where we can link data to whatever
> > voltagedomain we want to. See for example voltagedomains3xxx_data.c what
> > is done for omap34xx vs. omap36xx. Should we move this data over there
> > then...?
> I think it belongs to VP/VC data and not in OPP file (which was meant
> for OPPs in the first place).
> we might want to think about replacing these with device tree data
> someday - OPP data makes it
> kinda hard to distinguish IMHO when we do that.

Oh yea, vc_xxxxdata would be the most logical location for it now, I'll
move it there in the next version of the series.

-Tero

> 
> Regards,
> Nishanth Menon