[U-Boot,v5,02/18] rockchip: boot0: align to 0x20 for armv7 '_start'

Message ID 1507645279-25188-3-git-send-email-philipp.tomsich@theobroma-systems.com
State Accepted
Delegated to: Philipp Tomsich
Headers show
Series
  • rockchip: back-to-bootrom: replace assembly-implementation with C-code
Related show

Commit Message

Dr. Philipp Tomsich Oct. 10, 2017, 2:21 p.m.
From: Kever Yang <kever.yang@rock-chips.com>

The '_start' is using as vector table base address, and will write
to VBAR register, so it needs to be aligned to 0x20 for armv7.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
[Updated to current code base:]
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 arch/arm/include/asm/arch-rockchip/boot0.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Dr. Philipp Tomsich Nov. 7, 2017, 2:18 p.m. | #1
> From: Kever Yang <kever.yang@rock-chips.com>
> 
> The '_start' is using as vector table base address, and will write
> to VBAR register, so it needs to be aligned to 0x20 for armv7.
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> [Updated to current code base:]
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
> 
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  arch/arm/include/asm/arch-rockchip/boot0.h | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 

Applied to u-boot-rockchip/next, thanks!
Andy Yan Nov. 9, 2017, 12:59 p.m. | #2
Hi Phipipp, Kever:


On 2017年10月10日 22:21, Philipp Tomsich wrote:
> From: Kever Yang <kever.yang@rock-chips.com>
>
> The '_start' is using as vector table base address, and will write
> to VBAR register, so it needs to be aligned to 0x20 for armv7.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> [Updated to current code base:]
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>
> ---
>
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>   arch/arm/include/asm/arch-rockchip/boot0.h | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch-rockchip/boot0.h b/arch/arm/include/asm/arch-rockchip/boot0.h
> index 455d842..f7c6146 100644
> --- a/arch/arm/include/asm/arch-rockchip/boot0.h
> +++ b/arch/arm/include/asm/arch-rockchip/boot0.h
> @@ -6,12 +6,13 @@
>   
>   /*
>    * Execution starts on the instruction following this 4-byte header
> - * (containing the magic 'RK33').
> + * (containing the magic 'RK30', 'RK31', 'RK32' or 'RK33').  This
> + * magic constant will be written into the final image by the rkimage
> + * tool, but we need to reserve space for it here.
>    *
>    * To make life easier for everyone, we build the SPL binary with
>    * space for this 4-byte header already included in the binary.
>    */
> -
>   #ifdef CONFIG_SPL_BUILD
>   	/*
>   	 * We need to add 4 bytes of space for the 'RK33' at the
> @@ -26,6 +27,15 @@
>   	b reset	 /* may be overwritten --- should be 'nop' or a 'b reset' */
>   #endif
>   	b reset

     Do we really need the "b reset" here? the macro ARM_VECTORS already 
has a b reset.
Besides Joseph found  that the irq function will not work with this "b 
reset"
> +#if !defined(CONFIG_ARM64)
> +	/*
> +	 * For armv7, the addr '_start' will used as vector start address
> +	 * and write to VBAR register, which needs to aligned to 0x20.
> +	 */
> +	.align(5)
> +_start:
> +	ARM_VECTORS
> +#endif
>   
>   #if defined(CONFIG_ROCKCHIP_RK3399) && defined(CONFIG_SPL_BUILD)
>   	.space CONFIG_ROCKCHIP_SPL_RESERVE_IRAM	/* space for the ATF data */
Dr. Philipp Tomsich Nov. 9, 2017, 1:03 p.m. | #3
Andy,

On 9 Nov 2017, at 13:59, Andy Yan <andy.yan@rock-chips.com> wrote:
> 
> Hi Phipipp, Kever:
> 
> 
> On 2017年10月10日 22:21, Philipp Tomsich wrote:
>> From: Kever Yang <kever.yang@rock-chips.com>
>> 
>> The '_start' is using as vector table base address, and will write
>> to VBAR register, so it needs to be aligned to 0x20 for armv7.
>> 
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> [Updated to current code base:]
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> 
>> ---
>> 
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>> 
>>  arch/arm/include/asm/arch-rockchip/boot0.h | 14 ++++++++++++--
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/arm/include/asm/arch-rockchip/boot0.h b/arch/arm/include/asm/arch-rockchip/boot0.h
>> index 455d842..f7c6146 100644
>> --- a/arch/arm/include/asm/arch-rockchip/boot0.h
>> +++ b/arch/arm/include/asm/arch-rockchip/boot0.h
>> @@ -6,12 +6,13 @@
>>    /*
>>   * Execution starts on the instruction following this 4-byte header
>> - * (containing the magic 'RK33').
>> + * (containing the magic 'RK30', 'RK31', 'RK32' or 'RK33').  This
>> + * magic constant will be written into the final image by the rkimage
>> + * tool, but we need to reserve space for it here.
>>   *
>>   * To make life easier for everyone, we build the SPL binary with
>>   * space for this 4-byte header already included in the binary.
>>   */
>> -
>>  #ifdef CONFIG_SPL_BUILD
>>  	/*
>>  	 * We need to add 4 bytes of space for the 'RK33' at the
>> @@ -26,6 +27,15 @@
>>  	b reset	 /* may be overwritten --- should be 'nop' or a 'b reset' */
>>  #endif
>>  	b reset
> 
>     Do we really need the "b reset" here? the macro ARM_VECTORS already has a b reset.
> Besides Joseph found  that the irq function will not work with this "b reset”

The quoted code was from an older version of the series, but the ‘b reset’ is still there
in the newer version.

