[PATCH 1/2] OMAP: features: export symbol omap3_features

[PATCH 1/2] OMAP: features: export symbol omap3_features

[Omar Ramirez Luna]

From: Kevin Hilman <khilman@deeprootsystems.com>

Since omap3_features is a global variable, only code
built-in the kernel can make use of it and thus use
omap_has ##feat functions; exporting this as a kernel
symbol makes modules able to use feature detection
framework too.

Thread: http://marc.info/?l=linux-omap&m=128528822902211&w=2

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
CC: Tony Lindgren <tony@atomide.com>
CC: Russell King <linux@arm.linux.org.uk>
CC: Nishanth Menon <nm@ti.com>
CC: Paul Walmsley <paul@pwsan.com>
CC: Sanjeev Premi <premi@ti.com>
CC: linux-omap at vger.kernel.org
CC: linux-arm-kernel at lists.infradead.org
---
 arch/arm/mach-omap2/id.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
index 9a879f9..a8c6d19 100644
--- a/arch/arm/mach-omap2/id.c
+++ b/arch/arm/mach-omap2/id.c
@@ -31,6 +31,7 @@ static struct omap_chip_id omap_chip;
 static unsigned int omap_revision;
 
 u32 omap3_features;
+EXPORT_SYMBOL(omap3_features);
 
 unsigned int omap_rev(void)
 {
-- 
1.7.1

[PATCH 1/2] OMAP: features: export symbol omap3_features

[Kevin Hilman]

Omar Ramirez Luna <omar.ramirez@ti.com> writes:

> From: Kevin Hilman <khilman@deeprootsystems.com>

Minor nit, can you make the subject something like:

   OMAP: features: make feature checks available to modules

Thanks,

Kevin

> Since omap3_features is a global variable, only code
> built-in the kernel can make use of it and thus use
> omap_has ##feat functions; exporting this as a kernel
> symbol makes modules able to use feature detection
> framework too.
>
> Thread: http://marc.info/?l=linux-omap&m=128528822902211&w=2
>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
> CC: Tony Lindgren <tony@atomide.com>
> CC: Russell King <linux@arm.linux.org.uk>
> CC: Nishanth Menon <nm@ti.com>
> CC: Paul Walmsley <paul@pwsan.com>
> CC: Sanjeev Premi <premi@ti.com>
> CC: linux-omap at vger.kernel.org
> CC: linux-arm-kernel at lists.infradead.org
> ---
>  arch/arm/mach-omap2/id.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
> index 9a879f9..a8c6d19 100644
> --- a/arch/arm/mach-omap2/id.c
> +++ b/arch/arm/mach-omap2/id.c
> @@ -31,6 +31,7 @@ static struct omap_chip_id omap_chip;
>  static unsigned int omap_revision;
>  
>  u32 omap3_features;
> +EXPORT_SYMBOL(omap3_features);
>  
>  unsigned int omap_rev(void)
>  {

[PATCH 1/2] OMAP: features: export symbol omap3_features

[Paul Walmsley]

Hi Omar, Kevin,

On Thu, 23 Sep 2010, Omar Ramirez Luna wrote:

> From: Kevin Hilman <khilman@deeprootsystems.com>
> 
> Since omap3_features is a global variable, only code
> built-in the kernel can make use of it and thus use
> omap_has ##feat functions; exporting this as a kernel
> symbol makes modules able to use feature detection
> framework too.
> 
> Thread: http://marc.info/?l=linux-omap&m=128528822902211&w=2
> 
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
> CC: Tony Lindgren <tony@atomide.com>
> CC: Russell King <linux@arm.linux.org.uk>
> CC: Nishanth Menon <nm@ti.com>
> CC: Paul Walmsley <paul@pwsan.com>
> CC: Sanjeev Premi <premi@ti.com>
> CC: linux-omap at vger.kernel.org
> CC: linux-arm-kernel at lists.infradead.org
> ---
>  arch/arm/mach-omap2/id.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
> index 9a879f9..a8c6d19 100644
> --- a/arch/arm/mach-omap2/id.c
> +++ b/arch/arm/mach-omap2/id.c
> @@ -31,6 +31,7 @@ static struct omap_chip_id omap_chip;
>  static unsigned int omap_revision;
>  
>  u32 omap3_features;
> +EXPORT_SYMBOL(omap3_features);
>  
>  unsigned int omap_rev(void)
>  {
> -- 
> 1.7.1
> 

This omap3_features variable should not be used directly by any device 
drivers since it is an OMAP-ism.  This type of feature info should be 
passed through struct platform_data.  Looks like this would be quite easy 
to add by editing mach-omap2/devices.c and adding platform_data?

...

In the medium-term, definitely all of those 

    #if defined(CONFIG_ARCH_OMAP*)

and 

    if (cpu_is_omap*()) {

in this driver need to be removed.  The integration code (currently in 
mach-omap2/devices.c) is what should handle this, or better yet, struct 
dev_attr from hwmod.

Also the entire mailboxes driver at some point should be moved into 
drivers/* ...


- Paul

[PATCH 1/2] OMAP: features: export symbol omap3_features

[Nishanth Menon]

Paul Walmsley had written, on 09/24/2010 10:34 AM, the following:
[...]
>>  
>>  u32 omap3_features;
>> +EXPORT_SYMBOL(omap3_features);
>>  
>>  unsigned int omap_rev(void)
>>  {
>> -- 
>> 1.7.1
>>
> 
> This omap3_features variable should not be used directly by any device 
> drivers since it is an OMAP-ism.  This type of feature info should be 
> passed through struct platform_data.  Looks like this would be quite easy 
> to add by editing mach-omap2/devices.c and adding platform_data?
> 
> ...
> 
> In the medium-term, definitely all of those 
> 
>     #if defined(CONFIG_ARCH_OMAP*)
> 
> and 
> 
>     if (cpu_is_omap*()) {
> 
> in this driver need to be removed.  The integration code (currently in 
> mach-omap2/devices.c) is what should handle this, or better yet, struct 
> dev_attr from hwmod.
> 
> Also the entire mailboxes driver at some point should be moved into 
> drivers/* ...
just my 2 cents:

The intent of omap_has_featureX() is to ensure that the drivers dont do 
cpu_is_omap123(). Now if we had OMAP dma driver which has DMA chaining - 
what options do we have DMA driver?

a) we introduce it based on cpu_is_omap123() -> bad bad nightmare for 
maintenance
b) we introduce it based on module h/w block(TI internal terminology "IP 
block") revision -> unfortunately no luck in some of the h/w blocks.
c) we use if (omap_has_dma_chaining())

If the hwmod itself was expanded to contain feature, and erratas then it 
is excellent solution - one stop information source for all drivers and 
a standardized soln as well.. (but we dont have it on omap1).

by not exporting this, the drivers are restricted to being static, and 
exporting allows those omap specific ones to be loadable (if someone 
really wants so minimalist a system - i believe i2c is an example of 
loadable module)..

I believe omap_has_feature() is a readable and maintainable way of 
handling features and exporting helps those drivers to become modules 
(yes with the risk of some other driver misusing it as well) - this 
definitely is something we've been doing for some time in l-o I guess.. 
a better alternative might be hwmod - but I am no expert to comment on 
it's feasibility..

-- 
Regards,
Nishanth Menon

[PATCH 1/2] OMAP: features: export symbol omap3_features

[Paul Walmsley]

On Fri, 24 Sep 2010, Nishanth Menon wrote:

> Paul Walmsley had written, on 09/24/2010 10:34 AM, the following:
> [...]
> > >   u32 omap3_features;
> > > +EXPORT_SYMBOL(omap3_features);
> > >   unsigned int omap_rev(void)
> > >  {
> > > -- 
> > > 1.7.1
> > > 
> > 
> > This omap3_features variable should not be used directly by any device
> > drivers since it is an OMAP-ism.  This type of feature info should be passed
> > through struct platform_data.  Looks like this would be quite easy to add by
> > editing mach-omap2/devices.c and adding platform_data?
> > 
> > ...
> > 
> > In the medium-term, definitely all of those 
> >     #if defined(CONFIG_ARCH_OMAP*)
> > 
> > and 
> >     if (cpu_is_omap*()) {
> > 
> > in this driver need to be removed.  The integration code (currently in
> > mach-omap2/devices.c) is what should handle this, or better yet, struct
> > dev_attr from hwmod.
> > 
> > Also the entire mailboxes driver at some point should be moved into
> > drivers/* ...
> just my 2 cents:
> 
> The intent of omap_has_featureX() is to ensure that the drivers dont do
> cpu_is_omap123(). Now if we had OMAP dma driver which has DMA chaining - what
> options do we have DMA driver?
> 
> a) we introduce it based on cpu_is_omap123() -> bad bad nightmare for
> maintenance
> b) we introduce it based on module h/w block(TI internal terminology "IP
> block") revision -> unfortunately no luck in some of the h/w blocks.
> c) we use if (omap_has_dma_chaining())

d) you pass in errata/feature flags via a platform_data struct, like 
McBSP, McSPI, MMC, MUSB, I2C, etc. already do on OMAP.  On OMAP1, which 
doesn't have hwmod support, you set your platform_data in your OMAP1 
integration code.  On OMAP2+ (which has hwmod support), you define your 
errata/feature flags and any other integration data that you need to pass 
via the struct omap_hwmod.dev_attr field, then pass that data via struct 
platform_data in the OMAP2+ integration code.  See for example

http://www.mail-archive.com/linux-omap at vger.kernel.org/msg13288.html

and

http://marc.info/?l=linux-omap&m=124419789124570&w=2

(search for "dev_attr" in both cases)

...

As an aside, I think that any errata/features that can be automatically 
discriminated by the IP block revision register are better handled that 
way than by platform_data; but of course, as you mention, the hardware 
people sometimes neglect to bump that when they change something.


- Paul

[PATCH 1/2] OMAP: features: export symbol omap3_features

[Paul Walmsley]

On Fri, 24 Sep 2010, Paul Walmsley wrote:

> On Fri, 24 Sep 2010, Nishanth Menon wrote:
> 
> > The intent of omap_has_featureX() is to ensure that the drivers dont do
> > cpu_is_omap123(). Now if we had OMAP dma driver which has DMA chaining - what
> > options do we have DMA driver?
> > 
> > a) we introduce it based on cpu_is_omap123() -> bad bad nightmare for
> > maintenance
> > b) we introduce it based on module h/w block(TI internal terminology "IP
> > block") revision -> unfortunately no luck in some of the h/w blocks.
> > c) we use if (omap_has_dma_chaining())
> 
> d) you pass in errata/feature flags via a platform_data struct, like 
> McBSP, McSPI, MMC, MUSB, I2C, etc. already do on OMAP.  On OMAP1, which 
> doesn't have hwmod support, you set your platform_data in your OMAP1 
> integration code.  On OMAP2+ (which has hwmod support), you define your 
> errata/feature flags and any other integration data that you need to pass 
> via the struct omap_hwmod.dev_attr field, then pass that data via struct 
> platform_data in the OMAP2+ integration code.

Just to clarify something that may be unclear: there's no problem with 
calling cpu_is_omapXXXX(), or any other OMAP core-specific function, from 
the OMAP<->driver integration code, living in arch/arm/*omap*.  The 
results of those functions can then be passed through platform_data.  But 
there is a problem with calling OMAP core-specific functions directly from 
the driver code itself, since driver code should be completely 
platform-independent -- e.g., the same DMA controller could exist on OMAP, 
DaVinci, etc.


- Paul

[PATCH 1/2] OMAP: features: export symbol omap3_features

[Nishanth Menon]

Paul Walmsley had written, on 09/24/2010 12:17 PM, the following:
> On Fri, 24 Sep 2010, Paul Walmsley wrote:
> 
>> On Fri, 24 Sep 2010, Nishanth Menon wrote:
>>
>>> The intent of omap_has_featureX() is to ensure that the drivers dont do
>>> cpu_is_omap123(). Now if we had OMAP dma driver which has DMA chaining - what
>>> options do we have DMA driver?
>>>
>>> a) we introduce it based on cpu_is_omap123() -> bad bad nightmare for
>>> maintenance
>>> b) we introduce it based on module h/w block(TI internal terminology "IP
>>> block") revision -> unfortunately no luck in some of the h/w blocks.
>>> c) we use if (omap_has_dma_chaining())
>> d) you pass in errata/feature flags via a platform_data struct, like 
>> McBSP, McSPI, MMC, MUSB, I2C, etc. already do on OMAP.  On OMAP1, which 
>> doesn't have hwmod support, you set your platform_data in your OMAP1 
>> integration code.  On OMAP2+ (which has hwmod support), you define your 
>> errata/feature flags and any other integration data that you need to pass 
>> via the struct omap_hwmod.dev_attr field, then pass that data via struct 
>> platform_data in the OMAP2+ integration code.
> 
> Just to clarify something that may be unclear: there's no problem with 
> calling cpu_is_omapXXXX(), or any other OMAP core-specific function, from 
> the OMAP<->driver integration code, living in arch/arm/*omap*.  The 
> results of those functions can then be passed through platform_data.  But 
> there is a problem with calling OMAP core-specific functions directly from 
> the driver code itself, since driver code should be completely 
> platform-independent -- e.g., the same DMA controller could exist on OMAP, 
> DaVinci, etc.
> 
yep. your comment makes sense now. thanks for clarifying. NAK from me as 
well.

-- 
Regards,
Nishanth Menon