[PATCH v2 0/4] Introduce SCMI Quirks framework

[PATCH v2 0/4] Introduce SCMI Quirks framework

[Cristian Marussi]

Hi,

with the increasing adoption of SCMI across arm64 ecosystems, we have to
start considering how to deal and take care of out-of-spec SCMI firmware
platforms that are actively deployed in the wild, in a consistent manner.

This small series introduces a simple framework, based on static_keys,
that allows a user to:

- define a quirk and its matching conditions; quirks can match based on:

  	compatible / Vendor_ID / Sub_Vendor_ID / [Min_Vers, Max_Vers]

  from the longest matching sequence down to the shortest.

  When the SCMI core stack boots it will enable the matching quirks
  depending on the information gathered from the platform via Base
  protocol: any NULL match condition is ignored during matching and is
  interpreted as ANY, so you can decide to match on a very specific
  combination of compatibles and FW versions OR simply on a compatible.

- define a quirk code-block: simply a block of code, meant to play the
  magic quirk trick, defined in the proximity of where it will be used
  and gated by an implicit quirk static-key associated with the defined
  quirk

Patch 1/4 in the series is really unrelated to the Quirk framework
itself: it is a slight variation on a fix posted previously by Sibi around
PERF FastChannels and it is included here for simplicity, since the example
quirk provided later in this series has to be applied exactly where 1/3
applies its modifications.

Patch 2/4 introduces support for SCMI quirks: support is default-y in
Kconfig as of this series. All the quirks found defined are stored in
an hashtable at module initialization time.
Since V1 the quirks are matched on ranges of ImplementationVersion if
provided.
Since V2 the matching condition based on compatible, it is optionally
represented by a list of possible compatibles.

Later on, when the SCMI core stack probes and it has retrieved basic info
via Base protocol, all the matching quirks are enabled, which simply means
the related underlying specific quirks static-keys are enabled.

Patch 3/4 Convert a pre-existing quirk to use the Quirk framework: it is,
though, a peculiar quirk, since it was meant to match any firmware version
or from any vendor/sub_vendor.

Patch 4/4 introduces an example Quirk for a known problem on a known
platform as reported by Johan, Marc and Sibi, BUT note that the matching
condition in tha patch MUST be properly completed in the patch (I was not
sure what to use...see my comments)

The macro-salad still triggers a lot of checkpatch warns.

Any feedback and testing is very much welcome.

Thanks,
Cristian

---
V1 -> V2
- added support for matching against an optional list of compatibles
- compile the quirks even if Quirks framework is not enabled
- reduced CLOCK_DESCRIBE_RATES triplet Quirk size
- added more docs & examples

RFC -> V1
 - collected tags for patch 1/4
 - Added version ranges handling for Implementation Version
 - make Quirk frmwk default-y
 - add COMPILE_TEST depend
 - move quirk enabling logic out of Base protocol init to avoid possible
   deadlocks with cpu_lock (reported LOCKDEP splat from Johan)
 - fix Quirk matching conditions for quirk_perf_level_get_fc_force


Cristian Marussi (3):
  firmware: arm_scmi: Add Quirks framework
  firmware: arm_scmi: quirk: Fix CLOCK_DESCRIBE_RATES triplet
  [NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in
    attributes

Sibi Sankar (1):
  firmware: arm_scmi: Ensure that the message-id supports fastchannel

 drivers/firmware/arm_scmi/Kconfig     |  13 ++
 drivers/firmware/arm_scmi/Makefile    |   1 +
 drivers/firmware/arm_scmi/clock.c     |  33 +--
 drivers/firmware/arm_scmi/driver.c    | 111 ++++++---
 drivers/firmware/arm_scmi/protocols.h |   2 +
 drivers/firmware/arm_scmi/quirks.c    | 321 ++++++++++++++++++++++++++
 drivers/firmware/arm_scmi/quirks.h    |  52 +++++
 7 files changed, 486 insertions(+), 47 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/quirks.c
 create mode 100644 drivers/firmware/arm_scmi/quirks.h

-- 
2.47.0

[PATCH v2 1/4] firmware: arm_scmi: Ensure that the message-id supports fastchannel

[Cristian Marussi]

From: Sibi Sankar <quic_sibis@quicinc.com>

Currently the perf and powercap protocol relies on the protocol domain
attributes, which just ensures that one fastchannel per domain, before
instantiating fastchannels for all possible message-ids. Fix this by
ensuring that each message-id supports fastchannel before initialization.

Logs:
scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:0] - ret:-95. Using regular messaging.
scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:1] - ret:-95. Using regular messaging.
scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:2] - ret:-95. Using regular messaging.

CC: stable@vger.kernel.org
Reported-by: Johan Hovold <johan+linaro@kernel.org>
Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
Fixes: 6f9ea4dabd2d ("firmware: arm_scmi: Generalize the fast channel support")
Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
Tested-by: Johan Hovold <johan+linaro@kernel.org>
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
[Cristian: Modified the condition checked to establish support or not]
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
RFC -> V1
 - picked up a few tags

Since PROTOCOL_MESSAGE_ATTRIBUTES, used to check if message_id is supported,
is a mandatory command, it cannot fail so we must bail-out NOT only if FC was
not supported for that command but also if the query fails as a whole; so the
condition checked for bailing out is modified to:

	if (ret || !MSG_SUPPORTS_FASTCHANNEL(attributes)) {
---
 drivers/firmware/arm_scmi/driver.c    | 76 +++++++++++++++------------
 drivers/firmware/arm_scmi/protocols.h |  2 +
 2 files changed, 45 insertions(+), 33 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 1cf18cc8e63f..0e281fca0a38 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1738,6 +1738,39 @@ static int scmi_common_get_max_msg_size(const struct scmi_protocol_handle *ph)
 	return info->desc->max_msg_size;
 }
 
+/**
+ * scmi_protocol_msg_check  - Check protocol message attributes
+ *
+ * @ph: A reference to the protocol handle.
+ * @message_id: The ID of the message to check.
+ * @attributes: A parameter to optionally return the retrieved message
+ *		attributes, in case of Success.
+ *
+ * An helper to check protocol message attributes for a specific protocol
+ * and message pair.
+ *
+ * Return: 0 on SUCCESS
+ */
+static int scmi_protocol_msg_check(const struct scmi_protocol_handle *ph,
+				   u32 message_id, u32 *attributes)
+{
+	int ret;
+	struct scmi_xfer *t;
+
+	ret = xfer_get_init(ph, PROTOCOL_MESSAGE_ATTRIBUTES,
+			    sizeof(__le32), 0, &t);
+	if (ret)
+		return ret;
+
+	put_unaligned_le32(message_id, t->tx.buf);
+	ret = do_xfer(ph, t);
+	if (!ret && attributes)
+		*attributes = get_unaligned_le32(t->rx.buf);
+	xfer_put(ph, t);
+
+	return ret;
+}
+
 /**
  * struct scmi_iterator  - Iterator descriptor
  * @msg: A reference to the message TX buffer; filled by @prepare_message with
@@ -1879,6 +1912,7 @@ scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
 	int ret;
 	u32 flags;
 	u64 phys_addr;
+	u32 attributes;
 	u8 size;
 	void __iomem *addr;
 	struct scmi_xfer *t;
@@ -1887,6 +1921,15 @@ scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
 	struct scmi_msg_resp_desc_fc *resp;
 	const struct scmi_protocol_instance *pi = ph_to_pi(ph);
 
+	/* Check if the MSG_ID supports fastchannel */
+	ret = scmi_protocol_msg_check(ph, message_id, &attributes);
+	if (ret || !MSG_SUPPORTS_FASTCHANNEL(attributes)) {
+		dev_dbg(ph->dev,
+			"Skip FC init for 0x%02X/%d  domain:%d - ret:%d\n",
+			pi->proto->id, message_id, domain, ret);
+		return;
+	}
+
 	if (!p_addr) {
 		ret = -EINVAL;
 		goto err_out;
@@ -2004,39 +2047,6 @@ static void scmi_common_fastchannel_db_ring(struct scmi_fc_db_info *db)
 		SCMI_PROTO_FC_RING_DB(64);
 }
 
-/**
- * scmi_protocol_msg_check  - Check protocol message attributes
- *
- * @ph: A reference to the protocol handle.
- * @message_id: The ID of the message to check.
- * @attributes: A parameter to optionally return the retrieved message
- *		attributes, in case of Success.
- *
- * An helper to check protocol message attributes for a specific protocol
- * and message pair.
- *
- * Return: 0 on SUCCESS
- */
-static int scmi_protocol_msg_check(const struct scmi_protocol_handle *ph,
-				   u32 message_id, u32 *attributes)
-{
-	int ret;
-	struct scmi_xfer *t;
-
-	ret = xfer_get_init(ph, PROTOCOL_MESSAGE_ATTRIBUTES,
-			    sizeof(__le32), 0, &t);
-	if (ret)
-		return ret;
-
-	put_unaligned_le32(message_id, t->tx.buf);
-	ret = do_xfer(ph, t);
-	if (!ret && attributes)
-		*attributes = get_unaligned_le32(t->rx.buf);
-	xfer_put(ph, t);
-
-	return ret;
-}
-
 static const struct scmi_proto_helpers_ops helpers_ops = {
 	.extended_name_get = scmi_common_extended_name_get,
 	.get_max_msg_size = scmi_common_get_max_msg_size,
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index aaee57cdcd55..d62c4469d1fd 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -31,6 +31,8 @@
 
 #define SCMI_PROTOCOL_VENDOR_BASE	0x80
 
+#define MSG_SUPPORTS_FASTCHANNEL(x)	((x) & BIT(0))
+
 enum scmi_common_cmd {
 	PROTOCOL_VERSION = 0x0,
 	PROTOCOL_ATTRIBUTES = 0x1,
-- 
2.47.0

[PATCH v2 2/4] firmware: arm_scmi: Add Quirks framework

[Cristian Marussi]

Add a common framework to describe SCMI quirks and associate them with a
specific platform or a specific set of SCMI firmware versions.

All the matching SCMI quirks will be enabled when the SCMI core stack
probes and after all the needed SCMI firmware versioning information was
retrieved using Base protocol.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
V1 -> V2
- compile quirk snippets even when SCMI Quirks are disabled
- added support for quirks matches on multiple compatibles
  (via NULl terminated compats strings)
- removed stale unneeded include
- added some more docs

RFC->V1
- added handling of impl_ver ranges in quirk definition
- make Quirks Framework default-y
- match on all NULL/0 too..these are quirks that apply always anywhere !
- depends on COMPILE_TEST too
- move quirk enable calling logic out of Base protocol init (triggers a
  LOCKDEP issues around cpu locks (cpufreq, static_branch_enable interactions..)
---
 drivers/firmware/arm_scmi/Kconfig  |  13 ++
 drivers/firmware/arm_scmi/Makefile |   1 +
 drivers/firmware/arm_scmi/driver.c |  27 ++-
 drivers/firmware/arm_scmi/quirks.c | 316 +++++++++++++++++++++++++++++
 drivers/firmware/arm_scmi/quirks.h |  48 +++++
 5 files changed, 404 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/arm_scmi/quirks.c
 create mode 100644 drivers/firmware/arm_scmi/quirks.h

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index dabd874641d0..e3fb36825978 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -69,6 +69,19 @@ config ARM_SCMI_DEBUG_COUNTERS
 	  such useful debug counters. This can be helpful for debugging and
 	  SCMI monitoring.
 
+config ARM_SCMI_QUIRKS
+	bool "Enable SCMI Quirks framework"
+	depends on JUMP_LABEL || COMPILE_TEST
+	default y
+	help
+	  Enables support for SCMI Quirks framework to workaround SCMI platform
+	  firmware bugs on system already deployed in the wild.
+
+	  The framework allows the definition of platform-specific code quirks
+	  that will be associated and enabled only on the desired platforms
+	  depending on the SCMI firmware advertised versions and/or machine
+	  compatibles.
+
 source "drivers/firmware/arm_scmi/transports/Kconfig"
 source "drivers/firmware/arm_scmi/vendors/imx/Kconfig"
 
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index 9ac81adff567..780cd62b2f78 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -3,6 +3,7 @@ scmi-bus-y = bus.o
 scmi-core-objs := $(scmi-bus-y)
 
 scmi-driver-y = driver.o notify.o
+scmi-driver-$(CONFIG_ARM_SCMI_QUIRKS) += quirks.o
 scmi-driver-$(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT) += raw_mode.o
 scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o
 scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 0e281fca0a38..ffaa68cdf644 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -11,7 +11,7 @@
  * various power domain DVFS including the core/cluster, certain system
  * clocks configuration, thermal sensors and many others.
  *
- * Copyright (C) 2018-2024 ARM Ltd.
+ * Copyright (C) 2018-2025 ARM Ltd.
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -38,6 +38,7 @@
 
 #include "common.h"
 #include "notify.h"
+#include "quirks.h"
 
 #include "raw_mode.h"
 
@@ -3112,6 +3113,26 @@ static const struct scmi_desc *scmi_transport_setup(struct device *dev)
 	return &trans->desc;
 }
 
+static void scmi_enable_matching_quirks(struct scmi_info *info)
+{
+	struct scmi_revision_info *rev = &info->version;
+	const char *compatible = NULL;
+	struct device_node *root;
+
+	root = of_find_node_by_path("/");
+	if (root) {
+		of_property_read_string(root, "compatible", &compatible);
+		of_node_put(root);
+	}
+
+	dev_dbg(info->dev, "Looking for quirks matching: %s/%s/%s/0x%08X\n",
+		compatible, rev->vendor_id, rev->sub_vendor_id, rev->impl_ver);
+
+	/* Enable applicable quirks */
+	scmi_quirks_enable(info->dev, rev->vendor_id,
+			   rev->sub_vendor_id, rev->impl_ver);
+}
+
 static int scmi_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -3233,6 +3254,8 @@ static int scmi_probe(struct platform_device *pdev)
 	list_add_tail(&info->node, &scmi_list);
 	mutex_unlock(&scmi_list_mutex);
 
+	scmi_enable_matching_quirks(info);
+
 	for_each_available_child_of_node(np, child) {
 		u32 prot_id;
 
@@ -3391,6 +3414,8 @@ static struct dentry *scmi_debugfs_init(void)
 
 static int __init scmi_driver_init(void)
 {
+	scmi_quirks_initialize();
+
 	/* Bail out if no SCMI transport was configured */
 	if (WARN_ON(!IS_ENABLED(CONFIG_ARM_SCMI_HAVE_TRANSPORT)))
 		return -EINVAL;
diff --git a/drivers/firmware/arm_scmi/quirks.c b/drivers/firmware/arm_scmi/quirks.c
new file mode 100644
index 000000000000..9e93a8242182
--- /dev/null
+++ b/drivers/firmware/arm_scmi/quirks.c
@@ -0,0 +1,316 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Message Protocol Quirks
+ *
+ * Copyright (C) 2025 ARM Ltd.
+ */
+
+/**
+ * DOC: Theory of operation
+ *
+ * A framework to define SCMI quirks and their activation conditions based on
+ * existing static_keys Kernel facilities.
+ *
+ * Quirks are named and their activation conditions defined using the macro
+ * DEFINE_SCMI_QUIRK() in this file.
+ *
+ * After a quirk is defined, a corresponding entry must also be added to the
+ * global @scmi_quirks_table in this file using __DECLARE_SCMI_QUIRK_ENTRY().
+ *
+ * Additionally a corresponding quirk declaration must be added also to the
+ * quirk.h file using DECLARE_SCMI_QUIRK().
+ *
+ * The needed quirk code-snippet itself will be defined local to the SCMI code
+ * that is meant to fix and will be associated to the previously defined quirk
+ * and related activation conditions using the macro SCMI_QUIRK().
+ *
+ * At runtime, during the SCMI stack probe sequence, once the SCMI Server had
+ * advertised the running platform Vendor, SubVendor and Implementation Version
+ * data, all the defined quirks matching the activation conditions will be
+ * enabled.
+ *
+ * Example
+ *
+ * quirk.c
+ * -------
+ *  DEFINE_SCMI_QUIRK(fix_me, "vendor", "subvend", "0x12000-0x30000",
+ *		      "someone,plat_A", "another,plat_b", "vend,sku");
+ *
+ *  static struct scmi_quirk *scmi_quirks_table[] = {
+ *	...
+ *	__DECLARE_SCMI_QUIRK_ENTRY(fix_me),
+ *	NULL
+ *  };
+ *
+ * quirk.h
+ * -------
+ *  DECLARE_SCMI_QUIRK(fix_me);
+ *
+ * <somewhere_in_the_scmi_stack.c>
+ * ------------------------------
+ *
+ *  #define QUIRK_CODE_SNIPPET()		\
+ *  ({						\
+ *	if (p->condition)			\
+ *		a_ptr->calculated_val = 123;	\
+ *  })
+ *
+ *
+ *  int some_function_to_fix(int param, struct something *p)
+ *  {
+ *	struct some_strut *a_ptr;
+ *
+ *	a_ptr = some_load_func(p);
+ *	SCMI_QUIRK(fix_me, QUIRK_CODE_SNIPPET_FIX_ME);
+ *	some_more_func(a_ptr);
+ *	...
+ *
+ *	return 0;
+ *  }
+ *
+ */
+
+#include <linux/ctype.h>
+#include <linux/device.h>
+#include <linux/export.h>
+#include <linux/hashtable.h>
+#include <linux/kstrtox.h>
+#include <linux/of.h>
+#include <linux/static_key.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+#include "quirks.h"
+
+#define SCMI_QUIRKS_HT_SZ	4
+
+struct scmi_quirk {
+	bool enabled;
+	const char *name;
+	char *vendor;
+	char *sub_vendor_id;
+	char *impl_ver_range;
+	u32 start_range;
+	u32 end_range;
+	struct static_key_false *key;
+	struct hlist_node hash;
+	unsigned int hkey;
+	const char *const compats[];
+};
+
+#define __DEFINE_SCMI_QUIRK_ENTRY(_qn, _ven, _sub, _impl, ...)	\
+	static struct scmi_quirk scmi_quirk_entry_ ## _qn = {		\
+		.name = __stringify(quirk_ ## _qn),			\
+		.vendor = _ven,						\
+		.sub_vendor_id = _sub,					\
+		.impl_ver_range = _impl,				\
+		.key = &(scmi_quirk_ ## _qn),				\
+		.compats = { __VA_ARGS__ __VA_OPT__(,) NULL },		\
+	}
+
+#define __DECLARE_SCMI_QUIRK_ENTRY(_qn)		(&(scmi_quirk_entry_ ## _qn))
+
+/*
+ * Define a quirk by name and provide the matching tokens where:
+ *
+ *  _qn: A string which will be used to build the quirk and the global
+ *	 static_key names.
+ *  _ven : SCMI Vendor ID string match, NULL means any.
+ *  _sub : SCMI SubVendor ID string match, NULL means any.
+ *  _impl : SCMI Implementation Version string match, NULL means any.
+ *          This string can be used to express version ranges which will be
+ *          interpreted as follows:
+ *
+ *			NULL		[0, 0xFFFFFFFF]
+ *			"X"		[X, X]
+ *			"X-"		[X, 0xFFFFFFFF]
+ *			"-X"		[0, X]
+ *			"X-Y"		[X, Y]
+ *
+ *          with X <= Y and <v> in [X, Y] meaning X <= <v> <= Y
+ *
+ *  ... : An optional variadic macros argument used to provide a coma-separated
+ *	  list of compatible strings matches; when no variadic argument is
+ *	  provided, ANY compatible will match this quirk.
+ *
+ *  This implicitly define also a properly named global static-key that
+ *  will be used to dynamically enable the quirk at initialization time.
+ *
+ *  Note that it is possible to associate multiple quirks to the same
+ *  matching pattern, if your firmware quality is really astounding :P
+ *
+ * Example:
+ *
+ * Compatibles list NOT provided, so ANY compatible will match:
+ *
+ *  DEFINE_SCMI_QUIRK(my_new_issue, "Vend", "SVend", "0x12000-0x30000");
+ *
+ *
+ * A few compatibles provided to match against:
+ *
+ *  DEFINE_SCMI_QUIRK(my_new_issue, "Vend", "SVend", "0x12000-0x30000",
+ *		      "xvend,plat_a", "xvend,plat_b", "xvend,sku_name");
+ */
+#define DEFINE_SCMI_QUIRK(_qn, _ven, _sub, _impl, ...)			\
+	DEFINE_STATIC_KEY_FALSE(scmi_quirk_ ## _qn);			\
+	__DEFINE_SCMI_QUIRK_ENTRY(_qn, _ven, _sub, _impl, ##__VA_ARGS__)
+
+/*
+ * Same as DEFINE_SCMI_QUIRK but EXPORTED: this is meant to address quirks
+ * that possibly reside in code that is included in loadable kernel modules
+ * that needs to be able to access the global static keys at runtime to
+ * determine if enabled or not. (see SCMI_QUIRK to understand usage)
+ */
+#define DEFINE_SCMI_QUIRK_EXPORTED(_qn, _ven, _sub, _impl, ...)		\
+	DEFINE_STATIC_KEY_FALSE(scmi_quirk_ ## _qn);			\
+	EXPORT_SYMBOL_GPL(scmi_quirk_ ## _qn);				\
+	__DEFINE_SCMI_QUIRK_ENTRY(_qn, _ven, _sub, _impl, ##__VA_ARGS__)
+
+/* Global Quirks Definitions */
+
+/*
+ * Quirks Pointers Array
+ *
+ * This is filled at compile-time with the list of pointers to all the currently
+ * defined quirks descriptors.
+ */
+static struct scmi_quirk *scmi_quirks_table[] = {
+	NULL
+};
+
+/*
+ * Quirks HashTable
+ *
+ * A run-time populated hashtable containing all the defined quirks descriptors
+ * hashed by matching pattern.
+ */
+static DEFINE_READ_MOSTLY_HASHTABLE(scmi_quirks_ht, SCMI_QUIRKS_HT_SZ);
+
+static unsigned int scmi_quirk_signature(const char *vend, const char *sub_vend)
+{
+	char *signature, *p;
+	unsigned int hash32;
+	unsigned long hash = 0;
+
+	/* vendor_id/sub_vendor_id guaranteed <= SCMI_SHORT_NAME_MAX_SIZE */
+	signature = kasprintf(GFP_KERNEL, "|%s|%s|", vend ?: "", sub_vend ?: "");
+	if (!signature)
+		return 0;
+
+	pr_debug("SCMI Quirk Signature >>>%s<<<\n", signature);
+
+	p = signature;
+	while (*p)
+		hash = partial_name_hash(tolower(*p++), hash);
+	hash32 = end_name_hash(hash);
+
+	kfree(signature);
+
+	return hash32;
+}
+
+static int scmi_quirk_range_parse(struct scmi_quirk *quirk)
+{
+	const char *last, *first = quirk->impl_ver_range;
+	size_t len;
+	char *sep;
+	int ret;
+
+	quirk->start_range = 0;
+	quirk->end_range = 0xFFFFFFFF;
+	len = quirk->impl_ver_range ? strlen(quirk->impl_ver_range) : 0;
+	if (!len)
+		return 0;
+
+	last = first + len - 1;
+	sep = strchr(quirk->impl_ver_range, '-');
+	if (sep)
+		*sep = '\0';
+
+	if (sep == first) /* -X */
+		ret = kstrtouint(first + 1, 0, &quirk->end_range);
+	else /* X OR X- OR X-y */
+		ret = kstrtouint(first, 0, &quirk->start_range);
+	if (ret)
+		return ret;
+
+	if (!sep)
+		quirk->end_range = quirk->start_range;
+	else if (sep != last) /* x-Y */
+		ret = kstrtouint(sep + 1, 0, &quirk->end_range);
+
+	if (quirk->start_range > quirk->end_range)
+		return -EINVAL;
+
+	return ret;
+}
+
+void scmi_quirks_initialize(void)
+{
+	struct scmi_quirk *quirk;
+	int i;
+
+	for (i = 0, quirk = scmi_quirks_table[0]; quirk;
+	     i++, quirk = scmi_quirks_table[i]) {
+		int ret;
+
+		ret = scmi_quirk_range_parse(quirk);
+		if (ret) {
+			pr_err("SCMI skip QUIRK [%s] - BAD RANGE - |%s|\n",
+			       quirk->name, quirk->impl_ver_range);
+			continue;
+		}
+		quirk->hkey = scmi_quirk_signature(quirk->vendor,
+						   quirk->sub_vendor_id);
+
+		hash_add(scmi_quirks_ht, &quirk->hash, quirk->hkey);
+
+		pr_debug("Registered SCMI QUIRK [%s] -- %p - Key [0x%08X] - %s/%s/[0x%08X-0x%08X]\n",
+			 quirk->name, quirk, quirk->hkey,
+			 quirk->vendor, quirk->sub_vendor_id,
+			 quirk->start_range, quirk->end_range);
+	}
+
+	pr_debug("SCMI Quirks initialized\n");
+}
+
+void scmi_quirks_enable(struct device *dev, const char *vend,
+			const char *subv, const u32 impl)
+{
+	for (int i = 3; i >= 0; i--) {
+		struct scmi_quirk *quirk;
+		unsigned int hkey;
+
+		hkey = scmi_quirk_signature(i > 1 ? vend : NULL,
+					    i > 2 ? subv : NULL);
+
+		/*
+		 * Note that there could be multiple matches so we
+		 * will enable multiple quirk part of an hash collision
+		 * domain...BUT we cannot assume that ALL quirks on the
+		 * same collision domain are a full match.
+		 */
+		hash_for_each_possible(scmi_quirks_ht, quirk, hash, hkey) {
+			if (quirk->enabled || quirk->hkey != hkey ||
+			    impl < quirk->start_range ||
+			    impl > quirk->end_range)
+				continue;
+
+			if (quirk->compats[0] &&
+			    !of_machine_compatible_match(quirk->compats))
+				continue;
+
+			dev_info(dev, "Enabling SCMI Quirk [%s]\n",
+				 quirk->name);
+
+			dev_dbg(dev,
+				"Quirk matched on: %s/%s/%s/[0x%08X-0x%08X]\n",
+				quirk->compats[0], quirk->vendor,
+				quirk->sub_vendor_id,
+				quirk->start_range, quirk->end_range);
+
+			static_branch_enable(quirk->key);
+			quirk->enabled = true;
+		}
+	}
+}
diff --git a/drivers/firmware/arm_scmi/quirks.h b/drivers/firmware/arm_scmi/quirks.h
new file mode 100644
index 000000000000..28829b4f0646
--- /dev/null
+++ b/drivers/firmware/arm_scmi/quirks.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * System Control and Management Interface (SCMI) Message Protocol Quirks
+ *
+ * Copyright (C) 2025 ARM Ltd.
+ */
+#ifndef _SCMI_QUIRKS_H
+#define _SCMI_QUIRKS_H
+
+#include <linux/static_key.h>
+#include <linux/types.h>
+
+#ifdef CONFIG_ARM_SCMI_QUIRKS
+
+#define DECLARE_SCMI_QUIRK(_qn)						\
+	DECLARE_STATIC_KEY_FALSE(scmi_quirk_ ## _qn)
+
+/*
+ * A helper to associate the actual code snippet to use as a quirk
+ * named as _qn.
+ */
+#define SCMI_QUIRK(_qn, _blk)						\
+	do {								\
+		if (static_branch_unlikely(&(scmi_quirk_ ## _qn)))	\
+			(_blk);						\
+	} while (0)
+
+void scmi_quirks_initialize(void);
+void scmi_quirks_enable(struct device *dev, const char *vend,
+			const char *subv, const u32 impl);
+
+#else
+
+#define DECLARE_SCMI_QUIRK(_qn)
+/* Force quirks compilation even when SCMI Quirks are disabled */
+#define SCMI_QUIRK(_qn, _blk)						\
+	do {								\
+		if (0)							\
+			(_blk);						\
+	} while (0)
+
+static inline void scmi_quirks_initialize(void) { }
+static inline void scmi_quirks_enable(struct device *dev, const char *vend,
+				      const char *sub_vend, const u32 impl) { }
+
+#endif /* CONFIG_ARM_SCMI_QUIRKS */
+
+#endif /* _SCMI_QUIRKS_H */
-- 
2.47.0

[PATCH v2 3/4] firmware: arm_scmi: quirk: Fix CLOCK_DESCRIBE_RATES triplet

[Cristian Marussi]

Convert an existing quirk in CLOCK_DESCRIBE_RATES parsing to the new quirk
framework. This is a sort of a peculiar quirk since it matches any platform
and any firmware.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
V1 -> V2
- reduced the quirk size by placing the warn outside
- using new compatibles conditions
---
 drivers/firmware/arm_scmi/clock.c  | 33 ++++++++++++++++++------------
 drivers/firmware/arm_scmi/quirks.c |  2 ++
 drivers/firmware/arm_scmi/quirks.h |  3 +++
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 2ed2279388f0..afa7981efe82 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -11,6 +11,7 @@
 
 #include "protocols.h"
 #include "notify.h"
+#include "quirks.h"
 
 /* Updated only after ALL the mandatory features for that version are merged */
 #define SCMI_PROTOCOL_SUPPORTED_VERSION		0x30000
@@ -429,6 +430,23 @@ static void iter_clk_describe_prepare_message(void *message,
 	msg->rate_index = cpu_to_le32(desc_index);
 }
 
+#define QUIRK_OUT_OF_SPEC_TRIPLET					       \
+	({								       \
+		/*							       \
+		 * A known quirk: a triplet is returned but num_returned != 3  \
+		 * Check for a safe payload size and fix.		       \
+		 */							       \
+		if (st->num_returned != 3 && st->num_remaining == 0 &&	       \
+		    st->rx_len == sizeof(*r) + sizeof(__le32) * 2 * 3) {       \
+			st->num_returned = 3;				       \
+			st->num_remaining = 0;				       \
+		} else {						       \
+			dev_err(p->dev,					       \
+				"Cannot fix out-of-spec reply !\n");	       \
+			return -EPROTO;					       \
+		}							       \
+	})
+
 static int
 iter_clk_describe_update_state(struct scmi_iterator_state *st,
 			       const void *response, void *priv)
@@ -450,19 +468,8 @@ iter_clk_describe_update_state(struct scmi_iterator_state *st,
 			 p->clk->name, st->num_returned, st->num_remaining,
 			 st->rx_len);
 
-		/*
-		 * A known quirk: a triplet is returned but num_returned != 3
-		 * Check for a safe payload size and fix.
-		 */
-		if (st->num_returned != 3 && st->num_remaining == 0 &&
-		    st->rx_len == sizeof(*r) + sizeof(__le32) * 2 * 3) {
-			st->num_returned = 3;
-			st->num_remaining = 0;
-		} else {
-			dev_err(p->dev,
-				"Cannot fix out-of-spec reply !\n");
-			return -EPROTO;
-		}
+		SCMI_QUIRK(clock_rates_triplet_out_of_spec,
+			   QUIRK_OUT_OF_SPEC_TRIPLET);
 	}
 
 	return 0;
diff --git a/drivers/firmware/arm_scmi/quirks.c b/drivers/firmware/arm_scmi/quirks.c
index 9e93a8242182..120ac933ed2e 100644
--- a/drivers/firmware/arm_scmi/quirks.c
+++ b/drivers/firmware/arm_scmi/quirks.c
@@ -167,6 +167,7 @@ struct scmi_quirk {
 	__DEFINE_SCMI_QUIRK_ENTRY(_qn, _ven, _sub, _impl, ##__VA_ARGS__)
 
 /* Global Quirks Definitions */
+DEFINE_SCMI_QUIRK(clock_rates_triplet_out_of_spec, NULL, NULL, NULL);
 
 /*
  * Quirks Pointers Array
@@ -175,6 +176,7 @@ struct scmi_quirk {
  * defined quirks descriptors.
  */
 static struct scmi_quirk *scmi_quirks_table[] = {
+	__DECLARE_SCMI_QUIRK_ENTRY(clock_rates_triplet_out_of_spec),
 	NULL
 };
 
diff --git a/drivers/firmware/arm_scmi/quirks.h b/drivers/firmware/arm_scmi/quirks.h
index 28829b4f0646..7fdc496c94c7 100644
--- a/drivers/firmware/arm_scmi/quirks.h
+++ b/drivers/firmware/arm_scmi/quirks.h
@@ -45,4 +45,7 @@ static inline void scmi_quirks_enable(struct device *dev, const char *vend,
 
 #endif /* CONFIG_ARM_SCMI_QUIRKS */
 
+/* Quirk delarations */
+DECLARE_SCMI_QUIRK(clock_rates_triplet_out_of_spec);
+
 #endif /* _SCMI_QUIRKS_H */
-- 
2.47.0

[PATCH v2 4/4] [NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in attributes

[Cristian Marussi]

Some platform misreported the support of FastChannel when queried: ignore
that bit on selected platforms.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
Match features has to be set-up properly before upstreaming this.
Ideally the out-of-spec firmware should be matched with a quirk matching
pattern based on Vendor/SubVendor/ImplVersion....but it is NOT clear if the
platform at hand will ship with future fixed firmwares where the ImplVersion
field is properly handled.
If we cannot be sure about that, we should fallback to a compatible match.

v1 -> v2
- use multiple compats quirks syntax

RFC->V1
- fix QUIRKS conditions
---
 drivers/firmware/arm_scmi/driver.c | 8 ++++++++
 drivers/firmware/arm_scmi/quirks.c | 3 +++
 drivers/firmware/arm_scmi/quirks.h | 1 +
 3 files changed, 12 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index ffaa68cdf644..3b363bda2b1e 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1904,6 +1904,13 @@ struct scmi_msg_resp_desc_fc {
 	__le32 db_preserve_hmask;
 };
 
+#define QUIRK_PERF_FC_FORCE						\
+	({								\
+		if (pi->proto->id == SCMI_PROTOCOL_PERF &&		\
+		    message_id == 0x8 /* PERF_LEVEL_GET */)		\
+			attributes |= BIT(0);				\
+	})
+
 static void
 scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
 			     u8 describe_id, u32 message_id, u32 valid_size,
@@ -1924,6 +1931,7 @@ scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
 
 	/* Check if the MSG_ID supports fastchannel */
 	ret = scmi_protocol_msg_check(ph, message_id, &attributes);
+	SCMI_QUIRK(perf_level_get_fc_force, QUIRK_PERF_FC_FORCE);
 	if (ret || !MSG_SUPPORTS_FASTCHANNEL(attributes)) {
 		dev_dbg(ph->dev,
 			"Skip FC init for 0x%02X/%d  domain:%d - ret:%d\n",
diff --git a/drivers/firmware/arm_scmi/quirks.c b/drivers/firmware/arm_scmi/quirks.c
index 120ac933ed2e..64a5809da20b 100644
--- a/drivers/firmware/arm_scmi/quirks.c
+++ b/drivers/firmware/arm_scmi/quirks.c
@@ -168,6 +168,8 @@ struct scmi_quirk {
 
 /* Global Quirks Definitions */
 DEFINE_SCMI_QUIRK(clock_rates_triplet_out_of_spec, NULL, NULL, NULL);
+DEFINE_SCMI_QUIRK(perf_level_get_fc_force,
+		  "bad-vend", NULL, "0x20000-", "bad-compat", "bad-compat-2");
 
 /*
  * Quirks Pointers Array
@@ -177,6 +179,7 @@ DEFINE_SCMI_QUIRK(clock_rates_triplet_out_of_spec, NULL, NULL, NULL);
  */
 static struct scmi_quirk *scmi_quirks_table[] = {
 	__DECLARE_SCMI_QUIRK_ENTRY(clock_rates_triplet_out_of_spec),
+	__DECLARE_SCMI_QUIRK_ENTRY(perf_level_get_fc_force),
 	NULL
 };
 
diff --git a/drivers/firmware/arm_scmi/quirks.h b/drivers/firmware/arm_scmi/quirks.h
index 7fdc496c94c7..a71fde85a527 100644
--- a/drivers/firmware/arm_scmi/quirks.h
+++ b/drivers/firmware/arm_scmi/quirks.h
@@ -47,5 +47,6 @@ static inline void scmi_quirks_enable(struct device *dev, const char *vend,
 
 /* Quirk delarations */
 DECLARE_SCMI_QUIRK(clock_rates_triplet_out_of_spec);
+DECLARE_SCMI_QUIRK(perf_level_get_fc_force);
 
 #endif /* _SCMI_QUIRKS_H */
-- 
2.47.0

Re: [PATCH v2 2/4] firmware: arm_scmi: Add Quirks framework

[Johan Hovold]

On Fri, Apr 25, 2025 at 01:52:48PM +0100, Cristian Marussi wrote:
> Add a common framework to describe SCMI quirks and associate them with a
> specific platform or a specific set of SCMI firmware versions.
> 
> All the matching SCMI quirks will be enabled when the SCMI core stack
> probes and after all the needed SCMI firmware versioning information was
> retrieved using Base protocol.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> V1 -> V2
> - compile quirk snippets even when SCMI Quirks are disabled
> - added support for quirks matches on multiple compatibles
>   (via NULl terminated compats strings)
> - removed stale unneeded include
> - added some more docs

> +static void scmi_enable_matching_quirks(struct scmi_info *info)
> +{
> +	struct scmi_revision_info *rev = &info->version;
> +	const char *compatible = NULL;
> +	struct device_node *root;
> +
> +	root = of_find_node_by_path("/");
> +	if (root) {
> +		of_property_read_string(root, "compatible", &compatible);
> +		of_node_put(root);
> +	}
> +
> +	dev_dbg(info->dev, "Looking for quirks matching: %s/%s/%s/0x%08X\n",
> +		compatible, rev->vendor_id, rev->sub_vendor_id, rev->impl_ver);

You're now just looking up the most specific compatible string in order
to include it in this dev_dbg(). Since you're now matching on all
compatible strings, perhaps you can consider just dropping it.

> +
> +	/* Enable applicable quirks */
> +	scmi_quirks_enable(info->dev, rev->vendor_id,
> +			   rev->sub_vendor_id, rev->impl_ver);
> +}

> +/**
> + * DOC: Theory of operation
> + *
> + * A framework to define SCMI quirks and their activation conditions based on
> + * existing static_keys Kernel facilities.

nit: kernel

> +/*
> + * Define a quirk by name and provide the matching tokens where:
> + *
> + *  _qn: A string which will be used to build the quirk and the global
> + *	 static_key names.
> + *  _ven : SCMI Vendor ID string match, NULL means any.
> + *  _sub : SCMI SubVendor ID string match, NULL means any.
> + *  _impl : SCMI Implementation Version string match, NULL means any.
> + *          This string can be used to express version ranges which will be
> + *          interpreted as follows:
> + *
> + *			NULL		[0, 0xFFFFFFFF]
> + *			"X"		[X, X]
> + *			"X-"		[X, 0xFFFFFFFF]
> + *			"-X"		[0, X]
> + *			"X-Y"		[X, Y]
> + *
> + *          with X <= Y and <v> in [X, Y] meaning X <= <v> <= Y
> + *
> + *  ... : An optional variadic macros argument used to provide a coma-separated

comma

> + *	  list of compatible strings matches; when no variadic argument is
> + *	  provided, ANY compatible will match this quirk.

> +void scmi_quirks_enable(struct device *dev, const char *vend,
> +			const char *subv, const u32 impl)
> +{
> +	for (int i = 3; i >= 0; i--) {
> +		struct scmi_quirk *quirk;
> +		unsigned int hkey;
> +
> +		hkey = scmi_quirk_signature(i > 1 ? vend : NULL,
> +					    i > 2 ? subv : NULL);
> +
> +		/*
> +		 * Note that there could be multiple matches so we
> +		 * will enable multiple quirk part of an hash collision

nit: "quirks that are part of a"?

> +		 * domain...BUT we cannot assume that ALL quirks on the
> +		 * same collision domain are a full match.
> +		 */
> +		hash_for_each_possible(scmi_quirks_ht, quirk, hash, hkey) {
> +			if (quirk->enabled || quirk->hkey != hkey ||
> +			    impl < quirk->start_range ||
> +			    impl > quirk->end_range)
> +				continue;
> +
> +			if (quirk->compats[0] &&
> +			    !of_machine_compatible_match(quirk->compats))
> +				continue;
> +
> +			dev_info(dev, "Enabling SCMI Quirk [%s]\n",
> +				 quirk->name);
> +
> +			dev_dbg(dev,
> +				"Quirk matched on: %s/%s/%s/[0x%08X-0x%08X]\n",
> +				quirk->compats[0], quirk->vendor,

You can now match on more than one compats string, but I guess printing
just the first one is fine.

> +				quirk->sub_vendor_id,
> +				quirk->start_range, quirk->end_range);
> +
> +			static_branch_enable(quirk->key);
> +			quirk->enabled = true;
> +		}
> +	}
> +}

This seems to work as intended and I've tried matching on machine and
SoC compatible strings and/or vendor and protocol version:

Tested-by: Johan Hovold <johan+linaro@kernel.org>

Johan

Re: [PATCH v2 4/4] [NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in attributes

[Johan Hovold]

On Fri, Apr 25, 2025 at 01:52:50PM +0100, Cristian Marussi wrote:
> Some platform misreported the support of FastChannel when queried: ignore
> that bit on selected platforms.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> Match features has to be set-up properly before upstreaming this.
> Ideally the out-of-spec firmware should be matched with a quirk matching
> pattern based on Vendor/SubVendor/ImplVersion....but it is NOT clear if the
> platform at hand will ship with future fixed firmwares where the ImplVersion
> field is properly handled.
> If we cannot be sure about that, we should fallback to a compatible match.
> 
> v1 -> v2
> - use multiple compats quirks syntax
> 
> RFC->V1
> - fix QUIRKS conditions

>  /* Global Quirks Definitions */
>  DEFINE_SCMI_QUIRK(clock_rates_triplet_out_of_spec, NULL, NULL, NULL);
> +DEFINE_SCMI_QUIRK(perf_level_get_fc_force,
> +		  "bad-vend", NULL, "0x20000-", "bad-compat", "bad-compat-2");

Still works when matching on vendor and version (and/or machine or SoC
compatible):

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
Tested-by: Johan Hovold <johan+linaro@kernel.org>

I think we can go ahead and merge this based on vendor and version
"0x20000-".

Depending on what Sibi finds out, or if it turns out to be needed, we
can always add an upper version bound later.

Johan

Re: [PATCH v2 2/4] firmware: arm_scmi: Add Quirks framework

[Cristian Marussi]

On Sun, Apr 27, 2025 at 04:38:48PM +0200, Johan Hovold wrote:
> On Fri, Apr 25, 2025 at 01:52:48PM +0100, Cristian Marussi wrote:
> > Add a common framework to describe SCMI quirks and associate them with a
> > specific platform or a specific set of SCMI firmware versions.
> > 
> > All the matching SCMI quirks will be enabled when the SCMI core stack
> > probes and after all the needed SCMI firmware versioning information was
> > retrieved using Base protocol.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > V1 -> V2
> > - compile quirk snippets even when SCMI Quirks are disabled
> > - added support for quirks matches on multiple compatibles
> >   (via NULl terminated compats strings)
> > - removed stale unneeded include
> > - added some more docs
> 
> > +static void scmi_enable_matching_quirks(struct scmi_info *info)
> > +{
> > +	struct scmi_revision_info *rev = &info->version;
> > +	const char *compatible = NULL;
> > +	struct device_node *root;
> > +
> > +	root = of_find_node_by_path("/");
> > +	if (root) {
> > +		of_property_read_string(root, "compatible", &compatible);
> > +		of_node_put(root);
> > +	}
> > +
> > +	dev_dbg(info->dev, "Looking for quirks matching: %s/%s/%s/0x%08X\n",
> > +		compatible, rev->vendor_id, rev->sub_vendor_id, rev->impl_ver);
> 
> You're now just looking up the most specific compatible string in order
> to include it in this dev_dbg(). Since you're now matching on all
> compatible strings, perhaps you can consider just dropping it.
> 

Yes indeed I was not sure to keep all of this machinery just to print
the machine compatible that is used to try to match against the
(possible) list of compats....on the other side seemed useful to know
exactly what you are trying to match against....but maybe we can simply
assume that the machine compatible is well-known....

> > +
> > +	/* Enable applicable quirks */
> > +	scmi_quirks_enable(info->dev, rev->vendor_id,
> > +			   rev->sub_vendor_id, rev->impl_ver);
> > +}
> 
> > +/**
> > + * DOC: Theory of operation
> > + *
> > + * A framework to define SCMI quirks and their activation conditions based on
> > + * existing static_keys Kernel facilities.
> 
> nit: kernel
>

Yep.
 
> > +/*
> > + * Define a quirk by name and provide the matching tokens where:
> > + *
> > + *  _qn: A string which will be used to build the quirk and the global
> > + *	 static_key names.
> > + *  _ven : SCMI Vendor ID string match, NULL means any.
> > + *  _sub : SCMI SubVendor ID string match, NULL means any.
> > + *  _impl : SCMI Implementation Version string match, NULL means any.
> > + *          This string can be used to express version ranges which will be
> > + *          interpreted as follows:
> > + *
> > + *			NULL		[0, 0xFFFFFFFF]
> > + *			"X"		[X, X]
> > + *			"X-"		[X, 0xFFFFFFFF]
> > + *			"-X"		[0, X]
> > + *			"X-Y"		[X, Y]
> > + *
> > + *          with X <= Y and <v> in [X, Y] meaning X <= <v> <= Y
> > + *
> > + *  ... : An optional variadic macros argument used to provide a coma-separated
> 
> comma
> 

...ah

> > + *	  list of compatible strings matches; when no variadic argument is
> > + *	  provided, ANY compatible will match this quirk.
> 
> > +void scmi_quirks_enable(struct device *dev, const char *vend,
> > +			const char *subv, const u32 impl)
> > +{
> > +	for (int i = 3; i >= 0; i--) {
> > +		struct scmi_quirk *quirk;
> > +		unsigned int hkey;
> > +
> > +		hkey = scmi_quirk_signature(i > 1 ? vend : NULL,
> > +					    i > 2 ? subv : NULL);
> > +
> > +		/*
> > +		 * Note that there could be multiple matches so we
> > +		 * will enable multiple quirk part of an hash collision
> 
> nit: "quirks that are part of a"?
> 

mmm...as a non-native and poor English speaker I am, though, reasonably
confident that a/an is chosen based on the vowel/consonant SOUND of the
next word NOT on the effective letter...am I wrong ?
(then we could digress about which is the sound of a[n] 'hash' :P ...)

> > +		 * domain...BUT we cannot assume that ALL quirks on the
> > +		 * same collision domain are a full match.
> > +		 */
> > +		hash_for_each_possible(scmi_quirks_ht, quirk, hash, hkey) {
> > +			if (quirk->enabled || quirk->hkey != hkey ||
> > +			    impl < quirk->start_range ||
> > +			    impl > quirk->end_range)
> > +				continue;
> > +
> > +			if (quirk->compats[0] &&
> > +			    !of_machine_compatible_match(quirk->compats))
> > +				continue;
> > +
> > +			dev_info(dev, "Enabling SCMI Quirk [%s]\n",
> > +				 quirk->name);
> > +
> > +			dev_dbg(dev,
> > +				"Quirk matched on: %s/%s/%s/[0x%08X-0x%08X]\n",
> > +				quirk->compats[0], quirk->vendor,
> 
> You can now match on more than one compats string, but I guess printing
> just the first one is fine.
> 

Yes, same as above dev_dbg...not sure if it was meaningful really to
dump all the list and overload the log with all such info...

> > +				quirk->sub_vendor_id,
> > +				quirk->start_range, quirk->end_range);
> > +
> > +			static_branch_enable(quirk->key);
> > +			quirk->enabled = true;
> > +		}
> > +	}
> > +}
> 
> This seems to work as intended and I've tried matching on machine and
> SoC compatible strings and/or vendor and protocol version:
> 
> Tested-by: Johan Hovold <johan+linaro@kernel.org>
> 

Thanks for the review Johan.

Since you have tested the effective final quirk patch, may I ask you to
post straight away your final tested quirk patch on top of my next V3
(with your authorship of course..)...I will drop the [NOT FOR UPSTREAM]
example patch so that Sudeep can easily pick-up your patch.

Thanks,
Cristian

Re: [PATCH v2 4/4] [NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in attributes

[Cristian Marussi]

On Sun, Apr 27, 2025 at 04:46:47PM +0200, Johan Hovold wrote:
> On Fri, Apr 25, 2025 at 01:52:50PM +0100, Cristian Marussi wrote:
> > Some platform misreported the support of FastChannel when queried: ignore
> > that bit on selected platforms.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > Match features has to be set-up properly before upstreaming this.
> > Ideally the out-of-spec firmware should be matched with a quirk matching
> > pattern based on Vendor/SubVendor/ImplVersion....but it is NOT clear if the
> > platform at hand will ship with future fixed firmwares where the ImplVersion
> > field is properly handled.
> > If we cannot be sure about that, we should fallback to a compatible match.
> > 
> > v1 -> v2
> > - use multiple compats quirks syntax
> > 
> > RFC->V1
> > - fix QUIRKS conditions
> 
> >  /* Global Quirks Definitions */
> >  DEFINE_SCMI_QUIRK(clock_rates_triplet_out_of_spec, NULL, NULL, NULL);
> > +DEFINE_SCMI_QUIRK(perf_level_get_fc_force,
> > +		  "bad-vend", NULL, "0x20000-", "bad-compat", "bad-compat-2");
> 
> Still works when matching on vendor and version (and/or machine or SoC
> compatible):
> 
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
> Tested-by: Johan Hovold <johan+linaro@kernel.org>
> 
> I think we can go ahead and merge this based on vendor and version
> "0x20000-".
> 
> Depending on what Sibi finds out, or if it turns out to be needed, we
> can always add an upper version bound later.

Sure...sounds good.
Please post your final tested patch on top on my V3 as said elsewhere.

Thanks,
Cristian

Re: [PATCH v2 2/4] firmware: arm_scmi: Add Quirks framework

[Johan Hovold]

On Tue, Apr 29, 2025 at 11:47:54AM +0100, Cristian Marussi wrote:
> On Sun, Apr 27, 2025 at 04:38:48PM +0200, Johan Hovold wrote:
> > On Fri, Apr 25, 2025 at 01:52:48PM +0100, Cristian Marussi wrote:
> > > Add a common framework to describe SCMI quirks and associate them with a
> > > specific platform or a specific set of SCMI firmware versions.
> > > 
> > > All the matching SCMI quirks will be enabled when the SCMI core stack
> > > probes and after all the needed SCMI firmware versioning information was
> > > retrieved using Base protocol.

> > > +static void scmi_enable_matching_quirks(struct scmi_info *info)
> > > +{
> > > +	struct scmi_revision_info *rev = &info->version;
> > > +	const char *compatible = NULL;
> > > +	struct device_node *root;
> > > +
> > > +	root = of_find_node_by_path("/");
> > > +	if (root) {
> > > +		of_property_read_string(root, "compatible", &compatible);
> > > +		of_node_put(root);
> > > +	}
> > > +
> > > +	dev_dbg(info->dev, "Looking for quirks matching: %s/%s/%s/0x%08X\n",
> > > +		compatible, rev->vendor_id, rev->sub_vendor_id, rev->impl_ver);
> > 
> > You're now just looking up the most specific compatible string in order
> > to include it in this dev_dbg(). Since you're now matching on all
> > compatible strings, perhaps you can consider just dropping it.
> > 
> 
> Yes indeed I was not sure to keep all of this machinery just to print
> the machine compatible that is used to try to match against the
> (possible) list of compats....on the other side seemed useful to know
> exactly what you are trying to match against....but maybe we can simply
> assume that the machine compatible is well-known....

Perhaps it would have been more useful if you printed all the compatible
strings here and not just the most specific one, but yeah, there are
other ways to read this strings through sysfs.

Keeping as-is should be fine too.

> > > +		/*
> > > +		 * Note that there could be multiple matches so we
> > > +		 * will enable multiple quirk part of an hash collision
> > 
> > nit: "quirks that are part of a"?
> > 
> 
> mmm...as a non-native and poor English speaker I am, though, reasonably
> confident that a/an is chosen based on the vowel/consonant SOUND of the
> next word NOT on the effective letter...am I wrong ?
> (then we could digress about which is the sound of a[n] 'hash' :P ...)

That's my understanding as well, but 'hash' begins with a consonant
sound so I'm pretty sure it's "a hash".

> > > +		 * domain...BUT we cannot assume that ALL quirks on the
> > > +		 * same collision domain are a full match.
> > > +		 */
> > > +		hash_for_each_possible(scmi_quirks_ht, quirk, hash, hkey) {
> > > +			if (quirk->enabled || quirk->hkey != hkey ||
> > > +			    impl < quirk->start_range ||
> > > +			    impl > quirk->end_range)
> > > +				continue;
> > > +
> > > +			if (quirk->compats[0] &&
> > > +			    !of_machine_compatible_match(quirk->compats))
> > > +				continue;
> > > +
> > > +			dev_info(dev, "Enabling SCMI Quirk [%s]\n",
> > > +				 quirk->name);
> > > +
> > > +			dev_dbg(dev,
> > > +				"Quirk matched on: %s/%s/%s/[0x%08X-0x%08X]\n",
> > > +				quirk->compats[0], quirk->vendor,
> > 
> > You can now match on more than one compats string, but I guess printing
> > just the first one is fine.
> > 
> 
> Yes, same as above dev_dbg...not sure if it was meaningful really to
> dump all the list and overload the log with all such info...

Right, indicating there is some compatible string that's being used is
still useful to know.

> > > +				quirk->sub_vendor_id,
> > > +				quirk->start_range, quirk->end_range);
> > > +
> > > +			static_branch_enable(quirk->key);
> > > +			quirk->enabled = true;
> > > +		}
> > > +	}
> > > +}
> > 
> > This seems to work as intended and I've tried matching on machine and
> > SoC compatible strings and/or vendor and protocol version:
> > 
> > Tested-by: Johan Hovold <johan+linaro@kernel.org>
> > 
> 
> Thanks for the review Johan.
> 
> Since you have tested the effective final quirk patch, may I ask you to
> post straight away your final tested quirk patch on top of my next V3
> (with your authorship of course..)...I will drop the [NOT FOR UPSTREAM]
> example patch so that Sudeep can easily pick-up your patch.

Sure, I'll do so. Thanks for your help with getting this sorted!

Johan

Re: [PATCH v2 2/4] firmware: arm_scmi: Add Quirks framework

[Cristian Marussi]

On Tue, Apr 29, 2025 at 01:20:08PM +0200, Johan Hovold wrote:
> On Tue, Apr 29, 2025 at 11:47:54AM +0100, Cristian Marussi wrote:
> > On Sun, Apr 27, 2025 at 04:38:48PM +0200, Johan Hovold wrote:
> > > On Fri, Apr 25, 2025 at 01:52:48PM +0100, Cristian Marussi wrote:
> > > > Add a common framework to describe SCMI quirks and associate them with a
> > > > specific platform or a specific set of SCMI firmware versions.
> > > > 
> > > > All the matching SCMI quirks will be enabled when the SCMI core stack
> > > > probes and after all the needed SCMI firmware versioning information was
> > > > retrieved using Base protocol.
> 
> > > > +static void scmi_enable_matching_quirks(struct scmi_info *info)
> > > > +{
> > > > +	struct scmi_revision_info *rev = &info->version;
> > > > +	const char *compatible = NULL;
> > > > +	struct device_node *root;
> > > > +
> > > > +	root = of_find_node_by_path("/");
> > > > +	if (root) {
> > > > +		of_property_read_string(root, "compatible", &compatible);
> > > > +		of_node_put(root);
> > > > +	}
> > > > +
> > > > +	dev_dbg(info->dev, "Looking for quirks matching: %s/%s/%s/0x%08X\n",
> > > > +		compatible, rev->vendor_id, rev->sub_vendor_id, rev->impl_ver);
> > > 
> > > You're now just looking up the most specific compatible string in order
> > > to include it in this dev_dbg(). Since you're now matching on all
> > > compatible strings, perhaps you can consider just dropping it.
> > > 
> > 
> > Yes indeed I was not sure to keep all of this machinery just to print
> > the machine compatible that is used to try to match against the
> > (possible) list of compats....on the other side seemed useful to know
> > exactly what you are trying to match against....but maybe we can simply
> > assume that the machine compatible is well-known....
> 
> Perhaps it would have been more useful if you printed all the compatible
> strings here and not just the most specific one, but yeah, there are
> other ways to read this strings through sysfs.
> 
> Keeping as-is should be fine too.
> 

I have dropped the whole machinery.

> > > > +		/*
> > > > +		 * Note that there could be multiple matches so we
> > > > +		 * will enable multiple quirk part of an hash collision
> > > 
> > > nit: "quirks that are part of a"?
> > > 
> > 
> > mmm...as a non-native and poor English speaker I am, though, reasonably
> > confident that a/an is chosen based on the vowel/consonant SOUND of the
> > next word NOT on the effective letter...am I wrong ?
> > (then we could digress about which is the sound of a[n] 'hash' :P ...)
> 
> That's my understanding as well, but 'hash' begins with a consonant
> sound so I'm pretty sure it's "a hash".

:P ... so it is now certified publicly how bad my pronunciation skills are 

> 
> > > > +		 * domain...BUT we cannot assume that ALL quirks on the
> > > > +		 * same collision domain are a full match.
> > > > +		 */
> > > > +		hash_for_each_possible(scmi_quirks_ht, quirk, hash, hkey) {
> > > > +			if (quirk->enabled || quirk->hkey != hkey ||
> > > > +			    impl < quirk->start_range ||
> > > > +			    impl > quirk->end_range)
> > > > +				continue;
> > > > +
> > > > +			if (quirk->compats[0] &&
> > > > +			    !of_machine_compatible_match(quirk->compats))
> > > > +				continue;
> > > > +
> > > > +			dev_info(dev, "Enabling SCMI Quirk [%s]\n",
> > > > +				 quirk->name);
> > > > +
> > > > +			dev_dbg(dev,
> > > > +				"Quirk matched on: %s/%s/%s/[0x%08X-0x%08X]\n",
> > > > +				quirk->compats[0], quirk->vendor,
> > > 
> > > You can now match on more than one compats string, but I guess printing
> > > just the first one is fine.
> > > 
> > 
> > Yes, same as above dev_dbg...not sure if it was meaningful really to
> > dump all the list and overload the log with all such info...
> 
> Right, indicating there is some compatible string that's being used is
> still useful to know.
> 
> > > > +				quirk->sub_vendor_id,
> > > > +				quirk->start_range, quirk->end_range);
> > > > +
> > > > +			static_branch_enable(quirk->key);
> > > > +			quirk->enabled = true;
> > > > +		}
> > > > +	}
> > > > +}
> > > 
> > > This seems to work as intended and I've tried matching on machine and
> > > SoC compatible strings and/or vendor and protocol version:
> > > 
> > > Tested-by: Johan Hovold <johan+linaro@kernel.org>
> > > 
> > 
> > Thanks for the review Johan.
> > 
> > Since you have tested the effective final quirk patch, may I ask you to
> > post straight away your final tested quirk patch on top of my next V3
> > (with your authorship of course..)...I will drop the [NOT FOR UPSTREAM]
> > example patch so that Sudeep can easily pick-up your patch.
> 
> Sure, I'll do so. Thanks for your help with getting this sorted!
>

You're welcome.

Cheers,
Cristian