[PATCH 1/1] ARM: dts: Use more descriptive names for Exynos5420 PDs

[PATCH 1/1] ARM: dts: Use more descriptive names for Exynos5420 PDs

[Javier Martinez Canillas]

All the device nodes for the Exynos5420 power-domains have a quite
generic "power-domain" name. So in case of an error, the Exynos PD
driver shows the following (not very useful) message:

"Power domain power-domain disable failed"

Use descriptive names to know on which PD enable or disable failed.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/boot/dts/exynos5420.dtsi | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index b63b569ca4b4..4642cec50c0d 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -247,19 +247,19 @@
 		};
 	};
 
-	gsc_pd: power-domain at 10044000 {
+	gsc_pd: gsc-power-domain at 10044000 {
 		compatible = "samsung,exynos4210-pd";
 		reg = <0x10044000 0x20>;
 		#power-domain-cells = <0>;
 	};
 
-	isp_pd: power-domain at 10044020 {
+	isp_pd: isp-power-domain at 10044020 {
 		compatible = "samsung,exynos4210-pd";
 		reg = <0x10044020 0x20>;
 		#power-domain-cells = <0>;
 	};
 
-	mfc_pd: power-domain at 10044060 {
+	mfc_pd: mfc-power-domain at 10044060 {
 		compatible = "samsung,exynos4210-pd";
 		reg = <0x10044060 0x20>;
 		clocks = <&clock CLK_FIN_PLL>, <&clock CLK_MOUT_SW_ACLK333>,
@@ -268,13 +268,13 @@
 		#power-domain-cells = <0>;
 	};
 
-	msc_pd: power-domain at 10044120 {
+	msc_pd: msc-power-domain at 10044120 {
 		compatible = "samsung,exynos4210-pd";
 		reg = <0x10044120 0x20>;
 		#power-domain-cells = <0>;
 	};
 
-	disp_pd: power-domain at 100440C0 {
+	disp_pd: disp-power-domain at 100440C0 {
 		compatible = "samsung,exynos4210-pd";
 		reg = <0x100440C0 0x20>;
 		#power-domain-cells = <0>;
-- 
2.1.3

[PATCH 1/1] ARM: dts: Use more descriptive names for Exynos5420 PDs

[Sergei Shtylyov]

Hello.

On 02/06/2015 08:37 PM, Javier Martinez Canillas wrote:

> All the device nodes for the Exynos5420 power-domains have a quite
> generic "power-domain" name.

    And this is in conformance to the ePAPR standard.

> So in case of an error, the Exynos PD
> driver shows the following (not very useful) message:

> "Power domain power-domain disable failed"

    Why not fix the message instead to use the full device name?

> Use descriptive names to know on which PD enable or disable failed.

> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

WBR, Sergei

[PATCH 1/1] ARM: dts: Use more descriptive names for Exynos5420 PDs

[Javier Martinez Canillas]

Hello Sergei,

Thanks a lot for your feedback.

On 02/06/2015 08:09 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 02/06/2015 08:37 PM, Javier Martinez Canillas wrote:
> 
>> All the device nodes for the Exynos5420 power-domains have a quite
>> generic "power-domain" name.
> 
>     And this is in conformance to the ePAPR standard.
> 

True, I forgot that the ePAPR recommends that the node names should be
somewhat generic but OTOH this is the only Exynos DTSI file that follows
the standard for the power domain device nodes. All other Exynos DTSI
use a prefix to differentiate between each power domain.

>> So in case of an error, the Exynos PD
>> driver shows the following (not very useful) message:
> 
>> "Power domain power-domain disable failed"
> 
>     Why not fix the message instead to use the full device name?
>

Well, the full node name is also not very useful IMHO since you have
to check the DTSI or SoC manual to map the device node unit-address to
the corresponding power domain.

I used $subject when debugging an HDMI issue and instead of dropping
it, I just posted it in case someone considered useful. I don't really
mind if the patch is nacked / not picked.

Best regards,
Javier

[PATCH 1/1] ARM: dts: Use more descriptive names for Exynos5420 PDs

[Krzysztof Kozlowski]

2015-02-06 21:50 GMT+01:00 Javier Martinez Canillas
<javier.martinez@collabora.co.uk>:
> Hello Sergei,
>
> Thanks a lot for your feedback.
>
> On 02/06/2015 08:09 PM, Sergei Shtylyov wrote:
>> Hello.
>>
>> On 02/06/2015 08:37 PM, Javier Martinez Canillas wrote:
>>
>>> All the device nodes for the Exynos5420 power-domains have a quite
>>> generic "power-domain" name.
>>
>>     And this is in conformance to the ePAPR standard.
>>
>
> True, I forgot that the ePAPR recommends that the node names should be
> somewhat generic but OTOH this is the only Exynos DTSI file that follows
> the standard for the power domain device nodes. All other Exynos DTSI
> use a prefix to differentiate between each power domain.
>
>>> So in case of an error, the Exynos PD
>>> driver shows the following (not very useful) message:
>>
>>> "Power domain power-domain disable failed"
>>
>>     Why not fix the message instead to use the full device name?
>>
>
> Well, the full node name is also not very useful IMHO since you have
> to check the DTSI or SoC manual to map the device node unit-address to
> the corresponding power domain.
>
> I used $subject when debugging an HDMI issue and instead of dropping
> it, I just posted it in case someone considered useful. I don't really
> mind if the patch is nacked / not picked.

Additionally (on Arndale Octa):

$ cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
    domain                      status         slaves
           /device                                      runtime status
----------------------------------------------------------------------
power-domain                    on
    /devices/platform/amba/3880000.adma                 suspended
power-domain                    off
power-domain                    off
power-domain                    off
power-domain                    off
power-domain                    off

This really is not helpful. From the power domain debugfs code it is
complicated to extract of_node of power domain. It is easier to print
the name of power domain. But wait... all names are the same! :) So
why do we have the name in the first place?

[PATCH 1/1] ARM: dts: Use more descriptive names for Exynos5420 PDs

[Sergei Shtylyov]

Hello.

On 2/10/2015 2:46 PM, Krzysztof Kozlowski wrote:

>>>> All the device nodes for the Exynos5420 power-domains have a quite
>>>> generic "power-domain" name.

>>>      And this is in conformance to the ePAPR standard.

>> True, I forgot that the ePAPR recommends that the node names should be
>> somewhat generic but OTOH this is the only Exynos DTSI file that follows
>> the standard for the power domain device nodes. All other Exynos DTSI
>> use a prefix to differentiate between each power domain.

>>>> So in case of an error, the Exynos PD
>>>> driver shows the following (not very useful) message:

>>>> "Power domain power-domain disable failed"

>>>      Why not fix the message instead to use the full device name?

>> Well, the full node name is also not very useful IMHO since you have
>> to check the DTSI or SoC manual to map the device node unit-address to
>> the corresponding power domain.

>> I used $subject when debugging an HDMI issue and instead of dropping
>> it, I just posted it in case someone considered useful. I don't really
>> mind if the patch is nacked / not picked.

> Additionally (on Arndale Octa):

> $ cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
>      domain                      status         slaves
>             /device                                      runtime status
> ----------------------------------------------------------------------
> power-domain                    on
>      /devices/platform/amba/3880000.adma                 suspended
> power-domain                    off
> power-domain                    off
> power-domain                    off
> power-domain                    off
> power-domain                    off

> This really is not helpful. From the power domain debugfs code it is
> complicated to extract of_node of power domain.

    You shouldn't need it.

> It is easier to print
> the name of power domain. But wait... all names are the same! :) So
> why do we have the name in the first place?

    I'm not sure why the full platform device names aren't printed -- they 
should all be different.

WBR, Sergei

[PATCH 1/1] ARM: dts: Use more descriptive names for Exynos5420 PDs

[Krzysztof Kozlowski]

On wto, 2015-02-10 at 14:55 +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 2/10/2015 2:46 PM, Krzysztof Kozlowski wrote:
> > Additionally (on Arndale Octa):
> 
> > $ cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
> >      domain                      status         slaves
> >             /device                                      runtime status
> > ----------------------------------------------------------------------
> > power-domain                    on
> >      /devices/platform/amba/3880000.adma                 suspended
> > power-domain                    off
> > power-domain                    off
> > power-domain                    off
> > power-domain                    off
> > power-domain                    off
> 
> > This really is not helpful. From the power domain debugfs code it is
> > complicated to extract of_node of power domain.
> 
>     You shouldn't need it.
> 
> > It is easier to print
> > the name of power domain. But wait... all names are the same! :) So
> > why do we have the name in the first place?
> 
>     I'm not sure why the full platform device names aren't printed -- they 
> should all be different.

This debugfs code iterates over list of generic_pm_domains (gpd_list). I
cannot find function for translating from genpd to its platform device
so only genpd->name can be printed.

Best regards,
Krzysztof

[PATCH 1/1] ARM: dts: Use more descriptive names for Exynos5420 PDs

[Sergei Shtylyov]

On 2/10/2015 3:17 PM, Krzysztof Kozlowski wrote:

>>> Additionally (on Arndale Octa):

>>> $ cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
>>>       domain                      status         slaves
>>>              /device                                      runtime status
>>> ----------------------------------------------------------------------
>>> power-domain                    on
>>>       /devices/platform/amba/3880000.adma                 suspended
>>> power-domain                    off
>>> power-domain                    off
>>> power-domain                    off
>>> power-domain                    off
>>> power-domain                    off

>>> This really is not helpful. From the power domain debugfs code it is
>>> complicated to extract of_node of power domain.

>>      You shouldn't need it.

>>> It is easier to print
>>> the name of power domain. But wait... all names are the same! :) So
>>> why do we have the name in the first place?

>>      I'm not sure why the full platform device names aren't printed -- they
>> should all be different.

> This debugfs code iterates over list of generic_pm_domains (gpd_list). I
> cannot find function for translating from genpd to its platform device
> so only genpd->name can be printed.

    Then why power domains aren't just named with the platform device names?

> Best regards,
> Krzysztof

WBR, Sergei

[PATCH 1/1] ARM: dts: Use more descriptive names for Exynos5420 PDs

[Krzysztof Kozlowski]

On wto, 2015-02-10 at 15:21 +0300, Sergei Shtylyov wrote:
> On 2/10/2015 3:17 PM, Krzysztof Kozlowski wrote:
> 
> >>> Additionally (on Arndale Octa):
> 
> >>> $ cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
> >>>       domain                      status         slaves
> >>>              /device                                      runtime status
> >>> ----------------------------------------------------------------------
> >>> power-domain                    on
> >>>       /devices/platform/amba/3880000.adma                 suspended
> >>> power-domain                    off
> >>> power-domain                    off
> >>> power-domain                    off
> >>> power-domain                    off
> >>> power-domain                    off
> 
> >>> This really is not helpful. From the power domain debugfs code it is
> >>> complicated to extract of_node of power domain.
> 
> >>      You shouldn't need it.
> 
> >>> It is easier to print
> >>> the name of power domain. But wait... all names are the same! :) So
> >>> why do we have the name in the first place?
> 
> >>      I'm not sure why the full platform device names aren't printed -- they
> >> should all be different.
> 
> > This debugfs code iterates over list of generic_pm_domains (gpd_list). I
> > cannot find function for translating from genpd to its platform device
> > so only genpd->name can be printed.
> 
>     Then why power domains aren't just named with the platform device names?

Right, the mach-exynos/pm_domains.c set the name equal to OF node name.
I'll send a patch extending the name.

Best regards,
Krzysztof

[PATCH 1/1] ARM: dts: Use more descriptive names for Exynos5420 PDs

[Javier Martinez Canillas]

Hello,

On 02/10/2015 01:30 PM, Krzysztof Kozlowski wrote:
> On wto, 2015-02-10 at 15:21 +0300, Sergei Shtylyov wrote:
>> On 2/10/2015 3:17 PM, Krzysztof Kozlowski wrote:
>> 
>> >>> Additionally (on Arndale Octa):
>> 
>> >>> $ cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
>> >>>       domain                      status         slaves
>> >>>              /device                                      runtime status
>> >>> ----------------------------------------------------------------------
>> >>> power-domain                    on
>> >>>       /devices/platform/amba/3880000.adma                 suspended
>> >>> power-domain                    off
>> >>> power-domain                    off
>> >>> power-domain                    off
>> >>> power-domain                    off
>> >>> power-domain                    off
>> 
>> >>> This really is not helpful. From the power domain debugfs code it is
>> >>> complicated to extract of_node of power domain.
>>

Not very useful indeed.
 
>> >>      You shouldn't need it.
>> 
>> >>> It is easier to print
>> >>> the name of power domain. But wait... all names are the same! :) So
>> >>> why do we have the name in the first place?
>> 
>> >>      I'm not sure why the full platform device names aren't printed -- they
>> >> should all be different.
>> 

