[RFC 0/5] Introduce omap_has_feature

[RFC 0/5] Introduce omap_has_feature

[Tony Lindgren]

Hi,

Here's a quick experimental patches on testing features
with omap_has_feature as discussed based on earlier patches
by Nishant Menon.

These still need to be thought through a bit on how we want
to implement it.

Regards,

Tony

---

Nishanth Menon (1):
      omap2/3: id: fix sparse warning

Tony Lindgren (4):
      omap: Implement common omap_has_feature
      omap: Replace omap3_has_ macros with omap_has_feature
      omap: Remove old omap3_has_ macros
      omap: Allow testing for omap type with omap_has_feature


 arch/arm/mach-omap2/clock3xxx_data.c  |    2 -
 arch/arm/mach-omap2/id.c              |   70 +++++++++++++++++++++++----------
 arch/arm/mach-omap2/pm34xx.c          |    2 -
 arch/arm/plat-omap/common.c           |   15 +++++++
 arch/arm/plat-omap/include/plat/cpu.h |   69 ++++++++++++++++++---------------
 5 files changed, 104 insertions(+), 54 deletions(-)

-- 
Signature

[PATCH 1/5] omap2/3: id: fix sparse warning

[Tony Lindgren]

From: Nishanth Menon <nm@ti.com>

omap24xx_check_revision, omap3_check_features,
omap3_check_revision, omap4_check_revision, omap3_cpuinfo
are not used elsewhere, it should be static

Also fixes the following sparse warnings:
arch/arm/mach-omap2/id.c:105:13: warning: symbol 'omap24xx_check_revision' was not declared. Should it be static?
arch/arm/mach-omap2/id.c:167:13: warning: symbol 'omap3_check_features' was not declared. Should it be static?
arch/arm/mach-omap2/id.c:189:13: warning: symbol 'omap3_check_revision' was not declared. Should it be static?
arch/arm/mach-omap2/id.c:270:13: warning: symbol 'omap4_check_revision' was not declared. Should it be static?
arch/arm/mach-omap2/id.c:300:13: warning: symbol 'omap3_cpuinfo' was not declared. Should it be static?

Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/mach-omap2/id.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
index 16dbb9e..fd1904b 100644
--- a/arch/arm/mach-omap2/id.c
+++ b/arch/arm/mach-omap2/id.c
@@ -112,7 +112,7 @@ void omap_get_die_id(struct omap_die_id *odi)
 	odi->id_3 = read_tap_reg(OMAP_TAP_DIE_ID_3);
 }
 
-void __init omap24xx_check_revision(void)
+static void __init omap24xx_check_revision(void)
 {
 	int i, j;
 	u32 idcode, prod_id;
@@ -172,7 +172,7 @@ void __init omap24xx_check_revision(void)
 		omap3_features |= OMAP3_HAS_ ##feat;			\
 	}
 
-void __init omap3_check_features(void)
+static void __init omap3_check_features(void)
 {
 	u32 status;
 
@@ -196,7 +196,7 @@ void __init omap3_check_features(void)
 	 */
 }
 
-void __init omap3_check_revision(void)
+static void __init omap3_check_revision(void)
 {
 	u32 cpuid, idcode;
 	u16 hawkeye;
@@ -277,7 +277,7 @@ void __init omap3_check_revision(void)
 	}
 }
 
-void __init omap4_check_revision(void)
+static void __init omap4_check_revision(void)
 {
 	u32 idcode;
 	u16 hawkeye;
@@ -307,7 +307,7 @@ void __init omap4_check_revision(void)
 	if (omap3_has_ ##feat())		\
 		printk(#feat" ");
 
-void __init omap3_cpuinfo(void)
+static void __init omap3_cpuinfo(void)
 {
 	u8 rev = GET_OMAP_REVISION();
 	char cpu_name[16], cpu_rev[16];

[PATCH 2/5] omap: Implement common omap_has_feature

[Tony Lindgren]

Implement common omap_has_feature.

Intended to replace omap3_has_ functions

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/mach-omap2/id.c              |   32 ++++++++++++++++++++++++++++++++
 arch/arm/plat-omap/common.c           |   15 +++++++++++++++
 arch/arm/plat-omap/include/plat/cpu.h |    3 +++
 3 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
index fd1904b..a2e5965 100644
--- a/arch/arm/mach-omap2/id.c
+++ b/arch/arm/mach-omap2/id.c
@@ -29,7 +29,9 @@
 
 static struct omap_chip_id omap_chip;
 static unsigned int omap_revision;
+static u32 omap_features;
 
+/* REVISIT: Get rid of omap3_features */
 u32 omap3_features;
 
 unsigned int omap_rev(void)
@@ -112,6 +114,12 @@ void omap_get_die_id(struct omap_die_id *odi)
 	odi->id_3 = read_tap_reg(OMAP_TAP_DIE_ID_3);
 }
 
+u32 omap2_has_feature(u32 feat_mask)
+{
+	/* REVISIT: Add necessary omap2 feature tests here */
+	return ((feat_mask & omap_features) == feat_mask);
+}
+
 static void __init omap24xx_check_revision(void)
 {
 	int i, j;
@@ -164,6 +172,15 @@ static void __init omap24xx_check_revision(void)
 	if ((omap_rev() >> 8) & 0x0f)
 		pr_info("ES%x", (omap_rev() >> 12) & 0xf);
 	pr_info("\n");
+
+	omap_features = 0;
+	omap_init_features(omap2_has_feature);
+}
+
+u32 omap3_has_feature(u32 feat_mask)
+{
+	/* REVISIT: Add necessary omap3 feature tests here */
+	return ((feat_mask & omap_features) == feat_mask);
 }
 
 #define OMAP3_CHECK_FEATURE(status,feat)				\
@@ -194,6 +211,11 @@ static void __init omap3_check_features(void)
 	 * TODO: Get additional info (where applicable)
 	 *       e.g. Size of L2 cache.
 	 */
+
+	/* REVISIT: Get rid of omap3_features */
+	omap_features = omap3_features;
+
+	omap_init_features(omap3_has_feature);
 }
 
 static void __init omap3_check_revision(void)
@@ -277,6 +299,12 @@ static void __init omap3_check_revision(void)
 	}
 }
 
+u32 omap4_has_feature(u32 feat_mask)
+{
+	/* REVISIT: Add necessary omap4 feature tests here */
+	return ((feat_mask & omap_features) == feat_mask);
+}
+
 static void __init omap4_check_revision(void)
 {
 	u32 idcode;
@@ -297,6 +325,10 @@ static void __init omap4_check_revision(void)
 		omap_revision = OMAP4430_REV_ES1_0;
 		omap_chip.oc |= CHIP_IS_OMAP4430ES1;
 		pr_info("OMAP%04x %s\n", omap_rev() >> 16, rev_name);
+
+		omap_features = 0;
+		omap_init_features(omap4_has_feature);
+
 		return;
 	}
 
diff --git a/arch/arm/plat-omap/common.c b/arch/arm/plat-omap/common.c
index 893a53a..d00b242 100644
--- a/arch/arm/plat-omap/common.c
+++ b/arch/arm/plat-omap/common.c
@@ -89,6 +89,21 @@ void __init omap_reserve(void)
 	omap_vram_reserve_sdram_lmb();
 }
 
+static int (*_omap_check_feature)(u32 feat_mask);
+
+u32 omap_has_feature(u32 feat_mask)
+{
+	if (!_omap_check_feature)
+		return 0;
+
+	return _omap_check_feature(feat_mask);
+}
+
+void __init omap_init_features(u32 (*check_feature)(u32 feat))
+{
+	_omap_check_feature = check_feature;
+}
+
 /*
  * 32KHz clocksource ... always available, on pretty most chips except
  * OMAP 730 and 1510.  Other timers could be used as clocksources, with
diff --git a/arch/arm/plat-omap/include/plat/cpu.h b/arch/arm/plat-omap/include/plat/cpu.h
index aa2f4f0..127df06 100644
--- a/arch/arm/plat-omap/include/plat/cpu.h
+++ b/arch/arm/plat-omap/include/plat/cpu.h
@@ -49,6 +49,9 @@ struct omap_chip_id {
 	u8 type;
 };
 
+u32 omap_has_feature(u32 feat_mask);
+void omap_init_features(u32 (*check_feature)(u32 feat));
+
 #define OMAP_CHIP_INIT(x)	{ .oc = x }
 
 /*

Re: [PATCH 2/5] omap: Implement common omap_has_feature

[Nishanth Menon]

Tony Lindgren had written, on 07/08/2010 04:37 AM, the following:
> Implement common omap_has_feature.
> 
> Intended to replace omap3_has_ functions
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  arch/arm/mach-omap2/id.c              |   32 ++++++++++++++++++++++++++++++++
>  arch/arm/plat-omap/common.c           |   15 +++++++++++++++
>  arch/arm/plat-omap/include/plat/cpu.h |    3 +++
>  3 files changed, 50 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
> index fd1904b..a2e5965 100644
> --- a/arch/arm/mach-omap2/id.c
> +++ b/arch/arm/mach-omap2/id.c
> @@ -29,7 +29,9 @@
>  
>  static struct omap_chip_id omap_chip;
>  static unsigned int omap_revision;
> +static u32 omap_features;
>  
> +/* REVISIT: Get rid of omap3_features */
>  u32 omap3_features;
>  
>  unsigned int omap_rev(void)
> @@ -112,6 +114,12 @@ void omap_get_die_id(struct omap_die_id *odi)
>  	odi->id_3 = read_tap_reg(OMAP_TAP_DIE_ID_3);
>  }
>  
> +u32 omap2_has_feature(u32 feat_mask)
> +{
> +	/* REVISIT: Add necessary omap2 feature tests here */
> +	return ((feat_mask & omap_features) == feat_mask);
> +}
> +
I did consider this path initially,
a) Additional functional call overhead here. some of the calls to 
has_feature() will get called through pretty active paths, we would like 
it to be minimized to compile time optimized inline function as much as 
possible.(no reason why this cant me a inline macro in cpu.h?) -
the original series received a similar comment:
http://marc.info/?l=linux-omap&m=125018002127428&w=2

