[PATCH] 8xx: add cpm_get_cpmp()

[PATCH] 8xx: add cpm_get_cpmp()

[Aristeu Sergio Rozanski Filho]

8xx: add cpm_get_cpmp() to make cpmp visible to modules

Signed-off-by: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>

Index: 2.6-8xx/arch/ppc/8xx_io/commproc.c
===================================================================
--- 2.6-8xx.orig/arch/ppc/8xx_io/commproc.c	2005-08-03 17:26:05.000000000 -0300
+++ 2.6-8xx/arch/ppc/8xx_io/commproc.c	2005-08-05 17:13:21.000000000 -0300
@@ -462,3 +462,10 @@
 	return ((immap_t *)IMAP_ADDR)->im_cpm.cp_dpmem + offset;
 }
 EXPORT_SYMBOL(cpm_dpram_addr);
+
+cpm8xx_t *cpm_get_cpmp(void)
+{
+	return cpmp;
+}
+EXPORT_SYMBOL(cpm_get_cpmp);
+
Index: 2.6-8xx/include/asm/commproc.h
===================================================================
--- 2.6-8xx.orig/include/asm/commproc.h	2005-07-28 12:06:51.000000000 -0300
+++ 2.6-8xx/include/asm/commproc.h	2005-08-05 17:14:28.000000000 -0300
@@ -78,6 +78,7 @@
 extern void cpm_dpdump(void);
 extern void *cpm_dpram_addr(uint offset);
 extern void cpm_setbrg(uint brg, uint rate);
+extern cpm8xx_t *cpm_get_cpmp(void);
 
 extern uint m8xx_cpm_hostalloc(uint size);
 extern int  m8xx_cpm_hostfree(uint start);

Re: [PATCH] 8xx: add cpm_get_cpmp()

[Marcelo Tosatti]

Aris,

It already is exported, declared as

commproc.h:extern       cpm8xx_t        *cpmp;          /* Pointer to comm processor */


and many drivers use the pointer directly.

arch/ppc/8260_io/ drivers also use the same convention.

Not sure I see much advantage in changing it?

On Sat, Aug 06, 2005 at 03:03:53PM -0300, Aristeu Sergio Rozanski Filho wrote:
> 8xx: add cpm_get_cpmp() to make cpmp visible to modules
> 
> Signed-off-by: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
> 
> Index: 2.6-8xx/arch/ppc/8xx_io/commproc.c
> ===================================================================
> --- 2.6-8xx.orig/arch/ppc/8xx_io/commproc.c	2005-08-03 17:26:05.000000000 -0300
> +++ 2.6-8xx/arch/ppc/8xx_io/commproc.c	2005-08-05 17:13:21.000000000 -0300
> @@ -462,3 +462,10 @@
>  	return ((immap_t *)IMAP_ADDR)->im_cpm.cp_dpmem + offset;
>  }
>  EXPORT_SYMBOL(cpm_dpram_addr);
> +
> +cpm8xx_t *cpm_get_cpmp(void)
> +{
> +	return cpmp;
> +}
> +EXPORT_SYMBOL(cpm_get_cpmp);
> +
> Index: 2.6-8xx/include/asm/commproc.h
> ===================================================================
> --- 2.6-8xx.orig/include/asm/commproc.h	2005-07-28 12:06:51.000000000 -0300
> +++ 2.6-8xx/include/asm/commproc.h	2005-08-05 17:14:28.000000000 -0300
> @@ -78,6 +78,7 @@
>  extern void cpm_dpdump(void);
>  extern void *cpm_dpram_addr(uint offset);
>  extern void cpm_setbrg(uint brg, uint rate);
> +extern cpm8xx_t *cpm_get_cpmp(void);
>  
>  extern uint m8xx_cpm_hostalloc(uint size);
>  extern int  m8xx_cpm_hostfree(uint start);

Re: [PATCH] 8xx: add cpm_get_cpmp()

