[PATCH 5/5] arm: msm: smd: fix SMD modem processor sync condition

[PATCH 5/5] arm: msm: smd: fix SMD modem processor sync condition

[Daniel Walker]

From: Daniel Walker <c_dwalke@quicinc.com>

When booting up we need to wait for the modem processor to
partially boot. This is because the modem processor does
resource allocation for us. If we don't wait the modem won't
honor our requests and we end up crashing or in an unknown
state. This change just formalizes the waiting process.

Signed-off-by: Daniel Walker <c_dwalke@quicinc.com>
---
 arch/arm/mach-msm/proc_comm.c |   15 ++++++++++++++-
 arch/arm/mach-msm/proc_comm.h |    3 +++
 arch/arm/mach-msm/smd.c       |    7 +++++++
 3 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-msm/proc_comm.c b/arch/arm/mach-msm/proc_comm.c
index 915ee70..f001281 100644
--- a/arch/arm/mach-msm/proc_comm.c
+++ b/arch/arm/mach-msm/proc_comm.c
@@ -107,4 +107,17 @@ int msm_proc_comm(unsigned cmd, unsigned *data1, unsigned *data2)
 	return ret;
 }
 
-
+/*
+ * We need to wait for the ARM9 to at least partially boot
+ * up before we can continue. Since the ARM9 does resource
+ * allocation, if we dont' wait we could end up crashing or in
+ * and unknown state. This function should be called early to
+ * wait on the ARM9.
+ */
+void __init proc_comm_boot_wait(void)
+{
+	void __iomem *base = MSM_SHARED_RAM_BASE;
+ 
+	proc_comm_wait_for(base + MDM_STATUS, PCOM_READY);
+ 
+}
diff --git a/arch/arm/mach-msm/proc_comm.h b/arch/arm/mach-msm/proc_comm.h
index 0f5cdd3..12da4ca 100644
--- a/arch/arm/mach-msm/proc_comm.h
+++ b/arch/arm/mach-msm/proc_comm.h
@@ -16,6 +16,8 @@
 #ifndef _ARCH_ARM_MACH_MSM_PROC_COMM_H_
 #define _ARCH_ARM_MACH_MSM_PROC_COMM_H_
 
