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

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@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@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(+)

On 1/31/19 6:38 PM, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@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@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 ?
Does this apply to M2* too , since it has CA15 cores as well ?

>  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 ?

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

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

> +void smp_set_core_boot_addr(unsigned long addr, int corenr)
> +{
> +}
> +
> +void smp_kick_all_cpus(void)
> +{
> +}
> +
> +void smp_waitloop(unsigned int previous_address)
> +{
> +}
> +#endif
> +
>  #define ETHERNET_PHY_RESET	185	/* GPIO 5 31 */
>  
>  int board_init(void)
> @@ -89,6 +136,10 @@ int board_init(void)
>  	mdelay(10);
>  	gpio_direction_output(ETHERNET_PHY_RESET, 1);
>  
> +#ifdef CONFIG_ARMV7_NONSEC

Define empty function in case the macro is not set instead of ifdefing
every place that calls the rcar_gen2_timer_init() function.

> +	rcar_gen2_timer_init();
> +#endif
> +
>  	return 0;
>  }
>  
> diff --git a/board/renesas/stout/stout.c b/board/renesas/stout/stout.c
> index 85e30db..8d034a8 100644
> --- a/board/renesas/stout/stout.c
> +++ b/board/renesas/stout/stout.c
> @@ -77,6 +77,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)
> +{

Why is this stuff in board code ? It should be in driver code / core
code. And there should be only one copy of it.

> +	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) {
> +		/* 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);
> +	}
> +}
> +
> +/*
> + * In order not to break compilation if someone decides to build with PSCI
> + * support disabled, keep these dummy functions.
> + */
> +void smp_set_core_boot_addr(unsigned long addr, int corenr)
> +{
> +}
> +
> +void smp_kick_all_cpus(void)
> +{
> +}
> +
> +void smp_waitloop(unsigned int previous_address)
> +{
> +}
> +#endif
> +
>  #define ETHERNET_PHY_RESET	123	/* GPIO 3 31 */
>  
>  int board_init(void)
> @@ -92,6 +139,10 @@ int board_init(void)
>  	mdelay(20);
>  	gpio_direction_output(ETHERNET_PHY_RESET, 1);
>  
> +#ifdef CONFIG_ARMV7_NONSEC
> +	rcar_gen2_timer_init();
> +#endif
> +
>  	return 0;
>  }
>  
> diff --git a/include/configs/lager.h b/include/configs/lager.h
> index 89c5d01..d8a0b01 100644
> --- a/include/configs/lager.h
> +++ b/include/configs/lager.h
> @@ -48,4 +48,7 @@
>  #define CONFIG_SH_SCIF_CLK_FREQ		65000000
>  #endif
>  
> +/* The PERIPHBASE in the CBAR register is wrong, so override it */
> +#define CONFIG_ARM_GIC_BASE_ADDRESS		0xf1000000
> +
>  #endif	/* __LAGER_H */
> diff --git a/include/configs/stout.h b/include/configs/stout.h
> index 93d9805..7edb9d7 100644
> --- a/include/configs/stout.h
> +++ b/include/configs/stout.h
> @@ -56,4 +56,7 @@
>  #define CONFIG_SH_SCIF_CLK_FREQ		52000000
>  #endif
>  
> +/* The PERIPHBASE in the CBAR register is wrong, so override it */
> +#define CONFIG_ARM_GIC_BASE_ADDRESS		0xf1000000
> +
>  #endif	/* __STOUT_H */
>

On 05.02.19 20:48, Marek Vasut wrote:

Hi Marek

> On 1/31/19 6:38 PM, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@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@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.

>
>>   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?

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

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

>
>> +void smp_set_core_boot_addr(unsigned long addr, int corenr)
>> +{
>> +}
>> +
>> +void smp_kick_all_cpus(void)
>> +{
>> +}
>> +
>> +void smp_waitloop(unsigned int previous_address)
>> +{
>> +}
>> +#endif
>> +
>>   #define ETHERNET_PHY_RESET	185	/* GPIO 5 31 */
>>   
>>   int board_init(void)
>> @@ -89,6 +136,10 @@ int board_init(void)
>>   	mdelay(10);
>>   	gpio_direction_output(ETHERNET_PHY_RESET, 1);
>>   
>> +#ifdef CONFIG_ARMV7_NONSEC
> Define empty function in case the macro is not set instead of ifdefing
> every place that calls the rcar_gen2_timer_init() function.

