[PATCH v3 0/3] Stingray thermal driver support

[PATCH v3 0/3] Stingray thermal driver support

[Srinath Mannam]

These patches adds the stingray thermal driver and its
corresponding DT nodes with documentation.

Changes from v2:
  - All stingray TMON DT nodes are combine together into single.
    Temperature registers are combined into one mem resource.
    brcm,tmon-mask parameter has available TMONs mask value.
  - All available TMONs are initialized together in single
    instance of driver probe call.

Changes from v1:
  - Fixed auto build sparce warning.

Pramod Kumar (3):
  dt-bindings: thermal: Add binding document for SR thermal
  arm64: dts: stingray: Add Stingray Thermal DT support.
  thermal: broadcom: Add Stingray thermal driver

 .../bindings/thermal/brcm,sr-thermal.txt           |  23 +++
 .../arm64/boot/dts/broadcom/stingray/stingray.dtsi |  13 ++
 drivers/thermal/Kconfig                            |   3 +-
 drivers/thermal/broadcom/Kconfig                   |   9 +
 drivers/thermal/broadcom/Makefile                  |   1 +
 drivers/thermal/broadcom/sr-thermal.c              | 216 +++++++++++++++++++++
 6 files changed, 264 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/thermal/brcm,sr-thermal.txt
 create mode 100644 drivers/thermal/broadcom/sr-thermal.c

-- 
2.7.4

[PATCH v3 1/3] dt-bindings: thermal: Add binding document for SR thermal

[Srinath Mannam]

From: Pramod Kumar <pramod.kumar@broadcom.com>

Add binding document for supported thermal implementation
in Stingray.

Signed-off-by: Pramod Kumar <pramod.kumar@broadcom.com>
Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 .../bindings/thermal/brcm,sr-thermal.txt           | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/brcm,sr-thermal.txt

diff --git a/Documentation/devicetree/bindings/thermal/brcm,sr-thermal.txt b/Documentation/devicetree/bindings/thermal/brcm,sr-thermal.txt
new file mode 100644
index 0000000..fc9e6d3
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/brcm,sr-thermal.txt
@@ -0,0 +1,23 @@
+* Broadcom Stingray Thermal
+
+This binding describes thermal sensors that is part of Stingray SoCs.
+
+Required properties:
+- compatible : Must be "brcm,sr-thermal"
+- reg : memory where tmon data will be available.
+- brcm,tmon-mask: A one cell bit mask of valid TMON sources.
+                  Each bit represents single TMON source.
+
+Example:
+	tmons {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x0 0x8f100000 0x100>;
+
+		tmon: thermal@0 {
+			compatible = "brcm,sr-thermal";
+			reg = <0 0x40>;
+			brcm,tmon-mask = <0x3f>;
+		};
+	};
-- 
2.7.4

[PATCH v3 2/3] arm64: dts: stingray: Add Stingray Thermal DT support.

[Srinath Mannam]

From: Pramod Kumar <pramod.kumar@broadcom.com>

Add DT nodes for thermal zones memory base address
to read temperature.

Signed-off-by: Pramod Kumar <pramod.kumar@broadcom.com>
Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi b/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
index b203152..eeccf94 100644
--- a/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
+++ b/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
@@ -593,4 +593,17 @@
 			status = "disabled";
 		};
 	};
+
+	tmons {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x0 0x8f100000 0x100>;
+
+		tmon: thermal@0 {
+			compatible = "brcm,sr-thermal";
+			reg = <0x0 0x40>;
+			brcm,tmon-mask = <0x3f>;
+		};
+	};
 };
-- 
2.7.4

[PATCH v3 3/3] thermal: broadcom: Add Stingray thermal driver

[Srinath Mannam]

From: Pramod Kumar <pramod.kumar@broadcom.com>

Adds stingray thermal driver to monitor six
thermal zones temperature and trips at critical temperature.

Signed-off-by: Pramod Kumar <pramod.kumar@broadcom.com>
Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
Reviewed-by: Vikram Prakash <vikram.prakash@broadcom.com>
---
 drivers/thermal/Kconfig               |   3 +-
 drivers/thermal/broadcom/Kconfig      |   9 ++
 drivers/thermal/broadcom/Makefile     |   1 +
 drivers/thermal/broadcom/sr-thermal.c | 216 ++++++++++++++++++++++++++++++++++
 4 files changed, 228 insertions(+), 1 deletion(-)
 create mode 100644 drivers/thermal/broadcom/sr-thermal.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 8297988..26d39d4 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -416,7 +416,8 @@ config MTK_THERMAL
 	  controller present in Mediatek SoCs
 
 menu "Broadcom thermal drivers"
-depends on ARCH_BCM || ARCH_BRCMSTB || ARCH_BCM2835 || COMPILE_TEST
+depends on ARCH_BCM || ARCH_BRCMSTB || ARCH_BCM2835 || ARCH_BCM_IPROC || \
+		COMPILE_TEST
 source "drivers/thermal/broadcom/Kconfig"
 endmenu
 
