[U-Boot] [PATCH] mx6: Fix the reading of CPU revision

[U-Boot] [PATCH] mx6: Fix the reading of CPU revision

[Fabio Estevam]

Currently when booting a mx6 solo processor get_cpu_rev() returns 0x62xxx, which
is an invalid mx6 CPU revision. This causes run-time problems when trying to use
VPU library in the kernel, as this library loads the VPU firmware according
to the CPU type.

Fix get_cpu_rev() so that it correctly returns 0x61xxx for a mx6 solo.

While at it, also remove the duplicate definitions for MXC_CPU_ types.

Tested on a Wandboard solo and on a mx6qsabresd.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/cpu/armv7/mx6/soc.c              |   28 ++++++++++++----------------
 arch/arm/imx-common/cpu.c                 |   16 ++++++++++------
 arch/arm/include/asm/arch-mx5/sys_proto.h |    7 -------
 arch/arm/include/asm/arch-mx6/sys_proto.h |    7 -------
 4 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/arch/arm/cpu/armv7/mx6/soc.c b/arch/arm/cpu/armv7/mx6/soc.c
index 193ba12..87725eb 100644
--- a/arch/arm/cpu/armv7/mx6/soc.c
+++ b/arch/arm/cpu/armv7/mx6/soc.c
@@ -43,22 +43,18 @@ struct scu_regs {
 u32 get_cpu_rev(void)
 {
 	struct anatop_regs *anatop = (struct anatop_regs *)ANATOP_BASE_ADDR;
-	u32 reg = readl(&anatop->digprog_sololite);
-	u32 type = ((reg >> 16) & 0xff);
-
-	if (type != MXC_CPU_MX6SL) {
-		reg = readl(&anatop->digprog);
-		type = ((reg >> 16) & 0xff);
-		if (type == MXC_CPU_MX6DL) {
-			struct scu_regs *scu = (struct scu_regs *)SCU_BASE_ADDR;
-			u32 cfg = readl(&scu->config) & 3;
-
-			if (!cfg)
-				type = MXC_CPU_MX6SOLO;
-		}
-	}
-	reg &= 0xff;		/* mx6 silicon revision */
-	return (type << 12) | (reg + 0x10);
+	u32 fsl_system_rev;
+	u32 cpu_rev = readl(&anatop->digprog);
+
+	/* Chip Silicon ID */
+	fsl_system_rev = ((cpu_rev >> 16) & 0xFF) << 12;
+	/* Chip silicon major revision */
+	fsl_system_rev |= ((cpu_rev >> 8) & 0xFF) << 4;
+	fsl_system_rev += 0x10;
+	/* Chip silicon minor revision */
+	fsl_system_rev |= cpu_rev & 0xFF;
+
+	return fsl_system_rev;
 }
 
 void init_aips(void)
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
index a9b86c1..2f518c5 100644
--- a/arch/arm/imx-common/cpu.c
+++ b/arch/arm/imx-common/cpu.c
@@ -117,15 +117,19 @@ unsigned imx_ddr_size(void)
 
 #if defined(CONFIG_DISPLAY_CPUINFO)
 
+#define MXC_CPU_MX51		0x51
+#define MXC_CPU_MX53		0x53
+#define MXC_CPU_MX6SL		0x60
+#define MXC_CPU_MX6DL_S		0x61
+#define MXC_CPU_MX6Q_D		0x63
+
 const char *get_imx_type(u32 imxtype)
 {
 	switch (imxtype) {
-	case MXC_CPU_MX6Q:
-		return "6Q";	/* Quad-core version of the mx6 */
-	case MXC_CPU_MX6DL:
-		return "6DL";	/* Dual Lite version of the mx6 */
-	case MXC_CPU_MX6SOLO:
-		return "6SOLO";	/* Solo version of the mx6 */
+	case MXC_CPU_MX6Q_D:
+		return "6Q/D";	/* Quad/Dual version of the mx6 */
+	case MXC_CPU_MX6DL_S:
+		return "6DL/S";	/* Dual-Lite/Solo version of the mx6 */
 	case MXC_CPU_MX6SL:
 		return "6SL";	/* Solo-Lite version of the mx6 */
 	case MXC_CPU_MX51:
diff --git a/arch/arm/include/asm/arch-mx5/sys_proto.h b/arch/arm/include/asm/arch-mx5/sys_proto.h
index 93ad1c6..a2e88bb 100644
--- a/arch/arm/include/asm/arch-mx5/sys_proto.h
+++ b/arch/arm/include/asm/arch-mx5/sys_proto.h
@@ -24,13 +24,6 @@
 #ifndef _SYS_PROTO_H_
 #define _SYS_PROTO_H_
 
-#define MXC_CPU_MX51		0x51
-#define MXC_CPU_MX53		0x53
-#define MXC_CPU_MX6SL		0x60
-#define MXC_CPU_MX6DL		0x61
-#define MXC_CPU_MX6SOLO		0x62
-#define MXC_CPU_MX6Q		0x63
-
 #define is_soc_rev(rev)	((get_cpu_rev() & 0xFF) - rev)
 u32 get_cpu_rev(void);
 unsigned imx_ddr_size(void);
diff --git a/arch/arm/include/asm/arch-mx6/sys_proto.h b/arch/arm/include/asm/arch-mx6/sys_proto.h
index 3193297..0278317 100644
--- a/arch/arm/include/asm/arch-mx6/sys_proto.h
+++ b/arch/arm/include/asm/arch-mx6/sys_proto.h
@@ -24,13 +24,6 @@
 #ifndef _SYS_PROTO_H_
 #define _SYS_PROTO_H_
 
-#define MXC_CPU_MX51		0x51
-#define MXC_CPU_MX53		0x53
-#define MXC_CPU_MX6SL		0x60
-#define MXC_CPU_MX6DL		0x61
-#define MXC_CPU_MX6SOLO		0x62
-#define MXC_CPU_MX6Q		0x63
-
 #define is_soc_rev(rev)	((get_cpu_rev() & 0xFF) - rev)
 u32 get_cpu_rev(void);
 const char *get_imx_type(u32 imxtype);
-- 
1.7.9.5

[U-Boot] [PATCH] mx6: Fix the reading of CPU revision

[Eric Nelson]

Hi Fabio,

On 03/26/2013 05:54 AM, Fabio Estevam wrote:
> Currently when booting a mx6 solo processor get_cpu_rev() returns 0x62xxx, which
> is an invalid mx6 CPU revision. This causes run-time problems when trying to use
> VPU library in the kernel, as this library loads the VPU firmware according
> to the CPU type.
>
> Fix get_cpu_rev() so that it correctly returns 0x61xxx for a mx6 solo.
>
> While at it, also remove the duplicate definitions for MXC_CPU_ types.
>
> Tested on a Wandboard solo and on a mx6qsabresd.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>   arch/arm/cpu/armv7/mx6/soc.c              |   28 ++++++++++++----------------
>   arch/arm/imx-common/cpu.c                 |   16 ++++++++++------
>   arch/arm/include/asm/arch-mx5/sys_proto.h |    7 -------
>   arch/arm/include/asm/arch-mx6/sys_proto.h |    7 -------
>   4 files changed, 22 insertions(+), 36 deletions(-)
>
> diff --git a/arch/arm/cpu/armv7/mx6/soc.c b/arch/arm/cpu/armv7/mx6/soc.c
> index 193ba12..87725eb 100644
> --- a/arch/arm/cpu/armv7/mx6/soc.c
> +++ b/arch/arm/cpu/armv7/mx6/soc.c
> @@ -43,22 +43,18 @@ struct scu_regs {
>   u32 get_cpu_rev(void)
>   {
>   	struct anatop_regs *anatop = (struct anatop_regs *)ANATOP_BASE_ADDR;
> -	u32 reg = readl(&anatop->digprog_sololite);
> -	u32 type = ((reg >> 16) & 0xff);
> -
> -	if (type != MXC_CPU_MX6SL) {
> -		reg = readl(&anatop->digprog);
> -		type = ((reg >> 16) & 0xff);
> -		if (type == MXC_CPU_MX6DL) {
> -			struct scu_regs *scu = (struct scu_regs *)SCU_BASE_ADDR;
> -			u32 cfg = readl(&scu->config) & 3;
> -
> -			if (!cfg)
> -				type = MXC_CPU_MX6SOLO;
> -		}
> -	}
> -	reg &= 0xff;		/* mx6 silicon revision */
> -	return (type << 12) | (reg + 0x10);
> +	u32 fsl_system_rev;
> +	u32 cpu_rev = readl(&anatop->digprog);
> +
> +	/* Chip Silicon ID */
> +	fsl_system_rev = ((cpu_rev >> 16) & 0xFF) << 12;
> +	/* Chip silicon major revision */
> +	fsl_system_rev |= ((cpu_rev >> 8) & 0xFF) << 4;
> +	fsl_system_rev += 0x10;
> +	/* Chip silicon minor revision */
> +	fsl_system_rev |= cpu_rev & 0xFF;

Nitpick: 0x0F to avoid trashing major revision?

Tested on Quad (TO 1.0) and Solo (TO 1.1)

Tested-By: Eric Nelson <eric.nelson@boundarydevices.com>

[U-Boot] [PATCH] mx6: Fix the reading of CPU revision

[Dirk Behme]

Hi Fabio,

On 26.03.2013 13:54, Fabio Estevam wrote:
> Currently when booting a mx6 solo processor get_cpu_rev() returns 0x62xxx, which
> is an invalid mx6 CPU revision.

Do you have somewhere a list of valid CPU revisions? From two points of 
view:

a) the i.MX6 hardware spec

b) the VPU library

> This causes run-time problems when trying to use
> VPU library in the kernel, as this library loads the VPU firmware according
> to the CPU type.
> 
> Fix get_cpu_rev() so that it correctly returns 0x61xxx for a mx6 solo.
> 
> While at it, also remove the duplicate definitions for MXC_CPU_ types.
> 
> Tested on a Wandboard solo and on a mx6qsabresd.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/cpu/armv7/mx6/soc.c              |   28 ++++++++++++----------------
>  arch/arm/imx-common/cpu.c                 |   16 ++++++++++------
>  arch/arm/include/asm/arch-mx5/sys_proto.h |    7 -------
>  arch/arm/include/asm/arch-mx6/sys_proto.h |    7 -------
>  4 files changed, 22 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/mx6/soc.c b/arch/arm/cpu/armv7/mx6/soc.c
> index 193ba12..87725eb 100644
> --- a/arch/arm/cpu/armv7/mx6/soc.c
> +++ b/arch/arm/cpu/armv7/mx6/soc.c
> @@ -43,22 +43,18 @@ struct scu_regs {
>  u32 get_cpu_rev(void)
>  {
>  	struct anatop_regs *anatop = (struct anatop_regs *)ANATOP_BASE_ADDR;
> -	u32 reg = readl(&anatop->digprog_sololite);
> -	u32 type = ((reg >> 16) & 0xff);
> -
> -	if (type != MXC_CPU_MX6SL) {
> -		reg = readl(&anatop->digprog);
> -		type = ((reg >> 16) & 0xff);
> -		if (type == MXC_CPU_MX6DL) {
> -			struct scu_regs *scu = (struct scu_regs *)SCU_BASE_ADDR;
> -			u32 cfg = readl(&scu->config) & 3;
> -
> -			if (!cfg)
> -				type = MXC_CPU_MX6SOLO;
> -		}
> -	}
> -	reg &= 0xff;		/* mx6 silicon revision */
> -	return (type << 12) | (reg + 0x10);
> +	u32 fsl_system_rev;
> +	u32 cpu_rev = readl(&anatop->digprog);
> +
> +	/* Chip Silicon ID */
> +	fsl_system_rev = ((cpu_rev >> 16) & 0xFF) << 12;
> +	/* Chip silicon major revision */
> +	fsl_system_rev |= ((cpu_rev >> 8) & 0xFF) << 4;
> +	fsl_system_rev += 0x10;
> +	/* Chip silicon minor revision */
> +	fsl_system_rev |= cpu_rev & 0xFF;

You remove Troy's code here introduced with

http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=20332a066aff98f39419495821e14edd10b2a3f8

Troy's detection you remove here intentionally distinguishes between 
DualLite and Solo. You now re-introduce a common DL_S, again.

Additionally, you completely seem to drop checking for scu->config. I've 
already seen some (broken?) i.MX6Solo where this check was essential.

I can't talk about the "problems when trying to use VPU library in the 
kernel" (btw, which problems?) and the invalid 0x62xxx, but we used 
Troy's version of the detection successfully.

Best regards

Dirk

>  void init_aips(void)
> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
> index a9b86c1..2f518c5 100644
> --- a/arch/arm/imx-common/cpu.c
> +++ b/arch/arm/imx-common/cpu.c
> @@ -117,15 +117,19 @@ unsigned imx_ddr_size(void)
>  
>  #if defined(CONFIG_DISPLAY_CPUINFO)
>  
> +#define MXC_CPU_MX51		0x51
> +#define MXC_CPU_MX53		0x53
> +#define MXC_CPU_MX6SL		0x60
> +#define MXC_CPU_MX6DL_S		0x61
> +#define MXC_CPU_MX6Q_D		0x63
> +
>  const char *get_imx_type(u32 imxtype)
>  {
>  	switch (imxtype) {
> -	case MXC_CPU_MX6Q:
> -		return "6Q";	/* Quad-core version of the mx6 */
> -	case MXC_CPU_MX6DL:
> -		return "6DL";	/* Dual Lite version of the mx6 */
> -	case MXC_CPU_MX6SOLO:
> -		return "6SOLO";	/* Solo version of the mx6 */
> +	case MXC_CPU_MX6Q_D:
> +		return "6Q/D";	/* Quad/Dual version of the mx6 */
> +	case MXC_CPU_MX6DL_S:
> +		return "6DL/S";	/* Dual-Lite/Solo version of the mx6 */
>  	case MXC_CPU_MX6SL:
>  		return "6SL";	/* Solo-Lite version of the mx6 */
>  	case MXC_CPU_MX51:
> diff --git a/arch/arm/include/asm/arch-mx5/sys_proto.h b/arch/arm/include/asm/arch-mx5/sys_proto.h
> index 93ad1c6..a2e88bb 100644
> --- a/arch/arm/include/asm/arch-mx5/sys_proto.h
> +++ b/arch/arm/include/asm/arch-mx5/sys_proto.h
> @@ -24,13 +24,6 @@
>  #ifndef _SYS_PROTO_H_
>  #define _SYS_PROTO_H_
>  
> -#define MXC_CPU_MX51		0x51
> -#define MXC_CPU_MX53		0x53
> -#define MXC_CPU_MX6SL		0x60
> -#define MXC_CPU_MX6DL		0x61
> -#define MXC_CPU_MX6SOLO		0x62
> -#define MXC_CPU_MX6Q		0x63
> -
>  #define is_soc_rev(rev)	((get_cpu_rev() & 0xFF) - rev)
>  u32 get_cpu_rev(void);
>  unsigned imx_ddr_size(void);
> diff --git a/arch/arm/include/asm/arch-mx6/sys_proto.h b/arch/arm/include/asm/arch-mx6/sys_proto.h
> index 3193297..0278317 100644
> --- a/arch/arm/include/asm/arch-mx6/sys_proto.h
> +++ b/arch/arm/include/asm/arch-mx6/sys_proto.h
> @@ -24,13 +24,6 @@
>  #ifndef _SYS_PROTO_H_
>  #define _SYS_PROTO_H_
>  
> -#define MXC_CPU_MX51		0x51
> -#define MXC_CPU_MX53		0x53
> -#define MXC_CPU_MX6SL		0x60
> -#define MXC_CPU_MX6DL		0x61
> -#define MXC_CPU_MX6SOLO		0x62
> -#define MXC_CPU_MX6Q		0x63
> -
>  #define is_soc_rev(rev)	((get_cpu_rev() & 0xFF) - rev)
>  u32 get_cpu_rev(void);
>  const char *get_imx_type(u32 imxtype);


-- 
======================================================================
Dirk Behme                      Robert Bosch Car Multimedia GmbH
                                 CM-AI/PJ-CF32
Phone: +49 5121 49-3274         Dirk Behme
Fax:   +49 711 811 5053274      PO Box 77 77 77
mailto:dirk.behme at de.bosch.com  D-31132 Hildesheim - Germany

Bosch Group, Car Multimedia (CM)
              Automotive Navigation and Infotainment Systems (AI)
              ProJect - CoreFunctions (PJ-CF)

Robert Bosch Car Multimedia GmbH - Ein Unternehmen der Bosch Gruppe
Sitz: Hildesheim
Registergericht: Amtsgericht Hildesheim HRB 201334
Aufsichtsratsvorsitzender: Volkmar Denner
Gesch?ftsf?hrung: Uwe Thomas, Michael Bolle, Robby Drave, Egbert Hellwig
======================================================================

[U-Boot] [PATCH] mx6: Fix the reading of CPU revision

[Fabio Estevam]

Hi Dirk,

On Tue, Mar 26, 2013 at 12:43 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> Hi Fabio,
>
>
> On 26.03.2013 13:54, Fabio Estevam wrote:
>>
>> Currently when booting a mx6 solo processor get_cpu_rev() returns 0x62xxx,
>> which
>> is an invalid mx6 CPU revision.
>
>
> Do you have somewhere a list of valid CPU revisions? From two points of
> view:
>
> a) the i.MX6 hardware spec
>
> b) the VPU library

Sorry, I don't. I am basing the CPU revision numbers from FSL U-boot:
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/tree/board/freescale/common/fsl_sys_rev.c?h=imx_v2009.08_3.0.0

Adding Jason, in case he could clarify it.

> You remove Troy's code here introduced with
>
> http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=20332a066aff98f39419495821e14edd10b2a3f8
>
> Troy's detection you remove here intentionally distinguishes between
> DualLite and Solo. You now re-introduce a common DL_S, again.
>
> Additionally, you completely seem to drop checking for scu->config. I've
> already seen some (broken?) i.MX6Solo where this check was essential.
>
> I can't talk about the "problems when trying to use VPU library in the
> kernel" (btw, which problems?) and the invalid 0x62xxx, but we used Troy's
> version of the detection successfully.

Passing 0x62xxx as cpu_rev on a mx6solo caused the VPU issues described here:
https://community.freescale.com/thread/305396

Which cpu_rev value is returned with your mx6solo? Are you able to use VPU lib?

I will test solo lite when I have chance to add support for it.

Regards,

Fabio Estevam

[U-Boot] [PATCH] mx6: Fix the reading of CPU revision

[Dirk Behme]

On 26.03.2013 18:04, Fabio Estevam wrote:
> Hi Dirk,
> 
> On Tue, Mar 26, 2013 at 12:43 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> Hi Fabio,
>>
>>
>> On 26.03.2013 13:54, Fabio Estevam wrote:
>>> Currently when booting a mx6 solo processor get_cpu_rev() returns 0x62xxx,
>>> which
>>> is an invalid mx6 CPU revision.
>>
>> Do you have somewhere a list of valid CPU revisions? From two points of
>> view:
>>
>> a) the i.MX6 hardware spec
>>
>> b) the VPU library
> 
> Sorry, I don't. I am basing the CPU revision numbers from FSL U-boot:
> http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/tree/board/freescale/common/fsl_sys_rev.c?h=imx_v2009.08_3.0.0
> 
> Adding Jason, in case he could clarify it.
> 
>> You remove Troy's code here introduced with
>>
>> http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=20332a066aff98f39419495821e14edd10b2a3f8
>>
>> Troy's detection you remove here intentionally distinguishes between
>> DualLite and Solo. You now re-introduce a common DL_S, again.
>>
>> Additionally, you completely seem to drop checking for scu->config. I've
>> already seen some (broken?) i.MX6Solo where this check was essential.
>>
>> I can't talk about the "problems when trying to use VPU library in the
>> kernel" (btw, which problems?) and the invalid 0x62xxx, but we used Troy's
>> version of the detection successfully.
> 
> Passing 0x62xxx as cpu_rev on a mx6solo caused the VPU issues described here:
> https://community.freescale.com/thread/305396
> 
> Which cpu_rev value is returned with your mx6solo? Are you able to use VPU lib?

I'll check this.

Rethinking about the issue here, my recent understanding is:

a) We have a VPU library which only understands 0x63 (Quad) and 0x61 
(DualLite/Solo)

b) We have Troy's existing get_cpu_rev() [1] which seems to correctly 
decode the CPU revision (at least this is my impression from testing ;) 
). But reports 0x62 for the Solo which then isn't understood by the VPU 
library (to be checked).

