[PATCH V8 0/6] thermal: bcm2835: add thermal driver

[PATCH V8 0/6] thermal: bcm2835: add thermal driver

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

Add a thermal driver for the TSENSE device of the bcm2835/6/7 SOC.

If the firmware enables the HW, then the configuration is not touched.
In case the firmware has not enabled the device, then we try to set
it up correctly (which unfortunately can not get tested).

It exposes temperature and a critical trip point
(using a hardcoded default of 80C or the temperature configured
in the control register by the firmware, which reads as
407C currently)

The calibrations are (potentially) different for bcm2835, bcm2836
and bcm2837 and can get selected by the compatible property
in the device tree.

The driver also exposes the registers via debugfs.

Possible future enhancements:
* the device has the ability to trigger interrupts on reaching
  the programmed critical temperature.
  I have no knowledge which interrupt could be responsible
  for this on the ARM side, so if we get to know which irq
  it is we can implement that.
  Instead the driver right now implements polling in 1 second intervals
* the device can also reset the HW after the trip point
  has been reached (also with some delay, so that corrective
  actions can get taken) - this is currently not enabled by the
  firmware, but could.
* we could define more trip points for THERMAL_TRIP_HOT
* make the trip point limits modifiable (ops.set_trip_temp)

Note:
  No support for 32-bit arm bcm2837, as there is no
  arch/arm/boot/dts/bcm2836.dtsi upstream as of now.
  64-bit arm support is not tested

Changelog:
 V1 -> V2: renamed dt-binding documentation file
       	   added specific settings depending on compatible
	   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)
 	   added driver to multi_v7_defconfig
 V2 -> V3: made a module in multi_v7_defconfig
           fixed typo in dt-binding document
 V3 -> V4: moved driver back to thermal (not using bcm sub-directory)
       	   set polling interval to 1second (was 0ms, so interrupt driven)
 V4 -> V5: use correct compatiblity for different soc versions in dt
       	   support ARM64 for bcm2837 in devicetree and defconfig
 V5 -> V6: incorporated changes recommended by Stefan Wahren
 V6 -> V7: removed depends on ARCH_BCM2836 || ARCH_BCM2837 in Kconfig
 V7 -> V8: rebased

Martin Sperl (6):
  dt: bindings: add thermal device driver for bcm2835
  thermal: bcm2835: add thermal driver for bcm2835 soc
  ARM: bcm2835: dts: add thermal node to device-tree of bcm283x
  ARM64: bcm2835: dts: add thermal node to device-tree of bcm2837
  ARM: bcm2835: add thermal driver to default_config
  ARM64: bcm2835: add thermal driver to default_config

 .../bindings/thermal/brcm,bcm2835-thermal.txt      |  17 ++
 arch/arm/boot/dts/bcm2835.dtsi                     |   6 +
 arch/arm/boot/dts/bcm2836.dtsi                     |   6 +
 arch/arm/boot/dts/bcm283x.dtsi                     |   7 +
 arch/arm/configs/bcm2835_defconfig                 |   2 +
 arch/arm64/boot/dts/broadcom/bcm2837.dtsi          |   6 +
 arch/arm64/configs/defconfig                       |   1 +
 drivers/thermal/Kconfig                            |   8 +
 drivers/thermal/Makefile                           |   1 +
 drivers/thermal/bcm2835_thermal.c                  | 340 +++++++++++++++++++++
 10 files changed, 394 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/brcm,bcm2835-thermal.txt
 create mode 100644 drivers/thermal/bcm2835_thermal.c

--
2.1.4

[PATCH V8 0/6] thermal: bcm2835: add thermal driver

[kernel at martin.sperl.org]

From: Martin Sperl <kernel@martin.sperl.org>

Add a thermal driver for the TSENSE device of the bcm2835/6/7 SOC.

If the firmware enables the HW, then the configuration is not touched.
In case the firmware has not enabled the device, then we try to set
it up correctly (which unfortunately can not get tested).

It exposes temperature and a critical trip point
(using a hardcoded default of 80C or the temperature configured
in the control register by the firmware, which reads as
407C currently)

The calibrations are (potentially) different for bcm2835, bcm2836
and bcm2837 and can get selected by the compatible property
in the device tree.

The driver also exposes the registers via debugfs.

