RE: [PATCH 3/3] clk: qcom: Set BRANCH_HALT_DELAY flags for venus core0/1 clks

[Sricharan]

Hi Stephen,

>>
>> On 11/05/2016 01:48 AM, 'Stephen Boyd' wrote:
>> > Well I'm also curious which case is failing. Does turning on the
>> > clocks work after the gdsc is enabled? Does turning off the
>> > clocks fail because we don't know when the gdsc has turned off? I
>> > would hope that the firmware keeps the gdsc on when it's done
>> > processing things, goes idle, and hands back control to software.
>> > Right now I'm failing to see how the halt bits fail to toggle
>> > assuming that firmware isn't misbehaving and the kernel driver is
>> > power controlling in a coordinated manner with the firmware.
>>
>> What fails is turning ON the clocks after the gdsc is put under
>> hardware control (by fails I mean the halt checks fail to indicate
>> the clock is running, but register accesses etc thereafter suggest
>> the clocks are actually running)
>> The halt checks seem to work only while the gdsc is put in SW enabled
>> state.
>>
>
>Um... that is bad. I don't see how that is possible. It sounds
>like the clocks are not turning on when we're asking for them to
>turn on. The register accesses are always working because these
>subcore clks aren't required for register accesses. Most likely
>the GDSC for the subdomains is off (even after we thought we
>enabled it).
>
>Let's take a step back. The video hardware has three GDSCs. One
>for the main logic, and two for each subdomain. We're adding hw
>control for the two subdomains here. From what I can tell there
>isn't any hw control for the main domain. I see that there are
>two registers in the video hardware to control those subdomain hw
>enable signals that go to the GDSC. The reset value is OFF (not
>ON like was stated earlier), so it seems that if we switch the
>subdomain GDSCs on in these patches it will turn on for a short
>time, and then turn off when we switch into hw mode (by virtue of
>the way we enable the GDSC). Presumably we can assert these hw
>signal bits regardless of the subdomain power states, because
>otherwise we're in a chicken-egg situation for the firmware
>controlling this stuff.
>
>The proper sequence sounds like it should be:
>
>	1. Enable GDSC for main domain
>	2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk)
>	3. Write the two registers to assert hw signal for subdomains
>	4. Enable GDSCs for two subdomains
>	5. Enable clocks for subdomains (video_subcore{0,1}_clk)
>
>I can only guess that we're not doing this. Probably the sequence
>right now is:
>
>	1. Enable GDSC for main domain and both sub-domains
>	2. Enable clocks for main domain (video_{core,maxi,ahb,axi}_clk)
>	3. Enable clocks for subdomains (video_subcore{0,1}_clk)
>	<clk stuff OFF because hw signal is still deasserted>
>
>Sound right? If so, please fix the sequence in the video driver.
>

So the above is the sequence which is actually carried out on the
firmware side. The same can be done in host as well.
The clocks stuck issue indeed is not there with this. But with the
above sequence we need to add a step to do inverse of STEP3
above (ie write the registers to de-assert hw_signal), to keep
the subdomains in off, till firmware uses it. So the above sequence
helps to avoid masking the halt check, although the host really
does not wants to use these clocks, except setting it up for the
firmware.

Regards,
 Sricharan