[U-Boot] [PATCH 0/3] PSCI support for r8a7790 SoC (Lager/Stout boards)

[U-Boot] [PATCH 0/3] PSCI support for r8a7790 SoC (Lager/Stout boards)

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Hi, all.

The purpose of this patch series is to add PSCI support for Renesas boards
based on R-Car Gen2 r8a7790 SoC. Actually, our target in Stout board, but as
Lager board is also based on the same SoC, that patch series covers both.

The main goal of using PSCI is to have common interface to boot CPUs
from Hypervisor/Linux. PSCI is a generic well-known way to bring-up CPU
and proven to work. With this patch series applied all CPUs will be switched to
a non-secure Hypervisor mode. For running Type 1 Hypervisor (e.g. Xen) it is
mandatory requirement. From other side, there are recommendation to boot Linux
in Hypervisor mode on ARM. This allows KVM or other Hypervisor (e.g. Jailhouse)
to be useable on the platform. If there are no Hypervisor present, Linux will
just initialize the Hypervisor mode correctly and drop to Supervisor mode.

Till now, we have been using a *custom solution* for running Xen Hypervisor
on Lager/Stout boards, which requires a specific U-Boot version (which performs
СPUs boot in a non-generic way) and different hacks into R-Car Gen2 platform
code in Linux. We have a situation where different methods are used in order
to boot CPUs from Xen/Linux. Which results in a forced switching between different
U-Boot versions, when we need to switch between bare Linux and Xen,
which is quite inconvenient. This should be totally transparent to the user.

----------

Current patch series is based on the following commit:
425c0a43fbbec36571f6a03b530695b8b16a841d “Prepare v2019.01-rc3”

It was tested with bare Linux 5.0.0-rc3 and Xen 4.12.0-rc with Linux 5.0.0-rc3
as guest OS. Everything worked as expected.
In case of bare Linux, all CPU cores started in HYP mode and PSCI checker
confirmed that hotplug tests had successfully passed.

Just one note. For each secondary CPU boot, Linux complains about
“Spectre v2: firmware did not set auxiliary control register IBE bit,
system vulnerable”.
Probably because the corresponding ARM errata (CONFIG_ARM_CORTEX_A15_CVE_2017_5715, etc)
is not propagated to non-boot (secondary) CPUs.

You can also find patch series here (last 3 patches):
https://github.com/otyshchenko1/u-boot/commits/r8a7790_psci_upstream

----------

Please note:
1. Current patch series implies corresponding changes to Linux.

You can find them here (last 2 patches):
https://github.com/otyshchenko1/linux/commits/psci_upstream

I am about to push them as well.

2. As PSCI code expects a bigger “Maximum supported CPUs for PSCI” value (8)
than default option (4), you will get a compilation error:
arch/arm/mach-rmobile/psci.c:21:2: error: #error "invalid value for CONFIG_ARMV7_PSCI_NR_CPUS"
#error "invalid value for CONFIG_ARMV7_PSCI_NR_CPUS"

To resolve it, just run make menuconfig, set this option to 8
(or change in .config directly) and recompile.

----------

Oleksandr Tyshchenko (3):
  ARM: rmobile: Switch CPU to non-secure HYP mode for r8a7790 based
    boards
  ARM: rmobile: Add basic PSCI support for r8a7790 SoC
  ARM: rmobile: Add possibility to debug main PSCI commands

 arch/arm/mach-rmobile/Kconfig.32   |   6 +
 arch/arm/mach-rmobile/Makefile     |   6 +
 arch/arm/mach-rmobile/debug.h      |  91 +++++++++
 arch/arm/mach-rmobile/pm-r8a7790.c | 408 +++++++++++++++++++++++++++++++++++++
 arch/arm/mach-rmobile/pm-r8a7790.h |  72 +++++++
 arch/arm/mach-rmobile/psci.c       | 216 ++++++++++++++++++++
 board/renesas/lager/lager.c        |  51 +++++
 board/renesas/stout/stout.c        |  51 +++++
 include/configs/lager.h            |   5 +
 include/configs/stout.h            |   5 +
 10 files changed, 911 insertions(+)
 create mode 100644 arch/arm/mach-rmobile/debug.h
 create mode 100644 arch/arm/mach-rmobile/pm-r8a7790.c
 create mode 100644 arch/arm/mach-rmobile/pm-r8a7790.h
 create mode 100644 arch/arm/mach-rmobile/psci.c

-- 
2.7.4

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

[Oleksandr Tyshchenko]

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
 
 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) {
+		/* 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	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
+	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)
+{
+	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 */
-- 
2.7.4

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

[Marek Vasut]

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


-- 
Best regards,
Marek Vasut

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

[Oleksandr]

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 */
>>
>
-- 
Regards,

Oleksandr Tyshchenko

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

-- 
Best regards,
Marek Vasut

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

>
-- 
Regards,

Oleksandr Tyshchenko

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

[Oleksandr]

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

Oleksandr Tyshchenko

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

[Marek Vasut]

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

[...]

-- 
Best regards,
Marek Vasut

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

[Oleksandr]

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?

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

[Oleksandr]

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


>
>
>
-- 
Regards,

Oleksandr Tyshchenko

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

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut

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

[Marek Vasut]

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

-- 
Best regards,
Marek Vasut

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

[Oleksandr]

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
>

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

[Marek Vasut]

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.

[...]

-- 
Best regards,
Marek Vasut

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

[Ley Foon Tan]

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

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

[Oleksandr]

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.


>
> [...]
>
-- 
Regards,

Oleksandr Tyshchenko

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

[Patrick DELAUNAY]

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

[U-Boot] [PATCH 2/3] ARM: rmobile: Add basic PSCI support for r8a7790 SoC

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Also enable PSCI support for Stout and Lager boards where
actually the r8a7790 SoC is installed.

All secondary CPUs will be switched to a non-secure HYP mode
after booting.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 arch/arm/mach-rmobile/Kconfig.32   |   2 +
 arch/arm/mach-rmobile/Makefile     |   6 +
 arch/arm/mach-rmobile/pm-r8a7790.c | 408 +++++++++++++++++++++++++++++++++++++
 arch/arm/mach-rmobile/pm-r8a7790.h |  72 +++++++
 arch/arm/mach-rmobile/psci.c       | 193 ++++++++++++++++++
 include/configs/lager.h            |   2 +
 include/configs/stout.h            |   2 +
 7 files changed, 685 insertions(+)
 create mode 100644 arch/arm/mach-rmobile/pm-r8a7790.c
 create mode 100644 arch/arm/mach-rmobile/pm-r8a7790.h
 create mode 100644 arch/arm/mach-rmobile/psci.c

diff --git a/arch/arm/mach-rmobile/Kconfig.32 b/arch/arm/mach-rmobile/Kconfig.32
index a2e9e3d..728c323 100644
--- a/arch/arm/mach-rmobile/Kconfig.32
+++ b/arch/arm/mach-rmobile/Kconfig.32
@@ -78,6 +78,7 @@ config TARGET_LAGER
 	imply CMD_DM
 	select CPU_V7_HAS_NONSEC
 	select CPU_V7_HAS_VIRT
+	select ARCH_SUPPORT_PSCI
 
 config TARGET_KZM9G
 	bool "KZM9D board"
@@ -119,6 +120,7 @@ config TARGET_STOUT
 	imply CMD_DM
 	select CPU_V7_HAS_NONSEC
 	select CPU_V7_HAS_VIRT
+	select ARCH_SUPPORT_PSCI
 
 endchoice
 
diff --git a/arch/arm/mach-rmobile/Makefile b/arch/arm/mach-rmobile/Makefile
index 1f26ada..6f4c513 100644
--- a/arch/arm/mach-rmobile/Makefile
+++ b/arch/arm/mach-rmobile/Makefile
@@ -13,3 +13,9 @@ obj-$(CONFIG_SH73A0) += lowlevel_init.o cpu_info-sh73a0.o pfc-sh73a0.o
 obj-$(CONFIG_R8A7740) += lowlevel_init.o cpu_info-r8a7740.o pfc-r8a7740.o
 obj-$(CONFIG_RCAR_GEN2) += lowlevel_init_ca15.o cpu_info-rcar.o
 obj-$(CONFIG_RCAR_GEN3) += lowlevel_init_gen3.o cpu_info-rcar.o memmap-gen3.o
+
+ifndef CONFIG_SPL_BUILD
+ifdef CONFIG_R8A7790
+obj-$(CONFIG_ARMV7_PSCI) += psci.o pm-r8a7790.o
+endif
+endif
diff --git a/arch/arm/mach-rmobile/pm-r8a7790.c b/arch/arm/mach-rmobile/pm-r8a7790.c
new file mode 100644
index 0000000..c635cf6
--- /dev/null
+++ b/arch/arm/mach-rmobile/pm-r8a7790.c
@@ -0,0 +1,408 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * CPU power management support for Renesas r8a7790 SoC
+ *
+ * Contains functions to control ARM Cortex A15/A7 cores and
+ * related peripherals basically used for PSCI.
+ *
+ * Copyright (C) 2018 EPAM Systems Inc.
+ *
+ * Mainly based on Renesas R-Car Gen2 platform code from Linux:
+ *    arch/arm/mach-shmobile/...
+ */
+
+#include <common.h>
+#include <asm/secure.h>
+#include <asm/io.h>
+
+#include "pm-r8a7790.h"
+
+/*****************************************************************************
+ * APMU definitions
+ *****************************************************************************/
+#define CA15_APMU_BASE	0xe6152000
+#define CA7_APMU_BASE	0xe6151000
+
+/* Wake Up Control Register */
+#define WUPCR_OFFS		0x10
+/* Power Status Register */
+#define PSTR_OFFS		0x40
+/* CPUn Power Status Control Register */
+#define CPUNCR_OFFS(n)	(0x100 + (0x10 * (n)))
+
+#define CPUPWR_STANDBY	0x3
+
+/* Debug Resource Reset Control Register */
+#define DBGRCR_OFFS		0x180
+
+#define DBGCPUREN		BIT(24)	/* CPU Other Reset Req Enable */
+#define DBGCPUNREN(n)	BIT((n) + 20)	/* CPUn Reset Req Enable */
+#define DBGCPUPREN		BIT(19)	/* CPU Peripheral Reset Req Enable */
+
+/*****************************************************************************
+ * RST definitions
+ *****************************************************************************/
+#define RST_BASE	0xe6160000
+
+/* Boot Address Registers */
+#define CA15BAR		0x20
+#define CA7BAR		0x30
+
+/* Reset Control Registers */
+#define CA15RESCNT	0x40
+#define CA7RESCNT	0x44
+
+#define CA15RESCNT_CODE	0xa5a50000
+#define CA7RESCNT_CODE	0x5a5a0000
+
+/* SYS Boot Address Register */
+#define SBAR_BAREN	BIT(4)	/* SBAR is valid */
+
+/* Watchdog Timer Reset Control Register */
+#define WDTRSTCR		0x54
+
+#define WDTRSTCR_CODE	0xa55a0000
+
+/*****************************************************************************
+ * SYSC definitions
+ *****************************************************************************/
+#define SYSC_BASE	0xe6180000
+
+/* SYSC Status Register */
+#define SYSCSR		0x00
+/* Interrupt Status Register */
+#define SYSCISR		0x04
+/* Interrupt Status Clear Register */
+#define SYSCISCR	0x08
+/* Interrupt Enable Register */
+#define SYSCIER		0x0c
+/* Interrupt Mask Register */
+#define SYSCIMR		0x10
+
+/* Power Status Register */
+#define PWRSR_OFFS		0x00
+/* Power Resume Control Register */
+#define PWRONCR_OFFS	0x0c
+/* Power Shutoff/Resume Error Register */
+#define PWRER_OFFS		0x14
+
+/* PWRSR5 .. PWRER5 */
+#define CA15_SCU_CHAN_OFFS	0x180
+/* PWRSR3 .. PWRER3 */
+#define CA7_SCU_CHAN_OFFS	0x100
+
+#define CA15_SCU_ISR_BIT	12
+#define CA7_SCU_ISR_BIT		21
+
+#define SYSCSR_RETRIES		100
+#define SYSCSR_DELAY_US		1
+
+#define PWRER_RETRIES		100
+#define PWRER_DELAY_US		1
+
+#define SYSCISR_RETRIES		1000
+#define SYSCISR_DELAY_US	1
+
+#define CA15_SCU	0
+#define CA7_SCU		1
+
+/*****************************************************************************
+ * CCI-400 definitions
+ *****************************************************************************/
+#define CCI_BASE		0xf0090000
+#define CCI_SLAVE3		0x4000
+#define CCI_SLAVE4		0x5000
+#define CCI_SNOOP		0x0000
+#define CCI_STATUS		0x000c
+#define CCI_ENABLE_REQ	0x0003
+
+/*****************************************************************************
+ * RWDT definitions
+ *****************************************************************************/
+/* Watchdog Timer Counter Register */
+#define RWTCNT		0x0
+/* Watchdog Timer Control/Status Register A */
+#define RWTCSRA		0x4
+
+#define RWTCNT_CODE		0x5a5a0000
+#define RWTCSRA_CODE	0xa5a5a500
+
+#define RWTCSRA_WRFLG	BIT(5)
+#define RWTCSRA_TME		BIT(7)
+
+/*****************************************************************************
+ * Other definitions
+ *****************************************************************************/
+/* On-chip RAM */
+#define ICRAM1	0xe63c0000	/* Inter Connect RAM1 (4 KiB) */
+
+/* On-chip ROM */
+#define BOOTROM	0xe6340000
+
+/*****************************************************************************
+ * Functions which intended to be called from PSCI handlers. These functions
+ * marked as __secure and are placed in .secure section.
+ *****************************************************************************/
+void __secure r8a7790_apmu_power_on(int cpu)
+{
+	int cluster = r8a7790_cluster_id(cpu);
+	u32 apmu_base;
+
+	apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
+
+	/* Request power on */
+	writel(BIT(r8a7790_core_id(cpu)), apmu_base + WUPCR_OFFS);
+
+	/* Wait for APMU to finish */
+	while (readl(apmu_base + WUPCR_OFFS))
+		;
+}
+
+void __secure r8a7790_apmu_power_off(int cpu)
+{
+	int cluster = r8a7790_cluster_id(cpu);
+	u32 apmu_base;
+
+	apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
+
+	/* Request Core Standby for next WFI */
+	writel(CPUPWR_STANDBY, apmu_base + CPUNCR_OFFS(r8a7790_core_id(cpu)));
+}
+
+void __secure r8a7790_assert_reset(int cpu)
+{
+	int cluster = r8a7790_cluster_id(cpu);
+	u32 mask, magic, rescnt;
+
+	mask = BIT(3 - r8a7790_core_id(cpu));
+	magic = cluster == 0 ? CA15RESCNT_CODE : CA7RESCNT_CODE;
+	rescnt = RST_BASE + (cluster == 0 ? CA15RESCNT : CA7RESCNT);
+	writel((readl(rescnt) | mask) | magic, rescnt);
+}
+
+void __secure r8a7790_deassert_reset(int cpu)
+{
+	int cluster = r8a7790_cluster_id(cpu);
+	u32 mask, magic, rescnt;
+
+	mask = BIT(3 - r8a7790_core_id(cpu));
+	magic = cluster == 0 ? CA15RESCNT_CODE : CA7RESCNT_CODE;
+	rescnt = RST_BASE + (cluster == 0 ? CA15RESCNT : CA7RESCNT);
+	writel((readl(rescnt) & ~mask) | magic, rescnt);
+}
+
+void __secure r8a7790_system_reset(void)
+{
+	u32 bar;
+
+	/*
+	 * Before configuring internal watchdog timer (RWDT) to reboot system
+	 * we need to re-program BAR registers for the boot CPU to jump to
+	 * bootrom code. Without taking care of, the boot CPU will jump to
+	 * the reset vector previously installed in ICRAM1, since BAR registers
+	 * keep their values after watchdog triggered reset.
+	 */
+	bar = (BOOTROM >> 8) & 0xfffffc00;
+	writel(bar, RST_BASE + CA15BAR);
+	writel(bar | SBAR_BAREN, RST_BASE + CA15BAR);
+	writel(bar, RST_BASE + CA7BAR);
+	writel(bar | SBAR_BAREN, RST_BASE + CA7BAR);
+	dsb();
+
+	/* Now, configure watchdog timer to reboot the system */
+
+	/* Trigger reset when counter overflows */
+	writel(WDTRSTCR_CODE | 0x2, RST_BASE + WDTRSTCR);
+	dsb();
+
+	/* Stop counter */
+	writel(RWTCSRA_CODE, RWDT_BASE + RWTCSRA);
+
+	/* Initialize counter with the highest value  */
+	writel(RWTCNT_CODE | 0xffff, RWDT_BASE + RWTCNT);
+
+	while (readb(RWDT_BASE + RWTCSRA) & RWTCSRA_WRFLG)
+		;
+
+	/* Start counter */
+	writel(RWTCSRA_CODE | RWTCSRA_TME, RWDT_BASE + RWTCSRA);
+}
+
+/*****************************************************************************
+ * Functions which intended to be called from PSCI board initialization.
+ *****************************************************************************/
+static int sysc_power_up(int scu)
+{
+	u32 status, chan_offs, isr_bit;
+	int i, j, ret = 0;
+
+	if (scu == CA15_SCU) {
+		chan_offs = CA15_SCU_CHAN_OFFS;
+		isr_bit = CA15_SCU_ISR_BIT;
+	} else {
+		chan_offs = CA7_SCU_CHAN_OFFS;
+		isr_bit = CA7_SCU_ISR_BIT;
+	}
+
+	writel(BIT(isr_bit), SYSC_BASE + SYSCISCR);
+
+	/* Submit power resume request until it was accepted */
+	for (i = 0; i < PWRER_RETRIES; i++) {
+		/* Wait until SYSC is ready to accept a power resume request */
+		for (j = 0; j < SYSCSR_RETRIES; j++) {
+			if (readl(SYSC_BASE + SYSCSR) & BIT(1))
+				break;
+
+			udelay(SYSCSR_DELAY_US);
+		}
+
+		if (j == SYSCSR_RETRIES)
+			return -EAGAIN;
+
+		/* Submit power resume request */
+		writel(BIT(0), SYSC_BASE + chan_offs + PWRONCR_OFFS);
+
+		/* Check if power resume request was accepted */
+		status = readl(SYSC_BASE + chan_offs + PWRER_OFFS);
+		if (!(status & BIT(0)))
+			break;
+
+		udelay(PWRER_DELAY_US);
+	}
+
+	if (i == PWRER_RETRIES)
+		return -EIO;
+
+	/* Wait until the power resume request has completed */
+	for (i = 0; i < SYSCISR_RETRIES; i++) {
+		if (readl(SYSC_BASE + SYSCISR) & BIT(isr_bit))
+			break;
+		udelay(SYSCISR_DELAY_US);
+	}
+
+	if (i == SYSCISR_RETRIES)
+		ret = -EIO;
+
+	writel(BIT(isr_bit), SYSC_BASE + SYSCISCR);
+
+	return ret;
+}
+
+static bool sysc_power_is_off(int scu)
+{
+	u32 status, chan_offs;
+
+	chan_offs = scu == CA15_SCU ? CA15_SCU_CHAN_OFFS : CA7_SCU_CHAN_OFFS;
+
+	/* Check if SCU is in power shutoff state */
+	status = readl(SYSC_BASE + chan_offs + PWRSR_OFFS);
+	if (status & BIT(0))
+		return true;
+
+	return false;
+}
+
+static void apmu_setup_debug_mode(int cpu)
+{
+	int cluster = r8a7790_cluster_id(cpu);
+	u32 apmu_base, reg;
+
+	apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
+
+	/*
+	 * Enable reset requests derived from power shutoff to the AP-system
+	 * CPU cores in debug mode. Without taking care of, they may fail to
+	 * resume from the power shutoff state.
+	 */
+	reg = readl(apmu_base + DBGRCR_OFFS);
+	reg |= DBGCPUREN | DBGCPUNREN(r8a7790_core_id(cpu)) | DBGCPUPREN;
+	writel(reg, apmu_base + DBGRCR_OFFS);
+}
+
+/*
+ * Reset vector for secondary CPUs.
+ * This will be mapped@address 0 by SBAR register.
+ * We need _long_ jump to the physical address.
+ */
+asm("	.arm\n"
+	"	.align 12\n"
+	"	.globl shmobile_boot_vector\n"
+	"shmobile_boot_vector:\n"
+	"	ldr r1, 1f\n"
+	"	bx	r1\n"
+	"	.type shmobile_boot_vector, %function\n"
+	"	.size shmobile_boot_vector, .-shmobile_boot_vector\n"
+	"	.align	2\n"
+	"	.globl	shmobile_boot_fn\n"
+	"shmobile_boot_fn:\n"
+	"1:	.space	4\n"
+	"	.globl	shmobile_boot_size\n"
+	"shmobile_boot_size:\n"
+	"	.long	.-shmobile_boot_vector\n");
+
+extern void shmobile_boot_vector(void);
+extern unsigned long shmobile_boot_fn;
+extern unsigned long shmobile_boot_size;
+
+void r8a7790_prepare_cpus(unsigned long addr, u32 nr_cpus)
+{
+	int cpu = get_current_cpu();
+	int i, ret = 0;
+	u32 bar;
+
+	shmobile_boot_fn = addr;
+	dsb();
+
+	/* Install reset vector */
+	memcpy_toio((void __iomem *)ICRAM1, shmobile_boot_vector,
+		    shmobile_boot_size);
+	dmb();
+
+	/* Setup reset vectors */
+	bar = (ICRAM1 >> 8) & 0xfffffc00;
+	writel(bar, RST_BASE + CA15BAR);
+	writel(bar | SBAR_BAREN, RST_BASE + CA15BAR);
+	writel(bar, RST_BASE + CA7BAR);
+	writel(bar | SBAR_BAREN, RST_BASE + CA7BAR);
+	dsb();
+
+	/* Perform setup for debug mode for all CPUs */
+	for (i = 0; i < nr_cpus; i++)
+		apmu_setup_debug_mode(i);
+
+	/*
+	 * Indicate the completion status of power shutoff/resume procedure
+	 * for CA15/CA7 SCU.
+	 */
+	writel(BIT(CA15_SCU_ISR_BIT) | BIT(CA7_SCU_ISR_BIT),
+	       SYSC_BASE + SYSCIER);
+
+	/* Power on CA15/CA7 SCU */
+	if (sysc_power_is_off(CA15_SCU))
+		ret += sysc_power_up(CA15_SCU);
+	if (sysc_power_is_off(CA7_SCU))
+		ret += sysc_power_up(CA7_SCU);
+	if (ret)
+		printf("warning: some of secondary CPUs may not boot\n");
+
+	/* Keep secondary CPUs in reset */
+	for (i = 0; i < nr_cpus; i++) {
+		/* Make sure that we don't reset boot CPU */
+		if (i == cpu)
+			continue;
+
+		r8a7790_assert_reset(i);
+	}
+
+	/*
+	 * Enable snoop requests and DVM message requests for slave interfaces
+	 * S4 (CA7) and S3 (CA15).
+	 */
+	writel(readl(CCI_BASE + CCI_SLAVE3 + CCI_SNOOP) | CCI_ENABLE_REQ,
+	       CCI_BASE + CCI_SLAVE3 + CCI_SNOOP);
+	writel(readl(CCI_BASE + CCI_SLAVE4 + CCI_SNOOP) | CCI_ENABLE_REQ,
+	       CCI_BASE + CCI_SLAVE4 + CCI_SNOOP);
+	/* Wait for pending bit low */
+	while (readl(CCI_BASE + CCI_STATUS))
+		;
+}
diff --git a/arch/arm/mach-rmobile/pm-r8a7790.h b/arch/arm/mach-rmobile/pm-r8a7790.h
new file mode 100644
index 0000000..f649dd8
--- /dev/null
+++ b/arch/arm/mach-rmobile/pm-r8a7790.h
@@ -0,0 +1,72 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 EPAM Systems Inc.
+ */
+
+#ifndef __PM_R8A7790_H__
+#define __PM_R8A7790_H__
+
+#include <linux/types.h>
+
+void r8a7790_apmu_power_on(int cpu);
+void r8a7790_apmu_power_off(int cpu);
+void r8a7790_assert_reset(int cpu);
+void r8a7790_deassert_reset(int cpu);
+
+void r8a7790_prepare_cpus(unsigned long addr, u32 nr_cpus);
+void r8a7790_system_reset(void);
+
+#define R8A7790_NR_CLUSTERS				2
+#define R8A7790_NR_CPUS_PER_CLUSTER		4
+
+/* Convert linear CPU index to core/cluster ID */
+#define r8a7790_cluster_id(cpu)	((cpu) / R8A7790_NR_CPUS_PER_CLUSTER)
+#define r8a7790_core_id(cpu)	((cpu) % R8A7790_NR_CPUS_PER_CLUSTER)
+
+#define MPIDR_AFFLVL_MASK	GENMASK(7, 0)
+#define MPIDR_AFF0_SHIFT	0
+#define MPIDR_AFF1_SHIFT	8
+
+/* All functions are inline so that they can be called from .secure section. */
+
+/*
+ * Convert CPU ID in MPIDR format to linear CPU index.
+ *
+ * Below the possible CPU IDs and corresponding CPU indexes:
+ * CPU ID       CPU index
+ * 0x80000000 - 0x00000000
+ * 0x80000001 - 0x00000001
+ * 0x80000002 - 0x00000002
+ * 0x80000003 - 0x00000003
+ * 0x80000100 - 0x00000004
+ * 0x80000101 - 0x00000005
+ * 0x80000102 - 0x00000006
+ * 0x80000103 - 0x00000007
+ */
+static inline int mpidr_to_cpu_index(u32 mpidr)
+{
+	u32 cluster_id, cpu_id;
+
+	cluster_id = (mpidr >> MPIDR_AFF1_SHIFT) & MPIDR_AFFLVL_MASK;
+	cpu_id = (mpidr >> MPIDR_AFF0_SHIFT) & MPIDR_AFFLVL_MASK;
+
+	if (cluster_id >= R8A7790_NR_CLUSTERS)
+		return -1;
+
+	if (cpu_id >= R8A7790_NR_CPUS_PER_CLUSTER)
+		return -1;
+
+	return (cpu_id + (cluster_id * R8A7790_NR_CPUS_PER_CLUSTER));
+}
+
+/* Return an index of the CPU which performs this call */
+static inline int get_current_cpu(void)
+{
+	u32 mpidr;
+
+	asm volatile("mrc p15, 0, %0, c0, c0, 5" : "=r"(mpidr));
+
+	return mpidr_to_cpu_index(mpidr);
+}
+
+#endif /* __PM_R8A7790_H__ */
diff --git a/arch/arm/mach-rmobile/psci.c b/arch/arm/mach-rmobile/psci.c
new file mode 100644
index 0000000..95850b8
--- /dev/null
+++ b/arch/arm/mach-rmobile/psci.c
@@ -0,0 +1,193 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * This file implements basic PSCI support for Renesas r8a7790 SoC
+ *
+ * Copyright (C) 2018 EPAM Systems Inc.
+ *
+ * Based on arch/arm/mach-uniphier/arm32/psci.c
+ */
+
+#include <common.h>
+#include <linux/psci.h>
+#include <asm/io.h>
+#include <asm/psci.h>
+#include <asm/secure.h>
+
+#include "pm-r8a7790.h"
+
+#define R8A7790_PSCI_NR_CPUS	8
+#if R8A7790_PSCI_NR_CPUS > CONFIG_ARMV7_PSCI_NR_CPUS
+#error "invalid value for CONFIG_ARMV7_PSCI_NR_CPUS"
+#endif
+
+#define GICC_CTLR_OFFSET	0x2000
+
+/*
+ * The boot CPU is powered on by default, but it's index is not a const
+ * value. An index the boot CPU has, depends on whether it is CA15 (index 0)
+ * or CA7 (index 4).
+ * So, we update state for the boot CPU during PSCI board initialization.
+ */
+u8 psci_state[R8A7790_PSCI_NR_CPUS] __secure_data = {
+		PSCI_AFFINITY_LEVEL_OFF,
+		PSCI_AFFINITY_LEVEL_OFF,
+		PSCI_AFFINITY_LEVEL_OFF,
+		PSCI_AFFINITY_LEVEL_OFF,
+		PSCI_AFFINITY_LEVEL_OFF,
+		PSCI_AFFINITY_LEVEL_OFF,
+		PSCI_AFFINITY_LEVEL_OFF,
+		PSCI_AFFINITY_LEVEL_OFF};
+
+void __secure psci_set_state(int cpu, u8 state)
+{
+	psci_state[cpu] = state;
+	dsb();
+	isb();
+}
+
+u32 __secure psci_get_cpu_id(void)
+{
+	return get_current_cpu();
+}
+
+void __secure psci_arch_cpu_entry(void)
+{
+	int cpu = get_current_cpu();
+
+	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON);
+}
+
+int __secure psci_features(u32 function_id, u32 psci_fid)
+{
+	switch (psci_fid) {
+	case ARM_PSCI_0_2_FN_PSCI_VERSION:
+	case ARM_PSCI_0_2_FN_CPU_OFF:
+	case ARM_PSCI_0_2_FN_CPU_ON:
+	case ARM_PSCI_0_2_FN_AFFINITY_INFO:
+	case ARM_PSCI_0_2_FN_MIGRATE_INFO_TYPE:
+	case ARM_PSCI_0_2_FN_SYSTEM_OFF:
+	case ARM_PSCI_0_2_FN_SYSTEM_RESET:
+		return 0x0;
+	}
+
+	return ARM_PSCI_RET_NI;
+}
+
+u32 __secure psci_version(u32 function_id)
+{
+	return ARM_PSCI_VER_1_0;
+}
+
+int __secure psci_affinity_info(u32 function_id, u32 target_affinity,
+				u32 lowest_affinity_level)
+{
+	int cpu;
+
+	if (lowest_affinity_level > 0)
+		return ARM_PSCI_RET_INVAL;
+
+	cpu = mpidr_to_cpu_index(target_affinity);
+	if (cpu == -1)
+		return ARM_PSCI_RET_INVAL;
+
+	/* TODO flush cache */
+	return psci_state[cpu];
+}
+
+int __secure psci_migrate_info_type(u32 function_id)
+{
+	/* Trusted OS is either not present or does not require migration */
+	return 2;
+}
+
+int __secure psci_cpu_on(u32 function_id, u32 target_cpu, u32 entry_point,
+			 u32 context_id)
+{
+	int cpu;
+
+	cpu = mpidr_to_cpu_index(target_cpu);
+	if (cpu == -1)
+		return ARM_PSCI_RET_INVAL;
+
+	if (psci_state[cpu] == PSCI_AFFINITY_LEVEL_ON)
+		return ARM_PSCI_RET_ALREADY_ON;
+
+	if (psci_state[cpu] == PSCI_AFFINITY_LEVEL_ON_PENDING)
+		return ARM_PSCI_RET_ON_PENDING;
+
+	psci_save(cpu, entry_point, context_id);
+
+	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON_PENDING);
+
+	r8a7790_assert_reset(cpu);
+	r8a7790_apmu_power_on(cpu);
+	r8a7790_deassert_reset(cpu);
+
+	return ARM_PSCI_RET_SUCCESS;
+}
+
+int __secure psci_cpu_off(void)
+{
+	int cpu = get_current_cpu();
+
+	/*
+	 * Place the CPU interface in a state where it can never make a CPU
+	 * exit WFI as result of an asserted interrupt.
+	 */
+	writel(0, CONFIG_ARM_GIC_BASE_ADDRESS + GICC_CTLR_OFFSET);
+	dsb();
+
+	/* Select next sleep mode using the APMU */
+	r8a7790_apmu_power_off(cpu);
+
+	/* Do ARM specific CPU shutdown */
+	psci_cpu_off_common();
+
+	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_OFF);
+
+	/* Drain the WB before WFI */
+	dsb();
+
+	while (1)
+		wfi();
+}
+
+void __secure psci_system_reset(u32 function_id)
+{
+	r8a7790_system_reset();
+
+	/* Drain the WB before WFI */
+	dsb();
+
+	/* The system is about to be rebooted, so just waiting for this */
+	while (1)
+		wfi();
+}
+
+void __secure psci_system_off(u32 function_id)
+{
+	/* Drain the WB before WFI */
+	dsb();
+
+	/*
+	 * System Off is not implemented yet, so waiting for powering off
+	 * manually
+	 */
+	while (1)
+		wfi();
+}
+
+void psci_board_init(void)
+{
+	int cpu = get_current_cpu();
+
+	/* Update state for the boot CPU */
+	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON);
+
+	/*
+	 * Perform needed actions for the secondary CPUs to be ready
+	 * for powering on
+	 */
+	r8a7790_prepare_cpus((unsigned long)psci_cpu_entry,
+			     R8A7790_PSCI_NR_CPUS);
+}
diff --git a/include/configs/lager.h b/include/configs/lager.h
index d8a0b01..d70c147 100644
--- a/include/configs/lager.h
+++ b/include/configs/lager.h
@@ -51,4 +51,6 @@
 /* The PERIPHBASE in the CBAR register is wrong, so override it */
 #define CONFIG_ARM_GIC_BASE_ADDRESS		0xf1000000
 
