diff mbox series

[5/6] arm64: dts: mediatek: mt8173: Fix MFG_ASYNC power domain clock

Message ID 20240530083513.4135052-6-wenst@chromium.org (mailing list archive)
State New
Headers show
Series powervr: MT8173 GPU support | expand

Commit Message

Chen-Yu Tsai May 30, 2024, 8:35 a.m. UTC
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(-)

Comments

AngeloGioacchino Del Regno May 30, 2024, 10:03 a.m. UTC | #1
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>;
Chen-Yu Tsai June 5, 2024, 8:25 a.m. UTC | #2
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>;
>
>
AngeloGioacchino Del Regno June 5, 2024, 11:25 a.m. UTC | #3
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>;
>>
>>
Chen-Yu Tsai June 6, 2024, 3:28 a.m. UTC | #4
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 mbox series

Patch

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>;