[PATCH for 3.10 0/4] thermal: Add Armada 370/XP support

[PATCH for 3.10 0/4] thermal: Add Armada 370/XP support

[Ezequiel Garcia]

After Andrew Lunn objected the single driver approach [1] that merged
all the Marvell EBU SoC thermal drivers, we have agreed to proceed step
by step. For the time being we are adding support for Armada 370/XP only.
Given both Armada 370 and Armada XP have a similar feature set, it
makes sense to provide a single driver for them.

In the future we can unify this with the other drivers,
if this proves to be worthwhile.

This driver has been influenced by the existing Kirkwood driver,
also taking account of suggestions made by Jason Gunthorpe.

Testing has been done on the following boards:

For Armada 370: Globalscale Mirabox and Marvell Armada 370 RD
For Armada XP:  Plat'home Openblocks AX3 and Marvell Armada XP DB-MV784MP-GP

This series is based in v3.9-rc2. To make testing easier this patchset
has been pushed on a github branch [2].

Any feedback or comments are welcome!

[1] http://permalink.gmane.org/gmane.linux.power-management.general/32123
[2] https://github.com/MISL-EBU-System-SW/mainline-public/tree/armada-thermal

Ezequiel Garcia (4):
  thermal: Add driver for Armada 370/XP SoC thermal management
  ARM: mvebu: Add thermal support to Armada XP device tree
  ARM: mvebu: Add thermal support to Armada 370 device tree
  ARM: configs: Update mvebu defconfig for thermal

 .../devicetree/bindings/thermal/armada-thermal.txt |   22 ++
 arch/arm/boot/dts/armada-370.dtsi                  |    6 +
 arch/arm/boot/dts/armada-xp.dtsi                   |    6 +
 arch/arm/configs/mvebu_defconfig                   |    2 +
 drivers/thermal/Kconfig                            |    8 +
 drivers/thermal/Makefile                           |    1 +
 drivers/thermal/armada_thermal.c                   |  236 ++++++++++++++++++++
 7 files changed, 281 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/thermal/armada-thermal.txt
 create mode 100644 drivers/thermal/armada_thermal.c

-- 
1.7.8.6

[PATCH for 3.10 0/4] thermal: Add Armada 370/XP support

[Ezequiel Garcia]

After Andrew Lunn objected the single driver approach [1] that merged
all the Marvell EBU SoC thermal drivers, we have agreed to proceed step
by step. For the time being we are adding support for Armada 370/XP only.
Given both Armada 370 and Armada XP have a similar feature set, it
makes sense to provide a single driver for them.

In the future we can unify this with the other drivers,
if this proves to be worthwhile.

This driver has been influenced by the existing Kirkwood driver,
also taking account of suggestions made by Jason Gunthorpe.

Testing has been done on the following boards:

For Armada 370: Globalscale Mirabox and Marvell Armada 370 RD
For Armada XP:  Plat'home Openblocks AX3 and Marvell Armada XP DB-MV784MP-GP

This series is based in v3.9-rc2. To make testing easier this patchset
has been pushed on a github branch [2].

Any feedback or comments are welcome!

[1] http://permalink.gmane.org/gmane.linux.power-management.general/32123
[2] https://github.com/MISL-EBU-System-SW/mainline-public/tree/armada-thermal

Ezequiel Garcia (4):
  thermal: Add driver for Armada 370/XP SoC thermal management
  ARM: mvebu: Add thermal support to Armada XP device tree
  ARM: mvebu: Add thermal support to Armada 370 device tree
  ARM: configs: Update mvebu defconfig for thermal

 .../devicetree/bindings/thermal/armada-thermal.txt |   22 ++
 arch/arm/boot/dts/armada-370.dtsi                  |    6 +
 arch/arm/boot/dts/armada-xp.dtsi                   |    6 +
 arch/arm/configs/mvebu_defconfig                   |    2 +
 drivers/thermal/Kconfig                            |    8 +
 drivers/thermal/Makefile                           |    1 +
 drivers/thermal/armada_thermal.c                   |  236 ++++++++++++++++++++
 7 files changed, 281 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/thermal/armada-thermal.txt
 create mode 100644 drivers/thermal/armada_thermal.c

-- 
1.7.8.6

[PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal management

[Ezequiel Garcia]

This driver supports both Armada 370 and Armada XP SoC
thermal management controllers.

Armada 370 has a register to check a valid temperature, whereas
Armada XP does not. Each has a different initialization (i.e. calibration)
function. The temperature conversion formula is the same for both.

The controller present in each SoC have a very similar feature set,
so it corresponds to have one driver to support both of them.

Although this driver may present similarities to Dove and Kirkwood
thermal driver, the exact differences and coincidences are not fully
known. For this reason, support is given through a separate driver.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 .../devicetree/bindings/thermal/armada-thermal.txt |   22 ++
 drivers/thermal/Kconfig                            |    8 +
 drivers/thermal/Makefile                           |    1 +
 drivers/thermal/armada_thermal.c                   |  236 ++++++++++++++++++++
 4 files changed, 267 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/thermal/armada-thermal.txt
 create mode 100644 drivers/thermal/armada_thermal.c

diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
new file mode 100644
index 0000000..fff93d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
@@ -0,0 +1,22 @@
+* Marvell Armada 370/XP thermal management
+
+Required properties:
+
+- compatible:	Should be set to one of the following:
+		marvell,armada370-thermal
+		marvell,armadaxp-thermal
+
+- reg:		Device's register space.
+		Two entries are expected, see the examples below.
+		The first one is required for the sensor register;
+		the second one is required for the control register
+		to be used for sensor initialization (a.k.a. calibration).
+
+Example:
+
+	thermal@d0018300 {
+		compatible = "marvell,armada370-thermal";
+                reg = <0xd0018300 0x4
+		       0xd0018304 0x4>;
+		status = "okay";
+	};
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index a764f16..fcd5a27 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -134,6 +134,14 @@ config DOVE_THERMAL
 	  Support for the Dove thermal sensor driver in the Linux thermal
 	  framework.
 
+config ARMADA_THERMAL
+	tristate "Armada 370/XP thermal management"
+	depends on ARCH_MVEBU
+	depends on OF
+	help
+	  Enable this option if you want to have support for thermal management
+	  controller present in Armada 370 and Armada XP SoC.
+
 config DB8500_THERMAL
 	bool "DB8500 thermal management"
 	depends on ARCH_U8500
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index d3a2b38..4209054 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_THERMAL_GOV_USER_SPACE)	+= user_space.o
 obj-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
 
 # platform thermal drivers
+obj-$(CONFIG_ARMADA_THERMAL)	+= armada_thermal.o
 obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
 obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
 obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