>  static void __init omap24xx_check_revision(void)
>  {
>  	int i, j;
> @@ -164,6 +172,15 @@ static void __init omap24xx_check_revision(void)
>  	if ((omap_rev() >> 8) & 0x0f)
>  		pr_info("ES%x", (omap_rev() >> 12) & 0xf);
>  	pr_info("\n");
> +
> +	omap_features = 0;
> +	omap_init_features(omap2_has_feature);
> +}
> +
> +u32 omap3_has_feature(u32 feat_mask)
> +{
> +	/* REVISIT: Add necessary omap3 feature tests here */
> +	return ((feat_mask & omap_features) == feat_mask);
>  }
>  
>  #define OMAP3_CHECK_FEATURE(status,feat)				\
> @@ -194,6 +211,11 @@ static void __init omap3_check_features(void)
>  	 * TODO: Get additional info (where applicable)
>  	 *       e.g. Size of L2 cache.
>  	 */
> +
> +	/* REVISIT: Get rid of omap3_features */
> +	omap_features = omap3_features;
> +
> +	omap_init_features(omap3_has_feature);
>  }
>  
>  static void __init omap3_check_revision(void)
> @@ -277,6 +299,12 @@ static void __init omap3_check_revision(void)
>  	}
>  }
>  
> +u32 omap4_has_feature(u32 feat_mask)
> +{
> +	/* REVISIT: Add necessary omap4 feature tests here */
> +	return ((feat_mask & omap_features) == feat_mask);
> +}
> +
>  static void __init omap4_check_revision(void)
>  {
>  	u32 idcode;
> @@ -297,6 +325,10 @@ static void __init omap4_check_revision(void)
>  		omap_revision = OMAP4430_REV_ES1_0;
>  		omap_chip.oc |= CHIP_IS_OMAP4430ES1;
>  		pr_info("OMAP%04x %s\n", omap_rev() >> 16, rev_name);
> +
> +		omap_features = 0;
> +		omap_init_features(omap4_has_feature);
> +
>  		return;
>  	}
>  
> diff --git a/arch/arm/plat-omap/common.c b/arch/arm/plat-omap/common.c
> index 893a53a..d00b242 100644
> --- a/arch/arm/plat-omap/common.c
> +++ b/arch/arm/plat-omap/common.c
> @@ -89,6 +89,21 @@ void __init omap_reserve(void)
>  	omap_vram_reserve_sdram_lmb();
>  }
>  
> +static int (*_omap_check_feature)(u32 feat_mask);
> +
> +u32 omap_has_feature(u32 feat_mask)
> +{
> +	if (!_omap_check_feature)
> +		return 0;
> +
> +	return _omap_check_feature(feat_mask);
> +}
> +
> +void __init omap_init_features(u32 (*check_feature)(u32 feat))
> +{
> +	_omap_check_feature = check_feature;
> +}
> +
>  /*
>   * 32KHz clocksource ... always available, on pretty most chips except
>   * OMAP 730 and 1510.  Other timers could be used as clocksources, with
> diff --git a/arch/arm/plat-omap/include/plat/cpu.h b/arch/arm/plat-omap/include/plat/cpu.h
> index aa2f4f0..127df06 100644
> --- a/arch/arm/plat-omap/include/plat/cpu.h
> +++ b/arch/arm/plat-omap/include/plat/cpu.h
> @@ -49,6 +49,9 @@ struct omap_chip_id {
>  	u8 type;
>  };
>  
> +u32 omap_has_feature(u32 feat_mask);
the above crib -> it is better as an static inline function instead of 
explicit function call.

> +void omap_init_features(u32 (*check_feature)(u32 feat));
> +
>  #define OMAP_CHIP_INIT(x)	{ .oc = x }
>  
>  /*
> 


-- 
Regards,
Nishanth Menon

Re: [PATCH 2/5] omap: Implement common omap_has_feature

[Tony Lindgren]

* Nishanth Menon <nm@ti.com> [100708 17:46]:
> Tony Lindgren had written, on 07/08/2010 04:37 AM, the following:
> >
> >@@ -112,6 +114,12 @@ void omap_get_die_id(struct omap_die_id *odi)
> > 	odi->id_3 = read_tap_reg(OMAP_TAP_DIE_ID_3);
> > }
> >+u32 omap2_has_feature(u32 feat_mask)
> >+{
> >+	/* REVISIT: Add necessary omap2 feature tests here */
> >+	return ((feat_mask & omap_features) == feat_mask);
> >+}
> >+
>
> I did consider this path initially,
> a) Additional functional call overhead here. some of the calls to
> has_feature() will get called through pretty active paths, we would
> like it to be minimized to compile time optimized inline function as
> much as possible.(no reason why this cant me a inline macro in
> cpu.h?) -

Well it should not matter how slow this code is, it should only be
used to initialize things during __init.

Any code doing detection after initialization is wrong.

Regards,

Tony

Re: [PATCH 2/5] omap: Implement common omap_has_feature

[Nishanth Menon]

Tony Lindgren had written, on 07/09/2010 02:08 AM, the following:
> * Nishanth Menon <nm@ti.com> [100708 17:46]:
>> Tony Lindgren had written, on 07/08/2010 04:37 AM, the following:
>>> @@ -112,6 +114,12 @@ void omap_get_die_id(struct omap_die_id *odi)
>>> 	odi->id_3 = read_tap_reg(OMAP_TAP_DIE_ID_3);
>>> }
>>> +u32 omap2_has_feature(u32 feat_mask)
>>> +{
>>> +	/* REVISIT: Add necessary omap2 feature tests here */
>>> +	return ((feat_mask & omap_features) == feat_mask);
>>> +}
>>> +
>> I did consider this path initially,
>> a) Additional functional call overhead here. some of the calls to
>> has_feature() will get called through pretty active paths, we would
>> like it to be minimized to compile time optimized inline function as
>> much as possible.(no reason why this cant me a inline macro in
>> cpu.h?) -
> 
> Well it should not matter how slow this code is, it should only be
> used to initialize things during __init.
> 
> Any code doing detection after initialization is wrong.
couple of reasons why this may not make sense:
a) drivers can be modules -e.g. lets say dss driver would like to know 
if vrfb OR tiler is used -> __init wont make sense
b) has_feature could be used as part of data flow to make decision what 
is to be done.

e.g.

lets say dma chaining as a feature OR dma list as a feature, we should 
allow for usage such as the following:
int chain_dma(...)
{
	if (!omap_has_feature(OMAP_HAS_DMA_CHAINING)) {
		return -EINVAL;
	}
}

this means that we would prefer these kind of path to be high 
performance paths (data paths) - aka inline functions make more sense 
than incurring a function call overhead.

-- 
Regards,
Nishanth Menon

[PATCH 3/5] omap: Replace omap3_has_ macros with omap_has_feature

[Tony Lindgren]

Replace omap3_has_ macros with omap_has_feature

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/mach-omap2/clock3xxx_data.c  |    2 +-
 arch/arm/mach-omap2/id.c              |   22 +++++++++++-----------
 arch/arm/mach-omap2/pm34xx.c          |    2 +-
 arch/arm/plat-omap/include/plat/cpu.h |   12 ++++++------
 4 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-omap2/clock3xxx_data.c b/arch/arm/mach-omap2/clock3xxx_data.c
index c226798..2d2248f 100644
--- a/arch/arm/mach-omap2/clock3xxx_data.c
+++ b/arch/arm/mach-omap2/clock3xxx_data.c
@@ -3435,7 +3435,7 @@ int __init omap3xxx_clk_init(void)
 		cpu_clkflg |= CK_3505;
 	}
 
-	if (omap3_has_192mhz_clk())
+	if (omap_has_feature(OMAP3_HAS_192MHZ_CLK))
 		omap_96m_alwon_fck = omap_96m_alwon_fck_3630;
 
 	if (cpu_is_omap3630()) {
diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
index a2e5965..11184cf 100644
--- a/arch/arm/mach-omap2/id.c
+++ b/arch/arm/mach-omap2/id.c
@@ -336,7 +336,7 @@ static void __init omap4_check_revision(void)
 }
 
 #define OMAP3_SHOW_FEATURE(feat)		\
-	if (omap3_has_ ##feat())		\
+	if (omap_has_feature(feat))		\
 		printk(#feat" ");
 
 static void __init omap3_cpuinfo(void)
@@ -356,20 +356,20 @@ static void __init omap3_cpuinfo(void)
 		/*
 		 * AM35xx devices
 		 */
-		if (omap3_has_sgx()) {
+		if (omap_has_feature(OMAP3_HAS_SGX)) {
 			omap_revision = OMAP3517_REV(rev);
 			strcpy(cpu_name, "AM3517");
 		} else {
 			/* Already set in omap3_check_revision() */
 			strcpy(cpu_name, "AM3505");
 		}
-	} else if (omap3_has_iva() && omap3_has_sgx()) {
+	} else if (omap_has_feature(OMAP3_HAS_IVA | OMAP3_HAS_SGX)) {
 		/* OMAP3430, OMAP3525, OMAP3515, OMAP3503 devices */
 		strcpy(cpu_name, "OMAP3430/3530");
-	} else if (omap3_has_iva()) {
+	} else if (omap_has_feature(OMAP3_HAS_IVA)) {
 		omap_revision = OMAP3525_REV(rev);
 		strcpy(cpu_name, "OMAP3525");
-	} else if (omap3_has_sgx()) {
+	} else if (omap_has_feature(OMAP3_HAS_SGX)) {
 		omap_revision = OMAP3515_REV(rev);
 		strcpy(cpu_name, "OMAP3515");
 	} else {
@@ -400,12 +400,12 @@ static void __init omap3_cpuinfo(void)
 	/* Print verbose information */
 	pr_info("%s ES%s (", cpu_name, cpu_rev);
 
-	OMAP3_SHOW_FEATURE(l2cache);
-	OMAP3_SHOW_FEATURE(iva);
-	OMAP3_SHOW_FEATURE(sgx);
-	OMAP3_SHOW_FEATURE(neon);
-	OMAP3_SHOW_FEATURE(isp);
-	OMAP3_SHOW_FEATURE(192mhz_clk);
+	OMAP3_SHOW_FEATURE(OMAP3_HAS_L2CACHE);
+	OMAP3_SHOW_FEATURE(OMAP3_HAS_IVA);
+	OMAP3_SHOW_FEATURE(OMAP3_HAS_SGX);
+	OMAP3_SHOW_FEATURE(OMAP3_HAS_NEON);
+	OMAP3_SHOW_FEATURE(OMAP3_HAS_ISP);
+	OMAP3_SHOW_FEATURE(OMAP3_HAS_192MHZ_CLK);
 
 	printk(")\n");
 }
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index fb4994a..32e1005 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -385,7 +385,7 @@ void omap_sram_idle(void)
 	/* Enable IO-PAD and IO-CHAIN wakeups */
 	per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
 	core_next_state = pwrdm_read_next_pwrst(core_pwrdm);
-	if (omap3_has_io_wakeup() && \
+	if (omap_has_feature(OMAP3_HAS_IO_WAKEUP) &&		\
 			(per_next_state < PWRDM_POWER_ON ||
 			core_next_state < PWRDM_POWER_ON)) {
 		prm_set_mod_reg_bits(OMAP3430_EN_IO_MASK, WKUP_MOD, PM_WKEN);
diff --git a/arch/arm/plat-omap/include/plat/cpu.h b/arch/arm/plat-omap/include/plat/cpu.h
index 127df06..efee323 100644
--- a/arch/arm/plat-omap/include/plat/cpu.h
+++ b/arch/arm/plat-omap/include/plat/cpu.h
@@ -334,14 +334,14 @@ IS_OMAP_TYPE(3517, 0x3517)
 # undef cpu_is_omap3517
 # define cpu_is_omap3430()		is_omap3430()
 # define cpu_is_omap3503()		(cpu_is_omap3430() &&		\
-						(!omap3_has_iva()) &&	\
-						(!omap3_has_sgx()))
+					 (!omap_has_feature(OMAP3_HAS_IVA) &&	\
+						(!omap_has_feature(OMAP3_HAS_SGX)))
 # define cpu_is_omap3515()		(cpu_is_omap3430() &&		\
-						(!omap3_has_iva()) &&	\
-						(omap3_has_sgx()))
+						(!omap_has_feature(OMAP3_HAS_IVA)) &&	\
+						(omap_has_feature(OMAP3_HAS_SGX)))
 # define cpu_is_omap3525()		(cpu_is_omap3430() &&		\
-						(!omap3_has_sgx()) &&	\
-						(omap3_has_iva()))
+						(!omap_has_feature(OMAP3_HAS_SGX)) &&	\
+						(omap_has_feature(OMAP3_HAS_IVA)))
 # define cpu_is_omap3530()		(cpu_is_omap3430())
 # define cpu_is_omap3505()		is_omap3505()
 # define cpu_is_omap3517()		is_omap3517()

