| Message ID | 20230911183354.11487-4-linux@fw-web.de (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | add LVTS support for mt7988 | expand |
Il 11/09/23 20:33, Frank Wunderlich ha scritto: > From: Frank Wunderlich <frank-w@public-files.de> > > Add Support for mediatek fologic 880/MT7988. > > Signed-off-by: Frank Wunderlich <frank-w@public-files.de> > --- > drivers/thermal/mediatek/lvts_thermal.c | 73 +++++++++++++++++++++++++ > 1 file changed, 73 insertions(+) > > diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c > index c1004b4da3b6..48b257a3c80e 100644 > --- a/drivers/thermal/mediatek/lvts_thermal.c > +++ b/drivers/thermal/mediatek/lvts_thermal.c > @@ -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 > @@ -1272,6 +1274,67 @@ static int lvts_remove(struct platform_device *pdev) > return 0; > } > > +/* > + * LVTS MT7988 > + */ > +#define LVTS_HW_SHUTDOWN_MT7988 117000 Are you sure that this chip's Tj is >117°C ?! Looks a bit high... if it is exactly 117°C, I would suggest cutting earlier, either at 110 (safe side) or 115: after all, this is a life-saver feature and the chip is actually never meant to *constantly* work at 110°C (as it would degrade fast and say goodbye earlier than "planned"). > +//enum mt7988_lvts_domain { MT7988_AP_DOMAIN, MT7988_NUM_DOMAIN }; > + > +enum mt7988_lvts_sensor_enum { > + MT7988_TS3_0, > + MT7988_TS3_1, > + MT7988_TS3_2, > + MT7988_TS3_3, > + MT7988_TS4_0, > + MT7988_TS4_1, > + MT7988_TS4_2, > + MT7988_TS4_3, > + MT7988_NUM_TS > +}; This enumeration should be definitions in bindings (mediatek,lvts-thermal.h). Besides, the LVTS is about internal temperatures, so those TS3_x and 4_x can be renamed like what was done for MT8192 and MT8195: this is because you will never see TS3_2 being CPU2 on a board and CPU4 on another, being those - again - internal to the SoC, hence unchangeable. Another reason is that you'll anyway have to refer to those sensors in the devicetree to configure thermal trips and such, so... :-) Regards, Angelo
Am 13. September 2023 10:16:51 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>: Hi angelo, thanks for first look >Il 11/09/23 20:33, Frank Wunderlich ha scritto: >> From: Frank Wunderlich <frank-w@public-files.de> >> >> Add Support for mediatek fologic 880/MT7988. >> >> Signed-off-by: Frank Wunderlich <frank-w@public-files.de> >> --- >> drivers/thermal/mediatek/lvts_thermal.c | 73 +++++++++++++++++++++++++ >> 1 file changed, 73 insertions(+) >> >> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c >> index c1004b4da3b6..48b257a3c80e 100644 >> --- a/drivers/thermal/mediatek/lvts_thermal.c >> +++ b/drivers/thermal/mediatek/lvts_thermal.c >> @@ -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 >> @@ -1272,6 +1274,67 @@ static int lvts_remove(struct platform_device *pdev) >> return 0; >> } >> +/* >> + * LVTS MT7988 >> + */ >> +#define LVTS_HW_SHUTDOWN_MT7988 117000 > >Are you sure that this chip's Tj is >117°C ?! > >Looks a bit high... if it is exactly 117°C, I would suggest cutting earlier, >either at 110 (safe side) or 115: after all, this is a life-saver feature and >the chip is actually never meant to *constantly* work at 110°C (as it would >degrade fast and say goodbye earlier than "planned"). I took values from SDK https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/refs/heads/master/target/linux/mediatek/files-5.4/drivers/thermal/mediatek/soc_temp_lvts.c#1483 >> +//enum mt7988_lvts_domain { MT7988_AP_DOMAIN, MT7988_NUM_DOMAIN }; >> + >> +enum mt7988_lvts_sensor_enum { >> + MT7988_TS3_0, >> + MT7988_TS3_1, >> + MT7988_TS3_2, >> + MT7988_TS3_3, >> + MT7988_TS4_0, >> + MT7988_TS4_1, >> + MT7988_TS4_2, >> + MT7988_TS4_3, >> + MT7988_NUM_TS >> +}; >This enumeration should be definitions in bindings (mediatek,lvts-thermal.h). > >Besides, the LVTS is about internal temperatures, so those TS3_x and 4_x can >be renamed like what was done for MT8192 and MT8195: this is because you will >never see TS3_2 being CPU2 on a board and CPU4 on another, being those - again - >internal to the SoC, hence unchangeable. Right these sensors are internally only and i took naming from sdk to avoid confusion. And i have not more information about these internal sensors (special meaning),but their values are packed together to get the resulting (average) temperature. >Another reason is that you'll anyway have to refer to those sensors in the >devicetree to configure thermal trips and such, so... :-) In device tree it will look like this: https://github.com/frank-w/BPI-Router-Linux/blob/6.5-lvts/arch/arm64/boot/dts/mediatek/mt7988a.dtsi#L771 Daniel has also defined thermal trips there,but these are untested atm. I only verified temperature itself i get from sysfs as far as i can (start at ~40°C and reaching ~70 while running). >Regards, >Angelo regards Frank
Il 13/09/23 12:52, Frank Wunderlich ha scritto: > Am 13. September 2023 10:16:51 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>: > Hi angelo, > > thanks for first look > >> Il 11/09/23 20:33, Frank Wunderlich ha scritto: >>> From: Frank Wunderlich <frank-w@public-files.de> >>> >>> Add Support for mediatek fologic 880/MT7988. >>> >>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de> >>> --- >>> drivers/thermal/mediatek/lvts_thermal.c | 73 +++++++++++++++++++++++++ >>> 1 file changed, 73 insertions(+) >>> >>> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c >>> index c1004b4da3b6..48b257a3c80e 100644 >>> --- a/drivers/thermal/mediatek/lvts_thermal.c >>> +++ b/drivers/thermal/mediatek/lvts_thermal.c >>> @@ -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 >>> @@ -1272,6 +1274,67 @@ static int lvts_remove(struct platform_device *pdev) >>> return 0; >>> } >>> +/* >>> + * LVTS MT7988 >>> + */ >>> +#define LVTS_HW_SHUTDOWN_MT7988 117000 >> >> Are you sure that this chip's Tj is >117°C ?! >> >> Looks a bit high... if it is exactly 117°C, I would suggest cutting earlier, >> either at 110 (safe side) or 115: after all, this is a life-saver feature and >> the chip is actually never meant to *constantly* work at 110°C (as it would >> degrade fast and say goodbye earlier than "planned"). > > I took values from SDK > > https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/refs/heads/master/target/linux/mediatek/files-5.4/drivers/thermal/mediatek/soc_temp_lvts.c#1483 > That kernel also defines 117°C for MT8195, which leaves me a bit dubious. For safety, I would recommend using the same 105°C hw shutdown temperature for 7988 as well: after all if you think about it, reaching that kind of temperature means that there's a real problem (fan stopped working and/or heatsink detached) that needs attention. This is because you'll have trip points in devicetree that should throttle the CPU in case it reaches at least a dangerously high temperature (read: a temperature outside the recommended continuous operation range), bringing the temperatures down because of the throttling action; I would imagine throttling the CPU a bit down at 80°C, then a bit more at 90°C - but then, if the temps won't drop like that, there's clearly a HW-related issue that must be addressed (like the fan/heatsink scenario that I just described). Though, take this as an advice; I'll respect whichever decision you'll take. >>> +//enum mt7988_lvts_domain { MT7988_AP_DOMAIN, MT7988_NUM_DOMAIN }; >>> + >>> +enum mt7988_lvts_sensor_enum { >>> + MT7988_TS3_0, >>> + MT7988_TS3_1, >>> + MT7988_TS3_2, >>> + MT7988_TS3_3, >>> + MT7988_TS4_0, >>> + MT7988_TS4_1, >>> + MT7988_TS4_2, >>> + MT7988_TS4_3, >>> + MT7988_NUM_TS >>> +}; > >> This enumeration should be definitions in bindings (mediatek,lvts-thermal.h). >> >> Besides, the LVTS is about internal temperatures, so those TS3_x and 4_x can >> be renamed like what was done for MT8192 and MT8195: this is because you will >> never see TS3_2 being CPU2 on a board and CPU4 on another, being those - again - >> internal to the SoC, hence unchangeable. > > Right these sensors are internally only and i took naming from sdk to avoid confusion. And i have not more information about these internal sensors (special meaning),but their values are packed together to get the resulting (average) temperature. > >> Another reason is that you'll anyway have to refer to those sensors in the >> devicetree to configure thermal trips and such, so... :-) > > In device tree it will look like this: > > https://github.com/frank-w/BPI-Router-Linux/blob/6.5-lvts/arch/arm64/boot/dts/mediatek/mt7988a.dtsi#L771 > > Daniel has also defined thermal trips there,but these are untested atm. I only verified temperature itself i get from sysfs as far as i can (start at ~40°C and reaching ~70 while running). > Check how it's done in mt8192.dtsi and mt8195.dtsi: we're using the definitions from the bindings for thermal zones. At least for consistency (apart from other even more valid considerations), that's how it should be done: please do it like that. Besides, I think that you can definitely guess what `TS` is CPU related: checking the driver and devicetree from the downstream kernel that you provided, what I understand is: 1. Your naming TS3,4 corresponds to TS2,3 downstream (so it's wrong?) 2. Downstream TS2 is related to the CPU cores, so it should be - TS2_0 - CPU0 - TS2_1 - CPU1 - TS2_2 - CPU2 - TS2_3 - CPU3 The other set of thermal sensors seem to be completely unused, so we cannot guess just by looking at the code. Since you have the hardware, you may be able to take a position about what they are by checking what sensor heats up for each "action" (could be ethernet controller or infra or whatever else); if in doubt, just leave them out, but add a comment saying that there are more sensors and say where, so that if anyone finds what they're for, it'll be easy to add them. In any case, adding thermal sensors that you don't know about is meaningless, as the sense of all this is: - Monitoring temperatures of the hardware - Taking action to prevent temperature overrun but if you don't know what you're reading, you can't interpret temperatures for something unknown, hence you can't as well take action to prevent overruns, as you won't know what's the maximum temperature for "unknown thing" :-) Regards, Angelo
Hi, > Gesendet: Mittwoch, 13. September 2023 um 13:43 Uhr > Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com> > An: frank-w@public-files.de, "Frank Wunderlich" <linux@fw-web.de>, linux-mediatek@lists.infradead.org > Il 13/09/23 12:52, Frank Wunderlich ha scritto: > > Am 13. September 2023 10:16:51 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>: > > Hi angelo, > > > > thanks for first look > > > >> Il 11/09/23 20:33, Frank Wunderlich ha scritto: > >>> From: Frank Wunderlich <frank-w@public-files.de> > >>> > >>> Add Support for mediatek fologic 880/MT7988. > >>> > >>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de> > >>> --- > >>> drivers/thermal/mediatek/lvts_thermal.c | 73 +++++++++++++++++++++++++ > >>> 1 file changed, 73 insertions(+) > >>> > >>> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c > >>> index c1004b4da3b6..48b257a3c80e 100644 > >>> --- a/drivers/thermal/mediatek/lvts_thermal.c > >>> +++ b/drivers/thermal/mediatek/lvts_thermal.c > >>> @@ -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 > >>> @@ -1272,6 +1274,67 @@ static int lvts_remove(struct platform_device *pdev) > >>> return 0; > >>> } > >>> +/* > >>> + * LVTS MT7988 > >>> + */ > >>> +#define LVTS_HW_SHUTDOWN_MT7988 117000 > >> > >> Are you sure that this chip's Tj is >117°C ?! > >> > >> Looks a bit high... if it is exactly 117°C, I would suggest cutting earlier, > >> either at 110 (safe side) or 115: after all, this is a life-saver feature and > >> the chip is actually never meant to *constantly* work at 110°C (as it would > >> degrade fast and say goodbye earlier than "planned"). > > > > I took values from SDK > > > > https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/refs/heads/master/target/linux/mediatek/files-5.4/drivers/thermal/mediatek/soc_temp_lvts.c#1483 > > > > That kernel also defines 117°C for MT8195, which leaves me a bit dubious. > > For safety, I would recommend using the same 105°C hw shutdown temperature > for 7988 as well: after all if you think about it, reaching that kind of > temperature means that there's a real problem (fan stopped working and/or > heatsink detached) that needs attention. ack, will change to same value and put the define on top, abve the mt8195 > This is because you'll have trip points in devicetree that should throttle > the CPU in case it reaches at least a dangerously high temperature (read: > a temperature outside the recommended continuous operation range), bringing > the temperatures down because of the throttling action; I would imagine > throttling the CPU a bit down at 80°C, then a bit more at 90°C - but then, > if the temps won't drop like that, there's clearly a HW-related issue that > must be addressed (like the fan/heatsink scenario that I just described). > > Though, take this as an advice; I'll respect whichever decision you'll take. > > >>> +//enum mt7988_lvts_domain { MT7988_AP_DOMAIN, MT7988_NUM_DOMAIN }; > >>> + > >>> +enum mt7988_lvts_sensor_enum { > >>> + MT7988_TS3_0, > >>> + MT7988_TS3_1, > >>> + MT7988_TS3_2, > >>> + MT7988_TS3_3, > >>> + MT7988_TS4_0, > >>> + MT7988_TS4_1, > >>> + MT7988_TS4_2, > >>> + MT7988_TS4_3, > >>> + MT7988_NUM_TS > >>> +}; > > > >> This enumeration should be definitions in bindings (mediatek,lvts-thermal.h). > >> > >> Besides, the LVTS is about internal temperatures, so those TS3_x and 4_x can > >> be renamed like what was done for MT8192 and MT8195: this is because you will > >> never see TS3_2 being CPU2 on a board and CPU4 on another, being those - again - > >> internal to the SoC, hence unchangeable. > > > > Right these sensors are internally only and i took naming from sdk to avoid confusion. And i have not more information about these internal sensors (special meaning),but their values are packed together to get the resulting (average) temperature. > > > >> Another reason is that you'll anyway have to refer to those sensors in the > >> devicetree to configure thermal trips and such, so... :-) > > > > In device tree it will look like this: > > > > https://github.com/frank-w/BPI-Router-Linux/blob/6.5-lvts/arch/arm64/boot/dts/mediatek/mt7988a.dtsi#L771 > > > > Daniel has also defined thermal trips there,but these are untested atm. I only verified temperature itself i get from sysfs as far as i can (start at ~40°C and reaching ~70 while running). > > > > Check how it's done in mt8192.dtsi and mt8195.dtsi: we're using the definitions > from the bindings for thermal zones. > At least for consistency (apart from other even more valid considerations), that's > how it should be done: please do it like that. > > Besides, I think that you can definitely guess what `TS` is CPU related: checking > the driver and devicetree from the downstream kernel that you provided, what I > understand is: > > 1. Your naming TS3,4 corresponds to TS2,3 downstream (so it's wrong?) > 2. Downstream TS2 is related to the CPU cores, so it should be > - TS2_0 - CPU0 > - TS2_1 - CPU1 > - TS2_2 - CPU2 > - TS2_3 - CPU3 i took the names from efuse names (which are listed as comment above), had 2/3 before. need to ask mtk here. same for the second controller and if it is really a "lvts-ap". > The other set of thermal sensors seem to be completely unused, so we cannot guess > just by looking at the code. Since you have the hardware, you may be able to take > a position about what they are by checking what sensor heats up for each "action" > (could be ethernet controller or infra or whatever else); if in doubt, just leave > them out, but add a comment saying that there are more sensors and say where, so > that if anyone finds what they're for, it'll be easy to add them. > > In any case, adding thermal sensors that you don't know about is meaningless, as > the sense of all this is: > - Monitoring temperatures of the hardware > - Taking action to prevent temperature overrun > but if you don't know what you're reading, you can't interpret temperatures for > something unknown, hence you can't as well take action to prevent overruns, as > you won't know what's the maximum temperature for "unknown thing" :-) yes i have only the downstream kernel and some additional information like interupt number and the efuse offsets for calibration. And for the 8 sensors i get this information: "8 sensors are distributed across the whole soc in order to get the correct average temperature. chip designer didn’t disclose more detailed info to us." so i thought all 8 sensors are taken into account with both controllers to calculate 1 resulting temperature. In downstream i saw also only 1 controller in dts and i have only 1 interrupt. https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/refs/heads/master/target/linux/mediatek/files-5.4/arch/arm64/boot/dts/mediatek/mt7988.dtsi#579 and the compatible for both (mt7988 and mt8195) are without "ap" there, so i took the same. Where have you seen the mt7988-AP ? i have only seen 1 sensor which should be the SOC itself and this chip has metal surface which makes it hard to get real temperature (infrared thermometer gives wrong temp). > Regards, > Angelo
diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c index c1004b4da3b6..48b257a3c80e 100644 --- a/drivers/thermal/mediatek/lvts_thermal.c +++ b/drivers/thermal/mediatek/lvts_thermal.c @@ -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 @@ -1272,6 +1274,67 @@ static int lvts_remove(struct platform_device *pdev) return 0; } +/* + * LVTS MT7988 + */ +#define LVTS_HW_SHUTDOWN_MT7988 117000 +//enum mt7988_lvts_domain { MT7988_AP_DOMAIN, MT7988_NUM_DOMAIN }; + +enum mt7988_lvts_sensor_enum { + MT7988_TS3_0, + MT7988_TS3_1, + MT7988_TS3_2, + MT7988_TS3_3, + MT7988_TS4_0, + MT7988_TS4_1, + MT7988_TS4_2, + MT7988_TS4_3, + MT7988_NUM_TS +}; + +//Efuse base : 0x11f50000 +//lvts offset in efuse : 0x918 (set in efuse dts node as calibration data) +//offsets: +//918 = LVTS_3_0_COUNT_R +//91C = LVTS_3_1_COUNT_R +//920 = LVTS_3_2_COUNT_R +//924 = LVTS_3_3_COUNT_R +//928 = LVTS_3_COUNT_RC + +//92C = LVTS_4_0_COUNT_R +//930 = LVTS_4_1_COUNT_R +//934 = LVTS_4_2_COUNT_R +//938 = LVTS_4_3_COUNT_R +//93C = LVTS_4_COUNT_RC + +static const struct lvts_ctrl_data mt7988_lvts_data_ctrl[] = { + { + .cal_offset = { 0x00, 0x04, 0x08, 0x0c }, //918,91C,920,924 + .lvts_sensor = { + { .dt_id = MT7988_TS3_0 }, + { .dt_id = MT7988_TS3_1 }, + { .dt_id = MT7988_TS3_2 }, + { .dt_id = MT7988_TS3_3 } + }, + .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_TS4_0}, + { .dt_id = MT7988_TS4_1}, + { .dt_id = MT7988_TS4_2}, + { .dt_id = MT7988_TS4_3} + }, + .num_lvts_sensor = 4, + .offset = 0x100, + .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988, + } +}; + +//MT8195 static const struct lvts_ctrl_data mt8195_lvts_mcu_data_ctrl[] = { { .cal_offset = { 0x04, 0x07 }, @@ -1351,6 +1414,15 @@ static const struct lvts_ctrl_data mt8195_lvts_ap_data_ctrl[] = { } }; +static const struct lvts_data mt7988_lvts_data = { + .lvts_ctrl = mt7988_lvts_data_ctrl, + .num_lvts_ctrl = ARRAY_SIZE(mt7988_lvts_data_ctrl), + .coeff = { + .a = LVTS_COEFF_A_MT7988, + .b = 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), @@ -1372,6 +1444,7 @@ static const struct lvts_data mt8195_lvts_ap_data = { }; static const struct of_device_id lvts_of_match[] = { + { .compatible = "mediatek,mt7988-lvts", .data = &mt7988_lvts_data }, { .compatible = "mediatek,mt8195-lvts-mcu", .data = &mt8195_lvts_mcu_data }, { .compatible = "mediatek,mt8195-lvts-ap", .data = &mt8195_lvts_ap_data }, {},