Possible future enhancements:
* the device has the ability to trigger interrupts on reaching
  the programmed critical temperature.
  I have no knowledge which interrupt could be responsible
  for this on the ARM side, so if we get to know which irq
  it is we can implement that.
  Instead the driver right now implements polling in 1 second intervals
* the device can also reset the HW after the trip point
  has been reached (also with some delay, so that corrective
  actions can get taken) - this is currently not enabled by the
  firmware, but could.
* we could define more trip points for THERMAL_TRIP_HOT
* make the trip point limits modifiable (ops.set_trip_temp)

Note:
  No support for 32-bit arm bcm2837, as there is no
  arch/arm/boot/dts/bcm2836.dtsi upstream as of now.
  64-bit arm support is not tested

Changelog:
 V1 -> V2: renamed dt-binding documentation file
       	   added specific settings depending on compatible
	   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)
 	   added driver to multi_v7_defconfig
 V2 -> V3: made a module in multi_v7_defconfig
           fixed typo in dt-binding document
 V3 -> V4: moved driver back to thermal (not using bcm sub-directory)
       	   set polling interval to 1second (was 0ms, so interrupt driven)
 V4 -> V5: use correct compatiblity for different soc versions in dt
       	   support ARM64 for bcm2837 in devicetree and defconfig
 V5 -> V6: incorporated changes recommended by Stefan Wahren
 V6 -> V7: removed depends on ARCH_BCM2836 || ARCH_BCM2837 in Kconfig
 V7 -> V8: rebased

Martin Sperl (6):
  dt: bindings: add thermal device driver for bcm2835
  thermal: bcm2835: add thermal driver for bcm2835 soc
  ARM: bcm2835: dts: add thermal node to device-tree of bcm283x
  ARM64: bcm2835: dts: add thermal node to device-tree of bcm2837
  ARM: bcm2835: add thermal driver to default_config
  ARM64: bcm2835: add thermal driver to default_config

 .../bindings/thermal/brcm,bcm2835-thermal.txt      |  17 ++
 arch/arm/boot/dts/bcm2835.dtsi                     |   6 +
 arch/arm/boot/dts/bcm2836.dtsi                     |   6 +
 arch/arm/boot/dts/bcm283x.dtsi                     |   7 +
 arch/arm/configs/bcm2835_defconfig                 |   2 +
 arch/arm64/boot/dts/broadcom/bcm2837.dtsi          |   6 +
 arch/arm64/configs/defconfig                       |   1 +
 drivers/thermal/Kconfig                            |   8 +
 drivers/thermal/Makefile                           |   1 +
 drivers/thermal/bcm2835_thermal.c                  | 340 +++++++++++++++++++++
 10 files changed, 394 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/brcm,bcm2835-thermal.txt
 create mode 100644 drivers/thermal/bcm2835_thermal.c

--
2.1.4

[PATCH V8 1/6] dt: bindings: add thermal device driver for bcm2835

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

Add dt-binding documentation for bcm2835 SOC thermal sensor.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
Acked-by: Eric Anholt <eric@anholt.net>
Acked-by: Rob Herring <robh@kernel.org>

Changelog:
 V1 -> V2: renamed file to follow naming conventions
 V2 -> V3: removed 0x in node name
---
 .../bindings/thermal/brcm,bcm2835-thermal.txt           | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/brcm,bcm2835-thermal.txt

diff --git a/Documentation/devicetree/bindings/thermal/brcm,bcm2835-thermal.txt b/Documentation/devicetree/bindings/thermal/brcm,bcm2835-thermal.txt
new file mode 100644
index 0000000..474531d
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/brcm,bcm2835-thermal.txt
@@ -0,0 +1,17 @@
+Binding for Thermal Sensor driver for BCM2835 SoCs.
+
+Required parameters:
+-------------------
+
+compatible: 	should be one of: "brcm,bcm2835-thermal",
+		"brcm,bcm2836-thermal" or "brcm,bcm2837-thermal"
+reg:		Address range of the thermal registers.
+clocks: 	Phandle of the clock used by the thermal sensor.
+
+Example:
+
+thermal: thermal@7e212000 {
+	compatible = "brcm,bcm2835-thermal";
+	reg = <0x7e212000 0x8>;
+	clocks = <&clocks BCM2835_CLOCK_TSENS>;
+};
-- 
2.1.4

[PATCH V8 1/6] dt: bindings: add thermal device driver for bcm2835

