Received-SPF: pass (google.com: domain of linux-kernel+bounces-146375-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Peng Fan <peng.fan@nxp.com>
To: Cristian Marussi <cristian.marussi@arm.com>, "Peng Fan (OSS)"
	<peng.fan@oss.nxp.com>
CC: "sudeep.holla@arm.com" <sudeep.holla@arm.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] firmware: arm_scmi: power_control: support suspend
 command
Thread-Topic: [PATCH] firmware: arm_scmi: power_control: support suspend
 command
Thread-Index: AQHajxwygJMjjnYsXEamYB79VvBL0LFpzXAAgACrohA=
Date: Tue, 16 Apr 2024 07:01:42 +0000
Message-ID:
 <DU0PR04MB9417AC020C28AD5847CAB24088082@DU0PR04MB9417.eurprd04.prod.outlook.com>
References: <20240415101141.1591112-1-peng.fan@oss.nxp.com>
 <Zh2R4FZPmVOigfT9@pluto>
In-Reply-To: <Zh2R4FZPmVOigfT9@pluto>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?27kNXSx3LpwcffKxDma6F8037B986rJ484kiFlggqr3QRRbCR3y9kFmp+KbK?=
 =?us-ascii?Q?5Ke4BqqDt8bje2vB5uqqAbfDBef+AGnHVLSRUCRtQ+ExLF9jTF4YXZ8Tpo9Z?=
 =?us-ascii?Q?zWK8pSVcddGhmVWJzxgsT0gJyb7BGCch5avTO/iY1byx6ntSQk24DX9cRDxm?=
 =?us-ascii?Q?nPqY7/MFIzdqEJcA7LcHZWxnK39bOo3XF/1+9GsxiMsX8ck4PsVw4sDY26eC?=
 =?us-ascii?Q?Zyp23CqnhLUSyMhEUw9ZLqsRDmMMzqWumfOphyXThwOzKZ8cmCZEZkGNsbVO?=
 =?us-ascii?Q?CKrncV7bTClrjWViAJsDdpL6hWjQw6KH9aSWd8YCXVLFdIk5EUDehG7338Y1?=
 =?us-ascii?Q?A3kOm4Ml9nMioeTdjekQdqevIZTzaJHsUrVrsODrlCeIlcFnnqrZPZjW2Cqp?=
 =?us-ascii?Q?LxEYs4hEwW4zCJnXxZHdryFonf7O8sbW6mUwUIzLUoXZha3pu3/PWdwn3ii8?=
 =?us-ascii?Q?H+V4f39+2qAT76CzQXgMI/83SXfgJGxC5UY/1O3g8ZBSFqhf3JHgzARbEZ6Z?=
 =?us-ascii?Q?FrbZRmuRsbidVxOBJ5hC1N5rfWQr+y/2JFA5L3DiOHnI7A0VySbGWB6LjR1q?=
 =?us-ascii?Q?D6VBnA7eBGco8ClHWiStjqgYCTKRJThNRFdJoLeKWGWfe/Zq+UalnYsd+PUY?=
 =?us-ascii?Q?AOTOdw5rPr6D/99n87uNiLM4F/qFBZWNfIDvf/0EkraGC+i3Uo2pFDsYs/mR?=
 =?us-ascii?Q?M9PftI3gh4G2IEgoM9zmV/qMwIvHNZyfNN2l70TBOzGM5A79SiYlIQb1iaFe?=
 =?us-ascii?Q?rwfJByx2B9DNprr701egAS+EMj++Few62OiJlrQfep9yx5rWHRqdmpVy0lCG?=
 =?us-ascii?Q?jFF4vjDV096d9zrrz61B0DsN6rX4ZS+dq9pr1bMxggJerYi4VYwF3/Bc93sD?=
 =?us-ascii?Q?+BnlO3b7+/udSKUt9FLSnNOJXPrX+nXKrXNeAx6jkgnfG0yGoZ+BacCiSPHx?=
 =?us-ascii?Q?cci25Q4wrRDUoGKwRDPr4ZBfhedOnrYsIhqzjVgHrFt3Yu5o9JspnASxe/f+?=
 =?us-ascii?Q?Y04U5s4iVWC6B9d7B2xyBHXu3M5iKAZK5///fy0LWD4pgMphVfDkr0jitB8g?=
 =?us-ascii?Q?LW4VTl+3A+YhjVJl5F1E2kAnO0uL9KNhXCg2KbldMHJZThOQQMARWOFDZtNq?=
 =?us-ascii?Q?c66W0KhNp3DngOXO77Q8C9QTYoS1dT0jRYc5QdmkAmE6S/qMa+92XOzRVhRz?=
 =?us-ascii?Q?TMevOlNrSPN8lbnlqSPEPi2KHfHhdXz/J0vAi4L5mqYC2TuKyR5XZNMR83lO?=
 =?us-ascii?Q?tzt9ASuWcwYNop3Aaq4vvfyi5S8/EmuynSzpDqjsg/cZLHtT9srsPCOQBDcW?=
 =?us-ascii?Q?rkRi9SlRtPUqz0q7goPv2TS91T15hcLX0I7bgl8jEuFS3iscdxEAdJGp8Tdn?=
 =?us-ascii?Q?EQrcdZr2W9RCC0AslN29mp08Dh4txTwAsjFxAz8hoLUJLipE067sx4JZYGMR?=
 =?us-ascii?Q?x7DkqEA/enmf1/Jxa/tO3mHPZtzAM794M23XugXsyMySoCCE1F5u9Ql3FBIf?=
 =?us-ascii?Q?EY+kfIdPDk9TmjncXTS/9mFeP0xrf+Z/7AxH+2Jm+4qK57XLns2ECkJGW8Qh?=
 =?us-ascii?Q?JoaXn6EeKUfdai+9m3M=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DU0PR04MB9417.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bdb67739-d3bc-4bfa-198f-08dc5de3122a
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Apr 2024 07:01:42.3416
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: LH6Gv1oyHsvbkHlucQdRtEyZjAeqYpFEtok1lstGLuBNQS5fYNcfiHXD8hX7Lz69tdJVHVaYv7hnzYSXlPaIuA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: GV1PR04MB10197