Re: [PATCH 3/5] omap: Replace omap3_has_ macros with omap_has_feature

[Nishanth Menon]

Tony Lindgren had written, on 07/08/2010 04:37 AM, the following:
> Replace omap3_has_ macros with omap_has_feature
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  arch/arm/mach-omap2/clock3xxx_data.c  |    2 +-
>  arch/arm/mach-omap2/id.c              |   22 +++++++++++-----------
>  arch/arm/mach-omap2/pm34xx.c          |    2 +-
>  arch/arm/plat-omap/include/plat/cpu.h |   12 ++++++------
>  4 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/clock3xxx_data.c b/arch/arm/mach-omap2/clock3xxx_data.c
> index c226798..2d2248f 100644
> --- a/arch/arm/mach-omap2/clock3xxx_data.c
> +++ b/arch/arm/mach-omap2/clock3xxx_data.c
> @@ -3435,7 +3435,7 @@ int __init omap3xxx_clk_init(void)
>  		cpu_clkflg |= CK_3505;
>  	}
>  
> -	if (omap3_has_192mhz_clk())
> +	if (omap_has_feature(OMAP3_HAS_192MHZ_CLK))
>  		omap_96m_alwon_fck = omap_96m_alwon_fck_3630;
>  
>  	if (cpu_is_omap3630()) {
> diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
> index a2e5965..11184cf 100644
> --- a/arch/arm/mach-omap2/id.c
> +++ b/arch/arm/mach-omap2/id.c
> @@ -336,7 +336,7 @@ static void __init omap4_check_revision(void)
>  }
>  
>  #define OMAP3_SHOW_FEATURE(feat)		\
> -	if (omap3_has_ ##feat())		\
> +	if (omap_has_feature(feat))		\
>  		printk(#feat" ");
>  
>  static void __init omap3_cpuinfo(void)
> @@ -356,20 +356,20 @@ static void __init omap3_cpuinfo(void)
>  		/*
>  		 * AM35xx devices
>  		 */
> -		if (omap3_has_sgx()) {
> +		if (omap_has_feature(OMAP3_HAS_SGX)) {
>  			omap_revision = OMAP3517_REV(rev);
>  			strcpy(cpu_name, "AM3517");
>  		} else {
>  			/* Already set in omap3_check_revision() */
>  			strcpy(cpu_name, "AM3505");
>  		}
> -	} else if (omap3_has_iva() && omap3_has_sgx()) {
> +	} else if (omap_has_feature(OMAP3_HAS_IVA | OMAP3_HAS_SGX)) {
>  		/* OMAP3430, OMAP3525, OMAP3515, OMAP3503 devices */
>  		strcpy(cpu_name, "OMAP3430/3530");
> -	} else if (omap3_has_iva()) {
> +	} else if (omap_has_feature(OMAP3_HAS_IVA)) {
>  		omap_revision = OMAP3525_REV(rev);
>  		strcpy(cpu_name, "OMAP3525");
> -	} else if (omap3_has_sgx()) {
> +	} else if (omap_has_feature(OMAP3_HAS_SGX)) {
>  		omap_revision = OMAP3515_REV(rev);
>  		strcpy(cpu_name, "OMAP3515");
>  	} else {
> @@ -400,12 +400,12 @@ static void __init omap3_cpuinfo(void)
>  	/* Print verbose information */
>  	pr_info("%s ES%s (", cpu_name, cpu_rev);
>  
> -	OMAP3_SHOW_FEATURE(l2cache);
> -	OMAP3_SHOW_FEATURE(iva);
> -	OMAP3_SHOW_FEATURE(sgx);
> -	OMAP3_SHOW_FEATURE(neon);
> -	OMAP3_SHOW_FEATURE(isp);
> -	OMAP3_SHOW_FEATURE(192mhz_clk);
> +	OMAP3_SHOW_FEATURE(OMAP3_HAS_L2CACHE);
> +	OMAP3_SHOW_FEATURE(OMAP3_HAS_IVA);
> +	OMAP3_SHOW_FEATURE(OMAP3_HAS_SGX);
> +	OMAP3_SHOW_FEATURE(OMAP3_HAS_NEON);
> +	OMAP3_SHOW_FEATURE(OMAP3_HAS_ISP);
> +	OMAP3_SHOW_FEATURE(OMAP3_HAS_192MHZ_CLK);
>  
>  	printk(")\n");
>  }
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index fb4994a..32e1005 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -385,7 +385,7 @@ void omap_sram_idle(void)
>  	/* Enable IO-PAD and IO-CHAIN wakeups */
>  	per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
>  	core_next_state = pwrdm_read_next_pwrst(core_pwrdm);
> -	if (omap3_has_io_wakeup() && \
> +	if (omap_has_feature(OMAP3_HAS_IO_WAKEUP) &&		\
>  			(per_next_state < PWRDM_POWER_ON ||
>  			core_next_state < PWRDM_POWER_ON)) {
>  		prm_set_mod_reg_bits(OMAP3430_EN_IO_MASK, WKUP_MOD, PM_WKEN);
> diff --git a/arch/arm/plat-omap/include/plat/cpu.h b/arch/arm/plat-omap/include/plat/cpu.h
> index 127df06..efee323 100644
> --- a/arch/arm/plat-omap/include/plat/cpu.h
> +++ b/arch/arm/plat-omap/include/plat/cpu.h
> @@ -334,14 +334,14 @@ IS_OMAP_TYPE(3517, 0x3517)
>  # undef cpu_is_omap3517
>  # define cpu_is_omap3430()		is_omap3430()
>  # define cpu_is_omap3503()		(cpu_is_omap3430() &&		\
> -						(!omap3_has_iva()) &&	\
> -						(!omap3_has_sgx()))
> +					 (!omap_has_feature(OMAP3_HAS_IVA) &&	\
> +						(!omap_has_feature(OMAP3_HAS_SGX)))
>  # define cpu_is_omap3515()		(cpu_is_omap3430() &&		\
> -						(!omap3_has_iva()) &&	\
> -						(omap3_has_sgx()))
> +						(!omap_has_feature(OMAP3_HAS_IVA)) &&	\
> +						(omap_has_feature(OMAP3_HAS_SGX)))
>  # define cpu_is_omap3525()		(cpu_is_omap3430() &&		\
> -						(!omap3_has_sgx()) &&	\
> -						(omap3_has_iva()))
> +						(!omap_has_feature(OMAP3_HAS_SGX)) &&	\
> +						(omap_has_feature(OMAP3_HAS_IVA)))
>  # define cpu_is_omap3530()		(cpu_is_omap3430())
>  # define cpu_is_omap3505()		is_omap3505()
>  # define cpu_is_omap3517()		is_omap3517()
> 
Acked-by: Nishanth Menon <nm@ti.com>

-- 
Regards,
Nishanth Menon

[PATCH 4/5] omap: Remove old omap3_has_ macros

[Tony Lindgren]

Remove old omap3_has_ macros. Please use omap_has_feature()
instead.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/mach-omap2/id.c              |   14 ++++----------
 arch/arm/plat-omap/include/plat/cpu.h |   16 ----------------
 2 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
index 11184cf..123ed1e 100644
--- a/arch/arm/mach-omap2/id.c
+++ b/arch/arm/mach-omap2/id.c
@@ -31,9 +31,6 @@ static struct omap_chip_id omap_chip;
 static unsigned int omap_revision;
 static u32 omap_features;
 