[kernel at martin.sperl.org]

From: Martin Sperl <kernel@martin.sperl.org>

Add dt-binding documentation for bcm2835 SOC thermal sensor.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
Acked-by: Eric Anholt <eric@anholt.net>
Acked-by: Rob Herring <robh@kernel.org>

Changelog:
 V1 -> V2: renamed file to follow naming conventions
 V2 -> V3: removed 0x in node name
---
 .../bindings/thermal/brcm,bcm2835-thermal.txt           | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/brcm,bcm2835-thermal.txt

diff --git a/Documentation/devicetree/bindings/thermal/brcm,bcm2835-thermal.txt b/Documentation/devicetree/bindings/thermal/brcm,bcm2835-thermal.txt
new file mode 100644
index 0000000..474531d
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/brcm,bcm2835-thermal.txt
@@ -0,0 +1,17 @@
+Binding for Thermal Sensor driver for BCM2835 SoCs.
+
+Required parameters:
+-------------------
+
+compatible: 	should be one of: "brcm,bcm2835-thermal",
+		"brcm,bcm2836-thermal" or "brcm,bcm2837-thermal"
+reg:		Address range of the thermal registers.
+clocks: 	Phandle of the clock used by the thermal sensor.
+
+Example:
+
+thermal: thermal at 7e212000 {
+	compatible = "brcm,bcm2835-thermal";
+	reg = <0x7e212000 0x8>;
+	clocks = <&clocks BCM2835_CLOCK_TSENS>;
+};
-- 
2.1.4

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

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

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 <kernel@martin.sperl.org>
Acked-by: Eric Anholt <eric@anholt.net>
Acked-by: Stefan Wahren <stefan.wahren@i2se.com>

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/#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

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

[kernel at martin.sperl.org]

From: Martin Sperl <kernel@martin.sperl.org>

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 <kernel@martin.sperl.org>
Acked-by: Eric Anholt <eric@anholt.net>
Acked-by: Stefan Wahren <stefan.wahren@i2se.com>

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/#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

[PATCH V8 3/6] ARM: bcm2835: dts: add thermal node to device-tree of bcm283x

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

Add the node for the thermal sensor of the bcm2835-soc
to the device tree.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
Reviewed-by: Eric Anholt <eric@anholt.net>
Acked-by: Stefan Wahren <stefan.wahren@i2se.com>

Changelog:
V1 -> V5: generic settings is shared in bcm283x.dtsi, but disabled
	  moved the compatible string to the SOC specific dtsi
            for arm and arm64
V5 -> V6: fix remove 0x prefix from thermal@0x7e212000

Note: there is no arm/boot/dts/bcm2837.dtsi as of now,
      so the 32-bit rpi3 dt is not modified.
---
 arch/arm/boot/dts/bcm2835.dtsi | 6 ++++++
 arch/arm/boot/dts/bcm2836.dtsi | 6 ++++++
 arch/arm/boot/dts/bcm283x.dtsi | 7 +++++++
 3 files changed, 19 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index a78759e..0890d97 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -23,3 +23,9 @@
 		};
 	};
 };
+
+/* enable thermal sensor with the correct compatible property set */
+&thermal {
+	compatible = "brcm,bcm2835-thermal";
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/bcm2836.dtsi b/arch/arm/boot/dts/bcm2836.dtsi
index 9d0651d..519a44f 100644
--- a/arch/arm/boot/dts/bcm2836.dtsi
+++ b/arch/arm/boot/dts/bcm2836.dtsi
@@ -76,3 +76,9 @@
 	interrupt-parent = <&local_intc>;
 	interrupts = <8>;
 };
+
+/* enable thermal sensor with the correct compatible property set */
+&thermal {
+	compatible = "brcm,bcm2836-thermal";
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
index 46d46d8..f794a18 100644
--- a/arch/arm/boot/dts/bcm283x.dtsi
+++ b/arch/arm/boot/dts/bcm283x.dtsi
@@ -187,6 +187,13 @@
 			interrupts = <2 14>; /* pwa1 */
 		};
 
