[U-Boot] [U-Boot, 1/4] rockchip: rk322x: set the DDR region as non-secure in SPL

[Dr. Philipp Tomsich]

> On 27 Jul 2017, at 04:33, Kever Yang <kever.yang at 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 at rock-chips.com>
>>> Acked-by: Philipp Tomsich <philipp.tomsich at 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