I wonder if we could find a way to combine both parts without breaking 
the other? I.e. using Troy's get_cpu_rev() to correctly report the CPU 
revision (in U-Boot), but let the VPU library get the revision it 
understands?

Best regards

Dirk

[1] 
http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=20332a066aff98f39419495821e14edd10b2a3f8

[U-Boot] [PATCH] mx6: Fix the reading of CPU revision

[Dirk Behme]

On 27.03.2013 09:02, Dirk Behme wrote:
> On 26.03.2013 18:04, Fabio Estevam wrote:
>> Hi Dirk,
>>
>> On Tue, Mar 26, 2013 at 12:43 PM, Dirk Behme <dirk.behme@de.bosch.com> 
>> wrote:
>>> Hi Fabio,
>>>
>>>
>>> On 26.03.2013 13:54, Fabio Estevam wrote:
>>>> Currently when booting a mx6 solo processor get_cpu_rev() returns 
>>>> 0x62xxx,
>>>> which
>>>> is an invalid mx6 CPU revision.
>>>
>>> Do you have somewhere a list of valid CPU revisions? From two points of
>>> view:
>>>
>>> a) the i.MX6 hardware spec
>>>
>>> b) the VPU library
>>
>> Sorry, I don't. I am basing the CPU revision numbers from FSL U-boot:
>> http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/tree/board/freescale/common/fsl_sys_rev.c?h=imx_v2009.08_3.0.0 
>>
>>
>> Adding Jason, in case he could clarify it.
>>
>>> You remove Troy's code here introduced with
>>>
>>> http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=20332a066aff98f39419495821e14edd10b2a3f8 
>>>
>>>
>>> Troy's detection you remove here intentionally distinguishes between
>>> DualLite and Solo. You now re-introduce a common DL_S, again.
>>>
>>> Additionally, you completely seem to drop checking for scu->config. I've
>>> already seen some (broken?) i.MX6Solo where this check was essential.
>>>
>>> I can't talk about the "problems when trying to use VPU library in the
>>> kernel" (btw, which problems?) and the invalid 0x62xxx, but we used 
>>> Troy's
>>> version of the detection successfully.
>>
>> Passing 0x62xxx as cpu_rev on a mx6solo caused the VPU issues 
>> described here:
>> https://community.freescale.com/thread/305396
>>
>> Which cpu_rev value is returned with your mx6solo? Are you able to use 
>> VPU lib?
> 
> I'll check this.
> 
> Rethinking about the issue here, my recent understanding is:
> 
> a) We have a VPU library which only understands 0x63 (Quad) and 0x61 
> (DualLite/Solo)
> 
> b) We have Troy's existing get_cpu_rev() [1] which seems to correctly 
> decode the CPU revision (at least this is my impression from testing ;) 
> ). But reports 0x62 for the Solo which then isn't understood by the VPU 
> library (to be checked).

