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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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@mediatek.com>
>>>> Signed-off-by: Tinghan Shen <tinghan.shen@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@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

Thanks a lot,
Angelo