[2/3] thermal: sun50i: add thermal driver for h6

Commit Message

Yangtao Li May 12, 2019, 8:26 a.m. UTC

This patch adds the support for allwinner thermal sensor, within
allwinner SoC. It will register sensors for thermal framework
and use device tree to bind cooling device.

Based on driver code found here:
https://megous.com/git/linux and https://github.com/Allwinner-Homlet/H6-BSP4.9-linux

Signed-off-by: Yangtao Li <[e-mail]>
---
 MAINTAINERS                      |   7 +
 drivers/thermal/Kconfig          |  14 ++
 drivers/thermal/Makefile         |   1 +
 drivers/thermal/sun50i_thermal.c | 357 +++++++++++++++++++++++++++++++
 4 files changed, 379 insertions(+)
 create mode 100644 drivers/thermal/sun50i_thermal.c

Comments

Maxime Ripard May 12, 2019, 1:39 p.m. UTC | #1

Hi,

Thanks a lot for working on this!

On Sun, May 12, 2019 at 04:26:13AM -0400, Yangtao Li wrote:
> This patch adds the support for allwinner thermal sensor, within
> allwinner SoC. It will register sensors for thermal framework
> and use device tree to bind cooling device.
>
> Based on driver code found here:
> https://megous.com/git/linux and https://github.com/Allwinner-Homlet/H6-BSP4.9-linux

I wouldn't place the URL in the commit log. The commit log stays
forever in the linux history. Git repos and branches are going away
over time.

> Signed-off-by: Yangtao Li <[e-mail]>
> ---
>  MAINTAINERS                      |   7 +
>  drivers/thermal/Kconfig          |  14 ++
>  drivers/thermal/Makefile         |   1 +
>  drivers/thermal/sun50i_thermal.c | 357 +++++++++++++++++++++++++++++++

The long term goal is to support all the thermal sensors, not just the
H6. Since that controller was introduced with the sun8i family, it
makes more sense to use that prefix for the driver and the functions.

>  4 files changed, 379 insertions(+)
>  create mode 100644 drivers/thermal/sun50i_thermal.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3c65228e93c5..8da56582e72a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -674,6 +674,13 @@ L:	[e-mail]
>  S:	Maintained
>  F:	drivers/crypto/sunxi-ss/
>
> +ALLWINNER THERMAL DRIVER
> +M:	Yangtao Li <[e-mail]>
> +L:	[e-mail]
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/thermal/sun50i-thermal.txt
> +F:	drivers/thermal/sun50i_thermal.c
> +
>  ALLWINNER VPU DRIVER
>  M:	Maxime Ripard <[e-mail]>
>  M:	Paul Kocialkowski <[e-mail]>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 653aa27a25a4..2a8d1c98c6ca 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -252,6 +252,20 @@ config SPEAR_THERMAL
>  	  Enable this to plug the SPEAr thermal sensor driver into the Linux
>  	  thermal framework.
>
> +config SUN50I_THERMAL
> +	tristate "Allwinner sun50i thermal driver"
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	depends on NVMEM
> +	depends on OF
> +	depends on RESET_CONTROLLER
> +	help
> +	  Support for the sun50i thermal sensor driver into the Linux thermal
> +	  framework.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sun50i-thermal.
> +
>  config ROCKCHIP_THERMAL
>  	tristate "Rockchip thermal driver"
>  	depends on ARCH_ROCKCHIP || COMPILE_TEST
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 486d682be047..a09b30b90003 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -30,6 +30,7 @@ thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
>  # platform thermal drivers
>  obj-y				+= broadcom/
>  obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
> +obj-$(CONFIG_SUN50I_THERMAL)	+= sun50i_thermal.o
>  obj-$(CONFIG_ROCKCHIP_THERMAL)	+= rockchip_thermal.o
>  obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
>  obj-$(CONFIG_RCAR_GEN3_THERMAL)	+= rcar_gen3_thermal.o
> diff --git a/drivers/thermal/sun50i_thermal.c b/drivers/thermal/sun50i_thermal.c
> new file mode 100644
> index 000000000000..3bdb3677b3d4
> --- /dev/null
> +++ b/drivers/thermal/sun50i_thermal.c
> @@ -0,0 +1,357 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Thermal sensor driver for Allwinner SOC
> + * Copyright (C) 2019 Yangtao Li
> + *
> + * Based on the work of Icenowy Zheng <[e-mail]>
> + * Based on the work of Ondrej Jirman <[e-mail]>
> + * Based on the work of Josef Gajdusek <[e-mail]>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +#include <linux/thermal.h>
> +
> +#define MAX_SENSOR_NUM	4
> +
> +#define FT_TEMP_MASK				GENMASK(11, 0)
> +#define TEMP_CALIB_MASK				GENMASK(11, 0)
> +#define TEMP_TO_REG				672
> +#define CALIBRATE_DEFAULT			0x800
> +
> +#define SUN50I_THS_CTRL0			0x00
> +#define SUN50I_H6_THS_ENABLE			0x04
> +#define SUN50I_H6_THS_PC			0x08
> +#define SUN50I_H6_THS_MFC			0x30
> +#define SUN50I_H6_TEMP_CALIB			0xa0
> +#define SUN50I_H6_TEMP_DATA			0xc0
> +
> +#define SUN50I_THS_CTRL0_T_ACQ(x)		((GENMASK(15, 0) & (x)) << 16)
> +#define SUN50I_THS_FILTER_EN			BIT(2)
> +#define SUN50I_THS_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
> +#define SUN50I_H6_THS_PC_TEMP_PERIOD(x)		((GENMASK(19, 0) & (x)) << 12)
> +
> +/* millidegree celsius */
> +#define SUN50I_H6_FT_DEVIATION			7000
> +
> +struct tsens_device;
> +
> +struct tsensor {
> +	struct tsens_device		*tmdev;
> +	struct thermal_zone_device	*tzd;
> +	int				id;
> +};
> +
> +struct sun50i_thermal_chip {
> +	int	sensor_num;
> +	int	offset;
> +	int	scale;
> +	int	ft_deviation;
> +	int	temp_calib_base;
> +	int	temp_data_base;
> +	int	(*enable)(struct tsens_device *tmdev);
> +	int	(*disable)(struct tsens_device *tmdev);
> +};

I'm not super fond of having a lot of quirks that are not needed. If
we ever need those quirks when adding support for a new SoC, then
yeah, we should totally have some, but only when and if it's needed.

Otherwise, the driver is more complicated for no particular reason.

> +
> +struct tsens_device {

IIRC the acronym used by allwinner is THS, maybe we can just use that
as a prefix?

> +	const struct sun50i_thermal_chip	*chip;
> +	struct device				*dev;
> +	struct regmap				*regmap;
> +	struct reset_control			*reset;
> +	struct clk				*bus_clk;
> +	struct tsensor				sensor[MAX_SENSOR_NUM];
> +};
> +
> +/* Temp Unit: millidegree Celsius */
> +static int tsens_reg2temp(struct tsens_device *tmdev,
> +			      int reg)
> +{
> +	return (reg + tmdev->chip->offset) * tmdev->chip->scale;
> +}
> +
> +static int tsens_get_temp(void *data, int *temp)
> +{
> +	struct tsensor *s = data;
> +	struct tsens_device *tmdev = s->tmdev;
> +	int val;
> +
> +	regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> +		    0x4 * s->id, &val);
> +
> +	if (unlikely(val == 0))
> +		return -EBUSY;

I'm not sure why a val equals to 0 would be associated with EBUSY?

Also, it's not in a fast path, so you can drop the unlikely. Chances
are it's not that unlikely anyway.

> +	*temp = tsens_reg2temp(tmdev, val);
> +	if (tmdev->chip->ft_deviation)
> +		*temp += tmdev->chip->ft_deviation;
> +
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops tsens_ops = {
> +	.get_temp = tsens_get_temp,
> +};
> +
> +static const struct regmap_config config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};
> +
> +static int tsens_init(struct tsens_device *tmdev)
> +{
> +	struct device *dev = tmdev->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct resource *mem;
> +	void __iomem *base;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	tmdev->regmap = devm_regmap_init_mmio_clk(dev, "bus",
> +						  base,
> +						  &config);
> +	if (IS_ERR(tmdev->regmap))
> +		return PTR_ERR(tmdev->regmap);
> +
> +	tmdev->reset = devm_reset_control_get(dev, "bus");
> +	if (IS_ERR(tmdev->reset))
> +		return PTR_ERR(tmdev->reset);
> +
> +	tmdev->bus_clk = devm_clk_get(&pdev->dev, "bus");
> +	if (IS_ERR(tmdev->bus_clk))
> +		return PTR_ERR(tmdev->bus_clk);

You don't need to get that clock if regmap has it already.

> +	return 0;
> +}
> +
> +/*
> + * Even if the external calibration data stored in sid is not accessible,
> + * the THS hardware can still work, although the data won't be so accurate.
> + * The default value of calibration register is 0x800 for every sensor,
> + * and the calibration value is usually 0x7xx or 0x8xx, so they won't be
> + * away from the default value for a lot.
> + *
> + * So here we do not return error if the calibartion data is
> + * not available, except the probe needs deferring.
> + */
> +static int tsens_calibrate(struct tsens_device *tmdev)
> +{
> +	struct nvmem_cell *calcell;
> +	struct device *dev = tmdev->dev;
> +	u16 *caldata;
> +	size_t callen;
> +	int ft_temp;
> +	int i = 0;
> +
> +	calcell = devm_nvmem_cell_get(dev, "calib");
> +	if (IS_ERR(calcell)) {
> +		if (PTR_ERR(calcell) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		goto out;
> +	}
> +
> +	caldata = nvmem_cell_read(calcell, &callen);
> +	if (IS_ERR(caldata))
> +		goto out;
> +
> +	if (!caldata[0] || callen < 2 + 2 * tmdev->chip->sensor_num)
> +		goto out_free;

The first part of your or isn't obvious and should have a comment.

The second part shouldn't return 0 but an error

> +
> +	/*
> +	 * The calbration data on H6 is stored as temperature-value
> +	 * pair when being filled at factory test stage.
> +	 * The unit of stored FT temperature is 0.1 degreee celusis.
> +	 */
> +	ft_temp = caldata[0] & FT_TEMP_MASK;
> +
> +	for (; i < tmdev->chip->sensor_num; i++) {

Usually you would initialize i here, and not when declared.

> +		int reg = (int)caldata[i + 1];
> +		int sensor_temp = tsens_reg2temp(tmdev, reg);
> +		int delta, cdata, calib_offest;
> +
> +		/*
> +		 * To calculate the calibration value:
> +		 *
> +		 * X(in Celsius) = Ts - ft_temp
> +		 * delta = X * 10000 / TEMP_TO_REG
> +		 * cdata = CALIBRATE_DEFAULT - delta
> +		 *
> +		 * cdata: calibration value
> +		 */
> +		delta = (sensor_temp - ft_temp * 100) * 10 / TEMP_TO_REG;
> +		cdata = CALIBRATE_DEFAULT - delta;
> +		if (cdata & ~TEMP_CALIB_MASK) {
> +			dev_warn(dev, "sensor%d calibration value error", i);
> +
> +			continue;
> +		}
> +
> +		calib_offest = tmdev->chip->temp_calib_base + (i / 2) * 0x4;
> +
> +		if (i % 2) {
> +			int val;
> +
> +			regmap_read(tmdev->regmap, calib_offest, &val);
> +			val = (val & TEMP_CALIB_MASK) | (cdata << 16);
> +			regmap_write(tmdev->regmap, calib_offest, val);
> +		} else
> +			regmap_write(tmdev->regmap, calib_offest, cdata);

This should have brackets as well

> +	}
> +
> +out_free:
> +	kfree(caldata);
> +out:
> +	return 0;
> +}
> +
> +static int tsens_register(struct tsens_device *tmdev)
> +{
> +	struct thermal_zone_device *tzd;
> +	int i = 0;
> +
> +	for (; i < tmdev->chip->sensor_num; i++) {

Ditto

> +		tmdev->sensor[i].tmdev = tmdev;
> +		tmdev->sensor[i].id = i;
> +		tmdev->sensor[i].tzd = devm_thermal_zone_of_sensor_register(
> +					tmdev->dev, i, &tmdev->sensor[i],
> +					&tsens_ops);
> +		if (IS_ERR(tmdev->sensor[i].tzd))
> +			return PTR_ERR(tzd);
> +	}
> +
> +	return 0;
> +}
> +
> +static int tsens_probe(struct platform_device *pdev)
> +{
> +	struct tsens_device *tmdev;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> +	if (!tmdev)
> +		return -ENOMEM;
> +
> +	tmdev->dev = dev;
> +	tmdev->chip = of_device_get_match_data(&pdev->dev);
> +	if (!tmdev->chip)
> +		return -EINVAL;
> +
> +	ret = tsens_init(tmdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = tsens_register(tmdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = tmdev->chip->enable(tmdev);
> +	if (ret)
> +		return ret;
>
> +	platform_set_drvdata(pdev, tmdev);

Your registration should be the very last thing you do. Otherwise, you
have a small window where the get_temp callback can be called, but the
driver will not be functional yet.

> +	return ret;
> +}
> +
> +static int tsens_remove(struct platform_device *pdev)
> +{
> +	struct tsens_device *tmdev = platform_get_drvdata(pdev);
> +
> +	tmdev->chip->disable(tmdev);
> +
> +	return 0;
> +}
> +
> +static int sun50i_thermal_enable(struct tsens_device *tmdev)
> +{
> +	int ret, val;
> +
> +	ret = reset_control_deassert(tmdev->reset);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(tmdev->bus_clk);
> +	if (ret)
> +		goto assert_reset;

This is done by regmap as well

> +	ret = tsens_calibrate(tmdev);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * clkin = 24MHz
> +	 * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1)
> +	 *           = 20us
> +	 */
> +	regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
> +		     SUN50I_THS_CTRL0_T_ACQ(479));
> +	/* average over 4 samples */
> +	regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
> +		     SUN50I_THS_FILTER_EN |
> +		     SUN50I_THS_FILTER_TYPE(1));
> +	/* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */
> +	regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
> +		     SUN50I_H6_THS_PC_TEMP_PERIOD(58));
> +	/* enable sensor */
> +	val = GENMASK(tmdev->chip->sensor_num - 1, 0);
> +	regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val);
> +
> +	return 0;
> +
> +assert_reset:
> +	reset_control_assert(tmdev->reset);
> +
> +	return ret;

Can't we do that with runtime_pm?

Thanks!
Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Ondřej Jirman May 12, 2019, 9:41 p.m. UTC | #2

Hello Maxime,

On Sun, May 12, 2019 at 03:39:30PM +0200, Maxime Ripard wrote:
> Hi,
> 
> Thanks a lot for working on this!
> 
> On Sun, May 12, 2019 at 04:26:13AM -0400, Yangtao Li wrote:
> > This patch adds the support for allwinner thermal sensor, within
> > allwinner SoC. It will register sensors for thermal framework
> > and use device tree to bind cooling device.
> >
> > Based on driver code found here:
> > https://megous.com/git/linux and https://github.com/Allwinner-Homlet/H6-BSP4.9-linux
> 
> I wouldn't place the URL in the commit log. The commit log stays
> forever in the linux history. Git repos and branches are going away
> over time.
> 
> > Signed-off-by: Yangtao Li <[e-mail]>
> > ---
> >  MAINTAINERS                      |   7 +
> >  drivers/thermal/Kconfig          |  14 ++
> >  drivers/thermal/Makefile         |   1 +
> >  drivers/thermal/sun50i_thermal.c | 357 +++++++++++++++++++++++++++++++
> 
> The long term goal is to support all the thermal sensors, not just the
> H6. Since that controller was introduced with the sun8i family, it
> makes more sense to use that prefix for the driver and the functions.
> 
> >  4 files changed, 379 insertions(+)
> >  create mode 100644 drivers/thermal/sun50i_thermal.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 3c65228e93c5..8da56582e72a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -674,6 +674,13 @@ L:	[e-mail]
> >  S:	Maintained
> >  F:	drivers/crypto/sunxi-ss/
> >
> > +ALLWINNER THERMAL DRIVER
> > +M:	Yangtao Li <[e-mail]>
> > +L:	[e-mail]
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/thermal/sun50i-thermal.txt
> > +F:	drivers/thermal/sun50i_thermal.c
> > +
> >  ALLWINNER VPU DRIVER
> >  M:	Maxime Ripard <[e-mail]>
> >  M:	Paul Kocialkowski <[e-mail]>
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > index 653aa27a25a4..2a8d1c98c6ca 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -252,6 +252,20 @@ config SPEAR_THERMAL
> >  	  Enable this to plug the SPEAr thermal sensor driver into the Linux
> >  	  thermal framework.
> >
> > +config SUN50I_THERMAL
> > +	tristate "Allwinner sun50i thermal driver"
> > +	depends on ARCH_SUNXI || COMPILE_TEST
> > +	depends on HAS_IOMEM
> > +	depends on NVMEM
> > +	depends on OF
> > +	depends on RESET_CONTROLLER
> > +	help
> > +	  Support for the sun50i thermal sensor driver into the Linux thermal
> > +	  framework.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called sun50i-thermal.
> > +
> >  config ROCKCHIP_THERMAL
> >  	tristate "Rockchip thermal driver"
> >  	depends on ARCH_ROCKCHIP || COMPILE_TEST
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> > index 486d682be047..a09b30b90003 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -30,6 +30,7 @@ thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
> >  # platform thermal drivers
> >  obj-y				+= broadcom/
> >  obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
> > +obj-$(CONFIG_SUN50I_THERMAL)	+= sun50i_thermal.o
> >  obj-$(CONFIG_ROCKCHIP_THERMAL)	+= rockchip_thermal.o
> >  obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
> >  obj-$(CONFIG_RCAR_GEN3_THERMAL)	+= rcar_gen3_thermal.o
> > diff --git a/drivers/thermal/sun50i_thermal.c b/drivers/thermal/sun50i_thermal.c
> > new file mode 100644
> > index 000000000000..3bdb3677b3d4
> > --- /dev/null
> > +++ b/drivers/thermal/sun50i_thermal.c
> > @@ -0,0 +1,357 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Thermal sensor driver for Allwinner SOC
> > + * Copyright (C) 2019 Yangtao Li
> > + *
> > + * Based on the work of Icenowy Zheng <[e-mail]>
> > + * Based on the work of Ondrej Jirman <[e-mail]>
> > + * Based on the work of Josef Gajdusek <[e-mail]>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset.h>
> > +#include <linux/slab.h>
> > +#include <linux/thermal.h>
> > +
> > +#define MAX_SENSOR_NUM	4
> > +
> > +#define FT_TEMP_MASK				GENMASK(11, 0)
> > +#define TEMP_CALIB_MASK				GENMASK(11, 0)
> > +#define TEMP_TO_REG				672
> > +#define CALIBRATE_DEFAULT			0x800
> > +
> > +#define SUN50I_THS_CTRL0			0x00
> > +#define SUN50I_H6_THS_ENABLE			0x04
> > +#define SUN50I_H6_THS_PC			0x08
> > +#define SUN50I_H6_THS_MFC			0x30
> > +#define SUN50I_H6_TEMP_CALIB			0xa0
> > +#define SUN50I_H6_TEMP_DATA			0xc0
> > +
> > +#define SUN50I_THS_CTRL0_T_ACQ(x)		((GENMASK(15, 0) & (x)) << 16)
> > +#define SUN50I_THS_FILTER_EN			BIT(2)
> > +#define SUN50I_THS_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
> > +#define SUN50I_H6_THS_PC_TEMP_PERIOD(x)		((GENMASK(19, 0) & (x)) << 12)
> > +
> > +/* millidegree celsius */
> > +#define SUN50I_H6_FT_DEVIATION			7000
> > +
> > +struct tsens_device;
> > +
> > +struct tsensor {
> > +	struct tsens_device		*tmdev;
> > +	struct thermal_zone_device	*tzd;
> > +	int				id;
> > +};
> > +
> > +struct sun50i_thermal_chip {
> > +	int	sensor_num;
> > +	int	offset;
> > +	int	scale;
> > +	int	ft_deviation;
> > +	int	temp_calib_base;
> > +	int	temp_data_base;
> > +	int	(*enable)(struct tsens_device *tmdev);
> > +	int	(*disable)(struct tsens_device *tmdev);
> > +};
> 
> I'm not super fond of having a lot of quirks that are not needed. If
> we ever need those quirks when adding support for a new SoC, then
> yeah, we should totally have some, but only when and if it's needed.
> 
> Otherwise, the driver is more complicated for no particular reason.
> 
> > +
> > +struct tsens_device {
> 
> IIRC the acronym used by allwinner is THS, maybe we can just use that
> as a prefix?
> 
> > +	const struct sun50i_thermal_chip	*chip;
> > +	struct device				*dev;
> > +	struct regmap				*regmap;
> > +	struct reset_control			*reset;
> > +	struct clk				*bus_clk;
> > +	struct tsensor				sensor[MAX_SENSOR_NUM];
> > +};
> > +
> > +/* Temp Unit: millidegree Celsius */
> > +static int tsens_reg2temp(struct tsens_device *tmdev,
> > +			      int reg)
> > +{
> > +	return (reg + tmdev->chip->offset) * tmdev->chip->scale;
> > +}
> > +
> > +static int tsens_get_temp(void *data, int *temp)
> > +{
> > +	struct tsensor *s = data;
> > +	struct tsens_device *tmdev = s->tmdev;
> > +	int val;
> > +
> > +	regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> > +		    0x4 * s->id, &val);
> > +
> > +	if (unlikely(val == 0))
> > +		return -EBUSY;
> 
> I'm not sure why a val equals to 0 would be associated with EBUSY?

Thermal zone driver can (will) call get_temp before we got the
first interrupt and the thermal data. In that case val will be 0.

Resulting in:

 (val + offset) * scale = (-2794) * -67 = 187198

187°C and immediate shutdown during boot - based on cirtical
temperature being reached.

Busy here means, get_temp does not yet have data. Thermal zone
driver just reports any error to dmesg output.

	o.