-/* REVISIT: Get rid of omap3_features */
-u32 omap3_features;
-
 unsigned int omap_rev(void)
 {
 	return omap_revision;
@@ -186,14 +183,14 @@ u32 omap3_has_feature(u32 feat_mask)
 #define OMAP3_CHECK_FEATURE(status,feat)				\
 	if (((status & OMAP3_ ##feat## _MASK) 				\
 		>> OMAP3_ ##feat## _SHIFT) != FEAT_ ##feat## _NONE) { 	\
-		omap3_features |= OMAP3_HAS_ ##feat;			\
+		omap_features |= OMAP3_HAS_ ##feat;			\
 	}
 
 static void __init omap3_check_features(void)
 {
 	u32 status;
 
-	omap3_features = 0;
+	omap_features = 0;
 
 	status = omap_ctrl_readl(OMAP3_CONTROL_OMAP_STATUS);
 
@@ -203,18 +200,15 @@ static void __init omap3_check_features(void)
 	OMAP3_CHECK_FEATURE(status, NEON);
 	OMAP3_CHECK_FEATURE(status, ISP);
 	if (cpu_is_omap3630())
-		omap3_features |= OMAP3_HAS_192MHZ_CLK;
+		omap_features |= OMAP3_HAS_192MHZ_CLK;
 	if (!cpu_is_omap3505() && !cpu_is_omap3517())
-		omap3_features |= OMAP3_HAS_IO_WAKEUP;
+		omap_features |= OMAP3_HAS_IO_WAKEUP;
 
 	/*
 	 * TODO: Get additional info (where applicable)
 	 *       e.g. Size of L2 cache.
 	 */
 
-	/* REVISIT: Get rid of omap3_features */
-	omap_features = omap3_features;
-
 	omap_init_features(omap3_has_feature);
 }
 
diff --git a/arch/arm/plat-omap/include/plat/cpu.h b/arch/arm/plat-omap/include/plat/cpu.h
index efee323..96eac4d 100644
--- a/arch/arm/plat-omap/include/plat/cpu.h
+++ b/arch/arm/plat-omap/include/plat/cpu.h
@@ -439,8 +439,6 @@ void omap2_check_revision(void);
 /*
  * Runtime detection of OMAP3 features
  */
-extern u32 omap3_features;
-
 #define OMAP3_HAS_L2CACHE		BIT(0)
 #define OMAP3_HAS_IVA			BIT(1)
 #define OMAP3_HAS_SGX			BIT(2)
@@ -449,18 +447,4 @@ extern u32 omap3_features;
 #define OMAP3_HAS_192MHZ_CLK		BIT(5)
 #define OMAP3_HAS_IO_WAKEUP		BIT(6)
 
-#define OMAP3_HAS_FEATURE(feat,flag)			\
-static inline unsigned int omap3_has_ ##feat(void)	\
-{							\
-	return (omap3_features & OMAP3_HAS_ ##flag);	\
-}							\
-
-OMAP3_HAS_FEATURE(l2cache, L2CACHE)
-OMAP3_HAS_FEATURE(sgx, SGX)
-OMAP3_HAS_FEATURE(iva, IVA)
-OMAP3_HAS_FEATURE(neon, NEON)
-OMAP3_HAS_FEATURE(isp, ISP)
-OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
-OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
-
 #endif

Re: [PATCH 4/5] omap: Remove old omap3_has_ macros

[Nishanth Menon]

Tony Lindgren had written, on 07/08/2010 04:38 AM, the following:
> Remove old omap3_has_ macros. Please use omap_has_feature()
> instead.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  arch/arm/mach-omap2/id.c              |   14 ++++----------
>  arch/arm/plat-omap/include/plat/cpu.h |   16 ----------------
>  2 files changed, 4 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
> index 11184cf..123ed1e 100644
> --- a/arch/arm/mach-omap2/id.c
> +++ b/arch/arm/mach-omap2/id.c
> @@ -31,9 +31,6 @@ static struct omap_chip_id omap_chip;
>  static unsigned int omap_revision;
>  static u32 omap_features;
>  
> -/* REVISIT: Get rid of omap3_features */
> -u32 omap3_features;
> -
>  unsigned int omap_rev(void)
>  {
>  	return omap_revision;
> @@ -186,14 +183,14 @@ u32 omap3_has_feature(u32 feat_mask)
>  #define OMAP3_CHECK_FEATURE(status,feat)				\
>  	if (((status & OMAP3_ ##feat## _MASK) 				\
>  		>> OMAP3_ ##feat## _SHIFT) != FEAT_ ##feat## _NONE) { 	\
> -		omap3_features |= OMAP3_HAS_ ##feat;			\
> +		omap_features |= OMAP3_HAS_ ##feat;			\
>  	}
>  
>  static void __init omap3_check_features(void)
>  {
>  	u32 status;
>  
> -	omap3_features = 0;
> +	omap_features = 0;
>  
>  	status = omap_ctrl_readl(OMAP3_CONTROL_OMAP_STATUS);
>  
> @@ -203,18 +200,15 @@ static void __init omap3_check_features(void)
>  	OMAP3_CHECK_FEATURE(status, NEON);
>  	OMAP3_CHECK_FEATURE(status, ISP);
>  	if (cpu_is_omap3630())
> -		omap3_features |= OMAP3_HAS_192MHZ_CLK;
> +		omap_features |= OMAP3_HAS_192MHZ_CLK;
>  	if (!cpu_is_omap3505() && !cpu_is_omap3517())
> -		omap3_features |= OMAP3_HAS_IO_WAKEUP;
> +		omap_features |= OMAP3_HAS_IO_WAKEUP;
>  
>  	/*
>  	 * TODO: Get additional info (where applicable)
>  	 *       e.g. Size of L2 cache.
>  	 */
>  
> -	/* REVISIT: Get rid of omap3_features */
> -	omap_features = omap3_features;
> -
>  	omap_init_features(omap3_has_feature);
>  }
>  
> diff --git a/arch/arm/plat-omap/include/plat/cpu.h b/arch/arm/plat-omap/include/plat/cpu.h
> index efee323..96eac4d 100644
> --- a/arch/arm/plat-omap/include/plat/cpu.h
> +++ b/arch/arm/plat-omap/include/plat/cpu.h
> @@ -439,8 +439,6 @@ void omap2_check_revision(void);
>  /*
>   * Runtime detection of OMAP3 features
>   */
> -extern u32 omap3_features;
> -
>  #define OMAP3_HAS_L2CACHE		BIT(0)
>  #define OMAP3_HAS_IVA			BIT(1)
>  #define OMAP3_HAS_SGX			BIT(2)
> @@ -449,18 +447,4 @@ extern u32 omap3_features;
>  #define OMAP3_HAS_192MHZ_CLK		BIT(5)
>  #define OMAP3_HAS_IO_WAKEUP		BIT(6)
>  
> -#define OMAP3_HAS_FEATURE(feat,flag)			\
> -static inline unsigned int omap3_has_ ##feat(void)	\
> -{							\
> -	return (omap3_features & OMAP3_HAS_ ##flag);	\
> -}							\
> -
> -OMAP3_HAS_FEATURE(l2cache, L2CACHE)
> -OMAP3_HAS_FEATURE(sgx, SGX)
> -OMAP3_HAS_FEATURE(iva, IVA)
> -OMAP3_HAS_FEATURE(neon, NEON)
> -OMAP3_HAS_FEATURE(isp, ISP)
> -OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
> -OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
> -
>  #endif
> 
Acked-by: Nishanth Menon <nm@ti.com>

-- 
Regards,
Nishanth Menon

[PATCH 5/5] omap: Allow testing for omap type with omap_has_feature

[Tony Lindgren]

Allow testing for omap type with omap_has_feature. This
can be used to leave out cpu_is_omapxxxx checks.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/plat-omap/include/plat/cpu.h |   38 ++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/cpu.h b/arch/arm/plat-omap/include/plat/cpu.h
index 96eac4d..c117c3c 100644
--- a/arch/arm/plat-omap/include/plat/cpu.h
+++ b/arch/arm/plat-omap/include/plat/cpu.h
@@ -437,14 +437,36 @@ int omap_chip_is(struct omap_chip_id oci);
 void omap2_check_revision(void);
 
 /*
- * Runtime detection of OMAP3 features
+ * Runtime detection of OMAP features
  */
-#define OMAP3_HAS_L2CACHE		BIT(0)
-#define OMAP3_HAS_IVA			BIT(1)
-#define OMAP3_HAS_SGX			BIT(2)
-#define OMAP3_HAS_NEON			BIT(3)
-#define OMAP3_HAS_ISP			BIT(4)
-#define OMAP3_HAS_192MHZ_CLK		BIT(5)
-#define OMAP3_HAS_IO_WAKEUP		BIT(6)
+#define OMAP_FEAT_CLASS_OMAP1		BIT(24)
+#define OMAP_FEAT_CLASS_OMAP2		BIT(25)
+#define OMAP_FEAT_CLASS_OMAP3		BIT(26)
+#define OMAP_FEAT_CLASS_OMAP4		BIT(27)
+
+#define OMAP_HAS_L2CACHE		BIT(0)
+#define OMAP_HAS_IVA			BIT(1)
+#define OMAP_HAS_SGX			BIT(2)
+#define OMAP_HAS_NEON			BIT(3)
+#define OMAP_HAS_ISP			BIT(4)
+#define OMAP_HAS_192MHZ_CLK		BIT(5)
+#define OMAP_HAS_IO_WAKEUP		BIT(6)
+
+#define OMAP2_HAS_IVA			OMAP_FEAT_CLASS_OMAP2 | OMAP_HAS_IVA
+#define OMAP2_HAS_SGX			OMAP_FEAT_CLASS_OMAP2 | OMAP_HAS_SGX
+
+#define OMAP3_HAS_L2CACHE		OMAP_FEAT_CLASS_OMAP3 | OMAP_HAS_L2CACHE
+#define OMAP3_HAS_IVA			OMAP_FEAT_CLASS_OMAP3 | OMAP_HAS_IVA
+#define OMAP3_HAS_SGX			OMAP_FEAT_CLASS_OMAP3 | OMAP_HAS_SGX
+#define OMAP3_HAS_NEON			OMAP_FEAT_CLASS_OMAP3 | OMAP_HAS_NEON
+#define OMAP3_HAS_ISP			OMAP_FEAT_CLASS_OMAP3 | OMAP_HAS_ISP
+#define OMAP3_HAS_192MHZ_CLK		OMAP_FEAT_CLASS_OMAP3 | OMAP_HAS_192MHZ_CLK
+#define OMAP3_HAS_IO_WAKEUP		OMAP_FEAT_CLASS_OMAP3 | OMAP_HAS_IOWAKEUP
+
+#define OMAP4_HAS_L2CACHE		OMAP_FEAT_CLASS_OMAP4 | OMAP_HAS_L2CACHE
+#define OMAP4_HAS_IVA			OMAP_FEAT_CLASS_OMAP4 | OMAP_HAS_IVA
+#define OMAP4_HAS_SGX			OMAP_FEAT_CLASS_OMAP4 | OMAP_HAS_SGX
+#define OMAP4_HAS_NEON			OMAP_FEAT_CLASS_OMAP4 | OMAP_HAS_NEON
+#define OMAP4_HAS_ISP			OMAP_FEAT_CLASS_OMAP4 | OMAP_HAS_ISP
 
 #endif

Re: [PATCH 5/5] omap: Allow testing for omap type with omap_has_feature

[Nishanth Menon]

Tony Lindgren had written, on 07/08/2010 04:38 AM, the following:
> Allow testing for omap type with omap_has_feature. This
> can be used to leave out cpu_is_omapxxxx checks.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  arch/arm/plat-omap/include/plat/cpu.h |   38 ++++++++++++++++++++++++++-------
>  1 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/include/plat/cpu.h b/arch/arm/plat-omap/include/plat/cpu.h
> index 96eac4d..c117c3c 100644
> --- a/arch/arm/plat-omap/include/plat/cpu.h
> +++ b/arch/arm/plat-omap/include/plat/cpu.h
> @@ -437,14 +437,36 @@ int omap_chip_is(struct omap_chip_id oci);
>  void omap2_check_revision(void);
>  
>  /*
> - * Runtime detection of OMAP3 features
> + * Runtime detection of OMAP features
>   */
> -#define OMAP3_HAS_L2CACHE		BIT(0)
> -#define OMAP3_HAS_IVA			BIT(1)
> -#define OMAP3_HAS_SGX			BIT(2)
> -#define OMAP3_HAS_NEON			BIT(3)
> -#define OMAP3_HAS_ISP			BIT(4)
> -#define OMAP3_HAS_192MHZ_CLK		BIT(5)
> -#define OMAP3_HAS_IO_WAKEUP		BIT(6)
> +#define OMAP_FEAT_CLASS_OMAP1		BIT(24)
> +#define OMAP_FEAT_CLASS_OMAP2		BIT(25)
> +#define OMAP_FEAT_CLASS_OMAP3		BIT(26)
> +#define OMAP_FEAT_CLASS_OMAP4		BIT(27)
> +
> +#define OMAP_HAS_L2CACHE		BIT(0)
> +#define OMAP_HAS_IVA			BIT(1)
> +#define OMAP_HAS_SGX			BIT(2)
> +#define OMAP_HAS_NEON			BIT(3)
> +#define OMAP_HAS_ISP			BIT(4)
> +#define OMAP_HAS_192MHZ_CLK		BIT(5)
> +#define OMAP_HAS_IO_WAKEUP		BIT(6)
> +
> +#define OMAP2_HAS_IVA			OMAP_FEAT_CLASS_OMAP2 | OMAP_HAS_IVA
> +#define OMAP2_HAS_SGX			OMAP_FEAT_CLASS_OMAP2 | OMAP_HAS_SGX
> +
> +#define OMAP3_HAS_L2CACHE		OMAP_FEAT_CLASS_OMAP3 | OMAP_HAS_L2CACHE
> +#define OMAP3_HAS_IVA			OMAP_FEAT_CLASS_OMAP3 | OMAP_HAS_IVA
> +#define OMAP3_HAS_SGX			OMAP_FEAT_CLASS_OMAP3 | OMAP_HAS_SGX
> +#define OMAP3_HAS_NEON			OMAP_FEAT_CLASS_OMAP3 | OMAP_HAS_NEON
> +#define OMAP3_HAS_ISP			OMAP_FEAT_CLASS_OMAP3 | OMAP_HAS_ISP
> +#define OMAP3_HAS_192MHZ_CLK		OMAP_FEAT_CLASS_OMAP3 | OMAP_HAS_192MHZ_CLK
> +#define OMAP3_HAS_IO_WAKEUP		OMAP_FEAT_CLASS_OMAP3 | OMAP_HAS_IOWAKEUP
> +
> +#define OMAP4_HAS_L2CACHE		OMAP_FEAT_CLASS_OMAP4 | OMAP_HAS_L2CACHE
> +#define OMAP4_HAS_IVA			OMAP_FEAT_CLASS_OMAP4 | OMAP_HAS_IVA
> +#define OMAP4_HAS_SGX			OMAP_FEAT_CLASS_OMAP4 | OMAP_HAS_SGX
> +#define OMAP4_HAS_NEON			OMAP_FEAT_CLASS_OMAP4 | OMAP_HAS_NEON
> +#define OMAP4_HAS_ISP			OMAP_FEAT_CLASS_OMAP4 | OMAP_HAS_ISP
>  
>  #endif
> 
here is my contention:
there will be two ways to use this:
omap_has_feature(OMAP_HAS_SGX) and omap_has_feature(OMAP3_HAS_SGX)

OMAP_HAS_SGX should return true or false no matter what omap silicon it is.

OMAP3_HAS_SGX usage is meant for what? it is a mixture of cpu_is_omap3() 
and omap_has_feature(OMAP_HAS_SGX) - tries to do two things in one shot. 
which defeats why we are trying to introduce a generic omap_has_feature 
in the first place.
a) confusing as there seems to be two standards
b) redundant information use cpu_is_omapxyz() if needed.

IMHO:
+#define OMAP_HAS_L2CACHE		BIT(0)
+#define OMAP_HAS_IVA			BIT(1)
+#define OMAP_HAS_SGX			BIT(2)
+#define OMAP_HAS_NEON			BIT(3)
+#define OMAP_HAS_ISP			BIT(4)
+#define OMAP3_HAS_192MHZ_CLK		BIT(5)
+#define OMAP_HAS_IO_WAKEUP		BIT(6)
and later if needed
+#define OMAP4_SOME_NEW_OMAP4ONLY_FEATURE BIT(7)

where OMAP3_HAS is indicative that this is a OMAP3 *only* feature and 
should be used to differentiate between various omap3 silicon.

Benefits:
a) distinction b/w omap generic and omap family specific features
b) you get to define 32 features instead of reserving 24-32 for OMAP 
classes.

-- 
Regards,
Nishanth Menon

Re: [PATCH 5/5] omap: Allow testing for omap type with omap_has_feature

[Venkatraman S]

On Thu, Jul 8, 2010 at 8:33 PM, Nishanth Menon <nm@ti.com> wrote:
> Tony Lindgren had written, on 07/08/2010 04:38 AM, the following:
>>
>> Allow testing for omap type with omap_has_feature. This
>> can be used to leave out cpu_is_omapxxxx checks.
>>
>> Signed-off-by: Tony Lindgren <tony@atomide.com>
>> ---
>>  arch/arm/plat-omap/include/plat/cpu.h |   38
>> ++++++++++++++++++++++++++-------
>>  1 files changed, 30 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/include/plat/cpu.h
>> b/arch/arm/plat-omap/include/plat/cpu.h
>> index 96eac4d..c117c3c 100644
>> --- a/arch/arm/plat-omap/include/plat/cpu.h
>> +++ b/arch/arm/plat-omap/include/plat/cpu.h
>> @@ -437,14 +437,36 @@ int omap_chip_is(struct omap_chip_id oci);
>>  void omap2_check_revision(void);
>>  /*
>> - * Runtime detection of OMAP3 features
>> + * Runtime detection of OMAP features
>>  */
>> -#define OMAP3_HAS_L2CACHE              BIT(0)
>> -#define OMAP3_HAS_IVA                  BIT(1)
>> -#define OMAP3_HAS_SGX                  BIT(2)
>> -#define OMAP3_HAS_NEON                 BIT(3)
>> -#define OMAP3_HAS_ISP                  BIT(4)
>> -#define OMAP3_HAS_192MHZ_CLK           BIT(5)
>> -#define OMAP3_HAS_IO_WAKEUP            BIT(6)
>> +#define OMAP_FEAT_CLASS_OMAP1          BIT(24)
>> +#define OMAP_FEAT_CLASS_OMAP2          BIT(25)
>> +#define OMAP_FEAT_CLASS_OMAP3          BIT(26)
>> +#define OMAP_FEAT_CLASS_OMAP4          BIT(27)
>> +
>> +#define OMAP_HAS_L2CACHE               BIT(0)
>> +#define OMAP_HAS_IVA                   BIT(1)
>> +#define OMAP_HAS_SGX                   BIT(2)
>> +#define OMAP_HAS_NEON                  BIT(3)
>> +#define OMAP_HAS_ISP                   BIT(4)
>> +#define OMAP_HAS_192MHZ_CLK            BIT(5)
>> +#define OMAP_HAS_IO_WAKEUP             BIT(6)
>> +
>> +#define OMAP2_HAS_IVA                  OMAP_FEAT_CLASS_OMAP2 |
>> OMAP_HAS_IVA
>> +#define OMAP2_HAS_SGX                  OMAP_FEAT_CLASS_OMAP2 |
>> OMAP_HAS_SGX
>> +
>> +#define OMAP3_HAS_L2CACHE              OMAP_FEAT_CLASS_OMAP3 |
>> OMAP_HAS_L2CACHE
>> +#define OMAP3_HAS_IVA                  OMAP_FEAT_CLASS_OMAP3 |
>> OMAP_HAS_IVA
>> +#define OMAP3_HAS_SGX                  OMAP_FEAT_CLASS_OMAP3 |
>> OMAP_HAS_SGX
>> +#define OMAP3_HAS_NEON                 OMAP_FEAT_CLASS_OMAP3 |
>> OMAP_HAS_NEON
>> +#define OMAP3_HAS_ISP                  OMAP_FEAT_CLASS_OMAP3 |
>> OMAP_HAS_ISP
>> +#define OMAP3_HAS_192MHZ_CLK           OMAP_FEAT_CLASS_OMAP3 |
>> OMAP_HAS_192MHZ_CLK
>> +#define OMAP3_HAS_IO_WAKEUP            OMAP_FEAT_CLASS_OMAP3 |
>> OMAP_HAS_IOWAKEUP
>> +
>> +#define OMAP4_HAS_L2CACHE              OMAP_FEAT_CLASS_OMAP4 |
>> OMAP_HAS_L2CACHE
>> +#define OMAP4_HAS_IVA                  OMAP_FEAT_CLASS_OMAP4 |
>> OMAP_HAS_IVA
>> +#define OMAP4_HAS_SGX                  OMAP_FEAT_CLASS_OMAP4 |
>> OMAP_HAS_SGX
>> +#define OMAP4_HAS_NEON                 OMAP_FEAT_CLASS_OMAP4 |
>> OMAP_HAS_NEON
>> +#define OMAP4_HAS_ISP                  OMAP_FEAT_CLASS_OMAP4 |
>> OMAP_HAS_ISP
>>  #endif
>>
> here is my contention:
> there will be two ways to use this:
> omap_has_feature(OMAP_HAS_SGX) and omap_has_feature(OMAP3_HAS_SGX)
>
> OMAP_HAS_SGX should return true or false no matter what omap silicon it is.
>
> OMAP3_HAS_SGX usage is meant for what? it is a mixture of cpu_is_omap3() and
> omap_has_feature(OMAP_HAS_SGX) - tries to do two things in one shot. which
> defeats why we are trying to introduce a generic omap_has_feature in the
> first place.
> a) confusing as there seems to be two standards
> b) redundant information use cpu_is_omapxyz() if needed.
>
> IMHO:
> +#define OMAP_HAS_L2CACHE               BIT(0)
> +#define OMAP_HAS_IVA                   BIT(1)
> +#define OMAP_HAS_SGX                   BIT(2)
> +#define OMAP_HAS_NEON                  BIT(3)
> +#define OMAP_HAS_ISP                   BIT(4)
> +#define OMAP3_HAS_192MHZ_CLK           BIT(5)
> +#define OMAP_HAS_IO_WAKEUP             BIT(6)
> and later if needed
> +#define OMAP4_SOME_NEW_OMAP4ONLY_FEATURE BIT(7)
>
> where OMAP3_HAS is indicative that this is a OMAP3 *only* feature and should
> be used to differentiate between various omap3 silicon.
>
> Benefits:
> a) distinction b/w omap generic and omap family specific features
> b) you get to define 32 features instead of reserving 24-32 for OMAP
> classes.
>

