[V2,1/9] uclass: cpu: Add new API to get udevice for current CPU

When running on SoC with multiple clusters, the boot CPU may
not be fixed, saying booting from cluster A or cluster B.
Add a API that can return the udevice for current boot CPU.
Cpu driver needs to implement is_current_cpu interface for this
feature, otherwise the API only returns the first udevice in
cpu uclass.

Signed-off-by: Peng Fan <peng.fan at nxp.com>
Signed-off-by: Ye Li <ye.li at nxp.com>
---

V2:
 Per Simon's comment,
  - Add cpu_is_current
  - use uclass_foreach_dev_probe
  - Update code comment

 drivers/cpu/cpu-uclass.c | 34 ++++++++++++++++++++++++++++++++++
 include/cpu.h            | 23 +++++++++++++++++++++++
 2 files changed, 57 insertions(+)

Hi Peng,

On Fri, 1 May 2020 at 07:22, Peng Fan <peng.fan at nxp.com> wrote:
>
> When running on SoC with multiple clusters, the boot CPU may
> not be fixed, saying booting from cluster A or cluster B.
> Add a API that can return the udevice for current boot CPU.
> Cpu driver needs to implement is_current_cpu interface for this
> feature, otherwise the API only returns the first udevice in
> cpu uclass.
>
> Signed-off-by: Peng Fan <peng.fan at nxp.com>
> Signed-off-by: Ye Li <ye.li at nxp.com>
> ---
>
> V2:
>  Per Simon's comment,
>   - Add cpu_is_current
>   - use uclass_foreach_dev_probe
>   - Update code comment
>
>  drivers/cpu/cpu-uclass.c | 34 ++++++++++++++++++++++++++++++++++
>  include/cpu.h            | 23 +++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
>

Reviewed-by: Simon Glass <sjg at chromium.org>


