Skip to contentSkip to conversation

powerpc: Move CPM command handling into the cpm drivers

RFC/PATCH Jochen Friedrich 4 participants 9 messages Nov 22, 2007 – Nov 27, 2007

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

This patch moves the CPM command handling into commproc.c for CPM1 and cpm2_common.c. This is yet another preparation to get rid of drivers accessing the CPM via the global cpmp.

---
 arch/powerpc/sysdev/commproc.c          |   20 ++++++++++++++++++++
 arch/powerpc/sysdev/cpm2_common.c       |   17 +++++++++++++++++
 drivers/net/fs_enet/mac-fcc.c           |   10 +---------
 drivers/net/fs_enet/mac-scc.c           |   11 +----------
 drivers/serial/cpm_uart/cpm_uart_cpm1.c |    6 +-----
 drivers/serial/cpm_uart/cpm_uart_cpm2.c |    8 +-------
 include/asm-powerpc/cpm.h               |    1 +
 7 files changed, 42 insertions(+), 31 deletions(-)
arch/powerpc/sysdev/commproc.c +20 -0
index f6a6378..2bddbde 100644
@@ -240,6 +240,26 @@ void __init cpm_reset(void)
 #endif
 }
 
+#define MAX_CR_CMD_LOOPS        10000
+
+int cpm_command(u32 command, u8 opcode)
+{
+	int i;
+
+	if (command & 0xffffff0f)
+		return -EINVAL;
+
+	out_be16(&cpmp->cp_cpcr, command | CPM_CR_FLG | (opcode << 8));
+	for (i = 0; i < MAX_CR_CMD_LOOPS; i++)
+		if ((in_be16(&cpmp->cp_cpcr) & CPM_CR_FLG) == 0)
+			return 0;
+
+	printk(KERN_ERR "%s(): Not able to issue CPM command\n",
+		__FUNCTION__);
+	return -EIO;
+}
+EXPORT_SYMBOL(cpm_command);
+
 /* We used to do this earlier, but have to postpone as long as possible
  * to ensure the kernel VM is now running.
  */
arch/powerpc/sysdev/cpm2_common.c +17 -0
index 859362f..9e36b6b 100644
@@ -83,6 +83,23 @@ cpm2_reset(void)
 	cpmp = &cpm2_immr->im_cpm;
 }
 
+#define MAX_CR_CMD_LOOPS        10000
+
+int cpm_command(u32 command, u8 opcode)
+{
+	int i;
+
+	out_be32(&cpmp->cp_cpcr, command | opcode | CPM_CR_FLG);
+	for (i = 0; i < MAX_CR_CMD_LOOPS; i++)
+		if ((in_be32(&cpmp->cp_cpcr) & CPM_CR_FLG) == 0)
+			return 0;
+
+	printk(KERN_ERR "%s(): Not able to issue CPM command\n",
+		__FUNCTION__);
+	return -EIO;
+}
+EXPORT_SYMBOL(cpm_command);
+
 /* Set a baud rate generator.  This needs lots of work.  There are
  * eight BRGs, which can be connected to the CPM channels or output
  * as clocks.  The BRGs are in two different block of internal
drivers/net/fs_enet/mac-fcc.c +1 -9
index da4efbc..e363211 100644
@@ -81,16 +81,8 @@
 static inline int fcc_cr_cmd(struct fs_enet_private *fep, u32 op)
 {
 	const struct fs_platform_info *fpi = fep->fpi;
-	int i;
-
-	W32(cpmp, cp_cpcr, fpi->cp_command | op | CPM_CR_FLG);
-	for (i = 0; i < MAX_CR_CMD_LOOPS; i++)
-		if ((R32(cpmp, cp_cpcr) & CPM_CR_FLG) == 0)
-			return 0;
 
-	printk(KERN_ERR "%s(): Not able to issue CPM command\n",
-	       __FUNCTION__);
-	return 1;
+	return cpm_command(fpi->cp_command, op);
 }
 
 static int do_pd_setup(struct fs_enet_private *fep)
drivers/net/fs_enet/mac-scc.c +1 -10
index 03134f4..5ff856d 100644
@@ -89,21 +89,12 @@
  * Delay to wait for SCC reset command to complete (in us)
  */
 #define SCC_RESET_DELAY		50
-#define MAX_CR_CMD_LOOPS	10000
 
 static inline int scc_cr_cmd(struct fs_enet_private *fep, u32 op)
 {
 	const struct fs_platform_info *fpi = fep->fpi;
-	int i;
-
-	W16(cpmp, cp_cpcr, fpi->cp_command | CPM_CR_FLG | (op << 8));
-	for (i = 0; i < MAX_CR_CMD_LOOPS; i++)
-		if ((R16(cpmp, cp_cpcr) & CPM_CR_FLG) == 0)
-			return 0;
 
-	printk(KERN_ERR "%s(): Not able to issue CPM command\n",
-		__FUNCTION__);
-	return 1;
+	return cpm_command(fpi->cp_command, op);
 }
 
 static int do_pd_setup(struct fs_enet_private *fep)
