[PATCH v2 0/6] firmware: support i.MX95 SCMI BBM/MISC Extenstion

[PATCH v2 0/6] firmware: support i.MX95 SCMI BBM/MISC Extenstion

[Peng Fan (OSS)]

i.MX95 System Manager Firmware support vendor extension protocol:
- Battery Backed Module(BBM) Protocol for RTC and BUTTON feature.
- MISC Protocol for misc settings, such as BLK CTRL GPR settings, GPIO
expander settings.

This patchset is to support the two protocols and users that use the
protocols.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Shawn Guo <shawnguo@kernel.org>
To: Sascha Hauer <s.hauer@pengutronix.de>
To: Pengutronix Kernel Team <kernel@pengutronix.de>
To: Fabio Estevam <festevam@gmail.com>
To: Peng Fan <peng.fan@nxp.com>
To: Sudeep Holla <sudeep.holla@arm.com>
To: Cristian Marussi <cristian.marussi@arm.com>
Cc: devicetree@vger.kernel.org
Cc: imx@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

Changes in v2:
- Sorry for late update since v1.
- Add a new patch 1
- Address imx,scmi.yaml issues
- Address comments for imx-sm-bbm.c and imx-sm-misc.c
- I not add vendor id since related patches not landed in linux-next.
- Link to v1: https://lore.kernel.org/r/20240202-imx95-bbm-misc-v1-0-3cb743020933@nxp.com

---
Peng Fan (6):
      dt-bindings: firmware: arm,scmi: set additionalProperties to true
      dt-bindings: firmware: add i.MX SCMI Extension protocol
      firmware: arm_scmi: add initial support for i.MX BBM protocol
      firmware: arm_scmi: add initial support for i.MX MISC protocol
      firmware: imx: support BBM module
      firmware: imx: add i.MX95 MISC driver

 .../devicetree/bindings/firmware/arm,scmi.yaml     |   2 +-
 .../devicetree/bindings/firmware/imx,scmi.yaml     |  80 +++++
 drivers/firmware/arm_scmi/Kconfig                  |  20 ++
 drivers/firmware/arm_scmi/Makefile                 |   2 +
 drivers/firmware/arm_scmi/imx-sm-bbm.c             | 378 +++++++++++++++++++++
 drivers/firmware/arm_scmi/imx-sm-misc.c            | 305 +++++++++++++++++
 drivers/firmware/imx/Makefile                      |   2 +
 drivers/firmware/imx/sm-bbm.c                      | 317 +++++++++++++++++
 drivers/firmware/imx/sm-misc.c                     |  92 +++++
 include/linux/firmware/imx/sm.h                    |  33 ++
 include/linux/scmi_imx_protocol.h                  |  62 ++++
 11 files changed, 1292 insertions(+), 1 deletion(-)
---
base-commit: 2b3d5988ae2cb5cd945ddbc653f0a71706231fdd
change-id: 20240405-imx95-bbm-misc-v2-b5e9d24adc42

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

When adding vendor extension protocols, there is dt-schema warning:
"
imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do not match any
of the regexes: 'pinctrl-[0-9]+'
"

Set additionalProperties to true to address the issue.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 4591523b51a0..cfc613b65585 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -247,7 +247,7 @@ properties:
       reg:
         const: 0x18
 
-additionalProperties: false
+additionalProperties: true
 
 $defs:
   protocol-node:

-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension protocol

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Add i.MX SCMI Extension protocols bindings for:
 - Battery Backed Secure Module(BBSM)
 - MISC settings such as General Purpose Registers settings.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 .../devicetree/bindings/firmware/imx,scmi.yaml     | 80 ++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/imx,scmi.yaml b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
new file mode 100644
index 000000000000..7ee19a661d83
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2024 NXP
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/firmware/imx,scmi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: i.MX System Control and Management Interface(SCMI) Vendor Protocols Extension
+
+maintainers:
+  - Peng Fan <peng.fan@nxp.com>
+
+allOf:
+  - $ref: arm,scmi.yaml#
+
+properties:
+  protocol@81:
+    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
+    unevaluatedProperties: false
+    description:
+      The BBM Protocol is for managing Battery Backed Secure Module (BBSM) RTC
+      and the ON/OFF Key
+
+    properties:
+      reg:
+        const: 0x81
+
+    required:
+      - reg
+
+  protocol@84:
+    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
+    unevaluatedProperties: false
+    description:
+      The MISC Protocol is for managing SoC Misc settings, such as GPR settings
+
+    properties:
+      reg:
+        const: 0x84
+
+      wakeup-sources:
+        description:
+          Each entry consists of 2 integers, represents the source and electric signal edge
+        items:
+          items:
+            - description: the wakeup source
+            - description: the wakeup electric signal edge
+        $ref: /schemas/types.yaml#/definitions/uint32-matrix
+
+    required:
+      - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    firmware {
+        scmi {
+            compatible = "arm,scmi";
+            mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>;
+            shmem = <&scmi_buf0>, <&scmi_buf1>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            protocol@81 {
+                reg = <0x81>;
+            };
+
+            protocol@84 {
+                reg = <0x84>;
+                wakeup-sources = <0x8000 1
+                                  0x8001 1
+                                  0x8002 1
+                                  0x8003 1
+                                  0x8004 1>;
+            };
+         };
+    };
+...

-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

The i.MX BBM protocol is for managing i.MX BBM module which provides
RTC and BUTTON feature.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/arm_scmi/Kconfig      |  10 +
 drivers/firmware/arm_scmi/Makefile     |   1 +
 drivers/firmware/arm_scmi/imx-sm-bbm.c | 378 +++++++++++++++++++++++++++++++++
 include/linux/scmi_imx_protocol.h      |  45 ++++
 4 files changed, 434 insertions(+)

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index aa5842be19b2..56d11c9d9f47 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -181,3 +181,13 @@ config ARM_SCMI_POWER_CONTROL
 	  early shutdown/reboot SCMI requests.
 
 endmenu
+
+config IMX_SCMI_BBM_EXT
+	tristate "i.MX SCMI BBM EXTENSION"
+	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
+	default y if ARCH_MXC
+	help
+	  This enables i.MX System BBM control logic which supports RTC
+	  and BUTTON.
+
+	  This driver can also be built as a module.
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index a7bc4796519c..327687acf857 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -11,6 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
 scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
 scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
 scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
+scmi-protocols-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.o
 scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
 
 obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
diff --git a/drivers/firmware/arm_scmi/imx-sm-bbm.c b/drivers/firmware/arm_scmi/imx-sm-bbm.c
new file mode 100644
index 000000000000..92c0aedf65cc
--- /dev/null
+++ b/drivers/firmware/arm_scmi/imx-sm-bbm.c
@@ -0,0 +1,378 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) NXP BBM Protocol
+ *
+ * Copyright 2024 NXP
+ */
+
+#define pr_fmt(fmt) "SCMI Notifications BBM - " fmt
+
+#include <linux/bits.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/scmi_protocol.h>
+#include <linux/scmi_imx_protocol.h>
+
+#include "protocols.h"
+#include "notify.h"
+
+#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x10000
+
+enum scmi_imx_bbm_protocol_cmd {
+	IMX_BBM_GPR_SET = 0x3,
+	IMX_BBM_GPR_GET = 0x4,
+	IMX_BBM_RTC_ATTRIBUTES = 0x5,
+	IMX_BBM_RTC_TIME_SET = 0x6,
+	IMX_BBM_RTC_TIME_GET = 0x7,
+	IMX_BBM_RTC_ALARM_SET = 0x8,
+	IMX_BBM_BUTTON_GET = 0x9,
+	IMX_BBM_RTC_NOTIFY = 0xA,
+	IMX_BBM_BUTTON_NOTIFY = 0xB,
+};
+
+#define GET_RTCS_NR(x)	le32_get_bits((x), GENMASK(23, 16))
+#define GET_GPRS_NR(x)	le32_get_bits((x), GENMASK(15, 0))
+
+#define SCMI_IMX_BBM_NOTIFY_RTC_UPDATED		BIT(2)
+#define SCMI_IMX_BBM_NOTIFY_RTC_ROLLOVER	BIT(1)
+#define SCMI_IMX_BBM_NOTIFY_RTC_ALARM		BIT(0)
+
+#define SCMI_IMX_BBM_RTC_ALARM_ENABLE_FLAG	BIT(0)
+
+#define SCMI_IMX_BBM_NOTIFY_RTC_FLAG	\
+	(SCMI_IMX_BBM_NOTIFY_RTC_UPDATED | SCMI_IMX_BBM_NOTIFY_RTC_ROLLOVER | \
+	 SCMI_IMX_BBM_NOTIFY_RTC_ALARM)
+
+#define SCMI_IMX_BBM_EVENT_RTC_MASK		GENMASK(31, 24)
+
+struct scmi_imx_bbm_info {
+	u32 version;
+	int nr_rtc;
+	int nr_gpr;
+};
+
+struct scmi_msg_imx_bbm_protocol_attributes {
+	__le32 attributes;
+};
+
+struct scmi_imx_bbm_set_time {
+	__le32 id;
+	__le32 flags;
+	__le32 value_low;
+	__le32 value_high;
+};
+
+struct scmi_imx_bbm_get_time {
+	__le32 id;
+	__le32 flags;
+};
+
+struct scmi_imx_bbm_alarm_time {
+	__le32 id;
+	__le32 flags;
+	__le32 value_low;
+	__le32 value_high;
+};
+
+struct scmi_msg_imx_bbm_rtc_notify {
+	__le32 rtc_id;
+	__le32 flags;
+};
+
+struct scmi_msg_imx_bbm_button_notify {
+	__le32 flags;
+};
+
+struct scmi_imx_bbm_notify_payld {
+	__le32 flags;
+};
+
+static int scmi_imx_bbm_attributes_get(const struct scmi_protocol_handle *ph,
+				       struct scmi_imx_bbm_info *pi)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_imx_bbm_protocol_attributes *attr;
+
+	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, sizeof(*attr), &t);
+	if (ret)
+		return ret;
+
+	attr = t->rx.buf;
+
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret) {
+		pi->nr_rtc = GET_RTCS_NR(attr->attributes);
+		pi->nr_gpr = GET_GPRS_NR(attr->attributes);
+	}
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int scmi_imx_bbm_notify(const struct scmi_protocol_handle *ph,
+			       u32 src_id, int message_id, bool enable)
+{
+	int ret;
+	struct scmi_xfer *t;
+
+	if (message_id == IMX_BBM_RTC_NOTIFY) {
+		struct scmi_msg_imx_bbm_rtc_notify *rtc_notify;
+
+		ret = ph->xops->xfer_get_init(ph, message_id,
+					      sizeof(*rtc_notify), 0, &t);
+		if (ret)
+			return ret;
+
+		rtc_notify = t->tx.buf;
+		rtc_notify->rtc_id = cpu_to_le32(0);
+		rtc_notify->flags =
+			cpu_to_le32(enable ? SCMI_IMX_BBM_NOTIFY_RTC_FLAG : 0);
+	} else if (message_id == IMX_BBM_BUTTON_NOTIFY) {
+		struct scmi_msg_imx_bbm_button_notify *button_notify;
+
+		ret = ph->xops->xfer_get_init(ph, message_id,
+					      sizeof(*button_notify), 0, &t);
+		if (ret)
+			return ret;
+
+		button_notify = t->tx.buf;
+		button_notify->flags = cpu_to_le32(enable ? 1 : 0);
+	} else {
+		return -EINVAL;
+	}
+
+	ret = ph->xops->do_xfer(ph, t);
+
+	ph->xops->xfer_put(ph, t);
+	return ret;
+}
+
+static enum scmi_imx_bbm_protocol_cmd evt_2_cmd[] = {
+	IMX_BBM_RTC_NOTIFY,
+	IMX_BBM_BUTTON_NOTIFY
+};
+
+static int scmi_imx_bbm_set_notify_enabled(const struct scmi_protocol_handle *ph,
+					   u8 evt_id, u32 src_id, bool enable)
+{
+	int ret, cmd_id;
+
+	if (evt_id >= ARRAY_SIZE(evt_2_cmd))
+		return -EINVAL;
+
+	cmd_id = evt_2_cmd[evt_id];
+	ret = scmi_imx_bbm_notify(ph, src_id, cmd_id, enable);
+	if (ret)
+		pr_debug("FAIL_ENABLED - evt[%X] dom[%d] - ret:%d\n",
+			 evt_id, src_id, ret);
+
+	return ret;
+}
+
+static void *scmi_imx_bbm_fill_custom_report(const struct scmi_protocol_handle *ph,
+					     u8 evt_id, ktime_t timestamp,
+					     const void *payld, size_t payld_sz,
+					     void *report, u32 *src_id)
+{
+	const struct scmi_imx_bbm_notify_payld *p = payld;
+	struct scmi_imx_bbm_notif_report *r = report;
+
+	if (sizeof(*p) != payld_sz)
+		return NULL;
+
+	if (evt_id == SCMI_EVENT_IMX_BBM_RTC) {
+		r->is_rtc = true;
+		r->is_button = false;
+		r->timestamp = timestamp;
+		r->rtc_id = le32_get_bits(p->flags, SCMI_IMX_BBM_EVENT_RTC_MASK);
+		r->rtc_evt = le32_get_bits(p->flags, SCMI_IMX_BBM_NOTIFY_RTC_FLAG);
+		dev_dbg(ph->dev, "RTC: %d evt: %x\n", r->rtc_id, r->rtc_evt);
+		*src_id = r->rtc_evt;
+	} else if (evt_id == SCMI_EVENT_IMX_BBM_BUTTON) {
+		r->is_rtc = false;
+		r->is_button = true;
+		r->timestamp = timestamp;
+		dev_dbg(ph->dev, "BBM Button\n");
+		*src_id = 0;
+	} else {
+		WARN_ON_ONCE(1);
+		return NULL;
+	}
+
+	return r;
+}
+
+static const struct scmi_event scmi_imx_bbm_events[] = {
+	{
+		.id = SCMI_EVENT_IMX_BBM_RTC,
+		.max_payld_sz = sizeof(struct scmi_imx_bbm_notify_payld),
+		.max_report_sz = sizeof(struct scmi_imx_bbm_notif_report),
+	},
+	{
+		.id = SCMI_EVENT_IMX_BBM_BUTTON,
+		.max_payld_sz = sizeof(struct scmi_imx_bbm_notify_payld),
+		.max_report_sz = sizeof(struct scmi_imx_bbm_notif_report),
+	},
+};
+
+static const struct scmi_event_ops scmi_imx_bbm_event_ops = {
+	.set_notify_enabled = scmi_imx_bbm_set_notify_enabled,
+	.fill_custom_report = scmi_imx_bbm_fill_custom_report,
+};
+
+static const struct scmi_protocol_events scmi_imx_bbm_protocol_events = {
+	.queue_sz = SCMI_PROTO_QUEUE_SZ,
+	.ops = &scmi_imx_bbm_event_ops,
+	.evts = scmi_imx_bbm_events,
+	.num_events = ARRAY_SIZE(scmi_imx_bbm_events),
+	.num_sources = 1,
+};
+
+static int scmi_imx_bbm_protocol_init(const struct scmi_protocol_handle *ph)
+{
+	u32 version;
+	int ret;
+	struct scmi_imx_bbm_info *binfo;
+
+	ret = ph->xops->version_get(ph, &version);
+	if (ret)
+		return ret;
+
+	dev_info(ph->dev, "NXP SM BBM Version %d.%d\n",
+		 PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
+
+	binfo = devm_kzalloc(ph->dev, sizeof(*binfo), GFP_KERNEL);
+	if (!binfo)
+		return -ENOMEM;
+
+	ret = scmi_imx_bbm_attributes_get(ph, binfo);
+	if (ret)
+		return ret;
+
+	return ph->set_priv(ph, binfo, version);
+}
+
+static int scmi_imx_bbm_rtc_time_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_set_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_SET, sizeof(*cfg), 0, &t);
+	if (ret)
+		return ret;
+
+	cfg = t->tx.buf;
+	cfg->id = cpu_to_le32(rtc_id);
+	cfg->flags = 0;
+	cfg->value_low = lower_32_bits(sec);
+	cfg->value_high = upper_32_bits(sec);
+
+	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);
+
+	ret = ph->xops->do_xfer(ph, t);
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int scmi_imx_bbm_button_get(const struct scmi_protocol_handle *ph, u32 *state)
+{
+	struct scmi_xfer *t;
+	int ret;
+
+	ret = ph->xops->xfer_get_init(ph, IMX_BBM_BUTTON_GET, 0, sizeof(u32), &t);
+	if (ret)
+		return ret;
+
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret)
+		*state = get_unaligned_le32(t->rx.buf);
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static const struct scmi_imx_bbm_proto_ops scmi_imx_bbm_proto_ops = {
+	.rtc_time_get = scmi_imx_bbm_rtc_time_get,
+	.rtc_time_set = scmi_imx_bbm_rtc_time_set,
+	.rtc_alarm_set = scmi_imx_bbm_rtc_alarm_set,
+	.button_get = scmi_imx_bbm_button_get,
+};
+
+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,
+};
+
+module_scmi_protocol(scmi_imx_bbm);
diff --git a/include/linux/scmi_imx_protocol.h b/include/linux/scmi_imx_protocol.h
new file mode 100644
index 000000000000..90ce011a4429
--- /dev/null
+++ b/include/linux/scmi_imx_protocol.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * SCMI Message Protocol driver NXP extension header
+ *
+ * Copyright 2024 NXP.
+ */
+
+#ifndef _LINUX_SCMI_NXP_PROTOCOL_H
+#define _LINUX_SCMI_NXP_PROTOCOL_H
+
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/notifier.h>
+#include <linux/types.h>
+
+enum scmi_nxp_protocol {
+	SCMI_PROTOCOL_IMX_BBM = 0x81,
+};
+
+struct scmi_imx_bbm_proto_ops {
+	int (*rtc_time_set)(const struct scmi_protocol_handle *ph, u32 id,
+			    uint64_t sec);
+	int (*rtc_time_get)(const struct scmi_protocol_handle *ph, u32 id,
+			    u64 *val);
+	int (*rtc_alarm_set)(const struct scmi_protocol_handle *ph, u32 id,
+			     u64 sec);
+	int (*button_get)(const struct scmi_protocol_handle *ph, u32 *state);
+};
+
+enum scmi_nxp_notification_events {
+	SCMI_EVENT_IMX_BBM_RTC = 0x0,
+	SCMI_EVENT_IMX_BBM_BUTTON = 0x1,
+	SCMI_EVENT_IMX_MISC_CONTROL_DISABLED = 0x0,
+	SCMI_EVENT_IMX_MISC_CONTROL_FALLING_EDGE = 0x1,
+	SCMI_EVENT_IMX_MISC_CONTROL_RISING_EDGE = 0x2,
+};
+
+struct scmi_imx_bbm_notif_report {
+	bool			is_rtc;
+	bool			is_button;
+	ktime_t			timestamp;
+	unsigned int		rtc_id;
+	unsigned int		rtc_evt;
+};
+#endif

-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/6] firmware: arm_scmi: add initial support for i.MX MISC protocol

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

The i.MX MISC protocol is for misc settings, such as gpio expander
wakeup.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/arm_scmi/Kconfig       |  10 ++
 drivers/firmware/arm_scmi/Makefile      |   1 +
 drivers/firmware/arm_scmi/imx-sm-misc.c | 305 ++++++++++++++++++++++++++++++++
 include/linux/scmi_imx_protocol.h       |  17 ++
 4 files changed, 333 insertions(+)

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index 56d11c9d9f47..bfeae92f6420 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -191,3 +191,13 @@ config IMX_SCMI_BBM_EXT
 	  and BUTTON.
 
 	  This driver can also be built as a module.
+
+config IMX_SCMI_MISC_EXT
+	tristate "i.MX SCMI MISC EXTENSION"
+	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
+	default y if ARCH_MXC
+	help
+	  This enables i.MX System MISC control logic such as gpio expander
+	  wakeup
+
+	  This driver can also be built as a module.
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index 327687acf857..a23fde721222 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -12,6 +12,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
 scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
 scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
 scmi-protocols-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.o
+scmi-protocols-$(CONFIG_IMX_SCMI_MISC_EXT) += imx-sm-misc.o
 scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
 
 obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
diff --git a/drivers/firmware/arm_scmi/imx-sm-misc.c b/drivers/firmware/arm_scmi/imx-sm-misc.c
new file mode 100644
index 000000000000..1b0ec2281518
--- /dev/null
+++ b/drivers/firmware/arm_scmi/imx-sm-misc.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System control and Management Interface (SCMI) NXP MISC Protocol
+ *
+ * Copyright 2024 NXP
+ */
+
+#define pr_fmt(fmt) "SCMI Notifications MISC - " fmt
+
+#include <linux/bits.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/scmi_protocol.h>
+#include <linux/scmi_imx_protocol.h>
+
+#include "protocols.h"
+#include "notify.h"
+
+#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x10000
+
+enum scmi_imx_misc_protocol_cmd {
+	SCMI_IMX_MISC_CTRL_SET	= 0x3,
+	SCMI_IMX_MISC_CTRL_GET	= 0x4,
+	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
+};
+
+struct scmi_imx_misc_info {
+	u32 version;
+	u32 nr_dev_ctrl;
+	u32 nr_brd_ctrl;
+	u32 nr_reason;
+};
+
+struct scmi_msg_imx_misc_protocol_attributes {
+	__le32 attributes;
+};
+
+#define GET_BRD_CTRLS_NR(x)	le32_get_bits((x), GENMASK(31, 24))
+#define GET_REASONS_NR(x)	le32_get_bits((x), GENMASK(23, 16))
+#define GET_DEV_CTRLS_NR(x)	le32_get_bits((x), GENMASK(15, 0))
+#define BRD_CTRL_START_ID	BIT(15)
+
+struct scmi_imx_misc_ctrl_set_in {
+	__le32 id;
+	__le32 num;
+	__le32 value[MISC_MAX_VAL];
+};
+
+struct scmi_imx_misc_ctrl_notify_in {
+	__le32 ctrl_id;
+	__le32 flags;
+};
+
+struct scmi_imx_misc_ctrl_notify_payld {
+	__le32 ctrl_id;
+	__le32 flags;
+};
+
+struct scmi_imx_misc_ctrl_get_out {
+	__le32 num;
+	__le32 *val;
+};
+
+static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
+					struct scmi_imx_misc_info *mi)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_imx_misc_protocol_attributes *attr;
+
+	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
+				      sizeof(*attr), &t);
+	if (ret)
+		return ret;
+
+	attr = t->rx.buf;
+
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret) {
+		mi->nr_dev_ctrl = GET_DEV_CTRLS_NR(attr->attributes);
+		mi->nr_brd_ctrl = GET_BRD_CTRLS_NR(attr->attributes);
+		mi->nr_reason = GET_REASONS_NR(attr->attributes);
+		dev_info(ph->dev, "i.MX MISC NUM DEV CTRL: %d, NUM BRD CTRL: %d,NUM Reason: %d\n",
+			 mi->nr_dev_ctrl, mi->nr_brd_ctrl, mi->nr_reason);
+	}
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int scmi_imx_misc_ctrl_validate_id(const struct scmi_protocol_handle *ph,
+					  u32 ctrl_id)
+{
+	struct scmi_imx_misc_info *mi = ph->get_priv(ph);
+
+	if ((ctrl_id < BRD_CTRL_START_ID) && (ctrl_id > mi->nr_dev_ctrl))
+		return -EINVAL;
+	if (ctrl_id >= BRD_CTRL_START_ID + mi->nr_brd_ctrl)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int scmi_imx_misc_ctrl_notify(const struct scmi_protocol_handle *ph,
+				     u32 ctrl_id, u32 flags)
+{
+	struct scmi_imx_misc_ctrl_notify_in *in;
+	struct scmi_xfer *t;
+	int ret;
+
+	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
+	if (ret)
+		return ret;
+
+	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_NOTIFY,
+				      sizeof(*in), 0, &t);
+	if (ret)
+		return ret;
+
+	in = t->tx.buf;
+	in->ctrl_id = cpu_to_le32(ctrl_id);
+	in->flags = cpu_to_le32(flags);
+
+	ret = ph->xops->do_xfer(ph, t);
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int
+scmi_imx_misc_ctrl_set_notify_enabled(const struct scmi_protocol_handle *ph,
+				      u8 evt_id, u32 src_id, bool enable)
+{
+	int ret;
+
+	ret = scmi_imx_misc_ctrl_notify(ph, src_id, enable ? evt_id : 0);
+	if (ret)
+		dev_err(ph->dev, "FAIL_ENABLED - evt[%X] src[%d] - ret:%d\n",
+			evt_id, src_id, ret);
+
+	return ret;
+}
+
+static int scmi_imx_misc_ctrl_get_num_sources(const struct scmi_protocol_handle *ph)
+{
+	return GENMASK(15, 0);
+}
+
+static void *
+scmi_imx_misc_ctrl_fill_custom_report(const struct scmi_protocol_handle *ph,
+				      u8 evt_id, ktime_t timestamp,
+				      const void *payld, size_t payld_sz,
+				      void *report, u32 *src_id)
+{
+	const struct scmi_imx_misc_ctrl_notify_payld *p = payld;
+	struct scmi_imx_misc_ctrl_notify_report *r = report;
+
+	if (sizeof(*p) != payld_sz)
+		return NULL;
+
+	r->timestamp = timestamp;
+	r->ctrl_id = p->ctrl_id;
+	r->flags = p->flags;
+	*src_id = r->ctrl_id;
+	dev_dbg(ph->dev, "%s: ctrl_id: %d flags: %d\n", __func__,
+		r->ctrl_id, r->flags);
+
+	return r;
+}
+
+static const struct scmi_event_ops scmi_imx_misc_event_ops = {
+	.get_num_sources = scmi_imx_misc_ctrl_get_num_sources,
+	.set_notify_enabled = scmi_imx_misc_ctrl_set_notify_enabled,
+	.fill_custom_report = scmi_imx_misc_ctrl_fill_custom_report,
+};
+
+static const struct scmi_event scmi_imx_misc_events[] = {
+	{
+		.id = SCMI_EVENT_IMX_MISC_CONTROL_DISABLED,
+		.max_payld_sz = sizeof(struct scmi_imx_misc_ctrl_notify_payld),
+		.max_report_sz = sizeof(struct scmi_imx_misc_ctrl_notify_report),
+	},
+	{
+		.id = SCMI_EVENT_IMX_MISC_CONTROL_FALLING_EDGE,
+		.max_payld_sz = sizeof(struct scmi_imx_misc_ctrl_notify_payld),
+		.max_report_sz = sizeof(struct scmi_imx_misc_ctrl_notify_report),
+	},
+	{
+		.id = SCMI_EVENT_IMX_MISC_CONTROL_RISING_EDGE,
+		.max_payld_sz = sizeof(struct scmi_imx_misc_ctrl_notify_payld),
+		.max_report_sz = sizeof(struct scmi_imx_misc_ctrl_notify_report),
+	}
+};
+
+static struct scmi_protocol_events scmi_imx_misc_protocol_events = {
+	.queue_sz = SCMI_PROTO_QUEUE_SZ,
+	.ops = &scmi_imx_misc_event_ops,
+	.evts = scmi_imx_misc_events,
+	.num_events = ARRAY_SIZE(scmi_imx_misc_events),
+};
+
+static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
+{
+	struct scmi_imx_misc_info *minfo;
+	u32 version;
+	int ret;
+
+	ret = ph->xops->version_get(ph, &version);
+	if (ret)
+		return ret;
+
+	dev_info(ph->dev, "NXP SM MISC Version %d.%d\n",
+		 PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
+
+	minfo = devm_kzalloc(ph->dev, sizeof(*minfo), GFP_KERNEL);
+	if (!minfo)
+		return -ENOMEM;
+
+	ret = scmi_imx_misc_attributes_get(ph, minfo);
+	if (ret)
+		return ret;
+
+	return ph->set_priv(ph, minfo, version);
+}
+
+static int scmi_imx_misc_ctrl_get(const struct scmi_protocol_handle *ph,
+				  u32 ctrl_id, u32 *num, u32 *val)
+{
+	struct scmi_imx_misc_ctrl_get_out *out;
+	struct scmi_xfer *t;
+	int ret, i;
+
+	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
+	if (ret)
+		return ret;
+
+	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_GET, sizeof(u32),
+				      0, &t);
+	if (ret)
+		return ret;
+
+	put_unaligned_le32(ctrl_id, t->tx.buf);
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret) {
+		out = t->rx.buf;
+		*num = le32_to_cpu(out->num);
+		for (i = 0; i < *num && i < MISC_MAX_VAL; i++)
+			val[i] = le32_to_cpu(out->val[i]);
+	}
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle *ph,
+				  u32 ctrl_id, u32 num, u32 *val)
+{
+	struct scmi_imx_misc_ctrl_set_in *in;
+	struct scmi_xfer *t;
+	int ret, i;
+
+	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
+	if (ret)
+		return ret;
+
+	if (num > MISC_MAX_VAL)
+		return -EINVAL;
+
+	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_SET, sizeof(*in),
+				      0, &t);
+	if (ret)
+		return ret;
+
+	in = t->tx.buf;
+	in->id = cpu_to_le32(ctrl_id);
+	in->num = cpu_to_le32(num);
+	for (i = 0; i < num; i++)
+		in->value[i] = cpu_to_le32(val[i]);
+
+	ret = ph->xops->do_xfer(ph, t);
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
+	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
+	.misc_ctrl_get = scmi_imx_misc_ctrl_get,
+};
+
+static const struct scmi_protocol scmi_imx_misc = {
+	.id = SCMI_PROTOCOL_IMX_MISC,
+	.owner = THIS_MODULE,
+	.instance_init = &scmi_imx_misc_protocol_init,
+	.ops = &scmi_imx_misc_proto_ops,
+	.events = &scmi_imx_misc_protocol_events,
+	.supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
+};
+module_scmi_protocol(scmi_imx_misc);
diff --git a/include/linux/scmi_imx_protocol.h b/include/linux/scmi_imx_protocol.h
index 90ce011a4429..a69bd4a20f0f 100644
--- a/include/linux/scmi_imx_protocol.h
+++ b/include/linux/scmi_imx_protocol.h
@@ -13,8 +13,14 @@
 #include <linux/notifier.h>
 #include <linux/types.h>
 
