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

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

[Peng Fan (OSS)]

i.MX95 System Manager Firmware support vendor extension protocol:
- 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>
---
Peng Fan (5):
      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/nxp,scmi.yaml     |  64 ++++
 drivers/firmware/arm_scmi/Kconfig                  |  20 ++
 drivers/firmware/arm_scmi/Makefile                 |   2 +
 drivers/firmware/arm_scmi/imx-sm-bbm.c             | 381 +++++++++++++++++++++
 drivers/firmware/arm_scmi/imx-sm-misc.c            | 289 ++++++++++++++++
 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_nxp_protocol.h                  |  67 ++++
 10 files changed, 1267 insertions(+)
---
base-commit: 51b70ff55ed88edd19b080a524063446bcc34b62
change-id: 20240202-imx95-bbm-misc-d4a27c159823

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 1/5] dt-bindings: firmware: add i.MX SCMI Extension protocol

[Peng Fan (OSS)]

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

Add i.MX SCMI Extension protocol BBM and MISC binding.

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

diff --git a/Documentation/devicetree/bindings/firmware/nxp,scmi.yaml b/Documentation/devicetree/bindings/firmware/nxp,scmi.yaml
new file mode 100644
index 000000000000..00d6361bbbea
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/nxp,scmi.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2024 NXP
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/firmware/nxp,scmi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: i.MX System Control and Management Interface (SCMI) Protocol Extension
+
+maintainers:
+  - Peng Fan <peng.fan@nxp.com>
+
+allOf:
+  - $ref: arm,scmi.yaml#
+
+properties:
+  protocol@11:
+    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        const: 0x81
+
+  protocol@13:
+    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        const: 0x84
+
+      wakeup-sources:
+        description: each entry consists of 2 integers and represents the source and edge
+        items:
+          items:
+            - description: the wakeup source
+            - description: the wakeup edge
+        $ref: /schemas/types.yaml#/definitions/uint32-matrix
+
+additionalProperties: false
+
+examples:
+  - |
+    firmware {
+        scmi {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            protocol@81 {
+              reg = <0x81>;
+            };
+
+            protocol@84 {
+              reg = <0x84>;
+              wakeup-sources = <6 1
+                                7 1
+                                8 1
+                                9 1
+                                10 1>;
+            };
+         };
+    };
+...

-- 
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 1/5] dt-bindings: firmware: add i.MX SCMI Extension protocol

[Rob Herring]

On Fri, Feb 02, 2024 at 02:34:39PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Add i.MX SCMI Extension protocol BBM and MISC binding.

No idea what BBM and MISC are.

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

Wrong unit-address?

> +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> +    unevaluatedProperties: false

Description of what this protocol is needed.

> +
> +    properties:
> +      reg:
> +        const: 0x81
> +
> +  protocol@13:
> +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        const: 0x84
> +
> +      wakeup-sources:

Is this somehow generic?

> +        description: each entry consists of 2 integers and represents the source and edge

What does 'edge' mean in this context?

> +        items:
> +          items:
> +            - description: the wakeup source
> +            - description: the wakeup edge

Constraints?

> +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    firmware {
> +        scmi {


Need a compatible here so this actually gets tested.

> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            protocol@81 {
> +              reg = <0x81>;
> +            };
> +
> +            protocol@84 {
> +              reg = <0x84>;
> +              wakeup-sources = <6 1
> +                                7 1
> +                                8 1
> +                                9 1
> +                                10 1>;

<> around each entry. e.g. "<6 1>"

> +            };
> +         };
> +    };
> +...
> 
> -- 
> 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 1/5] dt-bindings: firmware: add i.MX SCMI Extension protocol

[Peng Fan]

Hi Rob,

Sorry for late reply.

> Subject: Re: [PATCH 1/5] dt-bindings: firmware: add i.MX SCMI Extension
> protocol
> 
> On Fri, Feb 02, 2024 at 02:34:39PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add i.MX SCMI Extension protocol BBM and MISC binding.
> 
> No idea what BBM and MISC are.
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: it is i.MX SCMI extension protocol, including BLK CTRL
settings, board level GPIO expander settings. 
> 
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  .../devicetree/bindings/firmware/nxp,scmi.yaml     | 64
> ++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/nxp,scmi.yaml
> > b/Documentation/devicetree/bindings/firmware/nxp,scmi.yaml
> > new file mode 100644
> > index 000000000000..00d6361bbbea
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/firmware/nxp,scmi.yaml
> > @@ -0,0 +1,64 @@
> > +# 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%2Fnxp%2Cscmi.yaml%23&data=05%7
> C02%7Cp
> >
> +eng.fan%40nxp.com%7C625d14c7c4f14d16289908dc2bdc9967%7C686ea1
> d3bc2b4c
> >
> +6fa92cd99c5c301635%7C0%7C0%7C638433473675932860%7CUnknown%
> 7CTWFpbGZsb
> >
> +3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D
> >
> +%7C0%7C%7C%7C&sdata=dP0%2FgyCwmWtSW9BNYWZQtunpgayjCl2AkSkj
> ZIZjn9o%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%7C625d14c7c4f14d16289908dc2bdc9967%7C686ea1d3bc2b4c6fa9
> 2cd99c5c
> >
> +301635%7C0%7C0%7C638433473675946764%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoiM
> >
> +C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7
> C%7C%7
> >
> +C&sdata=efmqKP8%2FyS4YoDLCb%2Fmxx72D7ZW2KxiEDhgnWdEUT1s%3D
> &reserved=0
> > +
> > +title: i.MX System Control and Management Interface (SCMI) Protocol
> > +Extension
> > +
> > +maintainers:
> > +  - Peng Fan <peng.fan@nxp.com>
> > +
> > +allOf:
> > +  - $ref: arm,scmi.yaml#
> > +
> > +properties:
> > +  protocol@11:
> 
> Wrong unit-address?

