Hi Tomas, On Tue, 2021-04-06 at 15:11 +0200, Hans de Goede wrote: > Hi, > > On 4/2/21 5:21 PM, Tomas Winkler wrote: > > From: Tamar Mashiah <tamar.mashiah@xxxxxxxxx> > > > > During PCH (platform/board) manufacturing process a global reset > > has to be induced in order for configuration changes take the effect > > upon following platform reset. > > This setting was commonly done by accessing PMC registers via > > /dev/mem > > but due to security concern /dev/mem access is much restricted, hence > > the reason for exposing this setting via dedicated sysfs interface. > > To prevent post manufacturing abuse the register is protected > > by hardware locking. > > The purpose of this reset functionality is not entirely clear to me. > > Is this only used during production of a board? Or is this also > something > which a user/reseller may use as part of a factory-reset procedure? > > If this is only used once during production, then I'm not sure if > introducing a sysfs file for this is desirable. > > Can you please provide a new version where the purpsoe of the newly > introduced sysfs file is made more clear, both in the commit-msg > as well as in the: Could you also rebase it on the for-next branch at https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git It has patches for intel_pmc_core. I also need to refactor my latest patch set on top of this one since you're adding the same new register as I am. > > Documentation/ABI/testing/sysfs-platform-intel-pmc > > File ? > > Regards, > > Hans > > > > > > > The register in MMIO space is defined for Cannon Lake and newer PCHs. > > > > 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> > > --- > > 2: > > 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) > > > > .../ABI/testing/sysfs-platform-intel-pmc | 11 +++ > > MAINTAINERS | 1 + > > drivers/platform/x86/intel_pmc_core.c | 97 > > +++++++++++++++++++ > > drivers/platform/x86/intel_pmc_core.h | 6 ++ > > 4 files changed, 115 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..7ce00e77fbcd > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-platform-intel-pmc > > @@ -0,0 +1,11 @@ > > +What: /sys/devices/platform/<platform>/global_reset > > +Date: Apr 2021 > > +KernelVersion: 5.13 > > +Contact: "Tomas Winkler" <tomas.winkler@xxxxxxxxx> > > +Description: > > + 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 global_reset will induce > > + a platform global reset upon consequent platform > > reset. > > + in case the register is not locked. > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 04f68e0cda64..618676eba8c8 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -9166,6 +9166,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 ee2f757515b0..8afc198550a4 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,99 @@ 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_global_reset(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 ssize_t global_reset_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 global_reset_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, ®); > > + if (err) > > + return err; > > + > > + /* allow only CF9 writes */ > > + if (reg != ETR3_CF9GR) > > + return -EINVAL; > > + > > + err = set_global_reset(pmcdev); > > + if (err) > > + return err; > > + > > + return len; > > +} > > +static DEVICE_ATTR_RW(global_reset); > > + > > +static struct attribute *pmc_attrs[] = { > > + &dev_attr_global_reset.attr, > > + NULL > > +}; > > + > > +static const struct attribute_group pmc_attr_group = { > > + .attrs = pmc_attrs, > > +}; > > + > > +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; > > @@ -1364,6 +1460,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; > > }; > > > > /** > > >