Some additional rethinking: I missed that we have a Linux kernel, too ;)

c) It's the job of the Linux kernel to export the CPU revision to the 
VPU library. In case the Linux kernel completely ignores what we are 
doing in U-Boot and calculates the CPU revision itself (*), e.g. by 
something like

http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/mach-mx6/mm.c?h=imx_3.0.35_1.1.0&id=207f80453c77bc32e04b5fef863f6fe50a7fd1a8#n60

we can do anything in U-Boot. Independent of the VPU library.

In this case I'd propose to just keep Troy's version of get_cpu_rev() as 
it is [1].

Sorry for the confusion, hopefully this is correct now ;)

Best regards

Dirk

(*) There might be U-Boot/Kernel combinations out there, where U-Boot 
exports the CPU revision via ATAGs to the kernel. But hopefully this 
doesn't affect us here (?)

> I wonder if we could find a way to combine both parts without breaking 
> the other? I.e. using Troy's get_cpu_rev() to correctly report the CPU 
> revision (in U-Boot), but let the VPU library get the revision it 
> understands?
> 
> Best regards
> 
> Dirk
> 
> [1] 
> http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=20332a066aff98f39419495821e14edd10b2a3f8 

[U-Boot] [PATCH] mx6: Fix the reading of CPU revision

[Fabio Estevam]

