Re: [PATCH 2/4] platform/x86/intel/pmt: update to bit access

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Fri, 16 May 2025, Michael J. Ruhl wrote:

> The current usage of BIT offsets limits adding new

BIT()

> functionality to the crashlog register access.

Please be more precise how it limits, as I don't buy it in the current 
form.

I suspect the limitation is self-imposed by not naming field properly with 
defines but putting GENMASK() inside defines that custom-coded also 
FIELD_GET/PREP() so please check if using FIELD_GET/PREP() within the 
functions (not in the macros) with named defines that only specify field 
masks can overcome the supposed limitations.

> Update the bit mask #defines to use a bit defined
> structure.

These seem quite short lines.

> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 116 +++++++++++++++-------
>  1 file changed, 79 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index 952bfe341f53..dba7e7c1585d 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -22,29 +22,12 @@
>  /* Crashlog discovery header types */
>  #define CRASH_TYPE_OOBMSM	1
>  
> -/* Control Flags */
> -#define CRASHLOG_FLAG_DISABLE		BIT(28)
> -
> -/*
> - * Bits 29 and 30 control the state of bit 31.
> - *
> - * Bit 29 will clear bit 31, if set, allowing a new crashlog to be captured.
> - * Bit 30 will immediately trigger a crashlog to be generated, setting bit 31.
> - * Bit 31 is the read-only status with a 1 indicating log is complete.
> - */
> -#define CRASHLOG_FLAG_TRIGGER_CLEAR	BIT(29)
> -#define CRASHLOG_FLAG_TRIGGER_EXECUTE	BIT(30)
> -#define CRASHLOG_FLAG_TRIGGER_COMPLETE	BIT(31)
> -#define CRASHLOG_FLAG_TRIGGER_MASK	GENMASK(31, 28)
> -
>  /* Crashlog Discovery Header */
>  #define CONTROL_OFFSET		0x0
>  #define GUID_OFFSET		0x4
>  #define BASE_OFFSET		0x8
>  #define SIZE_OFFSET		0xC
>  #define GET_ACCESS(v)		((v) & GENMASK(3, 0))
> -#define GET_TYPE(v)		(((v) & GENMASK(7, 4)) >> 4)
> -#define GET_VERSION(v)		(((v) & GENMASK(19, 16)) >> 16)
>  /* size is in bytes */
>  #define GET_SIZE(v)		((v) * sizeof(u32))
>  
> @@ -54,6 +37,39 @@ struct crashlog_entry {
>  	struct mutex			control_mutex;
>  };
>  
> +struct type1_ver0_base {
> +	u32 access_type:	4;  /* ro     0:3  */
> +	u32 crash_type:		4;  /* ro     4:7  */
> +	u32 count:		8;  /* ro     8:15 */
> +	u32 version:		4;  /* ro    16:19 */
> +	u32 rsvd:		8;  /* ro    20:27 */
> +	u32 disable:		1;  /* rw    28:28 */
> +	/*
> +	 * Bits 29 and 30 control the state of bit 31.
> +	 *
> +	 * Bit 29 will clear bit 31, if set, allowing a new crashlog to be captured.
> +	 * Bit 30 will immediately trigger a crashlog to be generated, setting bit 31.
> +	 * Bit 31 is the read-only status with a 1 indicating log is complete.
> +	 */
> +	u32 clear:		1;  /* rw    29:29 */
> +	u32 manual:		1;  /* rw/1s 30:30 */
> +	u32 complete:		1;  /* ro/v  31:31 */
> +};
> +
> +struct crashlog_status {
> +	union {
> +		struct type1_ver0_base stat;
> +		u32 status;
> +	};
> +};
> +
> +struct crashlog_control {
> +	union {
> +		struct type1_ver0_base ctrl;
> +		u32 control;
> +	};
> +};
> +
>  struct pmt_crashlog_priv {
>  	int			num_entries;
>  	struct crashlog_entry	entry[];
> @@ -64,27 +80,34 @@ struct pmt_crashlog_priv {
>   */
>  static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +	struct crashlog_status status = {
> +		.status = readl(entry->disc_table + CONTROL_OFFSET),
> +	};
>  
>  	/* return current value of the crashlog complete flag */
> -	return !!(control & CRASHLOG_FLAG_TRIGGER_COMPLETE);
> +	return status.stat.complete;
> +
>  }
>  
>  static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +	struct crashlog_status status = {
> +		.status = readl(entry->disc_table + CONTROL_OFFSET),
> +	};
>  
>  	/* return current value of the crashlog disabled flag */
> -	return !!(control & CRASHLOG_FLAG_DISABLE);
> +	return status.stat.disable;
>  }
>  
>  static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
>  {
> -	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
> +	struct crashlog_control discovery_header = {
> +		.control = readl(entry->disc_table + CONTROL_OFFSET),
> +	};
>  	u32 crash_type, version;
>  
> -	crash_type = GET_TYPE(discovery_header);
> -	version = GET_VERSION(discovery_header);
> +	crash_type = discovery_header.ctrl.crash_type;
> +	version = discovery_header.ctrl.version;
>  
>  	/*
>  	 * Currently we only recognize OOBMSM version 0 devices.
> @@ -96,37 +119,53 @@ static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
>  static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
>  				     bool disable)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +	struct crashlog_control control = {
> +		.control = readl(entry->disc_table + CONTROL_OFFSET),
> +	};
>  
>  	/* clear trigger bits so we are only modifying disable flag */
> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> +	control.ctrl.clear = 0;
> +	control.ctrl.manual = 0;
> +	control.ctrl.complete = 0;
>  
>  	if (disable)
> -		control |= CRASHLOG_FLAG_DISABLE;
> +		control.ctrl.disable = 1;
>  	else
> -		control &= ~CRASHLOG_FLAG_DISABLE;
> +		control.ctrl.disable = 0;
>  
> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> +	writel(control.control, entry->disc_table + CONTROL_OFFSET);
>  }
>  
>  static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +	struct crashlog_control control = {
> +		.control = readl(entry->disc_table + CONTROL_OFFSET),
> +	};
>  
> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> -	control |= CRASHLOG_FLAG_TRIGGER_CLEAR;
> +	/* clear trigger bits so we are only modifying disable flag */
> +	control.ctrl.disable = 0;
> +	control.ctrl.manual = 0;
> +	control.ctrl.complete = 0;
>  
> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> +	control.ctrl.clear = 1;
> +
> +	writel(control.control, entry->disc_table + CONTROL_OFFSET);
>  }
>  
>  static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +	struct crashlog_control control = {
> +		.control = readl(entry->disc_table + CONTROL_OFFSET),
> +	};
> +
> +	/* clear trigger bits so we are only modifying disable flag */
> +	control.ctrl.disable = 0;
> +	control.ctrl.clear = 0;
> +	control.ctrl.complete = 0;
>  
> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> -	control |= CRASHLOG_FLAG_TRIGGER_EXECUTE;
> +	control.ctrl.manual = 1;
>  
> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> +	writel(control.control, entry->disc_table + CONTROL_OFFSET);
>  }
>  
>  /*
> @@ -304,6 +343,9 @@ static int pmt_crashlog_probe(struct auxiliary_device *auxdev,
>  	size_t size;
>  	int i, ret;
>  
> +	BUILD_BUG_ON(sizeof(struct crashlog_control) != sizeof(u32));
> +	BUILD_BUG_ON(sizeof(struct crashlog_status) != sizeof(u32));
> +
>  	size = struct_size(priv, entry, intel_vsec_dev->num_resources);
>  	priv = devm_kzalloc(&auxdev->dev, size, GFP_KERNEL);
>  	if (!priv)
> 

-- 
 i.