| Message ID | 20230127072133.852818-1-jagan@edgeble.ai |
|---|---|
| State | Accepted |
| Commit | 1a45a031d75d8c1e4b63ff72ef5222e491f6481f |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Revert "rockchip: Only call binman when TPL available" | expand |
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
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.
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?
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!
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.
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.
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";
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";
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
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
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
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";
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(-)