> Also, it's not in a fast path, so you can drop the unlikely. Chances
> are it's not that unlikely anyway.
> 
> > +	*temp = tsens_reg2temp(tmdev, val);
> > +	if (tmdev->chip->ft_deviation)
> > +		*temp += tmdev->chip->ft_deviation;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct thermal_zone_of_device_ops tsens_ops = {
> > +	.get_temp = tsens_get_temp,
> > +};
> > +
> > +static const struct regmap_config config = {
> > +	.reg_bits = 32,
> > +	.val_bits = 32,
> > +	.reg_stride = 4,
> > +	.fast_io = true,
> > +};
> > +
> > +static int tsens_init(struct tsens_device *tmdev)
> > +{
> > +	struct device *dev = tmdev->dev;
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct resource *mem;
> > +	void __iomem *base;
> > +
> > +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	base = devm_ioremap_resource(dev, mem);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> > +
> > +	tmdev->regmap = devm_regmap_init_mmio_clk(dev, "bus",
> > +						  base,
> > +						  &config);
> > +	if (IS_ERR(tmdev->regmap))
> > +		return PTR_ERR(tmdev->regmap);
> > +
> > +	tmdev->reset = devm_reset_control_get(dev, "bus");
> > +	if (IS_ERR(tmdev->reset))
> > +		return PTR_ERR(tmdev->reset);
> > +
> > +	tmdev->bus_clk = devm_clk_get(&pdev->dev, "bus");
> > +	if (IS_ERR(tmdev->bus_clk))
> > +		return PTR_ERR(tmdev->bus_clk);
> 
> You don't need to get that clock if regmap has it already.
> 
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Even if the external calibration data stored in sid is not accessible,
> > + * the THS hardware can still work, although the data won't be so accurate.
> > + * The default value of calibration register is 0x800 for every sensor,
> > + * and the calibration value is usually 0x7xx or 0x8xx, so they won't be
> > + * away from the default value for a lot.
> > + *
> > + * So here we do not return error if the calibartion data is
> > + * not available, except the probe needs deferring.
> > + */
> > +static int tsens_calibrate(struct tsens_device *tmdev)
> > +{
> > +	struct nvmem_cell *calcell;
> > +	struct device *dev = tmdev->dev;
> > +	u16 *caldata;
> > +	size_t callen;
> > +	int ft_temp;
> > +	int i = 0;
> > +
> > +	calcell = devm_nvmem_cell_get(dev, "calib");
> > +	if (IS_ERR(calcell)) {
> > +		if (PTR_ERR(calcell) == -EPROBE_DEFER)
> > +			return -EPROBE_DEFER;
> > +
> > +		goto out;
> > +	}
> > +
> > +	caldata = nvmem_cell_read(calcell, &callen);
> > +	if (IS_ERR(caldata))
> > +		goto out;
> > +
> > +	if (!caldata[0] || callen < 2 + 2 * tmdev->chip->sensor_num)
> > +		goto out_free;
> 
> The first part of your or isn't obvious and should have a comment.
> 
> The second part shouldn't return 0 but an error
> 
> > +
> > +	/*
> > +	 * The calbration data on H6 is stored as temperature-value
> > +	 * pair when being filled at factory test stage.
> > +	 * The unit of stored FT temperature is 0.1 degreee celusis.
> > +	 */
> > +	ft_temp = caldata[0] & FT_TEMP_MASK;
> > +
> > +	for (; i < tmdev->chip->sensor_num; i++) {
> 
> Usually you would initialize i here, and not when declared.
> 
> > +		int reg = (int)caldata[i + 1];
> > +		int sensor_temp = tsens_reg2temp(tmdev, reg);
> > +		int delta, cdata, calib_offest;
> > +
> > +		/*
> > +		 * To calculate the calibration value:
> > +		 *
> > +		 * X(in Celsius) = Ts - ft_temp
> > +		 * delta = X * 10000 / TEMP_TO_REG
> > +		 * cdata = CALIBRATE_DEFAULT - delta
> > +		 *
> > +		 * cdata: calibration value
> > +		 */
> > +		delta = (sensor_temp - ft_temp * 100) * 10 / TEMP_TO_REG;
> > +		cdata = CALIBRATE_DEFAULT - delta;
> > +		if (cdata & ~TEMP_CALIB_MASK) {
> > +			dev_warn(dev, "sensor%d calibration value error", i);
> > +
> > +			continue;
> > +		}
> > +
> > +		calib_offest = tmdev->chip->temp_calib_base + (i / 2) * 0x4;
> > +
> > +		if (i % 2) {
> > +			int val;
> > +
> > +			regmap_read(tmdev->regmap, calib_offest, &val);
> > +			val = (val & TEMP_CALIB_MASK) | (cdata << 16);
> > +			regmap_write(tmdev->regmap, calib_offest, val);
> > +		} else
> > +			regmap_write(tmdev->regmap, calib_offest, cdata);
> 
> This should have brackets as well
> 
> > +	}
> > +
> > +out_free:
> > +	kfree(caldata);
> > +out:
> > +	return 0;
> > +}
> > +
> > +static int tsens_register(struct tsens_device *tmdev)
> > +{
> > +	struct thermal_zone_device *tzd;
> > +	int i = 0;
> > +
> > +	for (; i < tmdev->chip->sensor_num; i++) {
> 
> Ditto
> 
> > +		tmdev->sensor[i].tmdev = tmdev;
> > +		tmdev->sensor[i].id = i;
> > +		tmdev->sensor[i].tzd = devm_thermal_zone_of_sensor_register(
> > +					tmdev->dev, i, &tmdev->sensor[i],
> > +					&tsens_ops);
> > +		if (IS_ERR(tmdev->sensor[i].tzd))
> > +			return PTR_ERR(tzd);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int tsens_probe(struct platform_device *pdev)
> > +{
> > +	struct tsens_device *tmdev;
> > +	struct device *dev = &pdev->dev;
> > +	int ret;
> > +
> > +	tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > +	if (!tmdev)
> > +		return -ENOMEM;
> > +
> > +	tmdev->dev = dev;
> > +	tmdev->chip = of_device_get_match_data(&pdev->dev);
> > +	if (!tmdev->chip)
> > +		return -EINVAL;
> > +
> > +	ret = tsens_init(tmdev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = tsens_register(tmdev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = tmdev->chip->enable(tmdev);
> > +	if (ret)
> > +		return ret;
> >
> > +	platform_set_drvdata(pdev, tmdev);
> 
> Your registration should be the very last thing you do. Otherwise, you
> have a small window where the get_temp callback can be called, but the
> driver will not be functional yet.
> 
> > +	return ret;
> > +}
> > +
> > +static int tsens_remove(struct platform_device *pdev)
> > +{
> > +	struct tsens_device *tmdev = platform_get_drvdata(pdev);
> > +
> > +	tmdev->chip->disable(tmdev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int sun50i_thermal_enable(struct tsens_device *tmdev)
> > +{
> > +	int ret, val;
> > +
> > +	ret = reset_control_deassert(tmdev->reset);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_prepare_enable(tmdev->bus_clk);
> > +	if (ret)
> > +		goto assert_reset;
> 
> This is done by regmap as well
> 
> > +	ret = tsens_calibrate(tmdev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * clkin = 24MHz
> > +	 * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1)
> > +	 *           = 20us
> > +	 */
> > +	regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
> > +		     SUN50I_THS_CTRL0_T_ACQ(479));
> > +	/* average over 4 samples */
> > +	regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
> > +		     SUN50I_THS_FILTER_EN |
> > +		     SUN50I_THS_FILTER_TYPE(1));
> > +	/* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */
> > +	regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
> > +		     SUN50I_H6_THS_PC_TEMP_PERIOD(58));
> > +	/* enable sensor */
> > +	val = GENMASK(tmdev->chip->sensor_num - 1, 0);
> > +	regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val);
> > +
> > +	return 0;
> > +
> > +assert_reset:
> > +	reset_control_assert(tmdev->reset);
> > +
> > +	return ret;
> 
> Can't we do that with runtime_pm?
> 
> Thanks!
> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ondřej Jirman May 12, 2019, 10:16 p.m. UTC | #3

Hello Yangtao,

On Sun, May 12, 2019 at 04:26:13AM -0400, Yangtao Li wrote:
> diff --git a/drivers/thermal/sun50i_thermal.c b/drivers/thermal/sun50i_thermal.c
> new file mode 100644
> index 000000000000..3bdb3677b3d4
> --- /dev/null
> +++ b/drivers/thermal/sun50i_thermal.c
> @@ -0,0 +1,357 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Thermal sensor driver for Allwinner SOC
> + * Copyright (C) 2019 Yangtao Li
> + *
> + * Based on the work of Icenowy Zheng <[e-mail]>
> + * Based on the work of Ondrej Jirman <[e-mail]>
> + * Based on the work of Josef Gajdusek <[e-mail]>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +#include <linux/thermal.h>
> +
> +#define MAX_SENSOR_NUM	4
> +
> +#define FT_TEMP_MASK				GENMASK(11, 0)
> +#define TEMP_CALIB_MASK				GENMASK(11, 0)
> +#define TEMP_TO_REG				672
> +#define CALIBRATE_DEFAULT			0x800
> +
> +#define SUN50I_THS_CTRL0			0x00
> +#define SUN50I_H6_THS_ENABLE			0x04
> +#define SUN50I_H6_THS_PC			0x08
> +#define SUN50I_H6_THS_MFC			0x30
> +#define SUN50I_H6_TEMP_CALIB			0xa0
> +#define SUN50I_H6_TEMP_DATA			0xc0
> +
> +#define SUN50I_THS_CTRL0_T_ACQ(x)		((GENMASK(15, 0) & (x)) << 16)
> +#define SUN50I_THS_FILTER_EN			BIT(2)
> +#define SUN50I_THS_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
> +#define SUN50I_H6_THS_PC_TEMP_PERIOD(x)		((GENMASK(19, 0) & (x)) << 12)
> +
> +/* millidegree celsius */
> +#define SUN50I_H6_FT_DEVIATION			7000
> +
> +struct tsens_device;
> +
> +struct tsensor {
> +	struct tsens_device		*tmdev;
> +	struct thermal_zone_device	*tzd;
> +	int				id;
> +};
> +
> +struct sun50i_thermal_chip {
> +	int	sensor_num;
> +	int	offset;
> +	int	scale;
> +	int	ft_deviation;
> +	int	temp_calib_base;
> +	int	temp_data_base;
> +	int	(*enable)(struct tsens_device *tmdev);
> +	int	(*disable)(struct tsens_device *tmdev);
> +};
> +
> +
> +struct tsens_device {
> +	const struct sun50i_thermal_chip	*chip;
> +	struct device				*dev;
> +	struct regmap				*regmap;
> +	struct reset_control			*reset;
> +	struct clk				*bus_clk;
> +	struct tsensor				sensor[MAX_SENSOR_NUM];
> +};
> +
> +/* Temp Unit: millidegree Celsius */
> +static int tsens_reg2temp(struct tsens_device *tmdev,
> +			      int reg)

Please name all functions so that they are more clearly identifiable
in stack traces as belonging to this driver. For example:

  sun8i_ths_reg2temp

The same applies for all tsens_* functions below. tsens_* is too
generic.

> +{
> +	return (reg + tmdev->chip->offset) * tmdev->chip->scale;
> +}
> +
> +static int tsens_get_temp(void *data, int *temp)
> +{
> +	struct tsensor *s = data;
> +	struct tsens_device *tmdev = s->tmdev;
> +	int val;
> +
> +	regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> +		    0x4 * s->id, &val);
> +
> +	if (unlikely(val == 0))
> +		return -EBUSY;
> +
> +	*temp = tsens_reg2temp(tmdev, val);
> +	if (tmdev->chip->ft_deviation)
> +		*temp += tmdev->chip->ft_deviation;
> +
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops tsens_ops = {
> +	.get_temp = tsens_get_temp,
> +};
> +
> +static const struct regmap_config config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};
> +
> +static int tsens_init(struct tsens_device *tmdev)
> +{
> +	struct device *dev = tmdev->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct resource *mem;
> +	void __iomem *base;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	tmdev->regmap = devm_regmap_init_mmio_clk(dev, "bus",
> +						  base,
> +						  &config);
> +	if (IS_ERR(tmdev->regmap))
> +		return PTR_ERR(tmdev->regmap);
> +
> +	tmdev->reset = devm_reset_control_get(dev, "bus");
> +	if (IS_ERR(tmdev->reset))
> +		return PTR_ERR(tmdev->reset);
> +
> +	tmdev->bus_clk = devm_clk_get(&pdev->dev, "bus");
> +	if (IS_ERR(tmdev->bus_clk))
> +		return PTR_ERR(tmdev->bus_clk);
> +
> +	return 0;
> +}
> +
> +/*
> + * Even if the external calibration data stored in sid is not accessible,
> + * the THS hardware can still work, although the data won't be so accurate.
> + * The default value of calibration register is 0x800 for every sensor,
> + * and the calibration value is usually 0x7xx or 0x8xx, so they won't be
> + * away from the default value for a lot.
> + *
> + * So here we do not return error if the calibartion data is
> + * not available, except the probe needs deferring.
> + */
> +static int tsens_calibrate(struct tsens_device *tmdev)
> +{
> +	struct nvmem_cell *calcell;
> +	struct device *dev = tmdev->dev;
> +	u16 *caldata;
> +	size_t callen;
> +	int ft_temp;
> +	int i = 0;
> +
> +	calcell = devm_nvmem_cell_get(dev, "calib");
> +	if (IS_ERR(calcell)) {
> +		if (PTR_ERR(calcell) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		goto out;
> +	}
> +
> +	caldata = nvmem_cell_read(calcell, &callen);
> +	if (IS_ERR(caldata))
> +		goto out;
> +
> +	if (!caldata[0] || callen < 2 + 2 * tmdev->chip->sensor_num)
> +		goto out_free;
> +
> +	/*
> +	 * The calbration data on H6 is stored as temperature-value
> +	 * pair when being filled at factory test stage.
> +	 * The unit of stored FT temperature is 0.1 degreee celusis.
> +	 */

Please describe the calibration data layout more clearly.

> +	ft_temp = caldata[0] & FT_TEMP_MASK;
> +
> +	for (; i < tmdev->chip->sensor_num; i++) {
> +		int reg = (int)caldata[i + 1];
> +		int sensor_temp = tsens_reg2temp(tmdev, reg);
> +		int delta, cdata, calib_offest;
> +
> +		/*
> +		 * To calculate the calibration value:
> +		 *
> +		 * X(in Celsius) = Ts - ft_temp
> +		 * delta = X * 10000 / TEMP_TO_REG
> +		 * cdata = CALIBRATE_DEFAULT - delta
> +		 *
> +		 * cdata: calibration value
> +		 */
> +		delta = (sensor_temp - ft_temp * 100) * 10 / TEMP_TO_REG;
> +		cdata = CALIBRATE_DEFAULT - delta;
> +		if (cdata & ~TEMP_CALIB_MASK) {
> +			dev_warn(dev, "sensor%d calibration value error", i);

Please use a more descriptive error message. What error is this?

> +			continue;
> +		}
> +
> +		calib_offest = tmdev->chip->temp_calib_base + (i / 2) * 0x4;
> +		if (i % 2) {
> +			int val;
> +
> +			regmap_read(tmdev->regmap, calib_offest, &val);
> +			val = (val & TEMP_CALIB_MASK) | (cdata << 16);
> +			regmap_write(tmdev->regmap, calib_offest, val);
> +		} else
> +			regmap_write(tmdev->regmap, calib_offest, cdata);
> +	}
> +
> +out_free:
> +	kfree(caldata);
> +out:
> +	return 0;
> +}
> +
> +static int tsens_register(struct tsens_device *tmdev)
> +{
> +	struct thermal_zone_device *tzd;
> +	int i = 0;
> +
> +	for (; i < tmdev->chip->sensor_num; i++) {
> +		tmdev->sensor[i].tmdev = tmdev;
> +		tmdev->sensor[i].id = i;
> +		tmdev->sensor[i].tzd = devm_thermal_zone_of_sensor_register(
> +					tmdev->dev, i, &tmdev->sensor[i],
> +					&tsens_ops);
> +		if (IS_ERR(tmdev->sensor[i].tzd))
> +			return PTR_ERR(tzd);
> +	}
> +
> +	return 0;
> +}
> +
> +static int tsens_probe(struct platform_device *pdev)
> +{
> +	struct tsens_device *tmdev;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> +	if (!tmdev)
> +		return -ENOMEM;
> +
> +	tmdev->dev = dev;
> +	tmdev->chip = of_device_get_match_data(&pdev->dev);
> +	if (!tmdev->chip)
> +		return -EINVAL;
> +
> +	ret = tsens_init(tmdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = tsens_register(tmdev);
> +	if (ret)
> +		return ret;

Why split this out of probe into separate functions?

> +	ret = tmdev->chip->enable(tmdev);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, tmdev);
> +
> +	return ret;
> +}
> +
> +static int tsens_remove(struct platform_device *pdev)
> +{
> +	struct tsens_device *tmdev = platform_get_drvdata(pdev);
> +
> +	tmdev->chip->disable(tmdev);
> +
> +	return 0;
> +}
> +
> +static int sun50i_thermal_enable(struct tsens_device *tmdev)
> +{
> +	int ret, val;
> +
> +	ret = reset_control_deassert(tmdev->reset);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(tmdev->bus_clk);
> +	if (ret)
> +		goto assert_reset;
> +
> +	ret = tsens_calibrate(tmdev);
> +	if (ret)
> +		return ret;

If this fails (it may likely fail with EPROBE_DEFER) you are leaving reset
deasserted, and clock enabled.

Overall, I think, reset/clock management and nvmem reading will be common
to all the HW variants, so it doesn't make much sense splitting it out
of probe into separate functions, and makes it more error prone.

thank you and regards,
	o.

> +	/*
> +	 * clkin = 24MHz
> +	 * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1)
> +	 *           = 20us
> +	 */
> +	regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
> +		     SUN50I_THS_CTRL0_T_ACQ(479));
> +	/* average over 4 samples */
> +	regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
> +		     SUN50I_THS_FILTER_EN |
> +		     SUN50I_THS_FILTER_TYPE(1));
> +	/* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */
> +	regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
> +		     SUN50I_H6_THS_PC_TEMP_PERIOD(58));
> +	/* enable sensor */
> +	val = GENMASK(tmdev->chip->sensor_num - 1, 0);
> +	regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val);
> +
> +	return 0;
> +
> +assert_reset:
> +	reset_control_assert(tmdev->reset);
> +
> +	return ret;
> +}
> +
> +static int sun50i_thermal_disable(struct tsens_device *tmdev)
> +{
> +	clk_disable_unprepare(tmdev->bus_clk);
> +	reset_control_assert(tmdev->reset);
> +
> +	return 0;
> +}
> +
> +static const struct sun50i_thermal_chip sun50i_h6_ths = {
> +	.sensor_num = 2,
> +	.offset = -2794,
> +	.scale = -67,
> +	.ft_deviation = SUN50I_H6_FT_DEVIATION,
> +	.temp_calib_base = SUN50I_H6_TEMP_CALIB,
> +	.temp_data_base = SUN50I_H6_TEMP_DATA,
> +	.enable = sun50i_thermal_enable,
> +	.disable = sun50i_thermal_disable,
> +};
> +
> +static const struct of_device_id of_tsens_match[] = {
> +	{ .compatible = "allwinner,sun50i-h6-ths", .data = &sun50i_h6_ths },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, of_tsens_match);
> +
> +static struct platform_driver tsens_driver = {
> +	.probe = tsens_probe,
> +	.remove = tsens_remove,
> +	.driver = {
> +		.name = "sun50i-thermal",
> +		.of_match_table = of_tsens_match,
> +	},
> +};
> +module_platform_driver(tsens_driver);
> +
> +MODULE_DESCRIPTION("Thermal sensor driver for Allwinner SOC");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ondřej Jirman May 12, 2019, 10:39 p.m. UTC | #4

On Sun, May 12, 2019 at 04:26:13AM -0400, Yangtao Li wrote:
> This patch adds the support for allwinner thermal sensor, within
> allwinner SoC. It will register sensors for thermal framework
> and use device tree to bind cooling device.
> 
> Based on driver code found here:
> https://megous.com/git/linux and https://github.com/Allwinner-Homlet/H6-BSP4.9-linux
> 
> Signed-off-by: Yangtao Li <[e-mail]>
> ---
>  MAINTAINERS                      |   7 +
>  drivers/thermal/Kconfig          |  14 ++
>  drivers/thermal/Makefile         |   1 +
>  drivers/thermal/sun50i_thermal.c | 357 +++++++++++++++++++++++++++++++
>  4 files changed, 379 insertions(+)
>  create mode 100644 drivers/thermal/sun50i_thermal.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3c65228e93c5..8da56582e72a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -674,6 +674,13 @@ L:	[e-mail]
>  S:	Maintained
>  F:	drivers/crypto/sunxi-ss/
>  
> +ALLWINNER THERMAL DRIVER
> +M:	Yangtao Li <[e-mail]>
> +L:	[e-mail]
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/thermal/sun50i-thermal.txt
> +F:	drivers/thermal/sun50i_thermal.c
> +
>  ALLWINNER VPU DRIVER
>  M:	Maxime Ripard <[e-mail]>
>  M:	Paul Kocialkowski <[e-mail]>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 653aa27a25a4..2a8d1c98c6ca 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -252,6 +252,20 @@ config SPEAR_THERMAL
>  	  Enable this to plug the SPEAr thermal sensor driver into the Linux
>  	  thermal framework.
>  
> +config SUN50I_THERMAL
> +	tristate "Allwinner sun50i thermal driver"
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	depends on NVMEM
> +	depends on OF
> +	depends on RESET_CONTROLLER
> +	help
> +	  Support for the sun50i thermal sensor driver into the Linux thermal
> +	  framework.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sun50i-thermal.
> +
>  config ROCKCHIP_THERMAL
>  	tristate "Rockchip thermal driver"
>  	depends on ARCH_ROCKCHIP || COMPILE_TEST
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 486d682be047..a09b30b90003 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -30,6 +30,7 @@ thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
>  # platform thermal drivers
>  obj-y				+= broadcom/
>  obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
> +obj-$(CONFIG_SUN50I_THERMAL)	+= sun50i_thermal.o
>  obj-$(CONFIG_ROCKCHIP_THERMAL)	+= rockchip_thermal.o
>  obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
>  obj-$(CONFIG_RCAR_GEN3_THERMAL)	+= rcar_gen3_thermal.o
> diff --git a/drivers/thermal/sun50i_thermal.c b/drivers/thermal/sun50i_thermal.c
> new file mode 100644
> index 000000000000..3bdb3677b3d4
> --- /dev/null
> +++ b/drivers/thermal/sun50i_thermal.c
> @@ -0,0 +1,357 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Thermal sensor driver for Allwinner SOC
> + * Copyright (C) 2019 Yangtao Li
> + *
> + * Based on the work of Icenowy Zheng <[e-mail]>
> + * Based on the work of Ondrej Jirman <[e-mail]>
> + * Based on the work of Josef Gajdusek <[e-mail]>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +#include <linux/thermal.h>
> +
> +#define MAX_SENSOR_NUM	4
> +
> +#define FT_TEMP_MASK				GENMASK(11, 0)
> +#define TEMP_CALIB_MASK				GENMASK(11, 0)
> +#define TEMP_TO_REG				672
> +#define CALIBRATE_DEFAULT			0x800
> +
> +#define SUN50I_THS_CTRL0			0x00
> +#define SUN50I_H6_THS_ENABLE			0x04
> +#define SUN50I_H6_THS_PC			0x08
> +#define SUN50I_H6_THS_MFC			0x30
> +#define SUN50I_H6_TEMP_CALIB			0xa0
> +#define SUN50I_H6_TEMP_DATA			0xc0
> +
> +#define SUN50I_THS_CTRL0_T_ACQ(x)		((GENMASK(15, 0) & (x)) << 16)
> +#define SUN50I_THS_FILTER_EN			BIT(2)
> +#define SUN50I_THS_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
> +#define SUN50I_H6_THS_PC_TEMP_PERIOD(x)		((GENMASK(19, 0) & (x)) << 12)
> +
> +/* millidegree celsius */
> +#define SUN50I_H6_FT_DEVIATION			7000
> +
> +struct tsens_device;
> +
> +struct tsensor {
> +	struct tsens_device		*tmdev;
> +	struct thermal_zone_device	*tzd;
> +	int				id;
> +};
> +
> +struct sun50i_thermal_chip {
> +	int	sensor_num;
> +	int	offset;
> +	int	scale;
> +	int	ft_deviation;
> +	int	temp_calib_base;
> +	int	temp_data_base;
> +	int	(*enable)(struct tsens_device *tmdev);
> +	int	(*disable)(struct tsens_device *tmdev);
> +};
> +
> +
> +struct tsens_device {
> +	const struct sun50i_thermal_chip	*chip;
> +	struct device				*dev;
> +	struct regmap				*regmap;
> +	struct reset_control			*reset;
> +	struct clk				*bus_clk;
> +	struct tsensor				sensor[MAX_SENSOR_NUM];
> +};
> +
> +/* Temp Unit: millidegree Celsius */
> +static int tsens_reg2temp(struct tsens_device *tmdev,
> +			      int reg)
> +{
> +	return (reg + tmdev->chip->offset) * tmdev->chip->scale;
> +}
> +
> +static int tsens_get_temp(void *data, int *temp)
> +{
> +	struct tsensor *s = data;
> +	struct tsens_device *tmdev = s->tmdev;
> +	int val;
> +
> +	regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> +		    0x4 * s->id, &val);
> +
> +	if (unlikely(val == 0))
> +		return -EBUSY;
> +
> +	*temp = tsens_reg2temp(tmdev, val);
> +	if (tmdev->chip->ft_deviation)
> +		*temp += tmdev->chip->ft_deviation;
> +
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops tsens_ops = {
> +	.get_temp = tsens_get_temp,
> +};
> +
> +static const struct regmap_config config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};
> +
> +static int tsens_init(struct tsens_device *tmdev)
> +{
> +	struct device *dev = tmdev->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct resource *mem;
> +	void __iomem *base;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	tmdev->regmap = devm_regmap_init_mmio_clk(dev, "bus",
> +						  base,
> +						  &config);
> +	if (IS_ERR(tmdev->regmap))
> +		return PTR_ERR(tmdev->regmap);
> +
> +	tmdev->reset = devm_reset_control_get(dev, "bus");
> +	if (IS_ERR(tmdev->reset))
> +		return PTR_ERR(tmdev->reset);
> +
> +	tmdev->bus_clk = devm_clk_get(&pdev->dev, "bus");
> +	if (IS_ERR(tmdev->bus_clk))
> +		return PTR_ERR(tmdev->bus_clk);
> +
> +	return 0;
> +}
> +
> +/*
> + * Even if the external calibration data stored in sid is not accessible,
> + * the THS hardware can still work, although the data won't be so accurate.
> + * The default value of calibration register is 0x800 for every sensor,
> + * and the calibration value is usually 0x7xx or 0x8xx, so they won't be
> + * away from the default value for a lot.
> + *
> + * So here we do not return error if the calibartion data is
> + * not available, except the probe needs deferring.
> + */
> +static int tsens_calibrate(struct tsens_device *tmdev)
> +{
> +	struct nvmem_cell *calcell;
> +	struct device *dev = tmdev->dev;
> +	u16 *caldata;
> +	size_t callen;
> +	int ft_temp;
> +	int i = 0;
> +
> +	calcell = devm_nvmem_cell_get(dev, "calib");
> +	if (IS_ERR(calcell)) {
> +		if (PTR_ERR(calcell) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		goto out;
> +	}
> +
> +	caldata = nvmem_cell_read(calcell, &callen);
> +	if (IS_ERR(caldata))
> +		goto out;
> +
> +	if (!caldata[0] || callen < 2 + 2 * tmdev->chip->sensor_num)
> +		goto out_free;
> +
> +	/*
> +	 * The calbration data on H6 is stored as temperature-value
> +	 * pair when being filled at factory test stage.
> +	 * The unit of stored FT temperature is 0.1 degreee celusis.
> +	 */
> +	ft_temp = caldata[0] & FT_TEMP_MASK;
> +
> +	for (; i < tmdev->chip->sensor_num; i++) {
> +		int reg = (int)caldata[i + 1];
> +		int sensor_temp = tsens_reg2temp(tmdev, reg);
> +		int delta, cdata, calib_offest;
> +
> +		/*
> +		 * To calculate the calibration value:
> +		 *
> +		 * X(in Celsius) = Ts - ft_temp
> +		 * delta = X * 10000 / TEMP_TO_REG
> +		 * cdata = CALIBRATE_DEFAULT - delta
> +		 *
> +		 * cdata: calibration value
> +		 */
> +		delta = (sensor_temp - ft_temp * 100) * 10 / TEMP_TO_REG;
> +		cdata = CALIBRATE_DEFAULT - delta;
> +		if (cdata & ~TEMP_CALIB_MASK) {
> +			dev_warn(dev, "sensor%d calibration value error", i);
> +
> +			continue;
> +		}
> +
> +		calib_offest = tmdev->chip->temp_calib_base + (i / 2) * 0x4;
> +
> +		if (i % 2) {
> +			int val;
> +
> +			regmap_read(tmdev->regmap, calib_offest, &val);
> +			val = (val & TEMP_CALIB_MASK) | (cdata << 16);
> +			regmap_write(tmdev->regmap, calib_offest, val);
> +		} else
> +			regmap_write(tmdev->regmap, calib_offest, cdata);
> +	}
> +
> +out_free:
> +	kfree(caldata);
> +out:
> +	return 0;
> +}
> +
> +static int tsens_register(struct tsens_device *tmdev)
> +{
> +	struct thermal_zone_device *tzd;
> +	int i = 0;
> +
> +	for (; i < tmdev->chip->sensor_num; i++) {
> +		tmdev->sensor[i].tmdev = tmdev;
> +		tmdev->sensor[i].id = i;
> +		tmdev->sensor[i].tzd = devm_thermal_zone_of_sensor_register(
> +					tmdev->dev, i, &tmdev->sensor[i],
> +					&tsens_ops);
> +		if (IS_ERR(tmdev->sensor[i].tzd))
> +			return PTR_ERR(tzd);
> +	}
> +
> +	return 0;
> +}
> +
> +static int tsens_probe(struct platform_device *pdev)
> +{
> +	struct tsens_device *tmdev;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> +	if (!tmdev)
> +		return -ENOMEM;
> +
> +	tmdev->dev = dev;
> +	tmdev->chip = of_device_get_match_data(&pdev->dev);
> +	if (!tmdev->chip)
> +		return -EINVAL;
> +
> +	ret = tsens_init(tmdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = tsens_register(tmdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = tmdev->chip->enable(tmdev);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, tmdev);
> +
> +	return ret;
> +}
> +
> +static int tsens_remove(struct platform_device *pdev)
> +{
> +	struct tsens_device *tmdev = platform_get_drvdata(pdev);
> +
> +	tmdev->chip->disable(tmdev);
> +
> +	return 0;
> +}
> +
> +static int sun50i_thermal_enable(struct tsens_device *tmdev)
> +{
> +	int ret, val;
> +
> +	ret = reset_control_deassert(tmdev->reset);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(tmdev->bus_clk);
> +	if (ret)
> +		goto assert_reset;
> +
> +	ret = tsens_calibrate(tmdev);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * clkin = 24MHz
> +	 * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1)
> +	 *           = 20us
> +	 */
> +	regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
> +		     SUN50I_THS_CTRL0_T_ACQ(479));
> +	/* average over 4 samples */
> +	regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
> +		     SUN50I_THS_FILTER_EN |
> +		     SUN50I_THS_FILTER_TYPE(1));
> +	/* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */
> +	regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
> +		     SUN50I_H6_THS_PC_TEMP_PERIOD(58));