On Wed, Mar 27, 2013 at 5:57 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:

> Some additional rethinking: I missed that we have a Linux kernel, too ;)
>
> c) It's the job of the Linux kernel to export the CPU revision to the VPU
> library. In case the Linux kernel completely ignores what we are doing in
> U-Boot and calculates the CPU revision itself (*), e.g. by something like
>
> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/mach-mx6/mm.c?h=imx_3.0.35_1.1.0&id=207f80453c77bc32e04b5fef863f6fe50a7fd1a8#n60
>
> we can do anything in U-Boot. Independent of the VPU library.

Unfortunately VPU library relies on the bootloader to pass the correct
silicon revision.

Eric's tested passing 0 as get_cpu_rev and showed that VPU simply
cannot work on this case.

> In this case I'd propose to just keep Troy's version of get_cpu_rev() as it
> is [1].

This is proven to not to work with mx6solo and VPU, so we need the fix
I proposed.

Here is what I am planning to do:

1. Send a v2 of this patch with the small correction pointed out by Eric
2. Include a weak function to pass get_cpu_rev in common mx6 code

Then on top of that, one can send a patch that prints the mx6 silicon
strings by differentiating between a mx6dual-lite and mx6solo, if it
is worth.

Regards,

Fabio Estevam

[U-Boot] [PATCH] mx6: Fix the reading of CPU revision

