On Thu, Jul 8, 2010 at 8:33 PM, Nishanth Menon <nm@xxxxxx> 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@xxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