I still can't grok the need for the distinction in (a), and for
"" +#define OMAP4_SOME_NEW_OMAP4ONLY_FEATURE BIT(7)""  etc.

If that OMAP4ONLY_FEATURE has to be checked, then the code
to use it will also be OMAP4 specific.

IOW, as a user, there are 2 ways to use omap_has_xxxx()

void a_generic_funciton_for_all_omaps() {
     if (cpu_has_xxxx_feature()
         /* Do generic stuff */
}

void a_omap_4_specific_function()   {
    if (omap_has_that_new_feature()
          /* Do omap_4 specific stuff */
}

In a_generic_function_for_all_omaps(), if there is a need for checking
OMAP4_has_xxxx,
then the code will eventually be ugly.  There is going to be a
cpu_is_xxxx() overload, for things
not expressed through features framework.

I did read the other thread
http://marc.info/?l=linux-omap&m=127858108626850&w=2
and it's been discussed before as well. But I can't see a genuine use
case yet, and what is the loss involved
if a omap3_has_192mhz_clk is expressed as omap_has_192mhz_clk, even if
it is omap3 specific.

Thanks,
Venkat.
--
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 5/5] omap: Allow testing for omap type with omap_has_feature

[Nishanth Menon]

S, Venkatraman had written, on 07/08/2010 11:15 AM, the following:
> On Thu, Jul 8, 2010 at 8:33 PM, Nishanth Menon <nm@ti.com> wrote:
>> Tony Lindgren had written, on 07/08/2010 04:38 AM, the following:
>>> Allow testing for omap type with omap_has_feature. This
>>> can be used to leave out cpu_is_omapxxxx checks.
>>>
>>> Signed-off-by: Tony Lindgren <tony@atomide.com>
>>> ---
>>>  arch/arm/plat-omap/include/plat/cpu.h |   38
>>> ++++++++++++++++++++++++++-------
>>>  1 files changed, 30 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-omap/include/plat/cpu.h
>>> b/arch/arm/plat-omap/include/plat/cpu.h
>>> index 96eac4d..c117c3c 100644
>>> --- a/arch/arm/plat-omap/include/plat/cpu.h
>>> +++ b/arch/arm/plat-omap/include/plat/cpu.h
>>> @@ -437,14 +437,36 @@ int omap_chip_is(struct omap_chip_id oci);
>>>  void omap2_check_revision(void);
>>>  /*
>>> - * Runtime detection of OMAP3 features
>>> + * Runtime detection of OMAP features
>>>  */
>>> -#define OMAP3_HAS_L2CACHE              BIT(0)
>>> -#define OMAP3_HAS_IVA                  BIT(1)
>>> -#define OMAP3_HAS_SGX                  BIT(2)
>>> -#define OMAP3_HAS_NEON                 BIT(3)
>>> -#define OMAP3_HAS_ISP                  BIT(4)
>>> -#define OMAP3_HAS_192MHZ_CLK           BIT(5)
>>> -#define OMAP3_HAS_IO_WAKEUP            BIT(6)
>>> +#define OMAP_FEAT_CLASS_OMAP1          BIT(24)
>>> +#define OMAP_FEAT_CLASS_OMAP2          BIT(25)
>>> +#define OMAP_FEAT_CLASS_OMAP3          BIT(26)
>>> +#define OMAP_FEAT_CLASS_OMAP4          BIT(27)
>>> +
>>> +#define OMAP_HAS_L2CACHE               BIT(0)
>>> +#define OMAP_HAS_IVA                   BIT(1)
>>> +#define OMAP_HAS_SGX                   BIT(2)
>>> +#define OMAP_HAS_NEON                  BIT(3)
>>> +#define OMAP_HAS_ISP                   BIT(4)
>>> +#define OMAP_HAS_192MHZ_CLK            BIT(5)
>>> +#define OMAP_HAS_IO_WAKEUP             BIT(6)
>>> +
>>> +#define OMAP2_HAS_IVA                  OMAP_FEAT_CLASS_OMAP2 |
>>> OMAP_HAS_IVA
>>> +#define OMAP2_HAS_SGX                  OMAP_FEAT_CLASS_OMAP2 |
>>> OMAP_HAS_SGX
>>> +
>>> +#define OMAP3_HAS_L2CACHE              OMAP_FEAT_CLASS_OMAP3 |
>>> OMAP_HAS_L2CACHE
>>> +#define OMAP3_HAS_IVA                  OMAP_FEAT_CLASS_OMAP3 |
>>> OMAP_HAS_IVA
>>> +#define OMAP3_HAS_SGX                  OMAP_FEAT_CLASS_OMAP3 |
>>> OMAP_HAS_SGX
>>> +#define OMAP3_HAS_NEON                 OMAP_FEAT_CLASS_OMAP3 |
>>> OMAP_HAS_NEON
>>> +#define OMAP3_HAS_ISP                  OMAP_FEAT_CLASS_OMAP3 |
>>> OMAP_HAS_ISP
>>> +#define OMAP3_HAS_192MHZ_CLK           OMAP_FEAT_CLASS_OMAP3 |
>>> OMAP_HAS_192MHZ_CLK
>>> +#define OMAP3_HAS_IO_WAKEUP            OMAP_FEAT_CLASS_OMAP3 |
>>> OMAP_HAS_IOWAKEUP
>>> +
>>> +#define OMAP4_HAS_L2CACHE              OMAP_FEAT_CLASS_OMAP4 |
>>> OMAP_HAS_L2CACHE
>>> +#define OMAP4_HAS_IVA                  OMAP_FEAT_CLASS_OMAP4 |
>>> OMAP_HAS_IVA
>>> +#define OMAP4_HAS_SGX                  OMAP_FEAT_CLASS_OMAP4 |
>>> OMAP_HAS_SGX
>>> +#define OMAP4_HAS_NEON                 OMAP_FEAT_CLASS_OMAP4 |
>>> OMAP_HAS_NEON
>>> +#define OMAP4_HAS_ISP                  OMAP_FEAT_CLASS_OMAP4 |
>>> OMAP_HAS_ISP
>>>  #endif
>>>
>> here is my contention:
>> there will be two ways to use this:
>> omap_has_feature(OMAP_HAS_SGX) and omap_has_feature(OMAP3_HAS_SGX)
>>
>> OMAP_HAS_SGX should return true or false no matter what omap silicon it is.
>>
>> OMAP3_HAS_SGX usage is meant for what? it is a mixture of cpu_is_omap3() and
>> omap_has_feature(OMAP_HAS_SGX) - tries to do two things in one shot. which
>> defeats why we are trying to introduce a generic omap_has_feature in the
>> first place.
>> a) confusing as there seems to be two standards
>> b) redundant information use cpu_is_omapxyz() if needed.
>>
>> IMHO:
>> +#define OMAP_HAS_L2CACHE               BIT(0)
>> +#define OMAP_HAS_IVA                   BIT(1)
>> +#define OMAP_HAS_SGX                   BIT(2)
>> +#define OMAP_HAS_NEON                  BIT(3)
>> +#define OMAP_HAS_ISP                   BIT(4)
>> +#define OMAP3_HAS_192MHZ_CLK           BIT(5)
>> +#define OMAP_HAS_IO_WAKEUP             BIT(6)
>> and later if needed
>> +#define OMAP4_SOME_NEW_OMAP4ONLY_FEATURE BIT(7)
>>
>> where OMAP3_HAS is indicative that this is a OMAP3 *only* feature and should
>> be used to differentiate between various omap3 silicon.
>>
>> Benefits:
>> a) distinction b/w omap generic and omap family specific features
>> b) you get to define 32 features instead of reserving 24-32 for OMAP
>> classes.
>>
> 
> I still can't grok the need for the distinction in (a), and for
> "" +#define OMAP4_SOME_NEW_OMAP4ONLY_FEATURE BIT(7)""  etc.
> 

