Hi Keerthy,

On Mon, 11 Mar 2019 at 14:13, Keerthy j-keerthy@ti.com wrote:

Currently single instance temperature read out is supported. Enhance the same to support multiple instances.

Signed-off-by: Keerthy j-keerthy@ti.com

arch/arm/mach-imx/cpu.c | 2 +- drivers/mmc/omap_hsmmc.c | 2 +- drivers/thermal/imx_thermal.c | 2 +- drivers/thermal/thermal-uclass.c | 4 ++-- drivers/thermal/ti-bandgap.c | 13 ++++++++++--- include/thermal.h | 6 ++++-- 6 files changed, 19 insertions(+), 10 deletions(-)

Please can you add a simple test for the thermal uclass in test/dm?

diff --git a/arch/arm/mach-imx/cpu.c b/arch/arm/mach-imx/cpu.c index 6b83f92662..27d12b410e 100644 --- a/arch/arm/mach-imx/cpu.c +++ b/arch/arm/mach-imx/cpu.c @@ -231,7 +231,7 @@ int print_cpuinfo(void) printf("(%dC to %dC)", minc, maxc); ret = uclass_get_device(UCLASS_THERMAL, 0, &thermal_dev); if (!ret) {

•           ret = thermal_get_temp(thermal_dev, &cpu_tmp);
  

•           ret = thermal_get_temp(thermal_dev, 0, &cpu_tmp);
  
            if (!ret)
                    printf(" at %dC\n", cpu_tmp);
  

diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c index 826a39fad7..1872273dd9 100644 --- a/drivers/mmc/omap_hsmmc.c +++ b/drivers/mmc/omap_hsmmc.c @@ -642,7 +642,7 @@ static int omap_hsmmc_execute_tuning(struct udevice *dev, uint opcode) printf("Couldn't get thermal device for tuning\n"); return ret; }

•   ret = thermal_get_temp(thermal_dev, &temperature);
  

•   ret = thermal_get_temp(thermal_dev, 0, &temperature);
    if (ret) {
            printf("Couldn't get temperature for tuning\n");
            return ret;
  

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c index e50b85bd59..271a61356c 100644 --- a/drivers/thermal/imx_thermal.c +++ b/drivers/thermal/imx_thermal.c @@ -207,7 +207,7 @@ static int read_cpu_temperature(struct udevice *dev) } #endif

