[U-Boot] [PATCH] rockchip: reserve memory for rk3399 ATF data

[U-Boot] [PATCH] rockchip: reserve memory for rk3399 ATF data

[Kever Yang]

There are 3 region used by rk3399 ATF:
- bl31 code, locate at 0x10000;
- cortex-m0 code and data, locate at 0xff8c0000;
- bl31 data, locate at 0xff8c1000 ~ 0xff8c4000;

SPL_TEXT_BASE starts from 0xff8c2000, we need to reserve memory
for ATF data, or else there will have memory corrupt after SPL
load ATF image.

More detail about cortex-M0 code in ATF:
https://github.com/ARM-software/arm-trusted-firmware/commit/
8382e17c4c6bffd15119dfce1ee4372e3c1a7890

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 arch/arm/include/asm/arch-rockchip/boot0.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/include/asm/arch-rockchip/boot0.h b/arch/arm/include/asm/arch-rockchip/boot0.h
index 8d7bc9a..f85a4db 100644
--- a/arch/arm/include/asm/arch-rockchip/boot0.h
+++ b/arch/arm/include/asm/arch-rockchip/boot0.h
@@ -16,3 +16,7 @@
 	.space 0x4         /* space for the 'RK33' */
 #endif
 	b reset
+
+#ifdef defined(CONFIG_ROCKCHIP_RK3399) && defined(CONFIG_SPL_BUILD)
+	.space 0x4000         /* space for the ATF data */
+#endif
-- 
1.9.1

[U-Boot] [PATCH] rockchip: reserve memory for rk3399 ATF data

[Dr. Philipp Tomsich]

Kever,

Do we really need to change the SPL layout (i.e. BL2) for this?

The SPL code should remain independent of later stages. This change would tie the
U-Boot SPL (BL2) to a specific implementation/memory layout of the later BL31 stage.
It should rather remain the responsibility of the BL31 stage to properly relocate itself
as needed upon startup...

Our (yet unreleased) development tree (for ATF) uses a much less intrusive approach
to achieve the same result (using the knowledge that the ATF will not return to SPL and
thus allowing the ATF to overwrite memory areas previously used by SPL):
1.	ATF (BL31), the M0-firmware and the second-stage U-Boot are loaded as
	separate firmware blobs into DRAM using Andre's FIT image loader patches
2.	SPL transfers control to ATF (with a vendor-specific parameter payload, which
	contains the location of the M0 firmware in DRAM)
3.	ATF installs the M0 firmware into its final location

Note that we’ve split the M0 firmware off the ATF build (and into a separate repository),
as we’d otherwise end up with ELF files that has data/code/etc in the first MB of the
address space and the M0 binary at 0xff8c0000 — if you convert such an ELF to a binary,
you’d end up with a file size of approx. 4GB.  In other words: we don’t include the M0
binary into the ATF, but load it separately through the FIT image loader...

I’ll try to get our Cortex-M0 and ATF repositories pushed to our public GIT by early
next week, so you can review…

Regards,
Philipp.

> On 14 Apr 2017, at 12:21, Kever Yang <kever.yang@rock-chips.com> wrote:
> 
> There are 3 region used by rk3399 ATF:
> - bl31 code, locate at 0x10000;
> - cortex-m0 code and data, locate at 0xff8c0000;
> - bl31 data, locate at 0xff8c1000 ~ 0xff8c4000;
> 
> SPL_TEXT_BASE starts from 0xff8c2000, we need to reserve memory
> for ATF data, or else there will have memory corrupt after SPL
> load ATF image.
> 
> More detail about cortex-M0 code in ATF:
> https://github.com/ARM-software/arm-trusted-firmware/commit/
> 8382e17c4c6bffd15119dfce1ee4372e3c1a7890
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
> 
> arch/arm/include/asm/arch-rockchip/boot0.h | 4 ++++
> 1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/include/asm/arch-rockchip/boot0.h b/arch/arm/include/asm/arch-rockchip/boot0.h
> index 8d7bc9a..f85a4db 100644
> --- a/arch/arm/include/asm/arch-rockchip/boot0.h
> +++ b/arch/arm/include/asm/arch-rockchip/boot0.h
> @@ -16,3 +16,7 @@
> 	.space 0x4         /* space for the 'RK33' */
> #endif
> 	b reset
> +
> +#ifdef defined(CONFIG_ROCKCHIP_RK3399) && defined(CONFIG_SPL_BUILD)
> +	.space 0x4000         /* space for the ATF data */
> +#endif
> -- 
> 1.9.1
> 