OMAP_HAS_192MHZ_CLK -> does not indicate if this is omap3 ONLY feature 
(e.g. 3430 does not have it, 3630 has it) but we know that omap4, 2, 1 
etc dont need it.

in terms of readability, when i see omap_has_feature(OMAP3_HAS_xyz), I 
can immediately review the code/read the code with the context of omap3 
alone Vs if this code was used in omap4/2/1 context question why it is 
so and we can all improve.

e.g. if a generic clock code meant for all omaps used 192MHZ, I would 
question why is cpu specific feature being used there. which is easier 
with a OMAP3_ tag.


> If that OMAP4ONLY_FEATURE has to be checked, then the code
> to use it will also be OMAP4 specific.
> 
> IOW, as a user, there are 2 ways to use omap_has_xxxx()
> 
> void a_generic_funciton_for_all_omaps() {
>      if (cpu_has_xxxx_feature()
>          /* Do generic stuff */
> }
> 
> void a_omap_4_specific_function()   {
>     if (omap_has_that_new_feature()
>           /* Do omap_4 specific stuff */
> }

See above explanation. sitting in the shoes of a reviewer who looks 
through code not written by self, it helps to have some differentiation 
in definitions to aid review.


> 
> In a_generic_function_for_all_omaps(), if there is a need for checking
> OMAP4_has_xxxx,
> then the code will eventually be ugly.  There is going to be a
> cpu_is_xxxx() overload, for things
> not expressed through features framework.
No matter how elegant the framework is, someone definitely will find a 
crappy way to use it.. we've all been there, seen that and some of us 
(including yours truely) done that.

> 
> I did read the other thread
> http://marc.info/?l=linux-omap&m=127858108626850&w=2
> and it's been discussed before as well. But I can't see a genuine use
> case yet, and what is the loss involved
> if a omap3_has_192mhz_clk is expressed as omap_has_192mhz_clk, even if
> it is omap3 specific.
> 
> Thanks,
> Venkat.


-- 
Regards,
Nishanth Menon

Re: [PATCH 5/5] omap: Allow testing for omap type with omap_has_feature

[Venkatraman S]

On Thu, Jul 8, 2010 at 9:58 PM, Nishanth Menon <nm@ti.com> wrote:
> S, Venkatraman had written, on 07/08/2010 11:15 AM, the following:
>>
>> On Thu, Jul 8, 2010 at 8:33 PM, Nishanth Menon <nm@ti.com> wrote:
>>>
>>> Tony Lindgren had written, on 07/08/2010 04:38 AM, the following:
>>>>
>>>> Allow testing for omap type with omap_has_feature. This
>>>> can be used to leave out cpu_is_omapxxxx checks.
>>>>
>>>> Signed-off-by: Tony Lindgren <tony@atomide.com>
>>>> ---
>>>>  arch/arm/plat-omap/include/plat/cpu.h |   38
>>>> ++++++++++++++++++++++++++-------
>>>>  1 files changed, 30 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/arch/arm/plat-omap/include/plat/cpu.h
>>>> b/arch/arm/plat-omap/include/plat/cpu.h
>>>> index 96eac4d..c117c3c 100644
>>>> --- a/arch/arm/plat-omap/include/plat/cpu.h
>>>> +++ b/arch/arm/plat-omap/include/plat/cpu.h
>>>> @@ -437,14 +437,36 @@ int omap_chip_is(struct omap_chip_id oci);
>>>>  void omap2_check_revision(void);
>>>>  /*
>>>> - * Runtime detection of OMAP3 features
>>>> + * Runtime detection of OMAP features
>>>>  */
>>>> -#define OMAP3_HAS_L2CACHE              BIT(0)
>>>> -#define OMAP3_HAS_IVA                  BIT(1)
>>>> -#define OMAP3_HAS_SGX                  BIT(2)
>>>> -#define OMAP3_HAS_NEON                 BIT(3)
>>>> -#define OMAP3_HAS_ISP                  BIT(4)
>>>> -#define OMAP3_HAS_192MHZ_CLK           BIT(5)
>>>> -#define OMAP3_HAS_IO_WAKEUP            BIT(6)
>>>> +#define OMAP_FEAT_CLASS_OMAP1          BIT(24)
>>>> +#define OMAP_FEAT_CLASS_OMAP2          BIT(25)
>>>> +#define OMAP_FEAT_CLASS_OMAP3          BIT(26)
>>>> +#define OMAP_FEAT_CLASS_OMAP4          BIT(27)
>>>> +
>>>> +#define OMAP_HAS_L2CACHE               BIT(0)
>>>> +#define OMAP_HAS_IVA                   BIT(1)
>>>> +#define OMAP_HAS_SGX                   BIT(2)
>>>> +#define OMAP_HAS_NEON                  BIT(3)
>>>> +#define OMAP_HAS_ISP                   BIT(4)
>>>> +#define OMAP_HAS_192MHZ_CLK            BIT(5)
>>>> +#define OMAP_HAS_IO_WAKEUP             BIT(6)
>>>> +
>>>> +#define OMAP2_HAS_IVA                  OMAP_FEAT_CLASS_OMAP2 |
>>>> OMAP_HAS_IVA
>>>> +#define OMAP2_HAS_SGX                  OMAP_FEAT_CLASS_OMAP2 |
>>>> OMAP_HAS_SGX
>>>> +
>>>> +#define OMAP3_HAS_L2CACHE              OMAP_FEAT_CLASS_OMAP3 |
>>>> OMAP_HAS_L2CACHE
>>>> +#define OMAP3_HAS_IVA                  OMAP_FEAT_CLASS_OMAP3 |
>>>> OMAP_HAS_IVA
>>>> +#define OMAP3_HAS_SGX                  OMAP_FEAT_CLASS_OMAP3 |
>>>> OMAP_HAS_SGX
>>>> +#define OMAP3_HAS_NEON                 OMAP_FEAT_CLASS_OMAP3 |
>>>> OMAP_HAS_NEON
>>>> +#define OMAP3_HAS_ISP                  OMAP_FEAT_CLASS_OMAP3 |
>>>> OMAP_HAS_ISP
>>>> +#define OMAP3_HAS_192MHZ_CLK           OMAP_FEAT_CLASS_OMAP3 |
>>>> OMAP_HAS_192MHZ_CLK
>>>> +#define OMAP3_HAS_IO_WAKEUP            OMAP_FEAT_CLASS_OMAP3 |
>>>> OMAP_HAS_IOWAKEUP
>>>> +
>>>> +#define OMAP4_HAS_L2CACHE              OMAP_FEAT_CLASS_OMAP4 |
>>>> OMAP_HAS_L2CACHE
>>>> +#define OMAP4_HAS_IVA                  OMAP_FEAT_CLASS_OMAP4 |
>>>> OMAP_HAS_IVA
>>>> +#define OMAP4_HAS_SGX                  OMAP_FEAT_CLASS_OMAP4 |
>>>> OMAP_HAS_SGX
>>>> +#define OMAP4_HAS_NEON                 OMAP_FEAT_CLASS_OMAP4 |
>>>> OMAP_HAS_NEON
>>>> +#define OMAP4_HAS_ISP                  OMAP_FEAT_CLASS_OMAP4 |
>>>> OMAP_HAS_ISP
>>>>  #endif
>>>>
>>> here is my contention:
>>> there will be two ways to use this:
>>> omap_has_feature(OMAP_HAS_SGX) and omap_has_feature(OMAP3_HAS_SGX)
>>>
>>> OMAP_HAS_SGX should return true or false no matter what omap silicon it
>>> is.
>>>
>>> OMAP3_HAS_SGX usage is meant for what? it is a mixture of cpu_is_omap3()
>>> and
>>> omap_has_feature(OMAP_HAS_SGX) - tries to do two things in one shot.
>>> which
>>> defeats why we are trying to introduce a generic omap_has_feature in the
>>> first place.
>>> a) confusing as there seems to be two standards
>>> b) redundant information use cpu_is_omapxyz() if needed.
>>>
>>> IMHO:
>>> +#define OMAP_HAS_L2CACHE               BIT(0)
>>> +#define OMAP_HAS_IVA                   BIT(1)
>>> +#define OMAP_HAS_SGX                   BIT(2)
>>> +#define OMAP_HAS_NEON                  BIT(3)
>>> +#define OMAP_HAS_ISP                   BIT(4)
>>> +#define OMAP3_HAS_192MHZ_CLK           BIT(5)
>>> +#define OMAP_HAS_IO_WAKEUP             BIT(6)
>>> and later if needed
>>> +#define OMAP4_SOME_NEW_OMAP4ONLY_FEATURE BIT(7)
>>>
>>> where OMAP3_HAS is indicative that this is a OMAP3 *only* feature and
>>> should
>>> be used to differentiate between various omap3 silicon.
>>>
>>> Benefits:
>>> a) distinction b/w omap generic and omap family specific features
>>> b) you get to define 32 features instead of reserving 24-32 for OMAP
>>> classes.
>>>
>>
>> I still can't grok the need for the distinction in (a), and for
>> "" +#define OMAP4_SOME_NEW_OMAP4ONLY_FEATURE BIT(7)""  etc.
>>
>
> OMAP_HAS_192MHZ_CLK -> does not indicate if this is omap3 ONLY feature (e.g.
> 3430 does not have it, 3630 has it) but we know that omap4, 2, 1 etc dont
> need it.
>
> in terms of readability, when i see omap_has_feature(OMAP3_HAS_xyz), I can
> immediately review the code/read the code with the context of omap3 alone Vs
> if this code was used in omap4/2/1 context question why it is so and we can
> all improve.
>
> e.g. if a generic clock code meant for all omaps used 192MHZ, I would
> question why is cpu specific feature being used there. which is easier with
> a OMAP3_ tag.

