Re: [PATCH v7 01/15] RISC-V: Add riscv_get_intc_hartid() function

[Anup Patel]

On Wed, Aug 2, 2023 at 10:41 PM Andrew Jones <ajones@xxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Aug 02, 2023 at 08:30:04PM +0530, Anup Patel wrote:
> > We add a common riscv_get_intc_hartid() which help device drivers to
> > get hartid of the HART associated with a INTC (i.e. local interrupt
> > controller) fwnode. This new function is more generic compared to
> > the existing riscv_of_parent_hartid() function hence we also replace
> > use of riscv_of_parent_hartid() with riscv_get_intc_hartid().
> >
> > Also, while we are here let us update riscv_of_parent_hartid() to
> > always return the hartid irrespective whether the CPU/HART DT node
> > is disabled or not.
>
> This change should probably be a separate patch with its own
> justification in its commit message.

Okay, I will move this into a separate patch in the next revision.

>
> >
> > Signed-off-by: Anup Patel <apatel@xxxxxxxxxxxxxxxx>
> > ---
> >  arch/riscv/include/asm/processor.h |  4 +++-
> >  arch/riscv/kernel/cpu.c            | 26 ++++++++++++++++++++------
> >  drivers/irqchip/irq-riscv-intc.c   |  2 +-
> >  drivers/irqchip/irq-sifive-plic.c  |  3 ++-
> >  4 files changed, 26 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> > index c950a8d9edef..662da1e112dd 100644
> > --- a/arch/riscv/include/asm/processor.h
> > +++ b/arch/riscv/include/asm/processor.h
> > @@ -79,7 +79,9 @@ static inline void wait_for_interrupt(void)
> >  struct device_node;
> >  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_get_intc_hartid(struct fwnode_handle *node, unsigned long *hartid);
>
> Do we want a function that is named in a way that appears to be
> intc-specific in processor.h?

Yes, this is intended to be only for intc because only intc fwnode can have
"hartid" property.

>
> >
> >  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..c3eaa8a55bbe 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -81,21 +81,35 @@ int riscv_early_of_processor_hartid(struct device_node *node, unsigned long *har
> >   * To achieve this, we walk up the DT tree until we find an active
> >   * RISC-V core (HART) node and extract the cpuid from it.
> >   */
> > -int riscv_of_parent_hartid(struct device_node *node, unsigned long *hartid)
> > +static int riscv_of_parent_hartid(struct device_node *node,
> > +                               unsigned long *hartid)
> >  {
> > -     int rc;
> > -
> >       for (; node; node = node->parent) {
> >               if (of_device_is_compatible(node, "riscv")) {
> > -                     rc = riscv_of_processor_hartid(node, hartid);
> > -                     if (!rc)
> > -                             return 0;
> > +                     *hartid = (unsigned long)of_get_cpu_hwid(node, 0);
>
> Shouldn't we still do something like
>
>    if (*hartid == ~0UL) {
>        pr_warn_once("Found CPU without hart ID\n");
>        return -ENODEV;
>    }

Sure, I will add it in the next revision.

>
> > +                     return 0;
> >               }
> >       }
> >
> >       return -1;
> >  }
> >
> > +/* Find hart ID of the INTC fwnode. */
> > +int riscv_get_intc_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);
>
> This fwnode property read call seems premature, since we don't have any
> way to know that "hartid" will be a property of the intc since it's not a
> property documented in the DT binding. (I know Sunil has a series in
> progress which will introduce "hartid" for ACPI, but, even then, it seems
> like we need some documentation to point at that says '"hartid" is the
> name to use'.

Sure, I will return a failure if it is not an OF node and Sunil can include
a patch to extend this function for ACPI.

The idea here is that we create SW fwnodes for static ACPI tables and
the irqchip driver only uses fwnode APIs as an abstraction over DT and
ACPI. This way irqchip drivers work for both DT and ACPI with minimal
modifications.

Almost all properties of SW fwnodes are exactly same as defined by
the DT bindings except two synthetic properties:
1) "hartid" in INTC fwnode created only for ACPI
2) "gsi-base" in the APLIC fwnode created only for ACPI.

I suggest we should document both of these synthetic fwnode properties
in Documentation/riscv/acpi.rst since these are only for ACPI.

>
> > +             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)
> > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> > index 4adeee1bc391..65f4a2afb381 100644
> > --- a/drivers/irqchip/irq-riscv-intc.c
> > +++ b/drivers/irqchip/irq-riscv-intc.c
> > @@ -143,7 +143,7 @@ static int __init riscv_intc_init(struct device_node *node,
> >       int rc;
> >       unsigned long hartid;
> >
> > -     rc = riscv_of_parent_hartid(node, &hartid);
> > +     rc = riscv_get_intc_hartid(of_fwnode_handle(node), &hartid);
> >       if (rc < 0) {
> >               pr_warn("unable to find hart id for %pOF\n", node);
> >               return 0;
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index e1484905b7bd..56b0544b1f27 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -477,7 +477,8 @@ static int __init __plic_init(struct device_node *node,
> >                       continue;
> >               }
> >
> > -             error = riscv_of_parent_hartid(parent.np, &hartid);
> > +             error = riscv_get_intc_hartid(of_fwnode_handle(parent.np),
> > +                                           &hartid);
> >               if (error < 0) {
> >                       pr_warn("failed to parse hart ID for context %d.\n", i);
> >                       continue;
> > --
> > 2.34.1
> >
>
> Thanks,
> drew

Regards,
Anup