Re: [PATCH v8 04/20] platform/x86/intel/pmt: Add register access callbacks

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Sep 11, 2026 at 01:11:52PM -0700, Michael J. Ruhl wrote:
> Some HW does not have direct MMIO access to PMT control and data
> features.
> 
> Augment the current callback infrastructure (data access) to allow
> a registered driver to customize read/write access to the control
> paths for PMT usage.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 46 +++++++++++++++++++++--
>  include/linux/intel_vsec.h                | 14 ++++++-
>  2 files changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index b6540f3a9a17..912b61c334a6 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -11,10 +11,12 @@
>  #include <linux/auxiliary_bus.h>
>  #include <linux/cleanup.h>
>  #include <linux/intel_vsec.h>
> +#include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/pci.h>
> +#include <linux/printk.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
>  #include <linux/overflow.h>
> @@ -124,12 +126,36 @@ struct pmt_crashlog_priv {
>   * I/O
>   */
>  
> +static int pmt_crashlog_read_reg(struct intel_pmt_entry *entry, u32 *reg, u32 offset)
> +{
> +	int ret;
> +
> +	*reg = 0;
> +
> +	if (entry->cb && entry->cb->read_reg) {
> +		ret = entry->cb->read_reg(entry->dev, entry->header.guid, reg, offset);
> +		if (ret) {
> +			pr_err_ratelimited("failed to read reg: %d\n", ret);

I believe we should use dev_err_ratelimited no?!
since patch 1 we have the right dev handle anyway now

> +			return ret;
> +		}
> +	} else {
> +		*reg = readl(entry->disc_table + offset);
> +	}
> +
> +	return 0;
> +}
> +
>  /* Read, modify, write the control register, setting or clearing @bit based on @set */
>  static int pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
>  {
>  	const struct crashlog_control *control = &crashlog->info->control;
>  	struct intel_pmt_entry *entry = &crashlog->entry;
> -	u32 reg = readl(entry->disc_table + control->offset);
> +	u32 reg;
> +	int ret;
> +
> +	ret = pmt_crashlog_read_reg(entry, &reg, control->offset);
> +	if (ret)
> +		return ret;
>  
>  	reg &= ~control->trigger_mask;
>  
> @@ -138,7 +164,15 @@ static int pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
>  	else
>  		reg &= ~bit;
>  
> -	writel(reg, entry->disc_table + control->offset);
> +	if (entry->cb && entry->cb->write_reg) {
> +		ret = entry->cb->write_reg(entry->dev, entry->header.guid, reg, control->offset);
> +		if (ret) {
> +			pr_err_ratelimited("failed to write reg: %d\n", ret);
> +			return ret;
> +		}
> +	} else {
> +		writel(reg, entry->disc_table + control->offset);
> +	}
>  
>  	return 0;
>  }
> @@ -147,7 +181,13 @@ static int pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
>  static int pmt_crashlog_rc(struct crashlog_entry *crashlog, u32 bit, bool *state)
>  {
>  	const struct crashlog_status *status = &crashlog->info->status;
> -	u32 reg = readl(crashlog->entry.disc_table + status->offset);
> +	struct intel_pmt_entry *entry = &crashlog->entry;
> +	u32 reg;
> +	int ret;
> +
> +	ret = pmt_crashlog_read_reg(entry, &reg, status->offset);
> +	if (ret)
> +		return ret;
>  
>  	*state = !!(reg & bit);
>  
> diff --git a/include/linux/intel_vsec.h b/include/linux/intel_vsec.h
> index 843cda8f8644..917d9397a993 100644
> --- a/include/linux/intel_vsec.h
> +++ b/include/linux/intel_vsec.h
> @@ -90,13 +90,25 @@ enum intel_vsec_quirks {
>   * @read_telem: when specified, called by client driver to access PMT
>   * data (instead of direct copy).
>   * * dev:   device reference for the callback's use
> - * * guid:  ID of data to acccss
> + * * guid:  ID of data to access
>   * * data:  buffer for the data to be copied
>   * * off:   offset into the requested buffer
>   * * count: size of buffer
> + * @read_reg: when specified called by client driver to read PMT state
> + * * dev:      device reference for the callback's use
> + * * guid:     ID of data to access
> + * * reg_data: register data
> + * * offset:   offset of register to read
> + * @write_reg: when specified called by client driver to write PMT state
> + * * dev:      device reference for the callback's use
> + * * guid:     ID of data to access
> + * * reg_data: register data
> + * * offset:   offset of register to write
>   */
>  struct pmt_callbacks {
>  	int (*read_telem)(struct device *dev, u32 guid, u64 *data, loff_t off, u32 count);
> +	int (*read_reg)(struct device *dev, u32 guid, u32 *reg_data, u32 offset);
> +	int (*write_reg)(struct device *dev, u32 guid, u32 reg_data, u32 offset);
>  };
>  
>  struct vsec_feature_dependency {
> -- 
> 2.43.0
>