From: Eduardo Valentin <edubezval@gmail.com> To: Javi Merino <javi.merino@arm.com> Cc: Sascha Hauer <s.hauer@pengutronix.de>, linux-pm@vger.kernel.org, Zhang Rui <rui.zhang@intel.com>, mark.rutland@arm.com, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, robh+dt@kernel.org, linux-mediatek@lists.infradead.org, kernel@pengutronix.de, Matthias Brugger <matthias.bgg@gmail.com>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 2/3] thermal: Add Mediatek thermal controller support Date: Tue, 10 Nov 2015 10:26:30 -0800 [thread overview] Message-ID: <20151110182629.GA5240@localhost.localdomain> (raw) In-Reply-To: <20151110120554.GB3551@e104805> On Tue, Nov 10, 2015 at 12:05:54PM +0000, Javi Merino wrote: > On Mon, Nov 09, 2015 at 11:13:32AM +0100, Sascha Hauer wrote: <cut> > > + > > +/* > > + * The MT8173 thermal controller has four banks. Each bank can read up to > > + * four temperature sensors simultaneously. The MT8173 has a total of 5 > > + * temperature sensors. We use each bank to measure a certain area of the > > + * SoC. Since TS2 is located centrally in the SoC it is influenced by multiple > > + * areas, hence is used in different banks. > > + */ > > +static const struct mtk_thermal_bank_cfg bank_data[] = { > > + { > > + .num_sensors = 2, > > + .sensors = { MT8173_TS2, MT8173_TS3 }, > > + }, { > > + .num_sensors = 2, > > + .sensors = { MT8173_TS2, MT8173_TS4 }, > > + }, { > > + .num_sensors = 3, > > + .sensors = { MT8173_TS1, MT8173_TS2, MT8173_TSABB }, > > + }, { > > + .num_sensors = 1, > > + .sensors = { MT8173_TS2 }, > > + }, > > +}; Would it make sense to simply expose all sensors and let the configuration of their aggregation be done by DT? There is already ongoing effort to get aggregation functions generalized. > > + > > +struct mtk_thermal_sense_point { > > + int msr; > > + int adcpnp; > > +}; > > + > > +static const struct mtk_thermal_sense_point > > + sensing_points[MT8173_NUM_SENSORS_PER_ZONE] = { > > + { > > + .msr = TEMP_MSR0, > > + .adcpnp = TEMP_ADCPNP0, > > + }, { > > + .msr = TEMP_MSR1, > > + .adcpnp = TEMP_ADCPNP1, > > + }, { > > + .msr = TEMP_MSR2, > > + .adcpnp = TEMP_ADCPNP2, > > + }, { > > + .msr = TEMP_MSR3, > > + .adcpnp = TEMP_ADCPNP3, > > + }, > > +}; > > + > > +/** > > + * raw_to_mcelsius - convert a raw ADC value to mcelsius > > + * @mt: The thermal controller > > + * @raw: raw ADC value > > + * > > + * This converts the raw ADC value to mcelsius using the SoC specific > > + * calibration constants > > + */ > > +static int raw_to_mcelsius(struct mtk_thermal *mt, int sensno, s32 raw) > > +{ > > + s32 tmp; > > + > > + raw &= 0xfff; > > + > > + tmp = 203450520 << 3; > > + tmp /= 165 + mt->o_slope; > > + tmp /= 10000 + mt->adc_ge; > > + tmp *= raw - mt->vts[sensno] - 3350; > > + tmp >>= 3; > > + > > + return mt->degc_cali * 500 - tmp; > > +} > > + > > +/** > > + * mtk_thermal_get_bank - get bank > > + * @bank: The bank > > + * > > + * The bank registers are banked, we have to select a bank in the > > + * PTPCORESEL register to access it. > > + */ > > +static void mtk_thermal_get_bank(struct mtk_thermal_bank *bank) > > +{ > > + struct mtk_thermal *mt = bank->mt; > > + u32 val; > > + > > + mutex_lock(&mt->lock); > > + > > + val = readl(mt->thermal_base + PTPCORESEL); > > + val &= ~0xf; > > + val |= bank->id; > > + writel(val, mt->thermal_base + PTPCORESEL); > > +} > > + > > +/** > > + * mtk_thermal_put_bank - release bank > > + * @bank: The bank > > + * > > + * release a bank previously taken with mtk_thermal_get_bank, > > + */ > > +static void mtk_thermal_put_bank(struct mtk_thermal_bank *bank) > > +{ > > + struct mtk_thermal *mt = bank->mt; > > + > > + mutex_unlock(&mt->lock); > > +} > > + > > +/** > > + * mtk_thermal_bank_temperature - get the temperature of a bank > > + * @bank: The bank > > + * > > + * The temperature of a bank is considered the maximum temperature of > > + * the sensors associated to the bank. > > + */ > > +static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank) > > +{ > > + struct mtk_thermal *mt = bank->mt; > > + int temp, i, max; > > + u32 raw; > > + > > + temp = max = INT_MIN; > > + > > + for (i = 0; i < bank_data[bank->id].num_sensors; i++) { > > + raw = readl(mt->thermal_base + sensing_points[i].msr); > > + > > + temp = raw_to_mcelsius(mt, bank_data[bank->id].sensors[i], raw); > > + > > + /* > > + * The first read of a sensor often contains very high bogus > > + * temperature value. Filter these out so that the system does > > + * not immediately shut down. > > + */ > > + if (temp > 200000) > > + temp = 0; > > + > > + if (temp > max) > > + max = temp; > > + } > > + > > + return max; > > +} > > + > > +static int mtk_read_temp(void *data, int *temperature) > > +{ > > + struct mtk_thermal *mt = data; > > + int i; > > + int tempmax = INT_MIN; > > + > > + for (i = 0; i < MT8173_NUM_ZONES; i++) { > > + struct mtk_thermal_bank *bank = &mt->banks[i]; > > + int t; > > + > > + mtk_thermal_get_bank(bank); > > + > > + t = mtk_thermal_bank_temperature(bank); > > IIUIC, when you had multiple thermal zones > mtk_thermal_bank_temperature() made sense, but now it looks like > you're just doing the maximum of all sensors. Why bother with the > banks any more? Aren't you just calculating the maximum of all > sensors? As TS2 is present in all banks, there's no point in reading > it four times just to get the maximum of all sensors. > Yeah, agreed here. If that is your intention, maybe read each sensor one time, then compute the max of each subset from memory instead. > > + mtk_thermal_put_bank(bank); > > + > > + if (t > tempmax) > > + tempmax = t; > > + } > > + > > + *temperature = tempmax; > > + > > + return 0; > > +} > > + > > +static const struct thermal_zone_of_device_ops mtk_thermal_ops = { > > + .get_temp = mtk_read_temp, > > +}; > > Cheers, > Javi >
WARNING: multiple messages have this Message-ID (diff)
From: edubezval@gmail.com (Eduardo Valentin) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/3] thermal: Add Mediatek thermal controller support Date: Tue, 10 Nov 2015 10:26:30 -0800 [thread overview] Message-ID: <20151110182629.GA5240@localhost.localdomain> (raw) In-Reply-To: <20151110120554.GB3551@e104805> On Tue, Nov 10, 2015 at 12:05:54PM +0000, Javi Merino wrote: > On Mon, Nov 09, 2015 at 11:13:32AM +0100, Sascha Hauer wrote: <cut> > > + > > +/* > > + * The MT8173 thermal controller has four banks. Each bank can read up to > > + * four temperature sensors simultaneously. The MT8173 has a total of 5 > > + * temperature sensors. We use each bank to measure a certain area of the > > + * SoC. Since TS2 is located centrally in the SoC it is influenced by multiple > > + * areas, hence is used in different banks. > > + */ > > +static const struct mtk_thermal_bank_cfg bank_data[] = { > > + { > > + .num_sensors = 2, > > + .sensors = { MT8173_TS2, MT8173_TS3 }, > > + }, { > > + .num_sensors = 2, > > + .sensors = { MT8173_TS2, MT8173_TS4 }, > > + }, { > > + .num_sensors = 3, > > + .sensors = { MT8173_TS1, MT8173_TS2, MT8173_TSABB }, > > + }, { > > + .num_sensors = 1, > > + .sensors = { MT8173_TS2 }, > > + }, > > +}; Would it make sense to simply expose all sensors and let the configuration of their aggregation be done by DT? There is already ongoing effort to get aggregation functions generalized. > > + > > +struct mtk_thermal_sense_point { > > + int msr; > > + int adcpnp; > > +}; > > + > > +static const struct mtk_thermal_sense_point > > + sensing_points[MT8173_NUM_SENSORS_PER_ZONE] = { > > + { > > + .msr = TEMP_MSR0, > > + .adcpnp = TEMP_ADCPNP0, > > + }, { > > + .msr = TEMP_MSR1, > > + .adcpnp = TEMP_ADCPNP1, > > + }, { > > + .msr = TEMP_MSR2, > > + .adcpnp = TEMP_ADCPNP2, > > + }, { > > + .msr = TEMP_MSR3, > > + .adcpnp = TEMP_ADCPNP3, > > + }, > > +}; > > + > > +/** > > + * raw_to_mcelsius - convert a raw ADC value to mcelsius > > + * @mt: The thermal controller > > + * @raw: raw ADC value > > + * > > + * This converts the raw ADC value to mcelsius using the SoC specific > > + * calibration constants > > + */ > > +static int raw_to_mcelsius(struct mtk_thermal *mt, int sensno, s32 raw) > > +{ > > + s32 tmp; > > + > > + raw &= 0xfff; > > + > > + tmp = 203450520 << 3; > > + tmp /= 165 + mt->o_slope; > > + tmp /= 10000 + mt->adc_ge; > > + tmp *= raw - mt->vts[sensno] - 3350; > > + tmp >>= 3; > > + > > + return mt->degc_cali * 500 - tmp; > > +} > > + > > +/** > > + * mtk_thermal_get_bank - get bank > > + * @bank: The bank > > + * > > + * The bank registers are banked, we have to select a bank in the > > + * PTPCORESEL register to access it. > > + */ > > +static void mtk_thermal_get_bank(struct mtk_thermal_bank *bank) > > +{ > > + struct mtk_thermal *mt = bank->mt; > > + u32 val; > > + > > + mutex_lock(&mt->lock); > > + > > + val = readl(mt->thermal_base + PTPCORESEL); > > + val &= ~0xf; > > + val |= bank->id; > > + writel(val, mt->thermal_base + PTPCORESEL); > > +} > > + > > +/** > > + * mtk_thermal_put_bank - release bank > > + * @bank: The bank > > + * > > + * release a bank previously taken with mtk_thermal_get_bank, > > + */ > > +static void mtk_thermal_put_bank(struct mtk_thermal_bank *bank) > > +{ > > + struct mtk_thermal *mt = bank->mt; > > + > > + mutex_unlock(&mt->lock); > > +} > > + > > +/** > > + * mtk_thermal_bank_temperature - get the temperature of a bank > > + * @bank: The bank > > + * > > + * The temperature of a bank is considered the maximum temperature of > > + * the sensors associated to the bank. > > + */ > > +static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank) > > +{ > > + struct mtk_thermal *mt = bank->mt; > > + int temp, i, max; > > + u32 raw; > > + > > + temp = max = INT_MIN; > > + > > + for (i = 0; i < bank_data[bank->id].num_sensors; i++) { > > + raw = readl(mt->thermal_base + sensing_points[i].msr); > > + > > + temp = raw_to_mcelsius(mt, bank_data[bank->id].sensors[i], raw); > > + > > + /* > > + * The first read of a sensor often contains very high bogus > > + * temperature value. Filter these out so that the system does > > + * not immediately shut down. > > + */ > > + if (temp > 200000) > > + temp = 0; > > + > > + if (temp > max) > > + max = temp; > > + } > > + > > + return max; > > +} > > + > > +static int mtk_read_temp(void *data, int *temperature) > > +{ > > + struct mtk_thermal *mt = data; > > + int i; > > + int tempmax = INT_MIN; > > + > > + for (i = 0; i < MT8173_NUM_ZONES; i++) { > > + struct mtk_thermal_bank *bank = &mt->banks[i]; > > + int t; > > + > > + mtk_thermal_get_bank(bank); > > + > > + t = mtk_thermal_bank_temperature(bank); > > IIUIC, when you had multiple thermal zones > mtk_thermal_bank_temperature() made sense, but now it looks like > you're just doing the maximum of all sensors. Why bother with the > banks any more? Aren't you just calculating the maximum of all > sensors? As TS2 is present in all banks, there's no point in reading > it four times just to get the maximum of all sensors. > Yeah, agreed here. If that is your intention, maybe read each sensor one time, then compute the max of each subset from memory instead. > > + mtk_thermal_put_bank(bank); > > + > > + if (t > tempmax) > > + tempmax = t; > > + } > > + > > + *temperature = tempmax; > > + > > + return 0; > > +} > > + > > +static const struct thermal_zone_of_device_ops mtk_thermal_ops = { > > + .get_temp = mtk_read_temp, > > +}; > > Cheers, > Javi >
next prev parent reply other threads:[~2015-11-10 18:26 UTC|newest] Thread overview: 139+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-11-09 10:13 [PATCH v10] Add Mediatek thermal support Sascha Hauer 2015-11-09 10:13 ` Sascha Hauer 2015-11-09 10:13 ` Sascha Hauer 2015-11-09 10:13 ` [PATCH 1/3] dt-bindings: thermal: Add binding document for Mediatek thermal controller Sascha Hauer 2015-11-09 10:13 ` Sascha Hauer 2015-11-09 10:13 ` Sascha Hauer 2015-11-09 15:59 ` Rob Herring 2015-11-09 15:59 ` Rob Herring 2015-11-09 10:13 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-11-09 10:13 ` Sascha Hauer 2015-11-09 10:13 ` Sascha Hauer 2015-11-09 14:39 ` Andy Shevchenko 2015-11-09 14:39 ` Andy Shevchenko 2015-11-18 8:11 ` Sascha Hauer 2015-11-18 8:11 ` Sascha Hauer 2015-11-18 8:11 ` Sascha Hauer 2015-11-10 12:05 ` Javi Merino 2015-11-10 12:05 ` Javi Merino 2015-11-10 18:26 ` Eduardo Valentin [this message] 2015-11-10 18:26 ` Eduardo Valentin 2015-11-11 7:27 ` Sascha Hauer 2015-11-11 7:27 ` Sascha Hauer 2015-11-11 9:40 ` Javi Merino 2015-11-11 9:40 ` Javi Merino 2015-11-11 9:40 ` Javi Merino 2015-11-13 10:09 ` Sascha Hauer 2015-11-13 10:09 ` Sascha Hauer 2015-11-13 11:26 ` Javi Merino 2015-11-13 11:26 ` Javi Merino 2015-11-18 8:18 ` Sascha Hauer 2015-11-18 8:18 ` Sascha Hauer 2015-11-09 10:13 ` [PATCH 3/3] ARM64: dts: mt8173: Add thermal/auxadc device nodes Sascha Hauer 2015-11-09 10:13 ` Sascha Hauer -- strict thread matches above, loose matches on Subject: below -- 2015-11-30 11:42 [PATCH v12] Add Mediatek thermal support Sascha Hauer 2015-11-30 11:42 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-11-30 11:42 ` Sascha Hauer 2015-12-17 19:33 ` Eduardo Valentin 2015-12-17 19:33 ` Eduardo Valentin 2016-01-04 14:19 ` Sascha Hauer 2016-01-04 14:19 ` Sascha Hauer 2016-01-19 7:29 ` Sascha Hauer 2016-01-19 7:29 ` Sascha Hauer 2016-02-01 2:54 ` Eddie Huang 2016-02-01 2:54 ` Eddie Huang 2016-02-01 2:54 ` Eddie Huang 2016-02-15 2:11 ` Daniel Kurtz 2016-02-15 2:11 ` Daniel Kurtz 2016-02-15 2:11 ` Daniel Kurtz 2016-02-15 2:14 ` Daniel Kurtz 2016-02-15 2:14 ` Daniel Kurtz 2016-02-15 2:14 ` Daniel Kurtz 2016-02-17 17:05 ` Matthias Brugger 2016-02-17 17:05 ` Matthias Brugger 2016-02-17 17:05 ` Matthias Brugger 2016-02-18 10:56 ` Sascha Hauer 2016-02-18 10:56 ` Sascha Hauer 2016-02-18 10:56 ` Sascha Hauer 2016-02-18 14:28 ` Javi Merino 2016-02-18 14:28 ` Javi Merino 2016-02-18 14:28 ` Javi Merino 2016-02-18 15:15 ` Eduardo Valentin 2016-02-18 15:15 ` Eduardo Valentin 2016-02-18 15:15 ` Eduardo Valentin 2016-02-19 7:21 ` Sascha Hauer 2016-02-19 7:21 ` Sascha Hauer 2016-02-19 7:21 ` Sascha Hauer 2015-12-21 4:07 ` Daniel Kurtz 2015-12-21 4:07 ` Daniel Kurtz 2015-12-21 4:07 ` Daniel Kurtz 2016-01-04 14:31 ` Sascha Hauer 2016-01-04 14:31 ` Sascha Hauer 2016-01-04 14:31 ` Sascha Hauer 2016-01-04 15:43 ` Daniel Kurtz 2016-01-04 15:43 ` Daniel Kurtz 2016-01-04 15:43 ` Daniel Kurtz 2015-11-18 8:24 [PATCH v11] Add Mediatek thermal support Sascha Hauer 2015-11-18 8:24 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-11-18 8:24 ` Sascha Hauer 2015-11-24 6:06 ` dawei chien 2015-11-24 6:06 ` dawei chien 2015-11-24 6:06 ` dawei chien 2015-11-24 7:53 ` Sascha Hauer 2015-11-24 7:53 ` Sascha Hauer 2015-09-23 13:37 [PATCH v9] Add Mediatek thermal support Sascha Hauer 2015-09-23 13:37 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-09-23 13:37 ` Sascha Hauer 2015-09-23 18:31 ` Vladimir Zapolskiy 2015-09-23 18:31 ` Vladimir Zapolskiy 2015-09-23 18:31 ` Vladimir Zapolskiy 2015-09-30 6:14 ` Sascha Hauer 2015-09-30 6:14 ` Sascha Hauer 2015-09-29 23:04 ` Eduardo Valentin 2015-09-29 23:04 ` Eduardo Valentin 2015-09-29 23:04 ` Eduardo Valentin 2015-09-30 6:13 ` Sascha Hauer 2015-09-30 6:13 ` Sascha Hauer 2015-09-30 9:36 ` Punit Agrawal 2015-09-30 9:36 ` Punit Agrawal 2015-09-30 9:36 ` Punit Agrawal 2015-09-30 10:37 ` Sascha Hauer 2015-09-30 10:37 ` Sascha Hauer 2015-09-30 11:07 ` Punit Agrawal 2015-09-30 11:07 ` Punit Agrawal 2015-08-31 7:34 [PATCH v8] Add Mediatek thermal support Sascha Hauer 2015-08-31 7:34 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-08-31 7:34 ` Sascha Hauer 2015-08-31 7:34 ` Sascha Hauer 2015-09-14 7:32 ` Daniel Kurtz 2015-09-14 7:32 ` Daniel Kurtz 2015-09-14 7:32 ` Daniel Kurtz 2015-09-22 7:30 ` Daniel Kurtz 2015-09-22 7:30 ` Daniel Kurtz 2015-09-22 7:30 ` Daniel Kurtz 2015-09-22 8:30 ` Sascha Hauer 2015-09-22 8:30 ` Sascha Hauer 2015-09-22 8:30 ` Sascha Hauer 2015-08-27 6:41 [PATCH v7] Add Mediatek thermal support Sascha Hauer 2015-08-27 6:41 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-08-27 11:50 ` Punit Agrawal 2015-08-26 13:58 [PATCH v6] Add Mediatek thermal support Sascha Hauer 2015-08-26 13:58 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-08-20 8:05 [PATCH v5] Add Mediatek thermal support Sascha Hauer 2015-08-20 8:06 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-08-20 22:20 ` Eduardo Valentin 2015-08-20 22:29 ` Daniel Lezcano 2015-08-21 5:06 ` Sascha Hauer 2015-08-20 23:12 ` Daniel Lezcano 2015-08-26 13:54 ` Sascha Hauer 2015-08-26 14:02 ` Daniel Lezcano 2015-08-25 17:41 ` Daniel Kurtz 2015-08-07 13:55 [PATCH v4] Add Mediatek thermal support Sascha Hauer 2015-08-07 13:55 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-08-11 7:03 ` Daniel Kurtz 2015-08-20 7:57 ` Sascha Hauer 2015-08-05 12:25 [PATCH v3] Add Mediatek thermal support Sascha Hauer 2015-08-05 12:25 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-08-05 18:02 ` Daniel Kurtz 2015-08-06 8:10 ` Sascha Hauer 2015-07-21 7:59 [PATCH v2] Add Mediatek thermal support Sascha Hauer 2015-07-21 7:59 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-07-21 7:59 ` Sascha Hauer 2015-07-21 15:13 ` Daniel Kurtz 2015-07-21 15:13 ` Daniel Kurtz 2015-07-21 15:13 ` Daniel Kurtz 2015-08-05 10:20 ` Sascha Hauer 2015-08-05 10:20 ` Sascha Hauer 2015-08-05 10:20 ` Sascha Hauer 2015-07-13 10:34 [PATCH] thermal: Add Mediatek thermal support Sascha Hauer 2015-07-13 10:34 ` [PATCH 2/3] thermal: Add Mediatek thermal controller support Sascha Hauer 2015-07-13 10:34 ` Sascha Hauer
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20151110182629.GA5240@localhost.localdomain \ --to=edubezval@gmail.com \ --cc=devicetree@vger.kernel.org \ --cc=javi.merino@arm.com \ --cc=kernel@pengutronix.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=linux-pm@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=matthias.bgg@gmail.com \ --cc=robh+dt@kernel.org \ --cc=rui.zhang@intel.com \ --cc=s.hauer@pengutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.