Nishanth Menon <menon.nishanth@xxxxxxxxx> wrote:
> On 05/31/2010 07:06 PM, Venkatraman S wrote:
>>
>> Nishanth Menon<nm@xxxxxx> wrote:
>>>
>>> On 05/27/2010 01:24 PM, S, Venkatraman wrote:
>>>>
>>>> Nishanth Menon<nm@xxxxxx> 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.
>
It is this..
> or are you contending that since I did not add the OMAP2,4 logic
> to detect the same, this patch is not valid?
I understand that you might not have all platforms to test with, but
let's not provide a
'generic feature api' without it being available for the supported platforms.
It's incomplete without it.
Regards,
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