diff --git a/drivers/thermal/broadcom/Kconfig b/drivers/thermal/broadcom/Kconfig
index c106a15..dc9a9bd 100644
--- a/drivers/thermal/broadcom/Kconfig
+++ b/drivers/thermal/broadcom/Kconfig
@@ -22,3 +22,12 @@ config BCM_NS_THERMAL
 	  BCM4708, BCM4709, BCM5301x, BCM95852X, etc). It contains DMU (Device
 	  Management Unit) block with a thermal sensor that allows checking CPU
 	  temperature.
+
+config BCM_SR_THERMAL
+	tristate "Stingray thermal driver"
+	depends on ARCH_BCM_IPROC || COMPILE_TEST
+	default ARCH_BCM_IPROC
+	help
+	  Support for the Stingray family of SoCs. Its different blocks like
+	  iHost, CRMU and NITRO has thermal sensor that allows checking its
+	  temperature.
diff --git a/drivers/thermal/broadcom/Makefile b/drivers/thermal/broadcom/Makefile
index fae10ec..79df69e 100644
--- a/drivers/thermal/broadcom/Makefile
+++ b/drivers/thermal/broadcom/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_BCM2835_THERMAL)		+= bcm2835_thermal.o
 obj-$(CONFIG_BRCMSTB_THERMAL)		+= brcmstb_thermal.o
 obj-$(CONFIG_BCM_NS_THERMAL)		+= ns-thermal.o
