cmd: add temperature command

Currently, there is no way for users to check the readings from thermal
sensors from U-boot console, only some boards print it during boot.

So, lets add a simple "temperature" command that allows listing thermal
uclass devices and getting their value.

Note that the thermal devices are intenionally probed if list is used as
almost always they will not get probed otherwise and there is no way for
users to manually call probe on a certain device from console.

Assumption is made that temperature is returned in degrees C and not
milidegrees like in Linux as this is what most drivers seem to return.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
 cmd/Kconfig       |  6 ++++
 cmd/Makefile      |  1 +
 cmd/temperature.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 cmd/temperature.c

Hi Robert,

On Fri, 12 Aug 2022 at 06:19, Robert Marko <robert.marko@sartura.hr> wrote:
>
> Currently, there is no way for users to check the readings from thermal
> sensors from U-boot console, only some boards print it during boot.
>
> So, lets add a simple "temperature" command that allows listing thermal
> uclass devices and getting their value.
>
> Note that the thermal devices are intenionally probed if list is used as
> almost always they will not get probed otherwise and there is no way for
> users to manually call probe on a certain device from console.
>
> Assumption is made that temperature is returned in degrees C and not
> milidegrees like in Linux as this is what most drivers seem to return.
>
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> ---
>  cmd/Kconfig       |  6 ++++
>  cmd/Makefile      |  1 +
>  cmd/temperature.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 93 insertions(+)
>  create mode 100644 cmd/temperature.c

Don't forget to add doc/usage/command/.. and a sandbox test in test/cmd/

https://u-boot.readthedocs.io/en/latest/develop/testing.html

>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 3625ff2a50..9bd639c740 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1435,6 +1435,12 @@ config DEFAULT_SPI_MODE
>         depends on CMD_SPI
>         default 0
>
> +config CMD_TEMPERATURE
> +       bool "temperature"

How about:

temperature - display the temperature from thermal sensors

> +       depends on DM_THERMAL
> +       help
> +         Provides a way to get the temperature reading from thermal sensors.

It also allows listing.

> +
>  config CMD_TSI148
>         bool "tsi148 - Command to access tsi148 device"
>         help
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 5e43a1e022..8874462f1a 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -152,6 +152,7 @@ obj-$(CONFIG_CMD_STRINGS) += strings.o
>  obj-$(CONFIG_CMD_SMC) += smccc.o
>  obj-$(CONFIG_CMD_SYSBOOT) += sysboot.o
>  obj-$(CONFIG_CMD_STACKPROTECTOR_TEST) += stackprot_test.o
> +obj-$(CONFIG_CMD_TEMPERATURE) += temperature.o
>  obj-$(CONFIG_CMD_TERMINAL) += terminal.o
>  obj-$(CONFIG_CMD_TIME) += time.o
>  obj-$(CONFIG_CMD_TIMER) += timer.o
> diff --git a/cmd/temperature.c b/cmd/temperature.c
> new file mode 100644
> index 0000000000..ccf839058e
> --- /dev/null
> +++ b/cmd/temperature.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Copyright (c) 2022 Sartura Ltd.
> + * Written by Robert Marko <robert.marko@sartura.hr>
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <dm.h>
> +#include <dm/uclass-internal.h>

I hope you can drop this

