[PATCH] omap: Add macros to evaluate cpu revision

[PATCH] omap: Add macros to evaluate cpu revision

[Sanjeev Premi]

This patch adds macros to evaluate the cpu revision.
These macros increase readability by reducing the
repetitive code when multiple silicon and their
revisions have to be tested.

Example usage would be:
  if (cpu_rev_eq(omap34xx, ES_1_0))

Signed-off-by: Sanjeev Premi <premi@ti.com>
---
 arch/arm/plat-omap/include/plat/cpu.h |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/cpu.h b/arch/arm/plat-omap/include/plat/cpu.h
index aa2f4f0..14b1a44 100644
--- a/arch/arm/plat-omap/include/plat/cpu.h
+++ b/arch/arm/plat-omap/include/plat/cpu.h
@@ -70,6 +70,7 @@ unsigned int omap_rev(void);
 #define OMAP_REVBITS_20		0x20
 #define OMAP_REVBITS_30		0x30
 #define OMAP_REVBITS_40		0x40
+#define OMAP_REVBITS_50		0x50
 
 /*
  * Get the CPU revision for OMAP devices
@@ -460,4 +461,35 @@ OMAP3_HAS_FEATURE(isp, ISP)
 OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
 OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
 
+/*
+ * Map revision bits to silicon specific revisions
+ */
+#define ES_1_0		OMAP_REVBITS_00
+#define ES_2_0		OMAP_REVBITS_10
+#define ES_2_1		OMAP_REVBITS_20
+#define ES_3_0		OMAP_REVBITS_30
+#define ES_3_1		OMAP_REVBITS_40
+#define ES_3_1_2	OMAP_REVBITS_50
+
+/*
+ * Macros to evaluate CPU revision
+ */
+#define cpu_rev_lt(cpu,rev)	\
+	((cpu_is_omap ##cpu() && (GET_OMAP_REVISION() < (rev))) ? 1 : 0)
+
+#define cpu_rev_le(cpu,rev)	\
+	((cpu_is_omap ##cpu() && (GET_OMAP_REVISION() <= (rev))) ? 1 : 0)
+
+#define cpu_rev_eq(cpu,rev)	\
+	((cpu_is_omap ##cpu() && (GET_OMAP_REVISION() == (rev))) ? 1 : 0)
+
+#define cpu_rev_ne(cpu,rev)	\
+	((cpu_is_omap ##cpu() && (GET_OMAP_REVISION() != (rev))) ? 1 : 0)
+
+#define cpu_rev_ge(cpu,rev)	\
+	((cpu_is_omap ##cpu() && (GET_OMAP_REVISION() >= (rev))) ? 1 : 0)
+
+#define cpu_rev_gt(cpu,rev)	\
+	((cpu_is_omap ##cpu() && (GET_OMAP_REVISION() > (rev))) ? 1 : 0)
+
 #endif
-- 
1.6.6.1

Re: [PATCH] omap: Add macros to evaluate cpu revision

[Nishanth Menon]

Premi, Sanjeev had written, on 07/21/2010 10:12 AM, the following:
> This patch adds macros to evaluate the cpu revision.
> These macros increase readability by reducing the
> repetitive code when multiple silicon and their
> revisions have to be tested.
> 
> Example usage would be:
>   if (cpu_rev_eq(omap34xx, ES_1_0))
> 
> Signed-off-by: Sanjeev Premi <premi@ti.com>
> ---
>  arch/arm/plat-omap/include/plat/cpu.h |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/include/plat/cpu.h b/arch/arm/plat-omap/include/plat/cpu.h
> index aa2f4f0..14b1a44 100644
> --- a/arch/arm/plat-omap/include/plat/cpu.h
> +++ b/arch/arm/plat-omap/include/plat/cpu.h
> @@ -70,6 +70,7 @@ unsigned int omap_rev(void);
>  #define OMAP_REVBITS_20		0x20
>  #define OMAP_REVBITS_30		0x30
>  #define OMAP_REVBITS_40		0x40
> +#define OMAP_REVBITS_50		0x50
>  
>  /*
>   * Get the CPU revision for OMAP devices
> @@ -460,4 +461,35 @@ OMAP3_HAS_FEATURE(isp, ISP)
>  OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
>  OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
>  
> +/*
> + * Map revision bits to silicon specific revisions
> + */
> +#define ES_1_0		OMAP_REVBITS_00
probably need ES_1_1, 1_2 (considering 3630)
> +#define ES_2_0		OMAP_REVBITS_10
> +#define ES_2_1		OMAP_REVBITS_20
makes sense to go to 2_2
> +#define ES_3_0		OMAP_REVBITS_30
> +#define ES_3_1		OMAP_REVBITS_40
> +#define ES_3_1_2	OMAP_REVBITS_50
3_2?

> +
> +/*
> + * Macros to evaluate CPU revision
> + */
> +#define cpu_rev_lt(cpu,rev)	\
> +	((cpu_is_omap ##cpu() && (GET_OMAP_REVISION() < (rev))) ? 1 : 0)
> +
> +#define cpu_rev_le(cpu,rev)	\
> +	((cpu_is_omap ##cpu() && (GET_OMAP_REVISION() <= (rev))) ? 1 : 0)
> +
> +#define cpu_rev_eq(cpu,rev)	\
> +	((cpu_is_omap ##cpu() && (GET_OMAP_REVISION() == (rev))) ? 1 : 0)
> +
> +#define cpu_rev_ne(cpu,rev)	\
> +	((cpu_is_omap ##cpu() && (GET_OMAP_REVISION() != (rev))) ? 1 : 0)
> +
> +#define cpu_rev_ge(cpu,rev)	\
> +	((cpu_is_omap ##cpu() && (GET_OMAP_REVISION() >= (rev))) ? 1 : 0)
> +
> +#define cpu_rev_gt(cpu,rev)	\
> +	((cpu_is_omap ##cpu() && (GET_OMAP_REVISION() > (rev))) ? 1 : 0)
> +
>  #endif


-- 
Regards,
Nishanth Menon

RE: [PATCH] omap: Add macros to evaluate cpu revision

[Gadiyar, Anand]

> > @@ -460,4 +461,35 @@ OMAP3_HAS_FEATURE(isp, ISP)
> >  OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
> >  OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
> >  
> > +/*
> > + * Map revision bits to silicon specific revisions
> > + */
> > +#define ES_1_0		OMAP_REVBITS_00
> probably need ES_1_1, 1_2 (considering 3630)


This should be okay, since the 3630 is out with
these revisions, but...

> > +#define ES_2_0		OMAP_REVBITS_10
> > +#define ES_2_1		OMAP_REVBITS_20
> makes sense to go to 2_2
> > +#define ES_3_0		OMAP_REVBITS_30
> > +#define ES_3_1		OMAP_REVBITS_40
> > +#define ES_3_1_2	OMAP_REVBITS_50
> 3_2?

This may not make sense to add now as there are no
2.2 or 3.2 revisions of any OMAP3/4 silicon?


- Anand

Re: [PATCH] omap: Add macros to evaluate cpu revision

[Nishanth Menon]

On 07/22/2010 01:53 AM, Gadiyar, Anand wrote:
>>> @@ -460,4 +461,35 @@ OMAP3_HAS_FEATURE(isp, ISP)
>>>   OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
>>>   OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
>>>
>>> +/*
>>> + * Map revision bits to silicon specific revisions
>>> + */
>>> +#define ES_1_0		OMAP_REVBITS_00
>> probably need ES_1_1, 1_2 (considering 3630)
>
>
> This should be okay, since the 3630 is out with
> these revisions, but...
>
>>> +#define ES_2_0		OMAP_REVBITS_10
>>> +#define ES_2_1		OMAP_REVBITS_20
>> makes sense to go to 2_2
>>> +#define ES_3_0		OMAP_REVBITS_30
>>> +#define ES_3_1		OMAP_REVBITS_40
>>> +#define ES_3_1_2	OMAP_REVBITS_50
>> 3_2?
>
> This may not make sense to add now as there are no
> 2.2 or 3.2 revisions of any OMAP3/4 silicon?
>
Agreed for 3 and 4, but considering this is 
arch/arm/plat-omap/include/plat/cpu.h, does it make sense in looking all 
OMAPs?

Regards,
Nishanth Menon

RE: [PATCH] omap: Add macros to evaluate cpu revision

[Premi, Sanjeev]

> -----Original Message-----
> From: Menon, Nishanth 
> Sent: Thursday, July 22, 2010 3:08 PM
> To: Gadiyar, Anand
> Cc: Premi, Sanjeev; linux-omap@vger.kernel.org
> Subject: Re: [PATCH] omap: Add macros to evaluate cpu revision
> 
> On 07/22/2010 01:53 AM, Gadiyar, Anand wrote:
> >>> @@ -460,4 +461,35 @@ OMAP3_HAS_FEATURE(isp, ISP)
> >>>   OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
> >>>   OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
> >>>
> >>> +/*
> >>> + * Map revision bits to silicon specific revisions
> >>> + */
> >>> +#define ES_1_0		OMAP_REVBITS_00
> >> probably need ES_1_1, 1_2 (considering 3630)
> >
> >
> > This should be okay, since the 3630 is out with
> > these revisions, but...
> >
> >>> +#define ES_2_0		OMAP_REVBITS_10
> >>> +#define ES_2_1		OMAP_REVBITS_20
> >> makes sense to go to 2_2
> >>> +#define ES_3_0		OMAP_REVBITS_30
> >>> +#define ES_3_1		OMAP_REVBITS_40
> >>> +#define ES_3_1_2	OMAP_REVBITS_50
> >> 3_2?
> >
> > This may not make sense to add now as there are no
> > 2.2 or 3.2 revisions of any OMAP3/4 silicon?
> >
> Agreed for 3 and 4, but considering this is 
> arch/arm/plat-omap/include/plat/cpu.h, does it make sense in 
> looking all 
> OMAPs?

In this case, the best option would be to prefix OMAP34X_/ OMAP36X_
OMAP44X_ etc and define the ES revisions for each context.

~sanjeev
> 
> Regards,
> Nishanth Menon
> 

RE: [PATCH] omap: Add macros to evaluate cpu revision

[Shilimkar, Santosh]

Sanjeev,
> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Premi, Sanjeev
> Sent: Thursday, July 22, 2010 3:20 PM
> To: Menon, Nishanth; Gadiyar, Anand
> Cc: linux-omap@vger.kernel.org
> Subject: RE: [PATCH] omap: Add macros to evaluate cpu revision
> 
> > -----Original Message-----
> > From: Menon, Nishanth
> > Sent: Thursday, July 22, 2010 3:08 PM
> > To: Gadiyar, Anand
> > Cc: Premi, Sanjeev; linux-omap@vger.kernel.org
> > Subject: Re: [PATCH] omap: Add macros to evaluate cpu revision
> >
> > On 07/22/2010 01:53 AM, Gadiyar, Anand wrote:
> > >>> @@ -460,4 +461,35 @@ OMAP3_HAS_FEATURE(isp, ISP)
> > >>>   OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
> > >>>   OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
> > >>>
> > >>> +/*
> > >>> + * Map revision bits to silicon specific revisions
> > >>> + */
> > >>> +#define ES_1_0		OMAP_REVBITS_00
> > >> probably need ES_1_1, 1_2 (considering 3630)
> > >
> > >
> > > This should be okay, since the 3630 is out with
> > > these revisions, but...
> > >
> > >>> +#define ES_2_0		OMAP_REVBITS_10
> > >>> +#define ES_2_1		OMAP_REVBITS_20
> > >> makes sense to go to 2_2
> > >>> +#define ES_3_0		OMAP_REVBITS_30
> > >>> +#define ES_3_1		OMAP_REVBITS_40
> > >>> +#define ES_3_1_2	OMAP_REVBITS_50
> > >> 3_2?
> > >
> > > This may not make sense to add now as there are no
> > > 2.2 or 3.2 revisions of any OMAP3/4 silicon?
> > >
> > Agreed for 3 and 4, but considering this is
> > arch/arm/plat-omap/include/plat/cpu.h, does it make sense in
> > looking all
> > OMAPs?
> 
> In this case, the best option would be to prefix OMAP34X_/ OMAP36X_
> OMAP44X_ etc and define the ES revisions for each context.
> 
Can please put the usecase need here. Do you want to use this for
ERRATA handling or OPP handling etc etc ?

Regards,
Santosh

Re: [PATCH] omap: Add macros to evaluate cpu revision

[Nishanth Menon]

Premi, Sanjeev had written, on 07/22/2010 04:49 AM, the following:
>> -----Original Message-----
>> From: Menon, Nishanth 
>> Sent: Thursday, July 22, 2010 3:08 PM
>> To: Gadiyar, Anand
>> Cc: Premi, Sanjeev; linux-omap@vger.kernel.org
>> Subject: Re: [PATCH] omap: Add macros to evaluate cpu revision
>>
>> On 07/22/2010 01:53 AM, Gadiyar, Anand wrote:
>>>>> @@ -460,4 +461,35 @@ OMAP3_HAS_FEATURE(isp, ISP)
>>>>>   OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
>>>>>   OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
>>>>>
>>>>> +/*
>>>>> + * Map revision bits to silicon specific revisions
>>>>> + */
>>>>> +#define ES_1_0		OMAP_REVBITS_00
>>>> probably need ES_1_1, 1_2 (considering 3630)
>>>
>>> This should be okay, since the 3630 is out with
>>> these revisions, but...
>>>
>>>>> +#define ES_2_0		OMAP_REVBITS_10
>>>>> +#define ES_2_1		OMAP_REVBITS_20
>>>> makes sense to go to 2_2
>>>>> +#define ES_3_0		OMAP_REVBITS_30
>>>>> +#define ES_3_1		OMAP_REVBITS_40
>>>>> +#define ES_3_1_2	OMAP_REVBITS_50
>>>> 3_2?
>>> This may not make sense to add now as there are no
>>> 2.2 or 3.2 revisions of any OMAP3/4 silicon?
>>>
>> Agreed for 3 and 4, but considering this is 
>> arch/arm/plat-omap/include/plat/cpu.h, does it make sense in 
>> looking all 
>> OMAPs?
> 
> In this case, the best option would be to prefix OMAP34X_/ OMAP36X_
> OMAP44X_ etc and define the ES revisions for each context.

doing that is gonna make the code real dirty looking. at the very least 
mebbe bracket it in with #ifdef  with CONFIG_OMAP2PLUS?

-- 
Regards,
Nishanth Menon

RE: [PATCH] omap: Add macros to evaluate cpu revision

[Premi, Sanjeev]

> -----Original Message-----
> From: Menon, Nishanth 
> Sent: Thursday, July 22, 2010 4:18 PM
> To: Premi, Sanjeev
> Cc: Gadiyar, Anand; linux-omap@vger.kernel.org
> Subject: Re: [PATCH] omap: Add macros to evaluate cpu revision
> 
> Premi, Sanjeev had written, on 07/22/2010 04:49 AM, the following:
> >> -----Original Message-----
> >> From: Menon, Nishanth 
> >> Sent: Thursday, July 22, 2010 3:08 PM
> >> To: Gadiyar, Anand
> >> Cc: Premi, Sanjeev; linux-omap@vger.kernel.org
> >> Subject: Re: [PATCH] omap: Add macros to evaluate cpu revision
> >>
> >> On 07/22/2010 01:53 AM, Gadiyar, Anand wrote:
> >>>>> @@ -460,4 +461,35 @@ OMAP3_HAS_FEATURE(isp, ISP)
> >>>>>   OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
> >>>>>   OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
> >>>>>
> >>>>> +/*
> >>>>> + * Map revision bits to silicon specific revisions
> >>>>> + */
> >>>>> +#define ES_1_0		OMAP_REVBITS_00
> >>>> probably need ES_1_1, 1_2 (considering 3630)
> >>>
> >>> This should be okay, since the 3630 is out with
> >>> these revisions, but...
> >>>
> >>>>> +#define ES_2_0		OMAP_REVBITS_10
> >>>>> +#define ES_2_1		OMAP_REVBITS_20
> >>>> makes sense to go to 2_2
> >>>>> +#define ES_3_0		OMAP_REVBITS_30
> >>>>> +#define ES_3_1		OMAP_REVBITS_40
> >>>>> +#define ES_3_1_2	OMAP_REVBITS_50
> >>>> 3_2?
> >>> This may not make sense to add now as there are no
> >>> 2.2 or 3.2 revisions of any OMAP3/4 silicon?
> >>>
> >> Agreed for 3 and 4, but considering this is 
> >> arch/arm/plat-omap/include/plat/cpu.h, does it make sense in 
> >> looking all 
> >> OMAPs?
> > 
> > In this case, the best option would be to prefix OMAP34X_/ OMAP36X_
> > OMAP44X_ etc and define the ES revisions for each context.
> 
> doing that is gonna make the code real dirty looking. at the 

dirty?? How come? The intent is to increase readability.

> very least 
> mebbe bracket it in with #ifdef  with CONFIG_OMAP2PLUS?

What purpose does this #ifdef. The macro should/could be used
quite generically.

Here is a sample usage from one of the patch I am reworking
for submission here:

@@ -488,7 +494,9 @@ void omap_sram_idle(void)
        * of AUTO_CNT = 1 enabled. This takes care of errata 1.142.
        * Hence store/restore the SDRC_POWER register here.
        */
-       if (omap_rev() >= OMAP3430_REV_ES3_0 &&
+       if ((cpu_is_omap3630()
+               || cpu_is_omap3505() || cpu_is_omap3517()
+               || cpu_rev_ge(34xx, OMAP34XX_ES_3_0)) &&
            omap_type() != OMAP2_DEVICE_TYPE_GP &&
            core_next_state == PWRDM_POWER_OFF)
                sdrc_pwr = sdrc_read_reg(SDRC_POWER);

Don't try to look more into the actual content of this example,
but try to use existing macros to re-implement this condition.

omap_rev() is always > OMAP3430_REV_ES3_0 for all OMAP35x devices;
even for OMAP3530 at ES2.1 level (0x35302034 > 0x34304034)

And the original condition doesn't hold good.

Try to visualize silicon revision viz. "2.1" for OMAP3505 requiring
the same example condition to be updated.

~sanjeev
> 
> -- 
> Regards,
> Nishanth Menon
> 

Re: [PATCH] omap: Add macros to evaluate cpu revision

[Nishanth Menon]

Premi, Sanjeev had written, on 07/22/2010 06:20 AM, the following:
>> -----Original Message-----
>> From: Menon, Nishanth 
>> Sent: Thursday, July 22, 2010 4:18 PM
>> To: Premi, Sanjeev
>> Cc: Gadiyar, Anand; linux-omap@vger.kernel.org
>> Subject: Re: [PATCH] omap: Add macros to evaluate cpu revision
>>
>> Premi, Sanjeev had written, on 07/22/2010 04:49 AM, the following:
>>>> -----Original Message-----
>>>> From: Menon, Nishanth 
>>>> Sent: Thursday, July 22, 2010 3:08 PM
>>>> To: Gadiyar, Anand
>>>> Cc: Premi, Sanjeev; linux-omap@vger.kernel.org
>>>> Subject: Re: [PATCH] omap: Add macros to evaluate cpu revision
>>>>
>>>> On 07/22/2010 01:53 AM, Gadiyar, Anand wrote:
>>>>>>> @@ -460,4 +461,35 @@ OMAP3_HAS_FEATURE(isp, ISP)
>>>>>>>   OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
>>>>>>>   OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
>>>>>>>
>>>>>>> +/*
>>>>>>> + * Map revision bits to silicon specific revisions
>>>>>>> + */
>>>>>>> +#define ES_1_0		OMAP_REVBITS_00
>>>>>> probably need ES_1_1, 1_2 (considering 3630)
>>>>> This should be okay, since the 3630 is out with
>>>>> these revisions, but...
>>>>>
>>>>>>> +#define ES_2_0		OMAP_REVBITS_10
>>>>>>> +#define ES_2_1		OMAP_REVBITS_20
>>>>>> makes sense to go to 2_2
>>>>>>> +#define ES_3_0		OMAP_REVBITS_30
>>>>>>> +#define ES_3_1		OMAP_REVBITS_40
>>>>>>> +#define ES_3_1_2	OMAP_REVBITS_50
>>>>>> 3_2?
>>>>> This may not make sense to add now as there are no
>>>>> 2.2 or 3.2 revisions of any OMAP3/4 silicon?
>>>>>
>>>> Agreed for 3 and 4, but considering this is 
>>>> arch/arm/plat-omap/include/plat/cpu.h, does it make sense in 
>>>> looking all 
>>>> OMAPs?
>>> In this case, the best option would be to prefix OMAP34X_/ OMAP36X_
>>> OMAP44X_ etc and define the ES revisions for each context.
>> doing that is gonna make the code real dirty looking. at the 
> 
> dirty?? How come? The intent is to increase readability.
> 
huh? should we start the old debate again?
Read this thread
http://marc.info/?l=linux-omap&m=127903615629407&w=2

>> very least 
>> mebbe bracket it in with #ifdef  with CONFIG_OMAP2PLUS?
> 
> What purpose does this #ifdef. The macro should/could be used
> quite generically.

Now we are back in a full circle -> if you would like to introduce the 
patch for ALL omap silicon, you might want to consider 2420 and so on.. 
at the very least.

introducing this for OMAP3 and 4 alone does not allow logic to scale up.

information about the cputype is already being passed as a parameter, so 
it is just a matter of figuring out which ES revs should be defined there..


> 
> Here is a sample usage from one of the patch I am reworking
> for submission here:
> 
> @@ -488,7 +494,9 @@ void omap_sram_idle(void)
>         * of AUTO_CNT = 1 enabled. This takes care of errata 1.142.
>         * Hence store/restore the SDRC_POWER register here.
>         */
> -       if (omap_rev() >= OMAP3430_REV_ES3_0 &&
> +       if ((cpu_is_omap3630()
> +               || cpu_is_omap3505() || cpu_is_omap3517()
> +               || cpu_rev_ge(34xx, OMAP34XX_ES_3_0)) &&
cpu_rev_ge(34xx, OMAP34XX_ES_3_0) -> this is the cause of my comment on 
dirty code - redundant OMAP34XX_ in the macro when I do say it is 34xx 
in the first parameter!

>             omap_type() != OMAP2_DEVICE_TYPE_GP &&
>             core_next_state == PWRDM_POWER_OFF)
>                 sdrc_pwr = sdrc_read_reg(SDRC_POWER);
> 
> Don't try to look more into the actual content of this example,
> but try to use existing macros to re-implement this condition.
> 
> omap_rev() is always > OMAP3430_REV_ES3_0 for all OMAP35x devices;
> even for OMAP3530 at ES2.1 level (0x35302034 > 0x34304034)
> 
> And the original condition doesn't hold good.
> 
> Try to visualize silicon revision viz. "2.1" for OMAP3505 requiring
> the same example condition to be updated.

I see similar potential use in enabling quirks and features (the above 
code btw could be better handled with a single variable errata which is 
populated with a flag at pm_init time..

> 
> ~sanjeev
>> -- 
>> Regards,
>> Nishanth Menon


-- 
Regards,
Nishanth Menon

RE: [PATCH] omap: Add macros to evaluate cpu revision

[Premi, Sanjeev]

 
<snip>---<snip>
> 
> > 
> > Here is a sample usage from one of the patch I am reworking
> > for submission here:
> > 
> > @@ -488,7 +494,9 @@ void omap_sram_idle(void)
> >         * of AUTO_CNT = 1 enabled. This takes care of errata 1.142.
> >         * Hence store/restore the SDRC_POWER register here.
> >         */
> > -       if (omap_rev() >= OMAP3430_REV_ES3_0 &&
> > +       if ((cpu_is_omap3630()
> > +               || cpu_is_omap3505() || cpu_is_omap3517()
> > +               || cpu_rev_ge(34xx, OMAP34XX_ES_3_0)) &&
> cpu_rev_ge(34xx, OMAP34XX_ES_3_0) -> this is the cause of my 
> comment on 
> dirty code - redundant OMAP34XX_ in the macro when I do say 
> it is 34xx 
> in the first parameter!
> 
> >             omap_type() != OMAP2_DEVICE_TYPE_GP &&
> >             core_next_state == PWRDM_POWER_OFF)
> >                 sdrc_pwr = sdrc_read_reg(SDRC_POWER);
> > 
> > Don't try to look more into the actual content of this example,
> > but try to use existing macros to re-implement this condition.
> > 
> > omap_rev() is always > OMAP3430_REV_ES3_0 for all OMAP35x devices;
> > even for OMAP3530 at ES2.1 level (0x35302034 > 0x34304034)
> > 
> > And the original condition doesn't hold good.
> > 
> > Try to visualize silicon revision viz. "2.1" for OMAP3505 requiring
> > the same example condition to be updated.
> 
> I see similar potential use in enabling quirks and features 
> (the above 
> code btw could be better handled with a single variable 
> errata which is 
> populated with a flag at pm_init time..
> 

Will have detailed response later...
But, 3530, 3505 and 3517 are not erratas. These are different silicons.
AND this patch has no relation to power management.

More and more silicons are going to be added to linux-omap tree as
they are based. They all will follow their own revision lifecycle.

I have no issues to go back and add the 2420 etc. but I don't have
much information on their revision history. If you provide me info,
I can update the patch OR you can submit a follow-up patch with
related changes. The macros don't change due to addition of additional
silicon revisions.

~sanjeev

> > 
> > ~sanjeev
> >> -- 
> >> Regards,
> >> Nishanth Menon
> 
> 
> -- 
> Regards,
> Nishanth Menon
> 

RE: [PATCH] omap: Add macros to evaluate cpu revision

[Premi, Sanjeev]

> -----Original Message-----
> From: Menon, Nishanth 
> Sent: Thursday, July 22, 2010 5:16 PM
> To: Premi, Sanjeev
> Cc: Gadiyar, Anand; linux-omap@vger.kernel.org
> Subject: Re: [PATCH] omap: Add macros to evaluate cpu revision
> 
> Premi, Sanjeev had written, on 07/22/2010 06:20 AM, the following:
> >> -----Original Message-----
> >> From: Menon, Nishanth 
> >> Sent: Thursday, July 22, 2010 4:18 PM
> >> To: Premi, Sanjeev
> >> Cc: Gadiyar, Anand; linux-omap@vger.kernel.org
> >> Subject: Re: [PATCH] omap: Add macros to evaluate cpu revision
> >>
> >> Premi, Sanjeev had written, on 07/22/2010 04:49 AM, the following:
> >>>> -----Original Message-----
> >>>> From: Menon, Nishanth 
> >>>> Sent: Thursday, July 22, 2010 3:08 PM
> >>>> To: Gadiyar, Anand
> >>>> Cc: Premi, Sanjeev; linux-omap@vger.kernel.org
> >>>> Subject: Re: [PATCH] omap: Add macros to evaluate cpu revision
> >>>>
> >>>> On 07/22/2010 01:53 AM, Gadiyar, Anand wrote:
> >>>>>>> @@ -460,4 +461,35 @@ OMAP3_HAS_FEATURE(isp, ISP)
> >>>>>>>   OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
> >>>>>>>   OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
> >>>>>>>
> >>>>>>> +/*
> >>>>>>> + * Map revision bits to silicon specific revisions
> >>>>>>> + */
> >>>>>>> +#define ES_1_0		OMAP_REVBITS_00
> >>>>>> probably need ES_1_1, 1_2 (considering 3630)
> >>>>> This should be okay, since the 3630 is out with
> >>>>> these revisions, but...
> >>>>>
> >>>>>>> +#define ES_2_0		OMAP_REVBITS_10
> >>>>>>> +#define ES_2_1		OMAP_REVBITS_20
> >>>>>> makes sense to go to 2_2
> >>>>>>> +#define ES_3_0		OMAP_REVBITS_30
> >>>>>>> +#define ES_3_1		OMAP_REVBITS_40
> >>>>>>> +#define ES_3_1_2	OMAP_REVBITS_50
> >>>>>> 3_2?
> >>>>> This may not make sense to add now as there are no
> >>>>> 2.2 or 3.2 revisions of any OMAP3/4 silicon?
> >>>>>
> >>>> Agreed for 3 and 4, but considering this is 
> >>>> arch/arm/plat-omap/include/plat/cpu.h, does it make sense in 
> >>>> looking all 
> >>>> OMAPs?
> >>> In this case, the best option would be to prefix 
> OMAP34X_/ OMAP36X_
> >>> OMAP44X_ etc and define the ES revisions for each context.
> >> doing that is gonna make the code real dirty looking. at the 
> > 
> > dirty?? How come? The intent is to increase readability.
> > 
> huh? should we start the old debate again?
> Read this thread
> http://marc.info/?l=linux-omap&m=127903615629407&w=2
> 
> >> very least 
> >> mebbe bracket it in with #ifdef  with CONFIG_OMAP2PLUS?
> > 
> > What purpose does this #ifdef. The macro should/could be used
> > quite generically.
> 
> Now we are back in a full circle -> if you would like to 
> introduce the 
> patch for ALL omap silicon, you might want to consider 2420 
> and so on.. 
> at the very least.
> 
> introducing this for OMAP3 and 4 alone does not allow logic 
> to scale up.

[sp] The logic is only in the macros viz. cpu_rev_ge(), cpu_rev_le,
     etc. The support for other omap silicons means having the
     mapping of the revision bits to actual silicon version.

> 
> information about the cputype is already being passed as a 
> parameter, so 
> it is just a matter of figuring out which ES revs should be 
> defined there..

[sp] I have been trying to avoid creating another set of functions
     or introduce new data structures for the purpose. This means
     living with the problem of lower/uppercase in the definition
     e.g. cpu_is_omap34xx() and #define OMAP334XX_(*) macros.

     An added complexity is checking for "family" of devices and
     "specific" device in a family. 

    I am tried to limit the changes introduced and still keep the
    overall code readable - not necessarily reduction in code size.

> 
> > 
> > Here is a sample usage from one of the patch I am reworking
> > for submission here:
> > 
> > @@ -488,7 +494,9 @@ void omap_sram_idle(void)
> >         * of AUTO_CNT = 1 enabled. This takes care of errata 1.142.
> >         * Hence store/restore the SDRC_POWER register here.
> >         */
> > -       if (omap_rev() >= OMAP3430_REV_ES3_0 &&
> > +       if ((cpu_is_omap3630()
> > +               || cpu_is_omap3505() || cpu_is_omap3517()
> > +               || cpu_rev_ge(34xx, OMAP34XX_ES_3_0)) &&
> cpu_rev_ge(34xx, OMAP34XX_ES_3_0) -> this is the cause of my 
> comment on 
> dirty code - redundant OMAP34XX_ in the macro when I do say 
> it is 34xx 
> in the first parameter!
> 

[sp] Dirtiness is in eye of beholder :)
     I did say earlier, that the patch is meant for increasing
     readability. I could have overcome this by using lowercase
     for revision macros.

     I did think of "exploiting" enums; but had been avoiding
     the need for adding new data structures. It, however, can
     be ugly!

> >             omap_type() != OMAP2_DEVICE_TYPE_GP &&
> >             core_next_state == PWRDM_POWER_OFF)
> >                 sdrc_pwr = sdrc_read_reg(SDRC_POWER);
> > 
> > Don't try to look more into the actual content of this example,
> > but try to use existing macros to re-implement this condition.
> > 
> > omap_rev() is always > OMAP3430_REV_ES3_0 for all OMAP35x devices;
> > even for OMAP3530 at ES2.1 level (0x35302034 > 0x34304034)
> > 
> > And the original condition doesn't hold good.
> > 
> > Try to visualize silicon revision viz. "2.1" for OMAP3505 requiring
> > the same example condition to be updated.
> 
> I see similar potential use in enabling quirks and features 
> (the above 
> code btw could be better handled with a single variable 
> errata which is 
> populated with a flag at pm_init time..

[sp] The need for detection of cpu revision arises much before pm_init()
     is executed. The code snippet above was just an illustration. It is
     neither the first nor the only place where the revision comparison
     is necessary.

     I, possibly, did mention in an earlier comment that OMAP3505 and
     OMAP3530 are way too different to be handled as simple "errata".

> 
> > 
> > ~sanjeev
> >> -- 
> >> Regards,
> >> Nishanth Menon
> 
> 
> -- 
> Regards,
> Nishanth Menon
> 

RE: [PATCH] omap: Add macros to evaluate cpu revision

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Premi, Sanjeev
> Sent: Monday, July 26, 2010 8:57 PM
> To: Menon, Nishanth
> Cc: Gadiyar, Anand; linux-omap@vger.kernel.org
> Subject: RE: [PATCH] omap: Add macros to evaluate cpu revision
>
 
[snip] [snip]

> > >>>>>>> +#define ES_2_0		OMAP_REVBITS_10
> > >>>>>>> +#define ES_2_1		OMAP_REVBITS_20
> > >>>>>> makes sense to go to 2_2
> > >>>>>>> +#define ES_3_0		OMAP_REVBITS_30
> > >>>>>>> +#define ES_3_1		OMAP_REVBITS_40
> > >>>>>>> +#define ES_3_1_2	OMAP_REVBITS_50
> > >>>>>> 3_2?
> > >>>>> This may not make sense to add now as there are no
> > >>>>> 2.2 or 3.2 revisions of any OMAP3/4 silicon?
> > >>>>>
> > >>>> Agreed for 3 and 4, but considering this is 
> > >>>> arch/arm/plat-omap/include/plat/cpu.h, does it make sense in 
> > >>>> looking all 
> > >>>> OMAPs?
> > >>> In this case, the best option would be to prefix 
> > OMAP34X_/ OMAP36X_
> > >>> OMAP44X_ etc and define the ES revisions for each context.
> > >> doing that is gonna make the code real dirty looking. at the 
> > > 
> > > dirty?? How come? The intent is to increase readability.
> > > 
> > huh? should we start the old debate again?
> > Read this thread
> > http://marc.info/?l=linux-omap&m=127903615629407&w=2
> > 
> > >> very least 
> > >> mebbe bracket it in with #ifdef  with CONFIG_OMAP2PLUS?
> > > 
> > > What purpose does this #ifdef. The macro should/could be used
> > > quite generically.
> > 
> > Now we are back in a full circle -> if you would like to 
> > introduce the 
> > patch for ALL omap silicon, you might want to consider 2420 
> > and so on.. 
> > at the very least.
> > 
> > introducing this for OMAP3 and 4 alone does not allow logic 
> > to scale up.
> 
> [sp] The logic is only in the macros viz. cpu_rev_ge(), cpu_rev_le,
>      etc. The support for other omap silicons means having the
>      mapping of the revision bits to actual silicon version.
> 

[snip] [snip]

> > > 
> > > Here is a sample usage from one of the patch I am reworking
> > > for submission here:
> > > 
> > > @@ -488,7 +494,9 @@ void omap_sram_idle(void)
> > >         * of AUTO_CNT = 1 enabled. This takes care of 
> errata 1.142.
> > >         * Hence store/restore the SDRC_POWER register here.
> > >         */
> > > -       if (omap_rev() >= OMAP3430_REV_ES3_0 &&
> > > +       if ((cpu_is_omap3630()
> > > +               || cpu_is_omap3505() || cpu_is_omap3517()
> > > +               || cpu_rev_ge(34xx, OMAP34XX_ES_3_0)) &&
> > cpu_rev_ge(34xx, OMAP34XX_ES_3_0) -> this is the cause of my 
> > comment on 
> > dirty code - redundant OMAP34XX_ in the macro when I do say 
> > it is 34xx 
> > in the first parameter!
> > 
> 
> [sp] Dirtiness is in eye of beholder :)
>      I did say earlier, that the patch is meant for increasing
>      readability. I could have overcome this by using lowercase
>      for revision macros.
> 
>      I did think of "exploiting" enums; but had been avoiding
>      the need for adding new data structures. It, however, can
>      be ugly!
> 

[snip] [snip]

Below is an attempt to introduce enums to take care of uppercase
bit definitions vs lowercase function definitions. Of course it
is not a formal patch and I have tried to limit the patch only
for additions; not for deletions that can result due from the
enums declares below.

The revisions can now be done as:
 - cpu_rev_ge(omap34xx, es3_0)
 - cpu_rev_eq(am3505, es1_0)
 - etc.

Also, I am using revision bit values instead of another macro
representing them (e.g. OMAP_REVISION_BITS_10). Given the
structure below, I felt use of actual bit values is better.

Nishanth:
There may be copy-paste errors in the actual revision definitions
(let us not deviate into those for now).

~sanjeev


diff --git a/arch/arm/plat-omap/include/plat/cpu.h b/arch/arm/plat-omap/include/plat/cpu.h
index 2e2ae53..36a7047
--- a/arch/arm/plat-omap/include/plat/cpu.h
+++ b/arch/arm/plat-omap/include/plat/cpu.h
@@ -467,4 +469,103 @@ OMAP3_HAS_FEATURE(isp, ISP)
 OMAP3_HAS_FEATURE(192mhz_clk, 192MHZ_CLK)
 OMAP3_HAS_FEATURE(io_wakeup, IO_WAKEUP)
 
+/*
+ * Enumerate the CPU revisions for easy comparison against the
+ * revision bits specific for each processor family.
+ */
+#define DECLARE_CPU_REV(cpu)	enum revs_ ##cpu
+#define CPU_REV(cpu,rev,bits)	cpu## _ ##rev = bits
+
+DECLARE_CPU_REV(omap242x) {
+	CPU_REV(omap242x, es1_0, 0x00),
+	CPU_REV(omap242x, es2_0, 0x01),
+} ;
+
+DECLARE_CPU_REV(omap243x) {
+	CPU_REV(omap243x, es1_0, 0x00),
+} ;
+
+DECLARE_CPU_REV(omap34xx) {
+	CPU_REV(omap34xx, es1_0, 0x00),
+	CPU_REV(omap34xx, es2_0, 0x01),
+	CPU_REV(omap34xx, es2_1, 0x02),
+	CPU_REV(omap34xx, es3_0, 0x03),
+	CPU_REV(omap34xx, es3_1, 0x04),
+	CPU_REV(omap34xx, es3_1_2, 0x05),
+} ;
+
+DECLARE_CPU_REV(omap36xx) {
+	CPU_REV(omap36xx, es1_0, 0x00),
+	CPU_REV(omap36xx, es1_1, 0x01),
+} ;
+
+DECLARE_CPU_REV(omap3503) {
+	CPU_REV(omap3503, es1_0, 0x00),
+	CPU_REV(omap3503, es2_0, 0x01),
+	CPU_REV(omap3503, es2_1, 0x02),
+	CPU_REV(omap3503, es3_0, 0x03),
+	CPU_REV(omap3503, es3_1, 0x04),
+} ;
+
+DECLARE_CPU_REV(omap3515) {
+	CPU_REV(omap3515, es1_0, 0x00),
+	CPU_REV(omap3515, es2_0, 0x01),
+	CPU_REV(omap3515, es2_1, 0x02),
+	CPU_REV(omap3515, es3_0, 0x03),
+	CPU_REV(omap3515, es3_1, 0x04),
+} ;
+
+DECLARE_CPU_REV(omap3525) {
+	CPU_REV(omap3525, es1_0, 0x00),
+	CPU_REV(omap3525, es2_0, 0x01),
+	CPU_REV(omap3525, es2_1, 0x02),
+	CPU_REV(omap3525, es3_0, 0x03),
+	CPU_REV(omap3525, es3_1, 0x04),
+} ;
+
+DECLARE_CPU_REV(omap3530) {
+	CPU_REV(omap3530, es1_0, 0x00),
+	CPU_REV(omap3530, es2_0, 0x01),
+	CPU_REV(omap3530, es2_1, 0x02),
+	CPU_REV(omap3530, es3_0, 0x03),
+	CPU_REV(omap3530, es3_1, 0x04),
+} ;
+
+DECLARE_CPU_REV(am3505) {
+	CPU_REV(am3505, es1_0, 0x00),
+	CPU_REV(am3505, es1_1, 0x01),
+} ;
+
+DECLARE_CPU_REV(am3517) {
+	CPU_REV(am3517, es1_0, 0x00),
+	CPU_REV(am3517, es1_1, 0x01),
+} ;
+
+/*
+ * Macros to evaluate CPU revision
+ */
+#define cpu_rev_lt(cpu,rev)	\
+	((cpu_is_omap ##cpu()	\
+		&& (GET_OMAP_REVISION() < cpu## _ ##rev)) ? 1 : 0)
+
+#define cpu_rev_le(cpu,rev)	\
+	((cpu_is_omap ##cpu()	\
+		&& (GET_OMAP_REVISION() <= cpu## _ ##rev)) ? 1 : 0)
+
+#define cpu_rev_eq(cpu,rev)	\
+	((cpu_is_omap ##cpu()	\
+		&& (GET_OMAP_REVISION() == cpu## _ ##rev)) ? 1 : 0)
+
+#define cpu_rev_ne(cpu,rev)	\
+	((cpu_is_omap ##cpu()	\
+		&& (GET_OMAP_REVISION() != cpu## _ ##rev)) ? 1 : 0)
+
+#define cpu_rev_ge(cpu,rev)	\
+	((cpu_is_omap ##cpu()	\
+		&& (GET_OMAP_REVISION() >= cpu## _ ##rev)) ? 1 : 0)
+
+#define cpu_rev_gt(cpu,rev)	\
+	((cpu_is_omap ##cpu()	\
+		&& (GET_OMAP_REVISION() > cpu## _ ##rev)) ? 1 : 0)
+
 #endif

RE: [PATCH] omap: Add macros to evaluate cpu revision

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Premi, Sanjeev
> Sent: Thursday, August 12, 2010 8:21 PM
> To: Menon, Nishanth
> Cc: Gadiyar, Anand; linux-omap@vger.kernel.org
> Subject: RE: [PATCH] omap: Add macros to evaluate cpu revision
> 
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org 
> > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of 
> Premi, Sanjeev
> > Sent: Monday, July 26, 2010 8:57 PM
> > To: Menon, Nishanth
> > Cc: Gadiyar, Anand; linux-omap@vger.kernel.org
> > Subject: RE: [PATCH] omap: Add macros to evaluate cpu revision
> >
>  
> [snip] [snip]
> 
> > > >>>>>>> +#define ES_2_0		OMAP_REVBITS_10
> > > >>>>>>> +#define ES_2_1		OMAP_REVBITS_20
> > > >>>>>> makes sense to go to 2_2
> > > >>>>>>> +#define ES_3_0		OMAP_REVBITS_30
> > > >>>>>>> +#define ES_3_1		OMAP_REVBITS_40
> > > >>>>>>> +#define ES_3_1_2	OMAP_REVBITS_50
> > > >>>>>> 3_2?
> > > >>>>> This may not make sense to add now as there are no
> > > >>>>> 2.2 or 3.2 revisions of any OMAP3/4 silicon?
> > > >>>>>
> > > >>>> Agreed for 3 and 4, but considering this is 
> > > >>>> arch/arm/plat-omap/include/plat/cpu.h, does it make sense in 
> > > >>>> looking all 
> > > >>>> OMAPs?
> > > >>> In this case, the best option would be to prefix 
> > > OMAP34X_/ OMAP36X_
> > > >>> OMAP44X_ etc and define the ES revisions for each context.
> > > >> doing that is gonna make the code real dirty looking. at the 
> > > > 
> > > > dirty?? How come? The intent is to increase readability.
> > > > 
> > > huh? should we start the old debate again?
> > > Read this thread
> > > http://marc.info/?l=linux-omap&m=127903615629407&w=2
> > > 
> > > >> very least 
> > > >> mebbe bracket it in with #ifdef  with CONFIG_OMAP2PLUS?
> > > > 
> > > > What purpose does this #ifdef. The macro should/could be used
> > > > quite generically.
> > > 
> > > Now we are back in a full circle -> if you would like to 
> > > introduce the 
> > > patch for ALL omap silicon, you might want to consider 2420 
> > > and so on.. 
> > > at the very least.
> > > 
> > > introducing this for OMAP3 and 4 alone does not allow logic 
> > > to scale up.
> > 
> > [sp] The logic is only in the macros viz. cpu_rev_ge(), cpu_rev_le,
> >      etc. The support for other omap silicons means having the
> >      mapping of the revision bits to actual silicon version.
> > 
> 
> [snip] [snip]
> 
> > > > 
> > > > Here is a sample usage from one of the patch I am reworking
> > > > for submission here:
> > > > 
> > > > @@ -488,7 +494,9 @@ void omap_sram_idle(void)
> > > >         * of AUTO_CNT = 1 enabled. This takes care of 
> > errata 1.142.
> > > >         * Hence store/restore the SDRC_POWER register here.
> > > >         */
> > > > -       if (omap_rev() >= OMAP3430_REV_ES3_0 &&
> > > > +       if ((cpu_is_omap3630()
> > > > +               || cpu_is_omap3505() || cpu_is_omap3517()
> > > > +               || cpu_rev_ge(34xx, OMAP34XX_ES_3_0)) &&
> > > cpu_rev_ge(34xx, OMAP34XX_ES_3_0) -> this is the cause of my 
> > > comment on 
> > > dirty code - redundant OMAP34XX_ in the macro when I do say 
> > > it is 34xx 
> > > in the first parameter!
> > > 
> > 
> > [sp] Dirtiness is in eye of beholder :)
> >      I did say earlier, that the patch is meant for increasing
> >      readability. I could have overcome this by using lowercase
> >      for revision macros.
> > 
> >      I did think of "exploiting" enums; but had been avoiding
> >      the need for adding new data structures. It, however, can
> >      be ugly!
> > 
> 
> [snip] [snip]
> 
> Below is an attempt to introduce enums to take care of uppercase
> bit definitions vs lowercase function definitions. Of course it
> is not a formal patch and I have tried to limit the patch only
> for additions; not for deletions that can result due from the
> enums declares below.
> 
> The revisions can now be done as:
>  - cpu_rev_ge(omap34xx, es3_0)
>  - cpu_rev_eq(am3505, es1_0)
>  - etc.
> 
> Also, I am using revision bit values instead of another macro
> representing them (e.g. OMAP_REVISION_BITS_10). Given the
> structure below, I felt use of actual bit values is better.
> 
> Nishanth:
> There may be copy-paste errors in the actual revision definitions
> (let us not deviate into those for now).
> 
> ~sanjeev
> 
> 

Did not see any comment on the proposal. Will go ahead and submit
as formal patch tomorrow.

~sanjeev


[snip][snip] the diff for illustration removed [snip][snip]