SDHI and MMCIF with kzm9g-reference

SDHI and MMCIF with kzm9g-reference

[Simon Horman]

Hi Guennadi,

this is a heads-up about the state of SDHI and MMCIF with kzm9g-reference
in the current next and topic/all+next branches.

Currently the boot will not complete. It stalls in SDHI or MMCIF
initialisation (whichever comes first), though it doesn't say so.

The reason is msleep() calls. In the case of SDHI in the driver itself.
In the case of MMCIF in the MMC core code.

I had the following patch in next for a while but removed it a few days
ago because for some reason I thought my tests showed it was no longer needed.
I now believe it is needed and I will put it back into next and
topic/all+next ASAP. I do not plan to make any other changes to my branches
in what is left of today for me.


From: Simon Horman <horms+renesas@verge.net.au>
Date: Tue, 11 Dec 2012 10:20:21 +0900
Subject: [PATCH] ARM: clocksource: Initialise early

Initialise clock sources early, since some subsystems rely on them to
initialize.

This came up when working on DT support for the kzm9g board.
The prototype does not use early timers and I found that the
CMT clocksource needs to be initialised before the MMCIF MMC driver.

Cc: linux-kernel@vger.kernel.org
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/Makefile |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/Makefile b/drivers/Makefile
index 7863b9f..f156db5 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -37,6 +37,11 @@ obj-$(CONFIG_XEN)		+= xen/
 # regulators early, since some subsystems rely on them to initialize
 obj-$(CONFIG_REGULATOR)		+= regulator/
 
+# clocksource early, since some subsystems rely on them to initialize
+ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
+obj-y				+= clocksource/
+endif
+
 # tty/ comes before char/ so that the VT console is the boot-time
 # default.
 obj-y				+= tty/
@@ -114,9 +119,6 @@ obj-y				+= firmware/
 obj-$(CONFIG_CRYPTO)		+= crypto/
 obj-$(CONFIG_SUPERH)		+= sh/
 obj-$(CONFIG_ARCH_SHMOBILE)	+= sh/
-ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
-obj-y				+= clocksource/
-endif
 obj-$(CONFIG_DCA)		+= dca/
 obj-$(CONFIG_HID)		+= hid/
 obj-$(CONFIG_PPC_PS3)		+= ps3/
-- 
1.7.10.4

Re: SDHI and MMCIF with kzm9g-reference

[Guennadi Liakhovetski]

Hi Simon

On Fri, 1 Mar 2013, Simon Horman wrote:

> Hi Guennadi,
> 
> this is a heads-up about the state of SDHI and MMCIF with kzm9g-reference
> in the current next and topic/all+next branches.
> 
> Currently the boot will not complete. It stalls in SDHI or MMCIF
> initialisation (whichever comes first), though it doesn't say so.
> 
> The reason is msleep() calls. In the case of SDHI in the driver itself.
> In the case of MMCIF in the MMC core code.
> 
> I had the following patch in next for a while but removed it a few days
> ago because for some reason I thought my tests showed it was no longer needed.
> I now believe it is needed and I will put it back into next and
> topic/all+next ASAP. I do not plan to make any other changes to my branches
> in what is left of today for me.

Hmm, interesting. You've got mmcif and sdhi drivers compiled in, looks 
like, not as modules. You know, core driver people (gkh et al.) are pretty 
sensitive to the top-level Makefile changes like this. They affect 
everyone and violate the general principle - we shouldn't depend on the 
link order... My question to this though - you say kzm9g-reference is 
affected. Does it mean kzm9g with devices initialised from C code is ok? 
Maybe we could try to find what the difference is?

Thanks
Guennadi

> From: Simon Horman <horms+renesas@verge.net.au>
> Date: Tue, 11 Dec 2012 10:20:21 +0900
> Subject: [PATCH] ARM: clocksource: Initialise early
> 
> Initialise clock sources early, since some subsystems rely on them to
> initialize.
> 
> This came up when working on DT support for the kzm9g board.
> The prototype does not use early timers and I found that the
> CMT clocksource needs to be initialised before the MMCIF MMC driver.
> 
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
>  drivers/Makefile |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 7863b9f..f156db5 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -37,6 +37,11 @@ obj-$(CONFIG_XEN)		+= xen/
>  # regulators early, since some subsystems rely on them to initialize
>  obj-$(CONFIG_REGULATOR)		+= regulator/
>  
> +# clocksource early, since some subsystems rely on them to initialize
> +ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
> +obj-y				+= clocksource/
> +endif
> +
>  # tty/ comes before char/ so that the VT console is the boot-time
>  # default.
>  obj-y				+= tty/
> @@ -114,9 +119,6 @@ obj-y				+= firmware/
>  obj-$(CONFIG_CRYPTO)		+= crypto/
>  obj-$(CONFIG_SUPERH)		+= sh/
>  obj-$(CONFIG_ARCH_SHMOBILE)	+= sh/
> -ifndef CONFIG_ARCH_USES_GETTIMEOFFSET
> -obj-y				+= clocksource/
> -endif
>  obj-$(CONFIG_DCA)		+= dca/
>  obj-$(CONFIG_HID)		+= hid/
>  obj-$(CONFIG_PPC_PS3)		+= ps3/
> -- 
> 1.7.10.4
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: SDHI and MMCIF with kzm9g-reference

