On Tue, 19 Mar 2024 00:41:33 +0530 Amit Singh Tomar <amitsinght@marvell.com> wrote: Hi, > >> > >> With the DT bindings now describing the format of the CPU OPP tables, we can include the OPP table in each board's .dts file, and specify the CPU power supply. > >> This allows to enable DVFS, and get up to 50% of performance benefit in the highest OPP, or up to 60% power savings in the lowest OPP, compared to the fixed 1GHz @ 1.0V OPP we are running in by default > >> at the moment. > >> [Amit] Could you please elaborate, what test were run to see 50 % performance benefits? > > > > Currently all H616 boards running mainline firmware and kernels run at a > > fixed 1GHz CPU clock frequency. If you happen to have a good SoC (bin 1 or > > 3), this patchset will allow you to run at 1.5 GHz, which is 50% faster. > > So anything that scales with CPU frequency should run much quicker. > > > Okay, it would be interesting to see results of some benchmark here. But why? This is not a performance optimisation, it's adding a missing feature, because the CPU was locked to 1 GHz before, for safety reasons, due to missing thermal and DVFS capability. Now it's able to run at up to 1.5 GHz, as specified. If you are upset about the bold claim, I can just remove it from the commit message, it was just a heads up that we were leaving a lot of performance on the table at the moment. Posting absolute performance numbers is a tricky subject, legally, so please run your own: http://www.netlib.org/benchmark/linpackc.new $ musl-gcc -s -static -Ofast -o linpack linpack.c -lm -march=native -fno-math-errno -funsafe-math-optimizations $ gcc --version gcc (Debian 12.2.0-14) 12.2.0 # echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor # echo 1008000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed $ ./linpack # echo performance > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor $ ./linpack $ dc -e '3k $high $low /p' 1.498 Cheers, Andre
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. psvt->limit.string can only be 8 bytes so let's use the appropriate size macro ACPI_LIMIT_STR_MAX_LEN. Neither psvt->limit.string or psvt_user[i].limit.string requires the NUL-padding behavior that strncpy() provides as they have both been filled with NUL-bytes prior to the string operation. | memset(&psvt->limit, 0, sizeof(u64)); and | psvt_user = kzalloc(psvt_len, GFP_KERNEL); Let's use `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- Note: build-tested only. Found with: $ rg "strncpy\(" --- drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c index dc519a665c18..4b4a4d63e61f 100644 --- a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c +++ b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c @@ -309,7 +309,7 @@ static int acpi_parse_psvt(acpi_handle handle, int *psvt_count, struct psvt **ps if (knob->type == ACPI_TYPE_STRING) { memset(&psvt->limit, 0, sizeof(u64)); - strncpy(psvt->limit.string, psvt_ptr->limit.str_ptr, knob->string.length); + strscpy(psvt->limit.string, psvt_ptr->limit.str_ptr, ACPI_LIMIT_STR_MAX_LEN); } else { psvt->limit.integer = psvt_ptr->limit.integer; } @@ -468,7 +468,7 @@ static int fill_psvt(char __user *ubuf) psvt_user[i].unlimit_coeff = psvts[i].unlimit_coeff; psvt_user[i].control_knob_type = psvts[i].control_knob_type; if (psvt_user[i].control_knob_type == ACPI_TYPE_STRING) - strncpy(psvt_user[i].limit.string, psvts[i].limit.string, + strscpy(psvt_user[i].limit.string, psvts[i].limit.string, ACPI_LIMIT_STR_MAX_LEN); else psvt_user[i].limit.integer = psvts[i].limit.integer; --- base-commit: bf3a69c6861ff4dc7892d895c87074af7bc1c400 change-id: 20240318-strncpy-drivers-thermal-intel-int340x_thermal-acpi_thermal_rel-c-17070c1e42f3 Best regards, -- Justin Stitt <justinstitt@google.com>
From: Nicolas Pitre <npitre@baylibre.com> Inspired by the vendor kernel but adapted to the upstream thermal driver version. Signed-off-by: Nicolas Pitre <npitre@baylibre.com> --- arch/arm64/boot/dts/mediatek/mt8188.dtsi | 348 +++++++++++++++++++++++ 1 file changed, 348 insertions(+) diff --git a/arch/arm64/boot/dts/mediatek/mt8188.dtsi b/arch/arm64/boot/dts/mediatek/mt8188.dtsi index 5a3c58a77c..ea90ad4baa 100644 --- a/arch/arm64/boot/dts/mediatek/mt8188.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8188.dtsi @@ -12,6 +12,8 @@ #include <dt-bindings/pinctrl/mediatek,mt8188-pinfunc.h> #include <dt-bindings/power/mediatek,mt8188-power.h> #include <dt-bindings/reset/mt8188-resets.h> +#include <dt-bindings/thermal/thermal.h> +#include <dt-bindings/thermal/mediatek,lvts-thermal.h> / { compatible = "mediatek,mt8188"; @@ -311,6 +313,352 @@ psci { method = "smc"; }; + thermal_zones: thermal-zones { + cluster0-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_mcu MT8188_MCU_TS1_0>; + + trips { + cluster0_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cluster0_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cluster0_alert>; + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + cluster1-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_mcu MT8188_MCU_TS1_1>; + + trips { + cluster1_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cluster1_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cluster1_alert>; + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + cluster2-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_mcu MT8188_MCU_TS1_2>; + + trips { + cluster2_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cluster2_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cluster2_alert>; + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + cluster3-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_mcu MT8188_MCU_TS1_3>; + + trips { + cluster3_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cluster3_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cluster3_alert>; + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + cpu_big0-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_mcu MT8188_MCU_TS2_0>; + + trips { + cpu_big0_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cpu_big0_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cpu_big0_alert>; + cooling-device = <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + cpu_big1-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_mcu MT8188_MCU_TS2_1>; + + trips { + cpu_big1_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cpu_big1_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cpu_big1_alert>; + cooling-device = <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + apu-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_TS3_1>; + + trips { + apu_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + apu_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + gpu1-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_TS4_0>; + + trips { + gpu1_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + gpu1_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + gpu2-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_TS4_1>; + + trips { + gpu2_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + gpu2_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + soc1-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_TS4_2>; + + trips { + soc1_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + soc1_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + soc2-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_TS5_0>; + + trips { + soc2_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + soc2_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + soc3-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_TS5_1>; + + trips { + soc3_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + soc3_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + cam1-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_TS6_0>; + + trips { + cam1_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cam1_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + cam21-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts_ap MT8188_AP_TS6_1>; + + trips { + cam2_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cam2_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + }; + timer: timer { compatible = "arm,armv8-timer"; interrupt-parent = <&gic>; -- 2.44.0
From: Nicolas Pitre <npitre@baylibre.com> The golden temperature calibration value in nvram is not always the 3rd byte. A future commit will prove this assumption wrong. Signed-off-by: Nicolas Pitre <npitre@baylibre.com> --- drivers/thermal/mediatek/lvts_thermal.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c index e923d22c17..b20b70fd36 100644 --- a/drivers/thermal/mediatek/lvts_thermal.c +++ b/drivers/thermal/mediatek/lvts_thermal.c @@ -121,6 +121,7 @@ struct lvts_data { int num_lvts_ctrl; int temp_factor; int temp_offset; + int gt_calib_bit_offset; }; struct lvts_sensor { @@ -748,20 +749,21 @@ static int lvts_calibration_read(struct device *dev, struct lvts_domain *lvts_td return 0; } -static int lvts_golden_temp_init(struct device *dev, u8 *calib, int temp_offset) +static int lvts_golden_temp_init(struct device *dev, u8 *calib, + const struct lvts_data *lvts_data) { u32 gt; /* - * The golden temp information is contained in the 4th byte (index = 3) - * of efuse data. + * The golden temp information is contained in the first 32-bit + * word of efuse data at a specific bit offset. */ - gt = calib[3]; + gt = (((u32 *)calib)[0] >> lvts_data->gt_calib_bit_offset) & 0xff; if (gt && gt < LVTS_GOLDEN_TEMP_MAX) golden_temp = gt; - golden_temp_offset = golden_temp * 500 + temp_offset; + golden_temp_offset = golden_temp * 500 + lvts_data->temp_offset; return 0; } @@ -780,7 +782,7 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td, if (ret) return ret; - ret = lvts_golden_temp_init(dev, lvts_td->calib, lvts_data->temp_offset); + ret = lvts_golden_temp_init(dev, lvts_td->calib, lvts_data); if (ret) return ret; @@ -1622,6 +1624,7 @@ static const struct lvts_data mt7988_lvts_ap_data = { .num_lvts_ctrl = ARRAY_SIZE(mt7988_lvts_ap_data_ctrl), .temp_factor = LVTS_COEFF_A_MT7988, .temp_offset = LVTS_COEFF_B_MT7988, + .gt_calib_bit_offset = 24, }; static const struct lvts_data mt8186_lvts_data = { @@ -1629,16 +1632,19 @@ static const struct lvts_data mt8186_lvts_data = { .num_lvts_ctrl = ARRAY_SIZE(mt8186_lvts_data_ctrl), .temp_factor = LVTS_COEFF_A_MT8186, .temp_offset = LVTS_COEFF_B_MT8186, + .gt_calib_bit_offset = 24, }; static const struct lvts_data mt8192_lvts_mcu_data = { .lvts_ctrl = mt8192_lvts_mcu_data_ctrl, .num_lvts_ctrl = ARRAY_SIZE(mt8192_lvts_mcu_data_ctrl), + .gt_calib_bit_offset = 24, }; static const struct lvts_data mt8192_lvts_ap_data = { .lvts_ctrl = mt8192_lvts_ap_data_ctrl, .num_lvts_ctrl = ARRAY_SIZE(mt8192_lvts_ap_data_ctrl), + .gt_calib_bit_offset = 24, }; static const struct lvts_data mt8195_lvts_mcu_data = { @@ -1646,6 +1652,7 @@ static const struct lvts_data mt8195_lvts_mcu_data = { .num_lvts_ctrl = ARRAY_SIZE(mt8195_lvts_mcu_data_ctrl), .temp_factor = LVTS_COEFF_A_MT8195, .temp_offset = LVTS_COEFF_B_MT8195, + .gt_calib_bit_offset = 24, }; static const struct lvts_data mt8195_lvts_ap_data = { @@ -1653,6 +1660,7 @@ static const struct lvts_data mt8195_lvts_ap_data = { .num_lvts_ctrl = ARRAY_SIZE(mt8195_lvts_ap_data_ctrl), .temp_factor = LVTS_COEFF_A_MT8195, .temp_offset = LVTS_COEFF_B_MT8195, + .gt_calib_bit_offset = 24, }; static const struct of_device_id lvts_of_match[] = { -- 2.44.0
From: Nicolas Pitre <npitre@baylibre.com> Various values extracted from the vendor's kernel driver. Signed-off-by: Nicolas Pitre <npitre@baylibre.com> --- arch/arm64/boot/dts/mediatek/mt8188.dtsi | 35 ++++++++ drivers/thermal/mediatek/lvts_thermal.c | 102 +++++++++++++++++++++++ 2 files changed, 137 insertions(+) diff --git a/arch/arm64/boot/dts/mediatek/mt8188.dtsi b/arch/arm64/boot/dts/mediatek/mt8188.dtsi index b4315c9214..5a3c58a77c 100644 --- a/arch/arm64/boot/dts/mediatek/mt8188.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8188.dtsi @@ -11,6 +11,7 @@ #include <dt-bindings/phy/phy.h> #include <dt-bindings/pinctrl/mediatek,mt8188-pinfunc.h> #include <dt-bindings/power/mediatek,mt8188-power.h> +#include <dt-bindings/reset/mt8188-resets.h> / { compatible = "mediatek,mt8188"; @@ -357,6 +358,7 @@ infracfg_ao: syscon@10001000 { compatible = "mediatek,mt8188-infracfg-ao", "syscon"; reg = <0 0x10001000 0 0x1000>; #clock-cells = <1>; + #reset-cells = <1>; }; pericfg: syscon@10003000 { @@ -491,6 +493,17 @@ spi0: spi@1100a000 { status = "disabled"; }; + lvts_ap: thermal-sensor@1100b000 { + compatible = "mediatek,mt8188-lvts-ap"; + reg = <0 0x1100b000 0 0x1000>; + interrupts = <GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH 0>; + clocks = <&infracfg_ao CLK_INFRA_AO_THERM>; + resets = <&infracfg_ao MT8188_INFRA_RST1_THERMAL_CTRL_RST>; + nvmem-cells = <&lvts_efuse_data1>; + nvmem-cell-names = "lvts_calib_data1"; + #thermal-sensor-cells = <1>; + }; + spi1: spi@11010000 { compatible = "mediatek,mt8188-spi-ipm", "mediatek,spi-ipm"; #address-cells = <1>; @@ -604,6 +617,17 @@ mmc1: mmc@11240000 { status = "disabled"; }; + lvts_mcu: thermal-sensor@11278000 { + compatible = "mediatek,mt8188-lvts-mcu"; + reg = <0 0x11278000 0 0x1000>; + interrupts = <GIC_SPI 202 IRQ_TYPE_LEVEL_HIGH 0>; + clocks = <&infracfg_ao CLK_INFRA_AO_THERM>; + resets = <&infracfg_ao MT8188_INFRA_RST1_THERMAL_MCU_RST>; + nvmem-cells = <&lvts_efuse_data1>; + nvmem-cell-names = "lvts_calib_data1"; + #thermal-sensor-cells = <1>; + }; + i2c0: i2c@11280000 { compatible = "mediatek,mt8188-i2c"; reg = <0 0x11280000 0 0x1000>, @@ -827,6 +851,17 @@ imp_iic_wrap_en: clock-controller@11ec2000 { #clock-cells = <1>; }; + efuse: efuse@11f20000 { + compatible = "mediatek,mt8188-efuse", "mediatek,efuse"; + reg = <0 0x11f20000 0 0x1000>; + #address-cells = <1>; + #size-cells = <1>; + + lvts_efuse_data1: lvts1-calib@1ac { + reg = <0x1ac 0x40>; + }; + }; + mfgcfg: clock-controller@13fbf000 { compatible = "mediatek,mt8188-mfgcfg"; reg = <0 0x13fbf000 0 0x1000>; diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c index a23a93fc82..46882df640 100644 --- a/drivers/thermal/mediatek/lvts_thermal.c +++ b/drivers/thermal/mediatek/lvts_thermal.c @@ -1448,6 +1448,90 @@ static const struct lvts_ctrl_data mt8186_lvts_data_ctrl[] = { } }; +static const struct lvts_ctrl_data mt8188_lvts_mcu_data_ctrl[] = { + { + .lvts_sensor = { + { .dt_id = MT8188_MCU_TS1_0, + .cal_offsets = { 22, 23, 24 } }, + { .dt_id = MT8188_MCU_TS1_1, + .cal_offsets = { 25, 26, 27 } }, + { .dt_id = MT8188_MCU_TS1_2, + .cal_offsets = { 28, 29, 30 } }, + { .dt_id = MT8188_MCU_TS1_3, + .cal_offsets = { 31, 32, 33 } }, + }, + VALID_SENSOR_MAP(1, 1, 1, 1), + .offset = 0x0, + .hw_tshut_temp = 117000, + .mode = LVTS_MSR_FILTERED_MODE, + }, + { + .lvts_sensor = { + { .dt_id = MT8188_MCU_TS2_0, + .cal_offsets = { 34, 35, 36 } }, + { .dt_id = MT8188_MCU_TS2_1, + .cal_offsets = { 37, 38, 39 } }, + }, + VALID_SENSOR_MAP(1, 1, 0, 0), + .offset = 0x100, + .hw_tshut_temp = 117000, + .mode = LVTS_MSR_FILTERED_MODE, + } +}; + +static const struct lvts_ctrl_data mt8188_lvts_ap_data_ctrl[] = { + { + .lvts_sensor = { + + { /* unused */ }, + { .dt_id = MT8188_AP_TS3_1, + .cal_offsets = { 40, 41, 42 } }, + }, + VALID_SENSOR_MAP(0, 1, 0, 0), + .offset = 0x0, + .hw_tshut_temp = 117000, + .mode = LVTS_MSR_FILTERED_MODE, + }, + { + .lvts_sensor = { + { .dt_id = MT8188_AP_TS4_0, + .cal_offsets = { 43, 44, 45 } }, + { .dt_id = MT8188_AP_TS4_1, + .cal_offsets = { 46, 47, 48 } }, + { .dt_id = MT8188_AP_TS4_2, + .cal_offsets = { 49, 50, 51 } }, + }, + VALID_SENSOR_MAP(1, 1, 1, 0), + .offset = 0x100, + .hw_tshut_temp = 117000, + .mode = LVTS_MSR_FILTERED_MODE, + }, + { + .lvts_sensor = { + { .dt_id = MT8188_AP_TS5_0, + .cal_offsets = { 52, 53, 54 } }, + { .dt_id = MT8188_AP_TS5_1, + .cal_offsets = { 55, 56, 57 } }, + }, + VALID_SENSOR_MAP(1, 1, 0, 0), + .offset = 0x200, + .hw_tshut_temp = 117000, + .mode = LVTS_MSR_FILTERED_MODE, + }, + { + .lvts_sensor = { + { .dt_id = MT8188_AP_TS6_0, + .cal_offsets = { 58, 59, 60 } }, + { .dt_id = MT8188_AP_TS6_1, + .cal_offsets = { 61, 62, 63 } }, + }, + VALID_SENSOR_MAP(1, 1, 0, 0), + .offset = 0x300, + .hw_tshut_temp = 117000, + .mode = LVTS_MSR_FILTERED_MODE, + } +}; + static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = { { .lvts_sensor = { @@ -1645,6 +1729,22 @@ static const struct lvts_data mt8186_lvts_data = { .gt_calib_bit_offset = 24, }; +static const struct lvts_data mt8188_lvts_mcu_data = { + .lvts_ctrl = mt8188_lvts_mcu_data_ctrl, + .num_lvts_ctrl = ARRAY_SIZE(mt8188_lvts_mcu_data_ctrl), + .temp_factor = -250460, + .temp_offset = 250460, + .gt_calib_bit_offset = 20, +}; + +static const struct lvts_data mt8188_lvts_ap_data = { + .lvts_ctrl = mt8188_lvts_ap_data_ctrl, + .num_lvts_ctrl = ARRAY_SIZE(mt8188_lvts_ap_data_ctrl), + .temp_factor = -250460, + .temp_offset = 250460, + .gt_calib_bit_offset = 20, +}; + static const struct lvts_data mt8192_lvts_mcu_data = { .lvts_ctrl = mt8192_lvts_mcu_data_ctrl, .num_lvts_ctrl = ARRAY_SIZE(mt8192_lvts_mcu_data_ctrl), @@ -1676,6 +1776,8 @@ 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,mt8186-lvts", .data = &mt8186_lvts_data }, + { .compatible = "mediatek,mt8188-lvts-mcu", .data = &mt8188_lvts_mcu_data }, + { .compatible = "mediatek,mt8188-lvts-ap", .data = &mt8188_lvts_ap_data }, { .compatible = "mediatek,mt8192-lvts-mcu", .data = &mt8192_lvts_mcu_data }, { .compatible = "mediatek,mt8192-lvts-ap", .data = &mt8192_lvts_ap_data }, { .compatible = "mediatek,mt8195-lvts-mcu", .data = &mt8195_lvts_mcu_data }, -- 2.44.0
From: Nicolas Pitre <npitre@baylibre.com> Inspired by the vendor kernel but adapted to the upstream thermal driver version. Signed-off-by: Nicolas Pitre <npitre@baylibre.com> --- arch/arm64/boot/dts/mediatek/mt8186.dtsi | 236 +++++++++++++++++++++++ 1 file changed, 236 insertions(+) diff --git a/arch/arm64/boot/dts/mediatek/mt8186.dtsi b/arch/arm64/boot/dts/mediatek/mt8186.dtsi index 7b7a517a41..9865926459 100644 --- a/arch/arm64/boot/dts/mediatek/mt8186.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8186.dtsi @@ -13,6 +13,8 @@ #include <dt-bindings/power/mt8186-power.h> #include <dt-bindings/phy/phy.h> #include <dt-bindings/reset/mt8186-resets.h> +#include <dt-bindings/thermal/thermal.h> +#include <dt-bindings/thermal/mediatek,lvts-thermal.h> / { compatible = "mediatek,mt8186"; @@ -2115,4 +2117,238 @@ larb19: smi@1c10f000 { power-domains = <&spm MT8186_POWER_DOMAIN_IPE>; }; }; + + thermal_zones: thermal-zones { + cluster0-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts MT8186_TS1_0>; + + trips { + cluster0_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cluster0_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cluster0_alert>; + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + cluster1-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts MT8186_TS1_1>; + + trips { + cluster1_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cluster1_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cluster1_alert>; + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + cluster2-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts MT8186_TS1_2>; + + trips { + cluster2_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cluster2_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cluster2_alert>; + cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + cam-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts MT8186_TS1_3>; + + trips { + cam_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cam_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + nna-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts MT8186_TS3_0>; + + trips { + nna_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + nna_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + adsp-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts MT8186_TS3_1>; + + trips { + adsp_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + adsp_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + mfg-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts MT8186_TS3_2>; + + trips { + mfg_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + mfg_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + + cpu_big0-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts MT8186_TS2_0>; + + trips { + cpu_big0_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cpu_big0_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cpu_big0_alert>; + cooling-device = <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + cpu_big1-thermal { + polling-delay = <1000>; + polling-delay-passive = <250>; + thermal-sensors = <&lvts MT8186_TS2_1>; + + trips { + cpu_big1_alert: trip-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cpu_big1_crit: trip-crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&cpu_big1_alert>; + cooling-device = <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + }; }; -- 2.44.0
From: Nicolas Pitre <npitre@baylibre.com> Add LVTS thermal controller definition for MT8186. Signed-off-by: Nicolas Pitre <npitre@baylibre.com> --- .../bindings/thermal/mediatek,lvts-thermal.yaml | 2 ++ include/dt-bindings/thermal/mediatek,lvts-thermal.h | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml index e6665af52e..4173bae530 100644 --- a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml +++ b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml @@ -19,6 +19,7 @@ properties: compatible: enum: - mediatek,mt7988-lvts-ap + - mediatek,mt8186-lvts - mediatek,mt8192-lvts-ap - mediatek,mt8192-lvts-mcu - mediatek,mt8195-lvts-ap @@ -75,6 +76,7 @@ allOf: compatible: contains: enum: + - mediatek,mt8186-lvts - mediatek,mt8195-lvts-ap - mediatek,mt8195-lvts-mcu then: diff --git a/include/dt-bindings/thermal/mediatek,lvts-thermal.h b/include/dt-bindings/thermal/mediatek,lvts-thermal.h index 997e2f5512..3197ca6087 100644 --- a/include/dt-bindings/thermal/mediatek,lvts-thermal.h +++ b/include/dt-bindings/thermal/mediatek,lvts-thermal.h @@ -16,6 +16,16 @@ #define MT7988_ETHWARP_0 6 #define MT7988_ETHWARP_1 7 +#define MT8186_TS1_0 0 +#define MT8186_TS1_1 1 +#define MT8186_TS1_2 2 +#define MT8186_TS1_3 3 +#define MT8186_TS2_0 4 +#define MT8186_TS2_1 5 +#define MT8186_TS3_0 6 +#define MT8186_TS3_1 7 +#define MT8186_TS3_2 8 + #define MT8195_MCU_BIG_CPU0 0 #define MT8195_MCU_BIG_CPU1 1 #define MT8195_MCU_BIG_CPU2 2 -- 2.44.0
From: Nicolas Pitre <npitre@baylibre.com> We don't want to silently fetch garbage past the actual buffer. Signed-off-by: Nicolas Pitre <npitre@baylibre.com> --- drivers/thermal/mediatek/lvts_thermal.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c index 2c346ea7c6..ed1888fb24 100644 --- a/drivers/thermal/mediatek/lvts_thermal.c +++ b/drivers/thermal/mediatek/lvts_thermal.c @@ -674,7 +674,8 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl, */ static int lvts_calibration_init(struct device *dev, struct lvts_ctrl *lvts_ctrl, const struct lvts_ctrl_data *lvts_ctrl_data, - u8 *efuse_calibration) + u8 *efuse_calibration, + size_t calib_len) { int i; @@ -682,6 +683,11 @@ static int lvts_calibration_init(struct device *dev, struct lvts_ctrl *lvts_ctrl const struct lvts_sensor_data *sensor = &lvts_ctrl_data->lvts_sensor[i]; + if (sensor->cal_offsets[0] >= calib_len || + sensor->cal_offsets[1] >= calib_len || + sensor->cal_offsets[2] >= calib_len) + return -EINVAL; + lvts_ctrl->calibration[i] = (efuse_calibration[sensor->cal_offsets[0]] << 0) + (efuse_calibration[sensor->cal_offsets[1]] << 8) + @@ -791,7 +797,8 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td, ret = lvts_calibration_init(dev, &lvts_ctrl[i], &lvts_data->lvts_ctrl[i], - lvts_td->calib); + lvts_td->calib, + lvts_td->calib_len); if (ret) return ret; -- 2.44.0
This is a bunch of patches to support the MT8186 and MT8188 thermal sensor configurations. Several changes are needed to cope with oddities these SOCs implement. All values (calibration data offsets, etc.) were lifted and adapted from the vendor driver source code. Version 1 can be found here: https://lore.kernel.org/all/20240111223020.3593558-1-nico@fluxnic.net/T/ Changes from v1: - renamed CPU cluster thermal zones in DT - fixed logic to cope with empty controller slots at the beginning - isolated bindings to their own patches - added MT8188 default thermal zones diffstat: .../thermal/mediatek,lvts-thermal.yaml | 6 + arch/arm64/boot/dts/mediatek/mt8186.dtsi | 256 ++++++++++++ drivers/thermal/mediatek/lvts_thermal.c | 375 ++++++++++++++---- .../thermal/mediatek,lvts-thermal.h | 26 ++ 4 files changed, 585 insertions(+), 78 deletions(-) .../thermal/mediatek,lvts-thermal.yaml | 6 + arch/arm64/boot/dts/mediatek/mt8186.dtsi | 256 +++++++++++ arch/arm64/boot/dts/mediatek/mt8188.dtsi | 383 ++++++++++++++++ drivers/thermal/mediatek/lvts_thermal.c | 425 ++++++++++++++---- .../thermal/mediatek,lvts-thermal.h | 26 ++ 5 files changed, 997 insertions(+), 99 deletions(-)
From: Nicolas Pitre <npitre@baylibre.com> Calibration values are 24-bit wide. Those values so far appear to span only 16 bits but let's not push our luck. Found while looking at the original Mediatek driver code. Signed-off-by: Nicolas Pitre <npitre@baylibre.com> --- drivers/thermal/mediatek/lvts_thermal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c index 98d9c80bd4..8aa6a8675b 100644 --- a/drivers/thermal/mediatek/lvts_thermal.c +++ b/drivers/thermal/mediatek/lvts_thermal.c @@ -679,7 +679,7 @@ static int lvts_calibration_init(struct device *dev, struct lvts_ctrl *lvts_ctrl for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++) memcpy(&lvts_ctrl->calibration[i], - efuse_calibration + lvts_ctrl_data->cal_offset[i], 2); + efuse_calibration + lvts_ctrl_data->cal_offset[i], 3); return 0; } -- 2.44.0
From: Nicolas Pitre <npitre@baylibre.com> Some systems don't always populate sensor controller slots starting at slot 0. Use a bitmap instead of a count to indicate valid sensor slots. Also create a pretty iterator for that. About that iterator: it causes checkpatch to complain with "ERROR: Macros with multiple statements should be enclosed in a do - while loop". However this is not possible here. And many similar iterators do exist using the same form in the tree already. Signed-off-by: Nicolas Pitre <npitre@baylibre.com> --- drivers/thermal/mediatek/lvts_thermal.c | 64 ++++++++++++++----------- 1 file changed, 37 insertions(+), 27 deletions(-) diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c index b20b70fd36..a23a93fc82 100644 --- a/drivers/thermal/mediatek/lvts_thermal.c +++ b/drivers/thermal/mediatek/lvts_thermal.c @@ -110,12 +110,24 @@ struct lvts_sensor_data { struct lvts_ctrl_data { struct lvts_sensor_data lvts_sensor[LVTS_SENSOR_MAX]; + u8 valid_sensor_mask; int hw_tshut_temp; - int num_lvts_sensor; int offset; int mode; }; +#define VALID_SENSOR_MAP(s0, s1, s2, s3) \ + .valid_sensor_mask = (((s0) ? BIT(0) : 0) | \ + ((s1) ? BIT(1) : 0) | \ + ((s2) ? BIT(2) : 0) | \ + ((s3) ? BIT(3) : 0)) + +#define lvts_for_each_valid_sensor(i, lvts_ctrl_data) \ + for ((i) = 0; (i) < LVTS_SENSOR_MAX; (i)++) \ + if (!((lvts_ctrl_data)->valid_sensor_mask & BIT(i))) \ + continue; \ + else + struct lvts_data { const struct lvts_ctrl_data *lvts_ctrl; int num_lvts_ctrl; @@ -139,7 +151,6 @@ struct lvts_ctrl { const struct lvts_data *lvts_data; u32 calibration[LVTS_SENSOR_MAX]; u32 hw_tshut_raw_temp; - int num_lvts_sensor; int mode; void __iomem *base; int low_thresh; @@ -351,7 +362,7 @@ static bool lvts_should_update_thresh(struct lvts_ctrl *lvts_ctrl, int high) if (high > lvts_ctrl->high_thresh) return true; - for (i = 0; i < lvts_ctrl->num_lvts_sensor; i++) + lvts_for_each_valid_sensor(i, lvts_ctrl->lvts_data->lvts_ctrl) if (lvts_ctrl->sensors[i].high_thresh == lvts_ctrl->high_thresh && lvts_ctrl->sensors[i].low_thresh == lvts_ctrl->low_thresh) return false; @@ -555,6 +566,7 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl, const struct lvts_ctrl_data *lvts_ctrl_data) { struct lvts_sensor *lvts_sensor = lvts_ctrl->sensors; + void __iomem *msr_regs[] = { LVTS_MSR0(lvts_ctrl->base), LVTS_MSR1(lvts_ctrl->base), @@ -571,7 +583,7 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl, int i; - for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++) { + lvts_for_each_valid_sensor(i, lvts_ctrl_data) { int dt_id = lvts_ctrl_data->lvts_sensor[i].dt_id; @@ -611,8 +623,6 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl, lvts_sensor[i].high_thresh = INT_MIN; }; - lvts_ctrl->num_lvts_sensor = lvts_ctrl_data->num_lvts_sensor; - return 0; } @@ -683,7 +693,7 @@ static int lvts_calibration_init(struct device *dev, struct lvts_ctrl *lvts_ctrl { int i; - for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++) { + lvts_for_each_valid_sensor(i, lvts_ctrl_data) { const struct lvts_sensor_data *sensor = &lvts_ctrl_data->lvts_sensor[i]; @@ -1106,7 +1116,7 @@ static int lvts_ctrl_start(struct device *dev, struct lvts_ctrl *lvts_ctrl) u32 *sensor_bitmap = lvts_ctrl->mode == LVTS_MSR_IMMEDIATE_MODE ? sensor_imm_bitmap : sensor_filt_bitmap; - for (i = 0; i < lvts_ctrl->num_lvts_sensor; i++) { + lvts_for_each_valid_sensor(i, lvts_ctrl->lvts_data->lvts_ctrl) { int dt_id = lvts_sensors[i].dt_id; @@ -1329,7 +1339,7 @@ static const struct lvts_ctrl_data mt7988_lvts_ap_data_ctrl[] = { { .dt_id = MT7988_ETH2P5G_1, .cal_offsets = { 0x0c, 0x0d, 0x0e } } }, - .num_lvts_sensor = 4, + VALID_SENSOR_MAP(1, 1, 1, 1), .offset = 0x0, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988, }, @@ -1344,7 +1354,7 @@ static const struct lvts_ctrl_data mt7988_lvts_ap_data_ctrl[] = { { .dt_id = MT7988_ETHWARP_1, .cal_offsets = { 0x20, 0x21, 0x22 } } }, - .num_lvts_sensor = 4, + VALID_SENSOR_MAP(1, 1, 1, 1), .offset = 0x100, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988, } @@ -1408,7 +1418,7 @@ static const struct lvts_ctrl_data mt8186_lvts_data_ctrl[] = { { .dt_id = MT8186_TS1_3, .cal_offsets = { 12, 13, 14 } } }, - .num_lvts_sensor = 4, + VALID_SENSOR_MAP(1, 1, 1, 1), .offset = 0x0, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8186, }, @@ -1419,7 +1429,7 @@ static const struct lvts_ctrl_data mt8186_lvts_data_ctrl[] = { { .dt_id = MT8186_TS2_1, .cal_offsets = { 27, 20, 21 } } }, - .num_lvts_sensor = 2, + VALID_SENSOR_MAP(1, 1, 0, 0), .offset = 0x100, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8186, }, @@ -1432,7 +1442,7 @@ static const struct lvts_ctrl_data mt8186_lvts_data_ctrl[] = { { .dt_id = MT8186_TS3_2, .cal_offsets = { 39, 32, 33 } } }, - .num_lvts_sensor = 3, + VALID_SENSOR_MAP(1, 1, 1, 0), .offset = 0x200, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8186, } @@ -1446,7 +1456,7 @@ static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = { { .dt_id = MT8192_MCU_BIG_CPU1, .cal_offsets = { 0x08, 0x09, 0x0a } } }, - .num_lvts_sensor = 2, + VALID_SENSOR_MAP(1, 1, 0, 0), .offset = 0x0, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8192, .mode = LVTS_MSR_FILTERED_MODE, @@ -1458,7 +1468,7 @@ static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = { { .dt_id = MT8192_MCU_BIG_CPU3, .cal_offsets = { 0x10, 0x11, 0x12 } } }, - .num_lvts_sensor = 2, + VALID_SENSOR_MAP(1, 1, 0, 0), .offset = 0x100, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8192, .mode = LVTS_MSR_FILTERED_MODE, @@ -1474,7 +1484,7 @@ static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = { { .dt_id = MT8192_MCU_LITTLE_CPU3, .cal_offsets = { 0x20, 0x21, 0x22 } } }, - .num_lvts_sensor = 4, + VALID_SENSOR_MAP(1, 1, 1, 1), .offset = 0x200, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8192, .mode = LVTS_MSR_FILTERED_MODE, @@ -1489,7 +1499,7 @@ static const struct lvts_ctrl_data mt8192_lvts_ap_data_ctrl[] = { { .dt_id = MT8192_AP_VPU1, .cal_offsets = { 0x28, 0x29, 0x2a } } }, - .num_lvts_sensor = 2, + VALID_SENSOR_MAP(1, 1, 0, 0), .offset = 0x0, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8192, }, @@ -1500,7 +1510,7 @@ static const struct lvts_ctrl_data mt8192_lvts_ap_data_ctrl[] = { { .dt_id = MT8192_AP_GPU1, .cal_offsets = { 0x30, 0x31, 0x32 } } }, - .num_lvts_sensor = 2, + VALID_SENSOR_MAP(1, 1, 0, 0), .offset = 0x100, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8192, }, @@ -1511,7 +1521,7 @@ static const struct lvts_ctrl_data mt8192_lvts_ap_data_ctrl[] = { { .dt_id = MT8192_AP_CAM, .cal_offsets = { 0x38, 0x39, 0x3a } }, }, - .num_lvts_sensor = 2, + VALID_SENSOR_MAP(1, 1, 0, 0), .offset = 0x200, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8192, }, @@ -1524,7 +1534,7 @@ static const struct lvts_ctrl_data mt8192_lvts_ap_data_ctrl[] = { { .dt_id = MT8192_AP_MD2, .cal_offsets = { 0x44, 0x45, 0x46 } } }, - .num_lvts_sensor = 3, + VALID_SENSOR_MAP(1, 1, 1, 0), .offset = 0x300, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8192, } @@ -1538,7 +1548,7 @@ static const struct lvts_ctrl_data mt8195_lvts_mcu_data_ctrl[] = { { .dt_id = MT8195_MCU_BIG_CPU1, .cal_offsets = { 0x07, 0x08, 0x09 } } }, - .num_lvts_sensor = 2, + VALID_SENSOR_MAP(1, 1, 0, 0), .offset = 0x0, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195, }, @@ -1549,7 +1559,7 @@ static const struct lvts_ctrl_data mt8195_lvts_mcu_data_ctrl[] = { { .dt_id = MT8195_MCU_BIG_CPU3, .cal_offsets = { 0x10, 0x11, 0x12 } } }, - .num_lvts_sensor = 2, + VALID_SENSOR_MAP(1, 1, 0, 0), .offset = 0x100, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195, }, @@ -1564,7 +1574,7 @@ static const struct lvts_ctrl_data mt8195_lvts_mcu_data_ctrl[] = { { .dt_id = MT8195_MCU_LITTLE_CPU3, .cal_offsets = { 0x1f, 0x20, 0x21 } } }, - .num_lvts_sensor = 4, + VALID_SENSOR_MAP(1, 1, 1, 1), .offset = 0x200, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195, } @@ -1578,7 +1588,7 @@ static const struct lvts_ctrl_data mt8195_lvts_ap_data_ctrl[] = { { .dt_id = MT8195_AP_VPU1, .cal_offsets = { 0x28, 0x29, 0x2a } } }, - .num_lvts_sensor = 2, + VALID_SENSOR_MAP(1, 1, 0, 0), .offset = 0x0, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195, }, @@ -1589,7 +1599,7 @@ static const struct lvts_ctrl_data mt8195_lvts_ap_data_ctrl[] = { { .dt_id = MT8195_AP_GPU1, .cal_offsets = { 0x31, 0x32, 0x33 } } }, - .num_lvts_sensor = 2, + VALID_SENSOR_MAP(1, 1, 0, 0), .offset = 0x100, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195, }, @@ -1602,7 +1612,7 @@ static const struct lvts_ctrl_data mt8195_lvts_ap_data_ctrl[] = { { .dt_id = MT8195_AP_INFRA, .cal_offsets = { 0x3d, 0x3e, 0x3f } } }, - .num_lvts_sensor = 3, + VALID_SENSOR_MAP(1, 1, 1, 0), .offset = 0x200, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195, }, @@ -1613,7 +1623,7 @@ static const struct lvts_ctrl_data mt8195_lvts_ap_data_ctrl[] = { { .dt_id = MT8195_AP_CAM1, .cal_offsets = { 0x46, 0x47, 0x48 } } }, - .num_lvts_sensor = 2, + VALID_SENSOR_MAP(1, 1, 0, 0), .offset = 0x300, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195, } -- 2.44.0
From: Nicolas Pitre <npitre@baylibre.com> Add LVTS thermal controller definition for MT8188. Signed-off-by: Nicolas Pitre <npitre@baylibre.com> --- .../bindings/thermal/mediatek,lvts-thermal.yaml | 4 ++++ .../dt-bindings/thermal/mediatek,lvts-thermal.h | 16 ++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml index 4173bae530..331cf4e662 100644 --- a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml +++ b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml @@ -20,6 +20,8 @@ properties: enum: - mediatek,mt7988-lvts-ap - mediatek,mt8186-lvts + - mediatek,mt8188-lvts-ap + - mediatek,mt8188-lvts-mcu - mediatek,mt8192-lvts-ap - mediatek,mt8192-lvts-mcu - mediatek,mt8195-lvts-ap @@ -61,6 +63,8 @@ allOf: compatible: contains: enum: + - mediatek,mt8188-lvts-ap + - mediatek,mt8188-lvts-mcu - mediatek,mt8192-lvts-ap - mediatek,mt8192-lvts-mcu then: diff --git a/include/dt-bindings/thermal/mediatek,lvts-thermal.h b/include/dt-bindings/thermal/mediatek,lvts-thermal.h index 3197ca6087..04fa9d7821 100644 --- a/include/dt-bindings/thermal/mediatek,lvts-thermal.h +++ b/include/dt-bindings/thermal/mediatek,lvts-thermal.h @@ -26,6 +26,22 @@ #define MT8186_TS3_1 7 #define MT8186_TS3_2 8 +#define MT8188_MCU_TS1_0 0 +#define MT8188_MCU_TS1_1 1 +#define MT8188_MCU_TS1_2 2 +#define MT8188_MCU_TS1_3 3 +#define MT8188_MCU_TS2_0 4 +#define MT8188_MCU_TS2_1 5 + +#define MT8188_AP_TS3_1 0 +#define MT8188_AP_TS4_0 1 +#define MT8188_AP_TS4_1 2 +#define MT8188_AP_TS4_2 3 +#define MT8188_AP_TS5_0 4 +#define MT8188_AP_TS5_1 5 +#define MT8188_AP_TS6_0 6 +#define MT8188_AP_TS6_1 7 + #define MT8195_MCU_BIG_CPU0 0 #define MT8195_MCU_BIG_CPU1 1 #define MT8195_MCU_BIG_CPU2 2 -- 2.44.0
From: Nicolas Pitre <npitre@baylibre.com> Various values extracted from the vendor's kernel driver. Signed-off-by: Nicolas Pitre <npitre@baylibre.com> --- drivers/thermal/mediatek/lvts_thermal.c | 67 +++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c index ed1888fb24..e923d22c17 100644 --- a/drivers/thermal/mediatek/lvts_thermal.c +++ b/drivers/thermal/mediatek/lvts_thermal.c @@ -80,6 +80,8 @@ #define LVTS_SENSOR_MAX 4 #define LVTS_GOLDEN_TEMP_MAX 62 #define LVTS_GOLDEN_TEMP_DEFAULT 50 +#define LVTS_COEFF_A_MT8186 -204650 +#define LVTS_COEFF_B_MT8186 204650 #define LVTS_COEFF_A_MT8195 -250460 #define LVTS_COEFF_B_MT8195 250460 #define LVTS_COEFF_A_MT7988 -204650 @@ -92,6 +94,7 @@ #define LVTS_MSR_READ_WAIT_US (LVTS_MSR_READ_TIMEOUT_US / 2) #define LVTS_HW_SHUTDOWN_MT7988 105000 +#define LVTS_HW_SHUTDOWN_MT8186 105000 #define LVTS_HW_SHUTDOWN_MT8192 105000 #define LVTS_HW_SHUTDOWN_MT8195 105000 @@ -1377,6 +1380,62 @@ static int lvts_resume(struct device *dev) return 0; } +/* + * The MT8186 calibration data is stored as packed 3-byte little-endian + * values using a weird layout that makes sense only when viewed as a 32-bit + * hexadecimal word dump. Let's suppose SxBy where x = sensor number and + * y = byte number where the LSB is y=0. We then have: + * + * [S0B2-S0B1-S0B0-S1B2] [S1B1-S1B0-S2B2-S2B1] [S2B0-S3B2-S3B1-S3B0] + * + * However, when considering a byte stream, those appear as follows: + * + * [S1B2] [S0B0[ [S0B1] [S0B2] [S2B1] [S2B2] [S1B0] [S1B1] [S3B0] [S3B1] [S3B2] [S2B0] + * + * Hence the rather confusing offsets provided below. + */ +static const struct lvts_ctrl_data mt8186_lvts_data_ctrl[] = { + { + .lvts_sensor = { + { .dt_id = MT8186_TS1_0, + .cal_offsets = { 5, 6, 7 } }, + { .dt_id = MT8186_TS1_1, + .cal_offsets = { 10, 11, 4 } }, + { .dt_id = MT8186_TS1_2, + .cal_offsets = { 15, 8, 9 } }, + { .dt_id = MT8186_TS1_3, + .cal_offsets = { 12, 13, 14 } } + }, + .num_lvts_sensor = 4, + .offset = 0x0, + .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8186, + }, + { + .lvts_sensor = { + { .dt_id = MT8186_TS2_0, + .cal_offsets = { 22, 23, 16 } }, + { .dt_id = MT8186_TS2_1, + .cal_offsets = { 27, 20, 21 } } + }, + .num_lvts_sensor = 2, + .offset = 0x100, + .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8186, + }, + { + .lvts_sensor = { + { .dt_id = MT8186_TS3_0, + .cal_offsets = { 29, 30, 31 } }, + { .dt_id = MT8186_TS3_1, + .cal_offsets = { 34, 35, 28 } }, + { .dt_id = MT8186_TS3_2, + .cal_offsets = { 39, 32, 33 } } + }, + .num_lvts_sensor = 3, + .offset = 0x200, + .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8186, + } +}; + static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = { { .lvts_sensor = { @@ -1565,6 +1624,13 @@ static const struct lvts_data mt7988_lvts_ap_data = { .temp_offset = LVTS_COEFF_B_MT7988, }; +static const struct lvts_data mt8186_lvts_data = { + .lvts_ctrl = mt8186_lvts_data_ctrl, + .num_lvts_ctrl = ARRAY_SIZE(mt8186_lvts_data_ctrl), + .temp_factor = LVTS_COEFF_A_MT8186, + .temp_offset = LVTS_COEFF_B_MT8186, +}; + static const struct lvts_data mt8192_lvts_mcu_data = { .lvts_ctrl = mt8192_lvts_mcu_data_ctrl, .num_lvts_ctrl = ARRAY_SIZE(mt8192_lvts_mcu_data_ctrl), @@ -1591,6 +1657,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,mt8186-lvts", .data = &mt8186_lvts_data }, { .compatible = "mediatek,mt8192-lvts-mcu", .data = &mt8192_lvts_mcu_data }, { .compatible = "mediatek,mt8192-lvts-ap", .data = &mt8192_lvts_ap_data }, { .compatible = "mediatek,mt8195-lvts-mcu", .data = &mt8195_lvts_mcu_data }, -- 2.44.0
From: Nicolas Pitre <npitre@baylibre.com> Values extracted from vendor source tree. Signed-off-by: Nicolas Pitre <npitre@baylibre.com> --- arch/arm64/boot/dts/mediatek/mt8186.dtsi | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/arch/arm64/boot/dts/mediatek/mt8186.dtsi b/arch/arm64/boot/dts/mediatek/mt8186.dtsi index 2fec6fd1c1..7b7a517a41 100644 --- a/arch/arm64/boot/dts/mediatek/mt8186.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8186.dtsi @@ -1355,6 +1355,18 @@ spi0: spi@1100a000 { status = "disabled"; }; + lvts: lvts@1100b000 { + compatible = "mediatek,mt8186-lvts"; + #thermal-sensor-cells = <1>; + reg = <0 0x1100b000 0 0x1000>; + interrupts = <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH 0>; + clocks = <&infracfg_ao CLK_INFRA_AO_THERM>; + clock-names = "lvts_clk"; + resets = <&infracfg_ao MT8186_INFRA_THERMAL_CTRL_RST>; + nvmem-cells = <&lvts_e_data1 &lvts_e_data2>; + nvmem-cell-names = "e_data1","e_data2"; + }; + pwm0: pwm@1100e000 { compatible = "mediatek,mt8186-disp-pwm", "mediatek,mt8183-disp-pwm"; reg = <0 0x1100e000 0 0x1000>; @@ -1668,6 +1680,14 @@ efuse: efuse@11cb0000 { #address-cells = <1>; #size-cells = <1>; + lvts_e_data1: data1 { + reg = <0x1cc 0x14>; + }; + + lvts_e_data2: data1-1 { + reg = <0x2f8 0x14>; + }; + gpu_speedbin: gpu-speedbin@59c { reg = <0x59c 0x4>; bits = <0 3>; -- 2.44.0
From: Nicolas Pitre <npitre@baylibre.com> Current code assumes calibration values are always stored contiguously in host endian order. A future patch will prove this wrong. Let's specify the offset for each calibration byte instead. Signed-off-by: Nicolas Pitre <npitre@baylibre.com> --- drivers/thermal/mediatek/lvts_thermal.c | 165 ++++++++++++++---------- 1 file changed, 99 insertions(+), 66 deletions(-) diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c index 73ca2be0f5..2c346ea7c6 100644 --- a/drivers/thermal/mediatek/lvts_thermal.c +++ b/drivers/thermal/mediatek/lvts_thermal.c @@ -102,11 +102,11 @@ static int golden_temp_offset; struct lvts_sensor_data { int dt_id; + u8 cal_offsets[3]; }; struct lvts_ctrl_data { struct lvts_sensor_data lvts_sensor[LVTS_SENSOR_MAX]; - int cal_offset[LVTS_SENSOR_MAX]; int hw_tshut_temp; int num_lvts_sensor; int offset; @@ -668,8 +668,9 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl, * <-----ap--tc#3-----> <-----sensor#7-----> <-----sensor#8-----> * 0x40 | 0x41 | 0x42 | 0x43 | 0x44 | 0x45 | 0x46 | 0x47 | 0x48 * - * The data description gives the offset of the calibration data in - * this bytes stream for each sensor. + * Note: In some cases, values don't strictly follow a little endian ordering. + * The data description gives byte offsets constituting each calibration value + * for each sensor. */ static int lvts_calibration_init(struct device *dev, struct lvts_ctrl *lvts_ctrl, const struct lvts_ctrl_data *lvts_ctrl_data, @@ -677,9 +678,15 @@ static int lvts_calibration_init(struct device *dev, struct lvts_ctrl *lvts_ctrl { int i; - for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++) - memcpy(&lvts_ctrl->calibration[i], - efuse_calibration + lvts_ctrl_data->cal_offset[i], 3); + for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++) { + const struct lvts_sensor_data *sensor = + &lvts_ctrl_data->lvts_sensor[i]; + + lvts_ctrl->calibration[i] = + (efuse_calibration[sensor->cal_offsets[0]] << 0) + + (efuse_calibration[sensor->cal_offsets[1]] << 8) + + (efuse_calibration[sensor->cal_offsets[2]] << 16); + } return 0; } @@ -1300,24 +1307,30 @@ static void lvts_remove(struct platform_device *pdev) static const struct lvts_ctrl_data mt7988_lvts_ap_data_ctrl[] = { { - .cal_offset = { 0x00, 0x04, 0x08, 0x0c }, .lvts_sensor = { - { .dt_id = MT7988_CPU_0 }, - { .dt_id = MT7988_CPU_1 }, - { .dt_id = MT7988_ETH2P5G_0 }, - { .dt_id = MT7988_ETH2P5G_1 } + { .dt_id = MT7988_CPU_0, + .cal_offsets = { 0x00, 0x01, 0x02 } }, + { .dt_id = MT7988_CPU_1, + .cal_offsets = { 0x04, 0x05, 0x06 } }, + { .dt_id = MT7988_ETH2P5G_0, + .cal_offsets = { 0x08, 0x09, 0x0a } }, + { .dt_id = MT7988_ETH2P5G_1, + .cal_offsets = { 0x0c, 0x0d, 0x0e } } }, .num_lvts_sensor = 4, .offset = 0x0, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988, }, { - .cal_offset = { 0x14, 0x18, 0x1c, 0x20 }, .lvts_sensor = { - { .dt_id = MT7988_TOPS_0}, - { .dt_id = MT7988_TOPS_1}, - { .dt_id = MT7988_ETHWARP_0}, - { .dt_id = MT7988_ETHWARP_1} + { .dt_id = MT7988_TOPS_0, + .cal_offsets = { 0x14, 0x15, 0x16 } }, + { .dt_id = MT7988_TOPS_1, + .cal_offsets = { 0x18, 0x19, 0x1a } }, + { .dt_id = MT7988_ETHWARP_0, + .cal_offsets = { 0x1c, 0x1d, 0x1e } }, + { .dt_id = MT7988_ETHWARP_1, + .cal_offsets = { 0x20, 0x21, 0x22 } } }, .num_lvts_sensor = 4, .offset = 0x100, @@ -1359,10 +1372,11 @@ static int lvts_resume(struct device *dev) static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = { { - .cal_offset = { 0x04, 0x08 }, .lvts_sensor = { - { .dt_id = MT8192_MCU_BIG_CPU0 }, - { .dt_id = MT8192_MCU_BIG_CPU1 } + { .dt_id = MT8192_MCU_BIG_CPU0, + .cal_offsets = { 0x04, 0x05, 0x06 } }, + { .dt_id = MT8192_MCU_BIG_CPU1, + .cal_offsets = { 0x08, 0x09, 0x0a } } }, .num_lvts_sensor = 2, .offset = 0x0, @@ -1370,10 +1384,11 @@ static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = { .mode = LVTS_MSR_FILTERED_MODE, }, { - .cal_offset = { 0x0c, 0x10 }, .lvts_sensor = { - { .dt_id = MT8192_MCU_BIG_CPU2 }, - { .dt_id = MT8192_MCU_BIG_CPU3 } + { .dt_id = MT8192_MCU_BIG_CPU2, + .cal_offsets = { 0x0c, 0x0d, 0x0e } }, + { .dt_id = MT8192_MCU_BIG_CPU3, + .cal_offsets = { 0x10, 0x11, 0x12 } } }, .num_lvts_sensor = 2, .offset = 0x100, @@ -1381,12 +1396,15 @@ static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = { .mode = LVTS_MSR_FILTERED_MODE, }, { - .cal_offset = { 0x14, 0x18, 0x1c, 0x20 }, .lvts_sensor = { - { .dt_id = MT8192_MCU_LITTLE_CPU0 }, - { .dt_id = MT8192_MCU_LITTLE_CPU1 }, - { .dt_id = MT8192_MCU_LITTLE_CPU2 }, - { .dt_id = MT8192_MCU_LITTLE_CPU3 } + { .dt_id = MT8192_MCU_LITTLE_CPU0, + .cal_offsets = { 0x14, 0x15, 0x16 } }, + { .dt_id = MT8192_MCU_LITTLE_CPU1, + .cal_offsets = { 0x18, 0x19, 0x1a } }, + { .dt_id = MT8192_MCU_LITTLE_CPU2, + .cal_offsets = { 0x1c, 0x1d, 0x1e } }, + { .dt_id = MT8192_MCU_LITTLE_CPU3, + .cal_offsets = { 0x20, 0x21, 0x22 } } }, .num_lvts_sensor = 4, .offset = 0x200, @@ -1396,42 +1414,47 @@ static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = { }; static const struct lvts_ctrl_data mt8192_lvts_ap_data_ctrl[] = { - { - .cal_offset = { 0x24, 0x28 }, + { .lvts_sensor = { - { .dt_id = MT8192_AP_VPU0 }, - { .dt_id = MT8192_AP_VPU1 } + { .dt_id = MT8192_AP_VPU0, + .cal_offsets = { 0x24, 0x25, 0x26 } }, + { .dt_id = MT8192_AP_VPU1, + .cal_offsets = { 0x28, 0x29, 0x2a } } }, .num_lvts_sensor = 2, .offset = 0x0, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8192, }, { - .cal_offset = { 0x2c, 0x30 }, .lvts_sensor = { - { .dt_id = MT8192_AP_GPU0 }, - { .dt_id = MT8192_AP_GPU1 } + { .dt_id = MT8192_AP_GPU0, + .cal_offsets = { 0x2c, 0x2d, 0x2e } }, + { .dt_id = MT8192_AP_GPU1, + .cal_offsets = { 0x30, 0x31, 0x32 } } }, .num_lvts_sensor = 2, .offset = 0x100, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8192, }, { - .cal_offset = { 0x34, 0x38 }, .lvts_sensor = { - { .dt_id = MT8192_AP_INFRA }, - { .dt_id = MT8192_AP_CAM }, + { .dt_id = MT8192_AP_INFRA, + .cal_offsets = { 0x34, 0x35, 0x36 } }, + { .dt_id = MT8192_AP_CAM, + .cal_offsets = { 0x38, 0x39, 0x3a } }, }, .num_lvts_sensor = 2, .offset = 0x200, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8192, }, { - .cal_offset = { 0x3c, 0x40, 0x44 }, .lvts_sensor = { - { .dt_id = MT8192_AP_MD0 }, - { .dt_id = MT8192_AP_MD1 }, - { .dt_id = MT8192_AP_MD2 } + { .dt_id = MT8192_AP_MD0, + .cal_offsets = { 0x3c, 0x3d, 0x3e } }, + { .dt_id = MT8192_AP_MD1, + .cal_offsets = { 0x40, 0x41, 0x42 } }, + { .dt_id = MT8192_AP_MD2, + .cal_offsets = { 0x44, 0x45, 0x46 } } }, .num_lvts_sensor = 3, .offset = 0x300, @@ -1441,32 +1464,37 @@ static const struct lvts_ctrl_data mt8192_lvts_ap_data_ctrl[] = { static const struct lvts_ctrl_data mt8195_lvts_mcu_data_ctrl[] = { { - .cal_offset = { 0x04, 0x07 }, .lvts_sensor = { - { .dt_id = MT8195_MCU_BIG_CPU0 }, - { .dt_id = MT8195_MCU_BIG_CPU1 } + { .dt_id = MT8195_MCU_BIG_CPU0, + .cal_offsets = { 0x04, 0x05, 0x06 } }, + { .dt_id = MT8195_MCU_BIG_CPU1, + .cal_offsets = { 0x07, 0x08, 0x09 } } }, .num_lvts_sensor = 2, .offset = 0x0, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195, }, { - .cal_offset = { 0x0d, 0x10 }, .lvts_sensor = { - { .dt_id = MT8195_MCU_BIG_CPU2 }, - { .dt_id = MT8195_MCU_BIG_CPU3 } + { .dt_id = MT8195_MCU_BIG_CPU2, + .cal_offsets = { 0x0d, 0x0e, 0x0f } }, + { .dt_id = MT8195_MCU_BIG_CPU3, + .cal_offsets = { 0x10, 0x11, 0x12 } } }, .num_lvts_sensor = 2, .offset = 0x100, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195, }, { - .cal_offset = { 0x16, 0x19, 0x1c, 0x1f }, .lvts_sensor = { - { .dt_id = MT8195_MCU_LITTLE_CPU0 }, - { .dt_id = MT8195_MCU_LITTLE_CPU1 }, - { .dt_id = MT8195_MCU_LITTLE_CPU2 }, - { .dt_id = MT8195_MCU_LITTLE_CPU3 } + { .dt_id = MT8195_MCU_LITTLE_CPU0, + .cal_offsets = { 0x16, 0x17, 0x18 } }, + { .dt_id = MT8195_MCU_LITTLE_CPU1, + .cal_offsets = { 0x19, 0x1a, 0x1b } }, + { .dt_id = MT8195_MCU_LITTLE_CPU2, + .cal_offsets = { 0x1c, 0x1d, 0x1e } }, + { .dt_id = MT8195_MCU_LITTLE_CPU3, + .cal_offsets = { 0x1f, 0x20, 0x21 } } }, .num_lvts_sensor = 4, .offset = 0x200, @@ -1475,42 +1503,47 @@ static const struct lvts_ctrl_data mt8195_lvts_mcu_data_ctrl[] = { }; static const struct lvts_ctrl_data mt8195_lvts_ap_data_ctrl[] = { - { - .cal_offset = { 0x25, 0x28 }, + { .lvts_sensor = { - { .dt_id = MT8195_AP_VPU0 }, - { .dt_id = MT8195_AP_VPU1 } + { .dt_id = MT8195_AP_VPU0, + .cal_offsets = { 0x25, 0x26, 0x27 } }, + { .dt_id = MT8195_AP_VPU1, + .cal_offsets = { 0x28, 0x29, 0x2a } } }, .num_lvts_sensor = 2, .offset = 0x0, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195, }, { - .cal_offset = { 0x2e, 0x31 }, .lvts_sensor = { - { .dt_id = MT8195_AP_GPU0 }, - { .dt_id = MT8195_AP_GPU1 } + { .dt_id = MT8195_AP_GPU0, + .cal_offsets = { 0x2e, 0x2f, 0x30 } }, + { .dt_id = MT8195_AP_GPU1, + .cal_offsets = { 0x31, 0x32, 0x33 } } }, .num_lvts_sensor = 2, .offset = 0x100, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195, }, { - .cal_offset = { 0x37, 0x3a, 0x3d }, .lvts_sensor = { - { .dt_id = MT8195_AP_VDEC }, - { .dt_id = MT8195_AP_IMG }, - { .dt_id = MT8195_AP_INFRA }, + { .dt_id = MT8195_AP_VDEC, + .cal_offsets = { 0x37, 0x38, 0x39 } }, + { .dt_id = MT8195_AP_IMG, + .cal_offsets = { 0x3a, 0x3b, 0x3c } }, + { .dt_id = MT8195_AP_INFRA, + .cal_offsets = { 0x3d, 0x3e, 0x3f } } }, .num_lvts_sensor = 3, .offset = 0x200, .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195, }, { - .cal_offset = { 0x43, 0x46 }, .lvts_sensor = { - { .dt_id = MT8195_AP_CAM0 }, - { .dt_id = MT8195_AP_CAM1 } + { .dt_id = MT8195_AP_CAM0, + .cal_offsets = { 0x43, 0x44, 0x45 } }, + { .dt_id = MT8195_AP_CAM1, + .cal_offsets = { 0x46, 0x47, 0x48 } } }, .num_lvts_sensor = 2, .offset = 0x300, -- 2.44.0
From: Nicolas Pitre <npitre@baylibre.com> Move efuse data interpretation inside lvts_golden_temp_init() alongside the actual code retrieving wanted value. Signed-off-by: Nicolas Pitre <npitre@baylibre.com> --- drivers/thermal/mediatek/lvts_thermal.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c index 8aa6a8675b..73ca2be0f5 100644 --- a/drivers/thermal/mediatek/lvts_thermal.c +++ b/drivers/thermal/mediatek/lvts_thermal.c @@ -732,11 +732,15 @@ 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, int temp_offset) +static int lvts_golden_temp_init(struct device *dev, u8 *calib, int temp_offset) { u32 gt; - gt = (*value) >> 24; + /* + * The golden temp information is contained in the 4th byte (index = 3) + * of efuse data. + */ + gt = calib[3]; if (gt && gt < LVTS_GOLDEN_TEMP_MAX) golden_temp = gt; @@ -760,11 +764,7 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td, if (ret) return ret; - /* - * The golden temp information is contained in the first chunk - * of efuse data. - */ - ret = lvts_golden_temp_init(dev, (u32 *)lvts_td->calib, lvts_data->temp_offset); + ret = lvts_golden_temp_init(dev, lvts_td->calib, lvts_data->temp_offset); if (ret) return ret; -- 2.44.0
>>
>> -----Original Message-----
>> From: linux-arm-kernel <linux-arm-kernel-bounces@lists.infradead.org> On Behalf Of Andre Przywara
>> Sent: Monday, March 18, 2024 6:42 AM
>> To: Yangtao Li <tiny.windzz@gmail.com>; Viresh Kumar <vireshk@kernel.org>; Nishanth Menon <nm@ti.com>; Stephen Boyd <sboyd@kernel.org>; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; Chen-Yu Tsai <wens@csie.org>; Jernej Skrabec <jernej.skrabec@gmail.com>; Samuel Holland <samuel@sholland.org>; Rafael J . Wysocki <rafael@kernel.org>
>> Cc: linux-pm@vger.kernel.org; devicetree@vger.kernel.org; linux-sunxi@lists.linux.dev; linux-arm-kernel@lists.infradead.org; Brandon Cheo Fusi <fusibrandon13@gmail.com>; Martin Botka <martin.botka@somainline.org>; Martin Botka <martin.botka1@gmail.com>
>> Subject: [EXTERNAL] [PATCH v2 8/8] arm64: dts: allwinner: h616: enable DVFS for all boards
>>
>>
>> With the DT bindings now describing the format of the CPU OPP tables, we can include the OPP table in each board's .dts file, and specify the CPU power supply.
>> This allows to enable DVFS, and get up to 50% of performance benefit in the highest OPP, or up to 60% power savings in the lowest OPP, compared to the fixed 1GHz @ 1.0V OPP we are running in by default
>> at the moment.
>> [Amit] Could you please elaborate, what test were run to see 50 % performance benefits?
>
> Currently all H616 boards running mainline firmware and kernels run at a
> fixed 1GHz CPU clock frequency. If you happen to have a good SoC (bin 1 or
> 3), this patchset will allow you to run at 1.5 GHz, which is 50% faster.
> So anything that scales with CPU frequency should run much quicker.
>
Okay, it would be interesting to see results of some benchmark here.
Thanks
-Amit
If, for example, the power button is configured to suspend, holding it and releasing it after the machine has suspended, will wake the machine. Also on some machines, power button release events are sent during hibernation, even if the button wasn't used to hibernate the machine. This causes hibernation to be aborted. Signed-off-by: David McFarland <corngood@gmail.com> --- drivers/platform/x86/intel/hid.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/platform/x86/intel/hid.c b/drivers/platform/x86/intel/hid.c index 7457ca2b27a6..707de9895965 100644 --- a/drivers/platform/x86/intel/hid.c +++ b/drivers/platform/x86/intel/hid.c @@ -504,6 +504,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context) struct platform_device *device = context; struct intel_hid_priv *priv = dev_get_drvdata(&device->dev); unsigned long long ev_index; + struct key_entry *ke; int err; /* @@ -545,11 +546,16 @@ static void notify_handler(acpi_handle handle, u32 event, void *context) if (event == 0xc0 || !priv->array) return; - if (!sparse_keymap_entry_from_scancode(priv->array, event)) { + ke = sparse_keymap_entry_from_scancode(priv->array, event); + + if (!ke) { dev_info(&device->dev, "unknown event 0x%x\n", event); return; } + if (ke->type == KE_IGNORE) + return; + wakeup: pm_wakeup_hard_event(&device->dev); -- 2.42.0
I have a Dell Latitude 7430, and recently whenever I hibernate with `systemctl hibernate`, the machine would immediately wake. I bisected it to: 0c4cae1bc00d PM: hibernate: Avoid missing wakeup events during hibernation However, the underlying problem seems to be that during hibernation, my system gets a 0xcf (power button release) event, and the above change causes it to abort hibernation correctly. I also noticed that holding the power button down (when it's configured to suspend) causes the system to suspend and then wake upon release, if it's held long enough. I'm attaching a patch which fixes the problem for me, by skipping the wake on any of the release events. These events are all marked KEY_IGNORE, so think this is a reasonable thing to do. I'm a little worried about the consequences of doing this unconditionally in intel-hid. Perhaps it should be a quirk? David McFarland (1): platform/x86/intel/hid: Don't wake on 5-button releases drivers/platform/x86/intel/hid.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) -- 2.42.0
[-- Attachment #1: Type: text/plain, Size: 4360 bytes --] On Mon, 2024-03-18 at 18:31 +0000, Marc Zyngier wrote: > On Mon, 18 Mar 2024 18:15:36 +0000, > David Woodhouse <dwmw2@infradead.org> wrote: > > > > [1 <text/plain; UTF-8 (quoted-printable)>] > > On Mon, 2024-03-18 at 17:41 +0000, Marc Zyngier wrote: > > > On Mon, 18 Mar 2024 17:26:07 +0000, > > > David Woodhouse <dwmw2@infradead.org> wrote: > > > > > > > > [1 <text/plain; UTF-8 (quoted-printable)>] > > > > On Mon, 2024-03-18 at 16:57 +0000, Marc Zyngier wrote: > > > > > > > > > > > > > > > > > There *is* a way for a VMM to opt *out* of newer PSCI versions... by > > > > > > setting a per-vCPU "special" register that actually ends up setting the > > > > > > PSCI version KVM-wide. Quite why this isn't just a simple KVM_CAP, I > > > > > > have no idea. > > > > > > > > > > Because the expectations are that the VMM can blindly save/restore the > > > > > guest's state, including the PSCI version, and restore that blindly. > > > > > KVM CAPs are just a really bad design pattern for this sort of things. > > > > > > > > Hm, am I missing something here? Does the *guest* get to set the PSCI > > > > version somehow, and opt into the latest version that it understands > > > > regardless of what the firmware/host can support? > > > > > > No. The *VMM* sets the PSCI version by writing to a pseudo register. > > > It means that when the guest migrates, the VMM saves and restores that > > > version, and the guest doesn't see any change. > > > > And when you boot a guest image which has been working for years under > > a new kernel+KVM, your guest suddenly experiences a new PSCI version. > > As I said that's not just new optional functions; it's potentially even > > returning new error codes to the functions that said guest was already > > using. > > If you want to stick to a given PSCI version, you write the version > you want. > > > > > And when you *hibernate* a guest and then launch it again under a newer > > kernel+KVM, it experiences the same incompatibility. > > > > Unless the VMM realises this problem and opts *out* of the newer KVM > > behaviour, of course. This is very much unlike how we *normally* expose > > new KVM capabilities. > > This was discussed at length 5 or 6 years ago (opt-in vs opt-out). > > The feedback from QEMU (which is the only public VMM that does > anything remotely useful with this) was that opt-out was a better > model, specially as PSCI is the conduit for advertising the Spectre > mitigations and users (such as certain cloud vendors) were pretty keen > on guests seeing the mitigations advertised *by default*. OK. > And if you can spot any form of "normality" in the KVM interface, I'll > buy you whatever beer you want. It is all inconsistent crap, so I > think we're in pretty good company here. I'll give you that one :) > > > > > > I don't think we ever aspired to be able to hand an arbitrary KVM fd to > > > > a userspace VMM and have the VMM be able to drive that VM without > > > > having any a priori context, did we? > > > > > > Arbitrary? No. This is actually very specific and pretty well > > > documented. > > > > > > Also, to answer your question about why we treat 0.1 differently from > > > 0.2+: 0.1 didn't specify the PSCI SMC/HCR encoding, meaning that KVM > > > implemented something that was never fully specified. The VMM has to > > > provide firmware tables that describe that. With 0.2+, there is a > > > standard encoding for all functions, and the VMM doesn't have to > > > provide the encoding to the guest. > > > > Gotcha. So for that case we were *forced* to do things correctly and > > allow userspace to opt-in to the capability. While for 0.2 onwards we > > got away with this awfulness of silently upgrading the version without > > VMM consent. > > > > I was hoping to just follow the existing model of SYSTEM_RESET2 and not > > have to touch this awfulness with a barge-pole, but sure, whatever you > > want. > > Unless I'm reading the whole thing wrong (which isn't impossible given > that I'm jet-lagged to my eyeballs), SYSTEM_RESET2 doesn't have any > form of configuration. If PSCI 1.1 is selected, SYSTEM_RESET2 is > available. So that'd be the model to follow. Sorry, that was supposed to be SYSTEM_SUSPEND not SYSTEM_RESET2. But OK. [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 5965 bytes --]
On Mon, 18 Mar 2024 18:15:36 +0000, David Woodhouse <dwmw2@infradead.org> wrote: > > [1 <text/plain; UTF-8 (quoted-printable)>] > On Mon, 2024-03-18 at 17:41 +0000, Marc Zyngier wrote: > > On Mon, 18 Mar 2024 17:26:07 +0000, > > David Woodhouse <dwmw2@infradead.org> wrote: > > > > > > [1 <text/plain; UTF-8 (quoted-printable)>] > > > On Mon, 2024-03-18 at 16:57 +0000, Marc Zyngier wrote: > > > > > > > > > > > > > > There *is* a way for a VMM to opt *out* of newer PSCI versions... by > > > > > setting a per-vCPU "special" register that actually ends up setting the > > > > > PSCI version KVM-wide. Quite why this isn't just a simple KVM_CAP, I > > > > > have no idea. > > > > > > > > Because the expectations are that the VMM can blindly save/restore the > > > > guest's state, including the PSCI version, and restore that blindly. > > > > KVM CAPs are just a really bad design pattern for this sort of things. > > > > > > Hm, am I missing something here? Does the *guest* get to set the PSCI > > > version somehow, and opt into the latest version that it understands > > > regardless of what the firmware/host can support? > > > > No. The *VMM* sets the PSCI version by writing to a pseudo register. > > It means that when the guest migrates, the VMM saves and restores that > > version, and the guest doesn't see any change. > > And when you boot a guest image which has been working for years under > a new kernel+KVM, your guest suddenly experiences a new PSCI version. > As I said that's not just new optional functions; it's potentially even > returning new error codes to the functions that said guest was already > using. If you want to stick to a given PSCI version, you write the version you want. > > And when you *hibernate* a guest and then launch it again under a newer > kernel+KVM, it experiences the same incompatibility. > > Unless the VMM realises this problem and opts *out* of the newer KVM > behaviour, of course. This is very much unlike how we *normally* expose > new KVM capabilities. This was discussed at length 5 or 6 years ago (opt-in vs opt-out). The feedback from QEMU (which is the only public VMM that does anything remotely useful with this) was that opt-out was a better model, specially as PSCI is the conduit for advertising the Spectre mitigations and users (such as certain cloud vendors) were pretty keen on guests seeing the mitigations advertised *by default*. And if you can spot any form of "normality" in the KVM interface, I'll buy you whatever beer you want. It is all inconsistent crap, so I think we're in pretty good company here. > > > > I don't think we ever aspired to be able to hand an arbitrary KVM fd to > > > a userspace VMM and have the VMM be able to drive that VM without > > > having any a priori context, did we? > > > > Arbitrary? No. This is actually very specific and pretty well > > documented. > > > > Also, to answer your question about why we treat 0.1 differently from > > 0.2+: 0.1 didn't specify the PSCI SMC/HCR encoding, meaning that KVM > > implemented something that was never fully specified. The VMM has to > > provide firmware tables that describe that. With 0.2+, there is a > > standard encoding for all functions, and the VMM doesn't have to > > provide the encoding to the guest. > > Gotcha. So for that case we were *forced* to do things correctly and > allow userspace to opt-in to the capability. While for 0.2 onwards we > got away with this awfulness of silently upgrading the version without > VMM consent. > > I was hoping to just follow the existing model of SYSTEM_RESET2 and not > have to touch this awfulness with a barge-pole, but sure, whatever you > want. Unless I'm reading the whole thing wrong (which isn't impossible given that I'm jet-lagged to my eyeballs), SYSTEM_RESET2 doesn't have any form of configuration. If PSCI 1.1 is selected, SYSTEM_RESET2 is available. So that'd be the model to follow. M. -- Without deviation from the norm, progress is not possible.
[-- Attachment #1: Type: text/plain, Size: 2702 bytes --] On Mon, 2024-03-18 at 18:07 +0000, Marc Zyngier wrote: > On Mon, 18 Mar 2024 17:54:06 +0000, > David Woodhouse <dwmw2@infradead.org> wrote: > > > > [1 <text/plain; UTF-8 (quoted-printable)>] > > On Mon, 2024-03-18 at 17:29 +0000, Marc Zyngier wrote: > > > > > > Again, I really oppose this way of doing things. We already have an > > > infrastructure for selecting PSCI levels. You may not like it, but it > > > exists, and I'm not going entertain supporting yet another bike-shed > > > model. Adding an orthogonal cap for a feature that is specific to a > > > new PSCI version is just awful. > > > > Huh? This isn't a "new bike-shed model". This is a straight copy of > > what we *already* have for SYSTEM_RESET2. > > There is no KVM capability for SYSTEM_RESET2. It is directly > advertised to the guest when PSCI 1.1 is supported. Apologies, I got that wrong. It's SYSTEM_SUSPEND and the corresponding KVM_CAP_ARM_SYSTEM_SUSPEND that I was thinking of. Not SYSTEM_RESET2.I mixed those up. > > If I were bike-shedding, I wouldn't do separate caps for them; I'd have > > done it as a *bitmask* of the optional PSCI calls that should be > > enabled. > > > > The *mandatory* ones should obviously come from the PSCI version alone, > > but I can't see how that makes sense for the optional ones... > > The guest is in a position to probe for what is supported or not with > the PSCI_FEATURES call. Why would you add anything else? Because we don't want to silently *change* what's advertised to the guest with the VMM explicitly opting in. > > > Please make PSCI 1.3 the only version of PSCI supporting suspend in a > > > non-optional way, and be done with it. > > > > SYSTEM_OFF2 is an *optional* feature in PSCI v1.3. As are > > CLEAR_INV_MEMREGION and CLEAR_INV_MEMREGION_ATTRIBUTES. > > > > Are you suggesting that enabling v1.3 should automatically enable *all* > > of the optional features that were defined in that version (and > > previous versions) of the spec? > > No. We have everything we need to incrementally *add* features. So you > can perfectly implement PSCI 1.3 with only SYSTEM_OFF2, and only later > on add the rest, if ever. OK. It's still awful, but I suppose can live with that since existing VMMs will just see the same KVM_SYSTEM_EVENT_SHUTDOWN as before, and hopefully just won't understand the flag (and won't notice) the extra flag which says it's a hibernate. A VMM might *perhaps* check for flags it doesn't understand and complain about them, which is why we shouldn't really do that. But where PSCI is concerned it seems we've left best practice behind a long time ago, so I'll let it go. [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 5965 bytes --]
[-- Attachment #1: Type: text/plain, Size: 2842 bytes --] On Mon, 2024-03-18 at 17:41 +0000, Marc Zyngier wrote: > On Mon, 18 Mar 2024 17:26:07 +0000, > David Woodhouse <dwmw2@infradead.org> wrote: > > > > [1 <text/plain; UTF-8 (quoted-printable)>] > > On Mon, 2024-03-18 at 16:57 +0000, Marc Zyngier wrote: > > > > > > > > > > > There *is* a way for a VMM to opt *out* of newer PSCI versions... by > > > > setting a per-vCPU "special" register that actually ends up setting the > > > > PSCI version KVM-wide. Quite why this isn't just a simple KVM_CAP, I > > > > have no idea. > > > > > > Because the expectations are that the VMM can blindly save/restore the > > > guest's state, including the PSCI version, and restore that blindly. > > > KVM CAPs are just a really bad design pattern for this sort of things. > > > > Hm, am I missing something here? Does the *guest* get to set the PSCI > > version somehow, and opt into the latest version that it understands > > regardless of what the firmware/host can support? > > No. The *VMM* sets the PSCI version by writing to a pseudo register. > It means that when the guest migrates, the VMM saves and restores that > version, and the guest doesn't see any change. And when you boot a guest image which has been working for years under a new kernel+KVM, your guest suddenly experiences a new PSCI version. As I said that's not just new optional functions; it's potentially even returning new error codes to the functions that said guest was already using. And when you *hibernate* a guest and then launch it again under a newer kernel+KVM, it experiences the same incompatibility. Unless the VMM realises this problem and opts *out* of the newer KVM behaviour, of course. This is very much unlike how we *normally* expose new KVM capabilities. > > I don't think we ever aspired to be able to hand an arbitrary KVM fd to > > a userspace VMM and have the VMM be able to drive that VM without > > having any a priori context, did we? > > Arbitrary? No. This is actually very specific and pretty well > documented. > > Also, to answer your question about why we treat 0.1 differently from > 0.2+: 0.1 didn't specify the PSCI SMC/HCR encoding, meaning that KVM > implemented something that was never fully specified. The VMM has to > provide firmware tables that describe that. With 0.2+, there is a > standard encoding for all functions, and the VMM doesn't have to > provide the encoding to the guest. Gotcha. So for that case we were *forced* to do things correctly and allow userspace to opt-in to the capability. While for 0.2 onwards we got away with this awfulness of silently upgrading the version without VMM consent. I was hoping to just follow the existing model of SYSTEM_RESET2 and not have to touch this awfulness with a barge-pole, but sure, whatever you want. [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 5965 bytes --]
On Mon, 18 Mar 2024 17:54:06 +0000, David Woodhouse <dwmw2@infradead.org> wrote: > > [1 <text/plain; UTF-8 (quoted-printable)>] > On Mon, 2024-03-18 at 17:29 +0000, Marc Zyngier wrote: > > > > Again, I really oppose this way of doing things. We already have an > > infrastructure for selecting PSCI levels. You may not like it, but it > > exists, and I'm not going entertain supporting yet another bike-shed > > model. Adding an orthogonal cap for a feature that is specific to a > > new PSCI version is just awful. > > Huh? This isn't a "new bike-shed model". This is a straight copy of > what we *already* have for SYSTEM_RESET2. There is no KVM capability for SYSTEM_RESET2. It is directly advertised to the guest when PSCI 1.1 is supported. > If I were bike-shedding, I wouldn't do separate caps for them; I'd have > done it as a *bitmask* of the optional PSCI calls that should be > enabled. > > The *mandatory* ones should obviously come from the PSCI version alone, > but I can't see how that makes sense for the optional ones... The guest is in a position to probe for what is supported or not with the PSCI_FEATURES call. Why would you add anything else? > > > Please make PSCI 1.3 the only version of PSCI supporting suspend in a > > non-optional way, and be done with it. > > SYSTEM_OFF2 is an *optional* feature in PSCI v1.3. As are > CLEAR_INV_MEMREGION and CLEAR_INV_MEMREGION_ATTRIBUTES. > > Are you suggesting that enabling v1.3 should automatically enable *all* > of the optional features that were defined in that version (and > previous versions) of the spec? No. We have everything we need to incrementally *add* features. So you can perfectly implement PSCI 1.3 with only SYSTEM_OFF2, and only later on add the rest, if ever. M. -- Without deviation from the norm, progress is not possible.
[-- Attachment #1: Type: text/plain, Size: 1195 bytes --] On Mon, 2024-03-18 at 17:29 +0000, Marc Zyngier wrote: > > Again, I really oppose this way of doing things. We already have an > infrastructure for selecting PSCI levels. You may not like it, but it > exists, and I'm not going entertain supporting yet another bike-shed > model. Adding an orthogonal cap for a feature that is specific to a > new PSCI version is just awful. Huh? This isn't a "new bike-shed model". This is a straight copy of what we *already* have for SYSTEM_RESET2. If I were bike-shedding, I wouldn't do separate caps for them; I'd have done it as a *bitmask* of the optional PSCI calls that should be enabled. The *mandatory* ones should obviously come from the PSCI version alone, but I can't see how that makes sense for the optional ones... > Please make PSCI 1.3 the only version of PSCI supporting suspend in a > non-optional way, and be done with it. SYSTEM_OFF2 is an *optional* feature in PSCI v1.3. As are CLEAR_INV_MEMREGION and CLEAR_INV_MEMREGION_ATTRIBUTES. Are you suggesting that enabling v1.3 should automatically enable *all* of the optional features that were defined in that version (and previous versions) of the spec? [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 5965 bytes --]