+#define CONFIG_ARMV7_PSCI_1_0
+
 #endif	/* __LAGER_H */
diff --git a/include/configs/stout.h b/include/configs/stout.h
index 7edb9d7..0b20075 100644
--- a/include/configs/stout.h
+++ b/include/configs/stout.h
@@ -59,4 +59,6 @@
 /* The PERIPHBASE in the CBAR register is wrong, so override it */
 #define CONFIG_ARM_GIC_BASE_ADDRESS		0xf1000000
 
+#define CONFIG_ARMV7_PSCI_1_0
+
 #endif	/* __STOUT_H */
-- 
2.7.4

[U-Boot] [PATCH 2/3] ARM: rmobile: Add basic PSCI support for r8a7790 SoC

[Marek Vasut]

On 1/31/19 6:38 PM, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Also enable PSCI support for Stout and Lager boards where
> actually the r8a7790 SoC is installed.
> 
> All secondary CPUs will be switched to a non-secure HYP mode
> after booting.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  arch/arm/mach-rmobile/Kconfig.32   |   2 +
>  arch/arm/mach-rmobile/Makefile     |   6 +
>  arch/arm/mach-rmobile/pm-r8a7790.c | 408 +++++++++++++++++++++++++++++++++++++
>  arch/arm/mach-rmobile/pm-r8a7790.h |  72 +++++++
>  arch/arm/mach-rmobile/psci.c       | 193 ++++++++++++++++++
>  include/configs/lager.h            |   2 +
>  include/configs/stout.h            |   2 +
>  7 files changed, 685 insertions(+)
>  create mode 100644 arch/arm/mach-rmobile/pm-r8a7790.c
>  create mode 100644 arch/arm/mach-rmobile/pm-r8a7790.h
>  create mode 100644 arch/arm/mach-rmobile/psci.c
> 
> diff --git a/arch/arm/mach-rmobile/Kconfig.32 b/arch/arm/mach-rmobile/Kconfig.32
> index a2e9e3d..728c323 100644
> --- a/arch/arm/mach-rmobile/Kconfig.32
> +++ b/arch/arm/mach-rmobile/Kconfig.32
> @@ -78,6 +78,7 @@ config TARGET_LAGER
>  	imply CMD_DM
>  	select CPU_V7_HAS_NONSEC
>  	select CPU_V7_HAS_VIRT
> +	select ARCH_SUPPORT_PSCI
>  
>  config TARGET_KZM9G
>  	bool "KZM9D board"
> @@ -119,6 +120,7 @@ config TARGET_STOUT
>  	imply CMD_DM
>  	select CPU_V7_HAS_NONSEC
>  	select CPU_V7_HAS_VIRT
> +	select ARCH_SUPPORT_PSCI
>  
>  endchoice
>  
> diff --git a/arch/arm/mach-rmobile/Makefile b/arch/arm/mach-rmobile/Makefile
> index 1f26ada..6f4c513 100644
> --- a/arch/arm/mach-rmobile/Makefile
> +++ b/arch/arm/mach-rmobile/Makefile
> @@ -13,3 +13,9 @@ obj-$(CONFIG_SH73A0) += lowlevel_init.o cpu_info-sh73a0.o pfc-sh73a0.o
>  obj-$(CONFIG_R8A7740) += lowlevel_init.o cpu_info-r8a7740.o pfc-r8a7740.o
>  obj-$(CONFIG_RCAR_GEN2) += lowlevel_init_ca15.o cpu_info-rcar.o
>  obj-$(CONFIG_RCAR_GEN3) += lowlevel_init_gen3.o cpu_info-rcar.o memmap-gen3.o
> +
> +ifndef CONFIG_SPL_BUILD
> +ifdef CONFIG_R8A7790
> +obj-$(CONFIG_ARMV7_PSCI) += psci.o pm-r8a7790.o
> +endif
> +endif
> diff --git a/arch/arm/mach-rmobile/pm-r8a7790.c b/arch/arm/mach-rmobile/pm-r8a7790.c
> new file mode 100644
> index 0000000..c635cf6
> --- /dev/null
> +++ b/arch/arm/mach-rmobile/pm-r8a7790.c
> @@ -0,0 +1,408 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * CPU power management support for Renesas r8a7790 SoC
> + *
> + * Contains functions to control ARM Cortex A15/A7 cores and
> + * related peripherals basically used for PSCI.
> + *
> + * Copyright (C) 2018 EPAM Systems Inc.
> + *
> + * Mainly based on Renesas R-Car Gen2 platform code from Linux:
> + *    arch/arm/mach-shmobile/...
> + */
> +
> +#include <common.h>
> +#include <asm/secure.h>
> +#include <asm/io.h>
> +
> +#include "pm-r8a7790.h"
> +
> +/*****************************************************************************

I'd expect checkpatch to complain about these long lines of asterisks.

> + * APMU definitions
> + *****************************************************************************/
> +#define CA15_APMU_BASE	0xe6152000
> +#define CA7_APMU_BASE	0xe6151000

All these addresses should come from DT. And the driver should be DM
capable and live in drivers/

[...]

> +/*****************************************************************************
> + * Functions which intended to be called from PSCI handlers. These functions
> + * marked as __secure and are placed in .secure section.
> + *****************************************************************************/
> +void __secure r8a7790_apmu_power_on(int cpu)
> +{
> +	int cluster = r8a7790_cluster_id(cpu);
> +	u32 apmu_base;
> +
> +	apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
> +
> +	/* Request power on */
> +	writel(BIT(r8a7790_core_id(cpu)), apmu_base + WUPCR_OFFS);

wait_for_bit of some sorts ?

> +	/* Wait for APMU to finish */
> +	while (readl(apmu_base + WUPCR_OFFS))
> +		;

Can this spin forever ?

> +}
> +
> +void __secure r8a7790_apmu_power_off(int cpu)
> +{
> +	int cluster = r8a7790_cluster_id(cpu);
> +	u32 apmu_base;
> +
> +	apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
> +
> +	/* Request Core Standby for next WFI */
> +	writel(CPUPWR_STANDBY, apmu_base + CPUNCR_OFFS(r8a7790_core_id(cpu)));
> +}
> +
> +void __secure r8a7790_assert_reset(int cpu)
> +{
> +	int cluster = r8a7790_cluster_id(cpu);
> +	u32 mask, magic, rescnt;
> +
> +	mask = BIT(3 - r8a7790_core_id(cpu));
> +	magic = cluster == 0 ? CA15RESCNT_CODE : CA7RESCNT_CODE;
> +	rescnt = RST_BASE + (cluster == 0 ? CA15RESCNT : CA7RESCNT);
> +	writel((readl(rescnt) | mask) | magic, rescnt);
> +}
> +
> +void __secure r8a7790_deassert_reset(int cpu)
> +{
> +	int cluster = r8a7790_cluster_id(cpu);
> +	u32 mask, magic, rescnt;
> +
> +	mask = BIT(3 - r8a7790_core_id(cpu));
> +	magic = cluster == 0 ? CA15RESCNT_CODE : CA7RESCNT_CODE;
> +	rescnt = RST_BASE + (cluster == 0 ? CA15RESCNT : CA7RESCNT);
> +	writel((readl(rescnt) & ~mask) | magic, rescnt);
> +}
> +
> +void __secure r8a7790_system_reset(void)
> +{
> +	u32 bar;
> +
> +	/*
> +	 * Before configuring internal watchdog timer (RWDT) to reboot system
> +	 * we need to re-program BAR registers for the boot CPU to jump to
> +	 * bootrom code. Without taking care of, the boot CPU will jump to
> +	 * the reset vector previously installed in ICRAM1, since BAR registers
> +	 * keep their values after watchdog triggered reset.
> +	 */
> +	bar = (BOOTROM >> 8) & 0xfffffc00;
> +	writel(bar, RST_BASE + CA15BAR);
> +	writel(bar | SBAR_BAREN, RST_BASE + CA15BAR);
> +	writel(bar, RST_BASE + CA7BAR);
> +	writel(bar | SBAR_BAREN, RST_BASE + CA7BAR);
> +	dsb();
> +
> +	/* Now, configure watchdog timer to reboot the system */
> +
> +	/* Trigger reset when counter overflows */
> +	writel(WDTRSTCR_CODE | 0x2, RST_BASE + WDTRSTCR);
> +	dsb();
> +
> +	/* Stop counter */
> +	writel(RWTCSRA_CODE, RWDT_BASE + RWTCSRA);
> +
> +	/* Initialize counter with the highest value  */
> +	writel(RWTCNT_CODE | 0xffff, RWDT_BASE + RWTCNT);
> +
> +	while (readb(RWDT_BASE + RWTCSRA) & RWTCSRA_WRFLG)
> +		;
> +
> +	/* Start counter */
> +	writel(RWTCSRA_CODE | RWTCSRA_TME, RWDT_BASE + RWTCSRA);

This seems to reimplement the reset code that's in U-Boot already. Why ?

> +}
> +
> +/*****************************************************************************
> + * Functions which intended to be called from PSCI board initialization.
> + *****************************************************************************/
> +static int sysc_power_up(int scu)
> +{
> +	u32 status, chan_offs, isr_bit;
> +	int i, j, ret = 0;
> +
> +	if (scu == CA15_SCU) {
> +		chan_offs = CA15_SCU_CHAN_OFFS;
> +		isr_bit = CA15_SCU_ISR_BIT;
> +	} else {
> +		chan_offs = CA7_SCU_CHAN_OFFS;
> +		isr_bit = CA7_SCU_ISR_BIT;
> +	}
> +
> +	writel(BIT(isr_bit), SYSC_BASE + SYSCISCR);
> +
> +	/* Submit power resume request until it was accepted */
> +	for (i = 0; i < PWRER_RETRIES; i++) {
> +		/* Wait until SYSC is ready to accept a power resume request */
> +		for (j = 0; j < SYSCSR_RETRIES; j++) {
> +			if (readl(SYSC_BASE + SYSCSR) & BIT(1))
> +				break;
> +
> +			udelay(SYSCSR_DELAY_US);
> +		}
> +
> +		if (j == SYSCSR_RETRIES)
> +			return -EAGAIN;
> +
> +		/* Submit power resume request */
> +		writel(BIT(0), SYSC_BASE + chan_offs + PWRONCR_OFFS);
> +
> +		/* Check if power resume request was accepted */
> +		status = readl(SYSC_BASE + chan_offs + PWRER_OFFS);
> +		if (!(status & BIT(0)))
> +			break;
> +
> +		udelay(PWRER_DELAY_US);
> +	}
> +
> +	if (i == PWRER_RETRIES)
> +		return -EIO;
> +
> +	/* Wait until the power resume request has completed */
> +	for (i = 0; i < SYSCISR_RETRIES; i++) {
> +		if (readl(SYSC_BASE + SYSCISR) & BIT(isr_bit))
> +			break;
> +		udelay(SYSCISR_DELAY_US);
> +	}
> +
> +	if (i == SYSCISR_RETRIES)
> +		ret = -EIO;
> +
> +	writel(BIT(isr_bit), SYSC_BASE + SYSCISCR);
> +
> +	return ret;
> +}
> +
> +static bool sysc_power_is_off(int scu)
> +{
> +	u32 status, chan_offs;
> +
> +	chan_offs = scu == CA15_SCU ? CA15_SCU_CHAN_OFFS : CA7_SCU_CHAN_OFFS;
> +
> +	/* Check if SCU is in power shutoff state */
> +	status = readl(SYSC_BASE + chan_offs + PWRSR_OFFS);
> +	if (status & BIT(0))
> +		return true;
> +
> +	return false;
> +}
> +
> +static void apmu_setup_debug_mode(int cpu)
> +{
> +	int cluster = r8a7790_cluster_id(cpu);
> +	u32 apmu_base, reg;
> +
> +	apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
> +
> +	/*
> +	 * Enable reset requests derived from power shutoff to the AP-system
> +	 * CPU cores in debug mode. Without taking care of, they may fail to
> +	 * resume from the power shutoff state.
> +	 */
> +	reg = readl(apmu_base + DBGRCR_OFFS);
> +	reg |= DBGCPUREN | DBGCPUNREN(r8a7790_core_id(cpu)) | DBGCPUPREN;
> +	writel(reg, apmu_base + DBGRCR_OFFS);

setbits_le32()

> +}
> +
> +/*
> + * Reset vector for secondary CPUs.
> + * This will be mapped at address 0 by SBAR register.
> + * We need _long_ jump to the physical address.
> + */
> +asm("	.arm\n"
> +	"	.align 12\n"
> +	"	.globl shmobile_boot_vector\n"
> +	"shmobile_boot_vector:\n"
> +	"	ldr r1, 1f\n"
> +	"	bx	r1\n"
> +	"	.type shmobile_boot_vector, %function\n"
> +	"	.size shmobile_boot_vector, .-shmobile_boot_vector\n"
> +	"	.align	2\n"
> +	"	.globl	shmobile_boot_fn\n"
> +	"shmobile_boot_fn:\n"
> +	"1:	.space	4\n"
> +	"	.globl	shmobile_boot_size\n"
> +	"shmobile_boot_size:\n"
> +	"	.long	.-shmobile_boot_vector\n");

Why can't this be implemented in C ?

> +extern void shmobile_boot_vector(void);
> +extern unsigned long shmobile_boot_fn;
> +extern unsigned long shmobile_boot_size;

Are the externs necessary ?

