| Message ID | 5523E5A9.4080302@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Javier, On Tue, Apr 7, 2015 at 7:41 PM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > Hello Tomasz, > > On 04/07/2015 02:46 PM, Tomasz Figa wrote: >> 2015-04-07 13:56 GMT+02:00 Javier Martinez Canillas >> <javier.martinez@collabora.co.uk>: >>> 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 <javier.martinez@collabora.co.uk> > 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 <javier.martinez@collabora.co.uk> > --- > 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 <javier.martinez@collabora.co.uk> >> 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 <javier.martinez@collabora.co.uk> >> --- >> 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 <javier.martinez@collabora.co.uk> 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 <javier.martinez@collabora.co.uk> > 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 <javier.martinez@collabora.co.uk> Tested-by: Kevin Hilman <khilman@linaro.org> on exynos5800-peach-pi Kevin
2015-04-07 16:11 GMT+02:00 Javier Martinez Canillas <javier.martinez@collabora.co.uk>: > From 78aa551ebcb9a4a7ae9d5581c33e0c0f19fe5ad6 Mon Sep 17 00:00:00 2001 > From: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > 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 <javier.martinez@collabora.co.uk> > --- > 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 <javier.martinez@collabora.co.uk> 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 <javier.martinez@collabora.co.uk> >>> 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 <javier.martinez@collabora.co.uk> >>> --- >>> 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 <a.kesavan@samsung.com>. 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
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, }, };