[PATCH] arm64: dts: mt8192: Add adsp power domain controller

[PATCH] arm64: dts: mt8192: Add adsp power domain controller

[Allen-KH Cheng]

Add adsp power domain controller node for mt8192 SoC.

Signed-off-by: Allen-KH Cheng <allen-kh.cheng@mediatek.com>
---
Ref: https://lore.kernel.org/all/2ec80bd8-dfef-d2e6-eb41-6e6088043e33@collabora.com/
    [Allen-KH Cheng <allen-kh.cheng@mediatek.com>]
---
---
 arch/arm64/boot/dts/mediatek/mt8192.dtsi | 8 ++++++++
 include/dt-bindings/power/mt8192-power.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
index 424fc89cc6f7..e71afba871fc 100644
--- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
@@ -514,6 +514,14 @@
 						};
 					};
 				};
+
+				power-domain@MT8192_POWER_DOMAIN_ADSP {
+					reg = <MT8192_POWER_DOMAIN_ADSP>;
+					clocks = <&topckgen CLK_TOP_ADSP_SEL>;
+					clock-names = "adsp";
+					mediatek,infracfg = <&infracfg>;
+					#power-domain-cells = <0>;
+				};
 			};
 		};
 
diff --git a/include/dt-bindings/power/mt8192-power.h b/include/dt-bindings/power/mt8192-power.h
index 4eaa53d7270a..63e81cd0d06d 100644
--- a/include/dt-bindings/power/mt8192-power.h
+++ b/include/dt-bindings/power/mt8192-power.h
@@ -28,5 +28,6 @@
 #define MT8192_POWER_DOMAIN_CAM_RAWA	18
 #define MT8192_POWER_DOMAIN_CAM_RAWB	19
 #define MT8192_POWER_DOMAIN_CAM_RAWC	20
+#define MT8192_POWER_DOMAIN_ADSP	21
 
 #endif /* _DT_BINDINGS_POWER_MT8192_POWER_H */
-- 
2.18.0

Re: [PATCH] arm64: dts: mt8192: Add adsp power domain controller

[Chen-Yu Tsai]

On Thu, Dec 1, 2022 at 3:33 PM Allen-KH Cheng
<allen-kh.cheng@mediatek.com> wrote:
>
> Add adsp power domain controller node for mt8192 SoC.
>
> Signed-off-by: Allen-KH Cheng <allen-kh.cheng@mediatek.com>
> ---
> Ref: https://lore.kernel.org/all/2ec80bd8-dfef-d2e6-eb41-6e6088043e33@collabora.com/
>     [Allen-KH Cheng <allen-kh.cheng@mediatek.com>]
> ---
> ---
>  arch/arm64/boot/dts/mediatek/mt8192.dtsi | 8 ++++++++
>  include/dt-bindings/power/mt8192-power.h | 1 +
>  2 files changed, 9 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> index 424fc89cc6f7..e71afba871fc 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> @@ -514,6 +514,14 @@
>                                                 };
>                                         };
>                                 };
> +
> +                               power-domain@MT8192_POWER_DOMAIN_ADSP {
> +                                       reg = <MT8192_POWER_DOMAIN_ADSP>;
> +                                       clocks = <&topckgen CLK_TOP_ADSP_SEL>;
> +                                       clock-names = "adsp";
> +                                       mediatek,infracfg = <&infracfg>;
> +                                       #power-domain-cells = <0>;
> +                               };
>                         };
>                 };
>

Please also tie the new power domain to the SCP_ADSP clock.

ChenYu

> diff --git a/include/dt-bindings/power/mt8192-power.h b/include/dt-bindings/power/mt8192-power.h
> index 4eaa53d7270a..63e81cd0d06d 100644
> --- a/include/dt-bindings/power/mt8192-power.h
> +++ b/include/dt-bindings/power/mt8192-power.h
> @@ -28,5 +28,6 @@
>  #define MT8192_POWER_DOMAIN_CAM_RAWA   18
>  #define MT8192_POWER_DOMAIN_CAM_RAWB   19
>  #define MT8192_POWER_DOMAIN_CAM_RAWC   20
> +#define MT8192_POWER_DOMAIN_ADSP       21
>
>  #endif /* _DT_BINDINGS_POWER_MT8192_POWER_H */
> --
> 2.18.0
>

