[PATCH] OMAP: Exporting functions doing common register access

[PATCH] OMAP: Exporting functions doing common register access

[Anuj Aggarwal]

These functions need to be exported so that drivers (e.g. McBSP) can
be configured as modules.

McBSP driver gets built as a module when ASoC driver for OMAP3 EVM
is configured as module. McBSP driver uses functions like
omap_ctrl_readl/omap_ctrl_writel, which are defined in control.c
file but not exported. Without that, McBSP driver fails to build as
a module.

Signed-off-by: Anuj Aggarwal <anuj.aggarwal@ti.com>
---
 arch/arm/mach-omap2/control.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
index 6adb360..8285e1f 100644
--- a/arch/arm/mach-omap2/control.c
+++ b/arch/arm/mach-omap2/control.c
@@ -36,29 +36,35 @@ u8 omap_ctrl_readb(u16 offset)
 {
 	return __raw_readb(OMAP_CTRL_REGADDR(offset));
 }
+EXPORT_SYMBOL(omap_ctrl_readb);
 
 u16 omap_ctrl_readw(u16 offset)
 {
 	return __raw_readw(OMAP_CTRL_REGADDR(offset));
 }
+EXPORT_SYMBOL(omap_ctrl_readw);
 
 u32 omap_ctrl_readl(u16 offset)
 {
 	return __raw_readl(OMAP_CTRL_REGADDR(offset));
 }
+EXPORT_SYMBOL(omap_ctrl_readl);
 
 void omap_ctrl_writeb(u8 val, u16 offset)
 {
 	__raw_writeb(val, OMAP_CTRL_REGADDR(offset));
 }
+EXPORT_SYMBOL(omap_ctrl_writeb);
 
 void omap_ctrl_writew(u16 val, u16 offset)
 {
 	__raw_writew(val, OMAP_CTRL_REGADDR(offset));
 }
+EXPORT_SYMBOL(omap_ctrl_writew);
 
 void omap_ctrl_writel(u32 val, u16 offset)
 {
 	__raw_writel(val, OMAP_CTRL_REGADDR(offset));
 }
+EXPORT_SYMBOL(omap_ctrl_writel);
 
-- 
1.6.2.4

Re: [PATCH] OMAP: Exporting functions doing common register access

[Paul Walmsley]

Hello Anuj,

On Mon, 16 Nov 2009, Anuj Aggarwal wrote:

> These functions need to be exported so that drivers (e.g. McBSP) can
> be configured as modules.
> 
> McBSP driver gets built as a module when ASoC driver for OMAP3 EVM
> is configured as module. McBSP driver uses functions like
> omap_ctrl_readl/omap_ctrl_writel, which are defined in control.c
> file but not exported. Without that, McBSP driver fails to build as
> a module.
> 
> Signed-off-by: Anuj Aggarwal <anuj.aggarwal@ti.com>

This will prevent the McBSP driver from being usable across chips.  For 
example, if a future OMAP comes out with McBSP clock source switching 
implemented in a different way, or if a DaVinci chip comes out with a 
McBSP block but no SCM block, this either won't build or won't run.

A better approach would be to move the OMAP2/3-specific functions from the 
McBSP driver into the OMAP McBSP integration code, 
arch/arm/*omap*/mcbsp.c, and to define function pointers in 
struct platform_data for these functions, and to pass them from the 
integration code into the device driver code.  That will make these 
drivers independent of chip integration details.


- Paul

RE: [PATCH] OMAP: Exporting functions doing common register access

[Aggarwal, Anuj]

