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>
---
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/base.c   |   1 +
 drivers/firmware/arm_scmi/driver.c |  22 +++
 drivers/firmware/arm_scmi/quirks.c | 242 +++++++++++++++++++++++++++++
 drivers/firmware/arm_scmi/quirks.h |  40 +++++
 6 files changed, 319 insertions(+)
 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/base.c b/drivers/firmware/arm_scmi/base.c
index 86b376c50a13..00dc8f86c627 100644
--- a/drivers/firmware/arm_scmi/base.c
+++ b/drivers/firmware/arm_scmi/base.c
@@ -8,6 +8,7 @@
 #define pr_fmt(fmt) "SCMI Notifications BASE - " fmt
 
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/scmi_protocol.h>
 
 #include "common.h"
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 0e281fca0a38..1d500663004a 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -38,6 +38,7 @@
 
 #include "common.h"
 #include "notify.h"
+#include "quirks.h"
 
 #include "raw_mode.h"
 
@@ -3112,6 +3113,23 @@ 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);
+	}
+
+	/* Enable applicable quirks */
+	scmi_quirks_enable(info->dev, compatible,
+			   rev->vendor_id, rev->sub_vendor_id, rev->impl_ver);
+}
+
 static int scmi_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -3233,6 +3251,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 +3411,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..2f95b35b5ca1
--- /dev/null
+++ b/drivers/firmware/arm_scmi/quirks.c
@@ -0,0 +1,242 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Message Protocol Quirks
+ *
+ * Copyright (C) 2025 ARM Ltd.
+ */
+
+#include <linux/ctype.h>
+#include <linux/device.h>
+#include <linux/export.h>
+#include <linux/hashtable.h>
+#include <linux/kstrtox.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 *compatible;
+	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;
+};
+
+#define __DEFINE_SCMI_QUIRK_ENTRY(_qn, _comp, _ven, _sub, _impl)	\
+	static struct scmi_quirk scmi_quirk_entry_ ## _qn = {		\
+		.name = __stringify(quirk_ ## _qn),			\
+		.compatible = _comp,					\
+		.vendor = _ven,						\
+		.sub_vendor_id = _sub,					\
+		.impl_ver_range = _impl,				\
+		.key = &(scmi_quirk_ ## _qn),				\
+	}
+
+#define __DECLARE_SCMI_QUIRK_ENTRY(_qn)		(&(scmi_quirk_entry_ ## _qn))
+
+/*
+ * Define a quirk by name (_qn) and provide the matching tokens where:
+ *
+ *  _comp : compatible string, NULL means any.
+ *  _ven : SCMI Vendor ID string, NULL means any.
+ *  _sub : SCMI SubVendor ID string, NULL means any.
+ *  _impl : SCMI Implementation Version string, NULL means any.
+ *          This version string can express ranges using the following
+ *          syntax:
+ *
+ *			NULL		[0, 0xFFFFFFFF]
+ *			"X"		[X, X]
+ *			"X-"		[X, 0xFFFFFFFF]
+ *			"-X"		[0, X]
+ *			"X-Y"		[X, Y]
+ *
+ *          where <v> in [MIN, MAX] means:
+ *
+ *		MIN <= <v> <= MAX  && MIN <= MAX
+ *
+ *  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
+ */
+#define DEFINE_SCMI_QUIRK(_qn, _comp, _ven, _sub, _impl)		\
+	DEFINE_STATIC_KEY_FALSE(scmi_quirk_ ## _qn);			\
+	__DEFINE_SCMI_QUIRK_ENTRY(_qn, _comp, _ven, _sub, _impl)
+
+/*
+ * 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, _comp, _ven, _sub, _impl)	\
+	DEFINE_STATIC_KEY_FALSE(scmi_quirk_ ## _qn);			\
+	EXPORT_SYMBOL_GPL(scmi_quirk_ ## _qn);				\
+	__DEFINE_SCMI_QUIRK_ENTRY(_qn, _comp, _ven, _sub, _impl)
+
+/* 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 *compat, 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|%s|",
+			      compat ?: "", 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\n",
+			       quirk->name);
+			continue;
+		}
+		quirk->hkey = scmi_quirk_signature(quirk->compatible,
+						   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/%s/[0x%08X-0x%08X]\n",
+			 quirk->name, quirk, quirk->hkey, quirk->compatible,
+			 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 *compat,
+			const char *vend, const char *subv, const u32 impl)
+{
+	dev_dbg(dev, "Looking for quirks matching: %s/%s/%s/0x%08X\n",
+		compat, vend, subv, impl);
+
+	/* Lookup into scmi_quirks_ht using 2 loops: with/without compatible */
+	for (int k = 1; k >= 0 ; k--) {
+		const char *compat_sel = k > 0 ? compat : NULL;
+
+		for (int i = 3; i > 0; i--) {
+			struct scmi_quirk *quirk;
+			unsigned int hkey;
+
+			hkey = scmi_quirk_signature(compat_sel,
+						    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;
+
+				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->compatible, 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..0f1a14b13ba5
--- /dev/null
+++ b/drivers/firmware/arm_scmi/quirks.h
@@ -0,0 +1,40 @@
+/* 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)
+
+#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 *compat,
+			const char *vend, const char *subv, const u32 impl);
+
+#else
+
+#define DECLARE_SCMI_QUIRK(_qn)
+#define SCMI_QUIRK(_qn, _blk)
+
+static inline void scmi_quirks_initialize(void) { }
+static inline void scmi_quirks_enable(struct device *dev, const char *compat,
+				      const char *vend, const char *sub_vend,
+				      const u32 impl) { }
+
+#endif /* CONFIG_ARM_SCMI_QUIRKS */
+
+#endif /* _SCMI_QUIRKS_H */
-- 
2.47.0

On Tue, Apr 15, 2025 at 03:29:31PM +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>
> ---
> 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..)

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

Looks like you still only allow matching on the most specific compatible
string.

As we discussed in the RFC thread, this will result in one quirk entry
for each machine in a SoC family in case the issue is not machine
specific.

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

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

nit: space after // (if you insist on using c99 comments)

> +		ret = kstrtouint(sep + 1, 0, &quirk->end_range);
> +
> +	if (quirk->start_range > quirk->end_range)
> +		return -EINVAL;
> +
> +	return ret;
> +}

> +/* 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)
> +
> +#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 *compat,
> +			const char *vend, const char *subv, const u32 impl);
> +
> +#else
> +
> +#define DECLARE_SCMI_QUIRK(_qn)
> +#define SCMI_QUIRK(_qn, _blk)

As I mentioned earlier, wouldn't it be useful to always compile the
quirk code in order to make sure changes to surrounding code does not
break builds where the quirks are enabled?

For example, by adding something like

#define SCMI_QUIRK(_qn, _blk)						\
	do {								\
		if (0)							\
			(_blk);						\
	} while (0)

here?

I didn't really review this in detail, but it seems to work as intended
when matching on vendor and version:

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

On Wed, Apr 16, 2025 at 06:00:37PM +0200, Johan Hovold wrote:
> On Tue, Apr 15, 2025 at 03:29:31PM +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.
> > 

Hi Johan,

thanks for having a look..

> > 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>
> > ---
> > 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..)
> 
> > +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);
> 
> Looks like you still only allow matching on the most specific compatible
> string.
> 
> As we discussed in the RFC thread, this will result in one quirk entry
> for each machine in a SoC family in case the issue is not machine
> specific.
> 

Well, yes but the solution would be to add multiple compatible on the
same quirk line, which is definitely less cumbersome than adding
multiple quirk defs for the same quirk but does NOT scale anyway....

...anyway I will add that possibility..or I am missing something more ?

> > +		of_node_put(root);
> > +	}
> > +
> > +	/* Enable applicable quirks */
> > +	scmi_quirks_enable(info->dev, compatible,
> > +			   rev->vendor_id, rev->sub_vendor_id, rev->impl_ver);
> > +}
> 
> > +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
> 
> nit: space after // (if you insist on using c99 comments)
> 

..leftover...I'll remove C99

> > +		ret = kstrtouint(sep + 1, 0, &quirk->end_range);
> > +
> > +	if (quirk->start_range > quirk->end_range)
> > +		return -EINVAL;
> > +
> > +	return ret;
> > +}
> 
> > +/* 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)
> > +
> > +#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 *compat,
> > +			const char *vend, const char *subv, const u32 impl);
> > +
> > +#else
> > +
> > +#define DECLARE_SCMI_QUIRK(_qn)
> > +#define SCMI_QUIRK(_qn, _blk)
> 
> As I mentioned earlier, wouldn't it be useful to always compile the
> quirk code in order to make sure changes to surrounding code does not
> break builds where the quirks are enabled?
> 
> For example, by adding something like
> 
> #define SCMI_QUIRK(_qn, _blk)						\
> 	do {								\
> 		if (0)							\
> 			(_blk);						\
> 	} while (0)
> 
> here?
> 

Yes I will do.

> I didn't really review this in detail, but it seems to work as intended
> when matching on vendor and version:
> 
> Tested-by: Johan Hovold <johan+linaro@kernel.org>

Thanks,
Cristian

On Wed, Apr 16, 2025 at 05:37:13PM +0100, Cristian Marussi wrote:
> On Wed, Apr 16, 2025 at 06:00:37PM +0200, Johan Hovold wrote:
> > On Tue, Apr 15, 2025 at 03:29:31PM +0100, Cristian Marussi wrote:

> > > +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);
> > 
> > Looks like you still only allow matching on the most specific compatible
> > string.
> > 
> > As we discussed in the RFC thread, this will result in one quirk entry
> > for each machine in a SoC family in case the issue is not machine
> > specific.
> 
> Well, yes but the solution would be to add multiple compatible on the
> same quirk line, which is definitely less cumbersome than adding
> multiple quirk defs for the same quirk but does NOT scale anyway....
> 
> ...anyway I will add that possibility..or I am missing something more ?

I was referring to the need to match on other compatible strings than
the most specific one. For the ThinkPad T14s the strings are:

	"lenovo,thinkpad-t14s-lcd", "lenovo,thinkpad-t14s",
	"qcom,x1e78100", "qcom,x1e80100"

Here you most certainly would not want to match on
"lenovo,thinkpad-t14s-lcd" but rather on "lenovo,thinkpad-t14s" or one
of the SoC compatibles.

For the FC quirk we may have to match on compatible and then a single
SoC entry could cover tens of machines (and their SKU variants).

of_machine_is_compatible() can be used to match on any compatible
string, but not sure if that fits with your current implementation.
 
> > > +		of_node_put(root);
> > > +	}
> > > +
> > > +	/* Enable applicable quirks */
> > > +	scmi_quirks_enable(info->dev, compatible,
> > > +			   rev->vendor_id, rev->sub_vendor_id, rev->impl_ver);
> > > +}

Johan

On Thu, Apr 17, 2025 at 10:44:24AM +0200, Johan Hovold wrote:
> On Wed, Apr 16, 2025 at 05:37:13PM +0100, Cristian Marussi wrote:
> > On Wed, Apr 16, 2025 at 06:00:37PM +0200, Johan Hovold wrote:
> > > On Tue, Apr 15, 2025 at 03:29:31PM +0100, Cristian Marussi wrote:
> 
> > > > +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);
> > > 
> > > Looks like you still only allow matching on the most specific compatible
> > > string.
> > > 
> > > As we discussed in the RFC thread, this will result in one quirk entry
> > > for each machine in a SoC family in case the issue is not machine
> > > specific.
> > 
> > Well, yes but the solution would be to add multiple compatible on the
> > same quirk line, which is definitely less cumbersome than adding
> > multiple quirk defs for the same quirk but does NOT scale anyway....
> > 
> > ...anyway I will add that possibility..or I am missing something more ?
> 
> I was referring to the need to match on other compatible strings than
> the most specific one. For the ThinkPad T14s the strings are:
> 
> 	"lenovo,thinkpad-t14s-lcd", "lenovo,thinkpad-t14s",
> 	"qcom,x1e78100", "qcom,x1e80100"
> 
> Here you most certainly would not want to match on
> "lenovo,thinkpad-t14s-lcd" but rather on "lenovo,thinkpad-t14s" or one
> of the SoC compatibles.
> 
> For the FC quirk we may have to match on compatible and then a single
> SoC entry could cover tens of machines (and their SKU variants).
> 
> of_machine_is_compatible() can be used to match on any compatible
> string, but not sure if that fits with your current implementation.
>  

Yes I know, it will need a bit of rework on my side...the problem is
that anyway does not scale at all, even though matching on SoC could be
less cumbersome ... and the reason is that it is fundamentally wrong
to match SCMI Quirks on anything different from the SCMI vendor/subv/impl_ver
since these are fixes/workarounds against quirks that are completely Vendor
and fw-version specific...

...instead now we are considering using compatibles to overcome the fact
that the vendor probably messed (or will mess up) also the side of
the story related to SCMI fw versions management ... "quirking" SCMI stuff
on platform/sku compatibles is basically a quirk against their broken
fw release process...

...moreover this kind of carpet-quirking that hides the issue on any possible
fw version gives ZERO incentives to the aforementioned vendor to fix its
firmware...(or it fw-release process)...

Indeed, cross posting from your other mail thread, as of now we cannot
even be sure if the Vendor has somehow already updated the SCMI-related
firmware NEITHER we can be sure about this in the future since it has not
even confirmed how they are (or they will) be handling the Impl_Version field...

Having said that, I would add that in this specific case (FC quirk) since the
only way to make use of all of this SCMI stuff from the MicroZoft/ACPI world
is having working FCs and, since it's clear that our lovely vendor wont
certainly break this other lovely OS way of working with SCMI, MAYBE it could
be safe to just apply the quirk to this Vendor forever no matter what the
platform or FW version will be in the future...(so not using compats at all)

...but I will be very happy to leave all of these political/phylosophycal
decisions to Sudeep :P

Thanks,
Cristian

On Thu, Apr 17, 2025 at 12:10:25PM +0100, Cristian Marussi wrote:
> On Thu, Apr 17, 2025 at 10:44:24AM +0200, Johan Hovold wrote:
> > On Wed, Apr 16, 2025 at 05:37:13PM +0100, Cristian Marussi wrote:
> > > On Wed, Apr 16, 2025 at 06:00:37PM +0200, Johan Hovold wrote:
> > > > On Tue, Apr 15, 2025 at 03:29:31PM +0100, Cristian Marussi wrote:
> > 
> > > > > +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);
> > > > 
> > > > Looks like you still only allow matching on the most specific compatible
> > > > string.
> > > > 
> > > > As we discussed in the RFC thread, this will result in one quirk entry
> > > > for each machine in a SoC family in case the issue is not machine
> > > > specific.

Agreed, but we can predict that. You can infer just from the current state
of affairs. Today all machines based on soc X may need the quirk but the
firmware may vary across machines with same SoC.

> > > 
> > > Well, yes but the solution would be to add multiple compatible on the
> > > same quirk line, which is definitely less cumbersome than adding
> > > multiple quirk defs for the same quirk but does NOT scale anyway....
> > > 
> > > ...anyway I will add that possibility..or I am missing something more ?
> > 
> > I was referring to the need to match on other compatible strings than
> > the most specific one. For the ThinkPad T14s the strings are:
> > 
> > 	"lenovo,thinkpad-t14s-lcd", "lenovo,thinkpad-t14s",
> > 	"qcom,x1e78100", "qcom,x1e80100"
> > 
> > Here you most certainly would not want to match on
> > "lenovo,thinkpad-t14s-lcd" but rather on "lenovo,thinkpad-t14s" or one
> > of the SoC compatibles.
> > 

SoC compatibles is debatable but if we are sure, we can do that.

> > For the FC quirk we may have to match on compatible and then a single
> > SoC entry could cover tens of machines (and their SKU variants).
> > 

Well theoretically I agree, but even if one machine based on that SoC
doesn't need it we need to fall back to specific compatibles. That's the
argument IIUC as the firmwares depend to be machine specific most of
the time and also newer machines based on the same SoC may carry fixes
that may remove the need for the quirks.

> > of_machine_is_compatible() can be used to match on any compatible
> > string, but not sure if that fits with your current implementation.
> > 

I was thinking about the same when I looked at the code. Using it is
more elegant IMO.

> 
> Yes I know, it will need a bit of rework on my side...the problem is
> that anyway does not scale at all, even though matching on SoC could be
> less cumbersome ... and the reason is that it is fundamentally wrong
> to match SCMI Quirks on anything different from the SCMI vendor/subv/impl_ver
> since these are fixes/workarounds against quirks that are completely Vendor
> and fw-version specific...
> 

Agree!

> ...instead now we are considering using compatibles to overcome the fact
> that the vendor probably messed (or will mess up) also the side of
> the story related to SCMI fw versions management ... "quirking" SCMI stuff
> on platform/sku compatibles is basically a quirk against their broken
> fw release process...
> 

😄

> ...moreover this kind of carpet-quirking that hides the issue on any possible
> fw version gives ZERO incentives to the aforementioned vendor to fix its
> firmware...(or it fw-release process)...
> 

+1

> Indeed, cross posting from your other mail thread, as of now we cannot
> even be sure if the Vendor has somehow already updated the SCMI-related
> firmware NEITHER we can be sure about this in the future since it has not
> even confirmed how they are (or they will) be handling the Impl_Version field...
> 
> Having said that, I would add that in this specific case (FC quirk) since the
> only way to make use of all of this SCMI stuff from the MicroZoft/ACPI world
> is having working FCs and, since it's clear that our lovely vendor wont
> certainly break this other lovely OS way of working with SCMI, MAYBE it could
> be safe to just apply the quirk to this Vendor forever no matter what the
> platform or FW version will be in the future...(so not using compats at all)
> 

🤣

> ...but I will be very happy to leave all of these political/phylosophycal
> decisions to Sudeep :P
> 

Nice! I am happy to have generic compatible if we are sure all the machines
based on or using it needs it. We can fix it up in the future if required
by dropping generic compatibles and add all the more specific ones if the
generic compatible breaks(meaning it doesn't need quirk) on any of the
machines based on it as it is always hard to predict such things.


> Thanks,
> Cristian

-- 
Regards,
Sudeep

On Thu, Apr 17, 2025 at 03:41:56PM +0100, Sudeep Holla wrote:
> On Thu, Apr 17, 2025 at 12:10:25PM +0100, Cristian Marussi wrote:
> > On Thu, Apr 17, 2025 at 10:44:24AM +0200, Johan Hovold wrote:
> > > On Wed, Apr 16, 2025 at 05:37:13PM +0100, Cristian Marussi wrote:
> > > > On Wed, Apr 16, 2025 at 06:00:37PM +0200, Johan Hovold wrote:
> > > > > On Tue, Apr 15, 2025 at 03:29:31PM +0100, Cristian Marussi wrote:
> > > 
> > > > > > +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);
> > > > > 
> > > > > Looks like you still only allow matching on the most specific compatible
> > > > > string.
> > > > > 
> > > > > As we discussed in the RFC thread, this will result in one quirk entry
> > > > > for each machine in a SoC family in case the issue is not machine
> > > > > specific.
> 
> Agreed, but we can predict that. You can infer just from the current state
> of affairs. Today all machines based on soc X may need the quirk but the
> firmware may vary across machines with same SoC.

Sure, I was just highlighting this limitation in the current
implementation...

> > > I was referring to the need to match on other compatible strings than
> > > the most specific one. For the ThinkPad T14s the strings are:
> > > 
> > > 	"lenovo,thinkpad-t14s-lcd", "lenovo,thinkpad-t14s",
> > > 	"qcom,x1e78100", "qcom,x1e80100"
> > > 
> > > Here you most certainly would not want to match on
> > > "lenovo,thinkpad-t14s-lcd" but rather on "lenovo,thinkpad-t14s" or one
> > > of the SoC compatibles.

...and the fact that even if you want to avoid matching on SoC
compatible, the current implementation seems to be too limited to allow
matching on machine compatibles generally (i.e. given that there may be
variants of a particular machine).

We may not even need this for the FC quirk, this was just a general
observation.

> > > of_machine_is_compatible() can be used to match on any compatible
> > > string, but not sure if that fits with your current implementation.
> > > 
> 
> I was thinking about the same when I looked at the code. Using it is
> more elegant IMO.

It would be more flexible, even if you never intend to accept any quirks
that matches for an entire SoC.

> > ...moreover this kind of carpet-quirking that hides the issue on any possible
> > fw version gives ZERO incentives to the aforementioned vendor to fix its
> > firmware...(or it fw-release process)...

If a vendor truly only cares about some other OS then perhaps this
argument isn't that strong and we'll just increase our own maintenance
burden.

> > Indeed, cross posting from your other mail thread, as of now we cannot
> > even be sure if the Vendor has somehow already updated the SCMI-related
> > firmware NEITHER we can be sure about this in the future since it has not
> > even confirmed how they are (or they will) be handling the Impl_Version field...
> > 
> > Having said that, I would add that in this specific case (FC quirk) since the
> > only way to make use of all of this SCMI stuff from the MicroZoft/ACPI world
> > is having working FCs and, since it's clear that our lovely vendor wont
> > certainly break this other lovely OS way of working with SCMI, MAYBE it could
> > be safe to just apply the quirk to this Vendor forever no matter what the
> > platform or FW version will be in the future...(so not using compats at all)

My understanding is that the version has been bumped now albeit for
other reasons than fixing this particular bug. And since enabling FC for
these messages should be safe we will probably be able to match on
vendor/impl_version here.

Sibi is looking into this and should be able to provide an answer
shortly.

Johan

On Tue, Apr 22, 2025 at 12:41:47PM +0200, Johan Hovold wrote:
> On Thu, Apr 17, 2025 at 03:41:56PM +0100, Sudeep Holla wrote:
> > On Thu, Apr 17, 2025 at 12:10:25PM +0100, Cristian Marussi wrote:
> > > On Thu, Apr 17, 2025 at 10:44:24AM +0200, Johan Hovold wrote:
> > > > On Wed, Apr 16, 2025 at 05:37:13PM +0100, Cristian Marussi wrote:
> > > > > On Wed, Apr 16, 2025 at 06:00:37PM +0200, Johan Hovold wrote:
> > > > > > On Tue, Apr 15, 2025 at 03:29:31PM +0100, Cristian Marussi wrote:
> > > > 
> > > > > > > +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);
> > > > > > 
> > > > > > Looks like you still only allow matching on the most specific compatible
> > > > > > string.
> > > > > > 
> > > > > > As we discussed in the RFC thread, this will result in one quirk entry
> > > > > > for each machine in a SoC family in case the issue is not machine
> > > > > > specific.
> >

Hi,

a quick one that I am away from the keyboard currently ...
 
> > Agreed, but we can predict that. You can infer just from the current state
> > of affairs. Today all machines based on soc X may need the quirk but the
> > firmware may vary across machines with same SoC.
> 
> Sure, I was just highlighting this limitation in the current
> implementation...
> 
> > > > I was referring to the need to match on other compatible strings than
> > > > the most specific one. For the ThinkPad T14s the strings are:
> > > > 
> > > > 	"lenovo,thinkpad-t14s-lcd", "lenovo,thinkpad-t14s",
> > > > 	"qcom,x1e78100", "qcom,x1e80100"
> > > > 
> > > > Here you most certainly would not want to match on
> > > > "lenovo,thinkpad-t14s-lcd" but rather on "lenovo,thinkpad-t14s" or one
> > > > of the SoC compatibles.
> 
> ...and the fact that even if you want to avoid matching on SoC
> compatible, the current implementation seems to be too limited to allow
> matching on machine compatibles generally (i.e. given that there may be
> variants of a particular machine).
> 
> We may not even need this for the FC quirk, this was just a general
> observation.
> 
> > > > of_machine_is_compatible() can be used to match on any compatible
> > > > string, but not sure if that fits with your current implementation.
> > > > 
> > 
> > I was thinking about the same when I looked at the code. Using it is
> > more elegant IMO.
> 
> It would be more flexible, even if you never intend to accept any quirks
> that matches for an entire SoC.
> 


I already have a V2 under test that will define a quirk using a
__VA_ARGS__ so that you can specify a variable number of compats to
match against the platform running using of_machine_compatible_match()

+/*
+ * Define a quirk by name (_qn) and provide the matching tokens where:
+ *
+ *  _ven : SCMI Vendor ID string, NULL means any.
+ *  _sub : SCMI SubVendor ID string, NULL means any.
+ *  _impl : SCMI Implementation Version string, NULL means any.
+ *          This version string can express ranges using the following
+ *          syntax:
+ *
+ *                     NULL            [0, 0xFFFFFFFF]
+ *                     "X"             [X, X]
+ *                     "X-"            [X, 0xFFFFFFFF]
+ *                     "-X"            [0, X]
+ *                     "X-Y"           [X, Y]
+ *
+ *          where <v> in [MIN, MAX] means:
+ *
+ *             MIN <= <v> <= MAX  && MIN <= MAX
+ *
+ *  _comps : NULL terminated array of compatible strings.
+ *          An empty array means any.
+ *
+ *  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
+ */
+#define DEFINE_SCMI_QUIRK(_qn, _ven, _sub, _impl, ...)                 \

I will post V2 soon.

> > > ...moreover this kind of carpet-quirking that hides the issue on any possible
> > > fw version gives ZERO incentives to the aforementioned vendor to fix its
> > > firmware...(or it fw-release process)...
> 
> If a vendor truly only cares about some other OS then perhaps this
> argument isn't that strong and we'll just increase our own maintenance
> burden.
> 

indeed.

> > > Indeed, cross posting from your other mail thread, as of now we cannot
> > > even be sure if the Vendor has somehow already updated the SCMI-related
> > > firmware NEITHER we can be sure about this in the future since it has not
> > > even confirmed how they are (or they will) be handling the Impl_Version field...
> > > 
> > > Having said that, I would add that in this specific case (FC quirk) since the
> > > only way to make use of all of this SCMI stuff from the MicroZoft/ACPI world
> > > is having working FCs and, since it's clear that our lovely vendor wont
> > > certainly break this other lovely OS way of working with SCMI, MAYBE it could
> > > be safe to just apply the quirk to this Vendor forever no matter what the
> > > platform or FW version will be in the future...(so not using compats at all)
> 
> My understanding is that the version has been bumped now albeit for
> other reasons than fixing this particular bug. And since enabling FC for
> these messages should be safe we will probably be able to match on
> vendor/impl_version here.

So what is your latest Firmware version read ? no more 0x20000

[    0.116746] arm-scmi arm-scmi.0.auto: SCMI Protocol ?? 'Qualcom:' Firmware version ???????

> 
> Sibi is looking into this and should be able to provide an answer
> shortly.
> 

Good.

Thanks,
Cristian

On Tue, Apr 22, 2025 at 12:33:40PM +0100, Cristian Marussi wrote:
> On Tue, Apr 22, 2025 at 12:41:47PM +0200, Johan Hovold wrote:
> > On Thu, Apr 17, 2025 at 03:41:56PM +0100, Sudeep Holla wrote:
> > > On Thu, Apr 17, 2025 at 12:10:25PM +0100, Cristian Marussi wrote:
> > > > On Thu, Apr 17, 2025 at 10:44:24AM +0200, Johan Hovold wrote:

> > > > > of_machine_is_compatible() can be used to match on any compatible
> > > > > string, but not sure if that fits with your current implementation.
> > > 
> > > I was thinking about the same when I looked at the code. Using it is
> > > more elegant IMO.
> > 
> > It would be more flexible, even if you never intend to accept any quirks
> > that matches for an entire SoC.
> 
> I already have a V2 under test that will define a quirk using a
> __VA_ARGS__ so that you can specify a variable number of compats to
> match against the platform running using of_machine_compatible_match()

Sounds good.

> > My understanding is that the version has been bumped now albeit for
> > other reasons than fixing this particular bug. And since enabling FC for
> > these messages should be safe we will probably be able to match on
> > vendor/impl_version here.
> 
> So what is your latest Firmware version read ? no more 0x20000
> 
> [    0.116746] arm-scmi arm-scmi.0.auto: SCMI Protocol ?? 'Qualcom:' Firmware version ???????

It's still 0x20000 for this platform AFAIU, and that probably won't
change even if the fix for this particular bug makes it into the
firmware for the commercial devices.

Johan