+		thermal: thermal@7e212000 {
+			compatible = "brcm,bcm2835-thermal";
+			reg = <0x7e212000 0x8>;
+			clocks = <&clocks BCM2835_CLOCK_TSENS>;
+			status = "disabled";
+		};
+
 		aux: aux@0x7e215000 {
 			compatible = "brcm,bcm2835-aux";
 			#clock-cells = <1>;
-- 
2.1.4

[PATCH V8 3/6] ARM: bcm2835: dts: add thermal node to device-tree of bcm283x

[kernel at martin.sperl.org]

From: Martin Sperl <kernel@martin.sperl.org>

Add the node for the thermal sensor of the bcm2835-soc
to the device tree.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
Reviewed-by: Eric Anholt <eric@anholt.net>
Acked-by: Stefan Wahren <stefan.wahren@i2se.com>

Changelog:
V1 -> V5: generic settings is shared in bcm283x.dtsi, but disabled
	  moved the compatible string to the SOC specific dtsi
            for arm and arm64
V5 -> V6: fix remove 0x prefix from thermal at 0x7e212000

Note: there is no arm/boot/dts/bcm2837.dtsi as of now,
      so the 32-bit rpi3 dt is not modified.
---
 arch/arm/boot/dts/bcm2835.dtsi | 6 ++++++
 arch/arm/boot/dts/bcm2836.dtsi | 6 ++++++
 arch/arm/boot/dts/bcm283x.dtsi | 7 +++++++
 3 files changed, 19 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index a78759e..0890d97 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -23,3 +23,9 @@
 		};
 	};
 };
+
+/* enable thermal sensor with the correct compatible property set */
+&thermal {
+	compatible = "brcm,bcm2835-thermal";
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/bcm2836.dtsi b/arch/arm/boot/dts/bcm2836.dtsi
index 9d0651d..519a44f 100644
--- a/arch/arm/boot/dts/bcm2836.dtsi
+++ b/arch/arm/boot/dts/bcm2836.dtsi
@@ -76,3 +76,9 @@
 	interrupt-parent = <&local_intc>;
 	interrupts = <8>;
 };
+
+/* enable thermal sensor with the correct compatible property set */
+&thermal {
+	compatible = "brcm,bcm2836-thermal";
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
index 46d46d8..f794a18 100644
--- a/arch/arm/boot/dts/bcm283x.dtsi
+++ b/arch/arm/boot/dts/bcm283x.dtsi
@@ -187,6 +187,13 @@
 			interrupts = <2 14>; /* pwa1 */
 		};
 
+		thermal: thermal at 7e212000 {
+			compatible = "brcm,bcm2835-thermal";
+			reg = <0x7e212000 0x8>;
+			clocks = <&clocks BCM2835_CLOCK_TSENS>;
+			status = "disabled";
+		};
+
 		aux: aux at 0x7e215000 {
 			compatible = "brcm,bcm2835-aux";
 			#clock-cells = <1>;
-- 
2.1.4

[PATCH V8 4/6] ARM64: bcm2835: dts: add thermal node to device-tree of bcm2837

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

Add the node for the thermal sensor of the bcm2837-soc
to the device tree.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 arch/arm64/boot/dts/broadcom/bcm2837.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/broadcom/bcm2837.dtsi b/arch/arm64/boot/dts/broadcom/bcm2837.dtsi
index 8216bbb..0fc10fd 100644
--- a/arch/arm64/boot/dts/broadcom/bcm2837.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcm2837.dtsi
@@ -74,3 +74,9 @@
 	interrupt-parent = <&local_intc>;
 	interrupts = <8>;
 };
+
+/* enable thermal sensor with the correct compatible property set */
+&thermal {
+	compatible = "brcm,bcm2837-thermal";
+	status = "okay";
+};
-- 
2.1.4

[PATCH V8 4/6] ARM64: bcm2835: dts: add thermal node to device-tree of bcm2837

[kernel at martin.sperl.org]

From: Martin Sperl <kernel@martin.sperl.org>

Add the node for the thermal sensor of the bcm2837-soc
to the device tree.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 arch/arm64/boot/dts/broadcom/bcm2837.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/broadcom/bcm2837.dtsi b/arch/arm64/boot/dts/broadcom/bcm2837.dtsi
index 8216bbb..0fc10fd 100644
--- a/arch/arm64/boot/dts/broadcom/bcm2837.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcm2837.dtsi
@@ -74,3 +74,9 @@
 	interrupt-parent = <&local_intc>;
 	interrupts = <8>;
 };
+
+/* enable thermal sensor with the correct compatible property set */
+&thermal {
+	compatible = "brcm,bcm2837-thermal";
+	status = "okay";
+};
-- 
2.1.4