+#define SCMI_PAYLOAD_LEN	100
+
+#define SCMI_ARRAY(X, Y)	((SCMI_PAYLOAD_LEN - (X)) / sizeof(Y))
+#define MISC_MAX_VAL		SCMI_ARRAY(8, uint32_t)
+
 enum scmi_nxp_protocol {
 	SCMI_PROTOCOL_IMX_BBM = 0x81,
+	SCMI_PROTOCOL_IMX_MISC = 0x84,
 };
 
 struct scmi_imx_bbm_proto_ops {
@@ -42,4 +48,15 @@ struct scmi_imx_bbm_notif_report {
 	unsigned int		rtc_id;
 	unsigned int		rtc_evt;
 };
+
+struct scmi_imx_misc_ctrl_notify_report {
+	ktime_t			timestamp;
+	unsigned int		ctrl_id;
+	unsigned int		flags;
+};
+
+struct scmi_imx_misc_proto_ops {
+	int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32 id, u32 num, u32 *val);
+	int (*misc_ctrl_get)(const struct scmi_protocol_handle *ph, u32 id, u32 *num, u32 *val);
+};
 #endif

-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/6] firmware: imx: support BBM module

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

The BBM module provides RTC and BUTTON feature. To i.MX95, this module
is managed by System Manager. Linux could use i.MX SCMI BBM Extension
protocol to use RTC and BUTTON feature.

This driver is to use SCMI interface to get/set RTC, enable pwrkey.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/imx/Makefile |   1 +
 drivers/firmware/imx/sm-bbm.c | 317 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 318 insertions(+)

diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile
index 8f9f04a513a8..fb20e22074e1 100644
--- a/drivers/firmware/imx/Makefile
+++ b/drivers/firmware/imx/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_IMX_DSP)		+= imx-dsp.o
 obj-$(CONFIG_IMX_SCU)		+= imx-scu.o misc.o imx-scu-irq.o rm.o imx-scu-soc.o
+obj-${CONFIG_IMX_SCMI_BBM_EXT}	+= sm-bbm.o
diff --git a/drivers/firmware/imx/sm-bbm.c b/drivers/firmware/imx/sm-bbm.c
new file mode 100644
index 000000000000..fcb2ae8490c8
--- /dev/null
+++ b/drivers/firmware/imx/sm-bbm.c
@@ -0,0 +1,317 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2024 NXP.
+ */
+
+#include <linux/input.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+#include <linux/scmi_protocol.h>
+#include <linux/scmi_imx_protocol.h>
+#include <linux/suspend.h>
+
+#define DEBOUNCE_TIME		30
+#define REPEAT_INTERVAL		60
+
+struct scmi_imx_bbm {
+	struct rtc_device *rtc_dev;
+	struct scmi_protocol_handle *ph;
+	const struct scmi_imx_bbm_proto_ops *ops;
+	struct notifier_block nb;
+	int keycode;
+	int keystate;  /* 1:pressed */
+	bool suspended;
+	struct delayed_work check_work;
+	struct input_dev *input;
+};
+
+static int scmi_imx_bbm_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
+	struct scmi_protocol_handle *ph = bbnsm->ph;
+	u64 val;
+	int ret;
+
+	ret = bbnsm->ops->rtc_time_get(ph, 0, &val);
+	if (ret)
+		dev_err(dev, "%s: %d\n", __func__, ret);
+
+	rtc_time64_to_tm(val, tm);
+
+	return 0;
+}
+
+static int scmi_imx_bbm_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
+	struct scmi_protocol_handle *ph = bbnsm->ph;
+	u64 val;
+	int ret;
+
+	val = rtc_tm_to_time64(tm);
+
+	ret = bbnsm->ops->rtc_time_set(ph, 0, val);
+	if (ret)
+		dev_err(dev, "%s: %d\n", __func__, ret);
+
+	return 0;
+}
+
+static int scmi_imx_bbm_alarm_irq_enable(struct device *dev, unsigned int enable)
+{
+	return 0;
+}
+
+static int scmi_imx_bbm_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
+	struct scmi_protocol_handle *ph = bbnsm->ph;
+	struct rtc_time *alrm_tm = &alrm->time;
+	u64 val;
+	int ret;
+
+	val = rtc_tm_to_time64(alrm_tm);
+
+	ret = bbnsm->ops->rtc_alarm_set(ph, 0, val);
+	if (ret)
+		dev_err(dev, "%s: %d\n", __func__, ret);
+
+	return 0;
+}
+
+static const struct rtc_class_ops smci_imx_bbm_rtc_ops = {
+	.read_time = scmi_imx_bbm_read_time,
+	.set_time = scmi_imx_bbm_set_time,
+	.set_alarm = scmi_imx_bbm_set_alarm,
+	.alarm_irq_enable = scmi_imx_bbm_alarm_irq_enable,
+};
+
+static void scmi_imx_bbm_pwrkey_check_for_events(struct work_struct *work)
+{
+	struct scmi_imx_bbm *bbnsm = container_of(work, struct scmi_imx_bbm, check_work.work);
+	struct scmi_protocol_handle *ph = bbnsm->ph;
+	struct input_dev *input = bbnsm->input;
+	u32 state = 0;
+	int ret;
+
+	ret = bbnsm->ops->button_get(ph, &state);
+	if (ret) {
+		pr_err("%s: %d\n", __func__, ret);
+		return;
+	}
+
+	pr_debug("%s: state: %d, keystate %d\n", __func__, state, bbnsm->keystate);
+
+	/* only report new event if status changed */
+	if (state ^ bbnsm->keystate) {
+		bbnsm->keystate = state;
+		input_event(input, EV_KEY, bbnsm->keycode, state);
+		input_sync(input);
+		pm_relax(bbnsm->input->dev.parent);
+		pr_debug("EV_KEY: %x\n", bbnsm->keycode);
+	}
+
+	/* repeat check if pressed long */
+	if (state)
+		schedule_delayed_work(&bbnsm->check_work, msecs_to_jiffies(REPEAT_INTERVAL));
+}
+
+static int scmi_imx_bbm_pwrkey_event(struct scmi_imx_bbm *bbnsm)
+{
+	struct input_dev *input = bbnsm->input;
+
+	schedule_delayed_work(&bbnsm->check_work, msecs_to_jiffies(DEBOUNCE_TIME));
+
+	/*
+	 * Directly report key event after resume to make no key press
+	 * event is missed.
+	 */
+	if (bbnsm->suspended) {
+		bbnsm->keystate = 1;
+		input_event(input, EV_KEY, bbnsm->keycode, 1);
+		input_sync(input);
+	}
+
+	return 0;
+}
+
+static void scmi_imx_bbm_pwrkey_act(void *pdata)
+{
+	struct scmi_imx_bbm *bbnsm = pdata;
+
+	cancel_delayed_work_sync(&bbnsm->check_work);
+}
+
+static int scmi_imx_bbm_notifier(struct notifier_block *nb, unsigned long event, void *data)
+{
+	struct scmi_imx_bbm *bbnsm = container_of(nb, struct scmi_imx_bbm, nb);
+	struct scmi_imx_bbm_notif_report *r = data;
+
+	if (r->is_rtc)
+		rtc_update_irq(bbnsm->rtc_dev, 1, RTC_AF | RTC_IRQF);
+	if (r->is_button) {
+		pr_debug("BBM Button Power key pressed\n");
+		scmi_imx_bbm_pwrkey_event(bbnsm);
+	}
+
+	return 0;
+}
+
+static int scmi_imx_bbm_pwrkey_init(struct scmi_device *sdev)
+{
+	const struct scmi_handle *handle = sdev->handle;
+	struct device *dev = &sdev->dev;
+	struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
+	struct input_dev *input;
+	int ret;
+
+	if (device_property_read_u32(dev, "linux,code", &bbnsm->keycode)) {
+		bbnsm->keycode = KEY_POWER;
+		dev_warn(dev, "key code is not specified, using default KEY_POWER\n");
+	}
+
+	INIT_DELAYED_WORK(&bbnsm->check_work, scmi_imx_bbm_pwrkey_check_for_events);
+
+	input = devm_input_allocate_device(dev);
+	if (!input) {
+		dev_err(dev, "failed to allocate the input device for SCMI IMX BBM\n");
+		return -ENOMEM;
+	}
+
+	input->name = dev_name(dev);
+	input->phys = "bbnsm-pwrkey/input0";
+	input->id.bustype = BUS_HOST;
+
+	input_set_capability(input, EV_KEY, bbnsm->keycode);
+
+	ret = devm_add_action_or_reset(dev, scmi_imx_bbm_pwrkey_act, bbnsm);
+	if (ret) {
+		dev_err(dev, "failed to register remove action\n");
+		return ret;
+	}
+
+	bbnsm->input = input;
+
+	ret = handle->notify_ops->devm_event_notifier_register(sdev, SCMI_PROTOCOL_IMX_BBM,
+							       SCMI_EVENT_IMX_BBM_BUTTON,
+							       NULL, &bbnsm->nb);
+
+	if (ret)
+		dev_err(dev, "Failed to register BBM Button Events %d:", ret);
+
+	ret = input_register_device(input);
+	if (ret) {
+		dev_err(dev, "failed to register input device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int scmi_imx_bbm_rtc_init(struct scmi_device *sdev)
+{
+	const struct scmi_handle *handle = sdev->handle;
+	struct device *dev = &sdev->dev;
+	struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
+	int ret;
+
+	bbnsm->rtc_dev = devm_rtc_allocate_device(dev);
+	if (IS_ERR(bbnsm->rtc_dev))
+		return PTR_ERR(bbnsm->rtc_dev);
+
+	bbnsm->rtc_dev->ops = &smci_imx_bbm_rtc_ops;
+	bbnsm->rtc_dev->range_min = 0;
+	bbnsm->rtc_dev->range_max = U32_MAX;
+
+	ret = devm_rtc_register_device(bbnsm->rtc_dev);
+	if (ret)
+		return ret;
+
+	bbnsm->nb.notifier_call = &scmi_imx_bbm_notifier;
+	return handle->notify_ops->devm_event_notifier_register(sdev, SCMI_PROTOCOL_IMX_BBM,
+								SCMI_EVENT_IMX_BBM_RTC,
+								NULL, &bbnsm->nb);
+}
+
+static int scmi_imx_bbm_probe(struct scmi_device *sdev)
+{
+	const struct scmi_handle *handle = sdev->handle;
+	struct device *dev = &sdev->dev;
+	struct scmi_protocol_handle *ph;
+	struct scmi_imx_bbm *bbnsm;
+	int ret;
+
+	if (!handle)
+		return -ENODEV;
+
+	bbnsm = devm_kzalloc(dev, sizeof(struct scmi_imx_bbm), GFP_KERNEL);
+	if (!bbnsm)
+		return -ENOMEM;
+
+	bbnsm->ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_IMX_BBM, &ph);
+	if (IS_ERR(bbnsm->ops))
+		return PTR_ERR(bbnsm->ops);
+
+	bbnsm->ph = ph;
+
+	device_init_wakeup(dev, true);
+
+	dev_set_drvdata(dev, bbnsm);
+
+	ret = scmi_imx_bbm_rtc_init(sdev);
+	if (ret) {
+		dev_err(dev, "rtc init failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = scmi_imx_bbm_pwrkey_init(sdev);
+	if (ret) {
+		dev_err(dev, "pwr init failed: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __maybe_unused scmi_imx_bbm_suspend(struct device *dev)
+{
+	struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
+
+	bbnsm->suspended = true;
+
+	return 0;
+}
+
+static int __maybe_unused scmi_imx_bbm_resume(struct device *dev)
+{
+	struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
+
+	bbnsm->suspended = false;
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(scmi_imx_bbm_pm_ops, scmi_imx_bbm_suspend, scmi_imx_bbm_resume);
+
+static const struct scmi_device_id scmi_id_table[] = {
+	{ SCMI_PROTOCOL_IMX_BBM, "imx-bbm" },
+	{ },
+};
+MODULE_DEVICE_TABLE(scmi, scmi_id_table);
+
+static struct scmi_driver scmi_imx_bbm_driver = {
+	.driver = {
+		.pm = &scmi_imx_bbm_pm_ops,
+	},
+	.name = "scmi-imx-bbm",
+	.probe = scmi_imx_bbm_probe,
+	.id_table = scmi_id_table,
+};
+module_scmi_driver(scmi_imx_bbm_driver);
+
+MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>");
+MODULE_DESCRIPTION("IMX SM BBM driver");
+MODULE_LICENSE("GPL");

-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 6/6] firmware: imx: add i.MX95 MISC driver

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

The i.MX95 System manager exports SCMI MISC protocol for linux to do
various settings, such as set board gpio expander as wakeup source.

The driver is to add the support.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/imx/Makefile   |  1 +
 drivers/firmware/imx/sm-misc.c  | 92 +++++++++++++++++++++++++++++++++++++++++
 include/linux/firmware/imx/sm.h | 33 +++++++++++++++
 3 files changed, 126 insertions(+)

diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile
index fb20e22074e1..cb9c361d9b81 100644
--- a/drivers/firmware/imx/Makefile
+++ b/drivers/firmware/imx/Makefile
@@ -2,3 +2,4 @@
 obj-$(CONFIG_IMX_DSP)		+= imx-dsp.o
 obj-$(CONFIG_IMX_SCU)		+= imx-scu.o misc.o imx-scu-irq.o rm.o imx-scu-soc.o
 obj-${CONFIG_IMX_SCMI_BBM_EXT}	+= sm-bbm.o
+obj-${CONFIG_IMX_SCMI_MISC_EXT}	+= sm-misc.o
diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
new file mode 100644
index 000000000000..a5609de426f6
--- /dev/null
+++ b/drivers/firmware/imx/sm-misc.c
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2024 NXP.
+ */
+
+#include <linux/firmware/imx/sm.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/scmi_protocol.h>
+#include <linux/scmi_imx_protocol.h>
+
+static const struct scmi_imx_misc_proto_ops *imx_misc_ctrl_ops;
+static struct scmi_protocol_handle *ph;
+struct notifier_block scmi_imx_misc_ctrl_nb;
+
+int scmi_imx_misc_ctrl_set(u32 id, u32 val)
+{
+	if (!ph)
+		return -EPROBE_DEFER;
+
+	return imx_misc_ctrl_ops->misc_ctrl_set(ph, id, 1, &val);
+};
+EXPORT_SYMBOL(scmi_imx_misc_ctrl_set);
+
+int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val)
+{
+	if (!ph)
+		return -EPROBE_DEFER;
+
+	return imx_misc_ctrl_ops->misc_ctrl_get(ph, id, num, val);
+}
+EXPORT_SYMBOL(scmi_imx_misc_ctrl_get);
+
+static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
+				       unsigned long event, void *data)
+{
+	return 0;
+}
+
+static int scmi_imx_misc_ctrl_probe(struct scmi_device *sdev)
+{
+	const struct scmi_handle *handle = sdev->handle;
+	struct device_node *np = sdev->dev.of_node;
+	u32 src_id, evt_id, wu_num;
+	int ret, i;
+
+	if (!handle)
+		return -ENODEV;
+
+	imx_misc_ctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_IMX_MISC, &ph);
+	if (IS_ERR(imx_misc_ctrl_ops))
+		return PTR_ERR(imx_misc_ctrl_ops);
+
+	scmi_imx_misc_ctrl_nb.notifier_call = &scmi_imx_misc_ctrl_notifier;
+	wu_num = of_property_count_u32_elems(np, "wakeup-sources");
+	if (wu_num % 2) {
+		dev_err(&sdev->dev, "Invalid wakeup-sources\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < wu_num; i += 2) {
+		WARN_ON(of_property_read_u32_index(np, "wakeup-sources", i, &src_id));
+		WARN_ON(of_property_read_u32_index(np, "wakeup-sources", i + 1, &evt_id));
+		ret = handle->notify_ops->devm_event_notifier_register(sdev, SCMI_PROTOCOL_IMX_MISC,
+								       evt_id,
+								       &src_id,
+								       &scmi_imx_misc_ctrl_nb);
+		if (ret)
+			dev_err(&sdev->dev, "Failed to register scmi misc event: %d\n", src_id);
+	}
+
+	return 0;
+
+}
+
+static const struct scmi_device_id scmi_id_table[] = {
+	{ SCMI_PROTOCOL_IMX_MISC, "imx-misc-ctrl" },
+	{ },
+};
+MODULE_DEVICE_TABLE(scmi, scmi_id_table);
+
+static struct scmi_driver scmi_imx_misc_ctrl_driver = {
+	.name = "scmi-imx-misc-ctrl",
+	.probe = scmi_imx_misc_ctrl_probe,
+	.id_table = scmi_id_table,
+};
+module_scmi_driver(scmi_imx_misc_ctrl_driver);
+
+MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>");
+MODULE_DESCRIPTION("IMX SM MISC driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/firmware/imx/sm.h b/include/linux/firmware/imx/sm.h
new file mode 100644
index 000000000000..daad4bdf7d1c
--- /dev/null
+++ b/include/linux/firmware/imx/sm.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright 2024 NXP
+ */
+
+#ifndef _SCMI_IMX_H
+#define _SCMI_IMX_H
+
+#include <linux/bitfield.h>
+#include <linux/types.h>
+
+#define SCMI_IMX_CTRL_PDM_CLK_SEL	0	/* AON PDM clock sel */
+#define SCMI_IMX_CTRL_MQS1_SETTINGS	1	/* AON MQS settings */
+#define SCMI_IMX_CTRL_SAI1_MCLK		2	/* AON SAI1 MCLK */
+#define SCMI_IMX_CTRL_SAI3_MCLK		3	/* WAKE SAI3 MCLK */
+#define SCMI_IMX_CTRL_SAI4_MCLK		4	/* WAKE SAI4 MCLK */
+#define SCMI_IMX_CTRL_SAI5_MCLK		5	/* WAKE SAI5 MCLK */
+
+#if IS_ENABLED(CONFIG_IMX_SCMI_MISC_EXT)
+int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val);
+int scmi_imx_misc_ctrl_set(u32 id, u32 val);
+#else
+static inline int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int scmi_imx_misc_ctrl_set(u32 id, u32 val);
+{
+	return -EOPNOTSUPP;
+}
+#endif
+#endif

-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/6] firmware: arm_scmi: add initial support for i.MX MISC protocol

[Marco Felsch]

Hi Peng,

On 24-04-05, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The i.MX MISC protocol is for misc settings, such as gpio expander
> wakeup.

Can you elaborate a bit more please?

Regards,
  Marco


> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/arm_scmi/Kconfig       |  10 ++
>  drivers/firmware/arm_scmi/Makefile      |   1 +
>  drivers/firmware/arm_scmi/imx-sm-misc.c | 305 ++++++++++++++++++++++++++++++++
>  include/linux/scmi_imx_protocol.h       |  17 ++
>  4 files changed, 333 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index 56d11c9d9f47..bfeae92f6420 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -191,3 +191,13 @@ config IMX_SCMI_BBM_EXT
>  	  and BUTTON.
>  
>  	  This driver can also be built as a module.
> +
> +config IMX_SCMI_MISC_EXT
> +	tristate "i.MX SCMI MISC EXTENSION"
> +	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> +	default y if ARCH_MXC
> +	help
> +	  This enables i.MX System MISC control logic such as gpio expander
> +	  wakeup
> +
> +	  This driver can also be built as a module.
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index 327687acf857..a23fde721222 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -12,6 +12,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
>  scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
>  scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
>  scmi-protocols-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.o
> +scmi-protocols-$(CONFIG_IMX_SCMI_MISC_EXT) += imx-sm-misc.o
>  scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
>  
>  obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> diff --git a/drivers/firmware/arm_scmi/imx-sm-misc.c b/drivers/firmware/arm_scmi/imx-sm-misc.c
> new file mode 100644
> index 000000000000..1b0ec2281518
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/imx-sm-misc.c
> @@ -0,0 +1,305 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System control and Management Interface (SCMI) NXP MISC Protocol
> + *
> + * Copyright 2024 NXP
> + */
> +
> +#define pr_fmt(fmt) "SCMI Notifications MISC - " fmt
> +
> +#include <linux/bits.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/scmi_imx_protocol.h>
> +
> +#include "protocols.h"
> +#include "notify.h"
> +
> +#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x10000
> +
> +enum scmi_imx_misc_protocol_cmd {
> +	SCMI_IMX_MISC_CTRL_SET	= 0x3,
> +	SCMI_IMX_MISC_CTRL_GET	= 0x4,
> +	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
> +};
> +
> +struct scmi_imx_misc_info {
> +	u32 version;
> +	u32 nr_dev_ctrl;
> +	u32 nr_brd_ctrl;
> +	u32 nr_reason;
> +};
> +
> +struct scmi_msg_imx_misc_protocol_attributes {
> +	__le32 attributes;
> +};
> +
> +#define GET_BRD_CTRLS_NR(x)	le32_get_bits((x), GENMASK(31, 24))
> +#define GET_REASONS_NR(x)	le32_get_bits((x), GENMASK(23, 16))
> +#define GET_DEV_CTRLS_NR(x)	le32_get_bits((x), GENMASK(15, 0))
> +#define BRD_CTRL_START_ID	BIT(15)
> +
> +struct scmi_imx_misc_ctrl_set_in {
> +	__le32 id;
> +	__le32 num;
> +	__le32 value[MISC_MAX_VAL];
> +};
> +
> +struct scmi_imx_misc_ctrl_notify_in {
> +	__le32 ctrl_id;
> +	__le32 flags;
> +};
> +
> +struct scmi_imx_misc_ctrl_notify_payld {
> +	__le32 ctrl_id;
> +	__le32 flags;
> +};
> +
> +struct scmi_imx_misc_ctrl_get_out {
> +	__le32 num;
> +	__le32 *val;
> +};
> +
> +static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
> +					struct scmi_imx_misc_info *mi)
> +{
> +	int ret;
> +	struct scmi_xfer *t;
> +	struct scmi_msg_imx_misc_protocol_attributes *attr;
> +
> +	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
> +				      sizeof(*attr), &t);
> +	if (ret)
> +		return ret;
> +
> +	attr = t->rx.buf;
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret) {
> +		mi->nr_dev_ctrl = GET_DEV_CTRLS_NR(attr->attributes);
> +		mi->nr_brd_ctrl = GET_BRD_CTRLS_NR(attr->attributes);
> +		mi->nr_reason = GET_REASONS_NR(attr->attributes);
> +		dev_info(ph->dev, "i.MX MISC NUM DEV CTRL: %d, NUM BRD CTRL: %d,NUM Reason: %d\n",
> +			 mi->nr_dev_ctrl, mi->nr_brd_ctrl, mi->nr_reason);
> +	}
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
> +static int scmi_imx_misc_ctrl_validate_id(const struct scmi_protocol_handle *ph,
> +					  u32 ctrl_id)
> +{
> +	struct scmi_imx_misc_info *mi = ph->get_priv(ph);
> +
> +	if ((ctrl_id < BRD_CTRL_START_ID) && (ctrl_id > mi->nr_dev_ctrl))
> +		return -EINVAL;
> +	if (ctrl_id >= BRD_CTRL_START_ID + mi->nr_brd_ctrl)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int scmi_imx_misc_ctrl_notify(const struct scmi_protocol_handle *ph,
> +				     u32 ctrl_id, u32 flags)
> +{
> +	struct scmi_imx_misc_ctrl_notify_in *in;
> +	struct scmi_xfer *t;
> +	int ret;
> +
> +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> +	if (ret)
> +		return ret;
> +
> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_NOTIFY,
> +				      sizeof(*in), 0, &t);
> +	if (ret)
> +		return ret;
> +
> +	in = t->tx.buf;
> +	in->ctrl_id = cpu_to_le32(ctrl_id);
> +	in->flags = cpu_to_le32(flags);
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
> +static int
> +scmi_imx_misc_ctrl_set_notify_enabled(const struct scmi_protocol_handle *ph,
> +				      u8 evt_id, u32 src_id, bool enable)
> +{
> +	int ret;
> +
> +	ret = scmi_imx_misc_ctrl_notify(ph, src_id, enable ? evt_id : 0);
> +	if (ret)
> +		dev_err(ph->dev, "FAIL_ENABLED - evt[%X] src[%d] - ret:%d\n",
> +			evt_id, src_id, ret);
> +
> +	return ret;
> +}
> +
> +static int scmi_imx_misc_ctrl_get_num_sources(const struct scmi_protocol_handle *ph)
> +{
> +	return GENMASK(15, 0);
> +}
> +
> +static void *
> +scmi_imx_misc_ctrl_fill_custom_report(const struct scmi_protocol_handle *ph,
> +				      u8 evt_id, ktime_t timestamp,
> +				      const void *payld, size_t payld_sz,
> +				      void *report, u32 *src_id)
> +{
> +	const struct scmi_imx_misc_ctrl_notify_payld *p = payld;
> +	struct scmi_imx_misc_ctrl_notify_report *r = report;
> +
> +	if (sizeof(*p) != payld_sz)
> +		return NULL;
> +
> +	r->timestamp = timestamp;
> +	r->ctrl_id = p->ctrl_id;
> +	r->flags = p->flags;
> +	*src_id = r->ctrl_id;
> +	dev_dbg(ph->dev, "%s: ctrl_id: %d flags: %d\n", __func__,
> +		r->ctrl_id, r->flags);
> +
> +	return r;
> +}
> +
> +static const struct scmi_event_ops scmi_imx_misc_event_ops = {
> +	.get_num_sources = scmi_imx_misc_ctrl_get_num_sources,
> +	.set_notify_enabled = scmi_imx_misc_ctrl_set_notify_enabled,
> +	.fill_custom_report = scmi_imx_misc_ctrl_fill_custom_report,
> +};
> +
> +static const struct scmi_event scmi_imx_misc_events[] = {
> +	{
> +		.id = SCMI_EVENT_IMX_MISC_CONTROL_DISABLED,
> +		.max_payld_sz = sizeof(struct scmi_imx_misc_ctrl_notify_payld),
> +		.max_report_sz = sizeof(struct scmi_imx_misc_ctrl_notify_report),
> +	},
> +	{
> +		.id = SCMI_EVENT_IMX_MISC_CONTROL_FALLING_EDGE,
> +		.max_payld_sz = sizeof(struct scmi_imx_misc_ctrl_notify_payld),
> +		.max_report_sz = sizeof(struct scmi_imx_misc_ctrl_notify_report),
> +	},
> +	{
> +		.id = SCMI_EVENT_IMX_MISC_CONTROL_RISING_EDGE,
> +		.max_payld_sz = sizeof(struct scmi_imx_misc_ctrl_notify_payld),
> +		.max_report_sz = sizeof(struct scmi_imx_misc_ctrl_notify_report),
> +	}
> +};
> +
> +static struct scmi_protocol_events scmi_imx_misc_protocol_events = {
> +	.queue_sz = SCMI_PROTO_QUEUE_SZ,
> +	.ops = &scmi_imx_misc_event_ops,
> +	.evts = scmi_imx_misc_events,
> +	.num_events = ARRAY_SIZE(scmi_imx_misc_events),
> +};
> +
> +static int scmi_imx_misc_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> +	struct scmi_imx_misc_info *minfo;
> +	u32 version;
> +	int ret;
> +
> +	ret = ph->xops->version_get(ph, &version);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(ph->dev, "NXP SM MISC Version %d.%d\n",
> +		 PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> +	minfo = devm_kzalloc(ph->dev, sizeof(*minfo), GFP_KERNEL);
> +	if (!minfo)
> +		return -ENOMEM;
> +
> +	ret = scmi_imx_misc_attributes_get(ph, minfo);
> +	if (ret)
> +		return ret;
> +
> +	return ph->set_priv(ph, minfo, version);
> +}
> +
> +static int scmi_imx_misc_ctrl_get(const struct scmi_protocol_handle *ph,
> +				  u32 ctrl_id, u32 *num, u32 *val)
> +{
> +	struct scmi_imx_misc_ctrl_get_out *out;
> +	struct scmi_xfer *t;
> +	int ret, i;
> +
> +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> +	if (ret)
> +		return ret;
> +
> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_GET, sizeof(u32),
> +				      0, &t);
> +	if (ret)
> +		return ret;
> +
> +	put_unaligned_le32(ctrl_id, t->tx.buf);
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret) {
> +		out = t->rx.buf;
> +		*num = le32_to_cpu(out->num);
> +		for (i = 0; i < *num && i < MISC_MAX_VAL; i++)
> +			val[i] = le32_to_cpu(out->val[i]);
> +	}
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
> +static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle *ph,
> +				  u32 ctrl_id, u32 num, u32 *val)
> +{
> +	struct scmi_imx_misc_ctrl_set_in *in;
> +	struct scmi_xfer *t;
> +	int ret, i;
> +
> +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> +	if (ret)
> +		return ret;
> +
> +	if (num > MISC_MAX_VAL)
> +		return -EINVAL;
> +
> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_SET, sizeof(*in),
> +				      0, &t);
> +	if (ret)
> +		return ret;
> +
> +	in = t->tx.buf;
> +	in->id = cpu_to_le32(ctrl_id);
> +	in->num = cpu_to_le32(num);
> +	for (i = 0; i < num; i++)
> +		in->value[i] = cpu_to_le32(val[i]);
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
> +static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops = {
> +	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
> +	.misc_ctrl_get = scmi_imx_misc_ctrl_get,
> +};
> +
> +static const struct scmi_protocol scmi_imx_misc = {
> +	.id = SCMI_PROTOCOL_IMX_MISC,
> +	.owner = THIS_MODULE,
> +	.instance_init = &scmi_imx_misc_protocol_init,
> +	.ops = &scmi_imx_misc_proto_ops,
> +	.events = &scmi_imx_misc_protocol_events,
> +	.supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION,
> +};
> +module_scmi_protocol(scmi_imx_misc);
> diff --git a/include/linux/scmi_imx_protocol.h b/include/linux/scmi_imx_protocol.h
> index 90ce011a4429..a69bd4a20f0f 100644
> --- a/include/linux/scmi_imx_protocol.h
> +++ b/include/linux/scmi_imx_protocol.h
> @@ -13,8 +13,14 @@
>  #include <linux/notifier.h>
>  #include <linux/types.h>
>  
> +#define SCMI_PAYLOAD_LEN	100
> +
> +#define SCMI_ARRAY(X, Y)	((SCMI_PAYLOAD_LEN - (X)) / sizeof(Y))
> +#define MISC_MAX_VAL		SCMI_ARRAY(8, uint32_t)
> +
>  enum scmi_nxp_protocol {
>  	SCMI_PROTOCOL_IMX_BBM = 0x81,
> +	SCMI_PROTOCOL_IMX_MISC = 0x84,
>  };
>  
>  struct scmi_imx_bbm_proto_ops {
> @@ -42,4 +48,15 @@ struct scmi_imx_bbm_notif_report {
>  	unsigned int		rtc_id;
>  	unsigned int		rtc_evt;
>  };
> +
> +struct scmi_imx_misc_ctrl_notify_report {
> +	ktime_t			timestamp;
> +	unsigned int		ctrl_id;
> +	unsigned int		flags;
> +};
> +
> +struct scmi_imx_misc_proto_ops {
> +	int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32 id, u32 num, u32 *val);
> +	int (*misc_ctrl_get)(const struct scmi_protocol_handle *ph, u32 id, u32 *num, u32 *val);
> +};
>  #endif
> 
> -- 
> 2.37.1
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Krzysztof Kozlowski]

