On 27 Jul 2017, at 04:33, Kever Yang kever.yang@rock-chips.com wrote:
Hi Philipp,
On 07/27/2017 01:40 AM, Philipp Tomsich wrote:
Kever,
On Wed, 26 Jul 2017, Kever Yang wrote:
Lets set the all the DDR region as non secure in SPL, the trust like OPTEE should have the correct setting for it if there is one.
Signed-off-by: Kever Yang kever.yang@rock-chips.com Acked-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
arch/arm/mach-rockchip/rk322x-board-spl.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/arch/arm/mach-rockchip/rk322x-board-spl.c b/arch/arm/mach-rockchip/rk322x-board-spl.c index 15216c7..aa8a742 100644 --- a/arch/arm/mach-rockchip/rk322x-board-spl.c +++ b/arch/arm/mach-rockchip/rk322x-board-spl.c @@ -41,6 +41,8 @@ static struct rk322x_grf * const grf = (void *)GRF_BASE; CON_IOMUX_UART2SEL_MASK, CON_IOMUX_UART2SEL_21 << CON_IOMUX_UART2SEL_SHIFT); }
+#define SGRF_DDR_CON0 0x10150000
Please use a typed const-variable (e.g. 'const u32 *') in the function where this is needed.
I must have been tired yesterday: I meant 'u32 * const' (i.e. a constant-pointer to a mutable u32).
I don't understand why I can't use the macro directly and it's definitely const.
Having a macro (pasting an integer literal) does not convey the same type information and can not be local to a single function. By making this a constant, you’ll get full use of the typesystem (and there’s no implicit conversion from an integer literal when calling rk_clrreg). Additionally, the debug-info will contain the constant’s name.
This is similar to why Simon was moving towards enums wherever he could… only that I prefer the use of constants, as that provides the most accurate typeinfo.
void board_init_f(ulong dummy) { struct udevice *dev; @@ -71,6 +73,7 @@ void board_init_f(ulong dummy) return; }
- rk_clrreg(SGRF_DDR_CON0, 0x4000);
Could you add a comment here that explains what is set and what the meanings of these bits are? I think I've seen a comment somewhere (ATF?) explaining that the '4' comes from enabling some sort of bypass, but it would be great to be able to just look here and immediately understand what this line does...
Well, I will add comments here and also update the commit message.
Much appreciated.
Thanks,
- Kever
Also (without any comment explaining what goes on), it feels a bit odd that 'set all to non-secure' does not simply zero all bits.
#if defined(CONFIG_ROCKCHIP_SPL_BACK_TO_BROM) && !defined(CONFIG_SPL_BOARD_INIT) back_to_bootrom(); #endif