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/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. 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/p...
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/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 ?
+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 ? [...]