> -----Original Message-----
> From: Paul Walmsley [mailto:paul@pwsan.com]
> Sent: Tuesday, November 17, 2009 2:46 AM
> To: Aggarwal, Anuj
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] OMAP: Exporting functions doing common register
> access
> 
> Hello Anuj,
> 
> On Mon, 16 Nov 2009, Anuj Aggarwal wrote:
> 
> > These functions need to be exported so that drivers (e.g. McBSP) can
> > be configured as modules.
> >
> > McBSP driver gets built as a module when ASoC driver for OMAP3 EVM
> > is configured as module. McBSP driver uses functions like
> > omap_ctrl_readl/omap_ctrl_writel, which are defined in control.c
> > file but not exported. Without that, McBSP driver fails to build as
> > a module.
> >
> > Signed-off-by: Anuj Aggarwal <anuj.aggarwal@ti.com>
> 
> This will prevent the McBSP driver from being usable across chips.  For
> example, if a future OMAP comes out with McBSP clock source switching
> implemented in a different way, or if a DaVinci chip comes out with a
> McBSP block but no SCM block, this either won't build or won't run.
> 
> A better approach would be to move the OMAP2/3-specific functions from the
> McBSP driver into the OMAP McBSP integration code,
> arch/arm/*omap*/mcbsp.c, and to define function pointers in
> struct platform_data for these functions, and to pass them from the
> integration code into the device driver code.  That will make these
> drivers independent of chip integration details.
Sorry to confuse you, I was talking about the platform specific ASoC-McBSP code for OMAP (file - sound/soc/omap/omap-mcbsp.c) platform. The McBSP
driver - both platform and machine specific - doesn't use the functions
exported by the patch.

Including alsa-devel list too...
> 
> 
> - Paul

RE: [PATCH] OMAP: Exporting functions doing common register access

[Aggarwal, Anuj]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Aggarwal, Anuj
> Sent: Tuesday, November 17, 2009 11:28 AM
> To: Paul Walmsley
> Cc: linux-omap@vger.kernel.org; alsa-devel@alsa-project.org
> Subject: RE: [PATCH] OMAP: Exporting functions doing common register
> access
> 
> > -----Original Message-----
> > From: Paul Walmsley [mailto:paul@pwsan.com]
> > Sent: Tuesday, November 17, 2009 2:46 AM
> > To: Aggarwal, Anuj
> > Cc: linux-omap@vger.kernel.org
> > Subject: Re: [PATCH] OMAP: Exporting functions doing common register
> > access
> >
> > Hello Anuj,
> >
> > On Mon, 16 Nov 2009, Anuj Aggarwal wrote:
> >
> > > These functions need to be exported so that drivers (e.g. McBSP) can
> > > be configured as modules.
> > >
> > > McBSP driver gets built as a module when ASoC driver for OMAP3 EVM
> > > is configured as module. McBSP driver uses functions like
> > > omap_ctrl_readl/omap_ctrl_writel, which are defined in control.c
> > > file but not exported. Without that, McBSP driver fails to build as
> > > a module.
> > >
> > > Signed-off-by: Anuj Aggarwal <anuj.aggarwal@ti.com>
> >
> > This will prevent the McBSP driver from being usable across chips.  For
> > example, if a future OMAP comes out with McBSP clock source switching
> > implemented in a different way, or if a DaVinci chip comes out with a
> > McBSP block but no SCM block, this either won't build or won't run.
> >
> > A better approach would be to move the OMAP2/3-specific functions from
> the
> > McBSP driver into the OMAP McBSP integration code,
> > arch/arm/*omap*/mcbsp.c, and to define function pointers in
> > struct platform_data for these functions, and to pass them from the
> > integration code into the device driver code.  That will make these
> > drivers independent of chip integration details.
> Sorry to confuse you, I was talking about the platform specific ASoC-McBSP
> code for OMAP (file - sound/soc/omap/omap-mcbsp.c) platform. The McBSP
> driver - both platform and machine specific - doesn't use the functions
> exported by the patch.
> 
> Including alsa-devel list too...
[Aggarwal, Anuj] Any comments on this patch? Do I have to re-work this?
> >
> >
> > - Paul
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] OMAP: Exporting functions doing common register access

[Paul Walmsley]

On Wed, 18 Nov 2009, Aggarwal, Anuj wrote:

> [Aggarwal, Anuj] Any comments on this patch? Do I have to re-work this?

Yes.  omap_ctrl_*() should not be exported, as mentioned earlier.

A lot of rework will be needed at some point in sound/soc/omap/ to get all 
of the chip- and board-integration details out of these files.  DMA 
channels, IRQs, board data, etc. should all go into arch/arm/*omap*.  
plat-omap/mcbsp.c needs to be moved to somewhere under drivers/ at some 
point, but the platform_device setup for it should be kept under 
arch/arm/*omap*.

In the interim, I would suggest that you remove the the clock source and 
receiver source change functions from omap-mcbsp.c, split them into OMAP1 
and OMAP2/3 variants, and place them into arch/arm/mach-omap*/mcbsp.c.  
Expand struct omap_mcbsp_ops to add function pointers to those functions.  
Call those from soc/omap-mcbsp.c via mcbsp->pdata->ops. That way you won't 
need those exports.