> +#include <thermal.h>
> +
> +#define LIMIT_DEVNAME  30
> +
> +static int do_get(struct cmd_tbl *cmdtp, int flag, int argc,
> +                 char *const argv[])
> +{
> +       struct udevice *dev;
> +       int ret, temp;
> +
> +       if (argc < 2) {
> +               printf("thermal device not selected\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       ret = uclass_find_device_by_name(UCLASS_THERMAL, argv[1], &dev);

You should use the get function normally, since it probes the device
and for most devices you should only call their methods when the
device is probed/activated.

> +       if (ret) {
> +               printf("thermal device not found\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       ret = thermal_get_temp(dev, &temp);
> +       if (ret)
> +               return CMD_RET_FAILURE;
> +
> +       printf("%s: %d°C\n", dev->name, temp);
> +
> +       return CMD_RET_SUCCESS;
> +}
> +
> +static int do_list(struct cmd_tbl *cmdtp, int flag, int argc,
> +                  char *const argv[])
> +{
> +       struct udevice *dev;
> +
> +       printf("| %-*.*s| %-*.*s| %s\n",
> +              LIMIT_DEVNAME, LIMIT_DEVNAME, "Device",
> +              LIMIT_DEVNAME, LIMIT_DEVNAME, "Driver",
> +              "Parent");
> +
> +       uclass_foreach_dev_probe(UCLASS_THERMAL, dev) {
> +               printf("| %-*.*s| %-*.*s| %s\n",
> +                      LIMIT_DEVNAME, LIMIT_DEVNAME, dev->name,
> +                      LIMIT_DEVNAME, LIMIT_DEVNAME, dev->driver->name,
> +                      dev->parent->name);
> +       }

BTW as I'm sure you know, this loop probes the device, which is fine.

> +
> +       return CMD_RET_SUCCESS;
> +}
> +
> +static struct cmd_tbl temperature_subcmd[] = {
> +       U_BOOT_CMD_MKENT(list, 1, 1, do_list, "", ""),
> +       U_BOOT_CMD_MKENT(get, 2, 1, do_get, "", ""),
> +};
> +
> +static int do_temperature(struct cmd_tbl *cmdtp, int flag, int argc,
> +                         char *const argv[])
> +{
> +       struct cmd_tbl *cmd;
> +
> +       argc--;
> +       argv++;
> +
> +       cmd = find_cmd_tbl(argv[0], temperature_subcmd, ARRAY_SIZE(temperature_subcmd));
> +       if (!cmd || argc > cmd->maxargs)
> +               return CMD_RET_USAGE;
> +
> +       return cmd->cmd(cmdtp, flag, argc, argv);
> +}
> +
> +U_BOOT_CMD(temperature, CONFIG_SYS_MAXARGS, 1, do_temperature,
> +          "thermal sensor temperature",
> +          "list\t\tshow list of temperature sensors\n"
> +          "get [thermal device name]\tprint temperature"

in degrees C ?

> +);
> --
> 2.37.1
>

Regards,
Simon

On Fri, Aug 12, 2022 at 5:11 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Robert,
>
> On Fri, 12 Aug 2022 at 06:19, Robert Marko <robert.marko@sartura.hr> wrote:
> >
> > Currently, there is no way for users to check the readings from thermal
> > sensors from U-boot console, only some boards print it during boot.
> >
> > So, lets add a simple "temperature" command that allows listing thermal
> > uclass devices and getting their value.
> >
> > Note that the thermal devices are intenionally probed if list is used as
> > almost always they will not get probed otherwise and there is no way for
> > users to manually call probe on a certain device from console.
> >
> > Assumption is made that temperature is returned in degrees C and not
> > milidegrees like in Linux as this is what most drivers seem to return.
> >
> > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > ---
> >  cmd/Kconfig       |  6 ++++
> >  cmd/Makefile      |  1 +
> >  cmd/temperature.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 93 insertions(+)
> >  create mode 100644 cmd/temperature.c
>
> Don't forget to add doc/usage/command/.. and a sandbox test in test/cmd/
>
> https://u-boot.readthedocs.io/en/latest/develop/testing.html

Yeah, forgot those, will include them in v2.
>
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 3625ff2a50..9bd639c740 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -1435,6 +1435,12 @@ config DEFAULT_SPI_MODE
> >         depends on CMD_SPI
> >         default 0
> >
> > +config CMD_TEMPERATURE
> > +       bool "temperature"
>
> How about:
>
> temperature - display the temperature from thermal sensors
>
> > +       depends on DM_THERMAL
> > +       help
> > +         Provides a way to get the temperature reading from thermal sensors.
>
> It also allows listing.

Will expand on this and the previous point.
>
> > +
> >  config CMD_TSI148
> >         bool "tsi148 - Command to access tsi148 device"
> >         help
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index 5e43a1e022..8874462f1a 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -152,6 +152,7 @@ obj-$(CONFIG_CMD_STRINGS) += strings.o
> >  obj-$(CONFIG_CMD_SMC) += smccc.o
> >  obj-$(CONFIG_CMD_SYSBOOT) += sysboot.o
> >  obj-$(CONFIG_CMD_STACKPROTECTOR_TEST) += stackprot_test.o
> > +obj-$(CONFIG_CMD_TEMPERATURE) += temperature.o
> >  obj-$(CONFIG_CMD_TERMINAL) += terminal.o
> >  obj-$(CONFIG_CMD_TIME) += time.o
> >  obj-$(CONFIG_CMD_TIMER) += timer.o
> > diff --git a/cmd/temperature.c b/cmd/temperature.c
> > new file mode 100644
> > index 0000000000..ccf839058e
> > --- /dev/null
> > +++ b/cmd/temperature.c
> > @@ -0,0 +1,86 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +/*
> > + * Copyright (c) 2022 Sartura Ltd.
> > + * Written by Robert Marko <robert.marko@sartura.hr>
> > + */
> > +
> > +#include <common.h>
> > +#include <command.h>
> > +#include <dm.h>
> > +#include <dm/uclass-internal.h>
>
> I hope you can drop this
>
> > +#include <thermal.h>
> > +
> > +#define LIMIT_DEVNAME  30
> > +
> > +static int do_get(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                 char *const argv[])
> > +{
> > +       struct udevice *dev;
> > +       int ret, temp;
> > +
> > +       if (argc < 2) {
> > +               printf("thermal device not selected\n");
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       ret = uclass_get_device_by_name(UCLASS_THERMAL, argv[1], &dev);
>
> You should use the get function normally, since it probes the device
> and for most devices you should only call their methods when the
> device is probed/activated.

I actually wanted the behavior of uclass_get_device_by_name() but completely
missed that it exists, so that is why uclass_foreach_dev_probe() was
used in list
to probe the drivers, but it still allowed for user to directly feed
the device-name
and crash U-boot as device being passed to thermal_get_temp was not probed.

Will switch to uclass_get_device_by_name() in v2 to solve that.
>
> > +       if (ret) {
> > +               printf("thermal device not found\n");
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       ret = thermal_get_temp(dev, &temp);
> > +       if (ret)
> > +               return CMD_RET_FAILURE;
> > +
> > +       printf("%s: %d°C\n", dev->name, temp);
> > +
> > +       return CMD_RET_SUCCESS;
> > +}
> > +
> > +static int do_list(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                  char *const argv[])
> > +{
> > +       struct udevice *dev;
> > +
> > +       printf("| %-*.*s| %-*.*s| %s\n",
> > +              LIMIT_DEVNAME, LIMIT_DEVNAME, "Device",
> > +              LIMIT_DEVNAME, LIMIT_DEVNAME, "Driver",
> > +              "Parent");
> > +
> > +       uclass_foreach_dev_probe(UCLASS_THERMAL, dev) {
> > +               printf("| %-*.*s| %-*.*s| %s\n",
> > +                      LIMIT_DEVNAME, LIMIT_DEVNAME, dev->name,
> > +                      LIMIT_DEVNAME, LIMIT_DEVNAME, dev->driver->name,
> > +                      dev->parent->name);
> > +       }
>
> BTW as I'm sure you know, this loop probes the device, which is fine.

Yeah, that was done intentionally to make sure that all sensors are
visible, it also
had a side-effect of making an oversight with using non probing DM
internal device
get helper.

>
> > +
> > +       return CMD_RET_SUCCESS;
> > +}
> > +
> > +static struct cmd_tbl temperature_subcmd[] = {
> > +       U_BOOT_CMD_MKENT(list, 1, 1, do_list, "", ""),
> > +       U_BOOT_CMD_MKENT(get, 2, 1, do_get, "", ""),
> > +};
> > +
> > +static int do_temperature(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                         char *const argv[])
> > +{
> > +       struct cmd_tbl *cmd;
> > +
> > +       argc--;
> > +       argv++;
> > +
> > +       cmd = find_cmd_tbl(argv[0], temperature_subcmd, ARRAY_SIZE(temperature_subcmd));
> > +       if (!cmd || argc > cmd->maxargs)
> > +               return CMD_RET_USAGE;
> > +
> > +       return cmd->cmd(cmdtp, flag, argc, argv);
> > +}
> > +
> > +U_BOOT_CMD(temperature, CONFIG_SYS_MAXARGS, 1, do_temperature,
> > +          "thermal sensor temperature",
> > +          "list\t\tshow list of temperature sensors\n"
> > +          "get [thermal device name]\tprint temperature"
>
> in degrees C ?

Will add it in v2.

Thanks for the review.

Regards,
Robert
>
> > +);
> > --
> > 2.37.1
> >
>
> Regards,
> Simon