[U-Boot] [PATCH] rockchip: reserve memory for rk3399 ATF data

[Simon Glass]

Hi Philipp,

On 14 April 2017 at 04:51, Dr. Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> Kever,
>
> Do we really need to change the SPL layout (i.e. BL2) for this?
>
> The SPL code should remain independent of later stages. This change would tie the
> U-Boot SPL (BL2) to a specific implementation/memory layout of the later BL31 stage.
> It should rather remain the responsibility of the BL31 stage to properly relocate itself
> as needed upon startup...
>
> Our (yet unreleased) development tree (for ATF) uses a much less intrusive approach
> to achieve the same result (using the knowledge that the ATF will not return to SPL and
> thus allowing the ATF to overwrite memory areas previously used by SPL):
> 1.      ATF (BL31), the M0-firmware and the second-stage U-Boot are loaded as
>         separate firmware blobs into DRAM using Andre's FIT image loader patches
> 2.      SPL transfers control to ATF (with a vendor-specific parameter payload, which
>         contains the location of the M0 firmware in DRAM)
> 3.      ATF installs the M0 firmware into its final location
>
> Note that we’ve split the M0 firmware off the ATF build (and into a separate repository),
> as we’d otherwise end up with ELF files that has data/code/etc in the first MB of the
> address space and the M0 binary at 0xff8c0000 — if you convert such an ELF to a binary,
> you’d end up with a file size of approx. 4GB.  In other words: we don’t include the M0
> binary into the ATF, but load it separately through the FIT image loader...
>
> I’ll try to get our Cortex-M0 and ATF repositories pushed to our public GIT by early
> next week, so you can review…
>

So should we take this patch in the meantime, or is it a backwards step?

- Simon

[U-Boot] [PATCH] rockchip: reserve memory for rk3399 ATF data

[Kever Yang]

Hi Philipp,


On 04/14/2017 06:51 PM, Dr. Philipp Tomsich wrote:
> Kever,
>
> Do we really need to change the SPL layout (i.e. BL2) for this?
>
> The SPL code should remain independent of later stages. This change would tie the
> U-Boot SPL (BL2) to a specific implementation/memory layout of the later BL31 stage.
> It should rather remain the responsibility of the BL31 stage to properly relocate itself
> as needed upon startup...

We make this change because people think it's BL2's responsibility to 
load everything,
to its loading address. I don't have much detail, but I do know that the 
upstream
ATF of rk3399 is already using .elf format instead of .bin format and 
require BL2 to
load 3 region data, bl31 do not do any relocation.

I don't know if we can optimize the flow to reduce the memory 
dependency, I only
met one issue if I don't apply this patch:
board_fit_config_name_match function in 
arch/arm/mach-rockchip/rk3399-board-spl.c
is corrupt after bl31 images loaded.

This issue is because both BL2(SPL) and BL31 are using iram, I think 
it's OK to reserve
memory for bl31 when there is enough memory, just like we need U-Boot 
and kernel
reserved memory for ATF.
>
> Our (yet unreleased) development tree (for ATF) uses a much less intrusive approach
> to achieve the same result (using the knowledge that the ATF will not return to SPL and
> thus allowing the ATF to overwrite memory areas previously used by SPL):
> 1.	ATF (BL31), the M0-firmware and the second-stage U-Boot are loaded as
> 	separate firmware blobs into DRAM using Andre's FIT image loader patches
> 2.	SPL transfers control to ATF (with a vendor-specific parameter payload, which
> 	contains the location of the M0 firmware in DRAM)
> 3.	ATF installs the M0 firmware into its final location

