Hi,
* Premi, Sanjeev <[email protected]> [090810 20:15]:
>
>
> > -----Original Message-----
> > From: Tony Lindgren [mailto:[email protected]]
> > Sent: Friday, August 07, 2009 1:53 PM
> > To: Kevin Hilman
> > Cc: Premi, Sanjeev; [email protected]
> > Subject: Re: [PATCH 3/6] OMAP3: Add runtime check for OMAP35x
> >
>
> <snip>--<snip>
>
> > >
> > > Adding conditionals like
> > >
> > > if (omap3_has_iva2())
> > > ...
> > >
> > > and
> > >
> > > if (omap3_has_sgx())
> > > ...
> > >
> > > rather than having a long list of cpu_is checks that have
> > to be changed
> > > each time a new SoC comes out.
> >
> > Agreed.
> >
> > Tony
> >
> >
>
> Tony, Kevin,
>
> Here is a work in progress patch implementing the conditionals.
>
> I am looking for your suggestions on these:
> 1) The detection of ES2.0 is different between OMAP3530 and OMAP3430.
> For 3430 ES2.0, (idcode >> 28) == 0x0
> For 3530 ES2.0, (idcode >> 28) == 0x1
>
> What can be easy way to make this distinction?
Hmm, looks like in switch (rev) case 1 is not used, so I guess you
can use that? Hmm, might be worth checking if the 3530 ES2.0 is really
based on the 3430 ES2.1 core though..
> 2) How do we handle the power domain differences between OMAP34x
> and the new OMAP3517 and OMAP05 devices. The hawkeye is different,
> but, would it make sense to omap_revision to be like:
> 0x34050034, 0x34170034 - when we reuse the 34xx class.
Can't you use the bits documented in cpu.h:
/*
* omap_rev bits:
* CPU id bits (0730, 1510, 1710, 2422...) [31:16]
* CPU revision (See _REV_ defined in cpu.h) [15:08]
* CPU class bits (15xx, 16xx, 24xx, 34xx...) [07:00]
*/
unsigned int omap_rev(void);
Basically 0x3505??34, 0x3517??34 and so on? The cpu_is_omap34xx() is
omap_rev() & 0xff.
> 3) Currently, the macros like OMAP3430_REV_ES1_0 are defined to
> be whole numbers. I am trying to change them to something like:
>
> #define OMAP34XX_REV(type,rev) (OMAP34XX_CLASS & (type << 16) & (rev << 12))
>
> And use as (if needed):
>
> #define OMAP3430_REV_ES3_1 OMAP34XX_REV(0x30, 0x4)
>
> #define OMAP3403_REV_ES3_1 OMAP34XX_REV(0x03, 0x4)
>
> What is your view?
>
> In fact, wouldn't it be better if we tested for si rev independent
> from si type?
Well cpu_is_omap34xx() should be enough in most places, but I guess here you'd
want to test for the exact revision, and just define:
#define OMAP3505_REV_ES?_? 0x3505??34
> 4) These macros are, possibly, duplicating the data in omap_revision:
>
> #define CHIP_IS_OMAP3430ES3_0 (1 << 5)
> #define CHIP_IS_OMAP3430ES3_1 (1 << 6)
>
> But, might be used for faster checking. Is this duplication necessary?
These are currently needed for the clock framework, eventually we should
unify them..
> 5) Assuming that we are able to maintain current, macros, would this
> help in reducing the duplication?
>
> struct omap_id {
> u16 id; /* e.g. 0x3430, 0x3517, ... */
> u8 class; /* 34x */
> u8 subclass; /* 0x30, 0x03, 0x05, 0x15, 0x17 ... */
> u8 rev; /* 0x10 (for ES 1.0), 0x21 (for ES2.1) etc. */
> /* use nibble as decimal separator */
> }
Yeah we could do something like that as a separate patch eventually.
Few more comments inlined below.
>
> I have tried many approaches, before sending this mail,
> so there might be few duplications in the code below.
> The cpu_is_35xx() macros are also incomplete for now.
>
> Best regards,
> Sanjeev
>
> diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
> index a98201c..f17d4db 100644
> --- a/arch/arm/mach-omap2/id.c
> +++ b/arch/arm/mach-omap2/id.c
> @@ -25,9 +25,47 @@
> #include <mach/control.h>
> #include <mach/cpu.h>
>
> +#define OMAP3_CONTROL_OMAP_STATUS 0x044c
> +
> +#define OMAP3_SGX_SHIFT 13
> +#define OMAP3_SGX_MASK (3 << OMAP3_SGX_SHIFT)
> +#define FEAT_SGX_FULL 0
> +#define FEAT_SGX_HALF 1
> +#define FEAT_SGX_NONE 2
> +
> +#define OMAP3_IVA_SHIFT 12
> +#define OMAP3_IVA_MASK (1 << OMAP3_SGX_SHIFT)
> +#define FEAT_IVA 0
> +#define FEAT_IVA_NONE 1
> +
> +#define OMAP3_L2CACHE_SHIFT 10
> +#define OMAP3_L2CACHE_MASK (3 << OMAP3_L2CACHE_SHIFT)
> +#define FEAT_L2CACHE_0KB 0
> +#define FEAT_L2CACHE_64KB 1
> +#define FEAT_L2CACHE_128KB 2
> +#define FEAT_L2CACHE_256KB 3
> +
> +#define OMAP3_ISP_SHIFT 5
> +#define OMAP3_ISP_MASK (1<< OMAP3_ISP_SHIFT)
> +#define FEAT_ISP 0
> +#define FEAT_ISP_NONE 1
> +
> +#define OMAP3_NEON_SHIFT 4
> +#define OMAP3_NEON_MASK (1<< OMAP3_NEON_SHIFT)
> +#define FEAT_NEON 0
> +#define FEAT_NEON_NONE 1
> +
> static struct omap_chip_id omap_chip;
> static unsigned int omap_revision;
>
> +struct omap_feature {
> + u8 avail;
> + u32 attrib;
> +};
> +
> +static struct omap_feature feat_sgx;
> +static struct omap_feature feat_iva;
> +static struct omap_feature feat_l2cache;
We should probably just have static u32 omap_feat that is a bitmask
for the various features.
Then that could be moved to live in struct omap_id eventually.
> unsigned int omap_rev(void)
> {
> @@ -35,6 +73,24 @@ unsigned int omap_rev(void)
> }
> EXPORT_SYMBOL(omap_rev);
>
> +unsigned int omap3_has_sgx(void)
> +{
> + return feat_sgx.avail;
> +}
> +EXPORT_SYMBOL(omap3_has_sgx);
> +
> +unsigned int omap3_has_iva(void)
> +{
> + return feat_iva.avail;
> +}
> +EXPORT_SYMBOL(omap3_has_iva);
> +
> +unsigned int omap3_has_l2cache(void)
> +{
> + return feat_l2cache.avail;
> +}
> +EXPORT_SYMBOL(omap3_has_l2cache);
And then these become something like return omap_feat & OMAP_HAS_IVA2.
You should make the feature checking a separate patch, then the second
patch becomes simple for adding support for detecting 34xx properly.
Regards,
Tony
> +
> /**
> * omap_chip_is - test whether currently running OMAP matches a chip type
> * @oc: omap_chip_t to test against
> @@ -155,12 +211,32 @@ void __init omap24xx_check_revision(void)
> pr_info("\n");
> }
>
> -void __init omap34xx_check_revision(void)
> +void __init omap3_check_features(void)
> +{
> + u32 status;
> +
> + status = omap_ctrl_readl(OMAP3_CONTROL_OMAP_STATUS);
> +
> + /* Check for SGX */
> + feat_sgx.attrib = ((status & OMAP3_SGX_MASK) >> OMAP3_SGX_SHIFT) ;
> + feat_sgx.avail = (feat_sgx.attrib == FEAT_SGX_NONE) ? 0 : 1 ;
> +
> + /* Check for IVA */
> + feat_iva.attrib = ((status & OMAP3_IVA_MASK) >> OMAP3_IVA_SHIFT) ;
> + feat_iva.avail = (feat_iva.attrib == FEAT_IVA_NONE) ? 0 : 1 ;
> +
> + /* Check for L2 Cache */
> + feat_l2cache.attrib = ((status & OMAP3_L2CACHE_MASK) >> \
> + OMAP3_L2CACHE_SHIFT) ;
> + feat_l2cache.avail = (feat_l2cache.attrib == FEAT_L2CACHE_0KB) ? 0 : 1 ;
> +}
> +
> +void __init omap3_check_revision(void)
> {
> u32 cpuid, idcode;
> u16 hawkeye;
> u8 rev;
> - char *rev_name = "ES1.0";
> + char cpu_name[16]= "", rev_name[16] = "", feat_name[32] = "";
>
> /*
> * We cannot access revision registers on ES1.0.
> @@ -187,29 +263,50 @@ void __init omap34xx_check_revision(void)
> switch (rev) {
> case 0:
> omap_revision = OMAP3430_REV_ES2_0;
> - rev_name = "ES2.0";
> + strcat (rev_name, "ES2.0");
> break;
> case 2:
> omap_revision = OMAP3430_REV_ES2_1;
> - rev_name = "ES2.1";
> + strcat (rev_name, "ES2.1");
> break;
> case 3:
> omap_revision = OMAP3430_REV_ES3_0;
> - rev_name = "ES3.0";
> + strcat (rev_name, "ES3.0");
> break;
> case 4:
> omap_revision = OMAP3430_REV_ES3_1;
> - rev_name = "ES3.1";
> + strcat (rev_name, "ES3.1");
> break;
> default:
> /* Use the latest known revision as default */
> omap_revision = OMAP3430_REV_ES3_1;
> - rev_name = "Unknown revision\n";
> + strcat (rev_name, "Unknown revision");
> + }
> + }
> + else if (hawkeye == 0xb868) {
> + if (omap3_has_sgx()) {
> + omap_revision = OMAP35XX_REV(0x17, 0x0);
> + }
> + else {
> + omap_revision = OMAP35XX_REV(0x05, 0x0);
> }
> }
>
> out:
> - pr_info("OMAP%04x %s\n", omap_rev() >> 16, rev_name);
> + if (omap3_has_iva() && omap3_has_sgx()) {
> + strcat(cpu_name, "3430/3530");
> + }
> + else if (omap3_has_sgx()) {
> + strcat(cpu_name, "3525");
> + }
> + else if (omap3_has_iva()) {
> + strcat(cpu_name, "3515");
> + }
> + else {
> + strcat(cpu_name, "3505");
> + }
> +
> + pr_info("OMAP%s %s\n", cpu_name, rev_name);
> }
>
> /*
> @@ -223,8 +320,10 @@ void __init omap2_check_revision(void)
> */
> if (cpu_is_omap24xx())
> omap24xx_check_revision();
> - else if (cpu_is_omap34xx())
> - omap34xx_check_revision();
> + else if (cpu_is_omap34xx()) {
> + omap3_check_features();
> + omap3_check_revision();
> + }
> else if (cpu_is_omap44xx()) {
> printk(KERN_INFO "FIXME: CPU revision = OMAP4430\n");
> return;
> diff --git a/arch/arm/plat-omap/include/mach/cpu.h
> b/arch/arm/plat-omap/include/mach/cpu.h
> index 285eaa3..e9d6bc2 100644
> --- a/arch/arm/plat-omap/include/mach/cpu.h
> +++ b/arch/arm/plat-omap/include/mach/cpu.h
> @@ -363,6 +363,23 @@ IS_OMAP_TYPE(3430, 0x3430)
> #if defined(CONFIG_ARCH_OMAP34XX)
> # undef cpu_is_omap3430
> # define cpu_is_omap3430() is_omap3430()
> +
> +# undef cpu_is_omap3503
> +# undef cpu_is_omap3515
> +# undef cpu_is_omap3525
> +# undef cpu_is_omap3530
> +
> +# undef cpu_is_omap3505
> +# undef cpu_is_omap3517
> +
> +# define cpu_is_omap3503() (!omap3_has_iva() && !omap3_has_sgx())
> +# define cpu_is_omap3515() (!omap3_has_iva() && !omap3_has_sgx())
> +# define cpu_is_omap3525() (!omap3_has_iva() && !omap3_has_sgx())
> +# define cpu_is_omap3530() (omap3_has_iva() && omap3_has_sgx())
> +/* How to make these different from the ones above */
> +# define cpu_is_omap3505() (!omap3_has_iva() && !omap3_has_sgx())
> +# define cpu_is_omap3517() (!omap3_has_iva() && omap3_has_sgx())
> +
> #endif
>
> # if defined(CONFIG_ARCH_OMAP4)
> @@ -396,6 +413,12 @@ IS_OMAP_TYPE(3430, 0x3430)
> #define OMAP3430_REV_ES3_0 0x34303034
> #define OMAP3430_REV_ES3_1 0x34304034
>
> +
> +#define OMAP35XX_CLASS 0x35000035
> +
> +
> +#define OMAP35XX_REV(type,rev) (OMAP35XX_CLASS & (type << 16) & (rev
> << 12))
> +
> #define OMAP443X_CLASS 0x44300034
>
> /*
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
