| Message ID | 20100708093804.16352.82935.stgit@baageli.muru.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
* 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 -- 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
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.
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