The latest version ATF does no do this operation, which version of ATF 
are you using?
> Note that we’ve split the M0 firmware off the ATF build (and into a separate repository),
> as we’d otherwise end up with ELF files that has data/code/etc in the first MB of the
> address space and the M0 binary at 0xff8c0000 — if you convert such an ELF to a binary,
> you’d end up with a file size of approx. 4GB.  In other words: we don’t include the M0
> binary into the ATF, but load it separately through the FIT image loader...

I need 3 "loadable" binaries now, bl31, m0-firmware and bl31 data at 
iram, they are
load separately through the FIT, and maybe secure check in the future.
>
> I’ll try to get our Cortex-M0 and ATF repositories pushed to our public GIT by early
> next week, so you can review…

I will try to ask our ATF owner and related engineer to review, but 
since the latest
version which bl31 do not do the relocate already in production for our 
rk3399
chromebook and Android project, but I think people won't change there 
already
has a lot of discuss on this.

Thanks,
- Kever
>
> Regards,
> Philipp.
>
>> On 14 Apr 2017, at 12:21, Kever Yang <kever.yang@rock-chips.com> wrote:
>>
>> There are 3 region used by rk3399 ATF:
>> - bl31 code, locate at 0x10000;
>> - cortex-m0 code and data, locate at 0xff8c0000;
>> - bl31 data, locate at 0xff8c1000 ~ 0xff8c4000;
>>
>> SPL_TEXT_BASE starts from 0xff8c2000, we need to reserve memory
>> for ATF data, or else there will have memory corrupt after SPL
>> load ATF image.
>>
>> More detail about cortex-M0 code in ATF:
>> https://github.com/ARM-software/arm-trusted-firmware/commit/
>> 8382e17c4c6bffd15119dfce1ee4372e3c1a7890
>>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>
>> arch/arm/include/asm/arch-rockchip/boot0.h | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/arch-rockchip/boot0.h b/arch/arm/include/asm/arch-rockchip/boot0.h
>> index 8d7bc9a..f85a4db 100644
>> --- a/arch/arm/include/asm/arch-rockchip/boot0.h
>> +++ b/arch/arm/include/asm/arch-rockchip/boot0.h
>> @@ -16,3 +16,7 @@
>> 	.space 0x4         /* space for the 'RK33' */
>> #endif
>> 	b reset
>> +
>> +#ifdef defined(CONFIG_ROCKCHIP_RK3399) && defined(CONFIG_SPL_BUILD)
>> +	.space 0x4000         /* space for the ATF data */
>> +#endif
>> -- 
>> 1.9.1
>>
>

[U-Boot] [PATCH] rockchip: reserve memory for rk3399 ATF data

[Dr. Philipp Tomsich]

Kever,

Thanks for clarifying regarding what versions of ATF are out in production.
I still feel a bit uneasy, as the size of the Cortex-M0 firmware could grow in the future
(but then again, if that happens an alternative solution can be used).

Given that this is needed for backward compatiblity, let’s simply adopt this change.
Could you please make this a configurable option (via Kconfig), as I hope that we’ll
be able to deprecate it at some point.

Thanks,
Philipp.