Yes, but like I said in a previous email the fact that are different doesn't
necessarily mean that they will be helpful for debugging purposes.

>> > This debugfs code iterates over list of generic_pm_domains (gpd_list). I
>> > cannot find function for translating from genpd to its platform device
>> > so only genpd->name can be printed.
>> 
>>     Then why power domains aren't just named with the platform device names?
> 
> Right, the mach-exynos/pm_domains.c set the name equal to OF node name.
> I'll send a patch extending the name.
>

IIRC the OF core uses the device node unit address and node name to create
the platform device names so you will have something like 10044000.power-domain.

Same if using the node full_name since it will /power-domain at 10044000. In both
cases the DTS should have to be checked to know which power domain really is
unless someone knows by heart the power domains addresses.

But if using generic names for the power domains as suggested by ePAPR is so
important then we should change all the other Exynos DTS files which don't do.
 
> Best regards,
> Krzysztof
> 
> 

Best regards,
Javier

[PATCH 1/1] ARM: dts: Use more descriptive names for Exynos5420 PDs

[Sylwester Nawrocki]

On 10/02/15 13:46, Javier Martinez Canillas wrote:
>>>> This debugfs code iterates over list of generic_pm_domains (gpd_list). I
>>>> >> > cannot find function for translating from genpd to its platform device
>>>> >> > so only genpd->name can be printed.
>>> >> 
>>> >>     Then why power domains aren't just named with the platform device names?
>> > 
>> > Right, the mach-exynos/pm_domains.c set the name equal to OF node name.
>> > I'll send a patch extending the name.
>> >
> IIRC the OF core uses the device node unit address and node name to create
> the platform device names so you will have something like 10044000.power-domain.
> 
> Same if using the node full_name since it will /power-domain at 10044000. In both
> cases the DTS should have to be checked to know which power domain really is
> unless someone knows by heart the power domains addresses.
> 
> But if using generic names for the power domains as suggested by ePAPR is so
> important then we should change all the other Exynos DTS files which don't do.