-int imx_thermal_get_temp(struct udevice *dev, int *temp) +int imx_thermal_get_temp(struct udevice *dev, int instance, int *temp) { struct thermal_data *priv = dev_get_priv(dev); int cpu_tmp = 0; diff --git a/drivers/thermal/thermal-uclass.c b/drivers/thermal/thermal-uclass.c index a4ea1e2914..b4a31280cb 100644 --- a/drivers/thermal/thermal-uclass.c +++ b/drivers/thermal/thermal-uclass.c @@ -13,14 +13,14 @@ #include <linux/list.h>

-int thermal_get_temp(struct udevice *dev, int *temp) +int thermal_get_temp(struct udevice *dev, u8 instance, int *temp) { const struct dm_thermal_ops *ops = device_get_ops(dev);

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

•   return ops->get_temp(dev, temp);
  

•   return ops->get_temp(dev, instance, temp);
  

}

UCLASS_DRIVER(thermal) = { diff --git a/drivers/thermal/ti-bandgap.c b/drivers/thermal/ti-bandgap.c index b490391e96..6d3606b0fc 100644 --- a/drivers/thermal/ti-bandgap.c +++ b/drivers/thermal/ti-bandgap.c @@ -27,6 +27,7 @@ struct ti_bandgap { ulong base; int temperature; /* in mili degree celsius */

•   int                     sens_cnt;
  

};

/* @@ -158,11 +159,16 @@ static int dra752_adc_to_temp[] = { 123800, 124200, 124600, 124900, 125000, 125000, };

-static int ti_bandgap_get_temp(struct udevice *dev, int *temp) +static int ti_bandgap_get_temp(struct udevice *dev, u8 instance, int *temp) { struct ti_bandgap *bgp = dev_get_priv(dev);

•   bgp->temperature = 0x3ff & readl(bgp->base + CTRL_CORE_TEMP_SENSOR_MPU);
  

•   if (instance >= bgp->sens_cnt) {
  

•           printf("Only %d insatnces are supported\n", bgp->sens_cnt);
  

•           return -EINVAL;
  

•   }
  

•   bgp->temperature = 0x3ff & readl(bgp->base + instance * 4);
    *temp = dra752_adc_to_temp[bgp->temperature - DRA752_ADC_START_VALUE];
  
    return 0;
  

@@ -177,13 +183,14 @@ static int ti_bandgap_probe(struct udevice *dev) struct ti_bandgap *bgp = dev_get_priv(dev);

    bgp->base = devfdt_get_addr_index(dev, 1);

•   bgp->sens_cnt = dev_get_driver_data(dev);
  
    return 0;
  

}

static const struct udevice_id of_ti_bandgap_match[] = { {

•           .compatible = "ti,dra752-bandgap",
  

•           .compatible = "ti,dra752-bandgap", .data = 3,
    },
    {},
  

}; diff --git a/include/thermal.h b/include/thermal.h index 11d75256e0..82d80f3ded 100644 --- a/include/thermal.h +++ b/include/thermal.h @@ -9,7 +9,7 @@

#include <dm.h>

-int thermal_get_temp(struct udevice *dev, int *temp); +int thermal_get_temp(struct udevice *dev, u8 instance, int *temp);

Please add function comment.

/**

• struct dm_thermal_ops - Driver model Thermal operations

@@ -25,9 +25,11 @@ struct dm_thermal_ops { * It will enable and initialize any Thermal hardware as necessary. * * @dev: The Thermal device

•    * @instance:   The instance of sensor in case multiple sensors are
  

•    *              present if single sensor then this should be 0
     * @temp:       pointer that returns the measured temperature
     */
  

•   int (*get_temp)(struct udevice *dev, int *temp);
  

•   int (*get_temp)(struct udevice *dev, u8 instance, int *temp);
  

I don't like the name 'instance' very much. With pwm we use 'channel'. Would that be OK?

};

#endif /* _THERMAL_H_ */

2.17.1

Regards, Simon

Hi Keerthy,

On Tue, 19 Mar 2019 at 22:23, keerthy j-keerthy@ti.com wrote:

On 3/19/2019 6:53 AM, Simon Glass wrote:

...

Hi Keerthy,

On Mon, 11 Mar 2019 at 14:13, Keerthy j-keerthy@ti.com wrote:

...

Currently single instance temperature read out is supported. Enhance the same to support multiple instances.

Signed-off-by: Keerthy j-keerthy@ti.com

arch/arm/mach-imx/cpu.c | 2 +- drivers/mmc/omap_hsmmc.c | 2 +- drivers/thermal/imx_thermal.c | 2 +- drivers/thermal/thermal-uclass.c | 4 ++-- drivers/thermal/ti-bandgap.c | 13 ++++++++++--- include/thermal.h | 6 ++++-- 6 files changed, 19 insertions(+), 10 deletions(-)

Please can you add a simple test for the thermal uclass in test/dm?

Okay. I will try to add that.

...

...

diff --git a/arch/arm/mach-imx/cpu.c b/arch/arm/mach-imx/cpu.c index 6b83f92662..27d12b410e 100644 --- a/arch/arm/mach-imx/cpu.c +++ b/arch/arm/mach-imx/cpu.c @@ -231,7 +231,7 @@ int print_cpuinfo(void) printf("(%dC to %dC)", minc, maxc); ret = uclass_get_device(UCLASS_THERMAL, 0, &thermal_dev); if (!ret) {

•           ret = thermal_get_temp(thermal_dev, &cpu_tmp);
  

•           ret = thermal_get_temp(thermal_dev, 0, &cpu_tmp);
  
             if (!ret)
                     printf(" at %dC\n", cpu_tmp);
  

diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c index 826a39fad7..1872273dd9 100644 --- a/drivers/mmc/omap_hsmmc.c +++ b/drivers/mmc/omap_hsmmc.c @@ -642,7 +642,7 @@ static int omap_hsmmc_execute_tuning(struct udevice *dev, uint opcode) printf("Couldn't get thermal device for tuning\n"); return ret; }

•   ret = thermal_get_temp(thermal_dev, &temperature);
  

•   ret = thermal_get_temp(thermal_dev, 0, &temperature);
     if (ret) {
             printf("Couldn't get temperature for tuning\n");
             return ret;
  

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c index e50b85bd59..271a61356c 100644 --- a/drivers/thermal/imx_thermal.c +++ b/drivers/thermal/imx_thermal.c @@ -207,7 +207,7 @@ static int read_cpu_temperature(struct udevice *dev) } #endif

-int imx_thermal_get_temp(struct udevice *dev, int *temp) +int imx_thermal_get_temp(struct udevice *dev, int instance, int *temp) { struct thermal_data *priv = dev_get_priv(dev); int cpu_tmp = 0; diff --git a/drivers/thermal/thermal-uclass.c b/drivers/thermal/thermal-uclass.c index a4ea1e2914..b4a31280cb 100644 --- a/drivers/thermal/thermal-uclass.c +++ b/drivers/thermal/thermal-uclass.c @@ -13,14 +13,14 @@ #include <linux/list.h>

-int thermal_get_temp(struct udevice *dev, int *temp) +int thermal_get_temp(struct udevice *dev, u8 instance, int *temp) { const struct dm_thermal_ops *ops = device_get_ops(dev);

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

•   return ops->get_temp(dev, temp);
  

•   return ops->get_temp(dev, instance, temp);
  

  }

  UCLASS_DRIVER(thermal) = {

diff --git a/drivers/thermal/ti-bandgap.c b/drivers/thermal/ti-bandgap.c index b490391e96..6d3606b0fc 100644 --- a/drivers/thermal/ti-bandgap.c +++ b/drivers/thermal/ti-bandgap.c @@ -27,6 +27,7 @@ struct ti_bandgap { ulong base; int temperature; /* in mili degree celsius */

•   int                     sens_cnt;
  

  };

  /*

@@ -158,11 +159,16 @@ static int dra752_adc_to_temp[] = { 123800, 124200, 124600, 124900, 125000, 125000, };

-static int ti_bandgap_get_temp(struct udevice *dev, int *temp) +static int ti_bandgap_get_temp(struct udevice *dev, u8 instance, int *temp) { struct ti_bandgap *bgp = dev_get_priv(dev);

•   bgp->temperature = 0x3ff & readl(bgp->base + CTRL_CORE_TEMP_SENSOR_MPU);
  

•   if (instance >= bgp->sens_cnt) {
  

•           printf("Only %d insatnces are supported\n", bgp->sens_cnt);
  

•           return -EINVAL;
  

•   }
  

•   bgp->temperature = 0x3ff & readl(bgp->base + instance * 4);
     *temp = dra752_adc_to_temp[bgp->temperature - DRA752_ADC_START_VALUE];
  
     return 0;
  

@@ -177,13 +183,14 @@ static int ti_bandgap_probe(struct udevice *dev) struct ti_bandgap *bgp = dev_get_priv(dev);

     bgp->base = devfdt_get_addr_index(dev, 1);

•   bgp->sens_cnt = dev_get_driver_data(dev);
  
     return 0;
  

  }

  static const struct udevice_id of_ti_bandgap_match[] = { {

•           .compatible = "ti,dra752-bandgap",
  

•           .compatible = "ti,dra752-bandgap", .data = 3,
     },
     {},
  

  };

diff --git a/include/thermal.h b/include/thermal.h index 11d75256e0..82d80f3ded 100644 --- a/include/thermal.h +++ b/include/thermal.h @@ -9,7 +9,7 @@

#include <dm.h>

-int thermal_get_temp(struct udevice *dev, int *temp); +int thermal_get_temp(struct udevice *dev, u8 instance, int *temp);

Please add function comment.

Okay.

...

...

/**

• struct dm_thermal_ops - Driver model Thermal operations

@@ -25,9 +25,11 @@ struct dm_thermal_ops { * It will enable and initialize any Thermal hardware as necessary. * * @dev: The Thermal device

•    * @instance:   The instance of sensor in case multiple sensors are
  

•    *              present if single sensor then this should be 0
      * @temp:       pointer that returns the measured temperature
      */
  

•   int (*get_temp)(struct udevice *dev, int *temp);
  

•   int (*get_temp)(struct udevice *dev, u8 instance, int *temp);
  

I don't like the name 'instance' very much. With pwm we use 'channel'. Would that be OK?

How about 'sensors'? Basically multiple sensors give out temperatures of different thermal zones.

Hmm I still prefer channel to fit in with PWM. But if you really want to use 'sensor' (singular, not plural) then OK.

Regards, Simon

Hi Keerthy,

On Mon, 11 Mar 2019 at 14:13, Keerthy j-keerthy@ti.com wrote:

Add command line interface to read out temperatures from SoC. Takes two arguments. One is 'temperature' and the other is instance number. In case instance number is not provided by default 0th instance temperature is read out.

Signed-off-by: Keerthy j-keerthy@ti.com

cmd/Kconfig | 5 +++++ cmd/Makefile | 1 + cmd/thermal.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+) create mode 100644 cmd/thermal.c

diff --git a/cmd/Kconfig b/cmd/Kconfig index 4bcc5c4557..d252e93d64 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1044,6 +1044,11 @@ config CMD_SPI help SPI utility command.

+config CMD_THERMAL

•   bool "thermal"
  

•   help
  

•     THERMAL support.
  

Thermal support

config CMD_TSI148 bool "tsi148 - Command to access tsi148 device" help diff --git a/cmd/Makefile b/cmd/Makefile index acb85f49fb..2b66f9e36a 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -128,6 +128,7 @@ obj-$(CONFIG_CMD_SPI) += spi.o obj-$(CONFIG_CMD_STRINGS) += strings.o obj-$(CONFIG_CMD_SMC) += smccc.o obj-$(CONFIG_CMD_TERMINAL) += terminal.o +obj-$(CONFIG_CMD_THERMAL) += thermal.o obj-$(CONFIG_CMD_TIME) += time.o obj-$(CONFIG_CMD_TRACE) += trace.o obj-$(CONFIG_HUSH_PARSER) += test.o diff --git a/cmd/thermal.c b/cmd/thermal.c new file mode 100644 index 0000000000..a31f992425 --- /dev/null +++ b/cmd/thermal.c @@ -0,0 +1,59 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Thermal CMD
• • Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
• */

+#include <common.h> +#include <errno.h> +#include <dm.h> +#include <dm/uclass-internal.h>

Should go at the end

+#include <thermal.h>

+#define MAX_TEMP 150

Is this in celsius?

+static int do_read_temp(cmd_tbl_t *cmdtp, int flag, int argc,

•                   char * const argv[])
  

+{

•   struct udevice *tdev;
  

•   int cpu_temp, ret = 0;
  

•   u8 instance;
  

uint

Please use channel instead

•   const char *units;
  

•   if (argc > 2) {
  

•           printf("Max Number of arguments is 2\n");
  

•           return -EINVAL;
  

return CMD_RET_USAGE

If you return -ve values it can do strange things like cause the calling shell to exit.

•   }
  

•   /* In case instance is not given default 0th instnace is reported */
  

instance

•   if (argc == 2)
  

•           instance = (u8)simple_strtoul(argv[1], NULL, 10);
  

Drop u8 cast

•   else
  

•           instance = 0;
  

•   ret = uclass_get_device(UCLASS_THERMAL, 0, &tdev);
  

uclass_first_device_err()?

That is more robust than requiring it be seq #0.

•   if (!ret) {
  

•           ret = thermal_get_temp(tdev, instance, &cpu_temp);
  

•           if (!ret) {
  

•                   if (abs(cpu_temp) < MAX_TEMP)
  

•                           units = "C";
  

•                   else
  

•                           units = "mC";
  

The interface should be in mC, then. We can't have the range of the value determine the units.

•                   printf("Instance %d Temperature at %d%s\n", instance,
  

•                          cpu_temp, units);
  

•           } else {
  

•                   debug(" - invalid sensor data\n");
  

Maybe -ENOENT means invalid channel -ENODATA means the channel exists but temperature could not be read?

•           }
  

•   } else {
  

•           printf(" - invalid sensor device\n");
  

•   }
  

if (ret) return CMD_RET_FAILURE;

return 0

•   return ret;
  

Don't return -ve values from commands.

+}

+U_BOOT_CMD(

•   temperature, 2, 0, do_read_temp,
  

•   "Reads temperature of a given sensor",
  

•   " [sensor number] - number of the temperature sensor"
  

+);

2.17.1

Could we have a pytest for this command for sandbox?

Regards, Simon