Re: [PATCH v6] platform/x86: intel_pmc_core: export platform global reset bits via etr3 sysfs file

[Hans de Goede]

Hi,

On 4/11/21 4:15 PM, Tomas Winkler wrote:
> From: Tamar Mashiah <tamar.mashiah@xxxxxxxxx>
> 
> During PCH (platform/board) manufacturing process a global platform
> reset has to be induced in order for the configuration changes take
> the effect upon following platform reset. This is an internal platform
> state and is not intended to be used in the regular platform resets.
> The setting is exposed via ETR3 (Extended Test Mode Register 3).
> After the manufacturing process is completed the register cannot be
> written anymore and is hardware locked.
> This setting was commonly done by accessing PMC registers via /dev/mem
> but due to security concerns /dev/mem access is much more restricted,
> hence the reason for exposing this setting via the dedicated sysfs
> interface.
> To prevent post manufacturing abuse the register is protected
> by hardware locking and the file is set to read-only mode via is_visible
> handler.
> 
> The register in MMIO space is defined for Cannon Lake and newer PCHs.
> 
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> Cc: David E Box <david.e.box@xxxxxxxxx>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Signed-off-by: Tamar Mashiah <tamar.mashiah@xxxxxxxxx>
> Signed-off-by: Tomas Winkler <tomas.winkler@xxxxxxxxx>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans




> ---
> V2:
> 1. Add locking for reading the ET3 register  (Andy)
> 2. Fix few style issues (Andy)
> V3:
> 1. Resend
> v4:
> 1. Fix return statement (Andy)
> 2. Specify manufacturing process (Enrico)
> V5:
> 1. Rename sysfs file to etr3 (Hans)
> 2. Make file read only when register is locked (Hans)
> 3. Add more info to sysfs ABI documentation
> V5:
> 1. Parentheses around arithmetic in operand of '|' [-Wparentheses] (lkp)
>    656 |  return reg & ETR3_CF9LOCK ? attr->mode & SYSFS_PREALLOC | 0444 : attr->mode
> 
>  .../ABI/testing/sysfs-platform-intel-pmc      |  20 ++++
>  MAINTAINERS                                   |   1 +
>  drivers/platform/x86/intel_pmc_core.c         | 113 ++++++++++++++++++
>  drivers/platform/x86/intel_pmc_core.h         |   6 +
>  4 files changed, 140 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform-intel-pmc
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-intel-pmc b/Documentation/ABI/testing/sysfs-platform-intel-pmc
> new file mode 100644
> index 000000000000..ef199af75ab0
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-intel-pmc
> @@ -0,0 +1,20 @@
> +What:		/sys/devices/platform/<platform>/etr3
> +Date:		Apr 2021
> +KernelVersion:	5.13
> +Contact:	"Tomas Winkler" <tomas.winkler@xxxxxxxxx>
> +Description:
> +		The file exposes "Extended Test Mode Register 3" global
> +		reset bits. The bits are used during an Intel platform
> +		manufacturing process to indicate that consequent reset
> +		of the platform is a "global reset". This type of reset
> +		is required in order for manufacturing configurations
> +		to take effect.
> +
> +		Display global reset setting bits for PMC.
> +			* bit 31 - global reset is locked
> +			* bit 20 - global reset is set
> +		Writing bit 20 value to the etr3 will induce
> +		a platform "global reset" upon consequent platform reset,
> +		in case the register is not locked.
> +		The "global reset bit" should be locked on a production
> +		system and the file is in read-only mode.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7dd6b67f0f51..3e898660b5b4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9145,6 +9145,7 @@ M:	Rajneesh Bhardwaj <irenic.rajneesh@xxxxxxxxx>
>  M:	David E Box <david.e.box@xxxxxxxxx>
>  L:	platform-driver-x86@xxxxxxxxxxxxxxx
>  S:	Maintained
> +F:	Documentation/ABI/testing/sysfs-platform-intel-pmc
>  F:	drivers/platform/x86/intel_pmc_core*
>  
>  INTEL PMIC GPIO DRIVERS
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index b5888aeb4bcf..8fb4e6d1d68d 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -401,6 +401,7 @@ static const struct pmc_reg_map cnp_reg_map = {
>  	.pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
>  	.pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
>  	.ltr_ignore_max = CNP_NUM_IP_IGN_ALLOWED,
> +	.etr3_offset = ETR3_OFFSET,
>  };
>  
>  static const struct pmc_reg_map icl_reg_map = {
> @@ -418,6 +419,7 @@ static const struct pmc_reg_map icl_reg_map = {
>  	.pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
>  	.pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
>  	.ltr_ignore_max = ICL_NUM_IP_IGN_ALLOWED,
> +	.etr3_offset = ETR3_OFFSET,
>  };
>  
>  static const struct pmc_bit_map tgl_clocksource_status_map[] = {
> @@ -585,6 +587,7 @@ static const struct pmc_reg_map tgl_reg_map = {
>  	.lpm_sts = tgl_lpm_maps,
>  	.lpm_status_offset = TGL_LPM_STATUS_OFFSET,
>  	.lpm_live_status_offset = TGL_LPM_LIVE_STATUS_OFFSET,
> +	.etr3_offset = ETR3_OFFSET,
>  };
>  
>  static inline u32 pmc_core_reg_read(struct pmc_dev *pmcdev, int reg_offset)
> @@ -603,6 +606,115 @@ static inline u64 pmc_core_adjust_slp_s0_step(struct pmc_dev *pmcdev, u32 value)
>  	return (u64)value * pmcdev->map->slp_s0_res_counter_step;
>  }
>  
> +static int set_etr3(struct pmc_dev *pmcdev)
> +{
> +	const struct pmc_reg_map *map = pmcdev->map;
> +	u32 reg;
> +	int err;
> +
> +	if (!map->etr3_offset)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&pmcdev->lock);
> +
> +	/* check if CF9 is locked */
> +	reg = pmc_core_reg_read(pmcdev, map->etr3_offset);
> +	if (reg & ETR3_CF9LOCK) {
> +		err = -EACCES;
> +		goto out_unlock;
> +	}
> +
> +	/* write CF9 global reset bit */
> +	reg |= ETR3_CF9GR;
> +	pmc_core_reg_write(pmcdev, map->etr3_offset, reg);
> +
> +	reg = pmc_core_reg_read(pmcdev, map->etr3_offset);
> +	if (!(reg & ETR3_CF9GR)) {
> +		err = -EIO;
> +		goto out_unlock;
> +	}
> +
> +	err = 0;
> +
> +out_unlock:
> +	mutex_unlock(&pmcdev->lock);
> +	return err;
> +}
> +static umode_t etr3_is_visible(struct kobject *kobj,
> +				struct attribute *attr,
> +				int idx)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> +	const struct pmc_reg_map *map = pmcdev->map;
> +	u32 reg;
> +
> +	mutex_lock(&pmcdev->lock);
> +	reg = pmc_core_reg_read(pmcdev, map->etr3_offset);
> +	mutex_unlock(&pmcdev->lock);
> +
> +	return reg & ETR3_CF9LOCK ? attr->mode & (SYSFS_PREALLOC | 0444) : attr->mode;
> +}
> +
> +static ssize_t etr3_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> +	const struct pmc_reg_map *map = pmcdev->map;
> +	u32 reg;
> +
> +	if (!map->etr3_offset)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&pmcdev->lock);
> +
> +	reg = pmc_core_reg_read(pmcdev, map->etr3_offset);
> +	reg &= ETR3_CF9GR | ETR3_CF9LOCK;
> +
> +	mutex_unlock(&pmcdev->lock);
> +
> +	return sysfs_emit(buf, "0x%08x", reg);
> +}
> +
> +static ssize_t etr3_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t len)
> +{
> +	struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> +	int err;
> +	u32 reg;
> +
> +	err = kstrtouint(buf, 16, &reg);
> +	if (err)
> +		return err;
> +
> +	/* allow only CF9 writes */
> +	if (reg != ETR3_CF9GR)
> +		return -EINVAL;
> +
> +	err = set_etr3(pmcdev);
> +	if (err)
> +		return err;
> +
> +	return len;
> +}
> +static DEVICE_ATTR_RW(etr3);
> +
> +static struct attribute *pmc_attrs[] = {
> +	&dev_attr_etr3.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group pmc_attr_group = {
> +	.attrs = pmc_attrs,
> +	.is_visible = etr3_is_visible,
> +};
> +
> +static const struct attribute_group *pmc_dev_groups[] = {
> +	&pmc_attr_group,
> +	NULL
> +};
> +
>  static int pmc_core_dev_state_get(void *data, u64 *val)
>  {
>  	struct pmc_dev *pmcdev = data;
> @@ -1384,6 +1496,7 @@ static struct platform_driver pmc_core_driver = {
>  		.name = "intel_pmc_core",
>  		.acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
>  		.pm = &pmc_core_pm_ops,
> +		.dev_groups = pmc_dev_groups,
>  	},
>  	.probe = pmc_core_probe,
>  	.remove = pmc_core_remove,
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index f33cd2c34835..98ebdfe57138 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -200,6 +200,11 @@ enum ppfear_regs {
>  #define TGL_LPM_STATUS_OFFSET			0x1C3C
>  #define TGL_LPM_LIVE_STATUS_OFFSET		0x1C5C
>  
> +/* Extended Test Mode Register 3 (CNL and later) */
> +#define ETR3_OFFSET				0x1048
> +#define ETR3_CF9GR				BIT(20)
> +#define ETR3_CF9LOCK				BIT(31)
> +
>  const char *tgl_lpm_modes[] = {
>  	"S0i2.0",
>  	"S0i2.1",
> @@ -263,6 +268,7 @@ struct pmc_reg_map {
>  	const u32 lpm_residency_offset;
>  	const u32 lpm_status_offset;
>  	const u32 lpm_live_status_offset;
> +	const u32 etr3_offset;
>  };
>  
>  /**
>