[U-Boot] [PATCH 1/1] imx-common: cpu: add fdt_file environment variable

[U-Boot] [PATCH 1/1] imx-common: cpu: add fdt_file environment variable

[Troy Kisky]

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 "??";
+}
+
+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
-- 
1.8.1.2

[U-Boot] [PATCH 1/1] imx-common: cpu: add fdt_file environment variable

[Otavio Salvador]

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...

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH 1/1] imx-common: cpu: add fdt_file environment variable

[Marek Vasut]

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

[U-Boot] [PATCH 1/1] imx-common: cpu: add fdt_file environment variable

[Troy Kisky]

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

[U-Boot] [PATCH 1/1] imx-common: cpu: add fdt_file environment variable

[Eric Nelson]

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

[U-Boot] [PATCH 1/1] imx-common: cpu: add fdt_file environment variable

[Troy Kisky]

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

[U-Boot] [PATCH 1/1] imx-common: cpu: add fdt_file environment variable

[Marek Vasut]

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

[U-Boot] [PATCH 1/1] imx-common: cpu: add fdt_file environment variable

[Otavio Salvador]

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.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH 1/1] imx-common: cpu: add fdt_file environment variable

[Marek Vasut]

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

[U-Boot] [PATCH 1/1] imx-common: cpu: add fdt_file environment variable

[Troy Kisky]

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

[U-Boot] [PATCH 1/1] imx-common: cpu: add fdt_file environment variable

[Marek Vasut]

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.
[...]

[U-Boot] [PATCH 1/1] imx-common: cpu: add fdt_file environment variable

[Stefano Babic]

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

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH 1/1] imx-common: cpu: add fdt_file environment variable

[Troy Kisky]

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.