On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> When adding vendor extension protocols, there is dt-schema warning:
> "
> imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do not match any
> of the regexes: 'pinctrl-[0-9]+'
> "
> 
> Set additionalProperties to true to address the issue.

I do not see anything addressed here, except making the binding
accepting anything anywhere...

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension protocol

[Krzysztof Kozlowski]

On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Add i.MX SCMI Extension protocols bindings for:
>  - Battery Backed Secure Module(BBSM)

Which is what?

>  - MISC settings such as General Purpose Registers settings.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../devicetree/bindings/firmware/imx,scmi.yaml     | 80 ++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/imx,scmi.yaml b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> new file mode 100644
> index 000000000000..7ee19a661d83
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2024 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/firmware/imx,scmi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: i.MX System Control and Management Interface(SCMI) Vendor Protocols Extension
> +
> +maintainers:
> +  - Peng Fan <peng.fan@nxp.com>
> +
> +allOf:
> +  - $ref: arm,scmi.yaml#

Sorry, but arm,scmi is a final schema. Is your plan to define some
common part?

> +
> +properties:
> +  protocol@81:
> +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> +    unevaluatedProperties: false
> +    description:
> +      The BBM Protocol is for managing Battery Backed Secure Module (BBSM) RTC
> +      and the ON/OFF Key
> +
> +    properties:
> +      reg:
> +        const: 0x81
> +
> +    required:
> +      - reg
> +
> +  protocol@84:
> +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> +    unevaluatedProperties: false
> +    description:
> +      The MISC Protocol is for managing SoC Misc settings, such as GPR settings

Genera register is not a setting... this is a pleonasm. Please be more
specific what is the GPR, MISC protocol etc.

> +
> +    properties:
> +      reg:
> +        const: 0x84
> +
> +      wakeup-sources:
> +        description:
> +          Each entry consists of 2 integers, represents the source and electric signal edge

Can you answer questions from reviewers?

> +        items:
> +          items:
> +            - description: the wakeup source
> +            - description: the wakeup electric signal edge
> +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +
> +    required:
> +      - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    firmware {
> +        scmi {
> +            compatible = "arm,scmi";

> +            mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>;
> +            shmem = <&scmi_buf0>, <&scmi_buf1>;
> +
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            protocol@81 {
> +                reg = <0x81>;
> +            };
> +
> +            protocol@84 {
> +                reg = <0x84>;
> +                wakeup-sources = <0x8000 1
> +                                  0x8001 1
> +                                  0x8002 1
> +                                  0x8003 1
> +                                  0x8004 1>;

Nothing improved... If you are going to ignore reviews, then you will
only get NAKed.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Peng Fan]

> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> additionalProperties to true
> 
> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > When adding vendor extension protocols, there is dt-schema warning:
> > "
> > imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do not match
> > any of the regexes: 'pinctrl-[0-9]+'
> > "
> >
> > Set additionalProperties to true to address the issue.
> 
> I do not see anything addressed here, except making the binding accepting
> anything anywhere...

I not wanna add vendor protocols in arm,scmi.yaml, so will introduce
a new yaml imx.scmi.yaml which add i.MX SCMI protocol extension.

With additionalProperties set to false, I not know how, please suggest.

Thanks,
Peng.
> 
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension protocol

[Peng Fan]

> Subject: Re: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension
> protocol
> 
> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add i.MX SCMI Extension protocols bindings for:
> >  - Battery Backed Secure Module(BBSM)
> 
> Which is what?

I should say BBM(BBSM + BBNSM), BBM has RTC and ON/OFF
key features, but BBM is managed by SCMI firmware and exported
to agent by BBM protocol. So add bindings for i.MX BBM protocol.

Is this ok?

> 
> >  - MISC settings such as General Purpose Registers settings.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  .../devicetree/bindings/firmware/imx,scmi.yaml     | 80
> ++++++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > new file mode 100644
> > index 000000000000..7ee19a661d83
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright 2024
> > +NXP %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Ffirmware%2Fimx%2Cscmi.yaml%23&data=05%7
> C02%7Cp
> >
> +eng.fan%40nxp.com%7C5d16781d3eca425a342508dc562910b7%7C686ea
> 1d3bc2b4c
> >
> +6fa92cd99c5c301635%7C0%7C0%7C638479981570959816%7CUnknown%
> 7CTWFpbGZsb
> >
> +3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D
> >
> +%7C0%7C%7C%7C&sdata=mWNwPvu2eyF18MroVOBHb%2Fjeo%2BIHfV5V
> h%2F9ebdx65MM
> > +%3D&reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fmeta-
> schemas%2Fcore.yaml%23&data=05%7C02%7Cpeng.fan%40nx
> >
> +p.com%7C5d16781d3eca425a342508dc562910b7%7C686ea1d3bc2b4c6fa
> 92cd99c5c
> >
> +301635%7C0%7C0%7C638479981570971949%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoiM
> >
> +C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7
> C%7C%7
> >
> +C&sdata=v4XnGG00D4I8j5MJvDUVYMRTm7yRrvz0V3fUyc5KAAA%3D&reser
> ved=0
> > +
> > +title: i.MX System Control and Management Interface(SCMI) Vendor
> > +Protocols Extension
> > +
> > +maintainers:
> > +  - Peng Fan <peng.fan@nxp.com>
> > +
> > +allOf:
> > +  - $ref: arm,scmi.yaml#
> 
> Sorry, but arm,scmi is a final schema. Is your plan to define some common
> part?

No. I just wanna add vendor extension per SCMI spec. 

0x80-0xFF:
Reserved for vendor or platform-specific extensions to this interface

Each vendor may have different usage saying id 0x81, so I add
i.MX dt-schema file.

> 
> > +
> > +properties:
> > +  protocol@81:
> > +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > +    unevaluatedProperties: false
> > +    description:
> > +      The BBM Protocol is for managing Battery Backed Secure Module
> (BBSM) RTC
> > +      and the ON/OFF Key
> > +
> > +    properties:
> > +      reg:
> > +        const: 0x81
> > +
> > +    required:
> > +      - reg
> > +
> > +  protocol@84:
> > +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > +    unevaluatedProperties: false
> > +    description:
> > +      The MISC Protocol is for managing SoC Misc settings, such as
> > + GPR settings
> 
> Genera register is not a setting... this is a pleonasm. Please be more specific
> what is the GPR, MISC protocol etc.

The MISC Protocol is for managing SoC Misc settings, such as SAI MCLK/MQS in
Always On domain BLK CTRL,  SAI_CLK_SEL in WAKEUP BLK CTRL, gpio
expanders which is under control of SCMI firmware.

> > +
> > +    properties:
> > +      reg:
> > +        const: 0x84
> > +
> > +      wakeup-sources:
> > +        description:
> > +          Each entry consists of 2 integers, represents the source
> > + and electric signal edge
> 
> Can you answer questions from reviewers?

Sorry. Is this ok?
minItems: 1
maxItems: 32

> 
> > +        items:
> > +          items:
> > +            - description: the wakeup source
> > +            - description: the wakeup electric signal edge
> > +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +
> > +    required:
> > +      - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    firmware {
> > +        scmi {
> > +            compatible = "arm,scmi";
> 
> > +            mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>;
> > +            shmem = <&scmi_buf0>, <&scmi_buf1>;
> > +
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            protocol@81 {
> > +                reg = <0x81>;
> > +            };
> > +
> > +            protocol@84 {
> > +                reg = <0x84>;
> > +                wakeup-sources = <0x8000 1
> > +                                  0x8001 1
> > +                                  0x8002 1
> > +                                  0x8003 1
> > +                                  0x8004 1>;
> 
> Nothing improved... If you are going to ignore reviews, then you will only get
> NAKed.

Sorry, you mean the examples, or the whole dt-schema?

Thanks,
Peng.
> 
> Best regards,
> Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

> Subject: Re: [PATCH v2 4/6] firmware: arm_scmi: add initial support for i.MX
> MISC protocol
> 
> Hi Peng,
> 
> On 24-04-05, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The i.MX MISC protocol is for misc settings, such as gpio expander
> > wakeup.
> 
> Can you elaborate a bit more please?

The gpio expander is under M33(SCMI firmware used core) I2C control,
But the gpio expander supports board function such as PCIE_WAKEUP,
BTN_WAKEUP. So these are managed by MISC protocol.

SAI_CLK_MSEL in WAKEUP BLK CTRL is also managed by MISC Protocol.

And etc...

I will add more info in commit log in next version later, after I get more
reviews on the patchset.

Thanks,
Peng.

> 
> Regards,
>   Marco
> 
> 
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/firmware/arm_scmi/Kconfig       |  10 ++
> >  drivers/firmware/arm_scmi/Makefile      |   1 +
> >  drivers/firmware/arm_scmi/imx-sm-misc.c | 305
> ++++++++++++++++++++++++++++++++
> >  include/linux/scmi_imx_protocol.h       |  17 ++
> >  4 files changed, 333 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/Kconfig
> > b/drivers/firmware/arm_scmi/Kconfig
> > index 56d11c9d9f47..bfeae92f6420 100644
> > --- a/drivers/firmware/arm_scmi/Kconfig
> > +++ b/drivers/firmware/arm_scmi/Kconfig
> > @@ -191,3 +191,13 @@ config IMX_SCMI_BBM_EXT
> >  	  and BUTTON.
> >
> >  	  This driver can also be built as a module.
> > +
> > +config IMX_SCMI_MISC_EXT
> > +	tristate "i.MX SCMI MISC EXTENSION"
> > +	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> > +	default y if ARCH_MXC
> > +	help
> > +	  This enables i.MX System MISC control logic such as gpio expander
> > +	  wakeup
> > +
> > +	  This driver can also be built as a module.
> > diff --git a/drivers/firmware/arm_scmi/Makefile
> > b/drivers/firmware/arm_scmi/Makefile
> > index 327687acf857..a23fde721222 100644
> > --- a/drivers/firmware/arm_scmi/Makefile
> > +++ b/drivers/firmware/arm_scmi/Makefile
> > @@ -12,6 +12,7 @@ scmi-transport-
> $(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO)
> > += virtio.o
> >  scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
> > system.o voltage.o powercap.o
> >  scmi-protocols-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.o
> > +scmi-protocols-$(CONFIG_IMX_SCMI_MISC_EXT) += imx-sm-misc.o
> >  scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y)
> > $(scmi-transport-y)
> >
> >  obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o diff --git
> > a/drivers/firmware/arm_scmi/imx-sm-misc.c
> > b/drivers/firmware/arm_scmi/imx-sm-misc.c
> > new file mode 100644
> > index 000000000000..1b0ec2281518
> > --- /dev/null
> > +++ b/drivers/firmware/arm_scmi/imx-sm-misc.c
> > @@ -0,0 +1,305 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * System control and Management Interface (SCMI) NXP MISC Protocol
> > + *
> > + * Copyright 2024 NXP
> > + */
> > +
> > +#define pr_fmt(fmt) "SCMI Notifications MISC - " fmt
> > +
> > +#include <linux/bits.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/scmi_imx_protocol.h>
> > +
> > +#include "protocols.h"
> > +#include "notify.h"
> > +
> > +#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x10000
> > +
> > +enum scmi_imx_misc_protocol_cmd {
> > +	SCMI_IMX_MISC_CTRL_SET	= 0x3,
> > +	SCMI_IMX_MISC_CTRL_GET	= 0x4,
> > +	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
> > +};
> > +
> > +struct scmi_imx_misc_info {
> > +	u32 version;
> > +	u32 nr_dev_ctrl;
> > +	u32 nr_brd_ctrl;
> > +	u32 nr_reason;
> > +};
> > +
> > +struct scmi_msg_imx_misc_protocol_attributes {
> > +	__le32 attributes;
> > +};
> > +
> > +#define GET_BRD_CTRLS_NR(x)	le32_get_bits((x), GENMASK(31,
> 24))
> > +#define GET_REASONS_NR(x)	le32_get_bits((x), GENMASK(23, 16))
> > +#define GET_DEV_CTRLS_NR(x)	le32_get_bits((x), GENMASK(15, 0))
> > +#define BRD_CTRL_START_ID	BIT(15)
> > +
> > +struct scmi_imx_misc_ctrl_set_in {
> > +	__le32 id;
> > +	__le32 num;
> > +	__le32 value[MISC_MAX_VAL];
> > +};
> > +
> > +struct scmi_imx_misc_ctrl_notify_in {
> > +	__le32 ctrl_id;
> > +	__le32 flags;
> > +};
> > +
> > +struct scmi_imx_misc_ctrl_notify_payld {
> > +	__le32 ctrl_id;
> > +	__le32 flags;
> > +};
> > +
> > +struct scmi_imx_misc_ctrl_get_out {
> > +	__le32 num;
> > +	__le32 *val;
> > +};
> > +
> > +static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle
> *ph,
> > +					struct scmi_imx_misc_info *mi)
> > +{
> > +	int ret;
> > +	struct scmi_xfer *t;
> > +	struct scmi_msg_imx_misc_protocol_attributes *attr;
> > +
> > +	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
> > +				      sizeof(*attr), &t);
> > +	if (ret)
> > +		return ret;
> > +
> > +	attr = t->rx.buf;
> > +
> > +	ret = ph->xops->do_xfer(ph, t);
> > +	if (!ret) {
> > +		mi->nr_dev_ctrl = GET_DEV_CTRLS_NR(attr->attributes);
> > +		mi->nr_brd_ctrl = GET_BRD_CTRLS_NR(attr->attributes);
> > +		mi->nr_reason = GET_REASONS_NR(attr->attributes);
> > +		dev_info(ph->dev, "i.MX MISC NUM DEV CTRL: %d, NUM
> BRD CTRL: %d,NUM Reason: %d\n",
> > +			 mi->nr_dev_ctrl, mi->nr_brd_ctrl, mi->nr_reason);
> > +	}
> > +
> > +	ph->xops->xfer_put(ph, t);
> > +
> > +	return ret;
> > +}
> > +
> > +static int scmi_imx_misc_ctrl_validate_id(const struct
> scmi_protocol_handle *ph,
> > +					  u32 ctrl_id)
> > +{
> > +	struct scmi_imx_misc_info *mi = ph->get_priv(ph);
> > +
> > +	if ((ctrl_id < BRD_CTRL_START_ID) && (ctrl_id > mi->nr_dev_ctrl))
> > +		return -EINVAL;
> > +	if (ctrl_id >= BRD_CTRL_START_ID + mi->nr_brd_ctrl)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int scmi_imx_misc_ctrl_notify(const struct scmi_protocol_handle
> *ph,
> > +				     u32 ctrl_id, u32 flags)
> > +{
> > +	struct scmi_imx_misc_ctrl_notify_in *in;
> > +	struct scmi_xfer *t;
> > +	int ret;
> > +
> > +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_NOTIFY,
> > +				      sizeof(*in), 0, &t);
> > +	if (ret)
> > +		return ret;
> > +
> > +	in = t->tx.buf;
> > +	in->ctrl_id = cpu_to_le32(ctrl_id);
> > +	in->flags = cpu_to_le32(flags);
> > +
> > +	ret = ph->xops->do_xfer(ph, t);
> > +
> > +	ph->xops->xfer_put(ph, t);
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> > +scmi_imx_misc_ctrl_set_notify_enabled(const struct
> scmi_protocol_handle *ph,
> > +				      u8 evt_id, u32 src_id, bool enable) {
> > +	int ret;
> > +
> > +	ret = scmi_imx_misc_ctrl_notify(ph, src_id, enable ? evt_id : 0);
> > +	if (ret)
> > +		dev_err(ph->dev, "FAIL_ENABLED - evt[%X] src[%d] -
> ret:%d\n",
> > +			evt_id, src_id, ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int scmi_imx_misc_ctrl_get_num_sources(const struct
> > +scmi_protocol_handle *ph) {
> > +	return GENMASK(15, 0);
> > +}
> > +
> > +static void *
> > +scmi_imx_misc_ctrl_fill_custom_report(const struct scmi_protocol_handle
> *ph,
> > +				      u8 evt_id, ktime_t timestamp,
> > +				      const void *payld, size_t payld_sz,
> > +				      void *report, u32 *src_id)
> > +{
> > +	const struct scmi_imx_misc_ctrl_notify_payld *p = payld;
> > +	struct scmi_imx_misc_ctrl_notify_report *r = report;
> > +
> > +	if (sizeof(*p) != payld_sz)
> > +		return NULL;
> > +
> > +	r->timestamp = timestamp;
> > +	r->ctrl_id = p->ctrl_id;
> > +	r->flags = p->flags;
> > +	*src_id = r->ctrl_id;
> > +	dev_dbg(ph->dev, "%s: ctrl_id: %d flags: %d\n", __func__,
> > +		r->ctrl_id, r->flags);
> > +
> > +	return r;
> > +}
> > +
> > +static const struct scmi_event_ops scmi_imx_misc_event_ops = {
> > +	.get_num_sources = scmi_imx_misc_ctrl_get_num_sources,
> > +	.set_notify_enabled = scmi_imx_misc_ctrl_set_notify_enabled,
> > +	.fill_custom_report = scmi_imx_misc_ctrl_fill_custom_report,
> > +};
> > +
> > +static const struct scmi_event scmi_imx_misc_events[] = {
> > +	{
> > +		.id = SCMI_EVENT_IMX_MISC_CONTROL_DISABLED,
> > +		.max_payld_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_payld),
> > +		.max_report_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_report),
> > +	},
> > +	{
> > +		.id = SCMI_EVENT_IMX_MISC_CONTROL_FALLING_EDGE,
> > +		.max_payld_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_payld),
> > +		.max_report_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_report),
> > +	},
> > +	{
> > +		.id = SCMI_EVENT_IMX_MISC_CONTROL_RISING_EDGE,
> > +		.max_payld_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_payld),
> > +		.max_report_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_report),
> > +	}
> > +};
> > +
> > +static struct scmi_protocol_events scmi_imx_misc_protocol_events = {
> > +	.queue_sz = SCMI_PROTO_QUEUE_SZ,
> > +	.ops = &scmi_imx_misc_event_ops,
> > +	.evts = scmi_imx_misc_events,
> > +	.num_events = ARRAY_SIZE(scmi_imx_misc_events), };
> > +
> > +static int scmi_imx_misc_protocol_init(const struct
> > +scmi_protocol_handle *ph) {
> > +	struct scmi_imx_misc_info *minfo;
> > +	u32 version;
> > +	int ret;
> > +
> > +	ret = ph->xops->version_get(ph, &version);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_info(ph->dev, "NXP SM MISC Version %d.%d\n",
> > +		 PROTOCOL_REV_MAJOR(version),
> PROTOCOL_REV_MINOR(version));
> > +
> > +	minfo = devm_kzalloc(ph->dev, sizeof(*minfo), GFP_KERNEL);
> > +	if (!minfo)
> > +		return -ENOMEM;
> > +
> > +	ret = scmi_imx_misc_attributes_get(ph, minfo);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return ph->set_priv(ph, minfo, version); }
> > +
> > +static int scmi_imx_misc_ctrl_get(const struct scmi_protocol_handle *ph,
> > +				  u32 ctrl_id, u32 *num, u32 *val) {
> > +	struct scmi_imx_misc_ctrl_get_out *out;
> > +	struct scmi_xfer *t;
> > +	int ret, i;
> > +
> > +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_GET,
> sizeof(u32),
> > +				      0, &t);
> > +	if (ret)
> > +		return ret;
> > +
> > +	put_unaligned_le32(ctrl_id, t->tx.buf);
> > +	ret = ph->xops->do_xfer(ph, t);
> > +	if (!ret) {
> > +		out = t->rx.buf;
> > +		*num = le32_to_cpu(out->num);
> > +		for (i = 0; i < *num && i < MISC_MAX_VAL; i++)
> > +			val[i] = le32_to_cpu(out->val[i]);
> > +	}
> > +
> > +	ph->xops->xfer_put(ph, t);
> > +
> > +	return ret;
> > +}
> > +
> > +static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle *ph,
> > +				  u32 ctrl_id, u32 num, u32 *val) {
> > +	struct scmi_imx_misc_ctrl_set_in *in;
> > +	struct scmi_xfer *t;
> > +	int ret, i;
> > +
> > +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (num > MISC_MAX_VAL)
> > +		return -EINVAL;
> > +
> > +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_SET,
> sizeof(*in),
> > +				      0, &t);
> > +	if (ret)
> > +		return ret;
> > +
> > +	in = t->tx.buf;
> > +	in->id = cpu_to_le32(ctrl_id);
> > +	in->num = cpu_to_le32(num);
> > +	for (i = 0; i < num; i++)
> > +		in->value[i] = cpu_to_le32(val[i]);
> > +
> > +	ret = ph->xops->do_xfer(ph, t);
> > +
> > +	ph->xops->xfer_put(ph, t);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops =
> {
> > +	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
> > +	.misc_ctrl_get = scmi_imx_misc_ctrl_get, };
> > +
> > +static const struct scmi_protocol scmi_imx_misc = {
> > +	.id = SCMI_PROTOCOL_IMX_MISC,
> > +	.owner = THIS_MODULE,
> > +	.instance_init = &scmi_imx_misc_protocol_init,
> > +	.ops = &scmi_imx_misc_proto_ops,
> > +	.events = &scmi_imx_misc_protocol_events,
> > +	.supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION, };
> > +module_scmi_protocol(scmi_imx_misc);
> > diff --git a/include/linux/scmi_imx_protocol.h
> > b/include/linux/scmi_imx_protocol.h
> > index 90ce011a4429..a69bd4a20f0f 100644
> > --- a/include/linux/scmi_imx_protocol.h
> > +++ b/include/linux/scmi_imx_protocol.h
> > @@ -13,8 +13,14 @@
> >  #include <linux/notifier.h>
> >  #include <linux/types.h>
> >
> > +#define SCMI_PAYLOAD_LEN	100
> > +
> > +#define SCMI_ARRAY(X, Y)	((SCMI_PAYLOAD_LEN - (X)) / sizeof(Y))
> > +#define MISC_MAX_VAL		SCMI_ARRAY(8, uint32_t)
> > +
> >  enum scmi_nxp_protocol {
> >  	SCMI_PROTOCOL_IMX_BBM = 0x81,
> > +	SCMI_PROTOCOL_IMX_MISC = 0x84,
> >  };
> >
> >  struct scmi_imx_bbm_proto_ops {
> > @@ -42,4 +48,15 @@ struct scmi_imx_bbm_notif_report {
> >  	unsigned int		rtc_id;
> >  	unsigned int		rtc_evt;
> >  };
> > +
> > +struct scmi_imx_misc_ctrl_notify_report {
> > +	ktime_t			timestamp;
> > +	unsigned int		ctrl_id;
> > +	unsigned int		flags;
> > +};
> > +
> > +struct scmi_imx_misc_proto_ops {
> > +	int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32 id,
> u32 num, u32 *val);
> > +	int (*misc_ctrl_get)(const struct scmi_protocol_handle *ph, u32 id,
> > +u32 *num, u32 *val); };
> >  #endif
> >
> > --
> > 2.37.1
> >
> >
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension protocol

[Peng Fan]