Re: [PATCH] arm64: dts: mt8192: Add adsp power domain controller

[AngeloGioacchino Del Regno]

Il 01/12/22 08:33, Allen-KH Cheng ha scritto:
> Add adsp power domain controller node for mt8192 SoC.
> 
> Signed-off-by: Allen-KH Cheng <allen-kh.cheng@mediatek.com>
> ---
> Ref: https://lore.kernel.org/all/2ec80bd8-dfef-d2e6-eb41-6e6088043e33@collabora.com/
>      [Allen-KH Cheng <allen-kh.cheng@mediatek.com>]
> ---
> ---
>   arch/arm64/boot/dts/mediatek/mt8192.dtsi | 8 ++++++++
>   include/dt-bindings/power/mt8192-power.h | 1 +
>   2 files changed, 9 insertions(+)
> 

Allen, thanks for this one, but it's incomplete...

First of all, you must add the power domain on the driver itself, specifically,
in drivers/soc/mediatek/mt8192-pm-domains.h - otherwise this change will have no
effect!

...Then, as Chen-Yu said, you should also add the power domain to the scp_adsp
clock node as that's solving the lockup issue...

.......and last, but not least: we need a Fixes tag to backport this fix, here
and on the commit that adds the missing power domain in the driver.

Thanks,
Angelo

> diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> index 424fc89cc6f7..e71afba871fc 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> @@ -514,6 +514,14 @@
>   						};
>   					};
>   				};
> +
> +				power-domain@MT8192_POWER_DOMAIN_ADSP {
> +					reg = <MT8192_POWER_DOMAIN_ADSP>;
> +					clocks = <&topckgen CLK_TOP_ADSP_SEL>;
> +					clock-names = "adsp";
> +					mediatek,infracfg = <&infracfg>;
> +					#power-domain-cells = <0>;
> +				};
>   			};
>   		};
>   
> diff --git a/include/dt-bindings/power/mt8192-power.h b/include/dt-bindings/power/mt8192-power.h
> index 4eaa53d7270a..63e81cd0d06d 100644
> --- a/include/dt-bindings/power/mt8192-power.h
> +++ b/include/dt-bindings/power/mt8192-power.h
> @@ -28,5 +28,6 @@
>   #define MT8192_POWER_DOMAIN_CAM_RAWA	18
>   #define MT8192_POWER_DOMAIN_CAM_RAWB	19
>   #define MT8192_POWER_DOMAIN_CAM_RAWC	20
> +#define MT8192_POWER_DOMAIN_ADSP	21
>   
>   #endif /* _DT_BINDINGS_POWER_MT8192_POWER_H */
> 

Re: [PATCH] arm64: dts: mt8192: Add adsp power domain controller

[Chen-Yu Tsai]

On Thu, Dec 1, 2022 at 5:07 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 01/12/22 08:33, Allen-KH Cheng ha scritto:
> > Add adsp power domain controller node for mt8192 SoC.
> >
> > Signed-off-by: Allen-KH Cheng <allen-kh.cheng@mediatek.com>
> > ---
> > Ref: https://lore.kernel.org/all/2ec80bd8-dfef-d2e6-eb41-6e6088043e33@collabora.com/
> >      [Allen-KH Cheng <allen-kh.cheng@mediatek.com>]
> > ---
> > ---
> >   arch/arm64/boot/dts/mediatek/mt8192.dtsi | 8 ++++++++
> >   include/dt-bindings/power/mt8192-power.h | 1 +
> >   2 files changed, 9 insertions(+)
> >
>
> Allen, thanks for this one, but it's incomplete...
>
> First of all, you must add the power domain on the driver itself, specifically,
> in drivers/soc/mediatek/mt8192-pm-domains.h - otherwise this change will have no
> effect!

