Re: [PATCH 2/5] rockchip: rk3399-pinebook-pro: Fix SPL max size and SPI flash payload offset
Hi Quentin,
On 2023-06-29 10:38, Quentin Schulz wrote:
> Hi Jonas,
>
> On 6/29/23 00:11, Jonas Karlman wrote:
>> Hi Quentin,
>>
>> On 2023-06-28 15:49, Quentin Schulz wrote:
>>> Hi Jonas,
>>>
>>> On 6/27/23 21:10, Jonas Karlman wrote:
TPL max size is limited to 184 KB, SPL is loaded to 0x0 and TF-A is
loaded to 0x4, this limit SPL max size to 256 KB. With BootRom only
reading first 2 KB per 4 KB page of SPI flash, 880 KB may be needed for
TPL+SPL in a worst-case scenario. (184 KB + 256 KB) x 2 = 880 KB
Use 0xE (896 KB) as the payload offset in SPI flash, this allows
for a payload of 3168 KB before env offset start to overlap.
Also add CONFIG_ROCKCHIP_SPI_IMAGE=y to build a bootable SPI flash
image, u-boot-rockchip-spi.bin.
Signed-off-by: Jonas Karlman
---
arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi | 4
configs/pinebook-pro-rk3399_defconfig| 4 +++-
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
b/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
index ea7a5a17ae0f..88a77cad8d43 100644
--- a/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
@@ -10,10 +10,6 @@
chosen {
u-boot,spl-boot-order = "same-as-spl", ,
,
};
-
- config {
- u-boot,spl-payload-offset = <0x6>; /* @ 384KB */
- };
>>>
>>> Just a nitpick/question (and for the whole series): Is there any
>>> specific reason for going back to the Kconfig way
>>> (CONFIG_SYS_SPI_U_BOOT_OFFS) instead of just modifying the offset in the
>>> -u-boot DTSI?
>>
>> The main reason is that SYS_SPI_U_BOOT_OFFS is used as payload offset by
>> binman in rockchip-u-boot.dtsi, for building u-boot-rockchip-spi.bin.
>> And also to avoid any confusion on what value is used.
>>
>> rockchip-u-boot.dtsi:
>>offset = ;
>>
>
> Fair point, converging towards the default for Rockchip platforms seems
> like a good idea, thanks for the explanation.
>
>>>
>>> On a different note, that also means that we're effectively breaking
>>> current setups by moving the environment some other location. I cannot
>>> talk for the maintainer(s) and user(s) but I thought it was worth
>>> mentioning.
>>
>> The environment should not be affected by these changes. All rk3399
>> boards using ENV_IS_IN_SPI_FLASH all use ENV_OFFSET=0x3F8000. u-boot.itb
>> or u-boot.img can be up to 3168 KB before it affects environment.
>>
>>0x00: idbloader.img (TPL+SPL)
>>0x0E: u-boot.itb or u-boot.img (was before at 0x06)
>>0x3F8000: environment, same as before
>>
>> I am not expecting any issues for users, as long as the updated
>> instructions are followed when updating u-boot in SPI flash, i.e.
>> writing u-boot-rockchip-spi.bin at start of SPI flash.
>>
>
> Sorry, seems like I had a brain fart yesterday and mixed SPL payload (so
> U-Boot proper essentially) with U-Boot environment location. Thanks for
> correcting me.
>
> Since those values are dependent on the SoC and not on the hardware
> design, shouldn't we rather hardcode those values for all RK3399
> devices? i.e. set CONFIG_SYS_SPI_U_BOOT_OFFS to 0xe (it's currently
> 0 which is probably a bad default) and have SPL_MAX_SIZE set to 0x4?
> I'm asking now because I did the maths for RK3399 Puma and we got it
> wrong there too. We have TPL_MAX_SIZE set to 0x2e000 like all RK3399
> boards and SPL_MAX_SIZE to 0x2e000. So, (0x2e000 * 2) * 2 = 0xb8000.
> However we have u-boot,spl-payload-offset = <0x8>; so clearly there
> are cases where this would be an issue.
>
> In our case, changing the u-boot,spl-payload-offset isn't without
> backward compatibility issues because we want to be able to boot U-Boot
> proper from SPI but the TPL+SPL from eMMC for example (so the
> u-boot,spl-payload-offset from the DT in the eMMC needs to match the
> offset of U-Boot proper in the SPI). But since we are not aware of any
> user using the fallback mechanism this way rather than TPL+SPL from SPI
> and fallback to eMMC for U-Boot proper, we'd be fine changing it to
> something safer rather than limiting the size of SPL too much.
>
> Basically, what I am asking is whether we should make those RK3399
> defaults instead of fixing them one by one in DTS/defconfigs? I also do
> not know how critical this currently is, does it absolutely need to be
> in this release (we're late in the rc process right now I believe?) or
> can we manage to put more thoughts into making this generic for all
> RK3399 (and maybe other SoCs from Rockchip)?
>
You are correct, the updated SPL max size and SPI payload offset can
probably be used as defaults instead of just modifying a few boards
defconfig.
I left RK3399 Puma out of this series because current defconfig would
trigger a build error if the
Re: [PATCH 2/5] rockchip: rk3399-pinebook-pro: Fix SPL max size and SPI flash payload offset
Hi Jonas,
On 6/29/23 00:11, Jonas Karlman wrote:
Hi Quentin,
On 2023-06-28 15:49, Quentin Schulz wrote:
Hi Jonas,
On 6/27/23 21:10, Jonas Karlman wrote:
TPL max size is limited to 184 KB, SPL is loaded to 0x0 and TF-A is
loaded to 0x4, this limit SPL max size to 256 KB. With BootRom only
reading first 2 KB per 4 KB page of SPI flash, 880 KB may be needed for
TPL+SPL in a worst-case scenario. (184 KB + 256 KB) x 2 = 880 KB
Use 0xE (896 KB) as the payload offset in SPI flash, this allows
for a payload of 3168 KB before env offset start to overlap.
Also add CONFIG_ROCKCHIP_SPI_IMAGE=y to build a bootable SPI flash
image, u-boot-rockchip-spi.bin.
Signed-off-by: Jonas Karlman
---
arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi | 4
configs/pinebook-pro-rk3399_defconfig| 4 +++-
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
b/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
index ea7a5a17ae0f..88a77cad8d43 100644
--- a/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
@@ -10,10 +10,6 @@
chosen {
u-boot,spl-boot-order = "same-as-spl", , ,
};
-
- config {
- u-boot,spl-payload-offset = <0x6>; /* @ 384KB */
- };
Just a nitpick/question (and for the whole series): Is there any
specific reason for going back to the Kconfig way
(CONFIG_SYS_SPI_U_BOOT_OFFS) instead of just modifying the offset in the
-u-boot DTSI?
The main reason is that SYS_SPI_U_BOOT_OFFS is used as payload offset by
binman in rockchip-u-boot.dtsi, for building u-boot-rockchip-spi.bin.
And also to avoid any confusion on what value is used.
rockchip-u-boot.dtsi:
offset = ;
Fair point, converging towards the default for Rockchip platforms seems
like a good idea, thanks for the explanation.
On a different note, that also means that we're effectively breaking
current setups by moving the environment some other location. I cannot
talk for the maintainer(s) and user(s) but I thought it was worth
mentioning.
The environment should not be affected by these changes. All rk3399
boards using ENV_IS_IN_SPI_FLASH all use ENV_OFFSET=0x3F8000. u-boot.itb
or u-boot.img can be up to 3168 KB before it affects environment.
0x00: idbloader.img (TPL+SPL)
0x0E: u-boot.itb or u-boot.img (was before at 0x06)
0x3F8000: environment, same as before
I am not expecting any issues for users, as long as the updated
instructions are followed when updating u-boot in SPI flash, i.e.
writing u-boot-rockchip-spi.bin at start of SPI flash.
Sorry, seems like I had a brain fart yesterday and mixed SPL payload (so
U-Boot proper essentially) with U-Boot environment location. Thanks for
correcting me.
Since those values are dependent on the SoC and not on the hardware
design, shouldn't we rather hardcode those values for all RK3399
devices? i.e. set CONFIG_SYS_SPI_U_BOOT_OFFS to 0xe (it's currently
0 which is probably a bad default) and have SPL_MAX_SIZE set to 0x4?
I'm asking now because I did the maths for RK3399 Puma and we got it
wrong there too. We have TPL_MAX_SIZE set to 0x2e000 like all RK3399
boards and SPL_MAX_SIZE to 0x2e000. So, (0x2e000 * 2) * 2 = 0xb8000.
However we have u-boot,spl-payload-offset = <0x8>; so clearly there
are cases where this would be an issue.
In our case, changing the u-boot,spl-payload-offset isn't without
backward compatibility issues because we want to be able to boot U-Boot
proper from SPI but the TPL+SPL from eMMC for example (so the
u-boot,spl-payload-offset from the DT in the eMMC needs to match the
offset of U-Boot proper in the SPI). But since we are not aware of any
user using the fallback mechanism this way rather than TPL+SPL from SPI
and fallback to eMMC for U-Boot proper, we'd be fine changing it to
something safer rather than limiting the size of SPL too much.
Basically, what I am asking is whether we should make those RK3399
defaults instead of fixing them one by one in DTS/defconfigs? I also do
not know how critical this currently is, does it absolutely need to be
in this release (we're late in the rc process right now I believe?) or
can we manage to put more thoughts into making this generic for all
RK3399 (and maybe other SoCs from Rockchip)?
Otherwise, the maths sound good, so for the whole series:
Reviewed-by: Quentin Schulz
(thanks for the detailed commit log, really helps reviewing :) ).
Cheers,
Quentin
Regards,
Jonas
};
{
diff --git a/configs/pinebook-pro-rk3399_defconfig
b/configs/pinebook-pro-rk3399_defconfig
index 58a8b91aa60f..1109ebb7387b 100644
--- a/configs/pinebook-pro-rk3399_defconfig
+++ b/configs/pinebook-pro-rk3399_defconfig
@@ -12,6 +12,7 @@ CONFIG_ENV_OFFSET=0x3F8000
CONFIG_DEFAULT_DEVICE_TREE="rk3399-pinebook-pro"
CONFIG_DM_RESET=y
CONFIG_ROCKCHIP_RK3399=y
+CONFIG_ROCKCHIP_SPI_IMAGE=y
Re: [PATCH 2/5] rockchip: rk3399-pinebook-pro: Fix SPL max size and SPI flash payload offset
Hi Quentin,
On 2023-06-28 15:49, Quentin Schulz wrote:
> Hi Jonas,
>
> On 6/27/23 21:10, Jonas Karlman wrote:
>> TPL max size is limited to 184 KB, SPL is loaded to 0x0 and TF-A is
>> loaded to 0x4, this limit SPL max size to 256 KB. With BootRom only
>> reading first 2 KB per 4 KB page of SPI flash, 880 KB may be needed for
>> TPL+SPL in a worst-case scenario. (184 KB + 256 KB) x 2 = 880 KB
>>
>> Use 0xE (896 KB) as the payload offset in SPI flash, this allows
>> for a payload of 3168 KB before env offset start to overlap.
>>
>> Also add CONFIG_ROCKCHIP_SPI_IMAGE=y to build a bootable SPI flash
>> image, u-boot-rockchip-spi.bin.
>>
>> Signed-off-by: Jonas Karlman
>> ---
>> arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi | 4
>> configs/pinebook-pro-rk3399_defconfig| 4 +++-
>> 2 files changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
>> b/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
>> index ea7a5a17ae0f..88a77cad8d43 100644
>> --- a/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
>> +++ b/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
>> @@ -10,10 +10,6 @@
>> chosen {
>> u-boot,spl-boot-order = "same-as-spl", , ,
>>
>> };
>> -
>> -config {
>> -u-boot,spl-payload-offset = <0x6>; /* @ 384KB */
>> -};
>
> Just a nitpick/question (and for the whole series): Is there any
> specific reason for going back to the Kconfig way
> (CONFIG_SYS_SPI_U_BOOT_OFFS) instead of just modifying the offset in the
> -u-boot DTSI?
The main reason is that SYS_SPI_U_BOOT_OFFS is used as payload offset by
binman in rockchip-u-boot.dtsi, for building u-boot-rockchip-spi.bin.
And also to avoid any confusion on what value is used.
rockchip-u-boot.dtsi:
offset = ;
>
> On a different note, that also means that we're effectively breaking
> current setups by moving the environment some other location. I cannot
> talk for the maintainer(s) and user(s) but I thought it was worth
> mentioning.
The environment should not be affected by these changes. All rk3399
boards using ENV_IS_IN_SPI_FLASH all use ENV_OFFSET=0x3F8000. u-boot.itb
or u-boot.img can be up to 3168 KB before it affects environment.
0x00: idbloader.img (TPL+SPL)
0x0E: u-boot.itb or u-boot.img (was before at 0x06)
0x3F8000: environment, same as before
I am not expecting any issues for users, as long as the updated
instructions are followed when updating u-boot in SPI flash, i.e.
writing u-boot-rockchip-spi.bin at start of SPI flash.
Regards,
Jonas
>
>> };
>>
>>{
>> diff --git a/configs/pinebook-pro-rk3399_defconfig
>> b/configs/pinebook-pro-rk3399_defconfig
>> index 58a8b91aa60f..1109ebb7387b 100644
>> --- a/configs/pinebook-pro-rk3399_defconfig
>> +++ b/configs/pinebook-pro-rk3399_defconfig
>> @@ -12,6 +12,7 @@ CONFIG_ENV_OFFSET=0x3F8000
>> CONFIG_DEFAULT_DEVICE_TREE="rk3399-pinebook-pro"
>> CONFIG_DM_RESET=y
>> CONFIG_ROCKCHIP_RK3399=y
>> +CONFIG_ROCKCHIP_SPI_IMAGE=y
>> CONFIG_TARGET_PINEBOOK_PRO_RK3399=y
>> CONFIG_SPL_STACK=0x40
>> CONFIG_DEBUG_UART_BASE=0xFF1A
>> @@ -26,7 +27,7 @@ CONFIG_USE_PREBOOT=y
>> CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-pinebook-pro.dtb"
>> CONFIG_DISPLAY_BOARDINFO_LATE=y
>> CONFIG_MISC_INIT_R=y
>> -CONFIG_SPL_MAX_SIZE=0x2e000
>> +CONFIG_SPL_MAX_SIZE=0x4
>
> nitpick: We *could* have this in another commit after we move the
> environment to another location later in the SPI flash.
>
> Cheers,
> Quentin
Re: [PATCH 2/5] rockchip: rk3399-pinebook-pro: Fix SPL max size and SPI flash payload offset
Hi Jonas,
On 6/27/23 21:10, Jonas Karlman wrote:
TPL max size is limited to 184 KB, SPL is loaded to 0x0 and TF-A is
loaded to 0x4, this limit SPL max size to 256 KB. With BootRom only
reading first 2 KB per 4 KB page of SPI flash, 880 KB may be needed for
TPL+SPL in a worst-case scenario. (184 KB + 256 KB) x 2 = 880 KB
Use 0xE (896 KB) as the payload offset in SPI flash, this allows
for a payload of 3168 KB before env offset start to overlap.
Also add CONFIG_ROCKCHIP_SPI_IMAGE=y to build a bootable SPI flash
image, u-boot-rockchip-spi.bin.
Signed-off-by: Jonas Karlman
---
arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi | 4
configs/pinebook-pro-rk3399_defconfig| 4 +++-
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
b/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
index ea7a5a17ae0f..88a77cad8d43 100644
--- a/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
@@ -10,10 +10,6 @@
chosen {
u-boot,spl-boot-order = "same-as-spl", , ,
};
-
- config {
- u-boot,spl-payload-offset = <0x6>; /* @ 384KB */
- };
Just a nitpick/question (and for the whole series): Is there any
specific reason for going back to the Kconfig way
(CONFIG_SYS_SPI_U_BOOT_OFFS) instead of just modifying the offset in the
-u-boot DTSI?
On a different note, that also means that we're effectively breaking
current setups by moving the environment some other location. I cannot
talk for the maintainer(s) and user(s) but I thought it was worth
mentioning.
};
{
diff --git a/configs/pinebook-pro-rk3399_defconfig
b/configs/pinebook-pro-rk3399_defconfig
index 58a8b91aa60f..1109ebb7387b 100644
--- a/configs/pinebook-pro-rk3399_defconfig
+++ b/configs/pinebook-pro-rk3399_defconfig
@@ -12,6 +12,7 @@ CONFIG_ENV_OFFSET=0x3F8000
CONFIG_DEFAULT_DEVICE_TREE="rk3399-pinebook-pro"
CONFIG_DM_RESET=y
CONFIG_ROCKCHIP_RK3399=y
+CONFIG_ROCKCHIP_SPI_IMAGE=y
CONFIG_TARGET_PINEBOOK_PRO_RK3399=y
CONFIG_SPL_STACK=0x40
CONFIG_DEBUG_UART_BASE=0xFF1A
@@ -26,7 +27,7 @@ CONFIG_USE_PREBOOT=y
CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-pinebook-pro.dtb"
CONFIG_DISPLAY_BOARDINFO_LATE=y
CONFIG_MISC_INIT_R=y
-CONFIG_SPL_MAX_SIZE=0x2e000
+CONFIG_SPL_MAX_SIZE=0x4
nitpick: We *could* have this in another commit after we move the
environment to another location later in the SPI flash.
Cheers,
Quentin