+obj-$(CONFIG_BCM_SR_THERMAL)		+= sr-thermal.o
diff --git a/drivers/thermal/broadcom/sr-thermal.c b/drivers/thermal/broadcom/sr-thermal.c
new file mode 100644
index 0000000..909f80c
--- /dev/null
+++ b/drivers/thermal/broadcom/sr-thermal.c
@@ -0,0 +1,216 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Broadcom
+ */
+
+#include <linux/acpi.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/thermal.h>
+
+#define TMON_CRIT_TEMP          105000 /* temp in millidegree C */
+#define SR_TMON_MAX_LIST        6
+
+/*
+ * In stingray thermal IO memory,
+ * Total Number of available TMONs MASK is at offset 0
+ * temperature registers BASE is at 4 byte offset.
+ * Each TMON temperature register size is 4.
+ */
+#define SR_TMON_TEMP_BASE(id)   ((id) * 0x4)
+
+static const char * const sr_tmon_names[SR_TMON_MAX_LIST] = {
+	"sr_tmon_ihost0",
+	"sr_tmon_ihost1",
+	"sr_tmon_ihost2",
+	"sr_tmon_ihost3",
+	"sr_tmon_crmu",
+	"sr_tmon_nitro",
+};
+
+struct sr_tmon {
+	struct thermal_zone_device *tz;
+	unsigned int crit_temp;
+	unsigned int tmon_id;
+	struct sr_thermal *priv;
+};
+
+struct sr_thermal {
+	struct device *dev;
+	void __iomem *regs;
+	struct sr_tmon tmon[SR_TMON_MAX_LIST];
+};
+
+static int sr_get_temp(struct thermal_zone_device *tz, int *temp)
+{
+	struct sr_tmon *tmon = tz->devdata;
+	struct sr_thermal *sr_thermal = tmon->priv;
+
+	*temp = readl(sr_thermal->regs + SR_TMON_TEMP_BASE(tmon->tmon_id));
+
+	return 0;
+}
+
+static int sr_get_trip_type(struct thermal_zone_device *tz, int trip,
+					enum thermal_trip_type *type)
+{
+	struct sr_tmon *tmon = tz->devdata;
+	struct sr_thermal *sr_thermal = tmon->priv;
+
+	switch (trip) {
+	case 0:
+		*type = THERMAL_TRIP_CRITICAL;
+		break;
+	default:
+		dev_dbg(sr_thermal->dev,
+			"Driver does not support more than 1 trip point\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int sr_get_trip_temp(struct thermal_zone_device *tz, int trip, int *temp)
+{
+	struct sr_tmon *tmon = tz->devdata;
+	struct sr_thermal *sr_thermal = tmon->priv;
+
+	switch (trip) {
+	case 0:
+		*temp = tmon->crit_temp;
+		break;
+	default:
+		dev_dbg(sr_thermal->dev,
+			"Driver does not support more than 1 trip point\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int sr_set_trip_temp(struct thermal_zone_device *tz, int trip, int temp)
+{
+	struct sr_tmon *tmon = tz->devdata;
+
+	switch (trip) {
+	case 0:
+		/*
+		 * Allow the user to change critical temperature
+		 * as per their requirement, could be for debug
+		 * purpose, even if it's more than the recommended
+		 * critical temperature.
+		 */
+		tmon->crit_temp = temp;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static struct thermal_zone_device_ops sr_thermal_ops = {
+	.get_temp = sr_get_temp,
+	.get_trip_type = sr_get_trip_type,
+	.get_trip_temp = sr_get_trip_temp,
+	.set_trip_temp = sr_set_trip_temp,
+};
+
+static int sr_thermal_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sr_thermal *sr_thermal;
+	struct sr_tmon *tmon;
+	struct resource *res;
+	uint32_t sr_tmon_list = 0;
+	unsigned int i;
+	int ret;
+
+	sr_thermal = devm_kzalloc(dev, sizeof(*sr_thermal), GFP_KERNEL);
+	if (!sr_thermal)
+		return -ENOMEM;
+
+	sr_thermal->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sr_thermal->regs = (void __iomem *)devm_memremap(&pdev->dev, res->start,
+					 resource_size(res), MEMREMAP_WB);
+	if (IS_ERR(sr_thermal->regs)) {
+		dev_err(dev, "failed to get io address\n");
+		return PTR_ERR(sr_thermal->regs);
+	}
+
+	ret = device_property_read_u32(dev, "brcm,tmon-mask", &sr_tmon_list);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < SR_TMON_MAX_LIST; i++) {
+
+		if (!(sr_tmon_list & BIT(i)))
+			continue;
+
+		/* Flush temperature registers */
+		writel(0, sr_thermal->regs + SR_TMON_TEMP_BASE(i));
+		tmon = &sr_thermal->tmon[i];
+		tmon->crit_temp = TMON_CRIT_TEMP;
+		tmon->tmon_id = i;
+		tmon->priv = sr_thermal;
+		tmon->tz = thermal_zone_device_register(sr_tmon_names[i],
+				1, 1,
+				tmon,
+				&sr_thermal_ops,
+				NULL, 1000, 1000);
+		if (IS_ERR(tmon->tz))
+			goto err_exit;
+
+		dev_info(dev, "%s: registered\n", sr_tmon_names[i]);
+	}
+	platform_set_drvdata(pdev, sr_thermal);
+
+	return 0;
+
+err_exit:
+	while (--i >= 0) {
+		if (sr_thermal->tmon[i].tz)
+			thermal_zone_device_unregister(sr_thermal->tmon[i].tz);
+	}
+
+	return PTR_ERR(tmon->tz);
+}
+
+static int sr_thermal_remove(struct platform_device *pdev)
+{
+	struct sr_thermal *sr_thermal = platform_get_drvdata(pdev);
+	unsigned int i;
+
+	for (i = 0; i < SR_TMON_MAX_LIST; i++)
+		if (sr_thermal->tmon[i].tz)
+			thermal_zone_device_unregister(sr_thermal->tmon[i].tz);
+
+	return 0;
+}
+
+static const struct of_device_id sr_thermal_of_match[] = {
+	{ .compatible = "brcm,sr-thermal", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sr_thermal_of_match);
+
+static const struct acpi_device_id sr_thermal_acpi_ids[] = {
+	{ .id = "BRCM0500" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(acpi, sr_thermal_acpi_ids);
+
+static struct platform_driver sr_thermal_driver = {
+	.probe		= sr_thermal_probe,
+	.remove		= sr_thermal_remove,
+	.driver = {
+		.name = "sr-thermal",
+		.of_match_table = sr_thermal_of_match,
+		.acpi_match_table = ACPI_PTR(sr_thermal_acpi_ids),
+	},
+};
+module_platform_driver(sr_thermal_driver);
+
+MODULE_AUTHOR("Pramod Kumar <pramod.kumar@broadcom.com>");
+MODULE_DESCRIPTION("Stingray thermal driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v3 1/3] dt-bindings: thermal: Add binding document for SR thermal

[Rob Herring]

On Thu, Aug 09, 2018 at 06:24:57PM +0530, Srinath Mannam wrote:
> From: Pramod Kumar <pramod.kumar@broadcom.com>
> 
> Add binding document for supported thermal implementation
> in Stingray.
> 
> Signed-off-by: Pramod Kumar <pramod.kumar@broadcom.com>
> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> ---
>  .../bindings/thermal/brcm,sr-thermal.txt           | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/thermal/brcm,sr-thermal.txt

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 0/3] Stingray thermal driver support

[Florian Fainelli]

On 08/09/18 05:54, Srinath Mannam wrote:
> These patches adds the stingray thermal driver and its
> corresponding DT nodes with documentation.
> 
> Changes from v2:
>   - All stingray TMON DT nodes are combine together into single.
>     Temperature registers are combined into one mem resource.
>     brcm,tmon-mask parameter has available TMONs mask value.
>   - All available TMONs are initialized together in single
>     instance of driver probe call.
> 
> Changes from v1:
>   - Fixed auto build sparce warning.

Thermal maintainers, can you please review patch #3? The binding and DTS
changes can go through arm-soc if you would like, or you can take
patches 1 and 3 and I take patch 2, either way is fine.

Thank you

> 
> Pramod Kumar (3):
>   dt-bindings: thermal: Add binding document for SR thermal
>   arm64: dts: stingray: Add Stingray Thermal DT support.
>   thermal: broadcom: Add Stingray thermal driver
> 
>  .../bindings/thermal/brcm,sr-thermal.txt           |  23 +++
>  .../arm64/boot/dts/broadcom/stingray/stingray.dtsi |  13 ++
>  drivers/thermal/Kconfig                            |   3 +-
>  drivers/thermal/broadcom/Kconfig                   |   9 +
>  drivers/thermal/broadcom/Makefile                  |   1 +
>  drivers/thermal/broadcom/sr-thermal.c              | 216 +++++++++++++++++++++
>  6 files changed, 264 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/thermal/brcm,sr-thermal.txt
>  create mode 100644 drivers/thermal/broadcom/sr-thermal.c
> 

-- 
Florian

[PATCH v3 3/3] thermal: broadcom: Add Stingray thermal driver

[Daniel Lezcano]

On 09/08/2018 14:54, Srinath Mannam wrote:
> From: Pramod Kumar <pramod.kumar@broadcom.com>
> 
> Adds stingray thermal driver to monitor six
> thermal zones temperature and trips at critical temperature.

Hi Pramod,

could you elaborate a bit more the description? As you are introducing a
new driver it would be nice to give the sensor details.

> Signed-off-by: Pramod Kumar <pramod.kumar@broadcom.com>
> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> Reviewed-by: Vikram Prakash <vikram.prakash@broadcom.com>
> ---
>  drivers/thermal/Kconfig               |   3 +-
>  drivers/thermal/broadcom/Kconfig      |   9 ++
>  drivers/thermal/broadcom/Makefile     |   1 +
>  drivers/thermal/broadcom/sr-thermal.c | 216 ++++++++++++++++++++++++++++++++++
>  4 files changed, 228 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/thermal/broadcom/sr-thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 8297988..26d39d4 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -416,7 +416,8 @@ config MTK_THERMAL
>  	  controller present in Mediatek SoCs
>  
>  menu "Broadcom thermal drivers"
> -depends on ARCH_BCM || ARCH_BRCMSTB || ARCH_BCM2835 || COMPILE_TEST
> +depends on ARCH_BCM || ARCH_BRCMSTB || ARCH_BCM2835 || ARCH_BCM_IPROC || \
> +		COMPILE_TEST
>  source "drivers/thermal/broadcom/Kconfig"
>  endmenu
>  
> diff --git a/drivers/thermal/broadcom/Kconfig b/drivers/thermal/broadcom/Kconfig
> index c106a15..dc9a9bd 100644
> --- a/drivers/thermal/broadcom/Kconfig
> +++ b/drivers/thermal/broadcom/Kconfig
> @@ -22,3 +22,12 @@ config BCM_NS_THERMAL
>  	  BCM4708, BCM4709, BCM5301x, BCM95852X, etc). It contains DMU (Device
>  	  Management Unit) block with a thermal sensor that allows checking CPU
>  	  temperature.
> +
> +config BCM_SR_THERMAL
> +	tristate "Stingray thermal driver"
> +	depends on ARCH_BCM_IPROC || COMPILE_TEST
> +	default ARCH_BCM_IPROC
> +	help
> +	  Support for the Stingray family of SoCs. Its different blocks like
> +	  iHost, CRMU and NITRO has thermal sensor that allows checking its
> +	  temperature.
> diff --git a/drivers/thermal/broadcom/Makefile b/drivers/thermal/broadcom/Makefile
> index fae10ec..79df69e 100644
> --- a/drivers/thermal/broadcom/Makefile
> +++ b/drivers/thermal/broadcom/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_BCM2835_THERMAL)		+= bcm2835_thermal.o
>  obj-$(CONFIG_BRCMSTB_THERMAL)		+= brcmstb_thermal.o
>  obj-$(CONFIG_BCM_NS_THERMAL)		+= ns-thermal.o
> +obj-$(CONFIG_BCM_SR_THERMAL)		+= sr-thermal.o
> diff --git a/drivers/thermal/broadcom/sr-thermal.c b/drivers/thermal/broadcom/sr-thermal.c
> new file mode 100644
> index 0000000..909f80c
> --- /dev/null
> +++ b/drivers/thermal/broadcom/sr-thermal.c
> @@ -0,0 +1,216 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 Broadcom
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/thermal.h>
> +
> +#define TMON_CRIT_TEMP          105000 /* temp in millidegree C */

I suggest to move this in the DT?

> +#define SR_TMON_MAX_LIST        6
> +
> +/*
> + * In stingray thermal IO memory,
> + * Total Number of available TMONs MASK is at offset 0
> + * temperature registers BASE is at 4 byte offset.
> + * Each TMON temperature register size is 4.
> + */
> +#define SR_TMON_TEMP_BASE(id)   ((id) * 0x4)
> +
> +static const char * const sr_tmon_names[SR_TMON_MAX_LIST] = {

It will be more elegant to replace the macro SR_TMON_MAX_LIST by
ARRAY_SIZE(sr_tmon_names) and declare this array as:

static const char *const sr_tmon_name[] = {
	...
};

> +	"sr_tmon_ihost0",
> +	"sr_tmon_ihost1",
> +	"sr_tmon_ihost2",
> +	"sr_tmon_ihost3",
> +	"sr_tmon_crmu",
> +	"sr_tmon_nitro",
> +};
> +
> +struct sr_tmon {
> +	struct thermal_zone_device *tz;
> +	unsigned int crit_temp;
> +	unsigned int tmon_id;
> +	struct sr_thermal *priv;
> +};
> +
> +struct sr_thermal {
> +	struct device *dev;

This field is used for dev_dbg, may be it could be removed along with
the dev_dbg message?

> +	void __iomem *regs;
> +	struct sr_tmon tmon[SR_TMON_MAX_LIST];
> +};
> +
> +static int sr_get_temp(struct thermal_zone_device *tz, int *temp)
> +{
> +	struct sr_tmon *tmon = tz->devdata;
> +	struct sr_thermal *sr_thermal = tmon->priv;
> +
> +	*temp = readl(sr_thermal->regs + SR_TMON_TEMP_BASE(tmon->tmon_id));
> +
> +	return 0;
> +}
> +
> +static int sr_get_trip_type(struct thermal_zone_device *tz, int trip,
> +					enum thermal_trip_type *type)
> +{
> +	struct sr_tmon *tmon = tz->devdata;
> +	struct sr_thermal *sr_thermal = tmon->priv;
> +
> +	switch (trip) {
> +	case 0:
> +		*type = THERMAL_TRIP_CRITICAL;
> +		break;
> +	default:
> +		dev_dbg(sr_thermal->dev,
> +			"Driver does not support more than 1 trip point\n");
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int sr_get_trip_temp(struct thermal_zone_device *tz, int trip, int *temp)
> +{
> +	struct sr_tmon *tmon = tz->devdata;
> +	struct sr_thermal *sr_thermal = tmon->priv;
> +
> +	switch (trip) {
> +	case 0:
> +		*temp = tmon->crit_temp;
> +		break;
> +	default:
> +		dev_dbg(sr_thermal->dev,
> +			"Driver does not support more than 1 trip point\n");
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int sr_set_trip_temp(struct thermal_zone_device *tz, int trip, int temp)
> +{
> +	struct sr_tmon *tmon = tz->devdata;
> +
> +	switch (trip) {
> +	case 0:
> +		/*
> +		 * Allow the user to change critical temperature
> +		 * as per their requirement, could be for debug
> +		 * purpose, even if it's more than the recommended
> +		 * critical temperature.
> +		 */

Couldn't the user harm the hardware with a too high value?

Why not define this value in the DT?

> +		tmon->crit_temp = temp;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}

Is it possible to factor out these 3 functions above and remove the
'switch' in all of them ?

> +static struct thermal_zone_device_ops sr_thermal_ops = {
> +	.get_temp = sr_get_temp,
> +	.get_trip_type = sr_get_trip_type,
> +	.get_trip_temp = sr_get_trip_temp,
> +	.set_trip_temp = sr_set_trip_temp,
> +};
> +
> +static int sr_thermal_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sr_thermal *sr_thermal;
> +	struct sr_tmon *tmon;
> +	struct resource *res;
> +	uint32_t sr_tmon_list = 0;
> +	unsigned int i;
> +	int ret;
> +
> +	sr_thermal = devm_kzalloc(dev, sizeof(*sr_thermal), GFP_KERNEL);
> +	if (!sr_thermal)
> +		return -ENOMEM;
> +
> +	sr_thermal->dev = dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sr_thermal->regs = (void __iomem *)devm_memremap(&pdev->dev, res->start,
> +					 resource_size(res), MEMREMAP_WB);
> +	if (IS_ERR(sr_thermal->regs)) {
> +		dev_err(dev, "failed to get io address\n");
> +		return PTR_ERR(sr_thermal->regs);
> +	}
> +
> +	ret = device_property_read_u32(dev, "brcm,tmon-mask", &sr_tmon_list);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < SR_TMON_MAX_LIST; i++) {
> +
> +		if (!(sr_tmon_list & BIT(i)))
> +			continue;
> +
> +		/* Flush temperature registers */
> +		writel(0, sr_thermal->regs + SR_TMON_TEMP_BASE(i));
> +		tmon = &sr_thermal->tmon[i];

It is possible to initialize tmon to:

		tmon = &sr_thermal->tmon;

and then use the pointer increment:
		tmon++;


> +		tmon->crit_temp = TMON_CRIT_TEMP;
> +		tmon->tmon_id = i;
> +		tmon->priv = sr_thermal;
> +		tmon->tz = thermal_zone_device_register(sr_tmon_names[i],
> +				1, 1,
> +				tmon,
> +				&sr_thermal_ops,
> +				NULL, 1000, 1000);
> +		if (IS_ERR(tmon->tz))
> +			goto err_exit;
> +
> +		dev_info(dev, "%s: registered\n", sr_tmon_names[i]);
> +	}
> +	platform_set_drvdata(pdev, sr_thermal);
> +
> +	return 0;
> +
> +err_exit:
> +	while (--i >= 0) {
> +		if (sr_thermal->tmon[i].tz)
> +			thermal_zone_device_unregister(sr_thermal->tmon[i].tz);
> +	}
> +
> +	return PTR_ERR(tmon->tz);
> +}
> +
> +static int sr_thermal_remove(struct platform_device *pdev)
> +{
> +	struct sr_thermal *sr_thermal = platform_get_drvdata(pdev);
> +	unsigned int i;
> +
> +	for (i = 0; i < SR_TMON_MAX_LIST; i++)
> +		if (sr_thermal->tmon[i].tz)
> +			thermal_zone_device_unregister(sr_thermal->tmon[i].tz);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sr_thermal_of_match[] = {
> +	{ .compatible = "brcm,sr-thermal", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sr_thermal_of_match);
> +
> +static const struct acpi_device_id sr_thermal_acpi_ids[] = {
> +	{ .id = "BRCM0500" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(acpi, sr_thermal_acpi_ids);
> +
> +static struct platform_driver sr_thermal_driver = {
> +	.probe		= sr_thermal_probe,
> +	.remove		= sr_thermal_remove,
> +	.driver = {
> +		.name = "sr-thermal",
> +		.of_match_table = sr_thermal_of_match,
> +		.acpi_match_table = ACPI_PTR(sr_thermal_acpi_ids),
> +	},
> +};
> +module_platform_driver(sr_thermal_driver);
> +
> +MODULE_AUTHOR("Pramod Kumar <pramod.kumar@broadcom.com>");
> +MODULE_DESCRIPTION("Stingray thermal driver");
> +MODULE_LICENSE("GPL v2");
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v3 3/3] thermal: broadcom: Add Stingray thermal driver

[Srinath Mannam]

Hi Daniel,

Thank you for your review comments.
I will address all your comments in next patch set.

Regards,
Srinath.

On Fri, Sep 14, 2018 at 7:50 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 09/08/2018 14:54, Srinath Mannam wrote:
>> From: Pramod Kumar <pramod.kumar@broadcom.com>
>>
>> Adds stingray thermal driver to monitor six
>> thermal zones temperature and trips at critical temperature.
>
> Hi Pramod,
>
> could you elaborate a bit more the description? As you are introducing a
> new driver it would be nice to give the sensor details.
>
>> Signed-off-by: Pramod Kumar <pramod.kumar@broadcom.com>
>> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>> Reviewed-by: Vikram Prakash <vikram.prakash@broadcom.com>
>> ---
>>  drivers/thermal/Kconfig               |   3 +-
>>  drivers/thermal/broadcom/Kconfig      |   9 ++
>>  drivers/thermal/broadcom/Makefile     |   1 +
>>  drivers/thermal/broadcom/sr-thermal.c | 216 ++++++++++++++++++++++++++++++++++
>>  4 files changed, 228 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/thermal/broadcom/sr-thermal.c
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 8297988..26d39d4 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -416,7 +416,8 @@ config MTK_THERMAL
>>         controller present in Mediatek SoCs
>>
>>  menu "Broadcom thermal drivers"
>> -depends on ARCH_BCM || ARCH_BRCMSTB || ARCH_BCM2835 || COMPILE_TEST
>> +depends on ARCH_BCM || ARCH_BRCMSTB || ARCH_BCM2835 || ARCH_BCM_IPROC || \
>> +             COMPILE_TEST
>>  source "drivers/thermal/broadcom/Kconfig"
>>  endmenu
>>
>> diff --git a/drivers/thermal/broadcom/Kconfig b/drivers/thermal/broadcom/Kconfig
>> index c106a15..dc9a9bd 100644
>> --- a/drivers/thermal/broadcom/Kconfig
>> +++ b/drivers/thermal/broadcom/Kconfig
>> @@ -22,3 +22,12 @@ config BCM_NS_THERMAL
>>         BCM4708, BCM4709, BCM5301x, BCM95852X, etc). It contains DMU (Device
>>         Management Unit) block with a thermal sensor that allows checking CPU
>>         temperature.
>> +
>> +config BCM_SR_THERMAL
>> +     tristate "Stingray thermal driver"
>> +     depends on ARCH_BCM_IPROC || COMPILE_TEST
>> +     default ARCH_BCM_IPROC
>> +     help
>> +       Support for the Stingray family of SoCs. Its different blocks like
>> +       iHost, CRMU and NITRO has thermal sensor that allows checking its
>> +       temperature.
>> diff --git a/drivers/thermal/broadcom/Makefile b/drivers/thermal/broadcom/Makefile
>> index fae10ec..79df69e 100644
>> --- a/drivers/thermal/broadcom/Makefile
>> +++ b/drivers/thermal/broadcom/Makefile
>> @@ -1,3 +1,4 @@
>>  obj-$(CONFIG_BCM2835_THERMAL)                += bcm2835_thermal.o
>>  obj-$(CONFIG_BRCMSTB_THERMAL)                += brcmstb_thermal.o
>>  obj-$(CONFIG_BCM_NS_THERMAL)         += ns-thermal.o
>> +obj-$(CONFIG_BCM_SR_THERMAL)         += sr-thermal.o
>> diff --git a/drivers/thermal/broadcom/sr-thermal.c b/drivers/thermal/broadcom/sr-thermal.c
>> new file mode 100644
>> index 0000000..909f80c
>> --- /dev/null
>> +++ b/drivers/thermal/broadcom/sr-thermal.c
>> @@ -0,0 +1,216 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2018 Broadcom
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/thermal.h>
>> +
>> +#define TMON_CRIT_TEMP          105000 /* temp in millidegree C */
>
> I suggest to move this in the DT?
>
>> +#define SR_TMON_MAX_LIST        6
>> +
>> +/*
>> + * In stingray thermal IO memory,
>> + * Total Number of available TMONs MASK is at offset 0
>> + * temperature registers BASE is at 4 byte offset.
>> + * Each TMON temperature register size is 4.
>> + */
>> +#define SR_TMON_TEMP_BASE(id)   ((id) * 0x4)
>> +
>> +static const char * const sr_tmon_names[SR_TMON_MAX_LIST] = {
>
> It will be more elegant to replace the macro SR_TMON_MAX_LIST by
> ARRAY_SIZE(sr_tmon_names) and declare this array as:
>
> static const char *const sr_tmon_name[] = {
>         ...
> };
>
>> +     "sr_tmon_ihost0",
>> +     "sr_tmon_ihost1",
>> +     "sr_tmon_ihost2",
>> +     "sr_tmon_ihost3",
>> +     "sr_tmon_crmu",
>> +     "sr_tmon_nitro",
>> +};
>> +
>> +struct sr_tmon {
>> +     struct thermal_zone_device *tz;
>> +     unsigned int crit_temp;
>> +     unsigned int tmon_id;
>> +     struct sr_thermal *priv;
>> +};
>> +
>> +struct sr_thermal {
>> +     struct device *dev;
>
> This field is used for dev_dbg, may be it could be removed along with
> the dev_dbg message?
>
>> +     void __iomem *regs;
>> +     struct sr_tmon tmon[SR_TMON_MAX_LIST];
>> +};
>> +
>> +static int sr_get_temp(struct thermal_zone_device *tz, int *temp)
>> +{
>> +     struct sr_tmon *tmon = tz->devdata;
>> +     struct sr_thermal *sr_thermal = tmon->priv;
>> +
>> +     *temp = readl(sr_thermal->regs + SR_TMON_TEMP_BASE(tmon->tmon_id));
>> +
>> +     return 0;
>> +}
>> +
>> +static int sr_get_trip_type(struct thermal_zone_device *tz, int trip,
>> +                                     enum thermal_trip_type *type)
>> +{
>> +     struct sr_tmon *tmon = tz->devdata;
>> +     struct sr_thermal *sr_thermal = tmon->priv;
>> +
>> +     switch (trip) {
>> +     case 0:
>> +             *type = THERMAL_TRIP_CRITICAL;
>> +             break;
>> +     default:
>> +             dev_dbg(sr_thermal->dev,
>> +                     "Driver does not support more than 1 trip point\n");
>> +             return -EINVAL;
>> +     }
>> +     return 0;
>> +}
>> +
>> +static int sr_get_trip_temp(struct thermal_zone_device *tz, int trip, int *temp)
>> +{
>> +     struct sr_tmon *tmon = tz->devdata;
>> +     struct sr_thermal *sr_thermal = tmon->priv;
>> +
>> +     switch (trip) {
>> +     case 0:
>> +             *temp = tmon->crit_temp;
>> +             break;
>> +     default:
>> +             dev_dbg(sr_thermal->dev,
>> +                     "Driver does not support more than 1 trip point\n");
>> +             return -EINVAL;
>> +     }
>> +     return 0;
>> +}
>> +
>> +static int sr_set_trip_temp(struct thermal_zone_device *tz, int trip, int temp)
>> +{
>> +     struct sr_tmon *tmon = tz->devdata;
>> +
>> +     switch (trip) {
>> +     case 0:
>> +             /*
>> +              * Allow the user to change critical temperature
>> +              * as per their requirement, could be for debug
>> +              * purpose, even if it's more than the recommended
>> +              * critical temperature.
>> +              */
>
> Couldn't the user harm the hardware with a too high value?
>
> Why not define this value in the DT?
>
>> +             tmon->crit_temp = temp;
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +     return 0;
>> +}
>
> Is it possible to factor out these 3 functions above and remove the
> 'switch' in all of them ?
>
>> +static struct thermal_zone_device_ops sr_thermal_ops = {
>> +     .get_temp = sr_get_temp,
>> +     .get_trip_type = sr_get_trip_type,
>> +     .get_trip_temp = sr_get_trip_temp,
>> +     .set_trip_temp = sr_set_trip_temp,
>> +};
>> +
>> +static int sr_thermal_probe(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct sr_thermal *sr_thermal;
>> +     struct sr_tmon *tmon;
>> +     struct resource *res;
>> +     uint32_t sr_tmon_list = 0;
>> +     unsigned int i;
>> +     int ret;
>> +
>> +     sr_thermal = devm_kzalloc(dev, sizeof(*sr_thermal), GFP_KERNEL);
>> +     if (!sr_thermal)
>> +             return -ENOMEM;
>> +
>> +     sr_thermal->dev = dev;
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     sr_thermal->regs = (void __iomem *)devm_memremap(&pdev->dev, res->start,
>> +                                      resource_size(res), MEMREMAP_WB);
>> +     if (IS_ERR(sr_thermal->regs)) {
>> +             dev_err(dev, "failed to get io address\n");
>> +             return PTR_ERR(sr_thermal->regs);
>> +     }
>> +
>> +     ret = device_property_read_u32(dev, "brcm,tmon-mask", &sr_tmon_list);
>> +     if (ret)
>> +             return ret;
>> +
>> +     for (i = 0; i < SR_TMON_MAX_LIST; i++) {
>> +
>> +             if (!(sr_tmon_list & BIT(i)))
>> +                     continue;
>> +
>> +             /* Flush temperature registers */
>> +             writel(0, sr_thermal->regs + SR_TMON_TEMP_BASE(i));
>> +             tmon = &sr_thermal->tmon[i];
>
> It is possible to initialize tmon to:
>
>                 tmon = &sr_thermal->tmon;
>
> and then use the pointer increment:
>                 tmon++;
>
>
>> +             tmon->crit_temp = TMON_CRIT_TEMP;
>> +             tmon->tmon_id = i;
>> +             tmon->priv = sr_thermal;
>> +             tmon->tz = thermal_zone_device_register(sr_tmon_names[i],
>> +                             1, 1,
>> +                             tmon,
>> +                             &sr_thermal_ops,
>> +                             NULL, 1000, 1000);
>> +             if (IS_ERR(tmon->tz))
>> +                     goto err_exit;
>> +
>> +             dev_info(dev, "%s: registered\n", sr_tmon_names[i]);
>> +     }
>> +     platform_set_drvdata(pdev, sr_thermal);
>> +
>> +     return 0;
>> +
>> +err_exit:
>> +     while (--i >= 0) {
>> +             if (sr_thermal->tmon[i].tz)
>> +                     thermal_zone_device_unregister(sr_thermal->tmon[i].tz);
>> +     }
>> +
>> +     return PTR_ERR(tmon->tz);
>> +}
>> +
>> +static int sr_thermal_remove(struct platform_device *pdev)
>> +{
>> +     struct sr_thermal *sr_thermal = platform_get_drvdata(pdev);
>> +     unsigned int i;
>> +
>> +     for (i = 0; i < SR_TMON_MAX_LIST; i++)
>> +             if (sr_thermal->tmon[i].tz)
>> +                     thermal_zone_device_unregister(sr_thermal->tmon[i].tz);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id sr_thermal_of_match[] = {
>> +     { .compatible = "brcm,sr-thermal", },
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(of, sr_thermal_of_match);
>> +
>> +static const struct acpi_device_id sr_thermal_acpi_ids[] = {
>> +     { .id = "BRCM0500" },
>> +     { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, sr_thermal_acpi_ids);
>> +
>> +static struct platform_driver sr_thermal_driver = {
>> +     .probe          = sr_thermal_probe,
>> +     .remove         = sr_thermal_remove,
>> +     .driver = {
>> +             .name = "sr-thermal",
>> +             .of_match_table = sr_thermal_of_match,
>> +             .acpi_match_table = ACPI_PTR(sr_thermal_acpi_ids),
>> +     },
>> +};
>> +module_platform_driver(sr_thermal_driver);
>> +
>> +MODULE_AUTHOR("Pramod Kumar <pramod.kumar@broadcom.com>");
>> +MODULE_DESCRIPTION("Stingray thermal driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
>
> --
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>