If we extend this analogy, I can write
omap_dma_driver_init(OMAP3_NUM_CHANNELS)  only to make it "more
readable" and provide a omap3 specific context to the reader.
The whole point was that the user code doesn't have such klux.

>
>
>> If that OMAP4ONLY_FEATURE has to be checked, then the code
>> to use it will also be OMAP4 specific.
>>
>> IOW, as a user, there are 2 ways to use omap_has_xxxx()
>>
>> void a_generic_funciton_for_all_omaps() {
>>     if (cpu_has_xxxx_feature()
>>         /* Do generic stuff */
>> }
>>
>> void a_omap_4_specific_function()   {
>>    if (omap_has_that_new_feature()
>>          /* Do omap_4 specific stuff */
>> }
>
> See above explanation. sitting in the shoes of a reviewer who looks through
> code not written by self, it helps to have some differentiation in
> definitions to aid review.
>

    I think this 'lazy reviewability'  comes at the cost of very
abstraction  the features framework is intended to provide, not to
mention the question of correct selection (is this a OMAP4 specific
feature or is OMAP5 expected to have it ?). and upgradation.

    As mentioned before, the surrounding context of the use of
omap_has_feature() will provide enough clues about the cpu specific
nature of a feature, if at all needed.

Thanks,
Venkat.
--
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 5/5] omap: Allow testing for omap type with omap_has_feature

[Nishanth Menon]

Venkatraman S had written, on 07/08/2010 02:28 PM, the following:
> On Thu, Jul 8, 2010 at 9:58 PM, Nishanth Menon <nm@ti.com> wrote:
>> S, Venkatraman had written, on 07/08/2010 11:15 AM, the following:
>>> On Thu, Jul 8, 2010 at 8:33 PM, Nishanth Menon <nm@ti.com> wrote:
>>>> Tony Lindgren had written, on 07/08/2010 04:38 AM, the following:
>>>>> Allow testing for omap type with omap_has_feature. This
>>>>> can be used to leave out cpu_is_omapxxxx checks.
>>>>>
>>>>> Signed-off-by: Tony Lindgren <tony@atomide.com>
>>>>> ---
>>>>>  arch/arm/plat-omap/include/plat/cpu.h |   38
>>>>> ++++++++++++++++++++++++++-------
>>>>>  1 files changed, 30 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/plat-omap/include/plat/cpu.h
>>>>> b/arch/arm/plat-omap/include/plat/cpu.h
>>>>> index 96eac4d..c117c3c 100644
>>>>> --- a/arch/arm/plat-omap/include/plat/cpu.h
>>>>> +++ b/arch/arm/plat-omap/include/plat/cpu.h
>>>>> @@ -437,14 +437,36 @@ int omap_chip_is(struct omap_chip_id oci);
>>>>>  void omap2_check_revision(void);
>>>>>  /*
>>>>> - * Runtime detection of OMAP3 features
>>>>> + * Runtime detection of OMAP features
>>>>>  */
>>>>> -#define OMAP3_HAS_L2CACHE              BIT(0)
>>>>> -#define OMAP3_HAS_IVA                  BIT(1)
>>>>> -#define OMAP3_HAS_SGX                  BIT(2)
>>>>> -#define OMAP3_HAS_NEON                 BIT(3)
>>>>> -#define OMAP3_HAS_ISP                  BIT(4)
>>>>> -#define OMAP3_HAS_192MHZ_CLK           BIT(5)
>>>>> -#define OMAP3_HAS_IO_WAKEUP            BIT(6)
>>>>> +#define OMAP_FEAT_CLASS_OMAP1          BIT(24)
>>>>> +#define OMAP_FEAT_CLASS_OMAP2          BIT(25)
>>>>> +#define OMAP_FEAT_CLASS_OMAP3          BIT(26)
>>>>> +#define OMAP_FEAT_CLASS_OMAP4          BIT(27)
>>>>> +
>>>>> +#define OMAP_HAS_L2CACHE               BIT(0)
>>>>> +#define OMAP_HAS_IVA                   BIT(1)
>>>>> +#define OMAP_HAS_SGX                   BIT(2)
>>>>> +#define OMAP_HAS_NEON                  BIT(3)
>>>>> +#define OMAP_HAS_ISP                   BIT(4)
>>>>> +#define OMAP_HAS_192MHZ_CLK            BIT(5)
>>>>> +#define OMAP_HAS_IO_WAKEUP             BIT(6)
>>>>> +
>>>>> +#define OMAP2_HAS_IVA                  OMAP_FEAT_CLASS_OMAP2 |
>>>>> OMAP_HAS_IVA
>>>>> +#define OMAP2_HAS_SGX                  OMAP_FEAT_CLASS_OMAP2 |
>>>>> OMAP_HAS_SGX
>>>>> +
>>>>> +#define OMAP3_HAS_L2CACHE              OMAP_FEAT_CLASS_OMAP3 |
>>>>> OMAP_HAS_L2CACHE
>>>>> +#define OMAP3_HAS_IVA                  OMAP_FEAT_CLASS_OMAP3 |
>>>>> OMAP_HAS_IVA
>>>>> +#define OMAP3_HAS_SGX                  OMAP_FEAT_CLASS_OMAP3 |
>>>>> OMAP_HAS_SGX
>>>>> +#define OMAP3_HAS_NEON                 OMAP_FEAT_CLASS_OMAP3 |
>>>>> OMAP_HAS_NEON
>>>>> +#define OMAP3_HAS_ISP                  OMAP_FEAT_CLASS_OMAP3 |
>>>>> OMAP_HAS_ISP
>>>>> +#define OMAP3_HAS_192MHZ_CLK           OMAP_FEAT_CLASS_OMAP3 |
>>>>> OMAP_HAS_192MHZ_CLK
>>>>> +#define OMAP3_HAS_IO_WAKEUP            OMAP_FEAT_CLASS_OMAP3 |
>>>>> OMAP_HAS_IOWAKEUP
>>>>> +
>>>>> +#define OMAP4_HAS_L2CACHE              OMAP_FEAT_CLASS_OMAP4 |
>>>>> OMAP_HAS_L2CACHE
>>>>> +#define OMAP4_HAS_IVA                  OMAP_FEAT_CLASS_OMAP4 |
>>>>> OMAP_HAS_IVA
>>>>> +#define OMAP4_HAS_SGX                  OMAP_FEAT_CLASS_OMAP4 |
>>>>> OMAP_HAS_SGX
>>>>> +#define OMAP4_HAS_NEON                 OMAP_FEAT_CLASS_OMAP4 |
>>>>> OMAP_HAS_NEON
>>>>> +#define OMAP4_HAS_ISP                  OMAP_FEAT_CLASS_OMAP4 |
>>>>> OMAP_HAS_ISP
>>>>>  #endif
>>>>>
>>>> here is my contention:
>>>> there will be two ways to use this:
>>>> omap_has_feature(OMAP_HAS_SGX) and omap_has_feature(OMAP3_HAS_SGX)
>>>>
>>>> OMAP_HAS_SGX should return true or false no matter what omap silicon it
>>>> is.
>>>>
>>>> OMAP3_HAS_SGX usage is meant for what? it is a mixture of cpu_is_omap3()
>>>> and
>>>> omap_has_feature(OMAP_HAS_SGX) - tries to do two things in one shot.
>>>> which
>>>> defeats why we are trying to introduce a generic omap_has_feature in the
>>>> first place.
>>>> a) confusing as there seems to be two standards
>>>> b) redundant information use cpu_is_omapxyz() if needed.
>>>>
>>>> IMHO:
>>>> +#define OMAP_HAS_L2CACHE               BIT(0)
>>>> +#define OMAP_HAS_IVA                   BIT(1)
>>>> +#define OMAP_HAS_SGX                   BIT(2)
>>>> +#define OMAP_HAS_NEON                  BIT(3)
>>>> +#define OMAP_HAS_ISP                   BIT(4)
>>>> +#define OMAP3_HAS_192MHZ_CLK           BIT(5)
>>>> +#define OMAP_HAS_IO_WAKEUP             BIT(6)
>>>> and later if needed
>>>> +#define OMAP4_SOME_NEW_OMAP4ONLY_FEATURE BIT(7)
>>>>
>>>> where OMAP3_HAS is indicative that this is a OMAP3 *only* feature and
>>>> should
>>>> be used to differentiate between various omap3 silicon.
>>>>
>>>> Benefits:
>>>> a) distinction b/w omap generic and omap family specific features
>>>> b) you get to define 32 features instead of reserving 24-32 for OMAP
>>>> classes.
>>>>
>>> I still can't grok the need for the distinction in (a), and for
>>> "" +#define OMAP4_SOME_NEW_OMAP4ONLY_FEATURE BIT(7)""  etc.
>>>
>> OMAP_HAS_192MHZ_CLK -> does not indicate if this is omap3 ONLY feature (e.g.
>> 3430 does not have it, 3630 has it) but we know that omap4, 2, 1 etc dont
>> need it.
>>
>> in terms of readability, when i see omap_has_feature(OMAP3_HAS_xyz), I can
>> immediately review the code/read the code with the context of omap3 alone Vs
>> if this code was used in omap4/2/1 context question why it is so and we can
>> all improve.
>>
>> e.g. if a generic clock code meant for all omaps used 192MHZ, I would
>> question why is cpu specific feature being used there. which is easier with
>> a OMAP3_ tag.
> 
> If we extend this analogy, I can write
> omap_dma_driver_init(OMAP3_NUM_CHANNELS)  only to make it "more
> readable" and provide a omap3 specific context to the reader.
> The whole point was that the user code doesn't have such klux.