> Subject: RE: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension
> protocol
> 
> > Subject: Re: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI
> > Extension protocol
> >
> > On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > Add i.MX SCMI Extension protocols bindings for:
> > >  - Battery Backed Secure Module(BBSM)
> >
> > Which is what?
> 
> I should say BBM(BBSM + BBNSM), BBM has RTC and ON/OFF key features,
> but BBM is managed by SCMI firmware and exported to agent by BBM
> protocol. So add bindings for i.MX BBM protocol.
> 
> Is this ok?
> 
> >
> > >  - MISC settings such as General Purpose Registers settings.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  .../devicetree/bindings/firmware/imx,scmi.yaml     | 80
> > ++++++++++++++++++++++
> > >  1 file changed, 80 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > > b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > > new file mode 100644
> > > index 000000000000..7ee19a661d83
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > > @@ -0,0 +1,80 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright
> > > +2024 NXP %YAML 1.2
> > > +---
> > > +$id:
> > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> > > +vi
> > >
> >
> +cetree.org%2Fschemas%2Ffirmware%2Fimx%2Cscmi.yaml%23&data=05%7
> > C02%7Cp
> > >
> >
> +eng.fan%40nxp.com%7C5d16781d3eca425a342508dc562910b7%7C686ea
> > 1d3bc2b4c
> > >
> > +6fa92cd99c5c301635%7C0%7C0%7C638479981570959816%7CUnknown%
> > 7CTWFpbGZsb
> > >
> >
> +3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> > 0%3D
> > >
> >
> +%7C0%7C%7C%7C&sdata=mWNwPvu2eyF18MroVOBHb%2Fjeo%2BIHfV5V
> > h%2F9ebdx65MM
> > > +%3D&reserved=0
> > > +$schema:
> > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> > > +vi
> > > +cetree.org%2Fmeta-
> > schemas%2Fcore.yaml%23&data=05%7C02%7Cpeng.fan%40nx
> > >
> >
> +p.com%7C5d16781d3eca425a342508dc562910b7%7C686ea1d3bc2b4c6fa
> > 92cd99c5c
> > >
> >
> +301635%7C0%7C0%7C638479981570971949%7CUnknown%7CTWFpbGZs
> > b3d8eyJWIjoiM
> > >
> >
> +C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7
> > C%7C%7
> > >
> >
> +C&sdata=v4XnGG00D4I8j5MJvDUVYMRTm7yRrvz0V3fUyc5KAAA%3D&reser
> > ved=0
> > > +
> > > +title: i.MX System Control and Management Interface(SCMI) Vendor
> > > +Protocols Extension
> > > +
> > > +maintainers:
> > > +  - Peng Fan <peng.fan@nxp.com>
> > > +
> > > +allOf:
> > > +  - $ref: arm,scmi.yaml#
> >
> > Sorry, but arm,scmi is a final schema. Is your plan to define some
> > common part?
> 
> No. I just wanna add vendor extension per SCMI spec.
> 
> 0x80-0xFF:
> Reserved for vendor or platform-specific extensions to this interface
> 
> Each vendor may have different usage saying id 0x81, so I add i.MX dt-
> schema file.
> 
> >
> > > +
> > > +properties:
> > > +  protocol@81:
> > > +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > > +    unevaluatedProperties: false
> > > +    description:
> > > +      The BBM Protocol is for managing Battery Backed Secure Module
> > (BBSM) RTC
> > > +      and the ON/OFF Key
> > > +
> > > +    properties:
> > > +      reg:
> > > +        const: 0x81
> > > +
> > > +    required:
> > > +      - reg
> > > +
> > > +  protocol@84:
> > > +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > > +    unevaluatedProperties: false
> > > +    description:
> > > +      The MISC Protocol is for managing SoC Misc settings, such as
> > > + GPR settings
> >
> > Genera register is not a setting... this is a pleonasm. Please be more
> > specific what is the GPR, MISC protocol etc.
> 
> The MISC Protocol is for managing SoC Misc settings, such as SAI MCLK/MQS
> in Always On domain BLK CTRL,  SAI_CLK_SEL in WAKEUP BLK CTRL, gpio
> expanders which is under control of SCMI firmware.
> 
> > > +
> > > +    properties:
> > > +      reg:
> > > +        const: 0x84
> > > +
> > > +      wakeup-sources:
> > > +        description:
> > > +          Each entry consists of 2 integers, represents the source
> > > + and electric signal edge
> >
> > Can you answer questions from reviewers?
> 
> Sorry. Is this ok?
> minItems: 1
> maxItems: 32
> 
> >
> > > +        items:
> > > +          items:
> > > +            - description: the wakeup source
> > > +            - description: the wakeup electric signal edge
> > > +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > > +
> > > +    required:
> > > +      - reg
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    firmware {
> > > +        scmi {
> > > +            compatible = "arm,scmi";
> >
> > > +            mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>;
> > > +            shmem = <&scmi_buf0>, <&scmi_buf1>;
> > > +
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +
> > > +            protocol@81 {
> > > +                reg = <0x81>;
> > > +            };
> > > +
> > > +            protocol@84 {
> > > +                reg = <0x84>;
> > > +                wakeup-sources = <0x8000 1
> > > +                                  0x8001 1
> > > +                                  0x8002 1
> > > +                                  0x8003 1
> > > +                                  0x8004 1>;
> >
> > Nothing improved... If you are going to ignore reviews, then you will
> > only get NAKed.
> 
> Sorry, you mean the examples, or the whole dt-schema?

Missed Rob's comment, will use
< > for each entry.

Thanks,
Peng.

> 
> Thanks,
> Peng.
> >
> > Best regards,
> > Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Krzysztof Kozlowski]

On 07/04/2024 02:37, Peng Fan wrote:
>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
>> additionalProperties to true
>>
>> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
>>> From: Peng Fan <peng.fan@nxp.com>
>>>
>>> When adding vendor extension protocols, there is dt-schema warning:
>>> "
>>> imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do not match
>>> any of the regexes: 'pinctrl-[0-9]+'
>>> "
>>>
>>> Set additionalProperties to true to address the issue.
>>
>> I do not see anything addressed here, except making the binding accepting
>> anything anywhere...
> 
> I not wanna add vendor protocols in arm,scmi.yaml, so will introduce
> a new yaml imx.scmi.yaml which add i.MX SCMI protocol extension.
> 
> With additionalProperties set to false, I not know how, please suggest.

First of all, you cannot affect negatively existing devices (their
bindings) and your patch does exactly that. This should make you thing
what is the correct approach...

Rob gave you the comment about missing compatible - you still did not
address that.

You need common schema referenced in arm,scmi and your device specific
schema, also using it.


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension protocol

[Krzysztof Kozlowski]

On 07/04/2024 02:51, Peng Fan wrote:
>> Subject: Re: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension
>> protocol
>>
>> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
>>> From: Peng Fan <peng.fan@nxp.com>
>>>
>>> Add i.MX SCMI Extension protocols bindings for:
>>>  - Battery Backed Secure Module(BBSM)
>>
>> Which is what?
> 
> I should say BBM(BBSM + BBNSM), BBM has RTC and ON/OFF
> key features, but BBM is managed by SCMI firmware and exported
> to agent by BBM protocol. So add bindings for i.MX BBM protocol.
> 
> Is this ok?

No, I still don't know what is BBSM, BBNSM and BBM.

> 
>>
>>>  - MISC settings such as General Purpose Registers settings.
>>>
>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>> ---
>>>  .../devicetree/bindings/firmware/imx,scmi.yaml     | 80
>> ++++++++++++++++++++++
>>>  1 file changed, 80 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
>>> b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
>>> new file mode 100644
>>> index 000000000000..7ee19a661d83
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
>>> @@ -0,0 +1,80 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright 2024
>>> +NXP %YAML 1.2
>>> +---
>>> +$id:
>>> +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
>>>
>> +cetree.org%2Fschemas%2Ffirmware%2Fimx%2Cscmi.yaml%23&data=05%7
>> C02%7Cp
>>>
>> +eng.fan%40nxp.com%7C5d16781d3eca425a342508dc562910b7%7C686ea
>> 1d3bc2b4c
>>>
>> +6fa92cd99c5c301635%7C0%7C0%7C638479981570959816%7CUnknown%
>> 7CTWFpbGZsb
>>>
>> +3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
>> 0%3D
>>>
>> +%7C0%7C%7C%7C&sdata=mWNwPvu2eyF18MroVOBHb%2Fjeo%2BIHfV5V
>> h%2F9ebdx65MM
>>> +%3D&reserved=0
>>> +$schema:
>>> +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
>>> +cetree.org%2Fmeta-
>> schemas%2Fcore.yaml%23&data=05%7C02%7Cpeng.fan%40nx
>>>
>> +p.com%7C5d16781d3eca425a342508dc562910b7%7C686ea1d3bc2b4c6fa
>> 92cd99c5c
>>>
>> +301635%7C0%7C0%7C638479981570971949%7CUnknown%7CTWFpbGZs
>> b3d8eyJWIjoiM
>>>
>> +C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7
>> C%7C%7
>>>
>> +C&sdata=v4XnGG00D4I8j5MJvDUVYMRTm7yRrvz0V3fUyc5KAAA%3D&reser
>> ved=0
>>> +
>>> +title: i.MX System Control and Management Interface(SCMI) Vendor
>>> +Protocols Extension
>>> +
>>> +maintainers:
>>> +  - Peng Fan <peng.fan@nxp.com>
>>> +
>>> +allOf:
>>> +  - $ref: arm,scmi.yaml#
>>
>> Sorry, but arm,scmi is a final schema. Is your plan to define some common
>> part?
> 
> No. I just wanna add vendor extension per SCMI spec. 
> 
> 0x80-0xFF:
> Reserved for vendor or platform-specific extensions to this interface
> 
> Each vendor may have different usage saying id 0x81, so I add
> i.MX dt-schema file.
> 
>>
>>> +
>>> +properties:
>>> +  protocol@81:
>>> +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
>>> +    unevaluatedProperties: false
>>> +    description:
>>> +      The BBM Protocol is for managing Battery Backed Secure Module
>> (BBSM) RTC
>>> +      and the ON/OFF Key
>>> +
>>> +    properties:
>>> +      reg:
>>> +        const: 0x81
>>> +
>>> +    required:
>>> +      - reg
>>> +
>>> +  protocol@84:
>>> +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
>>> +    unevaluatedProperties: false
>>> +    description:
>>> +      The MISC Protocol is for managing SoC Misc settings, such as
>>> + GPR settings
>>
>> Genera register is not a setting... this is a pleonasm. Please be more specific
>> what is the GPR, MISC protocol etc.
> 
> The MISC Protocol is for managing SoC Misc settings, such as SAI MCLK/MQS in
> Always On domain BLK CTRL,  SAI_CLK_SEL in WAKEUP BLK CTRL, gpio
> expanders which is under control of SCMI firmware.

So like a bag for everything which you do not want to call something
specific?

No, be specific...

> 
>>> +
>>> +    properties:
>>> +      reg:
>>> +        const: 0x84
>>> +
>>> +      wakeup-sources:
>>> +        description:
>>> +          Each entry consists of 2 integers, represents the source
>>> + and electric signal edge
>>
>> Can you answer questions from reviewers?
> 
> Sorry. Is this ok?
> minItems: 1
> maxItems: 32

No. Does it answers Rob's question? I see zero correlation to his question.

Do not ignore emails from reviewers but respond to them.

> 
>>
>>> +        items:
>>> +          items:
>>> +            - description: the wakeup source
>>> +            - description: the wakeup electric signal edge
>>> +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>> +
>>> +    required:
>>> +      - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    firmware {
>>> +        scmi {
>>> +            compatible = "arm,scmi";
>>
>>> +            mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>;
>>> +            shmem = <&scmi_buf0>, <&scmi_buf1>;
>>> +
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>> +
>>> +            protocol@81 {
>>> +                reg = <0x81>;
>>> +            };
>>> +
>>> +            protocol@84 {
>>> +                reg = <0x84>;
>>> +                wakeup-sources = <0x8000 1
>>> +                                  0x8001 1
>>> +                                  0x8002 1
>>> +                                  0x8003 1
>>> +                                  0x8004 1>;
>>
>> Nothing improved... If you are going to ignore reviews, then you will only get
>> NAKed.
> 
> Sorry, you mean the examples, or the whole dt-schema?

*Read comments and respond to them*. Regardless where they are.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Peng Fan]

> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> additionalProperties to true
> 
> On 07/04/2024 02:37, Peng Fan wrote:
> >> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> >> additionalProperties to true
> >>
> >> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> >>> From: Peng Fan <peng.fan@nxp.com>
> >>>
> >>> When adding vendor extension protocols, there is dt-schema warning:
> >>> "
> >>> imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do not
> >>> match any of the regexes: 'pinctrl-[0-9]+'
> >>> "
> >>>
> >>> Set additionalProperties to true to address the issue.
> >>
> >> I do not see anything addressed here, except making the binding
> >> accepting anything anywhere...
> >
> > I not wanna add vendor protocols in arm,scmi.yaml, so will introduce a
> > new yaml imx.scmi.yaml which add i.MX SCMI protocol extension.
> >
> > With additionalProperties set to false, I not know how, please suggest.
> 
> First of all, you cannot affect negatively existing devices (their
> bindings) and your patch does exactly that. This should make you thing what
> is the correct approach...
> 
> Rob gave you the comment about missing compatible - you still did not
> address that.

I added the compatible in patch 2/6 in the examples "compatible = "arm,scmi";"
> 
> You need common schema referenced in arm,scmi and your device specific
> schema, also using it.

ok, let me try to figure it out.

Thanks,
Peng.
> 
> 
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension protocol

[Peng Fan]

> Subject: Re: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension
> protocol
> 
> On 07/04/2024 02:51, Peng Fan wrote:
> >> Subject: Re: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI
> >> Extension protocol
> >>
> >> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> >>> From: Peng Fan <peng.fan@nxp.com>
> >>>
> >>> Add i.MX SCMI Extension protocols bindings for:
> >>>  - Battery Backed Secure Module(BBSM)
> >>
> >> Which is what?
> >
> > I should say BBM(BBSM + BBNSM), BBM has RTC and ON/OFF key features,
> > but BBM is managed by SCMI firmware and exported to agent by BBM
> > protocol. So add bindings for i.MX BBM protocol.
> >
> > Is this ok?
> 
> No, I still don't know what is BBSM, BBNSM and BBM.

From RM:
The Battery Backup (BB) Domain contains the Battery Backed
Security Module (BBSM) and the Battery Backed Non-Secure
Module (BBNSM).
BBNSM:
The BBNSM is the interface to a non-interruptable power
supply (backup battery) and serves as the non-volatile logic
and storage for the chip. When the chip is powered off, the
BBNSM will maintain PMIC logic while connected to a backup supply.
Main features: RTC, PMIC Control, ONOFF Control
BBSM serves as nonvolatile security logic and storage for ELE
Main features: Monotonic counter, Secure RTC,
Zeroizable Master Key,
Security Violation and Tamper Detection

> 
> >
> >>
> >>>  - MISC settings such as General Purpose Registers settings.
> >>>
> >>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >>> ---
> >>>  .../devicetree/bindings/firmware/imx,scmi.yaml     | 80
> >> ++++++++++++++++++++++
> >>>  1 file changed, 80 insertions(+)
> >>>
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> >>> b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> >>> new file mode 100644
> >>> index 000000000000..7ee19a661d83
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> >>> @@ -0,0 +1,80 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright
> >>> +2024 NXP %YAML 1.2
> >>> +---
> >>> +$id:
> >>> +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> >>>
> +vi%2F&data=05%7C02%7Cpeng.fan%40nxp.com%7C410d9f1b5b874269dc6
> 908dc5
> >>>
> +6e0b628%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638480
> 77032584
> >>>
> +3394%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> uMzIiLC
> >>>
> +JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=e4zKRcyHbSCtn
> o6%2B%
> >>> +2BBaz%2FGhvPT0HikAdLmwi4VZxX4o%3D&reserved=0
> >>>
> >>
> +cetree.org%2Fschemas%2Ffirmware%2Fimx%2Cscmi.yaml%23&data=05%7
> >> C02%7Cp
> >>>
> >>
> +eng.fan%40nxp.com%7C5d16781d3eca425a342508dc562910b7%7C686ea
> >> 1d3bc2b4c
> >>>
> >>
> +6fa92cd99c5c301635%7C0%7C0%7C638479981570959816%7CUnknown%
> >> 7CTWFpbGZsb
> >>>
> >>
> +3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> >> 0%3D
> >>>
> >>
> +%7C0%7C%7C%7C&sdata=mWNwPvu2eyF18MroVOBHb%2Fjeo%2BIHfV5V
> >> h%2F9ebdx65MM
> >>> +%3D&reserved=0
> >>> +$schema:
> >>> +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> >>>
> +vi%2F&data=05%7C02%7Cpeng.fan%40nxp.com%7C410d9f1b5b874269dc6
> 908dc5
> >>>
> +6e0b628%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638480
> 77032585
> >>>
> +6477%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> uMzIiLC
> >>>
> +JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=y1OBJ%2FPp4
> MljifpmG
> >>> +ZM%2FB6Ab20ivqm2qef7gzEjbNmA%3D&reserved=0
> >>> +cetree.org%2Fmeta-
> >> schemas%2Fcore.yaml%23&data=05%7C02%7Cpeng.fan%40nx
> >>>
> >>
> +p.com%7C5d16781d3eca425a342508dc562910b7%7C686ea1d3bc2b4c6fa
> >> 92cd99c5c
> >>>
> >>
> +301635%7C0%7C0%7C638479981570971949%7CUnknown%7CTWFpbGZs
> >> b3d8eyJWIjoiM
> >>>
> >>
> +C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7
> >> C%7C%7
> >>>
> >>
> +C&sdata=v4XnGG00D4I8j5MJvDUVYMRTm7yRrvz0V3fUyc5KAAA%3D&reser
> >> ved=0
> >>> +
> >>> +title: i.MX System Control and Management Interface(SCMI) Vendor
> >>> +Protocols Extension
> >>> +
> >>> +maintainers:
> >>> +  - Peng Fan <peng.fan@nxp.com>
> >>> +
> >>> +allOf:
> >>> +  - $ref: arm,scmi.yaml#
> >>
> >> Sorry, but arm,scmi is a final schema. Is your plan to define some
> >> common part?
> >
> > No. I just wanna add vendor extension per SCMI spec.
> >
> > 0x80-0xFF:
> > Reserved for vendor or platform-specific extensions to this interface
> >
> > Each vendor may have different usage saying id 0x81, so I add i.MX
> > dt-schema file.
> >
> >>
> >>> +
> >>> +properties:
> >>> +  protocol@81:
> >>> +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> >>> +    unevaluatedProperties: false
> >>> +    description:
> >>> +      The BBM Protocol is for managing Battery Backed Secure Module
> >> (BBSM) RTC
> >>> +      and the ON/OFF Key
> >>> +
> >>> +    properties:
> >>> +      reg:
> >>> +        const: 0x81
> >>> +
> >>> +    required:
> >>> +      - reg
> >>> +
> >>> +  protocol@84:
> >>> +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> >>> +    unevaluatedProperties: false
> >>> +    description:
> >>> +      The MISC Protocol is for managing SoC Misc settings, such as
> >>> + GPR settings
> >>
> >> Genera register is not a setting... this is a pleonasm. Please be
> >> more specific what is the GPR, MISC protocol etc.
> >
> > The MISC Protocol is for managing SoC Misc settings, such as SAI
> > MCLK/MQS in Always On domain BLK CTRL,  SAI_CLK_SEL in WAKEUP BLK
> > CTRL, gpio expanders which is under control of SCMI firmware.
> 
> So like a bag for everything which you do not want to call something specific?
> 
> No, be specific...

This is not linux stuff, this is i.MX SCMI firmware design.

Sadly there is no public RM for i.MX95, we could not afford each settings
has a protocol ID, it is too heavy. The name MISC is not developed for linux,
it is firmware owner decided to use it.

> 
> >
> >>> +
> >>> +    properties:
> >>> +      reg:
> >>> +        const: 0x84
> >>> +
> >>> +      wakeup-sources:
> >>> +        description:
> >>> +          Each entry consists of 2 integers, represents the source
> >>> + and electric signal edge
> >>
> >> Can you answer questions from reviewers?
> >
> > Sorry. Is this ok?
> > minItems: 1
> > maxItems: 32
> 
> No. Does it answers Rob's question? I see zero correlation to his question.

Constraints and edge, right? Edge, I have use electric signal edge, so
what else?

> 
> Do not ignore emails from reviewers but respond to them.
> 
> >
> >>
> >>> +        items:
> >>> +          items:
> >>> +            - description: the wakeup source
> >>> +            - description: the wakeup electric signal edge
> >>> +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
> >>> +
> >>> +    required:
> >>> +      - reg
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    firmware {
> >>> +        scmi {
> >>> +            compatible = "arm,scmi";
> >>
> >>> +            mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>;
> >>> +            shmem = <&scmi_buf0>, <&scmi_buf1>;
> >>> +
> >>> +            #address-cells = <1>;
> >>> +            #size-cells = <0>;
> >>> +
> >>> +            protocol@81 {
> >>> +                reg = <0x81>;
> >>> +            };
> >>> +
> >>> +            protocol@84 {
> >>> +                reg = <0x84>;
> >>> +                wakeup-sources = <0x8000 1
> >>> +                                  0x8001 1
> >>> +                                  0x8002 1
> >>> +                                  0x8003 1
> >>> +                                  0x8004 1>;
> >>
> >> Nothing improved... If you are going to ignore reviews, then you will
> >> only get NAKed.
> >
> > Sorry, you mean the examples, or the whole dt-schema?
> 
> *Read comments and respond to them*. Regardless where they are.

Yeah. My bad.

Thanks,
Peng
> 
> Best regards,
> Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/6] firmware: arm_scmi: add initial support for i.MX MISC protocol

[Marco Felsch]

Hi Peng,

On 24-04-07, Peng Fan wrote:
> > Subject: Re: [PATCH v2 4/6] firmware: arm_scmi: add initial support for i.MX
> > MISC protocol
> > 
> > Hi Peng,
> > 
> > On 24-04-05, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > The i.MX MISC protocol is for misc settings, such as gpio expander
> > > wakeup.
> > 
> > Can you elaborate a bit more please?
> 
> The gpio expander is under M33(SCMI firmware used core) I2C control,

Due to missing technical references I guess that your specific EVK has
an i2c-expander connected to the system-critical-i2c bus? The
system-critical-i2c should be only used for system critical topics like
PMIC control.

> But the gpio expander supports board function such as PCIE_WAKEUP,
> BTN_WAKEUP. So these are managed by MISC protocol.

This seems more like an specific i.MX95-EVK problem too me since you
have conneccted the i2c-gpio-expander to the system-critical-i2c bus
instead of using an bus available within Linux. Also can you please
provide me a link with the propsoal for the MISC protocol? I can't find
any references within the SCMI v3.2
https://developer.arm.com/documentation/den0056/e/ nor within the SCP
firmware git: https://github.com/ARM-software/SCP-firmware.

> SAI_CLK_MSEL in WAKEUP BLK CTRL is also managed by MISC Protocol.

You recently said that we need blk-ctrl drivers for managing/controlling
the GPR stuff within Linux since the SCMI firmware does not support
this. Now blk-ctrl GPR control is supported by the firmware?

Regards,
  Marco

> 
> And etc...
> 
> I will add more info in commit log in next version later, after I get more
> reviews on the patchset.
> 
> Thanks,
> Peng.
> 
> > 
> > Regards,
> >   Marco
> > 
> > 
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/firmware/arm_scmi/Kconfig       |  10 ++
> > >  drivers/firmware/arm_scmi/Makefile      |   1 +
> > >  drivers/firmware/arm_scmi/imx-sm-misc.c | 305
> > ++++++++++++++++++++++++++++++++
> > >  include/linux/scmi_imx_protocol.h       |  17 ++
> > >  4 files changed, 333 insertions(+)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/Kconfig
> > > b/drivers/firmware/arm_scmi/Kconfig
> > > index 56d11c9d9f47..bfeae92f6420 100644
> > > --- a/drivers/firmware/arm_scmi/Kconfig
> > > +++ b/drivers/firmware/arm_scmi/Kconfig
> > > @@ -191,3 +191,13 @@ config IMX_SCMI_BBM_EXT
> > >  	  and BUTTON.
> > >
> > >  	  This driver can also be built as a module.
> > > +
> > > +config IMX_SCMI_MISC_EXT
> > > +	tristate "i.MX SCMI MISC EXTENSION"
> > > +	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> > > +	default y if ARCH_MXC
> > > +	help
> > > +	  This enables i.MX System MISC control logic such as gpio expander
> > > +	  wakeup
> > > +
> > > +	  This driver can also be built as a module.
> > > diff --git a/drivers/firmware/arm_scmi/Makefile
> > > b/drivers/firmware/arm_scmi/Makefile
> > > index 327687acf857..a23fde721222 100644
> > > --- a/drivers/firmware/arm_scmi/Makefile
> > > +++ b/drivers/firmware/arm_scmi/Makefile
> > > @@ -12,6 +12,7 @@ scmi-transport-
> > $(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO)
> > > += virtio.o
> > >  scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > > scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
> > > system.o voltage.o powercap.o
> > >  scmi-protocols-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.o
> > > +scmi-protocols-$(CONFIG_IMX_SCMI_MISC_EXT) += imx-sm-misc.o
> > >  scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y)
> > > $(scmi-transport-y)
> > >
> > >  obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o diff --git
> > > a/drivers/firmware/arm_scmi/imx-sm-misc.c
> > > b/drivers/firmware/arm_scmi/imx-sm-misc.c
> > > new file mode 100644
> > > index 000000000000..1b0ec2281518
> > > --- /dev/null
> > > +++ b/drivers/firmware/arm_scmi/imx-sm-misc.c
> > > @@ -0,0 +1,305 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * System control and Management Interface (SCMI) NXP MISC Protocol
> > > + *
> > > + * Copyright 2024 NXP
> > > + */
> > > +
> > > +#define pr_fmt(fmt) "SCMI Notifications MISC - " fmt
> > > +
> > > +#include <linux/bits.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/scmi_protocol.h>
> > > +#include <linux/scmi_imx_protocol.h>
> > > +
> > > +#include "protocols.h"
> > > +#include "notify.h"
> > > +
> > > +#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x10000
> > > +
> > > +enum scmi_imx_misc_protocol_cmd {
> > > +	SCMI_IMX_MISC_CTRL_SET	= 0x3,
> > > +	SCMI_IMX_MISC_CTRL_GET	= 0x4,
> > > +	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
> > > +};
> > > +
> > > +struct scmi_imx_misc_info {
> > > +	u32 version;
> > > +	u32 nr_dev_ctrl;
> > > +	u32 nr_brd_ctrl;
> > > +	u32 nr_reason;
> > > +};
> > > +
> > > +struct scmi_msg_imx_misc_protocol_attributes {
> > > +	__le32 attributes;
> > > +};
> > > +
> > > +#define GET_BRD_CTRLS_NR(x)	le32_get_bits((x), GENMASK(31,
> > 24))
> > > +#define GET_REASONS_NR(x)	le32_get_bits((x), GENMASK(23, 16))
> > > +#define GET_DEV_CTRLS_NR(x)	le32_get_bits((x), GENMASK(15, 0))
> > > +#define BRD_CTRL_START_ID	BIT(15)
> > > +
> > > +struct scmi_imx_misc_ctrl_set_in {
> > > +	__le32 id;
> > > +	__le32 num;
> > > +	__le32 value[MISC_MAX_VAL];
> > > +};
> > > +
> > > +struct scmi_imx_misc_ctrl_notify_in {
> > > +	__le32 ctrl_id;
> > > +	__le32 flags;
> > > +};
> > > +
> > > +struct scmi_imx_misc_ctrl_notify_payld {
> > > +	__le32 ctrl_id;
> > > +	__le32 flags;
> > > +};
> > > +
> > > +struct scmi_imx_misc_ctrl_get_out {
> > > +	__le32 num;
> > > +	__le32 *val;
> > > +};
> > > +
> > > +static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle
> > *ph,
> > > +					struct scmi_imx_misc_info *mi)
> > > +{
> > > +	int ret;
> > > +	struct scmi_xfer *t;
> > > +	struct scmi_msg_imx_misc_protocol_attributes *attr;
> > > +
> > > +	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
> > > +				      sizeof(*attr), &t);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	attr = t->rx.buf;
> > > +
> > > +	ret = ph->xops->do_xfer(ph, t);
> > > +	if (!ret) {
> > > +		mi->nr_dev_ctrl = GET_DEV_CTRLS_NR(attr->attributes);
> > > +		mi->nr_brd_ctrl = GET_BRD_CTRLS_NR(attr->attributes);
> > > +		mi->nr_reason = GET_REASONS_NR(attr->attributes);
> > > +		dev_info(ph->dev, "i.MX MISC NUM DEV CTRL: %d, NUM
> > BRD CTRL: %d,NUM Reason: %d\n",
> > > +			 mi->nr_dev_ctrl, mi->nr_brd_ctrl, mi->nr_reason);
> > > +	}
> > > +
> > > +	ph->xops->xfer_put(ph, t);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int scmi_imx_misc_ctrl_validate_id(const struct
> > scmi_protocol_handle *ph,
> > > +					  u32 ctrl_id)
> > > +{
> > > +	struct scmi_imx_misc_info *mi = ph->get_priv(ph);
> > > +
> > > +	if ((ctrl_id < BRD_CTRL_START_ID) && (ctrl_id > mi->nr_dev_ctrl))
> > > +		return -EINVAL;
> > > +	if (ctrl_id >= BRD_CTRL_START_ID + mi->nr_brd_ctrl)
> > > +		return -EINVAL;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int scmi_imx_misc_ctrl_notify(const struct scmi_protocol_handle
> > *ph,
> > > +				     u32 ctrl_id, u32 flags)
> > > +{
> > > +	struct scmi_imx_misc_ctrl_notify_in *in;
> > > +	struct scmi_xfer *t;
> > > +	int ret;
> > > +
> > > +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_NOTIFY,
> > > +				      sizeof(*in), 0, &t);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	in = t->tx.buf;
> > > +	in->ctrl_id = cpu_to_le32(ctrl_id);
> > > +	in->flags = cpu_to_le32(flags);
> > > +
> > > +	ret = ph->xops->do_xfer(ph, t);
> > > +
> > > +	ph->xops->xfer_put(ph, t);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int
> > > +scmi_imx_misc_ctrl_set_notify_enabled(const struct
> > scmi_protocol_handle *ph,
> > > +				      u8 evt_id, u32 src_id, bool enable) {
> > > +	int ret;
> > > +
> > > +	ret = scmi_imx_misc_ctrl_notify(ph, src_id, enable ? evt_id : 0);
> > > +	if (ret)
> > > +		dev_err(ph->dev, "FAIL_ENABLED - evt[%X] src[%d] -
> > ret:%d\n",
> > > +			evt_id, src_id, ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int scmi_imx_misc_ctrl_get_num_sources(const struct
> > > +scmi_protocol_handle *ph) {
> > > +	return GENMASK(15, 0);
> > > +}
> > > +
> > > +static void *
> > > +scmi_imx_misc_ctrl_fill_custom_report(const struct scmi_protocol_handle
> > *ph,
> > > +				      u8 evt_id, ktime_t timestamp,
> > > +				      const void *payld, size_t payld_sz,
> > > +				      void *report, u32 *src_id)
> > > +{
> > > +	const struct scmi_imx_misc_ctrl_notify_payld *p = payld;
> > > +	struct scmi_imx_misc_ctrl_notify_report *r = report;
> > > +
> > > +	if (sizeof(*p) != payld_sz)
> > > +		return NULL;
> > > +
> > > +	r->timestamp = timestamp;
> > > +	r->ctrl_id = p->ctrl_id;
> > > +	r->flags = p->flags;
> > > +	*src_id = r->ctrl_id;
> > > +	dev_dbg(ph->dev, "%s: ctrl_id: %d flags: %d\n", __func__,
> > > +		r->ctrl_id, r->flags);
> > > +
> > > +	return r;
> > > +}
> > > +
> > > +static const struct scmi_event_ops scmi_imx_misc_event_ops = {
> > > +	.get_num_sources = scmi_imx_misc_ctrl_get_num_sources,
> > > +	.set_notify_enabled = scmi_imx_misc_ctrl_set_notify_enabled,
> > > +	.fill_custom_report = scmi_imx_misc_ctrl_fill_custom_report,
> > > +};
> > > +
> > > +static const struct scmi_event scmi_imx_misc_events[] = {
> > > +	{
> > > +		.id = SCMI_EVENT_IMX_MISC_CONTROL_DISABLED,
> > > +		.max_payld_sz = sizeof(struct
> > scmi_imx_misc_ctrl_notify_payld),
> > > +		.max_report_sz = sizeof(struct
> > scmi_imx_misc_ctrl_notify_report),
> > > +	},
> > > +	{
> > > +		.id = SCMI_EVENT_IMX_MISC_CONTROL_FALLING_EDGE,
> > > +		.max_payld_sz = sizeof(struct
> > scmi_imx_misc_ctrl_notify_payld),
> > > +		.max_report_sz = sizeof(struct
> > scmi_imx_misc_ctrl_notify_report),
> > > +	},
> > > +	{
> > > +		.id = SCMI_EVENT_IMX_MISC_CONTROL_RISING_EDGE,
> > > +		.max_payld_sz = sizeof(struct
> > scmi_imx_misc_ctrl_notify_payld),
> > > +		.max_report_sz = sizeof(struct
> > scmi_imx_misc_ctrl_notify_report),
> > > +	}
> > > +};
> > > +
> > > +static struct scmi_protocol_events scmi_imx_misc_protocol_events = {
> > > +	.queue_sz = SCMI_PROTO_QUEUE_SZ,
> > > +	.ops = &scmi_imx_misc_event_ops,
> > > +	.evts = scmi_imx_misc_events,
> > > +	.num_events = ARRAY_SIZE(scmi_imx_misc_events), };
> > > +
> > > +static int scmi_imx_misc_protocol_init(const struct
> > > +scmi_protocol_handle *ph) {
> > > +	struct scmi_imx_misc_info *minfo;
> > > +	u32 version;
> > > +	int ret;
> > > +
> > > +	ret = ph->xops->version_get(ph, &version);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	dev_info(ph->dev, "NXP SM MISC Version %d.%d\n",
> > > +		 PROTOCOL_REV_MAJOR(version),
> > PROTOCOL_REV_MINOR(version));
> > > +
> > > +	minfo = devm_kzalloc(ph->dev, sizeof(*minfo), GFP_KERNEL);
> > > +	if (!minfo)
> > > +		return -ENOMEM;
> > > +
> > > +	ret = scmi_imx_misc_attributes_get(ph, minfo);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return ph->set_priv(ph, minfo, version); }
> > > +
> > > +static int scmi_imx_misc_ctrl_get(const struct scmi_protocol_handle *ph,
> > > +				  u32 ctrl_id, u32 *num, u32 *val) {
> > > +	struct scmi_imx_misc_ctrl_get_out *out;
> > > +	struct scmi_xfer *t;
> > > +	int ret, i;
> > > +
> > > +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_GET,
> > sizeof(u32),
> > > +				      0, &t);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	put_unaligned_le32(ctrl_id, t->tx.buf);
> > > +	ret = ph->xops->do_xfer(ph, t);
> > > +	if (!ret) {
> > > +		out = t->rx.buf;
> > > +		*num = le32_to_cpu(out->num);
> > > +		for (i = 0; i < *num && i < MISC_MAX_VAL; i++)
> > > +			val[i] = le32_to_cpu(out->val[i]);
> > > +	}
> > > +
> > > +	ph->xops->xfer_put(ph, t);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle *ph,
> > > +				  u32 ctrl_id, u32 num, u32 *val) {
> > > +	struct scmi_imx_misc_ctrl_set_in *in;
> > > +	struct scmi_xfer *t;
> > > +	int ret, i;
> > > +
> > > +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (num > MISC_MAX_VAL)
> > > +		return -EINVAL;
> > > +
> > > +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_SET,
> > sizeof(*in),
> > > +				      0, &t);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	in = t->tx.buf;
> > > +	in->id = cpu_to_le32(ctrl_id);
> > > +	in->num = cpu_to_le32(num);
> > > +	for (i = 0; i < num; i++)
> > > +		in->value[i] = cpu_to_le32(val[i]);
> > > +
> > > +	ret = ph->xops->do_xfer(ph, t);
> > > +
> > > +	ph->xops->xfer_put(ph, t);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops =
> > {
> > > +	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
> > > +	.misc_ctrl_get = scmi_imx_misc_ctrl_get, };
> > > +
> > > +static const struct scmi_protocol scmi_imx_misc = {
> > > +	.id = SCMI_PROTOCOL_IMX_MISC,
> > > +	.owner = THIS_MODULE,
> > > +	.instance_init = &scmi_imx_misc_protocol_init,
> > > +	.ops = &scmi_imx_misc_proto_ops,
> > > +	.events = &scmi_imx_misc_protocol_events,
> > > +	.supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION, };
> > > +module_scmi_protocol(scmi_imx_misc);
> > > diff --git a/include/linux/scmi_imx_protocol.h
> > > b/include/linux/scmi_imx_protocol.h
> > > index 90ce011a4429..a69bd4a20f0f 100644
> > > --- a/include/linux/scmi_imx_protocol.h
> > > +++ b/include/linux/scmi_imx_protocol.h
> > > @@ -13,8 +13,14 @@
> > >  #include <linux/notifier.h>
> > >  #include <linux/types.h>
> > >
> > > +#define SCMI_PAYLOAD_LEN	100
> > > +
> > > +#define SCMI_ARRAY(X, Y)	((SCMI_PAYLOAD_LEN - (X)) / sizeof(Y))
> > > +#define MISC_MAX_VAL		SCMI_ARRAY(8, uint32_t)
> > > +
> > >  enum scmi_nxp_protocol {
> > >  	SCMI_PROTOCOL_IMX_BBM = 0x81,
> > > +	SCMI_PROTOCOL_IMX_MISC = 0x84,
> > >  };
> > >
> > >  struct scmi_imx_bbm_proto_ops {
> > > @@ -42,4 +48,15 @@ struct scmi_imx_bbm_notif_report {
> > >  	unsigned int		rtc_id;
> > >  	unsigned int		rtc_evt;
> > >  };
> > > +
> > > +struct scmi_imx_misc_ctrl_notify_report {
> > > +	ktime_t			timestamp;
> > > +	unsigned int		ctrl_id;
> > > +	unsigned int		flags;
> > > +};
> > > +
> > > +struct scmi_imx_misc_proto_ops {
> > > +	int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32 id,
> > u32 num, u32 *val);
> > > +	int (*misc_ctrl_get)(const struct scmi_protocol_handle *ph, u32 id,
> > > +u32 *num, u32 *val); };
> > >  #endif
> > >
> > > --
> > > 2.37.1
> > >
> > >
> > >
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

> Subject: Re: [PATCH v2 4/6] firmware: arm_scmi: add initial support for i.MX
> MISC protocol
> 
> Hi Peng,
> 
> On 24-04-07, Peng Fan wrote:
> > > Subject: Re: [PATCH v2 4/6] firmware: arm_scmi: add initial support
> > > for i.MX MISC protocol
> > >
> > > Hi Peng,
> > >
> > > On 24-04-05, Peng Fan (OSS) wrote:
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > The i.MX MISC protocol is for misc settings, such as gpio expander
> > > > wakeup.
> > >
> > > Can you elaborate a bit more please?
> >
> > The gpio expander is under M33(SCMI firmware used core) I2C control,
> 
> Due to missing technical references I guess that your specific EVK has an i2c-
> expander connected to the system-critical-i2c bus? The system-critical-i2c
> should be only used for system critical topics like PMIC control.

Right.

> 
> > But the gpio expander supports board function such as PCIE_WAKEUP,
> > BTN_WAKEUP. So these are managed by MISC protocol.
> 
> This seems more like an specific i.MX95-EVK problem too me since you have
> conneccted the i2c-gpio-expander to the system-critical-i2c bus instead of
> using an bus available within Linux. Also can you please provide me a link
> with the propsoal for the MISC protocol? I can't find any references within the
> SCMI v3.2

It is i.MX VENDOR Extension, not a standard one in Spec.

> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdevelo
> per.arm.com%2Fdocumentation%2Fden0056%2Fe%2F&data=05%7C02%7Cp
> eng.fan%40nxp.com%7C6120357a772045a0618808dc56f22c95%7C686ea1d
> 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638480845336536607%7CUnk
> nown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik
> 1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=NI%2F8WMPuGzJwD74
> 1jcuknHZUR5uI2me9iEeWbeDKshE%3D&reserved=0 nor within the SCP
> firmware git:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .com%2FARM-software%2FSCP-
> firmware&data=05%7C02%7Cpeng.fan%40nxp.com%7C6120357a772045a06
> 18808dc56f22c95%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
> 638480845336550459%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C
> &sdata=N3bT9ItgvL4Z9xP1oxlmDTG%2FFjsXkuhJIA9wooJWfcM%3D&reserve
> d=0.
> 
> > SAI_CLK_MSEL in WAKEUP BLK CTRL is also managed by MISC Protocol.
> 
> You recently said that we need blk-ctrl drivers for managing/controlling the
> GPR stuff within Linux since the SCMI firmware does not support this. Now
> blk-ctrl GPR control is supported by the firmware?

AONMIX/WAKEUPMIX BLK CTRL is managed by SCMI firmware, for other
non system critical BLK CTRLs, they are managed by Linux directly, such as
GPU/VPU BLK CTRL and etc.

Regards,
Peng.
> 
> Regards,
>   Marco
> 
> >
> > And etc...
> >
> > I will add more info in commit log in next version later, after I get
> > more reviews on the patchset.
> >
> > Thanks,
> > Peng.
> >
> > >
> > > Regards,
> > >   Marco
> > >
> > >
> > > >
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > > >  drivers/firmware/arm_scmi/Kconfig       |  10 ++
> > > >  drivers/firmware/arm_scmi/Makefile      |   1 +
> > > >  drivers/firmware/arm_scmi/imx-sm-misc.c | 305
> > > ++++++++++++++++++++++++++++++++
> > > >  include/linux/scmi_imx_protocol.h       |  17 ++
> > > >  4 files changed, 333 insertions(+)
> > > >
> > > > diff --git a/drivers/firmware/arm_scmi/Kconfig
> > > > b/drivers/firmware/arm_scmi/Kconfig
> > > > index 56d11c9d9f47..bfeae92f6420 100644
> > > > --- a/drivers/firmware/arm_scmi/Kconfig
> > > > +++ b/drivers/firmware/arm_scmi/Kconfig
> > > > @@ -191,3 +191,13 @@ config IMX_SCMI_BBM_EXT
> > > >  	  and BUTTON.
> > > >
> > > >  	  This driver can also be built as a module.
> > > > +
> > > > +config IMX_SCMI_MISC_EXT
> > > > +	tristate "i.MX SCMI MISC EXTENSION"
> > > > +	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> > > > +	default y if ARCH_MXC
> > > > +	help
> > > > +	  This enables i.MX System MISC control logic such as gpio expander
> > > > +	  wakeup
> > > > +
> > > > +	  This driver can also be built as a module.
> > > > diff --git a/drivers/firmware/arm_scmi/Makefile
> > > > b/drivers/firmware/arm_scmi/Makefile
> > > > index 327687acf857..a23fde721222 100644
> > > > --- a/drivers/firmware/arm_scmi/Makefile
> > > > +++ b/drivers/firmware/arm_scmi/Makefile
> > > > @@ -12,6 +12,7 @@ scmi-transport-
> > > $(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO)
> > > > += virtio.o
> > > >  scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > > > scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
> > > > system.o voltage.o powercap.o
> > > >  scmi-protocols-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.o
> > > > +scmi-protocols-$(CONFIG_IMX_SCMI_MISC_EXT) += imx-sm-misc.o
> > > >  scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y)
> > > > $(scmi-transport-y)
> > > >
> > > >  obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o diff --git
> > > > a/drivers/firmware/arm_scmi/imx-sm-misc.c
> > > > b/drivers/firmware/arm_scmi/imx-sm-misc.c
> > > > new file mode 100644
> > > > index 000000000000..1b0ec2281518
> > > > --- /dev/null
> > > > +++ b/drivers/firmware/arm_scmi/imx-sm-misc.c
> > > > @@ -0,0 +1,305 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * System control and Management Interface (SCMI) NXP MISC
> > > > +Protocol
> > > > + *
> > > > + * Copyright 2024 NXP
> > > > + */
> > > > +
> > > > +#define pr_fmt(fmt) "SCMI Notifications MISC - " fmt
> > > > +
> > > > +#include <linux/bits.h>
> > > > +#include <linux/io.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/platform_device.h> #include
> > > > +<linux/scmi_protocol.h> #include <linux/scmi_imx_protocol.h>
> > > > +
> > > > +#include "protocols.h"
> > > > +#include "notify.h"
> > > > +
> > > > +#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x10000
> > > > +
> > > > +enum scmi_imx_misc_protocol_cmd {
> > > > +	SCMI_IMX_MISC_CTRL_SET	= 0x3,
> > > > +	SCMI_IMX_MISC_CTRL_GET	= 0x4,
> > > > +	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8, };
> > > > +
> > > > +struct scmi_imx_misc_info {
> > > > +	u32 version;
> > > > +	u32 nr_dev_ctrl;
> > > > +	u32 nr_brd_ctrl;
> > > > +	u32 nr_reason;
> > > > +};
> > > > +
> > > > +struct scmi_msg_imx_misc_protocol_attributes {
> > > > +	__le32 attributes;
> > > > +};
> > > > +
> > > > +#define GET_BRD_CTRLS_NR(x)	le32_get_bits((x), GENMASK(31,
> > > 24))
> > > > +#define GET_REASONS_NR(x)	le32_get_bits((x), GENMASK(23,
> 16))
> > > > +#define GET_DEV_CTRLS_NR(x)	le32_get_bits((x), GENMASK(15, 0))
> > > > +#define BRD_CTRL_START_ID	BIT(15)
> > > > +
> > > > +struct scmi_imx_misc_ctrl_set_in {
> > > > +	__le32 id;
> > > > +	__le32 num;
> > > > +	__le32 value[MISC_MAX_VAL];
> > > > +};
> > > > +
> > > > +struct scmi_imx_misc_ctrl_notify_in {
> > > > +	__le32 ctrl_id;
> > > > +	__le32 flags;
> > > > +};
> > > > +
> > > > +struct scmi_imx_misc_ctrl_notify_payld {
> > > > +	__le32 ctrl_id;
> > > > +	__le32 flags;
> > > > +};
> > > > +
> > > > +struct scmi_imx_misc_ctrl_get_out {
> > > > +	__le32 num;
> > > > +	__le32 *val;
> > > > +};
> > > > +
> > > > +static int scmi_imx_misc_attributes_get(const struct
> > > > +scmi_protocol_handle
> > > *ph,
> > > > +					struct scmi_imx_misc_info *mi) {
> > > > +	int ret;
> > > > +	struct scmi_xfer *t;
> > > > +	struct scmi_msg_imx_misc_protocol_attributes *attr;
> > > > +
> > > > +	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
> > > > +				      sizeof(*attr), &t);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	attr = t->rx.buf;
> > > > +
> > > > +	ret = ph->xops->do_xfer(ph, t);
> > > > +	if (!ret) {
> > > > +		mi->nr_dev_ctrl = GET_DEV_CTRLS_NR(attr->attributes);
> > > > +		mi->nr_brd_ctrl = GET_BRD_CTRLS_NR(attr->attributes);
> > > > +		mi->nr_reason = GET_REASONS_NR(attr->attributes);
> > > > +		dev_info(ph->dev, "i.MX MISC NUM DEV CTRL: %d, NUM
> > > BRD CTRL: %d,NUM Reason: %d\n",
> > > > +			 mi->nr_dev_ctrl, mi->nr_brd_ctrl, mi->nr_reason);
> > > > +	}
> > > > +
> > > > +	ph->xops->xfer_put(ph, t);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int scmi_imx_misc_ctrl_validate_id(const struct
> > > scmi_protocol_handle *ph,
> > > > +					  u32 ctrl_id)
> > > > +{
> > > > +	struct scmi_imx_misc_info *mi = ph->get_priv(ph);
> > > > +
> > > > +	if ((ctrl_id < BRD_CTRL_START_ID) && (ctrl_id > mi->nr_dev_ctrl))
> > > > +		return -EINVAL;
> > > > +	if (ctrl_id >= BRD_CTRL_START_ID + mi->nr_brd_ctrl)
> > > > +		return -EINVAL;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int scmi_imx_misc_ctrl_notify(const struct
> > > > +scmi_protocol_handle
> > > *ph,
> > > > +				     u32 ctrl_id, u32 flags)
> > > > +{
> > > > +	struct scmi_imx_misc_ctrl_notify_in *in;
> > > > +	struct scmi_xfer *t;
> > > > +	int ret;
> > > > +
> > > > +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_NOTIFY,
> > > > +				      sizeof(*in), 0, &t);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	in = t->tx.buf;
> > > > +	in->ctrl_id = cpu_to_le32(ctrl_id);
> > > > +	in->flags = cpu_to_le32(flags);
> > > > +
> > > > +	ret = ph->xops->do_xfer(ph, t);
> > > > +
> > > > +	ph->xops->xfer_put(ph, t);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int
> > > > +scmi_imx_misc_ctrl_set_notify_enabled(const struct
> > > scmi_protocol_handle *ph,
> > > > +				      u8 evt_id, u32 src_id, bool enable) {
> > > > +	int ret;
> > > > +
> > > > +	ret = scmi_imx_misc_ctrl_notify(ph, src_id, enable ? evt_id : 0);
> > > > +	if (ret)
> > > > +		dev_err(ph->dev, "FAIL_ENABLED - evt[%X] src[%d] -
> > > ret:%d\n",
> > > > +			evt_id, src_id, ret);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int scmi_imx_misc_ctrl_get_num_sources(const struct
> > > > +scmi_protocol_handle *ph) {
> > > > +	return GENMASK(15, 0);
> > > > +}
> > > > +
> > > > +static void *
> > > > +scmi_imx_misc_ctrl_fill_custom_report(const struct
> > > > +scmi_protocol_handle
> > > *ph,
> > > > +				      u8 evt_id, ktime_t timestamp,
> > > > +				      const void *payld, size_t payld_sz,
> > > > +				      void *report, u32 *src_id) {
> > > > +	const struct scmi_imx_misc_ctrl_notify_payld *p = payld;
> > > > +	struct scmi_imx_misc_ctrl_notify_report *r = report;
> > > > +
> > > > +	if (sizeof(*p) != payld_sz)
> > > > +		return NULL;
> > > > +
> > > > +	r->timestamp = timestamp;
> > > > +	r->ctrl_id = p->ctrl_id;
> > > > +	r->flags = p->flags;
> > > > +	*src_id = r->ctrl_id;
> > > > +	dev_dbg(ph->dev, "%s: ctrl_id: %d flags: %d\n", __func__,
> > > > +		r->ctrl_id, r->flags);
> > > > +
> > > > +	return r;
> > > > +}
> > > > +
> > > > +static const struct scmi_event_ops scmi_imx_misc_event_ops = {
> > > > +	.get_num_sources = scmi_imx_misc_ctrl_get_num_sources,
> > > > +	.set_notify_enabled = scmi_imx_misc_ctrl_set_notify_enabled,
> > > > +	.fill_custom_report = scmi_imx_misc_ctrl_fill_custom_report,
> > > > +};
> > > > +
> > > > +static const struct scmi_event scmi_imx_misc_events[] = {
> > > > +	{
> > > > +		.id = SCMI_EVENT_IMX_MISC_CONTROL_DISABLED,
> > > > +		.max_payld_sz = sizeof(struct
> > > scmi_imx_misc_ctrl_notify_payld),
> > > > +		.max_report_sz = sizeof(struct
> > > scmi_imx_misc_ctrl_notify_report),
> > > > +	},
> > > > +	{
> > > > +		.id = SCMI_EVENT_IMX_MISC_CONTROL_FALLING_EDGE,
> > > > +		.max_payld_sz = sizeof(struct
> > > scmi_imx_misc_ctrl_notify_payld),
> > > > +		.max_report_sz = sizeof(struct
> > > scmi_imx_misc_ctrl_notify_report),
> > > > +	},
> > > > +	{
> > > > +		.id = SCMI_EVENT_IMX_MISC_CONTROL_RISING_EDGE,
> > > > +		.max_payld_sz = sizeof(struct
> > > scmi_imx_misc_ctrl_notify_payld),
> > > > +		.max_report_sz = sizeof(struct
> > > scmi_imx_misc_ctrl_notify_report),
> > > > +	}
> > > > +};
> > > > +
> > > > +static struct scmi_protocol_events scmi_imx_misc_protocol_events = {
> > > > +	.queue_sz = SCMI_PROTO_QUEUE_SZ,
> > > > +	.ops = &scmi_imx_misc_event_ops,
> > > > +	.evts = scmi_imx_misc_events,
> > > > +	.num_events = ARRAY_SIZE(scmi_imx_misc_events), };
> > > > +
> > > > +static int scmi_imx_misc_protocol_init(const struct
> > > > +scmi_protocol_handle *ph) {
> > > > +	struct scmi_imx_misc_info *minfo;
> > > > +	u32 version;
> > > > +	int ret;
> > > > +
> > > > +	ret = ph->xops->version_get(ph, &version);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	dev_info(ph->dev, "NXP SM MISC Version %d.%d\n",
> > > > +		 PROTOCOL_REV_MAJOR(version),
> > > PROTOCOL_REV_MINOR(version));
> > > > +
> > > > +	minfo = devm_kzalloc(ph->dev, sizeof(*minfo), GFP_KERNEL);
> > > > +	if (!minfo)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = scmi_imx_misc_attributes_get(ph, minfo);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	return ph->set_priv(ph, minfo, version); }
> > > > +
> > > > +static int scmi_imx_misc_ctrl_get(const struct scmi_protocol_handle
> *ph,
> > > > +				  u32 ctrl_id, u32 *num, u32 *val) {
> > > > +	struct scmi_imx_misc_ctrl_get_out *out;
> > > > +	struct scmi_xfer *t;
> > > > +	int ret, i;
> > > > +
> > > > +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_GET,
> > > sizeof(u32),
> > > > +				      0, &t);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	put_unaligned_le32(ctrl_id, t->tx.buf);
> > > > +	ret = ph->xops->do_xfer(ph, t);
> > > > +	if (!ret) {
> > > > +		out = t->rx.buf;
> > > > +		*num = le32_to_cpu(out->num);
> > > > +		for (i = 0; i < *num && i < MISC_MAX_VAL; i++)
> > > > +			val[i] = le32_to_cpu(out->val[i]);
> > > > +	}
> > > > +
> > > > +	ph->xops->xfer_put(ph, t);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle
> *ph,
> > > > +				  u32 ctrl_id, u32 num, u32 *val) {
> > > > +	struct scmi_imx_misc_ctrl_set_in *in;
> > > > +	struct scmi_xfer *t;
> > > > +	int ret, i;
> > > > +
> > > > +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	if (num > MISC_MAX_VAL)
> > > > +		return -EINVAL;
> > > > +
> > > > +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_SET,
> > > sizeof(*in),
> > > > +				      0, &t);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	in = t->tx.buf;
> > > > +	in->id = cpu_to_le32(ctrl_id);
> > > > +	in->num = cpu_to_le32(num);
> > > > +	for (i = 0; i < num; i++)
> > > > +		in->value[i] = cpu_to_le32(val[i]);
> > > > +
> > > > +	ret = ph->xops->do_xfer(ph, t);
> > > > +
> > > > +	ph->xops->xfer_put(ph, t);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static const struct scmi_imx_misc_proto_ops
> > > > +scmi_imx_misc_proto_ops =
> > > {
> > > > +	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
> > > > +	.misc_ctrl_get = scmi_imx_misc_ctrl_get, };
> > > > +
> > > > +static const struct scmi_protocol scmi_imx_misc = {
> > > > +	.id = SCMI_PROTOCOL_IMX_MISC,
> > > > +	.owner = THIS_MODULE,
> > > > +	.instance_init = &scmi_imx_misc_protocol_init,
> > > > +	.ops = &scmi_imx_misc_proto_ops,
> > > > +	.events = &scmi_imx_misc_protocol_events,
> > > > +	.supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION, };
> > > > +module_scmi_protocol(scmi_imx_misc);
> > > > diff --git a/include/linux/scmi_imx_protocol.h
> > > > b/include/linux/scmi_imx_protocol.h
> > > > index 90ce011a4429..a69bd4a20f0f 100644
> > > > --- a/include/linux/scmi_imx_protocol.h
> > > > +++ b/include/linux/scmi_imx_protocol.h
> > > > @@ -13,8 +13,14 @@
> > > >  #include <linux/notifier.h>
> > > >  #include <linux/types.h>
> > > >
> > > > +#define SCMI_PAYLOAD_LEN	100
> > > > +
> > > > +#define SCMI_ARRAY(X, Y)	((SCMI_PAYLOAD_LEN - (X)) / sizeof(Y))
> > > > +#define MISC_MAX_VAL		SCMI_ARRAY(8, uint32_t)
> > > > +
> > > >  enum scmi_nxp_protocol {
> > > >  	SCMI_PROTOCOL_IMX_BBM = 0x81,
> > > > +	SCMI_PROTOCOL_IMX_MISC = 0x84,
> > > >  };
> > > >
> > > >  struct scmi_imx_bbm_proto_ops {
> > > > @@ -42,4 +48,15 @@ struct scmi_imx_bbm_notif_report {
> > > >  	unsigned int		rtc_id;
> > > >  	unsigned int		rtc_evt;
> > > >  };
> > > > +
> > > > +struct scmi_imx_misc_ctrl_notify_report {
> > > > +	ktime_t			timestamp;
> > > > +	unsigned int		ctrl_id;
> > > > +	unsigned int		flags;
> > > > +};
> > > > +
> > > > +struct scmi_imx_misc_proto_ops {
> > > > +	int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32
> > > > +id,
> > > u32 num, u32 *val);
> > > > +	int (*misc_ctrl_get)(const struct scmi_protocol_handle *ph, u32
> > > > +id,
> > > > +u32 *num, u32 *val); };
> > > >  #endif
> > > >
> > > > --
> > > > 2.37.1
> > > >
> > > >
> > > >
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Krzysztof Kozlowski]