Yeah. Fixed.

> 
> > +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > +    unevaluatedProperties: false
> 
> Description of what this protocol is needed.

Added.

> 
> > +
> > +    properties:
> > +      reg:
> > +        const: 0x81
> > +
> > +  protocol@13:
> > +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        const: 0x84
> > +
> > +      wakeup-sources:
> 
> Is this somehow generic?

I think it yes, but if you disagree, please suggest.

> 
> > +        description: each entry consists of 2 integers and represents
> > + the source and edge
> 
> What does 'edge' mean in this context?

Electric signal edge.

> 
> > +        items:
> > +          items:
> > +            - description: the wakeup source
> > +            - description: the wakeup edge
> 
> Constraints?

Will add in V3.
minItems: 1
maxItems: 32
> 
> > +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    firmware {
> > +        scmi {
> 
> 
> Need a compatible here so this actually gets tested.

Fixed.

> 
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            protocol@81 {
> > +              reg = <0x81>;
> > +            };
> > +
> > +            protocol@84 {
> > +              reg = <0x84>;
> > +              wakeup-sources = <6 1
> > +                                7 1
> > +                                8 1
> > +                                9 1
> > +                                10 1>;
> 
> <> around each entry. e.g. "<6 1>"

Fix in V3.

Thanks,
Peng.
> 
> > +            };
> > +         };
> > +    };
> > +...
> >
> > --
> > 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 1/5] dt-bindings: firmware: add i.MX SCMI Extension protocol

[Cristian Marussi]

On Fri, Feb 02, 2024 at 02:34:39PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Add i.MX SCMI Extension protocol BBM and MISC binding.
> 

Hi,

just a few remarks down below only about the SCMI related stuff...

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../devicetree/bindings/firmware/nxp,scmi.yaml     | 64 ++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/nxp,scmi.yaml b/Documentation/devicetree/bindings/firmware/nxp,scmi.yaml
> new file mode 100644
> index 000000000000..00d6361bbbea
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/nxp,scmi.yaml
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2024 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/firmware/nxp,scmi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: i.MX System Control and Management Interface (SCMI) Protocol Extension
> +

... (SCMI) Vendor Protocols

> +maintainers:
> +  - Peng Fan <peng.fan@nxp.com>
> +
> +allOf:
> +  - $ref: arm,scmi.yaml#
> +
> +properties:
> +  protocol@11:

protocol@81 ?

> +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        const: 0x81
> +
> +  protocol@13:

protocol@84 ?

> +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        const: 0x84
> +
> +      wakeup-sources:
> +        description: each entry consists of 2 integers and represents the source and edge
> +        items:
> +          items:
> +            - description: the wakeup source
> +            - description: the wakeup edge
> +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    firmware {
> +        scmi {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            protocol@81 {
> +              reg = <0x81>;
> +            };
> +
> +            protocol@84 {
> +              reg = <0x84>;
> +              wakeup-sources = <6 1
> +                                7 1
> +                                8 1
> +                                9 1
> +                                10 1>;
> +            };
> +         };
> +    };
> +...
> 

Thanks,
Cristian

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

[PATCH 2/5] 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 | 381 +++++++++++++++++++++++++++++++++
 include/linux/scmi_nxp_protocol.h      |  50 +++++
 4 files changed, 442 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..0e12aaeabc42
