[U-Boot] [PATCH 1/3] ARM: rmobile: Switch CPU to non-secure HYP mode for r8a7790 based boards

[Marek Vasut]

On 2/7/19 4:28 PM, Oleksandr wrote:
> 
> On 05.02.19 20:48, Marek Vasut wrote:
> 
> Hi Marek

Hi,

>> On 1/31/19 6:38 PM, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko at epam.com>
>>>
>>> Both Lager and Stout boards are based on r8a7790 SoC.
>>>
>>> Leave platform specific functions for bringing seconadary CPUs up empty,
>>> since our target is to use PSCI for that.
>>>
>>> Also take care of updating arch timer while we are in secure mode.
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko at epam.com>
>>> ---
>>>   arch/arm/mach-rmobile/Kconfig.32 |  4 ++++
>>>   board/renesas/lager/lager.c      | 51
>>> ++++++++++++++++++++++++++++++++++++++++
>>>   board/renesas/stout/stout.c      | 51
>>> ++++++++++++++++++++++++++++++++++++++++
>>>   include/configs/lager.h          |  3 +++
>>>   include/configs/stout.h          |  3 +++
>>>   5 files changed, 112 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-rmobile/Kconfig.32
>>> b/arch/arm/mach-rmobile/Kconfig.32
>>> index 076a019..a2e9e3d 100644
>>> --- a/arch/arm/mach-rmobile/Kconfig.32
>>> +++ b/arch/arm/mach-rmobile/Kconfig.32
>>> @@ -76,6 +76,8 @@ config TARGET_LAGER
>>>       select SUPPORT_SPL
>>>       select USE_TINY_PRINTF
>>>       imply CMD_DM
>>> +    select CPU_V7_HAS_NONSEC
>>> +    select CPU_V7_HAS_VIRT
>> Shouldn't this be a H2 CPU property instead of a board property ?
> 
> Probably yes, sounds reasonable. I will move these options under "config
> R8A7790".
> 
>> Does this apply to M2* too , since it has CA15 cores as well ?
> 
> I think, yes. But, without PSCI support being implemented for M2*, I
> think it is not worth to select these options for it as well.

It's basically the same SoC with two CPU cores less, how would that PSCI
be any different on M2 ?

>>>   config TARGET_KZM9G
>>>       bool "KZM9D board"
>>> @@ -115,6 +117,8 @@ config TARGET_STOUT
>>>       select SUPPORT_SPL
>>>       select USE_TINY_PRINTF
>>>       imply CMD_DM
>>> +    select CPU_V7_HAS_NONSEC
>>> +    select CPU_V7_HAS_VIRT
>>>     endchoice
>>>   diff --git a/board/renesas/lager/lager.c b/board/renesas/lager/lager.c
>>> index 062e88c..9b96cc4 100644
>>> --- a/board/renesas/lager/lager.c
>>> +++ b/board/renesas/lager/lager.c
>>> @@ -76,6 +76,53 @@ int board_early_init_f(void)
>>>       return 0;
>>>   }
>>>   +#ifdef CONFIG_ARMV7_NONSEC
>>> +#define TIMER_BASE        0xE6080000
>>> +#define TIMER_CNTCR        0x00
>>> +#define TIMER_CNTFID0    0x20
>>> +
>>> +/*
>>> + * Taking into the account that arch timer is only configurable in
>>> secure mode
>>> + * and we are going to enter kernel/hypervisor in a non-secure mode,
>>> update
>>> + * arch timer right now to avoid possible issues. Make sure arch
>>> timer is
>>> + * enabled and configured to use proper frequency.
>>> + */
>>> +static void rcar_gen2_timer_init(void)
>>> +{
>>> +    u32 freq = RMOBILE_XTAL_CLK / 2;
>>> +
>>> +    /*
>>> +     * Update the arch timer if it is either not running, or is not
>>> at the
>>> +     * right frequency.
>>> +     */
>>> +    if ((readl(TIMER_BASE + TIMER_CNTCR) & 1) == 0 ||
>>> +        readl(TIMER_BASE + TIMER_CNTFID0) != freq) {
>> What is this check about ?
> 
> Actually, this code for updating arch timer was borrowed from Linux
> almost as is.
> 
> Code in Linux uses this check in order to update timer only if required
> (either timer disabled or uses wrong freq).
> 
> This check avoids abort in Linux if loader has already updated timer and
> switched to non-secure mode.
> 
> 
> But here in U-Boot, while we are still in secure mode, we can safely
> remove this check and always update the timer. Shall I remove this check?

Shouldn't the timer be correctly configured by U-Boot in the first
place? And shouldn't this be done by timer driver rather than some
ad-hoc init code?

>>
>>> +        /* Update registers with correct frequency */
>>> +        writel(freq, TIMER_BASE + TIMER_CNTFID0);
>>> +        asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (freq));
>>> +
>>> +        /* Make sure arch timer is started by setting bit 0 of CNTCR */
>>> +        writel(1, TIMER_BASE + TIMER_CNTCR);
>> Shouldn't this be in the timer driver instead ?
> 
> Which timer driver? Probably, I missed something, but I didn't find any
> arch timer driver being used for Gen2.
> 
> I see that TMU timer (arch/sh/lib/time.c) is used as a system timer, but
> it is yet another IP.
> 
> Would it be correct, if I move arch timer updating code to
> arch/arm/mach-rmobile directory?
> 
> Actually, at the same location the corresponding code lives in Linux:
> 
> https://elixir.bootlin.com/linux/v5.0-rc5/source/arch/arm/mach-shmobile/setup-rcar-gen2.c#L61

Presumably if this is something else than TMU, it needs proper driver
that goes into drivers/ .

>>> +    }
>>> +}
>>> +
>>> +/*
>>> + * In order not to break compilation if someone decides to build
>>> with PSCI
>>> + * support disabled, keep these dummy functions.
>>> + */
>> That's currently the only use-case.
> 
> Shall I drop this comment and dummy functions below?

Since there is no PSCI support, yes.

[...]

I'd like to have a cover letter go with V2, which describes what you're
trying to achieve here.

-- 
Best regards,
Marek Vasut