[Dirk Behme]

On 27.03.2013 14:37, Fabio Estevam wrote:
> On Wed, Mar 27, 2013 at 5:57 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> 
>> Some additional rethinking: I missed that we have a Linux kernel, too ;)
>>
>> c) It's the job of the Linux kernel to export the CPU revision to the VPU
>> library. In case the Linux kernel completely ignores what we are doing in
>> U-Boot and calculates the CPU revision itself (*), e.g. by something like
>>
>> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/mach-mx6/mm.c?h=imx_3.0.35_1.1.0&id=207f80453c77bc32e04b5fef863f6fe50a7fd1a8#n60
>>
>> we can do anything in U-Boot. Independent of the VPU library.
> 
> Unfortunately VPU library relies on the bootloader to pass the correct
> silicon revision.

I don't think so, at least this depends on the kernel. To my 
understanding the VPU library relies on the kernel to pass the correct 
silicon revision. And where the kernel gets this information from seems 
to depend on the kernel version:

a) get it from U-Boot via ATAGs

b) calculate the value in the kernel independent of U-Boot

About which kernel are you talking? To my understanding, using a recent 
kernel with device tree there is no revision information passed from the 
boot loader to the kernel?

Instead of hacking U-Boot for these (old) kernels, I'd propose to fix 
the kernel to pass the 0x61/0x63 correctly to the VPU library. Or as 
Wolfgang mentioned, even better, fix the VPU library.

