[V8,2/6] thermal: bcm2835: add thermal driver for bcm2835 soc

Commit Message

Martin Sperl Nov. 2, 2016, 10:18 a.m. UTC

From: Martin Sperl <[e-mail]>

Add basic thermal driver for bcm2835 SOC.

This driver currently relies on the firmware setting up the
tsense HW block and does not set it up itself.

Signed-off-by: Martin Sperl <[e-mail]>
Acked-by: Eric Anholt <[e-mail]>
Acked-by: Stefan Wahren <[e-mail]>

ChangeLog:
 V1 -> V2: added specific settings depending on compatiblity
	   added trip point based on register
	   setting up ctrl-register if HW is not enabled by firmware
	     as per recommendation of Eric (untested)
	   check that clock frequency is in range
	     (1.9 - 5MHz - as per comment in clk-bcm2835.c)
 V2 -> V4: moved back to thermal (not using bcm sub-directory)
       	   set polling interval to 1second (was 0ms, so interrupt driven)
 V5 -> V6: added correct depends in KConfig
	   removed defined default for RESET_DELAY
	   removed obvious comments
	   clarify HW setup comments if not set up by FW already
	   move clk_prepare_enable to an earlier stage and add error handling
	   clarify warning when TS-clock runs out of recommended range
	   clk_disable_unprepare added in bcm2835_thermal_remove
	   added comment on recommended temperature ranges for SOC
 V6 -> V7: removed depends on ARCH_BCM2836 || ARCH_BCM2837 in Kconfig
 V7 -> V8: rebased
---
 drivers/thermal/Kconfig           |   8 +
 drivers/thermal/Makefile          |   1 +
 drivers/thermal/bcm2835_thermal.c | 340 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 349 insertions(+)
 create mode 100644 drivers/thermal/bcm2835_thermal.c

--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Zhang, Rui Nov. 15, 2016, 12:29 p.m. UTC | #1

On Wed, 2016-11-02 at 10:18 +0000, [e-mail] wrote:
> From: Martin Sperl <[e-mail]>
> 
> Add basic thermal driver for bcm2835 SOC.
> 
> This driver currently relies on the firmware setting up the
> tsense HW block and does not set it up itself.
> 
> Signed-off-by: Martin Sperl <[e-mail]>
> Acked-by: Eric Anholt <[e-mail]>
> Acked-by: Stefan Wahren <[e-mail]>
> 
Acked-by: Zhang Rui <[e-mail]>

