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 ? >>> 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/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. > 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. -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
