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

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

[Peng Fan]

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@nxp.com>
Signed-off-by: Ye Li <ye.li@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(+)

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

[PATCH V2 2/9] cpu: sandbox: support is_current

[Peng Fan]

Support querying whether U-Boot is running on current cpu

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 New

 drivers/cpu/cpu_sandbox.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/cpu/cpu_sandbox.c b/drivers/cpu/cpu_sandbox.c
index 05b384f6a4..30a12e5a53 100644
--- a/drivers/cpu/cpu_sandbox.c
+++ b/drivers/cpu/cpu_sandbox.c
@@ -36,11 +36,20 @@ int cpu_sandbox_get_vendor(struct udevice *dev, char *buf, int size)
 	return 0;
 }
 
+int cpu_sandbox_is_current(struct udevice *dev)
+{
+	if (!strcmp(dev->name, "cpu-test1"))
+		return 1;
+
+	return 0;
+}
+
 static const struct cpu_ops cpu_sandbox_ops = {
 	.get_desc = cpu_sandbox_get_desc,
 	.get_info = cpu_sandbox_get_info,
 	.get_count = cpu_sandbox_get_count,
 	.get_vendor = cpu_sandbox_get_vendor,
+	.is_current = cpu_sandbox_is_current,
 };
 
 int cpu_sandbox_probe(struct udevice *dev)
-- 
2.16.4

[PATCH V2 2/9] cpu: sandbox: support is_current

[Simon Glass]

On Fri, 1 May 2020 at 07:22, Peng Fan <peng.fan@nxp.com> wrote:
>
> Support querying whether U-Boot is running on current cpu
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>
> V2:
>  New
>
>  drivers/cpu/cpu_sandbox.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

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

[PATCH V2 3/9] test: cpu: test current cpu

[Peng Fan]

Add test whether the CPU is U-Boot is running from.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 New

 test/dm/cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/test/dm/cpu.c b/test/dm/cpu.c
index e6dc576ea3..def9b64a28 100644
--- a/test/dm/cpu.c
+++ b/test/dm/cpu.c
@@ -26,6 +26,8 @@ static int dm_test_cpu(struct unit_test_state *uts)
 		ut_assert(dev->flags & DM_FLAG_ACTIVATED);
 
 	ut_assertok(uclass_get_device_by_name(UCLASS_CPU, "cpu-test1", &dev));
+	ut_asserteq_ptr(cpu_get_current_dev(), dev);
+	ut_asserteq(cpu_is_current(dev), 1);
 
 	ut_assertok(cpu_get_desc(dev, text, sizeof(text)));
 	ut_assertok(strcmp(text, "LEG Inc. SuperMegaUltraTurbo CPU No. 1"));
-- 
2.16.4

[PATCH V2 3/9] test: cpu: test current cpu

[Simon Glass]

On Fri, 1 May 2020 at 07:22, Peng Fan <peng.fan@nxp.com> wrote:
>
> Add test whether the CPU is U-Boot is running from.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>
> V2:
>  New
>
>  test/dm/cpu.c | 2 ++
>  1 file changed, 2 insertions(+)

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

[PATCH V2 4/9] common: board_f: Use cpu_get_current_dev in print_cpuinfo

[Peng Fan]

From: Ye Li <ye.li@nxp.com>

Current print_cpuinfo gets the first udevice in CPU class to return
the cpu info. This has problem if the boot CPU is not fixed.

Changing to use new API cpu_get_current_dev to fix the issue.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Ye Li <ye.li@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 Add R-b tag

 common/board_f.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c
index 5c650f046c..5223453511 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -183,11 +183,11 @@ static int print_cpuinfo(void)
 	char desc[512];
 	int ret;
 
