Hi Krzysztof, Thanks for your review feedback, it is much appreciated! On Sun, 22 Dec 2024 at 14:24, Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote: > > On 13/12/2024 17:44, Peter Griffin wrote: > > To avoid dtschema warnings allow google,gs101-pmu to have > > two reg regions. > > > > Signed-off-by: Peter Griffin <peter.griffin@xxxxxxxxxx> > > --- > > I don't really like this patch, but also didn't want to submit the series > > with a dtschema warning ;-) > > > > Possibly a better solution is when Robs patch > > `mfd: syscon: Allow syscon nodes without a "syscon" compatible` [1] > > PMU which spans over two blocks is not a simple syscon. These would be > two syscon devices. > > If you request regmap from such syscon, which regmap you get? That is a good point, if other drivers in the future need access to the pmu-intr-gen registers then it would be good if this was modelled as its own syscon device. Another point to note is that only the PMU registers need the custom regmap registered in exynos-pmu, the PMU_INTR_GEN register region works with normal syscon / mmio accesses, so it would be a different regmap. > > I am not sure whether the PMU is really split here. Usually the main PMU > was only one and additional blocks called PMU were somehow specialized > per each IP block. PMU_INTR_GEN has its own entry in the memory map, so in that respect it's a "device" (it has its own 65k SFR region). PMU: Base Address 0x1746_0000 PMU_INTR_GEN: Base Address 0x1747_0000 The documentation isn't particularly detailed on PMU_INTR_GEN. In one place it says "One PMU interrupt generator for handshaking between PMU through interrupts". In another, "PMU and PMU_INTR_GEN are for Power management." and then we have the register names where the description it really an expanded version of the register name e.g. Register: GRP#_INTR_BID_ENABLE Description: Interrupt Bid Enable Reset Value: 0x0 Things might be a bit clearer if I had access to the firmware code on the other side of this PMU handshaking which I believe is the APM, but sadly I don't. > > 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? Thanks, Peter