> +void r8a7790_prepare_cpus(unsigned long addr, u32 nr_cpus)
> +{
> +	int cpu = get_current_cpu();
> +	int i, ret = 0;
> +	u32 bar;
> +
> +	shmobile_boot_fn = addr;
> +	dsb();
> +
> +	/* Install reset vector */
> +	memcpy_toio((void __iomem *)ICRAM1, shmobile_boot_vector,
> +		    shmobile_boot_size);
> +	dmb();

Does this take into consideration caches ?

> +	/* Setup reset vectors */
> +	bar = (ICRAM1 >> 8) & 0xfffffc00;
> +	writel(bar, RST_BASE + CA15BAR);
> +	writel(bar | SBAR_BAREN, RST_BASE + CA15BAR);
> +	writel(bar, RST_BASE + CA7BAR);
> +	writel(bar | SBAR_BAREN, RST_BASE + CA7BAR);
> +	dsb();
> +
> +	/* Perform setup for debug mode for all CPUs */
> +	for (i = 0; i < nr_cpus; i++)
> +		apmu_setup_debug_mode(i);
> +
> +	/*
> +	 * Indicate the completion status of power shutoff/resume procedure
> +	 * for CA15/CA7 SCU.
> +	 */
> +	writel(BIT(CA15_SCU_ISR_BIT) | BIT(CA7_SCU_ISR_BIT),
> +	       SYSC_BASE + SYSCIER);
> +
> +	/* Power on CA15/CA7 SCU */
> +	if (sysc_power_is_off(CA15_SCU))
> +		ret += sysc_power_up(CA15_SCU);
> +	if (sysc_power_is_off(CA7_SCU))
> +		ret += sysc_power_up(CA7_SCU);
> +	if (ret)
> +		printf("warning: some of secondary CPUs may not boot\n");

"some" is not very useful for debugging,.

> +	/* Keep secondary CPUs in reset */
> +	for (i = 0; i < nr_cpus; i++) {
> +		/* Make sure that we don't reset boot CPU */
> +		if (i == cpu)
> +			continue;
> +
> +		r8a7790_assert_reset(i);
> +	}
> +
> +	/*
> +	 * Enable snoop requests and DVM message requests for slave interfaces
> +	 * S4 (CA7) and S3 (CA15).
> +	 */
> +	writel(readl(CCI_BASE + CCI_SLAVE3 + CCI_SNOOP) | CCI_ENABLE_REQ,
> +	       CCI_BASE + CCI_SLAVE3 + CCI_SNOOP);
> +	writel(readl(CCI_BASE + CCI_SLAVE4 + CCI_SNOOP) | CCI_ENABLE_REQ,
> +	       CCI_BASE + CCI_SLAVE4 + CCI_SNOOP);
> +	/* Wait for pending bit low */
> +	while (readl(CCI_BASE + CCI_STATUS))
> +		;

can this spin forever ?

> +}
> diff --git a/arch/arm/mach-rmobile/pm-r8a7790.h b/arch/arm/mach-rmobile/pm-r8a7790.h
> new file mode 100644
> index 0000000..f649dd8
> --- /dev/null
> +++ b/arch/arm/mach-rmobile/pm-r8a7790.h
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 EPAM Systems Inc.
> + */
> +
> +#ifndef __PM_R8A7790_H__
> +#define __PM_R8A7790_H__
> +
> +#include <linux/types.h>
> +
> +void r8a7790_apmu_power_on(int cpu);
> +void r8a7790_apmu_power_off(int cpu);
> +void r8a7790_assert_reset(int cpu);
> +void r8a7790_deassert_reset(int cpu);
> +
> +void r8a7790_prepare_cpus(unsigned long addr, u32 nr_cpus);
> +void r8a7790_system_reset(void);
> +
> +#define R8A7790_NR_CLUSTERS				2
> +#define R8A7790_NR_CPUS_PER_CLUSTER		4
> +
> +/* Convert linear CPU index to core/cluster ID */
> +#define r8a7790_cluster_id(cpu)	((cpu) / R8A7790_NR_CPUS_PER_CLUSTER)
> +#define r8a7790_core_id(cpu)	((cpu) % R8A7790_NR_CPUS_PER_CLUSTER)
> +
> +#define MPIDR_AFFLVL_MASK	GENMASK(7, 0)
> +#define MPIDR_AFF0_SHIFT	0
> +#define MPIDR_AFF1_SHIFT	8
> +
> +/* All functions are inline so that they can be called from .secure section. */
> +
> +/*
> + * Convert CPU ID in MPIDR format to linear CPU index.
> + *
> + * Below the possible CPU IDs and corresponding CPU indexes:
> + * CPU ID       CPU index
> + * 0x80000000 - 0x00000000
> + * 0x80000001 - 0x00000001
> + * 0x80000002 - 0x00000002
> + * 0x80000003 - 0x00000003
> + * 0x80000100 - 0x00000004
> + * 0x80000101 - 0x00000005
> + * 0x80000102 - 0x00000006
> + * 0x80000103 - 0x00000007
> + */
> +static inline int mpidr_to_cpu_index(u32 mpidr)
> +{
> +	u32 cluster_id, cpu_id;
> +
> +	cluster_id = (mpidr >> MPIDR_AFF1_SHIFT) & MPIDR_AFFLVL_MASK;
> +	cpu_id = (mpidr >> MPIDR_AFF0_SHIFT) & MPIDR_AFFLVL_MASK;
> +
> +	if (cluster_id >= R8A7790_NR_CLUSTERS)
> +		return -1;
> +
> +	if (cpu_id >= R8A7790_NR_CPUS_PER_CLUSTER)
> +		return -1;
> +
> +	return (cpu_id + (cluster_id * R8A7790_NR_CPUS_PER_CLUSTER));
> +}
> +
> +/* Return an index of the CPU which performs this call */
> +static inline int get_current_cpu(void)
> +{
> +	u32 mpidr;
> +
> +	asm volatile("mrc p15, 0, %0, c0, c0, 5" : "=r"(mpidr));
> +
> +	return mpidr_to_cpu_index(mpidr);
> +}
> +
> +#endif /* __PM_R8A7790_H__ */
> diff --git a/arch/arm/mach-rmobile/psci.c b/arch/arm/mach-rmobile/psci.c
> new file mode 100644
> index 0000000..95850b8
> --- /dev/null
> +++ b/arch/arm/mach-rmobile/psci.c
> @@ -0,0 +1,193 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * This file implements basic PSCI support for Renesas r8a7790 SoC
> + *
> + * Copyright (C) 2018 EPAM Systems Inc.
> + *
> + * Based on arch/arm/mach-uniphier/arm32/psci.c
> + */
> +
> +#include <common.h>
> +#include <linux/psci.h>
> +#include <asm/io.h>
> +#include <asm/psci.h>
> +#include <asm/secure.h>
> +
> +#include "pm-r8a7790.h"
> +
> +#define R8A7790_PSCI_NR_CPUS	8
> +#if R8A7790_PSCI_NR_CPUS > CONFIG_ARMV7_PSCI_NR_CPUS
> +#error "invalid value for CONFIG_ARMV7_PSCI_NR_CPUS"
> +#endif
> +
> +#define GICC_CTLR_OFFSET	0x2000
> +
> +/*
> + * The boot CPU is powered on by default, but it's index is not a const
> + * value. An index the boot CPU has, depends on whether it is CA15 (index 0)
> + * or CA7 (index 4).
> + * So, we update state for the boot CPU during PSCI board initialization.
> + */
> +u8 psci_state[R8A7790_PSCI_NR_CPUS] __secure_data = {
> +		PSCI_AFFINITY_LEVEL_OFF,
> +		PSCI_AFFINITY_LEVEL_OFF,
> +		PSCI_AFFINITY_LEVEL_OFF,
> +		PSCI_AFFINITY_LEVEL_OFF,
> +		PSCI_AFFINITY_LEVEL_OFF,
> +		PSCI_AFFINITY_LEVEL_OFF,
> +		PSCI_AFFINITY_LEVEL_OFF,
> +		PSCI_AFFINITY_LEVEL_OFF};
> +
> +void __secure psci_set_state(int cpu, u8 state)
> +{
> +	psci_state[cpu] = state;
> +	dsb();
> +	isb();
> +}
> +
> +u32 __secure psci_get_cpu_id(void)
> +{
> +	return get_current_cpu();
> +}
> +
> +void __secure psci_arch_cpu_entry(void)
> +{
> +	int cpu = get_current_cpu();
> +
> +	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON);
> +}
> +
> +int __secure psci_features(u32 function_id, u32 psci_fid)
> +{
> +	switch (psci_fid) {
> +	case ARM_PSCI_0_2_FN_PSCI_VERSION:
> +	case ARM_PSCI_0_2_FN_CPU_OFF:
> +	case ARM_PSCI_0_2_FN_CPU_ON:
> +	case ARM_PSCI_0_2_FN_AFFINITY_INFO:
> +	case ARM_PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> +	case ARM_PSCI_0_2_FN_SYSTEM_OFF:
> +	case ARM_PSCI_0_2_FN_SYSTEM_RESET:
> +		return 0x0;
> +	}
> +
> +	return ARM_PSCI_RET_NI;
> +}
> +
> +u32 __secure psci_version(u32 function_id)
> +{
> +	return ARM_PSCI_VER_1_0;
> +}
> +
> +int __secure psci_affinity_info(u32 function_id, u32 target_affinity,
> +				u32 lowest_affinity_level)
> +{
> +	int cpu;
> +
> +	if (lowest_affinity_level > 0)
> +		return ARM_PSCI_RET_INVAL;
> +
> +	cpu = mpidr_to_cpu_index(target_affinity);
> +	if (cpu == -1)
> +		return ARM_PSCI_RET_INVAL;
> +
> +	/* TODO flush cache */
> +	return psci_state[cpu];
> +}
> +
> +int __secure psci_migrate_info_type(u32 function_id)
> +{
> +	/* Trusted OS is either not present or does not require migration */
> +	return 2;
> +}
> +
> +int __secure psci_cpu_on(u32 function_id, u32 target_cpu, u32 entry_point,
> +			 u32 context_id)
> +{
> +	int cpu;
> +
> +	cpu = mpidr_to_cpu_index(target_cpu);
> +	if (cpu == -1)
> +		return ARM_PSCI_RET_INVAL;
> +
> +	if (psci_state[cpu] == PSCI_AFFINITY_LEVEL_ON)
> +		return ARM_PSCI_RET_ALREADY_ON;
> +
> +	if (psci_state[cpu] == PSCI_AFFINITY_LEVEL_ON_PENDING)
> +		return ARM_PSCI_RET_ON_PENDING;
> +
> +	psci_save(cpu, entry_point, context_id);
> +
> +	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON_PENDING);
> +
> +	r8a7790_assert_reset(cpu);
> +	r8a7790_apmu_power_on(cpu);
> +	r8a7790_deassert_reset(cpu);
> +
> +	return ARM_PSCI_RET_SUCCESS;
> +}
> +
> +int __secure psci_cpu_off(void)
> +{
> +	int cpu = get_current_cpu();
> +
> +	/*
> +	 * Place the CPU interface in a state where it can never make a CPU
> +	 * exit WFI as result of an asserted interrupt.
> +	 */
> +	writel(0, CONFIG_ARM_GIC_BASE_ADDRESS + GICC_CTLR_OFFSET);
> +	dsb();
> +
> +	/* Select next sleep mode using the APMU */
> +	r8a7790_apmu_power_off(cpu);
> +
> +	/* Do ARM specific CPU shutdown */
> +	psci_cpu_off_common();
> +
> +	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_OFF);
> +
> +	/* Drain the WB before WFI */
> +	dsb();
> +
> +	while (1)
> +		wfi();
> +}
> +
> +void __secure psci_system_reset(u32 function_id)
> +{
> +	r8a7790_system_reset();
> +
> +	/* Drain the WB before WFI */
> +	dsb();
> +
> +	/* The system is about to be rebooted, so just waiting for this */
> +	while (1)
> +		wfi();
> +}
> +
> +void __secure psci_system_off(u32 function_id)
> +{
> +	/* Drain the WB before WFI */
> +	dsb();
> +
> +	/*
> +	 * System Off is not implemented yet, so waiting for powering off
> +	 * manually
> +	 */
> +	while (1)
> +		wfi();
> +}
> +
> +void psci_board_init(void)
> +{
> +	int cpu = get_current_cpu();
> +
> +	/* Update state for the boot CPU */
> +	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON);
> +
> +	/*
> +	 * Perform needed actions for the secondary CPUs to be ready
> +	 * for powering on
> +	 */
> +	r8a7790_prepare_cpus((unsigned long)psci_cpu_entry,
> +			     R8A7790_PSCI_NR_CPUS);
> +}
> diff --git a/include/configs/lager.h b/include/configs/lager.h
> index d8a0b01..d70c147 100644
> --- a/include/configs/lager.h
> +++ b/include/configs/lager.h
> @@ -51,4 +51,6 @@
>  /* The PERIPHBASE in the CBAR register is wrong, so override it */
>  #define CONFIG_ARM_GIC_BASE_ADDRESS		0xf1000000
>  
> +#define CONFIG_ARMV7_PSCI_1_0

Why is this in board code, shouldn't that be in platform code ?


[...]

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 2/3] ARM: rmobile: Add basic PSCI support for r8a7790 SoC

[Oleksandr]

On 05.02.19 20:55, Marek Vasut wrote:

Hi Marek

> On 1/31/19 6:38 PM, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Also enable PSCI support for Stout and Lager boards where
>> actually the r8a7790 SoC is installed.
>>
>> All secondary CPUs will be switched to a non-secure HYP mode
>> after booting.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>   arch/arm/mach-rmobile/Kconfig.32   |   2 +
>>   arch/arm/mach-rmobile/Makefile     |   6 +
>>   arch/arm/mach-rmobile/pm-r8a7790.c | 408 +++++++++++++++++++++++++++++++++++++
>>   arch/arm/mach-rmobile/pm-r8a7790.h |  72 +++++++
>>   arch/arm/mach-rmobile/psci.c       | 193 ++++++++++++++++++
>>   include/configs/lager.h            |   2 +
>>   include/configs/stout.h            |   2 +
>>   7 files changed, 685 insertions(+)
>>   create mode 100644 arch/arm/mach-rmobile/pm-r8a7790.c
>>   create mode 100644 arch/arm/mach-rmobile/pm-r8a7790.h
>>   create mode 100644 arch/arm/mach-rmobile/psci.c
>>
>> diff --git a/arch/arm/mach-rmobile/Kconfig.32 b/arch/arm/mach-rmobile/Kconfig.32
>> index a2e9e3d..728c323 100644
>> --- a/arch/arm/mach-rmobile/Kconfig.32
>> +++ b/arch/arm/mach-rmobile/Kconfig.32
>> @@ -78,6 +78,7 @@ config TARGET_LAGER
>>   	imply CMD_DM
>>   	select CPU_V7_HAS_NONSEC
>>   	select CPU_V7_HAS_VIRT
>> +	select ARCH_SUPPORT_PSCI
>>   
>>   config TARGET_KZM9G
>>   	bool "KZM9D board"
>> @@ -119,6 +120,7 @@ config TARGET_STOUT
>>   	imply CMD_DM
>>   	select CPU_V7_HAS_NONSEC
>>   	select CPU_V7_HAS_VIRT
>> +	select ARCH_SUPPORT_PSCI

To myself: Move this option under "config R8A7790".

>>   
>>   endchoice
>>   
>> diff --git a/arch/arm/mach-rmobile/Makefile b/arch/arm/mach-rmobile/Makefile
>> index 1f26ada..6f4c513 100644
>> --- a/arch/arm/mach-rmobile/Makefile
>> +++ b/arch/arm/mach-rmobile/Makefile
>> @@ -13,3 +13,9 @@ obj-$(CONFIG_SH73A0) += lowlevel_init.o cpu_info-sh73a0.o pfc-sh73a0.o
>>   obj-$(CONFIG_R8A7740) += lowlevel_init.o cpu_info-r8a7740.o pfc-r8a7740.o
>>   obj-$(CONFIG_RCAR_GEN2) += lowlevel_init_ca15.o cpu_info-rcar.o
>>   obj-$(CONFIG_RCAR_GEN3) += lowlevel_init_gen3.o cpu_info-rcar.o memmap-gen3.o
>> +
>> +ifndef CONFIG_SPL_BUILD
>> +ifdef CONFIG_R8A7790
>> +obj-$(CONFIG_ARMV7_PSCI) += psci.o pm-r8a7790.o
>> +endif
>> +endif
>> diff --git a/arch/arm/mach-rmobile/pm-r8a7790.c b/arch/arm/mach-rmobile/pm-r8a7790.c
>> new file mode 100644
>> index 0000000..c635cf6
>> --- /dev/null
>> +++ b/arch/arm/mach-rmobile/pm-r8a7790.c
>> @@ -0,0 +1,408 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * CPU power management support for Renesas r8a7790 SoC
>> + *
>> + * Contains functions to control ARM Cortex A15/A7 cores and
>> + * related peripherals basically used for PSCI.
>> + *
>> + * Copyright (C) 2018 EPAM Systems Inc.
>> + *
>> + * Mainly based on Renesas R-Car Gen2 platform code from Linux:
>> + *    arch/arm/mach-shmobile/...
>> + */
>> +
>> +#include <common.h>
>> +#include <asm/secure.h>
>> +#include <asm/io.h>
>> +
>> +#include "pm-r8a7790.h"
>> +
>> +/*****************************************************************************
> I'd expect checkpatch to complain about these long lines of asterisks.

No, there was no complain about it. I have checked. Anyway, I can remove 
them if required.

>> + * APMU definitions
>> + *****************************************************************************/
>> +#define CA15_APMU_BASE	0xe6152000
>> +#define CA7_APMU_BASE	0xe6151000
> All these addresses should come from DT. And the driver should be DM
> capable and live in drivers/
>
> [...]

All PSCI backends for ARMV7 in U-Boot which I was able to found, are 
located either in arch/arm/cpu/armv7/

or in arch/arm/mach-X. As well as other PSCI backends, this one will be 
located in a separate secure section and acts as secure monitor,

so it will be still alive, when U-Boot is gone away. Do we really want 
this one to go into drivers?

>> +/*****************************************************************************
>> + * Functions which intended to be called from PSCI handlers. These functions
>> + * marked as __secure and are placed in .secure section.
>> + *****************************************************************************/
>> +void __secure r8a7790_apmu_power_on(int cpu)
>> +{
>> +	int cluster = r8a7790_cluster_id(cpu);
>> +	u32 apmu_base;
>> +
>> +	apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
>> +
>> +	/* Request power on */
>> +	writel(BIT(r8a7790_core_id(cpu)), apmu_base + WUPCR_OFFS);
> wait_for_bit of some sorts ?
probably yes, will re-use
>> +	/* Wait for APMU to finish */
>> +	while (readl(apmu_base + WUPCR_OFFS))
>> +		;
> Can this spin forever ?

I didn't find anything in TRM which describes how long it may take. 
Linux also doesn't use timeout.

https://elixir.bootlin.com/linux/v5.0-rc5/source/arch/arm/mach-shmobile/platsmp-apmu.c#L46

Shall I add some timeout (probably 1s) in order to be completely safe?

>> +}
>> +
>> +void __secure r8a7790_apmu_power_off(int cpu)
>> +{
>> +	int cluster = r8a7790_cluster_id(cpu);
>> +	u32 apmu_base;
>> +
>> +	apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
>> +
>> +	/* Request Core Standby for next WFI */
>> +	writel(CPUPWR_STANDBY, apmu_base + CPUNCR_OFFS(r8a7790_core_id(cpu)));
>> +}
>> +
>> +void __secure r8a7790_assert_reset(int cpu)
>> +{
>> +	int cluster = r8a7790_cluster_id(cpu);
>> +	u32 mask, magic, rescnt;
>> +
>> +	mask = BIT(3 - r8a7790_core_id(cpu));
>> +	magic = cluster == 0 ? CA15RESCNT_CODE : CA7RESCNT_CODE;
>> +	rescnt = RST_BASE + (cluster == 0 ? CA15RESCNT : CA7RESCNT);
>> +	writel((readl(rescnt) | mask) | magic, rescnt);
>> +}
>> +
>> +void __secure r8a7790_deassert_reset(int cpu)
>> +{
>> +	int cluster = r8a7790_cluster_id(cpu);
>> +	u32 mask, magic, rescnt;
>> +
>> +	mask = BIT(3 - r8a7790_core_id(cpu));
>> +	magic = cluster == 0 ? CA15RESCNT_CODE : CA7RESCNT_CODE;
>> +	rescnt = RST_BASE + (cluster == 0 ? CA15RESCNT : CA7RESCNT);
>> +	writel((readl(rescnt) & ~mask) | magic, rescnt);
>> +}
>> +
>> +void __secure r8a7790_system_reset(void)
>> +{
>> +	u32 bar;
>> +
>> +	/*
>> +	 * Before configuring internal watchdog timer (RWDT) to reboot system
>> +	 * we need to re-program BAR registers for the boot CPU to jump to
>> +	 * bootrom code. Without taking care of, the boot CPU will jump to
>> +	 * the reset vector previously installed in ICRAM1, since BAR registers
>> +	 * keep their values after watchdog triggered reset.
>> +	 */
>> +	bar = (BOOTROM >> 8) & 0xfffffc00;
>> +	writel(bar, RST_BASE + CA15BAR);
>> +	writel(bar | SBAR_BAREN, RST_BASE + CA15BAR);
>> +	writel(bar, RST_BASE + CA7BAR);
>> +	writel(bar | SBAR_BAREN, RST_BASE + CA7BAR);
>> +	dsb();
>> +
>> +	/* Now, configure watchdog timer to reboot the system */
>> +
>> +	/* Trigger reset when counter overflows */
>> +	writel(WDTRSTCR_CODE | 0x2, RST_BASE + WDTRSTCR);
>> +	dsb();
>> +
>> +	/* Stop counter */
>> +	writel(RWTCSRA_CODE, RWDT_BASE + RWTCSRA);
>> +
>> +	/* Initialize counter with the highest value  */
>> +	writel(RWTCNT_CODE | 0xffff, RWDT_BASE + RWTCNT);
>> +
>> +	while (readb(RWDT_BASE + RWTCSRA) & RWTCSRA_WRFLG)
>> +		;
>> +
>> +	/* Start counter */
>> +	writel(RWTCSRA_CODE | RWTCSRA_TME, RWDT_BASE + RWTCSRA);
> This seems to reimplement the reset code that's in U-Boot already. Why ?

Yes. I had to re-implement. Let me describe why.

 From my understanding (I may mistake), the PSCI backend code which 
lives in secure section should be as lightweight as possible

and shouldn't call any U-Boot routines not marked as __secure, except 
simple static inline functions.

You can see PSCI implementation for any platform in U-Boot,  and only 
simple "macroses/inlines" are used across all of them.

Even for realizing some delay in code, they have specially implemented 
something simple. As an example __mdelay() realization here:

https://elixir.bootlin.com/u-boot/v2019.01/source/arch/arm/cpu/armv7/sunxi/psci.c#L61

I know that PMIC (for Lager) and CPLD (for Stout) assist SoC to perform 
reset. But, I couldn't use that functional here in PSCI backend, as it 
pulls a lot of code (i2c for PMIC, gpio manipulation for CPLD, etc),

so for that reason (AFAIK the PSCI system reset is a mandatory option) I 
chose WDT as a entity for doing a reset. This is quite simple and can be 
used on both boards, what is more that it can be used on other Gen2 SoC 
if required.

>> +}
>> +
>> +/*****************************************************************************
>> + * Functions which intended to be called from PSCI board initialization.
>> + *****************************************************************************/
>> +static int sysc_power_up(int scu)
>> +{
>> +	u32 status, chan_offs, isr_bit;
>> +	int i, j, ret = 0;
>> +
>> +	if (scu == CA15_SCU) {
>> +		chan_offs = CA15_SCU_CHAN_OFFS;
>> +		isr_bit = CA15_SCU_ISR_BIT;
>> +	} else {
>> +		chan_offs = CA7_SCU_CHAN_OFFS;
>> +		isr_bit = CA7_SCU_ISR_BIT;
>> +	}
>> +
>> +	writel(BIT(isr_bit), SYSC_BASE + SYSCISCR);
>> +
>> +	/* Submit power resume request until it was accepted */
>> +	for (i = 0; i < PWRER_RETRIES; i++) {
>> +		/* Wait until SYSC is ready to accept a power resume request */
>> +		for (j = 0; j < SYSCSR_RETRIES; j++) {
>> +			if (readl(SYSC_BASE + SYSCSR) & BIT(1))
>> +				break;
>> +
>> +			udelay(SYSCSR_DELAY_US);
>> +		}
>> +
>> +		if (j == SYSCSR_RETRIES)
>> +			return -EAGAIN;
>> +
>> +		/* Submit power resume request */
>> +		writel(BIT(0), SYSC_BASE + chan_offs + PWRONCR_OFFS);
>> +
>> +		/* Check if power resume request was accepted */
>> +		status = readl(SYSC_BASE + chan_offs + PWRER_OFFS);
>> +		if (!(status & BIT(0)))
>> +			break;
>> +
>> +		udelay(PWRER_DELAY_US);
>> +	}
>> +
>> +	if (i == PWRER_RETRIES)
>> +		return -EIO;
>> +
>> +	/* Wait until the power resume request has completed */
>> +	for (i = 0; i < SYSCISR_RETRIES; i++) {
>> +		if (readl(SYSC_BASE + SYSCISR) & BIT(isr_bit))
>> +			break;
>> +		udelay(SYSCISR_DELAY_US);
>> +	}
>> +
>> +	if (i == SYSCISR_RETRIES)
>> +		ret = -EIO;
>> +
>> +	writel(BIT(isr_bit), SYSC_BASE + SYSCISCR);
>> +
>> +	return ret;
>> +}
>> +
>> +static bool sysc_power_is_off(int scu)
>> +{
>> +	u32 status, chan_offs;
>> +
>> +	chan_offs = scu == CA15_SCU ? CA15_SCU_CHAN_OFFS : CA7_SCU_CHAN_OFFS;
>> +
>> +	/* Check if SCU is in power shutoff state */
>> +	status = readl(SYSC_BASE + chan_offs + PWRSR_OFFS);
>> +	if (status & BIT(0))
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +static void apmu_setup_debug_mode(int cpu)
>> +{
>> +	int cluster = r8a7790_cluster_id(cpu);
>> +	u32 apmu_base, reg;
>> +
>> +	apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
>> +
>> +	/*
>> +	 * Enable reset requests derived from power shutoff to the AP-system
>> +	 * CPU cores in debug mode. Without taking care of, they may fail to
>> +	 * resume from the power shutoff state.
>> +	 */
>> +	reg = readl(apmu_base + DBGRCR_OFFS);
>> +	reg |= DBGCPUREN | DBGCPUNREN(r8a7790_core_id(cpu)) | DBGCPUPREN;
>> +	writel(reg, apmu_base + DBGRCR_OFFS);
> setbits_le32()

Agree, will re-use

>> +}
>> +
>> +/*
>> + * Reset vector for secondary CPUs.
>> + * This will be mapped at address 0 by SBAR register.
>> + * We need _long_ jump to the physical address.
>> + */
>> +asm("	.arm\n"
>> +	"	.align 12\n"
>> +	"	.globl shmobile_boot_vector\n"
>> +	"shmobile_boot_vector:\n"
>> +	"	ldr r1, 1f\n"
>> +	"	bx	r1\n"
>> +	"	.type shmobile_boot_vector, %function\n"
>> +	"	.size shmobile_boot_vector, .-shmobile_boot_vector\n"
>> +	"	.align	2\n"
>> +	"	.globl	shmobile_boot_fn\n"
>> +	"shmobile_boot_fn:\n"
>> +	"1:	.space	4\n"
>> +	"	.globl	shmobile_boot_size\n"
>> +	"shmobile_boot_size:\n"
>> +	"	.long	.-shmobile_boot_vector\n");
> Why can't this be implemented in C ?

This "reset vector" code was ported from Linux:

https://elixir.bootlin.com/linux/v5.0-rc5/source/arch/arm/mach-shmobile/headsmp.S#L21

Really don't know whether it can be implemented in C.

I was thinking of moving this code to a separate ASM file in order not 
to mix C and ASM. What do you think about it?

>> +extern void shmobile_boot_vector(void);
>> +extern unsigned long shmobile_boot_fn;
>> +extern unsigned long shmobile_boot_size;
> Are the externs necessary ?

Yes, otherwise, compiler will complain.

I can hide them in a header file. Shall I do that with moving ASM code 
to a separate file?

>> +void r8a7790_prepare_cpus(unsigned long addr, u32 nr_cpus)
>> +{
>> +	int cpu = get_current_cpu();
>> +	int i, ret = 0;
>> +	u32 bar;
>> +
>> +	shmobile_boot_fn = addr;
>> +	dsb();
>> +
>> +	/* Install reset vector */
>> +	memcpy_toio((void __iomem *)ICRAM1, shmobile_boot_vector,
>> +		    shmobile_boot_size);
>> +	dmb();
> Does this take into consideration caches ?

Good question... Probably, I should have added flush_dcache_range() 
after copy.

>> +	/* Setup reset vectors */
>> +	bar = (ICRAM1 >> 8) & 0xfffffc00;
>> +	writel(bar, RST_BASE + CA15BAR);
>> +	writel(bar | SBAR_BAREN, RST_BASE + CA15BAR);
>> +	writel(bar, RST_BASE + CA7BAR);
>> +	writel(bar | SBAR_BAREN, RST_BASE + CA7BAR);
>> +	dsb();
>> +
>> +	/* Perform setup for debug mode for all CPUs */
>> +	for (i = 0; i < nr_cpus; i++)
>> +		apmu_setup_debug_mode(i);
>> +
>> +	/*
>> +	 * Indicate the completion status of power shutoff/resume procedure
>> +	 * for CA15/CA7 SCU.
>> +	 */
>> +	writel(BIT(CA15_SCU_ISR_BIT) | BIT(CA7_SCU_ISR_BIT),
>> +	       SYSC_BASE + SYSCIER);
>> +
>> +	/* Power on CA15/CA7 SCU */
>> +	if (sysc_power_is_off(CA15_SCU))
>> +		ret += sysc_power_up(CA15_SCU);
>> +	if (sysc_power_is_off(CA7_SCU))
>> +		ret += sysc_power_up(CA7_SCU);
>> +	if (ret)
>> +		printf("warning: some of secondary CPUs may not boot\n");
> "some" is not very useful for debugging,.

OK, will provide more concrete output.

>> +	/* Keep secondary CPUs in reset */
>> +	for (i = 0; i < nr_cpus; i++) {
>> +		/* Make sure that we don't reset boot CPU */
>> +		if (i == cpu)
>> +			continue;
>> +
>> +		r8a7790_assert_reset(i);
>> +	}
>> +
>> +	/*
>> +	 * Enable snoop requests and DVM message requests for slave interfaces
>> +	 * S4 (CA7) and S3 (CA15).
>> +	 */
>> +	writel(readl(CCI_BASE + CCI_SLAVE3 + CCI_SNOOP) | CCI_ENABLE_REQ,
>> +	       CCI_BASE + CCI_SLAVE3 + CCI_SNOOP);
>> +	writel(readl(CCI_BASE + CCI_SLAVE4 + CCI_SNOOP) | CCI_ENABLE_REQ,
>> +	       CCI_BASE + CCI_SLAVE4 + CCI_SNOOP);
>> +	/* Wait for pending bit low */
>> +	while (readl(CCI_BASE + CCI_STATUS))
>> +		;
> can this spin forever ?

Paragraph "3.3.4 Status Register" in "ARM CoreLink™CCI-400 Cache 
Coherent Interconnect" document says nothing

about possibility to spin forever. Just next:

"After writing to the snoop or DVM enable bits, the controller must wait 
for the register write to
complete, then test that the change_pending bit is LOW before it turns 
an attached device on or off."

>> +}
>> diff --git a/arch/arm/mach-rmobile/pm-r8a7790.h b/arch/arm/mach-rmobile/pm-r8a7790.h
>> new file mode 100644
>> index 0000000..f649dd8
>> --- /dev/null
>> +++ b/arch/arm/mach-rmobile/pm-r8a7790.h
>> @@ -0,0 +1,72 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2018 EPAM Systems Inc.
>> + */
>> +
>> +#ifndef __PM_R8A7790_H__
>> +#define __PM_R8A7790_H__
>> +
>> +#include <linux/types.h>
>> +
>> +void r8a7790_apmu_power_on(int cpu);
>> +void r8a7790_apmu_power_off(int cpu);
>> +void r8a7790_assert_reset(int cpu);
>> +void r8a7790_deassert_reset(int cpu);
>> +
>> +void r8a7790_prepare_cpus(unsigned long addr, u32 nr_cpus);
>> +void r8a7790_system_reset(void);
>> +
>> +#define R8A7790_NR_CLUSTERS				2
>> +#define R8A7790_NR_CPUS_PER_CLUSTER		4
>> +
>> +/* Convert linear CPU index to core/cluster ID */
>> +#define r8a7790_cluster_id(cpu)	((cpu) / R8A7790_NR_CPUS_PER_CLUSTER)
>> +#define r8a7790_core_id(cpu)	((cpu) % R8A7790_NR_CPUS_PER_CLUSTER)
>> +
>> +#define MPIDR_AFFLVL_MASK	GENMASK(7, 0)
>> +#define MPIDR_AFF0_SHIFT	0
>> +#define MPIDR_AFF1_SHIFT	8
>> +
>> +/* All functions are inline so that they can be called from .secure section. */
>> +
>> +/*
>> + * Convert CPU ID in MPIDR format to linear CPU index.
>> + *
>> + * Below the possible CPU IDs and corresponding CPU indexes:
>> + * CPU ID       CPU index
>> + * 0x80000000 - 0x00000000
>> + * 0x80000001 - 0x00000001
>> + * 0x80000002 - 0x00000002
>> + * 0x80000003 - 0x00000003
>> + * 0x80000100 - 0x00000004
>> + * 0x80000101 - 0x00000005
>> + * 0x80000102 - 0x00000006
>> + * 0x80000103 - 0x00000007
>> + */
>> +static inline int mpidr_to_cpu_index(u32 mpidr)
>> +{
>> +	u32 cluster_id, cpu_id;
>> +
>> +	cluster_id = (mpidr >> MPIDR_AFF1_SHIFT) & MPIDR_AFFLVL_MASK;
>> +	cpu_id = (mpidr >> MPIDR_AFF0_SHIFT) & MPIDR_AFFLVL_MASK;
>> +
>> +	if (cluster_id >= R8A7790_NR_CLUSTERS)
>> +		return -1;
>> +
>> +	if (cpu_id >= R8A7790_NR_CPUS_PER_CLUSTER)
>> +		return -1;
>> +
>> +	return (cpu_id + (cluster_id * R8A7790_NR_CPUS_PER_CLUSTER));
>> +}
>> +
>> +/* Return an index of the CPU which performs this call */
>> +static inline int get_current_cpu(void)
>> +{
>> +	u32 mpidr;
>> +
>> +	asm volatile("mrc p15, 0, %0, c0, c0, 5" : "=r"(mpidr));
>> +
>> +	return mpidr_to_cpu_index(mpidr);
>> +}
>> +
>> +#endif /* __PM_R8A7790_H__ */
>> diff --git a/arch/arm/mach-rmobile/psci.c b/arch/arm/mach-rmobile/psci.c
>> new file mode 100644
>> index 0000000..95850b8
>> --- /dev/null
>> +++ b/arch/arm/mach-rmobile/psci.c
>> @@ -0,0 +1,193 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * This file implements basic PSCI support for Renesas r8a7790 SoC
>> + *
>> + * Copyright (C) 2018 EPAM Systems Inc.
>> + *
>> + * Based on arch/arm/mach-uniphier/arm32/psci.c
>> + */
>> +
>> +#include <common.h>
>> +#include <linux/psci.h>
>> +#include <asm/io.h>
>> +#include <asm/psci.h>
>> +#include <asm/secure.h>
>> +
>> +#include "pm-r8a7790.h"
>> +
>> +#define R8A7790_PSCI_NR_CPUS	8
>> +#if R8A7790_PSCI_NR_CPUS > CONFIG_ARMV7_PSCI_NR_CPUS
>> +#error "invalid value for CONFIG_ARMV7_PSCI_NR_CPUS"
>> +#endif
>> +
>> +#define GICC_CTLR_OFFSET	0x2000
>> +
>> +/*
>> + * The boot CPU is powered on by default, but it's index is not a const
>> + * value. An index the boot CPU has, depends on whether it is CA15 (index 0)
>> + * or CA7 (index 4).
>> + * So, we update state for the boot CPU during PSCI board initialization.
>> + */
>> +u8 psci_state[R8A7790_PSCI_NR_CPUS] __secure_data = {
>> +		PSCI_AFFINITY_LEVEL_OFF,
>> +		PSCI_AFFINITY_LEVEL_OFF,
>> +		PSCI_AFFINITY_LEVEL_OFF,
>> +		PSCI_AFFINITY_LEVEL_OFF,
>> +		PSCI_AFFINITY_LEVEL_OFF,
>> +		PSCI_AFFINITY_LEVEL_OFF,
>> +		PSCI_AFFINITY_LEVEL_OFF,
>> +		PSCI_AFFINITY_LEVEL_OFF};
>> +
>> +void __secure psci_set_state(int cpu, u8 state)
>> +{
>> +	psci_state[cpu] = state;
>> +	dsb();
>> +	isb();
>> +}
>> +
>> +u32 __secure psci_get_cpu_id(void)
>> +{
>> +	return get_current_cpu();
>> +}
>> +
>> +void __secure psci_arch_cpu_entry(void)
>> +{
>> +	int cpu = get_current_cpu();
>> +
>> +	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON);
>> +}
>> +
>> +int __secure psci_features(u32 function_id, u32 psci_fid)
>> +{
>> +	switch (psci_fid) {
>> +	case ARM_PSCI_0_2_FN_PSCI_VERSION:
>> +	case ARM_PSCI_0_2_FN_CPU_OFF:
>> +	case ARM_PSCI_0_2_FN_CPU_ON:
>> +	case ARM_PSCI_0_2_FN_AFFINITY_INFO:
>> +	case ARM_PSCI_0_2_FN_MIGRATE_INFO_TYPE:
>> +	case ARM_PSCI_0_2_FN_SYSTEM_OFF:
>> +	case ARM_PSCI_0_2_FN_SYSTEM_RESET:
>> +		return 0x0;
>> +	}
>> +
>> +	return ARM_PSCI_RET_NI;
>> +}
>> +
>> +u32 __secure psci_version(u32 function_id)
>> +{
>> +	return ARM_PSCI_VER_1_0;
>> +}
>> +
>> +int __secure psci_affinity_info(u32 function_id, u32 target_affinity,
>> +				u32 lowest_affinity_level)
>> +{
>> +	int cpu;
>> +
>> +	if (lowest_affinity_level > 0)
>> +		return ARM_PSCI_RET_INVAL;
>> +
>> +	cpu = mpidr_to_cpu_index(target_affinity);
>> +	if (cpu == -1)
>> +		return ARM_PSCI_RET_INVAL;
>> +
>> +	/* TODO flush cache */
>> +	return psci_state[cpu];
>> +}
>> +
>> +int __secure psci_migrate_info_type(u32 function_id)
>> +{
>> +	/* Trusted OS is either not present or does not require migration */
>> +	return 2;
>> +}
>> +
>> +int __secure psci_cpu_on(u32 function_id, u32 target_cpu, u32 entry_point,
>> +			 u32 context_id)
>> +{
>> +	int cpu;
>> +
>> +	cpu = mpidr_to_cpu_index(target_cpu);
>> +	if (cpu == -1)
>> +		return ARM_PSCI_RET_INVAL;
>> +
>> +	if (psci_state[cpu] == PSCI_AFFINITY_LEVEL_ON)
>> +		return ARM_PSCI_RET_ALREADY_ON;
>> +
>> +	if (psci_state[cpu] == PSCI_AFFINITY_LEVEL_ON_PENDING)
>> +		return ARM_PSCI_RET_ON_PENDING;
>> +
>> +	psci_save(cpu, entry_point, context_id);
>> +
>> +	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON_PENDING);
>> +
>> +	r8a7790_assert_reset(cpu);
>> +	r8a7790_apmu_power_on(cpu);
>> +	r8a7790_deassert_reset(cpu);
>> +
>> +	return ARM_PSCI_RET_SUCCESS;
>> +}
>> +
>> +int __secure psci_cpu_off(void)
>> +{
>> +	int cpu = get_current_cpu();
>> +
>> +	/*
>> +	 * Place the CPU interface in a state where it can never make a CPU
>> +	 * exit WFI as result of an asserted interrupt.
>> +	 */
>> +	writel(0, CONFIG_ARM_GIC_BASE_ADDRESS + GICC_CTLR_OFFSET);
>> +	dsb();
>> +
>> +	/* Select next sleep mode using the APMU */
>> +	r8a7790_apmu_power_off(cpu);
>> +
>> +	/* Do ARM specific CPU shutdown */
>> +	psci_cpu_off_common();
>> +
>> +	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_OFF);
>> +
>> +	/* Drain the WB before WFI */
>> +	dsb();
>> +
>> +	while (1)
>> +		wfi();
>> +}
>> +
>> +void __secure psci_system_reset(u32 function_id)
>> +{
>> +	r8a7790_system_reset();
>> +
>> +	/* Drain the WB before WFI */
>> +	dsb();
>> +
>> +	/* The system is about to be rebooted, so just waiting for this */
>> +	while (1)
>> +		wfi();
>> +}
>> +
>> +void __secure psci_system_off(u32 function_id)
>> +{
>> +	/* Drain the WB before WFI */
>> +	dsb();
>> +
>> +	/*
>> +	 * System Off is not implemented yet, so waiting for powering off
>> +	 * manually
>> +	 */
>> +	while (1)
>> +		wfi();
>> +}
>> +
>> +void psci_board_init(void)
>> +{
>> +	int cpu = get_current_cpu();
>> +
>> +	/* Update state for the boot CPU */
>> +	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON);
>> +
>> +	/*
>> +	 * Perform needed actions for the secondary CPUs to be ready
>> +	 * for powering on
>> +	 */
>> +	r8a7790_prepare_cpus((unsigned long)psci_cpu_entry,
>> +			     R8A7790_PSCI_NR_CPUS);
>> +}
>> diff --git a/include/configs/lager.h b/include/configs/lager.h
>> index d8a0b01..d70c147 100644
>> --- a/include/configs/lager.h
>> +++ b/include/configs/lager.h
>> @@ -51,4 +51,6 @@
>>   /* The PERIPHBASE in the CBAR register is wrong, so override it */
>>   #define CONFIG_ARM_GIC_BASE_ADDRESS		0xf1000000
>>   
>> +#define CONFIG_ARMV7_PSCI_1_0
> Why is this in board code, shouldn't that be in platform code ?

