linux-kernel - RE: [PATCH v2 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]

Message-ID:
 <DU0PR04MB9417DCC510F2A08F3C6B1D8B88002@DU0PR04MB9417.eurprd04.prod.outlook.com>
Date: Mon, 8 Apr 2024 23:35:16 +0000
From: Peng Fan <peng.fan@....com>
To: Cristian Marussi <cristian.marussi@....com>, "Peng Fan (OSS)"
	<peng.fan@....nxp.com>
CC: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, 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>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 3/6] firmware: arm_scmi: add initial support for i.MX
 BBM protocol

> Subject: Re: [PATCH v2 3/6] firmware: arm_scmi: add initial support for iMX
> BBM protocol
> 
> On Fri, Apr 05, 2024 at 08:39:25PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@....com>
> >
> > The i.MX BBM protocol is for managing i.MX BBM module which provides
> > RTC and BUTTON feature.
> >

....
> > +#include "notify.h"
> > +
> > +#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x10000
> > +
> 
> I appreciate that you added versioning but I think a bit of documentation
> about what the protocol and its comamnds purpose is still lacking, as asked
> by Sudeep previously

Sorry for missing the previous comment. Will add some comments in the file.

> 
> 	https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%
> 2Flore.kernel.org%2Flinux-arm-
> kernel%2FZeGtoJ7ztSe8Kg8R%40bogus%2F%23t&data=05%7C02%7Cpeng.fa
> n%40nxp.com%7C37b12c01b51f4329e9e308dc57f66153%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C638481962901820964%7CUnknown
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> WwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=d2XRugSYyiFuUnE5R2Oz6p
> xmXBaPC9lZ%2Bb%2FcMBuXeKo%3D&reserved=0
> 
> > +enum scmi_imx_bbm_protocol_cmd {
> > +	IMX_BBM_GPR_SET = 0x3,

...
> > +	cfg->flags = 0;
> > +	cfg->value_low = lower_32_bits(sec);
> > +	cfg->value_high = upper_32_bits(sec);
> 
> Sorry I may have not been clear on this, but when I mentioned lower/upper
> helpers I did not mean that they solved ALSO the endianity problem, so I
> suppose that after having chunked your 64bits value in 2, you still want to
> transmit it as 2 LE quantity....this is generally the expectation for SCMI
> payloads...in this case any available documentation about the expected
> command layout would have helped...

Got it , will fix in v3.

> 
> > +
> > +	ret = ph->xops->do_xfer(ph, t);
> > +
> > +	ph->xops->xfer_put(ph, t);
> > +
> > +	return ret;
> > +}
> > +
> > +static int scmi_imx_bbm_rtc_time_get(const struct scmi_protocol_handle
> *ph,
> > +				     u32 rtc_id, u64 *value)
> > +{
> > +	struct scmi_imx_bbm_info *pi = ph->get_priv(ph);
> > +	struct scmi_imx_bbm_get_time *cfg;
> > +	struct scmi_xfer *t;
> > +	int ret;
> > +
> > +	if (rtc_id >= pi->nr_rtc)
> > +		return -EINVAL;
> > +
> > +	ret = ph->xops->xfer_get_init(ph, IMX_BBM_RTC_TIME_GET,
> sizeof(*cfg),
> > +				      sizeof(u64), &t);
> > +	if (ret)
> > +		return ret;
> > +
> > +	cfg = t->tx.buf;
> > +	cfg->id = cpu_to_le32(rtc_id);
> > +	cfg->flags = 0;
> > +
> > +	ret = ph->xops->do_xfer(ph, t);
> > +	if (!ret)
> > +		*value = get_unaligned_le64(t->rx.buf);
> > +
> > +	ph->xops->xfer_put(ph, t);
> > +
> > +	return ret;
> > +}
> > +
> > +static int scmi_imx_bbm_rtc_alarm_set(const struct scmi_protocol_handle
> *ph,
> > +				      u32 rtc_id, u64 sec)
> > +{
> > +	struct scmi_imx_bbm_info *pi = ph->get_priv(ph);
> > +	struct scmi_imx_bbm_alarm_time *cfg;
> > +	struct scmi_xfer *t;
> > +	int ret;
> > +
> > +	if (rtc_id >= pi->nr_rtc)
> > +		return -EINVAL;
> > +
> > +	ret = ph->xops->xfer_get_init(ph, IMX_BBM_RTC_ALARM_SET,
> sizeof(*cfg), 0, &t);
> > +	if (ret)
> > +		return ret;
> > +
> > +	cfg = t->tx.buf;
> > +	cfg->id = cpu_to_le32(rtc_id);
> > +	cfg->flags = SCMI_IMX_BBM_RTC_ALARM_ENABLE_FLAG;
> > +	cfg->value_low = lower_32_bits(sec);
> > +	cfg->value_high = upper_32_bits(sec);
> 
> Same.

Fix in V3.

Thanks,
Peng
> 
> Thanks,
> Cristian