linux-kernel - Re: [PATCH 2/4] dt-bindings: mfd: syscon: allow two reg regions for gs101-pmu

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] [day] [month] [year] [list]

Message-ID: <CADrjBPoLhc9Fy1FYr4w33n7bJH0XkddAyQL8zgHhj+7ONngu5Q@mail.gmail.com>
Date: Mon, 6 Jan 2025 13:41:09 +0000
From: Peter Griffin <peter.griffin@...aro.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Alim Akhtar <alim.akhtar@...sung.com>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Lee Jones <lee@...nel.org>, 
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	tudor.ambarus@...aro.org, andre.draszik@...aro.org, willmcvicker@...gle.com, 
	kernel-team@...roid.com
Subject: Re: [PATCH 2/4] dt-bindings: mfd: syscon: allow two reg regions for gs101-pmu

Hi Krzysztof,

Thanks for your feedback!

On Fri, 3 Jan 2025 at 17:14, Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On 30/12/2024 10:10, Peter Griffin wrote:
> >
> >>
> >> Maybe you have here two devices, maybe only one. If it is only one, then
> >> it is not a syscon anymore, IMO.
> >
> > I was going to suggest modelling PMU_INTR_GEN as its own sycon node,
> > and then either: -
> >
> > 1) Updating exynos-pmu driver to additionally take a phandle to
> > pmu-intr-gen syscon, and register the hotplug callbacks.
> >
> > or
> >
> > 2) Create a new driver named something like exynos-pm or exynos-cpupm
> > which obtains the PMU regmap and also a phandle to PMU_INTR_GEN
> > syscon, and register the call backs.
> >
> > Is there any preference from your side over approach 1 or 2, or maybe
> > something else entirely?
>
> No preference, choose whatever results in simpler or more readable code.
>
> Option 1 assumes that exynos-pmu on GS101 will drop the "syscon"
> compatible, although it still might expose syscon through drivers. Just
> the standard binding syscon does not feel fit here.

I agree we should drop syscon compatible for gs101 as it requires a
"special" regmap. However other Exynos based SoCs will likely want to
re-use this pmu_intr_gen CPU pm code and they will likely have syscon
compatible in their exynos-pmu node (as protecting PMU registers from
Linux AFAIK was a Google hardening measure).

So just to clarify, dropping syscon compatible on option 1 is because
it's gs101 "special" regmap, or because exynos-pmu node now references
additional pmu_intr_gen syscon?

>
> I don't have the hardware/user manual, so I don't know what PMU_INTR_GEN
> really is.

There isn't much description in the manual, but AIUI pmu_intr_gen is
just a way for the OS to trigger an interrupt so that the APM programs
the PMU registers instead of the OS programming PMU registers
directly. It looks like the system could also be configured to not use
APM (it would need different firmware), in which case the OS would
just program PMU registers directly.

I only see these GRP(x)_INTR_BID_ENABLE / GRP(x)_INTR_BID_CLEAR
registers mentioned in downstream code in the context of
flexpmu_cal_system_gs101.h (which is basically lists of registers to
program for different power/sleep modes - which looks like what
exynos-pmu is currently doing for older chipsets) and
flexpmu_cal_cpu_gs101.h (which is used for cpu on/off) related things.

So even if I split the CPU pm parts into a separate driver, it looks
like programming pmu-intr-gen regs would still be required to
enter/exit sleep modes.

With that in mind I think it seems more natural to grow the exynos-pmu
node & driver.

> GS downstream code has something like PMUCAL, which looks
> like separate device.

PMUCAL is just the PMU registers with a whole bunch of layering. I
believe CAL just stands for Cpu Abstraction Layer and seems to be used
in downstream Samsung driver code when they have a bunch of "generic"
driver code and then what appears to be a lot of automatically
generated header files for a particular SoC for reading/writing all
the SFRs.

The CAL suffix is used for PMU and also for clocks (CMU). Most of the
PMUCAL code is just accessing PMU and pmu-intr-gen registers (if APM
is configured). There are some places like flexpmu_cal_system_gs101.h
where it seems to be used as a convenient place to read/write
registers all over the SoC memory map (CMU, SYSREG* etc). So unpicking
that into all the various subsystems will be interesting.

Thanks,

Peter