| Message ID | 1364302440-18457-1-git-send-email-fabio.estevam@freescale.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
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>
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);
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
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
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
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
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
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
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
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,
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
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);
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(-)