[Simon Horman]

[ Cc Magnus ]

Hi Guennadi,

On Fri, Mar 01, 2013 at 12:34:53PM +0100, Guennadi Liakhovetski wrote:
> Hi Simon
> 
> On Fri, 1 Mar 2013, Simon Horman wrote:
> 
> > Hi Guennadi,
> > 
> > this is a heads-up about the state of SDHI and MMCIF with kzm9g-reference
> > in the current next and topic/all+next branches.
> > 
> > Currently the boot will not complete. It stalls in SDHI or MMCIF
> > initialisation (whichever comes first), though it doesn't say so.
> > 
> > The reason is msleep() calls. In the case of SDHI in the driver itself.
> > In the case of MMCIF in the MMC core code.
> > 
> > I had the following patch in next for a while but removed it a few days
> > ago because for some reason I thought my tests showed it was no longer needed.
> > I now believe it is needed and I will put it back into next and
> > topic/all+next ASAP. I do not plan to make any other changes to my branches
> > in what is left of today for me.
> 
> Hmm, interesting. You've got mmcif and sdhi drivers compiled in, looks 
> like, not as modules.

Yes, compiled-in. No modules. As per the defconfig.

> You know, core driver people (gkh et al.) are pretty 
> sensitive to the top-level Makefile changes like this. They affect 
> everyone and violate the general principle - we shouldn't depend on the 
> link order...

I'm not very keen on the change. And if not depending on linking
order is a rule, then we have a problem. I'll leave the patch out
of my tree for now.

> My question to this though - you say kzm9g-reference is 
> affected. Does it mean kzm9g with devices initialised from C code is ok? 
> Maybe we could try to find what the difference is?

The difference is the way that clock sources and timers are initialised,
though I've never quite understood if its only one of them that
leads to the problem or not.

In the current scheme there are no early devices for kzm9g and no early
timers. If, for example using the following small patch, I switch things
around to use early device initialisation for the CMT clock source and
early timers, then probing MMCIF and SDHI works. If not then there is the
problem I describe above.

The reason that the code is like it is and not the way it would be
with the patch below (errors aside), is because Magnus specifically
asked for no early devices or early timers.

