| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y44tdASIQ2DpL9re@makrotopia.org>
Date: Mon, 5 Dec 2022 17:42:12 +0000
From: Daniel Golle <daniel@...rotopia.org>
To: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>
Cc: linux-pm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
"Rafael J. Wysocki" <rafael@...nel.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Amit Kucheria <amitk@...nel.org>,
Zhang Rui <rui.zhang@...el.com>,
Matthias Brugger <matthias.bgg@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Steven Liu <steven.liu@...iatek.com>,
Henry Yen <Henry.Yen@...iatek.com>
Subject: Re: [PATCH v2 1/2] thermal: mediatek: add support for MT7986 and
MT7981
Hi Angelo,
On Thu, Dec 01, 2022 at 02:24:17PM +0100, AngeloGioacchino Del Regno wrote:
> Il 30/11/22 14:19, Daniel Golle ha scritto:
> > Add support for V3 generation thermal found in MT7986 and MT7981 SoCs.
> > Brings code to assign values from efuse as well as new function to
> > convert raw temperature to millidegree celsius, as found in MediaTek's
> > SDK sources (but cleaned up and de-duplicated)
> >
> > [1]: https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/baf36c7eef477aae1f8f2653b6c29e2caf48475b
> > Signed-off-by: Daniel Golle <daniel@...rotopia.org>
> > Reviewed-by: Henry Yen <henry.yen@...iatek.com>
> > ---
> > Changes since v1: Drop use of adc_oe field in efuse, Henry Yen confirmed
> > its use has been dropped intentionally in MTK SDK as well.
> >
> > drivers/thermal/mtk_thermal.c | 122 +++++++++++++++++++++++++++++++++-
> > 1 file changed, 119 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
> > index 8440692e3890..6a69419f8960 100644
> > --- a/drivers/thermal/mtk_thermal.c
> > +++ b/drivers/thermal/mtk_thermal.c
> > @@ -150,6 +150,21 @@
> > #define CALIB_BUF1_VALID_V2(x) (((x) >> 4) & 0x1)
> > #define CALIB_BUF1_O_SLOPE_SIGN_V2(x) (((x) >> 3) & 0x1)
> > +/*
> > + * Layout of the fuses providing the calibration data
> > + * These macros can be used for MT7981 and MT7986.
> > + */
> > +#define CALIB_BUF0_ADC_GE_V3(x) (((x) >> 0) & 0x3ff)
> > +#define CALIB_BUF0_ADC_OE_V3(x) (((x) >> 10) & 0x3ff)
> > +#define CALIB_BUF0_DEGC_CALI_V3(x) (((x) >> 20) & 0x3f)
> > +#define CALIB_BUF0_O_SLOPE_V3(x) (((x) >> 26) & 0x3f)
> > +#define CALIB_BUF1_VTS_TS1_V3(x) (((x) >> 0) & 0x1ff)
> > +#define CALIB_BUF1_VTS_TS2_V3(x) (((x) >> 21) & 0x1ff)
> > +#define CALIB_BUF1_VTS_TSABB_V3(x) (((x) >> 9) & 0x1ff)
> > +#define CALIB_BUF1_VALID_V3(x) (((x) >> 18) & 0x1)
> > +#define CALIB_BUF1_O_SLOPE_SIGN_V3(x) (((x) >> 19) & 0x1)
> > +#define CALIB_BUF1_ID_V3(x) (((x) >> 20) & 0x1)
> > +
> > enum {
> > VTS1,
> > VTS2,
> > @@ -163,6 +178,7 @@ enum {
> > enum mtk_thermal_version {
> > MTK_THERMAL_V1 = 1,
> > MTK_THERMAL_V2,
> > + MTK_THERMAL_V3,
> > };
> > /* MT2701 thermal sensors */
> > @@ -245,6 +261,27 @@ enum mtk_thermal_version {
> > /* The calibration coefficient of sensor */
> > #define MT8183_CALIBRATION 153
> > +/* AUXADC channel 11 is used for the temperature sensors */
> > +#define MT7986_TEMP_AUXADC_CHANNEL 11
> > +
> > +/* The total number of temperature sensors in the MT7986 */
> > +#define MT7986_NUM_SENSORS 1
> > +
> > +/* The number of banks in the MT7986 */
> > +#define MT7986_NUM_ZONES 1
> > +
> > +/* The number of sensing points per bank */
> > +#define MT7986_NUM_SENSORS_PER_ZONE 1
> > +
> > +/* MT7986 thermal sensors */
> > +#define MT7986_TS1 0
> > +
> > +/* The number of controller in the MT7986 */
> > +#define MT7986_NUM_CONTROLLER 1
> > +
> > +/* The calibration coefficient of sensor */
> > +#define MT7986_CALIBRATION 165
> > +
> > struct mtk_thermal;
> > struct thermal_bank_cfg {
> > @@ -386,6 +423,14 @@ static const int mt7622_mux_values[MT7622_NUM_SENSORS] = { 0, };
> > static const int mt7622_vts_index[MT7622_NUM_SENSORS] = { VTS1 };
> > static const int mt7622_tc_offset[MT7622_NUM_CONTROLLER] = { 0x0, };
> > +/* MT7986 thermal sensor data */
> > +static const int mt7986_bank_data[MT7986_NUM_SENSORS] = { MT7986_TS1, };
> > +static const int mt7986_msr[MT7986_NUM_SENSORS_PER_ZONE] = { TEMP_MSR0, };
> > +static const int mt7986_adcpnp[MT7986_NUM_SENSORS_PER_ZONE] = { TEMP_ADCPNP0, };
> > +static const int mt7986_mux_values[MT7986_NUM_SENSORS] = { 0, };
> > +static const int mt7986_vts_index[MT7986_NUM_SENSORS] = { VTS1 };
> > +static const int mt7986_tc_offset[MT7986_NUM_CONTROLLER] = { 0x0, };
> > +
> > /*
> > * The MT8173 thermal controller has four banks. Each bank can read up to
> > * four temperature sensors simultaneously. The MT8173 has a total of 5
> > @@ -549,6 +594,30 @@ static const struct mtk_thermal_data mt8183_thermal_data = {
> > .version = MTK_THERMAL_V1,
> > };
> > +/*
> > + * MT7986 uses AUXADC Channel 11 for raw data access.
> > + */
> > +static const struct mtk_thermal_data mt7986_thermal_data = {
> > + .auxadc_channel = MT7986_TEMP_AUXADC_CHANNEL,
> > + .num_banks = MT7986_NUM_ZONES,
> > + .num_sensors = MT7986_NUM_SENSORS,
> > + .vts_index = mt7986_vts_index,
> > + .cali_val = MT7986_CALIBRATION,
> > + .num_controller = MT7986_NUM_CONTROLLER,
> > + .controller_offset = mt7986_tc_offset,
> > + .need_switch_bank = true,
> > + .bank_data = {
> > + {
> > + .num_sensors = 1,
> > + .sensors = mt7986_bank_data,
> > + },
> > + },
> > + .msr = mt7986_msr,
> > + .adcpnp = mt7986_adcpnp,
> > + .sensor_mux_values = mt7986_mux_values,
> > + .version = MTK_THERMAL_V3,
> > +};
> > +
> > /**
> > * raw_to_mcelsius - convert a raw ADC value to mcelsius
> > * @mt: The thermal controller
> > @@ -603,6 +672,22 @@ static int raw_to_mcelsius_v2(struct mtk_thermal *mt, int sensno, s32 raw)
> > return (format_2 - tmp) * 100;
> > }
> > +static int raw_to_mcelsius_v3(struct mtk_thermal *mt, int sensno, s32 raw)
> > +{
> > + s32 tmp;
> > +
> > + if (raw == 0)
> > + return 0;
> > +
> > + raw &= 0xfff;
> > + tmp = 100000 * 15 / 16 * 10000;
> > + tmp /= 4096 - 512 + mt->adc_ge;
> > + tmp /= 1490;
> > + tmp *= raw - mt->vts[sensno] - 2900;
> > +
> > + return mt->degc_cali * 500 - tmp;
> > +}
> > +
> > /**
> > * mtk_thermal_get_bank - get bank
> > * @bank: The bank
> > @@ -659,9 +744,12 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
> > if (mt->conf->version == MTK_THERMAL_V1) {
> > temp = raw_to_mcelsius_v1(
> > mt, conf->bank_data[bank->id].sensors[i], raw);
> > - } else {
> > + } else if (mt->conf->version == MTK_THERMAL_V2) {
> > temp = raw_to_mcelsius_v2(
> > mt, conf->bank_data[bank->id].sensors[i], raw);
> > + } else {
> > + temp = raw_to_mcelsius_v3(
> > + mt, conf->bank_data[bank->id].sensors[i], raw);
> > }
>
> What about optimizing this with assigning a function pointer?
> Like that, we wouldn't check any version in there... as in that case we'd
> simply do something like
>
> temp = conf->raw_to_mcelsius(mt, conf->bank...blahblah...);
>
> ...and this would also mean that the snippet saying "the first read of a sensor
> often contains very high bogus temperature value [...]" would get merged in the v2
> of raw_to_mcelsius (as that function is used only in mtk_thermal_bank_temperature).
I found that Amjad Ouled-Ameur is taking care of converting that series
of if-else options into a function pointer allowing to easily call the
right raw_to_mcelsius function.
[PATCH v7 0/4] thermal: mediatek: Add support for MT8365 SoC
https://lore.kernel.org/linux-arm-kernel/4121bb6b-30db-7a23-f4c8-40afdda7a0b5@linaro.org/T/
Should I wait until this series is merged and then submit support
for MT7986 thermal on top of that?
>
> > /*
> > @@ -887,6 +975,26 @@ static int mtk_thermal_extract_efuse_v2(struct mtk_thermal *mt, u32 *buf)
> > return 0;
> > }
> > +static int mtk_thermal_extract_efuse_v3(struct mtk_thermal *mt, u32 *buf)
> > +{
> > + if (!CALIB_BUF1_VALID_V3(buf[1]))
> > + return -EINVAL;
> > +
> > + mt->adc_oe = CALIB_BUF0_ADC_OE_V3(buf[0]);
> > + mt->adc_ge = CALIB_BUF0_ADC_GE_V3(buf[0]);
> > + mt->degc_cali = CALIB_BUF0_DEGC_CALI_V3(buf[0]);
> > + mt->o_slope = CALIB_BUF0_O_SLOPE_V3(buf[0]);
> > + mt->vts[VTS1] = CALIB_BUF1_VTS_TS1_V3(buf[1]);
> > + mt->vts[VTS2] = CALIB_BUF1_VTS_TS2_V3(buf[1]);
> > + mt->vts[VTSABB] = CALIB_BUF1_VTS_TSABB_V3(buf[1]);
> > + mt->o_slope_sign = CALIB_BUF1_O_SLOPE_SIGN_V3(buf[1]);
> > +
> > + if (CALIB_BUF1_ID_V3(buf[1]) == 0)
> > + mt->o_slope = 0;
> > +
> > + return 0;
> > +}
> > +
> > static int mtk_thermal_get_calibration_data(struct device *dev,
> > struct mtk_thermal *mt)
> > {
> > @@ -897,6 +1005,7 @@ static int mtk_thermal_get_calibration_data(struct device *dev,
> > /* Start with default values */
> > mt->adc_ge = 512;
> > + mt->adc_oe = 512;
> > for (i = 0; i < mt->conf->num_sensors; i++)
> > mt->vts[i] = 260;
> > mt->degc_cali = 40;
> > @@ -924,8 +1033,10 @@ static int mtk_thermal_get_calibration_data(struct device *dev,
> > if (mt->conf->version == MTK_THERMAL_V1)
> > ret = mtk_thermal_extract_efuse_v1(mt, buf);
> > - else
> > + else if (mt->conf->version == MTK_THERMAL_V2)
> > ret = mtk_thermal_extract_efuse_v2(mt, buf);
> > + else
> > + ret = mtk_thermal_extract_efuse_v3(mt, buf);
>
> I propose to use a switch here instead.
>
> switch(mt->conf->version) {
> case MTK_THERMAL_V1:
> ....
> case MTK_THERMAL_V2:
> ....
> case MTK_THERMAL_V3:
> ....
> default:
> ret = -EINVAL;
> break;
> };
>
> This would also prevent a potential issue with getting an invalid calibration
> due to us calling the wrong version of the get_calibration() function, in which
> case, using the default calibration values would be at that point preferred.
>
> Regards,
> Angelo
>
>
>
Powered by blists - more mailing lists