new file mode 100644
index 0000000..dc83a86
--- /dev/null
+++ b/drivers/thermal/armada_thermal.c
@@ -0,0 +1,236 @@
+/*
+ * Marvell Armada 370/XP thermal sensor driver
+ *
+ * Copyright (C) 2013 Marvell
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/thermal.h>
+
+#define THERMAL_VALID_OFFSET		9
+#define THERMAL_VALID_MASK		0x1
+#define THERMAL_TEMP_OFFSET		10
+#define THERMAL_TEMP_MASK		0x1ff
+
+/* Thermal Manager Control and Status Register */
+#define PMU_TDC0_SW_RST_MASK		(0x1 << 1)
+#define PMU_TM_DISABLE_OFFS		0
+#define PMU_TM_DISABLE_MASK		(0x1 << PMU_TM_DISABLE_OFFS)
+#define PMU_TDC0_REF_CAL_CNT_OFFS	11
+#define PMU_TDC0_REF_CAL_CNT_MASK	(0x1ff << PMU_TDC0_REF_CAL_CNT_OFFS)
+#define PMU_TDC0_OTF_CAL_MASK		(0x1 << 30)
+#define PMU_TDC0_START_CAL_MASK		(0x1 << 25)
+
+struct armada_thermal_ops;
+
+/* Marvell EBU Thermal Sensor Dev Structure */
+struct armada_thermal_priv {
+	void __iomem *sensor;
+	void __iomem *control;
+	struct armada_thermal_ops *ops;
+};
+
+struct armada_thermal_ops {
+	/* Initialize the sensor */
+	void (*init_sensor)(struct armada_thermal_priv *);
+
+	/* Test for a valid sensor value (optional) */
+	bool (*is_valid)(struct armada_thermal_priv *);
+};
+
+static void armadaxp_init_sensor(struct armada_thermal_priv *priv)
+{
+	unsigned long reg;
+
+	/* ??? */
+	reg = readl_relaxed(priv->control);
+	reg |= PMU_TDC0_OTF_CAL_MASK;
+	writel(reg, priv->control);
+
+	/* Reference calibration value */
+	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
+	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
+	writel(reg, priv->control);
+
+	/* Reset the sensor */
+	reg = readl_relaxed(priv->control);
+	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control);
+
+	writel(reg, priv->control);
+
+	/* Enable the sensor */
+	reg = readl_relaxed(priv->sensor);
+	reg &= ~PMU_TM_DISABLE_MASK;
+	writel(reg, priv->sensor);
+}
+
+static void armada370_init_sensor(struct armada_thermal_priv *priv)
+{
+	unsigned long reg;
+
+	/* ??? */
+	reg = readl_relaxed(priv->control);
+	reg |= PMU_TDC0_OTF_CAL_MASK;
+	writel(reg, priv->control);
+
+	/* Reference calibration value */
+	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
+	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
+	writel(reg, priv->control);
+
+	/* ??? */
+	reg &= ~PMU_TDC0_START_CAL_MASK;
+	writel(reg, priv->control);
+
+	/* FIXME: Why do we need this delay? */
+	mdelay(10);
+}
+
+static bool armada_is_valid(struct armada_thermal_priv *priv)
+{
+	unsigned long reg = readl_relaxed(priv->sensor);
+
+	return (reg >> THERMAL_VALID_OFFSET) & THERMAL_VALID_MASK;
+}
+
+static int armada_get_temp(struct thermal_zone_device *thermal,
+			  unsigned long *temp)
+{
+	struct armada_thermal_priv *priv = thermal->devdata;
+	unsigned long reg;
+
+	/* Valid check */
+	if (priv->ops->is_valid && !priv->ops->is_valid(priv)) {
+		dev_err(&thermal->device,
+			"Temperature sensor reading not valid\n");
+		return -EIO;
+	}
+
+	reg = (readl_relaxed(priv->sensor) >> THERMAL_TEMP_OFFSET) &
+		THERMAL_TEMP_MASK;
+	*temp = (3153000000UL - (10000000UL*reg)) / 13825;
+	return 0;
+}
+
+static struct thermal_zone_device_ops ops = {
+	.get_temp = armada_get_temp,
+};
+
+static const struct armada_thermal_ops armadaxp_ops = {
+	.init_sensor = armadaxp_init_sensor,
+};
+
+static const struct armada_thermal_ops armada370_ops = {
+	.is_valid = armada_is_valid,
+	.init_sensor = armada370_init_sensor,
+};
+
+static const struct of_device_id armada_thermal_id_table[] = {
+	{
+		.compatible = "marvell,armadaxp-thermal",
+		.data       = &armadaxp_ops,
+	},
+	{
+		.compatible = "marvell,armada370-thermal",
+		.data       = &armada370_ops,
+	},
+	{
+		/* sentinel */
+	},
+};
+MODULE_DEVICE_TABLE(of, armada_thermal_id_table);
+
+static int armada_thermal_probe(struct platform_device *pdev)
+{
+	struct thermal_zone_device *thermal;
+	const struct of_device_id *match;
+	struct armada_thermal_priv *priv;
+	struct resource *res;
+
+	match = of_match_device(armada_thermal_id_table, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Failed to get platform resource\n");
+		return -ENODEV;
+	}
+
+	priv->sensor = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->sensor))
+		return PTR_ERR(priv->sensor);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res) {
+		dev_err(&pdev->dev, "Failed to get platform resource\n");
+		return -ENODEV;
+	}
+
+	priv->control = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->control))
+		return PTR_ERR(priv->control);
+
+	priv->ops = (struct armada_thermal_ops *)match->data;
+	priv->ops->init_sensor(priv);
+
+	thermal = thermal_zone_device_register("armada_thermal", 0, 0,
+					       priv, &ops, NULL, 0, 0);
+	if (IS_ERR(thermal)) {
+		dev_err(&pdev->dev,
+			"Failed to register thermal zone device\n");
+		return PTR_ERR(thermal);
+	}
+
+	platform_set_drvdata(pdev, thermal);
+
+	return 0;
+}
+
+static int armada_thermal_exit(struct platform_device *pdev)
+{
+	struct thermal_zone_device *armada_thermal =
+		platform_get_drvdata(pdev);
+
+	thermal_zone_device_unregister(armada_thermal);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct platform_driver armada_thermal_driver = {
+	.probe = armada_thermal_probe,
+	.remove = armada_thermal_exit,
+	.driver = {
+		.name = "armada_thermal",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(armada_thermal_id_table),
+	},
+};
+
+module_platform_driver(armada_thermal_driver);
+
+MODULE_AUTHOR("Ezequiel Garcia <ezequiel.garcia@free-electrons.com>");
+MODULE_DESCRIPTION("Armada 370/XP thermal driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.8.6

[PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal management

[Ezequiel Garcia]

This driver supports both Armada 370 and Armada XP SoC
thermal management controllers.

Armada 370 has a register to check a valid temperature, whereas
Armada XP does not. Each has a different initialization (i.e. calibration)
function. The temperature conversion formula is the same for both.

The controller present in each SoC have a very similar feature set,
so it corresponds to have one driver to support both of them.

Although this driver may present similarities to Dove and Kirkwood
thermal driver, the exact differences and coincidences are not fully
known. For this reason, support is given through a separate driver.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 .../devicetree/bindings/thermal/armada-thermal.txt |   22 ++
 drivers/thermal/Kconfig                            |    8 +
 drivers/thermal/Makefile                           |    1 +
 drivers/thermal/armada_thermal.c                   |  236 ++++++++++++++++++++
 4 files changed, 267 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/thermal/armada-thermal.txt
 create mode 100644 drivers/thermal/armada_thermal.c

diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
new file mode 100644
index 0000000..fff93d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
@@ -0,0 +1,22 @@
+* Marvell Armada 370/XP thermal management
+
+Required properties:
+
+- compatible:	Should be set to one of the following:
+		marvell,armada370-thermal
+		marvell,armadaxp-thermal
+
+- reg:		Device's register space.
+		Two entries are expected, see the examples below.
+		The first one is required for the sensor register;
+		the second one is required for the control register
+		to be used for sensor initialization (a.k.a. calibration).
+
+Example:
+
+	thermal at d0018300 {
+		compatible = "marvell,armada370-thermal";
+                reg = <0xd0018300 0x4
+		       0xd0018304 0x4>;
+		status = "okay";
+	};
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index a764f16..fcd5a27 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -134,6 +134,14 @@ config DOVE_THERMAL
 	  Support for the Dove thermal sensor driver in the Linux thermal
 	  framework.
 
+config ARMADA_THERMAL
+	tristate "Armada 370/XP thermal management"
+	depends on ARCH_MVEBU
+	depends on OF
+	help
+	  Enable this option if you want to have support for thermal management
+	  controller present in Armada 370 and Armada XP SoC.
+
 config DB8500_THERMAL
 	bool "DB8500 thermal management"
 	depends on ARCH_U8500
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index d3a2b38..4209054 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_THERMAL_GOV_USER_SPACE)	+= user_space.o
 obj-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
 
 # platform thermal drivers
+obj-$(CONFIG_ARMADA_THERMAL)	+= armada_thermal.o
 obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
 obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
 obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
new file mode 100644
index 0000000..dc83a86
--- /dev/null
+++ b/drivers/thermal/armada_thermal.c
@@ -0,0 +1,236 @@
+/*
+ * Marvell Armada 370/XP thermal sensor driver
+ *
+ * Copyright (C) 2013 Marvell
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/thermal.h>
+
+#define THERMAL_VALID_OFFSET		9
+#define THERMAL_VALID_MASK		0x1
+#define THERMAL_TEMP_OFFSET		10
+#define THERMAL_TEMP_MASK		0x1ff
+
+/* Thermal Manager Control and Status Register */
+#define PMU_TDC0_SW_RST_MASK		(0x1 << 1)
+#define PMU_TM_DISABLE_OFFS		0
+#define PMU_TM_DISABLE_MASK		(0x1 << PMU_TM_DISABLE_OFFS)
+#define PMU_TDC0_REF_CAL_CNT_OFFS	11
+#define PMU_TDC0_REF_CAL_CNT_MASK	(0x1ff << PMU_TDC0_REF_CAL_CNT_OFFS)
+#define PMU_TDC0_OTF_CAL_MASK		(0x1 << 30)
+#define PMU_TDC0_START_CAL_MASK		(0x1 << 25)
+
+struct armada_thermal_ops;
+
+/* Marvell EBU Thermal Sensor Dev Structure */
+struct armada_thermal_priv {
+	void __iomem *sensor;
+	void __iomem *control;
+	struct armada_thermal_ops *ops;
+};
+
+struct armada_thermal_ops {
+	/* Initialize the sensor */
+	void (*init_sensor)(struct armada_thermal_priv *);
+
+	/* Test for a valid sensor value (optional) */
+	bool (*is_valid)(struct armada_thermal_priv *);
+};
+
+static void armadaxp_init_sensor(struct armada_thermal_priv *priv)
+{
+	unsigned long reg;
+
+	/* ??? */
+	reg = readl_relaxed(priv->control);
+	reg |= PMU_TDC0_OTF_CAL_MASK;
+	writel(reg, priv->control);
+
+	/* Reference calibration value */
+	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
+	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
+	writel(reg, priv->control);
+
+	/* Reset the sensor */
+	reg = readl_relaxed(priv->control);
+	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control);
+
+	writel(reg, priv->control);
+
+	/* Enable the sensor */
+	reg = readl_relaxed(priv->sensor);
+	reg &= ~PMU_TM_DISABLE_MASK;
+	writel(reg, priv->sensor);
+}
+
+static void armada370_init_sensor(struct armada_thermal_priv *priv)
+{
+	unsigned long reg;
+
+	/* ??? */
+	reg = readl_relaxed(priv->control);
+	reg |= PMU_TDC0_OTF_CAL_MASK;
+	writel(reg, priv->control);
+
+	/* Reference calibration value */
+	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
+	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
+	writel(reg, priv->control);
+
+	/* ??? */
+	reg &= ~PMU_TDC0_START_CAL_MASK;
+	writel(reg, priv->control);
+
+	/* FIXME: Why do we need this delay? */
+	mdelay(10);
+}
+
+static bool armada_is_valid(struct armada_thermal_priv *priv)
+{
+	unsigned long reg = readl_relaxed(priv->sensor);
+
+	return (reg >> THERMAL_VALID_OFFSET) & THERMAL_VALID_MASK;
+}
+
+static int armada_get_temp(struct thermal_zone_device *thermal,
+			  unsigned long *temp)
+{
+	struct armada_thermal_priv *priv = thermal->devdata;
+	unsigned long reg;
+
+	/* Valid check */
+	if (priv->ops->is_valid && !priv->ops->is_valid(priv)) {
+		dev_err(&thermal->device,
+			"Temperature sensor reading not valid\n");
+		return -EIO;
+	}
+
+	reg = (readl_relaxed(priv->sensor) >> THERMAL_TEMP_OFFSET) &
+		THERMAL_TEMP_MASK;
+	*temp = (3153000000UL - (10000000UL*reg)) / 13825;
+	return 0;
+}
+
+static struct thermal_zone_device_ops ops = {
+	.get_temp = armada_get_temp,
+};
+
+static const struct armada_thermal_ops armadaxp_ops = {
+	.init_sensor = armadaxp_init_sensor,
+};
+
+static const struct armada_thermal_ops armada370_ops = {
+	.is_valid = armada_is_valid,
+	.init_sensor = armada370_init_sensor,
+};
+
+static const struct of_device_id armada_thermal_id_table[] = {
+	{
+		.compatible = "marvell,armadaxp-thermal",
+		.data       = &armadaxp_ops,
+	},
+	{
+		.compatible = "marvell,armada370-thermal",
+		.data       = &armada370_ops,
+	},
+	{
+		/* sentinel */
+	},
+};
+MODULE_DEVICE_TABLE(of, armada_thermal_id_table);
+
+static int armada_thermal_probe(struct platform_device *pdev)
+{
+	struct thermal_zone_device *thermal;
+	const struct of_device_id *match;
+	struct armada_thermal_priv *priv;
+	struct resource *res;
+
+	match = of_match_device(armada_thermal_id_table, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Failed to get platform resource\n");
+		return -ENODEV;
+	}
+
+	priv->sensor = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->sensor))
+		return PTR_ERR(priv->sensor);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res) {
+		dev_err(&pdev->dev, "Failed to get platform resource\n");
+		return -ENODEV;
+	}
+
+	priv->control = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->control))
+		return PTR_ERR(priv->control);
+
+	priv->ops = (struct armada_thermal_ops *)match->data;
+	priv->ops->init_sensor(priv);
+
+	thermal = thermal_zone_device_register("armada_thermal", 0, 0,
+					       priv, &ops, NULL, 0, 0);
+	if (IS_ERR(thermal)) {
+		dev_err(&pdev->dev,
+			"Failed to register thermal zone device\n");
+		return PTR_ERR(thermal);
+	}
+
+	platform_set_drvdata(pdev, thermal);
+
+	return 0;
+}
+
+static int armada_thermal_exit(struct platform_device *pdev)
+{
+	struct thermal_zone_device *armada_thermal =
+		platform_get_drvdata(pdev);
+
+	thermal_zone_device_unregister(armada_thermal);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct platform_driver armada_thermal_driver = {
+	.probe = armada_thermal_probe,
+	.remove = armada_thermal_exit,
+	.driver = {
+		.name = "armada_thermal",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(armada_thermal_id_table),
+	},
+};
+
+module_platform_driver(armada_thermal_driver);
+
+MODULE_AUTHOR("Ezequiel Garcia <ezequiel.garcia@free-electrons.com>");
+MODULE_DESCRIPTION("Armada 370/XP thermal driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.8.6

[PATCH 2/4] ARM: mvebu: Add thermal support to Armada XP device tree

[Ezequiel Garcia]

This patch adds support for the thermal controller available in
all Armada XP boards. This controller has two 4-byte registers:
one to read the thermal sensor, the other for sensor initialization.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
This patch depends on the patch to support Armada XP thermal driver:
'thermal: Add driver for Armada 370/XP SoC thermal management'

 arch/arm/boot/dts/armada-xp.dtsi |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index ca00d83..29dfeb6 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -151,5 +151,11 @@
 			status = "disabled";
 		};
 
+		thermal@d00182b0 {
+			compatible = "marvell,armadaxp-thermal";
+			reg = <0xd00182b0 0x4
+			       0xd00184d0 0x4>;
+			status = "okay";
+		};
 	};
 };
