Re: oprofile and ARM A9 hardware counter

[Jon Hunter <jon-hunter@ti.com>]

Hi Kevin,

On 05/08/2012 04:22 PM, Kevin Hilman wrote:
> Jon Hunter <jon-hunter@ti.com> writes:
> 
>> Hi Benoit,
>>
>> On 05/08/2012 06:01 AM, Cousson, Benoit wrote:
>>
>> [...]
>>
>>>>>> P.S. Please note there is also already a different fix in mainline for
>>>>>> the EMU clkdm data from Paul which adds the force wakeup flag and
>>>>>> removes the DISABLE_AUTO flag[1] (but leaves the ENABLE_AUTO flag,
>>>>>> because the hardware is capable.)
>>>>>
>>>>> Hmmm ... yes saw this, and you will have to excuse me as I don't fully
>>>>> follow the logic here. In fact, I am thinking we want the opposite ;-)
>>>>>
>>>>>  From looking, into this it seems to me that when PMU is running we want
>>>>> the EMU clock domain in software-wakeup state and when PMU is not
>>>>> running we want in the hardware auto state.
>>>>
>>>> So far, I'm with you.
>>>>
>>>>> By keeping the ENABLE_AUTO flag set, as soon as we enable the clock
>>>>> domain it is put right back into the HW_AUTO state
>>>>
>>>> This is only because it was in the HWSUP state when _enable was called.
>>>> If clkdm_deny_idle() is used, that behavior will change.
>>>>
>>>>> and hence PMU is
>>>>> not working (see _enable() function in
>>>>> arch/arm/mach-omap2/omap_hwmod.c)
>>>>>
>>>>> So really what I think we want is to remove the ENABLE_AUTO flag to keep
>>>>> the clock domain in software wake-up and use the DISABLE_AUTO flag to
>>>>> put the clock domain back in HW_AUTO (note this requires a patch to
>>>>> perform this 2nd part).
>>>>
>>>> Well, Paul will have to comment here for the final word, but IIUC, the
>>>> hwmod flags are supposed to indicate only what the HW is capable of.  If
>>>> we want to change the runtime behavior, we nee to use (or add) APIs to
>>>> change the beahvior.  In this case, clkdm_allow_idle(),
>>>> clkdm_deny_idle() are probably what is needed here.
>>>
>>> Yes, indeed, we should not hack the flags to fix that kind of issue. The
>>> flags describe what the HW is capable of, and the EMU CD can support
>>> HW_AUTO and SW_WAKEUP. AFAIK, the issue with that EMU CD is that the
>>> only valid next power state is OFF, meaning that no retention mode is
>>> supported. So any transition to idle will go to OFF and lead to a reset
>>> upon wakeup.
>>
>> No hacking intended here, just getting the flags correct ;-)
>>
>> So let me start from the beginning ...
>>
>> 1. I agree that for the EMU CD that the valid HW states are HW_AUTO and
>> SW_WKUP.
>>
>> 2. When the EMU CD is active (due to something like PMU), we want to
>> keep the CD in the SW_WKUP state, otherwise we can automatically
>> transition to idle and reset the IP (at least for omap4430).
> 
>> 3. When the EMU CD is inactive, we want to keep the CD in the HW_AUTO
>> state because SW_SLEEP is NOT supported.
>>
>> In the current code, we have the CLKDM_CAN_DISABLE_AUTO flag disabled
>> and the CLKDM_CAN_ENABLE_AUTO flag enabled. If CLKDM_CAN_ENABLE_AUTO is
>> set then the omap_pm_clkdms_setup() function will place the CD into
>> HW_AUTO regardless of CLKDM_CAN_DISABLE_AUTO, and the next time the
>> hwmod _enable() is called it is in the HW_AUTO state and so it is
>> allowed to idle. This is not what we want. Do you agree?
>>
>> If I set CLKDM_CAN_DISABLE_AUTO flag and disable CLKDM_CAN_ENABLE_AUTO,
>> then I do not have the above problem.
>>
>> To be honest, with you the more I look and test the code, the more
>> confused I am by the definition of the CLKDM_CAN_HWSUP ...
>>
>> #define CLKDM_CAN_HWSUP	(CLKDM_CAN_ENABLE_AUTO | CLKDM_CAN_DISABLE_AUTO)
>>
>> When I look at where these flags are used, I see that
>> CLKDM_CAN_ENABLE_AUTO is used in clkdm_allow_idle and
>> CLKDM_CAN_DISABLE_AUTO is used in clkdm_deny_idle. So this implies that ...
>>
>> CLKDM_CAN_ENABLE_AUTO = Supports HW_AUTO state when CD is active
>> CLKDM_CAN_DISABLE_AUTO = Does NOT supports HW_AUTO state when CD is active
>>
>> Are the above the correct definitions? 
> 
> Not quite. 
> 
> These flags describe the capabilities as defined in CLKTRCTRL field of
> the CLKSTCTRL register (e.g. CM_EMU_CLKSTCTRL)
> 
> CLKDM_CAN_ENABLE_AUTO: IP supports HW_AUTO state (and it can be enabled)
> CLKDM_CAN_DISABLE_AUTO: HW_AUTO feature can be disabled (a.k.a. NO_SLEEP)

Ok, so it that case it is wrong to set CAN_DISABLE_AUTO for the EMU CD.

> Note that in OMAP4, the latter called NO_SLEEP in the TRM, but in OMAP3
> it's described as "The automatic hardware-supervised mode is disabled"

Ah-ha. So this explains the naming! Thanks!

> What is confusing to me is that the OMAP4 TRM doesn't list the NO_SLEEP
> mode as supported by the EMU.   It seems to me that if the IP supports
> HW_AUTO, it should be able to be enabled *and* disabled. 
> 
> Maybe Paul/Benoit can clarify.

Yes, it definitely appears that EMU does not support NO_SLEEP according
to the TRM. However, even if it did, this would not help in this case as
we need to keep the CD in the SW_WKUP state while active. However, the
problem with this is it breaks the current software model that is used
to manage the CDs.

It is not a good solution, but for this domain it would appear that we
would need to have another flag, such as, "CAN_ENABLE_AUTO_ON_INACTIVE" :-(

Let me know what you think.

Cheers
Jon