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

[Oleksandr]

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

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 ?
I need some time to investigate. I will come up with an answer later.
>
>>>>    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?

Yes, this timer should be correctly configured by U-Boot. And yes, the timer

configuration code should be in a proper place.

>
>>>> +        /* 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/ .

Did I get your point correctly that new driver (specially for Gen2 arch 
timer) should be introduced in U-Boot and

the only one thing it is intended to do is to configure that timer for 
the future use by Linux/Hypervisor?

If yes, the only question I have is how that new driver is going to be 
populated? There is no corresponding node for arch timer in the device tree.

https://elixir.bootlin.com/linux/v5.0-rc5/source/arch/arm/boot/dts/r8a7790.dtsi

So, do I need specially for this case add arch timer node with reg and 
compatible properties?

Sorry if I ask obvious questions, not familiar enough with U-Boot.

>
>>>> +    }
>>>> +}
>>>> +
>>>> +/*
>>>> + * 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.

Understand.

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

Yes, sure. Cover letter is present for the current V1 as well.

I thought that I had CC-ed all.

This is a link to it:

http://u-boot.10912.n7.nabble.com/PATCH-0-3-PSCI-support-for-r8a7790-SoC-Lager-Stout-boards-td357352.html

>
-- 
Regards,

Oleksandr Tyshchenko