[PATCH] arm64: zynqmp: Do not describe u-boot.itb if SPL is disabled

[PATCH] arm64: zynqmp: Do not describe u-boot.itb if SPL is disabled

[Michal Simek]

There is no reason to describe u-boot.itb on system without SPL. Pretty
much this is cover all systems which are using only boot.bin which contains
all images inside.

Signed-off-by: Michal Simek <michal.simek@amd.com>
---

 board/xilinx/common/board.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
index 9641ed307b75..4f38b7d27684 100644
--- a/board/xilinx/common/board.c
+++ b/board/xilinx/common/board.c
@@ -43,7 +43,7 @@ struct efi_fw_image fw_images[] = {
 		.image_index = 1,
 	},
 #endif
-#if defined(XILINX_UBOOT_IMAGE_GUID)
+#if defined(XILINX_UBOOT_IMAGE_GUID) && defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)
 	{
 		.image_type_id = XILINX_UBOOT_IMAGE_GUID,
 		.fw_name = u"XILINX-UBOOT",
-- 
2.36.1

Re: [PATCH] arm64: zynqmp: Do not describe u-boot.itb if SPL is disabled

[Ilias Apalodimas]

On Fri, Feb 23, 2024 at 05:18:42PM +0100, Michal Simek wrote:
> There is no reason to describe u-boot.itb on system without SPL. Pretty
> much this is cover all systems which are using only boot.bin which contains
> all images inside.
>
> Signed-off-by: Michal Simek <michal.simek@amd.com>
> ---
>
>  board/xilinx/common/board.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
> index 9641ed307b75..4f38b7d27684 100644
> --- a/board/xilinx/common/board.c
> +++ b/board/xilinx/common/board.c
> @@ -43,7 +43,7 @@ struct efi_fw_image fw_images[] = {
>  		.image_index = 1,
>  	},
>  #endif
> -#if defined(XILINX_UBOOT_IMAGE_GUID)
> +#if defined(XILINX_UBOOT_IMAGE_GUID) && defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)

What happens if this is defined with CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="" ?

Cheers
/Ilias
>  	{
>  		.image_type_id = XILINX_UBOOT_IMAGE_GUID,
>  		.fw_name = u"XILINX-UBOOT",
> --
> 2.36.1
>

Re: [PATCH] arm64: zynqmp: Do not describe u-boot.itb if SPL is disabled

[Michal Simek]

On 3/5/24 16:47, Ilias Apalodimas wrote:
> On Fri, Feb 23, 2024 at 05:18:42PM +0100, Michal Simek wrote:
>> There is no reason to describe u-boot.itb on system without SPL. Pretty
>> much this is cover all systems which are using only boot.bin which contains
>> all images inside.
>>
>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>> ---
>>
>>   board/xilinx/common/board.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
>> index 9641ed307b75..4f38b7d27684 100644
>> --- a/board/xilinx/common/board.c
>> +++ b/board/xilinx/common/board.c
>> @@ -43,7 +43,7 @@ struct efi_fw_image fw_images[] = {
>>   		.image_index = 1,
>>   	},
>>   #endif
>> -#if defined(XILINX_UBOOT_IMAGE_GUID)
>> +#if defined(XILINX_UBOOT_IMAGE_GUID) && defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)
> 
> What happens if this is defined with CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="" ?

Your comment is valid but I am not aware about any CONFIG_IS, etc which checks 
that string is not empty. If name is "" it will return yes and second image is 
doing to be defined.

But I found handling in the code like this.

  36 #ifdef CONFIG_DEFAULT_FDT_FILE
  37                 if (strlen(CONFIG_DEFAULT_FDT_FILE)) {

which can be used in my second patch not to describe second image in
set_dfu_alt_info() if string is empty.

Thanks,
Michal

Re: [PATCH] arm64: zynqmp: Do not describe u-boot.itb if SPL is disabled

[Ilias Apalodimas]

Hi Michal

Apologies for the late reply

On Wed, 6 Mar 2024 at 09:48, Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 3/5/24 16:47, Ilias Apalodimas wrote:
> > On Fri, Feb 23, 2024 at 05:18:42PM +0100, Michal Simek wrote:
> >> There is no reason to describe u-boot.itb on system without SPL. Pretty
> >> much this is cover all systems which are using only boot.bin which contains
> >> all images inside.
> >>
> >> Signed-off-by: Michal Simek <michal.simek@amd.com>
> >> ---
> >>
> >>   board/xilinx/common/board.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
> >> index 9641ed307b75..4f38b7d27684 100644
> >> --- a/board/xilinx/common/board.c
> >> +++ b/board/xilinx/common/board.c
> >> @@ -43,7 +43,7 @@ struct efi_fw_image fw_images[] = {
> >>              .image_index = 1,
> >>      },
> >>   #endif
> >> -#if defined(XILINX_UBOOT_IMAGE_GUID)
> >> +#if defined(XILINX_UBOOT_IMAGE_GUID) && defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)
> >
> > What happens if this is defined with CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="" ?
>
> Your comment is valid but I am not aware about any CONFIG_IS, etc which checks
> that string is not empty. If name is "" it will return yes and second image is
> doing to be defined.
>
> But I found handling in the code like this.
>
>   36 #ifdef CONFIG_DEFAULT_FDT_FILE
>   37                 if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
>
> which can be used in my second patch not to describe second image in
> set_dfu_alt_info() if string is empty.

Yes, I think that's ok. The problem is that if we merge this as-is, we
would have to disable CONFIG_SPL_FS_FAT to make this work, which is a
bit misleading

Cheers
/Ilias
>
> Thanks,
> Michal

Re: [PATCH] arm64: zynqmp: Do not describe u-boot.itb if SPL is disabled

[Michal Simek]

On 3/12/24 07:14, Ilias Apalodimas wrote:
> Hi Michal
> 
> Apologies for the late reply
> 
> On Wed, 6 Mar 2024 at 09:48, Michal Simek <michal.simek@amd.com> wrote:
>>
>>
>>
>> On 3/5/24 16:47, Ilias Apalodimas wrote:
>>> On Fri, Feb 23, 2024 at 05:18:42PM +0100, Michal Simek wrote:
>>>> There is no reason to describe u-boot.itb on system without SPL. Pretty
>>>> much this is cover all systems which are using only boot.bin which contains
>>>> all images inside.
>>>>
>>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>>>> ---
>>>>
>>>>    board/xilinx/common/board.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
>>>> index 9641ed307b75..4f38b7d27684 100644
>>>> --- a/board/xilinx/common/board.c
>>>> +++ b/board/xilinx/common/board.c
>>>> @@ -43,7 +43,7 @@ struct efi_fw_image fw_images[] = {
>>>>               .image_index = 1,
>>>>       },
>>>>    #endif
>>>> -#if defined(XILINX_UBOOT_IMAGE_GUID)
>>>> +#if defined(XILINX_UBOOT_IMAGE_GUID) && defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)
>>>
>>> What happens if this is defined with CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="" ?
>>
>> Your comment is valid but I am not aware about any CONFIG_IS, etc which checks
>> that string is not empty. If name is "" it will return yes and second image is
>> doing to be defined.
>>
>> But I found handling in the code like this.
>>
>>    36 #ifdef CONFIG_DEFAULT_FDT_FILE
>>    37                 if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
>>
>> which can be used in my second patch not to describe second image in
>> set_dfu_alt_info() if string is empty.
> 
> Yes, I think that's ok. The problem is that if we merge this as-is, we
> would have to disable CONFIG_SPL_FS_FAT to make this work, which is a
> bit misleading

As Heinrich said not just this if you want to do it like this.
I think you will simply disable the whole SPL which will disable this symbol too.
But from my perspective SPL payload name is driving this option. Data can end up 
on partition or in raw mode but for dfu you need to use the name.

Thanks,
Michal

Re: [PATCH] arm64: zynqmp: Do not describe u-boot.itb if SPL is disabled

[Ilias Apalodimas]

On Tue, 12 Mar 2024 at 17:55, Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 3/12/24 07:14, Ilias Apalodimas wrote:
> > Hi Michal
> >
> > Apologies for the late reply
> >
> > On Wed, 6 Mar 2024 at 09:48, Michal Simek <michal.simek@amd.com> wrote:
> >>
> >>
> >>
> >> On 3/5/24 16:47, Ilias Apalodimas wrote:
> >>> On Fri, Feb 23, 2024 at 05:18:42PM +0100, Michal Simek wrote:
> >>>> There is no reason to describe u-boot.itb on system without SPL. Pretty
> >>>> much this is cover all systems which are using only boot.bin which contains
> >>>> all images inside.
> >>>>
> >>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
> >>>> ---
> >>>>
> >>>>    board/xilinx/common/board.c | 2 +-
> >>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
> >>>> index 9641ed307b75..4f38b7d27684 100644
> >>>> --- a/board/xilinx/common/board.c
> >>>> +++ b/board/xilinx/common/board.c
> >>>> @@ -43,7 +43,7 @@ struct efi_fw_image fw_images[] = {
> >>>>               .image_index = 1,
> >>>>       },
> >>>>    #endif
> >>>> -#if defined(XILINX_UBOOT_IMAGE_GUID)
> >>>> +#if defined(XILINX_UBOOT_IMAGE_GUID) && defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)
> >>>
> >>> What happens if this is defined with CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="" ?
> >>
> >> Your comment is valid but I am not aware about any CONFIG_IS, etc which checks
> >> that string is not empty. If name is "" it will return yes and second image is
> >> doing to be defined.
> >>
> >> But I found handling in the code like this.
> >>
> >>    36 #ifdef CONFIG_DEFAULT_FDT_FILE
> >>    37                 if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
> >>
> >> which can be used in my second patch not to describe second image in
> >> set_dfu_alt_info() if string is empty.
> >
> > Yes, I think that's ok. The problem is that if we merge this as-is, we
> > would have to disable CONFIG_SPL_FS_FAT to make this work, which is a
> > bit misleading
>
> As Heinrich said not just this if you want to do it like this.
> I think you will simply disable the whole SPL which will disable this symbol too.
> But from my perspective SPL payload name is driving this option. Data can end up
> on partition or in raw mode but for dfu you need to use the name.

Yes, but isn't SPL selected by the Kconfig automatically? I can't seem
to be able to disable it for the kria platforms

Thanks
/Ilias
>
> Thanks,
> Michal
>
>

Re: [PATCH] arm64: zynqmp: Do not describe u-boot.itb if SPL is disabled

[Michal Simek]

On 3/12/24 20:12, Ilias Apalodimas wrote:
> On Tue, 12 Mar 2024 at 17:55, Michal Simek <michal.simek@amd.com> wrote:
>>
>>
>>
>> On 3/12/24 07:14, Ilias Apalodimas wrote:
>>> Hi Michal
>>>
>>> Apologies for the late reply
>>>
>>> On Wed, 6 Mar 2024 at 09:48, Michal Simek <michal.simek@amd.com> wrote:
>>>>
>>>>
>>>>
>>>> On 3/5/24 16:47, Ilias Apalodimas wrote:
>>>>> On Fri, Feb 23, 2024 at 05:18:42PM +0100, Michal Simek wrote:
>>>>>> There is no reason to describe u-boot.itb on system without SPL. Pretty
>>>>>> much this is cover all systems which are using only boot.bin which contains
>>>>>> all images inside.
>>>>>>
>>>>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>>>>>> ---
>>>>>>
>>>>>>     board/xilinx/common/board.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
>>>>>> index 9641ed307b75..4f38b7d27684 100644
>>>>>> --- a/board/xilinx/common/board.c
>>>>>> +++ b/board/xilinx/common/board.c
>>>>>> @@ -43,7 +43,7 @@ struct efi_fw_image fw_images[] = {
>>>>>>                .image_index = 1,
>>>>>>        },
>>>>>>     #endif
>>>>>> -#if defined(XILINX_UBOOT_IMAGE_GUID)
>>>>>> +#if defined(XILINX_UBOOT_IMAGE_GUID) && defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)
>>>>>
>>>>> What happens if this is defined with CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="" ?
>>>>
>>>> Your comment is valid but I am not aware about any CONFIG_IS, etc which checks
>>>> that string is not empty. If name is "" it will return yes and second image is
>>>> doing to be defined.
>>>>
>>>> But I found handling in the code like this.
>>>>
>>>>     36 #ifdef CONFIG_DEFAULT_FDT_FILE
>>>>     37                 if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
>>>>
>>>> which can be used in my second patch not to describe second image in
>>>> set_dfu_alt_info() if string is empty.
>>>
>>> Yes, I think that's ok. The problem is that if we merge this as-is, we
>>> would have to disable CONFIG_SPL_FS_FAT to make this work, which is a
>>> bit misleading
>>
>> As Heinrich said not just this if you want to do it like this.
>> I think you will simply disable the whole SPL which will disable this symbol too.
>> But from my perspective SPL payload name is driving this option. Data can end up
>> on partition or in raw mode but for dfu you need to use the name.
> 
> Yes, but isn't SPL selected by the Kconfig automatically? I can't seem
> to be able to disable it for the kria platforms

Not in upstream but via your/AMD build in meta-ts.

Thanks,
Michal

Re: [PATCH] arm64: zynqmp: Do not describe u-boot.itb if SPL is disabled

[Ilias Apalodimas]

On Wed, 13 Mar 2024 at 08:42, Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 3/12/24 20:12, Ilias Apalodimas wrote:
> > On Tue, 12 Mar 2024 at 17:55, Michal Simek <michal.simek@amd.com> wrote:
> >>
> >>
> >>
> >> On 3/12/24 07:14, Ilias Apalodimas wrote:
> >>> Hi Michal
> >>>
> >>> Apologies for the late reply
> >>>
> >>> On Wed, 6 Mar 2024 at 09:48, Michal Simek <michal.simek@amd.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 3/5/24 16:47, Ilias Apalodimas wrote:
> >>>>> On Fri, Feb 23, 2024 at 05:18:42PM +0100, Michal Simek wrote:
> >>>>>> There is no reason to describe u-boot.itb on system without SPL. Pretty
> >>>>>> much this is cover all systems which are using only boot.bin which contains
> >>>>>> all images inside.
> >>>>>>
> >>>>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
> >>>>>> ---
> >>>>>>
> >>>>>>     board/xilinx/common/board.c | 2 +-
> >>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
> >>>>>> index 9641ed307b75..4f38b7d27684 100644
> >>>>>> --- a/board/xilinx/common/board.c
> >>>>>> +++ b/board/xilinx/common/board.c
> >>>>>> @@ -43,7 +43,7 @@ struct efi_fw_image fw_images[] = {
> >>>>>>                .image_index = 1,
> >>>>>>        },
> >>>>>>     #endif
> >>>>>> -#if defined(XILINX_UBOOT_IMAGE_GUID)
> >>>>>> +#if defined(XILINX_UBOOT_IMAGE_GUID) && defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)
> >>>>>
> >>>>> What happens if this is defined with CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="" ?
> >>>>
> >>>> Your comment is valid but I am not aware about any CONFIG_IS, etc which checks
> >>>> that string is not empty. If name is "" it will return yes and second image is
> >>>> doing to be defined.
> >>>>
> >>>> But I found handling in the code like this.
> >>>>
> >>>>     36 #ifdef CONFIG_DEFAULT_FDT_FILE
> >>>>     37                 if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
> >>>>
> >>>> which can be used in my second patch not to describe second image in
> >>>> set_dfu_alt_info() if string is empty.
> >>>
> >>> Yes, I think that's ok. The problem is that if we merge this as-is, we
> >>> would have to disable CONFIG_SPL_FS_FAT to make this work, which is a
> >>> bit misleading
> >>
> >> As Heinrich said not just this if you want to do it like this.
> >> I think you will simply disable the whole SPL which will disable this symbol too.
> >> But from my perspective SPL payload name is driving this option. Data can end up
> >> on partition or in raw mode but for dfu you need to use the name.
> >
> > Yes, but isn't SPL selected by the Kconfig automatically? I can't seem
> > to be able to disable it for the kria platforms
>
> Not in upstream but via your/AMD build in meta-ts.
>
> Thanks,
> Michal
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH] arm64: zynqmp: Do not describe u-boot.itb if SPL is disabled

[Ilias Apalodimas]

Hi Michal

On Wed, 13 Mar 2024 at 09:01, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Wed, 13 Mar 2024 at 08:42, Michal Simek <michal.simek@amd.com> wrote:
> >
> >
> >
> > On 3/12/24 20:12, Ilias Apalodimas wrote:
> > > On Tue, 12 Mar 2024 at 17:55, Michal Simek <michal.simek@amd.com> wrote:
> > >>
> > >>
> > >>
> > >> On 3/12/24 07:14, Ilias Apalodimas wrote:
> > >>> Hi Michal
> > >>>
> > >>> Apologies for the late reply
> > >>>
> > >>> On Wed, 6 Mar 2024 at 09:48, Michal Simek <michal.simek@amd.com> wrote:
> > >>>>
> > >>>>
> > >>>>
> > >>>> On 3/5/24 16:47, Ilias Apalodimas wrote:
> > >>>>> On Fri, Feb 23, 2024 at 05:18:42PM +0100, Michal Simek wrote:
> > >>>>>> There is no reason to describe u-boot.itb on system without SPL. Pretty
> > >>>>>> much this is cover all systems which are using only boot.bin which contains
> > >>>>>> all images inside.
> > >>>>>>
> > >>>>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
> > >>>>>> ---
> > >>>>>>
> > >>>>>>     board/xilinx/common/board.c | 2 +-
> > >>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>>>>
> > >>>>>> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
> > >>>>>> index 9641ed307b75..4f38b7d27684 100644
> > >>>>>> --- a/board/xilinx/common/board.c
> > >>>>>> +++ b/board/xilinx/common/board.c
> > >>>>>> @@ -43,7 +43,7 @@ struct efi_fw_image fw_images[] = {
> > >>>>>>                .image_index = 1,
> > >>>>>>        },
> > >>>>>>     #endif
> > >>>>>> -#if defined(XILINX_UBOOT_IMAGE_GUID)
> > >>>>>> +#if defined(XILINX_UBOOT_IMAGE_GUID) && defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)
> > >>>>>
> > >>>>> What happens if this is defined with CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="" ?
> > >>>>
> > >>>> Your comment is valid but I am not aware about any CONFIG_IS, etc which checks
> > >>>> that string is not empty. If name is "" it will return yes and second image is
> > >>>> doing to be defined.
> > >>>>
> > >>>> But I found handling in the code like this.
> > >>>>
> > >>>>     36 #ifdef CONFIG_DEFAULT_FDT_FILE
> > >>>>     37                 if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
> > >>>>
> > >>>> which can be used in my second patch not to describe second image in
> > >>>> set_dfu_alt_info() if string is empty.
> > >>>
> > >>> Yes, I think that's ok. The problem is that if we merge this as-is, we
> > >>> would have to disable CONFIG_SPL_FS_FAT to make this work, which is a
> > >>> bit misleading
> > >>
> > >> As Heinrich said not just this if you want to do it like this.
> > >> I think you will simply disable the whole SPL which will disable this symbol too.
> > >> But from my perspective SPL payload name is driving this option. Data can end up
> > >> on partition or in raw mode but for dfu you need to use the name.
> > >
> > > Yes, but isn't SPL selected by the Kconfig automatically? I can't seem
> > > to be able to disable it for the kria platforms
> >
> > Not in upstream but via your/AMD build in meta-ts.
> >
> > Thanks,
> > Michal
> >
>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Trying to compile xilinx_zynqmp_kria_defconfig with CONFIG_SPL unset
blows up with

 HOSTCC  scripts/dtc/dtc-lexer.lex.o
  HOSTCC  scripts/dtc/dtc-parser.tab.o
  COPY    u-boot.its
cp: missing destination file operand after 'u-boot.its'
Try 'cp --help' for more information.
make: *** [Makefile:1405: u-boot.its] Error 1
make: *** Waiting for unfinished jobs....
  HOSTLD  scripts/dtc/dtc

Cheers
/Ilias

Re: [PATCH] arm64: zynqmp: Do not describe u-boot.itb if SPL is disabled

[Michal Simek]

Hi,

On 3/14/24 09:34, Ilias Apalodimas wrote:
> Hi Michal
> 
> On Wed, 13 Mar 2024 at 09:01, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
>>
>> On Wed, 13 Mar 2024 at 08:42, Michal Simek <michal.simek@amd.com> wrote:
>>>
>>>
>>>
>>> On 3/12/24 20:12, Ilias Apalodimas wrote:
>>>> On Tue, 12 Mar 2024 at 17:55, Michal Simek <michal.simek@amd.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 3/12/24 07:14, Ilias Apalodimas wrote:
>>>>>> Hi Michal
>>>>>>
>>>>>> Apologies for the late reply
>>>>>>
>>>>>> On Wed, 6 Mar 2024 at 09:48, Michal Simek <michal.simek@amd.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 3/5/24 16:47, Ilias Apalodimas wrote:
>>>>>>>> On Fri, Feb 23, 2024 at 05:18:42PM +0100, Michal Simek wrote:
>>>>>>>>> There is no reason to describe u-boot.itb on system without SPL. Pretty
>>>>>>>>> much this is cover all systems which are using only boot.bin which contains
>>>>>>>>> all images inside.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>      board/xilinx/common/board.c | 2 +-
>>>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
>>>>>>>>> index 9641ed307b75..4f38b7d27684 100644
>>>>>>>>> --- a/board/xilinx/common/board.c
>>>>>>>>> +++ b/board/xilinx/common/board.c
>>>>>>>>> @@ -43,7 +43,7 @@ struct efi_fw_image fw_images[] = {
>>>>>>>>>                 .image_index = 1,
>>>>>>>>>         },
>>>>>>>>>      #endif
>>>>>>>>> -#if defined(XILINX_UBOOT_IMAGE_GUID)
>>>>>>>>> +#if defined(XILINX_UBOOT_IMAGE_GUID) && defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)
>>>>>>>>
>>>>>>>> What happens if this is defined with CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="" ?
>>>>>>>
>>>>>>> Your comment is valid but I am not aware about any CONFIG_IS, etc which checks
>>>>>>> that string is not empty. If name is "" it will return yes and second image is
>>>>>>> doing to be defined.
>>>>>>>
>>>>>>> But I found handling in the code like this.
>>>>>>>
>>>>>>>      36 #ifdef CONFIG_DEFAULT_FDT_FILE
>>>>>>>      37                 if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
>>>>>>>
>>>>>>> which can be used in my second patch not to describe second image in
>>>>>>> set_dfu_alt_info() if string is empty.
>>>>>>
>>>>>> Yes, I think that's ok. The problem is that if we merge this as-is, we
>>>>>> would have to disable CONFIG_SPL_FS_FAT to make this work, which is a
>>>>>> bit misleading
>>>>>
>>>>> As Heinrich said not just this if you want to do it like this.
>>>>> I think you will simply disable the whole SPL which will disable this symbol too.
>>>>> But from my perspective SPL payload name is driving this option. Data can end up
>>>>> on partition or in raw mode but for dfu you need to use the name.
>>>>
>>>> Yes, but isn't SPL selected by the Kconfig automatically? I can't seem
>>>> to be able to disable it for the kria platforms
>>>
>>> Not in upstream but via your/AMD build in meta-ts.
>>>
>>> Thanks,
>>> Michal
>>>
>>
>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> 
> Trying to compile xilinx_zynqmp_kria_defconfig with CONFIG_SPL unset
> blows up with
> 
>   HOSTCC  scripts/dtc/dtc-lexer.lex.o
>    HOSTCC  scripts/dtc/dtc-parser.tab.o
>    COPY    u-boot.its
> cp: missing destination file operand after 'u-boot.its'
> Try 'cp --help' for more information.
> make: *** [Makefile:1405: u-boot.its] Error 1
> make: *** Waiting for unfinished jobs....
>    HOSTLD  scripts/dtc/dtc

That's because u-boot.itb is selected in .config as target binary.
Because that entry is string, setup by default when SPL is enabled via 
defconfig. Then when you disable SPL via defconfig default setting is not 
changed and is still at origin value.
CONFIG_BUILD_TARGET="u-boot.itb"

If you do sed -i '/CONFIG_SPL/d' configs/xilinx_zynqmp_kria_defconfig
you will get that CONFIG_BUILD_TARGET=""
which is correct value without SPL.

Thanks,
Michal

Re: [PATCH] arm64: zynqmp: Do not describe u-boot.itb if SPL is disabled

[Heinrich Schuchardt]

On 05.03.24 16:47, Ilias Apalodimas wrote:
> On Fri, Feb 23, 2024 at 05:18:42PM +0100, Michal Simek wrote:
>> There is no reason to describe u-boot.itb on system without SPL. Pretty
>> much this is cover all systems which are using only boot.bin which contains
>> all images inside.
>>
>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>> ---
>>
>>   board/xilinx/common/board.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
>> index 9641ed307b75..4f38b7d27684 100644
>> --- a/board/xilinx/common/board.c
>> +++ b/board/xilinx/common/board.c
>> @@ -43,7 +43,7 @@ struct efi_fw_image fw_images[] = {
>>   		.image_index = 1,
>>   	},
>>   #endif
>> -#if defined(XILINX_UBOOT_IMAGE_GUID)
>> +#if defined(XILINX_UBOOT_IMAGE_GUID) && defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)
>
> What happens if this is defined with CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="" ?

CONFIG_SPL_FS_LOAD_PAYLOAD_NAME depends on SPL_FS_EXT4 || SPL_FS_FAT ||
SPL_FS_SQUASHFS || SPL_SEMIHOSTING. So it is only defined if SPL could
load a file.

CONFIG_SPL_FS_LOAD_PAYLOAD_NAME defaults to a non-blank name. If a user
provides an invalid name, SPL will not be able to load the file.

What is wrong here is to assume that *.itb has to be load as a file. We
can configure U-Boot SPL to load the itb from a raw partition.

The check might be too restrictive.

Best regards

Heinrich

>
> Cheers
> /Ilias
>>   	{
>>   		.image_type_id = XILINX_UBOOT_IMAGE_GUID,
>>   		.fw_name = u"XILINX-UBOOT",
>> --
>> 2.36.1
>>

Re: [PATCH] arm64: zynqmp: Do not describe u-boot.itb if SPL is disabled

[Michal Simek]

On 3/12/24 13:29, Heinrich Schuchardt wrote:
> On 05.03.24 16:47, Ilias Apalodimas wrote:
>> On Fri, Feb 23, 2024 at 05:18:42PM +0100, Michal Simek wrote:
>>> There is no reason to describe u-boot.itb on system without SPL. Pretty
>>> much this is cover all systems which are using only boot.bin which contains
>>> all images inside.
>>>
>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>>> ---
>>>
>>>   board/xilinx/common/board.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
>>> index 9641ed307b75..4f38b7d27684 100644
>>> --- a/board/xilinx/common/board.c
>>> +++ b/board/xilinx/common/board.c
>>> @@ -43,7 +43,7 @@ struct efi_fw_image fw_images[] = {
>>>           .image_index = 1,
>>>       },
>>>   #endif
>>> -#if defined(XILINX_UBOOT_IMAGE_GUID)
>>> +#if defined(XILINX_UBOOT_IMAGE_GUID) && 
>>> defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)
>>
>> What happens if this is defined with CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="" ?
> 
> CONFIG_SPL_FS_LOAD_PAYLOAD_NAME depends on SPL_FS_EXT4 || SPL_FS_FAT ||
> SPL_FS_SQUASHFS || SPL_SEMIHOSTING. So it is only defined if SPL could
> load a file.
> 
> CONFIG_SPL_FS_LOAD_PAYLOAD_NAME defaults to a non-blank name. If a user
> provides an invalid name, SPL will not be able to load the file.
> 
> What is wrong here is to assume that *.itb has to be load as a file. We
> can configure U-Boot SPL to load the itb from a raw partition.
> 
> The check might be too restrictive.

I am not sure I fully follow what you are saying here.
All current dfu rules via set_dfu_alt_info() (board/xilinx/zynqmp/zynqmp.c) are 
using CONFIG_SPL_FS_LOAD_PAYLOAD_NAME for second location description.
Obviously in qspi boot mode it is description for RAW. In sd one it is 
description with using fat.
Description for two guids make only sense in connection to using U-Boot SPL 
only. Because if SPL is not used likely you are going to use standard Xilinx 
solution which is pretty much all in one inside boot.bin which is described 
already.
This patch is just trying to get rid of description for second firmware if non 
SPL bootflow is used because pointer is likely not correct.

Thanks,
Michal

Re: [PATCH] arm64: zynqmp: Do not describe u-boot.itb if SPL is disabled

[Michal Simek]

On 2/23/24 17:18, Michal Simek wrote:
> There is no reason to describe u-boot.itb on system without SPL. Pretty
> much this is cover all systems which are using only boot.bin which contains
> all images inside.
> 
> Signed-off-by: Michal Simek <michal.simek@amd.com>
> ---
> 
>   board/xilinx/common/board.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
> index 9641ed307b75..4f38b7d27684 100644
> --- a/board/xilinx/common/board.c
> +++ b/board/xilinx/common/board.c
> @@ -43,7 +43,7 @@ struct efi_fw_image fw_images[] = {
>   		.image_index = 1,
>   	},
>   #endif
> -#if defined(XILINX_UBOOT_IMAGE_GUID)
> +#if defined(XILINX_UBOOT_IMAGE_GUID) && defined(CONFIG_SPL_FS_LOAD_PAYLOAD_NAME)
>   	{
>   		.image_type_id = XILINX_UBOOT_IMAGE_GUID,
>   		.fw_name = u"XILINX-UBOOT",

Applied.
M