Actually it's worse. The driver doesn't know about the new power domain,
and so it will fail to probe. We might need to make the power domain driver
fail gracefully and skip unknown power domains.

ChenYu

> ...Then, as Chen-Yu said, you should also add the power domain to the scp_adsp
> clock node as that's solving the lockup issue...
>
> .......and last, but not least: we need a Fixes tag to backport this fix, here
> and on the commit that adds the missing power domain in the driver.
>
> Thanks,
> Angelo
>
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > index 424fc89cc6f7..e71afba871fc 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > @@ -514,6 +514,14 @@
> >                                               };
> >                                       };
> >                               };
> > +
> > +                             power-domain@MT8192_POWER_DOMAIN_ADSP {
> > +                                     reg = <MT8192_POWER_DOMAIN_ADSP>;
> > +                                     clocks = <&topckgen CLK_TOP_ADSP_SEL>;
> > +                                     clock-names = "adsp";
> > +                                     mediatek,infracfg = <&infracfg>;
> > +                                     #power-domain-cells = <0>;
> > +                             };
> >                       };
> >               };
> >
> > diff --git a/include/dt-bindings/power/mt8192-power.h b/include/dt-bindings/power/mt8192-power.h
> > index 4eaa53d7270a..63e81cd0d06d 100644
> > --- a/include/dt-bindings/power/mt8192-power.h
> > +++ b/include/dt-bindings/power/mt8192-power.h
> > @@ -28,5 +28,6 @@
> >   #define MT8192_POWER_DOMAIN_CAM_RAWA        18
> >   #define MT8192_POWER_DOMAIN_CAM_RAWB        19
> >   #define MT8192_POWER_DOMAIN_CAM_RAWC        20
> > +#define MT8192_POWER_DOMAIN_ADSP     21
> >
> >   #endif /* _DT_BINDINGS_POWER_MT8192_POWER_H */
> >
>

Re: [PATCH] arm64: dts: mt8192: Add adsp power domain controller

[AngeloGioacchino Del Regno]

Il 01/12/22 10:10, Chen-Yu Tsai ha scritto:
> On Thu, Dec 1, 2022 at 5:07 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 01/12/22 08:33, Allen-KH Cheng ha scritto:
>>> Add adsp power domain controller node for mt8192 SoC.
>>>
>>> Signed-off-by: Allen-KH Cheng <allen-kh.cheng@mediatek.com>
>>> ---
>>> Ref: https://lore.kernel.org/all/2ec80bd8-dfef-d2e6-eb41-6e6088043e33@collabora.com/
>>>       [Allen-KH Cheng <allen-kh.cheng@mediatek.com>]
>>> ---
>>> ---
>>>    arch/arm64/boot/dts/mediatek/mt8192.dtsi | 8 ++++++++
>>>    include/dt-bindings/power/mt8192-power.h | 1 +
>>>    2 files changed, 9 insertions(+)
>>>
>>
>> Allen, thanks for this one, but it's incomplete...
>>
>> First of all, you must add the power domain on the driver itself, specifically,
>> in drivers/soc/mediatek/mt8192-pm-domains.h - otherwise this change will have no
>> effect!
> 
> Actually it's worse. The driver doesn't know about the new power domain,
> and so it will fail to probe. We might need to make the power domain driver
> fail gracefully and skip unknown power domains.
> 

Right. It's worse. I don't know, though, if gracefully skipping unknown power
domains in the driver would be a good decision... as sometimes error messages
go unnoticed.

When the platform "explodes" instead, you're forced to read that log carefully
and get it working again... Anyway, I'm only thinking out loud, nothing less and
nothing more than that :-)

By the way, we can probably expand on that topic a bit later, as it's outside of
the scope of this specific change.

Back to topic, if we get one series containing both changes (devicetree, bindings
and driver) with the right Fixes tags and/or Cc stable, we shouldn't have such
issue on backports so we can probably ignore that potential issue, I think? :-)

Cheers,
Angelo

> ChenYu
> 
>> ...Then, as Chen-Yu said, you should also add the power domain to the scp_adsp
>> clock node as that's solving the lockup issue...
>>
>> .......and last, but not least: we need a Fixes tag to backport this fix, here
>> and on the commit that adds the missing power domain in the driver.
>>
>> Thanks,
>> Angelo
>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
>>> index 424fc89cc6f7..e71afba871fc 100644
>>> --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
>>> @@ -514,6 +514,14 @@
>>>                                                };
>>>                                        };
>>>                                };
>>> +
>>> +                             power-domain@MT8192_POWER_DOMAIN_ADSP {
>>> +                                     reg = <MT8192_POWER_DOMAIN_ADSP>;
>>> +                                     clocks = <&topckgen CLK_TOP_ADSP_SEL>;
>>> +                                     clock-names = "adsp";
>>> +                                     mediatek,infracfg = <&infracfg>;
>>> +                                     #power-domain-cells = <0>;
>>> +                             };
>>>                        };
>>>                };
>>>
>>> diff --git a/include/dt-bindings/power/mt8192-power.h b/include/dt-bindings/power/mt8192-power.h
>>> index 4eaa53d7270a..63e81cd0d06d 100644
>>> --- a/include/dt-bindings/power/mt8192-power.h
>>> +++ b/include/dt-bindings/power/mt8192-power.h
>>> @@ -28,5 +28,6 @@
>>>    #define MT8192_POWER_DOMAIN_CAM_RAWA        18
>>>    #define MT8192_POWER_DOMAIN_CAM_RAWB        19
>>>    #define MT8192_POWER_DOMAIN_CAM_RAWC        20
>>> +#define MT8192_POWER_DOMAIN_ADSP     21
>>>
>>>    #endif /* _DT_BINDINGS_POWER_MT8192_POWER_H */
>>>
>>

Re: [PATCH] arm64: dts: mt8192: Add adsp power domain controller

[Chen-Yu Tsai]

On Thu, Dec 1, 2022 at 5:39 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 01/12/22 10:10, Chen-Yu Tsai ha scritto:
> > On Thu, Dec 1, 2022 at 5:07 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com> wrote:
> >>
> >> Il 01/12/22 08:33, Allen-KH Cheng ha scritto:
> >>> Add adsp power domain controller node for mt8192 SoC.
> >>>
> >>> Signed-off-by: Allen-KH Cheng <allen-kh.cheng@mediatek.com>
> >>> ---
> >>> Ref: https://lore.kernel.org/all/2ec80bd8-dfef-d2e6-eb41-6e6088043e33@collabora.com/
> >>>       [Allen-KH Cheng <allen-kh.cheng@mediatek.com>]
> >>> ---
> >>> ---
> >>>    arch/arm64/boot/dts/mediatek/mt8192.dtsi | 8 ++++++++
> >>>    include/dt-bindings/power/mt8192-power.h | 1 +
> >>>    2 files changed, 9 insertions(+)
> >>>
> >>
> >> Allen, thanks for this one, but it's incomplete...
> >>
> >> First of all, you must add the power domain on the driver itself, specifically,
> >> in drivers/soc/mediatek/mt8192-pm-domains.h - otherwise this change will have no
> >> effect!
> >
> > Actually it's worse. The driver doesn't know about the new power domain,
> > and so it will fail to probe. We might need to make the power domain driver
> > fail gracefully and skip unknown power domains.
> >
>
> Right. It's worse. I don't know, though, if gracefully skipping unknown power
> domains in the driver would be a good decision... as sometimes error messages
> go unnoticed.
>
> When the platform "explodes" instead, you're forced to read that log carefully
> and get it working again... Anyway, I'm only thinking out loud, nothing less and
> nothing more than that :-)

Me too. :)