drivers/serial/cpm_uart/cpm_uart_cpm1.c +1 -5
index 52fb044..6ea0366 100644
@@ -52,11 +52,7 @@
 #ifdef CONFIG_PPC_CPM_NEW_BINDING
 void cpm_line_cr_cmd(struct uart_cpm_port *port, int cmd)
 {
-	u16 __iomem *cpcr = &cpmp->cp_cpcr;
-
-	out_be16(cpcr, port->command | (cmd << 8) | CPM_CR_FLG);
-	while (in_be16(cpcr) & CPM_CR_FLG)
-		;
+	cpm_command(port->command, cmd);
 }
 #else
 void cpm_line_cr_cmd(struct uart_cpm_port *port, int cmd)
drivers/serial/cpm_uart/cpm_uart_cpm2.c +1 -7
index 882dbc1..def0158 100644
@@ -52,13 +52,7 @@
 #ifdef CONFIG_PPC_CPM_NEW_BINDING
 void cpm_line_cr_cmd(struct uart_cpm_port *port, int cmd)
 {
-	cpm_cpm2_t __iomem *cp = cpm2_map(im_cpm);
-
-	out_be32(&cp->cp_cpcr, port->command | cmd | CPM_CR_FLG);
-	while (in_be32(&cp->cp_cpcr) & CPM_CR_FLG)
-		;
-
-	cpm2_unmap(cp);
+	cpm_command(port->command, cmd);
 }
 #else
 void cpm_line_cr_cmd(struct uart_cpm_port *port, int cmd)
include/asm-powerpc/cpm.h +1 -0
index 48df9f3..fae83b1 100644
@@ -10,5 +10,6 @@ int cpm_muram_free(unsigned long offset);
 unsigned long cpm_muram_alloc_fixed(unsigned long offset, unsigned long size);
 void __iomem *cpm_muram_addr(unsigned long offset);
 dma_addr_t cpm_muram_dma(void __iomem *addr);
+int cpm_command(u32 command, u8 opcode);
 
 #endif
Signature · 1 line
1.5.3.4
#1 lore raw
Quoted 30 lines · Jochen Friedrich

This patch moves the CPM command handling into commproc.c for CPM1 and cpm2_common.c. This is yet another preparation to get rid of drivers accessing the CPM via the global cpmp.

---
 arch/powerpc/sysdev/commproc.c          |   20 ++++++++++++++++++++
 arch/powerpc/sysdev/cpm2_common.c       |   17 +++++++++++++++++
 drivers/net/fs_enet/mac-fcc.c           |   10 +---------
 drivers/net/fs_enet/mac-scc.c           |   11 +----------
 drivers/serial/cpm_uart/cpm_uart_cpm1.c |    6 +-----
 drivers/serial/cpm_uart/cpm_uart_cpm2.c |    8 +-------
 include/asm-powerpc/cpm.h               |    1 +
 7 files changed, 42 insertions(+), 31 deletions(-)
arch/powerpc/sysdev/commproc.c +7 -0
index f6a6378..2bddbde 100644
@@ -240,6 +240,26 @@ void __init cpm_reset(void)
 #endif
 }
 