--- /dev/null
+++ b/drivers/firmware/arm_scmi/imx-sm-bbm.c
@@ -0,0 +1,381 @@
+// 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_nxp_protocol.h>
+
+#include "protocols.h"
+#include "notify.h"
+
+#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x0
+
+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 BBM_PROTO_ATTR_NUM_RTC(x)  (((x) & 0xFFU) << 16U)
+#define BBM_PROTO_ATTR_NUM_GPR(x)  (((x) & 0xFFFFU) << 0U)
+
+#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_ID(x)        (((x) >> 24) & 0xFF)
+
+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;
+	struct scmi_msg_imx_bbm_rtc_notify *rtc_notify;
+	struct scmi_msg_imx_bbm_button_notify *button_notify;
+
+	if (message_id == IMX_BBM_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) {
+		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 = SCMI_IMX_BBM_EVENT_RTC_ID(le32_to_cpu(p->flags));
+		r->rtc_evt = le32_to_cpu(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(1);
+		return NULL;
+	}
+
+	return r;
+}
+
+static int scmi_imx_bbm_get_num_sources(const struct scmi_protocol_handle *ph)
+{
+	return 1;
+}
+
+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 = {
+	.get_num_sources = scmi_imx_bbm_get_num_sources,
+	.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),
+};
+
+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, SCMI_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 = cpu_to_le32(sec & 0xffffffff);
+	cfg->value_high = cpu_to_le32(sec >> 32);
+
+	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, SCMI_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, SCMI_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 = cpu_to_le32(sec & 0xffffffff);
+	cfg->value_high = cpu_to_le32(sec >> 32);
+
+	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, SCMI_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_nxp_protocol.h b/include/linux/scmi_nxp_protocol.h
new file mode 100644
index 000000000000..a2077e1ef5d8
--- /dev/null
+++ b/include/linux/scmi_nxp_protocol.h
@@ -0,0 +1,50 @@
+/* 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,
+};
+
+#define SCMI_IMX_BBM_RTC_TIME_SET	0x6
+#define SCMI_IMX_BBM_RTC_TIME_GET	0x7
+#define SCMI_IMX_BBM_RTC_ALARM_SET	0x8
+#define SCMI_IMX_BBM_BUTTON_GET		0x9
+
+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

Re: [PATCH 2/5] firmware: arm_scmi: add initial support for i.MX BBM protocol

[Cristian Marussi]

On Fri, Feb 02, 2024 at 02:34:40PM +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 | 381 +++++++++++++++++++++++++++++++++
>  include/linux/scmi_nxp_protocol.h      |  50 +++++
>  4 files changed, 442 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..0e12aaeabc42
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/imx-sm-bbm.c
> @@ -0,0 +1,381 @@
> +// 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_nxp_protocol.h>
> +
> +#include "protocols.h"
> +#include "notify.h"
> +
> +#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x0

Supported version 0 is a bit awkward...unless you fw returns version 0 too
for this protocol version, it will cause an attempted protocol version
negotitation (which is optionally supported on the fw side) and a splat of
warnings from the Kernel trying to use an older version (0) with a newer
platform version...it will carry-on anyway, just be aware of this.

> +
> +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 BBM_PROTO_ATTR_NUM_RTC(x)  (((x) & 0xFFU) << 16U)
> +#define BBM_PROTO_ATTR_NUM_GPR(x)  (((x) & 0xFFFFU) << 0U)

Are these 2 used anywhere ? (use bitfield & GENMASK anyway if needed)

> +
> +#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_ID(x)        (((x) >> 24) & 0xFF)

bitfield.h ... but more on this down below...

> +
> +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;
> +	struct scmi_msg_imx_bbm_rtc_notify *rtc_notify;
> +	struct scmi_msg_imx_bbm_button_notify *button_notify;

You can move these 2, each one inside its if blocks, right ?

> +
> +	if (message_id == IMX_BBM_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) {
> +		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 = SCMI_IMX_BBM_EVENT_RTC_ID(le32_to_cpu(p->flags));

mmm... are you sure you want to handle endianity on a bitfield before
extracting the ID ?

looking at SCMI_IMX_BBM_EVENT_RTC_ID above , cannot this just be something like

	le32_get_bits(p->flags, GENMASK(31, 24))

> +		r->rtc_evt = le32_to_cpu(p->flags) & SCMI_IMX_BBM_NOTIFY_RTC_FLAG;

Same here...I dont thing endianity-conversion play on a flag bitfield is
right...dont you just need bit 0,1,2 here to extract ? .... maybe wrong
since I can only guess what all the payoad fields represents...

> +		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(1);

...this can be heavy on receveing malformed notif... WARN_ON_ONCE() ?

> +		return NULL;
> +	}
> +
> +	return r;
> +}
> +
> +static int scmi_imx_bbm_get_num_sources(const struct scmi_protocol_handle *ph)
> +{
> +	return 1;
> +}

You can drop this...see down below.

> +
> +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 = {
> +	.get_num_sources = scmi_imx_bbm_get_num_sources,

if the number of sources is statically defined, you dont need this
helper and just set .num_sources=1 down below...as in
	
	drivers/firmware/arm_scmi/system.c

> +	.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, SCMI_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 = cpu_to_le32(sec & 0xffffffff);
> +	cfg->value_high = cpu_to_le32(sec >> 32);
> +
lower_32_bits() / upper_32_bits .. it also handles casting

> +	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, SCMI_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, SCMI_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 = cpu_to_le32(sec & 0xffffffff);
> +	cfg->value_high = cpu_to_le32(sec >> 32);

lower_32_bits() / upper_32_bits
> +
> +	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, SCMI_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,

...as said elsewhere you will *probably* need to add here:

	.vendor_id = "NXP_vendor_id"

or whatever is your 16-bytes NULL terminated vendor_id as exposed by
your platform SCMI fw in response the BASE_DISCOVER_VENDOR

and optionally a .sub_vendor_id / impl_ver (up to you)

.. *probably* because the mechanism has just been posted and
is under review

> +};
> +
> +module_scmi_protocol(scmi_imx_bbm);
> diff --git a/include/linux/scmi_nxp_protocol.h b/include/linux/scmi_nxp_protocol.h
> new file mode 100644
> index 000000000000..a2077e1ef5d8
> --- /dev/null
> +++ b/include/linux/scmi_nxp_protocol.h
> @@ -0,0 +1,50 @@
> +/* 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,
> +};
> +
> +#define SCMI_IMX_BBM_RTC_TIME_SET	0x6
> +#define SCMI_IMX_BBM_RTC_TIME_GET	0x7
> +#define SCMI_IMX_BBM_RTC_ALARM_SET	0x8
> +#define SCMI_IMX_BBM_BUTTON_GET		0x9
> +

Why these protocol internal commands are exposed here too ? they are
already defined above in the file containing the protocol implemenation
(rightly so)

> +struct scmi_imx_bbm_notif_report {
> +	bool			is_rtc;
> +	bool			is_button;
> +	ktime_t			timestamp;
> +	unsigned int		rtc_id;
> +	unsigned int		rtc_evt;
> +};
> +#endif
> 


All in all, beside the minor remarks above, LGTM.

Main open thing which remains (on our side) is the handling of vendor
protocol module by the core (as said)...I will chase Sudeep for an
opinion..also about the layout in the source code of all these new
upcoming vendor modules...maybe a common subdir ?

Thank,
Cristian

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

Re: [PATCH 2/5] firmware: arm_scmi: add initial support for i.MX BBM protocol

[Sudeep Holla]

On Fri, Feb 02, 2024 at 02:34:40PM +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.

BBM as in NAND Bad Block Management ?

As mentioned elsewhere for other vendor protocol implementations,
please provide as much documentation on these extensions as possible.
Please use the SCMI spec as the reference to provide documentation
as I expected it to be versioned and all these needs to be documented
for maintenance of upstream support.

Also explain briefly why this needs to be vendor extension even if it is
obvious. I prefer that way.

-- 
Regards,
Sudeep

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

[PATCH 3/5] 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 | 289 ++++++++++++++++++++++++++++++++
 include/linux/scmi_nxp_protocol.h       |  17 ++
 4 files changed, 317 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..7805d41cca4d
--- /dev/null
+++ b/drivers/firmware/arm_scmi/imx-sm-misc.c
@@ -0,0 +1,289 @@
+// 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_nxp_protocol.h>
+
+#include "protocols.h"
+#include "notify.h"
+
+#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x0
+
+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_ctrl;
+	u32 nr_reason;
+};
+
+struct scmi_msg_imx_misc_protocol_attributes {
+	__le32 attributes;
+};
+
+#define GET_REASONS_NR(x)	le32_get_bits((x), GENMASK(23, 16))
+#define GET_CTRLS_NR(x)		le32_get_bits((x), GENMASK(15, 0))
+
+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[MISC_MAX_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_ctrl = GET_CTRLS_NR(attr->attributes);
+		mi->nr_reason = GET_REASONS_NR(attr->attributes);
+		dev_info(ph->dev, "i.MX MISC NUM CTRL: %d, NUM Reason: %d\n",
+			 mi->nr_ctrl, mi->nr_reason);
+	}
+
+	ph->xops->xfer_put(ph, t);
+
+	return ret;
+}
+
+static int scmi_imx_misc_ctrl_notify(const struct scmi_protocol_handle *ph,
+				     u32 ctrl_id, u32 flags)
+{
+	struct scmi_imx_misc_info *mi = ph->get_priv(ph);
+	struct scmi_imx_misc_ctrl_notify_in *in;
+	struct scmi_xfer *t;
+	int ret;
+
+	if (ctrl_id >= mi->nr_ctrl)
+		return -EINVAL;
+
+	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)
+{
+	struct scmi_imx_misc_info *mi = ph->get_priv(ph);
+
+	return mi->nr_ctrl;
+}
+
+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;
+	if (src_id)
+		*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_info *mi = ph->get_priv(ph);
+	struct scmi_imx_misc_ctrl_get_out *out;
+	struct scmi_xfer *t;
+	int ret, i;
+
+	if (ctrl_id >= mi->nr_ctrl)
+		return -EINVAL;
+
+	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_GET, sizeof(u32),
+				      sizeof(*out), &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++)
+			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_info *mi = ph->get_priv(ph);
+	struct scmi_imx_misc_ctrl_set_in *in;
+	struct scmi_xfer *t;
+	int ret, i;
+
+	if (ctrl_id >= mi->nr_ctrl)
+		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,
+};
+
+module_scmi_protocol(scmi_imx_misc);
diff --git a/include/linux/scmi_nxp_protocol.h b/include/linux/scmi_nxp_protocol.h
index a2077e1ef5d8..45415a6ff845 100644
--- a/include/linux/scmi_nxp_protocol.h
+++ b/include/linux/scmi_nxp_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 {
@@ -47,4 +53,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 3/5] firmware: arm_scmi: add initial support for i.MX MISC protocol

[Cristian Marussi]

On Fri, Feb 02, 2024 at 02:34:41PM +0800, 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.
> 

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-misc.c | 289 ++++++++++++++++++++++++++++++++
>  include/linux/scmi_nxp_protocol.h       |  17 ++
>  4 files changed, 317 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..7805d41cca4d
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/imx-sm-misc.c
> @@ -0,0 +1,289 @@
> +// 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_nxp_protocol.h>
> +
> +#include "protocols.h"
> +#include "notify.h"
> +
> +#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x0
> +

Same considerations on this 0-versioning as in previous patch.

> +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_ctrl;
> +	u32 nr_reason;
> +};
> +
> +struct scmi_msg_imx_misc_protocol_attributes {
> +	__le32 attributes;
> +};
> +
> +#define GET_REASONS_NR(x)	le32_get_bits((x), GENMASK(23, 16))
> +#define GET_CTRLS_NR(x)		le32_get_bits((x), GENMASK(15, 0))
> +

Please lineup this tabs

> +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[MISC_MAX_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_ctrl = GET_CTRLS_NR(attr->attributes);
> +		mi->nr_reason = GET_REASONS_NR(attr->attributes);
> +		dev_info(ph->dev, "i.MX MISC NUM CTRL: %d, NUM Reason: %d\n",
> +			 mi->nr_ctrl, mi->nr_reason);
> +	}
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}
> +
> +static int scmi_imx_misc_ctrl_notify(const struct scmi_protocol_handle *ph,
> +				     u32 ctrl_id, u32 flags)
> +{
> +	struct scmi_imx_misc_info *mi = ph->get_priv(ph);
> +	struct scmi_imx_misc_ctrl_notify_in *in;
> +	struct scmi_xfer *t;
> +	int ret;
> +
> +	if (ctrl_id >= mi->nr_ctrl)
> +		return -EINVAL;
> +
> +	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);

so this is an evt_id I see below...maybe calling this param or event
instead of flags is less misleading....I would not expect to see endian
handling on a bitfield (and flags suggests just that to me...

> +
> +	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) {

No need for braces here. Please run checkpatch --strict on your next
series.

> +		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)
> +{
> +	struct scmi_imx_misc_info *mi = ph->get_priv(ph);
> +
> +	return mi->nr_ctrl;
> +}
> +
> +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;

Here you DO need instead endian-helpers when you access the payload p->
A static analyzer like smatch will have spotted this.

> +	if (src_id)

src_id param is assured to be non-NULL here...not neeed to check...as
you did not check in BBM module indeed...

> +		*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)
> +{

Does *num here contains val[] max size on input and number of elements
filled into val[] on output ? I assume so down below

> +	struct scmi_imx_misc_info *mi = ph->get_priv(ph);
> +	struct scmi_imx_misc_ctrl_get_out *out;
> +	struct scmi_xfer *t;
> +	int ret, i;
> +
> +	if (ctrl_id >= mi->nr_ctrl)
> +		return -EINVAL;
> +
> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_GET, sizeof(u32),
> +				      sizeof(*out), &t);

This *out size you require is really capped by the max transport size...so this
could fail depending on the transport...just be aware of this...not saying is bad
by itself...an alternative would be to define value inside the struct as 

  __le32 *value;

and here ask for an rx_size of 0 instead of sizeof(*out) so that you get
max transport payload size....then you will have to check if the
returned payload is as long as expected by the caller though...up 2 you really

> +	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);

out->num MUST be smaller than the original *num passed as input to fit
into *val...please check this before overwriting *num and bail out on overflow...

...moreover...out->val[] size is statically defined as MISC_MAX_VAL and
if you get here you know that you have enough space in t->rx.buf BUT you will
have also to check that the returned out->num is < MISC_MAX_VAL to avoid any
possible overflow while reading the payload if the value provided by fw
is bogus...

> +		for (i = 0; i < *num; 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_info *mi = ph->get_priv(ph);
> +	struct scmi_imx_misc_ctrl_set_in *in;
> +	struct scmi_xfer *t;
> +	int ret, i;
> +
> +	if (ctrl_id >= mi->nr_ctrl)
> +		return -EINVAL;
> +
> +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_SET, sizeof(*in),
> +				      0, &t);

Similarly as above here you could fail if the requestes size for TX is
bigger than allowed by the transport..

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

and num must be checked anyway to be < MISC_MAX_VAL before looping in->value

> +		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,
> +};
> +
> +module_scmi_protocol(scmi_imx_misc);
> diff --git a/include/linux/scmi_nxp_protocol.h b/include/linux/scmi_nxp_protocol.h
> index a2077e1ef5d8..45415a6ff845 100644
> --- a/include/linux/scmi_nxp_protocol.h
> +++ b/include/linux/scmi_nxp_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)
> +

All of these payload size calculation does NOT consider the fact that
the underlying buffer inside scmi_xfer are really dependent on
transport max_size...so as said above all the xfer_get_init could fail
depending on the size of the configured transport...just a heads
up...fine for me.

>  enum scmi_nxp_protocol {
>  	SCMI_PROTOCOL_IMX_BBM = 0x81,
> +	SCMI_PROTOCOL_IMX_MISC = 0x84,
>  };
>  
>  struct scmi_imx_bbm_proto_ops {
> @@ -47,4 +53,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);
> +};

Please add some Doc commenting describing these protocol_ops as in any
proto_ops found in scmi_protocol.h

(same for the other BBM Vendor protocol ops in the previous patch that I
missed in the other review.

Thanks,
Cristian

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

[PATCH 4/5] 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..c5bc571881c7
--- /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_nxp_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

Re: [PATCH 4/5] firmware: imx: support BBM module

[kernel test robot]

Hi Peng,

kernel test robot noticed the following build errors:

[auto build test ERROR on 51b70ff55ed88edd19b080a524063446bcc34b62]

url:    https://github.com/intel-lab-lkp/linux/commits/Peng-Fan-OSS/dt-bindings-firmware-add-i-MX-SCMI-Extension-protocol/20240202-143439
base:   51b70ff55ed88edd19b080a524063446bcc34b62
patch link:    https://lore.kernel.org/r/20240202-imx95-bbm-misc-v1-4-3cb743020933%40nxp.com
patch subject: [PATCH 4/5] firmware: imx: support BBM module
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20240204/202402041140.l2qPp6Gn-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240204/202402041140.l2qPp6Gn-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402041140.l2qPp6Gn-lkp@intel.com/

All errors (new ones prefixed by >>):

   s390-linux-ld: drivers/firmware/imx/sm-bbm.o: in function `scmi_imx_bbm_probe':
>> sm-bbm.c:(.text+0xbdc): undefined reference to `devm_rtc_allocate_device'
>> s390-linux-ld: sm-bbm.c:(.text+0xc90): undefined reference to `__devm_rtc_register_device'
   s390-linux-ld: drivers/firmware/imx/sm-bbm.o: in function `scmi_imx_bbm_notifier':
>> sm-bbm.c:(.text+0xeb6): undefined reference to `rtc_update_irq'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

Re: [PATCH 4/5] firmware: imx: support BBM module

[Cristian Marussi]

On Fri, Feb 02, 2024 at 02:34:42PM +0800, Peng Fan (OSS) wrote:
> 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.

Hi,

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

So you have not added a Kconfig for this but you just rely on the SCMI NXP BBM
Vendor module to be configured....this causes the kernel-bot build failure because
I suppose that the RTC subsystem is not compiled in since its dependency is not
stated anywhere...

you could/should add a Kconfig here for this driver with a select on
CONFIG_IMX_SCMI_BBM_EXT and the RTC subsystem and put the

	default y if ARCH_MXC

instead of placing that on CONFIG_IMX_SCMI_BBM_EXT

> diff --git a/drivers/firmware/imx/sm-bbm.c b/drivers/firmware/imx/sm-bbm.c
> new file mode 100644
> index 000000000000..c5bc571881c7
> --- /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_nxp_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);

You convert to tm and return success anyway on SCMI get error ?

> +
> +	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 Success on error to set ?

> +	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);
> +

Same pattern error--> success...I suppose is fine at this point but maybe
explain why in a comment....

> +	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);

there is a to_delayed_work() in workqueue.h to get the delayed work from
work and then in turn get to bbnsm...just to avoid relying on
delayed_work internal naming...

> +	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) {

So this bbnsm->suspended is checked here from inside the SCMI notifier and it
is set by a couple of pm_ops you provide down below which are called by
the core PM subsys, so is it not high likely that you could have issues with the
order of such reads/writes ? 

Would it be worth to add a READ_ONCE here and WRITE_ONCE in the
pm_ops...or I am overthinking ?

> +		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);

So you are registering for another SCMI event but you want to use the
same callback and notifier_bock to handle different events, BUT internally
the SCMI core creates a DISTINCT kernel regular notification chain for each event
and each resource (or one for ALL resources of an event) against which a
devm_event_notifier_register() has been called AND so, being a notification_chain
the notifier_blocks that you provide MUST be distinct, because the notification
chain is indeed a simply-linked list and so when you register bbnsm->nb the second
time you will indeed add the nb to another list at the same....

...thing which I suppose could work in your case since you have only nb/callback
per event BUT as soon as you (or someone else) will try to register another nb 
for these same events the 2 notification chains will start melting together....

...and it will be a hell to debug...

so IOW...even if it works now for you, please use 2 distinct nb_pwr. nb_rtc
notifier blocks with 2 distinct callbacks (to be able to use container_of in
them) to register to 2 distinct events...you can register for multiple sources
using only one nb BUT you cannot register for multiple events using the same
nb/callback as of now.

With this internal design the queues and the worker threads dispatching these
notifs are dedicated to a single event and possible to a single event/resource...
...no event ever queues behind any other...

This probably would need better clarification in the SCMI docs, my bad, and
maybe a new option to register for ANY event the same nb (like you can do with
src_id if you dont specify one), IF you are fine with the possibility that your
events notification will be serialized in a single queue.

> +
> +	if (ret)
> +		dev_err(dev, "Failed to register BBM Button Events %d:", ret);
> +

So why not failing if you could NOT register the notifications ?

> +	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);

As said, this will get mixed up when pwrkey_init tries to register again the same nb
for a different event...

> +}
> +
> +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);

sizeof(*bbnsm)

> +	if (!bbnsm)
> +		return -ENOMEM;
> +
> +	bbnsm->ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_IMX_BBM, &ph);

proto ops can be global really..since are always the same pointer even
if this is probed mutiple times... this could be

	bbnsm_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_IMX_BBM, &bbnsm->ph);

with bbnsm_ops static global to this file

> +	if (IS_ERR(bbnsm->ops))
> +		return PTR_ERR(bbnsm->ops);
> +
> +	bbnsm->ph = ph;
> +
> +	device_init_wakeup(dev, true);

Not fully familiar with this, but it seems to me that when this is
issued some wakeup related sysfs entries are created too...so I suppose
you want to disable this back on failure to have those entries removed.

or maybe just move this right before the final return 0....but I am not
sure if you want to have it called BEFORE the pwrkey notifier if
registered or AFTER is fine too...potentially loosing some wakeup, though.

> +
> +	dev_set_drvdata(dev, bbnsm);
> +
> +	ret = scmi_imx_bbm_rtc_init(sdev);
> +	if (ret) {
> +		dev_err(dev, "rtc init failed: %d\n", ret);

like ??
		device_init_wakeup(dev, false);

> +		return ret;
> +	}
> +
> +	ret = scmi_imx_bbm_pwrkey_init(sdev);
> +	if (ret) {
> +		dev_err(dev, "pwr init failed: %d\n", ret);
and... 
		device_init_wakeup(dev, false);

> +		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);
> +

Thanks,
Cristian

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

RE: [PATCH 4/5] firmware: imx: support BBM module

[Peng Fan]

Hi Cristian,

> Subject: Re: [PATCH 4/5] firmware: imx: support BBM module
> 
....

> > +msecs_to_jiffies(DEBOUNCE_TIME));
> > +
> > +	/*
> > +	 * Directly report key event after resume to make no key press
> > +	 * event is missed.
> > +	 */
> > +	if (bbnsm->suspended) {
> 
> So this bbnsm->suspended is checked here from inside the SCMI
> notifier and it is set by a couple of pm_ops you provide down below
> which are called by the core PM subsys, so is it not high likely that you
> could have issues with the order of such reads/writes ?
> 
> Would it be worth to add a READ_ONCE here and WRITE_ONCE in the
> pm_ops...or I am overthinking ?

Just checked other input drivers, only two use READ_ONCE.
It might be good to avoid potential issues with READ/WRITE_ONCE.

Other comments will be addressed. BTW,
I will split rtc and pwr key into two drivers and put
them in input and rtc directory.

Thanks,
Peng

> 
> > +		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);
> 
> So you are registering for another SCMI event but you want to use the
> same callback and notifier_bock to handle different events, BUT
> internally the SCMI core creates a DISTINCT kernel regular notification
> chain for each event and each resource (or one for ALL resources of an
> event) against which a
> devm_event_notifier_register() has been called AND so, being a
> notification_chain the notifier_blocks that you provide MUST be
> distinct, because the notification chain is indeed a simply-linked list
> and so when you register bbnsm->nb the second time you will indeed
> add the nb to another list at the same....
> 
> ...thing which I suppose could work in your case since you have only
> nb/callback per event BUT as soon as you (or someone else) will try to
> register another nb for these same events the 2 notification chains will
> start melting together....
> 
> ...and it will be a hell to debug...
> 
> so IOW...even if it works now for you, please use 2 distinct nb_pwr.
> nb_rtc notifier blocks with 2 distinct callbacks (to be able to use
> container_of in
> them) to register to 2 distinct events...you can register for multiple
> sources using only one nb BUT you cannot register for multiple events
> using the same nb/callback as of now.
> 
> With this internal design the queues and the worker threads
> dispatching these notifs are dedicated to a single event and possible to
> a single event/resource...
> ...no event ever queues behind any other...
> 
> This probably would need better clarification in the SCMI docs, my bad,
> and maybe a new option to register for ANY event the same nb (like
> you can do with src_id if you dont specify one), IF you are fine with the
> possibility that your events notification will be serialized in a single
> queue.
> 
> > +
> > +	if (ret)
> > +		dev_err(dev, "Failed to register BBM Button
> Events %d:", ret);
> > +
> 
> So why not failing if you could NOT register the notifications ?
> 
> > +	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);
> 
> As said, this will get mixed up when pwrkey_init tries to register again
> the same nb for a different event...
> 
> > +}
> > +
> > +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);
> 
> sizeof(*bbnsm)
> 
> > +	if (!bbnsm)
> > +		return -ENOMEM;
> > +
> > +	bbnsm->ops = handle->devm_protocol_get(sdev,
> SCMI_PROTOCOL_IMX_BBM,
> > +&ph);
> 
> proto ops can be global really..since are always the same pointer even
> if this is probed mutiple times... this could be
> 
> 	bbnsm_ops = handle->devm_protocol_get(sdev,
> SCMI_PROTOCOL_IMX_BBM, &bbnsm->ph);
> 
> with bbnsm_ops static global to this file
> 
> > +	if (IS_ERR(bbnsm->ops))
> > +		return PTR_ERR(bbnsm->ops);
> > +
> > +	bbnsm->ph = ph;
> > +
> > +	device_init_wakeup(dev, true);
> 
> Not fully familiar with this, but it seems to me that when this is issued
> some wakeup related sysfs entries are created too...so I suppose you
> want to disable this back on failure to have those entries removed.
> 
> or maybe just move this right before the final return 0....but I am not
> sure if you want to have it called BEFORE the pwrkey notifier if
> registered or AFTER is fine too...potentially loosing some wakeup,
> though.
> 
> > +
> > +	dev_set_drvdata(dev, bbnsm);
> > +
> > +	ret = scmi_imx_bbm_rtc_init(sdev);
> > +	if (ret) {
> > +		dev_err(dev, "rtc init failed: %d\n", ret);
> 
> like ??
> 		device_init_wakeup(dev, false);
> 
> > +		return ret;
> > +	}
> > +
> > +	ret = scmi_imx_bbm_pwrkey_init(sdev);
> > +	if (ret) {
> > +		dev_err(dev, "pwr init failed: %d\n", ret);
> and...
> 		device_init_wakeup(dev, false);
> 
> > +		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);
> > +
> 
> Thanks,
> Cristian