Perhaps we could assign OF aliases to the power domain device nodes in DT
and then in the power domains driver map those aliases to more descriptive
names when creating the power domains?

--
Regards,
Sylwester

[PATCH 1/1] ARM: dts: Use more descriptive names for Exynos5420 PDs

[Krzysztof Kozlowski]

On wto, 2015-02-10 at 14:00 +0100, Sylwester Nawrocki wrote:
> On 10/02/15 13:46, Javier Martinez Canillas wrote:
> >>>> This debugfs code iterates over list of generic_pm_domains (gpd_list). I
> >>>> >> > cannot find function for translating from genpd to its platform device
> >>>> >> > so only genpd->name can be printed.
> >>> >> 
> >>> >>     Then why power domains aren't just named with the platform device names?
> >> > 
> >> > Right, the mach-exynos/pm_domains.c set the name equal to OF node name.
> >> > I'll send a patch extending the name.
> >> >
> > IIRC the OF core uses the device node unit address and node name to create
> > the platform device names so you will have something like 10044000.power-domain.
> > 
> > Same if using the node full_name since it will /power-domain at 10044000. In both
> > cases the DTS should have to be checked to know which power domain really is
> > unless someone knows by heart the power domains addresses.

For the kernel developer that would be descriptive enough to find the
real domain but... as you said each time one would have to grep through
manual or DTS which is slower. However for end-user that still won't be
descriptive enough.

