[U-Boot] [PATCH 2/3] ARM: rmobile: Add basic PSCI support for r8a7790 SoC
Oleksandr
olekstysh at gmail.com
Tue Feb 12 19:26:02 UTC 2019
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.
>
More information about the U-Boot
mailing list