-- 
1.7.8.6

[PATCH 2/4] ARM: mvebu: Add thermal support to Armada XP device tree

[Ezequiel Garcia]

This patch adds support for the thermal controller available in
all Armada XP boards. This controller has two 4-byte registers:
one to read the thermal sensor, the other for sensor initialization.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
This patch depends on the patch to support Armada XP thermal driver:
'thermal: Add driver for Armada 370/XP SoC thermal management'

 arch/arm/boot/dts/armada-xp.dtsi |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index ca00d83..29dfeb6 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -151,5 +151,11 @@
 			status = "disabled";
 		};
 
+		thermal at d00182b0 {
+			compatible = "marvell,armadaxp-thermal";
+			reg = <0xd00182b0 0x4
+			       0xd00184d0 0x4>;
+			status = "okay";
+		};
 	};
 };
-- 
1.7.8.6

[PATCH 3/4] ARM: mvebu: Add thermal support to Armada 370 device tree

[Ezequiel Garcia]

This patch adds support for the thermal controller available in
all Armada 370 boards. This controller has two 4-byte registers:
one to read the thermal sensor, the other for sensor initialization.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
This patch depends on the patch to support Armada 370 thermal driver:
'thermal: Add driver for Armada 370/XP SoC thermal management'

 arch/arm/boot/dts/armada-370.dtsi |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index 8188d13..5831994 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -153,5 +153,11 @@
 			clocks = <&coreclk 0>;
 		};
 
+		thermal@d0018300 {
+			compatible = "marvell,armada370-thermal";
+			reg = <0xd0018300 0x4
+			       0xd0018304 0x4>;
+			status = "okay";
+		};
 	};
 };
-- 
1.7.8.6

[PATCH 3/4] ARM: mvebu: Add thermal support to Armada 370 device tree

[Ezequiel Garcia]

This patch adds support for the thermal controller available in
all Armada 370 boards. This controller has two 4-byte registers:
one to read the thermal sensor, the other for sensor initialization.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
This patch depends on the patch to support Armada 370 thermal driver:
'thermal: Add driver for Armada 370/XP SoC thermal management'

 arch/arm/boot/dts/armada-370.dtsi |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index 8188d13..5831994 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -153,5 +153,11 @@
 			clocks = <&coreclk 0>;
 		};
 
+		thermal at d0018300 {
+			compatible = "marvell,armada370-thermal";
+			reg = <0xd0018300 0x4
+			       0xd0018304 0x4>;
+			status = "okay";
+		};
 	};
 };
-- 
1.7.8.6

[PATCH 4/4] ARM: configs: Update mvebu defconfig for thermal

[Ezequiel Garcia]

The thermal management driver for Armada XP/370 has been added
so we update the defconfig to reflect this.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/configs/mvebu_defconfig |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/configs/mvebu_defconfig b/arch/arm/configs/mvebu_defconfig
index 2ec8119..fc7e8be 100644
--- a/arch/arm/configs/mvebu_defconfig
+++ b/arch/arm/configs/mvebu_defconfig
@@ -49,6 +49,8 @@ CONFIG_MTD_M25P80=y
 CONFIG_SERIAL_8250_DW=y
 CONFIG_GPIOLIB=y
 CONFIG_GPIO_SYSFS=y
+CONFIG_THERMAL=y
+CONFIG_ARMADA_THERMAL=y
 CONFIG_USB_SUPPORT=y
 CONFIG_USB=y
 CONFIG_USB_EHCI_HCD=y
-- 
1.7.8.6

[PATCH 4/4] ARM: configs: Update mvebu defconfig for thermal

[Ezequiel Garcia]

The thermal management driver for Armada XP/370 has been added
so we update the defconfig to reflect this.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/configs/mvebu_defconfig |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/configs/mvebu_defconfig b/arch/arm/configs/mvebu_defconfig
index 2ec8119..fc7e8be 100644
--- a/arch/arm/configs/mvebu_defconfig
+++ b/arch/arm/configs/mvebu_defconfig
@@ -49,6 +49,8 @@ CONFIG_MTD_M25P80=y
 CONFIG_SERIAL_8250_DW=y
 CONFIG_GPIOLIB=y
 CONFIG_GPIO_SYSFS=y
+CONFIG_THERMAL=y
+CONFIG_ARMADA_THERMAL=y
 CONFIG_USB_SUPPORT=y
 CONFIG_USB=y
 CONFIG_USB_EHCI_HCD=y
-- 
1.7.8.6

RE: [PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal management

[R, Durgadoss]

> -----Original Message-----
> From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> owner@vger.kernel.org] On Behalf Of Ezequiel Garcia
> Sent: Monday, March 25, 2013 8:57 PM
> To: linux-arm-kernel@lists.infradead.org; linux-pm@vger.kernel.org
> Cc: Lior Amsalem; Nobuhiro Iwamatsu; Zhang, Rui; Sebastian Hesselbarth;
> Andrew Lunn; Thomas Petazzoni; Gregory Clement; Jason Cooper; Ezequiel
> Garcia
> Subject: [PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal
> management
> 
> This driver supports both Armada 370 and Armada XP SoC
> thermal management controllers.
> 
> Armada 370 has a register to check a valid temperature, whereas
> Armada XP does not. Each has a different initialization (i.e. calibration)
> function. The temperature conversion formula is the same for both.
> 
> The controller present in each SoC have a very similar feature set,
> so it corresponds to have one driver to support both of them.
> 
> Although this driver may present similarities to Dove and Kirkwood
> thermal driver, the exact differences and coincidences are not fully
> known. For this reason, support is given through a separate driver.

Nice commit message, that has all the details :-)

> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  .../devicetree/bindings/thermal/armada-thermal.txt |   22 ++
>  drivers/thermal/Kconfig                            |    8 +
>  drivers/thermal/Makefile                           |    1 +
>  drivers/thermal/armada_thermal.c                   |  236
> ++++++++++++++++++++
>  4 files changed, 267 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/thermal/armada-
> thermal.txt
>  create mode 100644 drivers/thermal/armada_thermal.c
> 
> diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> new file mode 100644
> index 0000000..fff93d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> @@ -0,0 +1,22 @@
> +* Marvell Armada 370/XP thermal management
> +
> +Required properties:
> +
> +- compatible:	Should be set to one of the following:
> +		marvell,armada370-thermal
> +		marvell,armadaxp-thermal
> +
> +- reg:		Device's register space.
> +		Two entries are expected, see the examples below.
> +		The first one is required for the sensor register;
> +		the second one is required for the control register
> +		to be used for sensor initialization (a.k.a. calibration).
> +
> +Example:
> +
> +	thermal@d0018300 {
> +		compatible = "marvell,armada370-thermal";
> +                reg = <0xd0018300 0x4
> +		       0xd0018304 0x4>;
> +		status = "okay";
> +	};
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index a764f16..fcd5a27 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -134,6 +134,14 @@ config DOVE_THERMAL
>  	  Support for the Dove thermal sensor driver in the Linux thermal
>  	  framework.
> 
> +config ARMADA_THERMAL
> +	tristate "Armada 370/XP thermal management"
> +	depends on ARCH_MVEBU
> +	depends on OF
> +	help
> +	  Enable this option if you want to have support for thermal
> management
> +	  controller present in Armada 370 and Armada XP SoC.
> +
>  config DB8500_THERMAL
>  	bool "DB8500 thermal management"
>  	depends on ARCH_U8500
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index d3a2b38..4209054 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_THERMAL_GOV_USER_SPACE)	+=
> user_space.o
>  obj-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
> 
>  # platform thermal drivers
> +obj-$(CONFIG_ARMADA_THERMAL)	+= armada_thermal.o
>  obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
>  obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
>  obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
> diff --git a/drivers/thermal/armada_thermal.c
> b/drivers/thermal/armada_thermal.c
> new file mode 100644
> index 0000000..dc83a86
> --- /dev/null
> +++ b/drivers/thermal/armada_thermal.c
> @@ -0,0 +1,236 @@
> +/*
> + * Marvell Armada 370/XP thermal sensor driver
> + *
> + * Copyright (C) 2013 Marvell
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_device.h>
> +#include <linux/thermal.h>
> +
> +#define THERMAL_VALID_OFFSET		9
> +#define THERMAL_VALID_MASK		0x1
> +#define THERMAL_TEMP_OFFSET		10
> +#define THERMAL_TEMP_MASK		0x1ff
> +
> +/* Thermal Manager Control and Status Register */
> +#define PMU_TDC0_SW_RST_MASK		(0x1 << 1)
> +#define PMU_TM_DISABLE_OFFS		0
> +#define PMU_TM_DISABLE_MASK		(0x1 <<
> PMU_TM_DISABLE_OFFS)
> +#define PMU_TDC0_REF_CAL_CNT_OFFS	11
> +#define PMU_TDC0_REF_CAL_CNT_MASK	(0x1ff <<
> PMU_TDC0_REF_CAL_CNT_OFFS)
> +#define PMU_TDC0_OTF_CAL_MASK		(0x1 << 30)
> +#define PMU_TDC0_START_CAL_MASK		(0x1 << 25)
> +
> +struct armada_thermal_ops;
> +
> +/* Marvell EBU Thermal Sensor Dev Structure */
> +struct armada_thermal_priv {
> +	void __iomem *sensor;
> +	void __iomem *control;
> +	struct armada_thermal_ops *ops;
> +};
> +
> +struct armada_thermal_ops {
> +	/* Initialize the sensor */
> +	void (*init_sensor)(struct armada_thermal_priv *);
> +
> +	/* Test for a valid sensor value (optional) */
> +	bool (*is_valid)(struct armada_thermal_priv *);
> +};
> +
> +static void armadaxp_init_sensor(struct armada_thermal_priv *priv)
> +{
> +	unsigned long reg;
> +
> +	/* ??? */
> +	reg = readl_relaxed(priv->control);
> +	reg |= PMU_TDC0_OTF_CAL_MASK;
> +	writel(reg, priv->control);
> +
> +	/* Reference calibration value */
> +	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> +	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> +	writel(reg, priv->control);

I see these two blocks of code being the same for the below
function as well. Any specific reason for not making this block 
as a common function and calling it from both the 
_init_sensor functions ?

> +
> +	/* Reset the sensor */
> +	reg = readl_relaxed(priv->control);
> +	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control);
> +
> +	writel(reg, priv->control);
> +
> +	/* Enable the sensor */
> +	reg = readl_relaxed(priv->sensor);
> +	reg &= ~PMU_TM_DISABLE_MASK;
> +	writel(reg, priv->sensor);
> +}
> +
> +static void armada370_init_sensor(struct armada_thermal_priv *priv)
> +{
> +	unsigned long reg;
> +
> +	/* ??? */
> +	reg = readl_relaxed(priv->control);
> +	reg |= PMU_TDC0_OTF_CAL_MASK;
> +	writel(reg, priv->control);
> +
> +	/* Reference calibration value */
> +	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> +	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> +	writel(reg, priv->control);
> +
> +	/* ??? */
> +	reg &= ~PMU_TDC0_START_CAL_MASK;
> +	writel(reg, priv->control);
> +
> +	/* FIXME: Why do we need this delay? */
> +	mdelay(10);
> +}
> +
> +static bool armada_is_valid(struct armada_thermal_priv *priv)
> +{
> +	unsigned long reg = readl_relaxed(priv->sensor);
> +
> +	return (reg >> THERMAL_VALID_OFFSET) & THERMAL_VALID_MASK;
> +}
> +
> +static int armada_get_temp(struct thermal_zone_device *thermal,
> +			  unsigned long *temp)
> +{
> +	struct armada_thermal_priv *priv = thermal->devdata;
> +	unsigned long reg;
> +
> +	/* Valid check */
> +	if (priv->ops->is_valid && !priv->ops->is_valid(priv)) {
> +		dev_err(&thermal->device,
> +			"Temperature sensor reading not valid\n");
> +		return -EIO;
> +	}
> +
> +	reg = (readl_relaxed(priv->sensor) >> THERMAL_TEMP_OFFSET) &

Can we have the readl_relaxed call as a separate statement ?

> +		THERMAL_TEMP_MASK;
> +	*temp = (3153000000UL - (10000000UL*reg)) / 13825;

If I substitute 1 for 'reg' I get  227341.7721... 
Does this mean the temperature is 227 C ??

If you have the info, can you add a comment on what is the
valid range that 'reg' can take ?

Also, Is the resulting temperature
in MillidegreeCelsius ? If so, please add a comment saying so.

Thanks,
Durga

> +	return 0;
> +}
> +
> +static struct thermal_zone_device_ops ops = {
> +	.get_temp = armada_get_temp,
> +};
> +
> +static const struct armada_thermal_ops armadaxp_ops = {
> +	.init_sensor = armadaxp_init_sensor,
> +};
> +
> +static const struct armada_thermal_ops armada370_ops = {
> +	.is_valid = armada_is_valid,
> +	.init_sensor = armada370_init_sensor,
> +};
> +
> +static const struct of_device_id armada_thermal_id_table[] = {
> +	{
> +		.compatible = "marvell,armadaxp-thermal",
> +		.data       = &armadaxp_ops,
> +	},
> +	{
> +		.compatible = "marvell,armada370-thermal",
> +		.data       = &armada370_ops,
> +	},
> +	{
> +		/* sentinel */
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, armada_thermal_id_table);
> +
> +static int armada_thermal_probe(struct platform_device *pdev)
> +{
> +	struct thermal_zone_device *thermal;
> +	const struct of_device_id *match;
> +	struct armada_thermal_priv *priv;
> +	struct resource *res;
> +
> +	match = of_match_device(armada_thermal_id_table, &pdev->dev);
> +	if (!match)
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get platform resource\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->sensor = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->sensor))
> +		return PTR_ERR(priv->sensor);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get platform resource\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->control = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->control))
> +		return PTR_ERR(priv->control);
> +
> +	priv->ops = (struct armada_thermal_ops *)match->data;
> +	priv->ops->init_sensor(priv);
> +
> +	thermal = thermal_zone_device_register("armada_thermal", 0, 0,
> +					       priv, &ops, NULL, 0, 0);
> +	if (IS_ERR(thermal)) {
> +		dev_err(&pdev->dev,
> +			"Failed to register thermal zone device\n");
> +		return PTR_ERR(thermal);
> +	}
> +
> +	platform_set_drvdata(pdev, thermal);
> +
> +	return 0;
> +}
> +
> +static int armada_thermal_exit(struct platform_device *pdev)
> +{
> +	struct thermal_zone_device *armada_thermal =
> +		platform_get_drvdata(pdev);
> +
> +	thermal_zone_device_unregister(armada_thermal);
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver armada_thermal_driver = {
> +	.probe = armada_thermal_probe,
> +	.remove = armada_thermal_exit,
> +	.driver = {
> +		.name = "armada_thermal",
> +		.owner = THIS_MODULE,
> +		.of_match_table =
> of_match_ptr(armada_thermal_id_table),
> +	},
> +};
> +
> +module_platform_driver(armada_thermal_driver);
> +
> +MODULE_AUTHOR("Ezequiel Garcia <ezequiel.garcia@free-
> electrons.com>");
> +MODULE_DESCRIPTION("Armada 370/XP thermal driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.7.8.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal management

[R, Durgadoss]

> -----Original Message-----
> From: linux-pm-owner at vger.kernel.org [mailto:linux-pm-
> owner at vger.kernel.org] On Behalf Of Ezequiel Garcia
> Sent: Monday, March 25, 2013 8:57 PM
> To: linux-arm-kernel at lists.infradead.org; linux-pm at vger.kernel.org
> Cc: Lior Amsalem; Nobuhiro Iwamatsu; Zhang, Rui; Sebastian Hesselbarth;
> Andrew Lunn; Thomas Petazzoni; Gregory Clement; Jason Cooper; Ezequiel
> Garcia
> Subject: [PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal
> management
> 
> This driver supports both Armada 370 and Armada XP SoC
> thermal management controllers.
> 
> Armada 370 has a register to check a valid temperature, whereas
> Armada XP does not. Each has a different initialization (i.e. calibration)
> function. The temperature conversion formula is the same for both.
> 
> The controller present in each SoC have a very similar feature set,
> so it corresponds to have one driver to support both of them.
> 
> Although this driver may present similarities to Dove and Kirkwood
> thermal driver, the exact differences and coincidences are not fully
> known. For this reason, support is given through a separate driver.

Nice commit message, that has all the details :-)

> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  .../devicetree/bindings/thermal/armada-thermal.txt |   22 ++
>  drivers/thermal/Kconfig                            |    8 +
>  drivers/thermal/Makefile                           |    1 +
>  drivers/thermal/armada_thermal.c                   |  236
> ++++++++++++++++++++
>  4 files changed, 267 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/thermal/armada-
> thermal.txt
>  create mode 100644 drivers/thermal/armada_thermal.c
> 
> diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> new file mode 100644
> index 0000000..fff93d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> @@ -0,0 +1,22 @@
> +* Marvell Armada 370/XP thermal management
> +
> +Required properties:
> +
> +- compatible:	Should be set to one of the following:
> +		marvell,armada370-thermal
> +		marvell,armadaxp-thermal
> +
> +- reg:		Device's register space.
> +		Two entries are expected, see the examples below.
> +		The first one is required for the sensor register;
> +		the second one is required for the control register
> +		to be used for sensor initialization (a.k.a. calibration).
> +
> +Example:
> +
> +	thermal at d0018300 {
> +		compatible = "marvell,armada370-thermal";
> +                reg = <0xd0018300 0x4
> +		       0xd0018304 0x4>;
> +		status = "okay";
> +	};
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index a764f16..fcd5a27 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -134,6 +134,14 @@ config DOVE_THERMAL
>  	  Support for the Dove thermal sensor driver in the Linux thermal
>  	  framework.
> 
> +config ARMADA_THERMAL
> +	tristate "Armada 370/XP thermal management"
> +	depends on ARCH_MVEBU
> +	depends on OF
> +	help
> +	  Enable this option if you want to have support for thermal
> management
> +	  controller present in Armada 370 and Armada XP SoC.
> +
>  config DB8500_THERMAL
>  	bool "DB8500 thermal management"
>  	depends on ARCH_U8500
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index d3a2b38..4209054 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_THERMAL_GOV_USER_SPACE)	+=
> user_space.o
>  obj-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
> 
>  # platform thermal drivers
> +obj-$(CONFIG_ARMADA_THERMAL)	+= armada_thermal.o
>  obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
>  obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
>  obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
> diff --git a/drivers/thermal/armada_thermal.c
> b/drivers/thermal/armada_thermal.c
> new file mode 100644
> index 0000000..dc83a86
> --- /dev/null
> +++ b/drivers/thermal/armada_thermal.c
> @@ -0,0 +1,236 @@
> +/*
> + * Marvell Armada 370/XP thermal sensor driver
> + *
> + * Copyright (C) 2013 Marvell
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_device.h>
> +#include <linux/thermal.h>
> +
> +#define THERMAL_VALID_OFFSET		9
> +#define THERMAL_VALID_MASK		0x1
> +#define THERMAL_TEMP_OFFSET		10
> +#define THERMAL_TEMP_MASK		0x1ff
> +
> +/* Thermal Manager Control and Status Register */
> +#define PMU_TDC0_SW_RST_MASK		(0x1 << 1)
> +#define PMU_TM_DISABLE_OFFS		0
> +#define PMU_TM_DISABLE_MASK		(0x1 <<
> PMU_TM_DISABLE_OFFS)
> +#define PMU_TDC0_REF_CAL_CNT_OFFS	11
> +#define PMU_TDC0_REF_CAL_CNT_MASK	(0x1ff <<
> PMU_TDC0_REF_CAL_CNT_OFFS)
> +#define PMU_TDC0_OTF_CAL_MASK		(0x1 << 30)
> +#define PMU_TDC0_START_CAL_MASK		(0x1 << 25)
> +
> +struct armada_thermal_ops;
> +
> +/* Marvell EBU Thermal Sensor Dev Structure */
> +struct armada_thermal_priv {
> +	void __iomem *sensor;
> +	void __iomem *control;
> +	struct armada_thermal_ops *ops;
> +};
> +
> +struct armada_thermal_ops {
> +	/* Initialize the sensor */
> +	void (*init_sensor)(struct armada_thermal_priv *);
> +
> +	/* Test for a valid sensor value (optional) */
> +	bool (*is_valid)(struct armada_thermal_priv *);
> +};
> +
> +static void armadaxp_init_sensor(struct armada_thermal_priv *priv)
> +{
> +	unsigned long reg;
> +
> +	/* ??? */
> +	reg = readl_relaxed(priv->control);
> +	reg |= PMU_TDC0_OTF_CAL_MASK;
> +	writel(reg, priv->control);
> +
> +	/* Reference calibration value */
> +	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> +	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> +	writel(reg, priv->control);

I see these two blocks of code being the same for the below
function as well. Any specific reason for not making this block 
as a common function and calling it from both the 
_init_sensor functions ?

> +
> +	/* Reset the sensor */
> +	reg = readl_relaxed(priv->control);
> +	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control);
> +
> +	writel(reg, priv->control);
> +
> +	/* Enable the sensor */
> +	reg = readl_relaxed(priv->sensor);
> +	reg &= ~PMU_TM_DISABLE_MASK;
> +	writel(reg, priv->sensor);
> +}
> +
> +static void armada370_init_sensor(struct armada_thermal_priv *priv)
> +{
> +	unsigned long reg;
> +
> +	/* ??? */
> +	reg = readl_relaxed(priv->control);
> +	reg |= PMU_TDC0_OTF_CAL_MASK;
> +	writel(reg, priv->control);
> +
> +	/* Reference calibration value */
> +	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> +	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> +	writel(reg, priv->control);
> +
> +	/* ??? */
> +	reg &= ~PMU_TDC0_START_CAL_MASK;
> +	writel(reg, priv->control);
> +
> +	/* FIXME: Why do we need this delay? */
> +	mdelay(10);
> +}
> +
> +static bool armada_is_valid(struct armada_thermal_priv *priv)
> +{
> +	unsigned long reg = readl_relaxed(priv->sensor);
> +
> +	return (reg >> THERMAL_VALID_OFFSET) & THERMAL_VALID_MASK;
> +}
> +
> +static int armada_get_temp(struct thermal_zone_device *thermal,
> +			  unsigned long *temp)
> +{
> +	struct armada_thermal_priv *priv = thermal->devdata;
> +	unsigned long reg;
> +
> +	/* Valid check */
> +	if (priv->ops->is_valid && !priv->ops->is_valid(priv)) {
> +		dev_err(&thermal->device,
> +			"Temperature sensor reading not valid\n");
> +		return -EIO;
> +	}
> +
> +	reg = (readl_relaxed(priv->sensor) >> THERMAL_TEMP_OFFSET) &

Can we have the readl_relaxed call as a separate statement ?

> +		THERMAL_TEMP_MASK;
> +	*temp = (3153000000UL - (10000000UL*reg)) / 13825;

If I substitute 1 for 'reg' I get  227341.7721... 
Does this mean the temperature is 227 C ??

If you have the info, can you add a comment on what is the
valid range that 'reg' can take ?

Also, Is the resulting temperature
in MillidegreeCelsius ? If so, please add a comment saying so.

Thanks,
Durga

> +	return 0;
> +}
> +
> +static struct thermal_zone_device_ops ops = {
> +	.get_temp = armada_get_temp,
> +};
> +
> +static const struct armada_thermal_ops armadaxp_ops = {
> +	.init_sensor = armadaxp_init_sensor,
> +};
> +
> +static const struct armada_thermal_ops armada370_ops = {
> +	.is_valid = armada_is_valid,
> +	.init_sensor = armada370_init_sensor,
> +};
> +
> +static const struct of_device_id armada_thermal_id_table[] = {
> +	{
> +		.compatible = "marvell,armadaxp-thermal",
> +		.data       = &armadaxp_ops,
> +	},
> +	{
> +		.compatible = "marvell,armada370-thermal",
> +		.data       = &armada370_ops,
> +	},
> +	{
> +		/* sentinel */
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, armada_thermal_id_table);
> +
> +static int armada_thermal_probe(struct platform_device *pdev)
> +{
> +	struct thermal_zone_device *thermal;
> +	const struct of_device_id *match;
> +	struct armada_thermal_priv *priv;
> +	struct resource *res;
> +
> +	match = of_match_device(armada_thermal_id_table, &pdev->dev);
> +	if (!match)
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get platform resource\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->sensor = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->sensor))
> +		return PTR_ERR(priv->sensor);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get platform resource\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->control = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv->control))
> +		return PTR_ERR(priv->control);
> +
> +	priv->ops = (struct armada_thermal_ops *)match->data;
> +	priv->ops->init_sensor(priv);
> +
> +	thermal = thermal_zone_device_register("armada_thermal", 0, 0,
> +					       priv, &ops, NULL, 0, 0);
> +	if (IS_ERR(thermal)) {
> +		dev_err(&pdev->dev,
> +			"Failed to register thermal zone device\n");
> +		return PTR_ERR(thermal);
> +	}
> +
> +	platform_set_drvdata(pdev, thermal);
> +
> +	return 0;
> +}
> +
> +static int armada_thermal_exit(struct platform_device *pdev)
> +{
> +	struct thermal_zone_device *armada_thermal =
> +		platform_get_drvdata(pdev);
> +
> +	thermal_zone_device_unregister(armada_thermal);
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver armada_thermal_driver = {
> +	.probe = armada_thermal_probe,
> +	.remove = armada_thermal_exit,
> +	.driver = {
> +		.name = "armada_thermal",
> +		.owner = THIS_MODULE,
> +		.of_match_table =
> of_match_ptr(armada_thermal_id_table),
> +	},
> +};
> +
> +module_platform_driver(armada_thermal_driver);
> +
> +MODULE_AUTHOR("Ezequiel Garcia <ezequiel.garcia@free-
> electrons.com>");
> +MODULE_DESCRIPTION("Armada 370/XP thermal driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.7.8.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal management

[Andrew Lunn]

On Mon, Mar 25, 2013 at 12:27:06PM -0300, Ezequiel Garcia wrote:
> This driver supports both Armada 370 and Armada XP SoC
> thermal management controllers.
> 
> Armada 370 has a register to check a valid temperature, whereas
> Armada XP does not. Each has a different initialization (i.e. calibration)
> function. The temperature conversion formula is the same for both.
> 
> The controller present in each SoC have a very similar feature set,
> so it corresponds to have one driver to support both of them.
> 
> Although this driver may present similarities to Dove and Kirkwood
> thermal driver, the exact differences and coincidences are not fully
> known. For this reason, support is given through a separate driver.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  .../devicetree/bindings/thermal/armada-thermal.txt |   22 ++
>  drivers/thermal/Kconfig                            |    8 +
>  drivers/thermal/Makefile                           |    1 +
>  drivers/thermal/armada_thermal.c                   |  236 ++++++++++++++++++++
>  4 files changed, 267 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/thermal/armada-thermal.txt
>  create mode 100644 drivers/thermal/armada_thermal.c
> 
> diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> new file mode 100644
> index 0000000..fff93d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> @@ -0,0 +1,22 @@
> +* Marvell Armada 370/XP thermal management
> +
> +Required properties:
> +
> +- compatible:	Should be set to one of the following:
> +		marvell,armada370-thermal
> +		marvell,armadaxp-thermal
> +
> +- reg:		Device's register space.
> +		Two entries are expected, see the examples below.
> +		The first one is required for the sensor register;
> +		the second one is required for the control register
> +		to be used for sensor initialization (a.k.a. calibration).
> +
> +Example:
> +
> +	thermal@d0018300 {
> +		compatible = "marvell,armada370-thermal";
> +                reg = <0xd0018300 0x4
> +		       0xd0018304 0x4>;
> +		status = "okay";
> +	};
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index a764f16..fcd5a27 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -134,6 +134,14 @@ config DOVE_THERMAL
>  	  Support for the Dove thermal sensor driver in the Linux thermal
>  	  framework.
>  
> +config ARMADA_THERMAL
> +	tristate "Armada 370/XP thermal management"
> +	depends on ARCH_MVEBU
> +	depends on OF
> +	help
> +	  Enable this option if you want to have support for thermal management
> +	  controller present in Armada 370 and Armada XP SoC.
> +

Hi Ezequiel

For some unknown reason, the Kconfig entries are in reverse alphabetic
order. So you should insert this entry after DB8500. Same for the Makefile.

       Andrew

[PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal management

[Andrew Lunn]

On Mon, Mar 25, 2013 at 12:27:06PM -0300, Ezequiel Garcia wrote:
> This driver supports both Armada 370 and Armada XP SoC
> thermal management controllers.
> 
> Armada 370 has a register to check a valid temperature, whereas
> Armada XP does not. Each has a different initialization (i.e. calibration)
> function. The temperature conversion formula is the same for both.
> 
> The controller present in each SoC have a very similar feature set,
> so it corresponds to have one driver to support both of them.
> 
> Although this driver may present similarities to Dove and Kirkwood
> thermal driver, the exact differences and coincidences are not fully
> known. For this reason, support is given through a separate driver.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  .../devicetree/bindings/thermal/armada-thermal.txt |   22 ++
>  drivers/thermal/Kconfig                            |    8 +
>  drivers/thermal/Makefile                           |    1 +
>  drivers/thermal/armada_thermal.c                   |  236 ++++++++++++++++++++
>  4 files changed, 267 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/thermal/armada-thermal.txt
>  create mode 100644 drivers/thermal/armada_thermal.c
> 
> diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> new file mode 100644
> index 0000000..fff93d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> @@ -0,0 +1,22 @@
> +* Marvell Armada 370/XP thermal management
> +
> +Required properties:
> +
> +- compatible:	Should be set to one of the following:
> +		marvell,armada370-thermal
> +		marvell,armadaxp-thermal
> +
> +- reg:		Device's register space.
> +		Two entries are expected, see the examples below.
> +		The first one is required for the sensor register;
> +		the second one is required for the control register
> +		to be used for sensor initialization (a.k.a. calibration).
> +
> +Example:
> +
> +	thermal at d0018300 {
> +		compatible = "marvell,armada370-thermal";
> +                reg = <0xd0018300 0x4
> +		       0xd0018304 0x4>;
> +		status = "okay";
> +	};
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index a764f16..fcd5a27 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -134,6 +134,14 @@ config DOVE_THERMAL
>  	  Support for the Dove thermal sensor driver in the Linux thermal
>  	  framework.
>  
> +config ARMADA_THERMAL
> +	tristate "Armada 370/XP thermal management"
> +	depends on ARCH_MVEBU
> +	depends on OF
> +	help
> +	  Enable this option if you want to have support for thermal management
> +	  controller present in Armada 370 and Armada XP SoC.
> +

Hi Ezequiel

For some unknown reason, the Kconfig entries are in reverse alphabetic
order. So you should insert this entry after DB8500. Same for the Makefile.

       Andrew

Re: [PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal management

[Ezequiel Garcia]

Hi Durgadoss,

On Mon, Mar 25, 2013 at 05:27:24PM +0000, R, Durgadoss wrote:
[...]
> > +static void armadaxp_init_sensor(struct armada_thermal_priv *priv)
> > +{
> > +	unsigned long reg;
> > +
> > +	/* ??? */
> > +	reg = readl_relaxed(priv->control);
> > +	reg |= PMU_TDC0_OTF_CAL_MASK;
> > +	writel(reg, priv->control);
> > +
> > +	/* Reference calibration value */
> > +	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> > +	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> > +	writel(reg, priv->control);
> 
> I see these two blocks of code being the same for the below
> function as well. Any specific reason for not making this block 
> as a common function and calling it from both the 
> _init_sensor functions ?
> 

I think it's more clear if we define one init_sensor function per SoC.
The common code is really little and factor that out seems to me like
too much modularization.

> > +
> > +	/* Reset the sensor */
> > +	reg = readl_relaxed(priv->control);
> > +	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control);
> > +
> > +	writel(reg, priv->control);
> > +
> > +	/* Enable the sensor */
> > +	reg = readl_relaxed(priv->sensor);
> > +	reg &= ~PMU_TM_DISABLE_MASK;
> > +	writel(reg, priv->sensor);
> > +}
> > +
> > +static void armada370_init_sensor(struct armada_thermal_priv *priv)
> > +{
> > +	unsigned long reg;
> > +
> > +	/* ??? */
> > +	reg = readl_relaxed(priv->control);
> > +	reg |= PMU_TDC0_OTF_CAL_MASK;
> > +	writel(reg, priv->control);
> > +
> > +	/* Reference calibration value */
> > +	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> > +	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> > +	writel(reg, priv->control);
> > +
> > +	/* ??? */
> > +	reg &= ~PMU_TDC0_START_CAL_MASK;
> > +	writel(reg, priv->control);
> > +
> > +	/* FIXME: Why do we need this delay? */
> > +	mdelay(10);
> > +}
> > +
> > +static bool armada_is_valid(struct armada_thermal_priv *priv)
> > +{
> > +	unsigned long reg = readl_relaxed(priv->sensor);
> > +
> > +	return (reg >> THERMAL_VALID_OFFSET) & THERMAL_VALID_MASK;
> > +}
> > +
> > +static int armada_get_temp(struct thermal_zone_device *thermal,
> > +			  unsigned long *temp)
> > +{
> > +	struct armada_thermal_priv *priv = thermal->devdata;
> > +	unsigned long reg;
> > +
> > +	/* Valid check */
> > +	if (priv->ops->is_valid && !priv->ops->is_valid(priv)) {
> > +		dev_err(&thermal->device,
> > +			"Temperature sensor reading not valid\n");
> > +		return -EIO;
> > +	}
> > +
> > +	reg = (readl_relaxed(priv->sensor) >> THERMAL_TEMP_OFFSET) &
> 
> Can we have the readl_relaxed call as a separate statement ?
> 

Why would we want that? Do you think it'll be more readable?

> > +		THERMAL_TEMP_MASK;
> > +	*temp = (3153000000UL - (10000000UL*reg)) / 13825;
> 
> If I substitute 1 for 'reg' I get  227341.7721... 
> Does this mean the temperature is 227 C ??
> 

Yes, I guess so.

> If you have the info, can you add a comment on what is the
> valid range that 'reg' can take ?
>

No, I don't have the info. I guess that the valid range 'reg'
can take are the values that span a temperature between 25 ºC (or
lower if it's winter) and when your CPU is on fire :-)

> Also, Is the resulting temperature
> in MillidegreeCelsius ? If so, please add a comment saying so.
> 

Yes, the resulting temperature is in millidegree celsius,
as required by the thermal framework:

Documentation/thermal/sysfs-api.txt

Thanks for the review!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal management

[Ezequiel Garcia]

Hi Durgadoss,

On Mon, Mar 25, 2013 at 05:27:24PM +0000, R, Durgadoss wrote:
[...]
> > +static void armadaxp_init_sensor(struct armada_thermal_priv *priv)
> > +{
> > +	unsigned long reg;
> > +
> > +	/* ??? */
> > +	reg = readl_relaxed(priv->control);
> > +	reg |= PMU_TDC0_OTF_CAL_MASK;
> > +	writel(reg, priv->control);
> > +
> > +	/* Reference calibration value */
> > +	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> > +	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> > +	writel(reg, priv->control);
> 
> I see these two blocks of code being the same for the below
> function as well. Any specific reason for not making this block 
> as a common function and calling it from both the 
> _init_sensor functions ?
> 

I think it's more clear if we define one init_sensor function per SoC.
The common code is really little and factor that out seems to me like
too much modularization.

> > +
> > +	/* Reset the sensor */
> > +	reg = readl_relaxed(priv->control);
> > +	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control);
> > +
> > +	writel(reg, priv->control);
> > +
> > +	/* Enable the sensor */
> > +	reg = readl_relaxed(priv->sensor);
> > +	reg &= ~PMU_TM_DISABLE_MASK;
> > +	writel(reg, priv->sensor);
> > +}
> > +
> > +static void armada370_init_sensor(struct armada_thermal_priv *priv)
> > +{
> > +	unsigned long reg;
> > +
> > +	/* ??? */
> > +	reg = readl_relaxed(priv->control);
> > +	reg |= PMU_TDC0_OTF_CAL_MASK;
> > +	writel(reg, priv->control);
> > +
> > +	/* Reference calibration value */
> > +	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> > +	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> > +	writel(reg, priv->control);
> > +
> > +	/* ??? */
> > +	reg &= ~PMU_TDC0_START_CAL_MASK;
> > +	writel(reg, priv->control);
> > +
> > +	/* FIXME: Why do we need this delay? */
> > +	mdelay(10);
> > +}
> > +
> > +static bool armada_is_valid(struct armada_thermal_priv *priv)
> > +{
> > +	unsigned long reg = readl_relaxed(priv->sensor);
> > +
> > +	return (reg >> THERMAL_VALID_OFFSET) & THERMAL_VALID_MASK;
> > +}
> > +
> > +static int armada_get_temp(struct thermal_zone_device *thermal,
> > +			  unsigned long *temp)
> > +{
> > +	struct armada_thermal_priv *priv = thermal->devdata;
> > +	unsigned long reg;
> > +
> > +	/* Valid check */
> > +	if (priv->ops->is_valid && !priv->ops->is_valid(priv)) {
> > +		dev_err(&thermal->device,
> > +			"Temperature sensor reading not valid\n");
> > +		return -EIO;
> > +	}
> > +
> > +	reg = (readl_relaxed(priv->sensor) >> THERMAL_TEMP_OFFSET) &
> 
> Can we have the readl_relaxed call as a separate statement ?
> 

Why would we want that? Do you think it'll be more readable?

> > +		THERMAL_TEMP_MASK;
> > +	*temp = (3153000000UL - (10000000UL*reg)) / 13825;
> 
> If I substitute 1 for 'reg' I get  227341.7721... 
> Does this mean the temperature is 227 C ??
> 

Yes, I guess so.

> If you have the info, can you add a comment on what is the
> valid range that 'reg' can take ?
>

No, I don't have the info. I guess that the valid range 'reg'
can take are the values that span a temperature between 25 ?C (or
lower if it's winter) and when your CPU is on fire :-)

> Also, Is the resulting temperature
> in MillidegreeCelsius ? If so, please add a comment saying so.
> 

Yes, the resulting temperature is in millidegree celsius,
as required by the thermal framework:

Documentation/thermal/sysfs-api.txt

Thanks for the review!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

RE: [PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal management

[R, Durgadoss]

> -----Original Message-----
> From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> Sent: Tuesday, March 26, 2013 2:29 PM
> To: R, Durgadoss
> Cc: linux-arm-kernel@lists.infradead.org; linux-pm@vger.kernel.org; Lior
> Amsalem; Nobuhiro Iwamatsu; Zhang, Rui; Sebastian Hesselbarth; Andrew
> Lunn; Thomas Petazzoni; Gregory Clement; Jason Cooper
> Subject: Re: [PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal
> management
> 
> Hi Durgadoss,
> 
> On Mon, Mar 25, 2013 at 05:27:24PM +0000, R, Durgadoss wrote:
> [...]
> > > +static void armadaxp_init_sensor(struct armada_thermal_priv *priv)
> > > +{
> > > +	unsigned long reg;
> > > +
> > > +	/* ??? */
> > > +	reg = readl_relaxed(priv->control);
> > > +	reg |= PMU_TDC0_OTF_CAL_MASK;
> > > +	writel(reg, priv->control);
> > > +
> > > +	/* Reference calibration value */
> > > +	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> > > +	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> > > +	writel(reg, priv->control);
> >
> > I see these two blocks of code being the same for the below
> > function as well. Any specific reason for not making this block
> > as a common function and calling it from both the
> > _init_sensor functions ?
> >
> 
> I think it's more clear if we define one init_sensor function per SoC.
> The common code is really little and factor that out seems to me like
> too much modularization.

I would leave it up to you, then.

> 
> > > +
> > > +	/* Reset the sensor */
> > > +	reg = readl_relaxed(priv->control);
> > > +	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control);
> > > +
> > > +	writel(reg, priv->control);
> > > +
> > > +	/* Enable the sensor */
> > > +	reg = readl_relaxed(priv->sensor);
> > > +	reg &= ~PMU_TM_DISABLE_MASK;
> > > +	writel(reg, priv->sensor);
> > > +}
> > > +
> > > +static void armada370_init_sensor(struct armada_thermal_priv *priv)
> > > +{
> > > +	unsigned long reg;
> > > +
> > > +	/* ??? */
> > > +	reg = readl_relaxed(priv->control);
> > > +	reg |= PMU_TDC0_OTF_CAL_MASK;
> > > +	writel(reg, priv->control);
> > > +
> > > +	/* Reference calibration value */
> > > +	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> > > +	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> > > +	writel(reg, priv->control);
> > > +
> > > +	/* ??? */
> > > +	reg &= ~PMU_TDC0_START_CAL_MASK;
> > > +	writel(reg, priv->control);
> > > +
> > > +	/* FIXME: Why do we need this delay? */
> > > +	mdelay(10);
> > > +}
> > > +
> > > +static bool armada_is_valid(struct armada_thermal_priv *priv)
> > > +{
> > > +	unsigned long reg = readl_relaxed(priv->sensor);
> > > +
> > > +	return (reg >> THERMAL_VALID_OFFSET) & THERMAL_VALID_MASK;
> > > +}
> > > +
> > > +static int armada_get_temp(struct thermal_zone_device *thermal,
> > > +			  unsigned long *temp)
> > > +{
> > > +	struct armada_thermal_priv *priv = thermal->devdata;
> > > +	unsigned long reg;
> > > +
> > > +	/* Valid check */
> > > +	if (priv->ops->is_valid && !priv->ops->is_valid(priv)) {
> > > +		dev_err(&thermal->device,
> > > +			"Temperature sensor reading not valid\n");
> > > +		return -EIO;
> > > +	}
> > > +
> > > +	reg = (readl_relaxed(priv->sensor) >> THERMAL_TEMP_OFFSET) &
> >
> > Can we have the readl_relaxed call as a separate statement ?
> >
> 
> Why would we want that? Do you think it'll be more readable?

Yes,

> 
> > > +		THERMAL_TEMP_MASK;
> > > +	*temp = (3153000000UL - (10000000UL*reg)) / 13825;
> >
> > If I substitute 1 for 'reg' I get  227341.7721...
> > Does this mean the temperature is 227 C ??
> >
> 
> Yes, I guess so.
> 
> > If you have the info, can you add a comment on what is the
> > valid range that 'reg' can take ?
> >
> 
> No, I don't have the info. I guess that the valid range 'reg'
> can take are the values that span a temperature between 25 ºC (or
> lower if it's winter) and when your CPU is on fire :-)
> 
> > Also, Is the resulting temperature
> > in MillidegreeCelsius ? If so, please add a comment saying so.
> >
> 
> Yes, the resulting temperature is in millidegree celsius,
> as required by the thermal framework:

ok.

Thanks,
Durga

[PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal management

[R, Durgadoss]

> -----Original Message-----
> From: Ezequiel Garcia [mailto:ezequiel.garcia at free-electrons.com]
> Sent: Tuesday, March 26, 2013 2:29 PM
> To: R, Durgadoss
> Cc: linux-arm-kernel at lists.infradead.org; linux-pm at vger.kernel.org; Lior
> Amsalem; Nobuhiro Iwamatsu; Zhang, Rui; Sebastian Hesselbarth; Andrew
> Lunn; Thomas Petazzoni; Gregory Clement; Jason Cooper
> Subject: Re: [PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal
> management
> 
> Hi Durgadoss,
> 
> On Mon, Mar 25, 2013 at 05:27:24PM +0000, R, Durgadoss wrote:
> [...]
> > > +static void armadaxp_init_sensor(struct armada_thermal_priv *priv)
> > > +{
> > > +	unsigned long reg;
> > > +
> > > +	/* ??? */
> > > +	reg = readl_relaxed(priv->control);
> > > +	reg |= PMU_TDC0_OTF_CAL_MASK;
> > > +	writel(reg, priv->control);
> > > +
> > > +	/* Reference calibration value */
> > > +	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> > > +	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> > > +	writel(reg, priv->control);
> >
> > I see these two blocks of code being the same for the below
> > function as well. Any specific reason for not making this block
> > as a common function and calling it from both the
> > _init_sensor functions ?
> >
> 
> I think it's more clear if we define one init_sensor function per SoC.
> The common code is really little and factor that out seems to me like
> too much modularization.

I would leave it up to you, then.

> 
> > > +
> > > +	/* Reset the sensor */
> > > +	reg = readl_relaxed(priv->control);
> > > +	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control);
> > > +
> > > +	writel(reg, priv->control);
> > > +
> > > +	/* Enable the sensor */
> > > +	reg = readl_relaxed(priv->sensor);
> > > +	reg &= ~PMU_TM_DISABLE_MASK;
> > > +	writel(reg, priv->sensor);
> > > +}
> > > +
> > > +static void armada370_init_sensor(struct armada_thermal_priv *priv)
> > > +{
> > > +	unsigned long reg;
> > > +
> > > +	/* ??? */
> > > +	reg = readl_relaxed(priv->control);
> > > +	reg |= PMU_TDC0_OTF_CAL_MASK;
> > > +	writel(reg, priv->control);
> > > +
> > > +	/* Reference calibration value */
> > > +	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> > > +	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> > > +	writel(reg, priv->control);
> > > +
> > > +	/* ??? */
> > > +	reg &= ~PMU_TDC0_START_CAL_MASK;
> > > +	writel(reg, priv->control);
> > > +
> > > +	/* FIXME: Why do we need this delay? */
> > > +	mdelay(10);
> > > +}
> > > +
> > > +static bool armada_is_valid(struct armada_thermal_priv *priv)
> > > +{
> > > +	unsigned long reg = readl_relaxed(priv->sensor);
> > > +
> > > +	return (reg >> THERMAL_VALID_OFFSET) & THERMAL_VALID_MASK;
> > > +}
> > > +
> > > +static int armada_get_temp(struct thermal_zone_device *thermal,
> > > +			  unsigned long *temp)
> > > +{
> > > +	struct armada_thermal_priv *priv = thermal->devdata;
> > > +	unsigned long reg;
> > > +
> > > +	/* Valid check */
> > > +	if (priv->ops->is_valid && !priv->ops->is_valid(priv)) {
> > > +		dev_err(&thermal->device,
> > > +			"Temperature sensor reading not valid\n");
> > > +		return -EIO;
> > > +	}
> > > +
> > > +	reg = (readl_relaxed(priv->sensor) >> THERMAL_TEMP_OFFSET) &
> >
> > Can we have the readl_relaxed call as a separate statement ?
> >
> 
> Why would we want that? Do you think it'll be more readable?

Yes,

> 
> > > +		THERMAL_TEMP_MASK;
> > > +	*temp = (3153000000UL - (10000000UL*reg)) / 13825;
> >
> > If I substitute 1 for 'reg' I get  227341.7721...
> > Does this mean the temperature is 227 C ??
> >
> 
> Yes, I guess so.
> 
> > If you have the info, can you add a comment on what is the
> > valid range that 'reg' can take ?
> >
> 
> No, I don't have the info. I guess that the valid range 'reg'
> can take are the values that span a temperature between 25 ?C (or
> lower if it's winter) and when your CPU is on fire :-)
> 
> > Also, Is the resulting temperature
> > in MillidegreeCelsius ? If so, please add a comment saying so.
> >
> 
> Yes, the resulting temperature is in millidegree celsius,
> as required by the thermal framework:

ok.

Thanks,
Durga

Re: [PATCH for 3.10 0/4] thermal: Add Armada 370/XP support

[Ezequiel Garcia]

Hello,

On Mon, Mar 25, 2013 at 12:27:05PM -0300, Ezequiel Garcia wrote:
> After Andrew Lunn objected the single driver approach [1] that merged
> all the Marvell EBU SoC thermal drivers, we have agreed to proceed step
> by step. For the time being we are adding support for Armada 370/XP only.
> Given both Armada 370 and Armada XP have a similar feature set, it
> makes sense to provide a single driver for them.
> 
> In the future we can unify this with the other drivers,
> if this proves to be worthwhile.
> 
> This driver has been influenced by the existing Kirkwood driver,
> also taking account of suggestions made by Jason Gunthorpe.
> 
> Testing has been done on the following boards:
> 
> For Armada 370: Globalscale Mirabox and Marvell Armada 370 RD
> For Armada XP:  Plat'home Openblocks AX3 and Marvell Armada XP DB-MV784MP-GP
> 
> This series is based in v3.9-rc2. To make testing easier this patchset
> has been pushed on a github branch [2].
> 
> Any feedback or comments are welcome!
> 
> [1] http://permalink.gmane.org/gmane.linux.power-management.general/32123
> [2] https://github.com/MISL-EBU-System-SW/mainline-public/tree/armada-thermal

Now that Andrew Lunn has acked this series...

> 
> Ezequiel Garcia (4):
>   thermal: Add driver for Armada 370/XP SoC thermal management

@Zhang: If nobody else has any comments regarding this, would you take
the first patch through your tree?

>   ARM: mvebu: Add thermal support to Armada XP device tree
>   ARM: mvebu: Add thermal support to Armada 370 device tree
>   ARM: configs: Update mvebu defconfig for thermal

@Jason: if Zhang agrees to take the first patch, would you take the rest?

Thanks,

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH for 3.10 0/4] thermal: Add Armada 370/XP support

[Ezequiel Garcia]

Hello,

On Mon, Mar 25, 2013 at 12:27:05PM -0300, Ezequiel Garcia wrote:
> After Andrew Lunn objected the single driver approach [1] that merged
> all the Marvell EBU SoC thermal drivers, we have agreed to proceed step
> by step. For the time being we are adding support for Armada 370/XP only.
> Given both Armada 370 and Armada XP have a similar feature set, it
> makes sense to provide a single driver for them.
> 
> In the future we can unify this with the other drivers,
> if this proves to be worthwhile.
> 
> This driver has been influenced by the existing Kirkwood driver,
> also taking account of suggestions made by Jason Gunthorpe.
> 
> Testing has been done on the following boards:
> 
> For Armada 370: Globalscale Mirabox and Marvell Armada 370 RD
> For Armada XP:  Plat'home Openblocks AX3 and Marvell Armada XP DB-MV784MP-GP
> 
> This series is based in v3.9-rc2. To make testing easier this patchset
> has been pushed on a github branch [2].
> 
> Any feedback or comments are welcome!
> 
> [1] http://permalink.gmane.org/gmane.linux.power-management.general/32123
> [2] https://github.com/MISL-EBU-System-SW/mainline-public/tree/armada-thermal

Now that Andrew Lunn has acked this series...

> 
> Ezequiel Garcia (4):
>   thermal: Add driver for Armada 370/XP SoC thermal management

@Zhang: If nobody else has any comments regarding this, would you take
the first patch through your tree?

>   ARM: mvebu: Add thermal support to Armada XP device tree
>   ARM: mvebu: Add thermal support to Armada 370 device tree
>   ARM: configs: Update mvebu defconfig for thermal

@Jason: if Zhang agrees to take the first patch, would you take the rest?

Thanks,

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH for 3.10 0/4] thermal: Add Armada 370/XP support

[Jason Cooper]

On Wed, Mar 27, 2013 at 05:54:21AM -0300, Ezequiel Garcia wrote:
> On Mon, Mar 25, 2013 at 12:27:05PM -0300, Ezequiel Garcia wrote:
...
> Now that Andrew Lunn has acked this series...
> 
> > 
> > Ezequiel Garcia (4):
> >   thermal: Add driver for Armada 370/XP SoC thermal management
> 
> @Zhang: If nobody else has any comments regarding this, would you take
> the first patch through your tree?
> 
> >   ARM: mvebu: Add thermal support to Armada XP device tree
> >   ARM: mvebu: Add thermal support to Armada 370 device tree
> >   ARM: configs: Update mvebu defconfig for thermal
> 
> @Jason: if Zhang agrees to take the first patch, would you take the rest?

That's the #plan.  :)

thx,

Jason.

[PATCH for 3.10 0/4] thermal: Add Armada 370/XP support

[Jason Cooper]

On Wed, Mar 27, 2013 at 05:54:21AM -0300, Ezequiel Garcia wrote:
> On Mon, Mar 25, 2013 at 12:27:05PM -0300, Ezequiel Garcia wrote:
...
> Now that Andrew Lunn has acked this series...
> 
> > 
> > Ezequiel Garcia (4):
> >   thermal: Add driver for Armada 370/XP SoC thermal management
> 
> @Zhang: If nobody else has any comments regarding this, would you take
> the first patch through your tree?
> 
> >   ARM: mvebu: Add thermal support to Armada XP device tree
> >   ARM: mvebu: Add thermal support to Armada 370 device tree
> >   ARM: configs: Update mvebu defconfig for thermal
> 
> @Jason: if Zhang agrees to take the first patch, would you take the rest?

That's the #plan.  :)

thx,

Jason.