[PATCH] ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF

[PATCH] ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

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 <damm@opensource.se>
---

 Written against renesas-devel-v3.14-rc7-20140318

 arch/arm/mach-shmobile/board-lager-reference.c |    3 +++
 1 file changed, 3 insertions(+)

--- 0001/arch/arm/mach-shmobile/board-lager-reference.c
+++ work/arch/arm/mach-shmobile/board-lager-reference.c	2014-03-18 19:36:34.000000000 +0900
@@ -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" },
 };
 

Re: [PATCH] ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF

[Laurent Pinchart]

Hi Magnus,

On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> 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 ?

> Signed-off-by: Magnus Damm <damm@opensource.se>
> ---
> 
>  Written against renesas-devel-v3.14-rc7-20140318
> 
>  arch/arm/mach-shmobile/board-lager-reference.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> --- 0001/arch/arm/mach-shmobile/board-lager-reference.c
> +++ work/arch/arm/mach-shmobile/board-lager-reference.c	2014-03-18
> 19:36:34.000000000 +0900 @@ -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" },
>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF

[Ben Dooks]

On 18/03/14 14:25, Laurent Pinchart wrote:
> Hi Magnus,
>
> On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> 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.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF

[Laurent Pinchart]

Hi Ben,

On Tuesday 18 March 2014 14:48:36 Ben Dooks wrote:
> On 18/03/14 14:25, Laurent Pinchart wrote:
> > On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
> >> From: Magnus Damm <damm@opensource.se>
> >> 
> >> 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 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.

> 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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF

[Ben Dooks]

On 18/03/14 15:01, Laurent Pinchart wrote:
> Hi Ben,
>
> On Tuesday 18 March 2014 14:48:36 Ben Dooks wrote:
>> On 18/03/14 14:25, Laurent Pinchart wrote:
>>> On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
>>>> From: Magnus Damm <damm@opensource.se>
>>>>
>>>> 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 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.
>
>> 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?

> 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.

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

> 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.



-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF

[Geert Uytterhoeven]

Hi Ben et al,

On Tue, Mar 18, 2014 at 3:31 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> On 18/03/14 15:01, Laurent Pinchart wrote:
>> On Tuesday 18 March 2014 14:48:36 Ben Dooks wrote:
>>> On 18/03/14 14:25, Laurent Pinchart wrote:
>>>> On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
>>>>> From: Magnus Damm <damm@opensource.se>
>>>>>
>>>>> 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 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.
>>
>>> 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?

There are some similarities...

>> 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.
>
> 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
>
>> 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.

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 -- geert@linux-m68k.org

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

Re: [PATCH] ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF

[Laurent Pinchart]

Hi Geert,

On Tuesday 18 March 2014 16:27:19 Geert Uytterhoeven wrote:
> On Tue, Mar 18, 2014 at 3:31 PM, Ben Dooks wrote:
> > On 18/03/14 15:01, Laurent Pinchart wrote:
> >> On Tuesday 18 March 2014 14:48:36 Ben Dooks wrote:
> >>> On 18/03/14 14:25, Laurent Pinchart wrote:
> >>>> On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
> >>>>> From: Magnus Damm <damm@opensource.se>
> >>>>> 
> >>>>> 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 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.
> >> 
> >>> 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?
> 
> There are some similarities...
> 
> >> 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.
> > 
> > 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
> > 
> >> 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.
> 
> 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.

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.
 
> 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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF

[Simon Horman]

On Tue, Mar 18, 2014 at 02:25:53PM +0100, Laurent Pinchart wrote:
> Hi Magnus,
> 
> On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
> > From: Magnus Damm <damm@opensource.se>
> > 
> > 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.

> 
> > Signed-off-by: Magnus Damm <damm@opensource.se>
> > ---
> > 
> >  Written against renesas-devel-v3.14-rc7-20140318
> > 
> >  arch/arm/mach-shmobile/board-lager-reference.c |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > --- 0001/arch/arm/mach-shmobile/board-lager-reference.c
> > +++ work/arch/arm/mach-shmobile/board-lager-reference.c	2014-03-18
> > 19:36:34.000000000 +0900 @@ -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" },
> >  };
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH] ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF

[Simon Horman]

On Tue, Mar 18, 2014 at 09:52:47PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> 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 <damm@opensource.se>

Thanks, I have queued this up under the assumption that
a proper fix will be coming at some point.

Re: [PATCH] ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF

[Magnus Damm]

Hi Laurent,

On Tue, Mar 18, 2014 at 10:25 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> 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

Re: [PATCH] ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF

[Laurent Pinchart]

Hi Magnus,

On Wednesday 19 March 2014 10:03:32 Magnus Damm wrote:
> On Tue, Mar 18, 2014 at 10:25 PM, Laurent Pinchart wrote:
> > On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
> >> From: Magnus Damm <damm@opensource.se>
> >> 
> >> 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?

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF

[Magnus Damm]

Hi Laurent,

On Wed, Mar 19, 2014 at 7:01 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Wednesday 19 March 2014 10:03:32 Magnus Damm wrote:
>> On Tue, Mar 18, 2014 at 10:25 PM, Laurent Pinchart wrote:
>> > On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote:
>> >> From: Magnus Damm <damm@opensource.se>
>> >>
>> >> 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?
>
> 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.

> 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.

Please discuss this with Geert. I'm fine with any kind of short term
solution as long as it is working.

Thanks,

/ magnus