On 5/21/20 9:21 PM, Havard Skinnemoen wrote:
> +void npcm7xx_write_secondary_boot(ARMCPU *cpu, const struct arm_boot_info *info)
> +{
> + /*
> + * The default smpboot stub halts the secondary CPU with a 'wfi'
> + * instruction, but the arch/arm/mach-npcm/platsmp.c in the Linux kernel
> + * does not send an IPI to wake it up, so the second CPU fails to boot. So
> + * we need to provide our own smpboot stub that can not use 'wfi', it has
> + * to spin the secondary CPU until the first CPU writes to the SCRPAD reg.
> + */
> + uint32_t smpboot[] = {
static const uint32 ?
I think that would be unsafe due to the byte swapping, but I'll do it if we can get rid of that somehow.
> + 0xe59f2018, /* ldr r2, bootreg_addr */
> + 0xe3a00000, /* mov r0, #0 */
> + 0xe5820000, /* str r0, [r2] */
> + 0xe320f002, /* wfe */
> + 0xe5921000, /* ldr r1, [r2] */
> + 0xe1110001, /* tst r1, r1 */
> + 0x0afffffb, /* beq <wfe> */
> + 0xe12fff11, /* bx r1 */
> + NPCM7XX_SMP_BOOTREG_ADDR,
> + };
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(smpboot); i++) {
> + smpboot[i] = tswap32(smpboot[i]);
ah ! why do we need to swap the instructions ?
IIUC, it's necessary for the target to see the instruction words specified above when its endianness is different from the host. But perhaps I can specify it as a byte array instead. Would that work?
If that works, I should be able to drop the byte swapping and make the smpboot array constant. I don't think I'll be able to test it thoroughly though, as I don't have access to a big endian host.
> +static void npcm7xx_init(Object *obj)
> +{
> + NPCM7xxState *s = NPCM7XX(obj);
> + int i;
> +
> + for (i = 0; i < NPCM7XX_MAX_NUM_CPUS; i++) {
> + s->cpu[i] = ARM_CPU(
> + object_new_with_props(ARM_CPU_TYPE_NAME("cortex-a9"),
> + obj, "cpu[*]", &error_abort, NULL));
why allocate ? Can't you use :
ARMCPU cpu[NPCM7XX_MAX_NUM_CPUS];
and call object_initialize_child() ?
OK, I will try that.
> + for (i = 0; i < s->num_cpus; i++) {
> + object_property_set_int(OBJECT(s->cpu[i]),
> + arm_cpu_mp_affinity(i, NPCM7XX_MAX_NUM_CPUS),
> + "mp-affinity", &error_abort);
> + object_property_set_int(OBJECT(s->cpu[i]), NPCM7XX_GIC_CPU_IF_ADDR,
> + "reset-cbar", &error_abort);
> + object_property_set_bool(OBJECT(s->cpu[i]), true,
> + "reset-hivecs", &error_abort);
> +
> + /* TODO: Not sure why this is needed. */
It's disabling the security extensions.
Thanks, I'll update the comment.
> + if (object_property_find(OBJECT(s->cpu[i]), "has_el3", NULL)) {
> + object_property_set_bool(OBJECT(s->cpu[i]), false, "has_el3",
> + &error_abort);
> + }
> +
> + object_property_set_bool(OBJECT(s->cpu[i]), true, "realized", &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> + }
With this pattern, which initializes all possible CPU objects and only
realizes the requested ones, some CPU objects are left unrealized in
the QOM tree. It's is better to unparent them.
If the board has a fixed number of CPUs and you don't plan to activate
only a few of them for debug/test, I would make the "num_cpus" a class
property. See the Aspeed boards.
Will do, thanks.
> +
> + /* A9MPCORE peripherals */
> + object_property_set_int(OBJECT(&s->a9mpcore), s->num_cpus, "num-cpu",
> + &error_abort);
> + object_property_set_int(OBJECT(&s->a9mpcore), 160, "num-irq", &error_abort);
160 needs a define.
Will do, thanks.