+#define MAX_CR_CMD_LOOPS        10000
+
+int cpm_command(u32 command, u8 opcode)
+{
+       int i;
+
+       if (command & 0xffffff0f)

arch/powerpc/sysdev/commproc.c void __init cpm_reset(void) · line 259

Quoted 10 lines
arch/powerpc/sysdev/commproc.c +10 -0
+               return -EINVAL;
+
+       out_be16(&cpmp->cp_cpcr, command | CPM_CR_FLG | (opcode << 8));
+       for (i = 0; i < MAX_CR_CMD_LOOPS; i++)
+               if ((in_be16(&cpmp->cp_cpcr) & CPM_CR_FLG) == 0)
+                       return 0;
+
+       printk(KERN_ERR "%s(): Not able to issue CPM command\n",
+               __FUNCTION__);
+       return -EIO;

Do these need to be protected with a spin lock?

Cheers,
g.

Signature · 4 lines
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
[email protected]
(403) 399-0195
#2 lore raw
Quoted 10 lines · On Thu, 22 Nov 2007 11:36:29 -0700 Grant Likely wrote:
Quoted 10 lines
arch/powerpc/sysdev/commproc.c +8 -0
+int cpm_command(u32 command, u8 opcode)
+{
+       int i;
+
+       if (command & 0xffffff0f)
+               return -EINVAL;
+
+       out_be16(&cpmp->cp_cpcr, command | CPM_CR_FLG | (opcode <<

8));
+ for (i = 0; i < MAX_CR_CMD_LOOPS; i++)

Quoted 8 lines

arch/powerpc/sysdev/commproc.c void __init cpm_reset(void) · line 259

Quoted 6 lines
arch/powerpc/sysdev/commproc.c +6 -0
+               if ((in_be16(&cpmp->cp_cpcr) & CPM_CR_FLG) == 0)
+                       return 0;
+
+       printk(KERN_ERR "%s(): Not able to issue CPM command\n",
+               __FUNCTION__);
+       return -EIO;  

Do these need to be protected with a spin lock?

Even that might be not enough - we may have simultaneous call of this func in non-smp case... I was thinking of some kind of refcount, so one that is going to issue CPM command, must do say pq_cpmp_get() and another driver won't be able to mangle with cpcr while it's not done with previous request.

Yet I am not telling it was better the way it used to be - this approach looks okay but needs some efforts to defend against deadlocks while we are at it.

Signature · 1 line
Sincerely, Vitaly
#3 lore raw

Hi Vitaly,

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
arch/powerpc/sysdev/commproc.c +3 -0
+       printk(KERN_ERR "%s(): Not able to issue CPM command\n",
+               __FUNCTION__);
+       return -EIO;  
Quoted 8 lines
Quoted 2 lines

Do these need to be protected with a spin lock?

Even that might be not enough - we may have simultaneous call of this func in non-smp case... I was thinking of some kind of refcount, so one that is going to issue CPM command, must do say pq_cpmp_get() and another driver won't be able to mangle with cpcr while it's not done with previous request.

Yet I am not telling it was better the way it used to be - this approach looks okay but needs some efforts to defend against deadlocks while we are at it

Wouldn't spin_lock_irqsave() prevent a deadlock?

Thanks,
Jochen

#4 lore raw
Quoted 14 lines · On Sat, 24 Nov 2007 18:53:34 +0100 Jochen Friedrich wrote:

Hi Vitaly,

Quoted 12 lines
Quoted 6 lines

arch/powerpc/sysdev/commproc.c void __init cpm_reset(void) · line 259

Quoted 4 lines
arch/powerpc/sysdev/commproc.c +3 -0
+       printk(KERN_ERR "%s(): Not able to issue CPM command\n",
+               __FUNCTION__);
+       return -EIO;  

Do these need to be protected with a spin lock?

Even that might be not enough - we may have simultaneous call of this func in non-smp case... I was thinking of some kind of refcount, so one that is going to issue CPM command, must do say pq_cpmp_get() and another driver won't be able to mangle with cpcr while it's not done with previous request.

Quoted 6 lines
Quoted 3 lines

Yet I am not telling it was better the way it used to be - this approach looks okay but needs some efforts to defend against deadlocks while we are at it

Wouldn't spin_lock_irqsave() prevent a deadlock?

yes, I believe it is OK for now.

Quoted 2 lines

Thanks,
Jochen

Signature · 1 line
Sincerely, Vitaly
#5 lore raw
In reply to Vitaly Bordug
Quoted 2 lines · Vitaly Bordug

Even that might be not enough - we may have simultaneous call of this func in non-smp case...

Do you really think that every piece of code that uses spinlocks in the kernel is broken on non-SMP?

Quoted 3 lines

I was thinking of some kind of refcount, so one that is going to issue CPM command, must do say pq_cpmp_get() and another driver won't be able to mangle with cpcr while it's not done with previous request.

How on earth are you going to effect mutual exclusion using reference counting?

-Scott

#6 lore raw
Quoted 7 lines · On Mon, 26 Nov 2007 10:24:46 -0600 Scott Wood wrote:
Quoted 2 lines · Vitaly Bordug

Even that might be not enough - we may have simultaneous call of this func in non-smp case...

Do you really think that every piece of code that uses spinlocks in the kernel is broken on non-SMP?

No. I think spinlock is not universal save thing in such cases. See below.

Quoted 8 lines
Quoted 4 lines

I was thinking of some kind of refcount, so one that is going to issue CPM command, must do say pq_cpmp_get() and another driver won't be able to mangle with cpcr while it's not done with previous request.

How on earth are you going to effect mutual exclusion using reference counting?

perhaps I was not clear enough. That was a rough idea how to handle the whole thing, not just cpm_cr_cmd. This cpm command is a corner case, but there can be other actions that may confuse CPM being triggered simultaneously or overlapping. This is part of much bigger problem, and I was intended to have a look what people think about that.

Signature · 1 line
Sincerely, Vitaly
#7 lore raw
Quoted 4 lines · Vitaly Bordug

perhaps I was not clear enough. That was a rough idea how to handle the whole thing, not just cpm_cr_cmd. This cpm command is a corner case, but there can be other actions that may confuse CPM being triggered simultaneously or overlapping.

What kind of actions did you have in mind? Microcode patching?

-Scott

#8 lore raw
Quoted 8 lines · On Mon, 26 Nov 2007 15:41:19 -0600 Scott Wood wrote:
Quoted 4 lines · Vitaly Bordug

perhaps I was not clear enough. That was a rough idea how to handle the whole thing, not just cpm_cr_cmd. This cpm command is a corner case, but there can be other actions that may confuse CPM being triggered simultaneously or overlapping.

What kind of actions did you have in mind? Microcode patching?

microcode is another case to handle gracefully. There are also
"soft" cases like cpmux mess-up, incompatible SoC devices (which we are handling by logical exclude now but which do have natural reasons of "not leaving together" like shared dedicated GPIO or smth else) etc.

Signature · 1 line
Sincerely, Vitaly