> On 27 Jul 2017, at 04:33, Kever Yang <kever.y...@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.y...@rock-chips.com>
>>> Acked-by: Philipp Tomsich <philipp.toms...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