[PATCH V8 5/6] ARM: bcm2835: add thermal driver to default_config

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

Add the thermal driver to list of compiled modules
in the default config for bcm2835.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
Reviewed-by: Eric Anholt <eric@anholt.net>
---
 arch/arm/configs/bcm2835_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/bcm2835_defconfig b/arch/arm/configs/bcm2835_defconfig
index 79de828..4b89f4e 100644
--- a/arch/arm/configs/bcm2835_defconfig
+++ b/arch/arm/configs/bcm2835_defconfig
@@ -73,6 +73,8 @@ CONFIG_SPI_BCM2835=y
 CONFIG_SPI_BCM2835AUX=y
 CONFIG_GPIO_SYSFS=y
 # CONFIG_HWMON is not set
+CONFIG_THERMAL=y
+CONFIG_BCM2835_THERMAL=y
 CONFIG_WATCHDOG=y
 CONFIG_BCM2835_WDT=y
 CONFIG_DRM=y
-- 
2.1.4

[PATCH V8 5/6] ARM: bcm2835: add thermal driver to default_config

[kernel at martin.sperl.org]

From: Martin Sperl <kernel@martin.sperl.org>

Add the thermal driver to list of compiled modules
in the default config for bcm2835.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
Reviewed-by: Eric Anholt <eric@anholt.net>
---
 arch/arm/configs/bcm2835_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/bcm2835_defconfig b/arch/arm/configs/bcm2835_defconfig
index 79de828..4b89f4e 100644
--- a/arch/arm/configs/bcm2835_defconfig
+++ b/arch/arm/configs/bcm2835_defconfig
@@ -73,6 +73,8 @@ CONFIG_SPI_BCM2835=y
 CONFIG_SPI_BCM2835AUX=y
 CONFIG_GPIO_SYSFS=y
 # CONFIG_HWMON is not set
+CONFIG_THERMAL=y
+CONFIG_BCM2835_THERMAL=y
 CONFIG_WATCHDOG=y
 CONFIG_BCM2835_WDT=y
 CONFIG_DRM=y
-- 
2.1.4

[PATCH V8 6/6] ARM64: bcm2835: add thermal driver to default_config

[kernel]

From: Martin Sperl <kernel@martin.sperl.org>

Add the thermal driver for bcm2837 to list of compiled modules
in the default config.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>

Changelog:
 V7 -> V8: rebased

---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index dab2cb0..37b2f0a 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -271,6 +271,7 @@ CONFIG_THERMAL=y
 CONFIG_THERMAL_EMULATION=y
 CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y
 CONFIG_CPU_THERMAL=y
+CONFIG_BCM2835_THERMAL=y
 CONFIG_EXYNOS_THERMAL=y
 CONFIG_WATCHDOG=y
 CONFIG_RENESAS_WDT=y
--
2.1.4

[PATCH V8 6/6] ARM64: bcm2835: add thermal driver to default_config

[kernel at martin.sperl.org]

From: Martin Sperl <kernel@martin.sperl.org>

Add the thermal driver for bcm2837 to list of compiled modules
in the default config.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>

Changelog:
 V7 -> V8: rebased

---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index dab2cb0..37b2f0a 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -271,6 +271,7 @@ CONFIG_THERMAL=y
 CONFIG_THERMAL_EMULATION=y
 CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y
 CONFIG_CPU_THERMAL=y
+CONFIG_BCM2835_THERMAL=y
 CONFIG_EXYNOS_THERMAL=y
 CONFIG_WATCHDOG=y
 CONFIG_RENESAS_WDT=y
--
2.1.4

Re: [PATCH V8 0/6] thermal: bcm2835: add thermal driver

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 158 bytes --]

kernel@martin.sperl.org writes:

> From: Martin Sperl <kernel@martin.sperl.org>

Since Eduardo seems to be AFK, I've pulled this series to my -next
branches.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

[PATCH V8 0/6] thermal: bcm2835: add thermal driver

[Eric Anholt]

kernel at martin.sperl.org writes:

> From: Martin Sperl <kernel@martin.sperl.org>

Since Eduardo seems to be AFK, I've pulled this series to my -next
branches.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161111/b4d84fe9/attachment.sig>

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

[Zhang Rui]

