| Message ID | 20240221174610.3560775-1-alexander.sverdlin@siemens.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [RFC] power: reset: restart-poweroff: convert to module | expand |
Hi, On Wed, Feb 21, 2024 at 06:46:07PM +0100, A. Sverdlin wrote: > From: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > The necessity of having a fake platform device for a generic, platform > independent functionality is not obvious. > Some platforms requre device tree modification for this, some would require > ACPI tables modification, while functionality may be useful even to > end-users without required expertise. Convert the platform driver to > a simple module. > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> > --- > This RFC is merely to understand if this approach would be accepted. > Converting to "tristate" could follow or preceed this patch. 1. You cannot easily make this tristate, because of machine_restart(). 2. This is already using module_platform_driver(), so this has nothing to do with making it a module like the subject suggests. 3. This no longer applies, since the driver is now properly using devm_register_sys_off_handler instead of pm_power_off. 4. It's intentional, that a device needs to be described. This is _not_ meant as a general purpose poweroff driver. It's intended to be used with bootloader support, which keeps the system off as described in the comment at the start of the file. So: NAK -- Sebastian > diff --git a/drivers/power/reset/restart-poweroff.c b/drivers/power/reset/restart-poweroff.c > index 28f1822db1626..e1d94109f6823 100644 > --- a/drivers/power/reset/restart-poweroff.c > +++ b/drivers/power/reset/restart-poweroff.c > @@ -20,7 +20,7 @@ static void restart_poweroff_do_poweroff(void) > machine_restart(NULL); > } > > -static int restart_poweroff_probe(struct platform_device *pdev) > +static int __init restart_poweroff_init(void) > { > /* If a pm_power_off function has already been added, leave it alone */ > if (pm_power_off != NULL) { > @@ -33,12 +33,10 @@ static int restart_poweroff_probe(struct platform_device *pdev) > return 0; > } > > -static int restart_poweroff_remove(struct platform_device *pdev) > +static void __exit restart_poweroff_exit(void) > { > if (pm_power_off == &restart_poweroff_do_poweroff) > pm_power_off = NULL; > - > - return 0; > } > > static const struct of_device_id of_restart_poweroff_match[] = { > @@ -47,15 +45,8 @@ static const struct of_device_id of_restart_poweroff_match[] = { > }; > MODULE_DEVICE_TABLE(of, of_restart_poweroff_match); > > -static struct platform_driver restart_poweroff_driver = { > - .probe = restart_poweroff_probe, > - .remove = restart_poweroff_remove, > - .driver = { > - .name = "poweroff-restart", > - .of_match_table = of_restart_poweroff_match, > - }, > -}; > -module_platform_driver(restart_poweroff_driver); > +module_init(restart_poweroff_init); > +module_exit(restart_poweroff_exit); > > MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch"); > MODULE_DESCRIPTION("restart poweroff driver"); > -- > 2.43.0 >
On Wed, Feb 21, 2024 at 06:46:07PM +0100, A. Sverdlin wrote: > From: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > The necessity of having a fake platform device for a generic, platform > independent functionality is not obvious. > Some platforms requre device tree modification for this, some would require > ACPI tables modification, while functionality may be useful even to > end-users without required expertise. Convert the platform driver to > a simple module. > @@ -47,15 +45,8 @@ static const struct of_device_id of_restart_poweroff_match[] = { > }; > MODULE_DEVICE_TABLE(of, of_restart_poweroff_match); > > -static struct platform_driver restart_poweroff_driver = { > - .probe = restart_poweroff_probe, > - .remove = restart_poweroff_remove, > - .driver = { > - .name = "poweroff-restart", > - .of_match_table = of_restart_poweroff_match, > - }, > -}; of_restart_poweroff_match now seems to be disconnected from the driver. kirkwood-linkstation.dtsi: compatible = "restart-poweroff"; kirkwood-lsxl.dtsi: compatible = "restart-poweroff"; orion5x-linkstation.dtsi: compatible = "restart-poweroff"; orion5x-lswsgl.dts: compatible = "restart-poweroff"; How do these devices get this driver loaded? This appears to be another reason to NACK it. Andrew
On Wed, Feb 21, 2024 at 06:46:07PM +0100, A. Sverdlin wrote: > From: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > The necessity of having a fake platform device for a generic, platform > independent functionality is not obvious. > Some platforms requre device tree modification for this, some would require > ACPI tables modification, while functionality may be useful even to > end-users without required expertise. Convert the platform driver to > a simple module. > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> > --- > This RFC is merely to understand if this approach would be accepted. > Converting to "tristate" could follow or preceed this patch. So that is you use case here? Why do you want to be able to just load this driver, without using DT to indicate it is needed by the hardware? Andrew
On Wed, 2024-02-21 at 22:56 +0100, Andrew Lunn wrote: > > @@ -47,15 +45,8 @@ static const struct of_device_id of_restart_poweroff_match[] = { > > }; > > MODULE_DEVICE_TABLE(of, of_restart_poweroff_match); > > > > -static struct platform_driver restart_poweroff_driver = { > > - .probe = restart_poweroff_probe, > > - .remove = restart_poweroff_remove, > > - .driver = { > > - .name = "poweroff-restart", > > - .of_match_table = of_restart_poweroff_match, > > - }, > > -}; > > of_restart_poweroff_match now seems to be disconnected from the > driver. > > kirkwood-linkstation.dtsi: compatible = "restart-poweroff"; > kirkwood-lsxl.dtsi: compatible = "restart-poweroff"; > orion5x-linkstation.dtsi: compatible = "restart-poweroff"; > orion5x-lswsgl.dts: compatible = "restart-poweroff"; > > How do these devices get this driver loaded? > > This appears to be another reason to NACK it. That's why MODULE_DEVICE_TABLE() was preserved for backwards compatibility, because *loading* happens via MODULE_DEVICE_TABLE(). But I didn't realize it was never buildable as module as Sebastian pointed out, because of machine_restart(). For your use case it would continue to work as before I believe, just the callback would be installed because of the fact the code was compiled-in, not because there was a fake platform device. I also didn't understand what is so special about bootloader support for this functionality if no data is passed to the bootloader. After ARM-specifics was removed from the code quite some time ago any platform could re-use the code for the deployments which meant to be "always on". But If the resistance is so serious, so be it.
Hi Andrew! On Wed, 2024-02-21 at 23:37 +0100, Andrew Lunn wrote: > > The necessity of having a fake platform device for a generic, platform > > independent functionality is not obvious. > > Some platforms requre device tree modification for this, some would require > > ACPI tables modification, while functionality may be useful even to > > end-users without required expertise. Convert the platform driver to > > a simple module. > > > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > --- > > This RFC is merely to understand if this approach would be accepted. > > Converting to "tristate" could follow or preceed this patch. > > So that is you use case here? Why do you want to be able to just load > this driver, without using DT to indicate it is needed by the > hardware? Yes, the code is platform-independent now and can be re-used for deployments which meant to be "always on". One could actually even use it with off-the-shelf x86 hardware. But instantiating a platform device there would be a hack. Why not just control this code in the kernel config?
> I also didn't understand what is so special about bootloader support > for this functionality if no data is passed to the bootloader. > After ARM-specifics was removed from the code quite some time ago > any platform could re-use the code for the deployments which meant > to be "always on". If i'm remembering correctly.... When the box cold boots, the bootloader can see the boot reason, and chains into Linux. When the user does a shutdown, we re-enter the bootloader, and it sees it is a warm boot. It then spins, rather than chaining into Linux. The process of getting into the bootloader, a watchdog triggered reset, probably disables a lot of clocks and other bits of hardware, so spinning in the bootloader consumes less power than if Linux was to spin. So bootloader needs this functionality, to either chain, or to spin. Andrew
> Yes, the code is platform-independent now and can be re-used for deployments > which meant to be "always on". You need to be careful with the meaning of "always on". It is always on in that the hardware does not have any PMICs. It is impossible to turn the power off. This is a poweroff driver, and it powers the hardware off by dropping into the bootloader which then spins. > One could actually even use it with off-the-shelf x86 hardware. Is there off the shelf x86 which does not support turning the power off? I'm not familiar with x86 that much, but it seems to be a feature that has existed since the first IBM PC in 1980. Andrew
diff --git a/drivers/power/reset/restart-poweroff.c b/drivers/power/reset/restart-poweroff.c index 28f1822db1626..e1d94109f6823 100644 --- a/drivers/power/reset/restart-poweroff.c +++ b/drivers/power/reset/restart-poweroff.c @@ -20,7 +20,7 @@ static void restart_poweroff_do_poweroff(void) machine_restart(NULL); } -static int restart_poweroff_probe(struct platform_device *pdev) +static int __init restart_poweroff_init(void) { /* If a pm_power_off function has already been added, leave it alone */ if (pm_power_off != NULL) { @@ -33,12 +33,10 @@ static int restart_poweroff_probe(struct platform_device *pdev) return 0; } -static int restart_poweroff_remove(struct platform_device *pdev) +static void __exit restart_poweroff_exit(void) { if (pm_power_off == &restart_poweroff_do_poweroff) pm_power_off = NULL; - - return 0; } static const struct of_device_id of_restart_poweroff_match[] = { @@ -47,15 +45,8 @@ static const struct of_device_id of_restart_poweroff_match[] = { }; MODULE_DEVICE_TABLE(of, of_restart_poweroff_match); -static struct platform_driver restart_poweroff_driver = { - .probe = restart_poweroff_probe, - .remove = restart_poweroff_remove, - .driver = { - .name = "poweroff-restart", - .of_match_table = of_restart_poweroff_match, - }, -}; -module_platform_driver(restart_poweroff_driver); +module_init(restart_poweroff_init); +module_exit(restart_poweroff_exit); MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch"); MODULE_DESCRIPTION("restart poweroff driver");