On 07/04/2024 12:04, Peng Fan wrote:
>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
>> additionalProperties to true
>>
>> On 07/04/2024 02:37, Peng Fan wrote:
>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
>>>> additionalProperties to true
>>>>
>>>> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
>>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>>
>>>>> When adding vendor extension protocols, there is dt-schema warning:
>>>>> "
>>>>> imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do not
>>>>> match any of the regexes: 'pinctrl-[0-9]+'
>>>>> "
>>>>>
>>>>> Set additionalProperties to true to address the issue.
>>>>
>>>> I do not see anything addressed here, except making the binding
>>>> accepting anything anywhere...
>>>
>>> I not wanna add vendor protocols in arm,scmi.yaml, so will introduce a
>>> new yaml imx.scmi.yaml which add i.MX SCMI protocol extension.
>>>
>>> With additionalProperties set to false, I not know how, please suggest.
>>
>> First of all, you cannot affect negatively existing devices (their
>> bindings) and your patch does exactly that. This should make you thing what
>> is the correct approach...
>>
>> Rob gave you the comment about missing compatible - you still did not
>> address that.
> 
> I added the compatible in patch 2/6 in the examples "compatible = "arm,scmi";"

So you claim that your vendor extensions are the same or fully
compatible with arm,scmi and you add nothing... Are your
extensions/protocol valid for arm,scmi? If yes, why is this in separate
binding. If no, why you use someone else's compatible?

Maybe your binding is correct, feel free to convince me (and read first
writing bindings).

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Peng Fan]

> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> additionalProperties to true
> 
> On 07/04/2024 12:04, Peng Fan wrote:
> >> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> >> additionalProperties to true
> >>
> >> On 07/04/2024 02:37, Peng Fan wrote:
> >>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> >>>> additionalProperties to true
> >>>>
> >>>> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> >>>>> From: Peng Fan <peng.fan@nxp.com>
> >>>>>
> >>>>> When adding vendor extension protocols, there is dt-schema warning:
> >>>>> "
> >>>>> imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do not
> >>>>> match any of the regexes: 'pinctrl-[0-9]+'
> >>>>> "
> >>>>>
> >>>>> Set additionalProperties to true to address the issue.
> >>>>
> >>>> I do not see anything addressed here, except making the binding
> >>>> accepting anything anywhere...
> >>>
> >>> I not wanna add vendor protocols in arm,scmi.yaml, so will introduce
> >>> a new yaml imx.scmi.yaml which add i.MX SCMI protocol extension.
> >>>
> >>> With additionalProperties set to false, I not know how, please suggest.
> >>
> >> First of all, you cannot affect negatively existing devices (their
> >> bindings) and your patch does exactly that. This should make you
> >> thing what is the correct approach...
> >>
> >> Rob gave you the comment about missing compatible - you still did not
> >> address that.
> >
> > I added the compatible in patch 2/6 in the examples "compatible =
> "arm,scmi";"
> 
> So you claim that your vendor extensions are the same or fully compatible
> with arm,scmi and you add nothing... Are your extensions/protocol valid for
> arm,scmi?