> > 
> > But if using generic names for the power domains as suggested by ePAPR is so
> > important then we should change all the other Exynos DTS files which don't do.
> 
> Perhaps we could assign OF aliases to the power domain device nodes in DT
> and then in the power domains driver map those aliases to more descriptive
> names when creating the power domains?

That would required additional alias in DT but it could be the most
descriptive for a user.

Best regards,
Krzysztof

[PATCH 1/1] ARM: dts: Use more descriptive names for Exynos5420 PDs

[Sylwester Nawrocki]

On 10/02/15 14:14, Krzysztof Kozlowski wrote:
> On wto, 2015-02-10 at 14:00 +0100, Sylwester Nawrocki wrote:
>> > On 10/02/15 13:46, Javier Martinez Canillas wrote:
>>>>>> > >>>> This debugfs code iterates over list of generic_pm_domains (gpd_list). I
>>>>>>>>> > >>>> >> > cannot find function for translating from genpd to its platform device
>>>>>>>>> > >>>> >> > so only genpd->name can be printed.
>>>>>>> > >>> >> 
>>>>>>> > >>> >>     Then why power domains aren't just named with the platform device names?
>>>>> > >> > 
>>>>> > >> > Right, the mach-exynos/pm_domains.c set the name equal to OF node name.
>>>>> > >> > I'll send a patch extending the name.
>>>>> > >> >
>>> > > IIRC the OF core uses the device node unit address and node name to create
>>> > > the platform device names so you will have something like 10044000.power-domain.
>>> > > 
>>> > > Same if using the node full_name since it will /power-domain at 10044000. In both
>>> > > cases the DTS should have to be checked to know which power domain really is
>>> > > unless someone knows by heart the power domains addresses.
> For the kernel developer that would be descriptive enough to find the
> real domain but... as you said each time one would have to grep through
> manual or DTS which is slower. However for end-user that still won't be
> descriptive enough.
> 
>>> > > 
>>> > > But if using generic names for the power domains as suggested by ePAPR is so
>>> > > important then we should change all the other Exynos DTS files which don't do.
>> > 
>> > Perhaps we could assign OF aliases to the power domain device nodes in DT
>> > and then in the power domains driver map those aliases to more descriptive
>> > names when creating the power domains?
>
> That would required additional alias in DT but it could be the most
> descriptive for a user.

Yes, we could fall back to of_node->full_name if alias is not present in DT.

-- 
Regards,
Sylwester