I really like Troys existing code and see no reason to change it just 
due to the VPU library. Do the change in the kernel or the VPU library.

Best regards

Dirk

> Eric's tested passing 0 as get_cpu_rev and showed that VPU simply
> cannot work on this case.
> 
>> In this case I'd propose to just keep Troy's version of get_cpu_rev() as it
>> is [1].
> 
> This is proven to not to work with mx6solo and VPU, so we need the fix
> I proposed.
> 
> Here is what I am planning to do:
> 
> 1. Send a v2 of this patch with the small correction pointed out by Eric
> 2. Include a weak function to pass get_cpu_rev in common mx6 code
> 
> Then on top of that, one can send a patch that prints the mx6 silicon
> strings by differentiating between a mx6dual-lite and mx6solo, if it
> is worth.
> 
> Regards,
> 
> Fabio Estevam


-- 
======================================================================
Dirk Behme                      Robert Bosch Car Multimedia GmbH
                                 CM-AI/PJ-CF32
Phone: +49 5121 49-3274         Dirk Behme
Fax:   +49 711 811 5053274      PO Box 77 77 77
mailto:dirk.behme at de.bosch.com  D-31132 Hildesheim - Germany

Bosch Group, Car Multimedia (CM)
              Automotive Navigation and Infotainment Systems (AI)
              ProJect - CoreFunctions (PJ-CF)

