RE: [PATCH RESEND 2/4] thermal: imx8mm: Add support for i.MX8MM thermal monitoring unit

[Anson Huang <anson.huang@nxp.com>]

Hi, Daniel

> Subject: Re: [PATCH RESEND 2/4] thermal: imx8mm: Add support for
> i.MX8MM thermal monitoring unit
> 
> On 20/02/2020 09:17, Anson Huang wrote:
> > i.MX8MM has a thermal monitoring unit(TMU) inside, it ONLY has one
> > sensor for CPU, add support for reading immediate temperature of this
> > sensor.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > This patch is base on patch series:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml
> > .org%2Flkml%2F2020%2F2%2F19%2F1258&amp;data=02%7C01%7CAnson.
> Huang%40nx
> >
> p.com%7Ca98c5f5da20240aaf3a708d7bb7542b8%7C686ea1d3bc2b4c6fa92c
> d99c5c3
> >
> 01635%7C0%7C0%7C637183984613271294&amp;sdata=lJ4Yzp57kJg2fOIusb2
> 9e%2FB
> > 2P6Cpq6iSVQqS571IDOk%3D&amp;reserved=0
> > ---
> >  drivers/thermal/Kconfig          |  10 +++
> >  drivers/thermal/Makefile         |   1 +
> >  drivers/thermal/imx8mm_thermal.c | 134
> > +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 145 insertions(+)
> >  create mode 100644 drivers/thermal/imx8mm_thermal.c
> >
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index
> > d1cb8dc..972b169 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -262,6 +262,16 @@ config IMX_SC_THERMAL
> >  	  sensor. It supports one critical trip point and one
> >  	  passive trip point for each thermal sensor.
> >
> > +config IMX8MM_THERMAL
> > +	tristate "Temperature sensor driver for Freescale i.MX8MM SoC"
> > +	depends on ARCH_MXC
> > +	depends on OF
> > +	help
> > +	  Support for Thermal Monitoring Unit (TMU) found on Freescale
> i.MX8MM SoC.
> > +	  It supports one critical trip point and one passive trip point. The
> > +	  cpufreq is used as the cooling device to throttle CPUs when the
> passive
> > +	  trip is crossed.
> > +
> >  config MAX77620_THERMAL
> >  	tristate "Temperature sensor driver for Maxim MAX77620 PMIC"
> >  	depends on MFD_MAX77620
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile index
> > a11a6d8..120a05e 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -44,6 +44,7 @@ obj-$(CONFIG_ARMADA_THERMAL)	+=
> armada_thermal.o
> >  obj-$(CONFIG_TANGO_THERMAL)	+= tango_thermal.o
> >  obj-$(CONFIG_IMX_THERMAL)	+= imx_thermal.o
> >  obj-$(CONFIG_IMX_SC_THERMAL)	+= imx_sc_thermal.o
> > +obj-$(CONFIG_IMX8MM_THERMAL)	+= imx8mm_thermal.o
> >  obj-$(CONFIG_MAX77620_THERMAL)	+= max77620_thermal.o
> >  obj-$(CONFIG_QORIQ_THERMAL)	+= qoriq_thermal.o
> >  obj-$(CONFIG_DA9062_THERMAL)	+= da9062-thermal.o
> > diff --git a/drivers/thermal/imx8mm_thermal.c
> > b/drivers/thermal/imx8mm_thermal.c
> > new file mode 100644
> > index 0000000..04f8a8f
> > --- /dev/null
> > +++ b/drivers/thermal/imx8mm_thermal.c
> > @@ -0,0 +1,134 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2019 NXP.
> 
> Copyright (c) 2020, NXP Semiconductors
> 
> Authors: Anson Huang <Anson.Huang@nxp.com>

OK, I will update the copyright year and add the author.


