| Message ID | 20220704100028.19932-9-tinghan.shen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add driver nodes for MT8195 SoC | expand |
Il 04/07/22 12:00, Tinghan Shen ha scritto: > 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> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
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. > + reg = <0 0x10006000 0 0x1000>; > + #power-domain-cells = <1>; If it is simple MFD, then probably it is not a power domain provider. Decide. Best regards, Krzysztof
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? > > + 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. 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. Best regards, TingHan
On 04/07/2022 14:38, 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. > You mean we would need something like "mediatek,scpsys" as dummy compatible that's not bound to any driver? >> + reg = <0 0x10006000 0 0x1000>; >> + #power-domain-cells = <1>; > > If it is simple MFD, then probably it is not a power domain provider. > Decide. The SCPSYS IP block of MediaTek SoCs group several functionality, one is the power domain controller. Others are not yet implemented, but defining the scpsys as a MFD will give us the possibility to do so in the future. Regards, Matthias > > Best regards, > Krzysztof
On 06/07/2022 15:41, Matthias Brugger wrote: > > > On 04/07/2022 14:38, 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. >> > > You mean we would need something like "mediatek,scpsys" as dummy compatible > that's not bound to any driver? Yes. syscon (and simple-mfd) must always come with a specific compatible. > >>> + reg = <0 0x10006000 0 0x1000>; >>> + #power-domain-cells = <1>; >> >> If it is simple MFD, then probably it is not a power domain provider. >> Decide. > > The SCPSYS IP block of MediaTek SoCs group several functionality, one is the > power domain controller. Others are not yet implemented, but defining the scpsys > as a MFD will give us the possibility to do so in the future. No, quite the opposite. Having simple-mfd prevents you from implementing it correctly later as a driver, because you cannot remove it. It would be ABI break. It's fine to have one block being a simple MFD having several children, but then it's not a power controller. Children could be such power controller, but not simple-mfd. Rob explained this several times: https://lore.kernel.org/all/YXhINE00HG6hbQI4@robh.at.kernel.org/ https://lore.kernel.org/all/20220701000959.GA3588170-robh@kernel.org/ Best regards, Krzysztof
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. Best regards, Krzysztof
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
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@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 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? Your DTS should reflect the hardware, not some hacks. Best regards, Krzysztof
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@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 > > 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? Thanks, Angelo > Your DTS should reflect the hardware, not some hacks. > > Best regards, > Krzysztof
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@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 >> >> 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. Best regards, Krzysztof
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@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 >>> >>> 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. 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. 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). 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. 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 a practically forever-stub driver (practically, a copy of simple-pm-bus.c) would be beneficial in any way. Regards, Angelo > > Best regards, > Krzysztof
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@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 >>>> >>>> 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. > > 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
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@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 >>>>> >>>>> 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@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
On 12/07/2022 14:54, AngeloGioacchino Del Regno wrote: > 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@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 >>>>>> >>>>>> 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 comment was quite clear I think: > + #power-domain-cells = <1>; If it is simple MFD, then probably it is not a power domain provider. Decide. and you keep telling me that it is a power domain provider and MFD and something more. Anyway neither syscon nor simple-mfd can be without specific compatible. Best regards, Krzysztof
Il 12/07/22 14:58, Krzysztof Kozlowski ha scritto: > On 12/07/2022 14:54, AngeloGioacchino Del Regno wrote: >> 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@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 >>>>>>> >>>>>>> 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 comment was quite clear I think: > > > + #power-domain-cells = <1>; > If it is simple MFD, then probably it is not a power domain provider. > Decide. Yes it was quite clear. It's entirely my fault for misreading that part and I'm truly sorry for that. > > and you keep telling me that it is a power domain provider and MFD and > something more. > > Anyway neither syscon nor simple-mfd can be without specific compatible. > I believe, at this point, that adding a compatible like "mediatek,scpsys" in mfd/syscon.yaml should do? > Best regards, > Krzysztof
On 12/07/2022 15:03, AngeloGioacchino Del Regno wrote: >> and you keep telling me that it is a power domain provider and MFD and >> something more. >> >> Anyway neither syscon nor simple-mfd can be without specific compatible. >> > > I believe, at this point, that adding a compatible like "mediatek,scpsys" in > mfd/syscon.yaml should do? Yes. Or dedicated file, like other mediatek syscons. Best regards, Krzysztof
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"; + reg = <0 0x10006000 0 0x1000>; + #power-domain-cells = <1>; + + /* System Power Manager */ + spm: power-controller { + compatible = "mediatek,mt8195-power-controller"; + #address-cells = <1>; + #size-cells = <0>; + #power-domain-cells = <1>; + + /* power domain of the SoC */ + mfg0: power-domain@MT8195_POWER_DOMAIN_MFG0 { + reg = <MT8195_POWER_DOMAIN_MFG0>; + #address-cells = <1>; + #size-cells = <0>; + #power-domain-cells = <1>; + + power-domain@MT8195_POWER_DOMAIN_MFG1 { + reg = <MT8195_POWER_DOMAIN_MFG1>; + clocks = <&apmixedsys CLK_APMIXED_MFGPLL>; + clock-names = "mfg"; + mediatek,infracfg = <&infracfg_ao>; + #address-cells = <1>; + #size-cells = <0>; + #power-domain-cells = <1>; + + power-domain@MT8195_POWER_DOMAIN_MFG2 { + reg = <MT8195_POWER_DOMAIN_MFG2>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_MFG3 { + reg = <MT8195_POWER_DOMAIN_MFG3>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_MFG4 { + reg = <MT8195_POWER_DOMAIN_MFG4>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_MFG5 { + reg = <MT8195_POWER_DOMAIN_MFG5>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_MFG6 { + reg = <MT8195_POWER_DOMAIN_MFG6>; + #power-domain-cells = <0>; + }; + }; + }; + + power-domain@MT8195_POWER_DOMAIN_VPPSYS0 { + reg = <MT8195_POWER_DOMAIN_VPPSYS0>; + clocks = <&topckgen CLK_TOP_VPP>, + <&topckgen CLK_TOP_CAM>, + <&topckgen CLK_TOP_CCU>, + <&topckgen CLK_TOP_IMG>, + <&topckgen CLK_TOP_VENC>, + <&topckgen CLK_TOP_VDEC>, + <&topckgen CLK_TOP_WPE_VPP>, + <&topckgen CLK_TOP_CFG_VPP0>, + <&vppsys0 CLK_VPP0_SMI_COMMON>, + <&vppsys0 CLK_VPP0_GALS_VDO0_LARB0>, + <&vppsys0 CLK_VPP0_GALS_VDO0_LARB1>, + <&vppsys0 CLK_VPP0_GALS_VENCSYS>, + <&vppsys0 CLK_VPP0_GALS_VENCSYS_CORE1>, + <&vppsys0 CLK_VPP0_GALS_INFRA>, + <&vppsys0 CLK_VPP0_GALS_CAMSYS>, + <&vppsys0 CLK_VPP0_GALS_VPP1_LARB5>, + <&vppsys0 CLK_VPP0_GALS_VPP1_LARB6>, + <&vppsys0 CLK_VPP0_SMI_REORDER>, + <&vppsys0 CLK_VPP0_SMI_IOMMU>, + <&vppsys0 CLK_VPP0_GALS_IMGSYS_CAMSYS>, + <&vppsys0 CLK_VPP0_GALS_EMI0_EMI1>, + <&vppsys0 CLK_VPP0_SMI_SUB_COMMON_REORDER>, + <&vppsys0 CLK_VPP0_SMI_RSI>, + <&vppsys0 CLK_VPP0_SMI_COMMON_LARB4>, + <&vppsys0 CLK_VPP0_GALS_VDEC_VDEC_CORE1>, + <&vppsys0 CLK_VPP0_GALS_VPP1_WPE>, + <&vppsys0 CLK_VPP0_GALS_VDO0_VDO1_VENCSYS_CORE1>; + clock-names = "vppsys", "vppsys1", "vppsys2", "vppsys3", + "vppsys4", "vppsys5", "vppsys6", "vppsys7", + "vppsys0-0", "vppsys0-1", "vppsys0-2", "vppsys0-3", + "vppsys0-4", "vppsys0-5", "vppsys0-6", "vppsys0-7", + "vppsys0-8", "vppsys0-9", "vppsys0-10", "vppsys0-11", + "vppsys0-12", "vppsys0-13", "vppsys0-14", + "vppsys0-15", "vppsys0-16", "vppsys0-17", + "vppsys0-18"; + mediatek,infracfg = <&infracfg_ao>; + #address-cells = <1>; + #size-cells = <0>; + #power-domain-cells = <1>; + + power-domain@MT8195_POWER_DOMAIN_VDEC1 { + reg = <MT8195_POWER_DOMAIN_VDEC1>; + clocks = <&vdecsys CLK_VDEC_LARB1>; + clock-names = "vdec1-0"; + mediatek,infracfg = <&infracfg_ao>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_VENC_CORE1 { + reg = <MT8195_POWER_DOMAIN_VENC_CORE1>; + mediatek,infracfg = <&infracfg_ao>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_VDOSYS0 { + reg = <MT8195_POWER_DOMAIN_VDOSYS0>; + clocks = <&topckgen CLK_TOP_CFG_VDO0>, + <&vdosys0 CLK_VDO0_SMI_GALS>, + <&vdosys0 CLK_VDO0_SMI_COMMON>, + <&vdosys0 CLK_VDO0_SMI_EMI>, + <&vdosys0 CLK_VDO0_SMI_IOMMU>, + <&vdosys0 CLK_VDO0_SMI_LARB>, + <&vdosys0 CLK_VDO0_SMI_RSI>; + clock-names = "vdosys0", "vdosys0-0", "vdosys0-1", + "vdosys0-2", "vdosys0-3", + "vdosys0-4", "vdosys0-5"; + mediatek,infracfg = <&infracfg_ao>; + #address-cells = <1>; + #size-cells = <0>; + #power-domain-cells = <1>; + + power-domain@MT8195_POWER_DOMAIN_VPPSYS1 { + reg = <MT8195_POWER_DOMAIN_VPPSYS1>; + clocks = <&topckgen CLK_TOP_CFG_VPP1>, + <&vppsys1 CLK_VPP1_VPPSYS1_GALS>, + <&vppsys1 CLK_VPP1_VPPSYS1_LARB>; + clock-names = "vppsys1", "vppsys1-0", + "vppsys1-1"; + mediatek,infracfg = <&infracfg_ao>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_WPESYS { + reg = <MT8195_POWER_DOMAIN_WPESYS>; + clocks = <&wpesys CLK_WPE_SMI_LARB7>, + <&wpesys CLK_WPE_SMI_LARB8>, + <&wpesys CLK_WPE_SMI_LARB7_P>, + <&wpesys CLK_WPE_SMI_LARB8_P>; + clock-names = "wepsys-0", "wepsys-1", "wepsys-2", + "wepsys-3"; + mediatek,infracfg = <&infracfg_ao>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_VDEC0 { + reg = <MT8195_POWER_DOMAIN_VDEC0>; + clocks = <&vdecsys_soc CLK_VDEC_SOC_LARB1>; + clock-names = "vdec0-0"; + mediatek,infracfg = <&infracfg_ao>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_VDEC2 { + reg = <MT8195_POWER_DOMAIN_VDEC2>; + clocks = <&vdecsys_core1 CLK_VDEC_CORE1_LARB1>; + clock-names = "vdec2-0"; + mediatek,infracfg = <&infracfg_ao>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_VENC { + reg = <MT8195_POWER_DOMAIN_VENC>; + mediatek,infracfg = <&infracfg_ao>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_VDOSYS1 { + reg = <MT8195_POWER_DOMAIN_VDOSYS1>; + clocks = <&topckgen CLK_TOP_CFG_VDO1>, + <&vdosys1 CLK_VDO1_SMI_LARB2>, + <&vdosys1 CLK_VDO1_SMI_LARB3>, + <&vdosys1 CLK_VDO1_GALS>; + clock-names = "vdosys1", "vdosys1-0", + "vdosys1-1", "vdosys1-2"; + mediatek,infracfg = <&infracfg_ao>; + #address-cells = <1>; + #size-cells = <0>; + #power-domain-cells = <1>; + + power-domain@MT8195_POWER_DOMAIN_DP_TX { + reg = <MT8195_POWER_DOMAIN_DP_TX>; + mediatek,infracfg = <&infracfg_ao>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_EPD_TX { + reg = <MT8195_POWER_DOMAIN_EPD_TX>; + mediatek,infracfg = <&infracfg_ao>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_HDMI_TX { + reg = <MT8195_POWER_DOMAIN_HDMI_TX>; + clocks = <&topckgen CLK_TOP_HDMI_APB>; + clock-names = "hdmi_tx"; + #power-domain-cells = <0>; + }; + }; + + power-domain@MT8195_POWER_DOMAIN_IMG { + reg = <MT8195_POWER_DOMAIN_IMG>; + clocks = <&imgsys CLK_IMG_LARB9>, + <&imgsys CLK_IMG_GALS>; + clock-names = "img-0", "img-1"; + mediatek,infracfg = <&infracfg_ao>; + #address-cells = <1>; + #size-cells = <0>; + #power-domain-cells = <1>; + + power-domain@MT8195_POWER_DOMAIN_DIP { + reg = <MT8195_POWER_DOMAIN_DIP>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_IPE { + reg = <MT8195_POWER_DOMAIN_IPE>; + clocks = <&topckgen CLK_TOP_IPE>, + <&imgsys CLK_IMG_IPE>, + <&ipesys CLK_IPE_SMI_LARB12>; + clock-names = "ipe", "ipe-0", "ipe-1"; + mediatek,infracfg = <&infracfg_ao>; + #power-domain-cells = <0>; + }; + }; + + power-domain@MT8195_POWER_DOMAIN_CAM { + reg = <MT8195_POWER_DOMAIN_CAM>; + clocks = <&camsys CLK_CAM_LARB13>, + <&camsys CLK_CAM_LARB14>, + <&camsys CLK_CAM_CAM2MM0_GALS>, + <&camsys CLK_CAM_CAM2MM1_GALS>, + <&camsys CLK_CAM_CAM2SYS_GALS>; + clock-names = "cam-0", "cam-1", "cam-2", "cam-3", + "cam-4"; + mediatek,infracfg = <&infracfg_ao>; + #address-cells = <1>; + #size-cells = <0>; + #power-domain-cells = <1>; + + power-domain@MT8195_POWER_DOMAIN_CAM_RAWA { + reg = <MT8195_POWER_DOMAIN_CAM_RAWA>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_CAM_RAWB { + reg = <MT8195_POWER_DOMAIN_CAM_RAWB>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_CAM_MRAW { + reg = <MT8195_POWER_DOMAIN_CAM_MRAW>; + #power-domain-cells = <0>; + }; + }; + }; + }; + + power-domain@MT8195_POWER_DOMAIN_PCIE_MAC_P0 { + reg = <MT8195_POWER_DOMAIN_PCIE_MAC_P0>; + mediatek,infracfg = <&infracfg_ao>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_PCIE_MAC_P1 { + reg = <MT8195_POWER_DOMAIN_PCIE_MAC_P1>; + mediatek,infracfg = <&infracfg_ao>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_PCIE_PHY { + reg = <MT8195_POWER_DOMAIN_PCIE_PHY>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_SSUSB_PCIE_PHY { + reg = <MT8195_POWER_DOMAIN_SSUSB_PCIE_PHY>; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_CSI_RX_TOP { + reg = <MT8195_POWER_DOMAIN_CSI_RX_TOP>; + clocks = <&topckgen CLK_TOP_SENINF>, + <&topckgen CLK_TOP_SENINF2>; + clock-names = "csi_rx_top", "csi_rx_top1"; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_ETHER { + reg = <MT8195_POWER_DOMAIN_ETHER>; + clocks = <&pericfg_ao CLK_PERI_AO_ETHERNET_MAC>; + clock-names = "ether"; + #power-domain-cells = <0>; + }; + + power-domain@MT8195_POWER_DOMAIN_ADSP { + reg = <MT8195_POWER_DOMAIN_ADSP>; + clocks = <&topckgen CLK_TOP_ADSP>, + <&topckgen CLK_TOP_AUDIO_LOCAL_BUS>; + clock-names = "adsp", "adsp1"; + #address-cells = <1>; + #size-cells = <0>; + mediatek,infracfg = <&infracfg_ao>; + #power-domain-cells = <1>; + + power-domain@MT8195_POWER_DOMAIN_AUDIO { + reg = <MT8195_POWER_DOMAIN_AUDIO>; + clocks = <&topckgen CLK_TOP_A1SYS_HP>, + <&topckgen CLK_TOP_AUD_INTBUS>, + <&topckgen CLK_TOP_AUDIO_LOCAL_BUS>, + <&infracfg_ao CLK_INFRA_AO_AUDIO_26M_B>; + clock-names = "audio", "audio1", "audio2", + "audio3"; + mediatek,infracfg = <&infracfg_ao>; + #power-domain-cells = <0>; + }; + }; + }; + }; + watchdog: watchdog@10007000 { compatible = "mediatek,mt8195-wdt", "mediatek,mt6589-wdt";