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

[Marek Vasut <marek.vasut@gmail.com>]

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