[PATCH v7 0/4] thermal: mediatek: Add support for MT8365 SoC

[PATCH v7 0/4] thermal: mediatek: Add support for MT8365 SoC

[Amjad Ouled-Ameur]

This patchset adds thermal support for MT8365 SoC which contains three
thermal sensors.

Changes in v7:
- Fix devm_thermal_of_zone_register() error checks.
- Link to v6: https://lore.kernel.org/r/20221018-up-i350-thermal-bringup-
v6-0-c87b9f75550b@baylibre.com

Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
---
Amjad Ouled-Ameur (1):
      thermal: mediatek: add another get_temp ops for thermal sensors

Fabien Parent (2):
      dt-bindings: thermal: mediatek: add binding documentation for MT8365 SoC
      thermal: mediatek: add support for MT8365 SoC

Markus Schneider-Pargmann (1):
      thermal: mediatek: control buffer enablement tweaks

 .../bindings/thermal/mediatek-thermal.txt          |   1 +
 drivers/thermal/mtk_thermal.c                      | 192 +++++++++++++++++----
 2 files changed, 161 insertions(+), 32 deletions(-)
---
base-commit: 9d2bc364f67793cdd115f3ab92a18eaf85fee66f
change-id: 20221018-up-i350-thermal-bringup-ad386d37056f

Best regards,
-- 
Amjad Ouled-Ameur <aouledameur@baylibre.com>

[PATCH v7 0/4] thermal: mediatek: Add support for MT8365 SoC

[Amjad Ouled-Ameur]

This patchset adds thermal support for MT8365 SoC which contains three
thermal sensors.

Changes in v7:
- Fix devm_thermal_of_zone_register() error checks.
- Link to v6: https://lore.kernel.org/r/20221018-up-i350-thermal-bringup-
v6-0-c87b9f75550b@baylibre.com

Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
---
Amjad Ouled-Ameur (1):
      thermal: mediatek: add another get_temp ops for thermal sensors

Fabien Parent (2):
      dt-bindings: thermal: mediatek: add binding documentation for MT8365 SoC
      thermal: mediatek: add support for MT8365 SoC

Markus Schneider-Pargmann (1):
      thermal: mediatek: control buffer enablement tweaks

 .../bindings/thermal/mediatek-thermal.txt          |   1 +
 drivers/thermal/mtk_thermal.c                      | 192 +++++++++++++++++----
 2 files changed, 161 insertions(+), 32 deletions(-)
---
base-commit: 9d2bc364f67793cdd115f3ab92a18eaf85fee66f
change-id: 20221018-up-i350-thermal-bringup-ad386d37056f

Best regards,
-- 
Amjad Ouled-Ameur <aouledameur@baylibre.com>

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

[PATCH v7 1/4] dt-bindings: thermal: mediatek: add binding documentation for MT8365 SoC

[Amjad Ouled-Ameur]

From: Fabien Parent <fparent@baylibre.com>

Add the binding documentation for the thermal support on MT8365 SoC.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/thermal/mediatek-thermal.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/thermal/mediatek-thermal.txt b/Documentation/devicetree/bindings/thermal/mediatek-thermal.txt
index 5c7e7bdd029a..ba4ebffeade4 100644
--- a/Documentation/devicetree/bindings/thermal/mediatek-thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/mediatek-thermal.txt
@@ -14,6 +14,7 @@ Required properties:
   - "mediatek,mt2712-thermal" : For MT2712 family of SoCs
   - "mediatek,mt7622-thermal" : For MT7622 SoC
   - "mediatek,mt8183-thermal" : For MT8183 family of SoCs
+  - "mediatek,mt8365-thermal" : For MT8365 family of SoCs
   - "mediatek,mt8516-thermal", "mediatek,mt2701-thermal : For MT8516 family of SoCs
 - reg: Address range of the thermal controller
 - interrupts: IRQ for the thermal controller

-- 
b4 0.10.1

[PATCH v7 1/4] dt-bindings: thermal: mediatek: add binding documentation for MT8365 SoC

[Amjad Ouled-Ameur]

From: Fabien Parent <fparent@baylibre.com>

Add the binding documentation for the thermal support on MT8365 SoC.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/thermal/mediatek-thermal.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/thermal/mediatek-thermal.txt b/Documentation/devicetree/bindings/thermal/mediatek-thermal.txt
index 5c7e7bdd029a..ba4ebffeade4 100644
--- a/Documentation/devicetree/bindings/thermal/mediatek-thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/mediatek-thermal.txt
@@ -14,6 +14,7 @@ Required properties:
   - "mediatek,mt2712-thermal" : For MT2712 family of SoCs
   - "mediatek,mt7622-thermal" : For MT7622 SoC
   - "mediatek,mt8183-thermal" : For MT8183 family of SoCs
+  - "mediatek,mt8365-thermal" : For MT8365 family of SoCs
   - "mediatek,mt8516-thermal", "mediatek,mt2701-thermal : For MT8516 family of SoCs
 - reg: Address range of the thermal controller
 - interrupts: IRQ for the thermal controller

-- 
b4 0.10.1

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

[PATCH v7 2/4] thermal: mediatek: control buffer enablement tweaks

[Amjad Ouled-Ameur]

From: Markus Schneider-Pargmann <msp@baylibre.com>

Add logic in order to be able to turn on the control buffer on MT8365.
This change now allows to have control buffer support for MTK_THERMAL_V1,
and it allows to define the register offset, and mask used to enable it.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
Signed-off-by: Fabien Parent <fparent@baylibre.com>
Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/thermal/mtk_thermal.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
index 8440692e3890..d8ddceb75372 100644
--- a/drivers/thermal/mtk_thermal.c
+++ b/drivers/thermal/mtk_thermal.c
@@ -271,6 +271,9 @@ struct mtk_thermal_data {
 	bool need_switch_bank;
 	struct thermal_bank_cfg bank_data[MAX_NUM_ZONES];
 	enum mtk_thermal_version version;
+	u32 apmixed_buffer_ctl_reg;
+	u32 apmixed_buffer_ctl_mask;
+	u32 apmixed_buffer_ctl_set;
 };
 
 struct mtk_thermal {
@@ -514,6 +517,9 @@ static const struct mtk_thermal_data mt7622_thermal_data = {
 	.adcpnp = mt7622_adcpnp,
 	.sensor_mux_values = mt7622_mux_values,
 	.version = MTK_THERMAL_V2,
+	.apmixed_buffer_ctl_reg = APMIXED_SYS_TS_CON1,
+	.apmixed_buffer_ctl_mask = GENMASK(31, 6) | BIT(3),
+	.apmixed_buffer_ctl_set = BIT(0),
 };
 
 /*
@@ -963,14 +969,18 @@ static const struct of_device_id mtk_thermal_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, mtk_thermal_of_match);
 
-static void mtk_thermal_turn_on_buffer(void __iomem *apmixed_base)
+static void mtk_thermal_turn_on_buffer(struct mtk_thermal *mt,
+				       void __iomem *apmixed_base)
 {
-	int tmp;
+	u32 tmp;
+
+	if (!mt->conf->apmixed_buffer_ctl_reg)
+		return;
 
-	tmp = readl(apmixed_base + APMIXED_SYS_TS_CON1);
-	tmp &= ~(0x37);
-	tmp |= 0x1;
-	writel(tmp, apmixed_base + APMIXED_SYS_TS_CON1);
+	tmp = readl(apmixed_base + mt->conf->apmixed_buffer_ctl_reg);
+	tmp &= mt->conf->apmixed_buffer_ctl_mask;
+	tmp |= mt->conf->apmixed_buffer_ctl_set;
+	writel(tmp, apmixed_base + mt->conf->apmixed_buffer_ctl_reg);
 	udelay(200);
 }
 
@@ -1070,8 +1080,9 @@ static int mtk_thermal_probe(struct platform_device *pdev)
 		goto err_disable_clk_auxadc;
 	}
 
+	mtk_thermal_turn_on_buffer(mt, apmixed_base);
+
 	if (mt->conf->version == MTK_THERMAL_V2) {
-		mtk_thermal_turn_on_buffer(apmixed_base);
 		mtk_thermal_release_periodic_ts(mt, auxadc_base);
 	}
 

-- 
b4 0.10.1

[PATCH v7 2/4] thermal: mediatek: control buffer enablement tweaks

[Amjad Ouled-Ameur]

From: Markus Schneider-Pargmann <msp@baylibre.com>

Add logic in order to be able to turn on the control buffer on MT8365.
This change now allows to have control buffer support for MTK_THERMAL_V1,
and it allows to define the register offset, and mask used to enable it.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
Signed-off-by: Fabien Parent <fparent@baylibre.com>
Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/thermal/mtk_thermal.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
index 8440692e3890..d8ddceb75372 100644
--- a/drivers/thermal/mtk_thermal.c
+++ b/drivers/thermal/mtk_thermal.c
@@ -271,6 +271,9 @@ struct mtk_thermal_data {
 	bool need_switch_bank;
 	struct thermal_bank_cfg bank_data[MAX_NUM_ZONES];
 	enum mtk_thermal_version version;
+	u32 apmixed_buffer_ctl_reg;
+	u32 apmixed_buffer_ctl_mask;
+	u32 apmixed_buffer_ctl_set;
 };
 
 struct mtk_thermal {
@@ -514,6 +517,9 @@ static const struct mtk_thermal_data mt7622_thermal_data = {
 	.adcpnp = mt7622_adcpnp,
 	.sensor_mux_values = mt7622_mux_values,
 	.version = MTK_THERMAL_V2,
+	.apmixed_buffer_ctl_reg = APMIXED_SYS_TS_CON1,
+	.apmixed_buffer_ctl_mask = GENMASK(31, 6) | BIT(3),
+	.apmixed_buffer_ctl_set = BIT(0),
 };
 
 /*
@@ -963,14 +969,18 @@ static const struct of_device_id mtk_thermal_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, mtk_thermal_of_match);
 
-static void mtk_thermal_turn_on_buffer(void __iomem *apmixed_base)
+static void mtk_thermal_turn_on_buffer(struct mtk_thermal *mt,
+				       void __iomem *apmixed_base)
 {
-	int tmp;
+	u32 tmp;
+
+	if (!mt->conf->apmixed_buffer_ctl_reg)
+		return;
 
-	tmp = readl(apmixed_base + APMIXED_SYS_TS_CON1);
-	tmp &= ~(0x37);
-	tmp |= 0x1;
-	writel(tmp, apmixed_base + APMIXED_SYS_TS_CON1);
+	tmp = readl(apmixed_base + mt->conf->apmixed_buffer_ctl_reg);
+	tmp &= mt->conf->apmixed_buffer_ctl_mask;
+	tmp |= mt->conf->apmixed_buffer_ctl_set;
+	writel(tmp, apmixed_base + mt->conf->apmixed_buffer_ctl_reg);
 	udelay(200);
 }
 
@@ -1070,8 +1080,9 @@ static int mtk_thermal_probe(struct platform_device *pdev)
 		goto err_disable_clk_auxadc;
 	}
 
+	mtk_thermal_turn_on_buffer(mt, apmixed_base);
+
 	if (mt->conf->version == MTK_THERMAL_V2) {
-		mtk_thermal_turn_on_buffer(apmixed_base);
 		mtk_thermal_release_periodic_ts(mt, auxadc_base);
 	}
 

-- 
b4 0.10.1

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

[PATCH v7 3/4] thermal: mediatek: add support for MT8365 SoC

[Amjad Ouled-Ameur]

From: Fabien Parent <fparent@baylibre.com>

MT8365 is similar to the other SoCs supported by the driver. It has only
one bank and 3 actual sensors that can be multiplexed. There is another
one sensor that does not have usable data.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/thermal/mtk_thermal.c | 68 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
index d8ddceb75372..3a5df1440822 100644
--- a/drivers/thermal/mtk_thermal.c
+++ b/drivers/thermal/mtk_thermal.c
@@ -31,6 +31,7 @@
 #define AUXADC_CON2_V		0x010
 #define AUXADC_DATA(channel)	(0x14 + (channel) * 4)
 
+#define APMIXED_SYS_TS_CON0	0x600
 #define APMIXED_SYS_TS_CON1	0x604
 
 /* Thermal Controller Registers */
@@ -245,6 +246,17 @@ enum mtk_thermal_version {
 /* The calibration coefficient of sensor  */
 #define MT8183_CALIBRATION	153
 
+/* MT8365 */
+#define MT8365_TEMP_AUXADC_CHANNEL 11
+#define MT8365_CALIBRATION 164
+#define MT8365_NUM_CONTROLLER 1
+#define MT8365_NUM_BANKS 1
+#define MT8365_NUM_SENSORS 3
+#define MT8365_NUM_SENSORS_PER_ZONE 3
+#define MT8365_TS1 0
+#define MT8365_TS2 1
+#define MT8365_TS3 2
+
 struct mtk_thermal;
 
 struct thermal_bank_cfg {
@@ -389,6 +401,24 @@ static const int mt7622_mux_values[MT7622_NUM_SENSORS] = { 0, };
 static const int mt7622_vts_index[MT7622_NUM_SENSORS] = { VTS1 };
 static const int mt7622_tc_offset[MT7622_NUM_CONTROLLER] = { 0x0, };
 
+/* MT8365 thermal sensor data */
+static const int mt8365_bank_data[MT8365_NUM_SENSORS] = {
+	MT8365_TS1, MT8365_TS2, MT8365_TS3
+};
+
+static const int mt8365_msr[MT8365_NUM_SENSORS_PER_ZONE] = {
+	TEMP_MSR0, TEMP_MSR1, TEMP_MSR2
+};
+
+static const int mt8365_adcpnp[MT8365_NUM_SENSORS_PER_ZONE] = {
+	TEMP_ADCPNP0, TEMP_ADCPNP1, TEMP_ADCPNP2
+};
+
+static const int mt8365_mux_values[MT8365_NUM_SENSORS] = { 0, 1, 2 };
+static const int mt8365_tc_offset[MT8365_NUM_CONTROLLER] = { 0 };
+
+static const int mt8365_vts_index[MT8365_NUM_SENSORS] = { VTS1, VTS2, VTS3 };
+
 /*
  * The MT8173 thermal controller has four banks. Each bank can read up to
  * four temperature sensors simultaneously. The MT8173 has a total of 5
@@ -463,6 +493,40 @@ static const struct mtk_thermal_data mt2701_thermal_data = {
 	.version = MTK_THERMAL_V1,
 };
 
+/*
+ * The MT8365 thermal controller has one bank, which can read up to
+ * four temperature sensors simultaneously. The MT8365 has a total of 3
+ * temperature sensors.
+ *
+ * The thermal core only gets the maximum temperature of this one bank,
+ * so the bank concept wouldn't be necessary here. However, the SVS (Smart
+ * Voltage Scaling) unit makes its decisions based on the same bank
+ * data.
+ */
+static const struct mtk_thermal_data mt8365_thermal_data = {
+	.auxadc_channel = MT8365_TEMP_AUXADC_CHANNEL,
+	.num_banks = MT8365_NUM_BANKS,
+	.num_sensors = MT8365_NUM_SENSORS,
+	.vts_index = mt8365_vts_index,
+	.cali_val = MT8365_CALIBRATION,
+	.num_controller = MT8365_NUM_CONTROLLER,
+	.controller_offset = mt8365_tc_offset,
+	.need_switch_bank = false,
+	.bank_data = {
+		{
+			.num_sensors = MT8365_NUM_SENSORS,
+			.sensors = mt8365_bank_data
+		},
+	},
+	.msr = mt8365_msr,
+	.adcpnp = mt8365_adcpnp,
+	.sensor_mux_values = mt8365_mux_values,
+	.version = MTK_THERMAL_V1,
+	.apmixed_buffer_ctl_reg = APMIXED_SYS_TS_CON0,
+	.apmixed_buffer_ctl_mask = (u32) ~GENMASK(29, 28),
+	.apmixed_buffer_ctl_set = 0,
+};
+
 /*
  * The MT2712 thermal controller has one bank, which can read up to
  * four temperature sensors simultaneously. The MT2712 has a total of 4
@@ -964,6 +1028,10 @@ static const struct of_device_id mtk_thermal_of_match[] = {
 	{
 		.compatible = "mediatek,mt8183-thermal",
 		.data = (void *)&mt8183_thermal_data,
+	},
+	{
+		.compatible = "mediatek,mt8365-thermal",
+		.data = (void *)&mt8365_thermal_data,
 	}, {
 	},
 };

-- 
b4 0.10.1

[PATCH v7 3/4] thermal: mediatek: add support for MT8365 SoC

[Amjad Ouled-Ameur]

From: Fabien Parent <fparent@baylibre.com>

MT8365 is similar to the other SoCs supported by the driver. It has only
one bank and 3 actual sensors that can be multiplexed. There is another
one sensor that does not have usable data.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/thermal/mtk_thermal.c | 68 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
index d8ddceb75372..3a5df1440822 100644
--- a/drivers/thermal/mtk_thermal.c
+++ b/drivers/thermal/mtk_thermal.c
@@ -31,6 +31,7 @@
 #define AUXADC_CON2_V		0x010
 #define AUXADC_DATA(channel)	(0x14 + (channel) * 4)
 
+#define APMIXED_SYS_TS_CON0	0x600
 #define APMIXED_SYS_TS_CON1	0x604
 
 /* Thermal Controller Registers */
@@ -245,6 +246,17 @@ enum mtk_thermal_version {
 /* The calibration coefficient of sensor  */
 #define MT8183_CALIBRATION	153
 
+/* MT8365 */
+#define MT8365_TEMP_AUXADC_CHANNEL 11
+#define MT8365_CALIBRATION 164
+#define MT8365_NUM_CONTROLLER 1
+#define MT8365_NUM_BANKS 1
+#define MT8365_NUM_SENSORS 3
+#define MT8365_NUM_SENSORS_PER_ZONE 3
+#define MT8365_TS1 0
+#define MT8365_TS2 1
+#define MT8365_TS3 2
+
 struct mtk_thermal;
 
 struct thermal_bank_cfg {
@@ -389,6 +401,24 @@ static const int mt7622_mux_values[MT7622_NUM_SENSORS] = { 0, };
 static const int mt7622_vts_index[MT7622_NUM_SENSORS] = { VTS1 };
 static const int mt7622_tc_offset[MT7622_NUM_CONTROLLER] = { 0x0, };
 
+/* MT8365 thermal sensor data */
+static const int mt8365_bank_data[MT8365_NUM_SENSORS] = {
+	MT8365_TS1, MT8365_TS2, MT8365_TS3
+};
+
+static const int mt8365_msr[MT8365_NUM_SENSORS_PER_ZONE] = {
+	TEMP_MSR0, TEMP_MSR1, TEMP_MSR2
+};
+
+static const int mt8365_adcpnp[MT8365_NUM_SENSORS_PER_ZONE] = {
+	TEMP_ADCPNP0, TEMP_ADCPNP1, TEMP_ADCPNP2
+};
+
+static const int mt8365_mux_values[MT8365_NUM_SENSORS] = { 0, 1, 2 };
+static const int mt8365_tc_offset[MT8365_NUM_CONTROLLER] = { 0 };
+
+static const int mt8365_vts_index[MT8365_NUM_SENSORS] = { VTS1, VTS2, VTS3 };
+
 /*
  * The MT8173 thermal controller has four banks. Each bank can read up to
  * four temperature sensors simultaneously. The MT8173 has a total of 5
@@ -463,6 +493,40 @@ static const struct mtk_thermal_data mt2701_thermal_data = {
 	.version = MTK_THERMAL_V1,
 };
 
+/*
+ * The MT8365 thermal controller has one bank, which can read up to
+ * four temperature sensors simultaneously. The MT8365 has a total of 3
+ * temperature sensors.
+ *
+ * The thermal core only gets the maximum temperature of this one bank,
+ * so the bank concept wouldn't be necessary here. However, the SVS (Smart
+ * Voltage Scaling) unit makes its decisions based on the same bank
+ * data.
+ */
+static const struct mtk_thermal_data mt8365_thermal_data = {
+	.auxadc_channel = MT8365_TEMP_AUXADC_CHANNEL,
+	.num_banks = MT8365_NUM_BANKS,
+	.num_sensors = MT8365_NUM_SENSORS,
+	.vts_index = mt8365_vts_index,
+	.cali_val = MT8365_CALIBRATION,
+	.num_controller = MT8365_NUM_CONTROLLER,
+	.controller_offset = mt8365_tc_offset,
+	.need_switch_bank = false,
+	.bank_data = {
+		{
+			.num_sensors = MT8365_NUM_SENSORS,
+			.sensors = mt8365_bank_data
+		},
+	},
+	.msr = mt8365_msr,
+	.adcpnp = mt8365_adcpnp,
+	.sensor_mux_values = mt8365_mux_values,
+	.version = MTK_THERMAL_V1,
+	.apmixed_buffer_ctl_reg = APMIXED_SYS_TS_CON0,
+	.apmixed_buffer_ctl_mask = (u32) ~GENMASK(29, 28),
+	.apmixed_buffer_ctl_set = 0,
+};
+
 /*
  * The MT2712 thermal controller has one bank, which can read up to
  * four temperature sensors simultaneously. The MT2712 has a total of 4
@@ -964,6 +1028,10 @@ static const struct of_device_id mtk_thermal_of_match[] = {
 	{
 		.compatible = "mediatek,mt8183-thermal",
 		.data = (void *)&mt8183_thermal_data,
+	},
+	{
+		.compatible = "mediatek,mt8365-thermal",
+		.data = (void *)&mt8365_thermal_data,
 	}, {
 	},
 };

-- 
b4 0.10.1

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

[PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

Provide thermal zone to read thermal sensor in the SoC. We can read all the
thermal sensors value in the SoC by the node /sys/class/thermal/

In mtk_thermal_bank_temperature, return -EAGAIN instead of -EACCESS
on the first read of sensor that often are bogus values.
This can avoid following warning on boot:

  thermal thermal_zone6: failed to read out thermal zone (-13)

Signed-off-by: Michael Kao <michael.kao@mediatek.com>
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/thermal/mtk_thermal.c | 99 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 74 insertions(+), 25 deletions(-)

diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
index 3a5df1440822..b1f4d19edd4f 100644
--- a/drivers/thermal/mtk_thermal.c
+++ b/drivers/thermal/mtk_thermal.c
@@ -259,6 +259,11 @@ enum mtk_thermal_version {
 
 struct mtk_thermal;
 
+struct mtk_thermal_zone {
+	struct mtk_thermal *mt;
+	int id;
+};
+
 struct thermal_bank_cfg {
 	unsigned int num_sensors;
 	const int *sensors;
@@ -307,6 +312,8 @@ struct mtk_thermal {
 
 	const struct mtk_thermal_data *conf;
 	struct mtk_thermal_bank banks[MAX_NUM_ZONES];
+
+	int (*raw_to_mcelsius)(struct mtk_thermal *mt, int sensno, s32 raw);
 };
 
 /* MT8183 thermal sensor data */
@@ -709,6 +716,29 @@ static void mtk_thermal_put_bank(struct mtk_thermal_bank *bank)
 		mutex_unlock(&mt->lock);
 }
 
+static int _get_sensor_temp(struct mtk_thermal *mt, int id)
+{
+	u32 raw;
+	int temp;
+
+	const struct mtk_thermal_data *conf = mt->conf;
+
+	raw = readl(mt->thermal_base + conf->msr[id]);
+
+	temp = mt->raw_to_mcelsius(mt, id, raw);
+
+	/*
+	 * The first read of a sensor often contains very high bogus
+	 * temperature value. Filter these out so that the system does
+	 * not immediately shut down.
+	 */
+
+	if (temp > 200000)
+		return -EAGAIN;
+	else
+		return temp;
+}
+
 /**
  * mtk_thermal_bank_temperature - get the temperature of a bank
  * @bank:	The bank
@@ -721,26 +751,9 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
 	struct mtk_thermal *mt = bank->mt;
 	const struct mtk_thermal_data *conf = mt->conf;
 	int i, temp = INT_MIN, max = INT_MIN;
-	u32 raw;
 
 	for (i = 0; i < conf->bank_data[bank->id].num_sensors; i++) {
-		raw = readl(mt->thermal_base + conf->msr[i]);
-
-		if (mt->conf->version == MTK_THERMAL_V1) {
-			temp = raw_to_mcelsius_v1(
-				mt, conf->bank_data[bank->id].sensors[i], raw);
-		} else {
-			temp = raw_to_mcelsius_v2(
-				mt, conf->bank_data[bank->id].sensors[i], raw);
-		}
-
-		/*
-		 * The first read of a sensor often contains very high bogus
-		 * temperature value. Filter these out so that the system does
-		 * not immediately shut down.
-		 */
-		if (temp > 200000)
-			temp = 0;
+		temp = _get_sensor_temp(mt, i);
 
 		if (temp > max)
 			max = temp;
@@ -749,9 +762,10 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
 	return max;
 }
 
-static int mtk_read_temp(struct thermal_zone_device *tz, int *temperature)
+static int mtk_read_temp(struct thermal_zone_device *tzdev, int *temperature)
 {
-	struct mtk_thermal *mt = tz->devdata;
+	struct mtk_thermal_zone *tz = tzdev->devdata;
+	struct mtk_thermal *mt = tz->mt;
 	int i;
 	int tempmax = INT_MIN;
 
@@ -770,10 +784,28 @@ static int mtk_read_temp(struct thermal_zone_device *tz, int *temperature)
 	return 0;
 }
 
+static int mtk_read_sensor_temp(struct thermal_zone_device *tzdev, int *temperature)
+{
+	struct mtk_thermal_zone *tz = tzdev->devdata;
+	struct mtk_thermal *mt = tz->mt;
+	int id = tz->id - 1;
+
+	if (id < 0)
+		return -EACCES;
+
+	*temperature = _get_sensor_temp(mt, id);
+
+	return 0;
+}
+
 static const struct thermal_zone_device_ops mtk_thermal_ops = {
 	.get_temp = mtk_read_temp,
 };
 
+static const struct thermal_zone_device_ops mtk_thermal_sensor_ops = {
+	.get_temp = mtk_read_sensor_temp,
+};
+
 static void mtk_thermal_init_bank(struct mtk_thermal *mt, int num,
 				  u32 apmixed_phys_base, u32 auxadc_phys_base,
 				  int ctrl_id)
@@ -1072,6 +1104,7 @@ static int mtk_thermal_probe(struct platform_device *pdev)
 	u64 auxadc_phys_base, apmixed_phys_base;
 	struct thermal_zone_device *tzdev;
 	void __iomem *apmixed_base, *auxadc_base;
+	struct mtk_thermal_zone *tz;
 
 	mt = devm_kzalloc(&pdev->dev, sizeof(*mt), GFP_KERNEL);
 	if (!mt)
@@ -1150,6 +1183,9 @@ static int mtk_thermal_probe(struct platform_device *pdev)
 
 	mtk_thermal_turn_on_buffer(mt, apmixed_base);
 
+	mt->raw_to_mcelsius = (mt->conf->version == MTK_THERMAL_V1) ?
+				raw_to_mcelsius_v1 : raw_to_mcelsius_v2;
+
 	if (mt->conf->version == MTK_THERMAL_V2) {
 		mtk_thermal_release_periodic_ts(mt, auxadc_base);
 	}
@@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, mt);
 
-	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
-					      &mtk_thermal_ops);
-	if (IS_ERR(tzdev)) {
-		ret = PTR_ERR(tzdev);
-		goto err_disable_clk_peri_therm;
+	for (i = 0; i < mt->conf->num_sensors + 1; i++) {
+		tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
+		if (!tz)
+			return -ENOMEM;
+
+		tz->mt = mt;
+		tz->id = i;
+
+		tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
+							     &mtk_thermal_ops :
+							     &mtk_thermal_sensor_ops);
+
+		if (IS_ERR(tzdev)) {
+			ret = PTR_ERR(tzdev);
+			if (ret == -ENODEV)
+				continue;
+			goto err_disable_clk_peri_therm;
+		}
 	}
 
 	ret = devm_thermal_add_hwmon_sysfs(tzdev);

-- 
b4 0.10.1

[PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

Provide thermal zone to read thermal sensor in the SoC. We can read all the
thermal sensors value in the SoC by the node /sys/class/thermal/

In mtk_thermal_bank_temperature, return -EAGAIN instead of -EACCESS
on the first read of sensor that often are bogus values.
This can avoid following warning on boot:

  thermal thermal_zone6: failed to read out thermal zone (-13)

Signed-off-by: Michael Kao <michael.kao@mediatek.com>
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/thermal/mtk_thermal.c | 99 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 74 insertions(+), 25 deletions(-)

diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
index 3a5df1440822..b1f4d19edd4f 100644
--- a/drivers/thermal/mtk_thermal.c
+++ b/drivers/thermal/mtk_thermal.c
@@ -259,6 +259,11 @@ enum mtk_thermal_version {
 
 struct mtk_thermal;
 
+struct mtk_thermal_zone {
+	struct mtk_thermal *mt;
+	int id;
+};
+
 struct thermal_bank_cfg {
 	unsigned int num_sensors;
 	const int *sensors;
@@ -307,6 +312,8 @@ struct mtk_thermal {
 
 	const struct mtk_thermal_data *conf;
 	struct mtk_thermal_bank banks[MAX_NUM_ZONES];
+
+	int (*raw_to_mcelsius)(struct mtk_thermal *mt, int sensno, s32 raw);
 };
 
 /* MT8183 thermal sensor data */
@@ -709,6 +716,29 @@ static void mtk_thermal_put_bank(struct mtk_thermal_bank *bank)
 		mutex_unlock(&mt->lock);
 }
 
+static int _get_sensor_temp(struct mtk_thermal *mt, int id)
+{
+	u32 raw;
+	int temp;
+
+	const struct mtk_thermal_data *conf = mt->conf;
+
+	raw = readl(mt->thermal_base + conf->msr[id]);
+
+	temp = mt->raw_to_mcelsius(mt, id, raw);
+
+	/*
+	 * The first read of a sensor often contains very high bogus
+	 * temperature value. Filter these out so that the system does
+	 * not immediately shut down.
+	 */
+
+	if (temp > 200000)
+		return -EAGAIN;
+	else
+		return temp;
+}
+
 /**
  * mtk_thermal_bank_temperature - get the temperature of a bank
  * @bank:	The bank
@@ -721,26 +751,9 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
 	struct mtk_thermal *mt = bank->mt;
 	const struct mtk_thermal_data *conf = mt->conf;
 	int i, temp = INT_MIN, max = INT_MIN;
-	u32 raw;
 
 	for (i = 0; i < conf->bank_data[bank->id].num_sensors; i++) {
-		raw = readl(mt->thermal_base + conf->msr[i]);
-
-		if (mt->conf->version == MTK_THERMAL_V1) {
-			temp = raw_to_mcelsius_v1(
-				mt, conf->bank_data[bank->id].sensors[i], raw);
-		} else {
-			temp = raw_to_mcelsius_v2(
-				mt, conf->bank_data[bank->id].sensors[i], raw);
-		}
-
-		/*
-		 * The first read of a sensor often contains very high bogus
-		 * temperature value. Filter these out so that the system does
-		 * not immediately shut down.
-		 */
-		if (temp > 200000)
-			temp = 0;
+		temp = _get_sensor_temp(mt, i);
 
 		if (temp > max)
 			max = temp;
@@ -749,9 +762,10 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
 	return max;
 }
 
-static int mtk_read_temp(struct thermal_zone_device *tz, int *temperature)
+static int mtk_read_temp(struct thermal_zone_device *tzdev, int *temperature)
 {
-	struct mtk_thermal *mt = tz->devdata;
+	struct mtk_thermal_zone *tz = tzdev->devdata;
+	struct mtk_thermal *mt = tz->mt;
 	int i;
 	int tempmax = INT_MIN;
 
@@ -770,10 +784,28 @@ static int mtk_read_temp(struct thermal_zone_device *tz, int *temperature)
 	return 0;
 }
 
+static int mtk_read_sensor_temp(struct thermal_zone_device *tzdev, int *temperature)
+{
+	struct mtk_thermal_zone *tz = tzdev->devdata;
+	struct mtk_thermal *mt = tz->mt;
+	int id = tz->id - 1;
+
+	if (id < 0)
+		return -EACCES;
+
+	*temperature = _get_sensor_temp(mt, id);
+
+	return 0;
+}
+
 static const struct thermal_zone_device_ops mtk_thermal_ops = {
 	.get_temp = mtk_read_temp,
 };
 
+static const struct thermal_zone_device_ops mtk_thermal_sensor_ops = {
+	.get_temp = mtk_read_sensor_temp,
+};
+
 static void mtk_thermal_init_bank(struct mtk_thermal *mt, int num,
 				  u32 apmixed_phys_base, u32 auxadc_phys_base,
 				  int ctrl_id)
@@ -1072,6 +1104,7 @@ static int mtk_thermal_probe(struct platform_device *pdev)
 	u64 auxadc_phys_base, apmixed_phys_base;
 	struct thermal_zone_device *tzdev;
 	void __iomem *apmixed_base, *auxadc_base;
+	struct mtk_thermal_zone *tz;
 
 	mt = devm_kzalloc(&pdev->dev, sizeof(*mt), GFP_KERNEL);
 	if (!mt)
@@ -1150,6 +1183,9 @@ static int mtk_thermal_probe(struct platform_device *pdev)
 
 	mtk_thermal_turn_on_buffer(mt, apmixed_base);
 
+	mt->raw_to_mcelsius = (mt->conf->version == MTK_THERMAL_V1) ?
+				raw_to_mcelsius_v1 : raw_to_mcelsius_v2;
+
 	if (mt->conf->version == MTK_THERMAL_V2) {
 		mtk_thermal_release_periodic_ts(mt, auxadc_base);
 	}
@@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, mt);
 
-	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
-					      &mtk_thermal_ops);
-	if (IS_ERR(tzdev)) {
-		ret = PTR_ERR(tzdev);
-		goto err_disable_clk_peri_therm;
+	for (i = 0; i < mt->conf->num_sensors + 1; i++) {
+		tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
+		if (!tz)
+			return -ENOMEM;
+
+		tz->mt = mt;
+		tz->id = i;
+
+		tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
+							     &mtk_thermal_ops :
+							     &mtk_thermal_sensor_ops);
+
+		if (IS_ERR(tzdev)) {
+			ret = PTR_ERR(tzdev);
+			if (ret == -ENODEV)
+				continue;
+			goto err_disable_clk_peri_therm;
+		}
 	}
 
 	ret = devm_thermal_add_hwmon_sysfs(tzdev);

-- 
b4 0.10.1

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

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Daniel Lezcano]

On 18/11/2022 12:04, Amjad Ouled-Ameur wrote:
> Provide thermal zone to read thermal sensor in the SoC. We can read all the
> thermal sensors value in the SoC by the node /sys/class/thermal/
> 
> In mtk_thermal_bank_temperature, return -EAGAIN instead of -EACCESS
> on the first read of sensor that often are bogus values.
> This can avoid following warning on boot:
> 
>    thermal thermal_zone6: failed to read out thermal zone (-13)
> 
> Signed-off-by: Michael Kao <michael.kao@mediatek.com>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/thermal/mtk_thermal.c | 99 ++++++++++++++++++++++++++++++++-----------
>   1 file changed, 74 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
> index 3a5df1440822..b1f4d19edd4f 100644
> --- a/drivers/thermal/mtk_thermal.c
> +++ b/drivers/thermal/mtk_thermal.c
> @@ -259,6 +259,11 @@ enum mtk_thermal_version {
>   
>   struct mtk_thermal;
>   
> +struct mtk_thermal_zone {
> +	struct mtk_thermal *mt;
> +	int id;
> +};
> +
>   struct thermal_bank_cfg {
>   	unsigned int num_sensors;
>   	const int *sensors;
> @@ -307,6 +312,8 @@ struct mtk_thermal {
>   
>   	const struct mtk_thermal_data *conf;
>   	struct mtk_thermal_bank banks[MAX_NUM_ZONES];
> +
> +	int (*raw_to_mcelsius)(struct mtk_thermal *mt, int sensno, s32 raw);
>   };
>   
>   /* MT8183 thermal sensor data */
> @@ -709,6 +716,29 @@ static void mtk_thermal_put_bank(struct mtk_thermal_bank *bank)
>   		mutex_unlock(&mt->lock);
>   }
>   
> +static int _get_sensor_temp(struct mtk_thermal *mt, int id)
> +{
> +	u32 raw;
> +	int temp;
> +
> +	const struct mtk_thermal_data *conf = mt->conf;
> +
> +	raw = readl(mt->thermal_base + conf->msr[id]);
> +
> +	temp = mt->raw_to_mcelsius(mt, id, raw);
> +
> +	/*
> +	 * The first read of a sensor often contains very high bogus
> +	 * temperature value. Filter these out so that the system does
> +	 * not immediately shut down.
> +	 */
> +
> +	if (temp > 200000)
> +		return -EAGAIN;
> +	else
> +		return temp;
> +}
> +
>   /**
>    * mtk_thermal_bank_temperature - get the temperature of a bank
>    * @bank:	The bank
> @@ -721,26 +751,9 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
>   	struct mtk_thermal *mt = bank->mt;
>   	const struct mtk_thermal_data *conf = mt->conf;
>   	int i, temp = INT_MIN, max = INT_MIN;
> -	u32 raw;
>   
>   	for (i = 0; i < conf->bank_data[bank->id].num_sensors; i++) {
> -		raw = readl(mt->thermal_base + conf->msr[i]);
> -
> -		if (mt->conf->version == MTK_THERMAL_V1) {
> -			temp = raw_to_mcelsius_v1(
> -				mt, conf->bank_data[bank->id].sensors[i], raw);
> -		} else {
> -			temp = raw_to_mcelsius_v2(
> -				mt, conf->bank_data[bank->id].sensors[i], raw);
> -		}
> -
> -		/*
> -		 * The first read of a sensor often contains very high bogus
> -		 * temperature value. Filter these out so that the system does
> -		 * not immediately shut down.
> -		 */
> -		if (temp > 200000)
> -			temp = 0;
> +		temp = _get_sensor_temp(mt, i);
>   
>   		if (temp > max)
>   			max = temp;
> @@ -749,9 +762,10 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
>   	return max;
>   }
>   
> -static int mtk_read_temp(struct thermal_zone_device *tz, int *temperature)
> +static int mtk_read_temp(struct thermal_zone_device *tzdev, int *temperature)
>   {
> -	struct mtk_thermal *mt = tz->devdata;
> +	struct mtk_thermal_zone *tz = tzdev->devdata;
> +	struct mtk_thermal *mt = tz->mt;
>   	int i;
>   	int tempmax = INT_MIN;
>   
> @@ -770,10 +784,28 @@ static int mtk_read_temp(struct thermal_zone_device *tz, int *temperature)
>   	return 0;
>   }
>   
> +static int mtk_read_sensor_temp(struct thermal_zone_device *tzdev, int *temperature)
> +{
> +	struct mtk_thermal_zone *tz = tzdev->devdata;
> +	struct mtk_thermal *mt = tz->mt;
> +	int id = tz->id - 1;
> +
> +	if (id < 0)
> +		return -EACCES;
> +
> +	*temperature = _get_sensor_temp(mt, id);
> +
> +	return 0;
> +}
> +
>   static const struct thermal_zone_device_ops mtk_thermal_ops = {
>   	.get_temp = mtk_read_temp,
>   };
>   
> +static const struct thermal_zone_device_ops mtk_thermal_sensor_ops = {
> +	.get_temp = mtk_read_sensor_temp,
> +};
> +
>   static void mtk_thermal_init_bank(struct mtk_thermal *mt, int num,
>   				  u32 apmixed_phys_base, u32 auxadc_phys_base,
>   				  int ctrl_id)
> @@ -1072,6 +1104,7 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>   	u64 auxadc_phys_base, apmixed_phys_base;
>   	struct thermal_zone_device *tzdev;
>   	void __iomem *apmixed_base, *auxadc_base;
> +	struct mtk_thermal_zone *tz;
>   
>   	mt = devm_kzalloc(&pdev->dev, sizeof(*mt), GFP_KERNEL);
>   	if (!mt)
> @@ -1150,6 +1183,9 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>   
>   	mtk_thermal_turn_on_buffer(mt, apmixed_base);
>   
> +	mt->raw_to_mcelsius = (mt->conf->version == MTK_THERMAL_V1) ?
> +				raw_to_mcelsius_v1 : raw_to_mcelsius_v2;
> +
>   	if (mt->conf->version == MTK_THERMAL_V2) {
>   		mtk_thermal_release_periodic_ts(mt, auxadc_base);
>   	}
> @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, mt);
>   
> -	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
> -					      &mtk_thermal_ops);
> -	if (IS_ERR(tzdev)) {
> -		ret = PTR_ERR(tzdev);
> -		goto err_disable_clk_peri_therm;
> +	for (i = 0; i < mt->conf->num_sensors + 1; i++) {
> +		tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
> +		if (!tz)
> +			return -ENOMEM;
> +
> +		tz->mt = mt;
> +		tz->id = i;
> +
> +		tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
> +							     &mtk_thermal_ops :
> +							     &mtk_thermal_sensor_ops);

Here you use again the aggregation

> +
> +		if (IS_ERR(tzdev)) {
> +			ret = PTR_ERR(tzdev);
> +			if (ret == -ENODEV)
> +				continue;
> +			goto err_disable_clk_peri_therm;
> +		}
>   	}
>   
>   	ret = devm_thermal_add_hwmon_sysfs(tzdev);
> 

-- 
<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 v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Daniel Lezcano]

On 18/11/2022 12:04, Amjad Ouled-Ameur wrote:
> Provide thermal zone to read thermal sensor in the SoC. We can read all the
> thermal sensors value in the SoC by the node /sys/class/thermal/
> 
> In mtk_thermal_bank_temperature, return -EAGAIN instead of -EACCESS
> on the first read of sensor that often are bogus values.
> This can avoid following warning on boot:
> 
>    thermal thermal_zone6: failed to read out thermal zone (-13)
> 
> Signed-off-by: Michael Kao <michael.kao@mediatek.com>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/thermal/mtk_thermal.c | 99 ++++++++++++++++++++++++++++++++-----------
>   1 file changed, 74 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
> index 3a5df1440822..b1f4d19edd4f 100644
> --- a/drivers/thermal/mtk_thermal.c
> +++ b/drivers/thermal/mtk_thermal.c
> @@ -259,6 +259,11 @@ enum mtk_thermal_version {
>   
>   struct mtk_thermal;
>   
> +struct mtk_thermal_zone {
> +	struct mtk_thermal *mt;
> +	int id;
> +};
> +
>   struct thermal_bank_cfg {
>   	unsigned int num_sensors;
>   	const int *sensors;
> @@ -307,6 +312,8 @@ struct mtk_thermal {
>   
>   	const struct mtk_thermal_data *conf;
>   	struct mtk_thermal_bank banks[MAX_NUM_ZONES];
> +
> +	int (*raw_to_mcelsius)(struct mtk_thermal *mt, int sensno, s32 raw);
>   };
>   
>   /* MT8183 thermal sensor data */
> @@ -709,6 +716,29 @@ static void mtk_thermal_put_bank(struct mtk_thermal_bank *bank)
>   		mutex_unlock(&mt->lock);
>   }
>   
> +static int _get_sensor_temp(struct mtk_thermal *mt, int id)
> +{
> +	u32 raw;
> +	int temp;
> +
> +	const struct mtk_thermal_data *conf = mt->conf;
> +
> +	raw = readl(mt->thermal_base + conf->msr[id]);
> +
> +	temp = mt->raw_to_mcelsius(mt, id, raw);
> +
> +	/*
> +	 * The first read of a sensor often contains very high bogus
> +	 * temperature value. Filter these out so that the system does
> +	 * not immediately shut down.
> +	 */
> +
> +	if (temp > 200000)
> +		return -EAGAIN;
> +	else
> +		return temp;
> +}
> +
>   /**
>    * mtk_thermal_bank_temperature - get the temperature of a bank
>    * @bank:	The bank
> @@ -721,26 +751,9 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
>   	struct mtk_thermal *mt = bank->mt;
>   	const struct mtk_thermal_data *conf = mt->conf;
>   	int i, temp = INT_MIN, max = INT_MIN;
> -	u32 raw;
>   
>   	for (i = 0; i < conf->bank_data[bank->id].num_sensors; i++) {
> -		raw = readl(mt->thermal_base + conf->msr[i]);
> -
> -		if (mt->conf->version == MTK_THERMAL_V1) {
> -			temp = raw_to_mcelsius_v1(
> -				mt, conf->bank_data[bank->id].sensors[i], raw);
> -		} else {
> -			temp = raw_to_mcelsius_v2(
> -				mt, conf->bank_data[bank->id].sensors[i], raw);
> -		}
> -
> -		/*
> -		 * The first read of a sensor often contains very high bogus
> -		 * temperature value. Filter these out so that the system does
> -		 * not immediately shut down.
> -		 */
> -		if (temp > 200000)
> -			temp = 0;
> +		temp = _get_sensor_temp(mt, i);
>   
>   		if (temp > max)
>   			max = temp;
> @@ -749,9 +762,10 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
>   	return max;
>   }
>   
> -static int mtk_read_temp(struct thermal_zone_device *tz, int *temperature)
> +static int mtk_read_temp(struct thermal_zone_device *tzdev, int *temperature)
>   {
> -	struct mtk_thermal *mt = tz->devdata;
> +	struct mtk_thermal_zone *tz = tzdev->devdata;
> +	struct mtk_thermal *mt = tz->mt;
>   	int i;
>   	int tempmax = INT_MIN;
>   
> @@ -770,10 +784,28 @@ static int mtk_read_temp(struct thermal_zone_device *tz, int *temperature)
>   	return 0;
>   }
>   
> +static int mtk_read_sensor_temp(struct thermal_zone_device *tzdev, int *temperature)
> +{
> +	struct mtk_thermal_zone *tz = tzdev->devdata;
> +	struct mtk_thermal *mt = tz->mt;
> +	int id = tz->id - 1;
> +
> +	if (id < 0)
> +		return -EACCES;
> +
> +	*temperature = _get_sensor_temp(mt, id);
> +
> +	return 0;
> +}
> +
>   static const struct thermal_zone_device_ops mtk_thermal_ops = {
>   	.get_temp = mtk_read_temp,
>   };
>   
> +static const struct thermal_zone_device_ops mtk_thermal_sensor_ops = {
> +	.get_temp = mtk_read_sensor_temp,
> +};
> +
>   static void mtk_thermal_init_bank(struct mtk_thermal *mt, int num,
>   				  u32 apmixed_phys_base, u32 auxadc_phys_base,
>   				  int ctrl_id)
> @@ -1072,6 +1104,7 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>   	u64 auxadc_phys_base, apmixed_phys_base;
>   	struct thermal_zone_device *tzdev;
>   	void __iomem *apmixed_base, *auxadc_base;
> +	struct mtk_thermal_zone *tz;
>   
>   	mt = devm_kzalloc(&pdev->dev, sizeof(*mt), GFP_KERNEL);
>   	if (!mt)
> @@ -1150,6 +1183,9 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>   
>   	mtk_thermal_turn_on_buffer(mt, apmixed_base);
>   
> +	mt->raw_to_mcelsius = (mt->conf->version == MTK_THERMAL_V1) ?
> +				raw_to_mcelsius_v1 : raw_to_mcelsius_v2;
> +
>   	if (mt->conf->version == MTK_THERMAL_V2) {
>   		mtk_thermal_release_periodic_ts(mt, auxadc_base);
>   	}
> @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, mt);
>   
> -	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
> -					      &mtk_thermal_ops);
> -	if (IS_ERR(tzdev)) {
> -		ret = PTR_ERR(tzdev);
> -		goto err_disable_clk_peri_therm;
> +	for (i = 0; i < mt->conf->num_sensors + 1; i++) {
> +		tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
> +		if (!tz)
> +			return -ENOMEM;
> +
> +		tz->mt = mt;
> +		tz->id = i;
> +
> +		tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
> +							     &mtk_thermal_ops :
> +							     &mtk_thermal_sensor_ops);

Here you use again the aggregation

> +
> +		if (IS_ERR(tzdev)) {
> +			ret = PTR_ERR(tzdev);
> +			if (ret == -ENODEV)
> +				continue;
> +			goto err_disable_clk_peri_therm;
> +		}
>   	}
>   
>   	ret = devm_thermal_add_hwmon_sysfs(tzdev);
> 

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


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

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

Hi Daniel,
On Sun Dec 4, 2022 at 6:26 PM CET, Daniel Lezcano wrote:
> On 18/11/2022 12:04, Amjad Ouled-Ameur wrote:
> > Provide thermal zone to read thermal sensor in the SoC. We can read all the
> > thermal sensors value in the SoC by the node /sys/class/thermal/
> > 
> > In mtk_thermal_bank_temperature, return -EAGAIN instead of -EACCESS
> > on the first read of sensor that often are bogus values.
> > This can avoid following warning on boot:
> > 
> >    thermal thermal_zone6: failed to read out thermal zone (-13)
> > 
> > Signed-off-by: Michael Kao <michael.kao@mediatek.com>
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > ---
> >   drivers/thermal/mtk_thermal.c | 99 ++++++++++++++++++++++++++++++++-----------
> >   1 file changed, 74 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
> > index 3a5df1440822..b1f4d19edd4f 100644
> > --- a/drivers/thermal/mtk_thermal.c
> > +++ b/drivers/thermal/mtk_thermal.c
> > @@ -259,6 +259,11 @@ enum mtk_thermal_version {
> >   
> >   struct mtk_thermal;
> >   
> > +struct mtk_thermal_zone {
> > +	struct mtk_thermal *mt;
> > +	int id;
> > +};
> > +
> >   struct thermal_bank_cfg {
> >   	unsigned int num_sensors;
> >   	const int *sensors;
> > @@ -307,6 +312,8 @@ struct mtk_thermal {
> >   
> >   	const struct mtk_thermal_data *conf;
> >   	struct mtk_thermal_bank banks[MAX_NUM_ZONES];
> > +
> > +	int (*raw_to_mcelsius)(struct mtk_thermal *mt, int sensno, s32 raw);
> >   };
> >   
> >   /* MT8183 thermal sensor data */
> > @@ -709,6 +716,29 @@ static void mtk_thermal_put_bank(struct mtk_thermal_bank *bank)
> >   		mutex_unlock(&mt->lock);
> >   }
> >   
> > +static int _get_sensor_temp(struct mtk_thermal *mt, int id)
> > +{
> > +	u32 raw;
> > +	int temp;
> > +
> > +	const struct mtk_thermal_data *conf = mt->conf;
> > +
> > +	raw = readl(mt->thermal_base + conf->msr[id]);
> > +
> > +	temp = mt->raw_to_mcelsius(mt, id, raw);
> > +
> > +	/*
> > +	 * The first read of a sensor often contains very high bogus
> > +	 * temperature value. Filter these out so that the system does
> > +	 * not immediately shut down.
> > +	 */
> > +
> > +	if (temp > 200000)
> > +		return -EAGAIN;
> > +	else
> > +		return temp;
> > +}
> > +
> >   /**
> >    * mtk_thermal_bank_temperature - get the temperature of a bank
> >    * @bank:	The bank
> > @@ -721,26 +751,9 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
> >   	struct mtk_thermal *mt = bank->mt;
> >   	const struct mtk_thermal_data *conf = mt->conf;
> >   	int i, temp = INT_MIN, max = INT_MIN;
> > -	u32 raw;
> >   
> >   	for (i = 0; i < conf->bank_data[bank->id].num_sensors; i++) {
> > -		raw = readl(mt->thermal_base + conf->msr[i]);
> > -
> > -		if (mt->conf->version == MTK_THERMAL_V1) {
> > -			temp = raw_to_mcelsius_v1(
> > -				mt, conf->bank_data[bank->id].sensors[i], raw);
> > -		} else {
> > -			temp = raw_to_mcelsius_v2(
> > -				mt, conf->bank_data[bank->id].sensors[i], raw);
> > -		}
> > -
> > -		/*
> > -		 * The first read of a sensor often contains very high bogus
> > -		 * temperature value. Filter these out so that the system does
> > -		 * not immediately shut down.
> > -		 */
> > -		if (temp > 200000)
> > -			temp = 0;
> > +		temp = _get_sensor_temp(mt, i);
> >   
> >   		if (temp > max)
> >   			max = temp;
> > @@ -749,9 +762,10 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
> >   	return max;
> >   }
> >   
> > -static int mtk_read_temp(struct thermal_zone_device *tz, int *temperature)
> > +static int mtk_read_temp(struct thermal_zone_device *tzdev, int *temperature)
> >   {
> > -	struct mtk_thermal *mt = tz->devdata;
> > +	struct mtk_thermal_zone *tz = tzdev->devdata;
> > +	struct mtk_thermal *mt = tz->mt;
> >   	int i;
> >   	int tempmax = INT_MIN;
> >   
> > @@ -770,10 +784,28 @@ static int mtk_read_temp(struct thermal_zone_device *tz, int *temperature)
> >   	return 0;
> >   }
> >   
> > +static int mtk_read_sensor_temp(struct thermal_zone_device *tzdev, int *temperature)
> > +{
> > +	struct mtk_thermal_zone *tz = tzdev->devdata;
> > +	struct mtk_thermal *mt = tz->mt;
> > +	int id = tz->id - 1;
> > +
> > +	if (id < 0)
> > +		return -EACCES;
> > +
> > +	*temperature = _get_sensor_temp(mt, id);
> > +
> > +	return 0;
> > +}
> > +
> >   static const struct thermal_zone_device_ops mtk_thermal_ops = {
> >   	.get_temp = mtk_read_temp,
> >   };
> >   
> > +static const struct thermal_zone_device_ops mtk_thermal_sensor_ops = {
> > +	.get_temp = mtk_read_sensor_temp,
> > +};
> > +
> >   static void mtk_thermal_init_bank(struct mtk_thermal *mt, int num,
> >   				  u32 apmixed_phys_base, u32 auxadc_phys_base,
> >   				  int ctrl_id)
> > @@ -1072,6 +1104,7 @@ static int mtk_thermal_probe(struct platform_device *pdev)
> >   	u64 auxadc_phys_base, apmixed_phys_base;
> >   	struct thermal_zone_device *tzdev;
> >   	void __iomem *apmixed_base, *auxadc_base;
> > +	struct mtk_thermal_zone *tz;
> >   
> >   	mt = devm_kzalloc(&pdev->dev, sizeof(*mt), GFP_KERNEL);
> >   	if (!mt)
> > @@ -1150,6 +1183,9 @@ static int mtk_thermal_probe(struct platform_device *pdev)
> >   
> >   	mtk_thermal_turn_on_buffer(mt, apmixed_base);
> >   
> > +	mt->raw_to_mcelsius = (mt->conf->version == MTK_THERMAL_V1) ?
> > +				raw_to_mcelsius_v1 : raw_to_mcelsius_v2;
> > +
> >   	if (mt->conf->version == MTK_THERMAL_V2) {
> >   		mtk_thermal_release_periodic_ts(mt, auxadc_base);
> >   	}
> > @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
> >   
> >   	platform_set_drvdata(pdev, mt);
> >   
> > -	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
> > -					      &mtk_thermal_ops);
> > -	if (IS_ERR(tzdev)) {
> > -		ret = PTR_ERR(tzdev);
> > -		goto err_disable_clk_peri_therm;
> > +	for (i = 0; i < mt->conf->num_sensors + 1; i++) {
> > +		tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
> > +		if (!tz)
> > +			return -ENOMEM;
> > +
> > +		tz->mt = mt;
> > +		tz->id = i;
> > +
> > +		tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
> > +							     &mtk_thermal_ops :
> > +							     &mtk_thermal_sensor_ops);
>
> Here you use again the aggregation
I addressed this concern in V6, could you please take a look and let me
know what you think [0].

[0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@baylibre.com/

Regards,
Amjad
>
> > +
> > +		if (IS_ERR(tzdev)) {
> > +			ret = PTR_ERR(tzdev);
> > +			if (ret == -ENODEV)
> > +				continue;
> > +			goto err_disable_clk_peri_therm;
> > +		}
> >   	}
> >   
> >   	ret = devm_thermal_add_hwmon_sysfs(tzdev);
> > 
>
> -- 
> <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 v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

Hi Daniel,
On Sun Dec 4, 2022 at 6:26 PM CET, Daniel Lezcano wrote:
> On 18/11/2022 12:04, Amjad Ouled-Ameur wrote:
> > Provide thermal zone to read thermal sensor in the SoC. We can read all the
> > thermal sensors value in the SoC by the node /sys/class/thermal/
> > 
> > In mtk_thermal_bank_temperature, return -EAGAIN instead of -EACCESS
> > on the first read of sensor that often are bogus values.
> > This can avoid following warning on boot:
> > 
> >    thermal thermal_zone6: failed to read out thermal zone (-13)
> > 
> > Signed-off-by: Michael Kao <michael.kao@mediatek.com>
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > ---
> >   drivers/thermal/mtk_thermal.c | 99 ++++++++++++++++++++++++++++++++-----------
> >   1 file changed, 74 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
> > index 3a5df1440822..b1f4d19edd4f 100644
> > --- a/drivers/thermal/mtk_thermal.c
> > +++ b/drivers/thermal/mtk_thermal.c
> > @@ -259,6 +259,11 @@ enum mtk_thermal_version {
> >   
> >   struct mtk_thermal;
> >   
> > +struct mtk_thermal_zone {
> > +	struct mtk_thermal *mt;
> > +	int id;
> > +};
> > +
> >   struct thermal_bank_cfg {
> >   	unsigned int num_sensors;
> >   	const int *sensors;
> > @@ -307,6 +312,8 @@ struct mtk_thermal {
> >   
> >   	const struct mtk_thermal_data *conf;
> >   	struct mtk_thermal_bank banks[MAX_NUM_ZONES];
> > +
> > +	int (*raw_to_mcelsius)(struct mtk_thermal *mt, int sensno, s32 raw);
> >   };
> >   
> >   /* MT8183 thermal sensor data */
> > @@ -709,6 +716,29 @@ static void mtk_thermal_put_bank(struct mtk_thermal_bank *bank)
> >   		mutex_unlock(&mt->lock);
> >   }
> >   
> > +static int _get_sensor_temp(struct mtk_thermal *mt, int id)
> > +{
> > +	u32 raw;
> > +	int temp;
> > +
> > +	const struct mtk_thermal_data *conf = mt->conf;
> > +
> > +	raw = readl(mt->thermal_base + conf->msr[id]);
> > +
> > +	temp = mt->raw_to_mcelsius(mt, id, raw);
> > +
> > +	/*
> > +	 * The first read of a sensor often contains very high bogus
> > +	 * temperature value. Filter these out so that the system does
> > +	 * not immediately shut down.
> > +	 */
> > +
> > +	if (temp > 200000)
> > +		return -EAGAIN;
> > +	else
> > +		return temp;
> > +}
> > +
> >   /**
> >    * mtk_thermal_bank_temperature - get the temperature of a bank
> >    * @bank:	The bank
> > @@ -721,26 +751,9 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
> >   	struct mtk_thermal *mt = bank->mt;
> >   	const struct mtk_thermal_data *conf = mt->conf;
> >   	int i, temp = INT_MIN, max = INT_MIN;
> > -	u32 raw;
> >   
> >   	for (i = 0; i < conf->bank_data[bank->id].num_sensors; i++) {
> > -		raw = readl(mt->thermal_base + conf->msr[i]);
> > -
> > -		if (mt->conf->version == MTK_THERMAL_V1) {
> > -			temp = raw_to_mcelsius_v1(
> > -				mt, conf->bank_data[bank->id].sensors[i], raw);
> > -		} else {
> > -			temp = raw_to_mcelsius_v2(
> > -				mt, conf->bank_data[bank->id].sensors[i], raw);
> > -		}
> > -
> > -		/*
> > -		 * The first read of a sensor often contains very high bogus
> > -		 * temperature value. Filter these out so that the system does
> > -		 * not immediately shut down.
> > -		 */
> > -		if (temp > 200000)
> > -			temp = 0;
> > +		temp = _get_sensor_temp(mt, i);
> >   
> >   		if (temp > max)
> >   			max = temp;
> > @@ -749,9 +762,10 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
> >   	return max;
> >   }
> >   
> > -static int mtk_read_temp(struct thermal_zone_device *tz, int *temperature)
> > +static int mtk_read_temp(struct thermal_zone_device *tzdev, int *temperature)
> >   {
> > -	struct mtk_thermal *mt = tz->devdata;
> > +	struct mtk_thermal_zone *tz = tzdev->devdata;
> > +	struct mtk_thermal *mt = tz->mt;
> >   	int i;
> >   	int tempmax = INT_MIN;
> >   
> > @@ -770,10 +784,28 @@ static int mtk_read_temp(struct thermal_zone_device *tz, int *temperature)
> >   	return 0;
> >   }
> >   
> > +static int mtk_read_sensor_temp(struct thermal_zone_device *tzdev, int *temperature)
> > +{
> > +	struct mtk_thermal_zone *tz = tzdev->devdata;
> > +	struct mtk_thermal *mt = tz->mt;
> > +	int id = tz->id - 1;
> > +
> > +	if (id < 0)
> > +		return -EACCES;
> > +
> > +	*temperature = _get_sensor_temp(mt, id);
> > +
> > +	return 0;
> > +}
> > +
> >   static const struct thermal_zone_device_ops mtk_thermal_ops = {
> >   	.get_temp = mtk_read_temp,
> >   };
> >   
> > +static const struct thermal_zone_device_ops mtk_thermal_sensor_ops = {
> > +	.get_temp = mtk_read_sensor_temp,
> > +};
> > +
> >   static void mtk_thermal_init_bank(struct mtk_thermal *mt, int num,
> >   				  u32 apmixed_phys_base, u32 auxadc_phys_base,
> >   				  int ctrl_id)
> > @@ -1072,6 +1104,7 @@ static int mtk_thermal_probe(struct platform_device *pdev)
> >   	u64 auxadc_phys_base, apmixed_phys_base;
> >   	struct thermal_zone_device *tzdev;
> >   	void __iomem *apmixed_base, *auxadc_base;
> > +	struct mtk_thermal_zone *tz;
> >   
> >   	mt = devm_kzalloc(&pdev->dev, sizeof(*mt), GFP_KERNEL);
> >   	if (!mt)
> > @@ -1150,6 +1183,9 @@ static int mtk_thermal_probe(struct platform_device *pdev)
> >   
> >   	mtk_thermal_turn_on_buffer(mt, apmixed_base);
> >   
> > +	mt->raw_to_mcelsius = (mt->conf->version == MTK_THERMAL_V1) ?
> > +				raw_to_mcelsius_v1 : raw_to_mcelsius_v2;
> > +
> >   	if (mt->conf->version == MTK_THERMAL_V2) {
> >   		mtk_thermal_release_periodic_ts(mt, auxadc_base);
> >   	}
> > @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
> >   
> >   	platform_set_drvdata(pdev, mt);
> >   
> > -	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
> > -					      &mtk_thermal_ops);
> > -	if (IS_ERR(tzdev)) {
> > -		ret = PTR_ERR(tzdev);
> > -		goto err_disable_clk_peri_therm;
> > +	for (i = 0; i < mt->conf->num_sensors + 1; i++) {
> > +		tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
> > +		if (!tz)
> > +			return -ENOMEM;
> > +
> > +		tz->mt = mt;
> > +		tz->id = i;
> > +
> > +		tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
> > +							     &mtk_thermal_ops :
> > +							     &mtk_thermal_sensor_ops);
>
> Here you use again the aggregation
I addressed this concern in V6, could you please take a look and let me
know what you think [0].

[0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@baylibre.com/

Regards,
Amjad
>
> > +
> > +		if (IS_ERR(tzdev)) {
> > +			ret = PTR_ERR(tzdev);
> > +			if (ret == -ENODEV)
> > +				continue;
> > +			goto err_disable_clk_peri_therm;
> > +		}
> >   	}
> >   
> >   	ret = devm_thermal_add_hwmon_sysfs(tzdev);
> > 
>
> -- 
> <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


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

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Daniel Lezcano]

Hi Amjad,


On 05/12/2022 11:41, Amjad Ouled-Ameur wrote:

[ ... ]

>>> @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>>>    
>>>    	platform_set_drvdata(pdev, mt);
>>>    
>>> -	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
>>> -					      &mtk_thermal_ops);
>>> -	if (IS_ERR(tzdev)) {
>>> -		ret = PTR_ERR(tzdev);
>>> -		goto err_disable_clk_peri_therm;
>>> +	for (i = 0; i < mt->conf->num_sensors + 1; i++) {
>>> +		tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
>>> +		if (!tz)
>>> +			return -ENOMEM;
>>> +
>>> +		tz->mt = mt;
>>> +		tz->id = i;
>>> +
>>> +		tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
>>> +							     &mtk_thermal_ops :
>>> +							     &mtk_thermal_sensor_ops);
>>
>> Here you use again the aggregation
> I addressed this concern in V6, could you please take a look and let me
> know what you think [0].
> 
> [0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@baylibre.com/

May I misunderstanding but AFAICS, this patch is setting the 
mtk_thermal_ops if the sensor id is zero. The get_temp is computing the 
max temperature in this ops which is what we don't want to do.


-- 
<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 v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Daniel Lezcano]

Hi Amjad,


On 05/12/2022 11:41, Amjad Ouled-Ameur wrote:

[ ... ]

>>> @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>>>    
>>>    	platform_set_drvdata(pdev, mt);
>>>    
>>> -	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
>>> -					      &mtk_thermal_ops);
>>> -	if (IS_ERR(tzdev)) {
>>> -		ret = PTR_ERR(tzdev);
>>> -		goto err_disable_clk_peri_therm;
>>> +	for (i = 0; i < mt->conf->num_sensors + 1; i++) {
>>> +		tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
>>> +		if (!tz)
>>> +			return -ENOMEM;
>>> +
>>> +		tz->mt = mt;
>>> +		tz->id = i;
>>> +
>>> +		tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
>>> +							     &mtk_thermal_ops :
>>> +							     &mtk_thermal_sensor_ops);
>>
>> Here you use again the aggregation
> I addressed this concern in V6, could you please take a look and let me
> know what you think [0].
> 
> [0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@baylibre.com/

May I misunderstanding but AFAICS, this patch is setting the 
mtk_thermal_ops if the sensor id is zero. The get_temp is computing the 
max temperature in this ops which is what we don't want to do.


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


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

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

Hi Daniel,
On Mon Dec 5, 2022 at 8:39 PM CET, Daniel Lezcano wrote:
>
> Hi Amjad,
>
>
> On 05/12/2022 11:41, Amjad Ouled-Ameur wrote:
>
> [ ... ]
>
> >>> @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
> >>>    
> >>>    	platform_set_drvdata(pdev, mt);
> >>>    
> >>> -	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
> >>> -					      &mtk_thermal_ops);
> >>> -	if (IS_ERR(tzdev)) {
> >>> -		ret = PTR_ERR(tzdev);
> >>> -		goto err_disable_clk_peri_therm;
> >>> +	for (i = 0; i < mt->conf->num_sensors + 1; i++) {
> >>> +		tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
> >>> +		if (!tz)
> >>> +			return -ENOMEM;
> >>> +
> >>> +		tz->mt = mt;
> >>> +		tz->id = i;
> >>> +
> >>> +		tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
> >>> +							     &mtk_thermal_ops :
> >>> +							     &mtk_thermal_sensor_ops);
> >>
> >> Here you use again the aggregation
> > I addressed this concern in V6, could you please take a look and let me
> > know what you think [0].
> > 
> > [0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@baylibre.com/
>
> May I misunderstanding but AFAICS, this patch is setting the 
> mtk_thermal_ops if the sensor id is zero. The get_temp is computing the 
> max temperature in this ops which is what we don't want to do.

Correct, but I think that is out of scope of this patchset, as the current
driver already uses mtk_thermal_ops for sensor 0. The focus of this patchset
is to add support for the other sensors.

Besides, what do you suggest as a clean implementation if the current one
no longer meets thermal core requirements ?

Regards,
Amjad
>
>
> -- 
> <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 v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

Hi Daniel,
On Mon Dec 5, 2022 at 8:39 PM CET, Daniel Lezcano wrote:
>
> Hi Amjad,
>
>
> On 05/12/2022 11:41, Amjad Ouled-Ameur wrote:
>
> [ ... ]
>
> >>> @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
> >>>    
> >>>    	platform_set_drvdata(pdev, mt);
> >>>    
> >>> -	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
> >>> -					      &mtk_thermal_ops);
> >>> -	if (IS_ERR(tzdev)) {
> >>> -		ret = PTR_ERR(tzdev);
> >>> -		goto err_disable_clk_peri_therm;
> >>> +	for (i = 0; i < mt->conf->num_sensors + 1; i++) {
> >>> +		tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
> >>> +		if (!tz)
> >>> +			return -ENOMEM;
> >>> +
> >>> +		tz->mt = mt;
> >>> +		tz->id = i;
> >>> +
> >>> +		tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
> >>> +							     &mtk_thermal_ops :
> >>> +							     &mtk_thermal_sensor_ops);
> >>
> >> Here you use again the aggregation
> > I addressed this concern in V6, could you please take a look and let me
> > know what you think [0].
> > 
> > [0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@baylibre.com/
>
> May I misunderstanding but AFAICS, this patch is setting the 
> mtk_thermal_ops if the sensor id is zero. The get_temp is computing the 
> max temperature in this ops which is what we don't want to do.

Correct, but I think that is out of scope of this patchset, as the current
driver already uses mtk_thermal_ops for sensor 0. The focus of this patchset
is to add support for the other sensors.

Besides, what do you suggest as a clean implementation if the current one
no longer meets thermal core requirements ?

Regards,
Amjad
>
>
> -- 
> <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


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

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

Hi Daniel,

On 12/6/22 10:18, Amjad Ouled-Ameur wrote:
> Hi Daniel,
> On Mon Dec 5, 2022 at 8:39 PM CET, Daniel Lezcano wrote:
>> Hi Amjad,
>>
>>
>> On 05/12/2022 11:41, Amjad Ouled-Ameur wrote:
>>
>> [ ... ]
>>
>>>>> @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>>>>>     
>>>>>     	platform_set_drvdata(pdev, mt);
>>>>>     
>>>>> -	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
>>>>> -					      &mtk_thermal_ops);
>>>>> -	if (IS_ERR(tzdev)) {
>>>>> -		ret = PTR_ERR(tzdev);
>>>>> -		goto err_disable_clk_peri_therm;
>>>>> +	for (i = 0; i < mt->conf->num_sensors + 1; i++) {
>>>>> +		tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
>>>>> +		if (!tz)
>>>>> +			return -ENOMEM;
>>>>> +
>>>>> +		tz->mt = mt;
>>>>> +		tz->id = i;
>>>>> +
>>>>> +		tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
>>>>> +							     &mtk_thermal_ops :
>>>>> +							     &mtk_thermal_sensor_ops);
>>>> Here you use again the aggregation
>>> I addressed this concern in V6, could you please take a look and let me
>>> know what you think [0].
>>>
>>> [0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@baylibre.com/
>> May I misunderstanding but AFAICS, this patch is setting the
>> mtk_thermal_ops if the sensor id is zero. The get_temp is computing the
>> max temperature in this ops which is what we don't want to do.
> Correct, but I think that is out of scope of this patchset, as the current
> driver already uses mtk_thermal_ops for sensor 0. The focus of this patchset
> is to add support for the other sensors.
>
> Besides, what do you suggest as a clean implementation if the current one
> no longer meets thermal core requirements ?

Could you please address this ?


Kindly,

Amjad

> Regards,
> Amjad
>>
>> -- 
>> <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 v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

Hi Daniel,

On 12/6/22 10:18, Amjad Ouled-Ameur wrote:
> Hi Daniel,
> On Mon Dec 5, 2022 at 8:39 PM CET, Daniel Lezcano wrote:
>> Hi Amjad,
>>
>>
>> On 05/12/2022 11:41, Amjad Ouled-Ameur wrote:
>>
>> [ ... ]
>>
>>>>> @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>>>>>     
>>>>>     	platform_set_drvdata(pdev, mt);
>>>>>     
>>>>> -	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
>>>>> -					      &mtk_thermal_ops);
>>>>> -	if (IS_ERR(tzdev)) {
>>>>> -		ret = PTR_ERR(tzdev);
>>>>> -		goto err_disable_clk_peri_therm;
>>>>> +	for (i = 0; i < mt->conf->num_sensors + 1; i++) {
>>>>> +		tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
>>>>> +		if (!tz)
>>>>> +			return -ENOMEM;
>>>>> +
>>>>> +		tz->mt = mt;
>>>>> +		tz->id = i;
>>>>> +
>>>>> +		tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
>>>>> +							     &mtk_thermal_ops :
>>>>> +							     &mtk_thermal_sensor_ops);
>>>> Here you use again the aggregation
>>> I addressed this concern in V6, could you please take a look and let me
>>> know what you think [0].
>>>
>>> [0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@baylibre.com/
>> May I misunderstanding but AFAICS, this patch is setting the
>> mtk_thermal_ops if the sensor id is zero. The get_temp is computing the
>> max temperature in this ops which is what we don't want to do.
> Correct, but I think that is out of scope of this patchset, as the current
> driver already uses mtk_thermal_ops for sensor 0. The focus of this patchset
> is to add support for the other sensors.
>
> Besides, what do you suggest as a clean implementation if the current one
> no longer meets thermal core requirements ?

Could you please address this ?


Kindly,

Amjad

> Regards,
> Amjad
>>
>> -- 
>> <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

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

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Daniel Lezcano]

On 06/12/2022 10:18, Amjad Ouled-Ameur wrote:
> Hi Daniel,
> On Mon Dec 5, 2022 at 8:39 PM CET, Daniel Lezcano wrote:
>>
>> Hi Amjad,
>>
>>
>> On 05/12/2022 11:41, Amjad Ouled-Ameur wrote:
>>
>> [ ... ]
>>
>>>>> @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>>>>>     
>>>>>     	platform_set_drvdata(pdev, mt);
>>>>>     
>>>>> -	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
>>>>> -					      &mtk_thermal_ops);
>>>>> -	if (IS_ERR(tzdev)) {
>>>>> -		ret = PTR_ERR(tzdev);
>>>>> -		goto err_disable_clk_peri_therm;
>>>>> +	for (i = 0; i < mt->conf->num_sensors + 1; i++) {
>>>>> +		tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
>>>>> +		if (!tz)
>>>>> +			return -ENOMEM;
>>>>> +
>>>>> +		tz->mt = mt;
>>>>> +		tz->id = i;
>>>>> +
>>>>> +		tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
>>>>> +							     &mtk_thermal_ops :
>>>>> +							     &mtk_thermal_sensor_ops);
>>>>
>>>> Here you use again the aggregation
>>> I addressed this concern in V6, could you please take a look and let me
>>> know what you think [0].
>>>
>>> [0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@baylibre.com/
>>
>> May I misunderstanding but AFAICS, this patch is setting the
>> mtk_thermal_ops if the sensor id is zero. The get_temp is computing the
>> max temperature in this ops which is what we don't want to do.
> 
> Correct, but I think that is out of scope of this patchset, as the current
> driver already uses mtk_thermal_ops for sensor 0. The focus of this patchset
> is to add support for the other sensors.
> 
> Besides, what do you suggest as a clean implementation if the current one
> no longer meets thermal core requirements ?

IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 x 4 
Little, right ?

If it is the case, then a thermal zone per sensor with the trip points 
and a cooling device for each of them.

The two thermal zones for the big will share the same cooling device. 
The little thermal zone will have its own cooling device.

If there is the GPU, then its own cooling device also with devfreq.


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

-- 
<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 v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Daniel Lezcano]

On 06/12/2022 10:18, Amjad Ouled-Ameur wrote:
> Hi Daniel,
> On Mon Dec 5, 2022 at 8:39 PM CET, Daniel Lezcano wrote:
>>
>> Hi Amjad,
>>
>>
>> On 05/12/2022 11:41, Amjad Ouled-Ameur wrote:
>>
>> [ ... ]
>>
>>>>> @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>>>>>     
>>>>>     	platform_set_drvdata(pdev, mt);
>>>>>     
>>>>> -	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
>>>>> -					      &mtk_thermal_ops);
>>>>> -	if (IS_ERR(tzdev)) {
>>>>> -		ret = PTR_ERR(tzdev);
>>>>> -		goto err_disable_clk_peri_therm;
>>>>> +	for (i = 0; i < mt->conf->num_sensors + 1; i++) {
>>>>> +		tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
>>>>> +		if (!tz)
>>>>> +			return -ENOMEM;
>>>>> +
>>>>> +		tz->mt = mt;
>>>>> +		tz->id = i;
>>>>> +
>>>>> +		tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
>>>>> +							     &mtk_thermal_ops :
>>>>> +							     &mtk_thermal_sensor_ops);
>>>>
>>>> Here you use again the aggregation
>>> I addressed this concern in V6, could you please take a look and let me
>>> know what you think [0].
>>>
>>> [0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@baylibre.com/
>>
>> May I misunderstanding but AFAICS, this patch is setting the
>> mtk_thermal_ops if the sensor id is zero. The get_temp is computing the
>> max temperature in this ops which is what we don't want to do.
> 
> Correct, but I think that is out of scope of this patchset, as the current
> driver already uses mtk_thermal_ops for sensor 0. The focus of this patchset
> is to add support for the other sensors.
> 
> Besides, what do you suggest as a clean implementation if the current one
> no longer meets thermal core requirements ?

IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 x 4 
Little, right ?

If it is the case, then a thermal zone per sensor with the trip points 
and a cooling device for each of them.

The two thermal zones for the big will share the same cooling device. 
The little thermal zone will have its own cooling device.

If there is the GPU, then its own cooling device also with devfreq.


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

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


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

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

Hi Daniel,

On 12/29/22 16:49, Daniel Lezcano wrote:
> On 06/12/2022 10:18, Amjad Ouled-Ameur wrote:
>> Hi Daniel,
>> On Mon Dec 5, 2022 at 8:39 PM CET, Daniel Lezcano wrote:
>>>
>>> Hi Amjad,
>>>
>>>
>>> On 05/12/2022 11:41, Amjad Ouled-Ameur wrote:
>>>
>>> [ ... ]
>>>
>>>>>> @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>>>>>>             platform_set_drvdata(pdev, mt);
>>>>>>     -    tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
>>>>>> -                          &mtk_thermal_ops);
>>>>>> -    if (IS_ERR(tzdev)) {
>>>>>> -        ret = PTR_ERR(tzdev);
>>>>>> -        goto err_disable_clk_peri_therm;
>>>>>> +    for (i = 0; i < mt->conf->num_sensors + 1; i++) {
>>>>>> +        tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
>>>>>> +        if (!tz)
>>>>>> +            return -ENOMEM;
>>>>>> +
>>>>>> +        tz->mt = mt;
>>>>>> +        tz->id = i;
>>>>>> +
>>>>>> +        tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
>>>>>> +                                 &mtk_thermal_ops :
>>>>>> + &mtk_thermal_sensor_ops);
>>>>>
>>>>> Here you use again the aggregation
>>>> I addressed this concern in V6, could you please take a look and let me
>>>> know what you think [0].
>>>>
>>>> [0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@baylibre.com/
>>>
>>> May I misunderstanding but AFAICS, this patch is setting the
>>> mtk_thermal_ops if the sensor id is zero. The get_temp is computing the
>>> max temperature in this ops which is what we don't want to do.
>>
>> Correct, but I think that is out of scope of this patchset, as the current
>> driver already uses mtk_thermal_ops for sensor 0. The focus of this patchset
>> is to add support for the other sensors.
>>
>> Besides, what do you suggest as a clean implementation if the current one
>> no longer meets thermal core requirements ?
>
> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 x 4 Little, right ?

MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per sensor. Thermal zone 0 corresponds

to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has nothing to do with CPUs. The cooling device type

used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are present in the SoC for debug-purpose only, they are not supposed

to be used for production.


Regards,

Amjad

>
> If it is the case, then a thermal zone per sensor with the trip points and a cooling device for each of them.
>
> The two thermal zones for the big will share the same cooling device. The little thermal zone will have its own cooling device.
>
> If there is the GPU, then its own cooling device also with devfreq.
>
>
>>> <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 v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

Hi Daniel,

On 12/29/22 16:49, Daniel Lezcano wrote:
> On 06/12/2022 10:18, Amjad Ouled-Ameur wrote:
>> Hi Daniel,
>> On Mon Dec 5, 2022 at 8:39 PM CET, Daniel Lezcano wrote:
>>>
>>> Hi Amjad,
>>>
>>>
>>> On 05/12/2022 11:41, Amjad Ouled-Ameur wrote:
>>>
>>> [ ... ]
>>>
>>>>>> @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>>>>>>             platform_set_drvdata(pdev, mt);
>>>>>>     -    tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
>>>>>> -                          &mtk_thermal_ops);
>>>>>> -    if (IS_ERR(tzdev)) {
>>>>>> -        ret = PTR_ERR(tzdev);
>>>>>> -        goto err_disable_clk_peri_therm;
>>>>>> +    for (i = 0; i < mt->conf->num_sensors + 1; i++) {
>>>>>> +        tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
>>>>>> +        if (!tz)
>>>>>> +            return -ENOMEM;
>>>>>> +
>>>>>> +        tz->mt = mt;
>>>>>> +        tz->id = i;
>>>>>> +
>>>>>> +        tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
>>>>>> +                                 &mtk_thermal_ops :
>>>>>> + &mtk_thermal_sensor_ops);
>>>>>
>>>>> Here you use again the aggregation
>>>> I addressed this concern in V6, could you please take a look and let me
>>>> know what you think [0].
>>>>
>>>> [0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@baylibre.com/
>>>
>>> May I misunderstanding but AFAICS, this patch is setting the
>>> mtk_thermal_ops if the sensor id is zero. The get_temp is computing the
>>> max temperature in this ops which is what we don't want to do.
>>
>> Correct, but I think that is out of scope of this patchset, as the current
>> driver already uses mtk_thermal_ops for sensor 0. The focus of this patchset
>> is to add support for the other sensors.
>>
>> Besides, what do you suggest as a clean implementation if the current one
>> no longer meets thermal core requirements ?
>
> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 x 4 Little, right ?

MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per sensor. Thermal zone 0 corresponds

to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has nothing to do with CPUs. The cooling device type

used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are present in the SoC for debug-purpose only, they are not supposed

to be used for production.


Regards,

Amjad

>
> If it is the case, then a thermal zone per sensor with the trip points and a cooling device for each of them.
>
> The two thermal zones for the big will share the same cooling device. The little thermal zone will have its own cooling device.
>
> If there is the GPU, then its own cooling device also with devfreq.
>
>
>>> <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
>>
>

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

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

Hi Daniel,

On 1/19/23 18:03, Amjad Ouled-Ameur wrote:
> Hi Daniel,
>
> On 12/29/22 16:49, Daniel Lezcano wrote:
>> On 06/12/2022 10:18, Amjad Ouled-Ameur wrote:
>>> Hi Daniel,
>>> On Mon Dec 5, 2022 at 8:39 PM CET, Daniel Lezcano wrote:
>>>>
>>>> Hi Amjad,
>>>>
>>>>
>>>> On 05/12/2022 11:41, Amjad Ouled-Ameur wrote:
>>>>
>>>> [ ... ]
>>>>
>>>>>>> @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>>>>>>>             platform_set_drvdata(pdev, mt);
>>>>>>>     -    tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
>>>>>>> -                          &mtk_thermal_ops);
>>>>>>> -    if (IS_ERR(tzdev)) {
>>>>>>> -        ret = PTR_ERR(tzdev);
>>>>>>> -        goto err_disable_clk_peri_therm;
>>>>>>> +    for (i = 0; i < mt->conf->num_sensors + 1; i++) {
>>>>>>> +        tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
>>>>>>> +        if (!tz)
>>>>>>> +            return -ENOMEM;
>>>>>>> +
>>>>>>> +        tz->mt = mt;
>>>>>>> +        tz->id = i;
>>>>>>> +
>>>>>>> +        tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
>>>>>>> +                                 &mtk_thermal_ops :
>>>>>>> + &mtk_thermal_sensor_ops);
>>>>>>
>>>>>> Here you use again the aggregation
>>>>> I addressed this concern in V6, could you please take a look and let me
>>>>> know what you think [0].
>>>>>
>>>>> [0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@baylibre.com/
>>>>
>>>> May I misunderstanding but AFAICS, this patch is setting the
>>>> mtk_thermal_ops if the sensor id is zero. The get_temp is computing the
>>>> max temperature in this ops which is what we don't want to do.
>>>
>>> Correct, but I think that is out of scope of this patchset, as the current
>>> driver already uses mtk_thermal_ops for sensor 0. The focus of this patchset
>>> is to add support for the other sensors.
>>>
>>> Besides, what do you suggest as a clean implementation if the current one
>>> no longer meets thermal core requirements ?
>>
>> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 x 4 Little, right ?
>
> MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per sensor. Thermal zone 0 corresponds
>
> to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has nothing to do with CPUs. The cooling device type
>
> used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are present in the SoC for debug-purpose only, they are not supposed
>
> to be used for production.
>
After reconsidering the fact that zones 1, 2 and 3 are only used for dev/debug, it might be best to avoid

aggregation as you suggested, and keep only support for zone 0 in this driver. Thus I suggest I send a V8

where I keep only below fixes for this patch if that's okay with you:

- Define "raw_to_mcelsius" function pointer for "struct thermal_bank_cfg".

- Fix "mtk_thermal" variable in mtk_read_temp().

- Set "mt->raw_to_mcelsius" in probe().


For zones 1, 2 and 3 we can later add a different driver specific for dev/debug to probe them to

avoid confusion.


Regards,

Amjad

>
> Regards,
>
> Amjad
>
>>
>> If it is the case, then a thermal zone per sensor with the trip points and a cooling device for each of them.
>>
>> The two thermal zones for the big will share the same cooling device. The little thermal zone will have its own cooling device.
>>
>> If there is the GPU, then its own cooling device also with devfreq.
>>
>>
>>>> <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 v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

Hi Daniel,

On 1/19/23 18:03, Amjad Ouled-Ameur wrote:
> Hi Daniel,
>
> On 12/29/22 16:49, Daniel Lezcano wrote:
>> On 06/12/2022 10:18, Amjad Ouled-Ameur wrote:
>>> Hi Daniel,
>>> On Mon Dec 5, 2022 at 8:39 PM CET, Daniel Lezcano wrote:
>>>>
>>>> Hi Amjad,
>>>>
>>>>
>>>> On 05/12/2022 11:41, Amjad Ouled-Ameur wrote:
>>>>
>>>> [ ... ]
>>>>
>>>>>>> @@ -1161,11 +1197,24 @@ static int mtk_thermal_probe(struct platform_device *pdev)
>>>>>>>             platform_set_drvdata(pdev, mt);
>>>>>>>     -    tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
>>>>>>> -                          &mtk_thermal_ops);
>>>>>>> -    if (IS_ERR(tzdev)) {
>>>>>>> -        ret = PTR_ERR(tzdev);
>>>>>>> -        goto err_disable_clk_peri_therm;
>>>>>>> +    for (i = 0; i < mt->conf->num_sensors + 1; i++) {
>>>>>>> +        tz = devm_kmalloc(&pdev->dev, sizeof(*tz), GFP_KERNEL);
>>>>>>> +        if (!tz)
>>>>>>> +            return -ENOMEM;
>>>>>>> +
>>>>>>> +        tz->mt = mt;
>>>>>>> +        tz->id = i;
>>>>>>> +
>>>>>>> +        tzdev = devm_thermal_of_zone_register(&pdev->dev, i, tz, (i == 0) ?
>>>>>>> +                                 &mtk_thermal_ops :
>>>>>>> + &mtk_thermal_sensor_ops);
>>>>>>
>>>>>> Here you use again the aggregation
>>>>> I addressed this concern in V6, could you please take a look and let me
>>>>> know what you think [0].
>>>>>
>>>>> [0]: https://lore.kernel.org/all/5eb0cdc2-e9f9-dd42-bf80-b7dcd8bcc196@baylibre.com/
>>>>
>>>> May I misunderstanding but AFAICS, this patch is setting the
>>>> mtk_thermal_ops if the sensor id is zero. The get_temp is computing the
>>>> max temperature in this ops which is what we don't want to do.
>>>
>>> Correct, but I think that is out of scope of this patchset, as the current
>>> driver already uses mtk_thermal_ops for sensor 0. The focus of this patchset
>>> is to add support for the other sensors.
>>>
>>> Besides, what do you suggest as a clean implementation if the current one
>>> no longer meets thermal core requirements ?
>>
>> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 x 4 Little, right ?
>
> MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per sensor. Thermal zone 0 corresponds
>
> to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has nothing to do with CPUs. The cooling device type
>
> used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are present in the SoC for debug-purpose only, they are not supposed
>
> to be used for production.
>
After reconsidering the fact that zones 1, 2 and 3 are only used for dev/debug, it might be best to avoid

aggregation as you suggested, and keep only support for zone 0 in this driver. Thus I suggest I send a V8

where I keep only below fixes for this patch if that's okay with you:

- Define "raw_to_mcelsius" function pointer for "struct thermal_bank_cfg".

- Fix "mtk_thermal" variable in mtk_read_temp().

- Set "mt->raw_to_mcelsius" in probe().


For zones 1, 2 and 3 we can later add a different driver specific for dev/debug to probe them to

avoid confusion.


Regards,

Amjad

>
> Regards,
>
> Amjad
>
>>
>> If it is the case, then a thermal zone per sensor with the trip points and a cooling device for each of them.
>>
>> The two thermal zones for the big will share the same cooling device. The little thermal zone will have its own cooling device.
>>
>> If there is the GPU, then its own cooling device also with devfreq.
>>
>>
>>>> <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
>>>
>>

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

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Daniel Lezcano]

Hi Amjad,

On 24/01/2023 11:08, Amjad Ouled-Ameur wrote:

[ ... ]

>>>
>>> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 
>>> x 4 Little, right ?
>>
>> MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per sensor. 
>> Thermal zone 0 corresponds
>>
>> to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has nothing 
>> to do with CPUs. The cooling device type
>>
>> used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are present in 
>> the SoC for debug-purpose only, they are not supposed
>>
>> to be used for production.
>>
> After reconsidering the fact that zones 1, 2 and 3 are only used for 
> dev/debug, it might be best to avo >
> aggregation as you suggested, and keep only support for zone 0 in this 
> driver. Thus I suggest I send a V8
> 
> where I keep only below fixes for this patch if that's okay with you:
> 
> - Define "raw_to_mcelsius" function pointer for "struct thermal_bank_cfg".
> 
> - Fix "mtk_thermal" variable in mtk_read_temp().
> 
> - Set "mt->raw_to_mcelsius" in probe().
> 
> 
> For zones 1, 2 and 3 we can later add a different driver specific for 
> dev/debug to probe them to
> 
> avoid confusion.

You can add them in the driver and in the device tree, but just add the 
cooling device for the thermal zone 0.


-- 
<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 v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Daniel Lezcano]

Hi Amjad,

On 24/01/2023 11:08, Amjad Ouled-Ameur wrote:

[ ... ]

>>>
>>> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 
>>> x 4 Little, right ?
>>
>> MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per sensor. 
>> Thermal zone 0 corresponds
>>
>> to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has nothing 
>> to do with CPUs. The cooling device type
>>
>> used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are present in 
>> the SoC for debug-purpose only, they are not supposed
>>
>> to be used for production.
>>
> After reconsidering the fact that zones 1, 2 and 3 are only used for 
> dev/debug, it might be best to avo >
> aggregation as you suggested, and keep only support for zone 0 in this 
> driver. Thus I suggest I send a V8
> 
> where I keep only below fixes for this patch if that's okay with you:
> 
> - Define "raw_to_mcelsius" function pointer for "struct thermal_bank_cfg".
> 
> - Fix "mtk_thermal" variable in mtk_read_temp().
> 
> - Set "mt->raw_to_mcelsius" in probe().
> 
> 
> For zones 1, 2 and 3 we can later add a different driver specific for 
> dev/debug to probe them to
> 
> avoid confusion.

You can add them in the driver and in the device tree, but just add the 
cooling device for the thermal zone 0.


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


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

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

On 1/24/23 17:54, Daniel Lezcano wrote:
>
> Hi Amjad,
>
> On 24/01/2023 11:08, Amjad Ouled-Ameur wrote:
>
> [ ... ]
>
>>>>
>>>> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 x 4 Little, right ?
>>>
>>> MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per sensor. Thermal zone 0 corresponds
>>>
>>> to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has nothing to do with CPUs. The cooling device type
>>>
>>> used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are present in the SoC for debug-purpose only, they are not supposed
>>>
>>> to be used for production.
>>>
>> After reconsidering the fact that zones 1, 2 and 3 are only used for dev/debug, it might be best to avo >
>> aggregation as you suggested, and keep only support for zone 0 in this driver. Thus I suggest I send a V8
>>
>> where I keep only below fixes for this patch if that's okay with you:
>>
>> - Define "raw_to_mcelsius" function pointer for "struct thermal_bank_cfg".
>>
>> - Fix "mtk_thermal" variable in mtk_read_temp().
>>
>> - Set "mt->raw_to_mcelsius" in probe().
>>
>>
>> For zones 1, 2 and 3 we can later add a different driver specific for dev/debug to probe them to
>>
>> avoid confusion.
>
> You can add them in the driver and in the device tree, but just add the cooling device for the thermal zone 0.

Thermal zone 0 uses CPU{0..3} for passive cooling, in this case we should register cooling device with

cpufreq_cooling_register() for each CPU right ?

>
>

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

On 1/24/23 17:54, Daniel Lezcano wrote:
>
> Hi Amjad,
>
> On 24/01/2023 11:08, Amjad Ouled-Ameur wrote:
>
> [ ... ]
>
>>>>
>>>> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 x 4 Little, right ?
>>>
>>> MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per sensor. Thermal zone 0 corresponds
>>>
>>> to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has nothing to do with CPUs. The cooling device type
>>>
>>> used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are present in the SoC for debug-purpose only, they are not supposed
>>>
>>> to be used for production.
>>>
>> After reconsidering the fact that zones 1, 2 and 3 are only used for dev/debug, it might be best to avo >
>> aggregation as you suggested, and keep only support for zone 0 in this driver. Thus I suggest I send a V8
>>
>> where I keep only below fixes for this patch if that's okay with you:
>>
>> - Define "raw_to_mcelsius" function pointer for "struct thermal_bank_cfg".
>>
>> - Fix "mtk_thermal" variable in mtk_read_temp().
>>
>> - Set "mt->raw_to_mcelsius" in probe().
>>
>>
>> For zones 1, 2 and 3 we can later add a different driver specific for dev/debug to probe them to
>>
>> avoid confusion.
>
> You can add them in the driver and in the device tree, but just add the cooling device for the thermal zone 0.

Thermal zone 0 uses CPU{0..3} for passive cooling, in this case we should register cooling device with

cpufreq_cooling_register() for each CPU right ?

>
>

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

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Daniel Lezcano]

On 24/01/2023 18:46, Amjad Ouled-Ameur wrote:
> 
> On 1/24/23 17:54, Daniel Lezcano wrote:
>>
>> Hi Amjad,
>>
>> On 24/01/2023 11:08, Amjad Ouled-Ameur wrote:
>>
>> [ ... ]
>>
>>>>>
>>>>> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 
>>>>> 1 x 4 Little, right ?
>>>>
>>>> MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per sensor. 
>>>> Thermal zone 0 corresponds
>>>>
>>>> to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has 
>>>> nothing to do with CPUs. The cooling device type
>>>>
>>>> used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are present 
>>>> in the SoC for debug-purpose only, they are not supposed
>>>>
>>>> to be used for production.
>>>>
>>> After reconsidering the fact that zones 1, 2 and 3 are only used for 
>>> dev/debug, it might be best to avo >
>>> aggregation as you suggested, and keep only support for zone 0 in 
>>> this driver. Thus I suggest I send a V8
>>>
>>> where I keep only below fixes for this patch if that's okay with you:
>>>
>>> - Define "raw_to_mcelsius" function pointer for "struct 
>>> thermal_bank_cfg".
>>>
>>> - Fix "mtk_thermal" variable in mtk_read_temp().
>>>
>>> - Set "mt->raw_to_mcelsius" in probe().
>>>
>>>
>>> For zones 1, 2 and 3 we can later add a different driver specific for 
>>> dev/debug to probe them to
>>>
>>> avoid confusion.
>>
>> You can add them in the driver and in the device tree, but just add 
>> the cooling device for the thermal zone 0.
> 
> Thermal zone 0 uses CPU{0..3} for passive cooling, in this case we 
> should register cooling device with
> 
> cpufreq_cooling_register() for each CPU right ?

No, the OF code device tree does already that. You just have to register 
the different thermal zones.

Do you have a pointer to a device tree for this board and the thermal 
setup ?


-- 
<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 v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Daniel Lezcano]

On 24/01/2023 18:46, Amjad Ouled-Ameur wrote:
> 
> On 1/24/23 17:54, Daniel Lezcano wrote:
>>
>> Hi Amjad,
>>
>> On 24/01/2023 11:08, Amjad Ouled-Ameur wrote:
>>
>> [ ... ]
>>
>>>>>
>>>>> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 
>>>>> 1 x 4 Little, right ?
>>>>
>>>> MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per sensor. 
>>>> Thermal zone 0 corresponds
>>>>
>>>> to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has 
>>>> nothing to do with CPUs. The cooling device type
>>>>
>>>> used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are present 
>>>> in the SoC for debug-purpose only, they are not supposed
>>>>
>>>> to be used for production.
>>>>
>>> After reconsidering the fact that zones 1, 2 and 3 are only used for 
>>> dev/debug, it might be best to avo >
>>> aggregation as you suggested, and keep only support for zone 0 in 
>>> this driver. Thus I suggest I send a V8
>>>
>>> where I keep only below fixes for this patch if that's okay with you:
>>>
>>> - Define "raw_to_mcelsius" function pointer for "struct 
>>> thermal_bank_cfg".
>>>
>>> - Fix "mtk_thermal" variable in mtk_read_temp().
>>>
>>> - Set "mt->raw_to_mcelsius" in probe().
>>>
>>>
>>> For zones 1, 2 and 3 we can later add a different driver specific for 
>>> dev/debug to probe them to
>>>
>>> avoid confusion.
>>
>> You can add them in the driver and in the device tree, but just add 
>> the cooling device for the thermal zone 0.
> 
> Thermal zone 0 uses CPU{0..3} for passive cooling, in this case we 
> should register cooling device with
> 
> cpufreq_cooling_register() for each CPU right ?

No, the OF code device tree does already that. You just have to register 
the different thermal zones.

Do you have a pointer to a device tree for this board and the thermal 
setup ?


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


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

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

On 1/24/23 18:55, Daniel Lezcano wrote:
> On 24/01/2023 18:46, Amjad Ouled-Ameur wrote:
>>
>> On 1/24/23 17:54, Daniel Lezcano wrote:
>>>
>>> Hi Amjad,
>>>
>>> On 24/01/2023 11:08, Amjad Ouled-Ameur wrote:
>>>
>>> [ ... ]
>>>
>>>>>>
>>>>>> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 x 4 Little, right ?
>>>>>
>>>>> MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per sensor. Thermal zone 0 corresponds
>>>>>
>>>>> to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has nothing to do with CPUs. The cooling device type
>>>>>
>>>>> used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are present in the SoC for debug-purpose only, they are not supposed
>>>>>
>>>>> to be used for production.
>>>>>
>>>> After reconsidering the fact that zones 1, 2 and 3 are only used for dev/debug, it might be best to avo >
>>>> aggregation as you suggested, and keep only support for zone 0 in this driver. Thus I suggest I send a V8
>>>>
>>>> where I keep only below fixes for this patch if that's okay with you:
>>>>
>>>> - Define "raw_to_mcelsius" function pointer for "struct thermal_bank_cfg".
>>>>
>>>> - Fix "mtk_thermal" variable in mtk_read_temp().
>>>>
>>>> - Set "mt->raw_to_mcelsius" in probe().
>>>>
>>>>
>>>> For zones 1, 2 and 3 we can later add a different driver specific for dev/debug to probe them to
>>>>
>>>> avoid confusion.
>>>
>>> You can add them in the driver and in the device tree, but just add the cooling device for the thermal zone 0.
>>
>> Thermal zone 0 uses CPU{0..3} for passive cooling, in this case we should register cooling device with
>>
>> cpufreq_cooling_register() for each CPU right ?
>
> No, the OF code device tree does already that. You just have to register the different thermal zones.
>
> Do you have a pointer to a device tree for this board and the thermal setup ?

Sure, here is a dtsi for MT8365 SoC which contains thermal nodes [0].

[0]: https://lore.kernel.org/linux-arm-kernel/20220531135026.238475-17-fparent@baylibre.com/#Z31arch:arm64:boot:dts:mediatek:mt8365.dtsi

>
>

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

On 1/24/23 18:55, Daniel Lezcano wrote:
> On 24/01/2023 18:46, Amjad Ouled-Ameur wrote:
>>
>> On 1/24/23 17:54, Daniel Lezcano wrote:
>>>
>>> Hi Amjad,
>>>
>>> On 24/01/2023 11:08, Amjad Ouled-Ameur wrote:
>>>
>>> [ ... ]
>>>
>>>>>>
>>>>>> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 x 4 Little, right ?
>>>>>
>>>>> MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per sensor. Thermal zone 0 corresponds
>>>>>
>>>>> to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has nothing to do with CPUs. The cooling device type
>>>>>
>>>>> used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are present in the SoC for debug-purpose only, they are not supposed
>>>>>
>>>>> to be used for production.
>>>>>
>>>> After reconsidering the fact that zones 1, 2 and 3 are only used for dev/debug, it might be best to avo >
>>>> aggregation as you suggested, and keep only support for zone 0 in this driver. Thus I suggest I send a V8
>>>>
>>>> where I keep only below fixes for this patch if that's okay with you:
>>>>
>>>> - Define "raw_to_mcelsius" function pointer for "struct thermal_bank_cfg".
>>>>
>>>> - Fix "mtk_thermal" variable in mtk_read_temp().
>>>>
>>>> - Set "mt->raw_to_mcelsius" in probe().
>>>>
>>>>
>>>> For zones 1, 2 and 3 we can later add a different driver specific for dev/debug to probe them to
>>>>
>>>> avoid confusion.
>>>
>>> You can add them in the driver and in the device tree, but just add the cooling device for the thermal zone 0.
>>
>> Thermal zone 0 uses CPU{0..3} for passive cooling, in this case we should register cooling device with
>>
>> cpufreq_cooling_register() for each CPU right ?
>
> No, the OF code device tree does already that. You just have to register the different thermal zones.
>
> Do you have a pointer to a device tree for this board and the thermal setup ?

Sure, here is a dtsi for MT8365 SoC which contains thermal nodes [0].

[0]: https://lore.kernel.org/linux-arm-kernel/20220531135026.238475-17-fparent@baylibre.com/#Z31arch:arm64:boot:dts:mediatek:mt8365.dtsi

>
>

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

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Daniel Lezcano]

On 24/01/2023 23:27, Amjad Ouled-Ameur wrote:
> 
> On 1/24/23 18:55, Daniel Lezcano wrote:
>> On 24/01/2023 18:46, Amjad Ouled-Ameur wrote:
>>>
>>> On 1/24/23 17:54, Daniel Lezcano wrote:
>>>>
>>>> Hi Amjad,
>>>>
>>>> On 24/01/2023 11:08, Amjad Ouled-Ameur wrote:
>>>>
>>>> [ ... ]
>>>>
>>>>>>>
>>>>>>> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 
>>>>>>> 2Bigs, 1 x 4 Little, right ?
>>>>>>
>>>>>> MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per 
>>>>>> sensor. Thermal zone 0 corresponds
>>>>>>
>>>>>> to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has 
>>>>>> nothing to do with CPUs. The cooling device type
>>>>>>
>>>>>> used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are 
>>>>>> present in the SoC for debug-purpose only, they are not supposed
>>>>>>
>>>>>> to be used for production.
>>>>>>
>>>>> After reconsidering the fact that zones 1, 2 and 3 are only used 
>>>>> for dev/debug, it might be best to avo >
>>>>> aggregation as you suggested, and keep only support for zone 0 in 
>>>>> this driver. Thus I suggest I send a V8
>>>>>
>>>>> where I keep only below fixes for this patch if that's okay with you:
>>>>>
>>>>> - Define "raw_to_mcelsius" function pointer for "struct 
>>>>> thermal_bank_cfg".
>>>>>
>>>>> - Fix "mtk_thermal" variable in mtk_read_temp().
>>>>>
>>>>> - Set "mt->raw_to_mcelsius" in probe().
>>>>>
>>>>>
>>>>> For zones 1, 2 and 3 we can later add a different driver specific 
>>>>> for dev/debug to probe them to
>>>>>
>>>>> avoid confusion.
>>>>
>>>> You can add them in the driver and in the device tree, but just add 
>>>> the cooling device for the thermal zone 0.
>>>
>>> Thermal zone 0 uses CPU{0..3} for passive cooling, in this case we 
>>> should register cooling device with
>>>
>>> cpufreq_cooling_register() for each CPU right ?
>>
>> No, the OF code device tree does already that. You just have to 
>> register the different thermal zones.
>>
>> Do you have a pointer to a device tree for this board and the thermal 
>> setup ?
> 
> Sure, here is a dtsi for MT8365 SoC which contains thermal nodes [0].

 From my POV, there are two different setup with the DT but only one 
implementation with the driver.

The driver should register all the thermal zones for each CPUs. For 
that, make things nice with the defines for the dt-bindings like [1].

Then the device tree:

setup1:

Describe all the thermal zones in the DT which will be similar to [2]. 
Each CPU has a thermal zone, trip points and the same cooling device.

The first thermal zone reaching the trip temperature threshold will 
start the mitigation. The thermal framework takes care of multiple 
thermal zones sharing the same cooling device.

The result will be the same as the max temperature aggregation you did 
previously.

setup2:

Describe all the thermal zones in the DT [3], but add the cooling device 
only on the sensor you are interested in mitigating.


And finally, if the sensors should be used to describe a kind of 
temperature gradient, a linear equation could be used but that is not 
implemented yet in the thermal framework.

Hope that helps

   -- Daniel

[1] 
https://lore.kernel.org/linux-arm-kernel/5dd5c795-5e67-146d-7132-30fc90171d4c@collabora.com/T/#Z2e.:..:20230124131717.128660-3-bchihi::40baylibre.com:1include:dt-bindings:thermal:mediatek-lvts.h

[2] 
https://lore.kernel.org/linux-arm-kernel/5dd5c795-5e67-146d-7132-30fc90171d4c@collabora.com/T/#m303240c4da71f6f37831e5d1b6f3da771ae8dd90

[3] 
https://lore.kernel.org/linux-arm-kernel/5dd5c795-5e67-146d-7132-30fc90171d4c@collabora.com/T/#Z2e.:..:20230124131717.128660-6-bchihi::40baylibre.com:1arch:arm64:boot:dts:mediatek:mt8195.dtsi


-- 
<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 v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Daniel Lezcano]

On 24/01/2023 23:27, Amjad Ouled-Ameur wrote:
> 
> On 1/24/23 18:55, Daniel Lezcano wrote:
>> On 24/01/2023 18:46, Amjad Ouled-Ameur wrote:
>>>
>>> On 1/24/23 17:54, Daniel Lezcano wrote:
>>>>
>>>> Hi Amjad,
>>>>
>>>> On 24/01/2023 11:08, Amjad Ouled-Ameur wrote:
>>>>
>>>> [ ... ]
>>>>
>>>>>>>
>>>>>>> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 
>>>>>>> 2Bigs, 1 x 4 Little, right ?
>>>>>>
>>>>>> MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per 
>>>>>> sensor. Thermal zone 0 corresponds
>>>>>>
>>>>>> to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has 
>>>>>> nothing to do with CPUs. The cooling device type
>>>>>>
>>>>>> used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are 
>>>>>> present in the SoC for debug-purpose only, they are not supposed
>>>>>>
>>>>>> to be used for production.
>>>>>>
>>>>> After reconsidering the fact that zones 1, 2 and 3 are only used 
>>>>> for dev/debug, it might be best to avo >
>>>>> aggregation as you suggested, and keep only support for zone 0 in 
>>>>> this driver. Thus I suggest I send a V8
>>>>>
>>>>> where I keep only below fixes for this patch if that's okay with you:
>>>>>
>>>>> - Define "raw_to_mcelsius" function pointer for "struct 
>>>>> thermal_bank_cfg".
>>>>>
>>>>> - Fix "mtk_thermal" variable in mtk_read_temp().
>>>>>
>>>>> - Set "mt->raw_to_mcelsius" in probe().
>>>>>
>>>>>
>>>>> For zones 1, 2 and 3 we can later add a different driver specific 
>>>>> for dev/debug to probe them to
>>>>>
>>>>> avoid confusion.
>>>>
>>>> You can add them in the driver and in the device tree, but just add 
>>>> the cooling device for the thermal zone 0.
>>>
>>> Thermal zone 0 uses CPU{0..3} for passive cooling, in this case we 
>>> should register cooling device with
>>>
>>> cpufreq_cooling_register() for each CPU right ?
>>
>> No, the OF code device tree does already that. You just have to 
>> register the different thermal zones.
>>
>> Do you have a pointer to a device tree for this board and the thermal 
>> setup ?
> 
> Sure, here is a dtsi for MT8365 SoC which contains thermal nodes [0].

 From my POV, there are two different setup with the DT but only one 
implementation with the driver.

The driver should register all the thermal zones for each CPUs. For 
that, make things nice with the defines for the dt-bindings like [1].

Then the device tree:

setup1:

Describe all the thermal zones in the DT which will be similar to [2]. 
Each CPU has a thermal zone, trip points and the same cooling device.

The first thermal zone reaching the trip temperature threshold will 
start the mitigation. The thermal framework takes care of multiple 
thermal zones sharing the same cooling device.

The result will be the same as the max temperature aggregation you did 
previously.

setup2:

Describe all the thermal zones in the DT [3], but add the cooling device 
only on the sensor you are interested in mitigating.


And finally, if the sensors should be used to describe a kind of 
temperature gradient, a linear equation could be used but that is not 
implemented yet in the thermal framework.

Hope that helps

   -- Daniel

[1] 
https://lore.kernel.org/linux-arm-kernel/5dd5c795-5e67-146d-7132-30fc90171d4c@collabora.com/T/#Z2e.:..:20230124131717.128660-3-bchihi::40baylibre.com:1include:dt-bindings:thermal:mediatek-lvts.h

[2] 
https://lore.kernel.org/linux-arm-kernel/5dd5c795-5e67-146d-7132-30fc90171d4c@collabora.com/T/#m303240c4da71f6f37831e5d1b6f3da771ae8dd90

[3] 
https://lore.kernel.org/linux-arm-kernel/5dd5c795-5e67-146d-7132-30fc90171d4c@collabora.com/T/#Z2e.:..:20230124131717.128660-6-bchihi::40baylibre.com:1arch:arm64:boot:dts:mediatek:mt8195.dtsi


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


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

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

On 1/25/23 11:02, Daniel Lezcano wrote:
> On 24/01/2023 23:27, Amjad Ouled-Ameur wrote:
>>
>> On 1/24/23 18:55, Daniel Lezcano wrote:
>>> On 24/01/2023 18:46, Amjad Ouled-Ameur wrote:
>>>>
>>>> On 1/24/23 17:54, Daniel Lezcano wrote:
>>>>>
>>>>> Hi Amjad,
>>>>>
>>>>> On 24/01/2023 11:08, Amjad Ouled-Ameur wrote:
>>>>>
>>>>> [ ... ]
>>>>>
>>>>>>>>
>>>>>>>> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 x 4 Little, right ?
>>>>>>>
>>>>>>> MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per sensor. Thermal zone 0 corresponds
>>>>>>>
>>>>>>> to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has nothing to do with CPUs. The cooling device type
>>>>>>>
>>>>>>> used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are present in the SoC for debug-purpose only, they are not supposed
>>>>>>>
>>>>>>> to be used for production.
>>>>>>>
>>>>>> After reconsidering the fact that zones 1, 2 and 3 are only used for dev/debug, it might be best to avo >
>>>>>> aggregation as you suggested, and keep only support for zone 0 in this driver. Thus I suggest I send a V8
>>>>>>
>>>>>> where I keep only below fixes for this patch if that's okay with you:
>>>>>>
>>>>>> - Define "raw_to_mcelsius" function pointer for "struct thermal_bank_cfg".
>>>>>>
>>>>>> - Fix "mtk_thermal" variable in mtk_read_temp().
>>>>>>
>>>>>> - Set "mt->raw_to_mcelsius" in probe().
>>>>>>
>>>>>>
>>>>>> For zones 1, 2 and 3 we can later add a different driver specific for dev/debug to probe them to
>>>>>>
>>>>>> avoid confusion.
>>>>>
>>>>> You can add them in the driver and in the device tree, but just add the cooling device for the thermal zone 0.
>>>>
>>>> Thermal zone 0 uses CPU{0..3} for passive cooling, in this case we should register cooling device with
>>>>
>>>> cpufreq_cooling_register() for each CPU right ?
>>>
>>> No, the OF code device tree does already that. You just have to register the different thermal zones.
>>>
>>> Do you have a pointer to a device tree for this board and the thermal setup ?
>>
>> Sure, here is a dtsi for MT8365 SoC which contains thermal nodes [0].
>
> From my POV, there are two different setup with the DT but only one implementation with the driver.
>
> The driver should register all the thermal zones for each CPUs. For that, make things nice with the defines for the dt-bindings like [1].
>
> Then the device tree:
>
> setup1:
>
> Describe all the thermal zones in the DT which will be similar to [2]. Each CPU has a thermal zone, trip points and the same cooling device.
>
> The first thermal zone reaching the trip temperature threshold will start the mitigation. The thermal framework takes care of multiple thermal zones sharing the same cooling device.
>
> The result will be the same as the max temperature aggregation you did previously.
>
> setup2:
>
> Describe all the thermal zones in the DT [3], but add the cooling device only on the sensor you are interested in mitigating.
>
>
> And finally, if the sensors should be used to describe a kind of temperature gradient, a linear equation could be used but that is not implemented yet in the thermal framework.
>
> Hope that helps
>
Thank you Daniel for the thorough insights.

FYI, MT8195 SoC you referred to has a different thermal architecture and sensor mapping than on MT8365. The latter has only one

useful thermal zone corresponding to 4 CPUs at once. The other 3 thermal zones have no real purpose and will probably

not be defined in dtsi. Thus I sent a V8 of this series to keep only support for thermal zone 0 of interest.

Kindly,

Amjad

>   -- Daniel
>
> [1] https://lore.kernel.org/linux-arm-kernel/5dd5c795-5e67-146d-7132-30fc90171d4c@collabora.com/T/#Z2e.:..:20230124131717.128660-3-bchihi::40baylibre.com:1include:dt-bindings:thermal:mediatek-lvts.h
>
> [2] https://lore.kernel.org/linux-arm-kernel/5dd5c795-5e67-146d-7132-30fc90171d4c@collabora.com/T/#m303240c4da71f6f37831e5d1b6f3da771ae8dd90
>
> [3] https://lore.kernel.org/linux-arm-kernel/5dd5c795-5e67-146d-7132-30fc90171d4c@collabora.com/T/#Z2e.:..:20230124131717.128660-6-bchihi::40baylibre.com:1arch:arm64:boot:dts:mediatek:mt8195.dtsi
>
>

Re: [PATCH v7 4/4] thermal: mediatek: add another get_temp ops for thermal sensors

[Amjad Ouled-Ameur]

On 1/25/23 11:02, Daniel Lezcano wrote:
> On 24/01/2023 23:27, Amjad Ouled-Ameur wrote:
>>
>> On 1/24/23 18:55, Daniel Lezcano wrote:
>>> On 24/01/2023 18:46, Amjad Ouled-Ameur wrote:
>>>>
>>>> On 1/24/23 17:54, Daniel Lezcano wrote:
>>>>>
>>>>> Hi Amjad,
>>>>>
>>>>> On 24/01/2023 11:08, Amjad Ouled-Ameur wrote:
>>>>>
>>>>> [ ... ]
>>>>>
>>>>>>>>
>>>>>>>> IIUC, there is a sensor per couple of cores. 1 x 2Bigs, 1 x 2Bigs, 1 x 4 Little, right ?
>>>>>>>
>>>>>>> MT8365 SoC has 4 x A53 CPUs. The SoC has 4 thermal zones per sensor. Thermal zone 0 corresponds
>>>>>>>
>>>>>>> to all 4 x A53 CPUs, the other thermal zones (1, 2 and 3) has nothing to do with CPUs. The cooling device type
>>>>>>>
>>>>>>> used for CPUs is passive. FYI, thermal zones 1, 2 and 3 are present in the SoC for debug-purpose only, they are not supposed
>>>>>>>
>>>>>>> to be used for production.
>>>>>>>
>>>>>> After reconsidering the fact that zones 1, 2 and 3 are only used for dev/debug, it might be best to avo >
>>>>>> aggregation as you suggested, and keep only support for zone 0 in this driver. Thus I suggest I send a V8
>>>>>>
>>>>>> where I keep only below fixes for this patch if that's okay with you:
>>>>>>
>>>>>> - Define "raw_to_mcelsius" function pointer for "struct thermal_bank_cfg".
>>>>>>
>>>>>> - Fix "mtk_thermal" variable in mtk_read_temp().
>>>>>>
>>>>>> - Set "mt->raw_to_mcelsius" in probe().
>>>>>>
>>>>>>
>>>>>> For zones 1, 2 and 3 we can later add a different driver specific for dev/debug to probe them to
>>>>>>
>>>>>> avoid confusion.
>>>>>
>>>>> You can add them in the driver and in the device tree, but just add the cooling device for the thermal zone 0.
>>>>
>>>> Thermal zone 0 uses CPU{0..3} for passive cooling, in this case we should register cooling device with
>>>>
>>>> cpufreq_cooling_register() for each CPU right ?
>>>
>>> No, the OF code device tree does already that. You just have to register the different thermal zones.
>>>
>>> Do you have a pointer to a device tree for this board and the thermal setup ?
>>
>> Sure, here is a dtsi for MT8365 SoC which contains thermal nodes [0].
>
> From my POV, there are two different setup with the DT but only one implementation with the driver.
>
> The driver should register all the thermal zones for each CPUs. For that, make things nice with the defines for the dt-bindings like [1].
>
> Then the device tree:
>
> setup1:
>
> Describe all the thermal zones in the DT which will be similar to [2]. Each CPU has a thermal zone, trip points and the same cooling device.
>
> The first thermal zone reaching the trip temperature threshold will start the mitigation. The thermal framework takes care of multiple thermal zones sharing the same cooling device.
>
> The result will be the same as the max temperature aggregation you did previously.
>
> setup2:
>
> Describe all the thermal zones in the DT [3], but add the cooling device only on the sensor you are interested in mitigating.
>
>
> And finally, if the sensors should be used to describe a kind of temperature gradient, a linear equation could be used but that is not implemented yet in the thermal framework.
>
> Hope that helps
>
Thank you Daniel for the thorough insights.

FYI, MT8195 SoC you referred to has a different thermal architecture and sensor mapping than on MT8365. The latter has only one

useful thermal zone corresponding to 4 CPUs at once. The other 3 thermal zones have no real purpose and will probably

not be defined in dtsi. Thus I sent a V8 of this series to keep only support for thermal zone 0 of interest.

Kindly,

Amjad

>   -- Daniel
>
> [1] https://lore.kernel.org/linux-arm-kernel/5dd5c795-5e67-146d-7132-30fc90171d4c@collabora.com/T/#Z2e.:..:20230124131717.128660-3-bchihi::40baylibre.com:1include:dt-bindings:thermal:mediatek-lvts.h
>
> [2] https://lore.kernel.org/linux-arm-kernel/5dd5c795-5e67-146d-7132-30fc90171d4c@collabora.com/T/#m303240c4da71f6f37831e5d1b6f3da771ae8dd90
>
> [3] https://lore.kernel.org/linux-arm-kernel/5dd5c795-5e67-146d-7132-30fc90171d4c@collabora.com/T/#Z2e.:..:20230124131717.128660-6-bchihi::40baylibre.com:1arch:arm64:boot:dts:mediatek:mt8195.dtsi
>
>

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