Hi, Arnd,

On Fri, Sep 17, 2021 at 5:57 PM Arnd Bergmann [off-list ref] wrote:

On Fri, Sep 17, 2021 at 5:57 AM Huacai Chen [off-list ref] wrote:

quoted

+
+struct task_struct;
+
+struct plat_smp_ops {
+       void (*send_ipi_single)(int cpu, unsigned int action);
+       void (*send_ipi_mask)(const struct cpumask *mask, unsigned int action);
+       void (*smp_setup)(void);
+       void (*prepare_cpus)(unsigned int max_cpus);
+       int (*boot_secondary)(int cpu, struct task_struct *idle);
+       void (*init_secondary)(void);
+       void (*smp_finish)(void);
+#ifdef CONFIG_HOTPLUG_CPU
+       int (*cpu_disable)(void);
+       void (*cpu_die)(unsigned int cpu);
+#endif
+};

Do you foresee having more than one implementation of these in the
near future? If not, I would suggest leaving out the extra indirection
and just using direct function calls.

OK, let me rethink this, if it is still needed, I will tell you why.

quoted

+#ifdef CONFIG_SMP
+
+static inline void plat_smp_setup(void)
+{
+       extern const struct plat_smp_ops *mp_ops;       /* private */
+
+       mp_ops->smp_setup();
+}
+
+#else /* !CONFIG_SMP */
+
+static inline void plat_smp_setup(void) { }
+
+#endif /* !CONFIG_SMP */

You could even go further and do what arch/arm64 has, making
SMP support unconditional. This obviously depends on hardware
roadmaps, but if all harfdware you support has multiple cores,
then non-SMP mode just adds complexity.

As mentioned in another patch, we do have some MCU hardware (no FP, no
SMP, and even no MMU).

quoted

+
+#define MAX_CPUS 64

You CONFIG_NR_CPUS allows up to 256. I think you need to
adjust one of the numbers to match the other, or remove this
definition and just use CONFIG_NR_CPUS directly.

Legacy IPI method only supports at most 64 CPUs (limited by the MMIO
register space). Maybe we can remove the whole legacy method support,
then we can remove MAX_CPUS, too.

quoted

+
+static volatile void *ipi_set_regs[MAX_CPUS];
+static volatile void *ipi_clear_regs[MAX_CPUS];
+static volatile void *ipi_status_regs[MAX_CPUS];
+static volatile void *ipi_en_regs[MAX_CPUS];
+static volatile void *ipi_mailbox_buf[MAX_CPUS];

Why are these 'volatile'? If they are MMIO registers, they
should be __iomem, and accessed using readl()/writel()
 etc

Yes, they are MMIO registers and __iomem is needed.

Huacai

       Arnd