| Message ID | 4F52015A.2080003@googlemail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Dirk Behme, In message <4F52015A.2080003@googlemail.com> you wrote: > > > Agreed. If these patches were only for backward compatibility I would > > not complain much. But they are known to introduce forward incompati- > > bilities with all this MACH_ID stuff, and this is what I would like to > > avoid. > > Now I'm just trying to learn something regarding [1]: > > Which changes would you accept in the category 'backward compatibility'? There are 3 commits in this series: [PATCH 1/3] i.MX6: mx6qsabrelite: add CONFIG_REVISION_TAG [PATCH 2/3] i.MX6: mx6qsabrelite: add MACH_TYPE_MX6Q_SABRELITE [PATCH 3/3] i.MX6: mx6qsabrelite: add ext2 support I dislike #1 because it uses the completely undocumented CONFIG_REVISION_TAG, and I agree with Marek's and Stefano's comments. The problems I mentioned are with # 2, which now would depend on MACH_TYPE_MX6Q_SABRELITE, which may or may not exist. Also, I think we should not need this any more at all, as we now have DT support in Linux on ARM, too. I see no issues with # 3. > And which changes 'introduce forward incompatibilities', and what are > these incompatibilities? See the recent problems that occurred when RMK decided to "clean up" the machids file. > (assuming this will be changed to > > --- a/include/configs/mx6qsabrelite.h > +++ b/include/configs/mx6qsabrelite.h > @@ -27,6 +27,7 @@ > #define CONFIG_SYS_MX6_CLK32 32768 > #define CONFIG_DISPLAY_CPUINFO > #define CONFIG_DISPLAY_BOARDINFO > +#define CONFIG_MACH_TYPE 3769 Such a change would avoid breakages as the ones mentioned above, but is this nice? Either we have a mach-types.h that can be used, or we don't, in which case we should stop using any definitions from it, especially for new boards where it's not needed due to DT support in the kernel. Best regards, Wolfgang Denk
On 3/3/2012 6:30 AM, Wolfgang Denk wrote: > Dear Dirk Behme, > > In message<4F52015A.2080003@googlemail.com> you wrote: >>> Agreed. If these patches were only for backward compatibility I would >>> not complain much. But they are known to introduce forward incompati- >>> bilities with all this MACH_ID stuff, and this is what I would like to >>> avoid. >> Now I'm just trying to learn something regarding [1]: >> >> Which changes would you accept in the category 'backward compatibility'? > There are 3 commits in this series: > > [PATCH 1/3] i.MX6: mx6qsabrelite: add CONFIG_REVISION_TAG > [PATCH 2/3] i.MX6: mx6qsabrelite: add MACH_TYPE_MX6Q_SABRELITE > [PATCH 3/3] i.MX6: mx6qsabrelite: add ext2 support > > I dislike #1 because it uses the completely undocumented > CONFIG_REVISION_TAG, and I agree with Marek's and Stefano's comments. > > The problems I mentioned are with # 2, which now would depend on > MACH_TYPE_MX6Q_SABRELITE, which may or may not exist. > > Also, I think we should not need this any more at all, as we now have > DT support in Linux on ARM, too. > > I see no issues with # 3. > >> And which changes 'introduce forward incompatibilities', and what are >> these incompatibilities? > See the recent problems that occurred when RMK decided to "clean up" > the machids file. > > Would you rather that I take RMK's cleaned up file, and undelete the machines that u-boot uses? That would be more simple than adding to the board's config file. I can delete all of the mach_is_xxx macros in mach-types while I'm at it. Thanks Troy
Dear Troy Kisky, In message <4F52C327.3080309@boundarydevices.com> you wrote: > > >> And which changes 'introduce forward incompatibilities', and what are > >> these incompatibilities? > > See the recent problems that occurred when RMK decided to "clean up" > > the machids file. > > > Would you rather that I take RMK's cleaned up file, and undelete the > machines that u-boot > uses? That would be more simple than adding to the board's config file. > I can delete all of the mach_is_xxx macros in mach-types while I'm at it. I think we had this discussion before (when RMK's changes hit us), and it was decided not to do this. IIRC we decided not to do this. I have never understood why this mach_types thingy was needed (other rchitectures worked fine without it, or better), and now we are on the edge of obsoleting it. So all efforts trying to maintain this file are futile, and we would have to redo these for any updates of the file. I feel this is not a good investment of our time. Best regards, Wolfgang Denk
On 03/03/2012 14:30, Wolfgang Denk wrote: > Dear Dirk Behme, > Hi, > In message <4F52015A.2080003@googlemail.com> you wrote: >> >>> Agreed. If these patches were only for backward compatibility I would >>> not complain much. But they are known to introduce forward incompati- >>> bilities with all this MACH_ID stuff, and this is what I would like to >>> avoid. >> >> Now I'm just trying to learn something regarding [1]: >> >> Which changes would you accept in the category 'backward compatibility'? > > There are 3 commits in this series: > > [PATCH 1/3] i.MX6: mx6qsabrelite: add CONFIG_REVISION_TAG > [PATCH 2/3] i.MX6: mx6qsabrelite: add MACH_TYPE_MX6Q_SABRELITE > [PATCH 3/3] i.MX6: mx6qsabrelite: add ext2 support > > I dislike #1 because it uses the completely undocumented > CONFIG_REVISION_TAG, and I agree with Marek's and Stefano's comments. A lot of boards are currently set CONFIG_REVISION_TAG. Sure, it would be nice to document it. To be consistent we should drop this CONFIG_ from all boards, or add documentation for it. However, I am asking if this TAG is really needed. I have searched in 2.6.38 Kernel provided by Freescale if the TAG is really evaluated to set different revisions of the boards, but I have not found anything. Is it really needed ? If not, we should drop it. > > The problems I mentioned are with # 2, which now would depend on > MACH_TYPE_MX6Q_SABRELITE, which may or may not exist. Right, I agree. And we do not know (maybe it is not this case) if MACH_TYPE_MX6Q_SABRELITE will be dropped in the future. IMHO we should not use mach-types at all.. > > Also, I think we should not need this any more at all, as we now have > DT support in Linux on ARM, too. > > I see no issues with # 3. I will merge #3 into u-boot-imx > >> And which changes 'introduce forward incompatibilities', and what are >> these incompatibilities? > > See the recent problems that occurred when RMK decided to "clean up" > the machids file. > >> (assuming this will be changed to >> >> --- a/include/configs/mx6qsabrelite.h >> +++ b/include/configs/mx6qsabrelite.h >> @@ -27,6 +27,7 @@ >> #define CONFIG_SYS_MX6_CLK32 32768 >> #define CONFIG_DISPLAY_CPUINFO >> #define CONFIG_DISPLAY_BOARDINFO >> +#define CONFIG_MACH_TYPE 3769 > > Such a change would avoid breakages as the ones mentioned above, but > is this nice? Either we have a mach-types.h that can be used, or we > don't, Personally I vote for the second choice. U-boot does not use mach-types, and it is at the end only a parameter for the kernel. I think the solution in the patch is better as to try to maintain the mach-types file: not very nice, but setting its value is not different as several other setups inside the configuration file that are very board specifiv. And CONFIG_MACH_TYPE is well documented. > in which case we should stop using any definitions from it, > especially for new boards where it's not needed due to DT support in > the kernel. Agree completely - mach-types introduces a strong dependency with the kernel, and we do not need it. Best regards, Stefano Babic
Hi, On 03/04/12 13:09, Stefano Babic wrote: > On 03/03/2012 14:30, Wolfgang Denk wrote: >> Dear Dirk Behme, >> > > Hi, > >> In message <4F52015A.2080003@googlemail.com> you wrote: >>> >>>> Agreed. If these patches were only for backward compatibility I would >>>> not complain much. But they are known to introduce forward incompati- >>>> bilities with all this MACH_ID stuff, and this is what I would like to >>>> avoid. >>> >>> Now I'm just trying to learn something regarding [1]: >>> >>> Which changes would you accept in the category 'backward compatibility'? >> >> There are 3 commits in this series: >> >> [PATCH 1/3] i.MX6: mx6qsabrelite: add CONFIG_REVISION_TAG >> [PATCH 2/3] i.MX6: mx6qsabrelite: add MACH_TYPE_MX6Q_SABRELITE >> [PATCH 3/3] i.MX6: mx6qsabrelite: add ext2 support >> >> I dislike #1 because it uses the completely undocumented >> CONFIG_REVISION_TAG, and I agree with Marek's and Stefano's comments. > > A lot of boards are currently set CONFIG_REVISION_TAG. Sure, it would be > nice to document it. To be consistent we should drop this CONFIG_ from > all boards, or add documentation for it. > > However, I am asking if this TAG is really needed. I have searched in > 2.6.38 Kernel provided by Freescale if the TAG is really evaluated to > set different revisions of the boards, but I have not found anything. Is > it really needed ? If not, we should drop it. Linux's system_rev global variable is set to that value. You can check for it by "cat /proc/cpuinfo | grep Revision". > >> >> The problems I mentioned are with # 2, which now would depend on >> MACH_TYPE_MX6Q_SABRELITE, which may or may not exist. > > Right, I agree. And we do not know (maybe it is not this case) if > MACH_TYPE_MX6Q_SABRELITE will be dropped in the future. IMHO we should > not use mach-types at all.. +1 making the mach-type local to a board moves the maintenance burden to a board maintainer which is good because it can be decided on per board basis. Also, we should always raise the question if it is still needed for a particular board and don't let it in, if it is not... > >> >> Also, I think we should not need this any more at all, as we now have >> DT support in Linux on ARM, too. >> >> I see no issues with # 3. > > I will merge #3 into u-boot-imx > >> >>> And which changes 'introduce forward incompatibilities', and what are >>> these incompatibilities? >> >> See the recent problems that occurred when RMK decided to "clean up" >> the machids file. >> >>> (assuming this will be changed to >>> >>> --- a/include/configs/mx6qsabrelite.h >>> +++ b/include/configs/mx6qsabrelite.h >>> @@ -27,6 +27,7 @@ >>> #define CONFIG_SYS_MX6_CLK32 32768 >>> #define CONFIG_DISPLAY_CPUINFO >>> #define CONFIG_DISPLAY_BOARDINFO >>> +#define CONFIG_MACH_TYPE 3769 >> >> Such a change would avoid breakages as the ones mentioned above, but >> is this nice? Either we have a mach-types.h that can be used, or we >> don't, > > Personally I vote for the second choice. U-boot does not use mach-types, > and it is at the end only a parameter for the kernel. +1 > > I think the solution in the patch is better as to try to maintain the > mach-types file: not very nice, but setting its value is not different > as several other setups inside the configuration file that are very > board specifiv. +1 > And CONFIG_MACH_TYPE is well documented. 10x ;-) > >> in which case we should stop using any definitions from it, >> especially for new boards where it's not needed due to DT support in >> the kernel. > > Agree completely - mach-types introduces a strong dependency with the > kernel, and we do not need it. +1 I think, some more time is needed for the transition to DT. We should let the boards use non-DT configurations, but move the maintenance to the board maintainer (i.e. set the CONFIG_MACH_TYPE in the board config file). Also, soon there will be no non-DT boards accepted in the mainline kernel (probably, also, there will be no new machine types accepted in the machine types registry), so the need for the mach-type will just go away, no?
On 03/04/2012 04:09 AM, Stefano Babic wrote: > On 03/03/2012 14:30, Wolfgang Denk wrote: >> Dear Dirk Behme, >> >> <snip> >> >> There are 3 commits in this series: >> >> [PATCH 1/3] i.MX6: mx6qsabrelite: add CONFIG_REVISION_TAG >> [PATCH 2/3] i.MX6: mx6qsabrelite: add MACH_TYPE_MX6Q_SABRELITE >> [PATCH 3/3] i.MX6: mx6qsabrelite: add ext2 support >> >> I dislike #1 because it uses the completely undocumented >> CONFIG_REVISION_TAG, and I agree with Marek's and Stefano's comments. > > A lot of boards are currently set CONFIG_REVISION_TAG. Sure, it would be > nice to document it. To be consistent we should drop this CONFIG_ from > all boards, or add documentation for it. > > However, I am asking if this TAG is really needed. I have searched in > 2.6.38 Kernel provided by Freescale if the TAG is really evaluated to > set different revisions of the boards, but I have not found anything. Is > it really needed ? If not, we should drop it. > The linkage is really indirect. The ATAG item is still supported in the main-line kernel for ARM: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/kernel/setup.c;h=a255c39612ca3cfa10bddb7c7728216efeeb04d5;hb=HEAD#l704 The breakage I noticed was in the VPU driver, which refused to load with a zero-value in system_rev. The net result was no video playback in the Freescale Android ICS release.
On 04/03/2012 20:45, Eric Nelson wrote: > The linkage is really indirect. The ATAG item is still supported in the > main-line kernel for ARM: > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/kernel/setup.c;h=a255c39612ca3cfa10bddb7c7728216efeeb04d5;hb=HEAD#l704 > > > The breakage I noticed was in the VPU driver, which refused to load > with a zero-value in system_rev. The net result was no video playback > in the Freescale Android ICS release. However, I have not found a statement in FSL's kernel where system_rev in VPU driver is checked, directly or indirectly - neither in VPU driver nor in the board initialization code, nor in another MXC driver. It seems to me a side-effect in FSL's kernel, and for some not yet known reasons the problem disappears with this tag - or can reappear later, because we do not know the cause. And if I am not wrong, there are two macros in FSL's kernel checking the system_rev (arch/arm/plat-mxc/include/mach/mxc.h): #define imx_cpu_ver() (system_rev & 0xFF) #define board_is_rev(rev) (((system_rev & 0x0F00) == rev) ? 1 : 0) But you defines board_rev as: > +u32 get_board_rev(void) > +{ > + return 0x63000 ; Both macro still return 0x00...there is no change if the ATAG is not set. Sure that the problem you report is really bound to system_rev ? I am quite OT here, we are investigating an issue in FSL kernel on the U-boot ML. Anyway, the ATAG is supported and as I already said quite common for U-Boot boards. The commit message " Freescale 2.6.38 (Non-DT) kernels require the revision atag to enable the VPU." should be extended explaining the real cause (if known) or changed dropping VPU because there is no clear relationship between the ATAG and the issue. Best regards, Stefano Babic
Hi Wolfgang, Le 04/03/2012 09:39, Wolfgang Denk a écrit : > Dear Troy Kisky, > > In message<4F52C327.3080309@boundarydevices.com> you wrote: >> >>>> And which changes 'introduce forward incompatibilities', and what are >>>> these incompatibilities? >>> See the recent problems that occurred when RMK decided to "clean up" >>> the machids file. >>> >> Would you rather that I take RMK's cleaned up file, and undelete the >> machines that u-boot >> uses? That would be more simple than adding to the board's config file. >> I can delete all of the mach_is_xxx macros in mach-types while I'm at it. > > I think we had this discussion before (when RMK's changes hit us), and > it was decided not to do this. IIRC we decided not to do this. YRC. :) The retained strategy is to not fiddle with the Linux mach-type.h and to complement (and lampshade) any discrepancy between Linux and U-Boot support in the board config files. > I have never understood why this mach_types thingy was needed (other > rchitectures worked fine without it, or better), and now we are on the > edge of obsoleting it. So all efforts trying to maintain this file are > futile, and we would have to redo these for any updates of the file. > > I feel this is not a good investment of our time. Hopefully FDT will make mach-types obsolete, except for the rare boards which will want to keep support for it, at which point we'll decide to either maintain or own, reduced and mostly legacy, mach-type file, or to move mach-type declarations in the board's config files. > Best regards, > > Wolfgang Denk Amicalement,
Hi Stefano, Thanks for the feedback and the prod. On 03/07/2012 01:43 AM, Stefano Babic wrote: > On 04/03/2012 20:45, Eric Nelson wrote: > >> The linkage is really indirect. The ATAG item is still supported in the >> main-line kernel for ARM: >> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/kernel/setup.c;h=a255c39612ca3cfa10bddb7c7728216efeeb04d5;hb=HEAD#l704 >> >> The breakage I noticed was in the VPU driver, which refused to load >> with a zero-value in system_rev. The net result was no video playback >> in the Freescale Android ICS release. > > However, I have not found a statement in FSL's kernel where system_rev > in VPU driver is checked, directly or indirectly - neither in VPU driver > nor in the board initialization code, nor in another MXC driver. > It seems to me a side-effect in FSL's kernel, and for some not yet known > reasons the problem disappears with this tag - or can reappear later, > because we do not know the cause. > I did the same search but was apparently not as persistent as you were. The symptom is simple: Video won't play back on Android (R13.1 ICS) without the system revision but play nicely with it. > And if I am not wrong, there are two macros in FSL's kernel checking the > system_rev (arch/arm/plat-mxc/include/mach/mxc.h): > > #define imx_cpu_ver() (system_rev& 0xFF) > #define board_is_rev(rev) (((system_rev& 0x0F00) == rev) ? 1 : 0) > > But you defines board_rev as: >> +u32 get_board_rev(void) >> +{ >> + return 0x63000 ; > > Both macro still return 0x00...there is no change if the ATAG is not > set. Sure that the problem you report is really bound to system_rev ? I > am quite OT here, we are investigating an issue in FSL kernel on the > U-boot ML. > I think I just found the culprit, and it's in userspace, not in the kernel. In package imx-lib-11.11.01, file vpu/vpu.c, there's this routine that sets a global system_rev based on /proc/cpuinfo: static int get_system_rev(void) { FILE *fp; char buf[1024]; int nread; char *tmp, *rev; int ret = -1; fp = fopen("/proc/cpuinfo", "r"); if (fp == NULL) { perror("/proc/cpuinfo\n"); return ret; } nread = fread(buf, 1, sizeof(buf), fp); fclose(fp); if ((nread == 0) || (nread == sizeof(buf))) { fclose(fp); return ret; } buf[nread] = '\0'; tmp = strstr(buf, "Revision"); if (tmp != NULL) { rev = index(tmp, ':'); if (rev != NULL) { rev++; system_rev = strtoul(rev, NULL, 16); ret = 0; } } return ret; } The global is then exported via macros: vpu/vpu_io.c:unsigned int system_rev; vpu/vpu_io.c:static int get_system_rev(void) vpu/vpu_io.c: system_rev = strtoul(rev, NULL, 16); vpu/vpu_io.c: ret = get_system_rev(); vpu/vpu_lib.h:extern unsigned int system_rev; vpu/vpu_lib.h:#define mxc_cpu() (system_rev >> 12) vpu/vpu_lib.h:#define mxc_cpu_rev() (system_rev & 0xFF) and used to find the firmware file: vpu/vpu_util.c: sprintf(temp_str, "vpu_fw_imx%2x.bin", mxc_cpu()); ...all to support the userspace I/O for the VPU. We are way off topic here, but I certainly hope we can address this in the future and get a real driver written for the VPU. > Anyway, the ATAG is supported and as I already said quite common for > U-Boot boards. The commit message " Freescale 2.6.38 (Non-DT) kernels > require the revision atag to enable the VPU." should be extended > explaining the real cause (if known) or changed dropping VPU because > there is no clear relationship between the ATAG and the issue. > How about something more generic like this? "Freescale Linux distributions depend on system_rev". > Best regards, > Stefano Babic >
On 07/03/2012 16:53, Eric Nelson wrote: > Hi Stefano, > Hi Eric, > > I did the same search but was apparently not as persistent as you were. > The symptom is simple: Video won't play back on Android (R13.1 ICS) > without the system revision but play nicely with it. Ok, we know the symptoms... > I think I just found the culprit, and it's in userspace, not in the > kernel. In package imx-lib-11.11.01, file vpu/vpu.c, there's this > routine that sets a global system_rev based on /proc/cpuinfo: > > static int get_system_rev(void) > { > FILE *fp; > char buf[1024]; > int nread; > char *tmp, *rev; > int ret = -1; > > fp = fopen("/proc/cpuinfo", "r"); > if (fp == NULL) { > perror("/proc/cpuinfo\n"); > return ret; > } > > nread = fread(buf, 1, sizeof(buf), fp); > fclose(fp); > if ((nread == 0) || (nread == sizeof(buf))) { > fclose(fp); > return ret; > } > > buf[nread] = '\0'; > > tmp = strstr(buf, "Revision"); > if (tmp != NULL) { > rev = index(tmp, ':'); > if (rev != NULL) { > rev++; > system_rev = strtoul(rev, NULL, 16); > ret = 0; > } > } > > return ret; > } > > The global is then exported via macros: > vpu/vpu_io.c:unsigned int system_rev; > vpu/vpu_io.c:static int get_system_rev(void) > vpu/vpu_io.c: system_rev = strtoul(rev, NULL, 16); > vpu/vpu_io.c: ret = get_system_rev(); > vpu/vpu_lib.h:extern unsigned int system_rev; > vpu/vpu_lib.h:#define mxc_cpu() (system_rev >> 12) > vpu/vpu_lib.h:#define mxc_cpu_rev() (system_rev & 0xFF) > > and used to find the firmware file: > vpu/vpu_util.c: sprintf(temp_str, "vpu_fw_imx%2x.bin", mxc_cpu()); > > ...all to support the userspace I/O for the VPU. Understood - this is really crappy, because it makes so absurd dependencies that is very easy to break - and when it happens, nobody knows why, as we find now. Really this is a problem neither in u-boot nor in kernel... > > We are way off topic here, Well, we have now the cause... > but I certainly hope we can address this in the > future and get a real driver written for the VPU. ..in the mainline kernel... > >> Anyway, the ATAG is supported and as I already said quite common for >> U-Boot boards. The commit message " Freescale 2.6.38 (Non-DT) kernels >> require the revision atag to enable the VPU." should be extended >> explaining the real cause (if known) or changed dropping VPU because >> there is no clear relationship between the ATAG and the issue. >> > > How about something more generic like this? > "Freescale Linux distributions depend on system_rev". Because you deeply investigated and found the reason, I propose you add a full description indicating that the imx lib libraries depend on the system_rev in kernel to transfer the correct firmware. So we know it is neither a problem in u-boot nor in kernel, but we as u-bootlers are fair with some bad implemented libraries.... Normally I would say that the fix should be done where the bug is - we are introducing a work-around for a problem in user space. But as I stated previously, the revision tag is used on a lot of ARM boards, and there is no reason to reject it only on this board. Best regards, Stefano Babic
--- a/include/configs/mx6qsabrelite.h +++ b/include/configs/mx6qsabrelite.h @@ -27,6 +27,7 @@ #define CONFIG_SYS_MX6_CLK32 32768 #define CONFIG_DISPLAY_CPUINFO #define CONFIG_DISPLAY_BOARDINFO +#define CONFIG_MACH_TYPE 3769 #include <asm/arch/imx-regs.h>