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/plat...
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/p...
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/head...
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.