linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/6] Add support of New Amlogic temperature sensor for G12 SoCs
@ 2019-07-31 15:35 Guillaume La Roque
  2019-07-31 15:35 ` [PATCH v2 1/6] dt-bindings: thermal: Add DT bindings documentation for Amlogic Thermal Guillaume La Roque
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Guillaume La Roque @ 2019-07-31 15:35 UTC (permalink / raw)
  To: daniel.lezcano, khilman
  Cc: linux-pm, devicetree, linux-amlogic, linux-kernel, linux-arm-kernel

This patchs series add support of New Amlogic temperature sensor and minimal
thermal zone for SEI510 and ODROID-N2 boards.

First implementation was doing on IIO[1] but after comments i move on thermal framework.
Formulas and calibration values come from amlogic.

Changes since v1:
  - fix enum vs const in documentation for compatible
  - fix error with thermal-sensor-cells value set to 1 instead of 0
  - add some dependencies needed to add cooling-maps

Dependencies :
- patch 3,4 & 5: depends on Neil's patch and series :
              - missing dwc2 phy-names[1]
              - patchsets to add DVFS on G12a[3] which have deps on [4] and [5]

[1] https://lore.kernel.org/linux-amlogic/20190604144714.2009-1-glaroque@baylibre.com/
[2] https://lore.kernel.org/linux-amlogic/20190625123647.26117-1-narmstrong@baylibre.com/
[3] https://lore.kernel.org/linux-amlogic/20190729132622.7566-1-narmstrong@baylibre.com/
[4] https://lore.kernel.org/linux-amlogic/20190731084019.8451-5-narmstrong@baylibre.com/
[5] https://lore.kernel.org/linux-amlogic/20190729132622.7566-3-narmstrong@baylibre.com/

Guillaume La Roque (6):
  dt-bindings: thermal: Add DT bindings documentation for Amlogic
    Thermal
  thermal: amlogic: Add thermal driver to support G12 SoCs
  arm64: dts: amlogic: g12: add temperature sensor
  arm64: dts: meson: sei510: Add minimal thermal zone
  arm64: dts: amlogic: odroid-n2: add minimal thermal zone
  MAINTAINERS: add entry for Amlogic Thermal driver

 .../bindings/thermal/amlogic,thermal.yaml     |  58 +++
 MAINTAINERS                                   |   9 +
 .../boot/dts/amlogic/meson-g12-common.dtsi    |  22 ++
 .../boot/dts/amlogic/meson-g12a-sei510.dts    |  56 +++
 .../boot/dts/amlogic/meson-g12b-odroid-n2.dts |  60 ++++
 drivers/thermal/Kconfig                       |  11 +
 drivers/thermal/Makefile                      |   1 +
 drivers/thermal/amlogic_thermal.c             | 332 ++++++++++++++++++
 8 files changed, 549 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
 create mode 100644 drivers/thermal/amlogic_thermal.c

-- 
2.17.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 1/6] dt-bindings: thermal: Add DT bindings documentation for Amlogic Thermal
  2019-07-31 15:35 [PATCH v2 0/6] Add support of New Amlogic temperature sensor for G12 SoCs Guillaume La Roque
@ 2019-07-31 15:35 ` Guillaume La Roque
  2019-07-31 17:59   ` Rob Herring
  2019-07-31 15:35 ` [PATCH v2 2/6] thermal: amlogic: Add thermal driver to support G12 SoCs Guillaume La Roque
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Guillaume La Roque @ 2019-07-31 15:35 UTC (permalink / raw)
  To: daniel.lezcano, khilman
  Cc: linux-pm, devicetree, linux-amlogic, linux-kernel, linux-arm-kernel

Adding the devicetree binding documentation for the Amlogic temperature
sensor found in the Amlogic Meson G12 SoCs.
the G12A  and G12B SoCs are supported.

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
---
 .../bindings/thermal/amlogic,thermal.yaml     | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml

diff --git a/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml b/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
new file mode 100644
index 000000000000..f10537ab4c8b
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/thermal/amlogic,thermal.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic Thermal Driver
+
+maintainers:
+  - Guillaume La Roque <glaroque@baylibre.com>
+
+description: Amlogic Thermal driver
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - amlogic,g12-cpu-thermal
+              - amlogic,g12-ddr-thermal
+          - const:
+              - amlogic,g12-thermal
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  amlogic,ao-secure:
+    description: phandle to the ao-secure syscon
+    allOf:
+     - $ref: /schemas/types.yaml#/definitions/uint32
+
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - amlogic,ao-secure
+
+examples:
+  - |
+        cpu_temp: temperature-sensor@ff634800 {
+                compatible = "amlogic,g12-cpu-thermal",
+                             "amlogic,g12-thermal";
+                reg = <0x0 0xff634800 0x0 0x50>;
+                interrupts = <0x0 0x24 0x0>;
+                clocks = <&clk 164>;
+                status = "okay";
+                #thermal-sensor-cells = <0>;
+                amlogic,ao-secure = <&sec_AO>;
+        };
+...
\ No newline at end of file
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 2/6] thermal: amlogic: Add thermal driver to support G12 SoCs
  2019-07-31 15:35 [PATCH v2 0/6] Add support of New Amlogic temperature sensor for G12 SoCs Guillaume La Roque
  2019-07-31 15:35 ` [PATCH v2 1/6] dt-bindings: thermal: Add DT bindings documentation for Amlogic Thermal Guillaume La Roque
@ 2019-07-31 15:35 ` Guillaume La Roque
  2019-08-03 18:24   ` Martin Blumenstingl
  2019-07-31 15:35 ` [PATCH v2 3/6] arm64: dts: amlogic: g12: add temperature sensor Guillaume La Roque
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Guillaume La Roque @ 2019-07-31 15:35 UTC (permalink / raw)
  To: daniel.lezcano, khilman
  Cc: linux-pm, devicetree, linux-amlogic, linux-kernel, linux-arm-kernel

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
---
 drivers/thermal/Kconfig           |  11 +
 drivers/thermal/Makefile          |   1 +
 drivers/thermal/amlogic_thermal.c | 332 ++++++++++++++++++++++++++++++
 3 files changed, 344 insertions(+)
 create mode 100644 drivers/thermal/amlogic_thermal.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 9966364a6deb..0f31bb4bc372 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -348,6 +348,17 @@ config MTK_THERMAL
 	  Enable this option if you want to have support for thermal management
 	  controller present in Mediatek SoCs
 
+config AMLOGIC_THERMAL
+	tristate "Amlogic Thermal Support"
+	default ARCH_MESON
+	depends on OF && ARCH_MESON
+	help
+	  If you say yes here you get support for Amlogic Thermal
+	  for G12 SoC Family.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called amlogic_thermal.
+
 menu "Intel thermal drivers"
 depends on X86 || X86_INTEL_QUARK || COMPILE_TEST
 source "drivers/thermal/intel/Kconfig"
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 74a37c7f847a..baeb70bf0568 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -54,3 +54,4 @@ obj-$(CONFIG_MTK_THERMAL)	+= mtk_thermal.o
 obj-$(CONFIG_GENERIC_ADC_THERMAL)	+= thermal-generic-adc.o
 obj-$(CONFIG_ZX2967_THERMAL)	+= zx2967_thermal.o
 obj-$(CONFIG_UNIPHIER_THERMAL)	+= uniphier_thermal.o
+obj-$(CONFIG_AMLOGIC_THERMAL)     += amlogic_thermal.o
diff --git a/drivers/thermal/amlogic_thermal.c b/drivers/thermal/amlogic_thermal.c
new file mode 100644
index 000000000000..13cd4c42721a
--- /dev/null
+++ b/drivers/thermal/amlogic_thermal.c
@@ -0,0 +1,332 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Amlogic Meson Thermal Sensor Driver
+ *
+ * Copyright (C) 2017 Huan Biao <huan.biao@amlogic.com>
+ * Copyright (C) 2019 Guillaume La Roque <glaroque@baylibre.com>
+ *
+ * Register value to celsius temperature formulas:
+ *	Read_Val	    m * U
+ * U = ---------, Uptat = ---------
+ *	2^16		  1 + n * U
+ *
+ * Temperature = A * ( Uptat + u_efuse / 2^16 )- B
+ *
+ *  A B m n : calibration parameters
+ *  u_efuse : fused calibration value, it's a signed 16 bits value
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/thermal.h>
+
+#include "thermal_core.h"
+
+#define TSENSOR_CFG_REG1			0x4
+	#define TSENSOR_CFG_REG1_RSET_VBG	BIT(12)
+	#define TSENSOR_CFG_REG1_RSET_ADC	BIT(11)
+	#define TSENSOR_CFG_REG1_VCM_EN		BIT(10)
+	#define TSENSOR_CFG_REG1_VBG_EN		BIT(9)
+	#define TSENSOR_CFG_REG1_OUT_CTL	BIT(6)
+	#define TSENSOR_CFG_REG1_FILTER_EN	BIT(5)
+	#define TSENSOR_CFG_REG1_DEM_EN		BIT(3)
+	#define TSENSOR_CFG_REG1_CH_SEL		GENMASK(1, 0)
+	#define TSENSOR_CFG_REG1_ENABLE		\
+		(TSENSOR_CFG_REG1_FILTER_EN |	\
+		 TSENSOR_CFG_REG1_VCM_EN |	\
+		 TSENSOR_CFG_REG1_VBG_EN |	\
+		 TSENSOR_CFG_REG1_DEM_EN |	\
+		 TSENSOR_CFG_REG1_CH_SEL)
+
+#define TSENSOR_STAT0			0x40
+
+#define TSENSOR_STAT9			0x64
+
+#define TSENSOR_READ_TEMP_MASK		GENMASK(15, 0)
+#define TSENSOR_TEMP_MASK		GENMASK(11, 0)
+
+#define TSENSOR_TRIM_SIGN_MASK		BIT(15)
+#define TSENSOR_TRIM_TEMP_MASK		GENMASK(14, 0)
+#define TSENSOR_TRIM_VERSION_MASK	GENMASK(31, 24)
+
+#define TSENSOR_TRIM_VERSION(_version)	\
+	FIELD_GET(TSENSOR_TRIM_VERSION_MASK, _version)
+
+#define TSENSOR_TRIM_CALIB_VALID_MASK	(GENMASK(3, 2) | BIT(7))
+
+#define TSENSOR_CALIB_OFFSET	1
+#define TSENSOR_CALIB_SHIFT	4
+
+/**
+ * struct amlogic_thermal_soc_data
+ * @A, B, m, n: calibration parameters
+ * This structure is required for configuration of amlogic thermal driver.
+ */
+struct amlogic_thermal_soc_data {
+	int A;
+	int B;
+	int m;
+	int n;
+};
+
+/**
+ * struct amlogic_thermal_data
+ * @u_efuse_off: register offset to read fused calibration value
+ * @soc: calibration parameters structure pointer
+ * @regmap_config: regmap config for the device
+ * This structure is required for configuration of amlogic thermal driver.
+ */
+struct amlogic_thermal_data {
+	int u_efuse_off;
+	const struct amlogic_thermal_soc_data *soc;
+	const struct regmap_config *regmap_config;
+};
+
+struct amlogic_thermal {
+	struct platform_device *pdev;
+	const struct amlogic_thermal_data *data;
+	struct regmap *regmap;
+	struct regmap *sec_ao_map;
+	struct clk *clk;
+	struct thermal_zone_device *tzd;
+	u32 trim_info;
+	void __iomem *base;
+};
+
+/*
+ * Calculate a temperature value from a temperature code.
+ * The unit of the temperature is degree Celsius.
+ */
+static int code_to_temp(struct amlogic_thermal *pdata, int temp_code)
+{
+	const struct amlogic_thermal_soc_data *param = pdata->data->soc;
+	int temp;
+	s64 factor, Uptat, uefuse;
+
+	uefuse = pdata->trim_info & TSENSOR_TRIM_SIGN_MASK ?
+			     ~(pdata->trim_info & TSENSOR_TRIM_TEMP_MASK) + 1 :
+			     (pdata->trim_info & TSENSOR_TRIM_TEMP_MASK);
+
+	factor = param->n * temp_code;
+	factor = div_s64(factor, 100);
+
+	Uptat = temp_code * param->m;
+	Uptat = div_s64(Uptat, 100);
+	Uptat = Uptat * BIT(16);
+	Uptat = div_s64(Uptat, BIT(16) + factor);
+
+	temp = (Uptat + uefuse) * param->A;
+	temp = div_s64(temp, BIT(16));
+	temp = (temp - param->B) * 100;
+
+	return temp;
+}
+
+static int amlogic_thermal_initialize(struct amlogic_thermal *pdata)
+{
+	int ret = 0;
+	int ver;
+
+	regmap_read(pdata->sec_ao_map, pdata->data->u_efuse_off,
+		    &pdata->trim_info);
+
+	ver = TSENSOR_TRIM_VERSION(pdata->trim_info);
+
+	if ((ver & TSENSOR_TRIM_CALIB_VALID_MASK) == 0) {
+		ret = -EINVAL;
+		dev_err(&pdata->pdev->dev,
+			"tsensor thermal calibration not supported: 0x%x!\n",
+			ver);
+	}
+
+	return ret;
+}
+
+static int amlogic_thermal_enable(struct amlogic_thermal *data)
+{
+	clk_prepare_enable(data->clk);
+	regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
+			   TSENSOR_CFG_REG1_ENABLE, TSENSOR_CFG_REG1_ENABLE);
+
+	return 0;
+}
+
+static int amlogic_thermal_disable(struct amlogic_thermal *data)
+{
+	regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
+			   TSENSOR_CFG_REG1_ENABLE, 0);
+	clk_disable(data->clk);
+
+	return 0;
+}
+
+static int amlogic_thermal_get_temp(void *data, int *temp)
+{
+	unsigned int tvalue;
+	struct amlogic_thermal *pdata = data;
+
+	if (!data)
+		return -EINVAL;
+
+	regmap_read(pdata->regmap, TSENSOR_STAT0, &tvalue);
+	*temp = code_to_temp(pdata,
+			     tvalue & TSENSOR_READ_TEMP_MASK);
+
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops amlogic_thermal_ops = {
+	.get_temp	= amlogic_thermal_get_temp,
+};
+
+static const struct regmap_config amlogic_thermal_regmap_config_g12 = {
+	.reg_bits = 8,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = TSENSOR_STAT9,
+};
+
+static const struct amlogic_thermal_soc_data amlogic_thermal_g12 = {
+	.A = 9411,
+	.B = 3159,
+	.m = 424,
+	.n = 324,
+};
+
+static const struct amlogic_thermal_data amlogic_thermal_g12_cpu_param = {
+	.u_efuse_off = 0x128,
+	.soc = &amlogic_thermal_g12,
+	.regmap_config = &amlogic_thermal_regmap_config_g12,
+};
+
+static const struct amlogic_thermal_data amlogic_thermal_g12_ddr_param = {
+	.u_efuse_off = 0xF0,
+	.soc = &amlogic_thermal_g12,
+	.regmap_config = &amlogic_thermal_regmap_config_g12,
+};
+
+static const struct of_device_id of_amlogic_thermal_match[] = {
+	{
+		.compatible = "amlogic,g12-ddr-thermal",
+		.data = &amlogic_thermal_g12_ddr_param,
+	},
+	{
+		.compatible = "amlogic,g12-cpu-thermal",
+		.data = &amlogic_thermal_g12_cpu_param,
+	},
+	{ /* end */ }
+};
+MODULE_DEVICE_TABLE(of, of_amlogic_thermal_match);
+
+static int amlogic_thermal_probe(struct platform_device *pdev)
+{
+	struct amlogic_thermal *pdata;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	int ret;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->data = of_device_get_match_data(dev);
+	pdata->pdev = pdev;
+	platform_set_drvdata(pdev, pdata);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pdata->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pdata->base)) {
+		dev_err(dev, "failed to get io address\n");
+		return PTR_ERR(pdata->base);
+	}
+
+	pdata->regmap = devm_regmap_init_mmio(dev, pdata->base,
+					      pdata->data->regmap_config);
+	if (IS_ERR(pdata->regmap))
+		return PTR_ERR(pdata->regmap);
+
+	pdata->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pdata->clk)) {
+		if (PTR_ERR(pdata->clk) != -EPROBE_DEFER)
+			dev_err(dev, "failed to get clock\n");
+		return PTR_ERR(pdata->clk);
+	}
+
+	pdata->sec_ao_map = syscon_regmap_lookup_by_phandle
+		(pdev->dev.of_node, "amlogic,ao-secure");
+	if (IS_ERR(pdata->sec_ao_map)) {
+		dev_err(dev, "syscon regmap lookup failed.\n");
+		return PTR_ERR(pdata->sec_ao_map);
+	}
+
+	pdata->tzd = devm_thermal_zone_of_sensor_register
+				(&pdev->dev,
+				 0,
+				 pdata,
+				 &amlogic_thermal_ops);
+	if (IS_ERR(pdata->tzd)) {
+		ret = PTR_ERR(pdata->tzd);
+		dev_err(dev, "Failed to register tsensor: %d\n", ret);
+		return PTR_ERR(pdata->tzd);
+	}
+
+	ret = amlogic_thermal_initialize(pdata);
+	if (ret)
+		return ret;
+
+	ret = amlogic_thermal_enable(pdata);
+	if (ret)
+		clk_unprepare(pdata->clk);
+
+	return ret;
+}
+
+static int amlogic_thermal_remove(struct platform_device *pdev)
+{
+	struct amlogic_thermal *data = platform_get_drvdata(pdev);
+
+	return amlogic_thermal_disable(data);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int amlogic_thermal_suspend(struct device *dev)
+{
+	struct amlogic_thermal *data = dev_get_drvdata(dev);
+
+	return amlogic_thermal_disable(data);
+}
+
+static int amlogic_thermal_resume(struct device *dev)
+{
+	struct amlogic_thermal *data = dev_get_drvdata(dev);
+
+	return amlogic_thermal_enable(data);
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(amlogic_thermal_pm_ops,
+			 amlogic_thermal_suspend, amlogic_thermal_resume);
+
+static struct platform_driver amlogic_thermal_driver = {
+	.driver = {
+		.name		= "amlogic_thermal",
+		.pm		= &amlogic_thermal_pm_ops,
+		.of_match_table = of_amlogic_thermal_match,
+	},
+	.probe	= amlogic_thermal_probe,
+	.remove	= amlogic_thermal_remove,
+};
+
+module_platform_driver(amlogic_thermal_driver);
+
+MODULE_AUTHOR("Guillaume La Roque <glaroque@baylibre.com>");
+MODULE_DESCRIPTION("Amlogic thermal driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 3/6] arm64: dts: amlogic: g12: add temperature sensor
  2019-07-31 15:35 [PATCH v2 0/6] Add support of New Amlogic temperature sensor for G12 SoCs Guillaume La Roque
  2019-07-31 15:35 ` [PATCH v2 1/6] dt-bindings: thermal: Add DT bindings documentation for Amlogic Thermal Guillaume La Roque
  2019-07-31 15:35 ` [PATCH v2 2/6] thermal: amlogic: Add thermal driver to support G12 SoCs Guillaume La Roque
@ 2019-07-31 15:35 ` Guillaume La Roque
  2019-08-03 17:52   ` Martin Blumenstingl
  2019-07-31 15:35 ` [PATCH v2 4/6] arm64: dts: meson: sei510: Add minimal thermal zone Guillaume La Roque
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Guillaume La Roque @ 2019-07-31 15:35 UTC (permalink / raw)
  To: daniel.lezcano, khilman
  Cc: linux-pm, devicetree, linux-amlogic, linux-kernel, linux-arm-kernel

Add cpu and ddr temperature sensors for G12 Socs

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
---
 .../boot/dts/amlogic/meson-g12-common.dtsi    | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
index 06e186ca41e3..7f862a3490fb 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
@@ -1353,6 +1353,28 @@
 				};
 			};
 
+			cpu_temp: temperature-sensor@34800 {
+				compatible = "amlogic,g12-cpu-thermal",
+					     "amlogic,g12-thermal";
+				reg = <0x0 0x34800 0x0 0x50>;
+				interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>;
+				clocks = <&clkc CLKID_TS>;
+				status = "okay";
+				#thermal-sensor-cells = <0>;
+				amlogic,ao-secure = <&sec_AO>;
+			};
+
+			ddr_temp: temperature-sensor@34c00 {
+				compatible = "amlogic,g12-ddr-thermal",
+					     "amlogic,g12-thermal";
+				reg = <0x0 0x34c00 0x0 0x50>;
+				interrupts = <GIC_SPI 36 IRQ_TYPE_EDGE_RISING>;
+				clocks = <&clkc CLKID_TS>;
+				status = "okay";
+				#thermal-sensor-cells = <0>;
+				amlogic,ao-secure = <&sec_AO>;
+			};
+
 			usb2_phy0: phy@36000 {
 				compatible = "amlogic,g12a-usb2-phy";
 				reg = <0x0 0x36000 0x0 0x2000>;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 4/6] arm64: dts: meson: sei510: Add minimal thermal zone
  2019-07-31 15:35 [PATCH v2 0/6] Add support of New Amlogic temperature sensor for G12 SoCs Guillaume La Roque
                   ` (2 preceding siblings ...)
  2019-07-31 15:35 ` [PATCH v2 3/6] arm64: dts: amlogic: g12: add temperature sensor Guillaume La Roque
@ 2019-07-31 15:35 ` Guillaume La Roque
  2019-08-03 18:29   ` Martin Blumenstingl
  2019-07-31 15:35 ` [PATCH v2 5/6] arm64: dts: amlogic: odroid-n2: add " Guillaume La Roque
  2019-07-31 15:35 ` [PATCH v2 6/6] MAINTAINERS: add entry for Amlogic Thermal driver Guillaume La Roque
  5 siblings, 1 reply; 17+ messages in thread
From: Guillaume La Roque @ 2019-07-31 15:35 UTC (permalink / raw)
  To: daniel.lezcano, khilman
  Cc: linux-pm, devicetree, linux-amlogic, linux-kernel, linux-arm-kernel

Add minimal thermal zone for DDR and CPU sensor

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
---
 .../boot/dts/amlogic/meson-g12a-sei510.dts    | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
index 979449968a5f..2c16a2cba0a3 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
@@ -10,6 +10,7 @@
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/gpio/meson-g12a-gpio.h>
 #include <dt-bindings/sound/meson-g12a-tohdmitx.h>
+#include <dt-bindings/thermal/thermal.h>
 
 / {
 	compatible = "seirobotics,sei510", "amlogic,g12a";
@@ -33,6 +34,53 @@
 		ethernet0 = &ethmac;
 	};
 
+	thermal-zones {
+		cpu-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <100>;
+			thermal-sensors = <&cpu_temp>;
+
+			trips {
+				cpu_critical: cpu-critical {
+					temperature = <110000>; /* millicelsius */
+					hysteresis = <2000>; /* millicelsius */
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map {
+					trip = <&cpu_critical>;
+					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+
+		ddr-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <100>;
+			thermal-sensors = <&ddr_temp>;
+
+			trips {
+				ddr_critical: ddr-critical {
+					temperature = <110000>; /* millicelsius */
+					hysteresis = <2000>; /* millicelsius */
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map {
+					trip = <&ddr_critical>;
+					cooling-device = <&mali THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+	};
+
 	mono_dac: audio-codec-0 {
 		compatible = "maxim,max98357a";
 		#sound-dai-cells = <0>;
@@ -321,6 +369,7 @@
 	operating-points-v2 = <&cpu_opp_table>;
 	clocks = <&clkc CLKID_CPU_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu1 {
@@ -328,6 +377,7 @@
 	operating-points-v2 = <&cpu_opp_table>;
 	clocks = <&clkc CLKID_CPU_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu2 {
@@ -335,6 +385,7 @@
 	operating-points-v2 = <&cpu_opp_table>;
 	clocks = <&clkc CLKID_CPU_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu3 {
@@ -342,6 +393,7 @@
 	operating-points-v2 = <&cpu_opp_table>;
 	clocks = <&clkc CLKID_CPU_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cvbs_vdac_port {
@@ -368,6 +420,10 @@
 	status = "okay";
 };
 
+&mali {
+	#cooling-cells = <2>;
+};
+
 &hdmi_tx {
 	status = "okay";
 	pinctrl-0 = <&hdmitx_hpd_pins>, <&hdmitx_ddc_pins>;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 5/6] arm64: dts: amlogic: odroid-n2: add minimal thermal zone
  2019-07-31 15:35 [PATCH v2 0/6] Add support of New Amlogic temperature sensor for G12 SoCs Guillaume La Roque
                   ` (3 preceding siblings ...)
  2019-07-31 15:35 ` [PATCH v2 4/6] arm64: dts: meson: sei510: Add minimal thermal zone Guillaume La Roque
@ 2019-07-31 15:35 ` Guillaume La Roque
  2019-07-31 15:35 ` [PATCH v2 6/6] MAINTAINERS: add entry for Amlogic Thermal driver Guillaume La Roque
  5 siblings, 0 replies; 17+ messages in thread
From: Guillaume La Roque @ 2019-07-31 15:35 UTC (permalink / raw)
  To: daniel.lezcano, khilman
  Cc: linux-pm, devicetree, linux-amlogic, linux-kernel, linux-arm-kernel

Add minimal thermal zone for DDR and CPU sensor

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
---
 .../boot/dts/amlogic/meson-g12b-odroid-n2.dts | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
index 75ff8a7e373d..a7d73c0c8447 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
@@ -10,6 +10,7 @@
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/gpio/meson-g12a-gpio.h>
 #include <dt-bindings/sound/meson-g12a-tohdmitx.h>
+#include <dt-bindings/thermal/thermal.h>
 
 / {
 	compatible = "hardkernel,odroid-n2", "amlogic,g12b";
@@ -20,6 +21,55 @@
 		ethernet0 = &ethmac;
 	};
 
+	thermal-zones {
+		cpu-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <100>;
+			thermal-sensors = <&cpu_temp>;
+
+			trips {
+				cpu_critical: cpu-critical {
+					temperature = <110000>; /* millicelsius */
+					hysteresis = <2000>; /* millicelsius */
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map {
+					trip = <&cpu_critical>;
+					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu100 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu101 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu102 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu103 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+
+		ddr-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <100>;
+			thermal-sensors = <&ddr_temp>;
+
+			trips {
+				ddr_critical: ddr-critical {
+					temperature = <110000>; /* millicelsius */
+					hysteresis = <2000>; /* millicelsius */
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map {
+					trip = <&ddr_critical>;
+					cooling-device = <&mali THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+	};
+
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
@@ -288,6 +338,7 @@
 	operating-points-v2 = <&cpu_opp_table_0>;
 	clocks = <&clkc CLKID_CPU_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu1 {
@@ -295,6 +346,7 @@
 	operating-points-v2 = <&cpu_opp_table_0>;
 	clocks = <&clkc CLKID_CPU_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu100 {
@@ -302,6 +354,7 @@
 	operating-points-v2 = <&cpub_opp_table_1>;
 	clocks = <&clkc CLKID_CPUB_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu101 {
@@ -309,6 +362,7 @@
 	operating-points-v2 = <&cpub_opp_table_1>;
 	clocks = <&clkc CLKID_CPUB_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu102 {
@@ -316,6 +370,7 @@
 	operating-points-v2 = <&cpub_opp_table_1>;
 	clocks = <&clkc CLKID_CPUB_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu103 {
@@ -323,6 +378,7 @@
 	operating-points-v2 = <&cpub_opp_table_1>;
 	clocks = <&clkc CLKID_CPUB_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &ext_mdio {
@@ -377,6 +433,10 @@
 	};
 };
 
+&mali {
+	#cooling-cells = <2>;
+};
+
 &hdmi_tx {
 	status = "okay";
 	pinctrl-0 = <&hdmitx_hpd_pins>, <&hdmitx_ddc_pins>;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 6/6] MAINTAINERS: add entry for Amlogic Thermal driver
  2019-07-31 15:35 [PATCH v2 0/6] Add support of New Amlogic temperature sensor for G12 SoCs Guillaume La Roque
                   ` (4 preceding siblings ...)
  2019-07-31 15:35 ` [PATCH v2 5/6] arm64: dts: amlogic: odroid-n2: add " Guillaume La Roque
@ 2019-07-31 15:35 ` Guillaume La Roque
  5 siblings, 0 replies; 17+ messages in thread
From: Guillaume La Roque @ 2019-07-31 15:35 UTC (permalink / raw)
  To: daniel.lezcano, khilman
  Cc: linux-pm, devicetree, linux-amlogic, linux-kernel, linux-arm-kernel

Add myself as maintainer for Amlogic Thermal driver.

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index fb2b12f75c37..299f27d11058 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15910,6 +15910,15 @@ F:	Documentation/driver-api/thermal/cpu-cooling-api.rst
 F:	drivers/thermal/cpu_cooling.c
 F:	include/linux/cpu_cooling.h
 
+THERMAL DRIVER FOR AMLOGIC SOCS
+M:	Guillaume La Roque <glaroque@baylibre.com>
+L:	linux-pm@vger.kernel.org
+L:	linux-amlogic@lists.infradead.org
+W:	http://linux-meson.com/
+S:	Supported
+F:	drivers/thermal/amlogic_thermal.c
+F:	Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
+
 THINKPAD ACPI EXTRAS DRIVER
 M:	Henrique de Moraes Holschuh <ibm-acpi@hmh.eng.br>
 L:	ibm-acpi-devel@lists.sourceforge.net
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/6] dt-bindings: thermal: Add DT bindings documentation for Amlogic Thermal
  2019-07-31 15:35 ` [PATCH v2 1/6] dt-bindings: thermal: Add DT bindings documentation for Amlogic Thermal Guillaume La Roque
@ 2019-07-31 17:59   ` Rob Herring
  0 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2019-07-31 17:59 UTC (permalink / raw)
  To: Guillaume La Roque
  Cc: Daniel Lezcano, Kevin Hilman, open list:THERMAL, devicetree,
	linux-amlogic, linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

On Wed, Jul 31, 2019 at 9:36 AM Guillaume La Roque
<glaroque@baylibre.com> wrote:
>
> Adding the devicetree binding documentation for the Amlogic temperature
> sensor found in the Amlogic Meson G12 SoCs.
> the G12A  and G12B SoCs are supported.
>
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> ---
>  .../bindings/thermal/amlogic,thermal.yaml     | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
>
> diff --git a/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml b/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
> new file mode 100644
> index 000000000000..f10537ab4c8b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/thermal/amlogic,thermal.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Amlogic Thermal Driver

This is not a driver.

> +
> +maintainers:
> +  - Guillaume La Roque <glaroque@baylibre.com>
> +
> +description: Amlogic Thermal driver
> +
> +properties:
> +  compatible:
> +    oneOf:

oneOf is not necessary as there is only one of.

> +      - items:
> +          - enum:
> +              - amlogic,g12-cpu-thermal
> +              - amlogic,g12-ddr-thermal
> +          - const:
> +              - amlogic,g12-thermal

Please run 'make dt_binding_check'. You'll find this is not valid json-schema.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  amlogic,ao-secure:
> +    description: phandle to the ao-secure syscon
> +    allOf:
> +     - $ref: /schemas/types.yaml#/definitions/uint32
> +
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - amlogic,ao-secure
> +
> +examples:
> +  - |
> +        cpu_temp: temperature-sensor@ff634800 {
> +                compatible = "amlogic,g12-cpu-thermal",
> +                             "amlogic,g12-thermal";
> +                reg = <0x0 0xff634800 0x0 0x50>;

Examples are built now and this will have an error. The default
address and size cells are 1 for examples, so you either have to
override them or adjust this.

> +                interrupts = <0x0 0x24 0x0>;
> +                clocks = <&clk 164>;
> +                status = "okay";

Don't show status in examples.

> +                #thermal-sensor-cells = <0>;
> +                amlogic,ao-secure = <&sec_AO>;
> +        };
> +...
> \ No newline at end of file
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 3/6] arm64: dts: amlogic: g12: add temperature sensor
  2019-07-31 15:35 ` [PATCH v2 3/6] arm64: dts: amlogic: g12: add temperature sensor Guillaume La Roque
@ 2019-08-03 17:52   ` Martin Blumenstingl
  2019-08-05  9:41     ` guillaume La Roque
  0 siblings, 1 reply; 17+ messages in thread
From: Martin Blumenstingl @ 2019-08-03 17:52 UTC (permalink / raw)
  To: Guillaume La Roque
  Cc: daniel.lezcano, khilman, devicetree, linux-amlogic, linux-kernel,
	linux-arm-kernel, linux-pm

Hi Guillaume,

On Wed, Jul 31, 2019 at 5:36 PM Guillaume La Roque
<glaroque@baylibre.com> wrote:
>
> Add cpu and ddr temperature sensors for G12 Socs
>
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
with the nit-pick below addressed:
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

> ---
>  .../boot/dts/amlogic/meson-g12-common.dtsi    | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
> index 06e186ca41e3..7f862a3490fb 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
> @@ -1353,6 +1353,28 @@
>                                 };
>                         };
>
> +                       cpu_temp: temperature-sensor@34800 {
> +                               compatible = "amlogic,g12-cpu-thermal",
> +                                            "amlogic,g12-thermal";
> +                               reg = <0x0 0x34800 0x0 0x50>;
> +                               interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>;
> +                               clocks = <&clkc CLKID_TS>;
> +                               status = "okay";
I believe nodes are enabled automatically if they don't have a status property

> +                               #thermal-sensor-cells = <0>;
> +                               amlogic,ao-secure = <&sec_AO>;
> +                       };
> +
> +                       ddr_temp: temperature-sensor@34c00 {
> +                               compatible = "amlogic,g12-ddr-thermal",
> +                                            "amlogic,g12-thermal";
> +                               reg = <0x0 0x34c00 0x0 0x50>;
> +                               interrupts = <GIC_SPI 36 IRQ_TYPE_EDGE_RISING>;
> +                               clocks = <&clkc CLKID_TS>;
> +                               status = "okay";
same here


Martin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/6] thermal: amlogic: Add thermal driver to support G12 SoCs
  2019-07-31 15:35 ` [PATCH v2 2/6] thermal: amlogic: Add thermal driver to support G12 SoCs Guillaume La Roque
@ 2019-08-03 18:24   ` Martin Blumenstingl
  2019-08-05 12:48     ` guillaume La Roque
  0 siblings, 1 reply; 17+ messages in thread
From: Martin Blumenstingl @ 2019-08-03 18:24 UTC (permalink / raw)
  To: Guillaume La Roque
  Cc: daniel.lezcano, khilman, devicetree, linux-amlogic, linux-kernel,
	linux-arm-kernel, linux-pm

Hi Guillaume,

(I still don't have any experience with the thermal framework, so
below are some general comments)

On Wed, Jul 31, 2019 at 5:36 PM Guillaume La Roque
<glaroque@baylibre.com> wrote:
I would add a patch description here:
"
Amlogic G12A and G12B SoCs integrate two thermal sensors with the same design.
One is located close to the DDR (controller?) and the other one is
located close to the PLLs (between the CPU and GPU).

The calibration data for each of the thermal sensors instance is
stored in a different location within the AO region.

Implement reading the temperature from each thermal sensor.

The IP block has more functionality, which may be added to this driver
in the future:
- reading up to 16 stored temperature samples
- chip reset when the temperature exceeds a configurable threshold
- up to four interrupts when the temperature has risen above a
configurable threshold
- up to four interrupts when the temperature has fallen below a
configurable threshold
"

> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> ---
>  drivers/thermal/Kconfig           |  11 +
>  drivers/thermal/Makefile          |   1 +
>  drivers/thermal/amlogic_thermal.c | 332 ++++++++++++++++++++++++++++++
>  3 files changed, 344 insertions(+)
>  create mode 100644 drivers/thermal/amlogic_thermal.c
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 9966364a6deb..0f31bb4bc372 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -348,6 +348,17 @@ config MTK_THERMAL
>           Enable this option if you want to have support for thermal management
>           controller present in Mediatek SoCs
>
> +config AMLOGIC_THERMAL
we typically use "MESON" in the Kconfig symbols:
$ grep -c AMLOGIC .config
1
$ grep -c MESON .config
33

I also wonder if we should add G12 or G12A so we don't conflict with
upcoming thermal sensors with a different design (assuming that this
will be a thing).
for example we already have three different USB2 PHY drivers

[...]
> +/*
> + * Calculate a temperature value from a temperature code.
> + * The unit of the temperature is degree Celsius.
is it really degree Celsius or millicelsius?

> + */
> +static int code_to_temp(struct amlogic_thermal *pdata, int temp_code)
PREFIX_thermal_code_to_millicelsius?

[...]
> +static int amlogic_thermal_enable(struct amlogic_thermal *data)
> +{
> +       clk_prepare_enable(data->clk);
no clock error handling?

> +       regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
> +                          TSENSOR_CFG_REG1_ENABLE, TSENSOR_CFG_REG1_ENABLE);
> +
> +       return 0;
> +}
> +
> +static int amlogic_thermal_disable(struct amlogic_thermal *data)
> +{
> +       regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
> +                          TSENSOR_CFG_REG1_ENABLE, 0);
> +       clk_disable(data->clk);
either clk_disable_unprepare here or use only clk_enable in
amlogic_thermal_enable and move prepare/unprepare somewhere else

