[PATCH] Revert "rockchip: Only call binman when TPL available"

[PATCH] Revert "rockchip: Only call binman when TPL available"

[Jagan Teki]

This reverts commit f5315dd6290a588434e4f79bfd2886bb7df9210d.

[why]
TPL is not mandatory for not all Rockchip SoCs, some SoCs like
RK356x, and RK3588 still use mainline u-boot without TPL as
their ddr init programs are accessed via binaries provided by
Rockchip instead of ddr source code.

Marking TPL build makes it not able to build u-boot.itb on
RK356x targets so revert this so that it can build an SPL build
that would support all across Rockchip platforms.

Suggested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Signed-off-by: Jagan Teki <jagan@edgeble.ai>
---
 arch/arm/dts/rockchip-u-boot.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index 234fc5df43..6d1fd7769e 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -11,7 +11,7 @@
 	};
 };
 
-#ifdef CONFIG_TPL
+#ifdef CONFIG_SPL
 &binman {
 	simple-bin {
 		filename = "u-boot-rockchip.bin";
-- 
2.25.1

Re: [PATCH] Revert "rockchip: Only call binman when TPL available"

[Anand Moon]

Hi Jagan,

On Fri, 27 Jan 2023 at 12:52, Jagan Teki <jagan@edgeble.ai> wrote:
>
> This reverts commit f5315dd6290a588434e4f79bfd2886bb7df9210d.
>
> [why]
> TPL is not mandatory for not all Rockchip SoCs, some SoCs like
> RK356x, and RK3588 still use mainline u-boot without TPL as
> their ddr init programs are accessed via binaries provided by
> Rockchip instead of ddr source code.
>
> Marking TPL build makes it not able to build u-boot.itb on
> RK356x targets so revert this so that it can build an SPL build
> that would support all across Rockchip platforms.
>
> Suggested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Signed-off-by: Jagan Teki <jagan@edgeble.ai>
> ---

Please add my
Tested-by: Anand Moon <linux.amoon@gmail.com>  # CM3

Thanks

-Anand

Re: [PATCH] Revert "rockchip: Only call binman when TPL available"

[Jagan Teki]

On Fri, 27 Jan 2023 at 12:51, Jagan Teki <jagan@edgeble.ai> wrote:
>
> This reverts commit f5315dd6290a588434e4f79bfd2886bb7df9210d.
>
> [why]
> TPL is not mandatory for not all Rockchip SoCs, some SoCs like
> RK356x, and RK3588 still use mainline u-boot without TPL as
> their ddr init programs are accessed via binaries provided by
> Rockchip instead of ddr source code.
>
> Marking TPL build makes it not able to build u-boot.itb on
> RK356x targets so revert this so that it can build an SPL build
> that would support all across Rockchip platforms.
>
> Suggested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Signed-off-by: Jagan Teki <jagan@edgeble.ai>
> ---

Can someone apply this before -rc is released?

Thanks,
Jagan.

Re: [PATCH] Revert "rockchip: Only call binman when TPL available"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

On Mon, Jan 30, 2023 at 08:29:35PM +0530, Jagan Teki wrote:
> On Fri, 27 Jan 2023 at 12:51, Jagan Teki <jagan@edgeble.ai> wrote:
> >
> > This reverts commit f5315dd6290a588434e4f79bfd2886bb7df9210d.
> >
> > [why]
> > TPL is not mandatory for not all Rockchip SoCs, some SoCs like
> > RK356x, and RK3588 still use mainline u-boot without TPL as
> > their ddr init programs are accessed via binaries provided by
> > Rockchip instead of ddr source code.
> >
> > Marking TPL build makes it not able to build u-boot.itb on
> > RK356x targets so revert this so that it can build an SPL build
> > that would support all across Rockchip platforms.
> >
> > Suggested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > Signed-off-by: Jagan Teki <jagan@edgeble.ai>
> > ---
> 
> Can someone apply this before -rc is released?

Yes, but where's the follow-up series to make both cases work again, I
think I missed it?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "rockchip: Only call binman when TPL available"

[Jagan Teki]

On Mon, 30 Jan 2023 at 20:45, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Jan 30, 2023 at 08:29:35PM +0530, Jagan Teki wrote:
> > On Fri, 27 Jan 2023 at 12:51, Jagan Teki <jagan@edgeble.ai> wrote:
> > >
> > > This reverts commit f5315dd6290a588434e4f79bfd2886bb7df9210d.
> > >
> > > [why]
> > > TPL is not mandatory for not all Rockchip SoCs, some SoCs like
> > > RK356x, and RK3588 still use mainline u-boot without TPL as
> > > their ddr init programs are accessed via binaries provided by
> > > Rockchip instead of ddr source code.
> > >
> > > Marking TPL build makes it not able to build u-boot.itb on
> > > RK356x targets so revert this so that it can build an SPL build
> > > that would support all across Rockchip platforms.
> > >
> > > Suggested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > Signed-off-by: Jagan Teki <jagan@edgeble.ai>
> > > ---
> >
> > Can someone apply this before -rc is released?
>
> Yes, but where's the follow-up series to make both cases work again, I
> think I missed it?

No follow-up series from my side, I think this revert will compatible
with all rockchip platforms.

Jagan.

Re: [PATCH] Revert "rockchip: Only call binman when TPL available"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1555 bytes --]

On Tue, Jan 31, 2023 at 02:44:52AM +0530, Jagan Teki wrote:
> On Mon, 30 Jan 2023 at 20:45, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Jan 30, 2023 at 08:29:35PM +0530, Jagan Teki wrote:
> > > On Fri, 27 Jan 2023 at 12:51, Jagan Teki <jagan@edgeble.ai> wrote:
> > > >
> > > > This reverts commit f5315dd6290a588434e4f79bfd2886bb7df9210d.
> > > >
> > > > [why]
> > > > TPL is not mandatory for not all Rockchip SoCs, some SoCs like
> > > > RK356x, and RK3588 still use mainline u-boot without TPL as
> > > > their ddr init programs are accessed via binaries provided by
> > > > Rockchip instead of ddr source code.
> > > >
> > > > Marking TPL build makes it not able to build u-boot.itb on
> > > > RK356x targets so revert this so that it can build an SPL build
> > > > that would support all across Rockchip platforms.
> > > >
> > > > Suggested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > > Signed-off-by: Jagan Teki <jagan@edgeble.ai>
> > > > ---
> > >
> > > Can someone apply this before -rc is released?
> >
> > Yes, but where's the follow-up series to make both cases work again, I
> > think I missed it?
> 
> No follow-up series from my side, I think this revert will compatible
> with all rockchip platforms.

Well, I assumed there was some subset that needed this change or Kever
wouldn't have posted and merged his change so close to the release. And
I do think we need something with CONFIG_BUILD_TARGET so that this
problem would have been caught in CI, in the future.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "rockchip: Only call binman when TPL available"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 789 bytes --]

