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

[Peter Griffin]

Hi Krzysztof,

Thanks for your review feedback, it is much appreciated!

On Sun, 22 Dec 2024 at 14:24, Krzysztof Kozlowski <krzk at kernel.org> 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 at linaro.org>
> > ---
> > 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