I don't understand how this code compiled on OMAP1 in any case, since it 
doesn't have a System Control Module.


- Paul

Re: [PATCH] OMAP: Exporting functions doing common register access

[Jarkko Nikula]

On Wed, 18 Nov 2009 03:40:40 -0700 (MST)
Paul Walmsley <paul@pwsan.com> wrote:

> In the interim, I would suggest that you remove the the clock source and 
> receiver source change functions from omap-mcbsp.c, split them into OMAP1 
> and OMAP2/3 variants, and place them into arch/arm/mach-omap*/mcbsp.c.  
> Expand struct omap_mcbsp_ops to add function pointers to those functions.  
> Call those from soc/omap-mcbsp.c via mcbsp->pdata->ops. That way you won't 
> need those exports.
> 
Paul: What's your opinnion, would it be possible or would it be wise to
handle these McBSP clock route setups with the clock framework instead?

Functions omap_mcbsp_dai_set_clks_src and omap_mcbsp_dai_set_rcvr_src
are basically just setting up the input clock for McBSP SRG or McBSP1
receiver.

> I don't understand how this code compiled on OMAP1 in any case, since it 
> doesn't have a System Control Module.
> 
OMAP1 can include control.h as well :-)

Access is anyway protected with the cpu_class_is_omap1().

-- 
Jarkko

Re: [PATCH] OMAP: Exporting functions doing common register access

[Paul Walmsley]

Hi Jarkko,

On Wed, 18 Nov 2009, Jarkko Nikula wrote:

> On Wed, 18 Nov 2009 03:40:40 -0700 (MST)
> Paul Walmsley <paul@pwsan.com> wrote:
> 
> > In the interim, I would suggest that you remove the the clock source and 
> > receiver source change functions from omap-mcbsp.c, split them into OMAP1 
> > and OMAP2/3 variants, and place them into arch/arm/mach-omap*/mcbsp.c.  
> > Expand struct omap_mcbsp_ops to add function pointers to those functions.  
> > Call those from soc/omap-mcbsp.c via mcbsp->pdata->ops. That way you won't 
> > need those exports.
> > 
> Paul: What's your opinnion, would it be possible or would it be wise to
> handle these McBSP clock route setups with the clock framework instead?
> 
> Functions omap_mcbsp_dai_set_clks_src and omap_mcbsp_dai_set_rcvr_src
> are basically just setting up the input clock for McBSP SRG or McBSP1
> receiver.

Sure.  There already should be some support for it in clock34xx.h, but I 
doubt anyone's tested it:

static struct clk mcbsp1_fck = {
        .name           = "mcbsp_fck",
        .ops            = &clkops_omap2_dflt_wait,
        .id             = 1,
        .init           = &omap2_init_clksel_parent,
        .enable_reg     = OMAP_CM_REGADDR(CORE_MOD, CM_FCLKEN1),
        .enable_bit     = OMAP3430_EN_MCBSP1_SHIFT,
        .clksel_reg     = OMAP343X_CTRL_REGADDR(OMAP2_CONTROL_DEVCONF0),
        .clksel_mask    = OMAP2_MCBSP1_CLKS_MASK,
        .clksel         = mcbsp_15_clksel,
        .clkdm_name     = "core_l4_clkdm",
        .recalc         = &omap2_clksel_recalc,
};

etc.  Some similar entries would need to be added to the clock24xx.h file.

> > I don't understand how this code compiled on OMAP1 in any case, since it 
> > doesn't have a System Control Module.
> > 
> OMAP1 can include control.h as well :-)

Yeah, I guess it is this stuff in control.h that makes it work:

#define omap_ctrl_base_get()            0
#define omap_ctrl_readb(x)              0
#define omap_ctrl_readw(x)              0
#define omap_ctrl_readl(x)              0
#define omap_ctrl_writeb(x, y)          WARN_ON(1)
#define omap_ctrl_writew(x, y)          WARN_ON(1)
#define omap_ctrl_writel(x, y)          WARN_ON(1)

Would be nice to get rid of that at some point.


- Paul