[Aristeu Sergio Rozanski Filho]

> It already is exported, declared as
> 
> commproc.h:extern       cpm8xx_t        *cpmp;          /* Pointer to comm processor */
> 
> 
> and many drivers use the pointer directly.
> 
> arch/ppc/8260_io/ drivers also use the same convention.
> 
> Not sure I see much advantage in changing it?
but not with EXPORT_SYMBOL(). I noticed this while compiling i2c stuff
as module. also, I think it's better add a function to do this instead
doing EXPORT_SYMBOL() in a variable.
what you think?

-- 
Aristeu

Re: [PATCH] 8xx: add cpm_get_cpmp()

[Marcelo Tosatti]

On Sat, Aug 06, 2005 at 08:42:44PM -0300, Aristeu Sergio Rozanski Filho wrote:
> > It already is exported, declared as
> > 
> > commproc.h:extern       cpm8xx_t        *cpmp;          /* Pointer to comm processor */
> > 
> > 
> > and many drivers use the pointer directly.
> > 
> > arch/ppc/8260_io/ drivers also use the same convention.
> > 
> > Not sure I see much advantage in changing it?
> but not with EXPORT_SYMBOL(). I noticed this while compiling i2c stuff
> as module. 

Then other modules suffer from the same problem - what if you EXPORT_SYMBOL(cpmp)? 

> also, I think it's better add a function to do this instead
> doing EXPORT_SYMBOL() in a variable.
> what you think?

Well it is better in theory because it hides details, but in practice
I'm not sure its worth it - the CPM pointer (along with the whole immap)
never changes during runtime.

Re: [PATCH] 8xx: add cpm_get_cpmp()

[Dan Malek]

On Aug 6, 2005, at 7:27 PM, Marcelo Tosatti wrote:

> It already is exported, declared as
>
> commproc.h:extern       cpm8xx_t        *cpmp;          /* Pointer to 
> comm processor */
>
>
> and many drivers use the pointer directly.

We shouldn't be doing this.  All drivers should ioremap() any
peripheral spaces they use and not make any assumptions
someone else has done that already.  We've been making
such changes in the 82xx/83xx/85xx CPM2 drivers.  If it
isn't convenient to find the #defines for the ioremap(), we
should change that.  When I originally wrote all of this
code, it was long ago when we didn't have some of the
abstractions and it seemed any shortcuts for performance
were desired :-)


> arch/ppc/8260_io/ drivers also use the same convention.

If there are any drivers in here, they either aren't used or
on their way out.  The only things that should remain are
common support functions.


Thanks.

	-- Dan

Re: [PATCH] 8xx: add cpm_get_cpmp()

[Dan Malek]

On Aug 6, 2005, at 7:42 PM, Aristeu Sergio Rozanski Filho wrote:

> but not with EXPORT_SYMBOL(). I noticed this while compiling i2c stuff
> as module. also, I think it's better add a function to do this instead
> doing EXPORT_SYMBOL() in a variable.

Right.  We should just ioremap() :-)

Thanks.

	-- Dan

Re: [PATCH] 8xx: add cpm_get_cpmp()

[Marcelo Tosatti]

On Sat, Aug 06, 2005 at 10:40:37PM -0400, Dan Malek wrote:
> 
> On Aug 6, 2005, at 7:27 PM, Marcelo Tosatti wrote:
> 
> >It already is exported, declared as
> >
> >commproc.h:extern       cpm8xx_t        *cpmp;          /* Pointer to 
> >comm processor */
> >
> >
> >and many drivers use the pointer directly.
> 
> We shouldn't be doing this.  All drivers should ioremap() any
> peripheral spaces they use and not make any assumptions
> someone else has done that already.  We've been making
> such changes in the 82xx/83xx/85xx CPM2 drivers.  If it
> isn't convenient to find the #defines for the ioremap(), we
> should change that.  When I originally wrote all of this
> code, it was long ago when we didn't have some of the
> abstractions and it seemed any shortcuts for performance
> were desired :-)

