| Message ID | 20240518211159.142920-2-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] dt-bindings: mfd: mediatek,mt8195-scpsys: add mediatek,mt8365-scpsys | expand |
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 <krzysztof.kozlowski@linaro.org> 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 <angelogioacchino.delregno@collabora.com>
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 <krzysztof.kozlowski@linaro.org> > > 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
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 <krzysztof.kozlowski@linaro.org> >> >> 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
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 <krzysztof.kozlowski@linaro.org> >>> >>> 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.
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 <krzysztof.kozlowski@linaro.org> >>>> >>>> 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
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<krzysztof.kozlowski@linaro.org> >>>>> 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.
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<krzysztof.kozlowski@linaro.org> >>>>>> 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
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<krzysztof.kozlowski@linaro.org> >>>>>>> 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 <amergnat@baylibre.com> Tested-by: Alexandre Mergnat <amergnat@baylibre.com>
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>;
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 <krzysztof.kozlowski@linaro.org> --- arch/arm64/boot/dts/mediatek/mt8365.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)