diff --git a/arch/arm/mach-shmobile/board-kzm9g-reference.c b/arch/arm/mach-shmobile/board-kzm9g-reference.c
index 63992a2..604906a 100644
--- a/arch/arm/mach-shmobile/board-kzm9g-reference.c
+++ b/arch/arm/mach-shmobile/board-kzm9g-reference.c
@@ -96,10 +96,10 @@ static const char *kzm9g_boards_compat_dt[] __initdata = {
 DT_MACHINE_START(KZM9G_DT, "kzm9g-reference")
 	.smp		= smp_ops(sh73a0_smp_ops),
 	.map_io		= sh73a0_map_io,
-	.init_early	= sh73a0_init_delay,
+	.init_early	= sh73a0_add_early_devices_dt,
 	.nr_irqs	= NR_IRQS_LEGACY,
 	.init_irq	= irqchip_init,
 	.init_machine	= kzm_init,
-	.init_time	= shmobile_timer_init,
+	.init_time	= sh73a0_earlytimer_init,
 	.dt_compat	= kzm9g_boards_compat_dt,
 MACHINE_END
diff --git a/arch/arm/mach-shmobile/include/mach/common.h b/arch/arm/mach-shmobile/include/mach/common.h
index c401c8d..34c717f 100644
--- a/arch/arm/mach-shmobile/include/mach/common.h
+++ b/arch/arm/mach-shmobile/include/mach/common.h
@@ -34,12 +34,12 @@ extern int sh7372_do_idle_sysc(unsigned long sleep_mode);
 extern struct clk sh7372_extal1_clk;
 extern struct clk sh7372_extal2_clk;
 
-extern void sh73a0_init_delay(void);
 extern void sh73a0_init_irq(void);
 extern void sh73a0_init_irq_dt(void);
 extern void sh73a0_map_io(void);
 extern void sh73a0_earlytimer_init(void);
 extern void sh73a0_add_early_devices(void);
+extern void sh73a0_add_early_devices_dt(void);
 extern void sh73a0_add_standard_devices(void);
 extern void sh73a0_add_standard_devices_dt(void);
 extern void sh73a0_clock_init(void);
diff --git a/arch/arm/mach-shmobile/setup-sh73a0.c b/arch/arm/mach-shmobile/setup-sh73a0.c
index 6259e07..96dc21b 100644
--- a/arch/arm/mach-shmobile/setup-sh73a0.c
+++ b/arch/arm/mach-shmobile/setup-sh73a0.c
@@ -913,6 +913,9 @@ static struct platform_device *sh73a0_devices_dt[] __initdata = {
 	&scif6_device,
 	&scif7_device,
 	&scif8_device,
+};
+
+static struct platform_device *sh73a0_early_devices_dt[] __initdata = {
 	&cmt10_device,
 };
 
@@ -945,6 +948,8 @@ void __init sh73a0_add_standard_devices(void)
 
 	platform_add_devices(sh73a0_devices_dt,
 			    ARRAY_SIZE(sh73a0_devices_dt));
+	platform_add_devices(sh73a0_early_devices_dt,
+			    ARRAY_SIZE(sh73a0_devices_dt));
 	platform_add_devices(sh73a0_early_devices,
 			    ARRAY_SIZE(sh73a0_early_devices));
 	platform_add_devices(sh73a0_late_devices,
@@ -965,6 +970,8 @@ void __init sh73a0_add_early_devices(void)
 {
 	early_platform_add_devices(sh73a0_devices_dt,
 				   ARRAY_SIZE(sh73a0_devices_dt));
+	early_platform_add_devices(sh73a0_early_devices_dt,
+				   ARRAY_SIZE(sh73a0_early_devices_dt));
 	early_platform_add_devices(sh73a0_early_devices,
 				   ARRAY_SIZE(sh73a0_early_devices));
 
@@ -974,9 +981,10 @@ void __init sh73a0_add_early_devices(void)
 
 #ifdef CONFIG_USE_OF
 
-void __init sh73a0_init_delay(void)
+void __init sh73a0_add_early_devices_dt(void)
 {
-	shmobile_setup_delay(1196, 44, 46); /* Cortex-A9 @ 1196MHz */
+	early_platform_add_devices(sh73a0_early_devices_dt,
+				   ARRAY_SIZE(sh73a0_early_devices_dt));
 }
 
 static const struct of_dev_auxdata sh73a0_auxdata_lookup[] __initconst = {
@@ -1002,11 +1010,11 @@ static const char *sh73a0_boards_compat_dt[] __initdata = {
 DT_MACHINE_START(SH73A0_DT, "Generic SH73A0 (Flattened Device Tree)")
 	.smp		= smp_ops(sh73a0_smp_ops),
 	.map_io		= sh73a0_map_io,
-	.init_early	= sh73a0_init_delay,
+	.init_early	= sh73a0_add_early_devices_dt,
 	.nr_irqs	= NR_IRQS_LEGACY,
 	.init_irq	= irqchip_init,
 	.init_machine	= sh73a0_add_standard_devices_dt,
-	.init_time	= shmobile_timer_init,
+	.init_time	= sh73a0_earlytimer_init,
 	.dt_compat	= sh73a0_boards_compat_dt,
 MACHINE_END
 #endif /* CONFIG_USE_OF */

Re: SDHI and MMCIF with kzm9g-reference

[Magnus Damm]

Hi Simon,

On Fri, Mar 1, 2013 at 9:28 PM, Simon Horman <horms@verge.net.au> wrote:
> [ Cc Magnus ]
>
> Hi Guennadi,
>
> On Fri, Mar 01, 2013 at 12:34:53PM +0100, Guennadi Liakhovetski wrote:
>> Hi Simon
>>
>> On Fri, 1 Mar 2013, Simon Horman wrote:
>>
>> > Hi Guennadi,
>> >
>> > this is a heads-up about the state of SDHI and MMCIF with kzm9g-reference
>> > in the current next and topic/all+next branches.
>> >
>> > Currently the boot will not complete. It stalls in SDHI or MMCIF
>> > initialisation (whichever comes first), though it doesn't say so.
>> >
>> > The reason is msleep() calls. In the case of SDHI in the driver itself.
>> > In the case of MMCIF in the MMC core code.
>> >
>> > I had the following patch in next for a while but removed it a few days
>> > ago because for some reason I thought my tests showed it was no longer needed.
>> > I now believe it is needed and I will put it back into next and
>> > topic/all+next ASAP. I do not plan to make any other changes to my branches
>> > in what is left of today for me.
>>
>> Hmm, interesting. You've got mmcif and sdhi drivers compiled in, looks
>> like, not as modules.
>
> Yes, compiled-in. No modules. As per the defconfig.
>
>> You know, core driver people (gkh et al.) are pretty
>> sensitive to the top-level Makefile changes like this. They affect
>> everyone and violate the general principle - we shouldn't depend on the
>> link order...
>
> I'm not very keen on the change. And if not depending on linking
> order is a rule, then we have a problem. I'll leave the patch out
> of my tree for now.
>
>> My question to this though - you say kzm9g-reference is
>> affected. Does it mean kzm9g with devices initialised from C code is ok?
>> Maybe we could try to find what the difference is?
>
> The difference is the way that clock sources and timers are initialised,
> though I've never quite understood if its only one of them that
> leads to the problem or not.
>
> In the current scheme there are no early devices for kzm9g and no early
> timers. If, for example using the following small patch, I switch things
> around to use early device initialisation for the CMT clock source and
> early timers, then probing MMCIF and SDHI works. If not then there is the
> problem I describe above.
>
> The reason that the code is like it is and not the way it would be
> with the patch below (errors aside), is because Magnus specifically
> asked for no early devices or early timers.

Thanks for making sure that we're not enabling early timers when it is
unclear if we really need to.

Would it be possible for you to check if the timer drivers (CMT, TMU)
get probe():d before the MMC drivers? If not then perhaps it is
possible to adjust the order they are being added as platform devices
or the initcall level for the actual drivers. Using early devices may
be just a too big hammer from my POV.

Thanks,

/ magnus

Re: SDHI and MMCIF with kzm9g-reference

[Simon Horman]

On Mon, Mar 04, 2013 at 09:55:56PM +0900, Magnus Damm wrote:
> Hi Simon,
> 
> On Fri, Mar 1, 2013 at 9:28 PM, Simon Horman <horms@verge.net.au> wrote:
> > [ Cc Magnus ]
> >
> > Hi Guennadi,
> >
> > On Fri, Mar 01, 2013 at 12:34:53PM +0100, Guennadi Liakhovetski wrote:
> >> Hi Simon
> >>
> >> On Fri, 1 Mar 2013, Simon Horman wrote:
> >>
> >> > Hi Guennadi,
> >> >
> >> > this is a heads-up about the state of SDHI and MMCIF with kzm9g-reference
> >> > in the current next and topic/all+next branches.
> >> >
> >> > Currently the boot will not complete. It stalls in SDHI or MMCIF
> >> > initialisation (whichever comes first), though it doesn't say so.
> >> >
> >> > The reason is msleep() calls. In the case of SDHI in the driver itself.
> >> > In the case of MMCIF in the MMC core code.
> >> >
> >> > I had the following patch in next for a while but removed it a few days
> >> > ago because for some reason I thought my tests showed it was no longer needed.
> >> > I now believe it is needed and I will put it back into next and
> >> > topic/all+next ASAP. I do not plan to make any other changes to my branches
> >> > in what is left of today for me.
> >>
> >> Hmm, interesting. You've got mmcif and sdhi drivers compiled in, looks
> >> like, not as modules.
> >
> > Yes, compiled-in. No modules. As per the defconfig.
> >
> >> You know, core driver people (gkh et al.) are pretty
> >> sensitive to the top-level Makefile changes like this. They affect
> >> everyone and violate the general principle - we shouldn't depend on the
> >> link order...
> >
> > I'm not very keen on the change. And if not depending on linking
> > order is a rule, then we have a problem. I'll leave the patch out
> > of my tree for now.
> >
> >> My question to this though - you say kzm9g-reference is
> >> affected. Does it mean kzm9g with devices initialised from C code is ok?
> >> Maybe we could try to find what the difference is?
> >
> > The difference is the way that clock sources and timers are initialised,
> > though I've never quite understood if its only one of them that
> > leads to the problem or not.
> >
> > In the current scheme there are no early devices for kzm9g and no early
> > timers. If, for example using the following small patch, I switch things
> > around to use early device initialisation for the CMT clock source and
> > early timers, then probing MMCIF and SDHI works. If not then there is the
> > problem I describe above.
> >
> > The reason that the code is like it is and not the way it would be
> > with the patch below (errors aside), is because Magnus specifically
> > asked for no early devices or early timers.
> 
> Thanks for making sure that we're not enabling early timers when it is
> unclear if we really need to.

Thanks for reviewing the situation.

> Would it be possible for you to check if the timer drivers (CMT, TMU)
> get probe():d before the MMC drivers?

My analysis is that the clock sources are probed after MMCIF and SDHI.

> If not then perhaps it is
> possible to adjust the order they are being added as platform devices
> or the initcall level for the actual drivers. Using early devices may
> be just a too big hammer from my POV.

Sorry for not posting an update with regards to this earlier.

I had some discussions with Paul about this earlier today and he
made the same suggestion. I plan to test this idea tomorrow and at
this stage I expect that test to be successful.

Re: SDHI and MMCIF with kzm9g-reference

[Guennadi Liakhovetski]

On Mon, 4 Mar 2013, Simon Horman wrote:

> On Mon, Mar 04, 2013 at 09:55:56PM +0900, Magnus Damm wrote:
> > Hi Simon,
> > 
> > On Fri, Mar 1, 2013 at 9:28 PM, Simon Horman <horms@verge.net.au> wrote:
> > > [ Cc Magnus ]
> > >
> > > Hi Guennadi,
> > >
> > > On Fri, Mar 01, 2013 at 12:34:53PM +0100, Guennadi Liakhovetski wrote:
> > >> Hi Simon
> > >>
> > >> On Fri, 1 Mar 2013, Simon Horman wrote:
> > >>
> > >> > Hi Guennadi,
> > >> >
> > >> > this is a heads-up about the state of SDHI and MMCIF with kzm9g-reference
> > >> > in the current next and topic/all+next branches.
> > >> >
> > >> > Currently the boot will not complete. It stalls in SDHI or MMCIF
> > >> > initialisation (whichever comes first), though it doesn't say so.
> > >> >
> > >> > The reason is msleep() calls. In the case of SDHI in the driver itself.
> > >> > In the case of MMCIF in the MMC core code.
> > >> >
> > >> > I had the following patch in next for a while but removed it a few days
> > >> > ago because for some reason I thought my tests showed it was no longer needed.
> > >> > I now believe it is needed and I will put it back into next and
> > >> > topic/all+next ASAP. I do not plan to make any other changes to my branches
> > >> > in what is left of today for me.
> > >>
> > >> Hmm, interesting. You've got mmcif and sdhi drivers compiled in, looks
> > >> like, not as modules.
> > >
> > > Yes, compiled-in. No modules. As per the defconfig.
> > >
> > >> You know, core driver people (gkh et al.) are pretty
> > >> sensitive to the top-level Makefile changes like this. They affect
> > >> everyone and violate the general principle - we shouldn't depend on the
> > >> link order...
> > >
> > > I'm not very keen on the change. And if not depending on linking
> > > order is a rule, then we have a problem. I'll leave the patch out
> > > of my tree for now.
> > >
> > >> My question to this though - you say kzm9g-reference is
> > >> affected. Does it mean kzm9g with devices initialised from C code is ok?
> > >> Maybe we could try to find what the difference is?
> > >
> > > The difference is the way that clock sources and timers are initialised,
> > > though I've never quite understood if its only one of them that
> > > leads to the problem or not.
> > >
> > > In the current scheme there are no early devices for kzm9g and no early
> > > timers. If, for example using the following small patch, I switch things
> > > around to use early device initialisation for the CMT clock source and
> > > early timers, then probing MMCIF and SDHI works. If not then there is the
> > > problem I describe above.
> > >
> > > The reason that the code is like it is and not the way it would be
> > > with the patch below (errors aside), is because Magnus specifically
> > > asked for no early devices or early timers.
> > 
> > Thanks for making sure that we're not enabling early timers when it is
> > unclear if we really need to.
> 
> Thanks for reviewing the situation.
> 
> > Would it be possible for you to check if the timer drivers (CMT, TMU)
> > get probe():d before the MMC drivers?
> 
> My analysis is that the clock sources are probed after MMCIF and SDHI.

Related to this: the problem with TWD on kzm9g with DT was, that CMT was 
initialised too late too. The TWD driver should be initialised before the 
secondary CPU is brought up. It is performing a calibration, and if CMT 
isn't initialised by then, the calibration never ends... So, my "local 
hack" to just enable TWD on kzm9g-reference to test CPUFreq interaction 
was to use early timers too.

Thanks
Guennadi

> > If not then perhaps it is
> > possible to adjust the order they are being added as platform devices
> > or the initcall level for the actual drivers. Using early devices may
> > be just a too big hammer from my POV.
> 
> Sorry for not posting an update with regards to this earlier.
> 
> I had some discussions with Paul about this earlier today and he
> made the same suggestion. I plan to test this idea tomorrow and at
> this stage I expect that test to be successful.

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: SDHI and MMCIF with kzm9g-reference

[Magnus Damm]

On Mon, Mar 4, 2013 at 10:40 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Mon, 4 Mar 2013, Simon Horman wrote:
>> On Mon, Mar 04, 2013 at 09:55:56PM +0900, Magnus Damm wrote:
>> > On Fri, Mar 1, 2013 at 9:28 PM, Simon Horman <horms@verge.net.au> wrote:
>> > > The reason that the code is like it is and not the way it would be
>> > > with the patch below (errors aside), is because Magnus specifically
>> > > asked for no early devices or early timers.
>> >
>> > Thanks for making sure that we're not enabling early timers when it is
>> > unclear if we really need to.
>>
>> Thanks for reviewing the situation.
>>
>> > Would it be possible for you to check if the timer drivers (CMT, TMU)
>> > get probe():d before the MMC drivers?
>>
>> My analysis is that the clock sources are probed after MMCIF and SDHI.
>
> Related to this: the problem with TWD on kzm9g with DT was, that CMT was
> initialised too late too. The TWD driver should be initialised before the
> secondary CPU is brought up. It is performing a calibration, and if CMT
> isn't initialised by then, the calibration never ends... So, my "local
> hack" to just enable TWD on kzm9g-reference to test CPUFreq interaction
> was to use early timers too.

Uhm, if shmobile_setup_delay() is used then the delay calculation will
be bypassed. Not sure which state the code was in when you tried
though.

So the shmobile_setup_delay() function should work for both boot and
secondary cpus. Keep in mind that configurations with both local
timers (TWD) and broad cast timer needs to work.

Thanks,

/ magnus

Re: SDHI and MMCIF with kzm9g-reference

[Simon Horman]

[ CC Paul ]

On Mon, Mar 04, 2013 at 10:00:39PM +0900, Simon Horman wrote:
> On Mon, Mar 04, 2013 at 09:55:56PM +0900, Magnus Damm wrote:
> > Hi Simon,
> > 
> > On Fri, Mar 1, 2013 at 9:28 PM, Simon Horman <horms@verge.net.au> wrote:
> > > [ Cc Magnus ]
> > >
> > > Hi Guennadi,
> > >
> > > On Fri, Mar 01, 2013 at 12:34:53PM +0100, Guennadi Liakhovetski wrote:
> > >> Hi Simon
> > >>
> > >> On Fri, 1 Mar 2013, Simon Horman wrote:
> > >>
> > >> > Hi Guennadi,
> > >> >
> > >> > this is a heads-up about the state of SDHI and MMCIF with kzm9g-reference
> > >> > in the current next and topic/all+next branches.
> > >> >
> > >> > Currently the boot will not complete. It stalls in SDHI or MMCIF
> > >> > initialisation (whichever comes first), though it doesn't say so.
> > >> >
> > >> > The reason is msleep() calls. In the case of SDHI in the driver itself.
> > >> > In the case of MMCIF in the MMC core code.
> > >> >
> > >> > I had the following patch in next for a while but removed it a few days
> > >> > ago because for some reason I thought my tests showed it was no longer needed.
> > >> > I now believe it is needed and I will put it back into next and
> > >> > topic/all+next ASAP. I do not plan to make any other changes to my branches
> > >> > in what is left of today for me.
> > >>
> > >> Hmm, interesting. You've got mmcif and sdhi drivers compiled in, looks
> > >> like, not as modules.
> > >
> > > Yes, compiled-in. No modules. As per the defconfig.
> > >
> > >> You know, core driver people (gkh et al.) are pretty
> > >> sensitive to the top-level Makefile changes like this. They affect
> > >> everyone and violate the general principle - we shouldn't depend on the
> > >> link order...
> > >
> > > I'm not very keen on the change. And if not depending on linking
> > > order is a rule, then we have a problem. I'll leave the patch out
> > > of my tree for now.
> > >
> > >> My question to this though - you say kzm9g-reference is
> > >> affected. Does it mean kzm9g with devices initialised from C code is ok?
> > >> Maybe we could try to find what the difference is?
> > >
> > > The difference is the way that clock sources and timers are initialised,
> > > though I've never quite understood if its only one of them that
> > > leads to the problem or not.
> > >
> > > In the current scheme there are no early devices for kzm9g and no early
> > > timers. If, for example using the following small patch, I switch things
> > > around to use early device initialisation for the CMT clock source and
> > > early timers, then probing MMCIF and SDHI works. If not then there is the
> > > problem I describe above.
> > >
> > > The reason that the code is like it is and not the way it would be
> > > with the patch below (errors aside), is because Magnus specifically
> > > asked for no early devices or early timers.
> > 
> > Thanks for making sure that we're not enabling early timers when it is
> > unclear if we really need to.
> 
> Thanks for reviewing the situation.
> 
> > Would it be possible for you to check if the timer drivers (CMT, TMU)
> > get probe():d before the MMC drivers?
> 
> My analysis is that the clock sources are probed after MMCIF and SDHI.
> 
> > If not then perhaps it is
> > possible to adjust the order they are being added as platform devices
> > or the initcall level for the actual drivers. Using early devices may
> > be just a too big hammer from my POV.
> 
> Sorry for not posting an update with regards to this earlier.
> 
> I had some discussions with Paul about this earlier today and he
> made the same suggestion. I plan to test this idea tomorrow and at
> this stage I expect that test to be successful.

The following seems to work:

From: Simon Horman <horms+renesas@verge.net.au>

clocksource: sh_cmt: Set initcall level to subsys

The reason for this is to ensure that CMT is probed earlier
than with its previous initcall level, module init.

This came up as a problem with using kzm9g-reference which does
not make use of early timers or devices. In that scenario initialisation
of SDHI and MMCIF both stall on msleep() calls due to the absence
of a initialised clock source.

Boot tested on: armadillo800eva, mackerel and kzm9g

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
index b72b724..08d0c41 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
@@ -838,7 +838,7 @@ static void __exit sh_cmt_exit(void)
 }
 
 early_platform_init("earlytimer", &sh_cmt_device_driver);
-module_init(sh_cmt_init);
+subsys_initcall(sh_cmt_init);
 module_exit(sh_cmt_exit);
 
 MODULE_AUTHOR("Magnus Damm");

Re: SDHI and MMCIF with kzm9g-reference

[Paul Mundt]

On Tue, Mar 05, 2013 at 03:56:52PM +0900, Simon Horman wrote:
> clocksource: sh_cmt: Set initcall level to subsys
> 
> The reason for this is to ensure that CMT is probed earlier
> than with its previous initcall level, module init.
> 
> This came up as a problem with using kzm9g-reference which does
> not make use of early timers or devices. In that scenario initialisation
> of SDHI and MMCIF both stall on msleep() calls due to the absence
> of a initialised clock source.
> 
> Boot tested on: armadillo800eva, mackerel and kzm9g
> 
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> 
Any change you make for CMT equally applies to TMU and MTU2, and should
be made at the same time. We do not want these to get out of sync for no
reason.

Note that mmc is also under subsys, so you are still link order dependent
there. If you wish to avoid that, it needs to be no later than
arch_initcall.

Re: SDHI and MMCIF with kzm9g-reference

[Simon Horman]

On Tue, Mar 05, 2013 at 04:11:39PM +0900, Paul Mundt wrote:
> On Tue, Mar 05, 2013 at 03:56:52PM +0900, Simon Horman wrote:
> > clocksource: sh_cmt: Set initcall level to subsys
> > 
> > The reason for this is to ensure that CMT is probed earlier
> > than with its previous initcall level, module init.
> > 
> > This came up as a problem with using kzm9g-reference which does
> > not make use of early timers or devices. In that scenario initialisation
> > of SDHI and MMCIF both stall on msleep() calls due to the absence
> > of a initialised clock source.
> > 
> > Boot tested on: armadillo800eva, mackerel and kzm9g
> > 
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > 
> Any change you make for CMT equally applies to TMU and MTU2, and should
> be made at the same time. We do not want these to get out of sync for no
> reason.

Thanks, Magnus also suggested that to me.
I'll send a small series to fix them, CMT and EM_STI.

> Note that mmc is also under subsys, so you are still link order dependent
> there. If you wish to avoid that, it needs to be no later than
> arch_initcall.

It seems that at this time it is the SDHI and MMCIF drivers that are
causing the trouble I described earlier in the thread, not MMC itself. I
have no particular objections to moving the clock sources to arch_initcall.
But subsys_initcall seems sufficient for now.

Re: SDHI and MMCIF with kzm9g-reference

[Guennadi Liakhovetski]

Hi Magnus

On Mon, 4 Mar 2013, Magnus Damm wrote:

> On Mon, Mar 4, 2013 at 10:40 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > On Mon, 4 Mar 2013, Simon Horman wrote:
> >> On Mon, Mar 04, 2013 at 09:55:56PM +0900, Magnus Damm wrote:
> >> > On Fri, Mar 1, 2013 at 9:28 PM, Simon Horman <horms@verge.net.au> wrote:
> >> > > The reason that the code is like it is and not the way it would be
> >> > > with the patch below (errors aside), is because Magnus specifically
> >> > > asked for no early devices or early timers.
> >> >
> >> > Thanks for making sure that we're not enabling early timers when it is
> >> > unclear if we really need to.
> >>
> >> Thanks for reviewing the situation.
> >>
> >> > Would it be possible for you to check if the timer drivers (CMT, TMU)
> >> > get probe():d before the MMC drivers?
> >>
> >> My analysis is that the clock sources are probed after MMCIF and SDHI.
> >
> > Related to this: the problem with TWD on kzm9g with DT was, that CMT was
> > initialised too late too. The TWD driver should be initialised before the
> > secondary CPU is brought up. It is performing a calibration, and if CMT
> > isn't initialised by then, the calibration never ends... So, my "local
> > hack" to just enable TWD on kzm9g-reference to test CPUFreq interaction
> > was to use early timers too.
> 
> Uhm, if shmobile_setup_delay() is used then the delay calculation will
> be bypassed. Not sure which state the code was in when you tried
> though.
> 
> So the shmobile_setup_delay() function should work for both boot and
> secondary cpus. Keep in mind that configurations with both local
> timers (TWD) and broad cast timer needs to work.

Let me reply to this with a pseudo patch-series:-) Based on topic/all+next 
of today (tip commit a7dba2b "Merge branch 'topic/intc-external-irq' into 
topic/all+next") with 3 my hack-patches to bring kzm9g-reference up with a 
network, a naive way of adding a TWD clock like

diff --git a/arch/arm/mach-shmobile/board-kzm9g-reference.c b/arch/arm/mach-shmobile/board-kzm9g-reference.c
index 81d44d8..8253493 100644
--- a/arch/arm/mach-shmobile/board-kzm9g-reference.c
+++ b/arch/arm/mach-shmobile/board-kzm9g-reference.c
@@ -132,6 +132,14 @@ static void __init kzm_init(void)
 #endif
 }
 
+void sh73a0_register_twd(void);
+
+static __init void kzm9g_init_time(void)
+{
+	shmobile_timer_init();
+	sh73a0_register_twd();
+}
+
 static const char *kzm9g_boards_compat_dt[] __initdata = {
 	"renesas,kzm9g-reference",
 	NULL,
@@ -144,6 +152,6 @@ DT_MACHINE_START(KZM9G_DT, "kzm9g-reference")
 	.nr_irqs	= NR_IRQS_LEGACY,
 	.init_irq	= sh73a0_init_irq_intc_dt,
 	.init_machine	= kzm_init,
-	.init_time	= shmobile_timer_init,
+	.init_time	= kzm9g_init_time,
 	.dt_compat	= kzm9g_boards_compat_dt,
 MACHINE_END

locks up with no console output. To actually boot and get the twd running 
I have to do

diff --git a/arch/arm/mach-shmobile/board-kzm9g-reference.c b/arch/arm/mach-shmobile/board-kzm9g-reference.c
index 81d44d8..3eaaab6 100644
--- a/arch/arm/mach-shmobile/board-kzm9g-reference.c
+++ b/arch/arm/mach-shmobile/board-kzm9g-reference.c
@@ -140,10 +140,10 @@ static const char *kzm9g_boards_compat_dt[] __initdata = {
 DT_MACHINE_START(KZM9G_DT, "kzm9g-reference")
 	.smp		= smp_ops(sh73a0_smp_ops),
 	.map_io		= sh73a0_map_io,
-	.init_early	= sh73a0_init_delay,
+	.init_early	= sh73a0_add_early_devices,
 	.nr_irqs	= NR_IRQS_LEGACY,
 	.init_irq	= sh73a0_init_irq_intc_dt,
 	.init_machine	= kzm_init,
-	.init_time	= shmobile_timer_init,
+	.init_time	= sh73a0_earlytimer_init,
 	.dt_compat	= kzm9g_boards_compat_dt,
 MACHINE_END

Once again: follow-up patches 1 and 2 are not for the mainline. Patch 3 
might actually make be good - provided we get a confirmation, that twd 
frequency on sh73a0 is indeed fixed Z clock / 4.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/