thanks,
rui
> ChangeLog:
>  V1 -> V2: added specific settings depending on compatiblity
> 	   added trip point based on register
> 	   setting up ctrl-register if HW is not enabled by firmware
> 	     as per recommendation of Eric (untested)
> 	   check that clock frequency is in range
> 	     (1.9 - 5MHz - as per comment in clk-bcm2835.c)
>  V2 -> V4: moved back to thermal (not using bcm sub-directory)
>        	   set polling interval to 1second (was 0ms, so
> interrupt driven)
>  V5 -> V6: added correct depends in KConfig
> 	   removed defined default for RESET_DELAY
> 	   removed obvious comments
> 	   clarify HW setup comments if not set up by FW already
> 	   move clk_prepare_enable to an earlier stage and add error
> handling
> 	   clarify warning when TS-clock runs out of recommended range
> 	   clk_disable_unprepare added in bcm2835_thermal_remove
> 	   added comment on recommended temperature ranges for SOC
>  V6 -> V7: removed depends on ARCH_BCM2836 || ARCH_BCM2837 in Kconfig
>  V7 -> V8: rebased
> ---
>  drivers/thermal/Kconfig           |   8 +
>  drivers/thermal/Makefile          |   1 +
>  drivers/thermal/bcm2835_thermal.c | 340
> ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 349 insertions(+)
>  create mode 100644 drivers/thermal/bcm2835_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index a13541b..ab946ff 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -434,4 +434,12 @@ depends on (ARCH_QCOM && OF) || COMPILE_TEST
>  source "drivers/thermal/qcom/Kconfig"
>  endmenu
> 
> +config BCM2835_THERMAL
> +	tristate "Thermal sensors on bcm2835 SoC"
> +	depends on ARCH_BCM2835 || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	depends on OF
> +	help
> +	  Support for thermal sensors on Broadcom bcm2835 SoCs.
> +
>  endif
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index c92eb22..a10ebe0 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -55,3 +55,4 @@ obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra/
>  obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
>  obj-$(CONFIG_MTK_THERMAL)	+= mtk_thermal.o
>  obj-$(CONFIG_GENERIC_ADC_THERMAL)	+= thermal-generic-adc.o
> +obj-$(CONFIG_BCM2835_THERMAL)	+= bcm2835_thermal.o
> diff --git a/drivers/thermal/bcm2835_thermal.c
> b/drivers/thermal/bcm2835_thermal.c
> new file mode 100644
> index 0000000..3468c7b
> --- /dev/null
> +++ b/drivers/thermal/bcm2835_thermal.c
> @@ -0,0 +1,340 @@
> +/*
> + * Driver for Broadcom BCM2835 soc temperature sensor
> + *
> + * Copyright (C) 2016 Martin Sperl
> + *
> + * This program is free software; you can redistribute it and/or
> modify
> + * it under the terms of the GNU General Public License as published
> by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/thermal.h>
> +
> +#define BCM2835_TS_TSENSCTL			0x00
> +#define BCM2835_TS_TSENSSTAT			0x04
> +
> +#define BCM2835_TS_TSENSCTL_PRWDW		BIT(0)
> +#define BCM2835_TS_TSENSCTL_RSTB		BIT(1)
> +#define BCM2835_TS_TSENSCTL_CTRL_BITS		3
> +#define BCM2835_TS_TSENSCTL_CTRL_SHIFT		2
> +#define BCM2835_TS_TSENSCTL_CTRL_MASK		    \
> +	GENMASK(BCM2835_TS_TSENSCTL_CTRL_BITS +     \
> +		BCM2835_TS_TSENSCTL_CTRL_SHIFT - 1, \
> +		BCM2835_TS_TSENSCTL_CTRL_SHIFT)
> +#define BCM2835_TS_TSENSCTL_CTRL_DEFAULT	1
> +#define BCM2835_TS_TSENSCTL_EN_INT		BIT(5)
> +#define BCM2835_TS_TSENSCTL_DIRECT		BIT(6)
> +#define BCM2835_TS_TSENSCTL_CLR_INT		BIT(7)
> +#define BCM2835_TS_TSENSCTL_THOLD_SHIFT		8
> +#define BCM2835_TS_TSENSCTL_THOLD_BITS		10
> +#define BCM2835_TS_TSENSCTL_THOLD_MASK		     \
> +	GENMASK(BCM2835_TS_TSENSCTL_THOLD_BITS +     \
> +		BCM2835_TS_TSENSCTL_THOLD_SHIFT - 1, \
> +		BCM2835_TS_TSENSCTL_THOLD_SHIFT)
> +#define BCM2835_TS_TSENSCTL_RSTDELAY_SHIFT	18
> +#define BCM2835_TS_TSENSCTL_RSTDELAY_BITS	8
> +#define BCM2835_TS_TSENSCTL_REGULEN		BIT(26)
> +
> +#define BCM2835_TS_TSENSSTAT_DATA_BITS		10
> +#define BCM2835_TS_TSENSSTAT_DATA_SHIFT		0
> +#define BCM2835_TS_TSENSSTAT_DATA_MASK		     \
> +	GENMASK(BCM2835_TS_TSENSSTAT_DATA_BITS +     \
> +		BCM2835_TS_TSENSSTAT_DATA_SHIFT - 1, \
> +		BCM2835_TS_TSENSSTAT_DATA_SHIFT)
> +#define BCM2835_TS_TSENSSTAT_VALID		BIT(10)
> +#define BCM2835_TS_TSENSSTAT_INTERRUPT		BIT(11)
> +
> +struct bcm2835_thermal_info {
> +	int offset;
> +	int slope;
> +	int trip_temp;
> +};
> +
> +struct bcm2835_thermal_data {
> +	const struct bcm2835_thermal_info *info;
> +	void __iomem *regs;
> +	struct clk *clk;
> +	struct dentry *debugfsdir;
> +};
> +
> +static int bcm2835_thermal_adc2temp(
> +	const struct bcm2835_thermal_info *info, u32 adc)
> +{
> +	return info->offset + (adc * info->slope);
> +}
> +
> +static int bcm2835_thermal_temp2adc(
> +	const struct bcm2835_thermal_info *info, int temp)
> +{
> +	temp -= info->offset;
> +	temp /= info->slope;
> +
> +	if (temp < 0)
> +		temp = 0;
> +	if (temp >= BIT(BCM2835_TS_TSENSSTAT_DATA_BITS))
> +		temp = BIT(BCM2835_TS_TSENSSTAT_DATA_BITS) - 1;
> +
> +	return temp;
> +}
> +
> +static int bcm2835_thermal_get_trip_type(
> +	struct thermal_zone_device *tz, int trip,
> +	enum thermal_trip_type *type)
> +{
> +	*type = THERMAL_TRIP_CRITICAL;
> +	return 0;
> +}
> +
> +static int bcm2835_thermal_get_trip_temp(
> +	struct thermal_zone_device *tz, int trip, int *temp)
> +{
> +	struct bcm2835_thermal_data *data = tz->devdata;
> +	u32 val = readl(data->regs + BCM2835_TS_TSENSCTL);
> +
> +	/* get the THOLD bits */
> +	val &= BCM2835_TS_TSENSCTL_THOLD_MASK;
> +	val >>= BCM2835_TS_TSENSCTL_THOLD_SHIFT;
> +
> +	/* if it is zero then use the info value */
> +	if (val)
> +		*temp = bcm2835_thermal_adc2temp(data->info, val);
> +	else
> +		*temp = data->info->trip_temp;
> +
> +	return 0;
> +}
> +
> +static int bcm2835_thermal_get_temp(struct thermal_zone_device *tz,
> +				    int *temp)
> +{
> +	struct bcm2835_thermal_data *data = tz->devdata;
> +	u32 val = readl(data->regs + BCM2835_TS_TSENSSTAT);
> +
> +	if (!(val & BCM2835_TS_TSENSSTAT_VALID))
> +		return -EIO;
> +
> +	val &= BCM2835_TS_TSENSSTAT_DATA_MASK;
> +
> +	*temp = bcm2835_thermal_adc2temp(data->info, val);
> +
> +	return 0;
> +}
> +
> +static const struct debugfs_reg32 bcm2835_thermal_regs[] = {
> +	{
> +		.name = "ctl",
> +		.offset = 0
> +	},
> +	{
> +		.name = "stat",
> +		.offset = 4
> +	}
> +};
> +
> +static void bcm2835_thermal_debugfs(struct platform_device *pdev)
> +{
> +	struct thermal_zone_device *tz = platform_get_drvdata(pdev);
> +	struct bcm2835_thermal_data *data = tz->devdata;
> +	struct debugfs_regset32 *regset;
> +
> +	data->debugfsdir = debugfs_create_dir("bcm2835_thermal",
> NULL);
> +	if (!data->debugfsdir)
> +		return;
> +
> +	regset = devm_kzalloc(&pdev->dev, sizeof(*regset),
> GFP_KERNEL);
> +	if (!regset)
> +		return;
> +
> +	regset->regs = bcm2835_thermal_regs;
> +	regset->nregs = ARRAY_SIZE(bcm2835_thermal_regs);
> +	regset->base = data->regs;
> +
> +	debugfs_create_regset32("regset", S_IRUGO,
> +				data->debugfsdir, regset);
> +}
> +
> +static struct thermal_zone_device_ops bcm2835_thermal_ops  = {
> +	.get_temp = bcm2835_thermal_get_temp,
> +	.get_trip_temp = bcm2835_thermal_get_trip_temp,
> +	.get_trip_type = bcm2835_thermal_get_trip_type,
> +};
> +
> +static const struct of_device_id bcm2835_thermal_of_match_table[];
> +static int bcm2835_thermal_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	struct thermal_zone_device *tz;
> +	struct bcm2835_thermal_data *data;
> +	struct resource *res;
> +	int err;
> +	u32 val;
> +	unsigned long rate;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	match = of_match_device(bcm2835_thermal_of_match_table,
> +				&pdev->dev);
> +	if (!match)
> +		return -EINVAL;
> +	data->info = match->data;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(data->regs)) {
> +		err = PTR_ERR(data->regs);
> +		dev_err(&pdev->dev, "Could not get registers: %d\n",
> err);
> +		return err;
> +	}
> +
> +	data->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(data->clk)) {
> +		err = PTR_ERR(data->clk);
> +		if (err != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "Could not get clk:
> %d\n", err);
> +		return err;
> +	}
> +
> +	err = clk_prepare_enable(data->clk);
> +	if (err)
> +		return err;
> +
> +	rate = clk_get_rate(data->clk);
> +	if ((rate < 1920000) || (rate > 5000000))
> +		dev_warn(&pdev->dev,
> +			 "Clock %pCn running at %pCr Hz is outside
> of the recommended range: 1.92 to 5MHz\n",
> +			 data->clk, data->clk);
> +
> +	/*
> +	 * right now the FW does set up the HW-block, so we are not
> +	 * touching the configuration registers.
> +	 * But if the HW is not enabled, then set it up
> +	 * using "sane" values used by the firmware right now.
> +	 */
> +	val = readl(data->regs + BCM2835_TS_TSENSCTL);
> +	if (!(val & BCM2835_TS_TSENSCTL_RSTB)) {
> +		/* the basic required flags */
> +		val = (BCM2835_TS_TSENSCTL_CTRL_DEFAULT <<
> +		       BCM2835_TS_TSENSCTL_CTRL_SHIFT) |
> +		      BCM2835_TS_TSENSCTL_REGULEN;
> +
> +		/*
> +		 * reset delay using the current firmware value of
> 14
> +		 * - units of time are unknown.
> +		 */
> +		val |= (14 << BCM2835_TS_TSENSCTL_RSTDELAY_SHIFT);
> +
> +		/*  trip_adc value from info */
> +		val |= bcm2835_thermal_temp2adc(data->info,
> +						data->info-
> >trip_temp) <<
> +			BCM2835_TS_TSENSCTL_THOLD_SHIFT;
> +
> +		/* write the value back to the register as 2 steps
> */
> +		writel(val, data->regs + BCM2835_TS_TSENSCTL);
> +		val |= BCM2835_TS_TSENSCTL_RSTB;
> +		writel(val, data->regs + BCM2835_TS_TSENSCTL);
> +	}
> +
> +	/* register thermal zone with 1 trip point an 1s polling */
> +	tz = thermal_zone_device_register("bcm2835_thermal",
> +					  1, 0, data,
> +					  &bcm2835_thermal_ops,
> +					  NULL,
> +					  0, 1000);
> +	if (IS_ERR(tz)) {
> +		clk_disable_unprepare(data->clk);
> +		err = PTR_ERR(tz);
> +		dev_err(&pdev->dev,
> +			"Failed to register the thermal device:
> %d\n",
> +			err);
> +		return err;
> +	}
> +
> +	platform_set_drvdata(pdev, tz);
> +
> +	bcm2835_thermal_debugfs(pdev);
> +
> +	return 0;
> +}
> +
> +static int bcm2835_thermal_remove(struct platform_device *pdev)
> +{
> +	struct thermal_zone_device *tz = platform_get_drvdata(pdev);
> +	struct bcm2835_thermal_data *data = tz->devdata;
> +
> +	debugfs_remove_recursive(data->debugfsdir);
> +	thermal_zone_device_unregister(tz);
> +	clk_disable_unprepare(data->clk);
> +
> +	return 0;
> +}
> +
> +/*
> + * Note: as per Raspberry Foundation FAQ
> + * (https://www.raspberrypi.org/help/faqs/#performanceOperatingTempe
> rature)
> + * the recommended temperature range for the SOC -40C to +85C
> + * so the trip limit is set to 80C.
> + * this applies to all the BCM283X SOC
> + */
> +
> +static const struct of_device_id bcm2835_thermal_of_match_table[] =
> {
> +	{
> +		.compatible = "brcm,bcm2835-thermal",
> +		.data = &(struct bcm2835_thermal_info) {
> +			.offset = 407000,
> +			.slope = -538,
> +			.trip_temp = 80000
> +		}
> +	},
> +	{
> +		.compatible = "brcm,bcm2836-thermal",
> +		.data = &(struct bcm2835_thermal_info) {
> +			.offset = 407000,
> +			.slope = -538,
> +			.trip_temp = 80000
> +		}
> +	},
> +	{
> +		.compatible = "brcm,bcm2837-thermal",
> +		.data = &(struct bcm2835_thermal_info) {
> +			/* the bcm2837 needs adjustment of +5C */
> +			.offset = 407000 + 5000,
> +			.slope = -538,
> +			.trip_temp = 80000
> +		}
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, bcm2835_thermal_of_match_table);
> +
> +static struct platform_driver bcm2835_thermal_driver = {
> +	.probe = bcm2835_thermal_probe,
> +	.remove = bcm2835_thermal_remove,
> +	.driver = {
> +		.name = "bcm2835_thermal",
> +		.of_match_table = bcm2835_thermal_of_match_table,
> +	},
> +};
> +module_platform_driver(bcm2835_thermal_driver);
> +
> +MODULE_AUTHOR("Martin Sperl");
> +MODULE_DESCRIPTION("Thermal driver for bcm2835 chip");
> +MODULE_LICENSE("GPL");
> --
> 2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eduardo Valentin Nov. 17, 2016, 2:11 a.m. UTC | #2

Hey Martin,

Very sorry for the late feedback. Not so sure if this one got queued
already or not. Anyways, just minor questions as follows:

On Wed, Nov 02, 2016 at 10:18:22AM +0000, [e-mail] wrote:
> From: Martin Sperl <[e-mail]>
> 
> Add basic thermal driver for bcm2835 SOC.
> 
> This driver currently relies on the firmware setting up the
> tsense HW block and does not set it up itself.
> 
> Signed-off-by: Martin Sperl <[e-mail]>
> Acked-by: Eric Anholt <[e-mail]>
> Acked-by: Stefan Wahren <[e-mail]>
> 
> ChangeLog:
>  V1 -> V2: added specific settings depending on compatiblity
> 	   added trip point based on register
> 	   setting up ctrl-register if HW is not enabled by firmware
> 	     as per recommendation of Eric (untested)
> 	   check that clock frequency is in range
> 	     (1.9 - 5MHz - as per comment in clk-bcm2835.c)
>  V2 -> V4: moved back to thermal (not using bcm sub-directory)
>        	   set polling interval to 1second (was 0ms, so interrupt driven)
>  V5 -> V6: added correct depends in KConfig
> 	   removed defined default for RESET_DELAY
> 	   removed obvious comments
> 	   clarify HW setup comments if not set up by FW already
> 	   move clk_prepare_enable to an earlier stage and add error handling
> 	   clarify warning when TS-clock runs out of recommended range
> 	   clk_disable_unprepare added in bcm2835_thermal_remove
> 	   added comment on recommended temperature ranges for SOC
>  V6 -> V7: removed depends on ARCH_BCM2836 || ARCH_BCM2837 in Kconfig
>  V7 -> V8: rebased
> ---
>  drivers/thermal/Kconfig           |   8 +
>  drivers/thermal/Makefile          |   1 +
>  drivers/thermal/bcm2835_thermal.c | 340 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 349 insertions(+)
>  create mode 100644 drivers/thermal/bcm2835_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index a13541b..ab946ff 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -434,4 +434,12 @@ depends on (ARCH_QCOM && OF) || COMPILE_TEST
>  source "drivers/thermal/qcom/Kconfig"
>  endmenu
> 
> +config BCM2835_THERMAL
> +	tristate "Thermal sensors on bcm2835 SoC"
> +	depends on ARCH_BCM2835 || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	depends on OF
> +	help
> +	  Support for thermal sensors on Broadcom bcm2835 SoCs.
> +
>  endif
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index c92eb22..a10ebe0 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -55,3 +55,4 @@ obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra/
>  obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
>  obj-$(CONFIG_MTK_THERMAL)	+= mtk_thermal.o
>  obj-$(CONFIG_GENERIC_ADC_THERMAL)	+= thermal-generic-adc.o
> +obj-$(CONFIG_BCM2835_THERMAL)	+= bcm2835_thermal.o
> diff --git a/drivers/thermal/bcm2835_thermal.c b/drivers/thermal/bcm2835_thermal.c
> new file mode 100644
> index 0000000..3468c7b
> --- /dev/null
> +++ b/drivers/thermal/bcm2835_thermal.c
> @@ -0,0 +1,340 @@
> +/*
> + * Driver for Broadcom BCM2835 soc temperature sensor
> + *
> + * Copyright (C) 2016 Martin Sperl
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/thermal.h>
> +
> +#define BCM2835_TS_TSENSCTL			0x00
> +#define BCM2835_TS_TSENSSTAT			0x04
> +
> +#define BCM2835_TS_TSENSCTL_PRWDW		BIT(0)
> +#define BCM2835_TS_TSENSCTL_RSTB		BIT(1)
> +#define BCM2835_TS_TSENSCTL_CTRL_BITS		3
> +#define BCM2835_TS_TSENSCTL_CTRL_SHIFT		2
> +#define BCM2835_TS_TSENSCTL_CTRL_MASK		    \
> +	GENMASK(BCM2835_TS_TSENSCTL_CTRL_BITS +     \
> +		BCM2835_TS_TSENSCTL_CTRL_SHIFT - 1, \
> +		BCM2835_TS_TSENSCTL_CTRL_SHIFT)
> +#define BCM2835_TS_TSENSCTL_CTRL_DEFAULT	1
> +#define BCM2835_TS_TSENSCTL_EN_INT		BIT(5)
> +#define BCM2835_TS_TSENSCTL_DIRECT		BIT(6)
> +#define BCM2835_TS_TSENSCTL_CLR_INT		BIT(7)
> +#define BCM2835_TS_TSENSCTL_THOLD_SHIFT		8
> +#define BCM2835_TS_TSENSCTL_THOLD_BITS		10
> +#define BCM2835_TS_TSENSCTL_THOLD_MASK		     \
> +	GENMASK(BCM2835_TS_TSENSCTL_THOLD_BITS +     \
> +		BCM2835_TS_TSENSCTL_THOLD_SHIFT - 1, \
> +		BCM2835_TS_TSENSCTL_THOLD_SHIFT)
> +#define BCM2835_TS_TSENSCTL_RSTDELAY_SHIFT	18
> +#define BCM2835_TS_TSENSCTL_RSTDELAY_BITS	8
> +#define BCM2835_TS_TSENSCTL_REGULEN		BIT(26)
> +
> +#define BCM2835_TS_TSENSSTAT_DATA_BITS		10
> +#define BCM2835_TS_TSENSSTAT_DATA_SHIFT		0
> +#define BCM2835_TS_TSENSSTAT_DATA_MASK		     \
> +	GENMASK(BCM2835_TS_TSENSSTAT_DATA_BITS +     \
> +		BCM2835_TS_TSENSSTAT_DATA_SHIFT - 1, \
> +		BCM2835_TS_TSENSSTAT_DATA_SHIFT)
> +#define BCM2835_TS_TSENSSTAT_VALID		BIT(10)
> +#define BCM2835_TS_TSENSSTAT_INTERRUPT		BIT(11)
> +
> +struct bcm2835_thermal_info {
> +	int offset;
> +	int slope;
> +	int trip_temp;
> +};
> +
> +struct bcm2835_thermal_data {
> +	const struct bcm2835_thermal_info *info;
> +	void __iomem *regs;
> +	struct clk *clk;
> +	struct dentry *debugfsdir;
> +};
> +
> +static int bcm2835_thermal_adc2temp(
> +	const struct bcm2835_thermal_info *info, u32 adc)
> +{
> +	return info->offset + (adc * info->slope);

Any specific reason we cannot use thermal_zone_params->slope and
thermal_zone_params->offset?

> +}
> +
> +static int bcm2835_thermal_temp2adc(
> +	const struct bcm2835_thermal_info *info, int temp)
> +{
> +	temp -= info->offset;
> +	temp /= info->slope;
> +
> +	if (temp < 0)
> +		temp = 0;
> +	if (temp >= BIT(BCM2835_TS_TSENSSTAT_DATA_BITS))
> +		temp = BIT(BCM2835_TS_TSENSSTAT_DATA_BITS) - 1;
> +
> +	return temp;
> +}
> +
> +static int bcm2835_thermal_get_trip_type(
> +	struct thermal_zone_device *tz, int trip,
> +	enum thermal_trip_type *type)
> +{
> +	*type = THERMAL_TRIP_CRITICAL;
> +	return 0;
> +}
> +
> +static int bcm2835_thermal_get_trip_temp(
> +	struct thermal_zone_device *tz, int trip, int *temp)
> +{
> +	struct bcm2835_thermal_data *data = tz->devdata;
> +	u32 val = readl(data->regs + BCM2835_TS_TSENSCTL);
> +
> +	/* get the THOLD bits */
> +	val &= BCM2835_TS_TSENSCTL_THOLD_MASK;
> +	val >>= BCM2835_TS_TSENSCTL_THOLD_SHIFT;
> +
> +	/* if it is zero then use the info value */
> +	if (val)

Is this a read only register or is this driver supposed to program it?
In which scenario it would be 0? Can this be added as comments?

> +		*temp = bcm2835_thermal_adc2temp(data->info, val);
> +	else
> +		*temp = data->info->trip_temp;
> +
> +	return 0;
> +}
> +
> +static int bcm2835_thermal_get_temp(struct thermal_zone_device *tz,
> +				    int *temp)
> +{
> +	struct bcm2835_thermal_data *data = tz->devdata;
> +	u32 val = readl(data->regs + BCM2835_TS_TSENSSTAT);
> +
> +	if (!(val & BCM2835_TS_TSENSSTAT_VALID))

What cases you would get the valid bit not set? Do you need to wait for
the conversion to finish?

> +		return -EIO;
> +
> +	val &= BCM2835_TS_TSENSSTAT_DATA_MASK;
> +
> +	*temp = bcm2835_thermal_adc2temp(data->info, val);
> +
> +	return 0;
> +}
> +
> +static const struct debugfs_reg32 bcm2835_thermal_regs[] = {
> +	{
> +		.name = "ctl",
> +		.offset = 0
> +	},
> +	{
> +		.name = "stat",
> +		.offset = 4
> +	}
> +};
> +
> +static void bcm2835_thermal_debugfs(struct platform_device *pdev)
> +{
> +	struct thermal_zone_device *tz = platform_get_drvdata(pdev);
> +	struct bcm2835_thermal_data *data = tz->devdata;
> +	struct debugfs_regset32 *regset;
> +
> +	data->debugfsdir = debugfs_create_dir("bcm2835_thermal", NULL);
> +	if (!data->debugfsdir)
> +		return;
> +
> +	regset = devm_kzalloc(&pdev->dev, sizeof(*regset), GFP_KERNEL);
> +	if (!regset)
> +		return;
> +
> +	regset->regs = bcm2835_thermal_regs;
> +	regset->nregs = ARRAY_SIZE(bcm2835_thermal_regs);
> +	regset->base = data->regs;
> +
> +	debugfs_create_regset32("regset", S_IRUGO,
> +				data->debugfsdir, regset);
> +}
> +
> +static struct thermal_zone_device_ops bcm2835_thermal_ops  = {
> +	.get_temp = bcm2835_thermal_get_temp,
> +	.get_trip_temp = bcm2835_thermal_get_trip_temp,
> +	.get_trip_type = bcm2835_thermal_get_trip_type,
> +};
> +
> +static const struct of_device_id bcm2835_thermal_of_match_table[];
> +static int bcm2835_thermal_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	struct thermal_zone_device *tz;
> +	struct bcm2835_thermal_data *data;
> +	struct resource *res;
> +	int err;
> +	u32 val;
> +	unsigned long rate;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	match = of_match_device(bcm2835_thermal_of_match_table,
> +				&pdev->dev);
> +	if (!match)
> +		return -EINVAL;
> +	data->info = match->data;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(data->regs)) {
> +		err = PTR_ERR(data->regs);
> +		dev_err(&pdev->dev, "Could not get registers: %d\n", err);
> +		return err;
> +	}
> +
> +	data->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(data->clk)) {
> +		err = PTR_ERR(data->clk);
> +		if (err != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "Could not get clk: %d\n", err);
> +		return err;
> +	}
> +
> +	err = clk_prepare_enable(data->clk);
> +	if (err)
> +		return err;
> +
> +	rate = clk_get_rate(data->clk);
> +	if ((rate < 1920000) || (rate > 5000000))
> +		dev_warn(&pdev->dev,
> +			 "Clock %pCn running at %pCr Hz is outside of the recommended range: 1.92 to 5MHz\n",
> +			 data->clk, data->clk);
> +
> +	/*
> +	 * right now the FW does set up the HW-block, so we are not
> +	 * touching the configuration registers.
> +	 * But if the HW is not enabled, then set it up
> +	 * using "sane" values used by the firmware right now.
> +	 */
> +	val = readl(data->regs + BCM2835_TS_TSENSCTL);
> +	if (!(val & BCM2835_TS_TSENSCTL_RSTB)) {
> +		/* the basic required flags */
> +		val = (BCM2835_TS_TSENSCTL_CTRL_DEFAULT <<
> +		       BCM2835_TS_TSENSCTL_CTRL_SHIFT) |
> +		      BCM2835_TS_TSENSCTL_REGULEN;
> +
> +		/*
> +		 * reset delay using the current firmware value of 14
> +		 * - units of time are unknown.
> +		 */
> +		val |= (14 << BCM2835_TS_TSENSCTL_RSTDELAY_SHIFT);
> +
> +		/*  trip_adc value from info */
> +		val |= bcm2835_thermal_temp2adc(data->info,
> +						data->info->trip_temp) <<
> +			BCM2835_TS_TSENSCTL_THOLD_SHIFT;
> +
> +		/* write the value back to the register as 2 steps */
> +		writel(val, data->regs + BCM2835_TS_TSENSCTL);
> +		val |= BCM2835_TS_TSENSCTL_RSTB;
> +		writel(val, data->regs + BCM2835_TS_TSENSCTL);
> +	}
> +
> +	/* register thermal zone with 1 trip point an 1s polling */
> +	tz = thermal_zone_device_register("bcm2835_thermal",
> +					  1, 0, data,
> +					  &bcm2835_thermal_ops,
> +					  NULL,
> +					  0, 1000);
> +	if (IS_ERR(tz)) {
> +		clk_disable_unprepare(data->clk);
> +		err = PTR_ERR(tz);
> +		dev_err(&pdev->dev,
> +			"Failed to register the thermal device: %d\n",
> +			err);
> +		return err;
> +	}
> +
> +	platform_set_drvdata(pdev, tz);
> +
> +	bcm2835_thermal_debugfs(pdev);
> +
> +	return 0;
> +}
> +
> +static int bcm2835_thermal_remove(struct platform_device *pdev)
> +{
> +	struct thermal_zone_device *tz = platform_get_drvdata(pdev);
> +	struct bcm2835_thermal_data *data = tz->devdata;
> +
> +	debugfs_remove_recursive(data->debugfsdir);
> +	thermal_zone_device_unregister(tz);
> +	clk_disable_unprepare(data->clk);
> +
> +	return 0;
> +}
> +
> +/*
> + * Note: as per Raspberry Foundation FAQ
> + * (https://www.raspberrypi.org/help/faqs/#performanceOperatingTemperature)
> + * the recommended temperature range for the SOC -40C to +85C
> + * so the trip limit is set to 80C.
> + * this applies to all the BCM283X SOC
> + */
> +
> +static const struct of_device_id bcm2835_thermal_of_match_table[] = {
> +	{
> +		.compatible = "brcm,bcm2835-thermal",
> +		.data = &(struct bcm2835_thermal_info) {
> +			.offset = 407000,
> +			.slope = -538,
> +			.trip_temp = 80000
> +		}
> +	},
> +	{
> +		.compatible = "brcm,bcm2836-thermal",
> +		.data = &(struct bcm2835_thermal_info) {
> +			.offset = 407000,
> +			.slope = -538,
> +			.trip_temp = 80000
> +		}
> +	},
> +	{
> +		.compatible = "brcm,bcm2837-thermal",
> +		.data = &(struct bcm2835_thermal_info) {
> +			/* the bcm2837 needs adjustment of +5C */
> +			.offset = 407000 + 5000,
> +			.slope = -538,
> +			.trip_temp = 80000
> +		}
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, bcm2835_thermal_of_match_table);
> +
> +static struct platform_driver bcm2835_thermal_driver = {
> +	.probe = bcm2835_thermal_probe,
> +	.remove = bcm2835_thermal_remove,
> +	.driver = {
> +		.name = "bcm2835_thermal",
> +		.of_match_table = bcm2835_thermal_of_match_table,
> +	},
> +};
> +module_platform_driver(bcm2835_thermal_driver);
> +
> +MODULE_AUTHOR("Martin Sperl");
> +MODULE_DESCRIPTION("Thermal driver for bcm2835 chip");
> +MODULE_LICENSE("GPL");
> --
> 2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Martin Sperl Nov. 17, 2016, 9:51 a.m. UTC | #3

On 17.11.2016 03:11, Eduardo Valentin wrote:
> Hey Martin,
>
> Very sorry for the late feedback. Not so sure if this one got queued
> already or not. Anyways, just minor questions as follows:
>
> On Wed, Nov 02, 2016 at 10:18:22AM +0000, [e-mail] wrote:
>> From: Martin Sperl <[e-mail]>
>>
>> Add basic thermal driver for bcm2835 SOC.
>>
>> This driver currently relies on the firmware setting up the
>> tsense HW block and does not set it up itself.
>>
>> Signed-off-by: Martin Sperl <[e-mail]>
>> Acked-by: Eric Anholt <[e-mail]>
>> Acked-by: Stefan Wahren <[e-mail]>
>>
...
>> +static int bcm2835_thermal_adc2temp(
>> +	const struct bcm2835_thermal_info *info, u32 adc)
>> +{
>> +	return info->offset + (adc * info->slope);
>
> Any specific reason we cannot use thermal_zone_params->slope and
> thermal_zone_params->offset?

You could - the patch was just rebased to 4.9 and those slope and
offset just got merged during this cycle.

Do we really need to modify it - the patch has been around since 4.6.

>> +
>> +static int bcm2835_thermal_get_trip_temp(
>> +	struct thermal_zone_device *tz, int trip, int *temp)
>> +{
>> +	struct bcm2835_thermal_data *data = tz->devdata;
>> +	u32 val = readl(data->regs + BCM2835_TS_TSENSCTL);
>> +
>> +	/* get the THOLD bits */
>> +	val &= BCM2835_TS_TSENSCTL_THOLD_MASK;
>> +	val >>= BCM2835_TS_TSENSCTL_THOLD_SHIFT;
>> +
>> +	/* if it is zero then use the info value */
>> +	if (val)
>
> Is this a read only register or is this driver supposed to program it?
> In which scenario it would be 0? Can this be added as comments?

It is RW, but the Firmware typically sets up the thermal device with the 
correct values already - this is just a fallback.

>> +static int bcm2835_thermal_get_temp(struct thermal_zone_device *tz,
>> +				    int *temp)
>> +{
>> +	struct bcm2835_thermal_data *data = tz->devdata;
>> +	u32 val = readl(data->regs + BCM2835_TS_TSENSSTAT);
>> +
>> +	if (!(val & BCM2835_TS_TSENSSTAT_VALID))
>
> What cases you would get the valid bit not set? Do you need to wait for
> the conversion to finish?

I guess: if you have just enabled the HW-block (which the FW does much
in advance) and start to read the value immediately (before the first 
sample period has finished), then this will not be valid.

So do you need another version of the patchset that uses that new API?

Thanks,
	Martin
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eduardo Valentin Nov. 17, 2016, 3:10 p.m. UTC | #4

Hello Martin,

On Thu, Nov 17, 2016 at 10:51:33AM +0100, Martin Sperl wrote:
> 
> 
> On 17.11.2016 03:11, Eduardo Valentin wrote:
> >Hey Martin,
> >
> >Very sorry for the late feedback. Not so sure if this one got queued
> >already or not. Anyways, just minor questions as follows:
> >
> >On Wed, Nov 02, 2016 at 10:18:22AM +0000, [e-mail] wrote:
> >>From: Martin Sperl <[e-mail]>
> >>
> >>Add basic thermal driver for bcm2835 SOC.
> >>
> >>This driver currently relies on the firmware setting up the
> >>tsense HW block and does not set it up itself.
> >>
> >>Signed-off-by: Martin Sperl <[e-mail]>
> >>Acked-by: Eric Anholt <[e-mail]>
> >>Acked-by: Stefan Wahren <[e-mail]>
> >>
> ...
> >>+static int bcm2835_thermal_adc2temp(
> >>+	const struct bcm2835_thermal_info *info, u32 adc)
> >>+{
> >>+	return info->offset + (adc * info->slope);
> >
> >Any specific reason we cannot use thermal_zone_params->slope and
> >thermal_zone_params->offset?
> 
> You could - the patch was just rebased to 4.9 and those slope and
> offset just got merged during this cycle.
> 
> Do we really need to modify it - the patch has been around since 4.6.
> 
> >>+
> >>+static int bcm2835_thermal_get_trip_temp(
> >>+	struct thermal_zone_device *tz, int trip, int *temp)
> >>+{
> >>+	struct bcm2835_thermal_data *data = tz->devdata;
> >>+	u32 val = readl(data->regs + BCM2835_TS_TSENSCTL);
> >>+
> >>+	/* get the THOLD bits */
> >>+	val &= BCM2835_TS_TSENSCTL_THOLD_MASK;
> >>+	val >>= BCM2835_TS_TSENSCTL_THOLD_SHIFT;
> >>+
> >>+	/* if it is zero then use the info value */
> >>+	if (val)
> >
> >Is this a read only register or is this driver supposed to program it?
> >In which scenario it would be 0? Can this be added as comments?
> 
> It is RW, but the Firmware typically sets up the thermal device with the
> correct values already - this is just a fallback.

OK, but how do you differentiate from a buggy firmware or misconfigured
hardware? How do you know if the firmware is supposed to be loaded and
ready? There is no firmware loading in this driver. Also, there is no
dependency with a driver that loads firmware, or at least, I missed it.

> 
> >>+static int bcm2835_thermal_get_temp(struct thermal_zone_device *tz,
> >>+				    int *temp)
> >>+{
> >>+	struct bcm2835_thermal_data *data = tz->devdata;
> >>+	u32 val = readl(data->regs + BCM2835_TS_TSENSSTAT);
> >>+
> >>+	if (!(val & BCM2835_TS_TSENSSTAT_VALID))
> >
> >What cases you would get the valid bit not set? Do you need to wait for
> >the conversion to finish?
> 
> I guess: if you have just enabled the HW-block (which the FW does much
> in advance) and start to read the value immediately (before the first sample
> period has finished), then this will not be valid.
> 

Then again, how does this driver make sure the initialization procedure
is correct, and that by the time it is using the hardware, the hardware
is in a sane state?

Back to the original question, does it mean the hardware is in
some sort of continuous ADC conversion mode or reading the temperature
requires specific steps to get the conversion done, and therefore you
are checking the valid bit here?



> So do you need another version of the patchset that uses that new API?

I think the API usage is change that can be done together with
clarification for the above questions too: on hardware state,
firmware loading, maybe a master driver dependency, and the ADC
conversion sequence, which are not well clear to me on this driver. As long as
this is clarified and documented in the code (can be simple comments so
it is clear to whoever reads in the future), then I would be OK with
this driver. 

> 
> Thanks,
> 	Martin
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Martin Sperl Nov. 18, 2016, 8:32 a.m. UTC | #5

> On 17.11.2016, at 16:10, Eduardo Valentin <[e-mail]> wrote:
> 
> Hello Martin,
> 
> On Thu, Nov 17, 2016 at 10:51:33AM +0100, Martin Sperl wrote:
>> 
>> 
>> On 17.11.2016 03:11, Eduardo Valentin wrote:
>>> Hey Martin,
>>> 
>>> Very sorry for the late feedback. Not so sure if this one got queued
>>> already or not. Anyways, just minor questions as follows:
>>> 
>>> On Wed, Nov 02, 2016 at 10:18:22AM +0000, [e-mail] wrote:
>>>> From: Martin Sperl <[e-mail]>
>>>> 
>>>> Add basic thermal driver for bcm2835 SOC.
>>>> 
>>>> This driver currently relies on the firmware setting up the
>>>> tsense HW block and does not set it up itself.
>>>> 
>>>> Signed-off-by: Martin Sperl <[e-mail]>
>>>> Acked-by: Eric Anholt <[e-mail]>
>>>> Acked-by: Stefan Wahren <[e-mail]>
>>>> 
>> ...
>>>> +static int bcm2835_thermal_adc2temp(
>>>> +	const struct bcm2835_thermal_info *info, u32 adc)
>>>> +{
>>>> +	return info->offset + (adc * info->slope);
>>> 
>>> Any specific reason we cannot use thermal_zone_params->slope and
>>> thermal_zone_params->offset?
>> 
>> You could - the patch was just rebased to 4.9 and those slope and
>> offset just got merged during this cycle.
>> 
>> Do we really need to modify it - the patch has been around since 4.6.
>> 
>>>> +
>>>> +static int bcm2835_thermal_get_trip_temp(
>>>> +	struct thermal_zone_device *tz, int trip, int *temp)
>>>> +{
>>>> +	struct bcm2835_thermal_data *data = tz->devdata;
>>>> +	u32 val = readl(data->regs + BCM2835_TS_TSENSCTL);
>>>> +
>>>> +	/* get the THOLD bits */
>>>> +	val &= BCM2835_TS_TSENSCTL_THOLD_MASK;
>>>> +	val >>= BCM2835_TS_TSENSCTL_THOLD_SHIFT;
>>>> +
>>>> +	/* if it is zero then use the info value */
>>>> +	if (val)
>>> 
>>> Is this a read only register or is this driver supposed to program it?
>>> In which scenario it would be 0? Can this be added as comments?
>> 
>> It is RW, but the Firmware typically sets up the thermal device with the
>> correct values already - this is just a fallback.
> 
> OK, but how do you differentiate from a buggy firmware or misconfigured
> hardware? How do you know if the firmware is supposed to be loaded and
> ready? There is no firmware loading in this driver. Also, there is no
> dependency with a driver that loads firmware, or at least, I missed it.
> 
The way that firmware works on the RPI is quite different from most others I guess.
in principle you got 2 different CPUs on the bcm2835:
* ARM, which runs the linux instance
* VideoCore 4, which runs the firmware (loading from SD initially) and
  then booting the ARM.

So this Firmware on VC4 is the one that I am talking about.
Without the working firmware linux can not boot on arm.
As of now he firmware is also checking temperature of the SOC itself and
if it finds “critical” temperatures it will clock down ARM and the GPU.

Hope that this explains the “firmware” on bcm2835.

>> 
>>>> +static int bcm2835_thermal_get_temp(struct thermal_zone_device *tz,
>>>> +				    int *temp)
>>>> +{
>>>> +	struct bcm2835_thermal_data *data = tz->devdata;
>>>> +	u32 val = readl(data->regs + BCM2835_TS_TSENSSTAT);
>>>> +
>>>> +	if (!(val & BCM2835_TS_TSENSSTAT_VALID))
>>> 
>>> What cases you would get the valid bit not set? Do you need to wait for
>>> the conversion to finish?
>> 
>> I guess: if you have just enabled the HW-block (which the FW does much
>> in advance) and start to read the value immediately (before the first sample
>> period has finished), then this will not be valid.
>> 
> 
> Then again, how does this driver make sure the initialization procedure
> is correct, and that by the time it is using the hardware, the hardware
> is in a sane state?
> 
> Back to the original question, does it mean the hardware is in
> some sort of continuous ADC conversion mode or reading the temperature
> requires specific steps to get the conversion done, and therefore you
> are checking the valid bit here?

As far as I understand the conversion is continuous (as soon as the HW is
configured). This case is there primarily to handle the situation where
we initialize the HW ourselves (see line 226 and below), and we immediately
want to read the ADC value before the first conversion is finished.

The above mentioned “configuration if not running” reflect the values that
the FW is currently setting. We should not change those values as long as the
Firmware is also reading the temperature on its own.

> 
>> So do you need another version of the patchset that uses that new API?
> 
> I think the API usage is change that can be done together with
> clarification for the above questions too: on hardware state,
> firmware loading, maybe a master driver dependency, and the ADC
> conversion sequence, which are not well clear to me on this driver. As long as
> this is clarified and documented in the code (can be simple comments so
> it is clear to whoever reads in the future), then I would be OK with
> this driver. 

So how do you want this to get “documented” in the driver?
The setup and Firmware is a generic feature of the SOC, so if we would put
some clarifications in this driver, then we would need to put it in every
bcm283X driver (which seems unreasonable).

Thanks,
	Martin--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eduardo Valentin Nov. 19, 2016, 4:22 a.m. UTC | #6

Hello Martin,

Thanks for your patience to take the time to explain to me how the
firmware/linux split is done in your platform. Still, one thing is not
clear to me.

On Fri, Nov 18, 2016 at 09:32:47AM +0100, [e-mail] wrote:
> 

> The way that firmware works on the RPI is quite different from most others I guess.
> in principle you got 2 different CPUs on the bcm2835:
> * ARM, which runs the linux instance
> * VideoCore 4, which runs the firmware (loading from SD initially) and
>   then booting the ARM.
> 
> So this Firmware on VC4 is the one that I am talking about.
> Without the working firmware linux can not boot on arm.

Given that "without the working firmware linux can not boot on arm",

(...)

> As far as I understand the conversion is continuous (as soon as the HW is
> configured). This case is there primarily to handle the situation where
> we initialize the HW ourselves (see line 226 and below), and we immediately

and around line 226 we have the comment:
+       /*
+        * right now the FW does set up the HW-block, so we are not
+        * touching the configuration registers.
+        * But if the HW is not enabled, then set it up
+        * using "sane" values used by the firmware right now.
+        */


> want to read the ADC value before the first conversion is finished.
> 

then, does the firmware initializes the device or not?

What are the cases you would load this driver but still get an
uninitialized device? That looks like some bug workaround hidden
somewhere. Do system integrators/engineers need to be aware of this w/a?
Would the driver work right aways when the subsystem is loaded during
boot? How about module insertion?


Who has the ownership of this device?

> The above mentioned “configuration if not running” reflect the values that
> the FW is currently setting. We should not change those values as long as the
> Firmware is also reading the temperature on its own.

hmm.. that looks like racy to me. Again, How do you synchronize accesses to
this device? What if you configure the device and right after the
firmware updates the configs? How do you make sure the configs you are
writing here are the same used by the firmware? What if the firmware
version changes? What versions of the firmware does this driver support?

Would it make sense to simply always initialize the device? Do you have
a way to tell the firmware that it should not use the device?

Or, if you want to keep the device driver simply being a dummy reader,
would it make sense to simply avoid writing configurations to the
device, and simply retry to check if the firmware gets the device
initialized?

> 
> > 
> >> So do you need another version of the patchset that uses that new API?
> > 
> > I think the API usage is change that can be done together with
> > clarification for the above questions too: on hardware state,
> > firmware loading, maybe a master driver dependency, and the ADC
> > conversion sequence, which are not well clear to me on this driver. As long as
> > this is clarified and documented in the code (can be simple comments so
> > it is clear to whoever reads in the future), then I would be OK with
> > this driver. 
> 
> So how do you want this to get “documented” in the driver?
> The setup and Firmware is a generic feature of the SOC, so if we would put
> some clarifications in this driver, then we would need to put it in every
> bcm283X driver (which seems unreasonable).
> 

I think a simple comment explaining the firmware dependency and the
expected pre-conditions to get this driver working in a sane state would
do it.

A better device initialization would also be appreciated. Based on my
limited understanding of this platform, and your explanations, this
device seams to have a serious race condition with firmware while
accessing this device.

> Thanks,
> 	Martin
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Martin Sperl Nov. 22, 2016, 2:28 p.m. UTC | #7

Hi Eduardo!

On 19.11.2016 05:22, Eduardo Valentin wrote:
> Hello Martin,
> 
> Thanks for your patience to take the time to explain to me how the
> firmware/linux split is done in your platform. Still, one thing is not
> clear to me.
> 
> On Fri, Nov 18, 2016 at 09:32:47AM +0100, [e-mail] wrote:
>> 
> 
>> The way that firmware works on the RPI is quite different from most others I guess.
>> in principle you got 2 different CPUs on the bcm2835:
>> * ARM, which runs the linux instance
>> * VideoCore 4, which runs the firmware (loading from SD initially) and
>>  then booting the ARM.
>> 
>> So this Firmware on VC4 is the one that I am talking about.
>> Without the working firmware linux can not boot on arm.
> 
> Given that "without the working firmware linux can not boot on arm",
> 
> (...)
> 
>> As far as I understand the conversion is continuous (as soon as the HW is
>> configured). This case is there primarily to handle the situation where
>> we initialize the HW ourselves (see line 226 and below), and we immediately
> 
> and around line 226 we have the comment:
> +       /*
> +        * right now the FW does set up the HW-block, so we are not
> +        * touching the configuration registers.
> +        * But if the HW is not enabled, then set it up
> +        * using "sane" values used by the firmware right now.
> +        */
> 
> 
>> want to read the ADC value before the first conversion is finished.
>> 
> 
> then, does the firmware initializes the device or not?
> 

Yes, it does (normally)

> What are the cases you would load this driver but still get an
> uninitialized device? That looks like some bug workaround hidden
> somewhere. Do system integrators/engineers need to be aware of this w/a?
> Would the driver work right aways when the subsystem is loaded during
> boot? How about module insertion?
> 

I was asked to implement the "initialize" case just in case FW ever
stopped setting up the device itself, so that is why this code is
included.

> 
> Who has the ownership of this device?

Joined ownership I suppose...

> 
>> The above mentioned “configuration if not running” reflect the values that
>> the FW is currently setting. We should not change those values as long as the
>> Firmware is also reading the temperature on its own.
> 
> hmm.. that looks like racy to me. Again, How do you synchronize accesses to
> this device? What if you configure the device and right after the
> firmware updates the configs? How do you make sure the configs you are
> writing here are the same used by the firmware? What if the firmware
> version changes? What versions of the firmware does this driver support?
> 
> Would it make sense to simply always initialize the device? Do you have
> a way to tell the firmware that it should not use the device?
> 
> Or, if you want to keep the device driver simply being a dummy reader,
> would it make sense to simply avoid writing configurations to the
> device, and simply retry to check if the firmware gets the device
> initialized?

Again: the device registers are only ever written if the device is not started
already. Otherwise the driver only reads for the ADC register, so there
is no real race here.

> 
>> 
>>> 
>>>> So do you need another version of the patchset that uses that new API?
>>> 
>>> I think the API usage is change that can be done together with
>>> clarification for the above questions too: on hardware state,
>>> firmware loading, maybe a master driver dependency, and the ADC
>>> conversion sequence, which are not well clear to me on this driver. As long as
>>> this is clarified and documented in the code (can be simple comments so
>>> it is clear to whoever reads in the future), then I would be OK with
>>> this driver.
>> 
>> So how do you want this to get “documented” in the driver?
>> The setup and Firmware is a generic feature of the SOC, so if we would put
>> some clarifications in this driver, then we would need to put it in every
>> bcm283X driver (which seems unreasonable).
>> 
> 
> I think a simple comment explaining the firmware dependency and the
> expected pre-conditions to get this driver working in a sane state would
> do it.
> 
> A better device initialization would also be appreciated. Based on my
> limited understanding of this platform, and your explanations, this
> device seams to have a serious race condition with firmware while
> accessing this device.

Again: the firmware runs before the ARM is started and for all practical
purposes the firmware is (as of now) configuring the thermal device.

As for the use of thermal_zone_get_offset/slope: looking into the code
it looks like this actually blows up the code, as we now would need to
allocate thermal_zone_params and preset it with the “correct” values.

So more code to maintain and more memory consumed.
The only advantage I would see is that it would allow to set offset and
slope directly in the device tree.

Thanks,
		Martin--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eduardo Valentin Nov. 25, 2016, 5:20 a.m. UTC | #8

Hello,

On Tue, Nov 22, 2016 at 03:28:04PM +0100, Martin Sperl wrote:
> Hi Eduardo!
> 
> On 19.11.2016 05:22, Eduardo Valentin wrote:
> > Hello Martin,

<cut>

> 
> I was asked to implement the "initialize" case just in case FW ever
> stopped setting up the device itself, so that is why this code is
> included.

OK. Looks like we (like, we in the Linux side) do not understand the
conditions that firmware fails to initialize the thermal device, but we
still want to force an initialization, hoping to get the device in a sane
state, even if we do not know if the firmware is correctly booted or
not. And that is done silently, with no notification to user. I see.

> 
> > 
> > Who has the ownership of this device?
> 
> Joined ownership I suppose...
> 

with no synchronization mechanism?

> > 
> >> The above mentioned “configuration if not running” reflect the values that
> >> the FW is currently setting. We should not change those values as long as the
> >> Firmware is also reading the temperature on its own.
> > 
> > hmm.. that looks like racy to me. Again, How do you synchronize accesses to
> > this device? What if you configure the device and right after the
> > firmware updates the configs? How do you make sure the configs you are
> > writing here are the same used by the firmware? What if the firmware
> > version changes? What versions of the firmware does this driver support?
> > 
> > Would it make sense to simply always initialize the device? Do you have
> > a way to tell the firmware that it should not use the device?
> > 
> > Or, if you want to keep the device driver simply being a dummy reader,
> > would it make sense to simply avoid writing configurations to the
> > device, and simply retry to check if the firmware gets the device
> > initialized?
> 
> Again: the device registers are only ever written if the device is not started
> already. Otherwise the driver only reads for the ADC register, so there
> is no real race here.
> 

and no race?

To me, there is a race when you write to the config of this device,
given that there is no sync between the two. We do not know if the
firmware would be still attempting to initialize the device or not, do
we? 

> > 
> >> 
> >>> 
> >>>> So do you need another version of the patchset that uses that new API?
> >>> 
> >>> I think the API usage is change that can be done together with
> >>> clarification for the above questions too: on hardware state,
> >>> firmware loading, maybe a master driver dependency, and the ADC
> >>> conversion sequence, which are not well clear to me on this driver. As long as
> >>> this is clarified and documented in the code (can be simple comments so
> >>> it is clear to whoever reads in the future), then I would be OK with
> >>> this driver.
> >> 
> >> So how do you want this to get “documented” in the driver?
> >> The setup and Firmware is a generic feature of the SOC, so if we would put
> >> some clarifications in this driver, then we would need to put it in every
> >> bcm283X driver (which seems unreasonable).
> >> 
> > 
> > I think a simple comment explaining the firmware dependency and the
> > expected pre-conditions to get this driver working in a sane state would
> > do it.
> > 
> > A better device initialization would also be appreciated. Based on my
> > limited understanding of this platform, and your explanations, this
> > device seams to have a serious race condition with firmware while
> > accessing this device.
> 
> Again: the firmware runs before the ARM is started and for all practical
> purposes the firmware is (as of now) configuring the thermal device.
> 

Oh, OK, we know the firmware wont touch the thermal device, right?

Then, If that is the case, what is still not clear is in which conditions
the firmware would leave the thermal device uninitialized.

This driver seams to be only concerned of critical temperature. The
concern I see with this code, and the explanations I heard, is a side
effect of seeing spurious thermal shutdown, or even worse, getting the
device booted hot and never shutting down, due to (possible) race
between the two systems.

Besides, if the firmware would not touch the thermal device when ARM is
running, why not always writing to the device a configuration that you
know is your sane starting point? Just do not rely on what was
previously set. How do you know it is a sane configuration? Have you
done sensor calibration exercise to confirm temperature reads are
correct?

> As for the use of thermal_zone_get_offset/slope: looking into the code
> it looks like this actually blows up the code, as we now would need to
> allocate thermal_zone_params and preset it with the “correct” values.
> 
> So more code to maintain and more memory consumed.
> The only advantage I would see is that it would allow to set offset and
> slope directly in the device tree.
> 

Any specific reason not to use of-thermal, then?


> Thanks,
> 		Martin

BR,

Eduardo Valentin

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eric Anholt Nov. 28, 2016, 8:30 p.m. UTC | #9

Eduardo Valentin <[e-mail]> writes:

> Hello,
>
> On Tue, Nov 22, 2016 at 03:28:04PM +0100, Martin Sperl wrote:
>> Hi Eduardo!
>> 
>> On 19.11.2016 05:22, Eduardo Valentin wrote:
>> > Hello Martin,
>
> <cut>
>
>> 
>> I was asked to implement the "initialize" case just in case FW ever
>> stopped setting up the device itself, so that is why this code is
>> included.
>
> OK. Looks like we (like, we in the Linux side) do not understand the
> conditions that firmware fails to initialize the thermal device, but we
> still want to force an initialization, hoping to get the device in a sane
> state, even if we do not know if the firmware is correctly booted or
> not. And that is done silently, with no notification to user. I see.

The firmware today always initializes thermal.  I suggested adding the
init code because we (myself and the Pi Foundation) would like to reduce
how much closed firmware code is required in the platform, and the Linux
driver doing this would help make that possible in the future.

>> > Who has the ownership of this device?
>> 
>> Joined ownership I suppose...
>> 
>
> with no synchronization mechanism?

Correct, because none is necessary.

>> >> The above mentioned “configuration if not running” reflect the values that
>> >> the FW is currently setting. We should not change those values as long as the
>> >> Firmware is also reading the temperature on its own.
>> > 
>> > hmm.. that looks like racy to me. Again, How do you synchronize accesses to
>> > this device? What if you configure the device and right after the
>> > firmware updates the configs? How do you make sure the configs you are
>> > writing here are the same used by the firmware? What if the firmware
>> > version changes? What versions of the firmware does this driver support?
>> > 
>> > Would it make sense to simply always initialize the device? Do you have
>> > a way to tell the firmware that it should not use the device?
>> > 
>> > Or, if you want to keep the device driver simply being a dummy reader,
>> > would it make sense to simply avoid writing configurations to the
>> > device, and simply retry to check if the firmware gets the device
>> > initialized?
>> 
>> Again: the device registers are only ever written if the device is not started
>> already. Otherwise the driver only reads for the ADC register, so there
>> is no real race here.
>> 
>
> and no race?
>
> To me, there is a race when you write to the config of this device,
> given that there is no sync between the two. We do not know if the
> firmware would be still attempting to initialize the device or not, do
> we? 

Either the device was initialized by the firmware before handing off to
ARM (today's firmware) or it never will be (potential future firmware).

Eduardo Valentin Nov. 29, 2016, 1:34 a.m. UTC | #10

Hello Eric, Martin,

On Mon, Nov 28, 2016 at 12:30:38PM -0800, Eric Anholt wrote:
> Eduardo Valentin <[e-mail]> writes:
> 

<cut> 

> The firmware today always initializes thermal.  I suggested adding the
> init code because we (myself and the Pi Foundation) would like to reduce
> how much closed firmware code is required in the platform, and the Linux
> driver doing this would help make that possible in the future.

Oh I see. Backup code for future chips/firmware.

> 
> >> > Who has the ownership of this device?
> >> 
> >> Joined ownership I suppose...
> >> 
> >
> > with no synchronization mechanism?
> 
> Correct, because none is necessary.
> 



<cut>

> 
> Either the device was initialized by the firmware before handing off to
> ARM (today's firmware) or it never will be (potential future firmware).

And do you have a way to check if the firmware has the initialization
code or not? By firmware version, for example. Or even, chip version,
maybe?

If the current firmware will always initialize the chip, I would say,
ARM should simply read the registers, no initialization, unless it is
known that the firmware intentionally left the device uninitialized.

Again, just trying to avoid obscure misbehavior, when running into a
faulty state, and running silently broken.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eric Anholt Nov. 29, 2016, 10:12 p.m. UTC | #11

Eduardo Valentin <[e-mail]> writes:

> Hello Eric, Martin,
>
> On Mon, Nov 28, 2016 at 12:30:38PM -0800, Eric Anholt wrote:
>> Either the device was initialized by the firmware before handing off to
>> ARM (today's firmware) or it never will be (potential future firmware).
>
> And do you have a way to check if the firmware has the initialization
> code or not? By firmware version, for example. Or even, chip version,
> maybe?

We would know if it's not present because the register would be in its
power-on reset state, which is what the code is checking for.

Eduardo Valentin Nov. 30, 2016, 6:39 a.m. UTC | #12

Hello,

On Tue, Nov 29, 2016 at 02:12:46PM -0800, Eric Anholt wrote:
> Eduardo Valentin <[e-mail]> writes:
> 
> > Hello Eric, Martin,
> >
> > On Mon, Nov 28, 2016 at 12:30:38PM -0800, Eric Anholt wrote:
> >> Either the device was initialized by the firmware before handing off to
> >> ARM (today's firmware) or it never will be (potential future firmware).
> >
> > And do you have a way to check if the firmware has the initialization
> > code or not? By firmware version, for example. Or even, chip version,
> > maybe?
> 
> We would know if it's not present because the register would be in its
> power-on reset state, which is what the code is checking for.

This just looks odd for a driver, in its probe, to check if device is in
reset state, only then initializes it. if not, it assumes everything is fine.

Besides that, as described in the code, sounds like a quirk.

+        * right now the FW does set up the HW-block, so we are not
+        * touching the configuration registers.
+        * But if the HW is not enabled, then set it up
+        * using "sane" values used by the firmware right now.


And based on what you are describing now, it is not a quirk, but it is by  design (??)

Again, does it hurt to always initialize the device to a *sane* config?

BR,

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index a13541b..ab946ff 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -434,4 +434,12 @@  depends on (ARCH_QCOM && OF) || COMPILE_TEST
 source "drivers/thermal/qcom/Kconfig"
 endmenu

+config BCM2835_THERMAL
+	tristate "Thermal sensors on bcm2835 SoC"
+	depends on ARCH_BCM2835 || COMPILE_TEST
+	depends on HAS_IOMEM
+	depends on OF
+	help
+	  Support for thermal sensors on Broadcom bcm2835 SoCs.
+
 endif
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index c92eb22..a10ebe0 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -55,3 +55,4 @@  obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra/
 obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
 obj-$(CONFIG_MTK_THERMAL)	+= mtk_thermal.o
 obj-$(CONFIG_GENERIC_ADC_THERMAL)	+= thermal-generic-adc.o
+obj-$(CONFIG_BCM2835_THERMAL)	+= bcm2835_thermal.o
diff --git a/drivers/thermal/bcm2835_thermal.c b/drivers/thermal/bcm2835_thermal.c
new file mode 100644
index 0000000..3468c7b
--- /dev/null
+++ b/drivers/thermal/bcm2835_thermal.c
@@ -0,0 +1,340 @@ 
+/*
+ * Driver for Broadcom BCM2835 soc temperature sensor
+ *
+ * Copyright (C) 2016 Martin Sperl
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/thermal.h>
+
+#define BCM2835_TS_TSENSCTL			0x00
+#define BCM2835_TS_TSENSSTAT			0x04
+
+#define BCM2835_TS_TSENSCTL_PRWDW		BIT(0)
+#define BCM2835_TS_TSENSCTL_RSTB		BIT(1)
+#define BCM2835_TS_TSENSCTL_CTRL_BITS		3
+#define BCM2835_TS_TSENSCTL_CTRL_SHIFT		2
+#define BCM2835_TS_TSENSCTL_CTRL_MASK		    \
+	GENMASK(BCM2835_TS_TSENSCTL_CTRL_BITS +     \
+		BCM2835_TS_TSENSCTL_CTRL_SHIFT - 1, \
+		BCM2835_TS_TSENSCTL_CTRL_SHIFT)
+#define BCM2835_TS_TSENSCTL_CTRL_DEFAULT	1
+#define BCM2835_TS_TSENSCTL_EN_INT		BIT(5)
+#define BCM2835_TS_TSENSCTL_DIRECT		BIT(6)
+#define BCM2835_TS_TSENSCTL_CLR_INT		BIT(7)
+#define BCM2835_TS_TSENSCTL_THOLD_SHIFT		8
+#define BCM2835_TS_TSENSCTL_THOLD_BITS		10
+#define BCM2835_TS_TSENSCTL_THOLD_MASK		     \
+	GENMASK(BCM2835_TS_TSENSCTL_THOLD_BITS +     \
+		BCM2835_TS_TSENSCTL_THOLD_SHIFT - 1, \
+		BCM2835_TS_TSENSCTL_THOLD_SHIFT)
+#define BCM2835_TS_TSENSCTL_RSTDELAY_SHIFT	18
+#define BCM2835_TS_TSENSCTL_RSTDELAY_BITS	8
+#define BCM2835_TS_TSENSCTL_REGULEN		BIT(26)
+
+#define BCM2835_TS_TSENSSTAT_DATA_BITS		10
+#define BCM2835_TS_TSENSSTAT_DATA_SHIFT		0
+#define BCM2835_TS_TSENSSTAT_DATA_MASK		     \
+	GENMASK(BCM2835_TS_TSENSSTAT_DATA_BITS +     \
+		BCM2835_TS_TSENSSTAT_DATA_SHIFT - 1, \
+		BCM2835_TS_TSENSSTAT_DATA_SHIFT)
+#define BCM2835_TS_TSENSSTAT_VALID		BIT(10)
+#define BCM2835_TS_TSENSSTAT_INTERRUPT		BIT(11)
+
+struct bcm2835_thermal_info {
+	int offset;
+	int slope;
+	int trip_temp;
+};
+
+struct bcm2835_thermal_data {
+	const struct bcm2835_thermal_info *info;
+	void __iomem *regs;
+	struct clk *clk;
+	struct dentry *debugfsdir;
+};
+
+static int bcm2835_thermal_adc2temp(
+	const struct bcm2835_thermal_info *info, u32 adc)
+{
+	return info->offset + (adc * info->slope);
+}
+
+static int bcm2835_thermal_temp2adc(
+	const struct bcm2835_thermal_info *info, int temp)
+{
+	temp -= info->offset;
+	temp /= info->slope;
+
+	if (temp < 0)
+		temp = 0;
+	if (temp >= BIT(BCM2835_TS_TSENSSTAT_DATA_BITS))
+		temp = BIT(BCM2835_TS_TSENSSTAT_DATA_BITS) - 1;
+
+	return temp;
+}
+
+static int bcm2835_thermal_get_trip_type(
+	struct thermal_zone_device *tz, int trip,
+	enum thermal_trip_type *type)
+{
+	*type = THERMAL_TRIP_CRITICAL;
+	return 0;
+}
+
+static int bcm2835_thermal_get_trip_temp(
+	struct thermal_zone_device *tz, int trip, int *temp)
+{
+	struct bcm2835_thermal_data *data = tz->devdata;
+	u32 val = readl(data->regs + BCM2835_TS_TSENSCTL);
+
+	/* get the THOLD bits */
+	val &= BCM2835_TS_TSENSCTL_THOLD_MASK;
+	val >>= BCM2835_TS_TSENSCTL_THOLD_SHIFT;
+
+	/* if it is zero then use the info value */
+	if (val)
+		*temp = bcm2835_thermal_adc2temp(data->info, val);
+	else
+		*temp = data->info->trip_temp;
+
+	return 0;
+}
+
+static int bcm2835_thermal_get_temp(struct thermal_zone_device *tz,
+				    int *temp)
+{
+	struct bcm2835_thermal_data *data = tz->devdata;
+	u32 val = readl(data->regs + BCM2835_TS_TSENSSTAT);
+
+	if (!(val & BCM2835_TS_TSENSSTAT_VALID))
+		return -EIO;
+
+	val &= BCM2835_TS_TSENSSTAT_DATA_MASK;
+
+	*temp = bcm2835_thermal_adc2temp(data->info, val);
+
+	return 0;
+}
+
+static const struct debugfs_reg32 bcm2835_thermal_regs[] = {
+	{
+		.name = "ctl",
+		.offset = 0
+	},
+	{
+		.name = "stat",
+		.offset = 4
+	}
+};
+
+static void bcm2835_thermal_debugfs(struct platform_device *pdev)
+{
+	struct thermal_zone_device *tz = platform_get_drvdata(pdev);
+	struct bcm2835_thermal_data *data = tz->devdata;
+	struct debugfs_regset32 *regset;
+
+	data->debugfsdir = debugfs_create_dir("bcm2835_thermal", NULL);
+	if (!data->debugfsdir)
+		return;
+
+	regset = devm_kzalloc(&pdev->dev, sizeof(*regset), GFP_KERNEL);
+	if (!regset)
+		return;
+
+	regset->regs = bcm2835_thermal_regs;
+	regset->nregs = ARRAY_SIZE(bcm2835_thermal_regs);
+	regset->base = data->regs;
+
+	debugfs_create_regset32("regset", S_IRUGO,
+				data->debugfsdir, regset);
+}
+
+static struct thermal_zone_device_ops bcm2835_thermal_ops  = {
+	.get_temp = bcm2835_thermal_get_temp,
+	.get_trip_temp = bcm2835_thermal_get_trip_temp,
+	.get_trip_type = bcm2835_thermal_get_trip_type,
+};
+
+static const struct of_device_id bcm2835_thermal_of_match_table[];
+static int bcm2835_thermal_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	struct thermal_zone_device *tz;
+	struct bcm2835_thermal_data *data;
+	struct resource *res;
+	int err;
+	u32 val;
+	unsigned long rate;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	match = of_match_device(bcm2835_thermal_of_match_table,
+				&pdev->dev);
+	if (!match)
+		return -EINVAL;
+	data->info = match->data;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data->regs)) {
+		err = PTR_ERR(data->regs);
+		dev_err(&pdev->dev, "Could not get registers: %d\n", err);
+		return err;
+	}
+
+	data->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(data->clk)) {
+		err = PTR_ERR(data->clk);
+		if (err != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Could not get clk: %d\n", err);
+		return err;
+	}
+
+	err = clk_prepare_enable(data->clk);
+	if (err)
+		return err;
+
+	rate = clk_get_rate(data->clk);
+	if ((rate < 1920000) || (rate > 5000000))
+		dev_warn(&pdev->dev,
+			 "Clock %pCn running at %pCr Hz is outside of the recommended range: 1.92 to 5MHz\n",
+			 data->clk, data->clk);
+
+	/*
+	 * right now the FW does set up the HW-block, so we are not
+	 * touching the configuration registers.
+	 * But if the HW is not enabled, then set it up
+	 * using "sane" values used by the firmware right now.
+	 */
+	val = readl(data->regs + BCM2835_TS_TSENSCTL);
+	if (!(val & BCM2835_TS_TSENSCTL_RSTB)) {
+		/* the basic required flags */
+		val = (BCM2835_TS_TSENSCTL_CTRL_DEFAULT <<
+		       BCM2835_TS_TSENSCTL_CTRL_SHIFT) |
+		      BCM2835_TS_TSENSCTL_REGULEN;
+
+		/*
+		 * reset delay using the current firmware value of 14
+		 * - units of time are unknown.
+		 */
+		val |= (14 << BCM2835_TS_TSENSCTL_RSTDELAY_SHIFT);
+
+		/*  trip_adc value from info */
+		val |= bcm2835_thermal_temp2adc(data->info,
+						data->info->trip_temp) <<
+			BCM2835_TS_TSENSCTL_THOLD_SHIFT;
+
+		/* write the value back to the register as 2 steps */
+		writel(val, data->regs + BCM2835_TS_TSENSCTL);
+		val |= BCM2835_TS_TSENSCTL_RSTB;
+		writel(val, data->regs + BCM2835_TS_TSENSCTL);
+	}
+
+	/* register thermal zone with 1 trip point an 1s polling */
+	tz = thermal_zone_device_register("bcm2835_thermal",
+					  1, 0, data,
+					  &bcm2835_thermal_ops,
+					  NULL,
+					  0, 1000);
+	if (IS_ERR(tz)) {
+		clk_disable_unprepare(data->clk);
+		err = PTR_ERR(tz);
+		dev_err(&pdev->dev,
+			"Failed to register the thermal device: %d\n",
+			err);
+		return err;
+	}
+
+	platform_set_drvdata(pdev, tz);
+
+	bcm2835_thermal_debugfs(pdev);
+
+	return 0;
+}
+
+static int bcm2835_thermal_remove(struct platform_device *pdev)
+{
+	struct thermal_zone_device *tz = platform_get_drvdata(pdev);
+	struct bcm2835_thermal_data *data = tz->devdata;
+
+	debugfs_remove_recursive(data->debugfsdir);
+	thermal_zone_device_unregister(tz);
+	clk_disable_unprepare(data->clk);
+
+	return 0;
+}
+
+/*
+ * Note: as per Raspberry Foundation FAQ
+ * (https://www.raspberrypi.org/help/faqs/#performanceOperatingTemperature)
+ * the recommended temperature range for the SOC -40C to +85C
+ * so the trip limit is set to 80C.
+ * this applies to all the BCM283X SOC
+ */
+
+static const struct of_device_id bcm2835_thermal_of_match_table[] = {
+	{
+		.compatible = "brcm,bcm2835-thermal",
+		.data = &(struct bcm2835_thermal_info) {
+			.offset = 407000,
+			.slope = -538,
+			.trip_temp = 80000
+		}
+	},
+	{
+		.compatible = "brcm,bcm2836-thermal",
+		.data = &(struct bcm2835_thermal_info) {
+			.offset = 407000,
+			.slope = -538,
+			.trip_temp = 80000
+		}
+	},
+	{
+		.compatible = "brcm,bcm2837-thermal",
+		.data = &(struct bcm2835_thermal_info) {
+			/* the bcm2837 needs adjustment of +5C */
+			.offset = 407000 + 5000,
+			.slope = -538,
+			.trip_temp = 80000
+		}
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm2835_thermal_of_match_table);
+
+static struct platform_driver bcm2835_thermal_driver = {
+	.probe = bcm2835_thermal_probe,
+	.remove = bcm2835_thermal_remove,
+	.driver = {
+		.name = "bcm2835_thermal",
+		.of_match_table = bcm2835_thermal_of_match_table,
+	},
+};
+module_platform_driver(bcm2835_thermal_driver);
+
+MODULE_AUTHOR("Martin Sperl");
+MODULE_DESCRIPTION("Thermal driver for bcm2835 chip");
+MODULE_LICENSE("GPL");