Re: [PATCH v5 2/3] firmware: arm_scmi: Add SCMI System Power Control driver

[Cristian Marussi]

Hi Sudeep

thanks for the review.

On Mon, Sep 07, 2020 at 05:43:51PM +0100, Sudeep Holla wrote:
> On Wed, Aug 19, 2020 at 05:10:01PM +0100, Cristian Marussi wrote:
> > Add an SCMI System Power control driver to handle platform's requests
> > carried by SYSTEM_POWER_STATE_NOTIFIER notifications: such platform
> > requested system power state transitions are handled accordingly,
> > gracefully or forcefully, depending on the notifications' message flags.
> > 
> > Graceful requests are by default relayed to userspace using the same
> > Kernel API used to handle ACPI Shutdown bus events: alternatively, instead,
> > a few available module parameters can be used to tunnel instead such
> > requests to userspace via signals addressed to CAD pid.
> > 
> > When handling graceful requests, grant userspace processes a maximum
> > (configurable) time to perform their duties and then revert to a forceful
> > transition, so avoiding completely timing out platform's maximum grace time
> > and hitting possible abrupt power-cuts.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> > ----
> > V3 --> V4
> > - fix alignment checkpatch issues
> > V2 --> V3
> > - changed Kconfig to fix naming and defaulting to n
> > V1 --> V2
> > - use common event enums
> > - introduced optioanl alternative signal based comms 2 userspace
> > ---
> >  drivers/firmware/Kconfig                      |  12 +
> >  drivers/firmware/arm_scmi/Makefile            |   1 +
> >  drivers/firmware/arm_scmi/driver.c            |   1 +
> >  .../firmware/arm_scmi/scmi_power_control.c    | 389 ++++++++++++++++++
> >  4 files changed, 403 insertions(+)
> >  create mode 100644 drivers/firmware/arm_scmi/scmi_power_control.c
> > 
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index fbd785dd0513..a77545c08040 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -40,6 +40,18 @@ config ARM_SCMI_POWER_DOMAIN
> >  	  will be called scmi_pm_domain. Note this may needed early in boot
> >  	  before rootfs may be available.
> >  
> > +config ARM_SCMI_POWER_CONTROL
> > +	bool "SCMI system power control driver"
> > +	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> > +	default n
> > +	help
> > +	  This enables System Power control logic which binds system shutdown or
> > +	  reboot actions to SCMI System Power notifications generated by SCP
> > +	  firmware.
> > +
> > +	  Graceful requests' methods and timeout and can be configured using
> > +	  a few available module parameters.
> > +
> >  config ARM_SCPI_PROTOCOL
> >  	tristate "ARM System Control and Power Interface (SCPI) Message Protocol"
> >  	depends on ARM || ARM64 || COMPILE_TEST
> > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> > index 643f2320f976..6a0e24f9e0b1 100644
> > --- a/drivers/firmware/arm_scmi/Makefile
> > +++ b/drivers/firmware/arm_scmi/Makefile
> > @@ -7,3 +7,4 @@ scmi-transport-$(CONFIG_MAILBOX) += mailbox.o
> >  scmi-transport-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smc.o
> >  scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o
> >  obj-$(CONFIG_ARM_SCMI_POWER_DOMAIN) += scmi_pm_domain.o
> > +obj-$(CONFIG_ARM_SCMI_POWER_CONTROL) += scmi_power_control.o
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index f749af6e79fc..484b3f7126b5 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -732,6 +732,7 @@ struct scmi_prot_devnames {
> >  
> >  static struct scmi_prot_devnames devnames[] = {
> >  	{ SCMI_PROTOCOL_POWER,  { "genpd" },},
> > +	{ SCMI_PROTOCOL_SYSTEM, {"syspower" },},
> 
> 				^ missing space
> 
> Split the above into separate patch as it doesn't depend on new config
> and can be added without it. I plan to merge the scmi system protocol
> part for v5.10 and the above addition of device. We need to wait for more
> feedback on the driver front especially user notification part.
> 

I'll do in v6.

Cristian

> -- 
> Regards,
> Sudeep