On Wed, 2016-11-02 at 10:18 +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> 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 <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> Acked-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> Acked-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> 
Acked-by: Zhang Rui <rui.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Zhang Rui]

On Wed, 2016-11-02 at 10:18 +0000, kernel at martin.sperl.org wrote:
> From: Martin Sperl <kernel@martin.sperl.org>
> 
> 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 <kernel@martin.sperl.org>
> Acked-by: Eric Anholt <eric@anholt.net>
> Acked-by: Stefan Wahren <stefan.wahren@i2se.com>
> 
Acked-by: Zhang Rui <rui.zhang@intel.com>

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

Re: [PATCH V8 0/6] thermal: bcm2835: add thermal driver

[Zhang Rui]

On Fri, 2016-11-11 at 09:01 -0800, Eric Anholt wrote:
> kernel@martin.sperl.org writes:
> 
> > 
> > From: Martin Sperl <kernel@martin.sperl.org>
> Since Eduardo seems to be AFK, I've pulled this series to my -next
> branches.
I will take the thermal soc patches this time if we still don't have
response from Eduardo by the end of this week.

For this patches set, will you queue them up or do you prefer to go via
thermal tree?

thanks,
rui

[PATCH V8 0/6] thermal: bcm2835: add thermal driver

[Zhang Rui]

On Fri, 2016-11-11 at 09:01 -0800, Eric Anholt wrote:
> kernel at martin.sperl.org writes:
> 
> > 
> > From: Martin Sperl <kernel@martin.sperl.org>
> Since Eduardo seems to be AFK, I've pulled this series to my -next
> branches.
I will take the thermal soc patches this time if we still don't have
response from Eduardo by the end of this week.

For this patches set, will you queue them up or do you prefer to go via
thermal tree?

thanks,
rui

Re: [PATCH V8 0/6] thermal: bcm2835: add thermal driver

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 662 bytes --]

Zhang Rui <rui.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:

> On Fri, 2016-11-11 at 09:01 -0800, Eric Anholt wrote:
>> kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org writes:
>> 
>> > 
>> > From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>> Since Eduardo seems to be AFK, I've pulled this series to my -next
>> branches.
> I will take the thermal soc patches this time if we still don't have
> response from Eduardo by the end of this week.
>
> For this patches set, will you queue them up or do you prefer to go via
> thermal tree?

I was queuing up the entire series (including the driver) through the
SOC trees.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

[PATCH V8 0/6] thermal: bcm2835: add thermal driver

[Eric Anholt]

Zhang Rui <rui.zhang@intel.com> writes:

> On Fri, 2016-11-11 at 09:01 -0800, Eric Anholt wrote:
>> kernel at martin.sperl.org writes:
>> 
>> > 
>> > From: Martin Sperl <kernel@martin.sperl.org>
>> Since Eduardo seems to be AFK, I've pulled this series to my -next
>> branches.
> I will take the thermal soc patches this time if we still don't have
> response from Eduardo by the end of this week.
>
> For this patches set, will you queue them up or do you prefer to go via
> thermal tree?

I was queuing up the entire series (including the driver) through the
SOC trees.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161116/44f00e23/attachment-0001.sig>

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

[Eduardo Valentin]

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, kernel@martin.sperl.org wrote:
> From: Martin Sperl <kernel@martin.sperl.org>
> 
> 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 <kernel@martin.sperl.org>
> Acked-by: Eric Anholt <eric@anholt.net>
> Acked-by: Stefan Wahren <stefan.wahren@i2se.com>
> 
> 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

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

[Eduardo Valentin]

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, kernel at martin.sperl.org wrote:
> From: Martin Sperl <kernel@martin.sperl.org>
> 
> 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 <kernel@martin.sperl.org>
> Acked-by: Eric Anholt <eric@anholt.net>
> Acked-by: Stefan Wahren <stefan.wahren@i2se.com>
> 
> 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

Re: [PATCH V8 0/6] thermal: bcm2835: add thermal driver

[Eduardo Valentin]