OK makes sense (yep it was even discussed already).

Aris, sounds like you should proceed with cpm_get_cpmp() and change all
other drivers using it also.

> 
> 
> >arch/ppc/8260_io/ drivers also use the same convention.
> 
> If there are any drivers in here, they either aren't used or
> on their way out.  The only things that should remain are
> common support functions.

OK!

Re: [PATCH] 8xx: add cpm_get_cpmp()

[Dan Malek]

On Aug 7, 2005, at 12:31 AM, Marcelo Tosatti wrote:

> Aris, sounds like you should proceed with cpm_get_cpmp() and change all
> other drivers using it also.

It depends how you define the semantics of this function call.
Can you call it any (and all of the) time you need it, or does it
actually perform an ioremap() and you only want to call it
once?

I guess implemented properly it doesn't matter.  On the first
call it should do the ioremap() and on subsequent calls it
just returns the mapping.  This is one of those common
functions that should be in commproc.c.

Thanks.

	-- Dan

Re: [PATCH] 8xx: add cpm_get_cpmp()

[Aristeu Sergio Rozanski Filho]

> It depends how you define the semantics of this function call.
> Can you call it any (and all of the) time you need it, or does it
> actually perform an ioremap() and you only want to call it
> once?
what about don't cache it and call ioremap() from driver? (I guess
ioremap() already check if an area is already mapped, no?)

--
Aristeu

Re: [PATCH] 8xx: add cpm_get_cpmp()

[Marcelo Tosatti]

On Sun, Aug 07, 2005 at 12:44:32PM -0300, Aristeu Sergio Rozanski Filho wrote:
> > It depends how you define the semantics of this function call.
> > Can you call it any (and all of the) time you need it, or does it
> > actually perform an ioremap() and you only want to call it
> > once?
> what about don't cache it and call ioremap() from driver? (I guess
> ioremap() already check if an area is already mapped, no?)

Yep, ioremap() should be doing virtual address caching already, no?

Re: [PATCH] 8xx: add cpm_get_cpmp()

[Dan Malek]

On Aug 7, 2005, at 11:44 AM, Aristeu Sergio Rozanski Filho wrote:

> what about don't cache it and call ioremap() from driver? (I guess
> ioremap() already check if an area is already mapped, no?)

Either way.  I'm actually leaning toward these "pointer helper"
functions. :-)  Something like get_cpmp(), or get_immr(), that will
hide the details of the mapping, so you don't have to include
and know which #defines to use as part of an ioremap() call.

It seems to be more clear to me, and I'm thinking about making
the same changes to the CPM2 drivers.  It also allows a
performance versus compact code trade off, declaring these
as inline functions or as real functions.

Thanks.

	-- Dan

Re: [PATCH] 8xx: add cpm_get_cpmp()

[Eugene Surovegin]

On Sun, Aug 07, 2005 at 12:57:31PM -0300, Marcelo Tosatti wrote:
> On Sun, Aug 07, 2005 at 12:44:32PM -0300, Aristeu Sergio Rozanski Filho wrote:
> > > It depends how you define the semantics of this function call.
> > > Can you call it any (and all of the) time you need it, or does it
> > > actually perform an ioremap() and you only want to call it
> > > once?
> > what about don't cache it and call ioremap() from driver? (I guess
> > ioremap() already check if an area is already mapped, no?)
> 
> Yep, ioremap() should be doing virtual address caching already, no?

In general, ioremap() doesn't do any caching currently. Some subarchs 
do some limited caching, e.g. if BATs (classic PPC) or CAMs (e500) are 
available and were used previously for ioremap() mappings. 

Some time ago I made a trivial ioremap cache patch (useful on 4xx, 
which doesn't have BATs nor CAMs), although it wass really a hack and 
it was never merged :).

-- 
Eugene