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 <[email protected]> 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 <[email protected]> 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 <[email protected]> > > > > Signed-off-by: Ye Li <[email protected]> > > > > --- > > > > > > > > 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 <[email protected]> > > > > > > > > > > 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
