linux-kernel - Re: [PATCH 5/6] drivers/thermal/exynos: add initial Exynos 850 support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240723141658.374755-1-m.majewski2@samsung.com>
Date: Tue, 23 Jul 2024 16:16:54 +0200
From: Mateusz Majewski <m.majewski2@...sung.com>
To: Sam Protsenko <semen.protsenko@...aro.org>
Cc: Mateusz Majewski <m.majewski2@...sung.com>, linux-pm@...r.kernel.org,
	linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>, Krzysztof Kozlowski
	<krzk@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>, Daniel Lezcano
	<daniel.lezcano@...aro.org>, Zhang Rui <rui.zhang@...el.com>, Lukasz Luba
	<lukasz.luba@....com>, Rob Herring <robh@...nel.org>, Conor Dooley
	<conor+dt@...nel.org>, Alim Akhtar <alim.akhtar@...sung.com>
Subject: Re: [PATCH 5/6] drivers/thermal/exynos: add initial Exynos 850
 support

> Do you know what are the possible implications of not using ACPM? As I
> understand, ACPM is a Samsung's downstream framework which uses APM
> (Active Power Management) IP block internally to act as an IPC
> mechanism, which makes it possible to offload any PM related
> operations (which might get quite heavy, if we are to belive the TRM
> description of APM) from CPU to APM. I'm not against the direct
> registers access based implementation (in fact, I'm not sure how that
> APM/ACPM thing can be implemented in upstreamable way and if it's
> worth it at all). Just curious if we understand what we are
> potentially missing out, and if at some point we'll be forced to
> implement that ACPM thing anyway (for something else)?

Not sure honestly. The downstream v4.10 driver does many operations on
registers anyway...?

> Not sure if that's true, as already discussed in my comments for the
> previous patches. Looks like one clock is still needed, which is the
> PCLK bus clock (to interface registers) which might simultaneously act
> as an operating (functional) clock.

The code seems to be working correctly without this clock, both register
reads and writes. Maybe the support for extra sensors, which I couldn't
get to work, would require this clock?

> Exynos850 TRM says AVG_CONTROL offset is 0x38, and 0x58 is actually
> for THRESHOLD0_TEMP_RISE3_2 register.

Thank you so much! Will fix in v2. Though writing to the right place
doesn't seem to change much in practice, probably just means that the
correct mode is being used.

> Something seems off to me here. How come the shift value for EXYNOS7
> case is 8, but the mask is actually 9 bits long? Does it mean the
> first error field is 8 bits long, and the second error field is 9 bits
> long for EXYNOS7? I don't have the Exynos7 manual, so it's just a
> hunch. But if it's true, maybe this shift value has to be added in
> your [PATCH 2/6] to fix Exynos7 case?

I did not really want to mess with Exynos7 code, as we don't have an
Exynos7 board sadly. Honestly I feel like I should drop the 2/6 patch
completely and only modify the code to run on 850 correctly.

> Also, just an idea: those values (and other similar values) could be
> pre-calculated somewhere during the probe, stored in some struct (e.g.
> _variant or _chip) and then just used here.

sanitize_temp_error is only called one per probe and once per resume, so
probably little to gain?

Will also do all other.