> By the way, we can probably expand on that topic a bit later, as it's outside of
> the scope of this specific change.
>
> Back to topic, if we get one series containing both changes (devicetree, bindings
> and driver) with the right Fixes tags and/or Cc stable, we shouldn't have such
> issue on backports so we can probably ignore that potential issue, I think? :-)

Everything goes through the soc tree, so they should appear in Linus's tree
and get picked by stable at more or less the same time. I think we would
want the driver change to appear before the dts change, for bisectability's
sake (because of header dependencies and driver errors).

So we probably want:
1. driver + binding header changes
2. dtsi changes

And have these merged through fixes so that the history between them is linear.


ChenYu

> Cheers,
> Angelo
>
> > ChenYu
> >
> >> ...Then, as Chen-Yu said, you should also add the power domain to the scp_adsp
> >> clock node as that's solving the lockup issue...
> >>
> >> .......and last, but not least: we need a Fixes tag to backport this fix, here
> >> and on the commit that adds the missing power domain in the driver.
> >>
> >> Thanks,
> >> Angelo
> >>
> >>> diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> >>> index 424fc89cc6f7..e71afba871fc 100644
> >>> --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> >>> +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> >>> @@ -514,6 +514,14 @@
> >>>                                                };
> >>>                                        };
> >>>                                };
> >>> +
> >>> +                             power-domain@MT8192_POWER_DOMAIN_ADSP {
> >>> +                                     reg = <MT8192_POWER_DOMAIN_ADSP>;
> >>> +                                     clocks = <&topckgen CLK_TOP_ADSP_SEL>;
> >>> +                                     clock-names = "adsp";
> >>> +                                     mediatek,infracfg = <&infracfg>;
> >>> +                                     #power-domain-cells = <0>;
> >>> +                             };
> >>>                        };
> >>>                };
> >>>
> >>> diff --git a/include/dt-bindings/power/mt8192-power.h b/include/dt-bindings/power/mt8192-power.h
> >>> index 4eaa53d7270a..63e81cd0d06d 100644
> >>> --- a/include/dt-bindings/power/mt8192-power.h
> >>> +++ b/include/dt-bindings/power/mt8192-power.h
> >>> @@ -28,5 +28,6 @@
> >>>    #define MT8192_POWER_DOMAIN_CAM_RAWA        18
> >>>    #define MT8192_POWER_DOMAIN_CAM_RAWB        19
> >>>    #define MT8192_POWER_DOMAIN_CAM_RAWC        20
> >>> +#define MT8192_POWER_DOMAIN_ADSP     21
> >>>
> >>>    #endif /* _DT_BINDINGS_POWER_MT8192_POWER_H */
> >>>
> >>
>
>

Re: [PATCH] arm64: dts: mt8192: Add adsp power domain controller

[AngeloGioacchino Del Regno]

