diff mbox

[U-Boot,v2] common: board_f: Dont relocate FDT incase of CONFIG_OF_EMBED

Message ID 4f35f3cfb25f82fd43444985c500558a809e7cb0.1448609289.git.michal.simek@xilinx.com
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Michal Simek Nov. 27, 2015, 7:28 a.m. UTC 
From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>

Don't relocate fdt in case of CONFIG_OF EMBED as the fdt is
already embedded with u-boot image. Also update the fdt_blob
correctly in this case

Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Alexey Brodkin <abrodkin@synopsys.com>
---

Changes in v2:
- Fix commit message reported by Alexey

 common/board_f.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Bin Meng Nov. 27, 2015, 7:46 a.m. UTC | #1 
On Fri, Nov 27, 2015 at 3:28 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>
> Don't relocate fdt in case of CONFIG_OF EMBED as the fdt is
> already embedded with u-boot image. Also update the fdt_blob
> correctly in this case
>
> Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Reviewed-by: Alexey Brodkin <abrodkin@synopsys.com>
> ---
>
> Changes in v2:
> - Fix commit message reported by Alexey
>
>  common/board_f.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/common/board_f.c b/common/board_f.c
> index b035c90ff3b7..91bf8beea1c6 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -508,7 +508,7 @@ static int reserve_global_data(void)
>         return 0;
>  }
>
> -static int reserve_fdt(void)
> +static int __maybe_unused reserve_fdt(void)
>  {
>         /*
>          * If the device tree is sitting immediately above our image then we
> @@ -658,7 +658,7 @@ static int setup_dram_config(void)
>         return 0;
>  }
>
> -static int reloc_fdt(void)
> +static int __maybe_unused reloc_fdt(void)
>  {
>         if (gd->flags & GD_FLG_SKIP_RELOC)
>                 return 0;
> @@ -687,6 +687,9 @@ static int setup_reloc(void)
>         gd->reloc_off = gd->relocaddr - (CONFIG_SYS_TEXT_BASE + 0x400);
>  #endif
>  #endif
> +#ifdef CONFIG_OF_EMBED
> +       gd->fdt_blob += gd->reloc_off;
> +#endif
>         memcpy(gd->new_gd, (char *)gd, sizeof(gd_t));
>
>         debug("Relocation Offset is: %08lx\n", gd->reloc_off);
> @@ -938,7 +941,9 @@ static init_fnc_t init_sequence_f[] = {
>  #endif
>         setup_machine,
>         reserve_global_data,
> +#ifndef CONFIG_OF_EMBED
>         reserve_fdt,
> +#endif
>         reserve_arch,
>         reserve_stacks,
>         setup_dram_config,
> @@ -955,7 +960,9 @@ static init_fnc_t init_sequence_f[] = {
>         setup_board_extra,
>  #endif
>         INIT_FUNC_WATCHDOG_RESET
> +#ifndef CONFIG_OF_EMBED
>         reloc_fdt,
> +#endif
>         setup_reloc,
>  #if defined(CONFIG_X86) || defined(CONFIG_ARC)
>         copy_uboot_to_ram,
> --

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested on QEMU x86
Tested-by: Bin Meng <bmeng.cn@gmail.com>
Thomas Chou Nov. 29, 2015, 6:15 a.m. UTC | #2 
Hi Michal,

On 2015年11月27日 15:28, Michal Simek wrote:
> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>
> Don't relocate fdt in case of CONFIG_OF EMBED as the fdt is
> already embedded with u-boot image. Also update the fdt_blob
> correctly in this case
>
> Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Reviewed-by: Alexey Brodkin <abrodkin@synopsys.com>
> ---
>
> Changes in v2:
> - Fix commit message reported by Alexey
>
>   common/board_f.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>

Tested on nios2 boards.

Tested-by: Thomas Chou <thomas@wytron.com.tw>
Acked-by: Thomas Chou <thomas@wytron.com.tw>
Simon Glass Nov. 30, 2015, 11:17 p.m. UTC | #3 
Hi Michal,

On 27 November 2015 at 00:28, Michal Simek <michal.simek@xilinx.com> wrote:
> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>
> Don't relocate fdt in case of CONFIG_OF EMBED as the fdt is
> already embedded with u-boot image. Also update the fdt_blob
> correctly in this case
>
> Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Reviewed-by: Alexey Brodkin <abrodkin@synopsys.com>
> ---
>
> Changes in v2:
> - Fix commit message reported by Alexey
>
>  common/board_f.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/common/board_f.c b/common/board_f.c
> index b035c90ff3b7..91bf8beea1c6 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -508,7 +508,7 @@ static int reserve_global_data(void)
>         return 0;
>  }
>
> -static int reserve_fdt(void)
> +static int __maybe_unused reserve_fdt(void)
>  {
>         /*
>          * If the device tree is sitting immediately above our image then we
> @@ -658,7 +658,7 @@ static int setup_dram_config(void)
>         return 0;
>  }
>
> -static int reloc_fdt(void)
> +static int __maybe_unused reloc_fdt(void)
>  {
>         if (gd->flags & GD_FLG_SKIP_RELOC)
>                 return 0;
> @@ -687,6 +687,9 @@ static int setup_reloc(void)
>         gd->reloc_off = gd->relocaddr - (CONFIG_SYS_TEXT_BASE + 0x400);
>  #endif
>  #endif
> +#ifdef CONFIG_OF_EMBED
> +       gd->fdt_blob += gd->reloc_off;
> +#endif
>         memcpy(gd->new_gd, (char *)gd, sizeof(gd_t));
>
>         debug("Relocation Offset is: %08lx\n", gd->reloc_off);
> @@ -938,7 +941,9 @@ static init_fnc_t init_sequence_f[] = {
>  #endif
>         setup_machine,
>         reserve_global_data,
> +#ifndef CONFIG_OF_EMBED
>         reserve_fdt,
> +#endif

Can you please put the #ifdef in the function? At some point we are
hoping to get rid of all these #ifdefs.

>         reserve_arch,
>         reserve_stacks,
>         setup_dram_config,
> @@ -955,7 +960,9 @@ static init_fnc_t init_sequence_f[] = {
>         setup_board_extra,
>  #endif
>         INIT_FUNC_WATCHDOG_RESET
> +#ifndef CONFIG_OF_EMBED
>         reloc_fdt,
> +#endif
>         setup_reloc,
>  #if defined(CONFIG_X86) || defined(CONFIG_ARC)
>         copy_uboot_to_ram,
> --
> 1.9.1
>

Regards,
Simon
Michal Simek Dec. 1, 2015, 6:56 a.m. UTC | #4 
On 1.12.2015 00:17, Simon Glass wrote:
> Hi Michal,
> 
> On 27 November 2015 at 00:28, Michal Simek <michal.simek@xilinx.com> wrote:
>> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>>
>> Don't relocate fdt in case of CONFIG_OF EMBED as the fdt is
>> already embedded with u-boot image. Also update the fdt_blob
>> correctly in this case
>>
>> Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> Reviewed-by: Alexey Brodkin <abrodkin@synopsys.com>
>> ---
>>
>> Changes in v2:
>> - Fix commit message reported by Alexey
>>
>>  common/board_f.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/board_f.c b/common/board_f.c
>> index b035c90ff3b7..91bf8beea1c6 100644
>> --- a/common/board_f.c
>> +++ b/common/board_f.c
>> @@ -508,7 +508,7 @@ static int reserve_global_data(void)
>>         return 0;
>>  }
>>
>> -static int reserve_fdt(void)
>> +static int __maybe_unused reserve_fdt(void)
>>  {
>>         /*
>>          * If the device tree is sitting immediately above our image then we
>> @@ -658,7 +658,7 @@ static int setup_dram_config(void)
>>         return 0;
>>  }
>>
>> -static int reloc_fdt(void)
>> +static int __maybe_unused reloc_fdt(void)
>>  {
>>         if (gd->flags & GD_FLG_SKIP_RELOC)
>>                 return 0;
>> @@ -687,6 +687,9 @@ static int setup_reloc(void)
>>         gd->reloc_off = gd->relocaddr - (CONFIG_SYS_TEXT_BASE + 0x400);
>>  #endif
>>  #endif
>> +#ifdef CONFIG_OF_EMBED
>> +       gd->fdt_blob += gd->reloc_off;
>> +#endif
>>         memcpy(gd->new_gd, (char *)gd, sizeof(gd_t));
>>
>>         debug("Relocation Offset is: %08lx\n", gd->reloc_off);
>> @@ -938,7 +941,9 @@ static init_fnc_t init_sequence_f[] = {
>>  #endif
>>         setup_machine,
>>         reserve_global_data,
>> +#ifndef CONFIG_OF_EMBED
>>         reserve_fdt,
>> +#endif
> 
> Can you please put the #ifdef in the function? At some point we are
> hoping to get rid of all these #ifdefs.

Do you think that this is the best way how to handle this?
We are targeting small uboot running from OCM on Zynq. Moving ifdef to
function itself will caused that we will waste some bytes which do nothing.
Reference in table stays there and then return from that function too.
It it is not huge but this way seems to me better.

But no problem to change it if you think that this is the best way how
to handle this.

Thanks,
Michal
Simon Glass Dec. 1, 2015, 3:09 p.m. UTC | #5 
Hi Michal,

On 30 November 2015 at 23:56, Michal Simek <michal.simek@xilinx.com> wrote:
> On 1.12.2015 00:17, Simon Glass wrote:
>> Hi Michal,
>>
>> On 27 November 2015 at 00:28, Michal Simek <michal.simek@xilinx.com> wrote:
>>> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>>>
>>> Don't relocate fdt in case of CONFIG_OF EMBED as the fdt is
>>> already embedded with u-boot image. Also update the fdt_blob
>>> correctly in this case
>>>
>>> Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>> Reviewed-by: Alexey Brodkin <abrodkin@synopsys.com>
>>> ---
>>>
>>> Changes in v2:
>>> - Fix commit message reported by Alexey
>>>
>>>  common/board_f.c | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/common/board_f.c b/common/board_f.c
>>> index b035c90ff3b7..91bf8beea1c6 100644
>>> --- a/common/board_f.c
>>> +++ b/common/board_f.c
>>> @@ -508,7 +508,7 @@ static int reserve_global_data(void)
>>>         return 0;
>>>  }
>>>
>>> -static int reserve_fdt(void)
>>> +static int __maybe_unused reserve_fdt(void)
>>>  {
>>>         /*
>>>          * If the device tree is sitting immediately above our image then we
>>> @@ -658,7 +658,7 @@ static int setup_dram_config(void)
>>>         return 0;
>>>  }
>>>
>>> -static int reloc_fdt(void)
>>> +static int __maybe_unused reloc_fdt(void)
>>>  {
>>>         if (gd->flags & GD_FLG_SKIP_RELOC)
>>>                 return 0;
>>> @@ -687,6 +687,9 @@ static int setup_reloc(void)
>>>         gd->reloc_off = gd->relocaddr - (CONFIG_SYS_TEXT_BASE + 0x400);
>>>  #endif
>>>  #endif
>>> +#ifdef CONFIG_OF_EMBED
>>> +       gd->fdt_blob += gd->reloc_off;
>>> +#endif
>>>         memcpy(gd->new_gd, (char *)gd, sizeof(gd_t));
>>>
>>>         debug("Relocation Offset is: %08lx\n", gd->reloc_off);
>>> @@ -938,7 +941,9 @@ static init_fnc_t init_sequence_f[] = {
>>>  #endif
>>>         setup_machine,
>>>         reserve_global_data,
>>> +#ifndef CONFIG_OF_EMBED
>>>         reserve_fdt,
>>> +#endif
>>
>> Can you please put the #ifdef in the function? At some point we are
>> hoping to get rid of all these #ifdefs.
>
> Do you think that this is the best way how to handle this?
> We are targeting small uboot running from OCM on Zynq. Moving ifdef to
> function itself will caused that we will waste some bytes which do nothing.
> Reference in table stays there and then return from that function too.
> It it is not huge but this way seems to me better.
>
> But no problem to change it if you think that this is the best way how
> to handle this.

You should not be using CONFIG_OF_EMBED...

I recall you were originally worried about the name change from
u-boot.bin to u-boot-dtb.bin. Is that right?

Regards,
Simon
Michal Simek Dec. 1, 2015, 3:12 p.m. UTC | #6 
On 1.12.2015 16:09, Simon Glass wrote:
> Hi Michal,
> 
> On 30 November 2015 at 23:56, Michal Simek <michal.simek@xilinx.com> wrote:
>> On 1.12.2015 00:17, Simon Glass wrote:
>>> Hi Michal,
>>>
>>> On 27 November 2015 at 00:28, Michal Simek <michal.simek@xilinx.com> wrote:
>>>> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>>>>
>>>> Don't relocate fdt in case of CONFIG_OF EMBED as the fdt is
>>>> already embedded with u-boot image. Also update the fdt_blob
>>>> correctly in this case
>>>>
>>>> Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>> Reviewed-by: Alexey Brodkin <abrodkin@synopsys.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Fix commit message reported by Alexey
>>>>
>>>>  common/board_f.c | 11 +++++++++--
>>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/common/board_f.c b/common/board_f.c
>>>> index b035c90ff3b7..91bf8beea1c6 100644
>>>> --- a/common/board_f.c
>>>> +++ b/common/board_f.c
>>>> @@ -508,7 +508,7 @@ static int reserve_global_data(void)
>>>>         return 0;
>>>>  }
>>>>
>>>> -static int reserve_fdt(void)
>>>> +static int __maybe_unused reserve_fdt(void)
>>>>  {
>>>>         /*
>>>>          * If the device tree is sitting immediately above our image then we
>>>> @@ -658,7 +658,7 @@ static int setup_dram_config(void)
>>>>         return 0;
>>>>  }
>>>>
>>>> -static int reloc_fdt(void)
>>>> +static int __maybe_unused reloc_fdt(void)
>>>>  {
>>>>         if (gd->flags & GD_FLG_SKIP_RELOC)
>>>>                 return 0;
>>>> @@ -687,6 +687,9 @@ static int setup_reloc(void)
>>>>         gd->reloc_off = gd->relocaddr - (CONFIG_SYS_TEXT_BASE + 0x400);
>>>>  #endif
>>>>  #endif
>>>> +#ifdef CONFIG_OF_EMBED
>>>> +       gd->fdt_blob += gd->reloc_off;
>>>> +#endif
>>>>         memcpy(gd->new_gd, (char *)gd, sizeof(gd_t));
>>>>
>>>>         debug("Relocation Offset is: %08lx\n", gd->reloc_off);
>>>> @@ -938,7 +941,9 @@ static init_fnc_t init_sequence_f[] = {
>>>>  #endif
>>>>         setup_machine,
>>>>         reserve_global_data,
>>>> +#ifndef CONFIG_OF_EMBED
>>>>         reserve_fdt,
>>>> +#endif
>>>
>>> Can you please put the #ifdef in the function? At some point we are
>>> hoping to get rid of all these #ifdefs.
>>
>> Do you think that this is the best way how to handle this?
>> We are targeting small uboot running from OCM on Zynq. Moving ifdef to
>> function itself will caused that we will waste some bytes which do nothing.
>> Reference in table stays there and then return from that function too.
>> It it is not huge but this way seems to me better.
>>
>> But no problem to change it if you think that this is the best way how
>> to handle this.
> 
> You should not be using CONFIG_OF_EMBED...

For production board.
Again is there really need to move to that ifdef inside reserve_fdt
function?

> I recall you were originally worried about the name change from
> u-boot.bin to u-boot-dtb.bin. Is that right?

No problem with this now.

Thanks,
Michal
Simon Glass Dec. 1, 2015, 8:02 p.m. UTC | #7 
Hi Michal,

On 1 December 2015 at 08:12, Michal Simek <michal.simek@xilinx.com> wrote:
> On 1.12.2015 16:09, Simon Glass wrote:
>> Hi Michal,
>>
>> On 30 November 2015 at 23:56, Michal Simek <michal.simek@xilinx.com> wrote:
>>> On 1.12.2015 00:17, Simon Glass wrote:
>>>> Hi Michal,
>>>>
>>>> On 27 November 2015 at 00:28, Michal Simek <michal.simek@xilinx.com> wrote:
>>>>> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>>>>>
>>>>> Don't relocate fdt in case of CONFIG_OF EMBED as the fdt is
>>>>> already embedded with u-boot image. Also update the fdt_blob
>>>>> correctly in this case
>>>>>
>>>>> Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>> Reviewed-by: Alexey Brodkin <abrodkin@synopsys.com>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - Fix commit message reported by Alexey
>>>>>
>>>>>  common/board_f.c | 11 +++++++++--
>>>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/common/board_f.c b/common/board_f.c
>>>>> index b035c90ff3b7..91bf8beea1c6 100644
>>>>> --- a/common/board_f.c
>>>>> +++ b/common/board_f.c
>>>>> @@ -508,7 +508,7 @@ static int reserve_global_data(void)
>>>>>         return 0;
>>>>>  }
>>>>>
>>>>> -static int reserve_fdt(void)
>>>>> +static int __maybe_unused reserve_fdt(void)
>>>>>  {
>>>>>         /*
>>>>>          * If the device tree is sitting immediately above our image then we
>>>>> @@ -658,7 +658,7 @@ static int setup_dram_config(void)
>>>>>         return 0;
>>>>>  }
>>>>>
>>>>> -static int reloc_fdt(void)
>>>>> +static int __maybe_unused reloc_fdt(void)
>>>>>  {
>>>>>         if (gd->flags & GD_FLG_SKIP_RELOC)
>>>>>                 return 0;
>>>>> @@ -687,6 +687,9 @@ static int setup_reloc(void)
>>>>>         gd->reloc_off = gd->relocaddr - (CONFIG_SYS_TEXT_BASE + 0x400);
>>>>>  #endif
>>>>>  #endif
>>>>> +#ifdef CONFIG_OF_EMBED
>>>>> +       gd->fdt_blob += gd->reloc_off;
>>>>> +#endif
>>>>>         memcpy(gd->new_gd, (char *)gd, sizeof(gd_t));
>>>>>
>>>>>         debug("Relocation Offset is: %08lx\n", gd->reloc_off);
>>>>> @@ -938,7 +941,9 @@ static init_fnc_t init_sequence_f[] = {
>>>>>  #endif
>>>>>         setup_machine,
>>>>>         reserve_global_data,
>>>>> +#ifndef CONFIG_OF_EMBED
>>>>>         reserve_fdt,
>>>>> +#endif
>>>>
>>>> Can you please put the #ifdef in the function? At some point we are
>>>> hoping to get rid of all these #ifdefs.
>>>
>>> Do you think that this is the best way how to handle this?
>>> We are targeting small uboot running from OCM on Zynq. Moving ifdef to
>>> function itself will caused that we will waste some bytes which do nothing.
>>> Reference in table stays there and then return from that function too.
>>> It it is not huge but this way seems to me better.
>>>
>>> But no problem to change it if you think that this is the best way how
>>> to handle this.
>>
>> You should not be using CONFIG_OF_EMBED...
>
> For production board.
> Again is there really need to move to that ifdef inside reserve_fdt
> function?

Well if we are trying to remove the #ifdefs from the list, but there
is a patch that adds one, that is going in the wrong direction.

>
>> I recall you were originally worried about the name change from
>> u-boot.bin to u-boot-dtb.bin. Is that right?
>
> No problem with this now.

OK, so I don't really need a need to save a few bytes on code that is
only ever used for debugging/testing.

Regards,
Simon
diff mbox

Patch

diff --git a/common/board_f.c b/common/board_f.c
index b035c90ff3b7..91bf8beea1c6 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -508,7 +508,7 @@  static int reserve_global_data(void)
 	return 0;
 }
 
-static int reserve_fdt(void)
+static int __maybe_unused reserve_fdt(void)
 {
 	/*
 	 * If the device tree is sitting immediately above our image then we
@@ -658,7 +658,7 @@  static int setup_dram_config(void)
 	return 0;
 }
 
-static int reloc_fdt(void)
+static int __maybe_unused reloc_fdt(void)
 {
 	if (gd->flags & GD_FLG_SKIP_RELOC)
 		return 0;
@@ -687,6 +687,9 @@  static int setup_reloc(void)
 	gd->reloc_off = gd->relocaddr - (CONFIG_SYS_TEXT_BASE + 0x400);
 #endif
 #endif
+#ifdef CONFIG_OF_EMBED
+	gd->fdt_blob += gd->reloc_off;
+#endif
 	memcpy(gd->new_gd, (char *)gd, sizeof(gd_t));
 
 	debug("Relocation Offset is: %08lx\n", gd->reloc_off);
@@ -938,7 +941,9 @@  static init_fnc_t init_sequence_f[] = {
 #endif
 	setup_machine,
 	reserve_global_data,
+#ifndef CONFIG_OF_EMBED
 	reserve_fdt,
+#endif
 	reserve_arch,
 	reserve_stacks,
 	setup_dram_config,
@@ -955,7 +960,9 @@  static init_fnc_t init_sequence_f[] = {
 	setup_board_extra,
 #endif
 	INIT_FUNC_WATCHDOG_RESET
+#ifndef CONFIG_OF_EMBED
 	reloc_fdt,
+#endif
 	setup_reloc,
 #if defined(CONFIG_X86) || defined(CONFIG_ARC)
 	copy_uboot_to_ram,