> On 17 Apr 2017, at 11:15, Kever Yang <kever.yang@rock-chips.com> wrote:
> 
> Hi Philipp,
> 
> 
> On 04/14/2017 06:51 PM, Dr. Philipp Tomsich wrote:
>> Kever,
>> 
>> Do we really need to change the SPL layout (i.e. BL2) for this?
>> 
>> The SPL code should remain independent of later stages. This change would tie the
>> U-Boot SPL (BL2) to a specific implementation/memory layout of the later BL31 stage.
>> It should rather remain the responsibility of the BL31 stage to properly relocate itself
>> as needed upon startup...
> 
> We make this change because people think it's BL2's responsibility to load everything,
> to its loading address. I don't have much detail, but I do know that the upstream
> ATF of rk3399 is already using .elf format instead of .bin format and require BL2 to
> load 3 region data, bl31 do not do any relocation.
> 
> I don't know if we can optimize the flow to reduce the memory dependency, I only
> met one issue if I don't apply this patch:
> board_fit_config_name_match function in arch/arm/mach-rockchip/rk3399-board-spl.c
> is corrupt after bl31 images loaded.
> 
> This issue is because both BL2(SPL) and BL31 are using iram, I think it's OK to reserve
> memory for bl31 when there is enough memory, just like we need U-Boot and kernel
> reserved memory for ATF.
>> 
>> Our (yet unreleased) development tree (for ATF) uses a much less intrusive approach
>> to achieve the same result (using the knowledge that the ATF will not return to SPL and
>> thus allowing the ATF to overwrite memory areas previously used by SPL):
>> 1.	ATF (BL31), the M0-firmware and the second-stage U-Boot are loaded as
>> 	separate firmware blobs into DRAM using Andre's FIT image loader patches
>> 2.	SPL transfers control to ATF (with a vendor-specific parameter payload, which
>> 	contains the location of the M0 firmware in DRAM)
>> 3.	ATF installs the M0 firmware into its final location
> 
> The latest version ATF does no do this operation, which version of ATF are you using?
>> Note that we’ve split the M0 firmware off the ATF build (and into a separate repository),
>> as we’d otherwise end up with ELF files that has data/code/etc in the first MB of the
>> address space and the M0 binary at 0xff8c0000 — if you convert such an ELF to a binary,
>> you’d end up with a file size of approx. 4GB.  In other words: we don’t include the M0
>> binary into the ATF, but load it separately through the FIT image loader...
> 
> I need 3 "loadable" binaries now, bl31, m0-firmware and bl31 data at iram, they are
> load separately through the FIT, and maybe secure check in the future.

Same here.

>> 
>> I’ll try to get our Cortex-M0 and ATF repositories pushed to our public GIT by early
>> next week, so you can review…
> 
> I will try to ask our ATF owner and related engineer to review, but since the latest
> version which bl31 do not do the relocate already in production for our rk3399
> chromebook and Android project, but I think people won't change there already
> has a lot of discuss on this.
> 
> Thanks,
> - Kever
>> 
>> Regards,
>> Philipp.
>> 
>>> On 14 Apr 2017, at 12:21, Kever Yang <kever.yang@rock-chips.com> wrote:
>>> 
>>> There are 3 region used by rk3399 ATF:
>>> - bl31 code, locate at 0x10000;
>>> - cortex-m0 code and data, locate at 0xff8c0000;
>>> - bl31 data, locate at 0xff8c1000 ~ 0xff8c4000;
>>> 
>>> SPL_TEXT_BASE starts from 0xff8c2000, we need to reserve memory
>>> for ATF data, or else there will have memory corrupt after SPL
>>> load ATF image.
>>> 
>>> More detail about cortex-M0 code in ATF:
>>> https://github.com/ARM-software/arm-trusted-firmware/commit/
>>> 8382e17c4c6bffd15119dfce1ee4372e3c1a7890
>>> 
>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>> ---
>>> 
>>> arch/arm/include/asm/arch-rockchip/boot0.h | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>> 
>>> diff --git a/arch/arm/include/asm/arch-rockchip/boot0.h b/arch/arm/include/asm/arch-rockchip/boot0.h
>>> index 8d7bc9a..f85a4db 100644
>>> --- a/arch/arm/include/asm/arch-rockchip/boot0.h
>>> +++ b/arch/arm/include/asm/arch-rockchip/boot0.h
>>> @@ -16,3 +16,7 @@
>>> 	.space 0x4         /* space for the 'RK33' */
>>> #endif
>>> 	b reset
>>> +
>>> +#ifdef defined(CONFIG_ROCKCHIP_RK3399) && defined(CONFIG_SPL_BUILD)
>>> +	.space 0x4000         /* space for the ATF data */
>>> +#endif
>>> -- 
>>> 1.9.1
>>> 
>> 
> 
>