Hey Stephan, On Thursday, 4 June 2026 at 12:12 AM, Stephan Gerhold <[email protected]> wrote:
> On Tue, Jun 02, 2026 at 05:19:01PM +1000, Sam Day via B4 Relay wrote: > > From: Sam Day <[email protected]> > > > > Most MSM8916 devices lack PSCI support, instead they're brought online > > with a qcom SCM call to set the boot address, and some register poking > > of the APCS register block. > > > > Signed-off-by: Sam Day <[email protected]> > > --- > > arch/arm/mach-snapdragon/Makefile | 1 + > > arch/arm/mach-snapdragon/msm8916-smp.c | 138 > > +++++++++++++++++++++++++++++++++ > > 2 files changed, 139 insertions(+) > > > > diff --git a/arch/arm/mach-snapdragon/Makefile > > b/arch/arm/mach-snapdragon/Makefile > > index 343e825c6fd..584b8af055a 100644 > > --- a/arch/arm/mach-snapdragon/Makefile > > +++ b/arch/arm/mach-snapdragon/Makefile > > @@ -5,3 +5,4 @@ > > obj-y += board.o > > obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += capsule_update.o > > obj-$(CONFIG_OF_LIVE) += of_fixup.o > > +obj-$(CONFIG_ARMV8_SPIN_TABLE) += msm8916-smp.o > > diff --git a/arch/arm/mach-snapdragon/msm8916-smp.c > > b/arch/arm/mach-snapdragon/msm8916-smp.c > > new file mode 100644 > > index 00000000000..24f5590ccc3 > > --- /dev/null > > +++ b/arch/arm/mach-snapdragon/msm8916-smp.c > > @@ -0,0 +1,138 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * On MSM8916 devices that lack a PSCI implementation, firing up the > > secondary > > + * cores requires a call to TZ to set the boot address, and some poking of > > ACPS > > + * register block. > > + * > > + * Copyright (c) 2025 Linaro Ltd. > > + */ > > Was Linaro involved in this in 2025? Did you write the code yourself > (the init sequence, especially) or take it over from somewhere else? > Would be good to be precise here. Uh - I don't remember why Jan-2025 Sam thought to place Linaro copyrights here. I think the logic was that, since I don't care about owning the copyright, I figured I'd just assign it to the entity that owns a lot of the code in mach-snapdragon already. Anyway, since most of the code here came from the kernel, and I used lk2nd as a reference, I'll just preserve those copyrights in v3 instead. > > > + > > +#include <asm/io.h> > > +#include <asm/spin_table.h> > > +#include <asm/system.h> > > +#include <cpu_func.h> > > +#include <dm/ofnode.h> > > +#include <firmware/qcom/scm.h> > > +#include <linux/arm-smccc.h> > > +#include <linux/delay.h> > > + > > +#define APCS_CPU_PWR_CTL 0x04 > > +#define CORE_PWRD_UP BIT(7) > > +#define COREPOR_RST BIT(5) > > +#define CORE_RST BIT(4) > > +#define CORE_MEM_HS BIT(3) > > +#define CORE_MEM_CLAMP BIT(1) > > +#define CLAMP BIT(0) > > + > > +#define APC_PWR_GATE_CTL 0x14 > > +#define GDHS_CNT_SHIFT 24 > > +#define GDHS_EN BIT(0) > > + > > +static void qcom_boot_cortex_a53(phys_addr_t acc_base) > > +{ > > + u32 reg_val; > > + > > + /* Put the CPU into reset. */ > > + reg_val = CORE_RST | COREPOR_RST | CLAMP | CORE_MEM_CLAMP; > > + writel(reg_val, acc_base + APCS_CPU_PWR_CTL); > > + > > + /* Turn on the GDHS and set the GDHS_CNT to 16 XO clock cycles */ > > + writel(GDHS_EN | (0x10 << GDHS_CNT_SHIFT), acc_base + APC_PWR_GATE_CTL); > > + /* Wait for the GDHS to settle */ > > + udelay(2); > > + > > + reg_val &= ~CORE_MEM_CLAMP; > > + writel(reg_val, acc_base + APCS_CPU_PWR_CTL); > > + reg_val |= CORE_MEM_HS; > > + writel(reg_val, acc_base + APCS_CPU_PWR_CTL); > > + udelay(2); > > + > > + reg_val &= ~CLAMP; > > + writel(reg_val, acc_base + APCS_CPU_PWR_CTL); > > + udelay(2); > > + > > + /* Release CPU out of reset and bring it to life. */ > > + reg_val &= ~(CORE_RST | COREPOR_RST); > > + writel(reg_val, acc_base + APCS_CPU_PWR_CTL); > > + reg_val |= CORE_PWRD_UP; > > + writel(reg_val, acc_base + APCS_CPU_PWR_CTL); > > Nitpick (might be fine as-is, since Linux seems to have the same): > Strictly speaking, I think we need some memory barriers / DSBs in here > to make sure the write is complete before the udelay() starts (although > this is notoriously hard. If you're interested in this, take a look at > https://youtu.be/i6DayghhA8Q?t=1677, although I think it doesn't > translate exactly to U-Boot). Hmm yeah, it's odd because kernel is *not* issuing barriers in the cortex_a7 startup sequence, but *is* for kpss v1/v2. Since lk2nd already has prior art for this I'll just follow that lead in v3. > > > +} > > + > > +static int qcom_scm_set_boot_addr_mc(void *entry, unsigned int flags) > > +{ > > + struct qcom_scm_desc desc = { > > + .svc = QCOM_SCM_SVC_BOOT, > > + .cmd = QCOM_SCM_BOOT_SET_ADDR_MC, > > + .owner = ARM_SMCCC_OWNER_SIP, > > + .arginfo = QCOM_SCM_ARGS(6), > > + .args = { > > + (u64)entry, > > + /* Apply to all CPUs in all affinity levels */ > > + ~0ULL, ~0ULL, ~0ULL, ~0ULL, > > + flags, > > + }, > > + }; > > + > > + if (!qcom_scm_is_call_available(desc.svc, desc.cmd)) > > + return -EOPNOTSUPP; > > + > > + return qcom_scm_call(&desc, NULL); > > +} > > + > > +static bool boot_addr_set; > > + > > +int spin_table_boot_cpu(void *fdt, int cpu_offset) > > +{ > > + struct fdtdec_phandle_args acc; > > + u32 mpidr_aff, acc_base, reg; > > + int ret; > > + > > + if (fdt_node_check_compatible(fdt, 0, "qcom,msm8916")) > > + return 0; > > What if we have a qcom,apq8016? :) Wellll, in that case there's a functional TZ that can handle the PSCI call, right? ;) I suppose there's probably situations where that's not the case. Rabble rabble rabble. Anyway, I added this check in the eleventh hour right before submitting, because I was paranoid that it might impact some other platform (big-tent board code is great but also hellish). In retrospect this is unnecessary: spin_table_boot_cpu() is kicked by the generic code for each spin-table CPU it finds in the DT. We only force cores to spin-table enablement when we detect the busted-PSCI situation. I've removed this check entirely in v3. > > > + > > + reg = fdtdec_get_uint(fdt, cpu_offset, "reg", 0); > > + > > + if (fdt_node_check_compatible(fdt, cpu_offset, "arm,cortex-a53")) { > > + log_warning("CPU%d is not arm,cortex-a53 compatible\n", reg); > > + return -EINVAL; > > + } > > + > > + if (!boot_addr_set) { > > + debug("Setting CPU boot address to 0x%llx\n", > > + (phys_addr_t)&spin_table_reserve_begin); > > + ret = qcom_scm_set_boot_addr_mc(&spin_table_reserve_begin, > > + QCOM_SCM_BOOT_MC_FLAG_AARCH64 | > > + QCOM_SCM_BOOT_MC_FLAG_COLDBOOT); > > + if (ret) { > > + log_err("Failed to set CPU boot addr: %d\n", ret); > > + return ret; > > + } > > + > > + boot_addr_set = true; > > + } > > I would move this below log_info("Booting CPU"), so you don't waste > doing this for nothing if one of the other check fails. Good catch, fixed in v3. Thanks for the thorough review Stephan! -Sam > > > + > > + mpidr_aff = read_mpidr() & 0xffffff; > > + > > + if (reg == mpidr_aff) { > > + debug("Skipping boot of current CPU%d\n", reg); > > + return 0; > > + } > > + > > + ret = fdtdec_parse_phandle_with_args(fdt, cpu_offset, "qcom,acc", > > + NULL, 0, 0, &acc); > > + if (ret) { > > + log_err("Failed to parse qcom,acc phandle: %d\n", reg); > > + return ret; > > + } > > + > > + acc_base = fdtdec_get_addr_size_auto_noparent(fdt, acc.node, "reg", 0, > > + NULL, true); > > + if (!acc_base) { > > + log_err("Failed to parse qcom,acc regbase\n"); > > + return -EINVAL; > > + } > > + > > + log_info("Booting CPU%d @ 0x%x\n", reg, acc_base); > > + qcom_boot_cortex_a53(acc_base); > > + return 0; > > +} > > Thanks, > Stephan >
