From: javier.martinez@collabora•co.uk (Javier Martinez Canillas)
To: linux-arm-kernel@lists•infradead.org
Subject: [RFC PATCH v3 2/2] clk: exynos5420: Make sure MDMA0 clock is enabled during suspend
Date: Wed, 01 Apr 2015 13:44:10 +0200 [thread overview]
Message-ID: <551BDA0A.7010704@collabora.co.uk> (raw)
In-Reply-To: <551BD07E.2090506@samsung.com>
Hello Sylwester,
On 04/01/2015 01:03 PM, Sylwester Nawrocki wrote:
>
> On 31/03/15 22:00, Javier Martinez Canillas wrote:
>> On 03/31/2015 04:38 PM, Abhilash Kesavan wrote:
>>> javier.martinez at collabora.co.uk> wrote:
>
>>>> Unfortunately I don't fully understand why this clock needs to be
>>>> enabled. It would be good if someone at Samsung can explain in
>>>> more detail what the real problem really is.
>>>>
>>>
>>> I had a look at this some more today. The problem actually occurs when the
>>> mdma0 clock's parent - aclk266_g2d gets disabled. The run-time pm support
>>> in the dma driver disables mdma0 and in turn aclk266_g2d which causes the
>>> issue.
>>> From the User Manual, it appears that aclk266_g2d should be gated only when
>>> certain bits in the clock gating status register are 0. I cannot say for
>>> certain, but our gating the aclk266_g2d clock without the CG_STATUS bits
>>> being 0 could be a cause of the suspend failure.
>>>
>>
>> Thanks a lot for the explanation. I see the NOTE at the bottom of section
>> 7.9.1.159 CLK_GATE_BUS_TOP that mentions that. I'll add this information
>> to the commit message when posting as a proper patch instead of a RFC.
>>
>> I confirmed that changing the patch to prevent "aclk266_g2d" to be gated
>> instead of "mdm0" also makes the system to resume correctly from suspend
>> so I'll change that on the patch as well.
>>
>> I see that many of the Exynos5420 clocks (including "aclk266_g2d") use the
>> CLK_IGNORE_UNUSED flag but AFAIU it only prevents the common clock framework
>> to disable the clocks on init but doesn't prevent the clocks to be disabled
>> if all the clock childs are gated so the parent is gated as well.
>>
>>> As the CG_STATUS bits are not being checked anywhere in the kernel I think
>>> aclk266_g2d (and others in GATE_BUS_TOP) should not be gated. I am OK with
>>
>> For now I'll just add "aclk266_g2d" but later if needed all the GATE_BUS_TOP
>> clocks (and others) that should only be gated when CG_STATUS is 0 can be
>> added. My patch iterates over a list of clocks to be kept during suspend even
>> when there is only one for now so adding more later if needed will be trivial.
>
> It's not clear what subsystems affect state of the CG_STATUSx registers, it
> would be good if we could get more information on that. They are in the PMU
> block and are related to LPI (Low Power Interface handshaking), but what
> subsystems/peripheral blocks exactly are associated with them it's not clear
> from the documentation.
>
Yes, I've been looking at the docs again and found out a couple of things:
* Each GC_STATUSx register bit is associated with an IP hw block
* Some LPI_MASKx registers maps exactly with the GC_STATUSx (i.e: 0 and 1)
and others maps only partially (i.e: LPI_MASK2 and GC_STATUS2)
So it is related to LPI as you said and both LPI_MASKx and GC_STATUSx are
part of the PMU register address space.
In the particular case of aclk266_g2d, the doc says that the clock can only
be gated when CG_STATUS0[20] and CG_STATUS0[21] are 0. These are associated
with the SSS and SSS_SLIM respectively which AFAIU are crypto h/w modules.
> I think it's essential to understand what triggers changes in CG_STATUSx
> registers, before we start checking their value in the clock driver.
>
Indeed, we should really understand what the status on these registers
means. Also is not clear from the docs how much time should be waited,
how long until giving up, etc.
> Also it might be that there are indeed some clocks which must stay enabled
> over suspend/resume cycle, then the approach with enabling/disabling clocks
> in the clock driver might not be such a hack as it looks at first sight.
>
Having a clock driver to both a provider and consumer feels hacky to me as
well but I didn't find a better way to solve this issue... another option
is to have this workaround to solve the S2R issue while we figure out what
the the state in the CG_STATUSx really mean.
>> Or do you think that I should add all the GATE_BUS_TOP clocks now?
>
> No, please don't do that. That includes many important clocks and we should
> be certain what we are doing. I don't think it is expected to touch those
> clocks in that way, it would likely cause more issues.
>
>
Perfect, I just asked since it was not clear to me from Abhilash comment.
But I also agree to only focus on the clock that is causing issues now.
Best regards,
Javier
next prev parent reply other threads:[~2015-04-01 11:44 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-30 15:53 [RFC PATCH v3 0/2] ARM: EXYNOS: Fix Suspend-to-RAM on Exynos5420 Javier Martinez Canillas
2015-03-30 15:53 ` [RFC PATCH v3 1/2] clk: samsung: Add a clock lookup function Javier Martinez Canillas
2015-03-30 16:02 ` Tomasz Figa
2015-03-30 16:08 ` Javier Martinez Canillas
2015-03-31 1:40 ` Michael Turquette
2015-03-31 8:59 ` Javier Martinez Canillas
2015-04-01 1:29 ` Michael Turquette
2015-04-01 8:26 ` Javier Martinez Canillas
2015-03-30 15:53 ` [RFC PATCH v3 2/2] clk: exynos5420: Make sure MDMA0 clock is enabled during suspend Javier Martinez Canillas
2015-03-30 16:07 ` Tomasz Figa
2015-03-30 16:16 ` Javier Martinez Canillas
[not found] ` <CAM4voanL3A=dS8Z-ovi_-EDi9ctyaxZkvjajp+3ZjyNAnqR1aQ@mail.gmail.com>
2015-03-31 20:00 ` Javier Martinez Canillas
2015-04-01 11:03 ` Sylwester Nawrocki
2015-04-01 11:44 ` Javier Martinez Canillas [this message]
2015-04-01 17:31 ` Sylwester Nawrocki
2015-04-01 22:31 ` Javier Martinez Canillas
2015-04-02 12:22 ` Abhilash Kesavan
2015-04-07 10:59 ` Javier Martinez Canillas
2015-04-07 11:56 ` Javier Martinez Canillas
2015-04-07 12:46 ` Tomasz Figa
2015-04-07 14:11 ` Javier Martinez Canillas
2015-04-07 14:38 ` Abhilash Kesavan
2015-04-07 15:00 ` Javier Martinez Canillas
2015-04-08 1:50 ` Abhilash Kesavan
2015-04-07 18:51 ` Kevin Hilman
2015-04-07 21:28 ` Tomasz Figa
2015-04-08 5:36 ` Javier Martinez Canillas
2015-04-07 14:11 ` Abhilash Kesavan
2015-04-07 14:26 ` Javier Martinez Canillas
2015-03-31 21:02 ` Kevin Hilman
2015-04-01 3:19 ` Abhilash Kesavan
2015-04-01 4:03 ` Kevin Hilman
2015-04-01 9:16 ` Krzysztof Kozlowski
2015-04-01 19:02 ` Michael Turquette
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=551BDA0A.7010704@collabora.co.uk \
--to=javier.martinez@collabora$(echo .)co.uk \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox