Skip to contentSkip to conversation

add LVTS support for mt7988

PATCH Superseded Frank Wunderlich 4 participants 13 messages Sep 20, 2023 – Sep 21, 2023

Subsystems (3)
Device tree bindings Arm MediaTek Thermal

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Frank Wunderlich

author
#0 lore raw

v2 cover add LVTS support for mt7988

From: Frank Wunderlich <[email protected]>

This series makes allows soc specific temperature coefficients and adds support for mt7988 which has a different one.

Frank Wunderlich (4):
dt-bindings: thermal: mediatek: add mt7988 lvts compatible
dt-bindings: thermal: mediatek: Add LVTS thermal sensors for mt7988 thermal/drivers/mediatek/lvts_thermal: make coeff configurable thermal/drivers/mediatek/lvts_thermal: add mt7988 support

 .../thermal/mediatek,lvts-thermal.yaml        |  1 +
 drivers/thermal/mediatek/lvts_thermal.c       | 97 +++++++++++++++----
 .../thermal/mediatek,lvts-thermal.h           |  9 ++
 3 files changed, 90 insertions(+), 17 deletions(-)
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH v2 1/4 dt-bindings: thermal: mediatek: add mt7988 lvts compatible

From: Frank Wunderlich <[email protected]>

Add compatible string for mt7988 lvts application processor.

Signed-off-by Frank Wunderlich <[email protected]>

---
v2:
- change mt7988-lvts to mt7988-lvts-ap (Application Processor)
- not added Ack from Rob because of this change

---
 .../devicetree/bindings/thermal/mediatek,lvts-thermal.yaml       | 1 +
 1 file changed, 1 insertion(+)
Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml +1 -0
index fe9ae4c425c0..e6665af52ee6 100644
@@ -18,6 +18,7 @@ description: |
 properties:
   compatible:
     enum:
+      - mediatek,mt7988-lvts-ap
       - mediatek,mt8192-lvts-ap
       - mediatek,mt8192-lvts-mcu
       - mediatek,mt8195-lvts-ap
Signature · 1 line
2.34.1
maintainer Reviewed-by on v2 1/4
#2 lore raw

Re: [PATCH v2 1/4] dt-bindings: thermal: mediatek: add mt7988 lvts compatible

Il 20/09/23 19:49, Frank Wunderlich ha scritto:

Quoted 5 lines

From: Frank Wunderlich <[email protected]>

Add compatible string for mt7988 lvts application processor.

Signed-off-by Frank Wunderlich <[email protected]>
Reviewed-by AngeloGioacchino Del Regno <[email protected]>
Quoted 20 lines

---
v2:
- change mt7988-lvts to mt7988-lvts-ap (Application Processor)
- not added Ack from Rob because of this change

---
  .../devicetree/bindings/thermal/mediatek,lvts-thermal.yaml       | 1 +
  1 file changed, 1 insertion(+)
Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml +1 -0
index fe9ae4c425c0..e6665af52ee6 100644
@@ -18,6 +18,7 @@ description: |
  properties:
    compatible:
      enum:
+      - mediatek,mt7988-lvts-ap
        - mediatek,mt8192-lvts-ap
        - mediatek,mt8192-lvts-mcu
        - mediatek,mt8195-lvts-ap
author
#4 lore raw

PATCH v2 2/4 dt-bindings: thermal: mediatek: Add LVTS thermal sensors for mt7988

In reply to Frank Wunderlich

From: Frank Wunderlich <[email protected]>

Add sensor constants for MT7988.

Signed-off-by Frank Wunderlich <[email protected]>

---
v2:
- new patch (moved from driver code to binding header)
- give sensors more meaningful names

---
 include/dt-bindings/thermal/mediatek,lvts-thermal.h | 9 +++++++++
 1 file changed, 9 insertions(+)
include/dt-bindings/thermal/mediatek,lvts-thermal.h +9 -0
index 8fa5a46675c4..8c1fdc18cf34 100644
@@ -7,6 +7,15 @@
 #ifndef __MEDIATEK_LVTS_DT_H
 #define __MEDIATEK_LVTS_DT_H
 
+#define MT7988_CPU_0		0
+#define MT7988_CPU_1		1
+#define MT7988_ETH2P5G_0	2
+#define MT7988_ETH2P5G_1	3
+#define MT7988_TOPS_0		4
+#define MT7988_TOPS_1		5
+#define MT7988_ETHWARP_0	6
+#define MT7988_ETHWARP_1	7
+
 #define MT8195_MCU_BIG_CPU0     0
 #define MT8195_MCU_BIG_CPU1     1
 #define MT8195_MCU_BIG_CPU2     2
Signature · 1 line
2.34.1
maintainer Reviewed-by on v2 2/4
#5 lore raw

Re: [PATCH v2 2/4] dt-bindings: thermal: mediatek: Add LVTS thermal sensors for mt7988

Il 20/09/23 19:49, Frank Wunderlich ha scritto:

Quoted 5 lines

From: Frank Wunderlich <[email protected]>

Add sensor constants for MT7988.

Signed-off-by Frank Wunderlich <[email protected]>
Reviewed-by AngeloGioacchino Del Regno <[email protected]>
Quoted 28 lines

---
v2:
- new patch (moved from driver code to binding header)
- give sensors more meaningful names

---
  include/dt-bindings/thermal/mediatek,lvts-thermal.h | 9 +++++++++
  1 file changed, 9 insertions(+)
include/dt-bindings/thermal/mediatek,lvts-thermal.h +9 -0
index 8fa5a46675c4..8c1fdc18cf34 100644
@@ -7,6 +7,15 @@
  #ifndef __MEDIATEK_LVTS_DT_H
  #define __MEDIATEK_LVTS_DT_H
 
+#define MT7988_CPU_0		0
+#define MT7988_CPU_1		1
+#define MT7988_ETH2P5G_0	2
+#define MT7988_ETH2P5G_1	3
+#define MT7988_TOPS_0		4
+#define MT7988_TOPS_1		5
+#define MT7988_ETHWARP_0	6
+#define MT7988_ETHWARP_1	7
+
  #define MT8195_MCU_BIG_CPU0     0
  #define MT8195_MCU_BIG_CPU1     1
  #define MT8195_MCU_BIG_CPU2     2
author
#7 lore raw

PATCH v2 3/4 thermal/drivers/mediatek/lvts_thermal: make coeff configurable

In reply to Frank Wunderlich

From: Frank Wunderlich <[email protected]>

The upcoming mt7988 has different temperature coefficients so we cannot use constants in the functions lvts_golden_temp_init, lvts_golden_temp_init and lvts_raw_to_temp anymore.

Add a field in the lvts_ctrl pointing to the lvts_data which now contains the soc-specific temperature coefficents.

To make the code better readable, rename static int coeff_b to golden_temp_offset, COEFF_A to temp_factor and COEFF_B to temp_offset.

Signed-off-by Frank Wunderlich <[email protected]>

---
v2:
- rename static int coeff_b to golden_temp_offset
- rename coeff.a to temp_factor and coeff.b to temp_offset

---
 drivers/thermal/mediatek/lvts_thermal.c | 51 ++++++++++++++++---------
 1 file changed, 34 insertions(+), 17 deletions(-)
drivers/thermal/mediatek/lvts_thermal.c +34 -17
index effd9b00a424..c2669f405a94 100644
@@ -80,8 +80,8 @@
 #define LVTS_SENSOR_MAX				4
 #define LVTS_GOLDEN_TEMP_MAX		62
 #define LVTS_GOLDEN_TEMP_DEFAULT	50
-#define LVTS_COEFF_A				-250460
-#define LVTS_COEFF_B				250460
+#define LVTS_COEFF_A_MT8195			-250460
+#define LVTS_COEFF_B_MT8195			250460
 
 #define LVTS_MSR_IMMEDIATE_MODE		0
 #define LVTS_MSR_FILTERED_MODE		1

@@ -94,7 +94,7 @@
 #define LVTS_MINIMUM_THRESHOLD		20000
 
 static int golden_temp = LVTS_GOLDEN_TEMP_DEFAULT;
-static int coeff_b = LVTS_COEFF_B;
+static int golden_temp_offset;
 
 struct lvts_sensor_data {
 	int dt_id;

@@ -112,6 +112,8 @@ struct lvts_ctrl_data {
 struct lvts_data {
 	const struct lvts_ctrl_data *lvts_ctrl;
 	int num_lvts_ctrl;
+	int temp_factor;
+	int temp_offset;
 };
 
 struct lvts_sensor {

@@ -126,6 +128,7 @@ struct lvts_sensor {
 
 struct lvts_ctrl {
 	struct lvts_sensor sensors[LVTS_SENSOR_MAX];
+	const struct lvts_data *lvts_data;
 	u32 calibration[LVTS_SENSOR_MAX];
 	u32 hw_tshut_raw_temp;
 	int num_lvts_sensor;

@@ -247,21 +250,21 @@ static void lvts_debugfs_exit(struct lvts_domain *lvts_td) { }
 
 #endif
 
-static int lvts_raw_to_temp(u32 raw_temp)
+static int lvts_raw_to_temp(u32 raw_temp, int temp_factor)
 {
 	int temperature;
 
-	temperature = ((s64)(raw_temp & 0xFFFF) * LVTS_COEFF_A) >> 14;
-	temperature += coeff_b;
+	temperature = ((s64)(raw_temp & 0xFFFF) * temp_factor) >> 14;
+	temperature += golden_temp_offset;
 
 	return temperature;
 }
 
-static u32 lvts_temp_to_raw(int temperature)
+static u32 lvts_temp_to_raw(int temperature, int temp_factor)
 {
-	u32 raw_temp = ((s64)(coeff_b - temperature)) << 14;
+	u32 raw_temp = ((s64)(golden_temp_offset - temperature)) << 14;
 
-	raw_temp = div_s64(raw_temp, -LVTS_COEFF_A);
+	raw_temp = div_s64(raw_temp, -temp_factor);
 
 	return raw_temp;
 }

@@ -269,6 +272,9 @@ static u32 lvts_temp_to_raw(int temperature)
 static int lvts_get_temp(struct thermal_zone_device *tz, int *temp)
 {
 	struct lvts_sensor *lvts_sensor = thermal_zone_device_priv(tz);
+	struct lvts_ctrl *lvts_ctrl = container_of(lvts_sensor, struct lvts_ctrl,
+						   sensors[lvts_sensor->id]);
+	const struct lvts_data *lvts_data = lvts_ctrl->lvts_data;
 	void __iomem *msr = lvts_sensor->msr;
 	u32 value;
 	int rc;

@@ -301,7 +307,7 @@ static int lvts_get_temp(struct thermal_zone_device *tz, int *temp)
 	if (rc)
 		return -EAGAIN;
 
-	*temp = lvts_raw_to_temp(value & 0xFFFF);
+	*temp = lvts_raw_to_temp(value & 0xFFFF, lvts_data->temp_factor);
 
 	return 0;
 }

@@ -348,10 +354,13 @@ static bool lvts_should_update_thresh(struct lvts_ctrl *lvts_ctrl, int high)
 static int lvts_set_trips(struct thermal_zone_device *tz, int low, int high)
 {
 	struct lvts_sensor *lvts_sensor = thermal_zone_device_priv(tz);
-	struct lvts_ctrl *lvts_ctrl = container_of(lvts_sensor, struct lvts_ctrl, sensors[lvts_sensor->id]);
+	struct lvts_ctrl *lvts_ctrl = container_of(lvts_sensor, struct lvts_ctrl,
+						   sensors[lvts_sensor->id]);
+	const struct lvts_data *lvts_data = lvts_ctrl->lvts_data;
 	void __iomem *base = lvts_sensor->base;
-	u32 raw_low = lvts_temp_to_raw(low != -INT_MAX ? low : LVTS_MINIMUM_THRESHOLD);
-	u32 raw_high = lvts_temp_to_raw(high);
+	u32 raw_low = lvts_temp_to_raw(low != -INT_MAX ? low : LVTS_MINIMUM_THRESHOLD,
+				       lvts_data->temp_factor);
+	u32 raw_high = lvts_temp_to_raw(high, lvts_data->temp_factor);
 	bool should_update_thresh;
 
 	lvts_sensor->low_thresh = low;

@@ -692,7 +701,7 @@ static int lvts_calibration_read(struct device *dev, struct lvts_domain *lvts_td
 	return 0;
 }
 
-static int lvts_golden_temp_init(struct device *dev, u32 *value)
+static int lvts_golden_temp_init(struct device *dev, u32 *value, int temp_offset)
 {
 	u32 gt;
 

@@ -701,7 +710,7 @@ static int lvts_golden_temp_init(struct device *dev, u32 *value)
 	if (gt && gt < LVTS_GOLDEN_TEMP_MAX)
 		golden_temp = gt;
 
-	coeff_b = golden_temp * 500 + LVTS_COEFF_B;
+	golden_temp_offset = golden_temp * 500 + temp_offset;
 
 	return 0;
 }

@@ -724,7 +733,7 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td,
 	 * The golden temp information is contained in the first chunk
 	 * of efuse data.
 	 */
-	ret = lvts_golden_temp_init(dev, (u32 *)lvts_td->calib);
+	ret = lvts_golden_temp_init(dev, (u32 *)lvts_td->calib, lvts_data->temp_offset);
 	if (ret)
 		return ret;
 

@@ -735,6 +744,7 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td,
 	for (i = 0; i < lvts_data->num_lvts_ctrl; i++) {
 
 		lvts_ctrl[i].base = lvts_td->base + lvts_data->lvts_ctrl[i].offset;
+		lvts_ctrl[i].lvts_data = lvts_data;
 
 		ret = lvts_sensor_init(dev, &lvts_ctrl[i],
 				       &lvts_data->lvts_ctrl[i]);

@@ -758,7 +768,8 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td,
 		 * after initializing the calibration.
 		 */
 		lvts_ctrl[i].hw_tshut_raw_temp =
-			lvts_temp_to_raw(lvts_data->lvts_ctrl[i].hw_tshut_temp);
+			lvts_temp_to_raw(lvts_data->lvts_ctrl[i].hw_tshut_temp,
+					 lvts_data->temp_factor);
 
 		lvts_ctrl[i].low_thresh = INT_MIN;
 		lvts_ctrl[i].high_thresh = INT_MIN;

@@ -1223,6 +1234,8 @@ static int lvts_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
+	golden_temp_offset = lvts_data->temp_offset;
+
 	ret = lvts_domain_init(dev, lvts_td, lvts_data);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to initialize the lvts domain\n");

@@ -1338,11 +1351,15 @@ static const struct lvts_ctrl_data mt8195_lvts_ap_data_ctrl[] = {
 static const struct lvts_data mt8195_lvts_mcu_data = {
 	.lvts_ctrl	= mt8195_lvts_mcu_data_ctrl,
 	.num_lvts_ctrl	= ARRAY_SIZE(mt8195_lvts_mcu_data_ctrl),
+	.temp_factor	= LVTS_COEFF_A_MT8195,
+	.temp_offset	= LVTS_COEFF_B_MT8195,
 };
 
 static const struct lvts_data mt8195_lvts_ap_data = {
 	.lvts_ctrl	= mt8195_lvts_ap_data_ctrl,
 	.num_lvts_ctrl	= ARRAY_SIZE(mt8195_lvts_ap_data_ctrl),
+	.temp_factor	= LVTS_COEFF_A_MT8195,
+	.temp_offset	= LVTS_COEFF_B_MT8195,
 };
 
 static const struct of_device_id lvts_of_match[] = {
Signature · 1 line
2.34.1
maintainer Reviewed-by on v2 3/4
#8 lore raw

Re: [PATCH v2 3/4] thermal/drivers/mediatek/lvts_thermal: make coeff configurable

Il 20/09/23 19:50, Frank Wunderlich ha scritto:

Quoted 6 lines

From: Frank Wunderlich <[email protected]>

The upcoming mt7988 has different temperature coefficients so we cannot use constants in the functions lvts_golden_temp_init, lvts_golden_temp_init and lvts_raw_to_temp anymore.

Quoted 8 lines

Add a field in the lvts_ctrl pointing to the lvts_data which now contains the soc-specific temperature coefficents.

To make the code better readable, rename static int coeff_b to golden_temp_offset, COEFF_A to temp_factor and COEFF_B to temp_offset.

Signed-off-by Frank Wunderlich <[email protected]>

Now, that's good!

Reviewed-by AngeloGioacchino Del Regno <[email protected]>
author
#9 lore raw

PATCH v2 4/4 thermal/drivers/mediatek/lvts_thermal: add mt7988 support

In reply to Frank Wunderlich

From: Frank Wunderlich <[email protected]>

Add Support for Mediatek Filogic 880/MT7988 LVTS.

Signed-off-by Frank Wunderlich <[email protected]>

---
v2:
- use 105°C for hw shutdown
- move constants to binding file
- change coeff.a to temp_factor and coeff.b to temp_offset
- change to lvts to lvts-ap (Application Processor)
- drop comments about efuse offsets
- change comment of mt8195 to be similar to mt7988

---
 drivers/thermal/mediatek/lvts_thermal.c | 46 +++++++++++++++++++++++++
 1 file changed, 46 insertions(+)
drivers/thermal/mediatek/lvts_thermal.c +46 -0
index c2669f405a94..8fd1dc5adb16 100644
@@ -82,6 +82,8 @@
 #define LVTS_GOLDEN_TEMP_DEFAULT	50
 #define LVTS_COEFF_A_MT8195			-250460
 #define LVTS_COEFF_B_MT8195			250460
+#define LVTS_COEFF_A_MT7988			-204650
+#define LVTS_COEFF_B_MT7988			204650
 
 #define LVTS_MSR_IMMEDIATE_MODE		0
 #define LVTS_MSR_FILTERED_MODE		1

@@ -89,6 +91,7 @@
 #define LVTS_MSR_READ_TIMEOUT_US	400
 #define LVTS_MSR_READ_WAIT_US		(LVTS_MSR_READ_TIMEOUT_US / 2)
 
+#define LVTS_HW_SHUTDOWN_MT7988		105000
 #define LVTS_HW_SHUTDOWN_MT8195		105000
 
 #define LVTS_MINIMUM_THRESHOLD		20000

@@ -1269,6 +1272,41 @@ static int lvts_remove(struct platform_device *pdev)
 	return 0;
 }
 
+/*
+ * LVTS MT7988
+ */
+
+static const struct lvts_ctrl_data mt7988_lvts_ap_data_ctrl[] = {
+	{
+		.cal_offset = { 0x00, 0x04, 0x08, 0x0c }, //918,91C,920,924
+		.lvts_sensor = {
+			{ .dt_id = MT7988_CPU_0 }, // CPU 0,1
+			{ .dt_id = MT7988_CPU_1 }, // CPU 2,3
+			{ .dt_id = MT7988_ETH2P5G_0 }, // internal 2.5G Phy 1
+			{ .dt_id = MT7988_ETH2P5G_1 }  // internal 2.5G Phy 2
+		},
+		.num_lvts_sensor = 4,
+		.offset = 0x0,
+		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
+	},
+	{
+		.cal_offset = { 0x14, 0x18, 0x1c, 0x20 }, //92C,930,934,938
+		.lvts_sensor = {
+			{ .dt_id = MT7988_TOPS_0}, // TOPS
+			{ .dt_id = MT7988_TOPS_1}, // TOPS
+			{ .dt_id = MT7988_ETHWARP_0}, // WED 1
+			{ .dt_id = MT7988_ETHWARP_1}  // WED 2
+		},
+		.num_lvts_sensor = 4,
+		.offset = 0x100,
+		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
+	}
+};
+
+/*
+ * LVTS MT8195
+ */
+
 static const struct lvts_ctrl_data mt8195_lvts_mcu_data_ctrl[] = {
 	{
 		.cal_offset = { 0x04, 0x07 },

@@ -1348,6 +1386,13 @@ static const struct lvts_ctrl_data mt8195_lvts_ap_data_ctrl[] = {
 	}
 };
 
+static const struct lvts_data mt7988_lvts_ap_data = {
+	.lvts_ctrl	= mt7988_lvts_ap_data_ctrl,
+	.num_lvts_ctrl	= ARRAY_SIZE(mt7988_lvts_ap_data_ctrl),
+	.temp_factor	= LVTS_COEFF_A_MT7988,
+	.temp_offset	= LVTS_COEFF_B_MT7988,
+};
+
 static const struct lvts_data mt8195_lvts_mcu_data = {
 	.lvts_ctrl	= mt8195_lvts_mcu_data_ctrl,
 	.num_lvts_ctrl	= ARRAY_SIZE(mt8195_lvts_mcu_data_ctrl),

@@ -1363,6 +1408,7 @@ static const struct lvts_data mt8195_lvts_ap_data = {
 };
 
 static const struct of_device_id lvts_of_match[] = {
+	{ .compatible = "mediatek,mt7988-lvts-ap", .data = &mt7988_lvts_ap_data },
 	{ .compatible = "mediatek,mt8195-lvts-mcu", .data = &mt8195_lvts_mcu_data },
 	{ .compatible = "mediatek,mt8195-lvts-ap", .data = &mt8195_lvts_ap_data },
 	{},
Signature · 1 line
2.34.1
maintainer
#10 lore raw

Re: [PATCH v2 4/4] thermal/drivers/mediatek/lvts_thermal: add mt7988 support

Il 20/09/23 19:50, Frank Wunderlich ha scritto:

Quoted 31 lines

From: Frank Wunderlich <[email protected]>

Add Support for Mediatek Filogic 880/MT7988 LVTS.

Signed-off-by Frank Wunderlich <[email protected]>

---
v2:
- use 105°C for hw shutdown
- move constants to binding file
- change coeff.a to temp_factor and coeff.b to temp_offset
- change to lvts to lvts-ap (Application Processor)
- drop comments about efuse offsets
- change comment of mt8195 to be similar to mt7988

---
  drivers/thermal/mediatek/lvts_thermal.c | 46 +++++++++++++++++++++++++
  1 file changed, 46 insertions(+)
drivers/thermal/mediatek/lvts_thermal.c +2 -0
index c2669f405a94..8fd1dc5adb16 100644
@@ -82,6 +82,8 @@
  #define LVTS_GOLDEN_TEMP_DEFAULT	50
  #define LVTS_COEFF_A_MT8195			-250460
  #define LVTS_COEFF_B_MT8195			250460
+#define LVTS_COEFF_A_MT7988			-204650
+#define LVTS_COEFF_B_MT7988			204650
 
  #define LVTS_MSR_IMMEDIATE_MODE		0
  #define LVTS_MSR_FILTERED_MODE		1

drivers/thermal/mediatek/lvts_thermal.c line 94

Quoted 4 lines
drivers/thermal/mediatek/lvts_thermal.c +1 -0
@@ -89,6 +91,7 @@
  #define LVTS_MSR_READ_TIMEOUT_US	400
  #define LVTS_MSR_READ_WAIT_US		(LVTS_MSR_READ_TIMEOUT_US / 2)
 
+#define LVTS_HW_SHUTDOWN_MT7988		105000

I would simply reuse the definition of LVTS_HW_SHUTDOWN_MT8195....

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1278

Quoted 11 lines
drivers/thermal/mediatek/lvts_thermal.c +4 -0
  #define LVTS_HW_SHUTDOWN_MT8195		105000
 
  #define LVTS_MINIMUM_THRESHOLD		20000

@@ -1269,6 +1272,41 @@ static int lvts_remove(struct platform_device *pdev)
  	return 0;
  }
 
+/*
+ * LVTS MT7988
+ */
+

Please remove this big comment block, that's not needed.

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1281

Quoted 3 lines
drivers/thermal/mediatek/lvts_thermal.c +3 -0
+static const struct lvts_ctrl_data mt7988_lvts_ap_data_ctrl[] = {
+	{
+		.cal_offset = { 0x00, 0x04, 0x08, 0x0c }, //918,91C,920,924

This 918,91c,etc comment is not necessary

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1283

Quoted 2 lines
drivers/thermal/mediatek/lvts_thermal.c +2 -0
+		.lvts_sensor = {
+			{ .dt_id = MT7988_CPU_0 }, // CPU 0,1

If you want to retain those comments, you shall use the right style.

{ .dt_id = MT7988_CPU_0 }, /* CPU 0,1 */
{ .. } /* CPU 2,3 */
{ .. } /* Internal 2.5G PHY 1 */

etc

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1293

Quoted 10 lines
drivers/thermal/mediatek/lvts_thermal.c +10 -0
+			{ .dt_id = MT7988_CPU_1 }, // CPU 2,3
+			{ .dt_id = MT7988_ETH2P5G_0 }, // internal 2.5G Phy 1
+			{ .dt_id = MT7988_ETH2P5G_1 }  // internal 2.5G Phy 2
+		},
+		.num_lvts_sensor = 4,
+		.offset = 0x0,
+		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
+	},
+	{
+		.cal_offset = { 0x14, 0x18, 0x1c, 0x20 }, //92C,930,934,938

comment not needed

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1295

Quoted 2 lines
drivers/thermal/mediatek/lvts_thermal.c +2 -0
+		.lvts_sensor = {
+			{ .dt_id = MT7988_TOPS_0}, // TOPS > +			{ .dt_id = MT7988_TOPS_1}, // TOPS

The dt_id definition already says "TOPS", this comment is not needed.

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1298

Quoted 2 lines
drivers/thermal/mediatek/lvts_thermal.c +2 -0
+			{ .dt_id = MT7988_ETHWARP_0}, // WED 1
+			{ .dt_id = MT7988_ETHWARP_1}  // WED 2

Same comment about the format; /* WED 1 */

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1308

Quoted 10 lines
drivers/thermal/mediatek/lvts_thermal.c +10 -0
+		},
+		.num_lvts_sensor = 4,
+		.offset = 0x100,
+		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
+	}
+};
+
+/*
+ * LVTS MT8195
+ */

Please also remove this big comment block, it's not needed.

Apart from that, this patch looks good; v3 will be the golden one :-)

Cheers,
Angelo

Quoted 26 lines
drivers/thermal/mediatek/lvts_thermal.c +9 -0
+
  static const struct lvts_ctrl_data mt8195_lvts_mcu_data_ctrl[] = {
  	{
  		.cal_offset = { 0x04, 0x07 },

@@ -1348,6 +1386,13 @@ static const struct lvts_ctrl_data mt8195_lvts_ap_data_ctrl[] = {
  	}
  };
 
+static const struct lvts_data mt7988_lvts_ap_data = {
+	.lvts_ctrl	= mt7988_lvts_ap_data_ctrl,
+	.num_lvts_ctrl	= ARRAY_SIZE(mt7988_lvts_ap_data_ctrl),
+	.temp_factor	= LVTS_COEFF_A_MT7988,
+	.temp_offset	= LVTS_COEFF_B_MT7988,
+};
+
  static const struct lvts_data mt8195_lvts_mcu_data = {
  	.lvts_ctrl	= mt8195_lvts_mcu_data_ctrl,
  	.num_lvts_ctrl	= ARRAY_SIZE(mt8195_lvts_mcu_data_ctrl),

@@ -1363,6 +1408,7 @@ static const struct lvts_data mt8195_lvts_ap_data = {
  };
 
  static const struct of_device_id lvts_of_match[] = {
+	{ .compatible = "mediatek,mt7988-lvts-ap", .data = &mt7988_lvts_ap_data },
  	{ .compatible = "mediatek,mt8195-lvts-mcu", .data = &mt8195_lvts_mcu_data },
  	{ .compatible = "mediatek,mt8195-lvts-ap", .data = &mt8195_lvts_ap_data },
  	{},
author
#11 lore raw

Re: [PATCH v2 4/4] thermal/drivers/mediatek/lvts_thermal: add mt7988 support

Am 21. September 2023 09:54:35 MESZ schrieb AngeloGioacchino Del Regno <[email protected]>:

Quoted 13 lines

Il 20/09/23 19:50, Frank Wunderlich ha scritto:

Quoted 12 lines

From: Frank Wunderlich <[email protected]>

drivers/thermal/mediatek/lvts_thermal.c +2 -0
index c2669f405a94..8fd1dc5adb16 100644
@@ -82,6 +82,8 @@
  #define LVTS_GOLDEN_TEMP_DEFAULT	50
  #define LVTS_COEFF_A_MT8195			-250460
  #define LVTS_COEFF_B_MT8195			250460
+#define LVTS_COEFF_A_MT7988			-204650
+#define LVTS_COEFF_B_MT7988			204650
Quoted 8 lines

drivers/thermal/mediatek/lvts_thermal.c line 93

Quoted 6 lines
drivers/thermal/mediatek/lvts_thermal.c +0 -0
    #define LVTS_MSR_IMMEDIATE_MODE		0
  #define LVTS_MSR_FILTERED_MODE		1

@@ -89,6 +91,7 @@
  #define LVTS_MSR_READ_TIMEOUT_US	400
  #define LVTS_MSR_READ_WAIT_US		(LVTS_MSR_READ_TIMEOUT_US / 2)
  +#define LVTS_HW_SHUTDOWN_MT7988		105000

I would simply reuse the definition of LVTS_HW_SHUTDOWN_MT8195....

Hi angelo,
thanks for review.

Imho it should be separated...if someone thinks it needs to be changed later it will be changed not only for mt8195...a generic name can also cause problems if the next soc has different value.

Quoted 11 lines

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1277

Quoted 9 lines
drivers/thermal/mediatek/lvts_thermal.c +3 -0
  #define LVTS_HW_SHUTDOWN_MT8195		105000
    #define LVTS_MINIMUM_THRESHOLD		20000

@@ -1269,6 +1272,41 @@ static int lvts_remove(struct platform_device *pdev)
  	return 0;
  }
  +/*
+ * LVTS MT7988
+ */
+

Please remove this big comment block, that's not needed.

Ok,i drop the comments (maybe except the wed one where the name in technical document (i used for constants) does not point to wed function.

Quoted 53 lines

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1281

Quoted 3 lines
drivers/thermal/mediatek/lvts_thermal.c +3 -0
+static const struct lvts_ctrl_data mt7988_lvts_ap_data_ctrl[] = {
+	{
+		.cal_offset = { 0x00, 0x04, 0x08, 0x0c }, //918,91C,920,924

This 918,91c,etc comment is not necessary

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1283

Quoted 2 lines
drivers/thermal/mediatek/lvts_thermal.c +2 -0
+		.lvts_sensor = {
+			{ .dt_id = MT7988_CPU_0 }, // CPU 0,1

If you want to retain those comments, you shall use the right style.

{ .dt_id = MT7988_CPU_0 }, /* CPU 0,1 */
{ .. } /* CPU 2,3 */
{ .. } /* Internal 2.5G PHY 1 */

etc

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1293

Quoted 10 lines
drivers/thermal/mediatek/lvts_thermal.c +10 -0
+			{ .dt_id = MT7988_CPU_1 }, // CPU 2,3
+			{ .dt_id = MT7988_ETH2P5G_0 }, // internal 2.5G Phy 1
+			{ .dt_id = MT7988_ETH2P5G_1 }  // internal 2.5G Phy 2
+		},
+		.num_lvts_sensor = 4,
+		.offset = 0x0,
+		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
+	},
+	{
+		.cal_offset = { 0x14, 0x18, 0x1c, 0x20 }, //92C,930,934,938

comment not needed

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1294

Quoted 2 lines
drivers/thermal/mediatek/lvts_thermal.c +2 -0
+		.lvts_sensor = {
+			{ .dt_id = MT7988_TOPS_0}, // TOPS > +			{ .dt_id = MT7988_TOPS_1}, // TOPS

The dt_id definition already says "TOPS", this comment is not needed.

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1298

Quoted 2 lines
drivers/thermal/mediatek/lvts_thermal.c +2 -0
+			{ .dt_id = MT7988_ETHWARP_0}, // WED 1
+			{ .dt_id = MT7988_ETHWARP_1}  // WED 2

Same comment about the format; /* WED 1 */

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1308

Quoted 10 lines
drivers/thermal/mediatek/lvts_thermal.c +10 -0
+		},
+		.num_lvts_sensor = 4,
+		.offset = 0x100,
+		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
+	}
+};
+
+/*
+ * LVTS MT8195
+ */

Please also remove this big comment block, it's not needed.

Quoted 5 lines

Apart from that, this patch looks good; v3 will be the golden one :-)

Cheers,
Angelo

regards Frank

maintainer
#12 lore raw

Re: [PATCH v2 4/4] thermal/drivers/mediatek/lvts_thermal: add mt7988 support

Il 21/09/23 12:39, Frank Wunderlich ha scritto:

Quoted 23 lines

Am 21. September 2023 09:54:35 MESZ schrieb AngeloGioacchino Del Regno <[email protected]>:

Quoted 13 lines

Il 20/09/23 19:50, Frank Wunderlich ha scritto:

Quoted 12 lines

From: Frank Wunderlich <[email protected]>

drivers/thermal/mediatek/lvts_thermal.c +2 -0
index c2669f405a94..8fd1dc5adb16 100644
@@ -82,6 +82,8 @@
   #define LVTS_GOLDEN_TEMP_DEFAULT	50
   #define LVTS_COEFF_A_MT8195			-250460
   #define LVTS_COEFF_B_MT8195			250460
+#define LVTS_COEFF_A_MT7988			-204650
+#define LVTS_COEFF_B_MT7988			204650
Quoted 8 lines

drivers/thermal/mediatek/lvts_thermal.c line 93

Quoted 6 lines
drivers/thermal/mediatek/lvts_thermal.c +0 -0
     #define LVTS_MSR_IMMEDIATE_MODE		0
   #define LVTS_MSR_FILTERED_MODE		1

@@ -89,6 +91,7 @@
   #define LVTS_MSR_READ_TIMEOUT_US	400
   #define LVTS_MSR_READ_WAIT_US		(LVTS_MSR_READ_TIMEOUT_US / 2)
   +#define LVTS_HW_SHUTDOWN_MT7988		105000

I would simply reuse the definition of LVTS_HW_SHUTDOWN_MT8195....

Quoted 5 lines

Hi angelo,
thanks for review.

Imho it should be separated...if someone thinks it needs to be changed later it will be changed not only for mt8195...a generic name can also cause problems if the next soc has different value.

Okay, I don't really have strong opinions against that anyway.

Cheers
Angelo

Quoted 74 lines
Quoted 11 lines

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1277

Quoted 9 lines
drivers/thermal/mediatek/lvts_thermal.c +3 -0
   #define LVTS_HW_SHUTDOWN_MT8195		105000
     #define LVTS_MINIMUM_THRESHOLD		20000

@@ -1269,6 +1272,41 @@ static int lvts_remove(struct platform_device *pdev)
   	return 0;
   }
   +/*
+ * LVTS MT7988
+ */
+

Please remove this big comment block, that's not needed.

Ok,i drop the comments (maybe except the wed one where the name in technical document (i used for constants) does not point to wed function.

Quoted 53 lines

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1281

Quoted 3 lines
drivers/thermal/mediatek/lvts_thermal.c +3 -0
+static const struct lvts_ctrl_data mt7988_lvts_ap_data_ctrl[] = {
+	{
+		.cal_offset = { 0x00, 0x04, 0x08, 0x0c }, //918,91C,920,924

This 918,91c,etc comment is not necessary

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1283

Quoted 2 lines
drivers/thermal/mediatek/lvts_thermal.c +2 -0
+		.lvts_sensor = {
+			{ .dt_id = MT7988_CPU_0 }, // CPU 0,1

If you want to retain those comments, you shall use the right style.

{ .dt_id = MT7988_CPU_0 }, /* CPU 0,1 */
{ .. } /* CPU 2,3 */
{ .. } /* Internal 2.5G PHY 1 */

etc

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1293

Quoted 10 lines
drivers/thermal/mediatek/lvts_thermal.c +10 -0
+			{ .dt_id = MT7988_CPU_1 }, // CPU 2,3
+			{ .dt_id = MT7988_ETH2P5G_0 }, // internal 2.5G Phy 1
+			{ .dt_id = MT7988_ETH2P5G_1 }  // internal 2.5G Phy 2
+		},
+		.num_lvts_sensor = 4,
+		.offset = 0x0,
+		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
+	},
+	{
+		.cal_offset = { 0x14, 0x18, 0x1c, 0x20 }, //92C,930,934,938

comment not needed

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1294

Quoted 2 lines
drivers/thermal/mediatek/lvts_thermal.c +2 -0
+		.lvts_sensor = {
+			{ .dt_id = MT7988_TOPS_0}, // TOPS > +			{ .dt_id = MT7988_TOPS_1}, // TOPS

The dt_id definition already says "TOPS", this comment is not needed.

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1298

Quoted 2 lines
drivers/thermal/mediatek/lvts_thermal.c +2 -0
+			{ .dt_id = MT7988_ETHWARP_0}, // WED 1
+			{ .dt_id = MT7988_ETHWARP_1}  // WED 2

Same comment about the format; /* WED 1 */

drivers/thermal/mediatek/lvts_thermal.c static int lvts_remove(struct platform_device *pdev) · line 1308

Quoted 10 lines
drivers/thermal/mediatek/lvts_thermal.c +10 -0
+		},
+		.num_lvts_sensor = 4,
+		.offset = 0x100,
+		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
+	}
+};
+
+/*
+ * LVTS MT8195
+ */

Please also remove this big comment block, it's not needed.

Quoted 5 lines

Apart from that, this patch looks good; v3 will be the golden one :-)

Cheers,
Angelo

regards Frank