Re: [PATCH 4/5] firmware: imx: support BBM module

[Cristian Marussi]

On Fri, Feb 02, 2024 at 02:34:42PM +0800, Peng Fan (OSS) wrote:
> 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.
> 

Hi some further remarks questin about pwrkey down below.

> 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(+)
> 
 [snip]

> +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");
> +	}

This linux,code binding prop is searched in the SCMI device node, BUT
your BB< protocol binding does NOT mention it at all.

> +
> +	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;
> +}

I suppose you cannot use std SystemPower protocol and scmi_power_control
existent upstream driver because you are configuring the event keycode that
is associated with your button press event using linux,code DT properies
looked up above, right ? (which you need to define somewhere as said
above..)

I was thinking that maybe handling events associated with generic button-presses
could be done via some std SCMI protocols like PINCTRL/GPIO (IF IT HAD NOTIFICATIONS)
and some custom SCMI gpio-keys driver in the future (not now clearly :D)...thoughts ?

Thanks,
Cristian

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

[PATCH 5/5] 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..4410e69d256b
--- /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_nxp_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 5/5] firmware: imx: add i.MX95 MISC driver

[Cristian Marussi]

On Fri, Feb 02, 2024 at 02:34:43PM +0800, Peng Fan (OSS) wrote:
> 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.
> 

Hi,

> 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

Same considerations about missing Kconfig as in BBM and implicit
dependency on the NXP MISC vendor module...this way also you cannot even
NOT compile this module when the Vendor protocol is compiled in for,
say, testing purposes...

> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> new file mode 100644
> index 000000000000..4410e69d256b
> --- /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_nxp_protocol.h>
> +
> +static const struct scmi_imx_misc_proto_ops *imx_misc_ctrl_ops;
> +static struct scmi_protocol_handle *ph;

This global does NOT sound right...if there are multiple SCMI instances
defined in the DT this can be probed multiple times, and the MISC
protocol will be initialized multuple times, each instance will have
its distinct protocol_handle *ph...so store it somewhere like you did in
the BBM driver

> +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);
> +

Ok, now I suppose that you want to be sure to run just one instance if
this driver...

> +static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
> +				       unsigned long event, void *data)
> +{
> +	return 0;
> +}

What is the point of this ?

> +
> +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);

...when probed more than once this will lead to the same global nb registered on 2
different notification chains....

> +		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);
> +

All in all, I suppose the main thing to reason about this driver is if you
want/plan to allow for multiple instances of it to be loaded/probed on the same
running system or not...

If you think that this driver HAS STRICTLY to be probed once, and having
2 DT protocol nodes for MISC it is certainly an error, we will have to
add some mechianism in the SCMI core to be able to mark this as single
instance and refuse to create more than one device for this protocol...a
sort of generalization of what is done in a custom way by the core for
SYSTEM_POWER, since we dont want to have multiple sources of shutdown
events...

Alternnatively you will have to make this survive multiple
probes...keeping track of multiple *ph for each probe() and handling
that in your EXPORTED funcs...thing that seems awkard a bit...but you
only know how this is an will be used.

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

Thanks,
Cristian

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

Re: [PATCH 5/5] firmware: imx: add i.MX95 MISC driver

[Cristian Marussi]

On Fri, Feb 23, 2024 at 06:35:26PM +0000, Cristian Marussi wrote:
> On Fri, Feb 02, 2024 at 02:34:43PM +0800, Peng Fan (OSS) wrote:
> > 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.
> > 
> 
> Hi,
> 
> > 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
> 
> Same considerations about missing Kconfig as in BBM and implicit
> dependency on the NXP MISC vendor module...this way also you cannot even
> NOT compile this module when the Vendor protocol is compiled in for,
> say, testing purposes...
> 
> > diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> > new file mode 100644
> > index 000000000000..4410e69d256b
> > --- /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_nxp_protocol.h>
> > +
> > +static const struct scmi_imx_misc_proto_ops *imx_misc_ctrl_ops;
> > +static struct scmi_protocol_handle *ph;
> 
> This global does NOT sound right...if there are multiple SCMI instances
> defined in the DT this can be probed multiple times, and the MISC
> protocol will be initialized multuple times, each instance will have
> its distinct protocol_handle *ph...so store it somewhere like you did in
> the BBM driver
> 
> > +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);
> > +
> 
> Ok, now I suppose that you want to be sure to run just one instance if
> this driver...
> 
> > +static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
> > +				       unsigned long event, void *data)
> > +{
> > +	return 0;
> > +}
> 
> What is the point of this ?
> 
> > +
> > +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);
> 
> ...when probed more than once this will lead to the same global nb registered on 2
> different notification chains....
> 
> > +		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);
> > +
> 
> All in all, I suppose the main thing to reason about this driver is if you
> want/plan to allow for multiple instances of it to be loaded/probed on the same
> running system or not...
> 
> If you think that this driver HAS STRICTLY to be probed once, and having
> 2 DT protocol nodes for MISC it is certainly an error, we will have to
> add some mechianism in the SCMI core to be able to mark this as single
> instance and refuse to create more than one device for this protocol...a
> sort of generalization of what is done in a custom way by the core for
> SYSTEM_POWER, since we dont want to have multiple sources of shutdown
> events...

An easier solution would be of course for this driver to just check if
any global ph was already retrieved on a previous probe and just bail
out, but I want to have a chat with Sudeep about this approach.

Thanks,
Cristian

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