Agree, will do

>
>> +	rcar_gen2_timer_init();
>> +#endif
>> +
>>   	return 0;
>>   }
>>   
>> diff --git a/board/renesas/stout/stout.c b/board/renesas/stout/stout.c
>> index 85e30db..8d034a8 100644
>> --- a/board/renesas/stout/stout.c
>> +++ b/board/renesas/stout/stout.c
>> @@ -77,6 +77,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)
>> +{
> Why is this stuff in board code ? It should be in driver code / core
> code. And there should be only one copy of it.

Completely agree about the only one copy. When we move this code out of 
Stout/Lager board files, we will have only one of it.

>
>> +	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) {
>> +		/* 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);
>> +	}
>> +}
>> +
>> +/*
>> + * In order not to break compilation if someone decides to build with PSCI
>> + * support disabled, keep these dummy functions.
>> + */
>> +void smp_set_core_boot_addr(unsigned long addr, int corenr)
>> +{
>> +}
>> +
>> +void smp_kick_all_cpus(void)
>> +{
>> +}
>> +
>> +void smp_waitloop(unsigned int previous_address)
>> +{
>> +}
>> +#endif
>> +
>>   #define ETHERNET_PHY_RESET	123	/* GPIO 3 31 */
>>   
>>   int board_init(void)
>> @@ -92,6 +139,10 @@ int board_init(void)
>>   	mdelay(20);
>>   	gpio_direction_output(ETHERNET_PHY_RESET, 1);
>>   
>> +#ifdef CONFIG_ARMV7_NONSEC
>> +	rcar_gen2_timer_init();
>> +#endif
>> +
>>   	return 0;
>>   }
>>   
>> diff --git a/include/configs/lager.h b/include/configs/lager.h
>> index 89c5d01..d8a0b01 100644
>> --- a/include/configs/lager.h
>> +++ b/include/configs/lager.h
>> @@ -48,4 +48,7 @@
>>   #define CONFIG_SH_SCIF_CLK_FREQ		65000000
>>   #endif
>>   
>> +/* The PERIPHBASE in the CBAR register is wrong, so override it */
>> +#define CONFIG_ARM_GIC_BASE_ADDRESS		0xf1000000
>> +
>>   #endif	/* __LAGER_H */
>> diff --git a/include/configs/stout.h b/include/configs/stout.h
>> index 93d9805..7edb9d7 100644
>> --- a/include/configs/stout.h
>> +++ b/include/configs/stout.h
>> @@ -56,4 +56,7 @@
>>   #define CONFIG_SH_SCIF_CLK_FREQ		52000000
>>   #endif
>>   
>> +/* The PERIPHBASE in the CBAR register is wrong, so override it */
>> +#define CONFIG_ARM_GIC_BASE_ADDRESS		0xf1000000
>> +
>>   #endif	/* __STOUT_H */
>>
>

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@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@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.

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@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@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

>

On 07.02.19 19:19, Oleksandr wrote:
>
> 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@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@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.

 From the first look:

1. It should be different total number of CPUs:

For R8A7790 we have the following:

#define R8A7790_PSCI_NR_CPUS    8

But for R8A7791 it seems we need to use:

#define R8A7791_PSCI_NR_CPUS    2

2. It should be new pm-r8a7791.c file which will don't have any CA7 
related stuff (CPU cores, SCU, etc).

Or it should be the single pm-r8a779x.c which must distinguish what SoC 
is being used and do proper things.

>>
>>>>>    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 
>
>
>>

On 2/7/19 6:19 PM, Oleksandr wrote:

[...]

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

You would need a DT entry and a bit of code to start the timer in case
the PSCI is enabled, yes. This would then fit into the DM/DT paradigm.

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

Thanks

On 2/8/19 12:40 PM, Oleksandr wrote:
> 
> On 07.02.19 19:19, Oleksandr wrote:
>>
>> 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@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@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.
> 
> From the first look:
> 
> 1. It should be different total number of CPUs:
> 
> For R8A7790 we have the following:
> 
> #define R8A7790_PSCI_NR_CPUS    8
> 
> But for R8A7791 it seems we need to use:
> 
> #define R8A7791_PSCI_NR_CPUS    2

This information should be in the DT for each SoC, so you should extract
it from there.

> 2. It should be new pm-r8a7791.c file which will don't have any CA7
> related stuff (CPU cores, SCU, etc).

