| Message ID | 20240530083513.4135052-6-wenst@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | powervr: MT8173 GPU support | expand |
Il 30/05/24 10:35, Chen-Yu Tsai ha scritto: > The MFG_ASYNC domain, which is likely associated to the whole MFG block, > currently specifies clk26m as its domain clock. This is bogus, since the > clock is an external crystal with no controls. Also, the MFG block has > a independent CLK_TOP_AXI_MFG_IN_SEL clock, which according to the block > diagram, gates access to the hardware registers. Having this one as the > domain clock makes much more sense. This also fixes access to the MFGTOP > registers. > > Change the MFG_ASYNC domain clock to CLK_TOP_AXI_MFG_IN_SEL. > > Fixes: 8b6562644df9 ("arm64: dts: mediatek: Add mt8173 power domain controller") > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> Just one question... what happens if there's no GPU support at all and this power domain gets powered off? I expect the answer to be "nothing", so I'm preventively giving you my Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> ....but if I'm wrong and the answer isn't exactly "nothing", then I still agree with this commit, but only after removing the Fixes tag. Cheers, Angelo > --- > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi > index 3458be7f7f61..136b28f80cc2 100644 > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi > @@ -497,7 +497,7 @@ power-domain@MT8173_POWER_DOMAIN_USB { > }; > mfg_async: power-domain@MT8173_POWER_DOMAIN_MFG_ASYNC { > reg = <MT8173_POWER_DOMAIN_MFG_ASYNC>; > - clocks = <&clk26m>; > + clocks = <&topckgen CLK_TOP_AXI_MFG_IN_SEL>; > clock-names = "mfg"; > #address-cells = <1>; > #size-cells = <0>;
On Thu, May 30, 2024 at 6:03 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 30/05/24 10:35, Chen-Yu Tsai ha scritto: > > The MFG_ASYNC domain, which is likely associated to the whole MFG block, > > currently specifies clk26m as its domain clock. This is bogus, since the > > clock is an external crystal with no controls. Also, the MFG block has > > a independent CLK_TOP_AXI_MFG_IN_SEL clock, which according to the block > > diagram, gates access to the hardware registers. Having this one as the > > domain clock makes much more sense. This also fixes access to the MFGTOP > > registers. > > > > Change the MFG_ASYNC domain clock to CLK_TOP_AXI_MFG_IN_SEL. > > > > Fixes: 8b6562644df9 ("arm64: dts: mediatek: Add mt8173 power domain controller") > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > > Just one question... what happens if there's no GPU support at all and this > power domain gets powered off? > > I expect the answer to be "nothing", so I'm preventively giving you my Well it's powered off by default. Just double checked, and without the final patch: # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary domain status children performance /device runtime status ---------------------------------------------------------------------------------------------- mfg off-0 0 mfg_2d off-0 0 mfg mfg_async off-0 0 mfg_2d And with the last patch but with the powervr removed: # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary domain status children performance /device runtime status ---------------------------------------------------------------------------------------------- mfg_apm off-0 0 mfg off-0 0 mfg_apm /devices/platform/soc/13fff000.clock-controller suspended 0 mfg_2d off-0 0 mfg mfg_async off-0 0 mfg_2d Things seem to work OK. I can SSH in, and the framebuffer console on the screen works fine. Note that accessing the regmap through debugfs doesn't do much good. regmap doesn't handle runtime PM. And the syscon regmap isn't even tied to a struct device. Dumping the regmap through debugfs while the power domain is off gives all zeroes, likely due to bus isolation. > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Thanks! ChenYu > ....but if I'm wrong and the answer isn't exactly "nothing", then I still agree > with this commit, but only after removing the Fixes tag. > > Cheers, > Angelo > > > --- > > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi > > index 3458be7f7f61..136b28f80cc2 100644 > > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi > > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi > > @@ -497,7 +497,7 @@ power-domain@MT8173_POWER_DOMAIN_USB { > > }; > > mfg_async: power-domain@MT8173_POWER_DOMAIN_MFG_ASYNC { > > reg = <MT8173_POWER_DOMAIN_MFG_ASYNC>; > > - clocks = <&clk26m>; > > + clocks = <&topckgen CLK_TOP_AXI_MFG_IN_SEL>; > > clock-names = "mfg"; > > #address-cells = <1>; > > #size-cells = <0>; > >
Il 05/06/24 10:25, Chen-Yu Tsai ha scritto: > On Thu, May 30, 2024 at 6:03 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@collabora.com> wrote: >> >> Il 30/05/24 10:35, Chen-Yu Tsai ha scritto: >>> The MFG_ASYNC domain, which is likely associated to the whole MFG block, >>> currently specifies clk26m as its domain clock. This is bogus, since the >>> clock is an external crystal with no controls. Also, the MFG block has >>> a independent CLK_TOP_AXI_MFG_IN_SEL clock, which according to the block >>> diagram, gates access to the hardware registers. Having this one as the >>> domain clock makes much more sense. This also fixes access to the MFGTOP >>> registers. >>> >>> Change the MFG_ASYNC domain clock to CLK_TOP_AXI_MFG_IN_SEL. >>> >>> Fixes: 8b6562644df9 ("arm64: dts: mediatek: Add mt8173 power domain controller") >>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> >> >> Just one question... what happens if there's no GPU support at all and this >> power domain gets powered off? >> >> I expect the answer to be "nothing", so I'm preventively giving you my > > Well it's powered off by default. Just double checked, and without the final > patch: > > # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary > domain status children > performance > /device runtime status > ---------------------------------------------------------------------------------------------- > mfg off-0 > 0 > mfg_2d off-0 > 0 > mfg > mfg_async off-0 > 0 > mfg_2d > > And with the last patch but with the powervr removed: > > # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary > domain status children > performance > /device runtime status > ---------------------------------------------------------------------------------------------- > mfg_apm off-0 > 0 > mfg off-0 > 0 > mfg_apm > /devices/platform/soc/13fff000.clock-controller suspended > 0 > mfg_2d off-0 > 0 > mfg > mfg_async off-0 > 0 > mfg_2d > > Things seem to work OK. I can SSH in, and the framebuffer console on the screen > works fine. > > > Note that accessing the regmap through debugfs doesn't do much good. regmap > doesn't handle runtime PM. And the syscon regmap isn't even tied to a > struct device. Dumping the regmap through debugfs while the power domain > is off gives all zeroes, likely due to bus isolation. > The last part where you say "gives all zeroes" is actually the best outcome that I could have ever expected. So, well, many thanks for this very nice analysis and test. >> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> I confirm my green light. It's beautiful when this kind of patches come upstream especially with your replies actually removing any kind of possible doubt. > > Thanks! Thank *you* for caring about this old platform! Cheers, Angelo > > ChenYu > >> ....but if I'm wrong and the answer isn't exactly "nothing", then I still agree >> with this commit, but only after removing the Fixes tag. >> >> Cheers, >> Angelo >> >>> --- >>> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi >>> index 3458be7f7f61..136b28f80cc2 100644 >>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi >>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi >>> @@ -497,7 +497,7 @@ power-domain@MT8173_POWER_DOMAIN_USB { >>> }; >>> mfg_async: power-domain@MT8173_POWER_DOMAIN_MFG_ASYNC { >>> reg = <MT8173_POWER_DOMAIN_MFG_ASYNC>; >>> - clocks = <&clk26m>; >>> + clocks = <&topckgen CLK_TOP_AXI_MFG_IN_SEL>; >>> clock-names = "mfg"; >>> #address-cells = <1>; >>> #size-cells = <0>; >> >>
On Wed, Jun 5, 2024 at 7:25 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 05/06/24 10:25, Chen-Yu Tsai ha scritto: > > On Thu, May 30, 2024 at 6:03 PM AngeloGioacchino Del Regno > > <angelogioacchino.delregno@collabora.com> wrote: > >> > >> Il 30/05/24 10:35, Chen-Yu Tsai ha scritto: > >>> The MFG_ASYNC domain, which is likely associated to the whole MFG block, > >>> currently specifies clk26m as its domain clock. This is bogus, since the > >>> clock is an external crystal with no controls. Also, the MFG block has > >>> a independent CLK_TOP_AXI_MFG_IN_SEL clock, which according to the block > >>> diagram, gates access to the hardware registers. Having this one as the > >>> domain clock makes much more sense. This also fixes access to the MFGTOP > >>> registers. > >>> > >>> Change the MFG_ASYNC domain clock to CLK_TOP_AXI_MFG_IN_SEL. > >>> > >>> Fixes: 8b6562644df9 ("arm64: dts: mediatek: Add mt8173 power domain controller") > >>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > >> > >> Just one question... what happens if there's no GPU support at all and this > >> power domain gets powered off? > >> > >> I expect the answer to be "nothing", so I'm preventively giving you my > > > > Well it's powered off by default. Just double checked, and without the final > > patch: > > > > # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary > > domain status children > > performance > > /device runtime status > > ---------------------------------------------------------------------------------------------- > > mfg off-0 > > 0 > > mfg_2d off-0 > > 0 > > mfg > > mfg_async off-0 > > 0 > > mfg_2d > > > > And with the last patch but with the powervr removed: > > > > # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary > > domain status children > > performance > > /device runtime status > > ---------------------------------------------------------------------------------------------- > > mfg_apm off-0 > > 0 > > mfg off-0 > > 0 > > mfg_apm > > /devices/platform/soc/13fff000.clock-controller suspended > > 0 > > mfg_2d off-0 > > 0 > > mfg > > mfg_async off-0 > > 0 > > mfg_2d > > > > Things seem to work OK. I can SSH in, and the framebuffer console on the screen > > works fine. > > > > > > Note that accessing the regmap through debugfs doesn't do much good. regmap > > doesn't handle runtime PM. And the syscon regmap isn't even tied to a > > struct device. Dumping the regmap through debugfs while the power domain > > is off gives all zeroes, likely due to bus isolation. > > > > The last part where you say "gives all zeroes" is actually the best outcome that > I could have ever expected. > > So, well, many thanks for this very nice analysis and test. > > >> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > I confirm my green light. It's beautiful when this kind of patches come upstream > especially with your replies actually removing any kind of possible doubt. > > > > > Thanks! > > Thank *you* for caring about this old platform! Can you pick up this patch first? Frank mentioned that the GPU core takes two power domains. I asked MediaTek for more information but I don't know how long that will take. ChenYu > Cheers, > Angelo > > > > > ChenYu > > > >> ....but if I'm wrong and the answer isn't exactly "nothing", then I still agree > >> with this commit, but only after removing the Fixes tag. > >> > >> Cheers, > >> Angelo > >> > >>> --- > >>> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi > >>> index 3458be7f7f61..136b28f80cc2 100644 > >>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi > >>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi > >>> @@ -497,7 +497,7 @@ power-domain@MT8173_POWER_DOMAIN_USB { > >>> }; > >>> mfg_async: power-domain@MT8173_POWER_DOMAIN_MFG_ASYNC { > >>> reg = <MT8173_POWER_DOMAIN_MFG_ASYNC>; > >>> - clocks = <&clk26m>; > >>> + clocks = <&topckgen CLK_TOP_AXI_MFG_IN_SEL>; > >>> clock-names = "mfg"; > >>> #address-cells = <1>; > >>> #size-cells = <0>; > >> > >> >
diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi index 3458be7f7f61..136b28f80cc2 100644 --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi @@ -497,7 +497,7 @@ power-domain@MT8173_POWER_DOMAIN_USB { }; mfg_async: power-domain@MT8173_POWER_DOMAIN_MFG_ASYNC { reg = <MT8173_POWER_DOMAIN_MFG_ASYNC>; - clocks = <&clk26m>; + clocks = <&topckgen CLK_TOP_AXI_MFG_IN_SEL>; clock-names = "mfg"; #address-cells = <1>; #size-cells = <0>;
The MFG_ASYNC domain, which is likely associated to the whole MFG block, currently specifies clk26m as its domain clock. This is bogus, since the clock is an external crystal with no controls. Also, the MFG block has a independent CLK_TOP_AXI_MFG_IN_SEL clock, which according to the block diagram, gates access to the hardware registers. Having this one as the domain clock makes much more sense. This also fixes access to the MFGTOP registers. Change the MFG_ASYNC domain clock to CLK_TOP_AXI_MFG_IN_SEL. Fixes: 8b6562644df9 ("arm64: dts: mediatek: Add mt8173 power domain controller") Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> --- arch/arm64/boot/dts/mediatek/mt8173.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)