Also this math is not all that clear:

  period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms

SUN50I_H6_THS_PC_TEMP_PERIOD is a macro with an argument. So how does
this work?

Also, related to this, I've noticed that you removed the interrupt
processing from the original driver. Without that you have to make sure
that OF contains non-zero polling-delay and polling-delay-passive.

Nonzero values are necessary for enabling polling mode of the tz core,
otherwise tz core will not read values periodically from your driver.

You should documment it in the DT bindings, too. Or keep the interrupt
handling for THS.

regards,
	o.

> +	/* enable sensor */
> +	val = GENMASK(tmdev->chip->sensor_num - 1, 0);
> +	regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val);
> +
> +	return 0;
> +
> +assert_reset:
> +	reset_control_assert(tmdev->reset);
> +
> +	return ret;
> +}
> +
> +static int sun50i_thermal_disable(struct tsens_device *tmdev)
> +{
> +	clk_disable_unprepare(tmdev->bus_clk);
> +	reset_control_assert(tmdev->reset);
> +
> +	return 0;
> +}
> +
> +static const struct sun50i_thermal_chip sun50i_h6_ths = {
> +	.sensor_num = 2,
> +	.offset = -2794,
> +	.scale = -67,
> +	.ft_deviation = SUN50I_H6_FT_DEVIATION,
> +	.temp_calib_base = SUN50I_H6_TEMP_CALIB,
> +	.temp_data_base = SUN50I_H6_TEMP_DATA,
> +	.enable = sun50i_thermal_enable,
> +	.disable = sun50i_thermal_disable,
> +};
> +
> +static const struct of_device_id of_tsens_match[] = {
> +	{ .compatible = "allwinner,sun50i-h6-ths", .data = &sun50i_h6_ths },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, of_tsens_match);
> +
> +static struct platform_driver tsens_driver = {
> +	.probe = tsens_probe,
> +	.remove = tsens_remove,
> +	.driver = {
> +		.name = "sun50i-thermal",
> +		.of_match_table = of_tsens_match,
> +	},
> +};
> +module_platform_driver(tsens_driver);
> +
> +MODULE_DESCRIPTION("Thermal sensor driver for Allwinner SOC");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Maxime Ripard May 13, 2019, 10:01 a.m. UTC | #5

On Mon, May 13, 2019 at 12:39:55AM +0200, Ondřej Jirman wrote:
> > +	/*
> > +	 * clkin = 24MHz
> > +	 * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1)
> > +	 *           = 20us
> > +	 */
> > +	regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
> > +		     SUN50I_THS_CTRL0_T_ACQ(479));
> > +	/* average over 4 samples */
> > +	regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
> > +		     SUN50I_THS_FILTER_EN |
> > +		     SUN50I_THS_FILTER_TYPE(1));
> > +	/* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */
> > +	regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
> > +		     SUN50I_H6_THS_PC_TEMP_PERIOD(58));
>
> Also this math is not all that clear:
>
>   period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms
>
> SUN50I_H6_THS_PC_TEMP_PERIOD is a macro with an argument. So how does
> this work?
>
> Also, related to this, I've noticed that you removed the interrupt
> processing from the original driver. Without that you have to make sure
> that OF contains non-zero polling-delay and polling-delay-passive.
>
> Nonzero values are necessary for enabling polling mode of the tz core,
> otherwise tz core will not read values periodically from your driver.
>
> You should documment it in the DT bindings, too. Or keep the interrupt
> handling for THS.

If there's interrupts for this in the H6, yeah we should use them over
polling.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Maxime Ripard May 16, 2019, 3:02 p.m. UTC | #6

Hi,

On Sun, May 12, 2019 at 11:41:28PM +0200, Ondřej Jirman wrote:
> > > +static int tsens_get_temp(void *data, int *temp)
> > > +{
> > > +	struct tsensor *s = data;
> > > +	struct tsens_device *tmdev = s->tmdev;
> > > +	int val;
> > > +
> > > +	regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> > > +		    0x4 * s->id, &val);
> > > +
> > > +	if (unlikely(val == 0))
> > > +		return -EBUSY;
> >
> > I'm not sure why a val equals to 0 would be associated with EBUSY?
>
> Thermal zone driver can (will) call get_temp before we got the
> first interrupt and the thermal data. In that case val will be 0.
>
> Resulting in:
>
>  (val + offset) * scale = (-2794) * -67 = 187198
>
> 187°C and immediate shutdown during boot - based on cirtical
> temperature being reached.
>
> Busy here means, get_temp does not yet have data. Thermal zone
> driver just reports any error to dmesg output.

Ah, that makes sense.

I guess if we're switching to an interrupt-based driver, then we can
just use a waitqueue, or is get_temp supposed to be atomic?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Ondřej Jirman May 16, 2019, 5:36 p.m. UTC | #7

Hello Maxime,

On Thu, May 16, 2019 at 05:02:52PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Sun, May 12, 2019 at 11:41:28PM +0200, Ondřej Jirman wrote:
> > > > +static int tsens_get_temp(void *data, int *temp)
> > > > +{
> > > > +	struct tsensor *s = data;
> > > > +	struct tsens_device *tmdev = s->tmdev;
> > > > +	int val;
> > > > +
> > > > +	regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> > > > +		    0x4 * s->id, &val);
> > > > +
> > > > +	if (unlikely(val == 0))
> > > > +		return -EBUSY;
> > >
> > > I'm not sure why a val equals to 0 would be associated with EBUSY?
> >
> > Thermal zone driver can (will) call get_temp before we got the
> > first interrupt and the thermal data. In that case val will be 0.
> >
> > Resulting in:
> >
> >  (val + offset) * scale = (-2794) * -67 = 187198
> >
> > 187°C and immediate shutdown during boot - based on cirtical
> > temperature being reached.
> >
> > Busy here means, get_temp does not yet have data. Thermal zone
> > driver just reports any error to dmesg output.
> 
> Ah, that makes sense.
> 
> I guess if we're switching to an interrupt-based driver, then we can
> just use a waitqueue, or is get_temp supposed to be atomic?

I'm not entirely sure, because I might have inadverently used a combination of
interrupt and polling when testing this. It may be that if we set polling-delay
to 0 in dts, that tz core will not try to call get_temp prematurely at all, and
will simply wait for temperature update from the interrupt.

I guess this needs to be tested/checked in tz code.

regards,
	o.

> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Yangtao Li May 16, 2019, 5:51 p.m. UTC | #8

HI Maxime,

