| 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: <bae19559-0aea-422f-931f-b51aa8f3f5a3@arm.com>
Date: Mon, 6 Nov 2023 09:19:21 +0000
From: Lukasz Luba <lukasz.luba@....com>
To: m.majewski2@...sung.com
Cc: Bartlomiej Zolnierkiewicz <bzolnier@...il.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-samsung-soc@...r.kernel.org"
<linux-samsung-soc@...r.kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Amit Kucheria <amitk@...nel.org>,
Zhang Rui <rui.zhang@...el.com>,
ALIM AKHTAR <alim.akhtar@...sung.com>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Marek Szyprowski <m.szyprowski@...sung.com>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v4 8/8] thermal: exynos: use set_trips
On 11/2/23 10:35, Mateusz Majewski wrote:
> Hi,
>
>>> + th &= ~(0xff << 0);
>>> + th |= temp_to_code(data, temp) << 0;
>>
>> This 2-line pattern repeats a few times. It looks like a nice cadidate
>> for an inline function which can abstract that. Something like:
>>
>> val = update_temp_value(data, temp, threshold, LOW_TEMP_SHIFT)
>>
>> Assisted with the macros {LOW|HIGH|CRIT}_TEMP_SHIFT, the code
>> would look less convoluted IMO.
>> (The old code with the multiplication for the shift value wasn't
>> cleaner nor faster).
>
> What would you think about something like this?
>
> static void exynos_tmu_update_temp(struct exynos_tmu_data *data, int reg_off,
> int bit_off, u8 temp)
> {
> u32 th;
>
> th = readl(data->base + reg_off);
> th &= ~(0xff << bit_off);
> th |= temp_to_code(data, temp) << bit_off;
> writel(th, data->base + reg_off);
> }
>
> And then, it would be used like this:
>
> static void exynos4412_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
> {
> exynos_tmu_update_temp(data, EXYNOS_THD_TEMP_RISE, 24, temp);
> exynos_tmu_update_bit(data, EXYNOS_TMU_REG_CONTROL,
> EXYNOS_TMU_THERM_TRIP_EN_SHIFT, true);
> }
Yes, this looks good.
>
> Granted it's not as clear as if we had some macro like CRIT_TEMP_SHIFT, but
> we would need more than one variant anyway, as Exynos 5433 uses different
> values of reg_off, and the new function looks short and inviting IMHO.
Fair enough.
>
>>> -static void exynos7_tmu_set_trip_temp(struct exynos_tmu_data *data,
>>> - int trip, u8 temp)
>>> +static void exynos7_tmu_update_temp(struct exynos_tmu_data *data, u8 temp,
>>> + int idx, bool rise)
>>> {
>>> unsigned int reg_off, bit_off;
>>> u32 th;
>>> + void __iomem *reg;
>>>
>>> - reg_off = ((7 - trip) / 2) * 4;
>>> - bit_off = ((8 - trip) % 2);
>>> + reg_off = ((7 - idx) / 2) * 4;
>>
>> Why can't we just have a set of defined register macros and pick one
>> in some small function?
>> A lot of operations here, also some assumption.
>>
>>> + bit_off = ((8 - idx) % 2);
>>
>> So this can only be 0 or 1 and than it's used for the shift
>> multiplication. Also I don't know the history of older code and
>> if it was missed after some cleaning, but 'idx % 2' gives
>> equal values but w/o subtraction.
>>
>> BTW, the code assumes the 'idx' values are under control somewhere else.
>> Is that because the DT make sure in the schema that the range cannot be
>> too big?
>> What are the possible values for 'idx'?
>
> In the old code, the values of trip (which is the same thing, I will
> change the name back from idx) were limited by the value of data->ntrip,
> which was always 8 (value is per SoC). In the new code, there are only three
> variants:
>
> static void exynos7_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
> {
> exynos7_tmu_update_temp(data, temp, 0, false);
> }
>
> static void exynos7_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
> {
> exynos7_tmu_update_temp(data, temp, 1, true);
> }
>
> static void exynos7_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
> {
> /*
> * Like Exynos 4210, Exynos 7 does not seem to support critical temperature
> * handling in hardware. Again, we still set a separate interrupt for it.
> */
> exynos7_tmu_update_temp(data, temp, 7, true);
> }
>
> To be fair, considering the values are constant like this, I should probably
> just do the calculations myself and then in code just call exynos_tmu_update_temp
> (from above) and exynos_tmu_update_bit, like on all other SoCs. I guess I were
> a bit too scared to touch Exynos 7 code...
Yes, anything that can be pre-calculated with nice comment, would be
more desired. I would suggest to not be afraid about touching that
Exynos 7 code.
>
>>> - if (on) {
>>> - for (i = 0; i < data->ntrip; i++) {
>>> - if (thermal_zone_get_trip(tz, i, &trip))
>>> - continue;
>>> -
>>> - interrupt_en |=
>>> - (1 << (EXYNOS_TMU_INTEN_RISE0_SHIFT + i * 4));
>>> - }
>>> -
>>> - if (data->soc != SOC_ARCH_EXYNOS4210)
>>> - interrupt_en |=
>>> - interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT;
>>> -
>>> + if (on)
>>> con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT);
>>> - } else {
>>> + else
>>> con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT);
>>
>> Please also consider the BIT() helper here and above...
>
> Will do, but should I do this in a separate patch in these cases? I don't touch
> the con lines otherwise, and this patch is already humongous.
That would definitely deserve an extra patch to address it. Please add
to the patch set.
Regards,
Lukasz
Powered by blists - more mailing lists