I will move it to "rcar-gen2-common.h"

> [...]
>
-- 
Regards,

Oleksandr Tyshchenko

[U-Boot] [PATCH 2/3] ARM: rmobile: Add basic PSCI support for r8a7790 SoC

[Marek Vasut]

On 2/8/19 11:52 AM, Oleksandr wrote:
> 
> On 05.02.19 20:55, Marek Vasut wrote:
> 
> Hi Marek

Hi,

>> On 1/31/19 6:38 PM, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> Also enable PSCI support for Stout and Lager boards where
>>> actually the r8a7790 SoC is installed.
>>>
>>> All secondary CPUs will be switched to a non-secure HYP mode
>>> after booting.
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> ---
>>>   arch/arm/mach-rmobile/Kconfig.32   |   2 +
>>>   arch/arm/mach-rmobile/Makefile     |   6 +
>>>   arch/arm/mach-rmobile/pm-r8a7790.c | 408
>>> +++++++++++++++++++++++++++++++++++++
>>>   arch/arm/mach-rmobile/pm-r8a7790.h |  72 +++++++
>>>   arch/arm/mach-rmobile/psci.c       | 193 ++++++++++++++++++
>>>   include/configs/lager.h            |   2 +
>>>   include/configs/stout.h            |   2 +
>>>   7 files changed, 685 insertions(+)
>>>   create mode 100644 arch/arm/mach-rmobile/pm-r8a7790.c
>>>   create mode 100644 arch/arm/mach-rmobile/pm-r8a7790.h
>>>   create mode 100644 arch/arm/mach-rmobile/psci.c
>>>
>>> diff --git a/arch/arm/mach-rmobile/Kconfig.32
>>> b/arch/arm/mach-rmobile/Kconfig.32
>>> index a2e9e3d..728c323 100644
>>> --- a/arch/arm/mach-rmobile/Kconfig.32
>>> +++ b/arch/arm/mach-rmobile/Kconfig.32
>>> @@ -78,6 +78,7 @@ config TARGET_LAGER
>>>       imply CMD_DM
>>>       select CPU_V7_HAS_NONSEC
>>>       select CPU_V7_HAS_VIRT
>>> +    select ARCH_SUPPORT_PSCI
>>>     config TARGET_KZM9G
>>>       bool "KZM9D board"
>>> @@ -119,6 +120,7 @@ config TARGET_STOUT
>>>       imply CMD_DM
>>>       select CPU_V7_HAS_NONSEC
>>>       select CPU_V7_HAS_VIRT
>>> +    select ARCH_SUPPORT_PSCI
> 
> To myself: Move this option under "config R8A7790".
> 
>>>     endchoice
>>>   diff --git a/arch/arm/mach-rmobile/Makefile
>>> b/arch/arm/mach-rmobile/Makefile
>>> index 1f26ada..6f4c513 100644
>>> --- a/arch/arm/mach-rmobile/Makefile
>>> +++ b/arch/arm/mach-rmobile/Makefile
>>> @@ -13,3 +13,9 @@ obj-$(CONFIG_SH73A0) += lowlevel_init.o
>>> cpu_info-sh73a0.o pfc-sh73a0.o
>>>   obj-$(CONFIG_R8A7740) += lowlevel_init.o cpu_info-r8a7740.o
>>> pfc-r8a7740.o
>>>   obj-$(CONFIG_RCAR_GEN2) += lowlevel_init_ca15.o cpu_info-rcar.o
>>>   obj-$(CONFIG_RCAR_GEN3) += lowlevel_init_gen3.o cpu_info-rcar.o
>>> memmap-gen3.o
>>> +
>>> +ifndef CONFIG_SPL_BUILD
>>> +ifdef CONFIG_R8A7790
>>> +obj-$(CONFIG_ARMV7_PSCI) += psci.o pm-r8a7790.o
>>> +endif
>>> +endif
>>> diff --git a/arch/arm/mach-rmobile/pm-r8a7790.c
>>> b/arch/arm/mach-rmobile/pm-r8a7790.c
>>> new file mode 100644
>>> index 0000000..c635cf6
>>> --- /dev/null
>>> +++ b/arch/arm/mach-rmobile/pm-r8a7790.c
>>> @@ -0,0 +1,408 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * CPU power management support for Renesas r8a7790 SoC
>>> + *
>>> + * Contains functions to control ARM Cortex A15/A7 cores and
>>> + * related peripherals basically used for PSCI.
>>> + *
>>> + * Copyright (C) 2018 EPAM Systems Inc.
>>> + *
>>> + * Mainly based on Renesas R-Car Gen2 platform code from Linux:
>>> + *    arch/arm/mach-shmobile/...
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <asm/secure.h>
>>> +#include <asm/io.h>
>>> +
>>> +#include "pm-r8a7790.h"
>>> +
>>> +/*****************************************************************************
>>>
>> I'd expect checkpatch to complain about these long lines of asterisks.
> 
> No, there was no complain about it. I have checked. Anyway, I can remove
> them if required.

Yes please, keep the comment style consistent with the rest of the
codebase, which is also the kernel comment style.

>>> + * APMU definitions
>>> +
>>> *****************************************************************************/
>>>
>>> +#define CA15_APMU_BASE    0xe6152000
>>> +#define CA7_APMU_BASE    0xe6151000
>> All these addresses should come from DT. And the driver should be DM
>> capable and live in drivers/
>>
>> [...]
> 
> All PSCI backends for ARMV7 in U-Boot which I was able to found, are
> located either in arch/arm/cpu/armv7/
> 
> or in arch/arm/mach-X. As well as other PSCI backends, this one will be
> located in a separate secure section and acts as secure monitor,
> 
> so it will be still alive, when U-Boot is gone away. Do we really want
> this one to go into drivers?

I'd much prefer it if we stopped adding more stuff to arch/arm/mach-* ,
but I think we cannot avoid that in this case, can we ?

