Re: [PATCH v5 1/9] RISC-V: Add riscv_fw_parent_hartid() function

[Anup Patel]

On Tue, Jul 11, 2023 at 6:56 PM Andrew Jones <ajones@xxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Jul 10, 2023 at 03:13:13PM +0530, Anup Patel wrote:
> > We add common riscv_fw_parent_hartid() which help device drivers
> > to get parent hartid of the INTC (i.e. local interrupt controller)
> > fwnode. This should work for both DT and ACPI.
> >
> > Signed-off-by: Anup Patel <apatel@xxxxxxxxxxxxxxxx>
> > ---
> >  arch/riscv/include/asm/processor.h |  3 +++
> >  arch/riscv/kernel/cpu.c            | 16 ++++++++++++++++
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> > index c950a8d9edef..39dc23a18f88 100644
> > --- a/arch/riscv/include/asm/processor.h
> > +++ b/arch/riscv/include/asm/processor.h
> > @@ -81,6 +81,9 @@ int riscv_of_processor_hartid(struct device_node *node, unsigned long *hartid);
> >  int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *hartid);
> >  int riscv_of_parent_hartid(struct device_node *node, unsigned long *hartid);
> >
> > +struct fwnode_handle;
> > +int riscv_fw_parent_hartid(struct fwnode_handle *node, unsigned long *hartid);
> > +
> >  extern void riscv_fill_hwcap(void);
> >  extern int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
> >
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index a2fc952318e9..9be9b3b1f333 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -96,6 +96,22 @@ int riscv_of_parent_hartid(struct device_node *node, unsigned long *hartid)
> >       return -1;
> >  }
> >
> > +/* Find hart ID of the CPU fwnode under which given fwnode falls. */
>
> This comment matches the comment for riscv_of_parent_hartid(), but I don't
> think it will be correct for the !is_of_node(node) case since
> fwnode_property_read_u64_array() isn't obliged to walk up its tree.
> Looking ahead it appears riscv_fw_parent_hartid() is only called with the
> parent node, so we could just drop this function and use
> fwnode_property_read_u64_array() directly at the two call sites.

I think the function name riscv_fw_parent_hartid() is misleading. It should
be riscv_get_intc_hartid() because it is retrieving hartid based on INTC
fwnode.

Currently, this function is used in APLIC and IMSIC drivers but soon it
will be also used in PLIC driver with the upcoming PLIC ACPI support.

In fact, this patch should also replace riscv_of_parent_hartid() with
riscv_get_intc_hartid() in INTC and PLIC.

Regards,
Anup

>
> Thanks,
> drew
>
> > +int riscv_fw_parent_hartid(struct fwnode_handle *node, unsigned long *hartid)
> > +{
> > +     int rc;
> > +     u64 temp;
> > +
> > +     if (!is_of_node(node)) {
> > +             rc = fwnode_property_read_u64_array(node, "hartid", &temp, 1);
> > +             if (!rc)
> > +                     *hartid = temp;
> > +     } else
> > +             rc = riscv_of_parent_hartid(to_of_node(node), hartid);
> > +
> > +     return rc;
> > +}
> > +
> >  DEFINE_PER_CPU(struct riscv_cpuinfo, riscv_cpuinfo);
> >
> >  unsigned long riscv_cached_mvendorid(unsigned int cpu_id)
> > --
> > 2.34.1
> >