On Fri, Jan 27, 2023 at 12:51:33PM +0530, Jagan Teki wrote:

> This reverts commit f5315dd6290a588434e4f79bfd2886bb7df9210d.
> 
> [why]
> TPL is not mandatory for not all Rockchip SoCs, some SoCs like
> RK356x, and RK3588 still use mainline u-boot without TPL as
> their ddr init programs are accessed via binaries provided by
> Rockchip instead of ddr source code.
> 
> Marking TPL build makes it not able to build u-boot.itb on
> RK356x targets so revert this so that it can build an SPL build
> that would support all across Rockchip platforms.
> 
> Suggested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Signed-off-by: Jagan Teki <jagan@edgeble.ai>
> Tested-by: Anand Moon <linux.amoon@gmail.com>  # CM3

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "rockchip: Only call binman when TPL available"

[Kever Yang]

Hi

     I think I do this modify for those soc not have TPL, or else it 
will cause the CI build error.


TPL/ddr binary is mandatory in all rockchip SoCs now, but the mainline 
U-Boot may not

able to provide the the available TPL, will need ddr init binary from 
rockchip rkbin repository

instead.

So the policy is clear, the binary output on mainline U-Boot rockchip 
platform:

- TPL available: u-boot-rockchip.bin with TPL, SPL, ATF, U-Boot proper;

