RE: [PATCH v2 3/6] firmware: arm_scmi: add initial support for i.MX BBM protocol

[Peng Fan]

> 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@xxxxxxx>
> >
> > 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