soc: sunxi: select CONFIG_PM

PATCH In review Arnd Bergmann 4 participants 6 messages Jan 30, 2023 – Feb 4, 2023

Subsystems (2)

Arm/Allwinner sunXi SoC support SPARC + UltraSPARC (sparc/sparc64)

lore

Share & tools

lore

Arnd Bergmann

author

2023-01-30 13:04 UTC #0 lore raw

PATCH 1/1 soc: sunxi: select CONFIG_PM

From: Arnd Bergmann <[email protected]>

Selecting CONFIG_PM_GENERIC_DOMAINS without CONFIG_PM leads to a build failure:

WARNING: unmet direct dependencies detected for PM_GENERIC_DOMAINS
Depends on [n]: PM [=n]
Selected by [y]:
- SUN20I_PPU [=y] && (ARCH_SUNXI [=n] || COMPILE_TEST [=y])

drivers/base/power/domain_governor.c: In function 'default_suspend_ok':
drivers/base/power/domain_governor.c:85:24: error: 'struct dev_pm_info' has no member named 'ignore_children'
85 | if (!dev->power.ignore_children)
| ^
drivers/base/power/domain.c: In function 'genpd_queue_power_off_work':
drivers/base/power/domain.c:657:20: error: 'pm_wq' undeclared (first use in this function)
657 | queue_work(pm_wq, &genpd->power_off_work);
| ^~~~~

Unfortunately platforms are inconsistent between using 'select PM' and 'depends on PM' here. CONFIG_PM is a user-visible symbol, so in principle we should be using 'depends on', but on the other hand using 'select' here is more common among drivers/soc. Go with the majority for now, as this has a smaller risk of introducing circular dependencies. We may need to clean this up for consistency later.

Fixes 0e30ca5ab0a8 ("soc: sunxi: Add Allwinner D1 PPU driver")

Signed-off-by Arnd Bergmann <[email protected]>

---
 drivers/soc/sunxi/Kconfig | 1 +
 1 file changed, 1 insertion(+)

drivers/soc/sunxi/Kconfig +1 -0

index 29e9ba2d520d..02d0b338feb3 100644
@@ -23,6 +23,7 @@ config SUNXI_SRAM
 config SUN20I_PPU
 	bool "Allwinner D1 PPU power domain driver"
 	depends on ARCH_SUNXI || COMPILE_TEST
+	select PM
 	select PM_GENERIC_DOMAINS
 	help
 	  Say y to enable the PPU power domain driver. This saves power

Signature · 1 line

2.39.0

Jernej Škrabec

maintainer Acked-by on v1 1/1

2023-01-30 21:18 UTC #1 lore raw

Dne ponedeljek, 30. januar 2023 ob 14:04:47 CET je Arnd Bergmann napisal(a):

Quoted 30 lines

From: Arnd Bergmann <[email protected]>
Selecting CONFIG_PM_GENERIC_DOMAINS without CONFIG_PM leads to a build failure:
WARNING: unmet direct dependencies detected for PM_GENERIC_DOMAINS
Depends on [n]: PM [=n]
Selected by [y]:
- SUN20I_PPU [=y] && (ARCH_SUNXI [=n] || COMPILE_TEST [=y])
drivers/base/power/domain_governor.c: In function 'default_suspend_ok':
drivers/base/power/domain_governor.c:85:24: error: 'struct dev_pm_info' has
no member named 'ignore_children' 85 | if
(!dev->power.ignore_children)
| ^
drivers/base/power/domain.c: In function 'genpd_queue_power_off_work':
drivers/base/power/domain.c:657:20: error: 'pm_wq' undeclared (first use in
this function) 657 | queue_work(pm_wq, &genpd->power_off_work);
| ^~~~~
Unfortunately platforms are inconsistent between using 'select PM' and 'depends on PM' here. CONFIG_PM is a user-visible symbol, so in principle we should be using 'depends on', but on the other hand using 'select' here is more common among drivers/soc. Go with the majority for now, as this has a smaller risk of introducing circular dependencies. We may need to clean this up for consistency later.

Quoted 2 lines

Fixes 0e30ca5ab0a8 ("soc: sunxi: Add Allwinner D1 PPU driver")
Signed-off-by Arnd Bergmann <[email protected]>

Acked-by Jernej Skrabec <[email protected]>

Although, would it be better if commit in question is fixed and another PR is issued? That way we can avoid issue with broken config.

Best regards,
Jernej

Arnd Bergmann

2023-01-30 21:24 UTC #2 lore raw

Quoted 31 lines · Jernej Škrabec