RE: [PATCH] OMAP: Exporting functions doing common register access

[Aggarwal, Anuj]

> > > In the interim, I would suggest that you remove the the clock source
> and
> > > receiver source change functions from omap-mcbsp.c, split them into
> OMAP1
> > > and OMAP2/3 variants, and place them into arch/arm/mach-omap*/mcbsp.c.
> > > Expand struct omap_mcbsp_ops to add function pointers to those
> functions.
> > > Call those from soc/omap-mcbsp.c via mcbsp->pdata->ops. That way you
> won't
> > > need those exports.
> > >
> > Paul: What's your opinnion, would it be possible or would it be wise to
> > handle these McBSP clock route setups with the clock framework instead?
> >
> > Functions omap_mcbsp_dai_set_clks_src and omap_mcbsp_dai_set_rcvr_src
> > are basically just setting up the input clock for McBSP SRG or McBSP1
> > receiver.
> 
> Sure.  There already should be some support for it in clock34xx.h, but I
> doubt anyone's tested it:
> 
> static struct clk mcbsp1_fck = {
>         .name           = "mcbsp_fck",
>         .ops            = &clkops_omap2_dflt_wait,
>         .id             = 1,
>         .init           = &omap2_init_clksel_parent,
>         .enable_reg     = OMAP_CM_REGADDR(CORE_MOD, CM_FCLKEN1),
>         .enable_bit     = OMAP3430_EN_MCBSP1_SHIFT,
>         .clksel_reg     = OMAP343X_CTRL_REGADDR(OMAP2_CONTROL_DEVCONF0),
>         .clksel_mask    = OMAP2_MCBSP1_CLKS_MASK,
>         .clksel         = mcbsp_15_clksel,
>         .clkdm_name     = "core_l4_clkdm",
>         .recalc         = &omap2_clksel_recalc,
> };
> 
> etc.  Some similar entries would need to be added to the clock24xx.h file.
[Aggarwal, Anuj] One problem which I found in trying the above discussed 
approach is that I am not able to get mcbsp clock handles in omap-mcbsp.c 
file. To call clk_set_parent() with the new parent clock as "mcbsp_clks" 
(in case of an external clock), I need the mcbsp_fck clock handle. But this
handle is not shared with the omap_mcbsp_dai[].

What should be the right method of getting this handle in 
omap_mcbsp_dai_set_dai_sysclk(), in case I understood the concept properly.
Basically, we want to get rid of the low level stuff in this function 
and instead use standard clock framework APIs?

Re: [PATCH] OMAP: Exporting functions doing common register access

[Aggarwal, Anuj]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Aggarwal, Anuj
> Sent: Thursday, January 07, 2010 7:40 PM
> To: Paul Walmsley; Jarkko Nikula
> Cc: linux-omap@vger.kernel.org; alsa-devel@alsa-project.org; Lohithakshan,
> Ranjith
> Subject: RE: [PATCH] OMAP: Exporting functions doing common register
> access
> 
> > > > In the interim, I would suggest that you remove the the clock source
> > and
> > > > receiver source change functions from omap-mcbsp.c, split them into
> > OMAP1
> > > > and OMAP2/3 variants, and place them into arch/arm/mach-
> omap*/mcbsp.c.
> > > > Expand struct omap_mcbsp_ops to add function pointers to those
> > functions.
> > > > Call those from soc/omap-mcbsp.c via mcbsp->pdata->ops. That way you
> > won't
> > > > need those exports.
> > > >
> > > Paul: What's your opinnion, would it be possible or would it be wise
> to
> > > handle these McBSP clock route setups with the clock framework
> instead?
> > >
> > > Functions omap_mcbsp_dai_set_clks_src and omap_mcbsp_dai_set_rcvr_src
> > > are basically just setting up the input clock for McBSP SRG or McBSP1
> > > receiver.
> >
> > Sure.  There already should be some support for it in clock34xx.h, but I
> > doubt anyone's tested it:
> >
> > static struct clk mcbsp1_fck = {
> >         .name           = "mcbsp_fck",
> >         .ops            = &clkops_omap2_dflt_wait,
> >         .id             = 1,
> >         .init           = &omap2_init_clksel_parent,
> >         .enable_reg     = OMAP_CM_REGADDR(CORE_MOD, CM_FCLKEN1),
> >         .enable_bit     = OMAP3430_EN_MCBSP1_SHIFT,
> >         .clksel_reg     = OMAP343X_CTRL_REGADDR(OMAP2_CONTROL_DEVCONF0),
> >         .clksel_mask    = OMAP2_MCBSP1_CLKS_MASK,
> >         .clksel         = mcbsp_15_clksel,
> >         .clkdm_name     = "core_l4_clkdm",
> >         .recalc         = &omap2_clksel_recalc,
> > };
> >
> > etc.  Some similar entries would need to be added to the clock24xx.h
> file.
> [Aggarwal, Anuj] One problem which I found in trying the above discussed
> approach is that I am not able to get mcbsp clock handles in omap-mcbsp.c
> file. To call clk_set_parent() with the new parent clock as "mcbsp_clks"
> (in case of an external clock), I need the mcbsp_fck clock handle. But
> this
> handle is not shared with the omap_mcbsp_dai[].
> 
> What should be the right method of getting this handle in
> omap_mcbsp_dai_set_dai_sysclk(), in case I understood the concept
> properly.
> Basically, we want to get rid of the low level stuff in this function
> and instead use standard clock framework APIs?

