Hello,
On 31/03/15 22:00, Javier Martinez Canillas wrote:
> On 03/31/2015 04:38 PM, Abhilash Kesavan wrote:
>> [email protected]> 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.
I think it's essential to understand what triggers changes in CG_STATUSx
registers, before we start checking their value in the clock driver.
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.
> 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.
--
Thanks
Sylwester
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:
>>> [email protected]> 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
Hello Javier,
On 01/04/15 13:44, Javier Martinez Canillas wrote:
> 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:
>>>> [email protected]> wrote:
>>>> 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)
The CG_STATUSx and LPI_MASKx bits meaning is not matching according to
documentation I have. I guess you've got something newer than REV0.00?
> 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.
In my Exynos5420 UM ACLK_266_G2D is associated with CG_STATUS0 register
bits 22, 21, which in turn correspond to NR3D and DIS IP blocks, i.e.
the camera subsystem. Such a dependency would be rather surprising.
>> 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.
Exactly, I checked some kernels from http://opensource.samsung.com
(e.g. SM-N900_JB_Opensource.zip) for CG_STATUSx, but I didn't find anything
related to these registers yet, except the address macro definitions
and debug traces in the power domains driver.
>> 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.
Let's try to diagnose the issue best we can, then we would choose the most
accurate bug fix.
--
Regards,
Sylwester
Hello Sylwester,
On 04/01/2015 07:31 PM, Sylwester Nawrocki wrote:
> On 01/04/15 13:44, Javier Martinez Canillas wrote:
>> On 04/01/2015 01:03 PM, Sylwester Nawrocki wrote:
>>> 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)
>
> The CG_STATUSx and LPI_MASKx bits meaning is not matching according to
> documentation I have. I guess you've got something newer than REV0.00?
>
My Exynos5420 UM is Revision 1.00 dated February 2014 and GC_STATUS0 bits
maps LPI_MASK0 with the exception of bit 16 (NR3D) that is not mentioned
in GC_STATUS0, there is a hole between 15 (DIS) and 17 (FIMC_SCALERP).
GC_STATUS1 maps exactly with LPI_MASK1 and GC_STATUS2 and LPI_MASK2 have
the same bits from 0 to 5 and then differ from there.
>> 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.
>
> In my Exynos5420 UM ACLK_266_G2D is associated with CG_STATUS0 register
> bits 22, 21, which in turn correspond to NR3D and DIS IP blocks, i.e.
> the camera subsystem. Such a dependency would be rather surprising.
>
Sorry, it was a typo error and I actually meant CG_STATUS0 21 and 22 but
these correspond in the documentation I've to 21 (SSS) and 22 (SSS_SLIM).
As I mentioned before, DIS correspond to CG_STATUS0 15 and NR3D doesn't
have a corresponding bit in CG_STATUS0. But I don't know if that is an
error in the doc I've since is suspicious that is the only difference
between LPI_MASK0 and CG_STATUS0.
>>> 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.
>
> Exactly, I checked some kernels from http://opensource.samsung.com
> (e.g. SM-N900_JB_Opensource.zip) for CG_STATUSx, but I didn't find anything
> related to these registers yet, except the address macro definitions
> and debug traces in the power domains driver.
>
Yes, I also looked in the ChromiumOS v3.8 kernel but didn't find anything.
>>> 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.
>
> Let's try to diagnose the issue best we can, then we would choose the most
> accurate bug fix.
>
Sounds good to me.
Best regards,
Javier
Hi,
On Thu, Apr 2, 2015 at 4:01 AM, Javier Martinez Canillas
<[email protected]> wrote:
> Hello Sylwester,
>
> On 04/01/2015 07:31 PM, Sylwester Nawrocki wrote:
>> On 01/04/15 13:44, Javier Martinez Canillas wrote:
>>> On 04/01/2015 01:03 PM, Sylwester Nawrocki wrote:
>>>> 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)
>>
>> The CG_STATUSx and LPI_MASKx bits meaning is not matching according to
>> documentation I have. I guess you've got something newer than REV0.00?
>>
>
> My Exynos5420 UM is Revision 1.00 dated February 2014 and GC_STATUS0 bits
> maps LPI_MASK0 with the exception of bit 16 (NR3D) that is not mentioned
> in GC_STATUS0, there is a hole between 15 (DIS) and 17 (FIMC_SCALERP).
>
> GC_STATUS1 maps exactly with LPI_MASK1 and GC_STATUS2 and LPI_MASK2 have
> the same bits from 0 to 5 and then differ from there.
>
>>> 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.
>>
>> In my Exynos5420 UM ACLK_266_G2D is associated with CG_STATUS0 register
>> bits 22, 21, which in turn correspond to NR3D and DIS IP blocks, i.e.
>> the camera subsystem. Such a dependency would be rather surprising.
>>
>
> Sorry, it was a typo error and I actually meant CG_STATUS0 21 and 22 but
> these correspond in the documentation I've to 21 (SSS) and 22 (SSS_SLIM).
>
> As I mentioned before, DIS correspond to CG_STATUS0 15 and NR3D doesn't
> have a corresponding bit in CG_STATUS0. But I don't know if that is an
> error in the doc I've since is suspicious that is the only difference
> between LPI_MASK0 and CG_STATUS0.
>
>>>> 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.
>>
>> Exactly, I checked some kernels from http://opensource.samsung.com
>> (e.g. SM-N900_JB_Opensource.zip) for CG_STATUSx, but I didn't find anything
>> related to these registers yet, except the address macro definitions
>> and debug traces in the power domains driver.
>>
>
> Yes, I also looked in the ChromiumOS v3.8 kernel but didn't find anything.
>
>>>> 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.
>>
>> Let's try to diagnose the issue best we can, then we would choose the most
>> accurate bug fix.
>>
>
> Sounds good to me.
Based on the earlier comments I was trying to isolate if:
1) s2r fails because we gate aclk266_g2d (but it is one of those
clocks that needs to be always on prior to suspend).
2) s2r fails because we gate aclk266_g2d when CG_STATUS0[21:20] bits
are not 0 (thus not following the spec).
As I did not have access to the hardware guys who could possibly
confirm 1), I decided to
a) find a configuration where CG_STATUS0 allows gating of the
aclk266_g2d clock (i.e. CG_STATUS0[22:21] are 0).
b) disable the aclk266_g2d clock using such a configuration.
c) check s2r.
I found a configuration [1] which gave the following after boot-up:
# devmem 0x10040914
0xFD800014 (CG_STATUS0[22:21] is 0)
# devmem 0x10020700
0xC6F8DE9F (aclk266_g2d is enabled)
At this point s2r works.
I rebooted the board with the same config as above and then disabled
aclk266_g2d.
# devmem 0x10020700 32 0xC6F8DE9D
# devmem 0x10020700
0xC6F8DE9D (aclk266_g2d is disabled)
# devmem 0x10040914
0xFD800014
and tried s2r - It fails.
>From the results, disabling the clock seems to cause the issue rather
than the CG_STATUS violation. This is all a little confusing, so
please let me know if I have missed something.
Regards,
Abhilash
Hello Abhilash,
On 04/02/2015 02:22 PM, Abhilash Kesavan wrote:
> Hi,
>
> On Thu, Apr 2, 2015 at 4:01 AM, Javier Martinez Canillas
> <[email protected]> wrote:
>> Hello Sylwester,
>>
>> On 04/01/2015 07:31 PM, Sylwester Nawrocki wrote:
>>> On 01/04/15 13:44, Javier Martinez Canillas wrote:
>>>> On 04/01/2015 01:03 PM, Sylwester Nawrocki wrote:
>>>>> 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)
>>>
>>> The CG_STATUSx and LPI_MASKx bits meaning is not matching according to
>>> documentation I have. I guess you've got something newer than REV0.00?
>>>
>>
>> My Exynos5420 UM is Revision 1.00 dated February 2014 and GC_STATUS0 bits
>> maps LPI_MASK0 with the exception of bit 16 (NR3D) that is not mentioned
>> in GC_STATUS0, there is a hole between 15 (DIS) and 17 (FIMC_SCALERP).
>>
>> GC_STATUS1 maps exactly with LPI_MASK1 and GC_STATUS2 and LPI_MASK2 have
>> the same bits from 0 to 5 and then differ from there.
>>
>>>> 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.
>>>
>>> In my Exynos5420 UM ACLK_266_G2D is associated with CG_STATUS0 register
>>> bits 22, 21, which in turn correspond to NR3D and DIS IP blocks, i.e.
>>> the camera subsystem. Such a dependency would be rather surprising.
>>>
>>
>> Sorry, it was a typo error and I actually meant CG_STATUS0 21 and 22 but
>> these correspond in the documentation I've to 21 (SSS) and 22 (SSS_SLIM).
>>
>> As I mentioned before, DIS correspond to CG_STATUS0 15 and NR3D doesn't
>> have a corresponding bit in CG_STATUS0. But I don't know if that is an
>> error in the doc I've since is suspicious that is the only difference
>> between LPI_MASK0 and CG_STATUS0.
>>
>>>>> 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.
>>>
>>> Exactly, I checked some kernels from http://opensource.samsung.com
>>> (e.g. SM-N900_JB_Opensource.zip) for CG_STATUSx, but I didn't find anything
>>> related to these registers yet, except the address macro definitions
>>> and debug traces in the power domains driver.
>>>
>>
>> Yes, I also looked in the ChromiumOS v3.8 kernel but didn't find anything.
>>
>>>>> 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.
>>>
>>> Let's try to diagnose the issue best we can, then we would choose the most
>>> accurate bug fix.
>>>
>>
>> Sounds good to me.
>
> Based on the earlier comments I was trying to isolate if:
> 1) s2r fails because we gate aclk266_g2d (but it is one of those
> clocks that needs to be always on prior to suspend).
> 2) s2r fails because we gate aclk266_g2d when CG_STATUS0[21:20] bits
> are not 0 (thus not following the spec).
>
Thanks a lot for continue looking at this. I didn't have time to dig
deeper on this since last week.
> As I did not have access to the hardware guys who could possibly
> confirm 1), I decided to
> a) find a configuration where CG_STATUS0 allows gating of the
> aclk266_g2d clock (i.e. CG_STATUS0[22:21] are 0).
> b) disable the aclk266_g2d clock using such a configuration.
> c) check s2r.
>
> I found a configuration [1] which gave the following after boot-up:
I think you forgot the reference for [1] right? Since with latest
linux-next (20150402) I got:
> # devmem 0x10040914
> 0xFD800014 (CG_STATUS0[22:21] is 0)
# devmem 0x10040914
0xFFE00000 (CG_STATUS0[22:21] is 1)
> # devmem 0x10020700
> 0xC6F8DE9F (aclk266_g2d is enabled)
>
> At this point s2r works.
>
> I rebooted the board with the same config as above and then disabled
> aclk266_g2d.
>
> # devmem 0x10020700 32 0xC6F8DE9D
> # devmem 0x10020700
> 0xC6F8DE9D (aclk266_g2d is disabled)
> # devmem 0x10040914
> 0xFD800014
>
> and tried s2r - It fails.
>
> From the results, disabling the clock seems to cause the issue rather
> than the CG_STATUS violation. This is all a little confusing, so
> please let me know if I have missed something.
>
So IIUC the CG_STATUS0 bits were a red herring and the real problem
is that the aclk266_g2d needs to be enabled during suspend (although
we still don't know why).
It seems were are at a dead end now. Without being able to ask the HW
Samsung folks we would never know what's going on here...
I can re-post my patches to keep aclk266_g2d enabled during suspend
in the clk driver if that is the least bad option. But it would be
great to solve this issue otherwise S2R will remain to be broken for
Exynos5420 which will be really sad.
> Regards,
> Abhilash
>
Best regards,
Javier
On 04/07/2015 12:59 PM, Javier Martinez Canillas wrote:
>
> So IIUC the CG_STATUS0 bits were a red herring and the real problem
> is that the aclk266_g2d needs to be enabled during suspend (although
> we still don't know why).
>
> It seems were are at a dead end now. Without being able to ask the HW
> Samsung folks we would never know what's going on here...
>
Ok, I found another interesting data point. ACLK_266_G2D has as
constraints that CG_STATUS0[21:22] needs to be 0 before gating
the clock and as I mentioned before those are associated with
the SSS and SSS_SLIM HW crypto modules according the docs I've.
So I looked at the clock used by the SSS module and found that
CLK_SSS parent is ACLK_266_G2D but CLK_SSS is never requested
since drivers/crypto/s5p-sss.c isn't built for exynos_defconfig.
Enabling CONFIG_CRYPTO_DEV_S5P makes the system to resume without
any patches since the sss clock prevents aclk266_g2d to be gated.
But I wanted to know if it was really aclk266_g2d that was needed
or the actual sss clock since the kernel didn't manage that clock
without the driver enabled.
So I disabled the sss clock before trying a S2R:
# devmem 0x10018800 32 0xFFFFFFFB
(CLK_SSS in CLK_GATE_IP_G2D is gated)
and S2R worked anyways but I see that CLK_GATE_IP_G2D is reset to
its default value on S2R so maybe that is why it works anyways?
# devmem 0x10018800
0xFFFFFFFF (all CLK_GATE_IP_G2D clocks enabled including CLK_SSS)
Does this shed any more light? Could the problem be that the sss
clock parent (aclk266_g2d) is gated during S2R? Is the SSS module
required for S2R or is just that CLK_SSS prevents the parent to
be gated and so it is another red herring?
Best regards,
Javier
2015-04-07 13:56 GMT+02:00 Javier Martinez Canillas
<[email protected]>:
> So I disabled the sss clock before trying a S2R:
>
> # devmem 0x10018800 32 0xFFFFFFFB
> (CLK_SSS in CLK_GATE_IP_G2D is gated)
>
> and S2R worked anyways but I see that CLK_GATE_IP_G2D is reset to
> its default value on S2R so maybe that is why it works anyways?
Does the driver restore its value on resume (i.e. has it in the
save/restore array)? Remember that suspend causes all clock registers
to be reset. Then some of them will be configured by the lowest
bootloader stage after wake-up reset, but the kernel needs to restore
all of them.
>
> # devmem 0x10018800
> 0xFFFFFFFF (all CLK_GATE_IP_G2D clocks enabled including CLK_SSS)
>
> Does this shed any more light? Could the problem be that the sss
> clock parent (aclk266_g2d) is gated during S2R? Is the SSS module
> required for S2R or is just that CLK_SSS prevents the parent to
> be gated and so it is another red herring?
Does the board use secure firmware? If yes, it might require to do
some encryption on suspend, so if the firmware is broken and doesn't
control the clock itself, it might need the SSS clock to be running,
when the SLEEP SMC operation is called.
Anyway, I just realized that Exynos4 also need several clocks to be
ungated on suspend and this is handled by code [1] based on arrays
[2].
[1] http://lxr.free-electrons.com/source/drivers/clk/samsung/clk-exynos4.c#L309
[2] http://lxr.free-electrons.com/source/drivers/clk/samsung/clk-exynos4.c#L276
Could this method work for your case as well? There would be no need
to call any clock API at all, just low level register writes, which is
okay, since this is a low level driver anyway.
Best regards,
Tomasz
Hi Javier,
On Tue, Apr 7, 2015 at 4:29 PM, Javier Martinez Canillas
<[email protected]> wrote:
> Hello Abhilash,
>
> On 04/02/2015 02:22 PM, Abhilash Kesavan wrote:
>> Hi,
>>
>> On Thu, Apr 2, 2015 at 4:01 AM, Javier Martinez Canillas
>> <[email protected]> wrote:
>>> Hello Sylwester,
>>>
>>> On 04/01/2015 07:31 PM, Sylwester Nawrocki wrote:
>>>> On 01/04/15 13:44, Javier Martinez Canillas wrote:
>>>>> On 04/01/2015 01:03 PM, Sylwester Nawrocki wrote:
>>>>>> 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)
>>>>
>>>> The CG_STATUSx and LPI_MASKx bits meaning is not matching according to
>>>> documentation I have. I guess you've got something newer than REV0.00?
>>>>
>>>
>>> My Exynos5420 UM is Revision 1.00 dated February 2014 and GC_STATUS0 bits
>>> maps LPI_MASK0 with the exception of bit 16 (NR3D) that is not mentioned
>>> in GC_STATUS0, there is a hole between 15 (DIS) and 17 (FIMC_SCALERP).
>>>
>>> GC_STATUS1 maps exactly with LPI_MASK1 and GC_STATUS2 and LPI_MASK2 have
>>> the same bits from 0 to 5 and then differ from there.
>>>
>>>>> 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.
>>>>
>>>> In my Exynos5420 UM ACLK_266_G2D is associated with CG_STATUS0 register
>>>> bits 22, 21, which in turn correspond to NR3D and DIS IP blocks, i.e.
>>>> the camera subsystem. Such a dependency would be rather surprising.
>>>>
>>>
>>> Sorry, it was a typo error and I actually meant CG_STATUS0 21 and 22 but
>>> these correspond in the documentation I've to 21 (SSS) and 22 (SSS_SLIM).
>>>
>>> As I mentioned before, DIS correspond to CG_STATUS0 15 and NR3D doesn't
>>> have a corresponding bit in CG_STATUS0. But I don't know if that is an
>>> error in the doc I've since is suspicious that is the only difference
>>> between LPI_MASK0 and CG_STATUS0.
>>>
>>>>>> 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.
>>>>
>>>> Exactly, I checked some kernels from http://opensource.samsung.com
>>>> (e.g. SM-N900_JB_Opensource.zip) for CG_STATUSx, but I didn't find anything
>>>> related to these registers yet, except the address macro definitions
>>>> and debug traces in the power domains driver.
>>>>
>>>
>>> Yes, I also looked in the ChromiumOS v3.8 kernel but didn't find anything.
>>>
>>>>>> 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.
>>>>
>>>> Let's try to diagnose the issue best we can, then we would choose the most
>>>> accurate bug fix.
>>>>
>>>
>>> Sounds good to me.
>>
>> Based on the earlier comments I was trying to isolate if:
>> 1) s2r fails because we gate aclk266_g2d (but it is one of those
>> clocks that needs to be always on prior to suspend).
>> 2) s2r fails because we gate aclk266_g2d when CG_STATUS0[21:20] bits
>> are not 0 (thus not following the spec).
>>
>
> Thanks a lot for continue looking at this. I didn't have time to dig
> deeper on this since last week.
>
>> As I did not have access to the hardware guys who could possibly
>> confirm 1), I decided to
>> a) find a configuration where CG_STATUS0 allows gating of the
>> aclk266_g2d clock (i.e. CG_STATUS0[22:21] are 0).
>> b) disable the aclk266_g2d clock using such a configuration.
>> c) check s2r.
>>
>> I found a configuration [1] which gave the following after boot-up:
>
> I think you forgot the reference for [1] right? Since with latest
Yes, looks like I missed that. There are the changes I had:
diff --git a/arch/arm/boot/dts/exynos5420.dtsi
b/arch/arm/boot/dts/exynos5420.dtsi
index c0e98cf..3a9e21a 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -379,6 +379,7 @@
#dma-cells = <1>;
#dma-channels = <8>;
#dma-requests = <1>;
+ status = "disabled";
};
mdma1: mdma@11C10000 {
diff --git a/drivers/clk/samsung/clk-exynos5420.c
b/drivers/clk/samsung/clk-exynos5420.c
index 07d666c..38cb896 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -898,6 +898,7 @@ static struct samsung_gate_clock
exynos5x_gate_clks[] __initdata = {
GATE(CLK_G2D, "g2d", "aclk333_g2d", GATE_IP_G2D, 3, 0, 0),
GATE(CLK_SMMU_MDMA0, "smmu_mdma0", "aclk266_g2d", GATE_IP_G2D, 5, 0, 0),
GATE(CLK_SMMU_G2D, "smmu_g2d", "aclk333_g2d", GATE_IP_G2D, 7, 0, 0),
+ GATE(CLK_SLIMSSS, "slimsss", "aclk266_g2d", GATE_IP_G2D, 12, 0, 0),
GATE(0, "aclk200_fsys", "mout_user_aclk200_fsys",
GATE_BUS_FSYS0, 9, CLK_IGNORE_UNUSED, 0),
diff --git a/include/dt-bindings/clock/exynos5420.h
b/include/dt-bindings/clock/exynos5420.h
index 99da0d1..9459911 100644
--- a/include/dt-bindings/clock/exynos5420.h
+++ b/include/dt-bindings/clock/exynos5420.h
@@ -196,6 +196,7 @@
#define CLK_ACLK432_CAM 518
#define CLK_ACLK_FL1550_CAM 519
#define CLK_ACLK550_CAM 520
+#define CLK_SLIMSSS 521
/* mux clocks */
#define CLK_MOUT_HDMI 640
These changes gave me CG_STATUS0[22:21] as 0. I noticed that of the 2
CG_STATUS0 bits one did not turn 0 even when the mdma0 clock was kept
on. I decided to add the clock slimsss as it was the other clock
sourced from aclk266_g2d and noticed that the other bits turned 0.
> linux-next (20150402) I got:
>
>> # devmem 0x10040914
>> 0xFD800014 (CG_STATUS0[22:21] is 0)
>
> # devmem 0x10040914
> 0xFFE00000 (CG_STATUS0[22:21] is 1)
>
>> # devmem 0x10020700
>> 0xC6F8DE9F (aclk266_g2d is enabled)
>>
>> At this point s2r works.
>>
>> I rebooted the board with the same config as above and then disabled
>> aclk266_g2d.
>>
>> # devmem 0x10020700 32 0xC6F8DE9D
>> # devmem 0x10020700
>> 0xC6F8DE9D (aclk266_g2d is disabled)
>> # devmem 0x10040914
>> 0xFD800014
>>
>> and tried s2r - It fails.
>>
>> From the results, disabling the clock seems to cause the issue rather
>> than the CG_STATUS violation. This is all a little confusing, so
>> please let me know if I have missed something.
>>
>
> So IIUC the CG_STATUS0 bits were a red herring and the real problem
> is that the aclk266_g2d needs to be enabled during suspend (although
> we still don't know why).
>
> It seems were are at a dead end now. Without being able to ask the HW
> Samsung folks we would never know what's going on here...
Yes, though it increasingly looks like aclk266_g2d needs to stay ON
and we should use your patch that keeps it enabled prior to suspend.
Regards,
Abhilash
>
> I can re-post my patches to keep aclk266_g2d enabled during suspend
> in the clk driver if that is the least bad option. But it would be
> great to solve this issue otherwise S2R will remain to be broken for
> Exynos5420 which will be really sad.
>
>> Regards,
>> Abhilash
>>
>
> Best regards,
> Javier
Hello Tomasz,
On 04/07/2015 02:46 PM, Tomasz Figa wrote:
> 2015-04-07 13:56 GMT+02:00 Javier Martinez Canillas
> <[email protected]>:
>> So I disabled the sss clock before trying a S2R:
>>
>> # devmem 0x10018800 32 0xFFFFFFFB
>> (CLK_SSS in CLK_GATE_IP_G2D is gated)
>>
>> and S2R worked anyways but I see that CLK_GATE_IP_G2D is reset to
>> its default value on S2R so maybe that is why it works anyways?
>
> Does the driver restore its value on resume (i.e. has it in the
> save/restore array)? Remember that suspend causes all clock registers
> to be reset. Then some of them will be configured by the lowest
No, GATE_IP_G2D is not in the exynos5x_gate_clks array so it looses
the kernel after a suspend/resume cycle.
> bootloader stage after wake-up reset, but the kernel needs to restore
> all of them.
>
I see, thanks for the clarification. Then I think that is a bug and
GATE_IP_G2D needs to be added to the list of clocks to be saved and
restored right? That's a separate issue from our current S2R problem
though so it can be done later as a separate patch.
>>
>> # devmem 0x10018800
>> 0xFFFFFFFF (all CLK_GATE_IP_G2D clocks enabled including CLK_SSS)
>>
>> Does this shed any more light? Could the problem be that the sss
>> clock parent (aclk266_g2d) is gated during S2R? Is the SSS module
>> required for S2R or is just that CLK_SSS prevents the parent to
>> be gated and so it is another red herring?
>
> Does the board use secure firmware? If yes, it might require to do
> some encryption on suspend, so if the firmware is broken and doesn't
> control the clock itself, it might need the SSS clock to be running,
> when the SLEEP SMC operation is called.
>
No, the Chromebooks don't use secure firmware AFAIK.
> Anyway, I just realized that Exynos4 also need several clocks to be
> ungated on suspend and this is handled by code [1] based on arrays
> [2].
>
> [1] http://lxr.free-electrons.com/source/drivers/clk/samsung/clk-exynos4.c#L309
> [2] http://lxr.free-electrons.com/source/drivers/clk/samsung/clk-exynos4.c#L276
>
Yes I noticed that the Exynos5420 driver also does the same but did not
want to do it there because I didn't know what value should had been used
for all the other clocks in the CLK_GATE_BUS_TOP register. But if I get
your explanation right, it is safe to do so since the register is going to
be reset to its default values anyways.
> Could this method work for your case as well? There would be no need
> to call any clock API at all, just low level register writes, which is
> okay, since this is a low level driver anyway.
>
Yes, the following patch [0] is enough to make S2R working. If you think
that is correct then I'll post it as a proper patch then.
> Best regards,
> Tomasz
>
Best regards,
Javier
[0]
>From 78aa551ebcb9a4a7ae9d5581c33e0c0f19fe5ad6 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <[email protected]>
Date: Tue, 7 Apr 2015 15:53:27 +0200
Subject: [RFC PATCH] clk: exynos5420: Restore GATE_BUS_TOP on suspend
Commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power
Management support v12") added pm support for the pl330 dma driver but
it makes the clock for the Exynos5420 MDMA0 DMA controller to be gated
during suspend and this in turn makes its parent clock aclk266_g2d to
be gated. But the clock needs to be ungated prior suspend to allow the
system to be suspend and resumed correctly.
Add GATE_BUS_TOP register to the list of registers to be restored when
the system enters into a suspend state so aclk266_g2d will be ungated.
Thanks to Abhilash Kesavan for figuring out that this was the issue.
Fixes: ae43b32 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
drivers/clk/samsung/clk-exynos5420.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
index 07d666cc6a29..bea4a173eef5 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -271,6 +271,7 @@ static const struct samsung_clk_reg_dump exynos5420_set_clksrc[] = {
{ .offset = SRC_MASK_PERIC0, .value = 0x11111110, },
{ .offset = SRC_MASK_PERIC1, .value = 0x11111100, },
{ .offset = SRC_MASK_ISP, .value = 0x11111000, },
+ { .offset = GATE_BUS_TOP, .value = 0xffffffff, },
{ .offset = GATE_BUS_DISP1, .value = 0xffffffff, },
{ .offset = GATE_IP_PERIC, .value = 0xffffffff, },
};
--
2.1.4
Hello Abhilash,
On 04/07/2015 04:11 PM, Abhilash Kesavan wrote:
>
> Yes, though it increasingly looks like aclk266_g2d needs to stay ON
> and we should use your patch that keeps it enabled prior to suspend.
>
Indeed, could you please give me some feedback on the latest RFC patch
I shared [0] on this thread according to Tomasz comments?
> Regards,
> Abhilash
>>
Thanks a lot and best regards,
Javier
[0]: https://lkml.org/lkml/2015/4/7/537
Hi Javier,
On Tue, Apr 7, 2015 at 7:41 PM, Javier Martinez Canillas
<[email protected]> wrote:
> Hello Tomasz,
>
> On 04/07/2015 02:46 PM, Tomasz Figa wrote:
>> 2015-04-07 13:56 GMT+02:00 Javier Martinez Canillas
>> <[email protected]>:
>>> So I disabled the sss clock before trying a S2R:
>>>
>>> # devmem 0x10018800 32 0xFFFFFFFB
>>> (CLK_SSS in CLK_GATE_IP_G2D is gated)
>>>
>>> and S2R worked anyways but I see that CLK_GATE_IP_G2D is reset to
>>> its default value on S2R so maybe that is why it works anyways?
>>
>> Does the driver restore its value on resume (i.e. has it in the
>> save/restore array)? Remember that suspend causes all clock registers
>> to be reset. Then some of them will be configured by the lowest
>
> No, GATE_IP_G2D is not in the exynos5x_gate_clks array so it looses
> the kernel after a suspend/resume cycle.
>
>> bootloader stage after wake-up reset, but the kernel needs to restore
>> all of them.
>>
>
> I see, thanks for the clarification. Then I think that is a bug and
> GATE_IP_G2D needs to be added to the list of clocks to be saved and
> restored right? That's a separate issue from our current S2R problem
> though so it can be done later as a separate patch.
>
>>>
>>> # devmem 0x10018800
>>> 0xFFFFFFFF (all CLK_GATE_IP_G2D clocks enabled including CLK_SSS)
>>>
>>> Does this shed any more light? Could the problem be that the sss
>>> clock parent (aclk266_g2d) is gated during S2R? Is the SSS module
>>> required for S2R or is just that CLK_SSS prevents the parent to
>>> be gated and so it is another red herring?
>>
>> Does the board use secure firmware? If yes, it might require to do
>> some encryption on suspend, so if the firmware is broken and doesn't
>> control the clock itself, it might need the SSS clock to be running,
>> when the SLEEP SMC operation is called.
>>
>
> No, the Chromebooks don't use secure firmware AFAIK.
>
>> Anyway, I just realized that Exynos4 also need several clocks to be
>> ungated on suspend and this is handled by code [1] based on arrays
>> [2].
>>
>> [1] http://lxr.free-electrons.com/source/drivers/clk/samsung/clk-exynos4.c#L309
>> [2] http://lxr.free-electrons.com/source/drivers/clk/samsung/clk-exynos4.c#L276
>>
>
> Yes I noticed that the Exynos5420 driver also does the same but did not
> want to do it there because I didn't know what value should had been used
> for all the other clocks in the CLK_GATE_BUS_TOP register. But if I get
> your explanation right, it is safe to do so since the register is going to
> be reset to its default values anyways.
>
>> Could this method work for your case as well? There would be no need
>> to call any clock API at all, just low level register writes, which is
>> okay, since this is a low level driver anyway.
>>
>
> Yes, the following patch [0] is enough to make S2R working. If you think
> that is correct then I'll post it as a proper patch then.
>
>> Best regards,
>> Tomasz
>>
>
> Best regards,
> Javier
>
> [0]
> From 78aa551ebcb9a4a7ae9d5581c33e0c0f19fe5ad6 Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <[email protected]>
> Date: Tue, 7 Apr 2015 15:53:27 +0200
> Subject: [RFC PATCH] clk: exynos5420: Restore GATE_BUS_TOP on suspend
>
> Commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power
> Management support v12") added pm support for the pl330 dma driver but
> it makes the clock for the Exynos5420 MDMA0 DMA controller to be gated
> during suspend and this in turn makes its parent clock aclk266_g2d to
> be gated. But the clock needs to be ungated prior suspend to allow the
> system to be suspend and resumed correctly.
>
> Add GATE_BUS_TOP register to the list of registers to be restored when
> the system enters into a suspend state so aclk266_g2d will be ungated.
>
> Thanks to Abhilash Kesavan for figuring out that this was the issue.
>
> Fixes: ae43b32 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
> Signed-off-by: Javier Martinez Canillas <[email protected]>
> ---
> drivers/clk/samsung/clk-exynos5420.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> index 07d666cc6a29..bea4a173eef5 100644
> --- a/drivers/clk/samsung/clk-exynos5420.c
> +++ b/drivers/clk/samsung/clk-exynos5420.c
> @@ -271,6 +271,7 @@ static const struct samsung_clk_reg_dump exynos5420_set_clksrc[] = {
> { .offset = SRC_MASK_PERIC0, .value = 0x11111110, },
> { .offset = SRC_MASK_PERIC1, .value = 0x11111100, },
> { .offset = SRC_MASK_ISP, .value = 0x11111000, },
> + { .offset = GATE_BUS_TOP, .value = 0xffffffff, },
> { .offset = GATE_BUS_DISP1, .value = 0xffffffff, },
> { .offset = GATE_IP_PERIC, .value = 0xffffffff, },
> };
While there could be a concern that we are ungating all the clocks in
BUS_TOP, this looks like the least intrusive fix for the issue. Tested
this on Peach Pi, S2R works fine.
Thanks,
Abhilash
Hello Abhilash,
On 04/07/2015 04:38 PM, Abhilash Kesavan wrote:
>>
>> [0]
>> From 78aa551ebcb9a4a7ae9d5581c33e0c0f19fe5ad6 Mon Sep 17 00:00:00 2001
>> From: Javier Martinez Canillas <[email protected]>
>> Date: Tue, 7 Apr 2015 15:53:27 +0200
>> Subject: [RFC PATCH] clk: exynos5420: Restore GATE_BUS_TOP on suspend
>>
>> Commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power
>> Management support v12") added pm support for the pl330 dma driver but
>> it makes the clock for the Exynos5420 MDMA0 DMA controller to be gated
>> during suspend and this in turn makes its parent clock aclk266_g2d to
>> be gated. But the clock needs to be ungated prior suspend to allow the
>> system to be suspend and resumed correctly.
>>
>> Add GATE_BUS_TOP register to the list of registers to be restored when
>> the system enters into a suspend state so aclk266_g2d will be ungated.
>>
>> Thanks to Abhilash Kesavan for figuring out that this was the issue.
>>
>> Fixes: ae43b32 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>> ---
>> drivers/clk/samsung/clk-exynos5420.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
>> index 07d666cc6a29..bea4a173eef5 100644
>> --- a/drivers/clk/samsung/clk-exynos5420.c
>> +++ b/drivers/clk/samsung/clk-exynos5420.c
>> @@ -271,6 +271,7 @@ static const struct samsung_clk_reg_dump exynos5420_set_clksrc[] = {
>> { .offset = SRC_MASK_PERIC0, .value = 0x11111110, },
>> { .offset = SRC_MASK_PERIC1, .value = 0x11111100, },
>> { .offset = SRC_MASK_ISP, .value = 0x11111000, },
>> + { .offset = GATE_BUS_TOP, .value = 0xffffffff, },
>> { .offset = GATE_BUS_DISP1, .value = 0xffffffff, },
>> { .offset = GATE_IP_PERIC, .value = 0xffffffff, },
>> };
>
> While there could be a concern that we are ungating all the clocks in
Yes, I mentioned that but OTOH it will be even more dangerous to gate
clocks that should remain enabled so I used the register default values.
> BUS_TOP, this looks like the least intrusive fix for the issue. Tested
> this on Peach Pi, S2R works fine.
>
Thanks a lot for testing, does it mean I can add your Tested-by then when
posting it as a proper patch? I'll wait for Tomasz to comment before though.
> Thanks,
> Abhilash
>
Best regards,
Javier
Javier Martinez Canillas <[email protected]> writes:
[...]
> Yes, the following patch [0] is enough to make S2R working. If you think
> that is correct then I'll post it as a proper patch then.
[...]
> [0]
> From 78aa551ebcb9a4a7ae9d5581c33e0c0f19fe5ad6 Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <[email protected]>
> Date: Tue, 7 Apr 2015 15:53:27 +0200
> Subject: [RFC PATCH] clk: exynos5420: Restore GATE_BUS_TOP on suspend
>
> Commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power
> Management support v12") added pm support for the pl330 dma driver but
> it makes the clock for the Exynos5420 MDMA0 DMA controller to be gated
> during suspend and this in turn makes its parent clock aclk266_g2d to
> be gated. But the clock needs to be ungated prior suspend to allow the
> system to be suspend and resumed correctly.
>
> Add GATE_BUS_TOP register to the list of registers to be restored when
> the system enters into a suspend state so aclk266_g2d will be ungated.
>
> Thanks to Abhilash Kesavan for figuring out that this was the issue.
>
> Fixes: ae43b32 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
> Signed-off-by: Javier Martinez Canillas <[email protected]>
Tested-by: Kevin Hilman <[email protected]>
on exynos5800-peach-pi
Kevin
2015-04-07 16:11 GMT+02:00 Javier Martinez Canillas
<[email protected]>:
> From 78aa551ebcb9a4a7ae9d5581c33e0c0f19fe5ad6 Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <[email protected]>
> Date: Tue, 7 Apr 2015 15:53:27 +0200
> Subject: [RFC PATCH] clk: exynos5420: Restore GATE_BUS_TOP on suspend
>
> Commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power
> Management support v12") added pm support for the pl330 dma driver but
> it makes the clock for the Exynos5420 MDMA0 DMA controller to be gated
> during suspend and this in turn makes its parent clock aclk266_g2d to
> be gated. But the clock needs to be ungated prior suspend to allow the
> system to be suspend and resumed correctly.
>
> Add GATE_BUS_TOP register to the list of registers to be restored when
> the system enters into a suspend state so aclk266_g2d will be ungated.
>
> Thanks to Abhilash Kesavan for figuring out that this was the issue.
>
> Fixes: ae43b32 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
> Signed-off-by: Javier Martinez Canillas <[email protected]>
> ---
> drivers/clk/samsung/clk-exynos5420.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> index 07d666cc6a29..bea4a173eef5 100644
> --- a/drivers/clk/samsung/clk-exynos5420.c
> +++ b/drivers/clk/samsung/clk-exynos5420.c
> @@ -271,6 +271,7 @@ static const struct samsung_clk_reg_dump exynos5420_set_clksrc[] = {
> { .offset = SRC_MASK_PERIC0, .value = 0x11111110, },
> { .offset = SRC_MASK_PERIC1, .value = 0x11111100, },
> { .offset = SRC_MASK_ISP, .value = 0x11111000, },
> + { .offset = GATE_BUS_TOP, .value = 0xffffffff, },
> { .offset = GATE_BUS_DISP1, .value = 0xffffffff, },
> { .offset = GATE_IP_PERIC, .value = 0xffffffff, },
> };
> --
> 2.1.4
Looks good to me. You can consider this Acked-by, as long as Sylwester
is not opposed to this approach.
Best regards,
Tomasz
Hi Javier,
On Tue, Apr 7, 2015 at 8:30 PM, Javier Martinez Canillas
<[email protected]> wrote:
> Hello Abhilash,
>
> On 04/07/2015 04:38 PM, Abhilash Kesavan wrote:
>>>
>>> [0]
>>> From 78aa551ebcb9a4a7ae9d5581c33e0c0f19fe5ad6 Mon Sep 17 00:00:00 2001
>>> From: Javier Martinez Canillas <[email protected]>
>>> Date: Tue, 7 Apr 2015 15:53:27 +0200
>>> Subject: [RFC PATCH] clk: exynos5420: Restore GATE_BUS_TOP on suspend
>>>
>>> Commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power
>>> Management support v12") added pm support for the pl330 dma driver but
>>> it makes the clock for the Exynos5420 MDMA0 DMA controller to be gated
>>> during suspend and this in turn makes its parent clock aclk266_g2d to
>>> be gated. But the clock needs to be ungated prior suspend to allow the
>>> system to be suspend and resumed correctly.
>>>
>>> Add GATE_BUS_TOP register to the list of registers to be restored when
>>> the system enters into a suspend state so aclk266_g2d will be ungated.
>>>
>>> Thanks to Abhilash Kesavan for figuring out that this was the issue.
>>>
>>> Fixes: ae43b32 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
>>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>>> ---
>>> drivers/clk/samsung/clk-exynos5420.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
>>> index 07d666cc6a29..bea4a173eef5 100644
>>> --- a/drivers/clk/samsung/clk-exynos5420.c
>>> +++ b/drivers/clk/samsung/clk-exynos5420.c
>>> @@ -271,6 +271,7 @@ static const struct samsung_clk_reg_dump exynos5420_set_clksrc[] = {
>>> { .offset = SRC_MASK_PERIC0, .value = 0x11111110, },
>>> { .offset = SRC_MASK_PERIC1, .value = 0x11111100, },
>>> { .offset = SRC_MASK_ISP, .value = 0x11111000, },
>>> + { .offset = GATE_BUS_TOP, .value = 0xffffffff, },
>>> { .offset = GATE_BUS_DISP1, .value = 0xffffffff, },
>>> { .offset = GATE_IP_PERIC, .value = 0xffffffff, },
>>> };
>>
>> While there could be a concern that we are ungating all the clocks in
>
> Yes, I mentioned that but OTOH it will be even more dangerous to gate
> clocks that should remain enabled so I used the register default values.
>
>> BUS_TOP, this looks like the least intrusive fix for the issue. Tested
>> this on Peach Pi, S2R works fine.
>>
>
> Thanks a lot for testing, does it mean I can add your Tested-by then when
> posting it as a proper patch? I'll wait for Tomasz to comment before though.
Tested-by: Abhilash Kesavan <[email protected]>.
Abhilash
Hello Tomasz,
On 04/07/2015 11:28 PM, Tomasz Figa wrote:
>
> Looks good to me. You can consider this Acked-by, as long as Sylwester
> is not opposed to this approach.
>
Thanks a lot, I've posted it as a proper patch now.
> Best regards,
> Tomasz
>
Best regards,
Javier