Dne ponedeljek, 30. januar 2023 ob 14:04:47 CET je Arnd Bergmann napisal(a):
Quoted 30 lines
From: Arnd Bergmann <[email protected]>
Selecting CONFIG_PM_GENERIC_DOMAINS without CONFIG_PM leads to a build failure:
WARNING: unmet direct dependencies detected for PM_GENERIC_DOMAINS
Depends on [n]: PM [=n]
Selected by [y]:
- SUN20I_PPU [=y] && (ARCH_SUNXI [=n] || COMPILE_TEST [=y])
drivers/base/power/domain_governor.c: In function 'default_suspend_ok':
drivers/base/power/domain_governor.c:85:24: error: 'struct dev_pm_info' has
no member named 'ignore_children' 85 | if
(!dev->power.ignore_children)
| ^
drivers/base/power/domain.c: In function 'genpd_queue_power_off_work':
drivers/base/power/domain.c:657:20: error: 'pm_wq' undeclared (first use in
this function) 657 | queue_work(pm_wq, &genpd->power_off_work);
| ^~~~~
Unfortunately platforms are inconsistent between using 'select PM' and 'depends on PM' here. CONFIG_PM is a user-visible symbol, so in principle we should be using 'depends on', but on the other hand using 'select' here is more common among drivers/soc. Go with the majority for now, as this has a smaller risk of introducing circular dependencies. We may need to clean this up for consistency later.

Quoted 7 lines

Quoted 2 lines
Fixes 0e30ca5ab0a8 ("soc: sunxi: Add Allwinner D1 PPU driver")
Signed-off-by Arnd Bergmann <[email protected]>
Acked-by Jernej Skrabec <[email protected]>
Although, would it be better if commit in question is fixed and another PR is issued? That way we can avoid issue with broken config.

Agreed, merging it into the original commit would have been ideal. unfortunately the half of my brain that does the pull request didn't talk to the half that wrote the bugfix, so I already pulled it in after I sent the fix for linux-next...

I'll just add the fix with your Ack on top then.

Arnd

Randy Dunlap

2023-02-04 07:47 UTC #3 lore raw

In reply to Arnd Bergmann

Hi--

Quoted 26 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>
Selecting CONFIG_PM_GENERIC_DOMAINS without CONFIG_PM leads to a build failure:
WARNING: unmet direct dependencies detected for PM_GENERIC_DOMAINS
Depends on [n]: PM [=n]
Selected by [y]:
- SUN20I_PPU [=y] && (ARCH_SUNXI [=n] || COMPILE_TEST [=y])
drivers/base/power/domain_governor.c: In function 'default_suspend_ok':
drivers/base/power/domain_governor.c:85:24: error: 'struct dev_pm_info' has no member named 'ignore_children'
85 | if (!dev->power.ignore_children)
| ^
drivers/base/power/domain.c: In function 'genpd_queue_power_off_work':
drivers/base/power/domain.c:657:20: error: 'pm_wq' undeclared (first use in this function)
657 | queue_work(pm_wq, &genpd->power_off_work);
| ^~~~~
Unfortunately platforms are inconsistent between using 'select PM' and 'depends on PM' here. CONFIG_PM is a user-visible symbol, so in principle we should be using 'depends on', but on the other hand using 'select' here is more common among drivers/soc. Go with the majority for now, as this has a smaller risk of introducing circular dependencies. We may need to clean this up for consistency later.

Quoted 2 lines

Fixes 0e30ca5ab0a8 ("soc: sunxi: Add Allwinner D1 PPU driver")
Signed-off-by Arnd Bergmann <[email protected]>

With this patch (in linux-next-20230203), building on sparc32 with COMPILE_TEST=y and ARCH_SUNXI not set:

WARNING: unmet direct dependencies detected for PM
Depends on [n]: SPARC64 [=n]
Selected by [y]:
- SUN20I_PPU [=y] && (ARCH_SUNXI || COMPILE_TEST [=y])

WARNING: unmet direct dependencies detected for PM_GENERIC_DOMAINS
Depends on [n]: SPARC64 [=n] && PM [=y]
Selected by [y]:
- QCOM_GDSC [=y] && COMMON_CLK [=y] && PM [=y]
- SUN20I_PPU [=y] && (ARCH_SUNXI || COMPILE_TEST [=y])
- MESON_GX_PM_DOMAINS [=y] && (ARCH_MESON || COMPILE_TEST [=y]) && PM [=y] && OF [=y]
- BCM2835_POWER [=y] && (ARCH_BCM2835 || COMPILE_TEST [=y] && OF [=y]) && PM [=y]
- BCM_PMB [=y] && (ARCH_BCMBCA || COMPILE_TEST [=y] && OF [=y]) && PM [=y]
- ROCKCHIP_PM_DOMAINS [=y] && (ARCH_ROCKCHIP || COMPILE_TEST [=y]) && PM [=y] Selected by [m]:
- ARM_SCPI_POWER_DOMAIN [=m] && (ARM_SCPI_PROTOCOL [=m] || COMPILE_TEST [=y] && OF [=y]) && PM [=y]
- MESON_EE_PM_DOMAINS [=m] && (ARCH_MESON || COMPILE_TEST [=y]) && PM [=y] && OF [=y]
- QCOM_AOSS_QMP [=m] && (ARCH_QCOM || COMPILE_TEST [=y]) && MAILBOX [=y] && COMMON_CLK [=y] && PM [=y]

