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

  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.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 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(-)
  

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.

> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  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?

>  
>  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;
   }

> +			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'.

> +		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
  

On Wed, Aug 02, 2023 at 08:09:18PM +0300, Andrew Jones 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.

Yeah, it certainly needs a justification, not just an "oh, btw I did
this". It also invalidates the comment above the function, so that
would need to be changed too.

> > 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,
  

On Wed, Aug 02, 2023 at 08:30:04PM +0530, Anup Patel wrote:

> +/* 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);
> +		if (!rc)
> +			*hartid = temp;
> +	} else
> +		rc = riscv_of_parent_hartid(to_of_node(node), hartid);

This branch needs to be enclosed in braces too.

> +
> +	return rc;
> +}
> +
>  DEFINE_PER_CPU(struct riscv_cpuinfo, riscv_cpuinfo);
  

On Wed, Aug 2, 2023 at 10:41 PM Andrew Jones <ajones@ventanamicro.com> 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@ventanamicro.com>
> > ---
> >  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
  

On Wed, Aug 2, 2023 at 10:51 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Aug 02, 2023 at 08:30:04PM +0530, Anup Patel wrote:
>
> > +/* 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);
> > +             if (!rc)
> > +                     *hartid = temp;
> > +     } else
> > +             rc = riscv_of_parent_hartid(to_of_node(node), hartid);
>
> This branch needs to be enclosed in braces too.

Okay, I will update in the next revision.

>
> > +
> > +     return rc;
> > +}
> > +
> >  DEFINE_PER_CPU(struct riscv_cpuinfo, riscv_cpuinfo);
>

Regards,
Anup