-	ret = uclass_first_device_err(UCLASS_CPU, &dev);
-	if (ret) {
-		debug("%s: Could not get CPU device (err = %d)\n",
-		      __func__, ret);
-		return ret;
+	dev = cpu_get_current_dev();
+	if (!dev) {
+		debug("%s: Could not get CPU device\n",
+		      __func__);
+		return -ENODEV;
 	}
 
 	ret = cpu_get_desc(dev, desc, sizeof(desc));
-- 
2.16.4

[PATCH V2 5/9] cpu: imx8: reimplement get cpu count

[Peng Fan]

Return 4 is not correct on i.MX8DX/DXL/8QM and etc. we need to count
available cpu node with device_type "cpu".

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 Add R-b tag

 drivers/cpu/imx8_cpu.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/cpu/imx8_cpu.c b/drivers/cpu/imx8_cpu.c
index 95653683ac..7c54c290b8 100644
--- a/drivers/cpu/imx8_cpu.c
+++ b/drivers/cpu/imx8_cpu.c
@@ -118,7 +118,24 @@ static int cpu_imx_get_info(struct udevice *dev, struct cpu_info *info)
 
 static int cpu_imx_get_count(struct udevice *dev)
 {
-	return 4;
+	ofnode node;
+	int num = 0;
+
+	ofnode_for_each_subnode(node, dev_ofnode(dev->parent)) {
+		const char *device_type;
+
+		if (!ofnode_is_available(node))
+			continue;
+
+		device_type = ofnode_read_string(node, "device_type");
+		if (!device_type)
+			continue;
+
+		if (!strcmp(device_type, "cpu"))
+			num++;
+	}
+
+	return num;
 }
 
 static int cpu_imx_get_vendor(struct udevice *dev,  char *buf, int size)
-- 
2.16.4

[PATCH V2 6/9] cpu: imx8: support a72 as boot cpu

[Peng Fan]

Support booting i.MX8QM with A72 as boot cpu

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 Add R-b tag

 drivers/cpu/imx8_cpu.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/cpu/imx8_cpu.c b/drivers/cpu/imx8_cpu.c
index 7c54c290b8..d1bd4edfc7 100644
--- a/drivers/cpu/imx8_cpu.c
+++ b/drivers/cpu/imx8_cpu.c
@@ -20,6 +20,7 @@ struct cpu_imx_platdata {
 	const char *type;
 	u32 cpurev;
 	u32 freq_mhz;
+	u32 mpidr;
 };
 
 const char *get_imx8_type(u32 imxtype)
@@ -144,16 +145,28 @@ static int cpu_imx_get_vendor(struct udevice *dev,  char *buf, int size)
 	return 0;
 }
 
+static int cpu_imx_is_current(struct udevice *dev)
+{
+	struct cpu_imx_platdata *plat = dev_get_platdata(dev);
+
+	if (plat->mpidr == (read_mpidr() & 0xffff))
+		return 1;
+
+	return 0;
+}
+
 static const struct cpu_ops cpu_imx8_ops = {
 	.get_desc	= cpu_imx_get_desc,
 	.get_info	= cpu_imx_get_info,
 	.get_count	= cpu_imx_get_count,
 	.get_vendor	= cpu_imx_get_vendor,
+	.is_current	= cpu_imx_is_current,
 };
 
 static const struct udevice_id cpu_imx8_ids[] = {
 	{ .compatible = "arm,cortex-a35" },
 	{ .compatible = "arm,cortex-a53" },
+	{ .compatible = "arm,cortex-a72" },
 	{ }
 };
 
@@ -185,6 +198,12 @@ static int imx8_cpu_probe(struct udevice *dev)
 	plat->rev = get_imx8_rev(cpurev & 0xFFF);
 	plat->type = get_imx8_type((cpurev & 0xFF000) >> 12);
 	plat->freq_mhz = imx8_get_cpu_rate() / 1000000;
+	plat->mpidr = dev_read_addr(dev);
+	if (plat->mpidr == FDT_ADDR_T_NONE) {
+		printf("%s: Failed to get CPU reg property\n", __func__);
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
2.16.4

[PATCH V2 7/9] cpu: imx8: fix get core name and rate

[Peng Fan]

When current cpu is A53, using is_cortex_a53 could not detect A72
information, so check cpu device compatible property to get
the correct information.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 Add R-b tag

 drivers/cpu/imx8_cpu.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/cpu/imx8_cpu.c b/drivers/cpu/imx8_cpu.c
index d1bd4edfc7..c96a2d3563 100644
--- a/drivers/cpu/imx8_cpu.c
+++ b/drivers/cpu/imx8_cpu.c
@@ -48,13 +48,13 @@ const char *get_imx8_rev(u32 rev)
 	}
 }
 