Il 01/12/22 11:05, Chen-Yu Tsai ha scritto:
> On Thu, Dec 1, 2022 at 5:39 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 01/12/22 10:10, Chen-Yu Tsai ha scritto:
>>> On Thu, Dec 1, 2022 at 5:07 PM AngeloGioacchino Del Regno
>>> <angelogioacchino.delregno@collabora.com> wrote:
>>>>
>>>> Il 01/12/22 08:33, Allen-KH Cheng ha scritto:
>>>>> Add adsp power domain controller node for mt8192 SoC.
>>>>>
>>>>> Signed-off-by: Allen-KH Cheng <allen-kh.cheng@mediatek.com>
>>>>> ---
>>>>> Ref: https://lore.kernel.org/all/2ec80bd8-dfef-d2e6-eb41-6e6088043e33@collabora.com/
>>>>>        [Allen-KH Cheng <allen-kh.cheng@mediatek.com>]
>>>>> ---
>>>>> ---
>>>>>     arch/arm64/boot/dts/mediatek/mt8192.dtsi | 8 ++++++++
>>>>>     include/dt-bindings/power/mt8192-power.h | 1 +
>>>>>     2 files changed, 9 insertions(+)
>>>>>
>>>>
>>>> Allen, thanks for this one, but it's incomplete...
>>>>
>>>> First of all, you must add the power domain on the driver itself, specifically,
>>>> in drivers/soc/mediatek/mt8192-pm-domains.h - otherwise this change will have no
>>>> effect!
>>>
>>> Actually it's worse. The driver doesn't know about the new power domain,
>>> and so it will fail to probe. We might need to make the power domain driver
>>> fail gracefully and skip unknown power domains.
>>>
>>
>> Right. It's worse. I don't know, though, if gracefully skipping unknown power
>> domains in the driver would be a good decision... as sometimes error messages
>> go unnoticed.
>>
>> When the platform "explodes" instead, you're forced to read that log carefully
>> and get it working again... Anyway, I'm only thinking out loud, nothing less and
>> nothing more than that :-)
> 
> Me too. :)
> 
>> By the way, we can probably expand on that topic a bit later, as it's outside of
>> the scope of this specific change.
>>
>> Back to topic, if we get one series containing both changes (devicetree, bindings
>> and driver) with the right Fixes tags and/or Cc stable, we shouldn't have such
>> issue on backports so we can probably ignore that potential issue, I think? :-)
> 
> Everything goes through the soc tree, so they should appear in Linus's tree
> and get picked by stable at more or less the same time. I think we would
> want the driver change to appear before the dts change, for bisectability's
> sake (because of header dependencies and driver errors).
> 
> So we probably want:
> 1. driver + binding header changes
> 2. dtsi changes
> 
> And have these merged through fixes so that the history between them is linear.
> 

Perfect, I fully agree.

> 
> ChenYu
> 
>> Cheers,
>> Angelo
>>
>>> ChenYu
>>>
>>>> ...Then, as Chen-Yu said, you should also add the power domain to the scp_adsp
>>>> clock node as that's solving the lockup issue...
>>>>
>>>> .......and last, but not least: we need a Fixes tag to backport this fix, here
>>>> and on the commit that adds the missing power domain in the driver.
>>>>
>>>> Thanks,
>>>> Angelo
>>>>
>>>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
>>>>> index 424fc89cc6f7..e71afba871fc 100644
>>>>> --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
>>>>> +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
>>>>> @@ -514,6 +514,14 @@
>>>>>                                                 };
>>>>>                                         };
>>>>>                                 };
>>>>> +
>>>>> +                             power-domain@MT8192_POWER_DOMAIN_ADSP {
>>>>> +                                     reg = <MT8192_POWER_DOMAIN_ADSP>;
>>>>> +                                     clocks = <&topckgen CLK_TOP_ADSP_SEL>;
>>>>> +                                     clock-names = "adsp";
>>>>> +                                     mediatek,infracfg = <&infracfg>;
>>>>> +                                     #power-domain-cells = <0>;
>>>>> +                             };
>>>>>                         };
>>>>>                 };
>>>>>
>>>>> diff --git a/include/dt-bindings/power/mt8192-power.h b/include/dt-bindings/power/mt8192-power.h
>>>>> index 4eaa53d7270a..63e81cd0d06d 100644
>>>>> --- a/include/dt-bindings/power/mt8192-power.h
>>>>> +++ b/include/dt-bindings/power/mt8192-power.h
>>>>> @@ -28,5 +28,6 @@
>>>>>     #define MT8192_POWER_DOMAIN_CAM_RAWA        18
>>>>>     #define MT8192_POWER_DOMAIN_CAM_RAWB        19
>>>>>     #define MT8192_POWER_DOMAIN_CAM_RAWC        20
>>>>> +#define MT8192_POWER_DOMAIN_ADSP     21
>>>>>
>>>>>     #endif /* _DT_BINDINGS_POWER_MT8192_POWER_H */
>>>>>
>>>>
>>
>>

-- 
AngeloGioacchino Del Regno
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718

Re: [PATCH] arm64: dts: mt8192: Add adsp power domain controller