Robert Bosch Car Multimedia GmbH - Ein Unternehmen der Bosch Gruppe
Sitz: Hildesheim
Registergericht: Amtsgericht Hildesheim HRB 201334
Aufsichtsratsvorsitzender: Volkmar Denner
Gesch?ftsf?hrung: Uwe Thomas, Michael Bolle, Robby Drave, Egbert Hellwig
======================================================================

[U-Boot] [PATCH] mx6: Fix the reading of CPU revision

[Eric Nelson]

Hi Fabio,

On 03/27/2013 06:37 AM, Fabio Estevam wrote:
> On Wed, Mar 27, 2013 at 5:57 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>
>> Some additional rethinking: I missed that we have a Linux kernel, too ;)
>>
>> c) It's the job of the Linux kernel to export the CPU revision to the VPU
>> library. In case the Linux kernel completely ignores what we are doing in
>> U-Boot and calculates the CPU revision itself (*), e.g. by something like
>>
>> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/mach-mx6/mm.c?h=imx_3.0.35_1.1.0&id=207f80453c77bc32e04b5fef863f6fe50a7fd1a8#n60
>>
>> we can do anything in U-Boot. Independent of the VPU library.
>
> Unfortunately VPU library relies on the bootloader to pass the correct
> silicon revision.
>

The VPU library relies on the output of /proc/cpuinfo (specifically
the line beginning with "Revision".

The snippet (from vpu_io.h) is:

	tmp = strstr(buf, "Revision");
	if (tmp != NULL) {
		rev = index(tmp, ':');
		if (rev != NULL) {
			rev++;
			system_rev = strtoul(rev, NULL, 16);
			ret = 0;
		}
	}

This code should really be changed, so we don't have to carry this
data all the way from boot loader to /proc/cpuinfo.

Similar (but different) code is present in mxc_ipu_hl_lib.c
for the IPU.

In the case of the VPU library, it seems more sane to have the
VPU driver expose the particular IP revision present on the
system.

> Eric's tested passing 0 as get_cpu_rev and showed that VPU simply
> cannot work on this case.
>
>> In this case I'd propose to just keep Troy's version of get_cpu_rev() as it
>> is [1].
>
> This is proven to not to work with mx6solo and VPU, so we need the fix
> I proposed.
>
> Here is what I am planning to do:
>
> 1. Send a v2 of this patch with the small correction pointed out by Eric
> 2. Include a weak function to pass get_cpu_rev in common mx6 code
>
> Then on top of that, one can send a patch that prints the mx6 silicon
> strings by differentiating between a mx6dual-lite and mx6solo, if it
> is worth.
>

It seems a reasonable interim solution to provide backward
compatibility until the kernel driver(s) and userspace can be
fixed.

Another way of doing this that prevents get_cpu_rev() from
hiding the precise CPU is to do this in the "weak" version
of get_board_rev().

Regards,


Eric

[U-Boot] [PATCH] mx6: Fix the reading of CPU revision

[Dirk Behme]

Hi Eric,

On 27.03.2013 15:00, Eric Nelson wrote:
> Hi Fabio,
> 
> On 03/27/2013 06:37 AM, Fabio Estevam wrote:
>> On Wed, Mar 27, 2013 at 5:57 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>>
>>> Some additional rethinking: I missed that we have a Linux kernel, too ;)
>>>
>>> c) It's the job of the Linux kernel to export the CPU revision to the VPU
>>> library. In case the Linux kernel completely ignores what we are doing in
>>> U-Boot and calculates the CPU revision itself (*), e.g. by something like
>>>
>>> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/mach-mx6/mm.c?h=imx_3.0.35_1.1.0&id=207f80453c77bc32e04b5fef863f6fe50a7fd1a8#n60
>>>
>>> we can do anything in U-Boot. Independent of the VPU library.
>> Unfortunately VPU library relies on the bootloader to pass the correct
>> silicon revision.
>>
> 
> The VPU library relies on the output of /proc/cpuinfo (specifically
> the line beginning with "Revision".
> 
> The snippet (from vpu_io.h) is:
> 
> 	tmp = strstr(buf, "Revision");
> 	if (tmp != NULL) {
> 		rev = index(tmp, ':');
> 		if (rev != NULL) {
> 			rev++;
> 			system_rev = strtoul(rev, NULL, 16);
> 			ret = 0;
> 		}
> 	}
> 
> This code should really be changed, 

Yes :)

> so we don't have to carry this
> data all the way from boot loader to /proc/cpuinfo.

As mentioned in my previous mail, I have some doubts that *all* kernels 
pick the version from the boot loader. It's my understanding that this 
strongly depends on the kernel? I.e. there are kernels which get the 
version from the boot loader, e.g. via ATAGs. But there are kernels 
which are independent from the boot loader and calculate it on their 
own? E.g.