Hi Cristian,

> Subject: Re: [PATCH] firmware: arm_scmi: power_control: support suspend
> command
>=20
> On Mon, Apr 15, 2024 at 06:11:41PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Support System suspend notification. Using a work struct to call
> > pm_suspend. There is no way to pass suspend level to pm_suspend, so
> > use MEM as of now.
> >
>=20
> Hi Peng,
>=20
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  .../firmware/arm_scmi/scmi_power_control.c    | 20 ++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> >
>=20
> This LGTM in general, the only obsservation here is that while on shutdow=
n
> by issuing a orderly_reboot() we in fact ask PID_1 to start a shutdown fr=
om

Would you please share why PID_1 is involved here? orderly_reboot is just
schedule a work.
> userspace, when triggering a system suspend with pm_suspend() we do not
> involve userspace in the process, but I dont think there is another way i=
ndeed.

Userspace process should not be involved, otherwise the freezing process wi=
ll
never finish, and suspend will abort.

Thanks,
Peng.

>=20
> Thanks,
> Cristian
>=20
> > diff --git a/drivers/firmware/arm_scmi/scmi_power_control.c
> > b/drivers/firmware/arm_scmi/scmi_power_control.c
> > index 6eb7d2a4b6b1..beafca9957c7 100644
> > --- a/drivers/firmware/arm_scmi/scmi_power_control.c
> > +++ b/drivers/firmware/arm_scmi/scmi_power_control.c
> > @@ -50,6 +50,7 @@
> >  #include <linux/reboot.h>
> >  #include <linux/scmi_protocol.h>
> >  #include <linux/slab.h>
> > +#include <linux/suspend.h>
> >  #include <linux/time64.h>
> >  #include <linux/timer.h>
> >  #include <linux/types.h>
> > @@ -90,6 +91,7 @@ struct scmi_syspower_conf {
> >  	struct notifier_block reboot_nb;
> >
> >  	struct delayed_work forceful_work;
> > +	struct work_struct suspend_work;
> >  };
> >
> >  #define userspace_nb_to_sconf(x)	\
> > @@ -249,6 +251,9 @@ static void scmi_request_graceful_transition(struct
> scmi_syspower_conf *sc,
> >  	case SCMI_SYSTEM_WARMRESET:
> >  		orderly_reboot();
> >  		break;
> > +	case SCMI_SYSTEM_SUSPEND:
> > +		schedule_work(&sc->suspend_work);
> > +		break;
> >  	default:
> >  		break;
> >  	}
> > @@ -277,7 +282,8 @@ static int scmi_userspace_notifier(struct
> notifier_block *nb,
> >  	struct scmi_system_power_state_notifier_report *er =3D data;
> >  	struct scmi_syspower_conf *sc =3D userspace_nb_to_sconf(nb);
> >
> > -	if (er->system_state >=3D SCMI_SYSTEM_POWERUP) {
> > +	if (er->system_state >=3D SCMI_SYSTEM_MAX ||
> > +	    er->system_state =3D=3D SCMI_SYSTEM_POWERUP) {
> >  		dev_err(sc->dev, "Ignoring unsupported system_state:
> 0x%X\n",
> >  			er->system_state);
> >  		return NOTIFY_DONE;
> > @@ -315,6 +321,16 @@ static int scmi_userspace_notifier(struct
> notifier_block *nb,
> >  	return NOTIFY_OK;
> >  }
> >
> > +static void scmi_suspend_work_func(struct work_struct *work) {
> > +	struct scmi_syspower_conf *sc =3D
> > +		container_of(work, struct scmi_syspower_conf,
> suspend_work);
> > +
> > +	pm_suspend(PM_SUSPEND_MEM);
> > +
> > +	sc->state =3D SCMI_SYSPOWER_IDLE;
> > +}
> > +
> >  static int scmi_syspower_probe(struct scmi_device *sdev)  {
> >  	int ret;
> > @@ -338,6 +354,8 @@ static int scmi_syspower_probe(struct scmi_device
> *sdev)
> >  	sc->userspace_nb.notifier_call =3D &scmi_userspace_notifier;
> >  	sc->dev =3D &sdev->dev;
> >
> > +	INIT_WORK(&sc->suspend_work, scmi_suspend_work_func);
> > +
> >  	return handle->notify_ops->devm_event_notifier_register(sdev,
> >
> SCMI_PROTOCOL_SYSTEM,
> >
> SCMI_EVENT_SYSTEM_POWER_STATE_NOTIFIER,
> > --
> > 2.37.1
> >