I'd like to have a generic pm-gen2.c file , which parses the DT and
figures the configuration out that way. We are trying to get rid of all
the ad-hoc hardcoded configuration stuff in favor of DT.

> Or it should be the single pm-r8a779x.c which must distinguish what SoC
> is being used and do proper things.

Right

[...]

On 09.02.19 18:35, Marek Vasut wrote:

Hi

> On 2/7/19 6:19 PM, Oleksandr wrote:
>
> [...]
>
>>>>>> +        /* 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.
> You would need a DT entry and a bit of code to start the timer in case
> the PSCI is enabled, yes. This would then fit into the DM/DT paradigm.

I understand your point, thank you.

Will create a simple driver for arch timer in V2.


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

On 09.02.19 18:37, Marek Vasut wrote:

Hi

>>>>>>> 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.
>>  From the first look:
>>
>> 1. It should be different total number of CPUs:
>>
>> For R8A7790 we have the following:
>>
>> #define R8A7790_PSCI_NR_CPUS    8
>>
>> But for R8A7791 it seems we need to use:
>>
>> #define R8A7791_PSCI_NR_CPUS    2
> This information should be in the DT for each SoC, so you should extract
> it from there.
>
>> 2. It should be new pm-r8a7791.c file which will don't have any CA7
>> related stuff (CPU cores, SCU, etc).
> I'd like to have a generic pm-gen2.c file , which parses the DT and
> figures the configuration out that way. We are trying to get rid of all
> the ad-hoc hardcoded configuration stuff in favor of DT.
>
>> Or it should be the single pm-r8a779x.c which must distinguish what SoC
>> is being used and do proper things.
> Right


I agree to have a single generic pm-gen2.c file which covers all Gen2 SoCs.

But, unfortunately, I only have Stout boards (H2), and therefore can 
check only on them. This is why the current patch series adds support 
for H2 SoC only.

Theoretically, I could add support for M2 as well, but, I wouldn't have 
possibility to check.


I would focus on the r8a7790 for now, as the Stout board is our nearest 
target which we want to support in Xen and the PSCI feature is a 
mandatory option.

What do you think?

Hi, Marek

>>
>>> 2. It should be new pm-r8a7791.c file which will don't have any CA7
>>> related stuff (CPU cores, SCU, etc).
>> I'd like to have a generic pm-gen2.c file , which parses the DT and
>> figures the configuration out that way. We are trying to get rid of all
>> the ad-hoc hardcoded configuration stuff in favor of DT.
>>
>>> Or it should be the single pm-r8a779x.c which must distinguish what SoC
>>> is being used and do proper things.
>> Right
>
>
> I agree to have a single generic pm-gen2.c file which covers all Gen2 
> SoCs.
>
> But, unfortunately, I only have Stout boards (H2), and therefore can 
> check only on them. This is why the current patch series adds support 
> for H2 SoC only.
>
> Theoretically, I could add support for M2 as well, but, I wouldn't 
> have possibility to check.
>
>
> I would focus on the r8a7790 for now, as the Stout board is our 
> nearest target which we want to support in Xen and the PSCI feature is 
> a mandatory option.
>
> What do you think?

Could you, please, answer that question...


>
>
>

On 2/26/19 7:37 PM, Oleksandr wrote:
> 
> Hi, Marek

Hi,

>>>
>>>> 2. It should be new pm-r8a7791.c file which will don't have any CA7
>>>> related stuff (CPU cores, SCU, etc).
>>> I'd like to have a generic pm-gen2.c file , which parses the DT and
>>> figures the configuration out that way. We are trying to get rid of all
>>> the ad-hoc hardcoded configuration stuff in favor of DT.
>>>
>>>> Or it should be the single pm-r8a779x.c which must distinguish what SoC
>>>> is being used and do proper things.
>>> Right
>>
>>
>> I agree to have a single generic pm-gen2.c file which covers all Gen2
>> SoCs.
>>
>> But, unfortunately, I only have Stout boards (H2), and therefore can
>> check only on them. This is why the current patch series adds support
>> for H2 SoC only.
>>
>> Theoretically, I could add support for M2 as well, but, I wouldn't
>> have possibility to check.
>>
>>
>> I would focus on the r8a7790 for now, as the Stout board is our
>> nearest target which we want to support in Xen and the PSCI feature is
>> a mandatory option.
>>
>> What do you think?
> 
> Could you, please, answer that question...

I am sorry, I am too busy right now. I will answer that once I can
properly study the problem and give you a useful answer.

On Thu, 2019-03-14 at 01:16 +0100, Marek Vasut wrote:
> On 2/12/19 8:52 PM, Oleksandr wrote:
> 
> Hi,
> 
> [...]
> 
> I was thinking about this whole PSCI situation and how it's all
> implemented today. Basically what we do is generate a separate
> reduced
> shred of U-Boot, which will remain resident in memory and which could
> be
> called by some other software. That shred is a piece of U-Boot
> proper,
> but a lot of stuff is just torn away at runtime, so it's not the
> whole
> binary, just tattered remnants of it.
> 
> I really do not like this approach to the whole U-Boot PSCI in the
> first
> place, it seems really fragile and dangerous.
> 
> But look, U-Boot already has support for U-Boot SPL/TPL, which is
> small,
> can contain a full driver model, drivers and all the necessary
> support
> functionality. It is built from the same sources, at the same time,
> but
> it is not a shred of U-Boot proper but rather a separate entity.
> 
> So I wonder, can't we use this U-Boot SPL/TPL as the piece of code
> which
> would remain resident in memory and handle the PSCI calls instead ? I
> think U-Boot can load such a code or it could be somehow bundled with
> U-Boot proper (using a fitImage maybe ?). This way you won't have to
> re-implement all the drivers in arch/arm/, the DM/DT remains in place
> and the whole PSCI handler would be self-contained, so no need to
> fiddle
> with linker sections of U-Boot itself.
> 
> I am CCing other people who use this PSCI stuff in U-Boot, maybe they
> have some thoughts on this approach.
> 
> And I apologize again for taking this long to reply.
> 
> [...]
> 
Adding Chin Liang and Jeremy.


Regards
Ley Foon

On 2/12/19 8:52 PM, Oleksandr wrote:

Hi,

[...]

I was thinking about this whole PSCI situation and how it's all
implemented today. Basically what we do is generate a separate reduced
shred of U-Boot, which will remain resident in memory and which could be
called by some other software. That shred is a piece of U-Boot proper,
but a lot of stuff is just torn away at runtime, so it's not the whole
binary, just tattered remnants of it.

I really do not like this approach to the whole U-Boot PSCI in the first
place, it seems really fragile and dangerous.

But look, U-Boot already has support for U-Boot SPL/TPL, which is small,
can contain a full driver model, drivers and all the necessary support
functionality. It is built from the same sources, at the same time, but
it is not a shred of U-Boot proper but rather a separate entity.

So I wonder, can't we use this U-Boot SPL/TPL as the piece of code which
would remain resident in memory and handle the PSCI calls instead ? I
think U-Boot can load such a code or it could be somehow bundled with
U-Boot proper (using a fitImage maybe ?). This way you won't have to
re-implement all the drivers in arch/arm/, the DM/DT remains in place
and the whole PSCI handler would be self-contained, so no need to fiddle
with linker sections of U-Boot itself.

I am CCing other people who use this PSCI stuff in U-Boot, maybe they
have some thoughts on this approach.

And I apologize again for taking this long to reply.

[...]

On 14.03.19 02:16, Marek Vasut wrote:
> On 2/12/19 8:52 PM, Oleksandr wrote:
>
> Hi,

Hi

>
> [...]
>
> I was thinking about this whole PSCI situation and how it's all
> implemented today. Basically what we do is generate a separate reduced
> shred of U-Boot, which will remain resident in memory and which could be
> called by some other software. That shred is a piece of U-Boot proper,
> but a lot of stuff is just torn away at runtime, so it's not the whole
> binary, just tattered remnants of it.
>
> I really do not like this approach to the whole U-Boot PSCI in the first
> place, it seems really fragile and dangerous.
>
> But look, U-Boot already has support for U-Boot SPL/TPL, which is small,
> can contain a full driver model, drivers and all the necessary support
> functionality. It is built from the same sources, at the same time, but
> it is not a shred of U-Boot proper but rather a separate entity.
>
> So I wonder, can't we use this U-Boot SPL/TPL as the piece of code which
> would remain resident in memory and handle the PSCI calls instead ? I
> think U-Boot can load such a code or it could be somehow bundled with
> U-Boot proper (using a fitImage maybe ?). This way you won't have to
> re-implement all the drivers in arch/arm/, the DM/DT remains in place
> and the whole PSCI handler would be self-contained, so no need to fiddle
> with linker sections of U-Boot itself.
>
> I am CCing other people who use this PSCI stuff in U-Boot, maybe they
> have some thoughts on this approach.

Thank you for your honest answer.

Let's see what the community will say.


>
> And I apologize again for taking this long to reply.

No problem.


>
> [...]
>

Hi,

> From: Oleksandr <olekstysh@gmail.com>
> Sent: jeudi 14 mars 2019 12:37
> 
> 
> On 14.03.19 02:16, Marek Vasut wrote:
> > On 2/12/19 8:52 PM, Oleksandr wrote:
> >
> > Hi,
> 
> Hi
> 
> >
> > [...]
> >
> > I was thinking about this whole PSCI situation and how it's all
> > implemented today. Basically what we do is generate a separate reduced
> > shred of U-Boot, which will remain resident in memory and which could
> > be called by some other software. That shred is a piece of U-Boot
> > proper, but a lot of stuff is just torn away at runtime, so it's not
> > the whole binary, just tattered remnants of it.
> >
> > I really do not like this approach to the whole U-Boot PSCI in the
> > first place, it seems really fragile and dangerous.
> >
> > But look, U-Boot already has support for U-Boot SPL/TPL, which is
> > small, can contain a full driver model, drivers and all the necessary
> > support functionality. It is built from the same sources, at the same
> > time, but it is not a shred of U-Boot proper but rather a separate entity.
> >
> > So I wonder, can't we use this U-Boot SPL/TPL as the piece of code
> > which would remain resident in memory and handle the PSCI calls
> > instead ? I think U-Boot can load such a code or it could be somehow
> > bundled with U-Boot proper (using a fitImage maybe ?). This way you
> > won't have to re-implement all the drivers in arch/arm/, the DM/DT
> > remains in place and the whole PSCI handler would be self-contained,
> > so no need to fiddle with linker sections of U-Boot itself.
> >
> > I am CCing other people who use this PSCI stuff in U-Boot, maybe they
> > have some thoughts on this approach.
> 
> Thank you for your honest answer.
> 
> Let's see what the community will say.

I will answer for STM32MP1 platform.

On STM32MP157 chip, the PSCI support is expected minimal in U-Boot for basic boot defconfig.
  ROM code => SPL => U-Boot (install PSCI monitor) => Kernel
  => only CPU_ON/OFF for hotplug needed by kernel but no power management.

For full PSCI support (standby modes), we are using TF-A secure monitor (trusted boot defconfig) with full power support or OP-TEE secure monitor
  ROM code => TF-A (BL2 install secure monitor = BL32 : SP min) => U-Boot (non secure world) => Kernel
  ROM code => TF-A (BL2)  =>  secure OS = OP-TEE => U-Boot (non secure world) => Kernel
In these 2 cases U-Boot access secure resource with SPCI request (SMC call).


But it in the future of the basic boot, if we want add a full PSCI support in U-Boot for power management, we need at least access to some resources:
- I2C driver
- STPMIC1 driver
- DDR driver (to switch the DDR in self refresh mode)
- Clock /reset driver

I agree all this part are already managed by U-Boot drivers / u-class.
But we need also a way to have access to board information / DT description.

It is also why I don't implement the function psci_system_off() in ./arch/arm/mach-stm32mp/psci.c
=> I don't want recode a I2C driver in PSCI code...

void __secure psci_system_off(u32 function_id)
{
	/* System Off is not managed, waiting user power off
	 * TODO: handle I2C write in PMIC Main Control register bit 0 = SWOFF
	 */
	while (1)
		wfi();
}

So have a PSCI firmware with driver model based on the U-Boot code (as it is done for SPL/TPL) seems for me a good solution.

Question: what the right moment to install  the secure monitor ?

+ SPL=> U-Boot 	then U-Boot can use PSCI installed firmware but no access of secure resource in U-Boot
+ U-Boot => Kernel 	then U-Boot is executed in secure world can't use the PSCI firmware (as today)

PS: the first solution is used for some board when the secure monitor of TF-A = BL32 is installed by U-Boot (see CONFIG_SPL_ATF)

	ROM code => SPL : install BL32 compiled from TF-A code => U-Boot (non secure world) => Kernel

> 
> >
> > And I apologize again for taking this long to reply.
> 
> No problem.
> 
> 
> >
> > [...]
> >
> --
> Regards,
> 
> Oleksandr Tyshchenko

Regards 

Patrick