http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/mach-mx6/mm.c?h=imx_3.0.35_1.1.0&id=207f80453c77bc32e04b5fef863f6fe50a7fd1a8#n60

?

Best regards

Dirk

> Similar (but different) code is present in mxc_ipu_hl_lib.c
> for the IPU.
> 
> In the case of the VPU library, it seems more sane to have the
> VPU driver expose the particular IP revision present on the
> system.
> 
>> Eric's tested passing 0 as get_cpu_rev and showed that VPU simply
>> cannot work on this case.
>>
>>> In this case I'd propose to just keep Troy's version of get_cpu_rev() as it
>>> is [1].
>> This is proven to not to work with mx6solo and VPU, so we need the fix
>> I proposed.
>>
>> Here is what I am planning to do:
>>
>> 1. Send a v2 of this patch with the small correction pointed out by Eric
>> 2. Include a weak function to pass get_cpu_rev in common mx6 code
>>
>> Then on top of that, one can send a patch that prints the mx6 silicon
>> strings by differentiating between a mx6dual-lite and mx6solo, if it
>> is worth.
>>
> 
> It seems a reasonable interim solution to provide backward
> compatibility until the kernel driver(s) and userspace can be
> fixed.
> 
> Another way of doing this that prevents get_cpu_rev() from
> hiding the precise CPU is to do this in the "weak" version
> of get_board_rev().
> 
> Regards,

[U-Boot] [PATCH] mx6: Fix the reading of CPU revision

[Eric Nelson]

Hi Dirk,

On 03/27/2013 08:06 AM, Dirk Behme wrote:
> Hi Eric,
>
> On 27.03.2013 15:00, Eric Nelson wrote:
>> Hi Fabio,
>>
>> On 03/27/2013 06:37 AM, Fabio Estevam wrote:
>>> On Wed, Mar 27, 2013 at 5:57 AM, Dirk Behme <dirk.behme@de.bosch.com>
>>> wrote:
>>>
 >> <snip>
>>
>> The VPU library relies on the output of /proc/cpuinfo (specifically
>> the line beginning with "Revision".
>>
>> The snippet (from vpu_io.h) is:
>>
>>     tmp = strstr(buf, "Revision");
>>     if (tmp != NULL) {
>>         rev = index(tmp, ':');
>>
 >> <snip>
 >>
>> This code should really be changed,
>
> Yes :)
>

And this whole thing is really off-topic on the U-Boot list
except as a weak requirement on U-Boot.

>> so we don't have to carry this
>> data all the way from boot loader to /proc/cpuinfo.
>
> As mentioned in my previous mail, I have some doubts that *all* kernels
> pick the version from the boot loader. It's my understanding that this
> strongly depends on the kernel? I.e. there are kernels which get the
> version from the boot loader, e.g. via ATAGs. But there are kernels
> which are independent from the boot loader and calculate it on their
> own? E.g.
>
> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/mach-mx6/mm.c?h=imx_3.0.35_1.1.0&id=207f80453c77bc32e04b5fef863f6fe50a7fd1a8#n60
>

You're right of course. I believe only non-DT kernels are at issue here,
and it's just the ATAG callback get_board_rev() is at issue, not
get_cpu_rev().

Note that this is supposed to carry **board** revision, which lends
more weight to fixing it.

The kernel knows the proper CPU revision in all cases but publishes
the content of ATAG_REVISION in /proc/cpuinfo
	http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/kernel/setup.c?h=imx_3.0.35_1.1.0#n655
	http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/kernel/setup.c?h=imx_3.0.35_1.1.0#n1041

It seems straightforward to simply over-write 'system_rev' on those
kernels which need the hack.

Regards,


Eric

[U-Boot] [PATCH] mx6: Fix the reading of CPU revision

[Fabio Estevam]

On Wed, Mar 27, 2013 at 5:02 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:

> I'll check this.
>
> Rethinking about the issue here, my recent understanding is:
>
> a) We have a VPU library which only understands 0x63 (Quad) and 0x61
> (DualLite/Solo)

Correct.

> b) We have Troy's existing get_cpu_rev() [1] which seems to correctly decode
> the CPU revision (at least this is my impression from testing ;) ). But
> reports 0x62 for the Solo which then isn't understood by the VPU library (to
> be checked).

Correct.

> I wonder if we could find a way to combine both parts without breaking the
> other? I.e. using Troy's get_cpu_rev() to correctly report the CPU revision
> (in U-Boot), but let the VPU library get the revision it understands?

Yes, this could be possible. The original patch in this thread fixes
the returned value by get_cpu_rev(), so that we can have mainline
U-boot to boot a system that can use VPU on a mx6solo.

It is still possible to report the CPU revisions
(Quad/Dual-lite/Solo/Solo-lite) strings in boot time. We would need to
add a custom mx6 code for printing the strings.

Regards,

Fabio Estevam