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

[Cristian Marussi <cristian.marussi@arm.com>]

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