[Allen-KH Cheng (程冠勳)]

Hi Angelo and Chen-Yu,

On Thu, 2022-12-01 at 11:10 +0100, AngeloGioacchino Del Regno wrote:
> Il 01/12/22 11:05, Chen-Yu Tsai ha scritto:
> > On Thu, Dec 1, 2022 at 5:39 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com> wrote:
> > > 
> > > Il 01/12/22 10:10, Chen-Yu Tsai ha scritto:
> > > > On Thu, Dec 1, 2022 at 5:07 PM AngeloGioacchino Del Regno
> > > > <angelogioacchino.delregno@collabora.com> wrote:
> > > > > 
> > > > > Il 01/12/22 08:33, Allen-KH Cheng ha scritto:
> > > > > > Add adsp power domain controller node for mt8192 SoC.
> > > > > > 
> > > > > > Signed-off-by: Allen-KH Cheng <allen-kh.cheng@mediatek.com>
> > > > > > ---
> > > > > > Ref: 
> > > > > > https://lore.kernel.org/all/2ec80bd8-dfef-d2e6-eb41-6e6088043e33@collabora.com/
> > > > > >        [Allen-KH Cheng <allen-kh.cheng@mediatek.com>]
> > > > > > ---
> > > > > > ---
> > > > > >     arch/arm64/boot/dts/mediatek/mt8192.dtsi | 8 ++++++++
> > > > > >     include/dt-bindings/power/mt8192-power.h | 1 +
> > > > > >     2 files changed, 9 insertions(+)
> > > > > > 
> > > > > 
> > > > > Allen, thanks for this one, but it's incomplete...
> > > > > 
> > > > > First of all, you must add the power domain on the driver
> > > > > itself, specifically,
> > > > > in drivers/soc/mediatek/mt8192-pm-domains.h - otherwise this
> > > > > change will have no
> > > > > effect!
> > > > 
> > > > Actually it's worse. The driver doesn't know about the new
> > > > power domain,
> > > > and so it will fail to probe. We might need to make the power
> > > > domain driver
> > > > fail gracefully and skip unknown power domains.
> > > > 
> > > 
> > > Right. It's worse. I don't know, though, if gracefully skipping
> > > unknown power
> > > domains in the driver would be a good decision... as sometimes
> > > error messages
> > > go unnoticed.
> > > 
> > > When the platform "explodes" instead, you're forced to read that
> > > log carefully
> > > and get it working again... Anyway, I'm only thinking out loud,
> > > nothing less and
> > > nothing more than that :-)
> > 
> > Me too. :)
> > 
> > > By the way, we can probably expand on that topic a bit later, as
> > > it's outside of
> > > the scope of this specific change.
> > > 
> > > Back to topic, if we get one series containing both changes
> > > (devicetree, bindings
> > > and driver) with the right Fixes tags and/or Cc stable, we
> > > shouldn't have such
> > > issue on backports so we can probably ignore that potential
> > > issue, I think? :-)
> > 
> > Everything goes through the soc tree, so they should appear in
> > Linus's tree
> > and get picked by stable at more or less the same time. I think we
> > would
> > want the driver change to appear before the dts change, for
> > bisectability's
> > sake (because of header dependencies and driver errors).
> > 
> > So we probably want:
> > 1. driver + binding header changes
> > 2. dtsi changes
> > 
> > And have these merged through fixes so that the history between
> > them is linear.
> > 
> 
> Perfect, I fully agree.
> 

Thank you for your comments.

I need to check internally with my coworkers for driver and will update
v2.

Best Regards,
Allen

