Hi Krzysztof, Thanks for your feedback! On Fri, 3 Jan 2025 at 17:14, Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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
