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

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

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