[RFC/PATCH] powerpc: Move CPM command handling into the cpm drivers

[RFC/PATCH] powerpc: Move CPM command handling into the cpm drivers

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

Signed-off-by: Jochen Friedrich <jochen@scram.de>
---
 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(-)

diff --git a/arch/powerpc/sysdev/commproc.c b/arch/powerpc/sysdev/commproc.c
index f6a6378..2bddbde 100644
--- a/arch/powerpc/sysdev/commproc.c
+++ b/arch/powerpc/sysdev/commproc.c
@@ -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.
  */
diff --git a/arch/powerpc/sysdev/cpm2_common.c b/arch/powerpc/sysdev/cpm2_common.c
index 859362f..9e36b6b 100644
--- a/arch/powerpc/sysdev/cpm2_common.c
+++ b/arch/powerpc/sysdev/cpm2_common.c
@@ -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
diff --git a/drivers/net/fs_enet/mac-fcc.c b/drivers/net/fs_enet/mac-fcc.c
index da4efbc..e363211 100644
--- a/drivers/net/fs_enet/mac-fcc.c
+++ b/drivers/net/fs_enet/mac-fcc.c
@@ -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)
diff --git a/drivers/net/fs_enet/mac-scc.c b/drivers/net/fs_enet/mac-scc.c
index 03134f4..5ff856d 100644
--- a/drivers/net/fs_enet/mac-scc.c
+++ b/drivers/net/fs_enet/mac-scc.c
@@ -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)
diff --git a/drivers/serial/cpm_uart/cpm_uart_cpm1.c b/drivers/serial/cpm_uart/cpm_uart_cpm1.c
index 52fb044..6ea0366 100644
--- a/drivers/serial/cpm_uart/cpm_uart_cpm1.c
+++ b/drivers/serial/cpm_uart/cpm_uart_cpm1.c
@@ -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)
diff --git a/drivers/serial/cpm_uart/cpm_uart_cpm2.c b/drivers/serial/cpm_uart/cpm_uart_cpm2.c
index 882dbc1..def0158 100644
--- a/drivers/serial/cpm_uart/cpm_uart_cpm2.c
+++ b/drivers/serial/cpm_uart/cpm_uart_cpm2.c
@@ -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)
diff --git a/include/asm-powerpc/cpm.h b/include/asm-powerpc/cpm.h
index 48df9f3..fae83b1 100644
--- a/include/asm-powerpc/cpm.h
+++ b/include/asm-powerpc/cpm.h
@@ -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
-- 
1.5.3.4

Re: [RFC/PATCH] powerpc: Move CPM command handling into the cpm drivers

[Grant Likely]

On 11/22/07, Jochen Friedrich <jochen@scram.de> wrote:
> 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.
>
> Signed-off-by: Jochen Friedrich <jochen@scram.de>
> ---
>  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(-)
>
> diff --git a/arch/powerpc/sysdev/commproc.c b/arch/powerpc/sysdev/commproc.c
> index f6a6378..2bddbde 100644
> --- a/arch/powerpc/sysdev/commproc.c
> +++ b/arch/powerpc/sysdev/commproc.c
> @@ -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;

Do these need to be protected with a spin lock?

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [RFC/PATCH] powerpc: Move CPM command handling into the cpm drivers

[Vitaly Bordug]

On Thu, 22 Nov 2007 11:36:29 -0700
Grant Likely wrote:

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

-- 
Sincerely, Vitaly

Re: [RFC/PATCH] powerpc: Move CPM command handling into the cpm drivers

[Jochen Friedrich]

Hi Vitaly,

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

Wouldn't spin_lock_irqsave() prevent a deadlock?

Thanks,
Jochen

Re: [RFC/PATCH] powerpc: Move CPM command handling into the cpm drivers

[Vitaly Bordug]

On Sat, 24 Nov 2007 18:53:34 +0100
Jochen Friedrich wrote:

> Hi Vitaly,
> 
> >>> +       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
> 
> Wouldn't spin_lock_irqsave() prevent a deadlock?
> 
yes, I believe it is OK for now.

> Thanks,
> Jochen


-- 
Sincerely, Vitaly

Re: [RFC/PATCH] powerpc: Move CPM command handling into the cpm drivers

[Scott Wood]

On Fri, Nov 23, 2007 at 12:51:21AM +0300, Vitaly Bordug wrote:
> 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?

> 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

Re: [RFC/PATCH] powerpc: Move CPM command handling into the cpm drivers

[Vitaly Bordug]

On Mon, 26 Nov 2007 10:24:46 -0600
Scott Wood wrote:

> On Fri, Nov 23, 2007 at 12:51:21AM +0300, Vitaly Bordug wrote:
> > 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.

> > 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.
-- 
Sincerely, Vitaly

Re: [RFC/PATCH] powerpc: Move CPM command handling into the cpm drivers

[Scott Wood]

Vitaly Bordug wrote:
> 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

Re: [RFC/PATCH] powerpc: Move CPM command handling into the cpm drivers

[Vitaly Bordug]

On Mon, 26 Nov 2007 15:41:19 -0600
Scott Wood wrote:

> Vitaly Bordug wrote:
> > 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. 

-- 
Sincerely, Vitaly