Paul / Jarkko,

Any comments/pointers please?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] OMAP: Exporting functions doing common register access

[Paul Walmsley]

Hello Anuj,

On Thu, 7 Jan 2010, Aggarwal, Anuj wrote:

> > > > In the interim, I would suggest that you remove the the clock source
> > and
> > > > receiver source change functions from omap-mcbsp.c, split them into
> > OMAP1
> > > > and OMAP2/3 variants, and place them into arch/arm/mach-omap*/mcbsp.c.
> > > > Expand struct omap_mcbsp_ops to add function pointers to those
> > functions.
> > > > Call those from soc/omap-mcbsp.c via mcbsp->pdata->ops. That way you
> > won't
> > > > need those exports.
> > > >
> > > Paul: What's your opinnion, would it be possible or would it be wise to
> > > handle these McBSP clock route setups with the clock framework instead?
> > >
> > > Functions omap_mcbsp_dai_set_clks_src and omap_mcbsp_dai_set_rcvr_src
> > > are basically just setting up the input clock for McBSP SRG or McBSP1
> > > receiver.
> > 
> > Sure.  There already should be some support for it in clock34xx.h, but I
> > doubt anyone's tested it:
> > 
> > static struct clk mcbsp1_fck = {
> >         .name           = "mcbsp_fck",
> >         .ops            = &clkops_omap2_dflt_wait,
> >         .id             = 1,
> >         .init           = &omap2_init_clksel_parent,
> >         .enable_reg     = OMAP_CM_REGADDR(CORE_MOD, CM_FCLKEN1),
> >         .enable_bit     = OMAP3430_EN_MCBSP1_SHIFT,
> >         .clksel_reg     = OMAP343X_CTRL_REGADDR(OMAP2_CONTROL_DEVCONF0),
> >         .clksel_mask    = OMAP2_MCBSP1_CLKS_MASK,
> >         .clksel         = mcbsp_15_clksel,
> >         .clkdm_name     = "core_l4_clkdm",
> >         .recalc         = &omap2_clksel_recalc,
> > };
> > 
> > etc.  Some similar entries would need to be added to the clock24xx.h file.
> [Aggarwal, Anuj] One problem which I found in trying the above discussed 
> approach is that I am not able to get mcbsp clock handles in omap-mcbsp.c 
> file. 

Why not?  Looks like there are init and exit functions in there.  Those 
would seem like logical places for clk_get()/clk_put().

> To call clk_set_parent() with the new parent clock as "mcbsp_clks" 
> (in case of an external clock), I need the mcbsp_fck clock handle. But this
> handle is not shared with the omap_mcbsp_dai[].

Have you considered static variable(s)?

> What should be the right method of getting this handle in 
> omap_mcbsp_dai_set_dai_sysclk(), in case I understood the concept properly.
> Basically, we want to get rid of the low level stuff in this function 
> and instead use standard clock framework APIs?

Yep, the contents of both the omap_mcbsp_dai_set_clks_src() function and 
the omap_mcbsp_dai_set_rcvr_src() should be implementable with standard 
clock framework APIs.


- Paul