> diff --git a/drivers/cpu/cpu-uclass.c b/drivers/cpu/cpu-uclass.c
> index 457f77b7c8..33d38a0fde 100644
> --- a/drivers/cpu/cpu-uclass.c
> +++ b/drivers/cpu/cpu-uclass.c
> @@ -10,6 +10,7 @@
>  #include <errno.h>
>  #include <dm/lists.h>
>  #include <dm/root.h>
> +#include <linux/err.h>
>
>  int cpu_probe_all(void)
>  {
> @@ -34,6 +35,39 @@ int cpu_probe_all(void)
>         return 0;
>  }
>
> +int cpu_is_current(struct udevice *cpu)
> +{
> +       struct cpu_ops *ops = cpu_get_ops(cpu);
> +
> +       if (ops && ops->is_current) {
> +               if (ops->is_current(cpu))
> +                       return 1;

return 0 here I think

Also you should not check 'ops'.


> +       }
> +
> +       return -ENOSYS;
> +}
> +
> +struct udevice *cpu_get_current_dev(void)
> +{
> +       struct udevice *cpu;
> +       int ret;
> +
> +       uclass_foreach_dev_probe(UCLASS_CPU, cpu) {
> +               if (cpu_is_current(cpu) > 0)
> +                       return cpu;
> +       }
> +
> +       /* If can't find current cpu device, use the first dev instead */
> +       ret = uclass_first_device_err(UCLASS_CPU, &cpu);
> +       if (ret) {
> +               debug("%s: Could not get CPU device (err = %d)\n",
> +                     __func__, ret);
> +               return NULL;
> +       }
> +
> +       return cpu;
> +}
> +
>  int cpu_get_desc(struct udevice *dev, char *buf, int size)
>  {
>         struct cpu_ops *ops = cpu_get_ops(dev);
> diff --git a/include/cpu.h b/include/cpu.h
> index 6b1b6b37b3..2f283fe244 100644
> --- a/include/cpu.h
> +++ b/include/cpu.h
> @@ -89,6 +89,15 @@ struct cpu_ops {
>          * @return 0 if OK, -ENOSPC if buffer is too small, other -ve on error
>          */
>         int (*get_vendor)(struct udevice *dev, char *buf, int size);
> +
> +       /**
> +        * is_current() - Check if the CPU that U-Boot is currently running from
> +        *
> +        * @dev:        Device to check (UCLASS_CPU)
> +        * @return 1 if the CPU that U-Boot is currently running from, 0
> +        *         if not.
> +        */
> +       int (*is_current)(struct udevice *dev);
>  };
>
>  #define cpu_get_ops(dev)        ((struct cpu_ops *)(dev)->driver->ops)
> @@ -137,4 +146,18 @@ int cpu_get_vendor(struct udevice *dev, char *buf, int size);
>   */
>  int cpu_probe_all(void);
>
> +/**
> + * cpu_is_current() - Check if the CPU that U-Boot is currently running from
> + *
> + * Return: 1 if yes, - 0 if not
> + */
> +int cpu_is_current(struct udevice *cpu);
> +
> +/**
> + * cpu_get_current_dev() - Get CPU udevice for current CPU
> + *
> + * Return: udevice if OK, - NULL on error
> + */
> +struct udevice *cpu_get_current_dev(void);
> +
>  #endif
> --
> 2.16.4
>

Hi Simon,

> Subject: Re: [PATCH V2 1/9] uclass: cpu: Add new API to get udevice for
> current CPU
> 
> Hi Peng,
> 
> On Fri, 1 May 2020 at 07:22, Peng Fan <peng.fan at nxp.com> wrote:
> >
> > When running on SoC with multiple clusters, the boot CPU may not be
> > fixed, saying booting from cluster A or cluster B.
> > Add a API that can return the udevice for current boot CPU.
> > Cpu driver needs to implement is_current_cpu interface for this
> > feature, otherwise the API only returns the first udevice in cpu
> > uclass.
> >
> > Signed-off-by: Peng Fan <peng.fan at nxp.com>
> > Signed-off-by: Ye Li <ye.li at nxp.com>
> > ---
> >
> > V2:
> >  Per Simon's comment,
> >   - Add cpu_is_current
> >   - use uclass_foreach_dev_probe
> >   - Update code comment
> >
> >  drivers/cpu/cpu-uclass.c | 34 ++++++++++++++++++++++++++++++++++
> >  include/cpu.h            | 23 +++++++++++++++++++++++
> >  2 files changed, 57 insertions(+)
> >
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> 
> > diff --git a/drivers/cpu/cpu-uclass.c b/drivers/cpu/cpu-uclass.c index
> > 457f77b7c8..33d38a0fde 100644
> > --- a/drivers/cpu/cpu-uclass.c
> > +++ b/drivers/cpu/cpu-uclass.c
> > @@ -10,6 +10,7 @@
> >  #include <errno.h>
> >  #include <dm/lists.h>
> >  #include <dm/root.h>
> > +#include <linux/err.h>
> >
> >  int cpu_probe_all(void)
> >  {
> > @@ -34,6 +35,39 @@ int cpu_probe_all(void)
> >         return 0;
> >  }
> >
> > +int cpu_is_current(struct udevice *cpu) {
> > +       struct cpu_ops *ops = cpu_get_ops(cpu);
> > +
> > +       if (ops && ops->is_current) {
> > +               if (ops->is_current(cpu))
> > +                       return 1;
> 
> return 0 here I think

I prefer to use 1 here, since is_current return 0 seems
werid to show it is the cpu that uboot is running from.

> 
> Also you should not check 'ops'.

I'll drop it.

Thanks,
Peng.

> 
> 
> > +       }
> > +
> > +       return -ENOSYS;
> > +}
> > +
> > +struct udevice *cpu_get_current_dev(void) {
> > +       struct udevice *cpu;
> > +       int ret;
> > +
> > +       uclass_foreach_dev_probe(UCLASS_CPU, cpu) {
> > +               if (cpu_is_current(cpu) > 0)
> > +                       return cpu;
> > +       }
> > +
> > +       /* If can't find current cpu device, use the first dev instead */
> > +       ret = uclass_first_device_err(UCLASS_CPU, &cpu);
> > +       if (ret) {
> > +               debug("%s: Could not get CPU device (err = %d)\n",
> > +                     __func__, ret);
> > +               return NULL;
> > +       }
> > +
> > +       return cpu;
> > +}
> > +
> >  int cpu_get_desc(struct udevice *dev, char *buf, int size)  {
> >         struct cpu_ops *ops = cpu_get_ops(dev); diff --git
> > a/include/cpu.h b/include/cpu.h index 6b1b6b37b3..2f283fe244 100644
> > --- a/include/cpu.h
> > +++ b/include/cpu.h
> > @@ -89,6 +89,15 @@ struct cpu_ops {
> >          * @return 0 if OK, -ENOSPC if buffer is too small, other -ve on
> error
> >          */
> >         int (*get_vendor)(struct udevice *dev, char *buf, int size);
> > +
> > +       /**
> > +        * is_current() - Check if the CPU that U-Boot is currently running
> from
> > +        *
> > +        * @dev:        Device to check (UCLASS_CPU)
> > +        * @return 1 if the CPU that U-Boot is currently running from, 0
> > +        *         if not.
> > +        */
> > +       int (*is_current)(struct udevice *dev);
> >  };
> >
> >  #define cpu_get_ops(dev)        ((struct cpu_ops *)(dev)->driver->ops)
> > @@ -137,4 +146,18 @@ int cpu_get_vendor(struct udevice *dev, char *buf,
> int size);
> >   */
> >  int cpu_probe_all(void);
> >
> > +/**
> > + * cpu_is_current() - Check if the CPU that U-Boot is currently
> > +running from
> > + *
> > + * Return: 1 if yes, - 0 if not
> > + */
> > +int cpu_is_current(struct udevice *cpu);
> > +
> > +/**
> > + * cpu_get_current_dev() - Get CPU udevice for current CPU
> > + *
> > + * Return: udevice if OK, - NULL on error  */ struct udevice
> > +*cpu_get_current_dev(void);
> > +
> >  #endif
> > --
> > 2.16.4
> >

Hi Peng,

On Sun, 3 May 2020 at 07:14, Peng Fan <peng.fan at nxp.com> wrote:
>
> Hi Simon,
>
> > Subject: Re: [PATCH V2 1/9] uclass: cpu: Add new API to get udevice for
> > current CPU
> >
> > Hi Peng,
> >
> > On Fri, 1 May 2020 at 07:22, Peng Fan <peng.fan at nxp.com> wrote:
> > >
> > > When running on SoC with multiple clusters, the boot CPU may not be
> > > fixed, saying booting from cluster A or cluster B.
> > > Add a API that can return the udevice for current boot CPU.
> > > Cpu driver needs to implement is_current_cpu interface for this
> > > feature, otherwise the API only returns the first udevice in cpu
> > > uclass.
> > >
> > > Signed-off-by: Peng Fan <peng.fan at nxp.com>
> > > Signed-off-by: Ye Li <ye.li at nxp.com>
> > > ---
> > >
> > > V2:
> > >  Per Simon's comment,
> > >   - Add cpu_is_current
> > >   - use uclass_foreach_dev_probe
> > >   - Update code comment
> > >
> > >  drivers/cpu/cpu-uclass.c | 34 ++++++++++++++++++++++++++++++++++
> > >  include/cpu.h            | 23 +++++++++++++++++++++++
> > >  2 files changed, 57 insertions(+)
> > >
> >
> > Reviewed-by: Simon Glass <sjg at chromium.org>
> >
> >
> > > diff --git a/drivers/cpu/cpu-uclass.c b/drivers/cpu/cpu-uclass.c index
> > > 457f77b7c8..33d38a0fde 100644
> > > --- a/drivers/cpu/cpu-uclass.c
> > > +++ b/drivers/cpu/cpu-uclass.c
> > > @@ -10,6 +10,7 @@
> > >  #include <errno.h>
> > >  #include <dm/lists.h>
> > >  #include <dm/root.h>
> > > +#include <linux/err.h>
> > >
> > >  int cpu_probe_all(void)
> > >  {
> > > @@ -34,6 +35,39 @@ int cpu_probe_all(void)
> > >         return 0;
> > >  }
> > >
> > > +int cpu_is_current(struct udevice *cpu) {
> > > +       struct cpu_ops *ops = cpu_get_ops(cpu);
> > > +
> > > +       if (ops && ops->is_current) {
> > > +               if (ops->is_current(cpu))
> > > +                       return 1;
> >
> > return 0 here I think

Sorry that as very unclear. I mean that you should have something like:

       if (ops->is_current) {
               if (ops->is_current(cpu))
                       return 1;
               return 0;
       }
       return -ENOSYS;

since if the driver is not current you should return so, and not -ENOSYS.

Also, why not just return what the driver returns? E.g. if the driver
returns an error you should return it. The normal pattern used is:

struct cpu_ops *ops = cpu_get_ops(cpu);

if (!ops->is_current)
    return -ENOSYS;

ret = ops->is_current(cpu);
if (ret)
    return log_ret(ret);

return 0;

>
> I prefer to use 1 here, since is_current return 0 seems
> werid to show it is the cpu that uboot is running from.
>
> >
> > Also you should not check 'ops'.
>
> I'll drop it.
>
> Thanks,
> Peng.
>

Regards,
Simon

Hi Simon,

> Subject: Re: [PATCH V2 1/9] uclass: cpu: Add new API to get udevice for
> current CPU
> 
> Hi Peng,
> 
> On Sun, 3 May 2020 at 07:14, Peng Fan <peng.fan at nxp.com> wrote:
> >
> > Hi Simon,
> >
> > > Subject: Re: [PATCH V2 1/9] uclass: cpu: Add new API to get udevice
> > > for current CPU
> > >
> > > Hi Peng,
> > >
> > > On Fri, 1 May 2020 at 07:22, Peng Fan <peng.fan at nxp.com> wrote:
> > > >
> > > > When running on SoC with multiple clusters, the boot CPU may not
> > > > be fixed, saying booting from cluster A or cluster B.
> > > > Add a API that can return the udevice for current boot CPU.
> > > > Cpu driver needs to implement is_current_cpu interface for this
> > > > feature, otherwise the API only returns the first udevice in cpu
> > > > uclass.
> > > >
> > > > Signed-off-by: Peng Fan <peng.fan at nxp.com>
> > > > Signed-off-by: Ye Li <ye.li at nxp.com>
> > > > ---
> > > >
> > > > V2:
> > > >  Per Simon's comment,
> > > >   - Add cpu_is_current
> > > >   - use uclass_foreach_dev_probe
> > > >   - Update code comment
> > > >
> > > >  drivers/cpu/cpu-uclass.c | 34
> ++++++++++++++++++++++++++++++++++
> > > >  include/cpu.h            | 23 +++++++++++++++++++++++
> > > >  2 files changed, 57 insertions(+)
> > > >
> > >
> > > Reviewed-by: Simon Glass <sjg at chromium.org>
> > >
> > >
> > > > diff --git a/drivers/cpu/cpu-uclass.c b/drivers/cpu/cpu-uclass.c
> > > > index 457f77b7c8..33d38a0fde 100644
> > > > --- a/drivers/cpu/cpu-uclass.c
> > > > +++ b/drivers/cpu/cpu-uclass.c
> > > > @@ -10,6 +10,7 @@
> > > >  #include <errno.h>
> > > >  #include <dm/lists.h>
> > > >  #include <dm/root.h>
> > > > +#include <linux/err.h>
> > > >
> > > >  int cpu_probe_all(void)
> > > >  {
> > > > @@ -34,6 +35,39 @@ int cpu_probe_all(void)
> > > >         return 0;
> > > >  }
> > > >
> > > > +int cpu_is_current(struct udevice *cpu) {
> > > > +       struct cpu_ops *ops = cpu_get_ops(cpu);
> > > > +
> > > > +       if (ops && ops->is_current) {
> > > > +               if (ops->is_current(cpu))
> > > > +                       return 1;
> > >
> > > return 0 here I think
> 
> Sorry that as very unclear. I mean that you should have something like:
> 
>        if (ops->is_current) {
>                if (ops->is_current(cpu))
>                        return 1;
>                return 0;
>        }
>        return -ENOSYS;
> 
> since if the driver is not current you should return so, and not -ENOSYS.
> 
> Also, why not just return what the driver returns? E.g. if the driver returns an
> error you should return it. The normal pattern used is:
> 
> struct cpu_ops *ops = cpu_get_ops(cpu);
> 
> if (!ops->is_current)
>     return -ENOSYS;
> 
> ret = ops->is_current(cpu);
> if (ret)
>     return log_ret(ret);
> 
> return 0;

Since the patch has been applied by Stefano, I'll create a follow up patch
to address the comments you mentioned.

Thanks,
Peng.

> 
> >
> > I prefer to use 1 here, since is_current return 0 seems werid to show
> > it is the cpu that uboot is running from.
> >
> > >
> > > Also you should not check 'ops'.
> >
> > I'll drop it.
> >
> > Thanks,
> > Peng.
> >
> 
> Regards,
> Simon