> +
> +       return 0;
> +}
> +
> +static int amlogic_thermal_get_temp(void *data, int *temp)
> +{
> +       unsigned int tvalue;
> +       struct amlogic_thermal *pdata = data;
> +
> +       if (!data)
> +               return -EINVAL;
> +
> +       regmap_read(pdata->regmap, TSENSOR_STAT0, &tvalue);
> +       *temp = code_to_temp(pdata,
> +                            tvalue & TSENSOR_READ_TEMP_MASK);
maybe simply move the implementation from code_to_temp here?

[...]
> +static const struct amlogic_thermal_data amlogic_thermal_g12_cpu_param = {
> +       .u_efuse_off = 0x128,
> +       .soc = &amlogic_thermal_g12,
based on the variable name I expected this to be an enum of some sort.
I would have expected it to be called calibration_parameters or
similar to match the explanation in the driver description
(no need to change it if you prefer it like this, I just want to make
you aware of this)

> +       .regmap_config = &amlogic_thermal_regmap_config_g12,
if regmap_config is always the same you may also pass it directly to
devm_regmap_init_mmio

> +};
> +
> +static const struct amlogic_thermal_data amlogic_thermal_g12_ddr_param = {
> +       .u_efuse_off = 0xF0,
I believe we use lower-case hex everywhere else

[...]
> +static const struct of_device_id of_amlogic_thermal_match[] = {
> +       {
> +               .compatible = "amlogic,g12-ddr-thermal",
> +               .data = &amlogic_thermal_g12_ddr_param,
> +       },
> +       {
> +               .compatible = "amlogic,g12-cpu-thermal",
> +               .data = &amlogic_thermal_g12_cpu_param,
> +       },
> +       { /* end */ }
other drivers use "sentinel", but personally I have no preference here

[...]
> +       pdata->tzd = devm_thermal_zone_of_sensor_register
> +                               (&pdev->dev,
> +                                0,
> +                                pdata,
> +                                &amlogic_thermal_ops);
I believe the opening brace has to go onto the same line as the function name

[...]
> +       ret = amlogic_thermal_enable(pdata);
> +       if (ret)
> +               clk_unprepare(pdata->clk);
clk_disable_unprepare, else you'll leave the clock prepared

> +#ifdef CONFIG_PM_SLEEP
> +static int amlogic_thermal_suspend(struct device *dev)
> +{
> +       struct amlogic_thermal *data = dev_get_drvdata(dev);
> +
> +       return amlogic_thermal_disable(data);
> +}
> +
> +static int amlogic_thermal_resume(struct device *dev)
> +{
> +       struct amlogic_thermal *data = dev_get_drvdata(dev);
> +
> +       return amlogic_thermal_enable(data);
> +}
> +#endif
instead of using an #ifdef here annotate the suspend/resume functions
with __maybe_unused, see [0]


Martin


[0] https://lore.kernel.org/patchwork/patch/732981/

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 4/6] arm64: dts: meson: sei510: Add minimal thermal zone
  2019-07-31 15:35 ` [PATCH v2 4/6] arm64: dts: meson: sei510: Add minimal thermal zone Guillaume La Roque
@ 2019-08-03 18:29   ` Martin Blumenstingl
  2019-08-05 12:48     ` guillaume La Roque
  0 siblings, 1 reply; 17+ messages in thread
From: Martin Blumenstingl @ 2019-08-03 18:29 UTC (permalink / raw)
  To: Guillaume La Roque
  Cc: daniel.lezcano, khilman, devicetree, linux-amlogic, linux-kernel,
	linux-arm-kernel, linux-pm

Hi Guillaume,

On Wed, Jul 31, 2019 at 5:36 PM Guillaume La Roque
<glaroque@baylibre.com> wrote:
>
> Add minimal thermal zone for DDR and CPU sensor
so high DDR (controller?) temperatures will throttle Mali and high PLL
temperatures will throttle the CPU?
to get things started I'm fine with this, but I think it should be
mentioned here

> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> ---
>  .../boot/dts/amlogic/meson-g12a-sei510.dts    | 56 +++++++++++++++++++
>  1 file changed, 56 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
> index 979449968a5f..2c16a2cba0a3 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
> @@ -10,6 +10,7 @@
>  #include <dt-bindings/input/input.h>
>  #include <dt-bindings/gpio/meson-g12a-gpio.h>
>  #include <dt-bindings/sound/meson-g12a-tohdmitx.h>
> +#include <dt-bindings/thermal/thermal.h>
>
>  / {
>         compatible = "seirobotics,sei510", "amlogic,g12a";
> @@ -33,6 +34,53 @@
>                 ethernet0 = &ethmac;
>         };
>
> +       thermal-zones {
> +               cpu-thermal {
> +                       polling-delay = <1000>;
> +                       polling-delay-passive = <100>;
> +                       thermal-sensors = <&cpu_temp>;
> +
> +                       trips {
> +                               cpu_critical: cpu-critical {
> +                                       temperature = <110000>; /* millicelsius */
> +                                       hysteresis = <2000>; /* millicelsius */
> +                                       type = "critical";
> +                               };
> +                       };
> +
> +                       cooling-maps {
> +                               map {
> +                                       trip = <&cpu_critical>;
> +                                       cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +                                                        <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +                                                        <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +                                                        <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +                               };
> +                       };
> +               };
> +
> +               ddr-thermal {
> +                       polling-delay = <1000>;
> +                       polling-delay-passive = <100>;
> +                       thermal-sensors = <&ddr_temp>;
> +
> +                       trips {
> +                               ddr_critical: ddr-critical {
> +                                       temperature = <110000>; /* millicelsius */
> +                                       hysteresis = <2000>; /* millicelsius */
> +                                       type = "critical";
> +                               };
> +                       };
> +
> +                       cooling-maps {
> +                               map {
> +                                       trip = <&ddr_critical>;
> +                                       cooling-device = <&mali THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +                               };
> +                       };
> +               };
> +       };
> +
>         mono_dac: audio-codec-0 {
>                 compatible = "maxim,max98357a";
>                 #sound-dai-cells = <0>;
> @@ -321,6 +369,7 @@
>         operating-points-v2 = <&cpu_opp_table>;
>         clocks = <&clkc CLKID_CPU_CLK>;
>         clock-latency = <50000>;
> +       #cooling-cells = <2>;
>  };
>
>  &cpu1 {
> @@ -328,6 +377,7 @@
>         operating-points-v2 = <&cpu_opp_table>;
>         clocks = <&clkc CLKID_CPU_CLK>;
>         clock-latency = <50000>;
> +       #cooling-cells = <2>;
>  };
>
>  &cpu2 {
> @@ -335,6 +385,7 @@
>         operating-points-v2 = <&cpu_opp_table>;
>         clocks = <&clkc CLKID_CPU_CLK>;
>         clock-latency = <50000>;
> +       #cooling-cells = <2>;
>  };
>
>  &cpu3 {
> @@ -342,6 +393,7 @@
>         operating-points-v2 = <&cpu_opp_table>;
>         clocks = <&clkc CLKID_CPU_CLK>;
>         clock-latency = <50000>;
> +       #cooling-cells = <2>;
>  };
>
>  &cvbs_vdac_port {
> @@ -368,6 +420,10 @@
>         status = "okay";
>  };
>
> +&mali {
> +       #cooling-cells = <2>;
> +};
is there something device-specific in this patch? I'm wondering
whether we can move all of this go g12a.dtsi to simplify maintenance
in the future


Martin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 3/6] arm64: dts: amlogic: g12: add temperature sensor
  2019-08-03 17:52   ` Martin Blumenstingl
@ 2019-08-05  9:41     ` guillaume La Roque
  0 siblings, 0 replies; 17+ messages in thread
From: guillaume La Roque @ 2019-08-05  9:41 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: daniel.lezcano, khilman, devicetree, linux-amlogic, linux-kernel,
	linux-arm-kernel, linux-pm

hi Martin,


thanks for comments i will fix in v3.


guillaume

On 8/3/19 7:52 PM, Martin Blumenstingl wrote:
> Hi Guillaume,
>
> On Wed, Jul 31, 2019 at 5:36 PM Guillaume La Roque
> <glaroque@baylibre.com> wrote:
>> Add cpu and ddr temperature sensors for G12 Socs
>>
>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> with the nit-pick below addressed:
> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
>> ---
>>  .../boot/dts/amlogic/meson-g12-common.dtsi    | 22 +++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
>> index 06e186ca41e3..7f862a3490fb 100644
>> --- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
>> +++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
>> @@ -1353,6 +1353,28 @@
>>                                 };
>>                         };
>>
>> +                       cpu_temp: temperature-sensor@34800 {
>> +                               compatible = "amlogic,g12-cpu-thermal",
>> +                                            "amlogic,g12-thermal";
>> +                               reg = <0x0 0x34800 0x0 0x50>;
>> +                               interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>;
>> +                               clocks = <&clkc CLKID_TS>;
>> +                               status = "okay";
> I believe nodes are enabled automatically if they don't have a status property
>
>> +                               #thermal-sensor-cells = <0>;
>> +                               amlogic,ao-secure = <&sec_AO>;
>> +                       };
>> +
>> +                       ddr_temp: temperature-sensor@34c00 {
>> +                               compatible = "amlogic,g12-ddr-thermal",
>> +                                            "amlogic,g12-thermal";
>> +                               reg = <0x0 0x34c00 0x0 0x50>;
>> +                               interrupts = <GIC_SPI 36 IRQ_TYPE_EDGE_RISING>;
>> +                               clocks = <&clkc CLKID_TS>;
>> +                               status = "okay";
> same here
>
>
> Martin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 4/6] arm64: dts: meson: sei510: Add minimal thermal zone
  2019-08-03 18:29   ` Martin Blumenstingl
@ 2019-08-05 12:48     ` guillaume La Roque
  0 siblings, 0 replies; 17+ messages in thread
From: guillaume La Roque @ 2019-08-05 12:48 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: daniel.lezcano, khilman, devicetree, linux-amlogic, linux-kernel,
	linux-arm-kernel, linux-pm

Hi Martin,


On 8/3/19 8:29 PM, Martin Blumenstingl wrote:
> Hi Guillaume,
>
> On Wed, Jul 31, 2019 at 5:36 PM Guillaume La Roque
> <glaroque@baylibre.com> wrote:
>> Add minimal thermal zone for DDR and CPU sensor
> so high DDR (controller?) temperatures will throttle Mali and high PLL
> temperatures will throttle the CPU?
> to get things started I'm fine with this, but I think it should be
> mentioned here

i will add in commit description

>
>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
>> ---
>>  .../boot/dts/amlogic/meson-g12a-sei510.dts    | 56 +++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
>> index 979449968a5f..2c16a2cba0a3 100644
>> --- a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
>> +++ b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
>> @@ -10,6 +10,7 @@
>>  #include <dt-bindings/input/input.h>
>>  #include <dt-bindings/gpio/meson-g12a-gpio.h>
>>  #include <dt-bindings/sound/meson-g12a-tohdmitx.h>
>> +#include <dt-bindings/thermal/thermal.h>
>>
>>  / {
>>         compatible = "seirobotics,sei510", "amlogic,g12a";
>> @@ -33,6 +34,53 @@
>>                 ethernet0 = &ethmac;
>>         };
>>
>> +       thermal-zones {
>> +               cpu-thermal {
>> +                       polling-delay = <1000>;
>> +                       polling-delay-passive = <100>;
>> +                       thermal-sensors = <&cpu_temp>;
>> +
>> +                       trips {
>> +                               cpu_critical: cpu-critical {
>> +                                       temperature = <110000>; /* millicelsius */
>> +                                       hysteresis = <2000>; /* millicelsius */
>> +                                       type = "critical";
>> +                               };
>> +                       };
>> +
>> +                       cooling-maps {
>> +                               map {
>> +                                       trip = <&cpu_critical>;
>> +                                       cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
>> +                                                        <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
>> +                                                        <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
>> +                                                        <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>> +                               };
>> +                       };
>> +               };
>> +
>> +               ddr-thermal {
>> +                       polling-delay = <1000>;
>> +                       polling-delay-passive = <100>;
>> +                       thermal-sensors = <&ddr_temp>;
>> +
>> +                       trips {
>> +                               ddr_critical: ddr-critical {
>> +                                       temperature = <110000>; /* millicelsius */
>> +                                       hysteresis = <2000>; /* millicelsius */
>> +                                       type = "critical";
>> +                               };
>> +                       };
>> +
>> +                       cooling-maps {
>> +                               map {
>> +                                       trip = <&ddr_critical>;
>> +                                       cooling-device = <&mali THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>> +                               };
>> +                       };
>> +               };
>> +       };
>> +
>>         mono_dac: audio-codec-0 {
>>                 compatible = "maxim,max98357a";
>>                 #sound-dai-cells = <0>;
>> @@ -321,6 +369,7 @@
>>         operating-points-v2 = <&cpu_opp_table>;
>>         clocks = <&clkc CLKID_CPU_CLK>;
>>         clock-latency = <50000>;
>> +       #cooling-cells = <2>;
>>  };
>>
>>  &cpu1 {
>> @@ -328,6 +377,7 @@
>>         operating-points-v2 = <&cpu_opp_table>;
>>         clocks = <&clkc CLKID_CPU_CLK>;
>>         clock-latency = <50000>;
>> +       #cooling-cells = <2>;
>>  };
>>
>>  &cpu2 {
>> @@ -335,6 +385,7 @@
>>         operating-points-v2 = <&cpu_opp_table>;
>>         clocks = <&clkc CLKID_CPU_CLK>;
>>         clock-latency = <50000>;
>> +       #cooling-cells = <2>;
>>  };
>>
>>  &cpu3 {
>> @@ -342,6 +393,7 @@
>>         operating-points-v2 = <&cpu_opp_table>;
>>         clocks = <&clkc CLKID_CPU_CLK>;
>>         clock-latency = <50000>;
>> +       #cooling-cells = <2>;
>>  };
>>
>>  &cvbs_vdac_port {
>> @@ -368,6 +420,10 @@
>>         status = "okay";
>>  };
>>
>> +&mali {
>> +       #cooling-cells = <2>;
>> +};
> is there something device-specific in this patch? I'm wondering
> whether we can move all of this go g12a.dtsi to simplify maintenance
> in the future

this is depending of each board. actually it's same on all

but if a new one have a fan this value should be different or not.


>
>
> Martin


thanks,

Guillaume


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/6] thermal: amlogic: Add thermal driver to support G12 SoCs
  2019-08-03 18:24   ` Martin Blumenstingl
@ 2019-08-05 12:48     ` guillaume La Roque
  2019-08-06 19:52       ` Martin Blumenstingl
  0 siblings, 1 reply; 17+ messages in thread
From: guillaume La Roque @ 2019-08-05 12:48 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: daniel.lezcano, khilman, devicetree, linux-amlogic, linux-kernel,
	linux-arm-kernel, linux-pm

Hi Martin,

again thanks for your review.

On 8/3/19 8:24 PM, Martin Blumenstingl wrote:
> Hi Guillaume,
>
> (I still don't have any experience with the thermal framework, so
> below are some general comments)
>
> On Wed, Jul 31, 2019 at 5:36 PM Guillaume La Roque
> <glaroque@baylibre.com> wrote:
> I would add a patch description here:
> "
> Amlogic G12A and G12B SoCs integrate two thermal sensors with the same design.
> One is located close to the DDR (controller?) and the other one is
> located close to the PLLs (between the CPU and GPU).
>
> The calibration data for each of the thermal sensors instance is
> stored in a different location within the AO region.
>
> Implement reading the temperature from each thermal sensor.
>
> The IP block has more functionality, which may be added to this driver
> in the future:
> - reading up to 16 stored temperature samples

it's not working, you can verify it if you check the regmap define in the driver. in fact temp is only write in one register, it's confirmed by amlogic.

> - chip reset when the temperature exceeds a configurable threshold
> - up to four interrupts when the temperature has risen above a
> configurable threshold
> - up to four interrupts when the temperature has fallen below a
> configurable threshold
> "

agreed with you to add description  and optional without 16 register part.

>
>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
>> ---
>>  drivers/thermal/Kconfig           |  11 +
>>  drivers/thermal/Makefile          |   1 +
>>  drivers/thermal/amlogic_thermal.c | 332 ++++++++++++++++++++++++++++++
>>  3 files changed, 344 insertions(+)
>>  create mode 100644 drivers/thermal/amlogic_thermal.c
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 9966364a6deb..0f31bb4bc372 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -348,6 +348,17 @@ config MTK_THERMAL
>>           Enable this option if you want to have support for thermal management
>>           controller present in Mediatek SoCs
>>
>> +config AMLOGIC_THERMAL
> we typically use "MESON" in the Kconfig symbols:
> $ grep -c AMLOGIC .config
> 1
> $ grep -c MESON .config
> 33
>
> I also wonder if we should add G12 or G12A so we don't conflict with
> upcoming thermal sensors with a different design (assuming that this
> will be a thing).
> for example we already have three different USB2 PHY drivers
>
> [...]

i check with Neil and for new family it's better to use Amlogic instead of meson.

i don't add G12 because we already know it's same sensors for SM1 SoC family [0].

>> +/*
>> + * Calculate a temperature value from a temperature code.
>> + * The unit of the temperature is degree Celsius.
> is it really degree Celsius or millicelsius?
good point it's in millicelsius, i will fix all
>
>> + */
>> +static int code_to_temp(struct amlogic_thermal *pdata, int temp_code)
> PREFIX_thermal_code_to_millicelsius?
ok
> [...]
>> +static int amlogic_thermal_enable(struct amlogic_thermal *data)
>> +{
>> +       clk_prepare_enable(data->clk);
> no clock error handling?


i will fix

>
>> +       regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
>> +                          TSENSOR_CFG_REG1_ENABLE, TSENSOR_CFG_REG1_ENABLE);
>> +
>> +       return 0;
>> +}
>> +
>> +static int amlogic_thermal_disable(struct amlogic_thermal *data)
>> +{
>> +       regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
>> +                          TSENSOR_CFG_REG1_ENABLE, 0);
>> +       clk_disable(data->clk);
> either clk_disable_unprepare here or use only clk_enable in
> amlogic_thermal_enable and move prepare/unprepare somewhere else
i will align with enable and use clk_disable_unprepare
>
>> +
>> +       return 0;
>> +}
>> +
>> +static int amlogic_thermal_get_temp(void *data, int *temp)
>> +{
>> +       unsigned int tvalue;
>> +       struct amlogic_thermal *pdata = data;
>> +
>> +       if (!data)
>> +               return -EINVAL;
>> +
>> +       regmap_read(pdata->regmap, TSENSOR_STAT0, &tvalue);
>> +       *temp = code_to_temp(pdata,
>> +                            tvalue & TSENSOR_READ_TEMP_MASK);
> maybe simply move the implementation from code_to_temp here?

for the optional function it could be a problem if i move all in code_to_temp.

i prefer to have a function which are just do the conversion.

>
> [...]
>> +static const struct amlogic_thermal_data amlogic_thermal_g12_cpu_param = {
>> +       .u_efuse_off = 0x128,
>> +       .soc = &amlogic_thermal_g12,
> based on the variable name I expected this to be an enum of some sort.
> I would have expected it to be called calibration_parameters or
> similar to match the explanation in the driver description
> (no need to change it if you prefer it like this, I just want to make
> you aware of this)
ok to rename structure name
>
>> +       .regmap_config = &amlogic_thermal_regmap_config_g12,
> if regmap_config is always the same you may also pass it directly to
> devm_regmap_init_mmio

it's the same for now but we don't know if in the future  SoCs use same regmap.


>
>> +};
>> +
>> +static const struct amlogic_thermal_data amlogic_thermal_g12_ddr_param = {
>> +       .u_efuse_off = 0xF0,
> I believe we use lower-case hex everywhere else
>
> [...]
will fix
>> +static const struct of_device_id of_amlogic_thermal_match[] = {
>> +       {
>> +               .compatible = "amlogic,g12-ddr-thermal",
>> +               .data = &amlogic_thermal_g12_ddr_param,
>> +       },
>> +       {
>> +               .compatible = "amlogic,g12-cpu-thermal",
>> +               .data = &amlogic_thermal_g12_cpu_param,
>> +       },
>> +       { /* end */ }
> other drivers use "sentinel", but personally I have no preference here
>
> [...]
same for me i will do same as other drivers
>> +       pdata->tzd = devm_thermal_zone_of_sensor_register
>> +                               (&pdev->dev,
>> +                                0,
>> +                                pdata,
>> +                                &amlogic_thermal_ops);
> I believe the opening brace has to go onto the same line as the function name
>
> [...]
will fix
>> +       ret = amlogic_thermal_enable(pdata);
>> +       if (ret)
>> +               clk_unprepare(pdata->clk);
> clk_disable_unprepare, else you'll leave the clock prepared
will fix
>
>> +#ifdef CONFIG_PM_SLEEP
>> +static int amlogic_thermal_suspend(struct device *dev)
>> +{
>> +       struct amlogic_thermal *data = dev_get_drvdata(dev);
>> +
>> +       return amlogic_thermal_disable(data);
>> +}
>> +
>> +static int amlogic_thermal_resume(struct device *dev)
>> +{
>> +       struct amlogic_thermal *data = dev_get_drvdata(dev);
>> +
>> +       return amlogic_thermal_enable(data);
>> +}
>> +#endif
> instead of using an #ifdef here annotate the suspend/resume functions
> with __maybe_unused, see [0]
will fix
>
>
> Martin
>
>
> [0] https://lore.kernel.org/patchwork/patch/732981/

[0] https://lore.kernel.org/linux-arm-kernel/20190701104705.18271-1-narmstrong@baylibre.com/


thanks

Guillaume


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/6] thermal: amlogic: Add thermal driver to support G12 SoCs
  2019-08-05 12:48     ` guillaume La Roque
@ 2019-08-06 19:52       ` Martin Blumenstingl
  2019-08-08  2:59         ` Kevin Hilman
  0 siblings, 1 reply; 17+ messages in thread
From: Martin Blumenstingl @ 2019-08-06 19:52 UTC (permalink / raw)
  To: guillaume La Roque
  Cc: daniel.lezcano, khilman, devicetree, linux-amlogic, linux-kernel,
	linux-arm-kernel, linux-pm

Hi Guillaume,

On Mon, Aug 5, 2019 at 2:48 PM guillaume La Roque <glaroque@baylibre.com> wrote:
>
> Hi Martin,
>
> again thanks for your review.
you're welcome - thank you for working on the driver :-)

[...]
> > The IP block has more functionality, which may be added to this driver
> > in the future:
> > - reading up to 16 stored temperature samples
>
> it's not working, you can verify it if you check the regmap define in the driver. in fact temp is only write in one register, it's confirmed by amlogic.
I missed that - so please skip this part

[...]
> >> +config AMLOGIC_THERMAL
> > we typically use "MESON" in the Kconfig symbols:
> > $ grep -c AMLOGIC .config
> > 1
> > $ grep -c MESON .config
> > 33
> >
> > I also wonder if we should add G12 or G12A so we don't conflict with
> > upcoming thermal sensors with a different design (assuming that this
> > will be a thing).
> > for example we already have three different USB2 PHY drivers
> >
> > [...]
>
> i check with Neil and for new family it's better to use Amlogic instead of meson.
can you please share the considerations behind this decision?
if new drivers should use AMLOGIC_* Kconfig symbols instead of MESON_*
then we all should know about it

> i don't add G12 because we already know it's same sensors for SM1 SoC family [0].
my idea behind this was to avoid conflicts in the future
in case of the thermal driver we may be fine with using a generic name
assuming that Amlogic will not switch to a new IP block in the next
years
I'm not saying you have to change the name - I'm bringing this up so
you can decide for yourself based on examples from the past

here are a few examples:
- when Kevin upstreamed the MMC driver for GX he decided to use
MMC_MESON_GX for the Kconfig symbol name. it turns out that this is
smart because there are at least two other MMC controller IPs on the
32-bit SoCs. due to him including GX in the name the drivers are easy
to differentiate (MMC_MESON_MX_SDIO and MMC_MESON_MX_SDHC being the
other ones, while the latter is not upstream yet)
- when Carlo upstreamed the eFuse driver he decided to use MESON_EFUSE
for the Kconfig symbol name. I found out much later that the 32-bit
SoCs use a different IP (or at least direct register access instead of
going through Secure Monitor). the driver for the 32-bit SoCs now uses
MESON_MX_EFUSE. if you don't know which driver applies where then it's
easy to mix up MESON_EFUSE and MESON_MX_EFUSE
- when Jerome upstreamed the ALSA driver for AXG (which is also used
on G12A and G12B) he decided to use the SND_MESON_AXG_* prefix for the
Kconfig symbol names. in my opinion this was a good choice because GXM
and everything earlier (including the 32-bit SoCs) use a different
audio IP block. we won't have a Kconfig symbol name clash when a
driver for the "older" SoCs is upstreamed
- (there are more examples, Meson8b USB PHY driver, Meson8b DWMAC
glue, ... - just like there's many examples where the IP block is
mostly compatible with older generations: SAR ADC, RNG, SPI, ...)

I'm not sure what driver naming rules other mainline SoC teams use
to me it seems that the rule for Allwinner driver names is to use the
"code-name of the first SoC the IP block appeared in"

[...]
> >> +static int amlogic_thermal_get_temp(void *data, int *temp)
> >> +{
> >> +       unsigned int tvalue;
> >> +       struct amlogic_thermal *pdata = data;
> >> +
> >> +       if (!data)
> >> +               return -EINVAL;
> >> +
> >> +       regmap_read(pdata->regmap, TSENSOR_STAT0, &tvalue);
> >> +       *temp = code_to_temp(pdata,
> >> +                            tvalue & TSENSOR_READ_TEMP_MASK);
> > maybe simply move the implementation from code_to_temp here?
>
> for the optional function it could be a problem if i move all in code_to_temp.
>
> i prefer to have a function which are just do the conversion.
I didn't consider this before but you are right
if the other temperature registers (like IRQ thresholds) also use a
"temperature code" then it should be a dedicated function (so it'll be
easier to add more functionality to the driver)


Martin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/6] thermal: amlogic: Add thermal driver to support G12 SoCs
  2019-08-06 19:52       ` Martin Blumenstingl
@ 2019-08-08  2:59         ` Kevin Hilman
  2019-08-08 19:47           ` Martin Blumenstingl
  0 siblings, 1 reply; 17+ messages in thread
From: Kevin Hilman @ 2019-08-08  2:59 UTC (permalink / raw)
  To: Martin Blumenstingl, guillaume La Roque
  Cc: daniel.lezcano, devicetree, linux-amlogic, linux-kernel,
	linux-arm-kernel, linux-pm

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:

> Hi Guillaume,
>
> On Mon, Aug 5, 2019 at 2:48 PM guillaume La Roque <glaroque@baylibre.com> wrote:
>>
>> Hi Martin,
>>
>> again thanks for your review.
> you're welcome - thank you for working on the driver :-)
>
> [...]
>> > The IP block has more functionality, which may be added to this driver
>> > in the future:
>> > - reading up to 16 stored temperature samples
>>
>> it's not working, you can verify it if you check the regmap define in the driver. in fact temp is only write in one register, it's confirmed by amlogic.
> I missed that - so please skip this part
>
> [...]
>> >> +config AMLOGIC_THERMAL
>> > we typically use "MESON" in the Kconfig symbols:
>> > $ grep -c AMLOGIC .config
>> > 1
>> > $ grep -c MESON .config
>> > 33
>> >
>> > I also wonder if we should add G12 or G12A so we don't conflict with
>> > upcoming thermal sensors with a different design (assuming that this
>> > will be a thing).
>> > for example we already have three different USB2 PHY drivers
>> >
>> > [...]
>>
>> i check with Neil and for new family it's better to use Amlogic instead of meson.
> can you please share the considerations behind this decision?
> if new drivers should use AMLOGIC_* Kconfig symbols instead of MESON_*
> then we all should know about it
>
>> i don't add G12 because we already know it's same sensors for SM1 SoC family [0].
> my idea behind this was to avoid conflicts in the future
> in case of the thermal driver we may be fine with using a generic name
> assuming that Amlogic will not switch to a new IP block in the next
> years
> I'm not saying you have to change the name - I'm bringing this up so
> you can decide for yourself based on examples from the past
>
> here are a few examples:
> - when Kevin upstreamed the MMC driver for GX he decided to use
> MMC_MESON_GX for the Kconfig symbol name. it turns out that this is
> smart because there are at least two other MMC controller IPs on the
> 32-bit SoCs. due to him including GX in the name the drivers are easy
> to differentiate (MMC_MESON_MX_SDIO and MMC_MESON_MX_SDHC being the
> other ones, while the latter is not upstream yet)
> - when Carlo upstreamed the eFuse driver he decided to use MESON_EFUSE
> for the Kconfig symbol name. I found out much later that the 32-bit
> SoCs use a different IP (or at least direct register access instead of
> going through Secure Monitor). the driver for the 32-bit SoCs now uses
> MESON_MX_EFUSE. if you don't know which driver applies where then it's
> easy to mix up MESON_EFUSE and MESON_MX_EFUSE
> - when Jerome upstreamed the ALSA driver for AXG (which is also used
> on G12A and G12B) he decided to use the SND_MESON_AXG_* prefix for the
> Kconfig symbol names. in my opinion this was a good choice because GXM
> and everything earlier (including the 32-bit SoCs) use a different
> audio IP block. we won't have a Kconfig symbol name clash when a
> driver for the "older" SoCs is upstreamed
> - (there are more examples, Meson8b USB PHY driver, Meson8b DWMAC
> glue, ... - just like there's many examples where the IP block is
> mostly compatible with older generations: SAR ADC, RNG, SPI, ...)

While these are all good examples, you can see it can go both ways, so
there's really no way know up front what is the "right" way.  We only
know after the fact.  Unfortunately, we simply have no visibility into
future chips and where IP blocks may be shared or not (there are other
examples where vendors add a new version of an IP *and* keep the old
version. ;)

Even having worked inside a (different) SoC vendor and having some
knowledge about what IPs are shared, it's difficult to get this right.

> I'm not sure what driver naming rules other mainline SoC teams use
> to me it seems that the rule for Allwinner driver names is to use the
> "code-name of the first SoC the IP block appeared in"

That's a good rule of thumb (and one we generally follow) but I don't
feel it's important enough to enforce strictly either.

Kevin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/6] thermal: amlogic: Add thermal driver to support G12 SoCs
  2019-08-08  2:59         ` Kevin Hilman
@ 2019-08-08 19:47           ` Martin Blumenstingl
  0 siblings, 0 replies; 17+ messages in thread
From: Martin Blumenstingl @ 2019-08-08 19:47 UTC (permalink / raw)
  To: Kevin Hilman
  Cc: guillaume La Roque, daniel.lezcano, devicetree, linux-amlogic,
	linux-kernel, linux-arm-kernel, linux-pm

Hi Kevin,

On Thu, Aug 8, 2019 at 4:59 AM Kevin Hilman <khilman@baylibre.com> wrote:
>
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>
> > Hi Guillaume,
> >
> > On Mon, Aug 5, 2019 at 2:48 PM guillaume La Roque <glaroque@baylibre.com> wrote:
> >>
> >> Hi Martin,
> >>
> >> again thanks for your review.
> > you're welcome - thank you for working on the driver :-)
> >
> > [...]
> >> > The IP block has more functionality, which may be added to this driver
> >> > in the future:
> >> > - reading up to 16 stored temperature samples
> >>
> >> it's not working, you can verify it if you check the regmap define in the driver. in fact temp is only write in one register, it's confirmed by amlogic.
> > I missed that - so please skip this part
> >
> > [...]
> >> >> +config AMLOGIC_THERMAL
> >> > we typically use "MESON" in the Kconfig symbols:
> >> > $ grep -c AMLOGIC .config
> >> > 1
> >> > $ grep -c MESON .config
> >> > 33
> >> >
> >> > I also wonder if we should add G12 or G12A so we don't conflict with
> >> > upcoming thermal sensors with a different design (assuming that this
> >> > will be a thing).
> >> > for example we already have three different USB2 PHY drivers
> >> >
> >> > [...]
> >>
> >> i check with Neil and for new family it's better to use Amlogic instead of meson.
> > can you please share the considerations behind this decision?
> > if new drivers should use AMLOGIC_* Kconfig symbols instead of MESON_*
> > then we all should know about it
> >
> >> i don't add G12 because we already know it's same sensors for SM1 SoC family [0].
> > my idea behind this was to avoid conflicts in the future
> > in case of the thermal driver we may be fine with using a generic name
> > assuming that Amlogic will not switch to a new IP block in the next
> > years
> > I'm not saying you have to change the name - I'm bringing this up so
> > you can decide for yourself based on examples from the past
> >
> > here are a few examples:
> > - when Kevin upstreamed the MMC driver for GX he decided to use
> > MMC_MESON_GX for the Kconfig symbol name. it turns out that this is
> > smart because there are at least two other MMC controller IPs on the
> > 32-bit SoCs. due to him including GX in the name the drivers are easy
> > to differentiate (MMC_MESON_MX_SDIO and MMC_MESON_MX_SDHC being the
> > other ones, while the latter is not upstream yet)
> > - when Carlo upstreamed the eFuse driver he decided to use MESON_EFUSE
> > for the Kconfig symbol name. I found out much later that the 32-bit
> > SoCs use a different IP (or at least direct register access instead of
> > going through Secure Monitor). the driver for the 32-bit SoCs now uses
> > MESON_MX_EFUSE. if you don't know which driver applies where then it's
> > easy to mix up MESON_EFUSE and MESON_MX_EFUSE
> > - when Jerome upstreamed the ALSA driver for AXG (which is also used
> > on G12A and G12B) he decided to use the SND_MESON_AXG_* prefix for the
> > Kconfig symbol names. in my opinion this was a good choice because GXM
> > and everything earlier (including the 32-bit SoCs) use a different
> > audio IP block. we won't have a Kconfig symbol name clash when a
> > driver for the "older" SoCs is upstreamed
> > - (there are more examples, Meson8b USB PHY driver, Meson8b DWMAC
> > glue, ... - just like there's many examples where the IP block is
> > mostly compatible with older generations: SAR ADC, RNG, SPI, ...)
>
> While these are all good examples, you can see it can go both ways, so
> there's really no way know up front what is the "right" way.  We only
> know after the fact.  Unfortunately, we simply have no visibility into
> future chips and where IP blocks may be shared or not (there are other
> examples where vendors add a new version of an IP *and* keep the old
> version. ;)
>
> Even having worked inside a (different) SoC vendor and having some
> knowledge about what IPs are shared, it's difficult to get this right.
right. The fact that it'll be the IP block in SM1 will be backwards
compatible (or even the same) means that it has a longer life-span
than some of the USB PHY IP.
so I'm fine either way


Martin

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2019-08-08 19:47 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-31 15:35 [PATCH v2 0/6] Add support of New Amlogic temperature sensor for G12 SoCs Guillaume La Roque
2019-07-31 15:35 ` [PATCH v2 1/6] dt-bindings: thermal: Add DT bindings documentation for Amlogic Thermal Guillaume La Roque
2019-07-31 17:59   ` Rob Herring
2019-07-31 15:35 ` [PATCH v2 2/6] thermal: amlogic: Add thermal driver to support G12 SoCs Guillaume La Roque
2019-08-03 18:24   ` Martin Blumenstingl
2019-08-05 12:48     ` guillaume La Roque
2019-08-06 19:52       ` Martin Blumenstingl
2019-08-08  2:59         ` Kevin Hilman
2019-08-08 19:47           ` Martin Blumenstingl
2019-07-31 15:35 ` [PATCH v2 3/6] arm64: dts: amlogic: g12: add temperature sensor Guillaume La Roque
2019-08-03 17:52   ` Martin Blumenstingl
2019-08-05  9:41     ` guillaume La Roque
2019-07-31 15:35 ` [PATCH v2 4/6] arm64: dts: meson: sei510: Add minimal thermal zone Guillaume La Roque
2019-08-03 18:29   ` Martin Blumenstingl
2019-08-05 12:48     ` guillaume La Roque
2019-07-31 15:35 ` [PATCH v2 5/6] arm64: dts: amlogic: odroid-n2: add " Guillaume La Roque
2019-07-31 15:35 ` [PATCH v2 6/6] MAINTAINERS: add entry for Amlogic Thermal driver Guillaume La Roque

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).