I must sadly disagree on this. different concept entirely.
in the case of a dma driver, num_channels is defintely something you'd 
like to abstract out. the initialization makes sense, but from a driver 
context, using omap specific - nak.

> 
>>
>>> If that OMAP4ONLY_FEATURE has to be checked, then the code
>>> to use it will also be OMAP4 specific.
>>>
>>> IOW, as a user, there are 2 ways to use omap_has_xxxx()
>>>
>>> void a_generic_funciton_for_all_omaps() {
>>>     if (cpu_has_xxxx_feature()
>>>         /* Do generic stuff */
>>> }
>>>
>>> void a_omap_4_specific_function()   {
>>>    if (omap_has_that_new_feature()
>>>          /* Do omap_4 specific stuff */
>>> }
>> See above explanation. sitting in the shoes of a reviewer who looks through
>> code not written by self, it helps to have some differentiation in
>> definitions to aid review.
>>
> 
>     I think this 'lazy reviewability'  comes at the cost of very
> abstraction  the features framework is intended to provide, not to
> mention the question of correct selection (is this a OMAP4 specific
> feature or is OMAP5 expected to have it ?). and upgradation.
> 
>     As mentioned before, the surrounding context of the use of
> omap_has_feature() will provide enough clues about the cpu specific
> nature of a feature, if at all needed.
Does it really? when a new feature is added, dont we want to know if it 
is generic feature or a omap specific feature? where is the flag?

1 year down the line, neither of us will remember this discussion in 
detail. lets take an example what might happen:

assume OMAP_HAS_192MHZ is defined - with our good intentions of being omap3.

developer Y comes along for OMAP5, sees that there is a flag for 
OMAP_HAS_192MHZ and says - cool i need this and since it is already present,

writes code for
if(omap_has_feature(OMAP_HAS_192MHZ)) {
	/* enable some cruft errata */
}

in omap5 code. guess what he posts the patch to us, a quick reviewer has 
forgotten that OMAP_HAS_192MHZ was omap3 only feature, ignores the fact 
that the feature was not enabled in omap5's check_revision.. darn.. we 
have a bug to fix.

instead, the developer is now able to create two patches:
a) convert OMAP3_HAS_192MHZ into OMAP_HAS_192MHZ after adding the omap5 
check_revision
b) do his crufty code..

same applies for reviewers.

-- 
Regards,
Nishanth Menon

Re: [PATCH 5/5] omap: Allow testing for omap type with omap_has_feature

[Tony Lindgren]

* Nishanth Menon <nm@ti.com> [100708 22:31]:
> >
> >    I think this 'lazy reviewability'  comes at the cost of very
> >abstraction  the features framework is intended to provide, not to
> >mention the question of correct selection (is this a OMAP4 specific
> >feature or is OMAP5 expected to have it ?). and upgradation.
> >
> >    As mentioned before, the surrounding context of the use of
> >omap_has_feature() will provide enough clues about the cpu specific
> >nature of a feature, if at all needed.
>
> Does it really? when a new feature is added, dont we want to know if
> it is generic feature or a omap specific feature? where is the flag?

Yeah I don't know what we should do with these defines.. Kind of just
threw the patch out there.

If we already have omap specific omap_has_feature functions, we don't
need cpu_is_omapxxxx in most cases.

I suggest we only use the generic defines now, then look at it again
when we run out of the bits to define.

Regards,

Tony

Re: [PATCH 5/5] omap: Allow testing for omap type with omap_has_feature

[Nishanth Menon]

Tony Lindgren had written, on 07/09/2010 02:04 AM, the following:
> * Nishanth Menon <nm@ti.com> [100708 22:31]:
>>>    I think this 'lazy reviewability'  comes at the cost of very
>>> abstraction  the features framework is intended to provide, not to
>>> mention the question of correct selection (is this a OMAP4 specific
>>> feature or is OMAP5 expected to have it ?). and upgradation.
>>>
>>>    As mentioned before, the surrounding context of the use of
>>> omap_has_feature() will provide enough clues about the cpu specific
>>> nature of a feature, if at all needed.
>> Does it really? when a new feature is added, dont we want to know if
>> it is generic feature or a omap specific feature? where is the flag?
> 
> Yeah I don't know what we should do with these defines.. Kind of just
> threw the patch out there.
> 
> If we already have omap specific omap_has_feature functions, we don't
> need cpu_is_omapxxxx in most cases.
> 
> I suggest we only use the generic defines now, then look at it again
> when we run out of the bits to define.
ack.

-- 
Regards,
Nishanth Menon