arm: msm: smd: fix SMD modem processor sync condition
Subsystems (3)
PATCH 5/5 arm: msm: smd: fix SMD modem processor sync condition
From: Daniel Walker <[email protected]>
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.
--- 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(-)
arch/arm/mach-msm/proc_comm.c +14 -1
@@ -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 ); + +}
arch/arm/mach-msm/proc_comm.h +3 -0
@@ -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
arch/arm/mach-msm/smd.c +7 -0
@@ -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 ()) {
Signature · 1 line
1.6.2.3
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
Quoted 80 lines · Daniel Walker
From: Daniel Walker <[email protected]>
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 <[email protected]>---
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(-)arch/arm/mach-msm/proc_comm.c +14
-1@@ -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); + +}arch/arm/mach-msm/proc_comm.h +3
-0@@ -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 ); #endifarch/arm/mach-msm/smd.c +7
-1@@ -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
Quoted 7 lines · 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_commcall, it checks thePCOM_READYstate, 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
Quoted 10 lines · Daniel Walker
Quoted 7 lines · 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_commcall, it checks thePCOM_READYstate, 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.
Quoted 6 lines
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_commcall, it checks thePCOM_READYstate, and will block anyway" that's a hack .. What your saying is _maybe_ there exists aproc_commcall 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
Quoted 6 lines
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
Quoted 18 lines · Dima Zavin
Quoted 10 lines · Daniel Walker
Quoted 7 lines · 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_commcall, it checks thePCOM_READYstate, 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.Quoted 6 lines
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_commcall, it checks thePCOM_READYstate, and will block anyway" that's a hack .. What your saying is _maybe_ there exists aproc_commcall early enough to prevent a crash, or maybe not .. That's not formal enough.
Quoted 5 lines
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_commcall. 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
Quoted 25 lines · Daniel Walker
Quoted 18 lines · Dima Zavin
Quoted 10 lines · Daniel Walker
Quoted 7 lines · 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_commcall, it checks thePCOM_READYstate, 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.Quoted 6 lines
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_commcall, it checks thePCOM_READYstate, and will block anyway" that's a hack .. What your saying is _maybe_ there exists aproc_commcall early enough to prevent a crash, or maybe not .. That's not formal enough.Quoted 5 lines
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_commcall. 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.
Quoted 4 lines
If you don't make a
proc_callcall 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
Quoted 4 lines
Daniel
Quoted 10 lines · Dima Zavin
But
smd_core_initis already waiting for smd to be up. It waits forSMEM_SMSM_SHARED_STATEto be published. What doesproc_commhave to do with it except for the fact thatPCOM_READYgets 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 overloadingPCOM_READYand again masking issues. Especially when we move to chips that don't needproc_commfor most things, and 7x30 is one of them where we get a lot of local clock control, it really seems wrong to wait forproc_commto be up.
Quoted 2 lines
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
Quoted 7 lines · Daniel Walker
From: Daniel Walker <[email protected]>
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.
...
Quoted 4 lines
+/*
+ * 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.
arch/arm/mach-msm/proc_comm.c int msm_proc_comm(unsigned cmd, unsigned *data1, unsigned *data2) · line 123
Quoted 10 lines
+ * and unknown state. This function should be called early to
+ * wait on the ARM9.
+ */arch/arm/mach-msm/proc_comm.c +7
-0+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?
arch/arm/mach-msm/proc_comm.h line 20
Quoted 10 lines
arch/arm/mach-msm/proc_comm.h +2
-0@@ -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?
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
PATCH 1/1 arm: msm: smd: fix SMD modem processor sync condition
From: Daniel Walker <[email protected]>
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.
--- 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(-)
arch/arm/mach-msm/proc_comm.c +1 -11
@@ -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)
arch/arm/mach-msm/proc_comm.h +28 -0
@@ -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
arch/arm/mach-msm/smd.c +7 -0
@@ -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 ()) {
Signature · 1 line
1.6.2.3