On Sun, May 12, 2019 at 9:39 PM Maxime Ripard <[e-mail]> wrote:
>
> Hi,
>
> Thanks a lot for working on this!
>
> On Sun, May 12, 2019 at 04:26:13AM -0400, Yangtao Li wrote:
> > This patch adds the support for allwinner thermal sensor, within
> > allwinner SoC. It will register sensors for thermal framework
> > and use device tree to bind cooling device.
> >
> > Based on driver code found here:
> > https://megous.com/git/linux and https://github.com/Allwinner-Homlet/H6-BSP4.9-linux
>
> I wouldn't place the URL in the commit log. The commit log stays
> forever in the linux history. Git repos and branches are going away
> over time.
Removed.
>
> > Signed-off-by: Yangtao Li <[e-mail]>
> > ---
> >  MAINTAINERS                      |   7 +
> >  drivers/thermal/Kconfig          |  14 ++
> >  drivers/thermal/Makefile         |   1 +
> >  drivers/thermal/sun50i_thermal.c | 357 +++++++++++++++++++++++++++++++
>
> The long term goal is to support all the thermal sensors, not just the
> H6. Since that controller was introduced with the sun8i family, it
> makes more sense to use that prefix for the driver and the functions.
Done.
>
> >  4 files changed, 379 insertions(+)
> >  create mode 100644 drivers/thermal/sun50i_thermal.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 3c65228e93c5..8da56582e72a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -674,6 +674,13 @@ L:       [e-mail]
> >  S:   Maintained
> >  F:   drivers/crypto/sunxi-ss/
> >
> > +ALLWINNER THERMAL DRIVER
> > +M:   Yangtao Li <[e-mail]>
> > +L:   [e-mail]
> > +S:   Maintained
> > +F:   Documentation/devicetree/bindings/thermal/sun50i-thermal.txt
> > +F:   drivers/thermal/sun50i_thermal.c
> > +
> >  ALLWINNER VPU DRIVER
> >  M:   Maxime Ripard <[e-mail]>
> >  M:   Paul Kocialkowski <[e-mail]>
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > index 653aa27a25a4..2a8d1c98c6ca 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -252,6 +252,20 @@ config SPEAR_THERMAL
> >         Enable this to plug the SPEAr thermal sensor driver into the Linux
> >         thermal framework.
> >
> > +config SUN50I_THERMAL
> > +     tristate "Allwinner sun50i thermal driver"
> > +     depends on ARCH_SUNXI || COMPILE_TEST
> > +     depends on HAS_IOMEM
> > +     depends on NVMEM
> > +     depends on OF
> > +     depends on RESET_CONTROLLER
> > +     help
> > +       Support for the sun50i thermal sensor driver into the Linux thermal
> > +       framework.
> > +
> > +       To compile this driver as a module, choose M here: the
> > +       module will be called sun50i-thermal.
> > +
> >  config ROCKCHIP_THERMAL
> >       tristate "Rockchip thermal driver"
> >       depends on ARCH_ROCKCHIP || COMPILE_TEST
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> > index 486d682be047..a09b30b90003 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -30,6 +30,7 @@ thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
> >  # platform thermal drivers
> >  obj-y                                += broadcom/
> >  obj-$(CONFIG_SPEAR_THERMAL)  += spear_thermal.o
> > +obj-$(CONFIG_SUN50I_THERMAL) += sun50i_thermal.o
> >  obj-$(CONFIG_ROCKCHIP_THERMAL)       += rockchip_thermal.o
> >  obj-$(CONFIG_RCAR_THERMAL)   += rcar_thermal.o
> >  obj-$(CONFIG_RCAR_GEN3_THERMAL)      += rcar_gen3_thermal.o
> > diff --git a/drivers/thermal/sun50i_thermal.c b/drivers/thermal/sun50i_thermal.c
> > new file mode 100644
> > index 000000000000..3bdb3677b3d4
> > --- /dev/null
> > +++ b/drivers/thermal/sun50i_thermal.c
> > @@ -0,0 +1,357 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Thermal sensor driver for Allwinner SOC
> > + * Copyright (C) 2019 Yangtao Li
> > + *
> > + * Based on the work of Icenowy Zheng <[e-mail]>
> > + * Based on the work of Ondrej Jirman <[e-mail]>
> > + * Based on the work of Josef Gajdusek <[e-mail]>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset.h>
> > +#include <linux/slab.h>
> > +#include <linux/thermal.h>
> > +
> > +#define MAX_SENSOR_NUM       4
> > +
> > +#define FT_TEMP_MASK                         GENMASK(11, 0)
> > +#define TEMP_CALIB_MASK                              GENMASK(11, 0)
> > +#define TEMP_TO_REG                          672
> > +#define CALIBRATE_DEFAULT                    0x800
> > +
> > +#define SUN50I_THS_CTRL0                     0x00
> > +#define SUN50I_H6_THS_ENABLE                 0x04
> > +#define SUN50I_H6_THS_PC                     0x08
> > +#define SUN50I_H6_THS_MFC                    0x30
> > +#define SUN50I_H6_TEMP_CALIB                 0xa0
> > +#define SUN50I_H6_TEMP_DATA                  0xc0
> > +
> > +#define SUN50I_THS_CTRL0_T_ACQ(x)            ((GENMASK(15, 0) & (x)) << 16)
> > +#define SUN50I_THS_FILTER_EN                 BIT(2)
> > +#define SUN50I_THS_FILTER_TYPE(x)            (GENMASK(1, 0) & (x))
> > +#define SUN50I_H6_THS_PC_TEMP_PERIOD(x)              ((GENMASK(19, 0) & (x)) << 12)
> > +
> > +/* millidegree celsius */
> > +#define SUN50I_H6_FT_DEVIATION                       7000
> > +
> > +struct tsens_device;
> > +
> > +struct tsensor {
> > +     struct tsens_device             *tmdev;
> > +     struct thermal_zone_device      *tzd;
> > +     int                             id;
> > +};
> > +
> > +struct sun50i_thermal_chip {
> > +     int     sensor_num;
> > +     int     offset;
> > +     int     scale;
> > +     int     ft_deviation;
> > +     int     temp_calib_base;
> > +     int     temp_data_base;
> > +     int     (*enable)(struct tsens_device *tmdev);
> > +     int     (*disable)(struct tsens_device *tmdev);
> > +};
>
> I'm not super fond of having a lot of quirks that are not needed. If
> we ever need those quirks when adding support for a new SoC, then
> yeah, we should totally have some, but only when and if it's needed.
>
> Otherwise, the driver is more complicated for no particular reason.
This is unavoidable because of the difference in soc.
>
> > +
> > +struct tsens_device {
>
> IIRC the acronym used by allwinner is THS, maybe we can just use that
> as a prefix?
Done.
>
> > +     const struct sun50i_thermal_chip        *chip;
> > +     struct device                           *dev;
> > +     struct regmap                           *regmap;
> > +     struct reset_control                    *reset;
> > +     struct clk                              *bus_clk;
> > +     struct tsensor                          sensor[MAX_SENSOR_NUM];
> > +};
> > +
> > +/* Temp Unit: millidegree Celsius */
> > +static int tsens_reg2temp(struct tsens_device *tmdev,
> > +                           int reg)
> > +{
> > +     return (reg + tmdev->chip->offset) * tmdev->chip->scale;
> > +}
> > +
> > +static int tsens_get_temp(void *data, int *temp)
> > +{
> > +     struct tsensor *s = data;
> > +     struct tsens_device *tmdev = s->tmdev;
> > +     int val;
> > +
> > +     regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> > +                 0x4 * s->id, &val);
> > +
> > +     if (unlikely(val == 0))
> > +             return -EBUSY;
>
> I'm not sure why a val equals to 0 would be associated with EBUSY?
>
> Also, it's not in a fast path, so you can drop the unlikely. Chances
> are it's not that unlikely anyway.
>
BUSY:  Ths have no data yet.
> > +     *temp = tsens_reg2temp(tmdev, val);
> > +     if (tmdev->chip->ft_deviation)
> > +             *temp += tmdev->chip->ft_deviation;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct thermal_zone_of_device_ops tsens_ops = {
> > +     .get_temp = tsens_get_temp,
> > +};
> > +
> > +static const struct regmap_config config = {
> > +     .reg_bits = 32,
> > +     .val_bits = 32,
> > +     .reg_stride = 4,
> > +     .fast_io = true,
> > +};
> > +
> > +static int tsens_init(struct tsens_device *tmdev)
> > +{
> > +     struct device *dev = tmdev->dev;
> > +     struct platform_device *pdev = to_platform_device(dev);
> > +     struct resource *mem;
> > +     void __iomem *base;
> > +
> > +     mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     base = devm_ioremap_resource(dev, mem);
> > +     if (IS_ERR(base))
> > +             return PTR_ERR(base);
> > +
> > +     tmdev->regmap = devm_regmap_init_mmio_clk(dev, "bus",
> > +                                               base,
> > +                                               &config);
> > +     if (IS_ERR(tmdev->regmap))
> > +             return PTR_ERR(tmdev->regmap);
> > +
> > +     tmdev->reset = devm_reset_control_get(dev, "bus");
> > +     if (IS_ERR(tmdev->reset))
> > +             return PTR_ERR(tmdev->reset);
> > +
> > +     tmdev->bus_clk = devm_clk_get(&pdev->dev, "bus");
> > +     if (IS_ERR(tmdev->bus_clk))
> > +             return PTR_ERR(tmdev->bus_clk);
>
> You don't need to get that clock if regmap has it already.
It seems that clk init should be after reset reset_control_deassert.
So I convert devm_regmap_init_mmio_clk to devm_regmap_init_mmio.
>
> > +     return 0;
> > +}
> > +
> > +/*
> > + * Even if the external calibration data stored in sid is not accessible,
> > + * the THS hardware can still work, although the data won't be so accurate.
> > + * The default value of calibration register is 0x800 for every sensor,
> > + * and the calibration value is usually 0x7xx or 0x8xx, so they won't be
> > + * away from the default value for a lot.
> > + *
> > + * So here we do not return error if the calibartion data is
> > + * not available, except the probe needs deferring.
> > + */
> > +static int tsens_calibrate(struct tsens_device *tmdev)
> > +{
> > +     struct nvmem_cell *calcell;
> > +     struct device *dev = tmdev->dev;
> > +     u16 *caldata;
> > +     size_t callen;
> > +     int ft_temp;
> > +     int i = 0;
> > +
> > +     calcell = devm_nvmem_cell_get(dev, "calib");
> > +     if (IS_ERR(calcell)) {
> > +             if (PTR_ERR(calcell) == -EPROBE_DEFER)
> > +                     return -EPROBE_DEFER;
> > +
> > +             goto out;
> > +     }
> > +
> > +     caldata = nvmem_cell_read(calcell, &callen);
> > +     if (IS_ERR(caldata))
> > +             goto out;
> > +
> > +     if (!caldata[0] || callen < 2 + 2 * tmdev->chip->sensor_num)
> > +             goto out_free;
>
> The first part of your or isn't obvious and should have a comment.
>
> The second part shouldn't return 0 but an error
Done.
>
> > +
> > +     /*
> > +      * The calbration data on H6 is stored as temperature-value
> > +      * pair when being filled at factory test stage.
> > +      * The unit of stored FT temperature is 0.1 degreee celusis.
> > +      */
> > +     ft_temp = caldata[0] & FT_TEMP_MASK;
> > +
> > +     for (; i < tmdev->chip->sensor_num; i++) {
>
> Usually you would initialize i here, and not when declared.
Done.
>
> > +             int reg = (int)caldata[i + 1];
> > +             int sensor_temp = tsens_reg2temp(tmdev, reg);
> > +             int delta, cdata, calib_offest;
> > +
> > +             /*
> > +              * To calculate the calibration value:
> > +              *
> > +              * X(in Celsius) = Ts - ft_temp
> > +              * delta = X * 10000 / TEMP_TO_REG
> > +              * cdata = CALIBRATE_DEFAULT - delta
> > +              *
> > +              * cdata: calibration value
> > +              */
> > +             delta = (sensor_temp - ft_temp * 100) * 10 / TEMP_TO_REG;
> > +             cdata = CALIBRATE_DEFAULT - delta;
> > +             if (cdata & ~TEMP_CALIB_MASK) {
> > +                     dev_warn(dev, "sensor%d calibration value error", i);
> > +
> > +                     continue;
> > +             }
> > +
> > +             calib_offest = tmdev->chip->temp_calib_base + (i / 2) * 0x4;
> > +
> > +             if (i % 2) {
> > +                     int val;
> > +
> > +                     regmap_read(tmdev->regmap, calib_offest, &val);
> > +                     val = (val & TEMP_CALIB_MASK) | (cdata << 16);
> > +                     regmap_write(tmdev->regmap, calib_offest, val);
> > +             } else
> > +                     regmap_write(tmdev->regmap, calib_offest, cdata);
>
> This should have brackets as well
Done.
>
> > +     }
> > +
> > +out_free:
> > +     kfree(caldata);
> > +out:
> > +     return 0;
> > +}
> > +
> > +static int tsens_register(struct tsens_device *tmdev)
> > +{
> > +     struct thermal_zone_device *tzd;
> > +     int i = 0;
> > +
> > +     for (; i < tmdev->chip->sensor_num; i++) {
>
> Ditto
Done.
>
> > +             tmdev->sensor[i].tmdev = tmdev;
> > +             tmdev->sensor[i].id = i;
> > +             tmdev->sensor[i].tzd = devm_thermal_zone_of_sensor_register(
> > +                                     tmdev->dev, i, &tmdev->sensor[i],
> > +                                     &tsens_ops);
> > +             if (IS_ERR(tmdev->sensor[i].tzd))
> > +                     return PTR_ERR(tzd);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int tsens_probe(struct platform_device *pdev)
> > +{
> > +     struct tsens_device *tmdev;
> > +     struct device *dev = &pdev->dev;
> > +     int ret;
> > +
> > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > +     if (!tmdev)
> > +             return -ENOMEM;
> > +
> > +     tmdev->dev = dev;
> > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > +     if (!tmdev->chip)
> > +             return -EINVAL;
> > +
> > +     ret = tsens_init(tmdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = tsens_register(tmdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = tmdev->chip->enable(tmdev);
> > +     if (ret)
> > +             return ret;
> >
> > +     platform_set_drvdata(pdev, tmdev);
>
> Your registration should be the very last thing you do. Otherwise, you
> have a small window where the get_temp callback can be called, but the
> driver will not be functional yet.
No. Anyway, ths data qcquisition is ms level.
>
> > +     return ret;
> > +}
> > +
> > +static int tsens_remove(struct platform_device *pdev)
> > +{
> > +     struct tsens_device *tmdev = platform_get_drvdata(pdev);
> > +
> > +     tmdev->chip->disable(tmdev);
> > +
> > +     return 0;
> > +}
> > +
> > +static int sun50i_thermal_enable(struct tsens_device *tmdev)
> > +{
> > +     int ret, val;
> > +
> > +     ret = reset_control_deassert(tmdev->reset);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = clk_prepare_enable(tmdev->bus_clk);
> > +     if (ret)
> > +             goto assert_reset;
>
> This is done by regmap as well
>
> > +     ret = tsens_calibrate(tmdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * clkin = 24MHz
> > +      * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1)
> > +      *           = 20us
> > +      */
> > +     regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
> > +                  SUN50I_THS_CTRL0_T_ACQ(479));
> > +     /* average over 4 samples */
> > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
> > +                  SUN50I_THS_FILTER_EN |
> > +                  SUN50I_THS_FILTER_TYPE(1));
> > +     /* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */
> > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
> > +                  SUN50I_H6_THS_PC_TEMP_PERIOD(58));
> > +     /* enable sensor */
> > +     val = GENMASK(tmdev->chip->sensor_num - 1, 0);
> > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val);
> > +
> > +     return 0;
> > +
> > +assert_reset:
> > +     reset_control_assert(tmdev->reset);
> > +
> > +     return ret;
>
> Can't we do that with runtime_pm?
>
Saving energy doesn't make much sense compared to system security.

thx,
Yangtao
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Yangtao Li May 16, 2019, 6:06 p.m. UTC | #9

HI Ondřej,

On Mon, May 13, 2019 at 6:16 AM Ondřej Jirman <[e-mail]> wrote:
>
> Hello Yangtao,
>
> On Sun, May 12, 2019 at 04:26:13AM -0400, Yangtao Li wrote:
> > diff --git a/drivers/thermal/sun50i_thermal.c b/drivers/thermal/sun50i_thermal.c
> > new file mode 100644
> > index 000000000000..3bdb3677b3d4
> > --- /dev/null
> > +++ b/drivers/thermal/sun50i_thermal.c
> > @@ -0,0 +1,357 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Thermal sensor driver for Allwinner SOC
> > + * Copyright (C) 2019 Yangtao Li
> > + *
> > + * Based on the work of Icenowy Zheng <[e-mail]>
> > + * Based on the work of Ondrej Jirman <[e-mail]>
> > + * Based on the work of Josef Gajdusek <[e-mail]>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset.h>
> > +#include <linux/slab.h>
> > +#include <linux/thermal.h>
> > +
> > +#define MAX_SENSOR_NUM       4
> > +
> > +#define FT_TEMP_MASK                         GENMASK(11, 0)
> > +#define TEMP_CALIB_MASK                              GENMASK(11, 0)
> > +#define TEMP_TO_REG                          672
> > +#define CALIBRATE_DEFAULT                    0x800
> > +
> > +#define SUN50I_THS_CTRL0                     0x00
> > +#define SUN50I_H6_THS_ENABLE                 0x04
> > +#define SUN50I_H6_THS_PC                     0x08
> > +#define SUN50I_H6_THS_MFC                    0x30
> > +#define SUN50I_H6_TEMP_CALIB                 0xa0
> > +#define SUN50I_H6_TEMP_DATA                  0xc0
> > +
> > +#define SUN50I_THS_CTRL0_T_ACQ(x)            ((GENMASK(15, 0) & (x)) << 16)
> > +#define SUN50I_THS_FILTER_EN                 BIT(2)
> > +#define SUN50I_THS_FILTER_TYPE(x)            (GENMASK(1, 0) & (x))
> > +#define SUN50I_H6_THS_PC_TEMP_PERIOD(x)              ((GENMASK(19, 0) & (x)) << 12)
> > +
> > +/* millidegree celsius */
> > +#define SUN50I_H6_FT_DEVIATION                       7000
> > +
> > +struct tsens_device;
> > +
> > +struct tsensor {
> > +     struct tsens_device             *tmdev;
> > +     struct thermal_zone_device      *tzd;
> > +     int                             id;
> > +};
> > +
> > +struct sun50i_thermal_chip {
> > +     int     sensor_num;
> > +     int     offset;
> > +     int     scale;
> > +     int     ft_deviation;
> > +     int     temp_calib_base;
> > +     int     temp_data_base;
> > +     int     (*enable)(struct tsens_device *tmdev);
> > +     int     (*disable)(struct tsens_device *tmdev);
> > +};
> > +
> > +
> > +struct tsens_device {
> > +     const struct sun50i_thermal_chip        *chip;
> > +     struct device                           *dev;
> > +     struct regmap                           *regmap;
> > +     struct reset_control                    *reset;
> > +     struct clk                              *bus_clk;
> > +     struct tsensor                          sensor[MAX_SENSOR_NUM];
> > +};
> > +
> > +/* Temp Unit: millidegree Celsius */
> > +static int tsens_reg2temp(struct tsens_device *tmdev,
> > +                           int reg)
>
> Please name all functions so that they are more clearly identifiable
> in stack traces as belonging to this driver. For example:
>
>   sun8i_ths_reg2temp
>
> The same applies for all tsens_* functions below. tsens_* is too
> generic.

Done but no sun8i_ths_reg2temp.

ths_reg2tem() should be a generic func.
I think it should be suitable for all platforms， so no platform prefix.

>
> > +{
> > +     return (reg + tmdev->chip->offset) * tmdev->chip->scale;
> > +}
> > +
> > +static int tsens_get_temp(void *data, int *temp)
> > +{
> > +     struct tsensor *s = data;
> > +     struct tsens_device *tmdev = s->tmdev;
> > +     int val;
> > +
> > +     regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> > +                 0x4 * s->id, &val);
> > +
> > +     if (unlikely(val == 0))
> > +             return -EBUSY;
> > +
> > +     *temp = tsens_reg2temp(tmdev, val);
> > +     if (tmdev->chip->ft_deviation)
> > +             *temp += tmdev->chip->ft_deviation;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct thermal_zone_of_device_ops tsens_ops = {
> > +     .get_temp = tsens_get_temp,
> > +};
> > +
> > +static const struct regmap_config config = {
> > +     .reg_bits = 32,
> > +     .val_bits = 32,
> > +     .reg_stride = 4,
> > +     .fast_io = true,
> > +};
> > +
> > +static int tsens_init(struct tsens_device *tmdev)
> > +{
> > +     struct device *dev = tmdev->dev;
> > +     struct platform_device *pdev = to_platform_device(dev);
> > +     struct resource *mem;
> > +     void __iomem *base;
> > +
> > +     mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     base = devm_ioremap_resource(dev, mem);
> > +     if (IS_ERR(base))
> > +             return PTR_ERR(base);
> > +
> > +     tmdev->regmap = devm_regmap_init_mmio_clk(dev, "bus",
> > +                                               base,
> > +                                               &config);
> > +     if (IS_ERR(tmdev->regmap))
> > +             return PTR_ERR(tmdev->regmap);
> > +
> > +     tmdev->reset = devm_reset_control_get(dev, "bus");
> > +     if (IS_ERR(tmdev->reset))
> > +             return PTR_ERR(tmdev->reset);
> > +
> > +     tmdev->bus_clk = devm_clk_get(&pdev->dev, "bus");
> > +     if (IS_ERR(tmdev->bus_clk))
> > +             return PTR_ERR(tmdev->bus_clk);
> > +
> > +     return 0;
> > +}
> > +
> > +/*
> > + * Even if the external calibration data stored in sid is not accessible,
> > + * the THS hardware can still work, although the data won't be so accurate.
> > + * The default value of calibration register is 0x800 for every sensor,
> > + * and the calibration value is usually 0x7xx or 0x8xx, so they won't be
> > + * away from the default value for a lot.
> > + *
> > + * So here we do not return error if the calibartion data is
> > + * not available, except the probe needs deferring.
> > + */
> > +static int tsens_calibrate(struct tsens_device *tmdev)
> > +{
> > +     struct nvmem_cell *calcell;
> > +     struct device *dev = tmdev->dev;
> > +     u16 *caldata;
> > +     size_t callen;
> > +     int ft_temp;
> > +     int i = 0;
> > +
> > +     calcell = devm_nvmem_cell_get(dev, "calib");
> > +     if (IS_ERR(calcell)) {
> > +             if (PTR_ERR(calcell) == -EPROBE_DEFER)
> > +                     return -EPROBE_DEFER;
> > +
> > +             goto out;
> > +     }
> > +
> > +     caldata = nvmem_cell_read(calcell, &callen);
> > +     if (IS_ERR(caldata))
> > +             goto out;
> > +
> > +     if (!caldata[0] || callen < 2 + 2 * tmdev->chip->sensor_num)
> > +             goto out_free;
> > +
> > +     /*
> > +      * The calbration data on H6 is stored as temperature-value
> > +      * pair when being filled at factory test stage.
> > +      * The unit of stored FT temperature is 0.1 degreee celusis.
> > +      */
>
> Please describe the calibration data layout more clearly.
Done.
>
> > +     ft_temp = caldata[0] & FT_TEMP_MASK;
> > +
> > +     for (; i < tmdev->chip->sensor_num; i++) {
> > +             int reg = (int)caldata[i + 1];
> > +             int sensor_temp = tsens_reg2temp(tmdev, reg);
> > +             int delta, cdata, calib_offest;
> > +
> > +             /*
> > +              * To calculate the calibration value:
> > +              *
> > +              * X(in Celsius) = Ts - ft_temp
> > +              * delta = X * 10000 / TEMP_TO_REG
> > +              * cdata = CALIBRATE_DEFAULT - delta
> > +              *
> > +              * cdata: calibration value
> > +              */
> > +             delta = (sensor_temp - ft_temp * 100) * 10 / TEMP_TO_REG;
> > +             cdata = CALIBRATE_DEFAULT - delta;
> > +             if (cdata & ~TEMP_CALIB_MASK) {
> > +                     dev_warn(dev, "sensor%d calibration value error", i);
>
> Please use a more descriptive error message. What error is this?

See commnt in v2.

>
> > +                     continue;
> > +             }
> > +
> > +             calib_offest = tmdev->chip->temp_calib_base + (i / 2) * 0x4;
> > +             if (i % 2) {
> > +                     int val;
> > +
> > +                     regmap_read(tmdev->regmap, calib_offest, &val);
> > +                     val = (val & TEMP_CALIB_MASK) | (cdata << 16);
> > +                     regmap_write(tmdev->regmap, calib_offest, val);
> > +             } else
> > +                     regmap_write(tmdev->regmap, calib_offest, cdata);
> > +     }
> > +
> > +out_free:
> > +     kfree(caldata);
> > +out:
> > +     return 0;
> > +}
> > +
> > +static int tsens_register(struct tsens_device *tmdev)
> > +{
> > +     struct thermal_zone_device *tzd;
> > +     int i = 0;
> > +
> > +     for (; i < tmdev->chip->sensor_num; i++) {
> > +             tmdev->sensor[i].tmdev = tmdev;
> > +             tmdev->sensor[i].id = i;
> > +             tmdev->sensor[i].tzd = devm_thermal_zone_of_sensor_register(
> > +                                     tmdev->dev, i, &tmdev->sensor[i],
> > +                                     &tsens_ops);
> > +             if (IS_ERR(tmdev->sensor[i].tzd))
> > +                     return PTR_ERR(tzd);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int tsens_probe(struct platform_device *pdev)
> > +{
> > +     struct tsens_device *tmdev;
> > +     struct device *dev = &pdev->dev;
> > +     int ret;
> > +
> > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > +     if (!tmdev)
> > +             return -ENOMEM;
> > +
> > +     tmdev->dev = dev;
> > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > +     if (!tmdev->chip)
> > +             return -EINVAL;
> > +
> > +     ret = tsens_init(tmdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = tsens_register(tmdev);
> > +     if (ret)
> > +             return ret;
>
> Why split this out of probe into separate functions?
>
> > +     ret = tmdev->chip->enable(tmdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     platform_set_drvdata(pdev, tmdev);
> > +
> > +     return ret;
> > +}
> > +
> > +static int tsens_remove(struct platform_device *pdev)
> > +{
> > +     struct tsens_device *tmdev = platform_get_drvdata(pdev);
> > +
> > +     tmdev->chip->disable(tmdev);
> > +
> > +     return 0;
> > +}
> > +
> > +static int sun50i_thermal_enable(struct tsens_device *tmdev)
> > +{
> > +     int ret, val;
> > +
> > +     ret = reset_control_deassert(tmdev->reset);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = clk_prepare_enable(tmdev->bus_clk);
> > +     if (ret)
> > +             goto assert_reset;
> > +
> > +     ret = tsens_calibrate(tmdev);
> > +     if (ret)
> > +             return ret;
>
> If this fails (it may likely fail with EPROBE_DEFER) you are leaving reset
> deasserted, and clock enabled.
>
> Overall, I think, reset/clock management and nvmem reading will be common
> to all the HW variants, so it doesn't make much sense splitting it out
> of probe into separate functions, and makes it more error prone.

Our long-term goal is to support all platforms.
Bacicallt there is a differencr between each generation.
So I feel it necessary to isolate these differences.

Maybe:
At some point, we can draw a part of the public part and platform
difference into different
files. something like qcom thermal driver.

Regards,
Yangtao
>
> thank you and regards,
>         o.
>
> > +     /*
> > +      * clkin = 24MHz
> > +      * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1)
> > +      *           = 20us
> > +      */
> > +     regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
> > +                  SUN50I_THS_CTRL0_T_ACQ(479));
> > +     /* average over 4 samples */
> > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
> > +                  SUN50I_THS_FILTER_EN |
> > +                  SUN50I_THS_FILTER_TYPE(1));
> > +     /* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */
> > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
> > +                  SUN50I_H6_THS_PC_TEMP_PERIOD(58));
> > +     /* enable sensor */
> > +     val = GENMASK(tmdev->chip->sensor_num - 1, 0);
> > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val);
> > +
> > +     return 0;
> > +
> > +assert_reset:
> > +     reset_control_assert(tmdev->reset);
> > +
> > +     return ret;
> > +}
> > +
> > +static int sun50i_thermal_disable(struct tsens_device *tmdev)
> > +{
> > +     clk_disable_unprepare(tmdev->bus_clk);
> > +     reset_control_assert(tmdev->reset);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct sun50i_thermal_chip sun50i_h6_ths = {
> > +     .sensor_num = 2,
> > +     .offset = -2794,
> > +     .scale = -67,
> > +     .ft_deviation = SUN50I_H6_FT_DEVIATION,
> > +     .temp_calib_base = SUN50I_H6_TEMP_CALIB,
> > +     .temp_data_base = SUN50I_H6_TEMP_DATA,
> > +     .enable = sun50i_thermal_enable,
> > +     .disable = sun50i_thermal_disable,
> > +};
> > +
> > +static const struct of_device_id of_tsens_match[] = {
> > +     { .compatible = "allwinner,sun50i-h6-ths", .data = &sun50i_h6_ths },
> > +     { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, of_tsens_match);
> > +
> > +static struct platform_driver tsens_driver = {
> > +     .probe = tsens_probe,
> > +     .remove = tsens_remove,
> > +     .driver = {
> > +             .name = "sun50i-thermal",
> > +             .of_match_table = of_tsens_match,
> > +     },
> > +};
> > +module_platform_driver(tsens_driver);
> > +
> > +MODULE_DESCRIPTION("Thermal sensor driver for Allwinner SOC");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.17.0
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Yangtao Li May 16, 2019, 6:08 p.m. UTC | #10

Hi Ondřej,

On Mon, May 13, 2019 at 6:39 AM Ondřej Jirman <[e-mail]> wrote:
>
> On Sun, May 12, 2019 at 04:26:13AM -0400, Yangtao Li wrote:
> > This patch adds the support for allwinner thermal sensor, within
> > allwinner SoC. It will register sensors for thermal framework
> > and use device tree to bind cooling device.
> >
> > Based on driver code found here:
> > https://megous.com/git/linux and https://github.com/Allwinner-Homlet/H6-BSP4.9-linux
> >
> > Signed-off-by: Yangtao Li <[e-mail]>
> > ---
> >  MAINTAINERS                      |   7 +
> >  drivers/thermal/Kconfig          |  14 ++
> >  drivers/thermal/Makefile         |   1 +
> >  drivers/thermal/sun50i_thermal.c | 357 +++++++++++++++++++++++++++++++
> >  4 files changed, 379 insertions(+)
> >  create mode 100644 drivers/thermal/sun50i_thermal.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 3c65228e93c5..8da56582e72a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -674,6 +674,13 @@ L:       [e-mail]
> >  S:   Maintained
> >  F:   drivers/crypto/sunxi-ss/
> >
> > +ALLWINNER THERMAL DRIVER
> > +M:   Yangtao Li <[e-mail]>
> > +L:   [e-mail]
> > +S:   Maintained
> > +F:   Documentation/devicetree/bindings/thermal/sun50i-thermal.txt
> > +F:   drivers/thermal/sun50i_thermal.c
> > +
> >  ALLWINNER VPU DRIVER
> >  M:   Maxime Ripard <[e-mail]>
> >  M:   Paul Kocialkowski <[e-mail]>
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > index 653aa27a25a4..2a8d1c98c6ca 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -252,6 +252,20 @@ config SPEAR_THERMAL
> >         Enable this to plug the SPEAr thermal sensor driver into the Linux
> >         thermal framework.
> >
> > +config SUN50I_THERMAL
> > +     tristate "Allwinner sun50i thermal driver"
> > +     depends on ARCH_SUNXI || COMPILE_TEST
> > +     depends on HAS_IOMEM
> > +     depends on NVMEM
> > +     depends on OF
> > +     depends on RESET_CONTROLLER
> > +     help
> > +       Support for the sun50i thermal sensor driver into the Linux thermal
> > +       framework.
> > +
> > +       To compile this driver as a module, choose M here: the
> > +       module will be called sun50i-thermal.
> > +
> >  config ROCKCHIP_THERMAL
> >       tristate "Rockchip thermal driver"
> >       depends on ARCH_ROCKCHIP || COMPILE_TEST
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> > index 486d682be047..a09b30b90003 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -30,6 +30,7 @@ thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
> >  # platform thermal drivers
> >  obj-y                                += broadcom/
> >  obj-$(CONFIG_SPEAR_THERMAL)  += spear_thermal.o
> > +obj-$(CONFIG_SUN50I_THERMAL) += sun50i_thermal.o
> >  obj-$(CONFIG_ROCKCHIP_THERMAL)       += rockchip_thermal.o
> >  obj-$(CONFIG_RCAR_THERMAL)   += rcar_thermal.o
> >  obj-$(CONFIG_RCAR_GEN3_THERMAL)      += rcar_gen3_thermal.o
> > diff --git a/drivers/thermal/sun50i_thermal.c b/drivers/thermal/sun50i_thermal.c
> > new file mode 100644
> > index 000000000000..3bdb3677b3d4
> > --- /dev/null
> > +++ b/drivers/thermal/sun50i_thermal.c
> > @@ -0,0 +1,357 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Thermal sensor driver for Allwinner SOC
> > + * Copyright (C) 2019 Yangtao Li
> > + *
> > + * Based on the work of Icenowy Zheng <[e-mail]>
> > + * Based on the work of Ondrej Jirman <[e-mail]>
> > + * Based on the work of Josef Gajdusek <[e-mail]>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset.h>
> > +#include <linux/slab.h>
> > +#include <linux/thermal.h>
> > +
> > +#define MAX_SENSOR_NUM       4
> > +
> > +#define FT_TEMP_MASK                         GENMASK(11, 0)
> > +#define TEMP_CALIB_MASK                              GENMASK(11, 0)
> > +#define TEMP_TO_REG                          672
> > +#define CALIBRATE_DEFAULT                    0x800
> > +
> > +#define SUN50I_THS_CTRL0                     0x00
> > +#define SUN50I_H6_THS_ENABLE                 0x04
> > +#define SUN50I_H6_THS_PC                     0x08
> > +#define SUN50I_H6_THS_MFC                    0x30
> > +#define SUN50I_H6_TEMP_CALIB                 0xa0
> > +#define SUN50I_H6_TEMP_DATA                  0xc0
> > +
> > +#define SUN50I_THS_CTRL0_T_ACQ(x)            ((GENMASK(15, 0) & (x)) << 16)
> > +#define SUN50I_THS_FILTER_EN                 BIT(2)
> > +#define SUN50I_THS_FILTER_TYPE(x)            (GENMASK(1, 0) & (x))
> > +#define SUN50I_H6_THS_PC_TEMP_PERIOD(x)              ((GENMASK(19, 0) & (x)) << 12)
> > +
> > +/* millidegree celsius */
> > +#define SUN50I_H6_FT_DEVIATION                       7000
> > +
> > +struct tsens_device;
> > +
> > +struct tsensor {
> > +     struct tsens_device             *tmdev;
> > +     struct thermal_zone_device      *tzd;
> > +     int                             id;
> > +};
> > +
> > +struct sun50i_thermal_chip {
> > +     int     sensor_num;
> > +     int     offset;
> > +     int     scale;
> > +     int     ft_deviation;
> > +     int     temp_calib_base;
> > +     int     temp_data_base;
> > +     int     (*enable)(struct tsens_device *tmdev);
> > +     int     (*disable)(struct tsens_device *tmdev);
> > +};
> > +
> > +
> > +struct tsens_device {
> > +     const struct sun50i_thermal_chip        *chip;
> > +     struct device                           *dev;
> > +     struct regmap                           *regmap;
> > +     struct reset_control                    *reset;
> > +     struct clk                              *bus_clk;
> > +     struct tsensor                          sensor[MAX_SENSOR_NUM];
> > +};
> > +
> > +/* Temp Unit: millidegree Celsius */
> > +static int tsens_reg2temp(struct tsens_device *tmdev,
> > +                           int reg)
> > +{
> > +     return (reg + tmdev->chip->offset) * tmdev->chip->scale;
> > +}
> > +
> > +static int tsens_get_temp(void *data, int *temp)
> > +{
> > +     struct tsensor *s = data;
> > +     struct tsens_device *tmdev = s->tmdev;
> > +     int val;
> > +
> > +     regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> > +                 0x4 * s->id, &val);
> > +
> > +     if (unlikely(val == 0))
> > +             return -EBUSY;
> > +
> > +     *temp = tsens_reg2temp(tmdev, val);
> > +     if (tmdev->chip->ft_deviation)
> > +             *temp += tmdev->chip->ft_deviation;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct thermal_zone_of_device_ops tsens_ops = {
> > +     .get_temp = tsens_get_temp,
> > +};
> > +
> > +static const struct regmap_config config = {
> > +     .reg_bits = 32,
> > +     .val_bits = 32,
> > +     .reg_stride = 4,
> > +     .fast_io = true,
> > +};
> > +
> > +static int tsens_init(struct tsens_device *tmdev)
> > +{
> > +     struct device *dev = tmdev->dev;
> > +     struct platform_device *pdev = to_platform_device(dev);
> > +     struct resource *mem;
> > +     void __iomem *base;
> > +
> > +     mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     base = devm_ioremap_resource(dev, mem);
> > +     if (IS_ERR(base))
> > +             return PTR_ERR(base);
> > +
> > +     tmdev->regmap = devm_regmap_init_mmio_clk(dev, "bus",
> > +                                               base,
> > +                                               &config);
> > +     if (IS_ERR(tmdev->regmap))
> > +             return PTR_ERR(tmdev->regmap);
> > +
> > +     tmdev->reset = devm_reset_control_get(dev, "bus");
> > +     if (IS_ERR(tmdev->reset))
> > +             return PTR_ERR(tmdev->reset);
> > +
> > +     tmdev->bus_clk = devm_clk_get(&pdev->dev, "bus");
> > +     if (IS_ERR(tmdev->bus_clk))
> > +             return PTR_ERR(tmdev->bus_clk);
> > +
> > +     return 0;
> > +}
> > +
> > +/*
> > + * Even if the external calibration data stored in sid is not accessible,
> > + * the THS hardware can still work, although the data won't be so accurate.
> > + * The default value of calibration register is 0x800 for every sensor,
> > + * and the calibration value is usually 0x7xx or 0x8xx, so they won't be
> > + * away from the default value for a lot.
> > + *
> > + * So here we do not return error if the calibartion data is
> > + * not available, except the probe needs deferring.
> > + */
> > +static int tsens_calibrate(struct tsens_device *tmdev)
> > +{
> > +     struct nvmem_cell *calcell;
> > +     struct device *dev = tmdev->dev;
> > +     u16 *caldata;
> > +     size_t callen;
> > +     int ft_temp;
> > +     int i = 0;
> > +
> > +     calcell = devm_nvmem_cell_get(dev, "calib");
> > +     if (IS_ERR(calcell)) {
> > +             if (PTR_ERR(calcell) == -EPROBE_DEFER)
> > +                     return -EPROBE_DEFER;
> > +
> > +             goto out;
> > +     }
> > +
> > +     caldata = nvmem_cell_read(calcell, &callen);
> > +     if (IS_ERR(caldata))
> > +             goto out;
> > +
> > +     if (!caldata[0] || callen < 2 + 2 * tmdev->chip->sensor_num)
> > +             goto out_free;
> > +
> > +     /*
> > +      * The calbration data on H6 is stored as temperature-value
> > +      * pair when being filled at factory test stage.
> > +      * The unit of stored FT temperature is 0.1 degreee celusis.
> > +      */
> > +     ft_temp = caldata[0] & FT_TEMP_MASK;
> > +
> > +     for (; i < tmdev->chip->sensor_num; i++) {
> > +             int reg = (int)caldata[i + 1];
> > +             int sensor_temp = tsens_reg2temp(tmdev, reg);
> > +             int delta, cdata, calib_offest;
> > +
> > +             /*
> > +              * To calculate the calibration value:
> > +              *
> > +              * X(in Celsius) = Ts - ft_temp
> > +              * delta = X * 10000 / TEMP_TO_REG
> > +              * cdata = CALIBRATE_DEFAULT - delta
> > +              *
> > +              * cdata: calibration value
> > +              */
> > +             delta = (sensor_temp - ft_temp * 100) * 10 / TEMP_TO_REG;
> > +             cdata = CALIBRATE_DEFAULT - delta;
> > +             if (cdata & ~TEMP_CALIB_MASK) {
> > +                     dev_warn(dev, "sensor%d calibration value error", i);
> > +
> > +                     continue;
> > +             }
> > +
> > +             calib_offest = tmdev->chip->temp_calib_base + (i / 2) * 0x4;
> > +
> > +             if (i % 2) {
> > +                     int val;
> > +
> > +                     regmap_read(tmdev->regmap, calib_offest, &val);
> > +                     val = (val & TEMP_CALIB_MASK) | (cdata << 16);
> > +                     regmap_write(tmdev->regmap, calib_offest, val);
> > +             } else
> > +                     regmap_write(tmdev->regmap, calib_offest, cdata);
> > +     }
> > +
> > +out_free:
> > +     kfree(caldata);
> > +out:
> > +     return 0;
> > +}
> > +
> > +static int tsens_register(struct tsens_device *tmdev)
> > +{
> > +     struct thermal_zone_device *tzd;
> > +     int i = 0;
> > +
> > +     for (; i < tmdev->chip->sensor_num; i++) {
> > +             tmdev->sensor[i].tmdev = tmdev;
> > +             tmdev->sensor[i].id = i;
> > +             tmdev->sensor[i].tzd = devm_thermal_zone_of_sensor_register(
> > +                                     tmdev->dev, i, &tmdev->sensor[i],
> > +                                     &tsens_ops);
> > +             if (IS_ERR(tmdev->sensor[i].tzd))
> > +                     return PTR_ERR(tzd);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int tsens_probe(struct platform_device *pdev)
> > +{
> > +     struct tsens_device *tmdev;
> > +     struct device *dev = &pdev->dev;
> > +     int ret;
> > +
> > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > +     if (!tmdev)
> > +             return -ENOMEM;
> > +
> > +     tmdev->dev = dev;
> > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > +     if (!tmdev->chip)
> > +             return -EINVAL;
> > +
> > +     ret = tsens_init(tmdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = tsens_register(tmdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = tmdev->chip->enable(tmdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     platform_set_drvdata(pdev, tmdev);
> > +
> > +     return ret;
> > +}
> > +
> > +static int tsens_remove(struct platform_device *pdev)
> > +{
> > +     struct tsens_device *tmdev = platform_get_drvdata(pdev);
> > +
> > +     tmdev->chip->disable(tmdev);
> > +
> > +     return 0;
> > +}
> > +
> > +static int sun50i_thermal_enable(struct tsens_device *tmdev)
> > +{
> > +     int ret, val;
> > +
> > +     ret = reset_control_deassert(tmdev->reset);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = clk_prepare_enable(tmdev->bus_clk);
> > +     if (ret)
> > +             goto assert_reset;
> > +
> > +     ret = tsens_calibrate(tmdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * clkin = 24MHz
> > +      * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1)
> > +      *           = 20us
> > +      */
> > +     regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
> > +                  SUN50I_THS_CTRL0_T_ACQ(479));
> > +     /* average over 4 samples */
> > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
> > +                  SUN50I_THS_FILTER_EN |
> > +                  SUN50I_THS_FILTER_TYPE(1));
> > +     /* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */
> > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
> > +                  SUN50I_H6_THS_PC_TEMP_PERIOD(58));
>
> Also this math is not all that clear:
>
>   period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms
>
> SUN50I_H6_THS_PC_TEMP_PERIOD is a macro with an argument. So how does
> this work?
>
> Also, related to this, I've noticed that you removed the interrupt
> processing from the original driver. Without that you have to make sure
> that OF contains non-zero polling-delay and polling-delay-passive.
>
> Nonzero values are necessary for enabling polling mode of the tz core,
> otherwise tz core will not read values periodically from your driver.
>
> You should documment it in the DT bindings, too. Or keep the interrupt
> handling for THS.
See v2.

Thx,
Yangtao
>
> regards,
>         o.
>
> > +     /* enable sensor */
> > +     val = GENMASK(tmdev->chip->sensor_num - 1, 0);
> > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val);
> > +
> > +     return 0;
> > +
> > +assert_reset:
> > +     reset_control_assert(tmdev->reset);
> > +
> > +     return ret;
> > +}
> > +
> > +static int sun50i_thermal_disable(struct tsens_device *tmdev)
> > +{
> > +     clk_disable_unprepare(tmdev->bus_clk);
> > +     reset_control_assert(tmdev->reset);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct sun50i_thermal_chip sun50i_h6_ths = {
> > +     .sensor_num = 2,
> > +     .offset = -2794,
> > +     .scale = -67,
> > +     .ft_deviation = SUN50I_H6_FT_DEVIATION,
> > +     .temp_calib_base = SUN50I_H6_TEMP_CALIB,
> > +     .temp_data_base = SUN50I_H6_TEMP_DATA,
> > +     .enable = sun50i_thermal_enable,
> > +     .disable = sun50i_thermal_disable,
> > +};
> > +
> > +static const struct of_device_id of_tsens_match[] = {
> > +     { .compatible = "allwinner,sun50i-h6-ths", .data = &sun50i_h6_ths },
> > +     { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, of_tsens_match);
> > +
> > +static struct platform_driver tsens_driver = {
> > +     .probe = tsens_probe,
> > +     .remove = tsens_remove,
> > +     .driver = {
> > +             .name = "sun50i-thermal",
> > +             .of_match_table = of_tsens_match,
> > +     },
> > +};
> > +module_platform_driver(tsens_driver);
> > +
> > +MODULE_DESCRIPTION("Thermal sensor driver for Allwinner SOC");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.17.0
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Yangtao Li May 16, 2019, 6:10 p.m. UTC | #11

On Thu, May 16, 2019 at 11:03 PM Maxime Ripard
<[e-mail]> wrote:
>
> Hi,
>
> On Sun, May 12, 2019 at 11:41:28PM +0200, Ondřej Jirman wrote:
> > > > +static int tsens_get_temp(void *data, int *temp)
> > > > +{
> > > > + struct tsensor *s = data;
> > > > + struct tsens_device *tmdev = s->tmdev;
> > > > + int val;
> > > > +
> > > > + regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> > > > +             0x4 * s->id, &val);
> > > > +
> > > > + if (unlikely(val == 0))
> > > > +         return -EBUSY;
> > >
> > > I'm not sure why a val equals to 0 would be associated with EBUSY?
> >
> > Thermal zone driver can (will) call get_temp before we got the
> > first interrupt and the thermal data. In that case val will be 0.
> >
> > Resulting in:
> >
> >  (val + offset) * scale = (-2794) * -67 = 187198
> >
> > 187°C and immediate shutdown during boot - based on cirtical
> > temperature being reached.
> >
> > Busy here means, get_temp does not yet have data. Thermal zone
> > driver just reports any error to dmesg output.
>
> Ah, that makes sense.
>
> I guess if we're switching to an interrupt-based driver, then we can
> just use a waitqueue, or is get_temp supposed to be atomic?
I think get_temp should not be bloacked.

Yangtao
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Ondřej Jirman May 16, 2019, 6:29 p.m. UTC | #12

Hi Yangtao,

thank you for work on this driver.

On Fri, May 17, 2019 at 02:06:53AM +0800, Frank Lee wrote:
> HI Ondřej,
> 
> On Mon, May 13, 2019 at 6:16 AM Ondřej Jirman <[e-mail]> wrote:
> > > +
> > > +/* Temp Unit: millidegree Celsius */
> > > +static int tsens_reg2temp(struct tsens_device *tmdev,
> > > +                           int reg)
> >
> > Please name all functions so that they are more clearly identifiable
> > in stack traces as belonging to this driver. For example:
> >
> >   sun8i_ths_reg2temp
> >
> > The same applies for all tsens_* functions below. tsens_* is too
> > generic.
> 
> Done but no sun8i_ths_reg2temp.
> 
> ths_reg2tem() should be a generic func.
> I think it should be suitable for all platforms， so no platform prefix.

You've missed my point. The driver name is sun8i_thermal and if you get
and oops from the kernel you'll get a stack trace where there are just function
names. If you use too generic function names, it will not be clear which
driver is oopsing.

  - sun8i_ths_reg2temp will tell you much more clearly where to search than
  - ths_reg2temp

Of course you can always grep, but most thermal drivers are thermal sensor (ths)
drivers, and if multiple of them used this too-generic naming scheme you'd
have hard time debugging.

Look at other thermal drivers. They usually encode driver name in the function
names to help with identification (even if these are static driver-local
functions).

> > > +static int tsens_probe(struct platform_device *pdev)
> > > +{
> > > +     struct tsens_device *tmdev;
> > > +     struct device *dev = &pdev->dev;
> > > +     int ret;
> > > +
> > > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > > +     if (!tmdev)
> > > +             return -ENOMEM;
> > > +
> > > +     tmdev->dev = dev;
> > > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > > +     if (!tmdev->chip)
> > > +             return -EINVAL;
> > > +
> > > +     ret = tsens_init(tmdev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = tsens_register(tmdev);
> > > +     if (ret)
> > > +             return ret;
> >
> > Why split this out of probe into separate functions?
> >
> > > +     ret = tmdev->chip->enable(tmdev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     platform_set_drvdata(pdev, tmdev);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static int tsens_remove(struct platform_device *pdev)
> > > +{
> > > +     struct tsens_device *tmdev = platform_get_drvdata(pdev);
> > > +
> > > +     tmdev->chip->disable(tmdev);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int sun50i_thermal_enable(struct tsens_device *tmdev)
> > > +{
> > > +     int ret, val;
> > > +
> > > +     ret = reset_control_deassert(tmdev->reset);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = clk_prepare_enable(tmdev->bus_clk);
> > > +     if (ret)
> > > +             goto assert_reset;
> > > +
> > > +     ret = tsens_calibrate(tmdev);
> > > +     if (ret)
> > > +             return ret;
> >
> > If this fails (it may likely fail with EPROBE_DEFER) you are leaving reset
> > deasserted, and clock enabled.
> >
> > Overall, I think, reset/clock management and nvmem reading will be common
> > to all the HW variants, so it doesn't make much sense splitting it out
> > of probe into separate functions, and makes it more error prone.
> 
> Our long-term goal is to support all platforms.
> Bacicallt there is a differencr between each generation.
> So I feel it necessary to isolate these differences.
> 
> Maybe:
> At some point, we can draw a part of the public part and platform
> difference into different
> files. something like qcom thermal driver.

I understand, but I wrote ths drivers for H3/H5/A83T and it so far it looks like
all of them would share these 3 calls.

You'll be enabling clock/reset and callibrating everywhere. So putting this to
per-SoC function seems premature.

thank you and regards,
	o.

> Regards,
> Yangtao

Ondřej Jirman May 16, 2019, 9:11 p.m. UTC | #13

Hello,

On Fri, May 17, 2019 at 01:51:56AM +0800, Frank Lee wrote:
> > > +static int tsens_probe(struct platform_device *pdev)
> > > +{
> > > +     struct tsens_device *tmdev;
> > > +     struct device *dev = &pdev->dev;
> > > +     int ret;
> > > +
> > > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > > +     if (!tmdev)
> > > +             return -ENOMEM;
> > > +
> > > +     tmdev->dev = dev;
> > > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > > +     if (!tmdev->chip)
> > > +             return -EINVAL;
> > > +
> > > +     ret = tsens_init(tmdev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = tsens_register(tmdev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = tmdev->chip->enable(tmdev);
> > > +     if (ret)
> > > +             return ret;
> > >
> > > +     platform_set_drvdata(pdev, tmdev);
> >
> > Your registration should be the very last thing you do. Otherwise, you
> > have a small window where the get_temp callback can be called, but the
> > driver will not be functional yet.
> No. Anyway, ths data qcquisition is ms level.

Tz code can change in the future, and call the get_temp callback during
registration, and this would break. It's better to be correct, than make
dangerous assumptions. So platform_set_drvdata should be done somewhere 
prior to init_resource.

Enable should be after register though. Because otherwise you may be calling
tz update on non-registered tz from an interrupt handler.

> > > +     return ret;
> > > +}
> > > +

regards,
	o.

Maxime Ripard May 17, 2019, 7:31 a.m. UTC | #14

On Fri, May 17, 2019 at 02:10:47AM +0800, Frank Lee wrote:
> > On Sun, May 12, 2019 at 11:41:28PM +0200, Ondřej Jirman wrote:
> > > > > +static int tsens_get_temp(void *data, int *temp)
> > > > > +{
> > > > > + struct tsensor *s = data;
> > > > > + struct tsens_device *tmdev = s->tmdev;
> > > > > + int val;
> > > > > +
> > > > > + regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> > > > > +             0x4 * s->id, &val);
> > > > > +
> > > > > + if (unlikely(val == 0))
> > > > > +         return -EBUSY;
> > > >
> > > > I'm not sure why a val equals to 0 would be associated with EBUSY?
> > >
> > > Thermal zone driver can (will) call get_temp before we got the
> > > first interrupt and the thermal data. In that case val will be 0.
> > >
> > > Resulting in:
> > >
> > >  (val + offset) * scale = (-2794) * -67 = 187198
> > >
> > > 187°C and immediate shutdown during boot - based on cirtical
> > > temperature being reached.
> > >
> > > Busy here means, get_temp does not yet have data. Thermal zone
> > > driver just reports any error to dmesg output.
> >
> > Ah, that makes sense.
> >
> > I guess if we're switching to an interrupt-based driver, then we can
> > just use a waitqueue, or is get_temp supposed to be atomic?
>
> I think get_temp should not be bloacked.

Why not?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Maxime Ripard May 17, 2019, 7:36 a.m. UTC | #15

On Fri, May 17, 2019 at 01:51:56AM +0800, Frank Lee wrote:
> > > +struct sun50i_thermal_chip {
> > > +     int     sensor_num;
> > > +     int     offset;
> > > +     int     scale;
> > > +     int     ft_deviation;
> > > +     int     temp_calib_base;
> > > +     int     temp_data_base;
> > > +     int     (*enable)(struct tsens_device *tmdev);
> > > +     int     (*disable)(struct tsens_device *tmdev);
> > > +};
> >
> > I'm not super fond of having a lot of quirks that are not needed. If
> > we ever need those quirks when adding support for a new SoC, then
> > yeah, we should totally have some, but only when and if it's needed.
> >
> > Otherwise, the driver is more complicated for no particular reason.
>
> This is unavoidable because of the difference in soc.

I know, but this isn't my point.

My point is that at this time of the driver development, we don't know
what is going to be needed to support all of those SoCs.

Some of the parameters you added might not be needed, some parameters
might be missing, we don't know. So let's keep it simple for now.

> > > +static int tsens_probe(struct platform_device *pdev)
> > > +{
> > > +     struct tsens_device *tmdev;
> > > +     struct device *dev = &pdev->dev;
> > > +     int ret;
> > > +
> > > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > > +     if (!tmdev)
> > > +             return -ENOMEM;
> > > +
> > > +     tmdev->dev = dev;
> > > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > > +     if (!tmdev->chip)
> > > +             return -EINVAL;
> > > +
> > > +     ret = tsens_init(tmdev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = tsens_register(tmdev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = tmdev->chip->enable(tmdev);
> > > +     if (ret)
> > > +             return ret;
> > >
> > > +     platform_set_drvdata(pdev, tmdev);
> >
> > Your registration should be the very last thing you do. Otherwise, you
> > have a small window where the get_temp callback can be called, but the
> > driver will not be functional yet.
>
> No. Anyway, ths data qcquisition is ms level.

That's kind of irrelevant. There's nothing preventing get_temp to be
called right away.

> > > +     ret = tsens_calibrate(tmdev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /*
> > > +      * clkin = 24MHz
> > > +      * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1)
> > > +      *           = 20us
> > > +      */
> > > +     regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
> > > +                  SUN50I_THS_CTRL0_T_ACQ(479));
> > > +     /* average over 4 samples */
> > > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
> > > +                  SUN50I_THS_FILTER_EN |
> > > +                  SUN50I_THS_FILTER_TYPE(1));
> > > +     /* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */
> > > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
> > > +                  SUN50I_H6_THS_PC_TEMP_PERIOD(58));
> > > +     /* enable sensor */
> > > +     val = GENMASK(tmdev->chip->sensor_num - 1, 0);
> > > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val);
> > > +
> > > +     return 0;
> > > +
> > > +assert_reset:
> > > +     reset_control_assert(tmdev->reset);
> > > +
> > > +     return ret;
> >
> > Can't we do that with runtime_pm?
>
> Saving energy doesn't make much sense compared to system security.

I'm not sure what you mean by security.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Yangtao Li May 17, 2019, 4:34 p.m. UTC | #16

HI,

On Fri, May 17, 2019 at 2:29 AM Ondřej Jirman <[e-mail]> wrote:
>
> Hi Yangtao,
>
> thank you for work on this driver.
>
> On Fri, May 17, 2019 at 02:06:53AM +0800, Frank Lee wrote:
> > HI Ondřej,
> >
> > On Mon, May 13, 2019 at 6:16 AM Ondřej Jirman <[e-mail]> wrote:
> > > > +
> > > > +/* Temp Unit: millidegree Celsius */
> > > > +static int tsens_reg2temp(struct tsens_device *tmdev,
> > > > +                           int reg)
> > >
> > > Please name all functions so that they are more clearly identifiable
> > > in stack traces as belonging to this driver. For example:
> > >
> > >   sun8i_ths_reg2temp
> > >
> > > The same applies for all tsens_* functions below. tsens_* is too
> > > generic.
> >
> > Done but no sun8i_ths_reg2temp.
> >
> > ths_reg2tem() should be a generic func.
> > I think it should be suitable for all platforms， so no platform prefix.
>
> You've missed my point. The driver name is sun8i_thermal and if you get
> and oops from the kernel you'll get a stack trace where there are just function
> names. If you use too generic function names, it will not be clear which
> driver is oopsing.
>
>   - sun8i_ths_reg2temp will tell you much more clearly where to search than
>   - ths_reg2temp
>
> Of course you can always grep, but most thermal drivers are thermal sensor (ths)
> drivers, and if multiple of them used this too-generic naming scheme you'd
> have hard time debugging.
>
> Look at other thermal drivers. They usually encode driver name in the function
> names to help with identification (even if these are static driver-local
> functions).
>

Can we change to sunxi_ths_ prefix?

> > > > +static int tsens_probe(struct platform_device *pdev)
> > > > +{
> > > > +     struct tsens_device *tmdev;
> > > > +     struct device *dev = &pdev->dev;
> > > > +     int ret;
> > > > +
> > > > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > > > +     if (!tmdev)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     tmdev->dev = dev;
> > > > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > > > +     if (!tmdev->chip)
> > > > +             return -EINVAL;
> > > > +
> > > > +     ret = tsens_init(tmdev);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     ret = tsens_register(tmdev);
> > > > +     if (ret)
> > > > +             return ret;
> > >
> > > Why split this out of probe into separate functions?
> > >
> > > > +     ret = tmdev->chip->enable(tmdev);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     platform_set_drvdata(pdev, tmdev);
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
> > > > +static int tsens_remove(struct platform_device *pdev)
> > > > +{
> > > > +     struct tsens_device *tmdev = platform_get_drvdata(pdev);
> > > > +
> > > > +     tmdev->chip->disable(tmdev);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int sun50i_thermal_enable(struct tsens_device *tmdev)
> > > > +{
> > > > +     int ret, val;
> > > > +
> > > > +     ret = reset_control_deassert(tmdev->reset);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     ret = clk_prepare_enable(tmdev->bus_clk);
> > > > +     if (ret)
> > > > +             goto assert_reset;
> > > > +
> > > > +     ret = tsens_calibrate(tmdev);
> > > > +     if (ret)
> > > > +             return ret;
> > >
> > > If this fails (it may likely fail with EPROBE_DEFER) you are leaving reset
> > > deasserted, and clock enabled.
> > >
> > > Overall, I think, reset/clock management and nvmem reading will be common
> > > to all the HW variants, so it doesn't make much sense splitting it out
> > > of probe into separate functions, and makes it more error prone.
> >
> > Our long-term goal is to support all platforms.
> > Bacicallt there is a differencr between each generation.
> > So I feel it necessary to isolate these differences.
> >
> > Maybe:
> > At some point, we can draw a part of the public part and platform
> > difference into different
> > files. something like qcom thermal driver.
>
> I understand, but I wrote ths drivers for H3/H5/A83T and it so far it looks like
> all of them would share these 3 calls.
>
> You'll be enabling clock/reset and callibrating everywhere. So putting this to
> per-SoC function seems premature.

In fact, enalbe and disable are the suspend and resume functions.(PM
callback will be added in the future)
When exiting from s2ram, the register will become the initial value.
We need to do all the work, enabling reset/clk ,calibrating and
initializing other reg.

So I think it is no need to put enabling reset/clk and calibrating to
probe func, and I'd like
to keep enable and disable func.

>
> thank you and regards,
>         o.
>
> > Regards,
> > Yangtao

Yangtao Li May 17, 2019, 5:19 p.m. UTC | #17

On Fri, May 17, 2019 at 3:32 PM Maxime Ripard <[e-mail]> wrote:
>
> On Fri, May 17, 2019 at 02:10:47AM +0800, Frank Lee wrote:
> > > On Sun, May 12, 2019 at 11:41:28PM +0200, Ondřej Jirman wrote:
> > > > > > +static int tsens_get_temp(void *data, int *temp)
> > > > > > +{
> > > > > > + struct tsensor *s = data;
> > > > > > + struct tsens_device *tmdev = s->tmdev;
> > > > > > + int val;
> > > > > > +
> > > > > > + regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> > > > > > +             0x4 * s->id, &val);
> > > > > > +
> > > > > > + if (unlikely(val == 0))
> > > > > > +         return -EBUSY;
> > > > >
> > > > > I'm not sure why a val equals to 0 would be associated with EBUSY?
> > > >
> > > > Thermal zone driver can (will) call get_temp before we got the
> > > > first interrupt and the thermal data. In that case val will be 0.
> > > >
> > > > Resulting in:
> > > >
> > > >  (val + offset) * scale = (-2794) * -67 = 187198
> > > >
> > > > 187°C and immediate shutdown during boot - based on cirtical
> > > > temperature being reached.
> > > >
> > > > Busy here means, get_temp does not yet have data. Thermal zone
> > > > driver just reports any error to dmesg output.
> > >
> > > Ah, that makes sense.
> > >
> > > I guess if we're switching to an interrupt-based driver, then we can
> > > just use a waitqueue, or is get_temp supposed to be atomic?
> >
> > I think get_temp should not be bloacked.
>
> Why not?

Maybe, I am wrong. I also want to know if we should do this.

Yangtao

>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Yangtao Li May 17, 2019, 5:27 p.m. UTC | #18

On Fri, May 17, 2019 at 3:36 PM Maxime Ripard <[e-mail]> wrote:
>
> On Fri, May 17, 2019 at 01:51:56AM +0800, Frank Lee wrote:
> > > > +struct sun50i_thermal_chip {
> > > > +     int     sensor_num;
> > > > +     int     offset;
> > > > +     int     scale;
> > > > +     int     ft_deviation;
> > > > +     int     temp_calib_base;
> > > > +     int     temp_data_base;
> > > > +     int     (*enable)(struct tsens_device *tmdev);
> > > > +     int     (*disable)(struct tsens_device *tmdev);
> > > > +};
> > >
> > > I'm not super fond of having a lot of quirks that are not needed. If
> > > we ever need those quirks when adding support for a new SoC, then
> > > yeah, we should totally have some, but only when and if it's needed.
> > >
> > > Otherwise, the driver is more complicated for no particular reason.
> >
> > This is unavoidable because of the difference in soc.
>
> I know, but this isn't my point.
>
> My point is that at this time of the driver development, we don't know
> what is going to be needed to support all of those SoCs.
>
> Some of the parameters you added might not be needed, some parameters
> might be missing, we don't know. So let's keep it simple for now.
>
> > > > +static int tsens_probe(struct platform_device *pdev)
> > > > +{
> > > > +     struct tsens_device *tmdev;
> > > > +     struct device *dev = &pdev->dev;
> > > > +     int ret;
> > > > +
> > > > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > > > +     if (!tmdev)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     tmdev->dev = dev;
> > > > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > > > +     if (!tmdev->chip)
> > > > +             return -EINVAL;
> > > > +
> > > > +     ret = tsens_init(tmdev);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     ret = tsens_register(tmdev);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     ret = tmdev->chip->enable(tmdev);
> > > > +     if (ret)
> > > > +             return ret;
> > > >
> > > > +     platform_set_drvdata(pdev, tmdev);
> > >
> > > Your registration should be the very last thing you do. Otherwise, you
> > > have a small window where the get_temp callback can be called, but the
> > > driver will not be functional yet.
> >
> > No. Anyway, ths data qcquisition is ms level.
>
> That's kind of irrelevant. There's nothing preventing get_temp to be
> called right away.
As Ondřej said,

Registration after enabling will lead to call tz update on non-registered tz
from an interrupt handler.

>
> > > > +     ret = tsens_calibrate(tmdev);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     /*
> > > > +      * clkin = 24MHz
> > > > +      * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1)
> > > > +      *           = 20us
> > > > +      */
> > > > +     regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
> > > > +                  SUN50I_THS_CTRL0_T_ACQ(479));
> > > > +     /* average over 4 samples */
> > > > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
> > > > +                  SUN50I_THS_FILTER_EN |
> > > > +                  SUN50I_THS_FILTER_TYPE(1));
> > > > +     /* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */
> > > > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
> > > > +                  SUN50I_H6_THS_PC_TEMP_PERIOD(58));
> > > > +     /* enable sensor */
> > > > +     val = GENMASK(tmdev->chip->sensor_num - 1, 0);
> > > > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val);
> > > > +
> > > > +     return 0;
> > > > +
> > > > +assert_reset:
> > > > +     reset_control_assert(tmdev->reset);
> > > > +
> > > > +     return ret;
> > >
> > > Can't we do that with runtime_pm?
> >
> > Saving energy doesn't make much sense compared to system security.
>
> I'm not sure what you mean by security.

Protect system hardware from damage.

Thx,
Yangtao
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Vasily Khoruzhick May 17, 2019, 7:21 p.m. UTC | #19

On Sun, May 12, 2019 at 6:39 AM Maxime Ripard <[e-mail]> wrote:
>
> Hi,
>
> Thanks a lot for working on this!
>
> On Sun, May 12, 2019 at 04:26:13AM -0400, Yangtao Li wrote:
> > This patch adds the support for allwinner thermal sensor, within
> > allwinner SoC. It will register sensors for thermal framework
> > and use device tree to bind cooling device.
> >
> > Based on driver code found here:
> > https://megous.com/git/linux and https://github.com/Allwinner-Homlet/H6-BSP4.9-linux
>
> I wouldn't place the URL in the commit log. The commit log stays
> forever in the linux history. Git repos and branches are going away
> over time.
>
> > Signed-off-by: Yangtao Li <[e-mail]>
> > ---
> >  MAINTAINERS                      |   7 +
> >  drivers/thermal/Kconfig          |  14 ++
> >  drivers/thermal/Makefile         |   1 +
> >  drivers/thermal/sun50i_thermal.c | 357 +++++++++++++++++++++++++++++++
>
> The long term goal is to support all the thermal sensors, not just the
> H6. Since that controller was introduced with the sun8i family, it
> makes more sense to use that prefix for the driver and the functions.
>
> >  4 files changed, 379 insertions(+)
> >  create mode 100644 drivers/thermal/sun50i_thermal.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 3c65228e93c5..8da56582e72a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -674,6 +674,13 @@ L:       [e-mail]
> >  S:   Maintained
> >  F:   drivers/crypto/sunxi-ss/
> >
> > +ALLWINNER THERMAL DRIVER
> > +M:   Yangtao Li <[e-mail]>
> > +L:   [e-mail]
> > +S:   Maintained
> > +F:   Documentation/devicetree/bindings/thermal/sun50i-thermal.txt
> > +F:   drivers/thermal/sun50i_thermal.c
> > +
> >  ALLWINNER VPU DRIVER
> >  M:   Maxime Ripard <[e-mail]>
> >  M:   Paul Kocialkowski <[e-mail]>
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > index 653aa27a25a4..2a8d1c98c6ca 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -252,6 +252,20 @@ config SPEAR_THERMAL
> >         Enable this to plug the SPEAr thermal sensor driver into the Linux
> >         thermal framework.
> >
> > +config SUN50I_THERMAL
> > +     tristate "Allwinner sun50i thermal driver"
> > +     depends on ARCH_SUNXI || COMPILE_TEST
> > +     depends on HAS_IOMEM
> > +     depends on NVMEM
> > +     depends on OF
> > +     depends on RESET_CONTROLLER
> > +     help
> > +       Support for the sun50i thermal sensor driver into the Linux thermal
> > +       framework.
> > +
> > +       To compile this driver as a module, choose M here: the
> > +       module will be called sun50i-thermal.
> > +
> >  config ROCKCHIP_THERMAL
> >       tristate "Rockchip thermal driver"
> >       depends on ARCH_ROCKCHIP || COMPILE_TEST
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> > index 486d682be047..a09b30b90003 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -30,6 +30,7 @@ thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
> >  # platform thermal drivers
> >  obj-y                                += broadcom/
> >  obj-$(CONFIG_SPEAR_THERMAL)  += spear_thermal.o
> > +obj-$(CONFIG_SUN50I_THERMAL) += sun50i_thermal.o
> >  obj-$(CONFIG_ROCKCHIP_THERMAL)       += rockchip_thermal.o
> >  obj-$(CONFIG_RCAR_THERMAL)   += rcar_thermal.o
> >  obj-$(CONFIG_RCAR_GEN3_THERMAL)      += rcar_gen3_thermal.o
> > diff --git a/drivers/thermal/sun50i_thermal.c b/drivers/thermal/sun50i_thermal.c
> > new file mode 100644
> > index 000000000000..3bdb3677b3d4
> > --- /dev/null
> > +++ b/drivers/thermal/sun50i_thermal.c
> > @@ -0,0 +1,357 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Thermal sensor driver for Allwinner SOC
> > + * Copyright (C) 2019 Yangtao Li
> > + *
> > + * Based on the work of Icenowy Zheng <[e-mail]>
> > + * Based on the work of Ondrej Jirman <[e-mail]>
> > + * Based on the work of Josef Gajdusek <[e-mail]>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset.h>
> > +#include <linux/slab.h>
> > +#include <linux/thermal.h>
> > +
> > +#define MAX_SENSOR_NUM       4
> > +
> > +#define FT_TEMP_MASK                         GENMASK(11, 0)
> > +#define TEMP_CALIB_MASK                              GENMASK(11, 0)
> > +#define TEMP_TO_REG                          672
> > +#define CALIBRATE_DEFAULT                    0x800
> > +
> > +#define SUN50I_THS_CTRL0                     0x00
> > +#define SUN50I_H6_THS_ENABLE                 0x04
> > +#define SUN50I_H6_THS_PC                     0x08
> > +#define SUN50I_H6_THS_MFC                    0x30
> > +#define SUN50I_H6_TEMP_CALIB                 0xa0
> > +#define SUN50I_H6_TEMP_DATA                  0xc0
> > +
> > +#define SUN50I_THS_CTRL0_T_ACQ(x)            ((GENMASK(15, 0) & (x)) << 16)
> > +#define SUN50I_THS_FILTER_EN                 BIT(2)
> > +#define SUN50I_THS_FILTER_TYPE(x)            (GENMASK(1, 0) & (x))
> > +#define SUN50I_H6_THS_PC_TEMP_PERIOD(x)              ((GENMASK(19, 0) & (x)) << 12)
> > +
> > +/* millidegree celsius */
> > +#define SUN50I_H6_FT_DEVIATION                       7000
> > +
> > +struct tsens_device;
> > +
> > +struct tsensor {
> > +     struct tsens_device             *tmdev;
> > +     struct thermal_zone_device      *tzd;
> > +     int                             id;
> > +};
> > +
> > +struct sun50i_thermal_chip {
> > +     int     sensor_num;
> > +     int     offset;
> > +     int     scale;
> > +     int     ft_deviation;
> > +     int     temp_calib_base;
> > +     int     temp_data_base;
> > +     int     (*enable)(struct tsens_device *tmdev);
> > +     int     (*disable)(struct tsens_device *tmdev);
> > +};
>
> I'm not super fond of having a lot of quirks that are not needed. If
> we ever need those quirks when adding support for a new SoC, then
> yeah, we should totally have some, but only when and if it's needed.
>
> Otherwise, the driver is more complicated for no particular reason.
>
> > +
> > +struct tsens_device {
>
> IIRC the acronym used by allwinner is THS, maybe we can just use that
> as a prefix?
>
> > +     const struct sun50i_thermal_chip        *chip;
> > +     struct device                           *dev;
> > +     struct regmap                           *regmap;
> > +     struct reset_control                    *reset;
> > +     struct clk                              *bus_clk;
> > +     struct tsensor                          sensor[MAX_SENSOR_NUM];
> > +};
> > +
> > +/* Temp Unit: millidegree Celsius */
> > +static int tsens_reg2temp(struct tsens_device *tmdev,
> > +                           int reg)
> > +{
> > +     return (reg + tmdev->chip->offset) * tmdev->chip->scale;
> > +}
> > +
> > +static int tsens_get_temp(void *data, int *temp)
> > +{
> > +     struct tsensor *s = data;
> > +     struct tsens_device *tmdev = s->tmdev;
> > +     int val;
> > +
> > +     regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> > +                 0x4 * s->id, &val);
> > +
> > +     if (unlikely(val == 0))
> > +             return -EBUSY;
>
> I'm not sure why a val equals to 0 would be associated with EBUSY?

First few reads of temp data return 0, and in case of H6 (and A64) it
means max temperature, so kernel does emergency shutdown. I used
-ETIMEDOUT as a workaround in my tree, but it's not appropriate here
either. Any suggestions are welcome.

> Also, it's not in a fast path, so you can drop the unlikely. Chances
> are it's not that unlikely anyway.

As I said earlier, it's just few samples after start up.

> > +     *temp = tsens_reg2temp(tmdev, val);
> > +     if (tmdev->chip->ft_deviation)
> > +             *temp += tmdev->chip->ft_deviation;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct thermal_zone_of_device_ops tsens_ops = {
> > +     .get_temp = tsens_get_temp,
> > +};
> > +
> > +static const struct regmap_config config = {
> > +     .reg_bits = 32,
> > +     .val_bits = 32,
> > +     .reg_stride = 4,
> > +     .fast_io = true,
> > +};
> > +
> > +static int tsens_init(struct tsens_device *tmdev)
> > +{
> > +     struct device *dev = tmdev->dev;
> > +     struct platform_device *pdev = to_platform_device(dev);
> > +     struct resource *mem;
> > +     void __iomem *base;
> > +
> > +     mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     base = devm_ioremap_resource(dev, mem);
> > +     if (IS_ERR(base))
> > +             return PTR_ERR(base);
> > +
> > +     tmdev->regmap = devm_regmap_init_mmio_clk(dev, "bus",
> > +                                               base,
> > +                                               &config);
> > +     if (IS_ERR(tmdev->regmap))
> > +             return PTR_ERR(tmdev->regmap);
> > +
> > +     tmdev->reset = devm_reset_control_get(dev, "bus");
> > +     if (IS_ERR(tmdev->reset))
> > +             return PTR_ERR(tmdev->reset);
> > +
> > +     tmdev->bus_clk = devm_clk_get(&pdev->dev, "bus");
> > +     if (IS_ERR(tmdev->bus_clk))
> > +             return PTR_ERR(tmdev->bus_clk);
>
> You don't need to get that clock if regmap has it already.
>
> > +     return 0;
> > +}
> > +
> > +/*
> > + * Even if the external calibration data stored in sid is not accessible,
> > + * the THS hardware can still work, although the data won't be so accurate.
> > + * The default value of calibration register is 0x800 for every sensor,
> > + * and the calibration value is usually 0x7xx or 0x8xx, so they won't be
> > + * away from the default value for a lot.
> > + *
> > + * So here we do not return error if the calibartion data is
> > + * not available, except the probe needs deferring.
> > + */
> > +static int tsens_calibrate(struct tsens_device *tmdev)
> > +{
> > +     struct nvmem_cell *calcell;
> > +     struct device *dev = tmdev->dev;
> > +     u16 *caldata;
> > +     size_t callen;
> > +     int ft_temp;
> > +     int i = 0;
> > +
> > +     calcell = devm_nvmem_cell_get(dev, "calib");
> > +     if (IS_ERR(calcell)) {
> > +             if (PTR_ERR(calcell) == -EPROBE_DEFER)
> > +                     return -EPROBE_DEFER;
> > +
> > +             goto out;
> > +     }
> > +
> > +     caldata = nvmem_cell_read(calcell, &callen);
> > +     if (IS_ERR(caldata))
> > +             goto out;
> > +
> > +     if (!caldata[0] || callen < 2 + 2 * tmdev->chip->sensor_num)
> > +             goto out_free;
>
> The first part of your or isn't obvious and should have a comment.
>
> The second part shouldn't return 0 but an error
>
> > +
> > +     /*
> > +      * The calbration data on H6 is stored as temperature-value
> > +      * pair when being filled at factory test stage.
> > +      * The unit of stored FT temperature is 0.1 degreee celusis.
> > +      */
> > +     ft_temp = caldata[0] & FT_TEMP_MASK;
> > +
> > +     for (; i < tmdev->chip->sensor_num; i++) {
>
> Usually you would initialize i here, and not when declared.
>
> > +             int reg = (int)caldata[i + 1];
> > +             int sensor_temp = tsens_reg2temp(tmdev, reg);
> > +             int delta, cdata, calib_offest;
> > +
> > +             /*
> > +              * To calculate the calibration value:
> > +              *
> > +              * X(in Celsius) = Ts - ft_temp
> > +              * delta = X * 10000 / TEMP_TO_REG
> > +              * cdata = CALIBRATE_DEFAULT - delta
> > +              *
> > +              * cdata: calibration value
> > +              */
> > +             delta = (sensor_temp - ft_temp * 100) * 10 / TEMP_TO_REG;
> > +             cdata = CALIBRATE_DEFAULT - delta;
> > +             if (cdata & ~TEMP_CALIB_MASK) {
> > +                     dev_warn(dev, "sensor%d calibration value error", i);
> > +
> > +                     continue;
> > +             }
> > +
> > +             calib_offest = tmdev->chip->temp_calib_base + (i / 2) * 0x4;
> > +
> > +             if (i % 2) {
> > +                     int val;
> > +
> > +                     regmap_read(tmdev->regmap, calib_offest, &val);
> > +                     val = (val & TEMP_CALIB_MASK) | (cdata << 16);
> > +                     regmap_write(tmdev->regmap, calib_offest, val);
> > +             } else
> > +                     regmap_write(tmdev->regmap, calib_offest, cdata);
>
> This should have brackets as well
>
> > +     }
> > +
> > +out_free:
> > +     kfree(caldata);
> > +out:
> > +     return 0;
> > +}
> > +
> > +static int tsens_register(struct tsens_device *tmdev)
> > +{
> > +     struct thermal_zone_device *tzd;
> > +     int i = 0;
> > +
> > +     for (; i < tmdev->chip->sensor_num; i++) {
>
> Ditto
>
> > +             tmdev->sensor[i].tmdev = tmdev;
> > +             tmdev->sensor[i].id = i;
> > +             tmdev->sensor[i].tzd = devm_thermal_zone_of_sensor_register(
> > +                                     tmdev->dev, i, &tmdev->sensor[i],
> > +                                     &tsens_ops);
> > +             if (IS_ERR(tmdev->sensor[i].tzd))
> > +                     return PTR_ERR(tzd);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int tsens_probe(struct platform_device *pdev)
> > +{
> > +     struct tsens_device *tmdev;
> > +     struct device *dev = &pdev->dev;
> > +     int ret;
> > +
> > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > +     if (!tmdev)
> > +             return -ENOMEM;
> > +
> > +     tmdev->dev = dev;
> > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > +     if (!tmdev->chip)
> > +             return -EINVAL;
> > +
> > +     ret = tsens_init(tmdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = tsens_register(tmdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = tmdev->chip->enable(tmdev);
> > +     if (ret)
> > +             return ret;
> >
> > +     platform_set_drvdata(pdev, tmdev);
>
> Your registration should be the very last thing you do. Otherwise, you
> have a small window where the get_temp callback can be called, but the
> driver will not be functional yet.
>
> > +     return ret;
> > +}
> > +
> > +static int tsens_remove(struct platform_device *pdev)
> > +{
> > +     struct tsens_device *tmdev = platform_get_drvdata(pdev);
> > +
> > +     tmdev->chip->disable(tmdev);
> > +
> > +     return 0;
> > +}
> > +
> > +static int sun50i_thermal_enable(struct tsens_device *tmdev)
> > +{
> > +     int ret, val;
> > +
> > +     ret = reset_control_deassert(tmdev->reset);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = clk_prepare_enable(tmdev->bus_clk);
> > +     if (ret)
> > +             goto assert_reset;
>
> This is done by regmap as well
>
> > +     ret = tsens_calibrate(tmdev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * clkin = 24MHz
> > +      * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1)
> > +      *           = 20us
> > +      */
> > +     regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
> > +                  SUN50I_THS_CTRL0_T_ACQ(479));
> > +     /* average over 4 samples */
> > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
> > +                  SUN50I_THS_FILTER_EN |
> > +                  SUN50I_THS_FILTER_TYPE(1));
> > +     /* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */
> > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
> > +                  SUN50I_H6_THS_PC_TEMP_PERIOD(58));
> > +     /* enable sensor */
> > +     val = GENMASK(tmdev->chip->sensor_num - 1, 0);
> > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val);
> > +
> > +     return 0;
> > +
> > +assert_reset:
> > +     reset_control_assert(tmdev->reset);
> > +
> > +     return ret;
>
> Can't we do that with runtime_pm?
>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ondřej Jirman May 19, 2019, 2:22 p.m. UTC | #20

Hello Yangtao,

On Sat, May 18, 2019 at 12:34:57AM +0800, Frank Lee wrote:
> HI,
> 
> On Fri, May 17, 2019 at 2:29 AM Ondřej Jirman <[e-mail]> wrote:
> >
> > Hi Yangtao,
> >
> > thank you for work on this driver.
> >
> > On Fri, May 17, 2019 at 02:06:53AM +0800, Frank Lee wrote:
> > > HI Ondřej,
> > >
> > > On Mon, May 13, 2019 at 6:16 AM Ondřej Jirman <[e-mail]> wrote:
> > > > > +
> > > > > +/* Temp Unit: millidegree Celsius */
> > > > > +static int tsens_reg2temp(struct tsens_device *tmdev,
> > > > > +                           int reg)
> > > >
> > > > Please name all functions so that they are more clearly identifiable
> > > > in stack traces as belonging to this driver. For example:
> > > >
> > > >   sun8i_ths_reg2temp
> > > >
> > > > The same applies for all tsens_* functions below. tsens_* is too
> > > > generic.
> > >
> > > Done but no sun8i_ths_reg2temp.
> > >
> > > ths_reg2tem() should be a generic func.
> > > I think it should be suitable for all platforms， so no platform prefix.
> >
> > You've missed my point. The driver name is sun8i_thermal and if you get
> > and oops from the kernel you'll get a stack trace where there are just function
> > names. If you use too generic function names, it will not be clear which
> > driver is oopsing.
> >
> >   - sun8i_ths_reg2temp will tell you much more clearly where to search than
> >   - ths_reg2temp
> >
> > Of course you can always grep, but most thermal drivers are thermal sensor (ths)
> > drivers, and if multiple of them used this too-generic naming scheme you'd
> > have hard time debugging.
> >
> > Look at other thermal drivers. They usually encode driver name in the function
> > names to help with identification (even if these are static driver-local
> > functions).
> >
> 
> Can we change to sunxi_ths_ prefix?

It should probably match the driver name, but yes, that's better.

> > > > > +static int tsens_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +     struct tsens_device *tmdev;
> > > > > +     struct device *dev = &pdev->dev;
> > > > > +     int ret;
> > > > > +
> > > > > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > > > > +     if (!tmdev)
> > > > > +             return -ENOMEM;
> > > > > +
> > > > > +     tmdev->dev = dev;
> > > > > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > > > > +     if (!tmdev->chip)
> > > > > +             return -EINVAL;
> > > > > +
> > > > > +     ret = tsens_init(tmdev);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     ret = tsens_register(tmdev);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > >
> > > > Why split this out of probe into separate functions?
> > > >
> > > > > +     ret = tmdev->chip->enable(tmdev);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     platform_set_drvdata(pdev, tmdev);
> > > > > +
> > > > > +     return ret;
> > > > > +}
> > > > > +
> > > > > +static int tsens_remove(struct platform_device *pdev)
> > > > > +{
> > > > > +     struct tsens_device *tmdev = platform_get_drvdata(pdev);
> > > > > +
> > > > > +     tmdev->chip->disable(tmdev);
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static int sun50i_thermal_enable(struct tsens_device *tmdev)
> > > > > +{
> > > > > +     int ret, val;
> > > > > +
> > > > > +     ret = reset_control_deassert(tmdev->reset);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     ret = clk_prepare_enable(tmdev->bus_clk);
> > > > > +     if (ret)
> > > > > +             goto assert_reset;
> > > > > +
> > > > > +     ret = tsens_calibrate(tmdev);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > >
> > > > If this fails (it may likely fail with EPROBE_DEFER) you are leaving reset
> > > > deasserted, and clock enabled.
> > > >
> > > > Overall, I think, reset/clock management and nvmem reading will be common
> > > > to all the HW variants, so it doesn't make much sense splitting it out
> > > > of probe into separate functions, and makes it more error prone.
> > >
> > > Our long-term goal is to support all platforms.
> > > Bacicallt there is a differencr between each generation.
> > > So I feel it necessary to isolate these differences.
> > >
> > > Maybe:
> > > At some point, we can draw a part of the public part and platform
> > > difference into different
> > > files. something like qcom thermal driver.
> >
> > I understand, but I wrote ths drivers for H3/H5/A83T and it so far it looks like
> > all of them would share these 3 calls.
> >
> > You'll be enabling clock/reset and callibrating everywhere. So putting this to
> > per-SoC function seems premature.
> 
> In fact, enalbe and disable are the suspend and resume functions.(PM
> callback will be added in the future)
> When exiting from s2ram, the register will become the initial value.
> We need to do all the work, enabling reset/clk ,calibrating and
> initializing other reg.
> 
> So I think it is no need to put enabling reset/clk and calibrating to
> probe func, and I'd like
> to keep enable and disable func.

I know, I don't think it needs to be per-soc. These actions are all shared by
all SoCs. Maybe with an exception that some SoCs may need one more clock, but
that can be made optionally-required by some flag in struct sunxi_thermal_chip.

Only highly SoC specific thing is configuring the THS registers for sampling
frequency/averaging/enabling interrupts. The reset/clock enable is generic, and
already abstracted by the clock/reset framework.

So what I suggest is having:

sunxi_ths_enable()
	reset deassert
	bus clock prepare enable
	optionally module clock prepare enable (in the future)
	call per-soc calibration
	call per-soc setup callback

sunxi_ths_disable()
	reset assert
	bus clock unprepare disable
	optionally module clock unprepare disable

And if you could move devm_nvmem_cell_get to probe that should make per-SoC
calibration callback also less repetitive and could avoid undoing the enable
in case it returns EPROBE_DEFER (which is possible).

All this should make it easier to support PM in the future and add less
cumbersome to add support for A83T and H3/H5.

BTW, what are your plans for more SoC support? I'd like to add support for
A83T and H3/H5, maybe even during the 5.3 cycle if this driver happens to land
early enough. If you don't have any plans I'll take it on.

thank you and regards,
 	o.

> >
> > thank you and regards,
> >         o.
> >
> > > Regards,
> > > Yangtao
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Maxime Ripard May 20, 2019, 10:59 a.m. UTC | #21

On Sun, May 19, 2019 at 04:22:39PM +0200, Ondřej Jirman wrote:
> On Sat, May 18, 2019 at 12:34:57AM +0800, Frank Lee wrote:
> > HI,
> >
> > On Fri, May 17, 2019 at 2:29 AM Ondřej Jirman <[e-mail]> wrote:
> > >
> > > Hi Yangtao,
> > >
> > > thank you for work on this driver.
> > >
> > > On Fri, May 17, 2019 at 02:06:53AM +0800, Frank Lee wrote:
> > > > HI Ondřej,
> > > >
> > > > On Mon, May 13, 2019 at 6:16 AM Ondřej Jirman <[e-mail]> wrote:
> > > > > > +
> > > > > > +/* Temp Unit: millidegree Celsius */
> > > > > > +static int tsens_reg2temp(struct tsens_device *tmdev,
> > > > > > +                           int reg)
> > > > >
> > > > > Please name all functions so that they are more clearly identifiable
> > > > > in stack traces as belonging to this driver. For example:
> > > > >
> > > > >   sun8i_ths_reg2temp
> > > > >
> > > > > The same applies for all tsens_* functions below. tsens_* is too
> > > > > generic.
> > > >
> > > > Done but no sun8i_ths_reg2temp.
> > > >
> > > > ths_reg2tem() should be a generic func.
> > > > I think it should be suitable for all platforms， so no platform prefix.
> > >
> > > You've missed my point. The driver name is sun8i_thermal and if you get
> > > and oops from the kernel you'll get a stack trace where there are just function
> > > names. If you use too generic function names, it will not be clear which
> > > driver is oopsing.
> > >
> > >   - sun8i_ths_reg2temp will tell you much more clearly where to search than
> > >   - ths_reg2temp
> > >
> > > Of course you can always grep, but most thermal drivers are thermal sensor (ths)
> > > drivers, and if multiple of them used this too-generic naming scheme you'd
> > > have hard time debugging.
> > >
> > > Look at other thermal drivers. They usually encode driver name in the function
> > > names to help with identification (even if these are static driver-local
> > > functions).
> > >
> >
> > Can we change to sunxi_ths_ prefix?
>
> It should probably match the driver name, but yes, that's better.

Not really. This driver will not support all the Allwinner devices, so
sunxi is seriously misleading.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Maxime Ripard May 21, 2019, 7:41 a.m. UTC | #22

Hi,

On Sat, May 18, 2019 at 01:19:54AM +0800, Frank Lee wrote:
> On Fri, May 17, 2019 at 3:32 PM Maxime Ripard <[e-mail]> wrote:
> >
> > On Fri, May 17, 2019 at 02:10:47AM +0800, Frank Lee wrote:
> > > > On Sun, May 12, 2019 at 11:41:28PM +0200, Ondřej Jirman wrote:
> > > > > > > +static int tsens_get_temp(void *data, int *temp)
> > > > > > > +{
> > > > > > > + struct tsensor *s = data;
> > > > > > > + struct tsens_device *tmdev = s->tmdev;
> > > > > > > + int val;
> > > > > > > +
> > > > > > > + regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> > > > > > > +             0x4 * s->id, &val);
> > > > > > > +
> > > > > > > + if (unlikely(val == 0))
> > > > > > > +         return -EBUSY;
> > > > > >
> > > > > > I'm not sure why a val equals to 0 would be associated with EBUSY?
> > > > >
> > > > > Thermal zone driver can (will) call get_temp before we got the
> > > > > first interrupt and the thermal data. In that case val will be 0.
> > > > >
> > > > > Resulting in:
> > > > >
> > > > >  (val + offset) * scale = (-2794) * -67 = 187198
> > > > >
> > > > > 187°C and immediate shutdown during boot - based on cirtical
> > > > > temperature being reached.
> > > > >
> > > > > Busy here means, get_temp does not yet have data. Thermal zone
> > > > > driver just reports any error to dmesg output.
> > > >
> > > > Ah, that makes sense.
> > > >
> > > > I guess if we're switching to an interrupt-based driver, then we can
> > > > just use a waitqueue, or is get_temp supposed to be atomic?
> > >
> > > I think get_temp should not be bloacked.
> >
> > Why not?
>
> Maybe, I am wrong. I also want to know if we should do this.

I guess it really all depends on whether you can sleep or not in
get_temps. If you can, then you should wait for the value to be
converted and the THS raising an interrupt.

If you can't, then we should ask what the thermal frameworks expects
in such a case.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Maxime Ripard May 21, 2019, 7:47 a.m. UTC | #23

On Fri, May 17, 2019 at 12:21:57PM -0700, Vasily Khoruzhick wrote:
> On Sun, May 12, 2019 at 6:39 AM Maxime Ripard <[e-mail]> wrote:
> > > +static int tsens_get_temp(void *data, int *temp)
> > > +{
> > > +     struct tsensor *s = data;
> > > +     struct tsens_device *tmdev = s->tmdev;
> > > +     int val;
> > > +
> > > +     regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
> > > +                 0x4 * s->id, &val);
> > > +
> > > +     if (unlikely(val == 0))
> > > +             return -EBUSY;
> >
> > I'm not sure why a val equals to 0 would be associated with EBUSY?
>
> First few reads of temp data return 0, and in case of H6 (and A64) it
> means max temperature, so kernel does emergency shutdown. I used
> -ETIMEDOUT as a workaround in my tree, but it's not appropriate here
> either. Any suggestions are welcome.

If we can use the interrupts and wait for a value to be converted
before we read, then we should do that.

> > Also, it's not in a fast path, so you can drop the unlikely. Chances
> > are it's not that unlikely anyway.
>
> As I said earlier, it's just few samples after start up.

That's not really my point though. unlikely is tricky to get right,
because the compiler has his own meaning of what exactly unlikely
means statistically to be able to do the right branching
optimisations.

However, this particular real case scenario might not have the same
probability, which might result in a poor optimisation choice due to
the unlikely being there.

Moreover, this probability can evolve in the future. For example,
let's assume that we enable dynamic PM in the driver. Starting from
there, most of the reads become "first" reads, and your unlikely case
becomes the likely one.

My point was that, because of this, and because of the fact that it's
really not a hot path, we should just drop it.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Maxime Ripard May 21, 2019, 8:05 a.m. UTC | #24

On Sat, May 18, 2019 at 01:27:39AM +0800, Frank Lee wrote:
> On Fri, May 17, 2019 at 3:36 PM Maxime Ripard <[e-mail]> wrote:
> >
> > On Fri, May 17, 2019 at 01:51:56AM +0800, Frank Lee wrote:
> > > > > +struct sun50i_thermal_chip {
> > > > > +     int     sensor_num;
> > > > > +     int     offset;
> > > > > +     int     scale;
> > > > > +     int     ft_deviation;
> > > > > +     int     temp_calib_base;
> > > > > +     int     temp_data_base;
> > > > > +     int     (*enable)(struct tsens_device *tmdev);
> > > > > +     int     (*disable)(struct tsens_device *tmdev);
> > > > > +};
> > > >
> > > > I'm not super fond of having a lot of quirks that are not needed. If
> > > > we ever need those quirks when adding support for a new SoC, then
> > > > yeah, we should totally have some, but only when and if it's needed.
> > > >
> > > > Otherwise, the driver is more complicated for no particular reason.
> > >
> > > This is unavoidable because of the difference in soc.
> >
> > I know, but this isn't my point.
> >
> > My point is that at this time of the driver development, we don't know
> > what is going to be needed to support all of those SoCs.
> >
> > Some of the parameters you added might not be needed, some parameters
> > might be missing, we don't know. So let's keep it simple for now.
> >
> > > > > +static int tsens_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +     struct tsens_device *tmdev;
> > > > > +     struct device *dev = &pdev->dev;
> > > > > +     int ret;
> > > > > +
> > > > > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > > > > +     if (!tmdev)
> > > > > +             return -ENOMEM;
> > > > > +
> > > > > +     tmdev->dev = dev;
> > > > > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > > > > +     if (!tmdev->chip)
> > > > > +             return -EINVAL;
> > > > > +
> > > > > +     ret = tsens_init(tmdev);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     ret = tsens_register(tmdev);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     ret = tmdev->chip->enable(tmdev);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > >
> > > > > +     platform_set_drvdata(pdev, tmdev);
> > > >
> > > > Your registration should be the very last thing you do. Otherwise, you
> > > > have a small window where the get_temp callback can be called, but the
> > > > driver will not be functional yet.
> > >
> > > No. Anyway, ths data qcquisition is ms level.
> >
> > That's kind of irrelevant. There's nothing preventing get_temp to be
> > called right away.
>
> As Ondřej said,
>
> Registration after enabling will lead to call tz update on non-registered tz
> from an interrupt handler.

I'm probably missing something but you're not using the interrupts, so
how could an interrupt handler call it?

Also, other drivers seem to be doing that just fine (mtk_thermal for
example), so surely there's a way?

> > > > > +     ret = tsens_calibrate(tmdev);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     /*
> > > > > +      * clkin = 24MHz
> > > > > +      * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1)
> > > > > +      *           = 20us
> > > > > +      */
> > > > > +     regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
> > > > > +                  SUN50I_THS_CTRL0_T_ACQ(479));
> > > > > +     /* average over 4 samples */
> > > > > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
> > > > > +                  SUN50I_THS_FILTER_EN |
> > > > > +                  SUN50I_THS_FILTER_TYPE(1));
> > > > > +     /* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */
> > > > > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
> > > > > +                  SUN50I_H6_THS_PC_TEMP_PERIOD(58));
> > > > > +     /* enable sensor */
> > > > > +     val = GENMASK(tmdev->chip->sensor_num - 1, 0);
> > > > > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val);
> > > > > +
> > > > > +     return 0;
> > > > > +
> > > > > +assert_reset:
> > > > > +     reset_control_assert(tmdev->reset);
> > > > > +
> > > > > +     return ret;
> > > >
> > > > Can't we do that with runtime_pm?
> > >
> > > Saving energy doesn't make much sense compared to system security.
> >
> > I'm not sure what you mean by security.
>
> Protect system hardware from damage.

The point of runtime_pm is to keep the device on as long as it is
used, so it wouldn't change anything there.

I mean, you can even enable it in the probe if you want, my point is
that the hooks that you have are exact equivalents to the one provided
by runtime_pm already, so there's no need to define them in the first
place.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Ondřej Jirman May 21, 2019, 10:27 a.m. UTC | #25

Hi Maxime,

On Tue, May 21, 2019 at 10:05:15AM +0200, Maxime Ripard wrote:
> On Sat, May 18, 2019 at 01:27:39AM +0800, Frank Lee wrote:
> > On Fri, May 17, 2019 at 3:36 PM Maxime Ripard <[e-mail]> wrote:
> > >
> > > On Fri, May 17, 2019 at 01:51:56AM +0800, Frank Lee wrote:
> > > > > > +struct sun50i_thermal_chip {
> > > > > > +     int     sensor_num;
> > > > > > +     int     offset;
> > > > > > +     int     scale;
> > > > > > +     int     ft_deviation;
> > > > > > +     int     temp_calib_base;
> > > > > > +     int     temp_data_base;
> > > > > > +     int     (*enable)(struct tsens_device *tmdev);
> > > > > > +     int     (*disable)(struct tsens_device *tmdev);
> > > > > > +};
> > > > >
> > > > > I'm not super fond of having a lot of quirks that are not needed. If
> > > > > we ever need those quirks when adding support for a new SoC, then
> > > > > yeah, we should totally have some, but only when and if it's needed.
> > > > >
> > > > > Otherwise, the driver is more complicated for no particular reason.
> > > >
> > > > This is unavoidable because of the difference in soc.
> > >
> > > I know, but this isn't my point.
> > >
> > > My point is that at this time of the driver development, we don't know
> > > what is going to be needed to support all of those SoCs.
> > >
> > > Some of the parameters you added might not be needed, some parameters
> > > might be missing, we don't know. So let's keep it simple for now.
> > >
> > > > > > +static int tsens_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > +     struct tsens_device *tmdev;
> > > > > > +     struct device *dev = &pdev->dev;
> > > > > > +     int ret;
> > > > > > +
> > > > > > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > > > > > +     if (!tmdev)
> > > > > > +             return -ENOMEM;
> > > > > > +
> > > > > > +     tmdev->dev = dev;
> > > > > > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > > > > > +     if (!tmdev->chip)
> > > > > > +             return -EINVAL;
> > > > > > +
> > > > > > +     ret = tsens_init(tmdev);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     ret = tsens_register(tmdev);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     ret = tmdev->chip->enable(tmdev);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > >
> > > > > > +     platform_set_drvdata(pdev, tmdev);
> > > > >
> > > > > Your registration should be the very last thing you do. Otherwise, you
> > > > > have a small window where the get_temp callback can be called, but the
> > > > > driver will not be functional yet.
> > > >
> > > > No. Anyway, ths data qcquisition is ms level.
> > >
> > > That's kind of irrelevant. There's nothing preventing get_temp to be
> > > called right away.
> >
> > As Ondřej said,
> >
> > Registration after enabling will lead to call tz update on non-registered tz
> > from an interrupt handler.
> 
> I'm probably missing something but you're not using the interrupts, so
> how could an interrupt handler call it?
> 
> Also, other drivers seem to be doing that just fine (mtk_thermal for
> example), so surely there's a way?

Last version is using the interrupts.

Drivers do it in various ways. For example imx_thermal (and others like
hisi_thermal) does it the suggested way. It enables interrupts after thermal
zone registration, so that IRQ handler doesn't get invoked before the tzd is
registered.

regards,
	o.

> > > > > > +     ret = tsens_calibrate(tmdev);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     /*
> > > > > > +      * clkin = 24MHz
> > > > > > +      * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1)
> > > > > > +      *           = 20us
> > > > > > +      */
> > > > > > +     regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
> > > > > > +                  SUN50I_THS_CTRL0_T_ACQ(479));
> > > > > > +     /* average over 4 samples */
> > > > > > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
> > > > > > +                  SUN50I_THS_FILTER_EN |
> > > > > > +                  SUN50I_THS_FILTER_TYPE(1));
> > > > > > +     /* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */
> > > > > > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
> > > > > > +                  SUN50I_H6_THS_PC_TEMP_PERIOD(58));
> > > > > > +     /* enable sensor */
> > > > > > +     val = GENMASK(tmdev->chip->sensor_num - 1, 0);
> > > > > > +     regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val);
> > > > > > +
> > > > > > +     return 0;
> > > > > > +
> > > > > > +assert_reset:
> > > > > > +     reset_control_assert(tmdev->reset);
> > > > > > +
> > > > > > +     return ret;
> > > > >
> > > > > Can't we do that with runtime_pm?
> > > >
> > > > Saving energy doesn't make much sense compared to system security.
> > >
> > > I'm not sure what you mean by security.
> >
> > Protect system hardware from damage.
> 
> The point of runtime_pm is to keep the device on as long as it is
> used, so it wouldn't change anything there.
> 
> I mean, you can even enable it in the probe if you want, my point is
> that the hooks that you have are exact equivalents to the one provided
> by runtime_pm already, so there's no need to define them in the first
> place.
> 
> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Maxime Ripard May 21, 2019, 2:27 p.m. UTC | #26

On Tue, May 21, 2019 at 12:27:21PM +0200, Ondřej Jirman wrote:
> On Tue, May 21, 2019 at 10:05:15AM +0200, Maxime Ripard wrote:
> > On Sat, May 18, 2019 at 01:27:39AM +0800, Frank Lee wrote:
> > > On Fri, May 17, 2019 at 3:36 PM Maxime Ripard <[e-mail]> wrote:
> > > >
> > > > On Fri, May 17, 2019 at 01:51:56AM +0800, Frank Lee wrote:
> > > > > > > +struct sun50i_thermal_chip {
> > > > > > > +     int     sensor_num;
> > > > > > > +     int     offset;
> > > > > > > +     int     scale;
> > > > > > > +     int     ft_deviation;
> > > > > > > +     int     temp_calib_base;
> > > > > > > +     int     temp_data_base;
> > > > > > > +     int     (*enable)(struct tsens_device *tmdev);
> > > > > > > +     int     (*disable)(struct tsens_device *tmdev);
> > > > > > > +};
> > > > > >
> > > > > > I'm not super fond of having a lot of quirks that are not needed. If
> > > > > > we ever need those quirks when adding support for a new SoC, then
> > > > > > yeah, we should totally have some, but only when and if it's needed.
> > > > > >
> > > > > > Otherwise, the driver is more complicated for no particular reason.
> > > > >
> > > > > This is unavoidable because of the difference in soc.
> > > >
> > > > I know, but this isn't my point.
> > > >
> > > > My point is that at this time of the driver development, we don't know
> > > > what is going to be needed to support all of those SoCs.
> > > >
> > > > Some of the parameters you added might not be needed, some parameters
> > > > might be missing, we don't know. So let's keep it simple for now.
> > > >
> > > > > > > +static int tsens_probe(struct platform_device *pdev)
> > > > > > > +{
> > > > > > > +     struct tsens_device *tmdev;
> > > > > > > +     struct device *dev = &pdev->dev;
> > > > > > > +     int ret;
> > > > > > > +
> > > > > > > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > > > > > > +     if (!tmdev)
> > > > > > > +             return -ENOMEM;
> > > > > > > +
> > > > > > > +     tmdev->dev = dev;
> > > > > > > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > > > > > > +     if (!tmdev->chip)
> > > > > > > +             return -EINVAL;
> > > > > > > +
> > > > > > > +     ret = tsens_init(tmdev);
> > > > > > > +     if (ret)
> > > > > > > +             return ret;
> > > > > > > +
> > > > > > > +     ret = tsens_register(tmdev);
> > > > > > > +     if (ret)
> > > > > > > +             return ret;
> > > > > > > +
> > > > > > > +     ret = tmdev->chip->enable(tmdev);
> > > > > > > +     if (ret)
> > > > > > > +             return ret;
> > > > > > >
> > > > > > > +     platform_set_drvdata(pdev, tmdev);
> > > > > >
> > > > > > Your registration should be the very last thing you do. Otherwise, you
> > > > > > have a small window where the get_temp callback can be called, but the
> > > > > > driver will not be functional yet.
> > > > >
> > > > > No. Anyway, ths data qcquisition is ms level.
> > > >
> > > > That's kind of irrelevant. There's nothing preventing get_temp to be
> > > > called right away.
> > >
> > > As Ondřej said,
> > >
> > > Registration after enabling will lead to call tz update on non-registered tz
> > > from an interrupt handler.
> >
> > I'm probably missing something but you're not using the interrupts, so
> > how could an interrupt handler call it?
> >
> > Also, other drivers seem to be doing that just fine (mtk_thermal for
> > example), so surely there's a way?
>
> Last version is using the interrupts.
>
> Drivers do it in various ways. For example imx_thermal (and others like
> hisi_thermal) does it the suggested way. It enables interrupts after thermal
> zone registration, so that IRQ handler doesn't get invoked before the tzd is
> registered.

Enabling the interrupts after the registration makes sense, yes, but
filling the device private pointer with the private structure,
enabling the clocks, setting up the controller and so on can be done
before.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Ondřej Jirman May 21, 2019, 5:33 p.m. UTC | #27

On Tue, May 21, 2019 at 04:27:34PM +0200, Maxime Ripard wrote:
> On Tue, May 21, 2019 at 12:27:21PM +0200, Ondřej Jirman wrote:
> > On Tue, May 21, 2019 at 10:05:15AM +0200, Maxime Ripard wrote:
> > > On Sat, May 18, 2019 at 01:27:39AM +0800, Frank Lee wrote:
> > > > On Fri, May 17, 2019 at 3:36 PM Maxime Ripard <[e-mail]> wrote:
> > > > >
> > > > > On Fri, May 17, 2019 at 01:51:56AM +0800, Frank Lee wrote:
> > > > > > > > +struct sun50i_thermal_chip {
> > > > > > > > +     int     sensor_num;
> > > > > > > > +     int     offset;
> > > > > > > > +     int     scale;
> > > > > > > > +     int     ft_deviation;
> > > > > > > > +     int     temp_calib_base;
> > > > > > > > +     int     temp_data_base;
> > > > > > > > +     int     (*enable)(struct tsens_device *tmdev);
> > > > > > > > +     int     (*disable)(struct tsens_device *tmdev);
> > > > > > > > +};
> > > > > > >
> > > > > > > I'm not super fond of having a lot of quirks that are not needed. If
> > > > > > > we ever need those quirks when adding support for a new SoC, then
> > > > > > > yeah, we should totally have some, but only when and if it's needed.
> > > > > > >
> > > > > > > Otherwise, the driver is more complicated for no particular reason.
> > > > > >
> > > > > > This is unavoidable because of the difference in soc.
> > > > >
> > > > > I know, but this isn't my point.
> > > > >
> > > > > My point is that at this time of the driver development, we don't know
> > > > > what is going to be needed to support all of those SoCs.
> > > > >
> > > > > Some of the parameters you added might not be needed, some parameters
> > > > > might be missing, we don't know. So let's keep it simple for now.
> > > > >
> > > > > > > > +static int tsens_probe(struct platform_device *pdev)
> > > > > > > > +{
> > > > > > > > +     struct tsens_device *tmdev;
> > > > > > > > +     struct device *dev = &pdev->dev;
> > > > > > > > +     int ret;
> > > > > > > > +
> > > > > > > > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > > > > > > > +     if (!tmdev)
> > > > > > > > +             return -ENOMEM;
> > > > > > > > +
> > > > > > > > +     tmdev->dev = dev;
> > > > > > > > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > > > > > > > +     if (!tmdev->chip)
> > > > > > > > +             return -EINVAL;
> > > > > > > > +
> > > > > > > > +     ret = tsens_init(tmdev);
> > > > > > > > +     if (ret)
> > > > > > > > +             return ret;
> > > > > > > > +
> > > > > > > > +     ret = tsens_register(tmdev);
> > > > > > > > +     if (ret)
> > > > > > > > +             return ret;
> > > > > > > > +
> > > > > > > > +     ret = tmdev->chip->enable(tmdev);
> > > > > > > > +     if (ret)
> > > > > > > > +             return ret;
> > > > > > > >
> > > > > > > > +     platform_set_drvdata(pdev, tmdev);
> > > > > > >
> > > > > > > Your registration should be the very last thing you do. Otherwise, you
> > > > > > > have a small window where the get_temp callback can be called, but the
> > > > > > > driver will not be functional yet.
> > > > > >
> > > > > > No. Anyway, ths data qcquisition is ms level.
> > > > >
> > > > > That's kind of irrelevant. There's nothing preventing get_temp to be
> > > > > called right away.
> > > >
> > > > As Ondřej said,
> > > >
> > > > Registration after enabling will lead to call tz update on non-registered tz
> > > > from an interrupt handler.
> > >
> > > I'm probably missing something but you're not using the interrupts, so
> > > how could an interrupt handler call it?
> > >
> > > Also, other drivers seem to be doing that just fine (mtk_thermal for
> > > example), so surely there's a way?
> >
> > Last version is using the interrupts.
> >
> > Drivers do it in various ways. For example imx_thermal (and others like
> > hisi_thermal) does it the suggested way. It enables interrupts after thermal
> > zone registration, so that IRQ handler doesn't get invoked before the tzd is
> > registered.
> 
> Enabling the interrupts after the registration makes sense, yes, but
> filling the device private pointer with the private structure,
> enabling the clocks, setting up the controller and so on can be done
> before.

I agree. 

	o.

> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Yangtao Li May 25, 2019, 6:48 p.m. UTC | #28

HI Ondřej,

On Sun, May 19, 2019 at 10:22 PM Ondřej Jirman <[e-mail]> wrote:
>
> Hello Yangtao,
>
> On Sat, May 18, 2019 at 12:34:57AM +0800, Frank Lee wrote:
> > HI,
> >
> > On Fri, May 17, 2019 at 2:29 AM Ondřej Jirman <[e-mail]> wrote:
> > >
> > > Hi Yangtao,
> > >
> > > thank you for work on this driver.
> > >
> > > On Fri, May 17, 2019 at 02:06:53AM +0800, Frank Lee wrote:
> > > > HI Ondřej,
> > > >
> > > > On Mon, May 13, 2019 at 6:16 AM Ondřej Jirman <[e-mail]> wrote:
> > > > > > +
> > > > > > +/* Temp Unit: millidegree Celsius */
> > > > > > +static int tsens_reg2temp(struct tsens_device *tmdev,
> > > > > > +                           int reg)
> > > > >
> > > > > Please name all functions so that they are more clearly identifiable
> > > > > in stack traces as belonging to this driver. For example:
> > > > >
> > > > >   sun8i_ths_reg2temp
> > > > >
> > > > > The same applies for all tsens_* functions below. tsens_* is too
> > > > > generic.
> > > >
> > > > Done but no sun8i_ths_reg2temp.
> > > >
> > > > ths_reg2tem() should be a generic func.
> > > > I think it should be suitable for all platforms， so no platform prefix.
> > >
> > > You've missed my point. The driver name is sun8i_thermal and if you get
> > > and oops from the kernel you'll get a stack trace where there are just function
> > > names. If you use too generic function names, it will not be clear which
> > > driver is oopsing.
> > >
> > >   - sun8i_ths_reg2temp will tell you much more clearly where to search than
> > >   - ths_reg2temp
> > >
> > > Of course you can always grep, but most thermal drivers are thermal sensor (ths)
> > > drivers, and if multiple of them used this too-generic naming scheme you'd
> > > have hard time debugging.
> > >
> > > Look at other thermal drivers. They usually encode driver name in the function
> > > names to help with identification (even if these are static driver-local
> > > functions).
> > >
> >
> > Can we change to sunxi_ths_ prefix?
>
> It should probably match the driver name, but yes, that's better.
>
> > > > > > +static int tsens_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > +     struct tsens_device *tmdev;
> > > > > > +     struct device *dev = &pdev->dev;
> > > > > > +     int ret;
> > > > > > +
> > > > > > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > > > > > +     if (!tmdev)
> > > > > > +             return -ENOMEM;
> > > > > > +
> > > > > > +     tmdev->dev = dev;
> > > > > > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > > > > > +     if (!tmdev->chip)
> > > > > > +             return -EINVAL;
> > > > > > +
> > > > > > +     ret = tsens_init(tmdev);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     ret = tsens_register(tmdev);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > >
> > > > > Why split this out of probe into separate functions?
> > > > >
> > > > > > +     ret = tmdev->chip->enable(tmdev);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     platform_set_drvdata(pdev, tmdev);
> > > > > > +
> > > > > > +     return ret;
> > > > > > +}
> > > > > > +
> > > > > > +static int tsens_remove(struct platform_device *pdev)
> > > > > > +{
> > > > > > +     struct tsens_device *tmdev = platform_get_drvdata(pdev);
> > > > > > +
> > > > > > +     tmdev->chip->disable(tmdev);
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int sun50i_thermal_enable(struct tsens_device *tmdev)
> > > > > > +{
> > > > > > +     int ret, val;
> > > > > > +
> > > > > > +     ret = reset_control_deassert(tmdev->reset);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     ret = clk_prepare_enable(tmdev->bus_clk);
> > > > > > +     if (ret)
> > > > > > +             goto assert_reset;
> > > > > > +
> > > > > > +     ret = tsens_calibrate(tmdev);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > >
> > > > > If this fails (it may likely fail with EPROBE_DEFER) you are leaving reset
> > > > > deasserted, and clock enabled.
> > > > >
> > > > > Overall, I think, reset/clock management and nvmem reading will be common
> > > > > to all the HW variants, so it doesn't make much sense splitting it out
> > > > > of probe into separate functions, and makes it more error prone.
> > > >
> > > > Our long-term goal is to support all platforms.
> > > > Bacicallt there is a differencr between each generation.
> > > > So I feel it necessary to isolate these differences.
> > > >
> > > > Maybe:
> > > > At some point, we can draw a part of the public part and platform
> > > > difference into different
> > > > files. something like qcom thermal driver.
> > >
> > > I understand, but I wrote ths drivers for H3/H5/A83T and it so far it looks like
> > > all of them would share these 3 calls.
> > >
> > > You'll be enabling clock/reset and callibrating everywhere. So putting this to
> > > per-SoC function seems premature.
> >
> > In fact, enalbe and disable are the suspend and resume functions.(PM
> > callback will be added in the future)
> > When exiting from s2ram, the register will become the initial value.
> > We need to do all the work, enabling reset/clk ,calibrating and
> > initializing other reg.
> >
> > So I think it is no need to put enabling reset/clk and calibrating to
> > probe func, and I'd like
> > to keep enable and disable func.
>
> I know, I don't think it needs to be per-soc. These actions are all shared by
> all SoCs. Maybe with an exception that some SoCs may need one more clock, but
> that can be made optionally-required by some flag in struct sunxi_thermal_chip.
>
> Only highly SoC specific thing is configuring the THS registers for sampling
> frequency/averaging/enabling interrupts. The reset/clock enable is generic, and
> already abstracted by the clock/reset framework.
>
> So what I suggest is having:
>
> sunxi_ths_enable()
>         reset deassert
>         bus clock prepare enable
>         optionally module clock prepare enable (in the future)
>         call per-soc calibration
>         call per-soc setup callback
>
> sunxi_ths_disable()
>         reset assert
>         bus clock unprepare disable
>         optionally module clock unprepare disable
>
> And if you could move devm_nvmem_cell_get to probe that should make per-SoC
> calibration callback also less repetitive and could avoid undoing the enable
> in case it returns EPROBE_DEFER (which is possible).
>
> All this should make it easier to support PM in the future and add less
> cumbersome to add support for A83T and H3/H5.
>
> BTW, what are your plans for more SoC support? I'd like to add support for
> A83T and H3/H5, maybe even during the 5.3 cycle if this driver happens to land
> early enough. If you don't have any plans I'll take it on.
>

I plan to support h3 and a33 later.
Can you support other platforms?

Cheers,
Yangtao

> thank you and regards,
>         o.
>
> > >
> > > thank you and regards,
> > >         o.
> > >
> > > > Regards,
> > > > Yangtao
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Yangtao Li May 25, 2019, 6:51 p.m. UTC | #29

HI,

Following the discussion above, I made some changes.
I think it's time to consider V3 and see what else needs to be modified.

Thx,
Yangtao

Vasily Khoruzhick May 25, 2019, 7:53 p.m. UTC | #30

On Sat, May 25, 2019 at 11:48 AM Frank Lee <[e-mail]> wrote:
>
> HI Ondřej,
>
> On Sun, May 19, 2019 at 10:22 PM Ondřej Jirman <[e-mail]> wrote:
> >
> > Hello Yangtao,
> >
> > On Sat, May 18, 2019 at 12:34:57AM +0800, Frank Lee wrote:
> > > HI,
> > >
> > > On Fri, May 17, 2019 at 2:29 AM Ondřej Jirman <[e-mail]> wrote:
> > > >
> > > > Hi Yangtao,
> > > >
> > > > thank you for work on this driver.
> > > >
> > > > On Fri, May 17, 2019 at 02:06:53AM +0800, Frank Lee wrote:
> > > > > HI Ondřej,
> > > > >
> > > > > On Mon, May 13, 2019 at 6:16 AM Ondřej Jirman <[e-mail]> wrote:
> > > > > > > +
> > > > > > > +/* Temp Unit: millidegree Celsius */
> > > > > > > +static int tsens_reg2temp(struct tsens_device *tmdev,
> > > > > > > +                           int reg)
> > > > > >
> > > > > > Please name all functions so that they are more clearly identifiable
> > > > > > in stack traces as belonging to this driver. For example:
> > > > > >
> > > > > >   sun8i_ths_reg2temp
> > > > > >
> > > > > > The same applies for all tsens_* functions below. tsens_* is too
> > > > > > generic.
> > > > >
> > > > > Done but no sun8i_ths_reg2temp.
> > > > >
> > > > > ths_reg2tem() should be a generic func.
> > > > > I think it should be suitable for all platforms， so no platform prefix.
> > > >
> > > > You've missed my point. The driver name is sun8i_thermal and if you get
> > > > and oops from the kernel you'll get a stack trace where there are just function
> > > > names. If you use too generic function names, it will not be clear which
> > > > driver is oopsing.
> > > >
> > > >   - sun8i_ths_reg2temp will tell you much more clearly where to search than
> > > >   - ths_reg2temp
> > > >
> > > > Of course you can always grep, but most thermal drivers are thermal sensor (ths)
> > > > drivers, and if multiple of them used this too-generic naming scheme you'd
> > > > have hard time debugging.
> > > >
> > > > Look at other thermal drivers. They usually encode driver name in the function
> > > > names to help with identification (even if these are static driver-local
> > > > functions).
> > > >
> > >
> > > Can we change to sunxi_ths_ prefix?
> >
> > It should probably match the driver name, but yes, that's better.
> >
> > > > > > > +static int tsens_probe(struct platform_device *pdev)
> > > > > > > +{
> > > > > > > +     struct tsens_device *tmdev;
> > > > > > > +     struct device *dev = &pdev->dev;
> > > > > > > +     int ret;
> > > > > > > +
> > > > > > > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > > > > > > +     if (!tmdev)
> > > > > > > +             return -ENOMEM;
> > > > > > > +
> > > > > > > +     tmdev->dev = dev;
> > > > > > > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > > > > > > +     if (!tmdev->chip)
> > > > > > > +             return -EINVAL;
> > > > > > > +
> > > > > > > +     ret = tsens_init(tmdev);
> > > > > > > +     if (ret)
> > > > > > > +             return ret;
> > > > > > > +
> > > > > > > +     ret = tsens_register(tmdev);
> > > > > > > +     if (ret)
> > > > > > > +             return ret;
> > > > > >
> > > > > > Why split this out of probe into separate functions?
> > > > > >
> > > > > > > +     ret = tmdev->chip->enable(tmdev);
> > > > > > > +     if (ret)
> > > > > > > +             return ret;
> > > > > > > +
> > > > > > > +     platform_set_drvdata(pdev, tmdev);
> > > > > > > +
> > > > > > > +     return ret;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int tsens_remove(struct platform_device *pdev)
> > > > > > > +{
> > > > > > > +     struct tsens_device *tmdev = platform_get_drvdata(pdev);
> > > > > > > +
> > > > > > > +     tmdev->chip->disable(tmdev);
> > > > > > > +
> > > > > > > +     return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int sun50i_thermal_enable(struct tsens_device *tmdev)
> > > > > > > +{
> > > > > > > +     int ret, val;
> > > > > > > +
> > > > > > > +     ret = reset_control_deassert(tmdev->reset);
> > > > > > > +     if (ret)
> > > > > > > +             return ret;
> > > > > > > +
> > > > > > > +     ret = clk_prepare_enable(tmdev->bus_clk);
> > > > > > > +     if (ret)
> > > > > > > +             goto assert_reset;
> > > > > > > +
> > > > > > > +     ret = tsens_calibrate(tmdev);
> > > > > > > +     if (ret)
> > > > > > > +             return ret;
> > > > > >
> > > > > > If this fails (it may likely fail with EPROBE_DEFER) you are leaving reset
> > > > > > deasserted, and clock enabled.
> > > > > >
> > > > > > Overall, I think, reset/clock management and nvmem reading will be common
> > > > > > to all the HW variants, so it doesn't make much sense splitting it out
> > > > > > of probe into separate functions, and makes it more error prone.
> > > > >
> > > > > Our long-term goal is to support all platforms.
> > > > > Bacicallt there is a differencr between each generation.
> > > > > So I feel it necessary to isolate these differences.
> > > > >
> > > > > Maybe:
> > > > > At some point, we can draw a part of the public part and platform
> > > > > difference into different
> > > > > files. something like qcom thermal driver.
> > > >
> > > > I understand, but I wrote ths drivers for H3/H5/A83T and it so far it looks like
> > > > all of them would share these 3 calls.
> > > >
> > > > You'll be enabling clock/reset and callibrating everywhere. So putting this to
> > > > per-SoC function seems premature.
> > >
> > > In fact, enalbe and disable are the suspend and resume functions.(PM
> > > callback will be added in the future)
> > > When exiting from s2ram, the register will become the initial value.
> > > We need to do all the work, enabling reset/clk ,calibrating and
> > > initializing other reg.
> > >
> > > So I think it is no need to put enabling reset/clk and calibrating to
> > > probe func, and I'd like
> > > to keep enable and disable func.
> >
> > I know, I don't think it needs to be per-soc. These actions are all shared by
> > all SoCs. Maybe with an exception that some SoCs may need one more clock, but
> > that can be made optionally-required by some flag in struct sunxi_thermal_chip.
> >
> > Only highly SoC specific thing is configuring the THS registers for sampling
> > frequency/averaging/enabling interrupts. The reset/clock enable is generic, and
> > already abstracted by the clock/reset framework.
> >
> > So what I suggest is having:
> >
> > sunxi_ths_enable()
> >         reset deassert
> >         bus clock prepare enable
> >         optionally module clock prepare enable (in the future)
> >         call per-soc calibration
> >         call per-soc setup callback
> >
> > sunxi_ths_disable()
> >         reset assert
> >         bus clock unprepare disable
> >         optionally module clock unprepare disable
> >
> > And if you could move devm_nvmem_cell_get to probe that should make per-SoC
> > calibration callback also less repetitive and could avoid undoing the enable
> > in case it returns EPROBE_DEFER (which is possible).
> >
> > All this should make it easier to support PM in the future and add less
> > cumbersome to add support for A83T and H3/H5.
> >
> > BTW, what are your plans for more SoC support? I'd like to add support for
> > A83T and H3/H5, maybe even during the 5.3 cycle if this driver happens to land
> > early enough. If you don't have any plans I'll take it on.
> >
>
> I plan to support h3 and a33 later.
> Can you support other platforms?

I'll work on A64 once this driver lands.

> Cheers,
> Yangtao
>
> > thank you and regards,
> >         o.
> >
> > > >
> > > > thank you and regards,
> > > >         o.
> > > >
> > > > > Regards,
> > > > > Yangtao
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > [e-mail]
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ondřej Jirman May 26, 2019, 1:24 a.m. UTC | #31

Hello Yangtao,

On Sun, May 26, 2019 at 02:48:13AM +0800, Frank Lee wrote:
> On Sun, May 19, 2019 at 10:22 PM Ondřej Jirman <[e-mail]> wrote:
> >
> > I know, I don't think it needs to be per-soc. These actions are all shared by
> > all SoCs. Maybe with an exception that some SoCs may need one more clock, but
> > that can be made optionally-required by some flag in struct sunxi_thermal_chip.
> >
> > Only highly SoC specific thing is configuring the THS registers for sampling
> > frequency/averaging/enabling interrupts. The reset/clock enable is generic, and
> > already abstracted by the clock/reset framework.
> >
> > So what I suggest is having:
> >
> > sunxi_ths_enable()
> >         reset deassert
> >         bus clock prepare enable
> >         optionally module clock prepare enable (in the future)
> >         call per-soc calibration
> >         call per-soc setup callback
> >
> > sunxi_ths_disable()
> >         reset assert
> >         bus clock unprepare disable
> >         optionally module clock unprepare disable
> >
> > And if you could move devm_nvmem_cell_get to probe that should make per-SoC
> > calibration callback also less repetitive and could avoid undoing the enable
> > in case it returns EPROBE_DEFER (which is possible).
> >
> > All this should make it easier to support PM in the future and add less
> > cumbersome to add support for A83T and H3/H5.
> >
> > BTW, what are your plans for more SoC support? I'd like to add support for
> > A83T and H3/H5, maybe even during the 5.3 cycle if this driver happens to land
> > early enough. If you don't have any plans I'll take it on.
> >
> 
> I plan to support h3 and a33 later.
> Can you support other platforms?

Yes, I can do A83T. H5 is similar (the same?) as H3.

thank you and regards,
	o.

> Cheers,
> Yangtao
> 
> > thank you and regards,
> >         o.
> >
> > > >
> > > > thank you and regards,
> > > >         o.
> > > >
> > > > > Regards,
> > > > > Yangtao
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > [e-mail]
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 3c65228e93c5..8da56582e72a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -674,6 +674,13 @@  L:	[e-mail]
 S:	Maintained
 F:	drivers/crypto/sunxi-ss/
 
+ALLWINNER THERMAL DRIVER
+M:	Yangtao Li <[e-mail]>
+L:	[e-mail]
+S:	Maintained
+F:	Documentation/devicetree/bindings/thermal/sun50i-thermal.txt
+F:	drivers/thermal/sun50i_thermal.c
+
 ALLWINNER VPU DRIVER
 M:	Maxime Ripard <[e-mail]>
 M:	Paul Kocialkowski <[e-mail]>
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 653aa27a25a4..2a8d1c98c6ca 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -252,6 +252,20 @@  config SPEAR_THERMAL
 	  Enable this to plug the SPEAr thermal sensor driver into the Linux
 	  thermal framework.
 
+config SUN50I_THERMAL
+	tristate "Allwinner sun50i thermal driver"
+	depends on ARCH_SUNXI || COMPILE_TEST
+	depends on HAS_IOMEM
+	depends on NVMEM
+	depends on OF
+	depends on RESET_CONTROLLER
+	help
+	  Support for the sun50i thermal sensor driver into the Linux thermal
+	  framework.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called sun50i-thermal.
+
 config ROCKCHIP_THERMAL
 	tristate "Rockchip thermal driver"
 	depends on ARCH_ROCKCHIP || COMPILE_TEST
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 486d682be047..a09b30b90003 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -30,6 +30,7 @@  thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o
 # platform thermal drivers
 obj-y				+= broadcom/
 obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
+obj-$(CONFIG_SUN50I_THERMAL)	+= sun50i_thermal.o
 obj-$(CONFIG_ROCKCHIP_THERMAL)	+= rockchip_thermal.o
 obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
 obj-$(CONFIG_RCAR_GEN3_THERMAL)	+= rcar_gen3_thermal.o
diff --git a/drivers/thermal/sun50i_thermal.c b/drivers/thermal/sun50i_thermal.c
new file mode 100644
index 000000000000..3bdb3677b3d4
--- /dev/null
+++ b/drivers/thermal/sun50i_thermal.c
@@ -0,0 +1,357 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Thermal sensor driver for Allwinner SOC
+ * Copyright (C) 2019 Yangtao Li
+ *
+ * Based on the work of Icenowy Zheng <[e-mail]>
+ * Based on the work of Ondrej Jirman <[e-mail]>
+ * Based on the work of Josef Gajdusek <[e-mail]>
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include <linux/thermal.h>
+
+#define MAX_SENSOR_NUM	4
+
+#define FT_TEMP_MASK				GENMASK(11, 0)
+#define TEMP_CALIB_MASK				GENMASK(11, 0)
+#define TEMP_TO_REG				672
+#define CALIBRATE_DEFAULT			0x800
+
+#define SUN50I_THS_CTRL0			0x00
+#define SUN50I_H6_THS_ENABLE			0x04
+#define SUN50I_H6_THS_PC			0x08
+#define SUN50I_H6_THS_MFC			0x30
+#define SUN50I_H6_TEMP_CALIB			0xa0
+#define SUN50I_H6_TEMP_DATA			0xc0
+
+#define SUN50I_THS_CTRL0_T_ACQ(x)		((GENMASK(15, 0) & (x)) << 16)
+#define SUN50I_THS_FILTER_EN			BIT(2)
+#define SUN50I_THS_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
+#define SUN50I_H6_THS_PC_TEMP_PERIOD(x)		((GENMASK(19, 0) & (x)) << 12)
+
+/* millidegree celsius */
+#define SUN50I_H6_FT_DEVIATION			7000
+
+struct tsens_device;
+
+struct tsensor {
+	struct tsens_device		*tmdev;
+	struct thermal_zone_device	*tzd;
+	int				id;
+};
+
+struct sun50i_thermal_chip {
+	int	sensor_num;
+	int	offset;
+	int	scale;
+	int	ft_deviation;
+	int	temp_calib_base;
+	int	temp_data_base;
+	int	(*enable)(struct tsens_device *tmdev);
+	int	(*disable)(struct tsens_device *tmdev);
+};
+
+
+struct tsens_device {
+	const struct sun50i_thermal_chip	*chip;
+	struct device				*dev;
+	struct regmap				*regmap;
+	struct reset_control			*reset;
+	struct clk				*bus_clk;
+	struct tsensor				sensor[MAX_SENSOR_NUM];
+};
+
+/* Temp Unit: millidegree Celsius */
+static int tsens_reg2temp(struct tsens_device *tmdev,
+			      int reg)
+{
+	return (reg + tmdev->chip->offset) * tmdev->chip->scale;
+}
+
+static int tsens_get_temp(void *data, int *temp)
+{
+	struct tsensor *s = data;
+	struct tsens_device *tmdev = s->tmdev;
+	int val;
+
+	regmap_read(tmdev->regmap, tmdev->chip->temp_data_base +
+		    0x4 * s->id, &val);
+
+	if (unlikely(val == 0))
+		return -EBUSY;
+
+	*temp = tsens_reg2temp(tmdev, val);
+	if (tmdev->chip->ft_deviation)
+		*temp += tmdev->chip->ft_deviation;
+
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops tsens_ops = {
+	.get_temp = tsens_get_temp,
+};
+
+static const struct regmap_config config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+};
+
+static int tsens_init(struct tsens_device *tmdev)
+{
+	struct device *dev = tmdev->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct resource *mem;
+	void __iomem *base;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, mem);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	tmdev->regmap = devm_regmap_init_mmio_clk(dev, "bus",
+						  base,
+						  &config);
+	if (IS_ERR(tmdev->regmap))
+		return PTR_ERR(tmdev->regmap);
+
+	tmdev->reset = devm_reset_control_get(dev, "bus");
+	if (IS_ERR(tmdev->reset))
+		return PTR_ERR(tmdev->reset);
+
+	tmdev->bus_clk = devm_clk_get(&pdev->dev, "bus");
+	if (IS_ERR(tmdev->bus_clk))
+		return PTR_ERR(tmdev->bus_clk);
+
+	return 0;
+}
+
+/*
+ * Even if the external calibration data stored in sid is not accessible,
+ * the THS hardware can still work, although the data won't be so accurate.
+ * The default value of calibration register is 0x800 for every sensor,
+ * and the calibration value is usually 0x7xx or 0x8xx, so they won't be
+ * away from the default value for a lot.
+ *
+ * So here we do not return error if the calibartion data is
+ * not available, except the probe needs deferring.
+ */
+static int tsens_calibrate(struct tsens_device *tmdev)
+{
+	struct nvmem_cell *calcell;
+	struct device *dev = tmdev->dev;
+	u16 *caldata;
+	size_t callen;
+	int ft_temp;
+	int i = 0;
+
+	calcell = devm_nvmem_cell_get(dev, "calib");
+	if (IS_ERR(calcell)) {
+		if (PTR_ERR(calcell) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		goto out;
+	}
+
+	caldata = nvmem_cell_read(calcell, &callen);
+	if (IS_ERR(caldata))
+		goto out;
+
+	if (!caldata[0] || callen < 2 + 2 * tmdev->chip->sensor_num)
+		goto out_free;
+
+	/*
+	 * The calbration data on H6 is stored as temperature-value
+	 * pair when being filled at factory test stage.
+	 * The unit of stored FT temperature is 0.1 degreee celusis.
+	 */
+	ft_temp = caldata[0] & FT_TEMP_MASK;
+
+	for (; i < tmdev->chip->sensor_num; i++) {
+		int reg = (int)caldata[i + 1];
+		int sensor_temp = tsens_reg2temp(tmdev, reg);
+		int delta, cdata, calib_offest;
+
+		/*
+		 * To calculate the calibration value:
+		 *
+		 * X(in Celsius) = Ts - ft_temp
+		 * delta = X * 10000 / TEMP_TO_REG
+		 * cdata = CALIBRATE_DEFAULT - delta
+		 *
+		 * cdata: calibration value
+		 */
+		delta = (sensor_temp - ft_temp * 100) * 10 / TEMP_TO_REG;
+		cdata = CALIBRATE_DEFAULT - delta;
+		if (cdata & ~TEMP_CALIB_MASK) {
+			dev_warn(dev, "sensor%d calibration value error", i);
+
+			continue;
+		}
+
+		calib_offest = tmdev->chip->temp_calib_base + (i / 2) * 0x4;
+
+		if (i % 2) {
+			int val;
+
+			regmap_read(tmdev->regmap, calib_offest, &val);
+			val = (val & TEMP_CALIB_MASK) | (cdata << 16);
+			regmap_write(tmdev->regmap, calib_offest, val);
+		} else
+			regmap_write(tmdev->regmap, calib_offest, cdata);
+	}
+
+out_free:
+	kfree(caldata);
+out:
+	return 0;
+}
+
+static int tsens_register(struct tsens_device *tmdev)
+{
+	struct thermal_zone_device *tzd;
+	int i = 0;
+
+	for (; i < tmdev->chip->sensor_num; i++) {
+		tmdev->sensor[i].tmdev = tmdev;
+		tmdev->sensor[i].id = i;
+		tmdev->sensor[i].tzd = devm_thermal_zone_of_sensor_register(
+					tmdev->dev, i, &tmdev->sensor[i],
+					&tsens_ops);
+		if (IS_ERR(tmdev->sensor[i].tzd))
+			return PTR_ERR(tzd);
+	}
+
+	return 0;
+}
+
+static int tsens_probe(struct platform_device *pdev)
+{
+	struct tsens_device *tmdev;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
+	if (!tmdev)
+		return -ENOMEM;
+
+	tmdev->dev = dev;
+	tmdev->chip = of_device_get_match_data(&pdev->dev);
+	if (!tmdev->chip)
+		return -EINVAL;
+
+	ret = tsens_init(tmdev);
+	if (ret)
+		return ret;
+
+	ret = tsens_register(tmdev);
+	if (ret)
+		return ret;
+
+	ret = tmdev->chip->enable(tmdev);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, tmdev);
+
+	return ret;
+}
+
+static int tsens_remove(struct platform_device *pdev)
+{
+	struct tsens_device *tmdev = platform_get_drvdata(pdev);
+
+	tmdev->chip->disable(tmdev);
+
+	return 0;
+}
+
+static int sun50i_thermal_enable(struct tsens_device *tmdev)
+{
+	int ret, val;
+
+	ret = reset_control_deassert(tmdev->reset);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(tmdev->bus_clk);
+	if (ret)
+		goto assert_reset;
+
+	ret = tsens_calibrate(tmdev);
+	if (ret)
+		return ret;
+
+	/*
+	 * clkin = 24MHz
+	 * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1)
+	 *           = 20us
+	 */
+	regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
+		     SUN50I_THS_CTRL0_T_ACQ(479));
+	/* average over 4 samples */
+	regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
+		     SUN50I_THS_FILTER_EN |
+		     SUN50I_THS_FILTER_TYPE(1));
+	/* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */
+	regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
+		     SUN50I_H6_THS_PC_TEMP_PERIOD(58));
+	/* enable sensor */
+	val = GENMASK(tmdev->chip->sensor_num - 1, 0);
+	regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val);
+
+	return 0;
+
+assert_reset:
+	reset_control_assert(tmdev->reset);
+
+	return ret;
+}
+
+static int sun50i_thermal_disable(struct tsens_device *tmdev)
+{
+	clk_disable_unprepare(tmdev->bus_clk);
+	reset_control_assert(tmdev->reset);
+
+	return 0;
+}
+
+static const struct sun50i_thermal_chip sun50i_h6_ths = {
+	.sensor_num = 2,
+	.offset = -2794,
+	.scale = -67,
+	.ft_deviation = SUN50I_H6_FT_DEVIATION,
+	.temp_calib_base = SUN50I_H6_TEMP_CALIB,
+	.temp_data_base = SUN50I_H6_TEMP_DATA,
+	.enable = sun50i_thermal_enable,
+	.disable = sun50i_thermal_disable,
+};
+
+static const struct of_device_id of_tsens_match[] = {
+	{ .compatible = "allwinner,sun50i-h6-ths", .data = &sun50i_h6_ths },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, of_tsens_match);
+
+static struct platform_driver tsens_driver = {
+	.probe = tsens_probe,
+	.remove = tsens_remove,
+	.driver = {
+		.name = "sun50i-thermal",
+		.of_match_table = of_tsens_match,
+	},
+};
+module_platform_driver(tsens_driver);
+
+MODULE_DESCRIPTION("Thermal sensor driver for Allwinner SOC");
+MODULE_LICENSE("GPL v2");