Hi, Arnd,
On Fri, Sep 17, 2021 at 5:57 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Fri, Sep 17, 2021 at 5:57 AM Huacai Chen <chenhuacai@xxxxxxxxxxx> wrote:
>
> > +
> > +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.
>
> > +#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).
>
> > +
> > +#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.
>
> > +
> > +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
