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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