Hello,
On Wed, Nov 02, 2016 at 10:18:20AM +0000, kernel@martin.sperl.org wrote:
> From: Martin Sperl <kernel@martin.sperl.org>
> 
> Add a thermal driver for the TSENSE device of the bcm2835/6/7 SOC.
> 
> If the firmware enables the HW, then the configuration is not touched.
> In case the firmware has not enabled the device, then we try to set
> it up correctly (which unfortunately can not get tested).
> 
> It exposes temperature and a critical trip point
> (using a hardcoded default of 80C or the temperature configured
> in the control register by the firmware, which reads as
> 407C currently)
> 
> The calibrations are (potentially) different for bcm2835, bcm2836
> and bcm2837 and can get selected by the compatible property
> in the device tree.
> 
> The driver also exposes the registers via debugfs.
> 
> Possible future enhancements:
> * the device has the ability to trigger interrupts on reaching
>   the programmed critical temperature.
>   I have no knowledge which interrupt could be responsible
>   for this on the ARM side, so if we get to know which irq
>   it is we can implement that.
>   Instead the driver right now implements polling in 1 second intervals
> * the device can also reset the HW after the trip point
>   has been reached (also with some delay, so that corrective
>   actions can get taken) - this is currently not enabled by the
>   firmware, but could.
> * we could define more trip points for THERMAL_TRIP_HOT
> * make the trip point limits modifiable (ops.set_trip_temp)
> 
> Note:
>   No support for 32-bit arm bcm2837, as there is no
>   arch/arm/boot/dts/bcm2836.dtsi upstream as of now.
>   64-bit arm support is not tested
> 
> Changelog:
>  V1 -> V2: renamed dt-binding documentation file
>        	   added specific settings depending on compatible
> 	   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)
>  	   added driver to multi_v7_defconfig
>  V2 -> V3: made a module in multi_v7_defconfig
>            fixed typo in dt-binding document
>  V3 -> V4: moved driver back to thermal (not using bcm sub-directory)
>        	   set polling interval to 1second (was 0ms, so interrupt driven)
>  V4 -> V5: use correct compatiblity for different soc versions in dt
>        	   support ARM64 for bcm2837 in devicetree and defconfig
>  V5 -> V6: incorporated changes recommended by Stefan Wahren
>  V6 -> V7: removed depends on ARCH_BCM2836 || ARCH_BCM2837 in Kconfig
>  V7 -> V8: rebased
> 

Despite the minor questions on patch 2, specially on the request to use
existing slope and offset properties, I am ok with the other DT changes.

[PATCH V8 0/6] thermal: bcm2835: add thermal driver

[Eduardo Valentin]

Hello,
On Wed, Nov 02, 2016 at 10:18:20AM +0000, kernel at martin.sperl.org wrote:
> From: Martin Sperl <kernel@martin.sperl.org>
> 
> Add a thermal driver for the TSENSE device of the bcm2835/6/7 SOC.
> 
> If the firmware enables the HW, then the configuration is not touched.
> In case the firmware has not enabled the device, then we try to set
> it up correctly (which unfortunately can not get tested).
> 
> It exposes temperature and a critical trip point
> (using a hardcoded default of 80C or the temperature configured
> in the control register by the firmware, which reads as
> 407C currently)
> 
> The calibrations are (potentially) different for bcm2835, bcm2836
> and bcm2837 and can get selected by the compatible property
> in the device tree.
> 
> The driver also exposes the registers via debugfs.
> 
> Possible future enhancements:
> * the device has the ability to trigger interrupts on reaching
>   the programmed critical temperature.
>   I have no knowledge which interrupt could be responsible
>   for this on the ARM side, so if we get to know which irq
>   it is we can implement that.
>   Instead the driver right now implements polling in 1 second intervals
> * the device can also reset the HW after the trip point
>   has been reached (also with some delay, so that corrective
>   actions can get taken) - this is currently not enabled by the
>   firmware, but could.
> * we could define more trip points for THERMAL_TRIP_HOT
> * make the trip point limits modifiable (ops.set_trip_temp)
> 
> Note:
>   No support for 32-bit arm bcm2837, as there is no
>   arch/arm/boot/dts/bcm2836.dtsi upstream as of now.
>   64-bit arm support is not tested
> 
> Changelog:
>  V1 -> V2: renamed dt-binding documentation file
>        	   added specific settings depending on compatible
> 	   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)
>  	   added driver to multi_v7_defconfig
>  V2 -> V3: made a module in multi_v7_defconfig
>            fixed typo in dt-binding document
>  V3 -> V4: moved driver back to thermal (not using bcm sub-directory)
>        	   set polling interval to 1second (was 0ms, so interrupt driven)
>  V4 -> V5: use correct compatiblity for different soc versions in dt
>        	   support ARM64 for bcm2837 in devicetree and defconfig
>  V5 -> V6: incorporated changes recommended by Stefan Wahren
>  V6 -> V7: removed depends on ARCH_BCM2836 || ARCH_BCM2837 in Kconfig
>  V7 -> V8: rebased
> 

