| Message ID | 1423244258-24314-1-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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?
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
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
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
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
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@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
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@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
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@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
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@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.
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@10044000 { + gsc_pd: gsc-power-domain@10044000 { compatible = "samsung,exynos4210-pd"; reg = <0x10044000 0x20>; #power-domain-cells = <0>; }; - isp_pd: power-domain@10044020 { + isp_pd: isp-power-domain@10044020 { compatible = "samsung,exynos4210-pd"; reg = <0x10044020 0x20>; #power-domain-cells = <0>; }; - mfc_pd: power-domain@10044060 { + mfc_pd: mfc-power-domain@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@10044120 { + msc_pd: msc-power-domain@10044120 { compatible = "samsung,exynos4210-pd"; reg = <0x10044120 0x20>; #power-domain-cells = <0>; }; - disp_pd: power-domain@100440C0 { + disp_pd: disp-power-domain@100440C0 { compatible = "samsung,exynos4210-pd"; reg = <0x100440C0 0x20>; #power-domain-cells = <0>;
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(-)