> > 
> > ChenYu
> > 
> > > Cheers,
> > > Angelo
> > > 
> > > > ChenYu
> > > > 
> > > > > ...Then, as Chen-Yu said, you should also add the power
> > > > > domain to the scp_adsp
> > > > > clock node as that's solving the lockup issue...
> > > > > 
> > > > > .......and last, but not least: we need a Fixes tag to
> > > > > backport this fix, here
> > > > > and on the commit that adds the missing power domain in the
> > > > > driver.
> > > > > 
> > > > > Thanks,
> > > > > Angelo
> > > > > 
> > > > > > diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > > > > > b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > > > > > index 424fc89cc6f7..e71afba871fc 100644
> > > > > > --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > > > > > @@ -514,6 +514,14 @@
> > > > > >                                                 };
> > > > > >                                         };
> > > > > >                                 };
> > > > > > +
> > > > > > +                             power-domain@MT8192_POWER_DOM
> > > > > > AIN_ADSP {
> > > > > > +                                     reg =
> > > > > > <MT8192_POWER_DOMAIN_ADSP>;
> > > > > > +                                     clocks = <&topckgen
> > > > > > CLK_TOP_ADSP_SEL>;
> > > > > > +                                     clock-names = "adsp";
> > > > > > +                                     mediatek,infracfg =
> > > > > > <&infracfg>;
> > > > > > +                                     #power-domain-cells =
> > > > > > <0>;
> > > > > > +                             };
> > > > > >                         };
> > > > > >                 };
> > > > > > 
> > > > > > diff --git a/include/dt-bindings/power/mt8192-power.h
> > > > > > b/include/dt-bindings/power/mt8192-power.h
> > > > > > index 4eaa53d7270a..63e81cd0d06d 100644
> > > > > > --- a/include/dt-bindings/power/mt8192-power.h
> > > > > > +++ b/include/dt-bindings/power/mt8192-power.h
> > > > > > @@ -28,5 +28,6 @@
> > > > > >     #define MT8192_POWER_DOMAIN_CAM_RAWA        18
> > > > > >     #define MT8192_POWER_DOMAIN_CAM_RAWB        19
> > > > > >     #define MT8192_POWER_DOMAIN_CAM_RAWC        20
> > > > > > +#define MT8192_POWER_DOMAIN_ADSP     21
> > > > > > 
> > > > > >     #endif /* _DT_BINDINGS_POWER_MT8192_POWER_H */
> > > > > > 
> > > 
> > > 
> 
> 

Re: [PATCH] arm64: dts: mt8192: Add adsp power domain controller

[Krzysztof Kozlowski]

On 01/12/2022 08:33, Allen-KH Cheng wrote:
> Add adsp power domain controller node for mt8192 SoC.
> 
> Signed-off-by: Allen-KH Cheng <allen-kh.cheng@mediatek.com>
> ---
> Ref: https://lore.kernel.org/all/2ec80bd8-dfef-d2e6-eb41-6e6088043e33@collabora.com/
>     [Allen-KH Cheng <allen-kh.cheng@mediatek.com>]
> ---
> ---
>  arch/arm64/boot/dts/mediatek/mt8192.dtsi | 8 ++++++++
>  include/dt-bindings/power/mt8192-power.h | 1 +

Bindings are separate patches.

Best regards,
Krzysztof

Re: [PATCH] arm64: dts: mt8192: Add adsp power domain controller

[Allen-KH Cheng (程冠勳)]

On Thu, 2022-12-01 at 11:22 +0100, Krzysztof Kozlowski wrote:
> On 01/12/2022 08:33, Allen-KH Cheng wrote:
> > Add adsp power domain controller node for mt8192 SoC.
> > 
> > Signed-off-by: Allen-KH Cheng <allen-kh.cheng@mediatek.com>
> > ---
> > Ref: 
> > https://lore.kernel.org/all/2ec80bd8-dfef-d2e6-eb41-6e6088043e33@collabora.com/
> >     [Allen-KH Cheng <allen-kh.cheng@mediatek.com>]
> > ---
> > ---
> >  arch/arm64/boot/dts/mediatek/mt8192.dtsi | 8 ++++++++
> >  include/dt-bindings/power/mt8192-power.h | 1 +
> 
> Bindings are separate patches.
> 
> Best regards,
> Krzysztof
> 

Noted, will fix in next version.

Thanks,
Allen