- TPL not available: u-boot.itb with ATF, U-Boot proper.

                                         idbloader.bin generate by the 
TPL(ddr init binary instead) and SPL via mkimage cmd;


Thanks,
- Kever
On 2023/1/27 15:21, Jagan Teki wrote:
> This reverts commit f5315dd6290a588434e4f79bfd2886bb7df9210d.
>
> [why]
> TPL is not mandatory for not all Rockchip SoCs, some SoCs like
> RK356x, and RK3588 still use mainline u-boot without TPL as
> their ddr init programs are accessed via binaries provided by
> Rockchip instead of ddr source code.
>
> Marking TPL build makes it not able to build u-boot.itb on
> RK356x targets so revert this so that it can build an SPL build
> that would support all across Rockchip platforms.
>
> Suggested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Signed-off-by: Jagan Teki <jagan@edgeble.ai>
> ---
>   arch/arm/dts/rockchip-u-boot.dtsi | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index 234fc5df43..6d1fd7769e 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -11,7 +11,7 @@
>   	};
>   };
>   
> -#ifdef CONFIG_TPL
> +#ifdef CONFIG_SPL
>   &binman {
>   	simple-bin {
>   		filename = "u-boot-rockchip.bin";

Re: [PATCH] Revert "rockchip: Only call binman when TPL available"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2456 bytes --]

On Tue, Jan 31, 2023 at 10:53:38AM +0800, Kever Yang wrote:
> Hi
> 
>     I think I do this modify for those soc not have TPL, or else it will
> cause the CI build error.
> 
> 
> TPL/ddr binary is mandatory in all rockchip SoCs now, but the mainline
> U-Boot may not
> 
> able to provide the the available TPL, will need ddr init binary from
> rockchip rkbin repository
> 
> instead.
> 
> So the policy is clear, the binary output on mainline U-Boot rockchip
> platform:
> 
> - TPL available: u-boot-rockchip.bin with TPL, SPL, ATF, U-Boot proper;
> 
> - TPL not available: u-boot.itb with ATF, U-Boot proper.
> 
>                                         idbloader.bin generate by the
> TPL(ddr init binary instead) and SPL via mkimage cmd;

The thing is, this sounds an awful lot like the challenges that are
presented by imx8*, and sunxi and others now. We need to have binman
know what binaries need to be there for a functional output, build the
functional image by default, and then only CI will also pass the flag to
say "fake the missing binaries", so CI will pass, with bad binaries no
one would try and use, and real users will have the build fail due to
missing binaries.

> 
> 
> Thanks,
> - Kever
> On 2023/1/27 15:21, Jagan Teki wrote:
> > This reverts commit f5315dd6290a588434e4f79bfd2886bb7df9210d.
> > 
> > [why]
> > TPL is not mandatory for not all Rockchip SoCs, some SoCs like
> > RK356x, and RK3588 still use mainline u-boot without TPL as
> > their ddr init programs are accessed via binaries provided by
> > Rockchip instead of ddr source code.
> > 
> > Marking TPL build makes it not able to build u-boot.itb on
> > RK356x targets so revert this so that it can build an SPL build
> > that would support all across Rockchip platforms.
> > 
> > Suggested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > Signed-off-by: Jagan Teki <jagan@edgeble.ai>
> > ---
> >   arch/arm/dts/rockchip-u-boot.dtsi | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> > index 234fc5df43..6d1fd7769e 100644
> > --- a/arch/arm/dts/rockchip-u-boot.dtsi
> > +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> > @@ -11,7 +11,7 @@
> >   	};
> >   };
> > -#ifdef CONFIG_TPL
> > +#ifdef CONFIG_SPL
> >   &binman {
> >   	simple-bin {
> >   		filename = "u-boot-rockchip.bin";

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "rockchip: Only call binman when TPL available"

[Quentin Schulz]

Hi Kever,

On 1/31/23 03:53, Kever Yang wrote:
> Hi
> 
>      I think I do this modify for those soc not have TPL, or else it 
> will cause the CI build error.
> 
> 
> TPL/ddr binary is mandatory in all rockchip SoCs now, but the mainline 
> U-Boot may not
> 
> able to provide the the available TPL, will need ddr init binary from 
> rockchip rkbin repository
> 
> instead.
> 
> So the policy is clear, the binary output on mainline U-Boot rockchip 
> platform:
> 
> - TPL available: u-boot-rockchip.bin with TPL, SPL, ATF, U-Boot proper;
> 
> - TPL not available: u-boot.itb with ATF, U-Boot proper.
> 
>                                          idbloader.bin generate by the 
> TPL(ddr init binary instead) and SPL via mkimage cmd;
> 

If I understand correctly, the TPL might not be generated by U-Boot but 
externally provided by Rockchip in binary form? For those cases, the 
defconfig is expected to only request an SPL build I assume. However, 
idbloader.bin (from rockchip, U-Boot TPL replacement) shall still be 
passed with U-Boot SPL to mkimage?

In that case, I guess we could have a Kconfig option like 
CONFIG_EXTERNAL_TPL dependent on !TPL and have the user pass a TPL 
external path environment variable (EXTERNAL_TPL) the same way we do for 
BL31 or TEE in cmd_binman.

Then we should be able to adapt rockchip-u-boot.dtsi to manage this and 
still build a valid u-boot-rockchip.bin, e.g. replace
https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/rockchip-u-boot.dtsi#L23
#ifdef CONFIG_TPL
			multiple-data-files;

			u-boot-tpl {
			};
#endif
with
#if defined(CONFIG_TPL) || defined(CONFIG_EXTERNAL_TPL)
			multiple-data-files;
# ifdef CONFIG_TPL
			u-boot-tpl {
			};
# else
                         blob-ext {
                             filename = "${EXTERNAL_TPL}"
                         };
# endif
#endif

or something more appropriate for binman, like adapting u-boot-tpl to 
handle external blobs for example.

Cheers,
Quentin

Re: [PATCH] Revert "rockchip: Only call binman when TPL available"

[Simon Glass]

Hi Quentin,

On Tue, 31 Jan 2023 at 03:54, Quentin Schulz
<quentin.schulz@theobroma-systems.com> wrote:
>
> Hi Kever,
>
> On 1/31/23 03:53, Kever Yang wrote:
> > Hi
> >
> >      I think I do this modify for those soc not have TPL, or else it
> > will cause the CI build error.
> >
> >
> > TPL/ddr binary is mandatory in all rockchip SoCs now, but the mainline
> > U-Boot may not
> >
> > able to provide the the available TPL, will need ddr init binary from
> > rockchip rkbin repository
> >
> > instead.
> >
> > So the policy is clear, the binary output on mainline U-Boot rockchip
> > platform:
> >
> > - TPL available: u-boot-rockchip.bin with TPL, SPL, ATF, U-Boot proper;
> >
> > - TPL not available: u-boot.itb with ATF, U-Boot proper.
> >
> >                                          idbloader.bin generate by the
> > TPL(ddr init binary instead) and SPL via mkimage cmd;
> >
>
> If I understand correctly, the TPL might not be generated by U-Boot but
> externally provided by Rockchip in binary form? For those cases, the
> defconfig is expected to only request an SPL build I assume. However,
> idbloader.bin (from rockchip, U-Boot TPL replacement) shall still be
> passed with U-Boot SPL to mkimage?
>
> In that case, I guess we could have a Kconfig option like
> CONFIG_EXTERNAL_TPL dependent on !TPL and have the user pass a TPL
> external path environment variable (EXTERNAL_TPL) the same way we do for
> BL31 or TEE in cmd_binman.
>
> Then we should be able to adapt rockchip-u-boot.dtsi to manage this and
> still build a valid u-boot-rockchip.bin, e.g. replace
> https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/rockchip-u-boot.dtsi#L23
> #ifdef CONFIG_TPL
>                         multiple-data-files;
>
>                         u-boot-tpl {
>                         };
> #endif
> with
> #if defined(CONFIG_TPL) || defined(CONFIG_EXTERNAL_TPL)
>                         multiple-data-files;
> # ifdef CONFIG_TPL
>                         u-boot-tpl {
>                         };
> # else
>                          blob-ext {
>                              filename = "${EXTERNAL_TPL}"
>                          };
> # endif
> #endif
>
> or something more appropriate for binman, like adapting u-boot-tpl to
> handle external blobs for example.

That sounds right to me. But I would like a new entry type for this
(rockchip-tpl ?), as we do with BL31, etc.

Also, avoid the #idef around multiple-data-files - it is just easier
to always enable that.

Regards,
Simon

Re: [PATCH] Revert "rockchip: Only call binman when TPL available"

[Jonas Karlman]

Hi Simon and Quentin,

On 2023-02-04 23:23, Simon Glass wrote:
> Hi Quentin,
> 
> On Tue, 31 Jan 2023 at 03:54, Quentin Schulz
> <quentin.schulz@theobroma-systems.com> wrote:
>>
>> Hi Kever,
>>
>> On 1/31/23 03:53, Kever Yang wrote:
>>> Hi
>>>
>>>      I think I do this modify for those soc not have TPL, or else it
>>> will cause the CI build error.
>>>
>>>
>>> TPL/ddr binary is mandatory in all rockchip SoCs now, but the mainline
>>> U-Boot may not
>>>
>>> able to provide the the available TPL, will need ddr init binary from
>>> rockchip rkbin repository
>>>
>>> instead.
>>>
>>> So the policy is clear, the binary output on mainline U-Boot rockchip
>>> platform:
>>>
>>> - TPL available: u-boot-rockchip.bin with TPL, SPL, ATF, U-Boot proper;
>>>
>>> - TPL not available: u-boot.itb with ATF, U-Boot proper.
>>>
>>>                                          idbloader.bin generate by the
>>> TPL(ddr init binary instead) and SPL via mkimage cmd;
>>>
>>
>> If I understand correctly, the TPL might not be generated by U-Boot but
>> externally provided by Rockchip in binary form? For those cases, the
>> defconfig is expected to only request an SPL build I assume. However,
>> idbloader.bin (from rockchip, U-Boot TPL replacement) shall still be
>> passed with U-Boot SPL to mkimage?
>>
>> In that case, I guess we could have a Kconfig option like
>> CONFIG_EXTERNAL_TPL dependent on !TPL and have the user pass a TPL
>> external path environment variable (EXTERNAL_TPL) the same way we do for
>> BL31 or TEE in cmd_binman.
>>
>> Then we should be able to adapt rockchip-u-boot.dtsi to manage this and
>> still build a valid u-boot-rockchip.bin, e.g. replace
>> https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/rockchip-u-boot.dtsi#L23>>> #ifdef CONFIG_TPL
>>                         multiple-data-files;
>>
>>                         u-boot-tpl {
>>                         };
>> #endif
>> with
>> #if defined(CONFIG_TPL) || defined(CONFIG_EXTERNAL_TPL)
>>                         multiple-data-files;
>> # ifdef CONFIG_TPL
>>                         u-boot-tpl {
>>                         };
>> # else
>>                          blob-ext {
>>                              filename = "${EXTERNAL_TPL}"
>>                          };
>> # endif
>> #endif
>>
>> or something more appropriate for binman, like adapting u-boot-tpl to
>> handle external blobs for example.
> 
> That sounds right to me. But I would like a new entry type for this
> (rockchip-tpl ?), as we do with BL31, etc.
> 
> Also, avoid the #idef around multiple-data-files - it is just easier
> to always enable that.

I just sent out a series [1] based on a local patch I was already using,
added a more generic external-tpl entry to binman instead of a rockchip
specific one.

[1] https://patchwork.ozlabs.org/project/uboot/cover/20230205202116.2891673-1-jonas@kwiboo.se/

Regards,
Jonas

> 
> Regards,
> Simon