-const char *get_core_name(void)
+const char *get_core_name(struct udevice *dev)
 {
-	if (is_cortex_a35())
+	if (!device_is_compatible(dev, "arm,cortex-a35"))
 		return "A35";
-	else if (is_cortex_a53())
+	else if (!device_is_compatible(dev, "arm,cortex-a53"))
 		return "A53";
-	else if (is_cortex_a72())
+	else if (!device_is_compatible(dev, "arm,cortex-a72"))
 		return "A72";
 	else
 		return "?";
@@ -170,12 +170,19 @@ static const struct udevice_id cpu_imx8_ids[] = {
 	{ }
 };
 
-static ulong imx8_get_cpu_rate(void)
+static ulong imx8_get_cpu_rate(struct udevice *dev)
 {
 	ulong rate;
-	int ret;
-	int type = is_cortex_a35() ? SC_R_A35 : is_cortex_a53() ?
-		   SC_R_A53 : SC_R_A72;
+	int ret, type;
+
+	if (!device_is_compatible(dev, "arm,cortex-a35"))
+		type = SC_R_A35;
+	else if (!device_is_compatible(dev, "arm,cortex-a53"))
+		type = SC_R_A53;
+	else if (!device_is_compatible(dev, "arm,cortex-a72"))
+		type = SC_R_A72;
+	else
+		return 0;
 
 	ret = sc_pm_get_clock_rate(-1, type, SC_PM_CLK_CPU,
 				   (sc_pm_clock_rate_t *)&rate);
@@ -194,10 +201,10 @@ static int imx8_cpu_probe(struct udevice *dev)
 
 	cpurev = get_cpu_rev();
 	plat->cpurev = cpurev;
-	plat->name = get_core_name();
+	plat->name = get_core_name(dev);
 	plat->rev = get_imx8_rev(cpurev & 0xFFF);
 	plat->type = get_imx8_type((cpurev & 0xFF000) >> 12);
-	plat->freq_mhz = imx8_get_cpu_rate() / 1000000;
+	plat->freq_mhz = imx8_get_cpu_rate(dev) / 1000000;
 	plat->mpidr = dev_read_addr(dev);
 	if (plat->mpidr == FDT_ADDR_T_NONE) {
 		printf("%s: Failed to get CPU reg property\n", __func__);
-- 
2.16.4

[PATCH V2 8/9] cpu: imx_cpu: Print the CPU temperature for iMX8QM A72

[Peng Fan]

From: Ye Li <ye.li@nxp.com>

iMX8QM registers two thermal devices for CPUs, get the temperature
from "cpu-thermal1" device for A72

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

V2:
 Per Simon's comments, use uclass_get_device

 drivers/cpu/imx8_cpu.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/cpu/imx8_cpu.c b/drivers/cpu/imx8_cpu.c
index c96a2d3563..817e0ffd01 100644
--- a/drivers/cpu/imx8_cpu.c
+++ b/drivers/cpu/imx8_cpu.c
@@ -61,13 +61,15 @@ const char *get_core_name(struct udevice *dev)
 }
 
 #if IS_ENABLED(CONFIG_IMX_SCU_THERMAL)
-static int cpu_imx_get_temp(void)
+static int cpu_imx_get_temp(struct cpu_imx_platdata *plat)
 {
 	struct udevice *thermal_dev;
 	int cpu_tmp, ret;
 
-	ret = uclass_get_device_by_name(UCLASS_THERMAL, "cpu-thermal0",
-					&thermal_dev);
+	if (!strcmp(plat->name, "A72"))
+		ret = uclass_get_device(UCLASS_THERMAL, 1, &thermal_dev);
+	else
+		ret = uclass_get_device(UCLASS_THERMAL, 0, &thermal_dev);
 
 	if (!ret) {
 		ret = thermal_get_temp(thermal_dev, &cpu_tmp);
@@ -80,7 +82,7 @@ static int cpu_imx_get_temp(void)
 	return cpu_tmp;
 }
 #else
-static int cpu_imx_get_temp(void)
+static int cpu_imx_get_temp(struct cpu_imx_platdata *plat)
 {
 	return 0;
 }
@@ -89,7 +91,7 @@ static int cpu_imx_get_temp(void)
 int cpu_imx_get_desc(struct udevice *dev, char *buf, int size)
 {
 	struct cpu_imx_platdata *plat = dev_get_platdata(dev);
-	int ret;
+	int ret, temp;
 
 	if (size < 100)
 		return -ENOSPC;
@@ -98,9 +100,13 @@ int cpu_imx_get_desc(struct udevice *dev, char *buf, int size)
 		       plat->type, plat->rev, plat->name, plat->freq_mhz);
 
 	if (IS_ENABLED(CONFIG_IMX_SCU_THERMAL)) {
+		temp = cpu_imx_get_temp(plat);
 		buf = buf + ret;
 		size = size - ret;
-		ret = snprintf(buf, size, " at %dC", cpu_imx_get_temp());
+		if (temp != 0xdeadbeef)
+			ret = snprintf(buf, size, " at %dC", temp);
+		else
+			ret = snprintf(buf, size, " - invalid sensor data");
 	}
 
 	snprintf(buf + ret, size - ret, "\n");
-- 
2.16.4

[PATCH V2 8/9] cpu: imx_cpu: Print the CPU temperature for iMX8QM A72

[Simon Glass]

Hi Peng,

On Fri, 1 May 2020 at 07:22, Peng Fan <peng.fan@nxp.com> wrote:
>
> From: Ye Li <ye.li@nxp.com>
>
> iMX8QM registers two thermal devices for CPUs, get the temperature
> from "cpu-thermal1" device for A72
>
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>
> V2:
>  Per Simon's comments, use uclass_get_device
>
>  drivers/cpu/imx8_cpu.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)

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

This is still very strange code, but OK.

[PATCH V2 9/9] cpu: imx8: show RevC instead of Rev? at boot log

[Peng Fan]

From: Frank Li <Frank.Li@nxp.com>

Add REVC informaiton.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 Add R-b tag

 arch/arm/include/asm/arch-imx/cpu.h | 1 +
 drivers/cpu/imx8_cpu.c              | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/arch/arm/include/asm/arch-imx/cpu.h b/arch/arm/include/asm/arch-imx/cpu.h
index 5ade63665a..06b8122e7c 100644
--- a/arch/arm/include/asm/arch-imx/cpu.h
+++ b/arch/arm/include/asm/arch-imx/cpu.h
@@ -57,6 +57,7 @@
 
 #define CHIP_REV_A		0x0
 #define CHIP_REV_B		0x1
+#define CHIP_REV_C		0x2
 
 #define BOARD_REV_1_0           0x0
 #define BOARD_REV_2_0           0x1
diff --git a/drivers/cpu/imx8_cpu.c b/drivers/cpu/imx8_cpu.c
index 817e0ffd01..11e7d1e22e 100644
--- a/drivers/cpu/imx8_cpu.c
+++ b/drivers/cpu/imx8_cpu.c
@@ -43,6 +43,8 @@ const char *get_imx8_rev(u32 rev)
 		return "A";
 	case CHIP_REV_B:
 		return "B";
+	case CHIP_REV_C:
+		return "C";
 	default:
 		return "?";
 	}
-- 
2.16.4

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

[Simon Glass]

Hi Peng,

On Fri, 1 May 2020 at 07:22, Peng Fan <peng.fan@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@nxp.com>
> Signed-off-by: Ye Li <ye.li@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@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
>

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

[Peng Fan]

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@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@nxp.com>
> > Signed-off-by: Ye Li <ye.li@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@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
> >

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

[Simon Glass]

Hi Peng,

On Sun, 3 May 2020 at 07:14, Peng Fan <peng.fan@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@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@nxp.com>
> > > Signed-off-by: Ye Li <ye.li@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@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

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

[Peng Fan]

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@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@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@nxp.com>
> > > > Signed-off-by: Ye Li <ye.li@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@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