Despite the minor questions on patch 2, specially on the request to use
existing slope and offset properties, I am ok with the other DT changes.

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

[Martin Sperl]

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, kernel@martin.sperl.org wrote:
>> From: Martin Sperl <kernel@martin.sperl.org>
>>
>> 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 <kernel@martin.sperl.org>
>> Acked-by: Eric Anholt <eric@anholt.net>
>> Acked-by: Stefan Wahren <stefan.wahren@i2se.com>
>>
...
>> +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

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

[Martin Sperl]

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, kernel at martin.sperl.org wrote:
>> From: Martin Sperl <kernel@martin.sperl.org>
>>
>> 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 <kernel@martin.sperl.org>
>> Acked-by: Eric Anholt <eric@anholt.net>
>> Acked-by: Stefan Wahren <stefan.wahren@i2se.com>
>>
...
>> +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

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

[Eduardo Valentin]

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, kernel@martin.sperl.org wrote:
> >>From: Martin Sperl <kernel@martin.sperl.org>
> >>
> >>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 <kernel@martin.sperl.org>
> >>Acked-by: Eric Anholt <eric@anholt.net>
> >>Acked-by: Stefan Wahren <stefan.wahren@i2se.com>
> >>
> ...
> >>+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

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

[Eduardo Valentin]

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, kernel at martin.sperl.org wrote:
> >>From: Martin Sperl <kernel@martin.sperl.org>
> >>
> >>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 <kernel@martin.sperl.org>
> >>Acked-by: Eric Anholt <eric@anholt.net>
> >>Acked-by: Stefan Wahren <stefan.wahren@i2se.com>
> >>
> ...
> >>+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

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

[kernel-TqfNSX0MhmxHKSADF0wUEw]

> On 17.11.2016, at 16:10, Eduardo Valentin <edubezval-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
>>>> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>>>> 
>>>> 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 <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>>>> Acked-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
>>>> Acked-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
>>>> 
>> ...
>>>> +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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[kernel at martin.sperl.org]

> On 17.11.2016, at 16:10, Eduardo Valentin <edubezval@gmail.com> 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, kernel at martin.sperl.org wrote:
>>>> From: Martin Sperl <kernel@martin.sperl.org>
>>>> 
>>>> 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 <kernel@martin.sperl.org>
>>>> Acked-by: Eric Anholt <eric@anholt.net>
>>>> Acked-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>> 
>> ...
>>>> +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

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

[Eduardo Valentin]

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, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eduardo Valentin]

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, kernel at martin.sperl.org 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

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

[Martin Sperl]

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, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Martin Sperl]

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, kernel at martin.sperl.org 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

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

[Eduardo Valentin]

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eduardo Valentin]

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

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

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2864 bytes --]

Eduardo Valentin <edubezval-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Eric Anholt]

Eduardo Valentin <edubezval@gmail.com> 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).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161128/af046805/attachment.sig>

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

[Eduardo Valentin]

Hello Eric, Martin,

On Mon, Nov 28, 2016 at 12:30:38PM -0800, Eric Anholt wrote:
> Eduardo Valentin <edubezval@gmail.com> 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.

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

[Eduardo Valentin]

Hello Eric, Martin,

On Mon, Nov 28, 2016 at 12:30:38PM -0800, Eric Anholt wrote:
> Eduardo Valentin <edubezval@gmail.com> 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.

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

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 574 bytes --]

Eduardo Valentin <edubezval@gmail.com> 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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Eric Anholt]

Eduardo Valentin <edubezval@gmail.com> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161129/63bdda2f/attachment.sig>

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

[Eduardo Valentin]

Hello,

On Tue, Nov 29, 2016 at 02:12:46PM -0800, Eric Anholt wrote:
> Eduardo Valentin <edubezval@gmail.com> 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,

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

[Eduardo Valentin]

Hello,

On Tue, Nov 29, 2016 at 02:12:46PM -0800, Eric Anholt wrote:
> Eduardo Valentin <edubezval@gmail.com> 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,