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

[Krzysztof Kozlowski]

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 at mediatek.com>
>>>>>>>>> Signed-off-by: Tinghan Shen <tinghan.shen at mediatek.com>
>>>>>>>>> ---
>>>>>>>>>     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 at 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 at 10006000 {
+			compatible = "syscon", "simple-mfd";
+			reg = <0 0x10006000 0 0x1000>;
+			#power-domain-cells = <1>;
                        ^^^^^^^^^^^^^^^^^
Entire discussion started from this.

> 
> 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@robh.at.kernel.org/

> 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