What IRQ function does not work?
Shouldn't the IRQ be jumping into the ARM_VECTORS and never see the ‘b reset’?

>> +#if !defined(CONFIG_ARM64)
>> +	/*
>> +	 * For armv7, the addr '_start' will used as vector start address
>> +	 * and write to VBAR register, which needs to aligned to 0x20.
>> +	 */
>> +	.align(5)
>> +_start:
>> +	ARM_VECTORS
>> +#endif
>>    #if defined(CONFIG_ROCKCHIP_RK3399) && defined(CONFIG_SPL_BUILD)
>>  	.space CONFIG_ROCKCHIP_SPL_RESERVE_IRAM	/* space for the ATF data */
> 
>
Kever Yang Nov. 10, 2017, 3:43 a.m. | #4
Andy,


On 11/09/2017 05:03 AM, Dr. Philipp Tomsich wrote:
> Andy,
>
> On 9 Nov 2017, at 13:59, Andy Yan <andy.yan@rock-chips.com> wrote:
>> Hi Phipipp, Kever:
>>
>>
>> On 2017年10月10日 22:21, Philipp Tomsich wrote:
>>> From: Kever Yang <kever.yang@rock-chips.com>
>>>
>>> The '_start' is using as vector table base address, and will write
>>> to VBAR register, so it needs to be aligned to 0x20 for armv7.
>>>
>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>> [Updated to current code base:]
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>
>>> ---
>>>
>>> Changes in v5: None
>>> Changes in v4: None
>>> Changes in v3: None
>>> Changes in v2: None
>>>
>>>   arch/arm/include/asm/arch-rockchip/boot0.h | 14 ++++++++++++--
>>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/arch-rockchip/boot0.h b/arch/arm/include/asm/arch-rockchip/boot0.h
>>> index 455d842..f7c6146 100644
>>> --- a/arch/arm/include/asm/arch-rockchip/boot0.h
>>> +++ b/arch/arm/include/asm/arch-rockchip/boot0.h
>>> @@ -6,12 +6,13 @@
>>>     /*
>>>    * Execution starts on the instruction following this 4-byte header
>>> - * (containing the magic 'RK33').
>>> + * (containing the magic 'RK30', 'RK31', 'RK32' or 'RK33').  This
>>> + * magic constant will be written into the final image by the rkimage
>>> + * tool, but we need to reserve space for it here.
>>>    *
>>>    * To make life easier for everyone, we build the SPL binary with
>>>    * space for this 4-byte header already included in the binary.
>>>    */
>>> -
>>>   #ifdef CONFIG_SPL_BUILD
>>>   	/*
>>>   	 * We need to add 4 bytes of space for the 'RK33' at the
>>> @@ -26,6 +27,15 @@
>>>   	b reset	 /* may be overwritten --- should be 'nop' or a 'b reset' */
>>>   #endif
>>>   	b reset
>>      Do we really need the "b reset" here? the macro ARM_VECTORS already has a b reset.
>> Besides Joseph found  that the irq function will not work with this "b reset”
> The quoted code was from an older version of the series, but the ‘b reset’ is still there
> in the newer version.
>
> What IRQ function does not work?
> Shouldn't the IRQ be jumping into the ARM_VECTORS and never see the ‘b reset’?

We need the 'b reset' here,  this is not present at the same time with 
ARM_VECTORS,
I think what we need to handle is that we don't need boot0_hook in U-Boot,
let me send a patch to fix this.

Thanks,
- Kever
>
>>> +#if !defined(CONFIG_ARM64)
>>> +	/*
>>> +	 * For armv7, the addr '_start' will used as vector start address
>>> +	 * and write to VBAR register, which needs to aligned to 0x20.
>>> +	 */
>>> +	.align(5)
>>> +_start:
>>> +	ARM_VECTORS
>>> +#endif
>>>     #if defined(CONFIG_ROCKCHIP_RK3399) && defined(CONFIG_SPL_BUILD)
>>>   	.space CONFIG_ROCKCHIP_SPL_RESERVE_IRAM	/* space for the ATF data */
>>
>

Patch

diff --git a/arch/arm/include/asm/arch-rockchip/boot0.h b/arch/arm/include/asm/arch-rockchip/boot0.h
index 455d842..f7c6146 100644
--- a/arch/arm/include/asm/arch-rockchip/boot0.h
+++ b/arch/arm/include/asm/arch-rockchip/boot0.h
@@ -6,12 +6,13 @@ 
 
 /*
  * Execution starts on the instruction following this 4-byte header
- * (containing the magic 'RK33').
+ * (containing the magic 'RK30', 'RK31', 'RK32' or 'RK33').  This
+ * magic constant will be written into the final image by the rkimage
+ * tool, but we need to reserve space for it here.
  *
  * To make life easier for everyone, we build the SPL binary with
  * space for this 4-byte header already included in the binary.
  */
-
 #ifdef CONFIG_SPL_BUILD
 	/*
 	 * We need to add 4 bytes of space for the 'RK33' at the
@@ -26,6 +27,15 @@ 
 	b reset	 /* may be overwritten --- should be 'nop' or a 'b reset' */
 #endif
 	b reset
+#if !defined(CONFIG_ARM64)
+	/*
+	 * For armv7, the addr '_start' will used as vector start address
+	 * and write to VBAR register, which needs to aligned to 0x20.
+	 */
+	.align(5)
+_start:
+	ARM_VECTORS
+#endif
 
 #if defined(CONFIG_ROCKCHIP_RK3399) && defined(CONFIG_SPL_BUILD)
 	.space CONFIG_ROCKCHIP_SPL_RESERVE_IRAM	/* space for the ATF data */