Hi,
On 4/7/21 11:31 AM, Winkler, Tomas wrote:
>
>
>> Hi,
>>
>> On 4/7/21 8:51 AM, Winkler, Tomas wrote:
>>>>>
>>>>> 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?
>>>
>>> Board production and refurbishing of the board. I can try to rephrase but I
>> thought all the info is in the commit message.
>>> As a runtime feature a user can check that her/his platform is correctly
>> sealed.
>>
>> Manufacturing is clear, refurbishing is very much not clear, do you mean
>> actually desoldering the chip and replacing it with a new one ?
>>
>>>> If this is only used once during production, then I'm not sure if
>>>> introducing a sysfs file for this is desirable.
>>>
>>> What do you suggest, than? I'm just guessing is where are you heading
>>> so the answer is that the manufacturing is often already run on the
>> production OS installation, w/o going into details swapping or reconfiguring
>> the OS is not always an option.
>>> The manufacturer is also a user of ours.
>>
>> Ok, so lets compromise here, please make use of the visibility sysfs attribute
>> callback, which returns a umask and make the file read-only at the umask
>> level if it has been sealed, to make it clear to users that they cannot write to
>> it, the -EACCES error means 'Permission denied' so if the user is already root
>> they are going to get mightily confused if ls -l shows the file is writable.
> Okay, it seems a better solution if the file is the global reset,
> but maybe this path should not be taken if we rename it to extended_test_mode_register3, than it's better to get EACCESS on a specific bit.
Ack, I was thinking about this perhaps not being the best option if we
expose more bits myself too (when I wrote the rest of my email).
Still it might be an idea to do this if all bits which we allow setting
are locked ? Note this is just a suggestion / something to consider,
I think that if we rename the file to extended_test_mode_register3
most of my concerns are solved.
I'm still not entirely happy with the -EACCESS though, can you perhaps do a
dev_err_ratelimited() in that case, logging why the EACCESS is happening?
As I said before, returning EACCESS when the file-mode bits say the file is
writable and the user is root will lead to some head-scratching I'm afraid.
Another option would be to pick another errno value. Although even with
another errno value a dev_err_ratelimited() logging the reason of the
failure is probably a good idea.
>> Also on set you are checking that the written value is bit 20, and on show you
>> are showing the contents of the "Extended Test Mode Register 3" in hex, or
>> at least those bits you are willing to show.
>
> The intention was to left the user space behave same as with direct register access (/dev/mem)
Ack, that is fine.
>> So in essence what you are doing here is giving userspace (some) access to
>> the "Extended Test Mode Register 3", I would prefer to spell that out
>> explicitly. The global_reset sysfs file name to me too much hints at something
>> which the user can trigger / use while it is not intended for user usage.
>
> Yeah, Global reset is maybe too ambiguous name in a general context, this is not the standard platform reset.
Ack.
> I've left it in register form in order to keep the user space as it has access to the register (/dev/mem)
>>
>> Also the Documentation/ABI/testing/sysfs-platform-intel-pm file pretty much
>> describes this as direct register access rather then as some reset mechanism.
>>
>> So I think it would be better to call the new file
>> extended_test_mode_register3, this will also be useful if we need to provide
>> access to other bits in the same register later; and this will be a good
>> template to follow if we need to provide some access to other registers later
>> too.
>
> Need to sync with David on that he pointed just ow, that he plans to expose some more bits.
Ok, I assume I'll eventually see a new version appear.
Regards,
Hans
>>>> 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:
>>> Okay I can do that.
>>>>
>>>> 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;
>>>>> };
>>>>>
>>>>> /**
>>>>>
>>>
>