Yes. They are valid for arm,scmi.

 If yes, why is this in separate binding. If no, why you use someone
> else's compatible?

Per SCMI Spec
0x80-0xFF: Reserved for vendor or platform-specific extensions to
this interface

i.MX use 0x81 for BBM, 0x84 for MISC. But other vendors will use
the id for their own protocol.

I use a separate binding here is to avoid add more vendor stuff
in arm,scmi.yaml. Otherwise we will have to add a list as:
if nxp
xxx
else if qcom
xxx
else if xx
yyy.

I could add back i.mx extension to arm,scmi.yaml if people
agree.

Thanks
Peng.

> 
> Maybe your binding is correct, feel free to convince me (and read first writing
> bindings).
> 
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Krzysztof Kozlowski]

On 08/04/2024 01:50, Peng Fan wrote:
>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
>> additionalProperties to true
>>
>> On 07/04/2024 12:04, Peng Fan wrote:
>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
>>>> additionalProperties to true
>>>>
>>>> On 07/04/2024 02:37, Peng Fan wrote:
>>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
>>>>>> additionalProperties to true
>>>>>>
>>>>>> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
>>>>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>>>>
>>>>>>> When adding vendor extension protocols, there is dt-schema warning:
>>>>>>> "
>>>>>>> imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do not
>>>>>>> match any of the regexes: 'pinctrl-[0-9]+'
>>>>>>> "
>>>>>>>
>>>>>>> Set additionalProperties to true to address the issue.
>>>>>>
>>>>>> I do not see anything addressed here, except making the binding
>>>>>> accepting anything anywhere...
>>>>>
>>>>> I not wanna add vendor protocols in arm,scmi.yaml, so will introduce
>>>>> a new yaml imx.scmi.yaml which add i.MX SCMI protocol extension.
>>>>>
>>>>> With additionalProperties set to false, I not know how, please suggest.
>>>>
>>>> First of all, you cannot affect negatively existing devices (their
>>>> bindings) and your patch does exactly that. This should make you
>>>> thing what is the correct approach...
>>>>
>>>> Rob gave you the comment about missing compatible - you still did not
>>>> address that.
>>>
>>> I added the compatible in patch 2/6 in the examples "compatible =
>> "arm,scmi";"
>>
>> So you claim that your vendor extensions are the same or fully compatible
>> with arm,scmi and you add nothing... Are your extensions/protocol valid for
>> arm,scmi?
> 
> Yes. They are valid for arm,scmi.
> 
>  If yes, why is this in separate binding. If no, why you use someone
>> else's compatible?
> 
> Per SCMI Spec
> 0x80-0xFF: Reserved for vendor or platform-specific extensions to
> this interface
> 
> i.MX use 0x81 for BBM, 0x84 for MISC. But other vendors will use
> the id for their own protocol.

So how are they valid for arm,scmi? I don't understand.



Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Peng Fan]

> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> additionalProperties to true
> 
> On 08/04/2024 01:50, Peng Fan wrote:
> >> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> >> additionalProperties to true
> >>
> >> On 07/04/2024 12:04, Peng Fan wrote:
> >>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> >>>> additionalProperties to true
> >>>>
> >>>> On 07/04/2024 02:37, Peng Fan wrote:
> >>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> >>>>>> additionalProperties to true
> >>>>>>
> >>>>>> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> >>>>>>> From: Peng Fan <peng.fan@nxp.com>
> >>>>>>>
> >>>>>>> When adding vendor extension protocols, there is dt-schema
> warning:
> >>>>>>> "
> >>>>>>> imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do not
> >>>>>>> match any of the regexes: 'pinctrl-[0-9]+'
> >>>>>>> "
> >>>>>>>
> >>>>>>> Set additionalProperties to true to address the issue.
> >>>>>>
> >>>>>> I do not see anything addressed here, except making the binding
> >>>>>> accepting anything anywhere...
> >>>>>
> >>>>> I not wanna add vendor protocols in arm,scmi.yaml, so will
> >>>>> introduce a new yaml imx.scmi.yaml which add i.MX SCMI protocol
> extension.
> >>>>>
> >>>>> With additionalProperties set to false, I not know how, please suggest.
> >>>>
> >>>> First of all, you cannot affect negatively existing devices (their
> >>>> bindings) and your patch does exactly that. This should make you
> >>>> thing what is the correct approach...
> >>>>
> >>>> Rob gave you the comment about missing compatible - you still did
> >>>> not address that.
> >>>
> >>> I added the compatible in patch 2/6 in the examples "compatible =
> >> "arm,scmi";"
> >>
> >> So you claim that your vendor extensions are the same or fully
> >> compatible with arm,scmi and you add nothing... Are your
> >> extensions/protocol valid for arm,scmi?
> >
> > Yes. They are valid for arm,scmi.
> >
> >  If yes, why is this in separate binding. If no, why you use someone
> >> else's compatible?
> >
> > Per SCMI Spec
> > 0x80-0xFF: Reserved for vendor or platform-specific extensions to this
> > interface
> >
> > i.MX use 0x81 for BBM, 0x84 for MISC. But other vendors will use the
> > id for their own protocol.
> 
> So how are they valid for arm,scmi? I don't understand.

arm,scmi is a firmware compatible string. The protocol node is a sub-node.
I think the arm,scmi is that saying the firmware following
SCMI spec to implement the protocols.

For vendor reserved ID, firmware also follow the SCMI spec to implement
their own usage, so from firmware level, it is ARM SCMI spec compatible.

 
        firmware {                                                                                  
                scmi {                                                                              
                        compatible = "arm,scmi";                                                    
                        #address-cells = <1>;                                                       
                        #size-cells = <0>;                                                          
                                                                                                    
                        scmi_devpd: protocol@11 {                                                   
                                reg = <0x11>;                                                       
                                #power-domain-cells = <1>;                                          
                        };                                                                          
                                                        
                        scmi_sys_power: protocol@12 {                                               
                                reg = <0x12>;                                                       
                        };                                                                          
                                                                                                    
                        scmi_perf: protocol@13 {                                                    
                                reg = <0x13>;                                                       
                                #clock-cells = <1>;                                                 
                                #power-domain-cells = <1>;                                          
                        }; 

                       Scmi_bbm: protocol@81 {
                              reg = <0x81>;    ->vendor id
                              vendor,xyz; ---> vendor properties.
                      }                          

Thanks,
Peng.

> 
> 
> 
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Krzysztof Kozlowski]

On 08/04/2024 08:08, Peng Fan wrote:
>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
>> additionalProperties to true
>>
>> On 08/04/2024 01:50, Peng Fan wrote:
>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
>>>> additionalProperties to true
>>>>
>>>> On 07/04/2024 12:04, Peng Fan wrote:
>>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
>>>>>> additionalProperties to true
>>>>>>
>>>>>> On 07/04/2024 02:37, Peng Fan wrote:
>>>>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
>>>>>>>> additionalProperties to true
>>>>>>>>
>>>>>>>> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
>>>>>>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>>>>>>
>>>>>>>>> When adding vendor extension protocols, there is dt-schema
>> warning:
>>>>>>>>> "
>>>>>>>>> imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do not
>>>>>>>>> match any of the regexes: 'pinctrl-[0-9]+'
>>>>>>>>> "
>>>>>>>>>
>>>>>>>>> Set additionalProperties to true to address the issue.
>>>>>>>>
>>>>>>>> I do not see anything addressed here, except making the binding
>>>>>>>> accepting anything anywhere...
>>>>>>>
>>>>>>> I not wanna add vendor protocols in arm,scmi.yaml, so will
>>>>>>> introduce a new yaml imx.scmi.yaml which add i.MX SCMI protocol
>> extension.
>>>>>>>
>>>>>>> With additionalProperties set to false, I not know how, please suggest.
>>>>>>
>>>>>> First of all, you cannot affect negatively existing devices (their
>>>>>> bindings) and your patch does exactly that. This should make you
>>>>>> thing what is the correct approach...
>>>>>>
>>>>>> Rob gave you the comment about missing compatible - you still did
>>>>>> not address that.
>>>>>
>>>>> I added the compatible in patch 2/6 in the examples "compatible =
>>>> "arm,scmi";"
>>>>
>>>> So you claim that your vendor extensions are the same or fully
>>>> compatible with arm,scmi and you add nothing... Are your
>>>> extensions/protocol valid for arm,scmi?
>>>
>>> Yes. They are valid for arm,scmi.
>>>
>>>  If yes, why is this in separate binding. If no, why you use someone
>>>> else's compatible?
>>>
>>> Per SCMI Spec
>>> 0x80-0xFF: Reserved for vendor or platform-specific extensions to this
>>> interface
>>>
>>> i.MX use 0x81 for BBM, 0x84 for MISC. But other vendors will use the
>>> id for their own protocol.
>>
>> So how are they valid for arm,scmi? I don't understand.
> 
> arm,scmi is a firmware compatible string. The protocol node is a sub-node.
> I think the arm,scmi is that saying the firmware following
> SCMI spec to implement the protocols.
> 
> For vendor reserved ID, firmware also follow the SCMI spec to implement
> their own usage, so from firmware level, it is ARM SCMI spec compatible.

That's not the point. It is obvious that your firmware is compatible
with arm,scmi, but what you try to say in this and revised patch is that
every arm,scmi is compatible with your implementation. What you are
saying is that 0x84 is MISC protocol for every firmware, Qualcomm, NXP,
Samsung, TI, Mediatek etc.

I claim it is not true. 0x84 is not MISC for Qualcomm, for example.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Peng Fan]

> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> additionalProperties to true
> 
> On 08/04/2024 08:08, Peng Fan wrote:
> >> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> >> additionalProperties to true
> >>
> >> On 08/04/2024 01:50, Peng Fan wrote:
> >>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> >>>> additionalProperties to true
> >>>>
> >>>> On 07/04/2024 12:04, Peng Fan wrote:
> >>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> >>>>>> additionalProperties to true
> >>>>>>
> >>>>>> On 07/04/2024 02:37, Peng Fan wrote:
> >>>>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi:
> >>>>>>>> set additionalProperties to true
> >>>>>>>>
> >>>>>>>> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> >>>>>>>>> From: Peng Fan <peng.fan@nxp.com>
> >>>>>>>>>
> >>>>>>>>> When adding vendor extension protocols, there is dt-schema
> >> warning:
> >>>>>>>>> "
> >>>>>>>>> imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do
> >>>>>>>>> not match any of the regexes: 'pinctrl-[0-9]+'
> >>>>>>>>> "
> >>>>>>>>>
> >>>>>>>>> Set additionalProperties to true to address the issue.
> >>>>>>>>
> >>>>>>>> I do not see anything addressed here, except making the binding
> >>>>>>>> accepting anything anywhere...
> >>>>>>>
> >>>>>>> I not wanna add vendor protocols in arm,scmi.yaml, so will
> >>>>>>> introduce a new yaml imx.scmi.yaml which add i.MX SCMI protocol
> >> extension.
> >>>>>>>
> >>>>>>> With additionalProperties set to false, I not know how, please
> suggest.
> >>>>>>
> >>>>>> First of all, you cannot affect negatively existing devices
> >>>>>> (their
> >>>>>> bindings) and your patch does exactly that. This should make you
> >>>>>> thing what is the correct approach...
> >>>>>>
> >>>>>> Rob gave you the comment about missing compatible - you still did
> >>>>>> not address that.
> >>>>>
> >>>>> I added the compatible in patch 2/6 in the examples "compatible =
> >>>> "arm,scmi";"
> >>>>
> >>>> So you claim that your vendor extensions are the same or fully
> >>>> compatible with arm,scmi and you add nothing... Are your
> >>>> extensions/protocol valid for arm,scmi?
> >>>
> >>> Yes. They are valid for arm,scmi.
> >>>
> >>>  If yes, why is this in separate binding. If no, why you use someone
> >>>> else's compatible?
> >>>
> >>> Per SCMI Spec
> >>> 0x80-0xFF: Reserved for vendor or platform-specific extensions to
> >>> this interface
> >>>
> >>> i.MX use 0x81 for BBM, 0x84 for MISC. But other vendors will use the
> >>> id for their own protocol.
> >>
> >> So how are they valid for arm,scmi? I don't understand.
> >
> > arm,scmi is a firmware compatible string. The protocol node is a sub-node.
> > I think the arm,scmi is that saying the firmware following SCMI spec
> > to implement the protocols.
> >
> > For vendor reserved ID, firmware also follow the SCMI spec to
> > implement their own usage, so from firmware level, it is ARM SCMI spec
> compatible.
> 
> That's not the point. It is obvious that your firmware is compatible with
> arm,scmi, but what you try to say in this and revised patch is that every
> arm,scmi is compatible with your implementation. What you are saying is
> that 0x84 is MISC protocol for every firmware, Qualcomm, NXP, Samsung, TI,
> Mediatek etc.
> 
> I claim it is not true. 0x84 is not MISC for Qualcomm, for example.

You are right. I am lost now on how to add vendor ID support, using
arm,scmi.yaml or adding a new imx,scmi.yaml or else.

Please suggest.

Thanks,
Peng
> 
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Cristian Marussi]

On Fri, Apr 05, 2024 at 08:39:25PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The i.MX BBM protocol is for managing i.MX BBM module which provides
> RTC and BUTTON feature.
> 

Hi,

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/arm_scmi/Kconfig      |  10 +
>  drivers/firmware/arm_scmi/Makefile     |   1 +
>  drivers/firmware/arm_scmi/imx-sm-bbm.c | 378 +++++++++++++++++++++++++++++++++
>  include/linux/scmi_imx_protocol.h      |  45 ++++
>  4 files changed, 434 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index aa5842be19b2..56d11c9d9f47 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -181,3 +181,13 @@ config ARM_SCMI_POWER_CONTROL
>  	  early shutdown/reboot SCMI requests.
>  
>  endmenu
> +
> +config IMX_SCMI_BBM_EXT
> +	tristate "i.MX SCMI BBM EXTENSION"
> +	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> +	default y if ARCH_MXC
> +	help
> +	  This enables i.MX System BBM control logic which supports RTC
> +	  and BUTTON.
> +
> +	  This driver can also be built as a module.
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index a7bc4796519c..327687acf857 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -11,6 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
>  scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
>  scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
>  scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> +scmi-protocols-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.o
>  scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
>  
>  obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> diff --git a/drivers/firmware/arm_scmi/imx-sm-bbm.c b/drivers/firmware/arm_scmi/imx-sm-bbm.c
> new file mode 100644
> index 000000000000..92c0aedf65cc
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/imx-sm-bbm.c
> @@ -0,0 +1,378 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) NXP BBM Protocol
> + *
> + * Copyright 2024 NXP
> + */
> +
> +#define pr_fmt(fmt) "SCMI Notifications BBM - " fmt
> +
> +#include <linux/bits.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/scmi_imx_protocol.h>
> +
> +#include "protocols.h"
> +#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

	https://lore.kernel.org/linux-arm-kernel/ZeGtoJ7ztSe8Kg8R@bogus/#t

> +enum scmi_imx_bbm_protocol_cmd {
> +	IMX_BBM_GPR_SET = 0x3,
> +	IMX_BBM_GPR_GET = 0x4,
> +	IMX_BBM_RTC_ATTRIBUTES = 0x5,
> +	IMX_BBM_RTC_TIME_SET = 0x6,
> +	IMX_BBM_RTC_TIME_GET = 0x7,
> +	IMX_BBM_RTC_ALARM_SET = 0x8,
> +	IMX_BBM_BUTTON_GET = 0x9,
> +	IMX_BBM_RTC_NOTIFY = 0xA,
> +	IMX_BBM_BUTTON_NOTIFY = 0xB,
> +};
> +
> +#define GET_RTCS_NR(x)	le32_get_bits((x), GENMASK(23, 16))
> +#define GET_GPRS_NR(x)	le32_get_bits((x), GENMASK(15, 0))
> +
> +#define SCMI_IMX_BBM_NOTIFY_RTC_UPDATED		BIT(2)
> +#define SCMI_IMX_BBM_NOTIFY_RTC_ROLLOVER	BIT(1)
> +#define SCMI_IMX_BBM_NOTIFY_RTC_ALARM		BIT(0)
> +
> +#define SCMI_IMX_BBM_RTC_ALARM_ENABLE_FLAG	BIT(0)
> +
> +#define SCMI_IMX_BBM_NOTIFY_RTC_FLAG	\
> +	(SCMI_IMX_BBM_NOTIFY_RTC_UPDATED | SCMI_IMX_BBM_NOTIFY_RTC_ROLLOVER | \
> +	 SCMI_IMX_BBM_NOTIFY_RTC_ALARM)
> +
> +#define SCMI_IMX_BBM_EVENT_RTC_MASK		GENMASK(31, 24)
> +
> +struct scmi_imx_bbm_info {
> +	u32 version;
> +	int nr_rtc;
> +	int nr_gpr;
> +};
> +
> +struct scmi_msg_imx_bbm_protocol_attributes {
> +	__le32 attributes;
> +};
> +
> +struct scmi_imx_bbm_set_time {
> +	__le32 id;
> +	__le32 flags;
> +	__le32 value_low;
> +	__le32 value_high;
> +};
> +
> +struct scmi_imx_bbm_get_time {
> +	__le32 id;
> +	__le32 flags;
> +};
> +
> +struct scmi_imx_bbm_alarm_time {
> +	__le32 id;
> +	__le32 flags;
> +	__le32 value_low;
> +	__le32 value_high;
> +};
> +
> +struct scmi_msg_imx_bbm_rtc_notify {
> +	__le32 rtc_id;
> +	__le32 flags;
> +};
> +
> +struct scmi_msg_imx_bbm_button_notify {
> +	__le32 flags;
> +};
> +
> +struct scmi_imx_bbm_notify_payld {
> +	__le32 flags;
> +};
> +
> +static int scmi_imx_bbm_attributes_get(const struct scmi_protocol_handle *ph,
> +				       struct scmi_imx_bbm_info *pi)
> +{
> +	int ret;
> +	struct scmi_xfer *t;
> +	struct scmi_msg_imx_bbm_protocol_attributes *attr;
> +
> +	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, sizeof(*attr), &t);
> +	if (ret)
> +		return ret;
> +
> +	attr = t->rx.buf;
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret) {
> +		pi->nr_rtc = GET_RTCS_NR(attr->attributes);
> +		pi->nr_gpr = GET_GPRS_NR(attr->attributes);
> +	}
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
> +static int scmi_imx_bbm_notify(const struct scmi_protocol_handle *ph,
> +			       u32 src_id, int message_id, bool enable)
> +{
> +	int ret;
> +	struct scmi_xfer *t;
> +
> +	if (message_id == IMX_BBM_RTC_NOTIFY) {
> +		struct scmi_msg_imx_bbm_rtc_notify *rtc_notify;
> +
> +		ret = ph->xops->xfer_get_init(ph, message_id,
> +					      sizeof(*rtc_notify), 0, &t);
> +		if (ret)
> +			return ret;
> +
> +		rtc_notify = t->tx.buf;
> +		rtc_notify->rtc_id = cpu_to_le32(0);
> +		rtc_notify->flags =
> +			cpu_to_le32(enable ? SCMI_IMX_BBM_NOTIFY_RTC_FLAG : 0);
> +	} else if (message_id == IMX_BBM_BUTTON_NOTIFY) {
> +		struct scmi_msg_imx_bbm_button_notify *button_notify;
> +
> +		ret = ph->xops->xfer_get_init(ph, message_id,
> +					      sizeof(*button_notify), 0, &t);
> +		if (ret)
> +			return ret;
> +
> +		button_notify = t->tx.buf;
> +		button_notify->flags = cpu_to_le32(enable ? 1 : 0);
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	ret = ph->xops->do_xfer(ph, t);
> +
> +	ph->xops->xfer_put(ph, t);
> +	return ret;
> +}
> +
> +static enum scmi_imx_bbm_protocol_cmd evt_2_cmd[] = {
> +	IMX_BBM_RTC_NOTIFY,
> +	IMX_BBM_BUTTON_NOTIFY
> +};
> +
> +static int scmi_imx_bbm_set_notify_enabled(const struct scmi_protocol_handle *ph,
> +					   u8 evt_id, u32 src_id, bool enable)
> +{
> +	int ret, cmd_id;
> +
> +	if (evt_id >= ARRAY_SIZE(evt_2_cmd))
> +		return -EINVAL;
> +
> +	cmd_id = evt_2_cmd[evt_id];
> +	ret = scmi_imx_bbm_notify(ph, src_id, cmd_id, enable);
> +	if (ret)
> +		pr_debug("FAIL_ENABLED - evt[%X] dom[%d] - ret:%d\n",
> +			 evt_id, src_id, ret);
> +
> +	return ret;
> +}
> +
> +static void *scmi_imx_bbm_fill_custom_report(const struct scmi_protocol_handle *ph,
> +					     u8 evt_id, ktime_t timestamp,
> +					     const void *payld, size_t payld_sz,
> +					     void *report, u32 *src_id)
> +{
> +	const struct scmi_imx_bbm_notify_payld *p = payld;
> +	struct scmi_imx_bbm_notif_report *r = report;
> +
> +	if (sizeof(*p) != payld_sz)
> +		return NULL;
> +
> +	if (evt_id == SCMI_EVENT_IMX_BBM_RTC) {
> +		r->is_rtc = true;
> +		r->is_button = false;
> +		r->timestamp = timestamp;
> +		r->rtc_id = le32_get_bits(p->flags, SCMI_IMX_BBM_EVENT_RTC_MASK);
> +		r->rtc_evt = le32_get_bits(p->flags, SCMI_IMX_BBM_NOTIFY_RTC_FLAG);
> +		dev_dbg(ph->dev, "RTC: %d evt: %x\n", r->rtc_id, r->rtc_evt);
> +		*src_id = r->rtc_evt;
> +	} else if (evt_id == SCMI_EVENT_IMX_BBM_BUTTON) {
> +		r->is_rtc = false;
> +		r->is_button = true;
> +		r->timestamp = timestamp;
> +		dev_dbg(ph->dev, "BBM Button\n");
> +		*src_id = 0;
> +	} else {
> +		WARN_ON_ONCE(1);
> +		return NULL;
> +	}
> +
> +	return r;
> +}
> +
> +static const struct scmi_event scmi_imx_bbm_events[] = {
> +	{
> +		.id = SCMI_EVENT_IMX_BBM_RTC,
> +		.max_payld_sz = sizeof(struct scmi_imx_bbm_notify_payld),
> +		.max_report_sz = sizeof(struct scmi_imx_bbm_notif_report),
> +	},
> +	{
> +		.id = SCMI_EVENT_IMX_BBM_BUTTON,
> +		.max_payld_sz = sizeof(struct scmi_imx_bbm_notify_payld),
> +		.max_report_sz = sizeof(struct scmi_imx_bbm_notif_report),
> +	},
> +};
> +
> +static const struct scmi_event_ops scmi_imx_bbm_event_ops = {
> +	.set_notify_enabled = scmi_imx_bbm_set_notify_enabled,
> +	.fill_custom_report = scmi_imx_bbm_fill_custom_report,
> +};
> +
> +static const struct scmi_protocol_events scmi_imx_bbm_protocol_events = {
> +	.queue_sz = SCMI_PROTO_QUEUE_SZ,
> +	.ops = &scmi_imx_bbm_event_ops,
> +	.evts = scmi_imx_bbm_events,
> +	.num_events = ARRAY_SIZE(scmi_imx_bbm_events),
> +	.num_sources = 1,
> +};
> +
> +static int scmi_imx_bbm_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> +	u32 version;
> +	int ret;
> +	struct scmi_imx_bbm_info *binfo;
> +
> +	ret = ph->xops->version_get(ph, &version);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(ph->dev, "NXP SM BBM Version %d.%d\n",
> +		 PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> +	binfo = devm_kzalloc(ph->dev, sizeof(*binfo), GFP_KERNEL);
> +	if (!binfo)
> +		return -ENOMEM;
> +
> +	ret = scmi_imx_bbm_attributes_get(ph, binfo);
> +	if (ret)
> +		return ret;
> +
> +	return ph->set_priv(ph, binfo, version);
> +}
> +
> +static int scmi_imx_bbm_rtc_time_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_set_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_SET, sizeof(*cfg), 0, &t);
> +	if (ret)
> +		return ret;
> +
> +	cfg = t->tx.buf;
> +	cfg->id = cpu_to_le32(rtc_id);
> +	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...

> +
> +	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.

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 i.MX
> BBM protocol
> 
> On Fri, Apr 05, 2024 at 08:39:25PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Mon, Apr 08, 2024 at 07:04:43PM +0100, Cristian Marussi wrote:
> On Fri, Apr 05, 2024 at 08:39:25PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> > 
> > The i.MX BBM protocol is for managing i.MX BBM module which provides
> > RTC and BUTTON feature.
> > 
> 
> 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
> 
> 	https://lore.kernel.org/linux-arm-kernel/ZeGtoJ7ztSe8Kg8R@bogus/#t
>

I have decided to ignore all these vendor protocol patches until they have
some documentation to understand what these protocol are for, what are
the commands, their input/output parameter details, any conditions are the
caller and callee,..etc very similar to SCMI spec.

To start with can you please expand what is BBM or MISC protocol is ?
Don't expect me to respond if the requested details are still missing in
the future versions, I am going to ignore it silently.

I have asked for these in atleast 2 different threads may be not just NXP
patches but in one instance Qcom patches, but they apply equally here.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

Hi Sudeep,

> Subject: Re: [PATCH v2 3/6] firmware: arm_scmi: add initial support for i.MX
> BBM protocol
> 
> On Mon, Apr 08, 2024 at 07:04:43PM +0100, Cristian Marussi wrote:
> > On Fri, Apr 05, 2024 at 08:39:25PM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > The i.MX BBM protocol is for managing i.MX BBM module which provides
> > > RTC and BUTTON feature.
> > >
> >
> > 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
> >
> >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Flinux-arm-
> kernel%2FZeGtoJ7ztSe8Kg8R%40bogus%2F%23t&data=
> >
> 05%7C02%7Cpeng.fan%40nxp.com%7Ce92ff78b9126447afe9708dc587358d
> 4%7C686e
> >
> a1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638482499632395762%7C
> Unknown%7C
> >
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVC
> >
> I6Mn0%3D%7C0%7C%7C%7C&sdata=7QP%2BkkjHA3Sa0CdcbbObGG4kgYYK
> XAGA2r%2F%2F
> > x0MogqU%3D&reserved=0
> >
> 
> I have decided to ignore all these vendor protocol patches until they have
> some documentation to understand what these protocol are for, what are the
> commands, their input/output parameter details, any conditions are the
> caller and callee,..etc very similar to SCMI spec.

Where do you expect the documentation to be put?

similar as scmi_protocol.h, put in scmi_imx_protcol.h? 
> 
> To start with can you please expand what is BBM or MISC protocol is ?

ok. Sorry for missing your previous comment in v1. Let me write here briefly
first.

The Battery Backup (BB) Domain contains the Battery Backed Security
Module (BBSM) and the Battery Backed Non-Secure Module (BBNSM).
BBM protocol is to manage i.MX BBSM and BBNSM. This protocol supports
#define COMMAND_PROTOCOL_VERSION             0x0U                                                   
#define COMMAND_PROTOCOL_ATTRIBUTES          0x1U                                                   
#define COMMAND_PROTOCOL_MESSAGE_ATTRIBUTES  0x2U                                                   
#define COMMAND_BBM_GPR_SET                  0x3U                                                   
#define COMMAND_BBM_GPR_GET                  0x4U                                                   
#define COMMAND_BBM_RTC_ATTRIBUTES           0x5U                                                   
#define COMMAND_BBM_RTC_TIME_SET             0x6U                                                   
#define COMMAND_BBM_RTC_TIME_GET             0x7U                                                   
#define COMMAND_BBM_RTC_ALARM_SET            0x8U                                                   
#define COMMAND_BBM_BUTTON_GET               0x9U                                                   
#define COMMAND_BBM_RTC_NOTIFY               0xAU                                                   
#define COMMAND_BBM_BUTTON_NOTIFY            0xBU                                                   
#define COMMAND_NEGOTIATE_PROTOCOL_VERSION   0x10U

For now in this patchset for linux, we only use RTC, and BUTTON
for system wakeup

For MISC protocol, it is for various misc things, such as discover
build info, get rom passed data, get reset reason, get i.mx
cfg name, control set(for gpio expander under m33 control and
etc). The command as below:
51 #define COMMAND_PROTOCOL_VERSION             0x0U                                                   
  52 #define COMMAND_PROTOCOL_ATTRIBUTES          0x1U                                                   
  53 #define COMMAND_PROTOCOL_MESSAGE_ATTRIBUTES  0x2U                                                   
  54 #define COMMAND_MISC_CONTROL_SET             0x3U                                                   
  55 #define COMMAND_MISC_CONTROL_GET             0x4U                                                   
  56 #define COMMAND_MISC_CONTROL_ACTION          0x5U                                                   
  57 #define COMMAND_MISC_DISCOVER_BUILD_INFO     0x6U                                                   
  58 #define COMMAND_MISC_ROM_PASSOVER_GET        0x7U                                                   
  59 #define COMMAND_MISC_CONTROL_NOTIFY          0x8U                                                   
  60 #define COMMAND_MISC_REASON_ATTRIBUTES       0x9U                                                   
  61 #define COMMAND_MISC_RESET_REASON            0xAU                                                   
  62 #define COMMAND_MISC_SI_INFO                 0xBU                                                   
  63 #define COMMAND_MISC_CFG_INFO                0xCU                                                   
  64 #define COMMAND_MISC_SYSLOG                  0xDU                                                   
  65 #define COMMAND_NEGOTIATE_PROTOCOL_VERSION   0x10U

I will add the information in v3.

But as of now I am not sure
how to proceed with dt-binding, add in vendor stuff
in arm,scmi.yaml or add imx,scmi.yaml

Thanks,
Peng.

> Don't expect me to respond if the requested details are still missing in the
> future versions, I am going to ignore it silently.
> 
> I have asked for these in atleast 2 different threads may be not just NXP
> patches but in one instance Qcom patches, but they apply equally here.
> 
> --
> Regards,
> Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Peng Fan]

Hi Krzysztof,

> Subject: RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> additionalProperties to true
> 
> > Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > additionalProperties to true
> >
> > On 08/04/2024 08:08, Peng Fan wrote:
> > >> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > >> additionalProperties to true
> > >>
> > >> On 08/04/2024 01:50, Peng Fan wrote:
> > >>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > >>>> additionalProperties to true
> > >>>>
> > >>>> On 07/04/2024 12:04, Peng Fan wrote:
> > >>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi:
> > >>>>>> set additionalProperties to true
> > >>>>>>
> > >>>>>> On 07/04/2024 02:37, Peng Fan wrote:
> > >>>>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi:
> > >>>>>>>> set additionalProperties to true
> > >>>>>>>>
> > >>>>>>>> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > >>>>>>>>> From: Peng Fan <peng.fan@nxp.com>
> > >>>>>>>>>
> > >>>>>>>>> When adding vendor extension protocols, there is dt-schema
> > >> warning:
> > >>>>>>>>> "
> > >>>>>>>>> imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do
> > >>>>>>>>> not match any of the regexes: 'pinctrl-[0-9]+'
> > >>>>>>>>> "
> > >>>>>>>>>
> > >>>>>>>>> Set additionalProperties to true to address the issue.
> > >>>>>>>>
> > >>>>>>>> I do not see anything addressed here, except making the
> > >>>>>>>> binding accepting anything anywhere...
> > >>>>>>>
> > >>>>>>> I not wanna add vendor protocols in arm,scmi.yaml, so will
> > >>>>>>> introduce a new yaml imx.scmi.yaml which add i.MX SCMI
> > >>>>>>> protocol
> > >> extension.
> > >>>>>>>
> > >>>>>>> With additionalProperties set to false, I not know how, please
> > suggest.
> > >>>>>>
> > >>>>>> First of all, you cannot affect negatively existing devices
> > >>>>>> (their
> > >>>>>> bindings) and your patch does exactly that. This should make
> > >>>>>> you thing what is the correct approach...
> > >>>>>>
> > >>>>>> Rob gave you the comment about missing compatible - you still
> > >>>>>> did not address that.
> > >>>>>
> > >>>>> I added the compatible in patch 2/6 in the examples "compatible
> > >>>>> =
> > >>>> "arm,scmi";"
> > >>>>
> > >>>> So you claim that your vendor extensions are the same or fully
> > >>>> compatible with arm,scmi and you add nothing... Are your
> > >>>> extensions/protocol valid for arm,scmi?
> > >>>
> > >>> Yes. They are valid for arm,scmi.
> > >>>
> > >>>  If yes, why is this in separate binding. If no, why you use
> > >>> someone
> > >>>> else's compatible?
> > >>>
> > >>> Per SCMI Spec
> > >>> 0x80-0xFF: Reserved for vendor or platform-specific extensions to
> > >>> this interface
> > >>>
> > >>> i.MX use 0x81 for BBM, 0x84 for MISC. But other vendors will use
> > >>> the id for their own protocol.
> > >>
> > >> So how are they valid for arm,scmi? I don't understand.
> > >
> > > arm,scmi is a firmware compatible string. The protocol node is a sub-node.
> > > I think the arm,scmi is that saying the firmware following SCMI spec
> > > to implement the protocols.
> > >
> > > For vendor reserved ID, firmware also follow the SCMI spec to
> > > implement their own usage, so from firmware level, it is ARM SCMI
> > > spec
> > compatible.
> >
> > That's not the point. It is obvious that your firmware is compatible
> > with arm,scmi, but what you try to say in this and revised patch is
> > that every arm,scmi is compatible with your implementation. What you
> > are saying is that 0x84 is MISC protocol for every firmware, Qualcomm,
> > NXP, Samsung, TI, Mediatek etc.
> >
> > I claim it is not true. 0x84 is not MISC for Qualcomm, for example.
> 
> You are right. I am lost now on how to add vendor ID support, using
> arm,scmi.yaml or adding a new imx,scmi.yaml or else.

Do you have any suggestions on how to add vendor protocol in
dt-schema? I am not sure what to do next, still keep imx,scmi.yaml
or add vendor stuff in arm,scmi.yaml?

Thanks,
Peng.

> 
> Please suggest.
> 
> Thanks,
> Peng
> >
> > Best regards,
> > Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Tue, Apr 09, 2024 at 09:13:33AM +0000, Peng Fan wrote:
> Hi Sudeep,
>
> > Subject: Re: [PATCH v2 3/6] firmware: arm_scmi: add initial support for i.MX
> > BBM protocol
> >
> > On Mon, Apr 08, 2024 at 07:04:43PM +0100, Cristian Marussi wrote:
> > > On Fri, Apr 05, 2024 at 08:39:25PM +0800, Peng Fan (OSS) wrote:
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > The i.MX BBM protocol is for managing i.MX BBM module which provides
> > > > RTC and BUTTON feature.
> > > >
> > >
> > > 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
> > >
> > >
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > > .kernel.org%2Flinux-arm-
> > kernel%2FZeGtoJ7ztSe8Kg8R%40bogus%2F%23t&data=
> > >
> > 05%7C02%7Cpeng.fan%40nxp.com%7Ce92ff78b9126447afe9708dc587358d
> > 4%7C686e
> > >
> > a1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638482499632395762%7C
> > Unknown%7C
> > >
> > TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> > CJXVC
> > >
> > I6Mn0%3D%7C0%7C%7C%7C&sdata=7QP%2BkkjHA3Sa0CdcbbObGG4kgYYK
> > XAGA2r%2F%2F
> > > x0MogqU%3D&reserved=0
> > >
> >
> > I have decided to ignore all these vendor protocol patches until they have
> > some documentation to understand what these protocol are for, what are the
> > commands, their input/output parameter details, any conditions are the
> > caller and callee,..etc very similar to SCMI spec.
>
> Where do you expect the documentation to be put?
>

To begin with, we need all these vendor protocols in a directory say
vendors/nxp under drivers/firmware/arm_scmi. It can be a simple text file
under that. We can see later if we need any more formal version elsewhere
but that shouldn't be a blocker for these changes.

> similar as scmi_protocol.h, put in scmi_imx_protcol.h?
> >
> > To start with can you please expand what is BBM or MISC protocol is ?
>
> ok. Sorry for missing your previous comment in v1. Let me write here briefly
> first.
>

Thanks

> The Battery Backup (BB) Domain contains the Battery Backed Security
> Module (BBSM) and the Battery Backed Non-Secure Module (BBNSM).
> BBM protocol is to manage i.MX BBSM and BBNSM. This protocol supports
> #define COMMAND_PROTOCOL_VERSION             0x0U
> #define COMMAND_PROTOCOL_ATTRIBUTES          0x1U
> #define COMMAND_PROTOCOL_MESSAGE_ATTRIBUTES  0x2U
> #define COMMAND_BBM_GPR_SET                  0x3U
> #define COMMAND_BBM_GPR_GET                  0x4U
> #define COMMAND_BBM_RTC_ATTRIBUTES           0x5U
> #define COMMAND_BBM_RTC_TIME_SET             0x6U
> #define COMMAND_BBM_RTC_TIME_GET             0x7U
> #define COMMAND_BBM_RTC_ALARM_SET            0x8U
> #define COMMAND_BBM_BUTTON_GET               0x9U
> #define COMMAND_BBM_RTC_NOTIFY               0xAU
> #define COMMAND_BBM_BUTTON_NOTIFY            0xBU
> #define COMMAND_NEGOTIATE_PROTOCOL_VERSION   0x10U
>

Hopefully description of each of these commands cover what GPR above means
really.

> For now in this patchset for linux, we only use RTC, and BUTTON
> for system wakeup
>
> For MISC protocol, it is for various misc things, such as discover
> build info, get rom passed data, get reset reason, get i.mx
> cfg name, control set(for gpio expander under m33 control and
> etc). The command as below:
> #define COMMAND_PROTOCOL_VERSION             0x0U
> #define COMMAND_PROTOCOL_ATTRIBUTES          0x1U
> #define COMMAND_PROTOCOL_MESSAGE_ATTRIBUTES  0x2U
> #define COMMAND_MISC_CONTROL_SET             0x3U
> #define COMMAND_MISC_CONTROL_GET             0x4U
> #define COMMAND_MISC_CONTROL_ACTION          0x5U
> #define COMMAND_MISC_DISCOVER_BUILD_INFO     0x6U
> #define COMMAND_MISC_ROM_PASSOVER_GET        0x7U
> #define COMMAND_MISC_CONTROL_NOTIFY          0x8U
> #define COMMAND_MISC_REASON_ATTRIBUTES       0x9U
> #define COMMAND_MISC_RESET_REASON            0xAU
> #define COMMAND_MISC_SI_INFO                 0xBU
> #define COMMAND_MISC_CFG_INFO                0xCU
> #define COMMAND_MISC_SYSLOG                  0xDU
> #define COMMAND_NEGOTIATE_PROTOCOL_VERSION   0x10U
>

And same here. Just as an example what BUILD_INFO ? There will be 10s if not
100s of different image in the system. What does this BUILD_INFO provide ?
And why is this important over version or release info ?

These are simple pointers, expect more questions like this if the document
is not self sufficient in explaining such details.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 i.MX
> BBM protocol
> 
> On Tue, Apr 09, 2024 at 09:13:33AM +0000, Peng Fan wrote:
> > Hi Sudeep,
> >
> > > Subject: Re: [PATCH v2 3/6] firmware: arm_scmi: add initial support
> > > for i.MX BBM protocol
> > >
> > > On Mon, Apr 08, 2024 at 07:04:43PM +0100, Cristian Marussi wrote:
> > > > On Fri, Apr 05, 2024 at 08:39:25PM +0800, Peng Fan (OSS) wrote:
> > > > > From: Peng Fan <peng.fan@nxp.com>
> > > > >
> > > > > The i.MX BBM protocol is for managing i.MX BBM module which
> > > > > provides RTC and BUTTON feature.
> > > > >
> > > >
> > > > 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
> > > >
> > > >
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > >
> lore%2F&data=05%7C02%7Cpeng.fan%40nxp.com%7C6a989d0fb4c1454e94
> 3608
> > > >
> dc5882c563%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6384
> 825658
> > > >
> 71932293%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> V2luM
> > > >
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=OCvYPh3l94
> 0aQ
> > > > 7mbAgrfEJMhYmix9%2FUUOA6kZyZJYWQ%3D&reserved=0
> > > > .kernel.org%2Flinux-arm-
> > > kernel%2FZeGtoJ7ztSe8Kg8R%40bogus%2F%23t&data=
> > > >
> > >
> 05%7C02%7Cpeng.fan%40nxp.com%7Ce92ff78b9126447afe9708dc587358d
> > > 4%7C686e
> > > >
> > >
> a1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638482499632395762%7C
> > > Unknown%7C
> > > >
> > >
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> > > CJXVC
> > > >
> > >
> I6Mn0%3D%7C0%7C%7C%7C&sdata=7QP%2BkkjHA3Sa0CdcbbObGG4kgYYK
> > > XAGA2r%2F%2F
> > > > x0MogqU%3D&reserved=0
> > > >
> > >
> > > I have decided to ignore all these vendor protocol patches until
> > > they have some documentation to understand what these protocol are
> > > for, what are the commands, their input/output parameter details,
> > > any conditions are the caller and callee,..etc very similar to SCMI spec.
> >
> > Where do you expect the documentation to be put?
> >
> 
> To begin with, we need all these vendor protocols in a directory say
> vendors/nxp under drivers/firmware/arm_scmi. It can be a simple text file
> under that. We can see later if we need any more formal version elsewhere
> but that shouldn't be a blocker for these changes.
> 
> > similar as scmi_protocol.h, put in scmi_imx_protcol.h?
> > >
> > > To start with can you please expand what is BBM or MISC protocol is ?
> >
> > ok. Sorry for missing your previous comment in v1. Let me write here
> > briefly first.
> >
> 
> Thanks
> 
> > The Battery Backup (BB) Domain contains the Battery Backed Security
> > Module (BBSM) and the Battery Backed Non-Secure Module (BBNSM).
> > BBM protocol is to manage i.MX BBSM and BBNSM. This protocol supports
> > #define COMMAND_PROTOCOL_VERSION             0x0U
> > #define COMMAND_PROTOCOL_ATTRIBUTES          0x1U
> > #define COMMAND_PROTOCOL_MESSAGE_ATTRIBUTES  0x2U
> > #define COMMAND_BBM_GPR_SET                  0x3U
> > #define COMMAND_BBM_GPR_GET                  0x4U
> > #define COMMAND_BBM_RTC_ATTRIBUTES           0x5U
> > #define COMMAND_BBM_RTC_TIME_SET             0x6U
> > #define COMMAND_BBM_RTC_TIME_GET             0x7U
> > #define COMMAND_BBM_RTC_ALARM_SET            0x8U
> > #define COMMAND_BBM_BUTTON_GET               0x9U
> > #define COMMAND_BBM_RTC_NOTIFY               0xAU
> > #define COMMAND_BBM_BUTTON_NOTIFY            0xBU
> > #define COMMAND_NEGOTIATE_PROTOCOL_VERSION   0x10U
> >
> 
> Hopefully description of each of these commands cover what GPR above
> means really.

ok, will add more comment in the patch for the commands.

For GPR, there are some general purpose registers in BBM module that
could survive during power cycle, so users could set their own
value and use.

> 
> > For now in this patchset for linux, we only use RTC, and BUTTON for
> > system wakeup
> >
> > For MISC protocol, it is for various misc things, such as discover
> > build info, get rom passed data, get reset reason, get i.mx cfg name,
> > control set(for gpio expander under m33 control and etc). The command
> > as below:
> > #define COMMAND_PROTOCOL_VERSION             0x0U
> > #define COMMAND_PROTOCOL_ATTRIBUTES          0x1U
> > #define COMMAND_PROTOCOL_MESSAGE_ATTRIBUTES  0x2U
> > #define COMMAND_MISC_CONTROL_SET             0x3U
> > #define COMMAND_MISC_CONTROL_GET             0x4U
> > #define COMMAND_MISC_CONTROL_ACTION          0x5U
> > #define COMMAND_MISC_DISCOVER_BUILD_INFO     0x6U
> > #define COMMAND_MISC_ROM_PASSOVER_GET        0x7U
> > #define COMMAND_MISC_CONTROL_NOTIFY          0x8U
> > #define COMMAND_MISC_REASON_ATTRIBUTES       0x9U
> > #define COMMAND_MISC_RESET_REASON            0xAU
> > #define COMMAND_MISC_SI_INFO                 0xBU
> > #define COMMAND_MISC_CFG_INFO                0xCU
> > #define COMMAND_MISC_SYSLOG                  0xDU
> > #define COMMAND_NEGOTIATE_PROTOCOL_VERSION   0x10U
> >
> 
> And same here. Just as an example what BUILD_INFO ? There will be 10s if
> not 100s of different image in the system. What does this BUILD_INFO
> provide ?
> And why is this important over version or release info ?

BUILD_INFO here is to expose the commit hash and patch numbers.
This is firmware developer's decision to add this command, I could not
say it is less or more important.

> 
> These are simple pointers, expect more questions like this if the document is
> not self sufficient in explaining such details.

My plan is to add only the commands that this patch use, not add all the
commands. So in v3, there will be doc only for the commands included,
hope this is ok.

Thanks,
Peng.
> 
> --
> Regards,
> Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Cristian Marussi]

On Tue, Apr 09, 2024 at 09:25:10AM +0000, Peng Fan wrote:
> Hi Krzysztof,
> 
> > Subject: RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > additionalProperties to true
> > 
> > > Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > additionalProperties to true
> > >
> > > On 08/04/2024 08:08, Peng Fan wrote:
> > > >> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > >> additionalProperties to true
> > > >>
> > > >> On 08/04/2024 01:50, Peng Fan wrote:
> > > >>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > >>>> additionalProperties to true
> > > >>>>
> > > >>>> On 07/04/2024 12:04, Peng Fan wrote:
> > > >>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi:
> > > >>>>>> set additionalProperties to true
> > > >>>>>>
> > > >>>>>> On 07/04/2024 02:37, Peng Fan wrote:
> > > >>>>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi:
> > > >>>>>>>> set additionalProperties to true
> > > >>>>>>>>
> > > >>>>>>>> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > > >>>>>>>>> From: Peng Fan <peng.fan@nxp.com>
> > > >>>>>>>>>
> > > >>>>>>>>> When adding vendor extension protocols, there is dt-schema
> > > >> warning:
> > > >>>>>>>>> "
> > > >>>>>>>>> imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do
> > > >>>>>>>>> not match any of the regexes: 'pinctrl-[0-9]+'
> > > >>>>>>>>> "
> > > >>>>>>>>>
> > > >>>>>>>>> Set additionalProperties to true to address the issue.
> > > >>>>>>>>
> > > >>>>>>>> I do not see anything addressed here, except making the
> > > >>>>>>>> binding accepting anything anywhere...
> > > >>>>>>>
> > > >>>>>>> I not wanna add vendor protocols in arm,scmi.yaml, so will
> > > >>>>>>> introduce a new yaml imx.scmi.yaml which add i.MX SCMI
> > > >>>>>>> protocol
> > > >> extension.
> > > >>>>>>>
> > > >>>>>>> With additionalProperties set to false, I not know how, please
> > > suggest.
> > > >>>>>>
> > > >>>>>> First of all, you cannot affect negatively existing devices
> > > >>>>>> (their
> > > >>>>>> bindings) and your patch does exactly that. This should make
> > > >>>>>> you thing what is the correct approach...
> > > >>>>>>
> > > >>>>>> Rob gave you the comment about missing compatible - you still
> > > >>>>>> did not address that.
> > > >>>>>
> > > >>>>> I added the compatible in patch 2/6 in the examples "compatible
> > > >>>>> =
> > > >>>> "arm,scmi";"
> > > >>>>
> > > >>>> So you claim that your vendor extensions are the same or fully
> > > >>>> compatible with arm,scmi and you add nothing... Are your
> > > >>>> extensions/protocol valid for arm,scmi?
> > > >>>
> > > >>> Yes. They are valid for arm,scmi.
> > > >>>
> > > >>>  If yes, why is this in separate binding. If no, why you use
> > > >>> someone
> > > >>>> else's compatible?
> > > >>>
> > > >>> Per SCMI Spec
> > > >>> 0x80-0xFF: Reserved for vendor or platform-specific extensions to
> > > >>> this interface
> > > >>>
> > > >>> i.MX use 0x81 for BBM, 0x84 for MISC. But other vendors will use
> > > >>> the id for their own protocol.
> > > >>
> > > >> So how are they valid for arm,scmi? I don't understand.
> > > >
> > > > arm,scmi is a firmware compatible string. The protocol node is a sub-node.
> > > > I think the arm,scmi is that saying the firmware following SCMI spec
> > > > to implement the protocols.
> > > >
> > > > For vendor reserved ID, firmware also follow the SCMI spec to
> > > > implement their own usage, so from firmware level, it is ARM SCMI
> > > > spec
> > > compatible.
> > >
> > > That's not the point. It is obvious that your firmware is compatible
> > > with arm,scmi, but what you try to say in this and revised patch is
> > > that every arm,scmi is compatible with your implementation. What you
> > > are saying is that 0x84 is MISC protocol for every firmware, Qualcomm,
> > > NXP, Samsung, TI, Mediatek etc.
> > >
> > > I claim it is not true. 0x84 is not MISC for Qualcomm, for example.
> > 
> > You are right. I am lost now on how to add vendor ID support, using
> > arm,scmi.yaml or adding a new imx,scmi.yaml or else.
> 

Hi Peng,

I dont think in the following you will find the solution to the problem,
it is just to recap the situation and constraints around vendor protocol
bindings.

Describing SCMI vendors protocols is tricky because while on one side
the protocol node has to be rooted under the main scmi fw DT node (like
all the standard protocols) and be 'derived' from the arm,scmi.yaml
protocol-node definition, the optional additional properties of the a specific
vendor protocol nodes can be customized by each single vendor, and since,
as you said, you can have multiple protocols from different vendors sharing the
same protocol number, you could have multiple disjoint sets of valid properties
allowed under that same protocol node number; so on one side you have to stick
to some basic protocol-node defs and be rooted under the SCMI node, while on
the other side you will have multiple possibly allowed sets of additional
properties to check against, so IOW you cannot anyway just set
additionalProperties to false since that will result in no checks at all.

As a consequence, at runtime, in the final DTB shipped with a specific
platform you should have only one of the possible vendor nodes for each
of these overlapping protocols, and the SCMI core at probe time will
pick the proper protocol implementation based on the vendor/sub_vendor
IDs gathered from the running SCMI fw platform at init: this way you
can just build the usual "all-inclusive" defconfig without worrying
about vendor protocol clashes since the SCMI core can pick the right
protocol implementation, you should just had taken care to provide the
proper DTB for your protocol; BUT this also means that it is not possible
to add multiple DT bindings based on a 'if vendor' condition since the
vendor itself is NOT defined and not needed in the bindings since it is
discoverable at runtime.

So, after all of this blabbing of mine about this, I am wondering if it
is not possible that the solution is to handle each and every vendor
protocol node that appears with a block of addtional properties that
are picked via a oneOf statement from some external vendor specific
yaml.
(...in a similar way to how pinctrl additional properties are added...)


NOTE THAT the following is just an example of what I mean, it is certainly
wrong, incomplete annd maybe just not acceptable (and could cause DT
maintainers eyes to bleed :P)...

...so it is just fr the sake of explaining what I mean...

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index e9d3f043c4ed..3c38a1e3ffed 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -278,6 +278,22 @@ properties:
     required:
       - reg
 
+  protocol@81:
+    $ref: '#/$defs/protocol-node'
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        const: 0x81
+
+    patternProperties:
+      '$':
+        type: object
+        oneOf:
+          - $ref: /schemas/vendor-A/scmi-protos.yaml#
+          - $ref: /schemas/vendor-B/protos.yaml#
+        unevaluatedProperties: false
+
 additionalProperties: false
 
...with each new Vendor coming to the party adding a line under
oneOf...which would mean probably also having a protocol@NN for each new
protocol that appears...

Thanks,
Cristian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Tue, Apr 09, 2024 at 11:19:31AM +0000, Peng Fan wrote:
>
> ok, will add more comment in the patch for the commands.
>

No I meant add document/description similar to SCMI spec for each of these
commands. Not just one line comment. For std protocols, we can refer spec,
for these vendor protocols, just one like comment will not suffice. Describe
in more details and hence the request for separate TXT file for that.
Hope that is clear now, I have mentioned it several times already.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 i.MX
> BBM protocol
> 
> On Tue, Apr 09, 2024 at 11:19:31AM +0000, Peng Fan wrote:
> >
> > ok, will add more comment in the patch for the commands.
> >
> 
> No I meant add document/description similar to SCMI spec for each of these
> commands. Not just one line comment. For std protocols, we can refer spec,
> for these vendor protocols, just one like comment will not suffice. Describe in
> more details and hence the request for separate TXT file for that.
> Hope that is clear now, I have mentioned it several times already.

I see, will try to work with our firmware developers to write the details.

Thanks,
Peng.
> 
> --
> Regards,
> Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Rob Herring]

On Tue, Apr 9, 2024 at 7:01 AM Cristian Marussi
<cristian.marussi@arm.com> wrote:
>
> On Tue, Apr 09, 2024 at 09:25:10AM +0000, Peng Fan wrote:
> > Hi Krzysztof,
> >
> > > Subject: RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > additionalProperties to true
> > >
> > > > Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > > additionalProperties to true
> > > >
> > > > On 08/04/2024 08:08, Peng Fan wrote:
> > > > >> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > > >> additionalProperties to true
> > > > >>
> > > > >> On 08/04/2024 01:50, Peng Fan wrote:
> > > > >>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > > >>>> additionalProperties to true
> > > > >>>>
> > > > >>>> On 07/04/2024 12:04, Peng Fan wrote:
> > > > >>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi:
> > > > >>>>>> set additionalProperties to true
> > > > >>>>>>
> > > > >>>>>> On 07/04/2024 02:37, Peng Fan wrote:
> > > > >>>>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi:
> > > > >>>>>>>> set additionalProperties to true
> > > > >>>>>>>>
> > > > >>>>>>>> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > > > >>>>>>>>> From: Peng Fan <peng.fan@nxp.com>
> > > > >>>>>>>>>
> > > > >>>>>>>>> When adding vendor extension protocols, there is dt-schema
> > > > >> warning:
> > > > >>>>>>>>> "
> > > > >>>>>>>>> imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do
> > > > >>>>>>>>> not match any of the regexes: 'pinctrl-[0-9]+'
> > > > >>>>>>>>> "
> > > > >>>>>>>>>
> > > > >>>>>>>>> Set additionalProperties to true to address the issue.
> > > > >>>>>>>>
> > > > >>>>>>>> I do not see anything addressed here, except making the
> > > > >>>>>>>> binding accepting anything anywhere...
> > > > >>>>>>>
> > > > >>>>>>> I not wanna add vendor protocols in arm,scmi.yaml, so will
> > > > >>>>>>> introduce a new yaml imx.scmi.yaml which add i.MX SCMI
> > > > >>>>>>> protocol
> > > > >> extension.
> > > > >>>>>>>
> > > > >>>>>>> With additionalProperties set to false, I not know how, please
> > > > suggest.
> > > > >>>>>>
> > > > >>>>>> First of all, you cannot affect negatively existing devices
> > > > >>>>>> (their
> > > > >>>>>> bindings) and your patch does exactly that. This should make
> > > > >>>>>> you thing what is the correct approach...
> > > > >>>>>>
> > > > >>>>>> Rob gave you the comment about missing compatible - you still
> > > > >>>>>> did not address that.
> > > > >>>>>
> > > > >>>>> I added the compatible in patch 2/6 in the examples "compatible
> > > > >>>>> =
> > > > >>>> "arm,scmi";"
> > > > >>>>
> > > > >>>> So you claim that your vendor extensions are the same or fully
> > > > >>>> compatible with arm,scmi and you add nothing... Are your
> > > > >>>> extensions/protocol valid for arm,scmi?
> > > > >>>
> > > > >>> Yes. They are valid for arm,scmi.
> > > > >>>
> > > > >>>  If yes, why is this in separate binding. If no, why you use
> > > > >>> someone
> > > > >>>> else's compatible?
> > > > >>>
> > > > >>> Per SCMI Spec
> > > > >>> 0x80-0xFF: Reserved for vendor or platform-specific extensions to
> > > > >>> this interface
> > > > >>>
> > > > >>> i.MX use 0x81 for BBM, 0x84 for MISC. But other vendors will use
> > > > >>> the id for their own protocol.
> > > > >>
> > > > >> So how are they valid for arm,scmi? I don't understand.
> > > > >
> > > > > arm,scmi is a firmware compatible string. The protocol node is a sub-node.
> > > > > I think the arm,scmi is that saying the firmware following SCMI spec
> > > > > to implement the protocols.
> > > > >
> > > > > For vendor reserved ID, firmware also follow the SCMI spec to
> > > > > implement their own usage, so from firmware level, it is ARM SCMI
> > > > > spec
> > > > compatible.
> > > >
> > > > That's not the point. It is obvious that your firmware is compatible
> > > > with arm,scmi, but what you try to say in this and revised patch is
> > > > that every arm,scmi is compatible with your implementation. What you
> > > > are saying is that 0x84 is MISC protocol for every firmware, Qualcomm,
> > > > NXP, Samsung, TI, Mediatek etc.
> > > >
> > > > I claim it is not true. 0x84 is not MISC for Qualcomm, for example.
> > >
> > > You are right. I am lost now on how to add vendor ID support, using
> > > arm,scmi.yaml or adding a new imx,scmi.yaml or else.
> >
>
> Hi Peng,
>
> I dont think in the following you will find the solution to the problem,
> it is just to recap the situation and constraints around vendor protocol
> bindings.
>
> Describing SCMI vendors protocols is tricky because while on one side
> the protocol node has to be rooted under the main scmi fw DT node (like
> all the standard protocols) and be 'derived' from the arm,scmi.yaml
> protocol-node definition, the optional additional properties of the a specific
> vendor protocol nodes can be customized by each single vendor, and since,
> as you said, you can have multiple protocols from different vendors sharing the
> same protocol number, you could have multiple disjoint sets of valid properties
> allowed under that same protocol node number; so on one side you have to stick
> to some basic protocol-node defs and be rooted under the SCMI node, while on
> the other side you will have multiple possibly allowed sets of additional
> properties to check against, so IOW you cannot anyway just set
> additionalProperties to false since that will result in no checks at all.
>
> As a consequence, at runtime, in the final DTB shipped with a specific
> platform you should have only one of the possible vendor nodes for each
> of these overlapping protocols, and the SCMI core at probe time will
> pick the proper protocol implementation based on the vendor/sub_vendor
> IDs gathered from the running SCMI fw platform at init: this way you
> can just build the usual "all-inclusive" defconfig without worrying
> about vendor protocol clashes since the SCMI core can pick the right
> protocol implementation, you should just had taken care to provide the
> proper DTB for your protocol; BUT this also means that it is not possible
> to add multiple DT bindings based on a 'if vendor' condition since the
> vendor itself is NOT defined and not needed in the bindings since it is
> discoverable at runtime.
>
> So, after all of this blabbing of mine about this, I am wondering if it
> is not possible that the solution is to handle each and every vendor
> protocol node that appears with a block of addtional properties that
> are picked via a oneOf statement from some external vendor specific
> yaml.
> (...in a similar way to how pinctrl additional properties are added...)
>
>
> NOTE THAT the following is just an example of what I mean, it is certainly
> wrong, incomplete annd maybe just not acceptable (and could cause DT
> maintainers eyes to bleed :P)...
>
> ...so it is just fr the sake of explaining what I mean...
>
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index e9d3f043c4ed..3c38a1e3ffed 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -278,6 +278,22 @@ properties:
>      required:
>        - reg
>
> +  protocol@81:
> +    $ref: '#/$defs/protocol-node'
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        const: 0x81
> +
> +    patternProperties:
> +      '$':
> +        type: object

Did you mean to have child nodes under the protocol node rather than in it?

> +        oneOf:
> +          - $ref: /schemas/vendor-A/scmi-protos.yaml#
> +          - $ref: /schemas/vendor-B/protos.yaml#

Moved up one level, this would work, but it would have to be an
'anyOf' because it is possible that 2 vendors have the exact same set
of properties.

I can think of 2 other ways to structure this.

First, is a specific vendor protocol discoverable? Not that is 0x81
protocol present, but that 0x81 is vendor Foo's extra special
value-add protocol? If not, I think we should require a compatible
string on vendor protocols. Then the base SCMI schema can require just
that, and each vendor protocol defines its node with a $ref to
'#/$defs/protocol-node'.

The 2nd way is just a variation of the oneOf above, but do we do 1
file per vendor protocol or 1 file per vendor. Either should be
doable, just a matter of where 'protocol@81', etc. are defined.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Cristian Marussi]

On Tue, Apr 09, 2024 at 09:09:46AM -0500, Rob Herring wrote:
> On Tue, Apr 9, 2024 at 7:01 AM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> >
> > On Tue, Apr 09, 2024 at 09:25:10AM +0000, Peng Fan wrote:
> > > Hi Krzysztof,
> > >
> > > > Subject: RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > > additionalProperties to true
> > > >
> > > > > Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > > > additionalProperties to true
> > > > >
> > > > > On 08/04/2024 08:08, Peng Fan wrote:
> > > > > >> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > > > >> additionalProperties to true
> > > > > >>
> > > > > >> On 08/04/2024 01:50, Peng Fan wrote:
> > > > > >>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > > > >>>> additionalProperties to true
> > > > > >>>>
> > > > > >>>> On 07/04/2024 12:04, Peng Fan wrote:
> > > > > >>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi:
> > > > > >>>>>> set additionalProperties to true
> > > > > >>>>>>
> > > > > >>>>>> On 07/04/2024 02:37, Peng Fan wrote:
> > > > > >>>>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi:
> > > > > >>>>>>>> set additionalProperties to true
> > > > > >>>>>>>>
> > > > > >>>>>>>> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > > > > >>>>>>>>> From: Peng Fan <peng.fan@nxp.com>
> > > > > >>>>>>>>>
> > > > > >>>>>>>>> When adding vendor extension protocols, there is dt-schema
> > > > > >> warning:
> > > > > >>>>>>>>> "
> > > > > >>>>>>>>> imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do
> > > > > >>>>>>>>> not match any of the regexes: 'pinctrl-[0-9]+'
> > > > > >>>>>>>>> "
> > > > > >>>>>>>>>
> > > > > >>>>>>>>> Set additionalProperties to true to address the issue.
> > > > > >>>>>>>>
> > > > > >>>>>>>> I do not see anything addressed here, except making the
> > > > > >>>>>>>> binding accepting anything anywhere...
> > > > > >>>>>>>
> > > > > >>>>>>> I not wanna add vendor protocols in arm,scmi.yaml, so will
> > > > > >>>>>>> introduce a new yaml imx.scmi.yaml which add i.MX SCMI
> > > > > >>>>>>> protocol
> > > > > >> extension.
> > > > > >>>>>>>
> > > > > >>>>>>> With additionalProperties set to false, I not know how, please
> > > > > suggest.
> > > > > >>>>>>
> > > > > >>>>>> First of all, you cannot affect negatively existing devices
> > > > > >>>>>> (their
> > > > > >>>>>> bindings) and your patch does exactly that. This should make
> > > > > >>>>>> you thing what is the correct approach...
> > > > > >>>>>>
> > > > > >>>>>> Rob gave you the comment about missing compatible - you still
> > > > > >>>>>> did not address that.
> > > > > >>>>>
> > > > > >>>>> I added the compatible in patch 2/6 in the examples "compatible
> > > > > >>>>> =
> > > > > >>>> "arm,scmi";"
> > > > > >>>>
> > > > > >>>> So you claim that your vendor extensions are the same or fully
> > > > > >>>> compatible with arm,scmi and you add nothing... Are your
> > > > > >>>> extensions/protocol valid for arm,scmi?
> > > > > >>>
> > > > > >>> Yes. They are valid for arm,scmi.
> > > > > >>>
> > > > > >>>  If yes, why is this in separate binding. If no, why you use
> > > > > >>> someone
> > > > > >>>> else's compatible?
> > > > > >>>
> > > > > >>> Per SCMI Spec
> > > > > >>> 0x80-0xFF: Reserved for vendor or platform-specific extensions to
> > > > > >>> this interface
> > > > > >>>
> > > > > >>> i.MX use 0x81 for BBM, 0x84 for MISC. But other vendors will use
> > > > > >>> the id for their own protocol.
> > > > > >>
> > > > > >> So how are they valid for arm,scmi? I don't understand.
> > > > > >
> > > > > > arm,scmi is a firmware compatible string. The protocol node is a sub-node.
> > > > > > I think the arm,scmi is that saying the firmware following SCMI spec
> > > > > > to implement the protocols.
> > > > > >
> > > > > > For vendor reserved ID, firmware also follow the SCMI spec to
> > > > > > implement their own usage, so from firmware level, it is ARM SCMI
> > > > > > spec
> > > > > compatible.
> > > > >
> > > > > That's not the point. It is obvious that your firmware is compatible
> > > > > with arm,scmi, but what you try to say in this and revised patch is
> > > > > that every arm,scmi is compatible with your implementation. What you
> > > > > are saying is that 0x84 is MISC protocol for every firmware, Qualcomm,
> > > > > NXP, Samsung, TI, Mediatek etc.
> > > > >
> > > > > I claim it is not true. 0x84 is not MISC for Qualcomm, for example.
> > > >
> > > > You are right. I am lost now on how to add vendor ID support, using
> > > > arm,scmi.yaml or adding a new imx,scmi.yaml or else.
> > >
> >
> > Hi Peng,
> >
> > I dont think in the following you will find the solution to the problem,
> > it is just to recap the situation and constraints around vendor protocol
> > bindings.
> >
> > Describing SCMI vendors protocols is tricky because while on one side
> > the protocol node has to be rooted under the main scmi fw DT node (like
> > all the standard protocols) and be 'derived' from the arm,scmi.yaml
> > protocol-node definition, the optional additional properties of the a specific
> > vendor protocol nodes can be customized by each single vendor, and since,
> > as you said, you can have multiple protocols from different vendors sharing the
> > same protocol number, you could have multiple disjoint sets of valid properties
> > allowed under that same protocol node number; so on one side you have to stick
> > to some basic protocol-node defs and be rooted under the SCMI node, while on
> > the other side you will have multiple possibly allowed sets of additional
> > properties to check against, so IOW you cannot anyway just set
> > additionalProperties to false since that will result in no checks at all.
> >
> > As a consequence, at runtime, in the final DTB shipped with a specific
> > platform you should have only one of the possible vendor nodes for each
> > of these overlapping protocols, and the SCMI core at probe time will
> > pick the proper protocol implementation based on the vendor/sub_vendor
> > IDs gathered from the running SCMI fw platform at init: this way you
> > can just build the usual "all-inclusive" defconfig without worrying
> > about vendor protocol clashes since the SCMI core can pick the right
> > protocol implementation, you should just had taken care to provide the
> > proper DTB for your protocol; BUT this also means that it is not possible
> > to add multiple DT bindings based on a 'if vendor' condition since the
> > vendor itself is NOT defined and not needed in the bindings since it is
> > discoverable at runtime.
> >
> > So, after all of this blabbing of mine about this, I am wondering if it
> > is not possible that the solution is to handle each and every vendor
> > protocol node that appears with a block of addtional properties that
> > are picked via a oneOf statement from some external vendor specific
> > yaml.
> > (...in a similar way to how pinctrl additional properties are added...)
> >
> >
> > NOTE THAT the following is just an example of what I mean, it is certainly
> > wrong, incomplete annd maybe just not acceptable (and could cause DT
> > maintainers eyes to bleed :P)...
> >
> > ...so it is just fr the sake of explaining what I mean...
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > index e9d3f043c4ed..3c38a1e3ffed 100644
> > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > @@ -278,6 +278,22 @@ properties:
> >      required:
> >        - reg
> >
> > +  protocol@81:
> > +    $ref: '#/$defs/protocol-node'
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        const: 0x81
> > +
> > +    patternProperties:
> > +      '$':
> > +        type: object
> 
> Did you mean to have child nodes under the protocol node rather than in it?

... nope ... it is just as bad as my yaml-fu is :P ... but not sure if
vendors has also this needs or plain props will suffice...

> 
> > +        oneOf:
> > +          - $ref: /schemas/vendor-A/scmi-protos.yaml#
> > +          - $ref: /schemas/vendor-B/protos.yaml#
> 
> Moved up one level, this would work, but it would have to be an
> 'anyOf' because it is possible that 2 vendors have the exact same set
> of properties.
> 

ok

> I can think of 2 other ways to structure this.
> 
> First, is a specific vendor protocol discoverable? Not that is 0x81
> protocol present, but that 0x81 is vendor Foo's extra special
> value-add protocol? If not, I think we should require a compatible
> string on vendor protocols. Then the base SCMI schema can require just
> that, and each vendor protocol defines its node with a $ref to
> '#/$defs/protocol-node'.

Basically yes it is discoverable, since at runtime the SCMI core, early on,
normally discovers the vendor_id/sub_vendor_id by querying the platform via
Base protocol and then later only loads/initializes (by closest match) the
vendor protocols that are present in the DT AND that has been 'tagged' at
compile time with the same vendor_id/sub_vendor_id tuple (in the vendor
module code, struct scmi_protocol)

Of course you should take care to put the proper protocol@81 node in your
vendor_A DTB for the vendor_A SCMI driver to make use of the additional
vendor_A properties that you have defined under your node as referred
in your vendor-protos.yaml...if you botch that up I will load a protocol
and call your vendor_A driver with a vendor_X DT node.

DT is currrently vendor-agnostic.

> 
> The 2nd way is just a variation of the oneOf above, but do we do 1
> file per vendor protocol or 1 file per vendor. Either should be
> doable, just a matter of where 'protocol@81', etc. are defined.
> 

Oh, yes mine was just an ill example...one file per vendor will do just
fine: the important thing is that the list and the yaml itself can be
extended as new vendors appears (in a backward compatble way of course)

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension protocol

[Rob Herring]

On Fri, Apr 05, 2024 at 08:39:24PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Add i.MX SCMI Extension protocols bindings for:
>  - Battery Backed Secure Module(BBSM)
>  - MISC settings such as General Purpose Registers settings.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../devicetree/bindings/firmware/imx,scmi.yaml     | 80 ++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/imx,scmi.yaml b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> new file mode 100644
> index 000000000000..7ee19a661d83
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2024 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/firmware/imx,scmi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: i.MX System Control and Management Interface(SCMI) Vendor Protocols Extension
> +
> +maintainers:
> +  - Peng Fan <peng.fan@nxp.com>
> +
> +allOf:
> +  - $ref: arm,scmi.yaml#

This needs to be the other way around. Add a ref to this file in 
arm,scmi.yaml under an 'anyOf' entry.

> +
> +properties:
> +  protocol@81:
> +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> +    unevaluatedProperties: false
> +    description:
> +      The BBM Protocol is for managing Battery Backed Secure Module (BBSM) RTC
> +      and the ON/OFF Key
> +
> +    properties:
> +      reg:
> +        const: 0x81
> +
> +    required:
> +      - reg
> +
> +  protocol@84:
> +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> +    unevaluatedProperties: false
> +    description:
> +      The MISC Protocol is for managing SoC Misc settings, such as GPR settings
> +
> +    properties:
> +      reg:
> +        const: 0x84
> +
> +      wakeup-sources:
> +        description:
> +          Each entry consists of 2 integers, represents the source and electric signal edge
> +        items:
> +          items:
> +            - description: the wakeup source
> +            - description: the wakeup electric signal edge

No constraints on the entry values?

> +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +
> +    required:
> +      - reg
> +
> +additionalProperties: false

And then this can be true.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension protocol

[Peng Fan]

> Subject: Re: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension
> protocol
> 
> On Fri, Apr 05, 2024 at 08:39:24PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add i.MX SCMI Extension protocols bindings for:
> >  - Battery Backed Secure Module(BBSM)
> >  - MISC settings such as General Purpose Registers settings.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  .../devicetree/bindings/firmware/imx,scmi.yaml     | 80
> ++++++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > new file mode 100644
> > index 000000000000..7ee19a661d83
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright 2024
> > +NXP %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Ffirmware%2Fimx%2Cscmi.yaml%23&data=05%7
> C02%7Cp
> >
> +eng.fan%40nxp.com%7C25c72418c9864a73924808dc59826288%7C686ea
> 1d3bc2b4c
> >
> +6fa92cd99c5c301635%7C0%7C0%7C638483663734828123%7CUnknown%
> 7CTWFpbGZsb
> >
> +3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D
> >
> +%7C0%7C%7C%7C&sdata=Y2uSGDorqR9HK8PO4AQDQ%2FaTv%2BETnulvU
> C8u9ktDoio%3
> > +D&reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fmeta-
> schemas%2Fcore.yaml%23&data=05%7C02%7Cpeng.fan%40nx
> >
> +p.com%7C25c72418c9864a73924808dc59826288%7C686ea1d3bc2b4c6fa
> 92cd99c5c
> >
> +301635%7C0%7C0%7C638483663734841350%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoiM
> >
> +C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7
> C%7C%7
> >
> +C&sdata=h6lSgnPvR88CEBxmU%2B%2FCfCx9GHUrogWVxck38sIdhB4%3D&r
> eserved=0
> > +
> > +title: i.MX System Control and Management Interface(SCMI) Vendor
> > +Protocols Extension
> > +
> > +maintainers:
> > +  - Peng Fan <peng.fan@nxp.com>
> > +
> > +allOf:
> > +  - $ref: arm,scmi.yaml#
> 
> This needs to be the other way around. Add a ref to this file in arm,scmi.yaml
> under an 'anyOf' entry.

ok, I will give a try.

> 
> > +
> > +properties:
> > +  protocol@81:
> > +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > +    unevaluatedProperties: false
> > +    description:
> > +      The BBM Protocol is for managing Battery Backed Secure Module
> (BBSM) RTC
> > +      and the ON/OFF Key
> > +
> > +    properties:
> > +      reg:
> > +        const: 0x81
> > +
> > +    required:
> > +      - reg
> > +
> > +  protocol@84:
> > +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > +    unevaluatedProperties: false
> > +    description:
> > +      The MISC Protocol is for managing SoC Misc settings, such as
> > + GPR settings
> > +
> > +    properties:
> > +      reg:
> > +        const: 0x84
> > +
> > +      wakeup-sources:
> > +        description:
> > +          Each entry consists of 2 integers, represents the source and electric
> signal edge
> > +        items:
> > +          items:
> > +            - description: the wakeup source
> > +            - description: the wakeup electric signal edge
> 
> No constraints on the entry values?

I will change the property name to imx,wakup-sources with constraints
minItems: 1
maxItems: 32.

> 
> > +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +
> > +    required:
> > +      - reg
> > +
> > +additionalProperties: false
> 
> And then this can be true.

Fix in v3.

Thanks
Peng.

> 
> Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

[Peng Fan]

Hi Rob, Cristian,

> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> additionalProperties to true
> 
> On Tue, Apr 09, 2024 at 09:09:46AM -0500, Rob Herring wrote:
> > On Tue, Apr 9, 2024 at 7:01 AM Cristian Marussi
> > <cristian.marussi@arm.com> wrote:
> > >
> > > On Tue, Apr 09, 2024 at 09:25:10AM +0000, Peng Fan wrote:
> > > > Hi Krzysztof,
> > > >
> > > > > Subject: RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > > > additionalProperties to true
> > > > >
> > > > > > Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi:
> > > > > > set additionalProperties to true
> > > > > >
> > > > > > On 08/04/2024 08:08, Peng Fan wrote:
> > > > > > >> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware:
> > > > > > >> arm,scmi: set additionalProperties to true
> > > > > > >>
> > > > > > >> On 08/04/2024 01:50, Peng Fan wrote:
> > > > > > >>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware:
> > > > > > >>>> arm,scmi: set additionalProperties to true
> > > > > > >>>>
> > > > > > >>>> On 07/04/2024 12:04, Peng Fan wrote:
> > > > > > >>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi:
> > > > > > >>>>>> set additionalProperties to true
> > > > > > >>>>>>
> > > > > > >>>>>> On 07/04/2024 02:37, Peng Fan wrote:
> > > > > > >>>>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware:
> arm,scmi:
> > > > > > >>>>>>>> set additionalProperties to true
> > > > > > >>>>>>>>
> > > > > > >>>>>>>> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > > > > > >>>>>>>>> From: Peng Fan <peng.fan@nxp.com>
> > > > > > >>>>>>>>>
> > > > > > >>>>>>>>> When adding vendor extension protocols, there is
> > > > > > >>>>>>>>> dt-schema
> > > > > > >> warning:
> > > > > > >>>>>>>>> "
> > > > > > >>>>>>>>> imx,scmi.example.dtb: scmi: 'protocol@81',
> > > > > > >>>>>>>>> 'protocol@84' do not match any of the regexes: 'pinctrl-
> [0-9]+'
> > > > > > >>>>>>>>> "
> > > > > > >>>>>>>>>
> > > > > > >>>>>>>>> Set additionalProperties to true to address the issue.
> > > > > > >>>>>>>>
> > > > > > >>>>>>>> I do not see anything addressed here, except making
> > > > > > >>>>>>>> the binding accepting anything anywhere...
> > > > > > >>>>>>>
> > > > > > >>>>>>> I not wanna add vendor protocols in arm,scmi.yaml, so
> > > > > > >>>>>>> will introduce a new yaml imx.scmi.yaml which add i.MX
> > > > > > >>>>>>> SCMI protocol
> > > > > > >> extension.
> > > > > > >>>>>>>
> > > > > > >>>>>>> With additionalProperties set to false, I not know
> > > > > > >>>>>>> how, please
> > > > > > suggest.
> > > > > > >>>>>>
> > > > > > >>>>>> First of all, you cannot affect negatively existing
> > > > > > >>>>>> devices (their
> > > > > > >>>>>> bindings) and your patch does exactly that. This should
> > > > > > >>>>>> make you thing what is the correct approach...
> > > > > > >>>>>>
> > > > > > >>>>>> Rob gave you the comment about missing compatible - you
> > > > > > >>>>>> still did not address that.
> > > > > > >>>>>
> > > > > > >>>>> I added the compatible in patch 2/6 in the examples
> > > > > > >>>>> "compatible =
> > > > > > >>>> "arm,scmi";"
> > > > > > >>>>
> > > > > > >>>> So you claim that your vendor extensions are the same or
> > > > > > >>>> fully compatible with arm,scmi and you add nothing... Are
> > > > > > >>>> your extensions/protocol valid for arm,scmi?
> > > > > > >>>
> > > > > > >>> Yes. They are valid for arm,scmi.
> > > > > > >>>
> > > > > > >>>  If yes, why is this in separate binding. If no, why you
> > > > > > >>> use someone
> > > > > > >>>> else's compatible?
> > > > > > >>>
> > > > > > >>> Per SCMI Spec
> > > > > > >>> 0x80-0xFF: Reserved for vendor or platform-specific
> > > > > > >>> extensions to this interface
> > > > > > >>>
> > > > > > >>> i.MX use 0x81 for BBM, 0x84 for MISC. But other vendors
> > > > > > >>> will use the id for their own protocol.
> > > > > > >>
> > > > > > >> So how are they valid for arm,scmi? I don't understand.
> > > > > > >
> > > > > > > arm,scmi is a firmware compatible string. The protocol node is a
> sub-node.
> > > > > > > I think the arm,scmi is that saying the firmware following
> > > > > > > SCMI spec to implement the protocols.
> > > > > > >
> > > > > > > For vendor reserved ID, firmware also follow the SCMI spec
> > > > > > > to implement their own usage, so from firmware level, it is
> > > > > > > ARM SCMI spec
> > > > > > compatible.
> > > > > >
> > > > > > That's not the point. It is obvious that your firmware is
> > > > > > compatible with arm,scmi, but what you try to say in this and
> > > > > > revised patch is that every arm,scmi is compatible with your
> > > > > > implementation. What you are saying is that 0x84 is MISC
> > > > > > protocol for every firmware, Qualcomm, NXP, Samsung, TI, Mediatek
> etc.
> > > > > >
> > > > > > I claim it is not true. 0x84 is not MISC for Qualcomm, for example.
> > > > >
> > > > > You are right. I am lost now on how to add vendor ID support,
> > > > > using arm,scmi.yaml or adding a new imx,scmi.yaml or else.
> > > >
> > >
> > > Hi Peng,
> > >
> > > I dont think in the following you will find the solution to the
> > > problem, it is just to recap the situation and constraints around
> > > vendor protocol bindings.
> > >
> > > Describing SCMI vendors protocols is tricky because while on one
> > > side the protocol node has to be rooted under the main scmi fw DT
> > > node (like all the standard protocols) and be 'derived' from the
> > > arm,scmi.yaml protocol-node definition, the optional additional
> > > properties of the a specific vendor protocol nodes can be customized
> > > by each single vendor, and since, as you said, you can have multiple
> > > protocols from different vendors sharing the same protocol number,
> > > you could have multiple disjoint sets of valid properties allowed
> > > under that same protocol node number; so on one side you have to
> > > stick to some basic protocol-node defs and be rooted under the SCMI
> > > node, while on the other side you will have multiple possibly
> > > allowed sets of additional properties to check against, so IOW you cannot
> anyway just set additionalProperties to false since that will result in no checks
> at all.
> > >
> > > As a consequence, at runtime, in the final DTB shipped with a
> > > specific platform you should have only one of the possible vendor
> > > nodes for each of these overlapping protocols, and the SCMI core at
> > > probe time will pick the proper protocol implementation based on the
> > > vendor/sub_vendor IDs gathered from the running SCMI fw platform at
> > > init: this way you can just build the usual "all-inclusive"
> > > defconfig without worrying about vendor protocol clashes since the
> > > SCMI core can pick the right protocol implementation, you should
> > > just had taken care to provide the proper DTB for your protocol; BUT
> > > this also means that it is not possible to add multiple DT bindings
> > > based on a 'if vendor' condition since the vendor itself is NOT
> > > defined and not needed in the bindings since it is discoverable at runtime.
> > >
> > > So, after all of this blabbing of mine about this, I am wondering if
> > > it is not possible that the solution is to handle each and every
> > > vendor protocol node that appears with a block of addtional
> > > properties that are picked via a oneOf statement from some external
> > > vendor specific yaml.
> > > (...in a similar way to how pinctrl additional properties are
> > > added...)
> > >
> > >
> > > NOTE THAT the following is just an example of what I mean, it is
> > > certainly wrong, incomplete annd maybe just not acceptable (and
> > > could cause DT maintainers eyes to bleed :P)...
> > >
> > > ...so it is just fr the sake of explaining what I mean...
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > index e9d3f043c4ed..3c38a1e3ffed 100644
> > > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > @@ -278,6 +278,22 @@ properties:
> > >      required:
> > >        - reg
> > >
> > > +  protocol@81:
> > > +    $ref: '#/$defs/protocol-node'
> > > +    unevaluatedProperties: false
> > > +
> > > +    properties:
> > > +      reg:
> > > +        const: 0x81
> > > +
> > > +    patternProperties:
> > > +      '$':
> > > +        type: object
> >
> > Did you mean to have child nodes under the protocol node rather than in it?
> 
> ... nope ... it is just as bad as my yaml-fu is :P ... but not sure if vendors has
> also this needs or plain props will suffice...
> 
> >
> > > +        oneOf:
> > > +          - $ref: /schemas/vendor-A/scmi-protos.yaml#
> > > +          - $ref: /schemas/vendor-B/protos.yaml#
> >
> > Moved up one level, this would work, but it would have to be an
> > 'anyOf' because it is possible that 2 vendors have the exact same set
> > of properties.
> >

I try this:
  protocol@84:                                                                                      
    anyOf:                                                                                          
      - $ref: /schemas/firmware/imx,scmi.yaml#                                                      
      - $ref: /schemas/firmware/vendor-B,scmi.yaml#     

but unevaluatedProperties could not be set to false, otherwise the
example check will report
reg is unevaluated, fsl,wakeup-sources is unevaluated.

The imx,scmi.yaml is as below:
properties: 
 protocol@84:
    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
    unevaluatedProperties: false
    description:
      The MISC Protocol is for managing SoC Misc settings, such as GPR settings

    properties:
      reg:
        const: 0x84

      fsl,wakeup-sources:
        description:
          Each entry consists of 2 integers, represents the source and electric signal edge
        items:
          items:
            - description: the wakeup source
            - description: the wakeup electric signal edge
        minItems: 1
        maxItems: 32
        $ref: /schemas/types.yaml#/definitions/uint32-matrix

    required:
      - reg

additionalProperties: true


Is the upper ok?

Thanks,
Peng.
> 
> ok
> 
> > I can think of 2 other ways to structure this.
> >
> > First, is a specific vendor protocol discoverable? Not that is 0x81
> > protocol present, but that 0x81 is vendor Foo's extra special
> > value-add protocol? If not, I think we should require a compatible
> > string on vendor protocols. Then the base SCMI schema can require just
> > that, and each vendor protocol defines its node with a $ref to
> > '#/$defs/protocol-node'.
> 
> Basically yes it is discoverable, since at runtime the SCMI core, early on,
> normally discovers the vendor_id/sub_vendor_id by querying the platform
> via Base protocol and then later only loads/initializes (by closest match) the
> vendor protocols that are present in the DT AND that has been 'tagged' at
> compile time with the same vendor_id/sub_vendor_id tuple (in the vendor
> module code, struct scmi_protocol)
> 
> Of course you should take care to put the proper protocol@81 node in your
> vendor_A DTB for the vendor_A SCMI driver to make use of the additional
> vendor_A properties that you have defined under your node as referred in
> your vendor-protos.yaml...if you botch that up I will load a protocol and call
> your vendor_A driver with a vendor_X DT node.
> 
> DT is currrently vendor-agnostic.
> 
> >
> > The 2nd way is just a variation of the oneOf above, but do we do 1
> > file per vendor protocol or 1 file per vendor. Either should be
> > doable, just a matter of where 'protocol@81', etc. are defined.
> >
> 
> Oh, yes mine was just an ill example...one file per vendor will do just
> fine: the important thing is that the list and the yaml itself can be extended as
> new vendors appears (in a backward compatble way of course)
> 
> Thanks,
> Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel