Re: [PATCH 6/6] omap: move generic omap3 features to generic

[Nishanth Menon <menon.nishanth@gmail.com>]

On 05/31/2010 07:06 PM, Venkatraman S wrote:
> Nishanth Menon<nm@ti.com>  wrote:
>> On 05/27/2010 01:24 PM, S, Venkatraman wrote:
>>>
>>> Nishanth Menon<nm@ti.com>    wrote:
>>
>> [..]
>>>>
>>>> diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
>>>> index 809e13a..01555b6 100644
>>>> --- a/arch/arm/mach-omap2/id.c
>>>> +++ b/arch/arm/mach-omap2/id.c
>>>> @@ -161,7 +161,7 @@ static void __init omap24xx_check_revision(void)
>>>>   #define OMAP3_CHECK_FEATURE(status,feat)                               \
>>>>         if (((status&    OMAP3_ ##feat## _MASK)                           \
>>>>                 >>    OMAP3_ ##feat## _SHIFT) != FEAT_ ##feat## _NONE) {   \
>>>> -               omap3_features |= OMAP3_HAS_ ##feat;                    \
>>>> +               omap_features |= OMAP_HAS_ ##feat;                      \
>>>>         }
>>>
>>> "CHECK" sounds like a querying API, whereas the macro populates data.
>>> Maybe UPDATE or SET ?
>>>
>> Depends on where you are looking at it from: overall it is checking the
>> status bits from OMAP and deciding what features it has - this is
>> specifically important for 35xx series of processors. it is indeed a check
>> in that sense. if you look at it from features variable, yeah it is updating
>> it, but the idea of usage of the Macro is: check in status if feature X is
>> available.. which is what it does ;). btw, the intent of the current patch
>> was not meant to rename CHECK_FEATURE as it was very OMAP3 specific ;)
>>
>
> It's ok.  I don't want to worry too much about the name.
Thanks.
>
>>>>   static void __init omap3_check_features(void)
>>>> @@ -310,20 +310,20 @@ static void __init omap3_cpuinfo(void)
>>>>                 /*
>>
>> [...]
>>
>>>> +OMAP_HAS_FEATURE(, l2cache, L2CACHE)
>>>> +OMAP_HAS_FEATURE(, sgx, SGX)
>>>> +OMAP_HAS_FEATURE(, iva, IVA)
>>>> +OMAP_HAS_FEATURE(, neon, NEON)
>>>> +OMAP_HAS_FEATURE(, isp, ISP)
>>>> +
>>>> +/*
>>>>   * 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)
>>>> -#define OMAP3_HAS_NEON                 BIT(3)
>>>> -#define OMAP3_HAS_ISP                  BIT(4)
>>>>   #define OMAP3_HAS_192MHZ_CLK           BIT(5)
>>>>
>>>> -OMAP_HAS_FEATURE(3, l2cache, L2CACHE)
>>>> -OMAP_HAS_FEATURE(3, sgx, SGX)
>>>> -OMAP_HAS_FEATURE(3, iva, IVA)
>>>> -OMAP_HAS_FEATURE(3, neon, NEON)
>>>> -OMAP_HAS_FEATURE(3, isp, ISP)
>>>>   OMAP_HAS_FEATURE(3, 192mhz_clk, 192MHZ_CLK)
>>>>
>>>>   #endif
>>>> --
>>>
>>> What about feature detection for OMAP2 and OMAP4 (similar to
>>> omap3_check_features) ?
>>> At least a dummy implementation with warning messages would be good,
>>> so that they are not used without initialization.
>>
>> there is no need for warning messages, they will return as feature not
>> present. cpu.h is a common header and variable omap_features is in common.c,
>> the check_feature and id.c has not set that bit, the variable will remain 0,
>> hence omap_has_sgx() will return 0 unless someone enables that for lets say
>> OMAP4 ->  feel free to do it, as I mentioned in 0/6, this series was meant
>> solely to reorganize and provide an infrastructure for further development.
>>
>
> I don't agree. The patch / series is about making them 'generic', where generic
> is, to the minimum, "making the features exposed for major silicon versions".
> If they are not usable beyond OMAP3, it's not generic.
>
Which part is not generic? I am unable to comprehend your contention 
here. Are you contending that ISP, l2cache, neon, isp are *not* generic 
OMAP features? or are you contending that since I did not add the 
OMAP2,4 logic to detect the same, this patch is not valid?
all silicon revisions CAN use the function omap_has_sgx() now, even 
though I have not added detection logic to actually populate the same. 
if you do have a patch for detecting actual OMAP4/2 features feel free 
to add to the patch list, I am more than happy to ack them, if they look 
good - I personally dont have a omap4 platform at the very moment to 
write and post a patch - however trivial it may be.

Regards,
Nishanth Menon