+#include <linux/init.h>
+
 enum {
 	PCOM_CMD_IDLE = 0x0,
 	PCOM_CMD_DONE,
@@ -251,5 +253,6 @@ enum {
 		(((drvstr) & 0xF) << 17))
 
 int msm_proc_comm(unsigned cmd, unsigned *data1, unsigned *data2);
+void __init proc_comm_boot_wait(void);
 
 #endif
diff --git a/arch/arm/mach-msm/smd.c b/arch/arm/mach-msm/smd.c
index 2fa567e..4fec6df 100644
--- a/arch/arm/mach-msm/smd.c
+++ b/arch/arm/mach-msm/smd.c
@@ -1006,6 +1006,13 @@ static int __init msm_smd_probe(struct platform_device *pdev)
 {
 	pr_info("smd_init()\n");
 
+	/*
+	 * If we haven't waited for the ARM9 to boot up till now,
+	 * then we need to wait here. Otherwise this should just
+	 * return immediately.
+	 */
+	proc_comm_boot_wait();
+
 	INIT_WORK(&probe_work, smd_channel_probe_worker);
 
 	if (smd_core_init()) {
-- 
1.6.2.3

Re: [PATCH 5/5] arm: msm: smd: fix SMD modem processor sync condition

[Dima Zavin]

Do we really need a formalized blocking point here? The apps processor
can do other useful initialization work while the modem is booting.
The first time you do a proc_comm call, it checks the PCOM_READY
state, and will block anyway. Preventing the apps processor from
continuing until then is suboptimal. If there are bugs in the modem
code where it incorrectly stomps on shared resources, then those
should be fixed. This patch looks like a hack to me.

--Dima

On Mon, Apr 19, 2010 at 11:03 AM, Daniel Walker <dwalker@codeaurora.org> wrote:
> From: Daniel Walker <c_dwalke@quicinc.com>
>
> When booting up we need to wait for the modem processor to
> partially boot. This is because the modem processor does
> resource allocation for us. If we don't wait the modem won't
> honor our requests and we end up crashing or in an unknown
> state. This change just formalizes the waiting process.
>
> Signed-off-by: Daniel Walker <c_dwalke@quicinc.com>
> ---
>  arch/arm/mach-msm/proc_comm.c |   15 ++++++++++++++-
>  arch/arm/mach-msm/proc_comm.h |    3 +++
>  arch/arm/mach-msm/smd.c       |    7 +++++++
>  3 files changed, 24 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-msm/proc_comm.c b/arch/arm/mach-msm/proc_comm.c
> index 915ee70..f001281 100644
> --- a/arch/arm/mach-msm/proc_comm.c
> +++ b/arch/arm/mach-msm/proc_comm.c
> @@ -107,4 +107,17 @@ int msm_proc_comm(unsigned cmd, unsigned *data1, unsigned *data2)
>        return ret;
>  }
>
> -
> +/*
> + * We need to wait for the ARM9 to at least partially boot
> + * up before we can continue. Since the ARM9 does resource
> + * allocation, if we dont' wait we could end up crashing or in
> + * and unknown state. This function should be called early to
> + * wait on the ARM9.
> + */
> +void __init proc_comm_boot_wait(void)
> +{
> +       void __iomem *base = MSM_SHARED_RAM_BASE;
> +
> +       proc_comm_wait_for(base + MDM_STATUS, PCOM_READY);
> +
> +}
> diff --git a/arch/arm/mach-msm/proc_comm.h b/arch/arm/mach-msm/proc_comm.h
> index 0f5cdd3..12da4ca 100644
> --- a/arch/arm/mach-msm/proc_comm.h
> +++ b/arch/arm/mach-msm/proc_comm.h
> @@ -16,6 +16,8 @@
>  #ifndef _ARCH_ARM_MACH_MSM_PROC_COMM_H_
>  #define _ARCH_ARM_MACH_MSM_PROC_COMM_H_
>
> +#include <linux/init.h>
> +
>  enum {
>        PCOM_CMD_IDLE = 0x0,
>        PCOM_CMD_DONE,
> @@ -251,5 +253,6 @@ enum {
>                (((drvstr) & 0xF) << 17))
>
>  int msm_proc_comm(unsigned cmd, unsigned *data1, unsigned *data2);
> +void __init proc_comm_boot_wait(void);
>
>  #endif
> diff --git a/arch/arm/mach-msm/smd.c b/arch/arm/mach-msm/smd.c
> index 2fa567e..4fec6df 100644
> --- a/arch/arm/mach-msm/smd.c
> +++ b/arch/arm/mach-msm/smd.c
> @@ -1006,6 +1006,13 @@ static int __init msm_smd_probe(struct platform_device *pdev)
>  {
>        pr_info("smd_init()\n");
>
> +       /*
> +        * If we haven't waited for the ARM9 to boot up till now,
> +        * then we need to wait here. Otherwise this should just
> +        * return immediately.
> +        */
> +       proc_comm_boot_wait();
> +
>        INIT_WORK(&probe_work, smd_channel_probe_worker);
>
>        if (smd_core_init()) {
> --
> 1.6.2.3
>
>

Re: [PATCH 5/5] arm: msm: smd: fix SMD modem processor sync condition

[Daniel Walker]

On Mon, 2010-04-19 at 11:34 -0700, Dima Zavin wrote:
> Do we really need a formalized blocking point here? The apps processor
> can do other useful initialization work while the modem is booting.
> The first time you do a proc_comm call, it checks the PCOM_READY
> state, and will block anyway. Preventing the apps processor from
> continuing until then is suboptimal. If there are bugs in the modem
> code where it incorrectly stomps on shared resources, then those
> should be fixed. This patch looks like a hack to me.


Yes, we need to formalize a blocking point .. The apps processor waits
in this way no matter what you do .. Like your saying above "The first
time you do a proc_comm call, it checks the PCOM_READY state, and will
block anyway" that's a hack .. What your saying is _maybe_ there exists
a proc_comm call early enough to prevent a crash, or maybe not .. That's
not formal enough.

This patch makes this a formal process with a comment explain what is
happening, and we will never see a crash related to this again.

Daniel

Re: [PATCH 5/5] arm: msm: smd: fix SMD modem processor sync condition

[Dima Zavin]

On Mon, Apr 19, 2010 at 12:01 PM, Daniel Walker <dwalker@codeaurora.org> wrote:
> On Mon, 2010-04-19 at 11:34 -0700, Dima Zavin wrote:
>> Do we really need a formalized blocking point here? The apps processor
>> can do other useful initialization work while the modem is booting.
>> The first time you do a proc_comm call, it checks the PCOM_READY
>> state, and will block anyway. Preventing the apps processor from
>> continuing until then is suboptimal. If there are bugs in the modem
>> code where it incorrectly stomps on shared resources, then those
>> should be fixed. This patch looks like a hack to me.
>
>
> Yes, we need to formalize a blocking point .. The apps processor waits
> in this way no matter what you do .. Like your saying above "The first
> time you do a proc_comm call, it checks the PCOM_READY state, and will
> block anyway" that's a hack .. What your saying is _maybe_ there exists
> a proc_comm call early enough to prevent a crash, or maybe not .. That's
> not formal enough.

That's not at all what I am saying. There's no maybe. If I don't need
anything from the modem, I won't make a proc_comm call. If there is a
crash because the modem is modifying shared resources that affect the
apps processor without an appropriate synchronization point, then it's
a bug on the modem side. Making this change will only mask modem bugs.

--Dima

> This patch makes this a formal process with a comment explain what is
> happening, and we will never see a crash related to this again.
>
> Daniel
>
>

Re: [PATCH 5/5] arm: msm: smd: fix SMD modem processor sync condition

[Daniel Walker]

On Mon, 2010-04-19 at 12:06 -0700, Dima Zavin wrote:
> On Mon, Apr 19, 2010 at 12:01 PM, Daniel Walker <dwalker@codeaurora.org> wrote:
> > On Mon, 2010-04-19 at 11:34 -0700, Dima Zavin wrote:
> >> Do we really need a formalized blocking point here? The apps processor
> >> can do other useful initialization work while the modem is booting.
> >> The first time you do a proc_comm call, it checks the PCOM_READY
> >> state, and will block anyway. Preventing the apps processor from
> >> continuing until then is suboptimal. If there are bugs in the modem
> >> code where it incorrectly stomps on shared resources, then those
> >> should be fixed. This patch looks like a hack to me.
> >
> >
> > Yes, we need to formalize a blocking point .. The apps processor waits
> > in this way no matter what you do .. Like your saying above "The first
> > time you do a proc_comm call, it checks the PCOM_READY state, and will
> > block anyway" that's a hack .. What your saying is _maybe_ there exists
> > a proc_comm call early enough to prevent a crash, or maybe not .. That's
> > not formal enough.
> 
> That's not at all what I am saying. There's no maybe. If I don't need
> anything from the modem, I won't make a proc_comm call. If there is a
> crash because the modem is modifying shared resources that affect the
> apps processor without an appropriate synchronization point, then it's
> a bug on the modem side. Making this change will only mask modem bugs.

If you don't make a proc_call call SMD won't initialize properly early
on, since the modem may or may not be booted far enough to accept input
over SMD.. Then you can basically have a failed SMD init, which means
you crash when you actually need stuff through SMD.

Daniel

Re: [PATCH 5/5] arm: msm: smd: fix SMD modem processor sync condition

[Dima Zavin]

On Mon, Apr 19, 2010 at 12:11 PM, Daniel Walker <dwalker@codeaurora.org> wrote:
> On Mon, 2010-04-19 at 12:06 -0700, Dima Zavin wrote:
>> On Mon, Apr 19, 2010 at 12:01 PM, Daniel Walker <dwalker@codeaurora.org> wrote:
>> > On Mon, 2010-04-19 at 11:34 -0700, Dima Zavin wrote:
>> >> Do we really need a formalized blocking point here? The apps processor
>> >> can do other useful initialization work while the modem is booting.
>> >> The first time you do a proc_comm call, it checks the PCOM_READY
>> >> state, and will block anyway. Preventing the apps processor from
>> >> continuing until then is suboptimal. If there are bugs in the modem
>> >> code where it incorrectly stomps on shared resources, then those
>> >> should be fixed. This patch looks like a hack to me.
>> >
>> >
>> > Yes, we need to formalize a blocking point .. The apps processor waits
>> > in this way no matter what you do .. Like your saying above "The first
>> > time you do a proc_comm call, it checks the PCOM_READY state, and will
>> > block anyway" that's a hack .. What your saying is _maybe_ there exists
>> > a proc_comm call early enough to prevent a crash, or maybe not .. That's
>> > not formal enough.
>>
>> That's not at all what I am saying. There's no maybe. If I don't need
>> anything from the modem, I won't make a proc_comm call. If there is a
>> crash because the modem is modifying shared resources that affect the
>> apps processor without an appropriate synchronization point, then it's
>> a bug on the modem side. Making this change will only mask modem bugs.
>
> If you don't make a proc_call call SMD won't initialize properly early
> on, since the modem may or may not be booted far enough to accept input
> over SMD.. Then you can basically have a failed SMD init, which means
> you crash when you actually need stuff through SMD.

But smd_core_init is already waiting for smd to be up. It waits for
SMEM_SMSM_SHARED_STATE to be published. What does proc_comm have to do
with it except for the fact that PCOM_READY gets set relatively late
in the modem boot? If the shared state infrastructure is not yet
initialized, it shouldn't be publishing it. Otherwise, you are just
overloading PCOM_READY and again masking issues. Especially when we
move to chips that don't need proc_comm for most things, and 7x30 is
one of them where we get a lot of local clock control, it really seems
wrong to wait for proc_comm to be up.

You mentioned that this change will prevent some random crashes. Have
you traced them down to what exactly is failing?

Thanks.

--Dima

>
> Daniel
>
>

Re: [PATCH 5/5] arm: msm: smd: fix SMD modem processor sync condition

[Daniel Walker]

On Mon, 2010-04-19 at 12:23 -0700, Dima Zavin wrote:

> But smd_core_init is already waiting for smd to be up. It waits for
> SMEM_SMSM_SHARED_STATE to be published. What does proc_comm have to do
> with it except for the fact that PCOM_READY gets set relatively late
> in the modem boot? If the shared state infrastructure is not yet
> initialized, it shouldn't be publishing it. Otherwise, you are just
> overloading PCOM_READY and again masking issues. Especially when we
> move to chips that don't need proc_comm for most things, and 7x30 is
> one of them where we get a lot of local clock control, it really seems
> wrong to wait for proc_comm to be up.
> 
> You mentioned that this change will prevent some random crashes. Have
> you traced them down to what exactly is failing?

I traced the failures down to SMD failing to get initialized .. Be aware
that I'm using this version of SMD, but not a Google kernel. My kernel
has most of the drivers stripped away, with only SMD and MMC and a few
other things.

I've spoken with our SMD expert on this side, and waiting for PCOM_READY
appears to be the right thing to do here.. As for SMEM_SMSM_SHARED_STATE
I don't know, it's possible it's part of the modem simply not being
ready for input from SMD.. I can ask around on this side why that isn't
getting set or checked properly.

Even if this is a defect in the modem we still need to formalize this
check since this problem exists in released phones (like trout where I
found it).

Daniel

Re: [PATCH 5/5] arm: msm: smd: fix SMD modem processor sync condition

[Pavel Machek]

On Mon 2010-04-19 11:03:09, Daniel Walker wrote:
> From: Daniel Walker <c_dwalke@quicinc.com>
> 
> When booting up we need to wait for the modem processor to
> partially boot. This is because the modem processor does
> resource allocation for us. If we don't wait the modem won't
> honor our requests and we end up crashing or in an unknown
> state. This change just formalizes the waiting process.
...

> +/*
> + * We need to wait for the ARM9 to at least partially boot
> + * up before we can continue. Since the ARM9 does resource
> + * allocation, if we dont' wait we could end up crashing or in

typo.

> + * and unknown state. This function should be called early to
> + * wait on the ARM9.
> + */
> +void __init proc_comm_boot_wait(void)
> +{
> +	void __iomem *base = MSM_SHARED_RAM_BASE;
> + 
> +	proc_comm_wait_for(base + MDM_STATUS, PCOM_READY);
> + 
> +}

static inline, move it to header file?

> diff --git a/arch/arm/mach-msm/proc_comm.h b/arch/arm/mach-msm/proc_comm.h
> index 0f5cdd3..12da4ca 100644
> --- a/arch/arm/mach-msm/proc_comm.h
> +++ b/arch/arm/mach-msm/proc_comm.h
> @@ -16,6 +16,8 @@
>  #ifndef _ARCH_ARM_MACH_MSM_PROC_COMM_H_
>  #define _ARCH_ARM_MACH_MSM_PROC_COMM_H_
>  
> +#include <linux/init.h>
> +

...so that this is not needed?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH] arm: msm: smd: fix SMD modem processor sync condition

[Daniel Walker]

From: Daniel Walker <c_dwalke@quicinc.com>

I'm not sure I like this one because there is too much code moving around.
Maybe we need a few more cleanups on this file before we can do this.

--
When booting up we need to wait for the modem processor to
partially boot. This is because the modem processor does
resource allocation for us. If we don't wait the modem won't
honor our requests and we end up crashing or in an unknown
state. This change just formalizes the waiting process.

Signed-off-by: Daniel Walker <c_dwalke@quicinc.com>
---
 arch/arm/mach-msm/proc_comm.c |   12 +-----------
 arch/arm/mach-msm/proc_comm.h |   28 ++++++++++++++++++++++++++++
 arch/arm/mach-msm/smd.c       |    7 +++++++
 3 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-msm/proc_comm.c b/arch/arm/mach-msm/proc_comm.c
index 915ee70..2c255b1 100644
--- a/arch/arm/mach-msm/proc_comm.c
+++ b/arch/arm/mach-msm/proc_comm.c
@@ -30,16 +30,6 @@ static inline void notify_other_proc_comm(void)
 	writel(1, MSM_A2M_INT(6));
 }
 
-#define APP_COMMAND 0x00
-#define APP_STATUS  0x04
-#define APP_DATA1   0x08
-#define APP_DATA2   0x0C
-
-#define MDM_COMMAND 0x10
-#define MDM_STATUS  0x14
-#define MDM_DATA1   0x18
-#define MDM_DATA2   0x1C
-
 static DEFINE_SPINLOCK(proc_comm_lock);
 
 /* The higher level SMD support will install this to
@@ -55,7 +45,7 @@ int (*msm_check_for_modem_crash)(void);
  * restart so the msm_proc_comm() routine can restart
  * the operation from the beginning.
  */
-static int proc_comm_wait_for(void __iomem *addr, unsigned value)
+int proc_comm_wait_for(void __iomem *addr, unsigned value)
 {
 	for (;;) {
 		if (readl(addr) == value)
diff --git a/arch/arm/mach-msm/proc_comm.h b/arch/arm/mach-msm/proc_comm.h
index 0f5cdd3..cd5d63e 100644
--- a/arch/arm/mach-msm/proc_comm.h
+++ b/arch/arm/mach-msm/proc_comm.h
@@ -16,6 +16,8 @@
 #ifndef _ARCH_ARM_MACH_MSM_PROC_COMM_H_
 #define _ARCH_ARM_MACH_MSM_PROC_COMM_H_
 
+#include <mach/msm_iomap.h>
+
 enum {
 	PCOM_CMD_IDLE = 0x0,
 	PCOM_CMD_DONE,
@@ -250,6 +252,32 @@ enum {
 		(((pull) & 0x3) << 15)		| \
 		(((drvstr) & 0xF) << 17))
 
+#define APP_COMMAND 0x00
+#define APP_STATUS  0x04
+#define APP_DATA1   0x08
+#define APP_DATA2   0x0C
+
+#define MDM_COMMAND 0x10
+#define MDM_STATUS  0x14
+#define MDM_DATA1   0x18
+#define MDM_DATA2   0x1C
+
 int msm_proc_comm(unsigned cmd, unsigned *data1, unsigned *data2);
+int proc_comm_wait_for(void __iomem *addr, unsigned value);
+
+/*
+ * We need to wait for the ARM9 to at least partially boot
+ * up before we can continue. Since the ARM9 does resource
+ * allocation, if we don't wait we could end up crashing or in
+ * and unknown state. This function should be called early to
+ * wait on the ARM9.
+ */
+static inline void proc_comm_boot_wait(void)
+{
+	void __iomem *base = MSM_SHARED_RAM_BASE;
+
+	proc_comm_wait_for(base + MDM_STATUS, PCOM_READY);
+
+}
 
 #endif
diff --git a/arch/arm/mach-msm/smd.c b/arch/arm/mach-msm/smd.c
index c404892..bd75dc8 100644
--- a/arch/arm/mach-msm/smd.c
+++ b/arch/arm/mach-msm/smd.c
@@ -1058,6 +1058,13 @@ static int __init msm_smd_probe(struct platform_device *pdev)
 {
 	pr_info("smd_init()\n");
 
+	/*
+	 * If we haven't waited for the ARM9 to boot up till now,
+	 * then we need to wait here. Otherwise this should just
+	 * return immediately.
+	 */
+	proc_comm_boot_wait();
+
 	INIT_WORK(&probe_work, smd_channel_probe_worker);
 
 	if (smd_core_init()) {
-- 
1.6.2.3