Message-ID: <0ca0e46d-0685-8228-4d26-c6cf20d7a9fc@collabora.com>
Date:   Tue, 12 Jul 2022 14:54:11 +0200
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Tinghan Shen <tinghan.shen@...iatek.com>,
        Yong Wu <yong.wu@...iatek.com>, Joerg Roedel <joro@...tes.org>,
        Will Deacon <will@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Chun-Jie Chen <chun-jie.chen@...iatek.com>,
        Weiyi Lu <weiyi.lu@...iatek.com>
Cc:     iommu@...ts.linux-foundation.org,
        linux-mediatek@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Project_Global_Chrome_Upstream_Group@...iatek.com
Subject: Re: [PATCH v1 08/16] arm64: dts: mt8195: Add power domains controller

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@...iatek.com>
>>>>>>>>>> Signed-off-by: Tinghan Shen <tinghan.shen@...iatek.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@...06000 {
>>>>>>>>>> +			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@...06000 {
> +			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@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

Powered by blists - more mailing lists