> > + *
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/thermal.h>
> > +
> > +#include "thermal_core.h"
> > +
> > +#define TER			0x0	/* TMU enable */
> > +#define TRITSR			0x20	/* TMU immediate temp */
> > +
> > +#define TER_EN			BIT(31)
> > +#define TRITSR_VAL_MASK		0xff
> > +
> > +#define TEMP_LOW_LIMIT		10
> > +
> > +struct imx8mm_tmu {
> > +	struct thermal_zone_device *tzd;
> > +	void __iomem *base;
> > +	struct clk *clk;
> > +};
> > +
> > +static int tmu_get_temp(void *data, int *temp) {
> > +	struct imx8mm_tmu *tmu = data;
> > +	u32 val;
> > +
> > +	/* the temp sensor need about 1ms to finish the measurement */
> > +	usleep_range(1000, 2000);
> 
> Why do yo need to force a delay here? If the sensor can not be read more
> than one time every 1ms, then specify that in the DT switching the polling to
> the right value, no?

The polling time(2 seconds) is OK for this case, adding this sleep is to prevent user from reading
temperature from sysfs interface very frequently like less than 1ms, does it make sense? 


> 
> > +	val = readl_relaxed(tmu->base + TRITSR) & TRITSR_VAL_MASK;
> > +	if (val < TEMP_LOW_LIMIT)
> > +		return -EAGAIN;>
> > +	*temp = val * 1000;
> > +	return 0;
> > +}
> > +
> > +static struct thermal_zone_of_device_ops tmu_tz_ops = {
> > +	.get_temp = tmu_get_temp,
> > +};
> > +
> > +static int imx8mm_tmu_probe(struct platform_device *pdev) {
> > +	struct imx8mm_tmu *tmu;
> > +	u32 val;
> > +	int ret;
> > +
> > +	tmu = devm_kzalloc(&pdev->dev, sizeof(struct imx8mm_tmu),
> GFP_KERNEL);
> > +	if (!tmu)
> > +		return -ENOMEM;
> > +
> > +	tmu->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(tmu->base))
> > +		return PTR_ERR(tmu->base);
> > +
> > +	tmu->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(tmu->clk)) {
> > +		ret = PTR_ERR(tmu->clk);
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(&pdev->dev,
> > +				"failed to get tmu clock: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(tmu->clk);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to enable tmu clock: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	tmu->tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0,
> > +							tmu, &tmu_tz_ops);
> > +	if (IS_ERR(tmu->tzd)) {
> > +		dev_err(&pdev->dev,
> > +			"failed to register thermal zone sensor: %d\n", ret);
> > +		return PTR_ERR(tmu->tzd);
> > +	}
> > +
> > +	platform_set_drvdata(pdev, tmu);
> > +
> > +	/* enable the monitor */
> > +	val = readl_relaxed(tmu->base + TER);
> > +	val |= TER_EN;
> > +	writel_relaxed(val, tmu->base + TER);
> 
> A side note perhaps you can wrap:
> 
> static int imx8mm_start(struct imx8mm_tmu *tmu) {
> 	clk_prepare_enable()
> 	val = readl_relaxed(tmu->base + TER);
> 	val |= TER_EN;
> 	writel_relaxed(val, tmu->base + TER);
> }
> 
> and
> 
> static void imx8mm_stop(struct imx8mm_tmu *tmu) {
> 	val = readl_relaxed(tmu->base + TER);
> 	val &= ~TER_EN;
> 	writel_relaxed(val, tmu->base + TER);
> 	clk_disable_unprepare(tmu->clk);
> }
> 
> So the suspend/resume callbacks can call them directly if you decide to add
> them.
> 

Thanks for the suggestion, so far, the i.MX8MM thermal driver no need to have suspend/resume
callback, its clock/function will be automatically disabled after system enter suspend, if suspend/resume
callback is necessary later, I will wrap it.

Thanks,
Anson


> > +	return 0;
> > +}
> > +
> > +static int imx8mm_tmu_remove(struct platform_device *pdev) {
> > +	struct imx8mm_tmu *tmu = platform_get_drvdata(pdev);
> > +	u32 val;
> > +
> > +	/* disable TMU */
> > +	val = readl_relaxed(tmu->base + TER);
> > +	val &= ~TER_EN;
> > +	writel_relaxed(val, tmu->base + TER);
> > +
> > +	clk_disable_unprepare(tmu->clk);
> > +	platform_set_drvdata(pdev, NULL);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id imx8mm_tmu_table[] = {
> > +	{ .compatible = "fsl,imx8mm-tmu", },
> > +	{ },
> > +};
> > +
> > +static struct platform_driver imx8mm_tmu = {
> > +	.driver = {
> > +		.name	= "i.mx8mm_thermal",
> > +		.of_match_table = imx8mm_tmu_table,
> > +	},
> > +	.probe = imx8mm_tmu_probe,
> > +	.remove = imx8mm_tmu_remove,
> > +};
> > +module_platform_driver(imx8mm_tmu);
> > +
> > +MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
> > +MODULE_DESCRIPTION("i.MX8MM Thermal Monitor Unit driver");
> > +MODULE_LICENSE("GPL v2");
> >
> 
> 
> --
> 
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2F&amp;data=02%7C01%7CAnson.Huang%40nxp.com%7Ca98c5f
> 5da20240aaf3a708d7bb7542b8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C637183984613271294&amp;sdata=k6RmLpJLujsgfNDDSaBCA%2F
> gqUg7V%2Bex%2FvggZZeDTgOs%3D&amp;reserved=0> Linaro.org │ Open
> source software for ARM SoCs
> 
> Follow Linaro:
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> facebook.com%2Fpages%2FLinaro&amp;data=02%7C01%7CAnson.Huang%4
> 0nxp.com%7Ca98c5f5da20240aaf3a708d7bb7542b8%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C637183984613271294&amp;sdata=p1DDLK
> OLCE6wfVkVAk6F6dWoi9ectXvP0AfxWq1ixHw%3D&amp;reserved=0>
> Facebook |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitte
> r.com%2F%23!%2Flinaroorg&amp;data=02%7C01%7CAnson.Huang%40nxp.c
> om%7Ca98c5f5da20240aaf3a708d7bb7542b8%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C637183984613271294&amp;sdata=kdTPCBFzhu5J9
> R2ccrfEvgHNhMoiYyqhShXJZcpha2U%3D&amp;reserved=0> Twitter |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2Flinaro-
> blog%2F&amp;data=02%7C01%7CAnson.Huang%40nxp.com%7Ca98c5f5da2
> 0240aaf3a708d7bb7542b8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C637183984613271294&amp;sdata=vRqSYzQYTl1Wq8NZftukynnylaBazF
> LjijKlu%2B8CWiI%3D&amp;reserved=0> Blog