On Tue, 2012-05-08 at 14:45 +0530, Rajendra Nayak wrote:
> On Tuesday 08 May 2012 02:39 PM, Tero Kristo wrote:
> > On Tue, 2012-05-08 at 14:27 +0530, Rajendra Nayak wrote:
> >> On Tuesday 08 May 2012 02:06 PM, Tero Kristo wrote:
> >>> On Mon, 2012-05-07 at 17:19 -0700, Kevin Hilman wrote:
> >>>> Tero Kristo<t-kristo@xxxxxx> writes:
> >>>>
> >>>>> From: Axel Haslam<axelhaslam@xxxxxxxxx>
> >>>>>
> >>>>> On OMAP4, there is no support to read previous logic state
> >>>>> or previous memory state achieved when a power domain transitions
> >>>>> to RET. Instead there are module level context registers.
> >>>>>
> >>>>> In order to support the powerdomain level logic/mem_off_counters
> >>>>> on OMAP4, instead use the previous power state achieved (RET) and
> >>>>> the *programmed* logic/mem RET state to derive if a powerdomain lost
> >>>>> logic or did not.
> >>>>
> >>>> OK, but this also changes the behavior for OMAP3 as well, right? I
> >>>> don't see in the changelog how this affects OMAP3 and whether it is
> >>>> still expected to work on OMAP3. When changing common code like this,
> >>>> the changelog should describe the impacts on to all affected SoCs.
> >>>>
> >>>> As suggested by Vaibhav Bedia, now might be the right time to add this
> >>>> function to the SoC specific function pointers (struct pwrdm_ops.)
> >>>>
> >>>> Doing that, the existing function could be used for OMAP3 (and OMAP4 if
> >>>> the changelog describes that it can/should be used for both.)
> >>>>
> >>>> Then, when AM33xx support is added, it will be obvious where they will
> >>>> need to plugin support for that SoC.
> >>>
> >>> How about the following patch? It will add a couple of redundant
> >>> read_prev_pwrst calls, but works in the same way as the original patch,
> >>> without touching the generic code. Also, as there have been talks about
> >>> adding caching for some of the pwrdm registers (especially the
> >>> prev_pwrst), this might not be that big of an issue.
> >>>
> >>> If this looks good to you, I'll re-post the set with this patch.
> >>>
> >>> -Tero
> >>>
> >>>
> >>> diff --git a/arch/arm/mach-omap2/powerdomain44xx.c
> >>> b/arch/arm/mach-omap2/powerdomain44xx.c
> >>> index 9bfb8a0..3d5e8d4 100644
> >>> --- a/arch/arm/mach-omap2/powerdomain44xx.c
> >>> +++ b/arch/arm/mach-omap2/powerdomain44xx.c
> >>> @@ -155,6 +155,14 @@ static int omap4_pwrdm_read_logic_retst(struct
> >>> powerdomain *pwrdm)
> >>> return v;
> >>> }
> >>>
> >>> +static int omap4_pwrdm_read_prev_logic_pwrst(struct powerdomain *pwrdm)
> >>> +{
> >>> + if (omap4_pwrdm_read_prev_pwrst(pwrdm) != PWRDM_POWER_RET)
> >>> + return PWRDM_POWER_ON;
> >>> +
> >>> + return omap4_pwrdm_read_logic_retst(pwrdm);
> >>
> >> Looks good to me. Do these ever get called with target state programmed
> >> to OFF?
> >
> > At least with current kernel code, no. But you are right, it might be
> > better to change these to check against> PWRDM_POWER_RET.
>
> But in case of target power state being programmed to OFF, the logic
> retst might not be programmed at all, so in case of target state being
> OFF, returning the logic retst programmed might as well return
> PWRDM_POWER_ON itself, right?
You are right, so better to check against OFF and return prev logic +
membank states as OFF also if that is the case, and with RET, return the
programmed state.
-Tero
>
> >
> > -Tero
> >
> >>
> >> regards,
> >> Rajendra
> >>
> >>> +}
> >>> +
> >>> static int omap4_pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8
> >>> bank)
> >>> {
> >>> u32 m, v;
> >>> @@ -183,6 +191,14 @@ static int omap4_pwrdm_read_mem_retst(struct
> >>> powerdomain *pwrdm, u8 bank)
> >>> return v;
> >>> }
> >>>
> >>> +static int omap4_pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm,
> >>> u8 bank)
> >>> +{
> >>> + if (omap4_pwrdm_read_prev_pwrst(pwrdm) != PWRDM_POWER_RET)
> >>> + return PWRDM_POWER_ON;
> >>> +
> >>> + return omap4_pwrdm_read_mem_retst(pwrdm, bank);
> >>> +}
> >>> +
> >>> static int omap4_pwrdm_wait_transition(struct powerdomain *pwrdm)
> >>> {
> >>> u32 c = 0;
> >>> @@ -256,9 +272,11 @@ struct pwrdm_ops omap4_pwrdm_operations = {
> >>> .pwrdm_clear_all_prev_pwrst = omap4_pwrdm_clear_all_prev_pwrst,
> >>> .pwrdm_set_logic_retst = omap4_pwrdm_set_logic_retst,
> >>> .pwrdm_read_logic_pwrst = omap4_pwrdm_read_logic_pwrst,
> >>> + .pwrdm_read_prev_logic_pwrst = omap4_pwrdm_read_prev_logic_pwrst,
> >>> .pwrdm_read_logic_retst = omap4_pwrdm_read_logic_retst,
> >>> .pwrdm_read_mem_pwrst = omap4_pwrdm_read_mem_pwrst,
> >>> .pwrdm_read_mem_retst = omap4_pwrdm_read_mem_retst,
> >>> + .pwrdm_read_prev_mem_pwrst = omap4_pwrdm_read_prev_mem_pwrst,
> >>> .pwrdm_set_mem_onst = omap4_pwrdm_set_mem_onst,
> >>> .pwrdm_set_mem_retst = omap4_pwrdm_set_mem_retst,
> >>> .pwrdm_wait_transition = omap4_pwrdm_wait_transition,
> >>>
> >>>
> >>
> >
> >
>
--
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