WARNING: unmet direct dependencies detected for PM_GENERIC_DOMAINS_OF
Depends on [n]: SPARC64 [=n] && PM_GENERIC_DOMAINS [=y] && OF [=y] Selected by [y]:
- MESON_GX_PM_DOMAINS [=y] && (ARCH_MESON || COMPILE_TEST [=y]) && PM [=y] && OF [=y] Selected by [m]:
- MESON_EE_PM_DOMAINS [=m] && (ARCH_MESON || COMPILE_TEST [=y]) && PM [=y] && OF [=y]

Apparently sparc32 does not support PM (arch/sparc/Kconfig):

if SPARC64
source "kernel/power/Kconfig"
endif

so I think that SUN20I_PPU should also depend on !SPARC32. Does that make sense?

Thanks.

Quoted 16 lines

---
 drivers/soc/sunxi/Kconfig | 1 +
 1 file changed, 1 insertion(+)

drivers/soc/sunxi/Kconfig +1 -0

index 29e9ba2d520d..02d0b338feb3 100644
@@ -23,6 +23,7 @@ config SUNXI_SRAM
 config SUN20I_PPU
 	bool "Allwinner D1 PPU power domain driver"
 	depends on ARCH_SUNXI || COMPILE_TEST
+	select PM
 	select PM_GENERIC_DOMAINS
 	help
 	  Say y to enable the PPU power domain driver. This saves power

Signature · 1 line

~Randy

Arnd Bergmann

2023-02-04 10:39 UTC #4 lore raw

Quoted 10 lines · Randy Dunlap

On 1/30/23 05:04, Arnd Bergmann wrote:
Apparently sparc32 does not support PM (arch/sparc/Kconfig):
if SPARC64
source "kernel/power/Kconfig"
endif
so I think that SUN20I_PPU should also depend on !SPARC32. Does that make sense?

I would suggest working around this in arch/sparc/ instead of every driver that uses 'select PM', perhaps something like

arch/sparc/Kconfig +1 -1

@@ -283,7 +283,7 @@ config ARCH_FORCE_MAX_ORDER
          This config option is actually maximum order plus one. For example,
          a value of 13 means that the largest free memory block is 2^12 pages.
 
-if SPARC64
+if SPARC64 || COMPILE_TEST
 source "kernel/power/Kconfig"
 endif

The issue does not happen anywhere else, as it's fine if kernel/power/Kconfig is not included at all. I'm also not too worried about random sparc32 configs since it's pretty much impossible to build a sparc32 allmodconfig or randconfig, with all the other bugs that runs into.

Arnd

Randy Dunlap

2023-02-04 16:54 UTC #5 lore raw

Quoted 18 lines · Arnd Bergmann

Quoted 10 lines · Randy Dunlap
On 1/30/23 05:04, Arnd Bergmann wrote:
Apparently sparc32 does not support PM (arch/sparc/Kconfig):
if SPARC64
source "kernel/power/Kconfig"
endif
so I think that SUN20I_PPU should also depend on !SPARC32. Does that make sense?
I would suggest working around this in arch/sparc/ instead of every driver that uses 'select PM', perhaps something like

arch/sparc/Kconfig config ARCH_FORCE_MAX_ORDER · line 289

Quoted 8 lines

arch/sparc/Kconfig +1 -1

@@ -283,7 +283,7 @@ config ARCH_FORCE_MAX_ORDER
          This config option is actually maximum order plus one. For example,
          a value of 13 means that the largest free memory block is 2^12 pages.
 
-if SPARC64
+if SPARC64 || COMPILE_TEST
 source "kernel/power/Kconfig"
 endif

That looks good. Thanks.

Quoted 5 lines

The issue does not happen anywhere else, as it's fine if kernel/power/Kconfig is not included at all. I'm also not too worried about random sparc32 configs since it's pretty much impossible to build a sparc32 allmodconfig or randconfig, with all the other bugs that runs into.

OK/ack.

Signature · 1 line

~Randy