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