>>> +/*****************************************************************************
>>>
>>> + * Functions which intended to be called from PSCI handlers. These
>>> functions
>>> + * marked as __secure and are placed in .secure section.
>>> +
>>> *****************************************************************************/
>>>
>>> +void __secure r8a7790_apmu_power_on(int cpu)
>>> +{
>>> +    int cluster = r8a7790_cluster_id(cpu);
>>> +    u32 apmu_base;
>>> +
>>> +    apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
>>> +
>>> +    /* Request power on */
>>> +    writel(BIT(r8a7790_core_id(cpu)), apmu_base + WUPCR_OFFS);
>> wait_for_bit of some sorts ?
> probably yes, will re-use
>>> +    /* Wait for APMU to finish */
>>> +    while (readl(apmu_base + WUPCR_OFFS))
>>> +        ;
>> Can this spin forever ?
> 
> I didn't find anything in TRM which describes how long it may take.
> Linux also doesn't use timeout.
> 
> https://elixir.bootlin.com/linux/v5.0-rc5/source/arch/arm/mach-shmobile/platsmp-apmu.c#L46
> 
> 
> Shall I add some timeout (probably 1s) in order to be completely safe?

I think so, because if you start spinning in here forever, the system
will just completely freeze without any way of recovering. I don't think
that's what you want to happen while using virtualization, right ?

>>> +}
>>> +
>>> +void __secure r8a7790_apmu_power_off(int cpu)
>>> +{
>>> +    int cluster = r8a7790_cluster_id(cpu);
>>> +    u32 apmu_base;
>>> +
>>> +    apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
>>> +
>>> +    /* Request Core Standby for next WFI */
>>> +    writel(CPUPWR_STANDBY, apmu_base +
>>> CPUNCR_OFFS(r8a7790_core_id(cpu)));
>>> +}
>>> +
>>> +void __secure r8a7790_assert_reset(int cpu)
>>> +{
>>> +    int cluster = r8a7790_cluster_id(cpu);
>>> +    u32 mask, magic, rescnt;
>>> +
>>> +    mask = BIT(3 - r8a7790_core_id(cpu));
>>> +    magic = cluster == 0 ? CA15RESCNT_CODE : CA7RESCNT_CODE;
>>> +    rescnt = RST_BASE + (cluster == 0 ? CA15RESCNT : CA7RESCNT);
>>> +    writel((readl(rescnt) | mask) | magic, rescnt);
>>> +}
>>> +
>>> +void __secure r8a7790_deassert_reset(int cpu)
>>> +{
>>> +    int cluster = r8a7790_cluster_id(cpu);
>>> +    u32 mask, magic, rescnt;
>>> +
>>> +    mask = BIT(3 - r8a7790_core_id(cpu));
>>> +    magic = cluster == 0 ? CA15RESCNT_CODE : CA7RESCNT_CODE;
>>> +    rescnt = RST_BASE + (cluster == 0 ? CA15RESCNT : CA7RESCNT);
>>> +    writel((readl(rescnt) & ~mask) | magic, rescnt);
>>> +}
>>> +
>>> +void __secure r8a7790_system_reset(void)
>>> +{
>>> +    u32 bar;
>>> +
>>> +    /*
>>> +     * Before configuring internal watchdog timer (RWDT) to reboot
>>> system
>>> +     * we need to re-program BAR registers for the boot CPU to jump to
>>> +     * bootrom code. Without taking care of, the boot CPU will jump to
>>> +     * the reset vector previously installed in ICRAM1, since BAR
>>> registers
>>> +     * keep their values after watchdog triggered reset.
>>> +     */
>>> +    bar = (BOOTROM >> 8) & 0xfffffc00;
>>> +    writel(bar, RST_BASE + CA15BAR);
>>> +    writel(bar | SBAR_BAREN, RST_BASE + CA15BAR);
>>> +    writel(bar, RST_BASE + CA7BAR);
>>> +    writel(bar | SBAR_BAREN, RST_BASE + CA7BAR);
>>> +    dsb();
>>> +
>>> +    /* Now, configure watchdog timer to reboot the system */
>>> +
>>> +    /* Trigger reset when counter overflows */
>>> +    writel(WDTRSTCR_CODE | 0x2, RST_BASE + WDTRSTCR);
>>> +    dsb();
>>> +
>>> +    /* Stop counter */
>>> +    writel(RWTCSRA_CODE, RWDT_BASE + RWTCSRA);
>>> +
>>> +    /* Initialize counter with the highest value  */
>>> +    writel(RWTCNT_CODE | 0xffff, RWDT_BASE + RWTCNT);
>>> +
>>> +    while (readb(RWDT_BASE + RWTCSRA) & RWTCSRA_WRFLG)
>>> +        ;
>>> +
>>> +    /* Start counter */
>>> +    writel(RWTCSRA_CODE | RWTCSRA_TME, RWDT_BASE + RWTCSRA);
>> This seems to reimplement the reset code that's in U-Boot already. Why ?
> 
> Yes. I had to re-implement. Let me describe why.
> 
> From my understanding (I may mistake), the PSCI backend code which lives
> in secure section should be as lightweight as possible
> 
> and shouldn't call any U-Boot routines not marked as __secure, except
> simple static inline functions.
> 
> You can see PSCI implementation for any platform in U-Boot,  and only
> simple "macroses/inlines" are used across all of them.
> 
> Even for realizing some delay in code, they have specially implemented
> something simple. As an example __mdelay() realization here:
> 
> https://elixir.bootlin.com/u-boot/v2019.01/source/arch/arm/cpu/armv7/sunxi/psci.c#L61

Can the U-Boot code be refactored somehow to avoid the duplication ?

> I know that PMIC (for Lager) and CPLD (for Stout) assist SoC to perform
> reset. But, I couldn't use that functional here in PSCI backend, as it
> pulls a lot of code (i2c for PMIC, gpio manipulation for CPLD, etc),

How can the reset work properly if the CPLD/PMIC isn't even used then ?

> so for that reason (AFAIK the PSCI system reset is a mandatory option) I
> chose WDT as a entity for doing a reset. This is quite simple and can be
> used on both boards, what is more that it can be used on other Gen2 SoC
> if required.
> 
>>> +}
>>> +
>>> +/*****************************************************************************
>>>
>>> + * Functions which intended to be called from PSCI board
>>> initialization.
>>> +
>>> *****************************************************************************/
>>>
>>> +static int sysc_power_up(int scu)
>>> +{
>>> +    u32 status, chan_offs, isr_bit;
>>> +    int i, j, ret = 0;
>>> +
>>> +    if (scu == CA15_SCU) {
>>> +        chan_offs = CA15_SCU_CHAN_OFFS;
>>> +        isr_bit = CA15_SCU_ISR_BIT;
>>> +    } else {
>>> +        chan_offs = CA7_SCU_CHAN_OFFS;
>>> +        isr_bit = CA7_SCU_ISR_BIT;
>>> +    }
>>> +
>>> +    writel(BIT(isr_bit), SYSC_BASE + SYSCISCR);
>>> +
>>> +    /* Submit power resume request until it was accepted */
>>> +    for (i = 0; i < PWRER_RETRIES; i++) {
>>> +        /* Wait until SYSC is ready to accept a power resume request */
>>> +        for (j = 0; j < SYSCSR_RETRIES; j++) {
>>> +            if (readl(SYSC_BASE + SYSCSR) & BIT(1))
>>> +                break;
>>> +
>>> +            udelay(SYSCSR_DELAY_US);
>>> +        }
>>> +
>>> +        if (j == SYSCSR_RETRIES)
>>> +            return -EAGAIN;
>>> +
>>> +        /* Submit power resume request */
>>> +        writel(BIT(0), SYSC_BASE + chan_offs + PWRONCR_OFFS);
>>> +
>>> +        /* Check if power resume request was accepted */
>>> +        status = readl(SYSC_BASE + chan_offs + PWRER_OFFS);
>>> +        if (!(status & BIT(0)))
>>> +            break;
>>> +
>>> +        udelay(PWRER_DELAY_US);
>>> +    }
>>> +
>>> +    if (i == PWRER_RETRIES)
>>> +        return -EIO;
>>> +
>>> +    /* Wait until the power resume request has completed */
>>> +    for (i = 0; i < SYSCISR_RETRIES; i++) {
>>> +        if (readl(SYSC_BASE + SYSCISR) & BIT(isr_bit))
>>> +            break;
>>> +        udelay(SYSCISR_DELAY_US);
>>> +    }
>>> +
>>> +    if (i == SYSCISR_RETRIES)
>>> +        ret = -EIO;
>>> +
>>> +    writel(BIT(isr_bit), SYSC_BASE + SYSCISCR);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static bool sysc_power_is_off(int scu)
>>> +{
>>> +    u32 status, chan_offs;
>>> +
>>> +    chan_offs = scu == CA15_SCU ? CA15_SCU_CHAN_OFFS :
>>> CA7_SCU_CHAN_OFFS;
>>> +
>>> +    /* Check if SCU is in power shutoff state */
>>> +    status = readl(SYSC_BASE + chan_offs + PWRSR_OFFS);
>>> +    if (status & BIT(0))
>>> +        return true;
>>> +
>>> +    return false;
>>> +}
>>> +
>>> +static void apmu_setup_debug_mode(int cpu)
>>> +{
>>> +    int cluster = r8a7790_cluster_id(cpu);
>>> +    u32 apmu_base, reg;
>>> +
>>> +    apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
>>> +
>>> +    /*
>>> +     * Enable reset requests derived from power shutoff to the
>>> AP-system
>>> +     * CPU cores in debug mode. Without taking care of, they may
>>> fail to
>>> +     * resume from the power shutoff state.
>>> +     */
>>> +    reg = readl(apmu_base + DBGRCR_OFFS);
>>> +    reg |= DBGCPUREN | DBGCPUNREN(r8a7790_core_id(cpu)) | DBGCPUPREN;
>>> +    writel(reg, apmu_base + DBGRCR_OFFS);
>> setbits_le32()
> 
> Agree, will re-use
> 
>>> +}
>>> +
>>> +/*
>>> + * Reset vector for secondary CPUs.
>>> + * This will be mapped at address 0 by SBAR register.
>>> + * We need _long_ jump to the physical address.
>>> + */
>>> +asm("    .arm\n"
>>> +    "    .align 12\n"
>>> +    "    .globl shmobile_boot_vector\n"
>>> +    "shmobile_boot_vector:\n"
>>> +    "    ldr r1, 1f\n"
>>> +    "    bx    r1\n"
>>> +    "    .type shmobile_boot_vector, %function\n"
>>> +    "    .size shmobile_boot_vector, .-shmobile_boot_vector\n"
>>> +    "    .align    2\n"
>>> +    "    .globl    shmobile_boot_fn\n"
>>> +    "shmobile_boot_fn:\n"
>>> +    "1:    .space    4\n"
>>> +    "    .globl    shmobile_boot_size\n"
>>> +    "shmobile_boot_size:\n"
>>> +    "    .long    .-shmobile_boot_vector\n");
>> Why can't this be implemented in C ?
> 
> This "reset vector" code was ported from Linux:
> 
> https://elixir.bootlin.com/linux/v5.0-rc5/source/arch/arm/mach-shmobile/headsmp.S#L21
> 
> 
> Really don't know whether it can be implemented in C.
> 
> I was thinking of moving this code to a separate ASM file in order not
> to mix C and ASM. What do you think about it?

U-Boot already has a reset vector code, can't that be reused ?

>>> +extern void shmobile_boot_vector(void);
>>> +extern unsigned long shmobile_boot_fn;
>>> +extern unsigned long shmobile_boot_size;
>> Are the externs necessary ?
> 
> Yes, otherwise, compiler will complain.
> 
> I can hide them in a header file. Shall I do that with moving ASM code
> to a separate file?

Only if you cannot reuse the U-Boot reset vector ones , which I think
you can ?

>>> +void r8a7790_prepare_cpus(unsigned long addr, u32 nr_cpus)
>>> +{
>>> +    int cpu = get_current_cpu();
>>> +    int i, ret = 0;
>>> +    u32 bar;
>>> +
>>> +    shmobile_boot_fn = addr;
>>> +    dsb();
>>> +
>>> +    /* Install reset vector */
>>> +    memcpy_toio((void __iomem *)ICRAM1, shmobile_boot_vector,
>>> +            shmobile_boot_size);
>>> +    dmb();
>> Does this take into consideration caches ?
> 
> Good question... Probably, I should have added flush_dcache_range()
> after copy.
> 
>>> +    /* Setup reset vectors */
>>> +    bar = (ICRAM1 >> 8) & 0xfffffc00;
>>> +    writel(bar, RST_BASE + CA15BAR);
>>> +    writel(bar | SBAR_BAREN, RST_BASE + CA15BAR);
>>> +    writel(bar, RST_BASE + CA7BAR);
>>> +    writel(bar | SBAR_BAREN, RST_BASE + CA7BAR);
>>> +    dsb();
>>> +
>>> +    /* Perform setup for debug mode for all CPUs */
>>> +    for (i = 0; i < nr_cpus; i++)
>>> +        apmu_setup_debug_mode(i);
>>> +
>>> +    /*
>>> +     * Indicate the completion status of power shutoff/resume procedure
>>> +     * for CA15/CA7 SCU.
>>> +     */
>>> +    writel(BIT(CA15_SCU_ISR_BIT) | BIT(CA7_SCU_ISR_BIT),
>>> +           SYSC_BASE + SYSCIER);
>>> +
>>> +    /* Power on CA15/CA7 SCU */
>>> +    if (sysc_power_is_off(CA15_SCU))
>>> +        ret += sysc_power_up(CA15_SCU);
>>> +    if (sysc_power_is_off(CA7_SCU))
>>> +        ret += sysc_power_up(CA7_SCU);
>>> +    if (ret)
>>> +        printf("warning: some of secondary CPUs may not boot\n");
>> "some" is not very useful for debugging,.
> 
> OK, will provide more concrete output.
> 
>>> +    /* Keep secondary CPUs in reset */
>>> +    for (i = 0; i < nr_cpus; i++) {
>>> +        /* Make sure that we don't reset boot CPU */
>>> +        if (i == cpu)
>>> +            continue;
>>> +
>>> +        r8a7790_assert_reset(i);
>>> +    }
>>> +
>>> +    /*
>>> +     * Enable snoop requests and DVM message requests for slave
>>> interfaces
>>> +     * S4 (CA7) and S3 (CA15).
>>> +     */
>>> +    writel(readl(CCI_BASE + CCI_SLAVE3 + CCI_SNOOP) | CCI_ENABLE_REQ,
>>> +           CCI_BASE + CCI_SLAVE3 + CCI_SNOOP);
>>> +    writel(readl(CCI_BASE + CCI_SLAVE4 + CCI_SNOOP) | CCI_ENABLE_REQ,
>>> +           CCI_BASE + CCI_SLAVE4 + CCI_SNOOP);
>>> +    /* Wait for pending bit low */
>>> +    while (readl(CCI_BASE + CCI_STATUS))
>>> +        ;
>> can this spin forever ?
> 
> Paragraph "3.3.4 Status Register" in "ARM CoreLink™CCI-400 Cache
> Coherent Interconnect" document says nothing
> 
> about possibility to spin forever. Just next:
> 
> "After writing to the snoop or DVM enable bits, the controller must wait
> for the register write to
> complete, then test that the change_pending bit is LOW before it turns
> an attached device on or off."

So what if this code spins forever, will this also completely hang Linux
which calls this code ?
[...]

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 2/3] ARM: rmobile: Add basic PSCI support for r8a7790 SoC

[Oleksandr]

On 09.02.19 18:32, Marek Vasut wrote:
> On 2/8/19 11:52 AM, Oleksandr wrote:
>> On 05.02.19 20:55, Marek Vasut wrote:
>>
>> Hi Marek
> Hi,

Hi Marek

>>> On 1/31/19 6:38 PM, Oleksandr Tyshchenko wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> Also enable PSCI support for Stout and Lager boards where
>>>> actually the r8a7790 SoC is installed.
>>>>
>>>> All secondary CPUs will be switched to a non-secure HYP mode
>>>> after booting.
>>>>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> ---
>>>>    arch/arm/mach-rmobile/Kconfig.32   |   2 +
>>>>    arch/arm/mach-rmobile/Makefile     |   6 +
>>>>    arch/arm/mach-rmobile/pm-r8a7790.c | 408
>>>> +++++++++++++++++++++++++++++++++++++
>>>>    arch/arm/mach-rmobile/pm-r8a7790.h |  72 +++++++
>>>>    arch/arm/mach-rmobile/psci.c       | 193 ++++++++++++++++++
>>>>    include/configs/lager.h            |   2 +
>>>>    include/configs/stout.h            |   2 +
>>>>    7 files changed, 685 insertions(+)
>>>>    create mode 100644 arch/arm/mach-rmobile/pm-r8a7790.c
>>>>    create mode 100644 arch/arm/mach-rmobile/pm-r8a7790.h
>>>>    create mode 100644 arch/arm/mach-rmobile/psci.c
>>>>
>>>> diff --git a/arch/arm/mach-rmobile/Kconfig.32
>>>> b/arch/arm/mach-rmobile/Kconfig.32
>>>> index a2e9e3d..728c323 100644
>>>> --- a/arch/arm/mach-rmobile/Kconfig.32
>>>> +++ b/arch/arm/mach-rmobile/Kconfig.32
>>>> @@ -78,6 +78,7 @@ config TARGET_LAGER
>>>>        imply CMD_DM
>>>>        select CPU_V7_HAS_NONSEC
>>>>        select CPU_V7_HAS_VIRT
>>>> +    select ARCH_SUPPORT_PSCI
>>>>      config TARGET_KZM9G
>>>>        bool "KZM9D board"
>>>> @@ -119,6 +120,7 @@ config TARGET_STOUT
>>>>        imply CMD_DM
>>>>        select CPU_V7_HAS_NONSEC
>>>>        select CPU_V7_HAS_VIRT
>>>> +    select ARCH_SUPPORT_PSCI
>> To myself: Move this option under "config R8A7790".
>>
>>>>      endchoice
>>>>    diff --git a/arch/arm/mach-rmobile/Makefile
>>>> b/arch/arm/mach-rmobile/Makefile
>>>> index 1f26ada..6f4c513 100644
>>>> --- a/arch/arm/mach-rmobile/Makefile
>>>> +++ b/arch/arm/mach-rmobile/Makefile
>>>> @@ -13,3 +13,9 @@ obj-$(CONFIG_SH73A0) += lowlevel_init.o
>>>> cpu_info-sh73a0.o pfc-sh73a0.o
>>>>    obj-$(CONFIG_R8A7740) += lowlevel_init.o cpu_info-r8a7740.o
>>>> pfc-r8a7740.o
>>>>    obj-$(CONFIG_RCAR_GEN2) += lowlevel_init_ca15.o cpu_info-rcar.o
>>>>    obj-$(CONFIG_RCAR_GEN3) += lowlevel_init_gen3.o cpu_info-rcar.o
>>>> memmap-gen3.o
>>>> +
>>>> +ifndef CONFIG_SPL_BUILD
>>>> +ifdef CONFIG_R8A7790
>>>> +obj-$(CONFIG_ARMV7_PSCI) += psci.o pm-r8a7790.o
>>>> +endif
>>>> +endif
>>>> diff --git a/arch/arm/mach-rmobile/pm-r8a7790.c
>>>> b/arch/arm/mach-rmobile/pm-r8a7790.c
>>>> new file mode 100644
>>>> index 0000000..c635cf6
>>>> --- /dev/null
>>>> +++ b/arch/arm/mach-rmobile/pm-r8a7790.c
>>>> @@ -0,0 +1,408 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * CPU power management support for Renesas r8a7790 SoC
>>>> + *
>>>> + * Contains functions to control ARM Cortex A15/A7 cores and
>>>> + * related peripherals basically used for PSCI.
>>>> + *
>>>> + * Copyright (C) 2018 EPAM Systems Inc.
>>>> + *
>>>> + * Mainly based on Renesas R-Car Gen2 platform code from Linux:
>>>> + *    arch/arm/mach-shmobile/...
>>>> + */
>>>> +
>>>> +#include <common.h>
>>>> +#include <asm/secure.h>
>>>> +#include <asm/io.h>
>>>> +
>>>> +#include "pm-r8a7790.h"
>>>> +
>>>> +/*****************************************************************************
>>>>
>>> I'd expect checkpatch to complain about these long lines of asterisks.
>> No, there was no complain about it. I have checked. Anyway, I can remove
>> them if required.
> Yes please, keep the comment style consistent with the rest of the
> codebase, which is also the kernel comment style.

OK, will remove


>
>>>> + * APMU definitions
>>>> +
>>>> *****************************************************************************/
>>>>
>>>> +#define CA15_APMU_BASE    0xe6152000
>>>> +#define CA7_APMU_BASE    0xe6151000
>>> All these addresses should come from DT. And the driver should be DM
>>> capable and live in drivers/
>>>
>>> [...]
>> All PSCI backends for ARMV7 in U-Boot which I was able to found, are
>> located either in arch/arm/cpu/armv7/
>>
>> or in arch/arm/mach-X. As well as other PSCI backends, this one will be
>> located in a separate secure section and acts as secure monitor,
>>
>> so it will be still alive, when U-Boot is gone away. Do we really want
>> this one to go into drivers?
> I'd much prefer it if we stopped adding more stuff to arch/arm/mach-* ,
> but I think we cannot avoid that in this case, can we ?

I am afraid, we can't avoid.


>
>>>> +/*****************************************************************************
>>>>
>>>> + * Functions which intended to be called from PSCI handlers. These
>>>> functions
>>>> + * marked as __secure and are placed in .secure section.
>>>> +
>>>> *****************************************************************************/
>>>>
>>>> +void __secure r8a7790_apmu_power_on(int cpu)
>>>> +{
>>>> +    int cluster = r8a7790_cluster_id(cpu);
>>>> +    u32 apmu_base;
>>>> +
>>>> +    apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
>>>> +
>>>> +    /* Request power on */
>>>> +    writel(BIT(r8a7790_core_id(cpu)), apmu_base + WUPCR_OFFS);
>>> wait_for_bit of some sorts ?
>> probably yes, will re-use
>>>> +    /* Wait for APMU to finish */
>>>> +    while (readl(apmu_base + WUPCR_OFFS))
>>>> +        ;
>>> Can this spin forever ?
>> I didn't find anything in TRM which describes how long it may take.
>> Linux also doesn't use timeout.
>>
>> https://elixir.bootlin.com/linux/v5.0-rc5/source/arch/arm/mach-shmobile/platsmp-apmu.c#L46
>>
>>
>> Shall I add some timeout (probably 1s) in order to be completely safe?
> I think so, because if you start spinning in here forever, the system
> will just completely freeze without any way of recovering.

Yes, the CPU executing that PSCI request (SMC) will block here.


> I don't think
> that's what you want to happen while using virtualization, right ?

Absolutely, will use timeout.


>
>>>> +}
>>>> +
>>>> +void __secure r8a7790_apmu_power_off(int cpu)
>>>> +{
>>>> +    int cluster = r8a7790_cluster_id(cpu);
>>>> +    u32 apmu_base;
>>>> +
>>>> +    apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
>>>> +
>>>> +    /* Request Core Standby for next WFI */
>>>> +    writel(CPUPWR_STANDBY, apmu_base +
>>>> CPUNCR_OFFS(r8a7790_core_id(cpu)));
>>>> +}
>>>> +
>>>> +void __secure r8a7790_assert_reset(int cpu)
>>>> +{
>>>> +    int cluster = r8a7790_cluster_id(cpu);
>>>> +    u32 mask, magic, rescnt;
>>>> +
>>>> +    mask = BIT(3 - r8a7790_core_id(cpu));
>>>> +    magic = cluster == 0 ? CA15RESCNT_CODE : CA7RESCNT_CODE;
>>>> +    rescnt = RST_BASE + (cluster == 0 ? CA15RESCNT : CA7RESCNT);
>>>> +    writel((readl(rescnt) | mask) | magic, rescnt);
>>>> +}
>>>> +
>>>> +void __secure r8a7790_deassert_reset(int cpu)
>>>> +{
>>>> +    int cluster = r8a7790_cluster_id(cpu);
>>>> +    u32 mask, magic, rescnt;
>>>> +
>>>> +    mask = BIT(3 - r8a7790_core_id(cpu));
>>>> +    magic = cluster == 0 ? CA15RESCNT_CODE : CA7RESCNT_CODE;
>>>> +    rescnt = RST_BASE + (cluster == 0 ? CA15RESCNT : CA7RESCNT);
>>>> +    writel((readl(rescnt) & ~mask) | magic, rescnt);
>>>> +}
>>>> +
>>>> +void __secure r8a7790_system_reset(void)
>>>> +{
>>>> +    u32 bar;
>>>> +
>>>> +    /*
>>>> +     * Before configuring internal watchdog timer (RWDT) to reboot
>>>> system
>>>> +     * we need to re-program BAR registers for the boot CPU to jump to
>>>> +     * bootrom code. Without taking care of, the boot CPU will jump to
>>>> +     * the reset vector previously installed in ICRAM1, since BAR
>>>> registers
>>>> +     * keep their values after watchdog triggered reset.
>>>> +     */
>>>> +    bar = (BOOTROM >> 8) & 0xfffffc00;
>>>> +    writel(bar, RST_BASE + CA15BAR);
>>>> +    writel(bar | SBAR_BAREN, RST_BASE + CA15BAR);
>>>> +    writel(bar, RST_BASE + CA7BAR);
>>>> +    writel(bar | SBAR_BAREN, RST_BASE + CA7BAR);
>>>> +    dsb();
>>>> +
>>>> +    /* Now, configure watchdog timer to reboot the system */
>>>> +
>>>> +    /* Trigger reset when counter overflows */
>>>> +    writel(WDTRSTCR_CODE | 0x2, RST_BASE + WDTRSTCR);
>>>> +    dsb();
>>>> +
>>>> +    /* Stop counter */
>>>> +    writel(RWTCSRA_CODE, RWDT_BASE + RWTCSRA);
>>>> +
>>>> +    /* Initialize counter with the highest value  */
>>>> +    writel(RWTCNT_CODE | 0xffff, RWDT_BASE + RWTCNT);
>>>> +
>>>> +    while (readb(RWDT_BASE + RWTCSRA) & RWTCSRA_WRFLG)
>>>> +        ;
>>>> +
>>>> +    /* Start counter */
>>>> +    writel(RWTCSRA_CODE | RWTCSRA_TME, RWDT_BASE + RWTCSRA);
>>> This seems to reimplement the reset code that's in U-Boot already. Why ?
>> Yes. I had to re-implement. Let me describe why.
>>
>>  From my understanding (I may mistake), the PSCI backend code which lives
>> in secure section should be as lightweight as possible
>>
>> and shouldn't call any U-Boot routines not marked as __secure, except
>> simple static inline functions.
>>
>> You can see PSCI implementation for any platform in U-Boot,  and only
>> simple "macroses/inlines" are used across all of them.
>>
>> Even for realizing some delay in code, they have specially implemented
>> something simple. As an example __mdelay() realization here:
>>
>> https://elixir.bootlin.com/u-boot/v2019.01/source/arch/arm/cpu/armv7/sunxi/psci.c#L61
> Can the U-Boot code be refactored somehow to avoid the duplication ?

Sorry, what duplication you are speaking about?


>
>> I know that PMIC (for Lager) and CPLD (for Stout) assist SoC to perform
>> reset. But, I couldn't use that functional here in PSCI backend, as it
>> pulls a lot of code (i2c for PMIC, gpio manipulation for CPLD, etc),
> How can the reset work properly if the CPLD/PMIC isn't even used then ?


We ask WDT to perform a CPU reset, although this is not the same reset 
as an external reset from CPLD/PMIC,

but, why not use it, if we don't have alternative? This is certainly 
better than nothing, I think.

Actually, we ask WDT to do what it is intended to do, and it seems to 
work properly as the system up and running after WDT reset in 100% cases.

What is more, this PSCI reset implementation could be common for Gen2 
SoCs where WDT is present...


>> so for that reason (AFAIK the PSCI system reset is a mandatory option) I
>> chose WDT as a entity for doing a reset. This is quite simple and can be
>> used on both boards, what is more that it can be used on other Gen2 SoC
>> if required.
>>
>>>> +}
>>>> +
>>>> +/*****************************************************************************
>>>>
>>>> + * Functions which intended to be called from PSCI board
>>>> initialization.
>>>> +
>>>> *****************************************************************************/
>>>>
>>>> +static int sysc_power_up(int scu)
>>>> +{
>>>> +    u32 status, chan_offs, isr_bit;
>>>> +    int i, j, ret = 0;
>>>> +
>>>> +    if (scu == CA15_SCU) {
>>>> +        chan_offs = CA15_SCU_CHAN_OFFS;
>>>> +        isr_bit = CA15_SCU_ISR_BIT;
>>>> +    } else {
>>>> +        chan_offs = CA7_SCU_CHAN_OFFS;
>>>> +        isr_bit = CA7_SCU_ISR_BIT;
>>>> +    }
>>>> +
>>>> +    writel(BIT(isr_bit), SYSC_BASE + SYSCISCR);
>>>> +
>>>> +    /* Submit power resume request until it was accepted */
>>>> +    for (i = 0; i < PWRER_RETRIES; i++) {
>>>> +        /* Wait until SYSC is ready to accept a power resume request */
>>>> +        for (j = 0; j < SYSCSR_RETRIES; j++) {
>>>> +            if (readl(SYSC_BASE + SYSCSR) & BIT(1))
>>>> +                break;
>>>> +
>>>> +            udelay(SYSCSR_DELAY_US);
>>>> +        }
>>>> +
>>>> +        if (j == SYSCSR_RETRIES)
>>>> +            return -EAGAIN;
>>>> +
>>>> +        /* Submit power resume request */
>>>> +        writel(BIT(0), SYSC_BASE + chan_offs + PWRONCR_OFFS);
>>>> +
>>>> +        /* Check if power resume request was accepted */
>>>> +        status = readl(SYSC_BASE + chan_offs + PWRER_OFFS);
>>>> +        if (!(status & BIT(0)))
>>>> +            break;
>>>> +
>>>> +        udelay(PWRER_DELAY_US);
>>>> +    }
>>>> +
>>>> +    if (i == PWRER_RETRIES)
>>>> +        return -EIO;
>>>> +
>>>> +    /* Wait until the power resume request has completed */
>>>> +    for (i = 0; i < SYSCISR_RETRIES; i++) {
>>>> +        if (readl(SYSC_BASE + SYSCISR) & BIT(isr_bit))
>>>> +            break;
>>>> +        udelay(SYSCISR_DELAY_US);
>>>> +    }
>>>> +
>>>> +    if (i == SYSCISR_RETRIES)
>>>> +        ret = -EIO;
>>>> +
>>>> +    writel(BIT(isr_bit), SYSC_BASE + SYSCISCR);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static bool sysc_power_is_off(int scu)
>>>> +{
>>>> +    u32 status, chan_offs;
>>>> +
>>>> +    chan_offs = scu == CA15_SCU ? CA15_SCU_CHAN_OFFS :
>>>> CA7_SCU_CHAN_OFFS;
>>>> +
>>>> +    /* Check if SCU is in power shutoff state */
>>>> +    status = readl(SYSC_BASE + chan_offs + PWRSR_OFFS);
>>>> +    if (status & BIT(0))
>>>> +        return true;
>>>> +
>>>> +    return false;
>>>> +}
>>>> +
>>>> +static void apmu_setup_debug_mode(int cpu)
>>>> +{
>>>> +    int cluster = r8a7790_cluster_id(cpu);
>>>> +    u32 apmu_base, reg;
>>>> +
>>>> +    apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
>>>> +
>>>> +    /*
>>>> +     * Enable reset requests derived from power shutoff to the
>>>> AP-system
>>>> +     * CPU cores in debug mode. Without taking care of, they may
>>>> fail to
>>>> +     * resume from the power shutoff state.
>>>> +     */
>>>> +    reg = readl(apmu_base + DBGRCR_OFFS);
>>>> +    reg |= DBGCPUREN | DBGCPUNREN(r8a7790_core_id(cpu)) | DBGCPUPREN;
>>>> +    writel(reg, apmu_base + DBGRCR_OFFS);
>>> setbits_le32()
>> Agree, will re-use
>>
>>>> +}
>>>> +
>>>> +/*
>>>> + * Reset vector for secondary CPUs.
>>>> + * This will be mapped at address 0 by SBAR register.
>>>> + * We need _long_ jump to the physical address.
>>>> + */
>>>> +asm("    .arm\n"
>>>> +    "    .align 12\n"
>>>> +    "    .globl shmobile_boot_vector\n"
>>>> +    "shmobile_boot_vector:\n"
>>>> +    "    ldr r1, 1f\n"
>>>> +    "    bx    r1\n"
>>>> +    "    .type shmobile_boot_vector, %function\n"
>>>> +    "    .size shmobile_boot_vector, .-shmobile_boot_vector\n"
>>>> +    "    .align    2\n"
>>>> +    "    .globl    shmobile_boot_fn\n"
>>>> +    "shmobile_boot_fn:\n"
>>>> +    "1:    .space    4\n"
>>>> +    "    .globl    shmobile_boot_size\n"
>>>> +    "shmobile_boot_size:\n"
>>>> +    "    .long    .-shmobile_boot_vector\n");
>>> Why can't this be implemented in C ?
>> This "reset vector" code was ported from Linux:
>>
>> https://elixir.bootlin.com/linux/v5.0-rc5/source/arch/arm/mach-shmobile/headsmp.S#L21
>>
>>
>> Really don't know whether it can be implemented in C.
>>
>> I was thinking of moving this code to a separate ASM file in order not
>> to mix C and ASM. What do you think about it?
> U-Boot already has a reset vector code, can't that be reused ?

I don't think. Being honest, I couldn't find an obvious way how to reuse 
(I assume you meant arch/arm/cpu/armv7/start.S).

The newly turned on secondary CPU entry should be common 
"psci_cpu_entry", which does proper things.

And this reset vector is just "a small piece of code" to be located in 
on-chip RAM (with limited size) and used for the jump stub...


>
>>>> +extern void shmobile_boot_vector(void);
>>>> +extern unsigned long shmobile_boot_fn;
>>>> +extern unsigned long shmobile_boot_size;
>>> Are the externs necessary ?
>> Yes, otherwise, compiler will complain.
>>
>> I can hide them in a header file. Shall I do that with moving ASM code
>> to a separate file?
> Only if you cannot reuse the U-Boot reset vector ones , which I think
> you can ?

I tried to explain above, why I can't reuse. So, if you and other 
reviewers OK with it,

I will move it to a separate file in V2.


>
>>>> +void r8a7790_prepare_cpus(unsigned long addr, u32 nr_cpus)
>>>> +{
>>>> +    int cpu = get_current_cpu();
>>>> +    int i, ret = 0;
>>>> +    u32 bar;
>>>> +
>>>> +    shmobile_boot_fn = addr;
>>>> +    dsb();
>>>> +
>>>> +    /* Install reset vector */
>>>> +    memcpy_toio((void __iomem *)ICRAM1, shmobile_boot_vector,
>>>> +            shmobile_boot_size);
>>>> +    dmb();
>>> Does this take into consideration caches ?
>> Good question... Probably, I should have added flush_dcache_range()
>> after copy.
>>
>>>> +    /* Setup reset vectors */
>>>> +    bar = (ICRAM1 >> 8) & 0xfffffc00;
>>>> +    writel(bar, RST_BASE + CA15BAR);
>>>> +    writel(bar | SBAR_BAREN, RST_BASE + CA15BAR);
>>>> +    writel(bar, RST_BASE + CA7BAR);
>>>> +    writel(bar | SBAR_BAREN, RST_BASE + CA7BAR);
>>>> +    dsb();
>>>> +
>>>> +    /* Perform setup for debug mode for all CPUs */
>>>> +    for (i = 0; i < nr_cpus; i++)
>>>> +        apmu_setup_debug_mode(i);
>>>> +
>>>> +    /*
>>>> +     * Indicate the completion status of power shutoff/resume procedure
>>>> +     * for CA15/CA7 SCU.
>>>> +     */
>>>> +    writel(BIT(CA15_SCU_ISR_BIT) | BIT(CA7_SCU_ISR_BIT),
>>>> +           SYSC_BASE + SYSCIER);
>>>> +
>>>> +    /* Power on CA15/CA7 SCU */
>>>> +    if (sysc_power_is_off(CA15_SCU))
>>>> +        ret += sysc_power_up(CA15_SCU);
>>>> +    if (sysc_power_is_off(CA7_SCU))
>>>> +        ret += sysc_power_up(CA7_SCU);
>>>> +    if (ret)
>>>> +        printf("warning: some of secondary CPUs may not boot\n");
>>> "some" is not very useful for debugging,.
>> OK, will provide more concrete output.
>>
>>>> +    /* Keep secondary CPUs in reset */
>>>> +    for (i = 0; i < nr_cpus; i++) {
>>>> +        /* Make sure that we don't reset boot CPU */
>>>> +        if (i == cpu)
>>>> +            continue;
>>>> +
>>>> +        r8a7790_assert_reset(i);
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * Enable snoop requests and DVM message requests for slave
>>>> interfaces
>>>> +     * S4 (CA7) and S3 (CA15).
>>>> +     */
>>>> +    writel(readl(CCI_BASE + CCI_SLAVE3 + CCI_SNOOP) | CCI_ENABLE_REQ,
>>>> +           CCI_BASE + CCI_SLAVE3 + CCI_SNOOP);
>>>> +    writel(readl(CCI_BASE + CCI_SLAVE4 + CCI_SNOOP) | CCI_ENABLE_REQ,
>>>> +           CCI_BASE + CCI_SLAVE4 + CCI_SNOOP);
>>>> +    /* Wait for pending bit low */
>>>> +    while (readl(CCI_BASE + CCI_STATUS))
>>>> +        ;
>>> can this spin forever ?
>> Paragraph "3.3.4 Status Register" in "ARM CoreLink™CCI-400 Cache
>> Coherent Interconnect" document says nothing
>>
>> about possibility to spin forever. Just next:
>>
>> "After writing to the snoop or DVM enable bits, the controller must wait
>> for the register write to
>> complete, then test that the change_pending bit is LOW before it turns
>> an attached device on or off."
> So what if this code spins forever, will this also completely hang Linux
> which calls this code ?
> [...]


In this case, no. This is PSCI board initialization code, which is being 
executed while we are still in U-Boot.

Unlike r8a7790_apmu_power_on()/r8a7790_apmu_power_off() functions, which 
are indented to be called from PSCI handlers (at runtime), when the 
"main" U-Boot is gone away.

If this code spins forever, U-Boot will get stuck before even starting 
Linux/Hypervisor.

Probably, it would be better to add safe timeout (~1s) here as well.


>
-- 
Regards,

Oleksandr Tyshchenko

[U-Boot] [PATCH 2/3] ARM: rmobile: Add basic PSCI support for r8a7790 SoC

[Marek Vasut]

On 2/11/19 9:10 PM, Oleksandr wrote:

[...]

>>> Yes. I had to re-implement. Let me describe why.
>>>
>>>  From my understanding (I may mistake), the PSCI backend code which
>>> lives
>>> in secure section should be as lightweight as possible
>>>
>>> and shouldn't call any U-Boot routines not marked as __secure, except
>>> simple static inline functions.
>>>
>>> You can see PSCI implementation for any platform in U-Boot,  and only
>>> simple "macroses/inlines" are used across all of them.
>>>
>>> Even for realizing some delay in code, they have specially implemented
>>> something simple. As an example __mdelay() realization here:
>>>
>>> https://elixir.bootlin.com/u-boot/v2019.01/source/arch/arm/cpu/armv7/sunxi/psci.c#L61
>>>
>> Can the U-Boot code be refactored somehow to avoid the duplication ?
> 
> Sorry, what duplication you are speaking about?

It is my impression that we're reimplementing code we already have
either in drivers/ or in Linux, again, in arch/arm/ . Isn't it the case ?

>>> I know that PMIC (for Lager) and CPLD (for Stout) assist SoC to perform
>>> reset. But, I couldn't use that functional here in PSCI backend, as it
>>> pulls a lot of code (i2c for PMIC, gpio manipulation for CPLD, etc),
>> How can the reset work properly if the CPLD/PMIC isn't even used then ?
> 
> 
> We ask WDT to perform a CPU reset, although this is not the same reset
> as an external reset from CPLD/PMIC,
> 
> but, why not use it, if we don't have alternative? This is certainly
> better than nothing, I think.

Do we need to do a full board reset in this case ?

> Actually, we ask WDT to do what it is intended to do, and it seems to
> work properly as the system up and running after WDT reset in 100% cases.
> 
> What is more, this PSCI reset implementation could be common for Gen2
> SoCs where WDT is present...
> 
> 
>>> so for that reason (AFAIK the PSCI system reset is a mandatory option) I
>>> chose WDT as a entity for doing a reset. This is quite simple and can be
>>> used on both boards, what is more that it can be used on other Gen2 SoC
>>> if required.
>>>
>>>>> +}
>>>>> +
>>>>> +/*****************************************************************************
>>>>>
>>>>>
>>>>> + * Functions which intended to be called from PSCI board
>>>>> initialization.
>>>>> +
>>>>> *****************************************************************************/
>>>>>
>>>>>
>>>>> +static int sysc_power_up(int scu)
>>>>> +{
>>>>> +    u32 status, chan_offs, isr_bit;
>>>>> +    int i, j, ret = 0;
>>>>> +
>>>>> +    if (scu == CA15_SCU) {
>>>>> +        chan_offs = CA15_SCU_CHAN_OFFS;
>>>>> +        isr_bit = CA15_SCU_ISR_BIT;
>>>>> +    } else {
>>>>> +        chan_offs = CA7_SCU_CHAN_OFFS;
>>>>> +        isr_bit = CA7_SCU_ISR_BIT;
>>>>> +    }
>>>>> +
>>>>> +    writel(BIT(isr_bit), SYSC_BASE + SYSCISCR);
>>>>> +
>>>>> +    /* Submit power resume request until it was accepted */
>>>>> +    for (i = 0; i < PWRER_RETRIES; i++) {
>>>>> +        /* Wait until SYSC is ready to accept a power resume
>>>>> request */
>>>>> +        for (j = 0; j < SYSCSR_RETRIES; j++) {
>>>>> +            if (readl(SYSC_BASE + SYSCSR) & BIT(1))
>>>>> +                break;
>>>>> +
>>>>> +            udelay(SYSCSR_DELAY_US);
>>>>> +        }
>>>>> +
>>>>> +        if (j == SYSCSR_RETRIES)
>>>>> +            return -EAGAIN;
>>>>> +
>>>>> +        /* Submit power resume request */
>>>>> +        writel(BIT(0), SYSC_BASE + chan_offs + PWRONCR_OFFS);
>>>>> +
>>>>> +        /* Check if power resume request was accepted */
>>>>> +        status = readl(SYSC_BASE + chan_offs + PWRER_OFFS);
>>>>> +        if (!(status & BIT(0)))
>>>>> +            break;
>>>>> +
>>>>> +        udelay(PWRER_DELAY_US);
>>>>> +    }
>>>>> +
>>>>> +    if (i == PWRER_RETRIES)
>>>>> +        return -EIO;
>>>>> +
>>>>> +    /* Wait until the power resume request has completed */
>>>>> +    for (i = 0; i < SYSCISR_RETRIES; i++) {
>>>>> +        if (readl(SYSC_BASE + SYSCISR) & BIT(isr_bit))
>>>>> +            break;
>>>>> +        udelay(SYSCISR_DELAY_US);
>>>>> +    }
>>>>> +
>>>>> +    if (i == SYSCISR_RETRIES)
>>>>> +        ret = -EIO;
>>>>> +
>>>>> +    writel(BIT(isr_bit), SYSC_BASE + SYSCISCR);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static bool sysc_power_is_off(int scu)
>>>>> +{
>>>>> +    u32 status, chan_offs;
>>>>> +
>>>>> +    chan_offs = scu == CA15_SCU ? CA15_SCU_CHAN_OFFS :
>>>>> CA7_SCU_CHAN_OFFS;
>>>>> +
>>>>> +    /* Check if SCU is in power shutoff state */
>>>>> +    status = readl(SYSC_BASE + chan_offs + PWRSR_OFFS);
>>>>> +    if (status & BIT(0))
>>>>> +        return true;
>>>>> +
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>> +static void apmu_setup_debug_mode(int cpu)
>>>>> +{
>>>>> +    int cluster = r8a7790_cluster_id(cpu);
>>>>> +    u32 apmu_base, reg;
>>>>> +
>>>>> +    apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
>>>>> +
>>>>> +    /*
>>>>> +     * Enable reset requests derived from power shutoff to the
>>>>> AP-system
>>>>> +     * CPU cores in debug mode. Without taking care of, they may
>>>>> fail to
>>>>> +     * resume from the power shutoff state.
>>>>> +     */
>>>>> +    reg = readl(apmu_base + DBGRCR_OFFS);
>>>>> +    reg |= DBGCPUREN | DBGCPUNREN(r8a7790_core_id(cpu)) | DBGCPUPREN;
>>>>> +    writel(reg, apmu_base + DBGRCR_OFFS);
>>>> setbits_le32()
>>> Agree, will re-use
>>>
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Reset vector for secondary CPUs.
>>>>> + * This will be mapped at address 0 by SBAR register.
>>>>> + * We need _long_ jump to the physical address.
>>>>> + */
>>>>> +asm("    .arm\n"
>>>>> +    "    .align 12\n"
>>>>> +    "    .globl shmobile_boot_vector\n"
>>>>> +    "shmobile_boot_vector:\n"
>>>>> +    "    ldr r1, 1f\n"
>>>>> +    "    bx    r1\n"
>>>>> +    "    .type shmobile_boot_vector, %function\n"
>>>>> +    "    .size shmobile_boot_vector, .-shmobile_boot_vector\n"
>>>>> +    "    .align    2\n"
>>>>> +    "    .globl    shmobile_boot_fn\n"
>>>>> +    "shmobile_boot_fn:\n"
>>>>> +    "1:    .space    4\n"
>>>>> +    "    .globl    shmobile_boot_size\n"
>>>>> +    "shmobile_boot_size:\n"
>>>>> +    "    .long    .-shmobile_boot_vector\n");
>>>> Why can't this be implemented in C ?
>>> This "reset vector" code was ported from Linux:
>>>
>>> https://elixir.bootlin.com/linux/v5.0-rc5/source/arch/arm/mach-shmobile/headsmp.S#L21
>>>
>>>
>>>
>>> Really don't know whether it can be implemented in C.
>>>
>>> I was thinking of moving this code to a separate ASM file in order not
>>> to mix C and ASM. What do you think about it?
>> U-Boot already has a reset vector code, can't that be reused ?
> 
> I don't think. Being honest, I couldn't find an obvious way how to reuse
> (I assume you meant arch/arm/cpu/armv7/start.S).

Maybe it needs some additional work first ?
It seems Altera socfpga somehow uses the U-Boot reset vectors for PSCI,
so it should at least be possible.

> The newly turned on secondary CPU entry should be common
> "psci_cpu_entry", which does proper things.
> 
> And this reset vector is just "a small piece of code" to be located in
> on-chip RAM (with limited size) and used for the jump stub...

We already have the SPL reset vectors in SRAM, maybe that can be
recycled somehow ?

>>>>> +extern void shmobile_boot_vector(void);
>>>>> +extern unsigned long shmobile_boot_fn;
>>>>> +extern unsigned long shmobile_boot_size;
>>>> Are the externs necessary ?
>>> Yes, otherwise, compiler will complain.
>>>
>>> I can hide them in a header file. Shall I do that with moving ASM code
>>> to a separate file?
>> Only if you cannot reuse the U-Boot reset vector ones , which I think
>> you can ?
> 
> I tried to explain above, why I can't reuse. So, if you and other
> reviewers OK with it,
> 
> I will move it to a separate file in V2.

I don't quite see why it's a problem to use the U-Boot reset vectors.
Sure, it might be needed to adjust that code first if it cannot be used
as-is.

>>>>> +void r8a7790_prepare_cpus(unsigned long addr, u32 nr_cpus)
>>>>> +{
>>>>> +    int cpu = get_current_cpu();
>>>>> +    int i, ret = 0;
>>>>> +    u32 bar;
>>>>> +
>>>>> +    shmobile_boot_fn = addr;
>>>>> +    dsb();
>>>>> +
>>>>> +    /* Install reset vector */
>>>>> +    memcpy_toio((void __iomem *)ICRAM1, shmobile_boot_vector,
>>>>> +            shmobile_boot_size);
>>>>> +    dmb();
>>>> Does this take into consideration caches ?
>>> Good question... Probably, I should have added flush_dcache_range()
>>> after copy.
>>>
>>>>> +    /* Setup reset vectors */
>>>>> +    bar = (ICRAM1 >> 8) & 0xfffffc00;
>>>>> +    writel(bar, RST_BASE + CA15BAR);
>>>>> +    writel(bar | SBAR_BAREN, RST_BASE + CA15BAR);
>>>>> +    writel(bar, RST_BASE + CA7BAR);
>>>>> +    writel(bar | SBAR_BAREN, RST_BASE + CA7BAR);
>>>>> +    dsb();
>>>>> +
>>>>> +    /* Perform setup for debug mode for all CPUs */
>>>>> +    for (i = 0; i < nr_cpus; i++)
>>>>> +        apmu_setup_debug_mode(i);
>>>>> +
>>>>> +    /*
>>>>> +     * Indicate the completion status of power shutoff/resume
>>>>> procedure
>>>>> +     * for CA15/CA7 SCU.
>>>>> +     */
>>>>> +    writel(BIT(CA15_SCU_ISR_BIT) | BIT(CA7_SCU_ISR_BIT),
>>>>> +           SYSC_BASE + SYSCIER);
>>>>> +
>>>>> +    /* Power on CA15/CA7 SCU */
>>>>> +    if (sysc_power_is_off(CA15_SCU))
>>>>> +        ret += sysc_power_up(CA15_SCU);
>>>>> +    if (sysc_power_is_off(CA7_SCU))
>>>>> +        ret += sysc_power_up(CA7_SCU);
>>>>> +    if (ret)
>>>>> +        printf("warning: some of secondary CPUs may not boot\n");
>>>> "some" is not very useful for debugging,.
>>> OK, will provide more concrete output.
>>>
>>>>> +    /* Keep secondary CPUs in reset */
>>>>> +    for (i = 0; i < nr_cpus; i++) {
>>>>> +        /* Make sure that we don't reset boot CPU */
>>>>> +        if (i == cpu)
>>>>> +            continue;
>>>>> +
>>>>> +        r8a7790_assert_reset(i);
>>>>> +    }
>>>>> +
>>>>> +    /*
>>>>> +     * Enable snoop requests and DVM message requests for slave
>>>>> interfaces
>>>>> +     * S4 (CA7) and S3 (CA15).
>>>>> +     */
>>>>> +    writel(readl(CCI_BASE + CCI_SLAVE3 + CCI_SNOOP) | CCI_ENABLE_REQ,
>>>>> +           CCI_BASE + CCI_SLAVE3 + CCI_SNOOP);
>>>>> +    writel(readl(CCI_BASE + CCI_SLAVE4 + CCI_SNOOP) | CCI_ENABLE_REQ,
>>>>> +           CCI_BASE + CCI_SLAVE4 + CCI_SNOOP);
>>>>> +    /* Wait for pending bit low */
>>>>> +    while (readl(CCI_BASE + CCI_STATUS))
>>>>> +        ;
>>>> can this spin forever ?
>>> Paragraph "3.3.4 Status Register" in "ARM CoreLink™CCI-400 Cache
>>> Coherent Interconnect" document says nothing
>>>
>>> about possibility to spin forever. Just next:
>>>
>>> "After writing to the snoop or DVM enable bits, the controller must wait
>>> for the register write to
>>> complete, then test that the change_pending bit is LOW before it turns
>>> an attached device on or off."
>> So what if this code spins forever, will this also completely hang Linux
>> which calls this code ?
>> [...]
> 
> 
> In this case, no. This is PSCI board initialization code, which is being
> executed while we are still in U-Boot.
> 
> Unlike r8a7790_apmu_power_on()/r8a7790_apmu_power_off() functions, which
> are indented to be called from PSCI handlers (at runtime), when the
> "main" U-Boot is gone away.
> 
> If this code spins forever, U-Boot will get stuck before even starting
> Linux/Hypervisor.
> 
> Probably, it would be better to add safe timeout (~1s) here as well.

I think so.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 2/3] ARM: rmobile: Add basic PSCI support for r8a7790 SoC

[Oleksandr]

On 11.02.19 22:40, Marek Vasut wrote:

Hi
> On 2/11/19 9:10 PM, Oleksandr wrote:
>
> [...]
>
>>>> Yes. I had to re-implement. Let me describe why.
>>>>
>>>>   From my understanding (I may mistake), the PSCI backend code which
>>>> lives
>>>> in secure section should be as lightweight as possible
>>>>
>>>> and shouldn't call any U-Boot routines not marked as __secure, except
>>>> simple static inline functions.
>>>>
>>>> You can see PSCI implementation for any platform in U-Boot,  and only
>>>> simple "macroses/inlines" are used across all of them.
>>>>
>>>> Even for realizing some delay in code, they have specially implemented
>>>> something simple. As an example __mdelay() realization here:
>>>>
>>>> https://elixir.bootlin.com/u-boot/v2019.01/source/arch/arm/cpu/armv7/sunxi/psci.c#L61
>>>>
>>> Can the U-Boot code be refactored somehow to avoid the duplication ?
>> Sorry, what duplication you are speaking about?
> It is my impression that we're reimplementing code we already have
> either in drivers/ or in Linux, again, in arch/arm/ . Isn't it the case ?

All this code (for preparing, powering up/down the CPUs and related 
peripherals) which this patch introduces, is new for U-Boot.

But, yes, it is present in Linux (arch/arm/mach-shmobile/...).


>
>>>> I know that PMIC (for Lager) and CPLD (for Stout) assist SoC to perform
>>>> reset. But, I couldn't use that functional here in PSCI backend, as it
>>>> pulls a lot of code (i2c for PMIC, gpio manipulation for CPLD, etc),
>>> How can the reset work properly if the CPLD/PMIC isn't even used then ?
>>
>> We ask WDT to perform a CPU reset, although this is not the same reset
>> as an external reset from CPLD/PMIC,
>>
>> but, why not use it, if we don't have alternative? This is certainly
>> better than nothing, I think.
> Do we need to do a full board reset in this case ?

After WDT reset CPU will be brought up to bootrom code, then starts 
executing SPL, U-Boot...

So, we will get all required SoC/Board peripherals re-initialized, I think.


>
>> Actually, we ask WDT to do what it is intended to do, and it seems to
>> work properly as the system up and running after WDT reset in 100% cases.
>>
>> What is more, this PSCI reset implementation could be common for Gen2
>> SoCs where WDT is present...
>>
>>
>>>> so for that reason (AFAIK the PSCI system reset is a mandatory option) I
>>>> chose WDT as a entity for doing a reset. This is quite simple and can be
>>>> used on both boards, what is more that it can be used on other Gen2 SoC
>>>> if required.
>>>>
>>>>>> +}
>>>>>> +
>>>>>> +/*****************************************************************************
>>>>>>
>>>>>>
>>>>>> + * Functions which intended to be called from PSCI board
>>>>>> initialization.
>>>>>> +
>>>>>> *****************************************************************************/
>>>>>>
>>>>>>
>>>>>> +static int sysc_power_up(int scu)
>>>>>> +{
>>>>>> +    u32 status, chan_offs, isr_bit;
>>>>>> +    int i, j, ret = 0;
>>>>>> +
>>>>>> +    if (scu == CA15_SCU) {
>>>>>> +        chan_offs = CA15_SCU_CHAN_OFFS;
>>>>>> +        isr_bit = CA15_SCU_ISR_BIT;
>>>>>> +    } else {
>>>>>> +        chan_offs = CA7_SCU_CHAN_OFFS;
>>>>>> +        isr_bit = CA7_SCU_ISR_BIT;
>>>>>> +    }
>>>>>> +
>>>>>> +    writel(BIT(isr_bit), SYSC_BASE + SYSCISCR);
>>>>>> +
>>>>>> +    /* Submit power resume request until it was accepted */
>>>>>> +    for (i = 0; i < PWRER_RETRIES; i++) {
>>>>>> +        /* Wait until SYSC is ready to accept a power resume
>>>>>> request */
>>>>>> +        for (j = 0; j < SYSCSR_RETRIES; j++) {
>>>>>> +            if (readl(SYSC_BASE + SYSCSR) & BIT(1))
>>>>>> +                break;
>>>>>> +
>>>>>> +            udelay(SYSCSR_DELAY_US);
>>>>>> +        }
>>>>>> +
>>>>>> +        if (j == SYSCSR_RETRIES)
>>>>>> +            return -EAGAIN;
>>>>>> +
>>>>>> +        /* Submit power resume request */
>>>>>> +        writel(BIT(0), SYSC_BASE + chan_offs + PWRONCR_OFFS);
>>>>>> +
>>>>>> +        /* Check if power resume request was accepted */
>>>>>> +        status = readl(SYSC_BASE + chan_offs + PWRER_OFFS);
>>>>>> +        if (!(status & BIT(0)))
>>>>>> +            break;
>>>>>> +
>>>>>> +        udelay(PWRER_DELAY_US);
>>>>>> +    }
>>>>>> +
>>>>>> +    if (i == PWRER_RETRIES)
>>>>>> +        return -EIO;
>>>>>> +
>>>>>> +    /* Wait until the power resume request has completed */
>>>>>> +    for (i = 0; i < SYSCISR_RETRIES; i++) {
>>>>>> +        if (readl(SYSC_BASE + SYSCISR) & BIT(isr_bit))
>>>>>> +            break;
>>>>>> +        udelay(SYSCISR_DELAY_US);
>>>>>> +    }
>>>>>> +
>>>>>> +    if (i == SYSCISR_RETRIES)
>>>>>> +        ret = -EIO;
>>>>>> +
>>>>>> +    writel(BIT(isr_bit), SYSC_BASE + SYSCISCR);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static bool sysc_power_is_off(int scu)
>>>>>> +{
>>>>>> +    u32 status, chan_offs;
>>>>>> +
>>>>>> +    chan_offs = scu == CA15_SCU ? CA15_SCU_CHAN_OFFS :
>>>>>> CA7_SCU_CHAN_OFFS;
>>>>>> +
>>>>>> +    /* Check if SCU is in power shutoff state */
>>>>>> +    status = readl(SYSC_BASE + chan_offs + PWRSR_OFFS);
>>>>>> +    if (status & BIT(0))
>>>>>> +        return true;
>>>>>> +
>>>>>> +    return false;
>>>>>> +}
>>>>>> +
>>>>>> +static void apmu_setup_debug_mode(int cpu)
>>>>>> +{
>>>>>> +    int cluster = r8a7790_cluster_id(cpu);
>>>>>> +    u32 apmu_base, reg;
>>>>>> +
>>>>>> +    apmu_base = cluster == 0 ? CA15_APMU_BASE : CA7_APMU_BASE;
>>>>>> +
>>>>>> +    /*
>>>>>> +     * Enable reset requests derived from power shutoff to the
>>>>>> AP-system
>>>>>> +     * CPU cores in debug mode. Without taking care of, they may
>>>>>> fail to
>>>>>> +     * resume from the power shutoff state.
>>>>>> +     */
>>>>>> +    reg = readl(apmu_base + DBGRCR_OFFS);
>>>>>> +    reg |= DBGCPUREN | DBGCPUNREN(r8a7790_core_id(cpu)) | DBGCPUPREN;
>>>>>> +    writel(reg, apmu_base + DBGRCR_OFFS);
>>>>> setbits_le32()
>>>> Agree, will re-use
>>>>
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * Reset vector for secondary CPUs.
>>>>>> + * This will be mapped at address 0 by SBAR register.
>>>>>> + * We need _long_ jump to the physical address.
>>>>>> + */
>>>>>> +asm("    .arm\n"
>>>>>> +    "    .align 12\n"
>>>>>> +    "    .globl shmobile_boot_vector\n"
>>>>>> +    "shmobile_boot_vector:\n"
>>>>>> +    "    ldr r1, 1f\n"
>>>>>> +    "    bx    r1\n"
>>>>>> +    "    .type shmobile_boot_vector, %function\n"
>>>>>> +    "    .size shmobile_boot_vector, .-shmobile_boot_vector\n"
>>>>>> +    "    .align    2\n"
>>>>>> +    "    .globl    shmobile_boot_fn\n"
>>>>>> +    "shmobile_boot_fn:\n"
>>>>>> +    "1:    .space    4\n"
>>>>>> +    "    .globl    shmobile_boot_size\n"
>>>>>> +    "shmobile_boot_size:\n"
>>>>>> +    "    .long    .-shmobile_boot_vector\n");
>>>>> Why can't this be implemented in C ?
>>>> This "reset vector" code was ported from Linux:
>>>>
>>>> https://elixir.bootlin.com/linux/v5.0-rc5/source/arch/arm/mach-shmobile/headsmp.S#L21
>>>>
>>>>
>>>>
>>>> Really don't know whether it can be implemented in C.
>>>>
>>>> I was thinking of moving this code to a separate ASM file in order not
>>>> to mix C and ASM. What do you think about it?
>>> U-Boot already has a reset vector code, can't that be reused ?
>> I don't think. Being honest, I couldn't find an obvious way how to reuse
>> (I assume you meant arch/arm/cpu/armv7/start.S).
> Maybe it needs some additional work first ?
> It seems Altera socfpga somehow uses the U-Boot reset vectors for PSCI,
> so it should at least be possible.

Could you, please, point me in code? Unfortunately, I wasn't able to find.


>
>> The newly turned on secondary CPU entry should be common
>> "psci_cpu_entry", which does proper things.
>>
>> And this reset vector is just "a small piece of code" to be located in
>> on-chip RAM (with limited size) and used for the jump stub...
> We already have the SPL reset vectors in SRAM, maybe that can be
> recycled somehow ?


The only idea I have, how it may be recycled (not sure whether it will 
work...)


diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index 0cb6dd39cc..69acf4677b 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -36,6 +36,12 @@
  #endif

  reset:
+
+#if defined(CONFIG_ARMV7_PSCI) && !defined(CONFIG_SPL_BUILD)
+       b psci_cpu_entry_jump
+       /* return only if this is not "a newly turned on CPU" using PSCI) */
+#endif
+
         /* Allow the board to save important registers */
         b       save_boot_params
  save_boot_params_ret:
@@ -128,6 +134,21 @@ ENDPROC(switch_to_hypervisor)
         .weak   switch_to_hypervisor
  #endif

+/*
+ * Each platform which implements psci_cpu_entry_jump function should 
perform
+ * in the following way:
+ *
+ * If the executing this call CPU is exactly that CPU we are expecting 
to be
+ * powered on, then jump to psci_cpu_entry and never return.
+ * Otherwise return to the caller.
+ */
+#if defined(CONFIG_ARMV7_PSCI) && !defined(CONFIG_SPL_BUILD)
+ENTRY(psci_cpu_entry_jump)
+       movs    pc, lr
+ENDPROC(psci_cpu_entry_jump)
+.weak psci_cpu_entry_jump
+#endif
+
  /*************************************************************************
   *
   * cpu_init_cp15


What do you think?


It would be much appreciated, if you could provide some hints.


>
>>>>>> +extern void shmobile_boot_vector(void);
>>>>>> +extern unsigned long shmobile_boot_fn;
>>>>>> +extern unsigned long shmobile_boot_size;
>>>>> Are the externs necessary ?
>>>> Yes, otherwise, compiler will complain.
>>>>
>>>> I can hide them in a header file. Shall I do that with moving ASM code
>>>> to a separate file?
>>> Only if you cannot reuse the U-Boot reset vector ones , which I think
>>> you can ?
>> I tried to explain above, why I can't reuse. So, if you and other
>> reviewers OK with it,
>>
>> I will move it to a separate file in V2.
> I don't quite see why it's a problem to use the U-Boot reset vectors.
> Sure, it might be needed to adjust that code first if it cannot be used
> as-is.
>
>>>>>> +void r8a7790_prepare_cpus(unsigned long addr, u32 nr_cpus)
>>>>>> +{
>>>>>> +    int cpu = get_current_cpu();
>>>>>> +    int i, ret = 0;
>>>>>> +    u32 bar;
>>>>>> +
>>>>>> +    shmobile_boot_fn = addr;
>>>>>> +    dsb();
>>>>>> +
>>>>>> +    /* Install reset vector */
>>>>>> +    memcpy_toio((void __iomem *)ICRAM1, shmobile_boot_vector,
>>>>>> +            shmobile_boot_size);
>>>>>> +    dmb();
>>>>> Does this take into consideration caches ?
>>>> Good question... Probably, I should have added flush_dcache_range()
>>>> after copy.
>>>>
>>>>>> +    /* Setup reset vectors */
>>>>>> +    bar = (ICRAM1 >> 8) & 0xfffffc00;
>>>>>> +    writel(bar, RST_BASE + CA15BAR);
>>>>>> +    writel(bar | SBAR_BAREN, RST_BASE + CA15BAR);
>>>>>> +    writel(bar, RST_BASE + CA7BAR);
>>>>>> +    writel(bar | SBAR_BAREN, RST_BASE + CA7BAR);
>>>>>> +    dsb();
>>>>>> +
>>>>>> +    /* Perform setup for debug mode for all CPUs */
>>>>>> +    for (i = 0; i < nr_cpus; i++)
>>>>>> +        apmu_setup_debug_mode(i);
>>>>>> +
>>>>>> +    /*
>>>>>> +     * Indicate the completion status of power shutoff/resume
>>>>>> procedure
>>>>>> +     * for CA15/CA7 SCU.
>>>>>> +     */
>>>>>> +    writel(BIT(CA15_SCU_ISR_BIT) | BIT(CA7_SCU_ISR_BIT),
>>>>>> +           SYSC_BASE + SYSCIER);
>>>>>> +
>>>>>> +    /* Power on CA15/CA7 SCU */
>>>>>> +    if (sysc_power_is_off(CA15_SCU))
>>>>>> +        ret += sysc_power_up(CA15_SCU);
>>>>>> +    if (sysc_power_is_off(CA7_SCU))
>>>>>> +        ret += sysc_power_up(CA7_SCU);
>>>>>> +    if (ret)
>>>>>> +        printf("warning: some of secondary CPUs may not boot\n");
>>>>> "some" is not very useful for debugging,.
>>>> OK, will provide more concrete output.
>>>>
>>>>>> +    /* Keep secondary CPUs in reset */
>>>>>> +    for (i = 0; i < nr_cpus; i++) {
>>>>>> +        /* Make sure that we don't reset boot CPU */
>>>>>> +        if (i == cpu)
>>>>>> +            continue;
>>>>>> +
>>>>>> +        r8a7790_assert_reset(i);
>>>>>> +    }
>>>>>> +
>>>>>> +    /*
>>>>>> +     * Enable snoop requests and DVM message requests for slave
>>>>>> interfaces
>>>>>> +     * S4 (CA7) and S3 (CA15).
>>>>>> +     */
>>>>>> +    writel(readl(CCI_BASE + CCI_SLAVE3 + CCI_SNOOP) | CCI_ENABLE_REQ,
>>>>>> +           CCI_BASE + CCI_SLAVE3 + CCI_SNOOP);
>>>>>> +    writel(readl(CCI_BASE + CCI_SLAVE4 + CCI_SNOOP) | CCI_ENABLE_REQ,
>>>>>> +           CCI_BASE + CCI_SLAVE4 + CCI_SNOOP);
>>>>>> +    /* Wait for pending bit low */
>>>>>> +    while (readl(CCI_BASE + CCI_STATUS))
>>>>>> +        ;
>>>>> can this spin forever ?
>>>> Paragraph "3.3.4 Status Register" in "ARM CoreLink™CCI-400 Cache
>>>> Coherent Interconnect" document says nothing
>>>>
>>>> about possibility to spin forever. Just next:
>>>>
>>>> "After writing to the snoop or DVM enable bits, the controller must wait
>>>> for the register write to
>>>> complete, then test that the change_pending bit is LOW before it turns
>>>> an attached device on or off."
>>> So what if this code spins forever, will this also completely hang Linux
>>> which calls this code ?
>>> [...]
>>
>> In this case, no. This is PSCI board initialization code, which is being
>> executed while we are still in U-Boot.
>>
>> Unlike r8a7790_apmu_power_on()/r8a7790_apmu_power_off() functions, which
>> are indented to be called from PSCI handlers (at runtime), when the
>> "main" U-Boot is gone away.
>>
>> If this code spins forever, U-Boot will get stuck before even starting
>> Linux/Hypervisor.
>>
>> Probably, it would be better to add safe timeout (~1s) here as well.
> I think so.
>

[U-Boot] [PATCH 2/3] ARM: rmobile: Add basic PSCI support for r8a7790 SoC

[Oleksandr]

Hi, Marek


>>>>>>> +}
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * Reset vector for secondary CPUs.
>>>>>>> + * This will be mapped at address 0 by SBAR register.
>>>>>>> + * We need _long_ jump to the physical address.
>>>>>>> + */
>>>>>>> +asm("    .arm\n"
>>>>>>> +    "    .align 12\n"
>>>>>>> +    "    .globl shmobile_boot_vector\n"
>>>>>>> +    "shmobile_boot_vector:\n"
>>>>>>> +    "    ldr r1, 1f\n"
>>>>>>> +    "    bx    r1\n"
>>>>>>> +    "    .type shmobile_boot_vector, %function\n"
>>>>>>> +    "    .size shmobile_boot_vector, .-shmobile_boot_vector\n"
>>>>>>> +    "    .align    2\n"
>>>>>>> +    "    .globl    shmobile_boot_fn\n"
>>>>>>> +    "shmobile_boot_fn:\n"
>>>>>>> +    "1:    .space    4\n"
>>>>>>> +    "    .globl    shmobile_boot_size\n"
>>>>>>> +    "shmobile_boot_size:\n"
>>>>>>> +    "    .long    .-shmobile_boot_vector\n");
>>>>>> Why can't this be implemented in C ?
>>>>> This "reset vector" code was ported from Linux:
>>>>>
>>>>> https://elixir.bootlin.com/linux/v5.0-rc5/source/arch/arm/mach-shmobile/headsmp.S#L21 
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Really don't know whether it can be implemented in C.
>>>>>
>>>>> I was thinking of moving this code to a separate ASM file in order 
>>>>> not
>>>>> to mix C and ASM. What do you think about it?
>>>> U-Boot already has a reset vector code, can't that be reused ?
>>> I don't think. Being honest, I couldn't find an obvious way how to 
>>> reuse
>>> (I assume you meant arch/arm/cpu/armv7/start.S).
>> Maybe it needs some additional work first ?
>> It seems Altera socfpga somehow uses the U-Boot reset vectors for PSCI,
>> so it should at least be possible.
>
> Could you, please, point me in code? Unfortunately, I wasn't able to 
> find.
>
>
>>
>>> The newly turned on secondary CPU entry should be common
>>> "psci_cpu_entry", which does proper things.
>>>
>>> And this reset vector is just "a small piece of code" to be located in
>>> on-chip RAM (with limited size) and used for the jump stub...
>> We already have the SPL reset vectors in SRAM, maybe that can be
>> recycled somehow ?
>
>
> The only idea I have, how it may be recycled (not sure whether it will 
> work...)
>
>
> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> index 0cb6dd39cc..69acf4677b 100644
> --- a/arch/arm/cpu/armv7/start.S
> +++ b/arch/arm/cpu/armv7/start.S
> @@ -36,6 +36,12 @@
>  #endif
>
>  reset:
> +
> +#if defined(CONFIG_ARMV7_PSCI) && !defined(CONFIG_SPL_BUILD)
> +       b psci_cpu_entry_jump
> +       /* return only if this is not "a newly turned on CPU" using 
> PSCI) */
> +#endif
> +
>         /* Allow the board to save important registers */
>         b       save_boot_params
>  save_boot_params_ret:
> @@ -128,6 +134,21 @@ ENDPROC(switch_to_hypervisor)
>         .weak   switch_to_hypervisor
>  #endif
>
> +/*
> + * Each platform which implements psci_cpu_entry_jump function should 
> perform
> + * in the following way:
> + *
> + * If the executing this call CPU is exactly that CPU we are 
> expecting to be
> + * powered on, then jump to psci_cpu_entry and never return.
> + * Otherwise return to the caller.
> + */
> +#if defined(CONFIG_ARMV7_PSCI) && !defined(CONFIG_SPL_BUILD)
> +ENTRY(psci_cpu_entry_jump)
> +       movs    pc, lr
> +ENDPROC(psci_cpu_entry_jump)
> +.weak psci_cpu_entry_jump
> +#endif
> +
>  /************************************************************************* 
>
>   *
>   * cpu_init_cp15
>
>
> What do you think?
>
>
> It would be much appreciated, if you could provide some hints.


Don't want to be annoying, but it would be really nice to get my 
questions answered...


-- 
Regards,

Oleksandr Tyshchenko

[U-Boot] [PATCH 2/3] ARM: rmobile: Add basic PSCI support for r8a7790 SoC

[Marek Vasut]

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

Hi,

>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/*
>>>>>>>> + * Reset vector for secondary CPUs.
>>>>>>>> + * This will be mapped at address 0 by SBAR register.
>>>>>>>> + * We need _long_ jump to the physical address.
>>>>>>>> + */
>>>>>>>> +asm("    .arm\n"
>>>>>>>> +    "    .align 12\n"
>>>>>>>> +    "    .globl shmobile_boot_vector\n"
>>>>>>>> +    "shmobile_boot_vector:\n"
>>>>>>>> +    "    ldr r1, 1f\n"
>>>>>>>> +    "    bx    r1\n"
>>>>>>>> +    "    .type shmobile_boot_vector, %function\n"
>>>>>>>> +    "    .size shmobile_boot_vector, .-shmobile_boot_vector\n"
>>>>>>>> +    "    .align    2\n"
>>>>>>>> +    "    .globl    shmobile_boot_fn\n"
>>>>>>>> +    "shmobile_boot_fn:\n"
>>>>>>>> +    "1:    .space    4\n"
>>>>>>>> +    "    .globl    shmobile_boot_size\n"
>>>>>>>> +    "shmobile_boot_size:\n"
>>>>>>>> +    "    .long    .-shmobile_boot_vector\n");
>>>>>>> Why can't this be implemented in C ?
>>>>>> This "reset vector" code was ported from Linux:
>>>>>>
>>>>>> https://elixir.bootlin.com/linux/v5.0-rc5/source/arch/arm/mach-shmobile/headsmp.S#L21
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Really don't know whether it can be implemented in C.
>>>>>>
>>>>>> I was thinking of moving this code to a separate ASM file in order
>>>>>> not
>>>>>> to mix C and ASM. What do you think about it?
>>>>> U-Boot already has a reset vector code, can't that be reused ?
>>>> I don't think. Being honest, I couldn't find an obvious way how to
>>>> reuse
>>>> (I assume you meant arch/arm/cpu/armv7/start.S).
>>> Maybe it needs some additional work first ?
>>> It seems Altera socfpga somehow uses the U-Boot reset vectors for PSCI,
>>> so it should at least be possible.
>>
>> Could you, please, point me in code? Unfortunately, I wasn't able to
>> find.
>>
>>
>>>
>>>> The newly turned on secondary CPU entry should be common
>>>> "psci_cpu_entry", which does proper things.
>>>>
>>>> And this reset vector is just "a small piece of code" to be located in
>>>> on-chip RAM (with limited size) and used for the jump stub...
>>> We already have the SPL reset vectors in SRAM, maybe that can be
>>> recycled somehow ?
>>
>>
>> The only idea I have, how it may be recycled (not sure whether it will
>> work...)
>>
>>
>> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
>> index 0cb6dd39cc..69acf4677b 100644
>> --- a/arch/arm/cpu/armv7/start.S
>> +++ b/arch/arm/cpu/armv7/start.S
>> @@ -36,6 +36,12 @@
>>  #endif
>>
>>  reset:
>> +
>> +#if defined(CONFIG_ARMV7_PSCI) && !defined(CONFIG_SPL_BUILD)
>> +       b psci_cpu_entry_jump
>> +       /* return only if this is not "a newly turned on CPU" using
>> PSCI) */
>> +#endif
>> +
>>         /* Allow the board to save important registers */
>>         b       save_boot_params
>>  save_boot_params_ret:
>> @@ -128,6 +134,21 @@ ENDPROC(switch_to_hypervisor)
>>         .weak   switch_to_hypervisor
>>  #endif
>>
>> +/*
>> + * Each platform which implements psci_cpu_entry_jump function should
>> perform
>> + * in the following way:
>> + *
>> + * If the executing this call CPU is exactly that CPU we are
>> expecting to be
>> + * powered on, then jump to psci_cpu_entry and never return.
>> + * Otherwise return to the caller.
>> + */
>> +#if defined(CONFIG_ARMV7_PSCI) && !defined(CONFIG_SPL_BUILD)
>> +ENTRY(psci_cpu_entry_jump)
>> +       movs    pc, lr
>> +ENDPROC(psci_cpu_entry_jump)
>> +.weak psci_cpu_entry_jump
>> +#endif
>> +
>>  /*************************************************************************
>>
>>   *
>>   * cpu_init_cp15
>>
>>
>> What do you think?
>>
>>
>> It would be much appreciated, if you could provide some hints.
> 
> 
> Don't want to be annoying, but it would be really nice to get my
> questions answered...

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.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 2/3] ARM: rmobile: Add basic PSCI support for r8a7790 SoC

[Oleksandr Tyshchenko]

Hi, Marek

sorry for possible format issue.


ср, 27 февр. 2019 г., 23:02 Marek Vasut <marek.vasut@gmail.com>:

> On 2/26/19 8:37 PM, Oleksandr wrote:
> >
> > Hi, Marek
>
> Hi,
>
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +/*
> >>>>>>>> + * Reset vector for secondary CPUs.
> >>>>>>>> + * This will be mapped at address 0 by SBAR register.
> >>>>>>>> + * We need _long_ jump to the physical address.
> >>>>>>>> + */
> >>>>>>>> +asm("    .arm\n"
> >>>>>>>> +    "    .align 12\n"
> >>>>>>>> +    "    .globl shmobile_boot_vector\n"
> >>>>>>>> +    "shmobile_boot_vector:\n"
> >>>>>>>> +    "    ldr r1, 1f\n"
> >>>>>>>> +    "    bx    r1\n"
> >>>>>>>> +    "    .type shmobile_boot_vector, %function\n"
> >>>>>>>> +    "    .size shmobile_boot_vector, .-shmobile_boot_vector\n"
> >>>>>>>> +    "    .align    2\n"
> >>>>>>>> +    "    .globl    shmobile_boot_fn\n"
> >>>>>>>> +    "shmobile_boot_fn:\n"
> >>>>>>>> +    "1:    .space    4\n"
> >>>>>>>> +    "    .globl    shmobile_boot_size\n"
> >>>>>>>> +    "shmobile_boot_size:\n"
> >>>>>>>> +    "    .long    .-shmobile_boot_vector\n");
> >>>>>>> Why can't this be implemented in C ?
> >>>>>> This "reset vector" code was ported from Linux:
> >>>>>>
> >>>>>>
> https://elixir.bootlin.com/linux/v5.0-rc5/source/arch/arm/mach-shmobile/headsmp.S#L21
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> Really don't know whether it can be implemented in C.
> >>>>>>
> >>>>>> I was thinking of moving this code to a separate ASM file in order
> >>>>>> not
> >>>>>> to mix C and ASM. What do you think about it?
> >>>>> U-Boot already has a reset vector code, can't that be reused ?
> >>>> I don't think. Being honest, I couldn't find an obvious way how to
> >>>> reuse
> >>>> (I assume you meant arch/arm/cpu/armv7/start.S).
> >>> Maybe it needs some additional work first ?
> >>> It seems Altera socfpga somehow uses the U-Boot reset vectors for PSCI,
> >>> so it should at least be possible.
> >>
> >> Could you, please, point me in code? Unfortunately, I wasn't able to
> >> find.
> >>
> >>
> >>>
> >>>> The newly turned on secondary CPU entry should be common
> >>>> "psci_cpu_entry", which does proper things.
> >>>>
> >>>> And this reset vector is just "a small piece of code" to be located in
> >>>> on-chip RAM (with limited size) and used for the jump stub...
> >>> We already have the SPL reset vectors in SRAM, maybe that can be
> >>> recycled somehow ?
> >>
> >>
> >> The only idea I have, how it may be recycled (not sure whether it will
> >> work...)
> >>
> >>
> >> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> >> index 0cb6dd39cc..69acf4677b 100644
> >> --- a/arch/arm/cpu/armv7/start.S
> >> +++ b/arch/arm/cpu/armv7/start.S
> >> @@ -36,6 +36,12 @@
> >>  #endif
> >>
> >>  reset:
> >> +
> >> +#if defined(CONFIG_ARMV7_PSCI) && !defined(CONFIG_SPL_BUILD)
> >> +       b psci_cpu_entry_jump
> >> +       /* return only if this is not "a newly turned on CPU" using
> >> PSCI) */
> >> +#endif
> >> +
> >>         /* Allow the board to save important registers */
> >>         b       save_boot_params
> >>  save_boot_params_ret:
> >> @@ -128,6 +134,21 @@ ENDPROC(switch_to_hypervisor)
> >>         .weak   switch_to_hypervisor
> >>  #endif
> >>
> >> +/*
> >> + * Each platform which implements psci_cpu_entry_jump function should
> >> perform
> >> + * in the following way:
> >> + *
> >> + * If the executing this call CPU is exactly that CPU we are
> >> expecting to be
> >> + * powered on, then jump to psci_cpu_entry and never return.
> >> + * Otherwise return to the caller.
> >> + */
> >> +#if defined(CONFIG_ARMV7_PSCI) && !defined(CONFIG_SPL_BUILD)
> >> +ENTRY(psci_cpu_entry_jump)
> >> +       movs    pc, lr
> >> +ENDPROC(psci_cpu_entry_jump)
> >> +.weak psci_cpu_entry_jump
> >> +#endif
> >> +
> >>
>  /*************************************************************************
> >>
> >>   *
> >>   * cpu_init_cp15
> >>
> >>
> >> What do you think?
> >>
> >>
> >> It would be much appreciated, if you could provide some hints.
> >
> >
> > Don't want to be annoying, but it would be really nice to get my
> > questions answered...
>
> 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.
>

No problem, I will wait.

Thank you.


> --
> Best regards,
> Marek Vasut
>

[U-Boot] [PATCH 3/3] ARM: rmobile: Add possibility to debug main PSCI commands

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Also take care of the fact that Lager and Stout boards use
different serial interface for console.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 arch/arm/mach-rmobile/debug.h | 91 +++++++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-rmobile/psci.c  | 23 +++++++++++
 2 files changed, 114 insertions(+)
 create mode 100644 arch/arm/mach-rmobile/debug.h

diff --git a/arch/arm/mach-rmobile/debug.h b/arch/arm/mach-rmobile/debug.h
new file mode 100644
index 0000000..5d4ec77
--- /dev/null
+++ b/arch/arm/mach-rmobile/debug.h
@@ -0,0 +1,91 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Contains functions used for PSCI debug.
+ *
+ * Copyright (C) 2018 EPAM Systems Inc.
+ *
+ * Based on arch/arm/mach-uniphier/debug.h
+ */
+
+#ifndef __DEBUG_H__
+#define __DEBUG_H__
+
+#include <asm/io.h>
+
+/* SCIFA definitions */
+#define SCIFA_BASE		0xe6c40000
+
+#define SCIFA_SCASSR	0x14	/* Serial status register */
+#define SCIFA_SCAFTDR	0x20	/* Transmit FIFO data register */
+
+/* SCIF definitions */
+#define SCIF_BASE		0xe6e60000
+
+#define SCIF_SCFSR		0x10	/* Serial status register */
+#define SCIF_SCFTDR		0x0c	/* Transmit FIFO data register */
+
+/* Common for both interfaces definitions */
+#define SCFSR_TDFE		BIT(5) /* Transmit FIFO Data Empty */
+#define SCFSR_TEND		BIT(6) /* Transmission End */
+
+#ifdef CONFIG_SCIF_A
+#define UART_BASE			SCIFA_BASE
+#define UART_STATUS_REG		SCIFA_SCASSR
+#define UART_TX_FIFO_REG	SCIFA_SCAFTDR
+#else
+#define UART_BASE			SCIF_BASE
+#define UART_STATUS_REG		SCIF_SCFSR
+#define UART_TX_FIFO_REG	SCIF_SCFTDR
+#endif
+
+/* All functions are inline so that they can be called from .secure section. */
+
+#ifdef DEBUG
+static inline void debug_putc(int c)
+{
+	void __iomem *base = (void __iomem *)UART_BASE;
+
+	while (!(readw(base + UART_STATUS_REG) & SCFSR_TDFE))
+		;
+
+	writeb(c, base + UART_TX_FIFO_REG);
+	writew(readw(base + UART_STATUS_REG) & ~(SCFSR_TEND | SCFSR_TDFE),
+	       base + UART_STATUS_REG);
+}
+
+static inline void debug_puts(const char *s)
+{
+	while (*s) {
+		if (*s == '\n')
+			debug_putc('\r');
+
+		debug_putc(*s++);
+	}
+}
+
+static inline void debug_puth(unsigned long val)
+{
+	int i;
+	unsigned char c;
+
+	for (i = 8; i--; ) {
+		c = ((val >> (i * 4)) & 0xf);
+		c += (c >= 10) ? 'a' - 10 : '0';
+		debug_putc(c);
+	}
+}
+#else
+static inline void debug_putc(int c)
+{
+}
+
+static inline void debug_puts(const char *s)
+{
+}
+
+static inline void debug_puth(unsigned long val)
+{
+}
+#endif
+
+#endif /* __DEBUG_H__ */
diff --git a/arch/arm/mach-rmobile/psci.c b/arch/arm/mach-rmobile/psci.c
index 95850b8..4d78b0f 100644
--- a/arch/arm/mach-rmobile/psci.c
+++ b/arch/arm/mach-rmobile/psci.c
@@ -14,6 +14,7 @@
 #include <asm/secure.h>
 
 #include "pm-r8a7790.h"
+#include "debug.h"
 
 #define R8A7790_PSCI_NR_CPUS	8
 #if R8A7790_PSCI_NR_CPUS > CONFIG_ARMV7_PSCI_NR_CPUS
@@ -105,6 +106,16 @@ int __secure psci_cpu_on(u32 function_id, u32 target_cpu, u32 entry_point,
 {
 	int cpu;
 
+	debug_puts("[U-Boot PSCI] cpu_on: cpu=");
+	debug_puth(get_current_cpu());
+	debug_puts(", target_cpu=");
+	debug_puth(target_cpu);
+	debug_puts(", entry_point=");
+	debug_puth(entry_point);
+	debug_puts(", context_id=");
+	debug_puth(context_id);
+	debug_puts("\n");
+
 	cpu = mpidr_to_cpu_index(target_cpu);
 	if (cpu == -1)
 		return ARM_PSCI_RET_INVAL;
@@ -130,6 +141,10 @@ int __secure psci_cpu_off(void)
 {
 	int cpu = get_current_cpu();
 
+	debug_puts("[U-Boot PSCI] cpu_off: cpu=");
+	debug_puth(cpu);
+	debug_puts("\n");
+
 	/*
 	 * Place the CPU interface in a state where it can never make a CPU
 	 * exit WFI as result of an asserted interrupt.
@@ -154,6 +169,10 @@ int __secure psci_cpu_off(void)
 
 void __secure psci_system_reset(u32 function_id)
 {
+	debug_puts("[U-Boot PSCI] system_reset: cpu=");
+	debug_puth(get_current_cpu());
+	debug_puts("\n");
+
 	r8a7790_system_reset();
 
 	/* Drain the WB before WFI */
@@ -166,6 +185,10 @@ void __secure psci_system_reset(u32 function_id)
 
 void __secure psci_system_off(u32 function_id)
 {
+	debug_puts("[U-Boot PSCI] system_off: cpu=");
+	debug_puth(get_current_cpu());
+	debug_puts("\n");
+
 	/* Drain the WB before WFI */
 	dsb();
 
-- 
2.7.4

[U-Boot] [PATCH 3/3] ARM: rmobile: Add possibility to debug main PSCI commands

[Marek Vasut]

On 1/31/19 6:38 PM, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Also take care of the fact that Lager and Stout boards use
> different serial interface for console.

This describes something else than $subject , please split the patchset
such that one patch does one thing and describes that one thing in the
commit message.

> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  arch/arm/mach-rmobile/debug.h | 91 +++++++++++++++++++++++++++++++++++++++++++
>  arch/arm/mach-rmobile/psci.c  | 23 +++++++++++
>  2 files changed, 114 insertions(+)
>  create mode 100644 arch/arm/mach-rmobile/debug.h
> 
> diff --git a/arch/arm/mach-rmobile/debug.h b/arch/arm/mach-rmobile/debug.h
> new file mode 100644
> index 0000000..5d4ec77
> --- /dev/null
> +++ b/arch/arm/mach-rmobile/debug.h
> @@ -0,0 +1,91 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Contains functions used for PSCI debug.
> + *
> + * Copyright (C) 2018 EPAM Systems Inc.
> + *
> + * Based on arch/arm/mach-uniphier/debug.h
> + */
> +
> +#ifndef __DEBUG_H__
> +#define __DEBUG_H__
> +
> +#include <asm/io.h>
> +
> +/* SCIFA definitions */
> +#define SCIFA_BASE		0xe6c40000

Should come from DT

> +#define SCIFA_SCASSR	0x14	/* Serial status register */
> +#define SCIFA_SCAFTDR	0x20	/* Transmit FIFO data register */
> +
> +/* SCIF definitions */
> +#define SCIF_BASE		0xe6e60000
> +
> +#define SCIF_SCFSR		0x10	/* Serial status register */
> +#define SCIF_SCFTDR		0x0c	/* Transmit FIFO data register */
> +
> +/* Common for both interfaces definitions */
> +#define SCFSR_TDFE		BIT(5) /* Transmit FIFO Data Empty */
> +#define SCFSR_TEND		BIT(6) /* Transmission End */
> +
> +#ifdef CONFIG_SCIF_A
> +#define UART_BASE			SCIFA_BASE
> +#define UART_STATUS_REG		SCIFA_SCASSR
> +#define UART_TX_FIFO_REG	SCIFA_SCAFTDR
> +#else
> +#define UART_BASE			SCIF_BASE
> +#define UART_STATUS_REG		SCIF_SCFSR
> +#define UART_TX_FIFO_REG	SCIF_SCFTDR
> +#endif
> +
> +/* All functions are inline so that they can be called from .secure section. */
> +
> +#ifdef DEBUG
> +static inline void debug_putc(int c)
> +{
> +	void __iomem *base = (void __iomem *)UART_BASE;
> +
> +	while (!(readw(base + UART_STATUS_REG) & SCFSR_TDFE))
> +		;
> +
> +	writeb(c, base + UART_TX_FIFO_REG);
> +	writew(readw(base + UART_STATUS_REG) & ~(SCFSR_TEND | SCFSR_TDFE),
> +	       base + UART_STATUS_REG);

Is this some implementation of debug uart API or is this a
reimplementation of the serial driver ?

> +}
> +
> +static inline void debug_puts(const char *s)
> +{
> +	while (*s) {
> +		if (*s == '\n')
> +			debug_putc('\r');
> +
> +		debug_putc(*s++);
> +	}
> +}
> +
> +static inline void debug_puth(unsigned long val)
> +{
> +	int i;
> +	unsigned char c;
> +
> +	for (i = 8; i--; ) {
> +		c = ((val >> (i * 4)) & 0xf);
> +		c += (c >= 10) ? 'a' - 10 : '0';
> +		debug_putc(c);
> +	}
> +}
> +#else
> +static inline void debug_putc(int c)
> +{
> +}
> +
> +static inline void debug_puts(const char *s)
> +{
> +}
> +
> +static inline void debug_puth(unsigned long val)
> +{
> +}
> +#endif
> +
> +#endif /* __DEBUG_H__ */
> diff --git a/arch/arm/mach-rmobile/psci.c b/arch/arm/mach-rmobile/psci.c
> index 95850b8..4d78b0f 100644
> --- a/arch/arm/mach-rmobile/psci.c
> +++ b/arch/arm/mach-rmobile/psci.c
> @@ -14,6 +14,7 @@
>  #include <asm/secure.h>
>  
>  #include "pm-r8a7790.h"
> +#include "debug.h"
>  
>  #define R8A7790_PSCI_NR_CPUS	8
>  #if R8A7790_PSCI_NR_CPUS > CONFIG_ARMV7_PSCI_NR_CPUS
> @@ -105,6 +106,16 @@ int __secure psci_cpu_on(u32 function_id, u32 target_cpu, u32 entry_point,
>  {
>  	int cpu;
>  
> +	debug_puts("[U-Boot PSCI] cpu_on: cpu=");
> +	debug_puth(get_current_cpu());
> +	debug_puts(", target_cpu=");
> +	debug_puth(target_cpu);
> +	debug_puts(", entry_point=");
> +	debug_puth(entry_point);
> +	debug_puts(", context_id=");
> +	debug_puth(context_id);
> +	debug_puts("\n");
> +
>  	cpu = mpidr_to_cpu_index(target_cpu);
>  	if (cpu == -1)
>  		return ARM_PSCI_RET_INVAL;
> @@ -130,6 +141,10 @@ int __secure psci_cpu_off(void)
>  {
>  	int cpu = get_current_cpu();
>  
> +	debug_puts("[U-Boot PSCI] cpu_off: cpu=");
> +	debug_puth(cpu);
> +	debug_puts("\n");
> +
>  	/*
>  	 * Place the CPU interface in a state where it can never make a CPU
>  	 * exit WFI as result of an asserted interrupt.
> @@ -154,6 +169,10 @@ int __secure psci_cpu_off(void)
>  
>  void __secure psci_system_reset(u32 function_id)
>  {
> +	debug_puts("[U-Boot PSCI] system_reset: cpu=");
> +	debug_puth(get_current_cpu());
> +	debug_puts("\n");
> +
>  	r8a7790_system_reset();
>  
>  	/* Drain the WB before WFI */
> @@ -166,6 +185,10 @@ void __secure psci_system_reset(u32 function_id)
>  
>  void __secure psci_system_off(u32 function_id)
>  {
> +	debug_puts("[U-Boot PSCI] system_off: cpu=");
> +	debug_puth(get_current_cpu());
> +	debug_puts("\n");
> +
>  	/* Drain the WB before WFI */
>  	dsb();
>  
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 3/3] ARM: rmobile: Add possibility to debug main PSCI commands

[Oleksandr]

On 05.02.19 20:56, Marek Vasut wrote:

Hi Marek

> On 1/31/19 6:38 PM, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Also take care of the fact that Lager and Stout boards use
>> different serial interface for console.
> This describes something else than $subject , please split the patchset
> such that one patch does one thing and describes that one thing in the
> commit message.

Yes, make sense. Will split.

>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>   arch/arm/mach-rmobile/debug.h | 91 +++++++++++++++++++++++++++++++++++++++++++
>>   arch/arm/mach-rmobile/psci.c  | 23 +++++++++++
>>   2 files changed, 114 insertions(+)
>>   create mode 100644 arch/arm/mach-rmobile/debug.h
>>
>> diff --git a/arch/arm/mach-rmobile/debug.h b/arch/arm/mach-rmobile/debug.h
>> new file mode 100644
>> index 0000000..5d4ec77
>> --- /dev/null
>> +++ b/arch/arm/mach-rmobile/debug.h
>> @@ -0,0 +1,91 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Contains functions used for PSCI debug.
>> + *
>> + * Copyright (C) 2018 EPAM Systems Inc.
>> + *
>> + * Based on arch/arm/mach-uniphier/debug.h
>> + */
>> +
>> +#ifndef __DEBUG_H__
>> +#define __DEBUG_H__
>> +
>> +#include <asm/io.h>
>> +
>> +/* SCIFA definitions */
>> +#define SCIFA_BASE		0xe6c40000
> Should come from DT

I have just found that rcar-base.h already contains #define-s for all 
SCIF(A)s.
>
>> +#define SCIFA_SCASSR	0x14	/* Serial status register */
>> +#define SCIFA_SCAFTDR	0x20	/* Transmit FIFO data register */
>> +
>> +/* SCIF definitions */
>> +#define SCIF_BASE		0xe6e60000
>> +
>> +#define SCIF_SCFSR		0x10	/* Serial status register */
>> +#define SCIF_SCFTDR		0x0c	/* Transmit FIFO data register */
>> +
>> +/* Common for both interfaces definitions */
>> +#define SCFSR_TDFE		BIT(5) /* Transmit FIFO Data Empty */
>> +#define SCFSR_TEND		BIT(6) /* Transmission End */
>> +
>> +#ifdef CONFIG_SCIF_A
>> +#define UART_BASE			SCIFA_BASE
>> +#define UART_STATUS_REG		SCIFA_SCASSR
>> +#define UART_TX_FIFO_REG	SCIFA_SCAFTDR
>> +#else
>> +#define UART_BASE			SCIF_BASE
>> +#define UART_STATUS_REG		SCIF_SCFSR
>> +#define UART_TX_FIFO_REG	SCIF_SCFTDR
>> +#endif
>> +
>> +/* All functions are inline so that they can be called from .secure section. */
>> +
>> +#ifdef DEBUG
>> +static inline void debug_putc(int c)
>> +{
>> +	void __iomem *base = (void __iomem *)UART_BASE;
>> +
>> +	while (!(readw(base + UART_STATUS_REG) & SCFSR_TDFE))
>> +		;
>> +
>> +	writeb(c, base + UART_TX_FIFO_REG);
>> +	writew(readw(base + UART_STATUS_REG) & ~(SCFSR_TEND | SCFSR_TDFE),
>> +	       base + UART_STATUS_REG);
> Is this some implementation of debug uart API or is this a
> reimplementation of the serial driver ?

I would say it is some implementation of debug uart API.

Let me explain why it is needed. We need something very simple to be 
called from the PSCI backend

in order to have possibility for debugging it. Actually the only thing 
we need is a simple function to write a char into uart TX register.

Actually I borrowed that idea from the implementation for mach-uniphier 
and modified according to the SCIF(A) usage.

These are examples, how it looks like:

1.

[    3.193974] psci_checker: Trying to turn off and on again group 1 
(CPUs 4-7)
[U-Boot PSCI] cpu_off: cpu=00000004
[    3.233648] Retrying again to check for CPU kill
[    3.238269] CPU4 killed.
[U-Boot PSCI] cpu_off: cpu=00000005
[    3.263678] Retrying again to check for CPU kill
[    3.268298] CPU5 killed.
[U-Boot PSCI] cpu_off: cpu=00000006
[    3.293648] Retrying again to check for CPU kill
[    3.298268] CPU6 killed.
[U-Boot PSCI] cpu_off: cpu=00000007
[    3.323691] Retrying again to check for CPU kill
[    3.328310] CPU7 killed.
[U-Boot PSCI] cpu_on: cpu=00000001, target_cpu=00000100, 
entry_point=48102440, context_id=00000000
[U-Boot PSCI] cpu_on: cpu=00000002, target_cpu=00000101, 
entry_point=48102440, context_id=00000000
[U-Boot PSCI] cpu_on: cpu=00000001, target_cpu=00000102, 
entry_point=48102440, context_id=00000000
[U-Boot PSCI] cpu_on: cpu=00000001, target_cpu=00000103, 
entry_point=48102440, context_id=00000000


2.
(XEN) Bringing up CPU4
[U-Boot PSCI] cpu_on: cpu=00000000, target_cpu=00000100, 
entry_point=4800004c, context_id=0030e208
- CPU 00000100 booting -
- Xen starting in Hyp mode -
- Setting up control registers -
- Turning on paging -
- Ready -
(XEN) Adding cpu 4 to runqueue 0
(XEN) CPU 4 booted.
(XEN) Bringing up CPU5
[U-Boot PSCI] cpu_on: cpu=00000000, target_cpu=00000101, 
entry_point=4800004c, context_id=0030e208
- CPU 00000101 booting -
- Xen starting in Hyp mode -
- Setting up control registers -
- Turning on paging -
- Ready -
(XEN) Adding cpu 5 to runqueue 0
(XEN) CPU 5 booted.
(XEN) Bringing up CPU6
[U-Boot PSCI] cpu_on: cpu=00000000, target_cpu=00000102, 
entry_point=4800004c, context_id=0030e208
- CPU 00000102 booting -
- Xen starting in Hyp mode -
- Setting up control registers -
- Turning on paging -
- Ready -
(XEN) Adding cpu 6 to runqueue 0
(XEN) CPU 6 booted.
(XEN) Bringing up CPU7
[U-Boot PSCI] cpu_on: cpu=00000000, target_cpu=00000103, 
entry_point=4800004c, context_id=0030e208
- CPU 00000103 booting -
- Xen starting in Hyp mode -
- Setting up control registers -
- Turning on paging -
- Ready -
(XEN) Adding cpu 7 to runqueue 0
(XEN) Brought up 8 CPUs
(XEN) CPU 7 booted.

>> +}
>> +
>> +static inline void debug_puts(const char *s)
>> +{
>> +	while (*s) {
>> +		if (*s == '\n')
>> +			debug_putc('\r');
>> +
>> +		debug_putc(*s++);
>> +	}
>> +}
>> +
>> +static inline void debug_puth(unsigned long val)
>> +{
>> +	int i;
>> +	unsigned char c;
>> +
>> +	for (i = 8; i--; ) {
>> +		c = ((val >> (i * 4)) & 0xf);
>> +		c += (c >= 10) ? 'a' - 10 : '0';
>> +		debug_putc(c);
>> +	}
>> +}
>> +#else
>> +static inline void debug_putc(int c)
>> +{
>> +}
>> +
>> +static inline void debug_puts(const char *s)
>> +{
>> +}
>> +
>> +static inline void debug_puth(unsigned long val)
>> +{
>> +}
>> +#endif
>> +
>> +#endif /* __DEBUG_H__ */
>> diff --git a/arch/arm/mach-rmobile/psci.c b/arch/arm/mach-rmobile/psci.c
>> index 95850b8..4d78b0f 100644
>> --- a/arch/arm/mach-rmobile/psci.c
>> +++ b/arch/arm/mach-rmobile/psci.c
>> @@ -14,6 +14,7 @@
>>   #include <asm/secure.h>
>>   
>>   #include "pm-r8a7790.h"
>> +#include "debug.h"
>>   
>>   #define R8A7790_PSCI_NR_CPUS	8
>>   #if R8A7790_PSCI_NR_CPUS > CONFIG_ARMV7_PSCI_NR_CPUS
>> @@ -105,6 +106,16 @@ int __secure psci_cpu_on(u32 function_id, u32 target_cpu, u32 entry_point,
>>   {
>>   	int cpu;
>>   
>> +	debug_puts("[U-Boot PSCI] cpu_on: cpu=");
>> +	debug_puth(get_current_cpu());
>> +	debug_puts(", target_cpu=");
>> +	debug_puth(target_cpu);
>> +	debug_puts(", entry_point=");
>> +	debug_puth(entry_point);
>> +	debug_puts(", context_id=");
>> +	debug_puth(context_id);
>> +	debug_puts("\n");
>> +
>>   	cpu = mpidr_to_cpu_index(target_cpu);
>>   	if (cpu == -1)
>>   		return ARM_PSCI_RET_INVAL;
>> @@ -130,6 +141,10 @@ int __secure psci_cpu_off(void)
>>   {
>>   	int cpu = get_current_cpu();
>>   
>> +	debug_puts("[U-Boot PSCI] cpu_off: cpu=");
>> +	debug_puth(cpu);
>> +	debug_puts("\n");
>> +
>>   	/*
>>   	 * Place the CPU interface in a state where it can never make a CPU
>>   	 * exit WFI as result of an asserted interrupt.
>> @@ -154,6 +169,10 @@ int __secure psci_cpu_off(void)
>>   
>>   void __secure psci_system_reset(u32 function_id)
>>   {
>> +	debug_puts("[U-Boot PSCI] system_reset: cpu=");
>> +	debug_puth(get_current_cpu());
>> +	debug_puts("\n");
>> +
>>   	r8a7790_system_reset();
>>   
>>   	/* Drain the WB before WFI */
>> @@ -166,6 +185,10 @@ void __secure psci_system_reset(u32 function_id)
>>   
>>   void __secure psci_system_off(u32 function_id)
>>   {
>> +	debug_puts("[U-Boot PSCI] system_off: cpu=");
>> +	debug_puth(get_current_cpu());
>> +	debug_puts("\n");
>> +
>>   	/* Drain the WB before WFI */
>>   	dsb();
>>   
>>
>
-- 
Regards,

Oleksandr Tyshchenko

[U-Boot] [PATCH 3/3] ARM: rmobile: Add possibility to debug main PSCI commands

[Marek Vasut]

On 2/8/19 1:47 PM, Oleksandr wrote:
> 
> On 05.02.19 20:56, Marek Vasut wrote:
> 
> Hi Marek

Hi,

>> On 1/31/19 6:38 PM, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> Also take care of the fact that Lager and Stout boards use
>>> different serial interface for console.
>> This describes something else than $subject , please split the patchset
>> such that one patch does one thing and describes that one thing in the
>> commit message.
> 
> Yes, make sense. Will split.
> 
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> ---
>>>   arch/arm/mach-rmobile/debug.h | 91
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>   arch/arm/mach-rmobile/psci.c  | 23 +++++++++++
>>>   2 files changed, 114 insertions(+)
>>>   create mode 100644 arch/arm/mach-rmobile/debug.h
>>>
>>> diff --git a/arch/arm/mach-rmobile/debug.h
>>> b/arch/arm/mach-rmobile/debug.h
>>> new file mode 100644
>>> index 0000000..5d4ec77
>>> --- /dev/null
>>> +++ b/arch/arm/mach-rmobile/debug.h
>>> @@ -0,0 +1,91 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Contains functions used for PSCI debug.
>>> + *
>>> + * Copyright (C) 2018 EPAM Systems Inc.
>>> + *
>>> + * Based on arch/arm/mach-uniphier/debug.h
>>> + */
>>> +
>>> +#ifndef __DEBUG_H__
>>> +#define __DEBUG_H__
>>> +
>>> +#include <asm/io.h>
>>> +
>>> +/* SCIFA definitions */
>>> +#define SCIFA_BASE        0xe6c40000
>> Should come from DT
> 
> I have just found that rcar-base.h already contains #define-s for all
> SCIF(A)s.

So does the DT, and the addresses should come from DT, not from ad-hoc
constants in U-Boot. Those will likely be removed at some point.

>>> +#define SCIFA_SCASSR    0x14    /* Serial status register */
>>> +#define SCIFA_SCAFTDR    0x20    /* Transmit FIFO data register */
>>> +
>>> +/* SCIF definitions */
>>> +#define SCIF_BASE        0xe6e60000
>>> +
>>> +#define SCIF_SCFSR        0x10    /* Serial status register */
>>> +#define SCIF_SCFTDR        0x0c    /* Transmit FIFO data register */
>>> +
>>> +/* Common for both interfaces definitions */
>>> +#define SCFSR_TDFE        BIT(5) /* Transmit FIFO Data Empty */
>>> +#define SCFSR_TEND        BIT(6) /* Transmission End */
>>> +
>>> +#ifdef CONFIG_SCIF_A
>>> +#define UART_BASE            SCIFA_BASE
>>> +#define UART_STATUS_REG        SCIFA_SCASSR
>>> +#define UART_TX_FIFO_REG    SCIFA_SCAFTDR
>>> +#else
>>> +#define UART_BASE            SCIF_BASE
>>> +#define UART_STATUS_REG        SCIF_SCFSR
>>> +#define UART_TX_FIFO_REG    SCIF_SCFTDR
>>> +#endif
>>> +
>>> +/* All functions are inline so that they can be called from .secure
>>> section. */
>>> +
>>> +#ifdef DEBUG
>>> +static inline void debug_putc(int c)
>>> +{
>>> +    void __iomem *base = (void __iomem *)UART_BASE;
>>> +
>>> +    while (!(readw(base + UART_STATUS_REG) & SCFSR_TDFE))
>>> +        ;
>>> +
>>> +    writeb(c, base + UART_TX_FIFO_REG);
>>> +    writew(readw(base + UART_STATUS_REG) & ~(SCFSR_TEND | SCFSR_TDFE),
>>> +           base + UART_STATUS_REG);
>> Is this some implementation of debug uart API or is this a
>> reimplementation of the serial driver ?
> 
> I would say it is some implementation of debug uart API.
> 
> Let me explain why it is needed. We need something very simple to be
> called from the PSCI backend
> 
> in order to have possibility for debugging it. Actually the only thing
> we need is a simple function to write a char into uart TX register.
> 
> Actually I borrowed that idea from the implementation for mach-uniphier
> and modified according to the SCIF(A) usage.
> 
> These are examples, how it looks like:
> 
> 1.
> 
> [    3.193974] psci_checker: Trying to turn off and on again group 1
> (CPUs 4-7)
> [U-Boot PSCI] cpu_off: cpu=00000004
> [    3.233648] Retrying again to check for CPU kill
> [    3.238269] CPU4 killed.
> [U-Boot PSCI] cpu_off: cpu=00000005
> [    3.263678] Retrying again to check for CPU kill
> [    3.268298] CPU5 killed.
> [U-Boot PSCI] cpu_off: cpu=00000006
> [    3.293648] Retrying again to check for CPU kill
> [    3.298268] CPU6 killed.
> [U-Boot PSCI] cpu_off: cpu=00000007
> [    3.323691] Retrying again to check for CPU kill
> [    3.328310] CPU7 killed.
> [U-Boot PSCI] cpu_on: cpu=00000001, target_cpu=00000100,
> entry_point=48102440, context_id=00000000
> [U-Boot PSCI] cpu_on: cpu=00000002, target_cpu=00000101,
> entry_point=48102440, context_id=00000000
> [U-Boot PSCI] cpu_on: cpu=00000001, target_cpu=00000102,
> entry_point=48102440, context_id=00000000
> [U-Boot PSCI] cpu_on: cpu=00000001, target_cpu=00000103,
> entry_point=48102440, context_id=00000000
> 
> 
> 2.
> (XEN) Bringing up CPU4
> [U-Boot PSCI] cpu_on: cpu=00000000, target_cpu=00000100,
> entry_point=4800004c, context_id=0030e208
> - CPU 00000100 booting -
> - Xen starting in Hyp mode -
> - Setting up control registers -
> - Turning on paging -
> - Ready -
> (XEN) Adding cpu 4 to runqueue 0
> (XEN) CPU 4 booted.
> (XEN) Bringing up CPU5
> [U-Boot PSCI] cpu_on: cpu=00000000, target_cpu=00000101,
> entry_point=4800004c, context_id=0030e208
> - CPU 00000101 booting -
> - Xen starting in Hyp mode -
> - Setting up control registers -
> - Turning on paging -
> - Ready -
> (XEN) Adding cpu 5 to runqueue 0
> (XEN) CPU 5 booted.
> (XEN) Bringing up CPU6
> [U-Boot PSCI] cpu_on: cpu=00000000, target_cpu=00000102,
> entry_point=4800004c, context_id=0030e208
> - CPU 00000102 booting -
> - Xen starting in Hyp mode -
> - Setting up control registers -
> - Turning on paging -
> - Ready -
> (XEN) Adding cpu 6 to runqueue 0
> (XEN) CPU 6 booted.
> (XEN) Bringing up CPU7
> [U-Boot PSCI] cpu_on: cpu=00000000, target_cpu=00000103,
> entry_point=4800004c, context_id=0030e208
> - CPU 00000103 booting -
> - Xen starting in Hyp mode -
> - Setting up control registers -
> - Turning on paging -
> - Ready -
> (XEN) Adding cpu 7 to runqueue 0
> (XEN) Brought up 8 CPUs
> (XEN) CPU 7 booted.

OK, this should then sit in drivers/serial/ .
The UART base address and properties should be selected via DT, not via
some ad-hoc constant hard-coded into U-Boot.

[...]

-- 
Best regards,
Marek Vasut