ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF
Magnus Damm
maintainerPATCH 1/1 ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF
From: Magnus Damm <[email protected]>
Add MMCIF1, SDHI0 and SDHI2 to the clock workaround list for Lager multiplatform. Without these additional lines wakeup from Suspend-to-RAM never happens.
---
Written against renesas-devel-v3.14-rc7-20140318
arch/arm/mach-shmobile/board-lager-reference.c | 3 +++ 1 file changed, 3 insertions(+)
arch/arm/mach-shmobile/board-lager-reference.c +3 -0
@@ -114,7 +114,10 @@ static const struct clk_name clk_names[]static const struct clk_name clk_enables[]__initconst = { {"ether" ,NULL ,"ee700000.ethernet" }, {"msiof1" ,NULL ,"e6e10000.spi" }, + {"mmcif1" ,NULL ,"ee220000.mmc" }, {"qspi_mod" ,NULL ,"e6b10000.spi" }, + {"sdhi0" ,NULL ,"ee100000.sd" }, + {"sdhi2" ,NULL ,"ee140000.sd" }, {"thermal" ,NULL ,"e61f0000.thermal" }, };
Hi Magnus,
Quoted 5 lines · On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
Add MMCIF1, SDHI0 and SDHI2 to the clock workaround list for Lager multiplatform. Without these additional lines wakeup from Suspend-to-RAM never happens.
What about fixing the root cause instead of piling up hacks ?
Quoted 21 lines
Signed-off-by Magnus Damm <[email protected]>---
Written against renesas-devel-v3.14-rc7-20140318
arch/arm/mach-shmobile/board-lager-reference.c | 3 +++ 1 file changed, 3 insertions(+)arch/arm/mach-shmobile/board-lager-reference.c +0
-019:36:34.000000000 +0900 @@ -114,7 +114,10 @@ static const struct
clk_nameclk_names[]
static const structclk_nameclk_enables[]__initconst= {arch/arm/mach-shmobile/board-lager-reference.c +3
-0{ "ether", NULL, "ee700000.ethernet" }, { "msiof1", NULL, "e6e10000.spi" }, + { "mmcif1", NULL, "ee220000.mmc" }, { "qspi_mod", NULL, "e6b10000.spi" }, + { "sdhi0", NULL, "ee100000.sd" }, + { "sdhi2", NULL, "ee140000.sd" }, { "thermal", NULL, "e61f0000.thermal" }, };
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 10 lines · Laurent Pinchart
Hi Magnus,
Quoted 5 lines · On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
Add MMCIF1, SDHI0 and SDHI2 to the clock workaround list for Lager multiplatform. Without these additional lines wakeup from Suspend-to-RAM never happens.
What about fixing the root cause instead of piling up hacks ?
See RFC to move the drivers/sh/pm_runtime.c out of drivers/sh.
I need to address the issue that the code does not actually allow any devices to PM.
Signature · 2 lines
Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius
Hi Ben,
Quoted 11 lines · On Tuesday 18 March 2014 14:48:36 Ben Dooks wrote:
Quoted 8 lines · Laurent Pinchart
Quoted 5 lines · On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
Add MMCIF1, SDHI0 and SDHI2 to the clock workaround list for Lager multiplatform. Without these additional lines wakeup from Suspend-to-RAM never happens.
What about fixing the root cause instead of piling up hacks ?
See RFC to move the
drivers/sh/pm_runtime.cout ofdrivers/sh.
I wasn't sure whether the problem that this patch tries to work around was related to runtime PM as it wasn't mentioned in the commit message.
Quoted 2 lines
I need to address the issue that the code does not actually allow any devices to PM.
I've replied to your RFC and have studied the issue further since then.
First of all, I think we should get Rafael Wysocki and Felipe Balbi in the loop, as Rafael is probably the reference person for runtime PM, and Felipe has submitted an alternative proposal to manage device clocks automatically.
Then, just copying the drivers/sh/pm-runtime.c over to drivers/base/ doesn't seem to be a good idea to me. Core infrastructure code in drivers/base/ needs to be properly documented, and thus properly understood. I feel like we're rushing thing, taking code that seem to work but that isn't really understood, and turning it into core code.
I think we should take a step back here, understand exactly what we're doing, and write documentation.
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 18 lines · Laurent Pinchart
Hi Ben,
Quoted 11 lines · On Tuesday 18 March 2014 14:48:36 Ben Dooks wrote:
Quoted 8 lines · Laurent Pinchart
Quoted 5 lines · On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
Add MMCIF1, SDHI0 and SDHI2 to the clock workaround list for Lager multiplatform. Without these additional lines wakeup from Suspend-to-RAM never happens.
What about fixing the root cause instead of piling up hacks ?
See RFC to move the
drivers/sh/pm_runtime.cout ofdrivers/sh.I wasn't sure whether the problem that this patch tries to work around was related to runtime PM as it wasn't mentioned in the commit message.
Quoted 8 lines
Quoted 2 lines
I need to address the issue that the code does not actually allow any devices to PM.
I've replied to your RFC and have studied the issue further since then.
First of all, I think we should get Rafael Wysocki and Felipe Balbi in the loop, as Rafael is probably the reference person for runtime PM, and Felipe has submitted an alternative proposal to manage device clocks automatically.
I thought Felipes was pretty much what was already implemented in the drivers/base/power/clock_ops.c driver?
Quoted 5 lines
Then, just copying the
drivers/sh/pm-runtime.cover todrivers/base/doesn't seem to be a good idea to me. Core infrastructure code indrivers/base/needs to be properly documented, and thus properly understood. I feel like we're rushing thing, taking code that seem to work but that isn't really understood, and turning it into core code.
It does not need to be moved there, it would probably be better off having drivers/sh bein build and at-least fixing the two issues with the drivers/sh/pm_runtime.c
Quoted 2 lines
I think we should take a step back here, understand exactly what we're doing, and write documentation.
I would suggest that we build drivers/sh/runtime_pm.c for the multiplatform case and add initialsiation calls from the relevant arch setup files in arch/arm/mach-shmobile.
Once this is done we can then look at fixing the issues with making some generic code.
Signature · 2 lines
Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius
Hi Ben et al,
Quoted 22 lines · Ben Dooks
Quoted 21 lines · Laurent Pinchart
Quoted 11 lines · On Tuesday 18 March 2014 14:48:36 Ben Dooks wrote:
Quoted 8 lines · Laurent Pinchart
Quoted 5 lines · On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
Add MMCIF1, SDHI0 and SDHI2 to the clock workaround list for Lager multiplatform. Without these additional lines wakeup from Suspend-to-RAM never happens.
What about fixing the root cause instead of piling up hacks ?
See RFC to move the
drivers/sh/pm_runtime.cout ofdrivers/sh.I wasn't sure whether the problem that this patch tries to work around was related to runtime PM as it wasn't mentioned in the commit message.
Quoted 2 lines
I need to address the issue that the code does not actually allow any devices to PM.
I've replied to your RFC and have studied the issue further since then.
Quoted 8 lines
Quoted 5 lines
First of all, I think we should get Rafael Wysocki and Felipe Balbi in the loop, as Rafael is probably the reference person for runtime PM, and Felipe
has submitted an alternative proposal to manage device clocks automatically.I thought Felipes was pretty much what was already implemented in the
drivers/base/power/clock_ops.cdriver?
There are some similarities...
Quoted 17 lines
Quoted 8 lines
Then, just copying the
drivers/sh/pm-runtime.cover todrivers/base/doesn't
seem to be a good idea to me. Core infrastructure code indrivers/base/needs
to be properly documented, and thus properly understood. I feel like we're rushing thing, taking code that seem to work but that isn't really understood,
and turning it into core code.It does not need to be moved there, it would probably be better off having
drivers/shbein build and at-least fixing the two issues with thedrivers/sh/pm_runtime.cQuoted 3 lines
I think we should take a step back here, understand exactly what we're doing,
and write documentation.
Quoted 6 lines
I would suggest that we build
drivers/sh/runtime_pm.cfor the multiplatform case and add initialsiation calls from the relevant arch setup files inarch/arm/mach-shmobile.Once this is done we can then look at fixing the issues with making some generic code.
I agree with Laurent that we should fix the root cause, and that moving drivers/sh/runtime_pm.c to drivers/base without a full understanding of the ramifications is not a good idea.
I also agree with Ben that we can fix and enable drivers/sh/runtime_pm.c for the multi-platform case as a short-term solution, until we have a proper understanding, and have fixed the root cause in a way acceptable for all parties.
May I also emphasize that, while we may not understand drivers/sh/runtime_pm.c 100% yet, this code has been in active use for a while.
FWIW, disabling it in a -legacy kernel also leads to disabled clocks.
Now, back to the "understanding" part...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
Quoted 57 lines · On Tuesday 18 March 2014 16:27:19 Geert Uytterhoeven wrote:
Quoted 22 lines · Ben Dooks
Quoted 21 lines · Laurent Pinchart
Quoted 11 lines · On Tuesday 18 March 2014 14:48:36 Ben Dooks wrote:
Quoted 8 lines · Laurent Pinchart
Quoted 5 lines · On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
Add MMCIF1, SDHI0 and SDHI2 to the clock workaround list for Lager multiplatform. Without these additional lines wakeup from Suspend-to-RAM never happens.
What about fixing the root cause instead of piling up hacks ?
See RFC to move the
drivers/sh/pm_runtime.cout ofdrivers/sh.I wasn't sure whether the problem that this patch tries to work around was related to runtime PM as it wasn't mentioned in the commit message.
Quoted 2 lines
I need to address the issue that the code does not actually allow any devices to PM.
I've replied to your RFC and have studied the issue further since then.
Quoted 7 lines
Quoted 4 lines
First of all, I think we should get Rafael Wysocki and Felipe Balbi in the loop, as Rafael is probably the reference person for runtime PM, and Felipe has submitted an alternative proposal to manage device clocks automatically.
I thought Felipes was pretty much what was already implemented in the
drivers/base/power/clock_ops.cdriver?There are some similarities...
Quoted 13 lines
Quoted 5 lines
Then, just copying the
drivers/sh/pm-runtime.cover todrivers/base/doesn't seem to be a good idea to me. Core infrastructure code indrivers/base/needs to be properly documented, and thus properly understood. I feel like we're rushing thing, taking code that seem to work but that isn't really understood, and turning it into core code.It does not need to be moved there, it would probably be better off having
drivers/shbein build and at-least fixing the two issues with thedrivers/sh/pm_runtime.cQuoted 2 lines
I think we should take a step back here, understand exactly what we're doing, and write documentation.
Quoted 6 lines
I would suggest that we build
drivers/sh/runtime_pm.cfor the multiplatform case and add initialsiation calls from the relevant arch setup files inarch/arm/mach-shmobile.Once this is done we can then look at fixing the issues with making some generic code.
I agree with Laurent that we should fix the root cause, and that moving
drivers/sh/runtime_pm.ctodrivers/basewithout a full understanding of the ramifications is not a good idea.
Quoted 8 lines
I also agree with Ben that we can fix and enable
drivers/sh/runtime_pm.cfor the multi-platform case as a short-term solution, until we have a proper understanding, and have fixed the root cause in a way acceptable for all parties.May I also emphasize that, while we may not understand
drivers/sh/runtime_pm.c100% yet, this code has been in active use for a while. FWIW, disabling it in a -legacy kernel also leads to disabled clocks.
I agree with you, I think enabling drivers/sh/pm_runtime.c as a short term fix is a better workaround than keeping the clocks enabled at all time in platform code.
Quoted 1 line
Now, back to the "understanding" part...
I've had a quick look around and found that pm_clk_add_notifier() is used by three other platforms only, in arch/arm/mach-keystone/pm_domain.c, arch/arm/mach-omap1/pm_bus.c and arch/arm/mach-davinci/pm_domain.c. The three implementations are very similar, so there's room for code sharing.
On the other hand, four platforms using that infrastructure is not a lot, given that the use case should be quite common. There could be several reasons for that, from pm_clk_add_notifier() being too well hidden to a better solution used on other platforms. I believe this should be investigated to find out what the best solution to the problem at hand is.
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 10 lines · Laurent Pinchart
Hi Magnus,
Quoted 5 lines · On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
Add MMCIF1, SDHI0 and SDHI2 to the clock workaround list for Lager multiplatform. Without these additional lines wakeup from Suspend-to-RAM never happens.
What about fixing the root cause instead of piling up hacks ?
Hi Laurent,
I agree that hacks are piling up and that this is not entirely satisfactory. I also agree that we should fix the root cause. But in the meantime I think it is appropriate to apply work-arounds so that the code we have works and can be used.
Quoted 28 lines
Quoted 22 lines
Signed-off-by Magnus Damm <[email protected]>---
Written against renesas-devel-v3.14-rc7-20140318
arch/arm/mach-shmobile/board-lager-reference.c | 3 +++ 1 file changed, 3 insertions(+)arch/arm/mach-shmobile/board-lager-reference.c +0
-019:36:34.000000000 +0900 @@ -114,7 +114,10 @@ static const struct
clk_nameclk_names[]
static const structclk_nameclk_enables[]__initconst= {arch/arm/mach-shmobile/board-lager-reference.c +3
-0{ "ether", NULL, "ee700000.ethernet" }, { "msiof1", NULL, "e6e10000.spi" }, + { "mmcif1", NULL, "ee220000.mmc" }, { "qspi_mod", NULL, "e6b10000.spi" }, + { "sdhi0", NULL, "ee100000.sd" }, + { "sdhi2", NULL, "ee140000.sd" }, { "thermal", NULL, "e61f0000.thermal" }, };Signature · 3 lines
Regards, Laurent Pinchart
Quoted 7 lines · Magnus Damm
From: Magnus Damm <[email protected]>
Add MMCIF1, SDHI0 and SDHI2 to the clock workaround list for Lager multiplatform. Without these additional lines wakeup from Suspend-to-RAM never happens.
Signed-off-by Magnus Damm <[email protected]>
Thanks, I have queued this up under the assumption that a proper fix will be coming at some point.
Magnus Damm
maintainerHi Laurent,
Quoted 10 lines · Laurent Pinchart
Hi Magnus,
Quoted 5 lines · On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
Add MMCIF1, SDHI0 and SDHI2 to the clock workaround list for Lager multiplatform. Without these additional lines wakeup from Suspend-to-RAM never happens.
What about fixing the root cause instead of piling up hacks ?
-ENOPATCH? =)
Perhaps we need to learn what the root cause is before making a proper patch?
Thanks,
/ magnus
Hi Magnus,
Quoted 8 lines · On Wednesday 19 March 2014 10:03:32 Magnus Damm wrote:
Quoted 7 lines · Laurent Pinchart
Quoted 5 lines · On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
Add MMCIF1, SDHI0 and SDHI2 to the clock workaround list for Lager multiplatform. Without these additional lines wakeup from Suspend-to-RAM never happens.
Quoted 6 lines
Quoted 1 line
What about fixing the root cause instead of piling up hacks ?
-ENOPATCH? =)Perhaps we need to learn what the root cause is before making a proper patch?
Sure :-) Could you elaborate a bit on what issue the patch fixes ? Is S2R broken completely, only with multiplatform kernels, only when runtime PM is enabled ? The commit message isn't very explicit about that.
Ben mentioned that he was working on a workaround to use drivers/sh/pm_runtime.c as-is with multi-platform kernels. Assuming the S2R problem is limited to multi-platform kernels, I would prefer that workaround if that's fine with you as well.
Signature · 3 lines
Regards, Laurent Pinchart
Magnus Damm
maintainerHi Laurent,
Quoted 13 lines · Laurent Pinchart
Hi Magnus,
Quoted 10 lines · On Wednesday 19 March 2014 10:03:32 Magnus Damm wrote:
Quoted 8 lines · Laurent Pinchart
Quoted 5 lines · On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
Add MMCIF1, SDHI0 and SDHI2 to the clock workaround list for Lager multiplatform. Without these additional lines wakeup from Suspend-to-RAM never happens.
What about fixing the root cause instead of piling up hacks ?
Quoted 8 lines
Quoted 4 lines
-ENOPATCH? =)Perhaps we need to learn what the root cause is before making a proper patch?
Sure :-) Could you elaborate a bit on what issue the patch fixes ? Is S2R broken completely, only with multiplatform kernels, only when runtime PM is enabled ? The commit message isn't very explicit about that.
Suspend-to-RAM is working fine in the legacy case. However it is not for Multiplatform, more specifically shmobile_defconfig which includes Runtime PM disabled. I tracked it down to SDHI on Koelsch and Lager and also MMCIF on Lager.
Quoted 4 lines
Ben mentioned that he was working on a workaround to use
drivers/sh/pm_runtime.cas-is with multi-platform kernels. Assuming the S2R problem is limited to multi-platform kernels, I would prefer that workaround if that's fine with you as well.
Please discuss this with Geert. I'm fine with any kind of short term solution as long as it is working.
Thanks,
/ magnus