Re: [PATCH v1 08/16] arm64: dts: mt8195: Add power domains controller

From: AngeloGioacchino Del Regno
Date: Tue Jul 12 2022 - 08:54:21 EST

Il 12/07/22 14:47, Krzysztof Kozlowski ha scritto:
On 12/07/2022 12:33, AngeloGioacchino Del Regno wrote:
Il 12/07/22 11:03, Krzysztof Kozlowski ha scritto:
On 12/07/2022 10:53, AngeloGioacchino Del Regno wrote:
Il 12/07/22 10:37, Krzysztof Kozlowski ha scritto:
On 12/07/2022 10:17, AngeloGioacchino Del Regno wrote:
Il 06/07/22 17:18, Krzysztof Kozlowski ha scritto:
On 06/07/2022 14:00, Tinghan Shen wrote:
Hi Krzysztof,

After discussing your message with our power team,
we realized that we need your help to ensure we fully understand you.

On Mon, 2022-07-04 at 14:38 +0200, Krzysztof Kozlowski wrote:
On 04/07/2022 12:00, Tinghan Shen wrote:
Add power domains controller node for mt8195.

Signed-off-by: Weiyi Lu <weiyi.lu@xxxxxxxxxxxx>
Signed-off-by: Tinghan Shen <tinghan.shen@xxxxxxxxxxxx>
---
arch/arm64/boot/dts/mediatek/mt8195.dtsi | 327 +++++++++++++++++++++++
1 file changed, 327 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8195.dtsi b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
index 8d59a7da3271..d52e140d9271 100644
--- a/arch/arm64/boot/dts/mediatek/mt8195.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
@@ -10,6 +10,7 @@
#include <dt-bindings/interrupt-controller/irq.h>
#include <dt-bindings/phy/phy.h>
#include <dt-bindings/pinctrl/mt8195-pinfunc.h>
+#include <dt-bindings/power/mt8195-power.h>
/ {
compatible = "mediatek,mt8195";
@@ -338,6 +339,332 @@
#interrupt-cells = <2>;
};
+ scpsys: syscon@10006000 {
+ compatible = "syscon", "simple-mfd";

These compatibles cannot be alone.

the scpsys sub node has the compatible of the power domain driver.
do you suggest that the compatible in the sub node should move to here?

Not necessarily, depends. You have here device node representing system
registers. They need they own compatibles, just like everywhere in the
kernel (except the broken cases...).

Whether this should be compatible of power-domain driver, it depends
what this device node is. I don't know, I don't have your datasheets or
your architecture diagrams...


+ reg = <0 0x10006000 0 0x1000>;
+ #power-domain-cells = <1>;

If it is simple MFD, then probably it is not a power domain provider.
Decide.

this MFD device is the power controller on mt8195.

Then it is not a simple MFD but a power controller. Do not use
"simple-mfd" compatible.

Some features need
to do some operations on registers in this node. We think that implement
the operation of these registers as the MFD device can provide flexibility
for future use. We want to clarify if you're saying that an MFD device
cannot be a power domain provider.

MFD device is Linuxism, so it has nothing to do here. I am talking only
about simple-mfd. simple-mfd is a simple device only instantiating
children and not providing anything to anyone. Neither to children. This
the most important part. The children do not depend on anything from
simple-mfd device. For example simple-mfd device can be shut down
(gated) and children should still operate. Being a power domain
controller, contradicts this usually.


If my interpretation of this issue is right, I have pushed a solution for it.
Krzysztof, Matthias, can you please check [1] and give feedback, so that
Tinghan can rewrite this commit ASAP?

Reason is - I need the MT8195 devicetree to be complete to push the remaining
pieces for Tomato Chromebooks, of course.

[1]: https://patchwork.kernel.org/project/linux-mediatek/list/?series=658527

I have two or three similar discussions, so maybe I lost the context,
but I don't understand how your fix is matching real hardware.

In the patchset here, Tinghan claimed that power domain controller is a
child of 10006000. 10006000 is also a power domain controller. This was
explicitly described by the DTS code.

Now you abandon this hierarchy in favor of syscon. If the hierarchy was
correct, your patchset does not match the hardware, so it's a no-go.
Describe the hardware.

However maybe this patch did not make any sense and there is no
relationship parent-child... so what do you guys send here? Bunch of
hacks and work-arounds?


For how I get it, hardware side, the SPM (System Power Manager) resides inside
of the SCPSYS block (consequently, in that iospace).

As Matthias pointed out earlier, SCPSYS provides more functionality, but the
only one that's currently implemented upstream is the System Power Manager,
responsible for managing the MTCMOS (power domains).

In any case, now I'm a little confused on how to proceed, can you please give
some suggestion?

You should make SCPSYS (0x10006000, AFAIU) a proper driver with its own
compatible (followed by syscon if needed), even if now it is almost
empty stub. The driver should populate OF children and then you can
embed SPM inside and reference to parent's regmap. No need for
simple-mfd. Later the SCPSYS can grow, if you ever need it.



I see an issue with such approach: the SCPSYS doesn't have a mailbox, doesn't
need power management from the Linux side, doesn't have any register to check
HW revision, it's always online (hence it doesn't need Linux to boot it), it
doesn't need any root clock, nor regulator, nor mmu context, and there's no
retrievable "boot log" of any sort.

No problems, there are other drivers who do not need any resources,
except address space.


Hence, a driver with its own compatible would be an empty stub forever: it's
not going to get any "scpsys root handling" at all, because there's none to do.

But it is a power domain provider, so you need to embed it in some
dirver, don't you?


Digging through some downstream kernels, the only other functionality that I
can find in the SCPSYS is a MODULE_RESET (which is used to reset the SCP System)
and a INFRA_IRQ_SET, used to set "wake locks" on the AP and CONNSYS (modem).

So why was power domain provider added to SCPSYS? Guys, I don't know
your architecture, so I deduct it based on pieces of DTS code I see.


Both of these may only be used in the SCP mailbox driver (which is *not* SCPSYS)
to perform an ipi_send operation (but currently we simply en/disable the clock
and that's enough), or to perform a reset and firmware reload of the SCP (but
currently we're simply powering off and back on: this may change in the future).

So, at the end of the day, we would end up having a copy of simple-pm-bus and
nothing else, which doesn't look like being optimal at all.

No, because you need that power domain driver, don't you? If you don't
need power domain provider/driver, why the heck this is there:

+ scpsys: syscon@10006000 {
+ compatible = "syscon", "simple-mfd";
+ reg = <0 0x10006000 0 0x1000>;
+ #power-domain-cells = <1>;
^^^^^^^^^^^^^^^^^
Entire discussion started from this.


Is this all a huge misunderstanding? It probably is, at least partially.

That node shouldn't have any #power-domain-cells, the only PD is the SPM node
(mediatek,mt8195-power-controller), not the scpsys parent! Ugh...


My own vision is that either using syscon (as shown in the series that you've
checked), keeping "simple-mfd", or changing it to "simple-bus" (whatever) is
the cleanest (and best approach) - please otherwise explain why having such

Again, simple-mfd is just MFD, not a power domain provider.

simple-bus should not have it's own address space, so combining it with
syscon is rather wrong.

https://lore.kernel.org/linux-devicetree/Ynq52E93mcTXcw9H@xxxxxxxxxxxxxxxxxx/

a practically forever-stub driver (practically, a copy of simple-pm-bus.c)
would be beneficial in any way.


Of course not, but your DTS is saying it is not a stub.

Best regards,
Krzysztof