| Message ID | 1396132481-9446-1-git-send-email-troy.kisky@boundarydevices.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
On Sat, Mar 29, 2014 at 7:34 PM, Troy Kisky <troy.kisky@boundarydevices.com> wrote: > This removes one block in the move toward 1 u-boot > for both a mx6q (quad) and mx6dl (duallite) processor. > > Now fdt_file hardcoded value can be removed. > > Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> Nice! I do think it is in the right direction. I will give it a try soon...
Hi Troy, On 03/29/2014 03:34 PM, Troy Kisky wrote: > This removes one block in the move toward 1 u-boot > for both a mx6q (quad) and mx6dl (duallite) processor. > > Now fdt_file hardcoded value can be removed. > > Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> > --- > arch/arm/imx-common/cpu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > arch/arm/lib/board.c | 7 +++++++ > 2 files changed, 51 insertions(+) > > diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c > index a77c4de..5d48011 100644 > --- a/arch/arm/imx-common/cpu.c > +++ b/arch/arm/imx-common/cpu.c > @@ -180,3 +180,47 @@ void arch_preboot_os(void) > ipuv3_fb_shutdown(); > } > #endif > + > +const char *get_dtb_prefix(u32 imxtype) > +{ > + switch (imxtype) { > + case MXC_CPU_MX6Q: > + case MXC_CPU_MX6D: > + return "imx6q"; /* Quad/Dual-core version of the mx6 */ > + case MXC_CPU_MX6DL: > + case MXC_CPU_MX6SOLO: > + return "imx6dl"; /* Dual Lite/Solo version of the mx6 */ > + case MXC_CPU_MX6SL: > + return "imx6sl"; /* Solo-Lite version of the mx6 */ > + case MXC_CPU_MX51: > + return "imx51"; > + case MXC_CPU_MX53: > + return "imx53"; > + } > + return "??"; > +} > + I really dislike this implementation of naming policy in code. > +int cpu_late_init(void) > +{ > + char buf[128]; > + const char *board; > + u32 imxtype = (get_cpu_rev() >> 12) & 0xff; > + > + if (getenv("fdt_file")) > + return 0; > + board = getenv("board"); > + if (!board) { > + board = CONFIG_SYS_BOARD; And this part seems to impose a board-naming requirement to implement the dtb-naming requirement: > + if ((board[0] == 'm') && (board[1] == 'x')) { > + if (board[2] == '6') { > + board += 3; > + } else if (board[2] == '5') { > + if ((board[3] == '1') || (board[3] == '3')) > + board += 4; > + } > + } > + } > + sprintf(buf, "%s-%s.dtb", get_dtb_prefix(imxtype), board); > + setenv("fdt_file", buf); > + return 0; > +} I sent patches last year to provide default "cpu" and "board" environment variables that could be used by boot scripts to implement this part. http://lists.denx.de/pipermail/u-boot/2013-November/thread.html#167129 That seems more generally useful. Otherwise, I'd rather just see an environment-variable convention. Regards, Eric
On Sunday, March 30, 2014 at 12:04:31 AM, Otavio Salvador wrote: > On Sat, Mar 29, 2014 at 7:34 PM, Troy Kisky > > <troy.kisky@boundarydevices.com> wrote: > > This removes one block in the move toward 1 u-boot > > for both a mx6q (quad) and mx6dl (duallite) processor. > > > > Now fdt_file hardcoded value can be removed. > > > > Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> > > Nice! I do think it is in the right direction. I will give it a try soon... Full NAK on this patch, I completely agree with Eric that this approach is totally wrong. This adds stuff which should be pulled from DT into common code, this is just NAK. Best regards, Marek Vasut
Hi Troy, On 29/03/2014 23:34, Troy Kisky wrote: > This removes one block in the move toward 1 u-boot > for both a mx6q (quad) and mx6dl (duallite) processor. > > Now fdt_file hardcoded value can be removed. > > Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> > --- I have a general problem with this implementation. I am ok if, as you proposed some times ago, there is a general rule for the "default" dtb file in the CONFIG_EXTRA_ENV. However, you are binding in code naming conventions. In U-boot, it must be allowed to set the environment as the user wants, and this must be not overwritten by such an internal code. I mean: a board user, if he wants, should be allowed to do something as setenv fdt_file my_preferred_dtb_name.dtb and this must work when the file is loaded from storage - this is not possible if the rule chosen from user is overwritten by code. This makes the environment useless and generates headaches for a lot of users. They will ask themselves why the wrong file is taken when they tried in any way to set it differently... > arch/arm/imx-common/cpu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > arch/arm/lib/board.c | 7 +++++++ > 2 files changed, 51 insertions(+) > > diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c > index a77c4de..5d48011 100644 > --- a/arch/arm/imx-common/cpu.c > +++ b/arch/arm/imx-common/cpu.c > @@ -180,3 +180,47 @@ void arch_preboot_os(void) > ipuv3_fb_shutdown(); > } > #endif > + > +const char *get_dtb_prefix(u32 imxtype) > +{ > + switch (imxtype) { > + case MXC_CPU_MX6Q: > + case MXC_CPU_MX6D: > + return "imx6q"; /* Quad/Dual-core version of the mx6 */ > + case MXC_CPU_MX6DL: > + case MXC_CPU_MX6SOLO: > + return "imx6dl"; /* Dual Lite/Solo version of the mx6 */ > + case MXC_CPU_MX6SL: > + return "imx6sl"; /* Solo-Lite version of the mx6 */ > + case MXC_CPU_MX51: > + return "imx51"; > + case MXC_CPU_MX53: > + return "imx53"; > + } > + return "??"; > +} > + > +int cpu_late_init(void) > +{ > + char buf[128]; > + const char *board; > + u32 imxtype = (get_cpu_rev() >> 12) & 0xff; > + > + if (getenv("fdt_file")) > + return 0; > + board = getenv("board"); > + if (!board) { > + board = CONFIG_SYS_BOARD; > + if ((board[0] == 'm') && (board[1] == 'x')) { > + if (board[2] == '6') { > + board += 3; > + } else if (board[2] == '5') { > + if ((board[3] == '1') || (board[3] == '3')) > + board += 4; > + } > + } > + } > + sprintf(buf, "%s-%s.dtb", get_dtb_prefix(imxtype), board); > + setenv("fdt_file", buf); > + return 0; > +} > diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c > index e9a7708..61cee98 100644 > --- a/arch/arm/lib/board.c > +++ b/arch/arm/lib/board.c > @@ -496,6 +496,11 @@ static void display_fdt_model(const void *blob) > } > #endif > > +int __weak cpu_late_init(void) > +{ > + return 0; > +} > + > /************************************************************************ > * > * This is the next part if the initialization sequence: we are now > @@ -649,6 +654,8 @@ void board_init_r(gd_t *id, ulong dest_addr) > board_late_init(); > #endif > > + cpu_late_init(); > + > #ifdef CONFIG_BITBANGMII > bb_miiphy_init(); > #endif > Best regards, Stefano Babic
On 3/30/2014 9:30 AM, Stefano Babic wrote: > Hi Troy, > > On 29/03/2014 23:34, Troy Kisky wrote: >> This removes one block in the move toward 1 u-boot >> for both a mx6q (quad) and mx6dl (duallite) processor. >> >> Now fdt_file hardcoded value can be removed. >> >> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> >> --- > > I have a general problem with this implementation. I am ok if, as you > proposed some times ago, there is a general rule for the "default" dtb > file in the CONFIG_EXTRA_ENV. > > However, you are binding in code naming conventions. In U-boot, it must > be allowed to set the environment as the user wants, and this must be > not overwritten by such an internal code. In the patch, the code returns without any changes if fdt_file is already defined. So, I don't know what you are referring to here. > > I mean: a board user, if he wants, should be allowed to do something as > > setenv fdt_file my_preferred_dtb_name.dtb > > and this must work when the file is loaded from storage - this is not > possible if the rule chosen from user is overwritten by code. I agree, but it does work. > > This makes the environment useless and generates headaches for a lot of > users. They will ask themselves why the wrong file is taken when they > tried in any way to set it differently... Still no disagreement.
On 3/30/2014 7:52 AM, Marek Vasut wrote: > On Sunday, March 30, 2014 at 12:04:31 AM, Otavio Salvador wrote: >> On Sat, Mar 29, 2014 at 7:34 PM, Troy Kisky >> >> <troy.kisky@boundarydevices.com> wrote: >>> This removes one block in the move toward 1 u-boot >>> for both a mx6q (quad) and mx6dl (duallite) processor. >>> >>> Now fdt_file hardcoded value can be removed. >>> >>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> >> >> Nice! I do think it is in the right direction. I will give it a try soon... > > Full NAK on this patch, I completely agree with Eric that this approach is > totally wrong. This adds stuff which should be pulled from DT into common code, > this is just NAK. > > Best regards, > Marek Vasut > I am not sure what you are suggesting. Don't you have an chicken and egg problem? Troy
On 3/29/2014 4:11 PM, Eric Nelson wrote: > Hi Troy, > > On 03/29/2014 03:34 PM, Troy Kisky wrote: >> This removes one block in the move toward 1 u-boot >> for both a mx6q (quad) and mx6dl (duallite) processor. >> >> Now fdt_file hardcoded value can be removed. >> >> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> >> --- >> arch/arm/imx-common/cpu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> arch/arm/lib/board.c | 7 +++++++ >> 2 files changed, 51 insertions(+) >> >> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c >> index a77c4de..5d48011 100644 >> --- a/arch/arm/imx-common/cpu.c >> +++ b/arch/arm/imx-common/cpu.c >> @@ -180,3 +180,47 @@ void arch_preboot_os(void) >> ipuv3_fb_shutdown(); >> } >> #endif >> + >> +const char *get_dtb_prefix(u32 imxtype) >> +{ >> + switch (imxtype) { >> + case MXC_CPU_MX6Q: >> + case MXC_CPU_MX6D: >> + return "imx6q"; /* Quad/Dual-core version of the mx6 */ >> + case MXC_CPU_MX6DL: >> + case MXC_CPU_MX6SOLO: >> + return "imx6dl"; /* Dual Lite/Solo version of the mx6 */ >> + case MXC_CPU_MX6SL: >> + return "imx6sl"; /* Solo-Lite version of the mx6 */ >> + case MXC_CPU_MX51: >> + return "imx51"; >> + case MXC_CPU_MX53: >> + return "imx53"; >> + } >> + return "??"; >> +} >> + > > I really dislike this implementation of naming policy in code. It is not truly a policy. It is a convenience which can be ignored if so desired. Though I do agree that cpu and board environment variables would also be useful. Still, a cpu variable would still require some scripting to combine the quad/dual, duallite/solo. So, your way is not as convenient for dtb file names. Troy
On Monday, March 31, 2014 at 08:36:55 PM, Troy Kisky wrote: > On 3/29/2014 4:11 PM, Eric Nelson wrote: > > Hi Troy, > > > > On 03/29/2014 03:34 PM, Troy Kisky wrote: > >> This removes one block in the move toward 1 u-boot > >> for both a mx6q (quad) and mx6dl (duallite) processor. > >> > >> Now fdt_file hardcoded value can be removed. > >> > >> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> > >> --- > >> > >> arch/arm/imx-common/cpu.c | 44 > >> ++++++++++++++++++++++++++++++++++++++++++++ arch/arm/lib/board.c > >> | 7 +++++++ > >> 2 files changed, 51 insertions(+) > >> > >> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c > >> index a77c4de..5d48011 100644 > >> --- a/arch/arm/imx-common/cpu.c > >> +++ b/arch/arm/imx-common/cpu.c > >> @@ -180,3 +180,47 @@ void arch_preboot_os(void) > >> > >> ipuv3_fb_shutdown(); > >> > >> } > >> #endif > >> > >> + > >> +const char *get_dtb_prefix(u32 imxtype) > >> +{ > >> + switch (imxtype) { > >> + case MXC_CPU_MX6Q: > >> + case MXC_CPU_MX6D: > >> + return "imx6q"; /* Quad/Dual-core version of the mx6 */ > >> + case MXC_CPU_MX6DL: > >> + case MXC_CPU_MX6SOLO: > >> + return "imx6dl"; /* Dual Lite/Solo version of the mx6 */ > >> + case MXC_CPU_MX6SL: > >> + return "imx6sl"; /* Solo-Lite version of the mx6 */ > >> + case MXC_CPU_MX51: > >> + return "imx51"; > >> + case MXC_CPU_MX53: > >> + return "imx53"; > >> + } > >> + return "??"; > >> +} > >> + > > > > I really dislike this implementation of naming policy in code. > > It is not truly a policy. It is a convenience which can be ignored > if so desired. Though I do agree that cpu and board environment variables > would also be useful. Still, a cpu variable would still require > some scripting to combine the quad/dual, duallite/solo. So, your > way is not as convenient for dtb file names. You can make the CPU code set the CPU type into some variable. Afterwards, some scripts in the HUSH can assemble the DTB name based on those variables. This would be much more generic and re-usable than this ... Best regards, Marek Vasut
On Mon, Mar 31, 2014 at 4:22 PM, Marek Vasut <marex@denx.de> wrote: > On Monday, March 31, 2014 at 08:36:55 PM, Troy Kisky wrote: >> On 3/29/2014 4:11 PM, Eric Nelson wrote: >> > Hi Troy, >> > >> > On 03/29/2014 03:34 PM, Troy Kisky wrote: >> >> This removes one block in the move toward 1 u-boot >> >> for both a mx6q (quad) and mx6dl (duallite) processor. >> >> >> >> Now fdt_file hardcoded value can be removed. >> >> >> >> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> >> >> --- >> >> >> >> arch/arm/imx-common/cpu.c | 44 >> >> ++++++++++++++++++++++++++++++++++++++++++++ arch/arm/lib/board.c >> >> | 7 +++++++ >> >> 2 files changed, 51 insertions(+) >> >> >> >> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c >> >> index a77c4de..5d48011 100644 >> >> --- a/arch/arm/imx-common/cpu.c >> >> +++ b/arch/arm/imx-common/cpu.c >> >> @@ -180,3 +180,47 @@ void arch_preboot_os(void) >> >> >> >> ipuv3_fb_shutdown(); >> >> >> >> } >> >> #endif >> >> >> >> + >> >> +const char *get_dtb_prefix(u32 imxtype) >> >> +{ >> >> + switch (imxtype) { >> >> + case MXC_CPU_MX6Q: >> >> + case MXC_CPU_MX6D: >> >> + return "imx6q"; /* Quad/Dual-core version of the mx6 */ >> >> + case MXC_CPU_MX6DL: >> >> + case MXC_CPU_MX6SOLO: >> >> + return "imx6dl"; /* Dual Lite/Solo version of the mx6 */ >> >> + case MXC_CPU_MX6SL: >> >> + return "imx6sl"; /* Solo-Lite version of the mx6 */ >> >> + case MXC_CPU_MX51: >> >> + return "imx51"; >> >> + case MXC_CPU_MX53: >> >> + return "imx53"; >> >> + } >> >> + return "??"; >> >> +} >> >> + >> > >> > I really dislike this implementation of naming policy in code. >> >> It is not truly a policy. It is a convenience which can be ignored >> if so desired. Though I do agree that cpu and board environment variables >> would also be useful. Still, a cpu variable would still require >> some scripting to combine the quad/dual, duallite/solo. So, your >> way is not as convenient for dtb file names. > > You can make the CPU code set the CPU type into some variable. Afterwards, some > scripts in the HUSH can assemble the DTB name based on those variables. This > would be much more generic and re-usable than this ... The problem is this script will be mostly the same for most boards.
On Monday, March 31, 2014 at 09:38:02 PM, Otavio Salvador wrote: > On Mon, Mar 31, 2014 at 4:22 PM, Marek Vasut <marex@denx.de> wrote: > > On Monday, March 31, 2014 at 08:36:55 PM, Troy Kisky wrote: > >> On 3/29/2014 4:11 PM, Eric Nelson wrote: > >> > Hi Troy, > >> > > >> > On 03/29/2014 03:34 PM, Troy Kisky wrote: > >> >> This removes one block in the move toward 1 u-boot > >> >> for both a mx6q (quad) and mx6dl (duallite) processor. > >> >> > >> >> Now fdt_file hardcoded value can be removed. > >> >> > >> >> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> > >> >> --- > >> >> > >> >> arch/arm/imx-common/cpu.c | 44 > >> >> ++++++++++++++++++++++++++++++++++++++++++++ arch/arm/lib/board.c > >> >> > >> >> | 7 +++++++ > >> >> > >> >> 2 files changed, 51 insertions(+) > >> >> > >> >> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c > >> >> index a77c4de..5d48011 100644 > >> >> --- a/arch/arm/imx-common/cpu.c > >> >> +++ b/arch/arm/imx-common/cpu.c > >> >> @@ -180,3 +180,47 @@ void arch_preboot_os(void) > >> >> > >> >> ipuv3_fb_shutdown(); > >> >> > >> >> } > >> >> #endif > >> >> > >> >> + > >> >> +const char *get_dtb_prefix(u32 imxtype) > >> >> +{ > >> >> + switch (imxtype) { > >> >> + case MXC_CPU_MX6Q: > >> >> + case MXC_CPU_MX6D: > >> >> + return "imx6q"; /* Quad/Dual-core version of the mx6 > >> >> */ + case MXC_CPU_MX6DL: > >> >> + case MXC_CPU_MX6SOLO: > >> >> + return "imx6dl"; /* Dual Lite/Solo version of the mx6 */ > >> >> + case MXC_CPU_MX6SL: > >> >> + return "imx6sl"; /* Solo-Lite version of the mx6 */ > >> >> + case MXC_CPU_MX51: > >> >> + return "imx51"; > >> >> + case MXC_CPU_MX53: > >> >> + return "imx53"; > >> >> + } > >> >> + return "??"; > >> >> +} > >> >> + > >> > > >> > I really dislike this implementation of naming policy in code. > >> > >> It is not truly a policy. It is a convenience which can be ignored > >> if so desired. Though I do agree that cpu and board environment > >> variables would also be useful. Still, a cpu variable would still > >> require some scripting to combine the quad/dual, duallite/solo. So, > >> your way is not as convenient for dtb file names. > > > > You can make the CPU code set the CPU type into some variable. > > Afterwards, some scripts in the HUSH can assemble the DTB name based on > > those variables. This would be much more generic and re-usable than this > > ... > > The problem is this script will be mostly the same for most boards. This does by no means justify poluting code with non-reusable convoluted stuff. A script which is part of the environment can be tweaked on a running system as seen fit at any later date, updating bootloader on a running system is often not an option. Furthermore, having partial information which can be used for decisionmaking is much better than having a lengthy string which needs to be parsed first. Especially with the limited parsing options we have in some configurations of U- Boot. Best regards, Marek Vasut
On 3/31/2014 12:47 PM, Marek Vasut wrote: > On Monday, March 31, 2014 at 09:38:02 PM, Otavio Salvador wrote: >> On Mon, Mar 31, 2014 at 4:22 PM, Marek Vasut <marex@denx.de> wrote: >>> On Monday, March 31, 2014 at 08:36:55 PM, Troy Kisky wrote: >>>> On 3/29/2014 4:11 PM, Eric Nelson wrote: >>>>> Hi Troy, >>>>> >>>>> On 03/29/2014 03:34 PM, Troy Kisky wrote: >>>>>> This removes one block in the move toward 1 u-boot >>>>>> for both a mx6q (quad) and mx6dl (duallite) processor. >>>>>> >>>>>> Now fdt_file hardcoded value can be removed. >>>>>> >>>>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> >>>>>> --- >>>>>> >>>>>> arch/arm/imx-common/cpu.c | 44 >>>>>> ++++++++++++++++++++++++++++++++++++++++++++ arch/arm/lib/board.c >>>>>> >>>>>> | 7 +++++++ >>>>>> >>>>>> 2 files changed, 51 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c >>>>>> index a77c4de..5d48011 100644 >>>>>> --- a/arch/arm/imx-common/cpu.c >>>>>> +++ b/arch/arm/imx-common/cpu.c >>>>>> @@ -180,3 +180,47 @@ void arch_preboot_os(void) >>>>>> >>>>>> ipuv3_fb_shutdown(); >>>>>> >>>>>> } >>>>>> #endif >>>>>> >>>>>> + >>>>>> +const char *get_dtb_prefix(u32 imxtype) >>>>>> +{ >>>>>> + switch (imxtype) { >>>>>> + case MXC_CPU_MX6Q: >>>>>> + case MXC_CPU_MX6D: >>>>>> + return "imx6q"; /* Quad/Dual-core version of the mx6 >>>>>> */ + case MXC_CPU_MX6DL: >>>>>> + case MXC_CPU_MX6SOLO: >>>>>> + return "imx6dl"; /* Dual Lite/Solo version of the mx6 */ >>>>>> + case MXC_CPU_MX6SL: >>>>>> + return "imx6sl"; /* Solo-Lite version of the mx6 */ >>>>>> + case MXC_CPU_MX51: >>>>>> + return "imx51"; >>>>>> + case MXC_CPU_MX53: >>>>>> + return "imx53"; >>>>>> + } >>>>>> + return "??"; >>>>>> +} >>>>>> + >>>>> >>>>> I really dislike this implementation of naming policy in code. >>>> >>>> It is not truly a policy. It is a convenience which can be ignored >>>> if so desired. Though I do agree that cpu and board environment >>>> variables would also be useful. Still, a cpu variable would still >>>> require some scripting to combine the quad/dual, duallite/solo. So, >>>> your way is not as convenient for dtb file names. >>> >>> You can make the CPU code set the CPU type into some variable. >>> Afterwards, some scripts in the HUSH can assemble the DTB name based on >>> those variables. This would be much more generic and re-usable than this >>> ... >> >> The problem is this script will be mostly the same for most boards. > > This does by no means justify poluting code with non-reusable convoluted stuff. > A script which is part of the environment can be tweaked on a running system as > seen fit at any later date, updating bootloader on a running system is often not > an option. > > Furthermore, having partial information which can be used for decisionmaking is > much better than having a lengthy string which needs to be parsed first. > Especially with the limited parsing options we have in some configurations of U- > Boot. > I can agree to that, if I understand you correctly. So are you fine with having a board and cpu variable? If so, what should the cpu variable contain? I propose cpu defaults to "IMX%s", get_imx_type(imxtype) so cpu=IMX6Q, IMX6D, IMX6DL, IMX6SOLO, IMX6SL, IMX51, IMX53 and board defaults to plain CONFIG_SYS_BOARD. So, mx6sabresd would need to set board=sabresd. in mx6sabresd.h setenv board=sabresd and in some common file setenv dtbpIMX6Q setenv dtbprefix imx6q setenv dtbpIMX6D setenv dtbprefix imx6d setenv dtbpIMX6DL setenv dtbprefix imx6dl setenv dtbpIMX6SOLO setenv dtbprefix imx6dl setenv dtbpIMX6SL setenv dtbprefix imx6sl setenv find_dtb_file 'run dtbp${cpu} ; setenv fdt_file $dtbprefix-$board' run find_dtb_file echo $fdt_file -------- Or if you know a better way please speak up. Thanks Troy
On Monday, March 31, 2014 at 11:09:14 PM, Troy Kisky wrote: > On 3/31/2014 12:47 PM, Marek Vasut wrote: > > On Monday, March 31, 2014 at 09:38:02 PM, Otavio Salvador wrote: > >> On Mon, Mar 31, 2014 at 4:22 PM, Marek Vasut <marex@denx.de> wrote: > >>> On Monday, March 31, 2014 at 08:36:55 PM, Troy Kisky wrote: > >>>> On 3/29/2014 4:11 PM, Eric Nelson wrote: > >>>>> Hi Troy, > >>>>> > >>>>> On 03/29/2014 03:34 PM, Troy Kisky wrote: > >>>>>> This removes one block in the move toward 1 u-boot > >>>>>> for both a mx6q (quad) and mx6dl (duallite) processor. > >>>>>> > >>>>>> Now fdt_file hardcoded value can be removed. > >>>>>> > >>>>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> > >>>>>> --- > >>>>>> > >>>>>> arch/arm/imx-common/cpu.c | 44 > >>>>>> ++++++++++++++++++++++++++++++++++++++++++++ arch/arm/lib/board.c > >>>>>> > >>>>>> | 7 +++++++ > >>>>>> > >>>>>> 2 files changed, 51 insertions(+) > >>>>>> > >>>>>> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c > >>>>>> index a77c4de..5d48011 100644 > >>>>>> --- a/arch/arm/imx-common/cpu.c > >>>>>> +++ b/arch/arm/imx-common/cpu.c > >>>>>> @@ -180,3 +180,47 @@ void arch_preboot_os(void) > >>>>>> > >>>>>> ipuv3_fb_shutdown(); > >>>>>> > >>>>>> } > >>>>>> #endif > >>>>>> > >>>>>> + > >>>>>> +const char *get_dtb_prefix(u32 imxtype) > >>>>>> +{ > >>>>>> + switch (imxtype) { > >>>>>> + case MXC_CPU_MX6Q: > >>>>>> + case MXC_CPU_MX6D: > >>>>>> + return "imx6q"; /* Quad/Dual-core version of the mx6 > >>>>>> */ + case MXC_CPU_MX6DL: > >>>>>> + case MXC_CPU_MX6SOLO: > >>>>>> + return "imx6dl"; /* Dual Lite/Solo version of the mx6 */ > >>>>>> + case MXC_CPU_MX6SL: > >>>>>> + return "imx6sl"; /* Solo-Lite version of the mx6 */ > >>>>>> + case MXC_CPU_MX51: > >>>>>> + return "imx51"; > >>>>>> + case MXC_CPU_MX53: > >>>>>> + return "imx53"; > >>>>>> + } > >>>>>> + return "??"; > >>>>>> +} > >>>>>> + > >>>>> > >>>>> I really dislike this implementation of naming policy in code. > >>>> > >>>> It is not truly a policy. It is a convenience which can be ignored > >>>> if so desired. Though I do agree that cpu and board environment > >>>> variables would also be useful. Still, a cpu variable would still > >>>> require some scripting to combine the quad/dual, duallite/solo. So, > >>>> your way is not as convenient for dtb file names. > >>> > >>> You can make the CPU code set the CPU type into some variable. > >>> Afterwards, some scripts in the HUSH can assemble the DTB name based on > >>> those variables. This would be much more generic and re-usable than > >>> this ... > >> > >> The problem is this script will be mostly the same for most boards. > > > > This does by no means justify poluting code with non-reusable convoluted > > stuff. A script which is part of the environment can be tweaked on a > > running system as seen fit at any later date, updating bootloader on a > > running system is often not an option. > > > > Furthermore, having partial information which can be used for > > decisionmaking is much better than having a lengthy string which needs > > to be parsed first. Especially with the limited parsing options we have > > in some configurations of U- Boot. > > I can agree to that, if I understand you correctly. So are you fine with > having a board and cpu variable? If so, what should the cpu variable > contain? Looks reasonable. > I propose cpu defaults to "IMX%s", get_imx_type(imxtype) This should be introduced and work for _all_ CPUs and must not be imx specific. Otherwise, for i.MX, you can pick whatever is suitable, IMX%s is OK. > so cpu=IMX6Q, IMX6D, IMX6DL, IMX6SOLO, IMX6SL, IMX51, IMX53 > > and board defaults to plain CONFIG_SYS_BOARD. So, mx6sabresd > would need to set board=sabresd. Again, should work across all boards. > in mx6sabresd.h > setenv board=sabresd > > and in some common file > setenv dtbpIMX6Q setenv dtbprefix imx6q This must be part of a per-board environment if and only if the board can contain multiple CPU configurations. [...]
diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c index a77c4de..5d48011 100644 --- a/arch/arm/imx-common/cpu.c +++ b/arch/arm/imx-common/cpu.c @@ -180,3 +180,47 @@ void arch_preboot_os(void) ipuv3_fb_shutdown(); } #endif + +const char *get_dtb_prefix(u32 imxtype) +{ + switch (imxtype) { + case MXC_CPU_MX6Q: + case MXC_CPU_MX6D: + return "imx6q"; /* Quad/Dual-core version of the mx6 */ + case MXC_CPU_MX6DL: + case MXC_CPU_MX6SOLO: + return "imx6dl"; /* Dual Lite/Solo version of the mx6 */ + case MXC_CPU_MX6SL: + return "imx6sl"; /* Solo-Lite version of the mx6 */ + case MXC_CPU_MX51: + return "imx51"; + case MXC_CPU_MX53: + return "imx53"; + } + return "??"; +} + +int cpu_late_init(void) +{ + char buf[128]; + const char *board; + u32 imxtype = (get_cpu_rev() >> 12) & 0xff; + + if (getenv("fdt_file")) + return 0; + board = getenv("board"); + if (!board) { + board = CONFIG_SYS_BOARD; + if ((board[0] == 'm') && (board[1] == 'x')) { + if (board[2] == '6') { + board += 3; + } else if (board[2] == '5') { + if ((board[3] == '1') || (board[3] == '3')) + board += 4; + } + } + } + sprintf(buf, "%s-%s.dtb", get_dtb_prefix(imxtype), board); + setenv("fdt_file", buf); + return 0; +} diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c index e9a7708..61cee98 100644 --- a/arch/arm/lib/board.c +++ b/arch/arm/lib/board.c @@ -496,6 +496,11 @@ static void display_fdt_model(const void *blob) } #endif +int __weak cpu_late_init(void) +{ + return 0; +} + /************************************************************************ * * This is the next part if the initialization sequence: we are now @@ -649,6 +654,8 @@ void board_init_r(gd_t *id, ulong dest_addr) board_late_init(); #endif + cpu_late_init(); + #ifdef CONFIG_BITBANGMII bb_miiphy_init(); #endif
This removes one block in the move toward 1 u-boot for both a mx6q (quad) and mx6dl (duallite) processor. Now fdt_file hardcoded value can be removed. Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> --- arch/arm/imx-common/cpu.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ arch/arm/lib/board.c | 7 +++++++ 2 files changed, 51 insertions(+)