linux-kernel - RE: [PATCH v4 3/6] firmware: arm_scmi: add initial support for i.MX BBM protocol

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 20 Jun 2024 03:13:30 +0000
From: Peng Fan <peng.fan@....com>
To: Cristian Marussi <cristian.marussi@....com>, "Peng Fan (OSS)"
	<peng.fan@....nxp.com>
CC: Jonathan Corbet <corbet@....net>, Shawn Guo <shawnguo@...nel.org>, Sascha
 Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team
	<kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>, Sudeep Holla
	<sudeep.holla@....com>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>
Subject: RE: [PATCH v4 3/6] firmware: arm_scmi: add initial support for i.MX
 BBM protocol

> Subject: Re: [PATCH v4 3/6] firmware: arm_scmi: add initial support for
> i.MX BBM protocol
> 
> On Fri, May 24, 2024 at 04:56:45PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@....com>
> >
> > i.MX95 has a battery-backed module(BBM), which has persistent
> storage
> > (GPR), an RTC, and the ON/OFF button. The System Manager(SM)
> firmware
> > use SCMI vendor protocol(SCMI BBM) to let agent be able to use GPR,
> > RTC and ON/OFF button.
> >
> > Signed-off-by: Peng Fan <peng.fan@....com>
> > ---
> >  drivers/firmware/arm_scmi/Kconfig          |   2 +
> >  drivers/firmware/arm_scmi/Makefile         |   1 +
> >  drivers/firmware/arm_scmi/imx/Kconfig      |  14 ++
> >  drivers/firmware/arm_scmi/imx/Makefile     |   2 +
> >  drivers/firmware/arm_scmi/imx/imx-sm-bbm.c | 380
> +++++++++++++++++++++++++++++
> >  include/linux/scmi_imx_protocol.h          |  42 ++++
> >  6 files changed, 441 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/Kconfig
> > b/drivers/firmware/arm_scmi/Kconfig
> > index aa5842be19b2..79846cbaf71b 100644
> > --- a/drivers/firmware/arm_scmi/Kconfig
> > +++ b/drivers/firmware/arm_scmi/Kconfig
> > @@ -180,4 +180,6 @@ config ARM_SCMI_POWER_CONTROL
> >  	  called scmi_power_control. Note this may needed early in
> boot to catch
> >  	  early shutdown/reboot SCMI requests.
> >
> > +source "drivers/firmware/arm_scmi/imx/Kconfig"
> > +
> 
> It could be that we fold all the Vendor drivers under
> drivers/firmware/arm_scmi/vendors once it is merged...but we will
> take care of this reowrk/refctor...still not sure about this details.

ok. Sudeep may comment more on this.

> 
> >  endmenu
> > diff --git a/drivers/firmware/arm_scmi/Makefile
> > b/drivers/firmware/arm_scmi/Makefile
> > index fd59f58ce8a2..fb9407fef60c 100644
> > --- a/drivers/firmware/arm_scmi/Makefile
> > +++ b/drivers/firmware/arm_scmi/Makefile

.....
> > +
> > +	ret = scmi_imx_bbm_attributes_get(ph, binfo);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return ph->set_priv(ph, binfo, version); }
> 
> I would move this init down below, right before the scmi_imx_bbm and
> after the proto_ops definition,  for consistency and readability.

Yeah. Fix in v5.

> 
> > +

....
> > +	.button_get = scmi_imx_bbm_button_get, };
> > +
> 
> ...just here the init
> 
> > +static const struct scmi_protocol scmi_imx_bbm = {
> > +	.id = SCMI_PROTOCOL_IMX_BBM,
> > +	.owner = THIS_MODULE,
> > +	.instance_init = &scmi_imx_bbm_protocol_init,
> > +	.ops = &scmi_imx_bbm_proto_ops,
> > +	.events = &scmi_imx_bbm_protocol_events,
> > +	.supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
> > +	.vendor_id = "NXP",
> > +	.sub_vendor_id = "i.MX95 EVK",
> > +};
> > +
> 
> Beside this, LGTM.
> 
> Reviewed-by: Cristian Marussi <cristian.marussi@....com>

Thanks for help reviewing the patches.

Thanks,
Peng.

> 
> Thanks,
> Cristian