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

[Johan Hovold <johan@kernel.org>]

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