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

[Johan Hovold]

On Tue, Apr 01, 2025 at 01:25:45PM +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@xxxxxxx>
> ---
> 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.
> ---
>  drivers/firmware/arm_scmi/driver.c | 8 ++++++++
>  drivers/firmware/arm_scmi/quirks.c | 3 +++
>  drivers/firmware/arm_scmi/quirks.h | 3 +++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 4266ef852c48..212456305bc9 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 == 0x5 /* PERF_LEVEL_GET */)		\

This should be logical AND and PERF_LEVEL_GET is 0x8 (currently
fastchannel is enabled for all PERF messages).

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

This is cool and I assume can be used to minimise overhead in hot paths.
Perhaps you can have concerns about readability and remembering to
update the quirk implementation if the code here changes.

Does it even get compile tested if SCMI_QUIRKS is disabled?

>  	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 83798bc3b043..78d51bd0e5b5 100644
> --- a/drivers/firmware/arm_scmi/quirks.c
> +++ b/drivers/firmware/arm_scmi/quirks.c
> @@ -70,6 +70,8 @@ struct scmi_quirk {
>  	__DEFINE_SCMI_QUIRK_ENTRY(_qn, _comp, _ven, _sub, _impl)
>  
>  /* Global Quirks Definitions */
> +DEFINE_SCMI_QUIRK(perf_level_get_fc_force,
> +		  "your-bad-compatible", NULL, NULL, 0x0);

At first I tried matching on the SoC (fallback) compatible without
success until I noticed you need to put the primary machine compatible
here. For the SoC at hand, that would mean adding 10 or so entries since
all current commercial devices would be affected by this.

Matching on vendor and protocol works.

>  /*
>   * Quirks Pointers Array
> @@ -78,6 +80,7 @@ struct scmi_quirk {
>   * defined quirks descriptors.
>   */
>  static struct scmi_quirk *scmi_quirks_table[] = {
> +	__DECLARE_SCMI_QUIRK_ENTRY(perf_level_get_fc_force),
>  	NULL
>  };

Johan