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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux