2024-05-18 21:12:20

by Krzysztof Kozlowski

[permalink] [raw]
Subject: [PATCH 1/4] dt-bindings: mfd: mediatek,mt8195-scpsys: add mediatek,mt8365-scpsys

Add a new mediatek,mt8365-scpsys compatible, for the SCPSYS syscon block
having power controller. Previously the DTS was re-using SYSCFG
compatible, but that does not seem right, because SYSCFG does not have
children.

Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
.../devicetree/bindings/mfd/mediatek,mt8195-scpsys.yaml | 1 +
1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt8195-scpsys.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt8195-scpsys.yaml
index c8c4812fffe2..1aa27d14145b 100644
--- a/Documentation/devicetree/bindings/mfd/mediatek,mt8195-scpsys.yaml
+++ b/Documentation/devicetree/bindings/mfd/mediatek,mt8195-scpsys.yaml
@@ -24,6 +24,7 @@ properties:
- mediatek,mt8186-scpsys
- mediatek,mt8192-scpsys
- mediatek,mt8195-scpsys
+ - mediatek,mt8365-scpsys
- const: syscon
- const: simple-mfd

--
2.43.0



2024-05-18 21:12:25

by Krzysztof Kozlowski

[permalink] [raw]
Subject: [PATCH 2/4] arm64: dts: mediatek: mt8365: use a specific SCPSYS compatible

SoCs should use dedicated compatibles for each of their syscon nodes to
precisely describe the block. Using an incorrect compatible does not
allow to properly match/validate children of the syscon device. Replace
SYSCFG compatible, which does not have children, with a new dedicated
one for SCPSYS block.

Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
arch/arm64/boot/dts/mediatek/mt8365.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8365.dtsi b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
index 24581f7410aa..d3da5a22c2d2 100644
--- a/arch/arm64/boot/dts/mediatek/mt8365.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
@@ -300,7 +300,7 @@ syscfg_pctl: syscfg-pctl@10005000 {
};

scpsys: syscon@10006000 {
- compatible = "mediatek,mt8365-syscfg", "syscon", "simple-mfd";
+ compatible = "mediatek,mt8365-scpsys", "syscon", "simple-mfd";
reg = <0 0x10006000 0 0x1000>;
#power-domain-cells = <1>;

--
2.43.0


2024-05-18 21:12:56

by Krzysztof Kozlowski

[permalink] [raw]
Subject: [PATCH 4/4] arm64: dts: mediatek: mt8173-elm: correct PMIC's syscon reg entry

The MT6297 PMIC has address/size cells == 1, thus its syscon child node
has incorrect number of entries in "reg" property. Fix dtbs_check
warning:

mt8173-elm.dtb: syscon@c000: reg: [[0, 49152], [0, 264]] is too long

Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
index 6d962d437e02..0e2439860223 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
@@ -1138,7 +1138,7 @@ rtc: mt6397rtc {
syscfg_pctl_pmic: syscon@c000 {
compatible = "mediatek,mt6397-pctl-pmic-syscfg",
"syscon";
- reg = <0 0x0000c000 0 0x0108>;
+ reg = <0x0000c000 0x0108>;
};
};
};
--
2.43.0


2024-05-18 21:13:06

by Krzysztof Kozlowski

[permalink] [raw]
Subject: [PATCH 3/4] arm64: dts: mediatek: mt8365: drop incorrect power-domain-cells

The top SCPSYS node is not a power domain provider. It's child
"power-controller" is instead. Fix dtbs_check warnings like:

mt8365-evk.dtb: syscon@10006000: '#power-domain-cells' does not match any of the regexes: 'pinctrl-[0-9]+'

Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
arch/arm64/boot/dts/mediatek/mt8365.dtsi | 1 -
1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8365.dtsi b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
index d3da5a22c2d2..eb449bfa8803 100644
--- a/arch/arm64/boot/dts/mediatek/mt8365.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
@@ -302,7 +302,6 @@ syscfg_pctl: syscfg-pctl@10005000 {
scpsys: syscon@10006000 {
compatible = "mediatek,mt8365-scpsys", "syscon", "simple-mfd";
reg = <0 0x10006000 0 0x1000>;
- #power-domain-cells = <1>;

/* System Power Manager */
spm: power-controller {
--
2.43.0


Subject: Re: [PATCH 2/4] arm64: dts: mediatek: mt8365: use a specific SCPSYS compatible

Il 18/05/24 23:11, Krzysztof Kozlowski ha scritto:
> SoCs should use dedicated compatibles for each of their syscon nodes to
> precisely describe the block. Using an incorrect compatible does not
> allow to properly match/validate children of the syscon device. Replace
> SYSCFG compatible, which does not have children, with a new dedicated
> one for SCPSYS block.
>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>

Technically, that's not a SCPSYS block, but called SYSCFG in MT8365, but the
meaning and the functioning is the same, so it's fine for me.

Reviewed-by: AngeloGioacchino Del Regno <[email protected]>


Subject: Re: [PATCH 3/4] arm64: dts: mediatek: mt8365: drop incorrect power-domain-cells

Il 18/05/24 23:11, Krzysztof Kozlowski ha scritto:
> The top SCPSYS node is not a power domain provider. It's child
> "power-controller" is instead. Fix dtbs_check warnings like:
>
> mt8365-evk.dtb: syscon@10006000: '#power-domain-cells' does not match any of the regexes: 'pinctrl-[0-9]+'
>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>

Well if you're fixing that by migrating to scpsys compatible, you might as well
resolve all of the warnings in one commit, removing that power-domain-cells
property in patch [2/4], otherwise this one is technically a fix for that.

Please squash [2/4] and [3/4], like that it just makes more sense.

Cheers,
Angelo

> ---
> arch/arm64/boot/dts/mediatek/mt8365.dtsi | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365.dtsi b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> index d3da5a22c2d2..eb449bfa8803 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> @@ -302,7 +302,6 @@ syscfg_pctl: syscfg-pctl@10005000 {
> scpsys: syscon@10006000 {
> compatible = "mediatek,mt8365-scpsys", "syscon", "simple-mfd";
> reg = <0 0x10006000 0 0x1000>;
> - #power-domain-cells = <1>;
>
> /* System Power Manager */
> spm: power-controller {


Subject: Re: [PATCH 1/4] dt-bindings: mfd: mediatek,mt8195-scpsys: add mediatek,mt8365-scpsys

Il 18/05/24 23:11, Krzysztof Kozlowski ha scritto:
> Add a new mediatek,mt8365-scpsys compatible, for the SCPSYS syscon block
> having power controller. Previously the DTS was re-using SYSCFG
> compatible, but that does not seem right, because SYSCFG does not have
> children.
>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>

Reviewed-by: AngeloGioacchino Del Regno <[email protected]>



2024-05-20 10:03:14

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 3/4] arm64: dts: mediatek: mt8365: drop incorrect power-domain-cells

On 20/05/2024 11:58, AngeloGioacchino Del Regno wrote:
> Il 18/05/24 23:11, Krzysztof Kozlowski ha scritto:
>> The top SCPSYS node is not a power domain provider. It's child
>> "power-controller" is instead. Fix dtbs_check warnings like:
>>
>> mt8365-evk.dtb: syscon@10006000: '#power-domain-cells' does not match any of the regexes: 'pinctrl-[0-9]+'
>>
>> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>
> Well if you're fixing that by migrating to scpsys compatible, you might as well
> resolve all of the warnings in one commit, removing that power-domain-cells
> property in patch [2/4], otherwise this one is technically a fix for that.
>
> Please squash [2/4] and [3/4], like that it just makes more sense.
>

That's independent thing. Previous compatible - syscfg - also did not
allow power domains. The difference is that bindings did not print a
warning without my change. We can reverse the patches if this is more
suitable.

Best regards,
Krzysztof


2024-05-20 10:03:42

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 2/4] arm64: dts: mediatek: mt8365: use a specific SCPSYS compatible

On 20/05/2024 11:55, AngeloGioacchino Del Regno wrote:
> Il 18/05/24 23:11, Krzysztof Kozlowski ha scritto:
>> SoCs should use dedicated compatibles for each of their syscon nodes to
>> precisely describe the block. Using an incorrect compatible does not
>> allow to properly match/validate children of the syscon device. Replace
>> SYSCFG compatible, which does not have children, with a new dedicated
>> one for SCPSYS block.
>>
>> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>
> Technically, that's not a SCPSYS block, but called SYSCFG in MT8365, but the
> meaning and the functioning is the same, so it's fine for me.

So there are two syscfg blocks? With exactly the same set of registers
or different?

Best regards,
Krzysztof


Subject: Re: [PATCH 3/4] arm64: dts: mediatek: mt8365: drop incorrect power-domain-cells

Il 20/05/24 12:03, Krzysztof Kozlowski ha scritto:
> On 20/05/2024 11:58, AngeloGioacchino Del Regno wrote:
>> Il 18/05/24 23:11, Krzysztof Kozlowski ha scritto:
>>> The top SCPSYS node is not a power domain provider. It's child
>>> "power-controller" is instead. Fix dtbs_check warnings like:
>>>
>>> mt8365-evk.dtb: syscon@10006000: '#power-domain-cells' does not match any of the regexes: 'pinctrl-[0-9]+'
>>>
>>> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>>
>> Well if you're fixing that by migrating to scpsys compatible, you might as well
>> resolve all of the warnings in one commit, removing that power-domain-cells
>> property in patch [2/4], otherwise this one is technically a fix for that.
>>
>> Please squash [2/4] and [3/4], like that it just makes more sense.
>>
>
> That's independent thing. Previous compatible - syscfg - also did not
> allow power domains. The difference is that bindings did not print a
> warning without my change. We can reverse the patches if this is more
> suitable.
>

You're still introducing a warning with patch 2.

As for swapping the order, that could also be a solution, but I still don't see
that as an independent thing - in any case, swapping them is something I can do
while applying, eventually.




Subject: Re: [PATCH 4/4] arm64: dts: mediatek: mt8173-elm: correct PMIC's syscon reg entry

Il 18/05/24 23:11, Krzysztof Kozlowski ha scritto:
> The MT6297 PMIC has address/size cells == 1, thus its syscon child node
> has incorrect number of entries in "reg" property. Fix dtbs_check
> warning:
>
> mt8173-elm.dtb: syscon@c000: reg: [[0, 49152], [0, 264]] is too long
>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>

That node is unused and not needed at all, even...
Unless anyone has any strong opinion against, I suggest to simply remove it.

Cheers,
Angelo

> ---
> arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> index 6d962d437e02..0e2439860223 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> @@ -1138,7 +1138,7 @@ rtc: mt6397rtc {
> syscfg_pctl_pmic: syscon@c000 {
> compatible = "mediatek,mt6397-pctl-pmic-syscfg",
> "syscon";
> - reg = <0 0x0000c000 0 0x0108>;
> + reg = <0x0000c000 0x0108>;
> };
> };
> };


Subject: Re: [PATCH 2/4] arm64: dts: mediatek: mt8365: use a specific SCPSYS compatible

Il 20/05/24 12:03, Krzysztof Kozlowski ha scritto:
> On 20/05/2024 11:55, AngeloGioacchino Del Regno wrote:
>> Il 18/05/24 23:11, Krzysztof Kozlowski ha scritto:
>>> SoCs should use dedicated compatibles for each of their syscon nodes to
>>> precisely describe the block. Using an incorrect compatible does not
>>> allow to properly match/validate children of the syscon device. Replace
>>> SYSCFG compatible, which does not have children, with a new dedicated
>>> one for SCPSYS block.
>>>
>>> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>>
>> Technically, that's not a SCPSYS block, but called SYSCFG in MT8365, but the
>> meaning and the functioning is the same, so it's fine for me.
>
> So there are two syscfg blocks? With exactly the same set of registers
> or different?
>

I'm not sure about that, I don't have the MT8365 datasheet...

Adding Alexandre to the loop - I think he can clarify as he should have the
required documentation.

Cheers

2024-05-20 13:19:10

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 4/4] arm64: dts: mediatek: mt8173-elm: correct PMIC's syscon reg entry

On 20/05/2024 12:07, AngeloGioacchino Del Regno wrote:
> Il 18/05/24 23:11, Krzysztof Kozlowski ha scritto:
>> The MT6297 PMIC has address/size cells == 1, thus its syscon child node
>> has incorrect number of entries in "reg" property. Fix dtbs_check
>> warning:
>>
>> mt8173-elm.dtb: syscon@c000: reg: [[0, 49152], [0, 264]] is too long
>>
>> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>
> That node is unused and not needed at all, even...
> Unless anyone has any strong opinion against, I suggest to simply remove it.

Sure, works for me.

Best regards,
Krzysztof


2024-05-20 15:23:23

by Alexandre Mergnat

[permalink] [raw]
Subject: Re: [PATCH 2/4] arm64: dts: mediatek: mt8365: use a specific SCPSYS compatible

Hello Krzysztof,

On 20/05/2024 12:12, AngeloGioacchino Del Regno wrote:
> Il 20/05/24 12:03, Krzysztof Kozlowski ha scritto:
>> On 20/05/2024 11:55, AngeloGioacchino Del Regno wrote:
>>> Il 18/05/24 23:11, Krzysztof Kozlowski ha scritto:
>>>> SoCs should use dedicated compatibles for each of their syscon nodes to
>>>> precisely describe the block.  Using an incorrect compatible does not
>>>> allow to properly match/validate children of the syscon device.  Replace
>>>> SYSCFG compatible, which does not have children, with a new dedicated
>>>> one for SCPSYS block.
>>>>
>>>> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>>>
>>> Technically, that's not a SCPSYS block, but called SYSCFG in MT8365, but the
>>> meaning and the functioning is the same, so it's fine for me.
>>
>> So there are two syscfg blocks? With exactly the same set of registers
>> or different?
>>
>
> I'm not sure about that, I don't have the MT8365 datasheet...
>
> Adding Alexandre to the loop - I think he can clarify as he should have the
> required documentation.

Unfortunately, The SCPSYS (@10006000) isn't documented, but according to the functionnal
specification, it seems to have only one block.

I don't have the history why SYSCFG instead of SCPSYS.

I've tested your serie and have a regression at the kernel boot time:
[ 7.738117] mtk-power-controller 10006000.syscon:power-controller: Failed to create device link
(0x180) with 14000000.syscon

It's related to your patch 3/4.

--
Regards,
Alexandre

2024-05-20 21:02:32

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 1/4] dt-bindings: mfd: mediatek,mt8195-scpsys: add mediatek,mt8365-scpsys


On Sat, 18 May 2024 23:11:56 +0200, Krzysztof Kozlowski wrote:
> Add a new mediatek,mt8365-scpsys compatible, for the SCPSYS syscon block
> having power controller. Previously the DTS was re-using SYSCFG
> compatible, but that does not seem right, because SYSCFG does not have
> children.
>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
> ---
> .../devicetree/bindings/mfd/mediatek,mt8195-scpsys.yaml | 1 +
> 1 file changed, 1 insertion(+)
>

Acked-by: Rob Herring (Arm) <[email protected]>


2024-05-21 08:22:58

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 2/4] arm64: dts: mediatek: mt8365: use a specific SCPSYS compatible

On 20/05/2024 17:23, Alexandre Mergnat wrote:
> Hello Krzysztof,
>
> On 20/05/2024 12:12, AngeloGioacchino Del Regno wrote:
>> Il 20/05/24 12:03, Krzysztof Kozlowski ha scritto:
>>> On 20/05/2024 11:55, AngeloGioacchino Del Regno wrote:
>>>> Il 18/05/24 23:11, Krzysztof Kozlowski ha scritto:
>>>>> SoCs should use dedicated compatibles for each of their syscon nodes to
>>>>> precisely describe the block.  Using an incorrect compatible does not
>>>>> allow to properly match/validate children of the syscon device.  Replace
>>>>> SYSCFG compatible, which does not have children, with a new dedicated
>>>>> one for SCPSYS block.
>>>>>
>>>>> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>>>>
>>>> Technically, that's not a SCPSYS block, but called SYSCFG in MT8365, but the
>>>> meaning and the functioning is the same, so it's fine for me.
>>>
>>> So there are two syscfg blocks? With exactly the same set of registers
>>> or different?
>>>
>>
>> I'm not sure about that, I don't have the MT8365 datasheet...
>>
>> Adding Alexandre to the loop - I think he can clarify as he should have the
>> required documentation.
>
> Unfortunately, The SCPSYS (@10006000) isn't documented, but according to the functionnal
> specification, it seems to have only one block.
>
> I don't have the history why SYSCFG instead of SCPSYS.
>
> I've tested your serie and have a regression at the kernel boot time:
> [ 7.738117] mtk-power-controller 10006000.syscon:power-controller: Failed to create device link
> (0x180) with 14000000.syscon
>
> It's related to your patch 3/4.

I don't see how this could be related. The error is mentioning entirely
different node - mmsys. No driver binds to 10006000.syscon, except the
MFD syscon of course, so my change should have zero effect on drivers.

The mtk-pm-domains (so child of patch affected in 3/4) only takes regmap
from the parent, so the cells again are not related.

Just to be sure: you are testing mainline or next, without any other
patches on top except mine?

>

Best regards,
Krzysztof


2024-05-21 13:27:06

by Alexandre Mergnat

[permalink] [raw]
Subject: Re: [PATCH 2/4] arm64: dts: mediatek: mt8365: use a specific SCPSYS compatible



On 21/05/2024 10:22, Krzysztof Kozlowski wrote:
> On 20/05/2024 17:23, Alexandre Mergnat wrote:
>> Hello Krzysztof,
>>
>> On 20/05/2024 12:12, AngeloGioacchino Del Regno wrote:
>>> Il 20/05/24 12:03, Krzysztof Kozlowski ha scritto:
>>>> On 20/05/2024 11:55, AngeloGioacchino Del Regno wrote:
>>>>> Il 18/05/24 23:11, Krzysztof Kozlowski ha scritto:
>>>>>> SoCs should use dedicated compatibles for each of their syscon nodes to
>>>>>> precisely describe the block.  Using an incorrect compatible does not
>>>>>> allow to properly match/validate children of the syscon device.  Replace
>>>>>> SYSCFG compatible, which does not have children, with a new dedicated
>>>>>> one for SCPSYS block.
>>>>>>
>>>>>> Signed-off-by: Krzysztof Kozlowski<[email protected]>
>>>>> Technically, that's not a SCPSYS block, but called SYSCFG in MT8365, but the
>>>>> meaning and the functioning is the same, so it's fine for me.
>>>> So there are two syscfg blocks? With exactly the same set of registers
>>>> or different?
>>>>
>>> I'm not sure about that, I don't have the MT8365 datasheet...
>>>
>>> Adding Alexandre to the loop - I think he can clarify as he should have the
>>> required documentation.
>> Unfortunately, The SCPSYS (@10006000) isn't documented, but according to the functionnal
>> specification, it seems to have only one block.
>>
>> I don't have the history why SYSCFG instead of SCPSYS.
>>
>> I've tested your serie and have a regression at the kernel boot time:
>> [ 7.738117] mtk-power-controller 10006000.syscon:power-controller: Failed to create device link
>> (0x180) with 14000000.syscon
>>
>> It's related to your patch 3/4.
> I don't see how this could be related. The error is mentioning entirely
> different node - mmsys. No driver binds to 10006000.syscon, except the
> MFD syscon of course, so my change should have zero effect on drivers.
>
> The mtk-pm-domains (so child of patch affected in 3/4) only takes regmap
> from the parent, so the cells again are not related.
>
> Just to be sure: you are testing mainline or next, without any other
> patches on top except mine?

I've tested on next

* a018995ac19c (HEAD -> temp, me/temp) arm64: dts: mediatek: mt8173-elm: correct PMIC's syscon reg entry
* 0f118436c61c arm64: dts: mediatek: mt8365: drop incorrect power-domain-cells
* d40e424fe6dc arm64: dts: mediatek: mt8365: use a specific SCPSYS compatible
* d7caa08a4a9b dt-bindings: mfd: mediatek,mt8195-scpsys: add mediatek,mt8365-scpsys
* 82d92a9a1b9e (tag: next-20240515, linux-next/master) Add linux-next specific files for 20240515
* 77ba09d6e7cb Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git
|\
| * dedcf3a8e704 tools/power turbostat: version 2024.05.10
| * baac2f4c7f3b tools/power turbostat: Ignore pkg_cstate_limit when it is not available
| * a0525800e2dc tools/power turbostat: Fix order of strings in pkg_cstate_limit_strings
| * ffc2e3d90e6f tools/power turbostat: Read Package-cstates via perf


I did the test with and without "0f118436c61c arm64: dts: mediatek: mt8365: drop incorrect
power-domain-cells"

Without this specific patch, no regression.


--
Regards,
Alexandre

Subject: Re: [PATCH 2/4] arm64: dts: mediatek: mt8365: use a specific SCPSYS compatible

Il 21/05/24 15:26, Alexandre Mergnat ha scritto:
>
>
> On 21/05/2024 10:22, Krzysztof Kozlowski wrote:
>> On 20/05/2024 17:23, Alexandre Mergnat wrote:
>>> Hello Krzysztof,
>>>
>>> On 20/05/2024 12:12, AngeloGioacchino Del Regno wrote:
>>>> Il 20/05/24 12:03, Krzysztof Kozlowski ha scritto:
>>>>> On 20/05/2024 11:55, AngeloGioacchino Del Regno wrote:
>>>>>> Il 18/05/24 23:11, Krzysztof Kozlowski ha scritto:
>>>>>>> SoCs should use dedicated compatibles for each of their syscon nodes to
>>>>>>> precisely describe the block.  Using an incorrect compatible does not
>>>>>>> allow to properly match/validate children of the syscon device.  Replace
>>>>>>> SYSCFG compatible, which does not have children, with a new dedicated
>>>>>>> one for SCPSYS block.
>>>>>>>
>>>>>>> Signed-off-by: Krzysztof Kozlowski<[email protected]>
>>>>>> Technically, that's not a SCPSYS block, but called SYSCFG in MT8365, but the
>>>>>> meaning and the functioning is the same, so it's fine for me.
>>>>> So there are two syscfg blocks? With exactly the same set of registers
>>>>> or different?
>>>>>
>>>> I'm not sure about that, I don't have the MT8365 datasheet...
>>>>
>>>> Adding Alexandre to the loop - I think he can clarify as he should have the
>>>> required documentation.
>>> Unfortunately, The SCPSYS (@10006000) isn't documented, but according to the
>>> functionnal
>>> specification, it seems to have only one block.
>>>
>>> I don't have the history why SYSCFG instead of SCPSYS.
>>>
>>> I've tested your serie and have a regression at the kernel boot time:
>>> [    7.738117] mtk-power-controller 10006000.syscon:power-controller: Failed to
>>> create device link
>>> (0x180) with 14000000.syscon
>>>
>>> It's related to your patch 3/4.
>> I don't see how this could be related. The error is mentioning entirely
>> different node - mmsys. No driver binds to 10006000.syscon, except the
>> MFD syscon of course, so my change should have zero effect on drivers.
>>
>> The mtk-pm-domains (so child of patch affected in 3/4) only takes regmap
>> from the parent, so the cells again are not related.
>>
>> Just to be sure: you are testing mainline or next, without any other
>> patches on top except mine?
>
> I've tested on next
>
> * a018995ac19c (HEAD -> temp, me/temp) arm64: dts: mediatek: mt8173-elm: correct
> PMIC's syscon reg entry
> * 0f118436c61c arm64: dts: mediatek: mt8365: drop incorrect power-domain-cells
> * d40e424fe6dc arm64: dts: mediatek: mt8365: use a specific SCPSYS compatible
> * d7caa08a4a9b dt-bindings: mfd: mediatek,mt8195-scpsys: add mediatek,mt8365-scpsys
> * 82d92a9a1b9e (tag: next-20240515, linux-next/master) Add linux-next specific
> files for 20240515
> *   77ba09d6e7cb Merge branch 'next' of
> git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git
> |\
> | * dedcf3a8e704 tools/power turbostat: version 2024.05.10
> | * baac2f4c7f3b tools/power turbostat: Ignore pkg_cstate_limit when it is not
> available
> | * a0525800e2dc tools/power turbostat: Fix order of strings in
> pkg_cstate_limit_strings
> | * ffc2e3d90e6f tools/power turbostat: Read Package-cstates via perf
>
>
> I did the test with and without "0f118436c61c arm64: dts: mediatek: mt8365: drop
> incorrect power-domain-cells"
>
> Without this specific patch, no regression.
>
>

Honestly, that makes very little sense to me - that property is useless and it's
like it's never been there... at least, no MTK driver is parsing that and there's
definitely no power domain in the top node (a child does, but not the parent).

Is this a flaky result? Did you actually try to reboot multiple times to check if
the platform is *really broken* after that commit?

Sorry, it's not mistrust or anything, but I've been in this situation multiple
times in the past, usually always on linux-next (because it's constantly broken :P)

Cheers
Angelo

2024-05-21 17:28:33

by Alexandre Mergnat

[permalink] [raw]
Subject: Re: [PATCH 2/4] arm64: dts: mediatek: mt8365: use a specific SCPSYS compatible



On 21/05/2024 16:13, AngeloGioacchino Del Regno wrote:
> Il 21/05/24 15:26, Alexandre Mergnat ha scritto:
>>
>>
>> On 21/05/2024 10:22, Krzysztof Kozlowski wrote:
>>> On 20/05/2024 17:23, Alexandre Mergnat wrote:
>>>> Hello Krzysztof,
>>>>
>>>> On 20/05/2024 12:12, AngeloGioacchino Del Regno wrote:
>>>>> Il 20/05/24 12:03, Krzysztof Kozlowski ha scritto:
>>>>>> On 20/05/2024 11:55, AngeloGioacchino Del Regno wrote:
>>>>>>> Il 18/05/24 23:11, Krzysztof Kozlowski ha scritto:
>>>>>>>> SoCs should use dedicated compatibles for each of their syscon nodes to
>>>>>>>> precisely describe the block.  Using an incorrect compatible does not
>>>>>>>> allow to properly match/validate children of the syscon device.  Replace
>>>>>>>> SYSCFG compatible, which does not have children, with a new dedicated
>>>>>>>> one for SCPSYS block.
>>>>>>>>
>>>>>>>> Signed-off-by: Krzysztof Kozlowski<[email protected]>
>>>>>>> Technically, that's not a SCPSYS block, but called SYSCFG in MT8365, but the
>>>>>>> meaning and the functioning is the same, so it's fine for me.
>>>>>> So there are two syscfg blocks? With exactly the same set of registers
>>>>>> or different?
>>>>>>
>>>>> I'm not sure about that, I don't have the MT8365 datasheet...
>>>>>
>>>>> Adding Alexandre to the loop - I think he can clarify as he should have the
>>>>> required documentation.
>>>> Unfortunately, The SCPSYS (@10006000) isn't documented, but according to the functionnal
>>>> specification, it seems to have only one block.
>>>>
>>>> I don't have the history why SYSCFG instead of SCPSYS.
>>>>
>>>> I've tested your serie and have a regression at the kernel boot time:
>>>> [    7.738117] mtk-power-controller 10006000.syscon:power-controller: Failed to create device link
>>>> (0x180) with 14000000.syscon
>>>>
>>>> It's related to your patch 3/4.
>>> I don't see how this could be related. The error is mentioning entirely
>>> different node - mmsys. No driver binds to 10006000.syscon, except the
>>> MFD syscon of course, so my change should have zero effect on drivers.
>>>
>>> The mtk-pm-domains (so child of patch affected in 3/4) only takes regmap
>>> from the parent, so the cells again are not related.
>>>
>>> Just to be sure: you are testing mainline or next, without any other
>>> patches on top except mine?
>>
>> I've tested on next
>>
>> * a018995ac19c (HEAD -> temp, me/temp) arm64: dts: mediatek: mt8173-elm: correct PMIC's syscon reg
>> entry
>> * 0f118436c61c arm64: dts: mediatek: mt8365: drop incorrect power-domain-cells
>> * d40e424fe6dc arm64: dts: mediatek: mt8365: use a specific SCPSYS compatible
>> * d7caa08a4a9b dt-bindings: mfd: mediatek,mt8195-scpsys: add mediatek,mt8365-scpsys
>> * 82d92a9a1b9e (tag: next-20240515, linux-next/master) Add linux-next specific files for 20240515
>> *   77ba09d6e7cb Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git
>> |\
>> | * dedcf3a8e704 tools/power turbostat: version 2024.05.10
>> | * baac2f4c7f3b tools/power turbostat: Ignore pkg_cstate_limit when it is not available
>> | * a0525800e2dc tools/power turbostat: Fix order of strings in pkg_cstate_limit_strings
>> | * ffc2e3d90e6f tools/power turbostat: Read Package-cstates via perf
>>
>>
>> I did the test with and without "0f118436c61c arm64: dts: mediatek: mt8365: drop incorrect
>> power-domain-cells"
>>
>> Without this specific patch, no regression.
>>
>>
>
> Honestly, that makes very little sense to me - that property is useless and it's
> like it's never been there... at least, no MTK driver is parsing that and there's
> definitely no power domain in the top node (a child does, but not the parent).
>
> Is this a flaky result? Did you actually try to reboot multiple times to check if
> the platform is *really broken* after that commit?
>
> Sorry, it's not mistrust or anything, but I've been in this situation multiple
> times in the past, usually always on linux-next (because it's constantly broken :P)

MMMmm you're right, I can't reproduce this time...
Sorry for the noise.

Reviewed-by: Alexandre Mergnat <[email protected]>
Tested-by: Alexandre Mergnat <[email protected]>

--
Regards,
Alexandre

2024-05-21 17:36:33

by Alexandre Mergnat

[permalink] [raw]
Subject: Re: [PATCH 1/4] dt-bindings: mfd: mediatek,mt8195-scpsys: add mediatek,mt8365-scpsys

Reviewed-by: Alexandre Mergnat <[email protected]>

On 18/05/2024 23:11, Krzysztof Kozlowski wrote:
> Add a new mediatek,mt8365-scpsys compatible, for the SCPSYS syscon block
> having power controller. Previously the DTS was re-using SYSCFG
> compatible, but that does not seem right, because SYSCFG does not have
> children.

--
Regards,
Alexandre

2024-05-31 14:20:14

by Lee Jones

[permalink] [raw]
Subject: Re: (subset) [PATCH 1/4] dt-bindings: mfd: mediatek,mt8195-scpsys: add mediatek,mt8365-scpsys

On Sat, 18 May 2024 23:11:56 +0200, Krzysztof Kozlowski wrote:
> Add a new mediatek,mt8365-scpsys compatible, for the SCPSYS syscon block
> having power controller. Previously the DTS was re-using SYSCFG
> compatible, but that does not seem right, because SYSCFG does not have
> children.
>
>

Applied, thanks!

[1/4] dt-bindings: mfd: mediatek,mt8195-scpsys: add mediatek,mt8365-scpsys
commit: 50c779be27066b4e25873a15580999008d068ff7

--
Lee Jones [李琼斯]