2023-03-31 17:14:16

by srinivas pandruvada

[permalink] [raw]
Subject: [PATCH] thermal/drivers/intel/int340x: Add DLVR support

Add support for DLVR (Digital Linear Voltage Regulator) attributes,
which can be used to control RFIM.
Here instead of "fivr" another directory "dlvr" is created with DLVR
attributes:

/sys/bus/pci/devices/0000:00:04.0/dlvr
├── dlvr_freq_mhz
├── dlvr_freq_select
├── dlvr_hardware_rev
├── dlvr_pll_busy
├── dlvr_rfim_enable
└── dlvr_spread_spectrum_pct

Attributes
dlvr_freq_mhz (RO):
Current DLVR PLL frequency in MHz.

dlvr_freq_select (RW):
Sets DLVR PLL clock frequency.

dlvr_hardware_rev (RO):
DLVR hardware revision.

dlvr_pll_busy (RO):
PLL can't accept frequency change when set.

dlvr_rfim_enable (RW):
0: Disable RF frequency hopping, 1: Enable RF frequency hopping.

dlvr_spread_spectrum_pct (RW)
A write to this register updates the DLVR spread spectrum percent value.

Signed-off-by: Srinivas Pandruvada <[email protected]>
---
.../driver-api/thermal/intel_dptf.rst | 25 ++++++
.../processor_thermal_device.c | 3 +-
.../processor_thermal_device.h | 1 +
.../processor_thermal_device_pci.c | 2 +-
.../int340x_thermal/processor_thermal_rfim.c | 80 +++++++++++++++++--
5 files changed, 104 insertions(+), 7 deletions(-)

diff --git a/Documentation/driver-api/thermal/intel_dptf.rst b/Documentation/driver-api/thermal/intel_dptf.rst
index f5c193cccbda..da906deb6a7d 100644
--- a/Documentation/driver-api/thermal/intel_dptf.rst
+++ b/Documentation/driver-api/thermal/intel_dptf.rst
@@ -264,6 +264,31 @@ DVFS attributes
``rfi_disable (RW)``
Disable DDR rate change feature

+DLVR attributes
+
+:file:`/sys/bus/pci/devices/0000\:00\:04.0/dlvr/`
+
+``dlvr_hardware_rev`` (RO)
+ DLVR hardware revision.
+
+``dlvr_freq_mhz`` (RO)
+ Current DLVR PLL frequency in MHz.
+
+``dlvr_freq_select`` (RW)
+ Sets DLVR PLL clock frequency. Once set, and enabled via
+ dlvr_rfim_enable, the dlvr_freq_mhz will show the current
+ DLVR PLL frequency.
+
+``dlvr_pll_busy`` (RO)
+ PLL can't accept frequency change when set.
+
+``dlvr_rfim_enable`` (RW)
+ 0: Disable RF frequency hopping, 1: Enable RF frequency hopping.
+
+``dlvr_spread_spectrum_pct`` (RW)
+ Sets DLVR spread spectrum percent value.
+
+
DPTF Power supply and Battery Interface
----------------------------------------

diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
index a1dc18be7609..3ca0a2f5937f 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
@@ -337,7 +337,8 @@ int proc_thermal_mmio_add(struct pci_dev *pdev,
}

if (feature_mask & PROC_THERMAL_FEATURE_FIVR ||
- feature_mask & PROC_THERMAL_FEATURE_DVFS) {
+ feature_mask & PROC_THERMAL_FEATURE_DVFS ||
+ feature_mask & PROC_THERMAL_FEATURE_DLVR) {
ret = proc_thermal_rfim_add(pdev, proc_priv);
if (ret) {
dev_err(&pdev->dev, "failed to add RFIM interface\n");
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
index 7d52fcff4937..7acaa8f1b896 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
@@ -60,6 +60,7 @@ struct rapl_mmio_regs {
#define PROC_THERMAL_FEATURE_FIVR 0x02
#define PROC_THERMAL_FEATURE_DVFS 0x04
#define PROC_THERMAL_FEATURE_MBOX 0x08
+#define PROC_THERMAL_FEATURE_DLVR 0x10

#if IS_ENABLED(CONFIG_PROC_THERMAL_MMIO_RAPL)
int proc_thermal_rapl_add(struct pci_dev *pdev, struct proc_thermal_device *proc_priv);
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
index 90526f46c9b1..aaff67377250 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
@@ -352,7 +352,7 @@ static SIMPLE_DEV_PM_OPS(proc_thermal_pci_pm, proc_thermal_pci_suspend,

static const struct pci_device_id proc_thermal_pci_ids[] = {
{ PCI_DEVICE_DATA(INTEL, ADL_THERMAL, PROC_THERMAL_FEATURE_RAPL | PROC_THERMAL_FEATURE_FIVR | PROC_THERMAL_FEATURE_DVFS | PROC_THERMAL_FEATURE_MBOX) },
- { PCI_DEVICE_DATA(INTEL, MTLP_THERMAL, PROC_THERMAL_FEATURE_RAPL | PROC_THERMAL_FEATURE_FIVR | PROC_THERMAL_FEATURE_DVFS | PROC_THERMAL_FEATURE_MBOX) },
+ { PCI_DEVICE_DATA(INTEL, MTLP_THERMAL, PROC_THERMAL_FEATURE_RAPL | PROC_THERMAL_FEATURE_FIVR | PROC_THERMAL_FEATURE_DVFS | PROC_THERMAL_FEATURE_MBOX | PROC_THERMAL_FEATURE_DLVR) },
{ PCI_DEVICE_DATA(INTEL, RPL_THERMAL, PROC_THERMAL_FEATURE_RAPL | PROC_THERMAL_FEATURE_FIVR | PROC_THERMAL_FEATURE_DVFS | PROC_THERMAL_FEATURE_MBOX) },
{ },
};
diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
index 92ed1213fe37..951a0869982a 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
@@ -39,6 +39,25 @@ static const struct mmio_reg tgl_fivr_mmio_regs[] = {
{ 1, 0x5A14, 2, 0x3, 1}, /* fivr_fffc_rev */
};

+static const char * const dlvr_strings[] = {
+ "dlvr_spread_spectrum_pct",
+ "dlvr_rfim_enable",
+ "dlvr_freq_select",
+ "dlvr_hardware_rev",
+ "dlvr_freq_mhz",
+ "dlvr_pll_busy",
+ NULL
+};
+
+static const struct mmio_reg dlvr_mmio_regs[] = {
+ { 0, 0x15A08, 5, 0x1F, 0}, /* dlvr_spread_spectrum_pct */
+ { 0, 0x15A08, 1, 0x1, 7}, /* dlvr_rfim_enable */
+ { 0, 0x15A08, 12, 0xFFF, 8}, /* dlvr_freq_select */
+ { 1, 0x15A10, 2, 0x3, 30}, /* dlvr_hardware_rev */
+ { 1, 0x15A10, 16, 0xFFFF, 0}, /* dlvr_freq_mhz */
+ { 1, 0x15A10, 1, 0x1, 16}, /* dlvr_pll_busy */
+};
+
/* These will represent sysfs attribute names */
static const char * const dvfs_strings[] = {
"rfi_restriction_run_busy",
@@ -78,14 +97,16 @@ static ssize_t suffix##_show(struct device *dev,\
int ret;\
\
proc_priv = pci_get_drvdata(pdev);\
- if (table) {\
+ if (table == 1) {\
match_strs = (const char **)dvfs_strings;\
mmio_regs = adl_dvfs_mmio_regs;\
- } else { \
+ } else if (table == 2) { \
+ match_strs = (const char **)dlvr_strings;\
+ mmio_regs = dlvr_mmio_regs;\
+ } else {\
match_strs = (const char **)fivr_strings;\
mmio_regs = tgl_fivr_mmio_regs;\
} \
- \
ret = match_string(match_strs, -1, attr->attr.name);\
if (ret < 0)\
return ret;\
@@ -109,10 +130,13 @@ static ssize_t suffix##_store(struct device *dev,\
u32 mask;\
\
proc_priv = pci_get_drvdata(pdev);\
- if (table) {\
+ if (table == 1) {\
match_strs = (const char **)dvfs_strings;\
mmio_regs = adl_dvfs_mmio_regs;\
- } else { \
+ } else if (table == 2) { \
+ match_strs = (const char **)dlvr_strings;\
+ mmio_regs = dlvr_mmio_regs;\
+ } else {\
match_strs = (const char **)fivr_strings;\
mmio_regs = tgl_fivr_mmio_regs;\
} \
@@ -147,6 +171,39 @@ RFIM_STORE(spread_spectrum_clk_enable, 0)
RFIM_STORE(rfi_vco_ref_code, 0)
RFIM_STORE(fivr_fffc_rev, 0)

+RFIM_SHOW(dlvr_spread_spectrum_pct, 2)
+RFIM_SHOW(dlvr_hardware_rev, 2)
+RFIM_SHOW(dlvr_freq_mhz, 2)
+RFIM_SHOW(dlvr_pll_busy, 2)
+RFIM_SHOW(dlvr_freq_select, 2)
+RFIM_SHOW(dlvr_rfim_enable, 2)
+
+RFIM_STORE(dlvr_spread_spectrum_pct, 2)
+RFIM_STORE(dlvr_rfim_enable, 2)
+RFIM_STORE(dlvr_freq_select, 2)
+
+static DEVICE_ATTR_RW(dlvr_spread_spectrum_pct);
+static DEVICE_ATTR_RW(dlvr_freq_select);
+static DEVICE_ATTR_RO(dlvr_hardware_rev);
+static DEVICE_ATTR_RO(dlvr_freq_mhz);
+static DEVICE_ATTR_RO(dlvr_pll_busy);
+static DEVICE_ATTR_RW(dlvr_rfim_enable);
+
+static struct attribute *dlvr_attrs[] = {
+ &dev_attr_dlvr_spread_spectrum_pct.attr,
+ &dev_attr_dlvr_freq_select.attr,
+ &dev_attr_dlvr_hardware_rev.attr,
+ &dev_attr_dlvr_freq_mhz.attr,
+ &dev_attr_dlvr_pll_busy.attr,
+ &dev_attr_dlvr_rfim_enable.attr,
+ NULL
+};
+
+static const struct attribute_group dlvr_attribute_group = {
+ .attrs = dlvr_attrs,
+ .name = "dlvr"
+};
+
static DEVICE_ATTR_RW(vco_ref_code_lo);
static DEVICE_ATTR_RW(vco_ref_code_hi);
static DEVICE_ATTR_RW(spread_spectrum_pct);
@@ -277,12 +334,22 @@ int proc_thermal_rfim_add(struct pci_dev *pdev, struct proc_thermal_device *proc
return ret;
}

+ if (proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_DLVR) {
+ ret = sysfs_create_group(&pdev->dev.kobj, &dlvr_attribute_group);
+ if (ret)
+ return ret;
+ }
+
if (proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_DVFS) {
ret = sysfs_create_group(&pdev->dev.kobj, &dvfs_attribute_group);
if (ret && proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_FIVR) {
sysfs_remove_group(&pdev->dev.kobj, &fivr_attribute_group);
return ret;
}
+ if (ret && proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_DLVR) {
+ sysfs_remove_group(&pdev->dev.kobj, &dlvr_attribute_group);
+ return ret;
+ }
}

return 0;
@@ -296,6 +363,9 @@ void proc_thermal_rfim_remove(struct pci_dev *pdev)
if (proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_FIVR)
sysfs_remove_group(&pdev->dev.kobj, &fivr_attribute_group);

+ if (proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_DLVR)
+ sysfs_remove_group(&pdev->dev.kobj, &dlvr_attribute_group);
+
if (proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_DVFS)
sysfs_remove_group(&pdev->dev.kobj, &dvfs_attribute_group);
}
--
2.34.1


2023-04-03 18:55:57

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH] thermal/drivers/intel/int340x: Add DLVR support

On Fri, Mar 31, 2023 at 6:53 PM Srinivas Pandruvada
<[email protected]> wrote:
>
> Add support for DLVR (Digital Linear Voltage Regulator) attributes,
> which can be used to control RFIM.
> Here instead of "fivr" another directory "dlvr" is created with DLVR
> attributes:
>
> /sys/bus/pci/devices/0000:00:04.0/dlvr
> ├── dlvr_freq_mhz
> ├── dlvr_freq_select
> ├── dlvr_hardware_rev
> ├── dlvr_pll_busy
> ├── dlvr_rfim_enable
> └── dlvr_spread_spectrum_pct
>
> Attributes
> dlvr_freq_mhz (RO):
> Current DLVR PLL frequency in MHz.
>
> dlvr_freq_select (RW):
> Sets DLVR PLL clock frequency.
>
> dlvr_hardware_rev (RO):
> DLVR hardware revision.
>
> dlvr_pll_busy (RO):
> PLL can't accept frequency change when set.
>
> dlvr_rfim_enable (RW):
> 0: Disable RF frequency hopping, 1: Enable RF frequency hopping.
>
> dlvr_spread_spectrum_pct (RW)
> A write to this register updates the DLVR spread spectrum percent value.

How is this attribute going to be used by user space in practice?
Also should it be split like the frequency one (for consistency)?

> Signed-off-by: Srinivas Pandruvada <[email protected]>
> ---
> .../driver-api/thermal/intel_dptf.rst | 25 ++++++
> .../processor_thermal_device.c | 3 +-
> .../processor_thermal_device.h | 1 +
> .../processor_thermal_device_pci.c | 2 +-
> .../int340x_thermal/processor_thermal_rfim.c | 80 +++++++++++++++++--
> 5 files changed, 104 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/driver-api/thermal/intel_dptf.rst b/Documentation/driver-api/thermal/intel_dptf.rst
> index f5c193cccbda..da906deb6a7d 100644
> --- a/Documentation/driver-api/thermal/intel_dptf.rst
> +++ b/Documentation/driver-api/thermal/intel_dptf.rst
> @@ -264,6 +264,31 @@ DVFS attributes
> ``rfi_disable (RW)``
> Disable DDR rate change feature
>
> +DLVR attributes
> +
> +:file:`/sys/bus/pci/devices/0000\:00\:04.0/dlvr/`
> +
> +``dlvr_hardware_rev`` (RO)
> + DLVR hardware revision.
> +
> +``dlvr_freq_mhz`` (RO)
> + Current DLVR PLL frequency in MHz.
> +
> +``dlvr_freq_select`` (RW)
> + Sets DLVR PLL clock frequency. Once set, and enabled via
> + dlvr_rfim_enable, the dlvr_freq_mhz will show the current
> + DLVR PLL frequency.
> +
> +``dlvr_pll_busy`` (RO)
> + PLL can't accept frequency change when set.
> +
> +``dlvr_rfim_enable`` (RW)
> + 0: Disable RF frequency hopping, 1: Enable RF frequency hopping.
> +
> +``dlvr_spread_spectrum_pct`` (RW)
> + Sets DLVR spread spectrum percent value.
> +
> +
> DPTF Power supply and Battery Interface
> ----------------------------------------
>
> diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> index a1dc18be7609..3ca0a2f5937f 100644
> --- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> +++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> @@ -337,7 +337,8 @@ int proc_thermal_mmio_add(struct pci_dev *pdev,
> }
>
> if (feature_mask & PROC_THERMAL_FEATURE_FIVR ||
> - feature_mask & PROC_THERMAL_FEATURE_DVFS) {
> + feature_mask & PROC_THERMAL_FEATURE_DVFS ||
> + feature_mask & PROC_THERMAL_FEATURE_DLVR) {
> ret = proc_thermal_rfim_add(pdev, proc_priv);
> if (ret) {
> dev_err(&pdev->dev, "failed to add RFIM interface\n");
> diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> index 7d52fcff4937..7acaa8f1b896 100644
> --- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> +++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> @@ -60,6 +60,7 @@ struct rapl_mmio_regs {
> #define PROC_THERMAL_FEATURE_FIVR 0x02
> #define PROC_THERMAL_FEATURE_DVFS 0x04
> #define PROC_THERMAL_FEATURE_MBOX 0x08
> +#define PROC_THERMAL_FEATURE_DLVR 0x10
>
> #if IS_ENABLED(CONFIG_PROC_THERMAL_MMIO_RAPL)
> int proc_thermal_rapl_add(struct pci_dev *pdev, struct proc_thermal_device *proc_priv);
> diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
> index 90526f46c9b1..aaff67377250 100644
> --- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
> +++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
> @@ -352,7 +352,7 @@ static SIMPLE_DEV_PM_OPS(proc_thermal_pci_pm, proc_thermal_pci_suspend,
>
> static const struct pci_device_id proc_thermal_pci_ids[] = {
> { PCI_DEVICE_DATA(INTEL, ADL_THERMAL, PROC_THERMAL_FEATURE_RAPL | PROC_THERMAL_FEATURE_FIVR | PROC_THERMAL_FEATURE_DVFS | PROC_THERMAL_FEATURE_MBOX) },
> - { PCI_DEVICE_DATA(INTEL, MTLP_THERMAL, PROC_THERMAL_FEATURE_RAPL | PROC_THERMAL_FEATURE_FIVR | PROC_THERMAL_FEATURE_DVFS | PROC_THERMAL_FEATURE_MBOX) },
> + { PCI_DEVICE_DATA(INTEL, MTLP_THERMAL, PROC_THERMAL_FEATURE_RAPL | PROC_THERMAL_FEATURE_FIVR | PROC_THERMAL_FEATURE_DVFS | PROC_THERMAL_FEATURE_MBOX | PROC_THERMAL_FEATURE_DLVR) },
> { PCI_DEVICE_DATA(INTEL, RPL_THERMAL, PROC_THERMAL_FEATURE_RAPL | PROC_THERMAL_FEATURE_FIVR | PROC_THERMAL_FEATURE_DVFS | PROC_THERMAL_FEATURE_MBOX) },
> { },
> };
> diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
> index 92ed1213fe37..951a0869982a 100644
> --- a/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
> +++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
> @@ -39,6 +39,25 @@ static const struct mmio_reg tgl_fivr_mmio_regs[] = {
> { 1, 0x5A14, 2, 0x3, 1}, /* fivr_fffc_rev */
> };
>
> +static const char * const dlvr_strings[] = {
> + "dlvr_spread_spectrum_pct",
> + "dlvr_rfim_enable",
> + "dlvr_freq_select",
> + "dlvr_hardware_rev",
> + "dlvr_freq_mhz",
> + "dlvr_pll_busy",
> + NULL
> +};
> +
> +static const struct mmio_reg dlvr_mmio_regs[] = {
> + { 0, 0x15A08, 5, 0x1F, 0}, /* dlvr_spread_spectrum_pct */
> + { 0, 0x15A08, 1, 0x1, 7}, /* dlvr_rfim_enable */
> + { 0, 0x15A08, 12, 0xFFF, 8}, /* dlvr_freq_select */
> + { 1, 0x15A10, 2, 0x3, 30}, /* dlvr_hardware_rev */
> + { 1, 0x15A10, 16, 0xFFFF, 0}, /* dlvr_freq_mhz */
> + { 1, 0x15A10, 1, 0x1, 16}, /* dlvr_pll_busy */
> +};
> +
> /* These will represent sysfs attribute names */
> static const char * const dvfs_strings[] = {
> "rfi_restriction_run_busy",
> @@ -78,14 +97,16 @@ static ssize_t suffix##_show(struct device *dev,\
> int ret;\
> \
> proc_priv = pci_get_drvdata(pdev);\
> - if (table) {\
> + if (table == 1) {\
> match_strs = (const char **)dvfs_strings;\
> mmio_regs = adl_dvfs_mmio_regs;\
> - } else { \
> + } else if (table == 2) { \
> + match_strs = (const char **)dlvr_strings;\
> + mmio_regs = dlvr_mmio_regs;\
> + } else {\
> match_strs = (const char **)fivr_strings;\
> mmio_regs = tgl_fivr_mmio_regs;\
> } \
> - \
> ret = match_string(match_strs, -1, attr->attr.name);\
> if (ret < 0)\
> return ret;\
> @@ -109,10 +130,13 @@ static ssize_t suffix##_store(struct device *dev,\
> u32 mask;\
> \
> proc_priv = pci_get_drvdata(pdev);\
> - if (table) {\
> + if (table == 1) {\
> match_strs = (const char **)dvfs_strings;\
> mmio_regs = adl_dvfs_mmio_regs;\
> - } else { \
> + } else if (table == 2) { \
> + match_strs = (const char **)dlvr_strings;\
> + mmio_regs = dlvr_mmio_regs;\
> + } else {\
> match_strs = (const char **)fivr_strings;\
> mmio_regs = tgl_fivr_mmio_regs;\
> } \
> @@ -147,6 +171,39 @@ RFIM_STORE(spread_spectrum_clk_enable, 0)
> RFIM_STORE(rfi_vco_ref_code, 0)
> RFIM_STORE(fivr_fffc_rev, 0)
>
> +RFIM_SHOW(dlvr_spread_spectrum_pct, 2)
> +RFIM_SHOW(dlvr_hardware_rev, 2)
> +RFIM_SHOW(dlvr_freq_mhz, 2)
> +RFIM_SHOW(dlvr_pll_busy, 2)
> +RFIM_SHOW(dlvr_freq_select, 2)
> +RFIM_SHOW(dlvr_rfim_enable, 2)
> +
> +RFIM_STORE(dlvr_spread_spectrum_pct, 2)
> +RFIM_STORE(dlvr_rfim_enable, 2)
> +RFIM_STORE(dlvr_freq_select, 2)
> +
> +static DEVICE_ATTR_RW(dlvr_spread_spectrum_pct);
> +static DEVICE_ATTR_RW(dlvr_freq_select);
> +static DEVICE_ATTR_RO(dlvr_hardware_rev);
> +static DEVICE_ATTR_RO(dlvr_freq_mhz);
> +static DEVICE_ATTR_RO(dlvr_pll_busy);
> +static DEVICE_ATTR_RW(dlvr_rfim_enable);
> +
> +static struct attribute *dlvr_attrs[] = {
> + &dev_attr_dlvr_spread_spectrum_pct.attr,
> + &dev_attr_dlvr_freq_select.attr,
> + &dev_attr_dlvr_hardware_rev.attr,
> + &dev_attr_dlvr_freq_mhz.attr,
> + &dev_attr_dlvr_pll_busy.attr,
> + &dev_attr_dlvr_rfim_enable.attr,
> + NULL
> +};
> +
> +static const struct attribute_group dlvr_attribute_group = {
> + .attrs = dlvr_attrs,
> + .name = "dlvr"
> +};
> +
> static DEVICE_ATTR_RW(vco_ref_code_lo);
> static DEVICE_ATTR_RW(vco_ref_code_hi);
> static DEVICE_ATTR_RW(spread_spectrum_pct);
> @@ -277,12 +334,22 @@ int proc_thermal_rfim_add(struct pci_dev *pdev, struct proc_thermal_device *proc
> return ret;
> }
>
> + if (proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_DLVR) {
> + ret = sysfs_create_group(&pdev->dev.kobj, &dlvr_attribute_group);
> + if (ret)
> + return ret;
> + }
> +
> if (proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_DVFS) {
> ret = sysfs_create_group(&pdev->dev.kobj, &dvfs_attribute_group);
> if (ret && proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_FIVR) {
> sysfs_remove_group(&pdev->dev.kobj, &fivr_attribute_group);
> return ret;
> }
> + if (ret && proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_DLVR) {
> + sysfs_remove_group(&pdev->dev.kobj, &dlvr_attribute_group);
> + return ret;
> + }
> }
>
> return 0;
> @@ -296,6 +363,9 @@ void proc_thermal_rfim_remove(struct pci_dev *pdev)
> if (proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_FIVR)
> sysfs_remove_group(&pdev->dev.kobj, &fivr_attribute_group);
>
> + if (proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_DLVR)
> + sysfs_remove_group(&pdev->dev.kobj, &dlvr_attribute_group);
> +
> if (proc_priv->mmio_feature_mask & PROC_THERMAL_FEATURE_DVFS)
> sysfs_remove_group(&pdev->dev.kobj, &dvfs_attribute_group);
> }
> --
> 2.34.1
>

2023-04-04 16:44:44

by srinivas pandruvada

[permalink] [raw]
Subject: Re: [PATCH] thermal/drivers/intel/int340x: Add DLVR support

On Mon, 2023-04-03 at 20:37 +0200, Rafael J. Wysocki wrote:
> On Fri, Mar 31, 2023 at 6:53 PM Srinivas Pandruvada
> <[email protected]> wrote:
> >
> > Add support for DLVR (Digital Linear Voltage Regulator) attributes,
> > which can be used to control RFIM.
> > Here instead of "fivr" another directory "dlvr" is created with
> > DLVR
> > attributes:
> >
> > /sys/bus/pci/devices/0000:00:04.0/dlvr
> > ├── dlvr_freq_mhz
> > ├── dlvr_freq_select
> > ├── dlvr_hardware_rev
> > ├── dlvr_pll_busy
> > ├── dlvr_rfim_enable
> > └── dlvr_spread_spectrum_pct
> >
> > Attributes
> > dlvr_freq_mhz (RO):
> > Current DLVR PLL frequency in MHz.
> >
> > dlvr_freq_select (RW):
> > Sets DLVR PLL clock frequency.
> >
> > dlvr_hardware_rev (RO):
> > DLVR hardware revision.
> >
> > dlvr_pll_busy (RO):
> > PLL can't accept frequency change when set.
> >
> > dlvr_rfim_enable (RW):
> > 0: Disable RF frequency hopping, 1: Enable RF frequency hopping.
> >
> > dlvr_spread_spectrum_pct (RW)
> > A write to this register updates the DLVR spread spectrum percent
> > value.
>
> How is this attribute going to be used by user space in practice?
Spread spectrum percent helps to reduce the DLVR clock noise to meet
regulatory compliance. This spreading % increases bandwidth of signal
transmission and hence reduces the effects of interference, noise, and
signal fading.


> Also should it be split like the frequency one (for consistency)?
This is a RW field and is applied immediately unlike frequency, where
it is two step process. First you specify and enable and then see the
effect. So they are two fields.

Thanks,
Srinivas

>
> > Signed-off-by: Srinivas Pandruvada
> > <[email protected]>
> > ---
> >  .../driver-api/thermal/intel_dptf.rst         | 25 ++++++
> >  .../processor_thermal_device.c                |  3 +-
> >  .../processor_thermal_device.h                |  1 +
> >  .../processor_thermal_device_pci.c            |  2 +-
> >  .../int340x_thermal/processor_thermal_rfim.c  | 80
> > +++++++++++++++++--
> >  5 files changed, 104 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/driver-api/thermal/intel_dptf.rst
> > b/Documentation/driver-api/thermal/intel_dptf.rst
> > index f5c193cccbda..da906deb6a7d 100644
> > --- a/Documentation/driver-api/thermal/intel_dptf.rst
> > +++ b/Documentation/driver-api/thermal/intel_dptf.rst
> > @@ -264,6 +264,31 @@ DVFS attributes
> >  ``rfi_disable (RW)``
> >         Disable DDR rate change feature
> >
> > +DLVR attributes
> > +
> > +:file:`/sys/bus/pci/devices/0000\:00\:04.0/dlvr/`
> > +
> > +``dlvr_hardware_rev`` (RO)
> > +       DLVR hardware revision.
> > +
> > +``dlvr_freq_mhz`` (RO)
> > +       Current DLVR PLL frequency in MHz.
> > +
> > +``dlvr_freq_select`` (RW)
> > +       Sets DLVR PLL clock frequency. Once set, and enabled via
> > +       dlvr_rfim_enable, the dlvr_freq_mhz will show the current
> > +       DLVR PLL frequency.
> > +
> > +``dlvr_pll_busy`` (RO)
> > +       PLL can't accept frequency change when set.
> > +
> > +``dlvr_rfim_enable`` (RW)
> > +       0: Disable RF frequency hopping, 1: Enable RF frequency
> > hopping.
> > +
> > +``dlvr_spread_spectrum_pct`` (RW)
> > +       Sets DLVR spread spectrum percent value.
> > +
> > +
> >  DPTF Power supply and Battery Interface
> >  ----------------------------------------
> >
> > diff --git
> > a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> > b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> > index a1dc18be7609..3ca0a2f5937f 100644
> > ---
> > a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> > +++
> > b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.c
> > @@ -337,7 +337,8 @@ int proc_thermal_mmio_add(struct pci_dev *pdev,
> >         }
> >
> >         if (feature_mask & PROC_THERMAL_FEATURE_FIVR ||
> > -           feature_mask & PROC_THERMAL_FEATURE_DVFS) {
> > +           feature_mask & PROC_THERMAL_FEATURE_DVFS ||
> > +           feature_mask & PROC_THERMAL_FEATURE_DLVR) {
> >                 ret = proc_thermal_rfim_add(pdev, proc_priv);
> >                 if (ret) {
> >                         dev_err(&pdev->dev, "failed to add RFIM
> > interface\n");
> > diff --git
> > a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> > b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> > index 7d52fcff4937..7acaa8f1b896 100644
> > ---
> > a/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> > +++
> > b/drivers/thermal/intel/int340x_thermal/processor_thermal_device.h
> > @@ -60,6 +60,7 @@ struct rapl_mmio_regs {
> >  #define PROC_THERMAL_FEATURE_FIVR      0x02
> >  #define PROC_THERMAL_FEATURE_DVFS      0x04
> >  #define PROC_THERMAL_FEATURE_MBOX      0x08
> > +#define PROC_THERMAL_FEATURE_DLVR      0x10
> >
> >  #if IS_ENABLED(CONFIG_PROC_THERMAL_MMIO_RAPL)
> >  int proc_thermal_rapl_add(struct pci_dev *pdev, struct
> > proc_thermal_device *proc_priv);
> > diff --git
> > a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pc
> > i.c
> > b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pc
> > i.c
> > index 90526f46c9b1..aaff67377250 100644
> > ---
> > a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pc
> > i.c
> > +++
> > b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pc
> > i.c
> > @@ -352,7 +352,7 @@ static SIMPLE_DEV_PM_OPS(proc_thermal_pci_pm,
> > proc_thermal_pci_suspend,
> >
> >  static const struct pci_device_id proc_thermal_pci_ids[] = {
> >         { PCI_DEVICE_DATA(INTEL, ADL_THERMAL,
> > PROC_THERMAL_FEATURE_RAPL | PROC_THERMAL_FEATURE_FIVR |
> > PROC_THERMAL_FEATURE_DVFS | PROC_THERMAL_FEATURE_MBOX) },
> > -       { PCI_DEVICE_DATA(INTEL, MTLP_THERMAL,
> > PROC_THERMAL_FEATURE_RAPL | PROC_THERMAL_FEATURE_FIVR |
> > PROC_THERMAL_FEATURE_DVFS | PROC_THERMAL_FEATURE_MBOX) },
> > +       { PCI_DEVICE_DATA(INTEL, MTLP_THERMAL,
> > PROC_THERMAL_FEATURE_RAPL | PROC_THERMAL_FEATURE_FIVR |
> > PROC_THERMAL_FEATURE_DVFS | PROC_THERMAL_FEATURE_MBOX |
> > PROC_THERMAL_FEATURE_DLVR) },
> >         { PCI_DEVICE_DATA(INTEL, RPL_THERMAL,
> > PROC_THERMAL_FEATURE_RAPL | PROC_THERMAL_FEATURE_FIVR |
> > PROC_THERMAL_FEATURE_DVFS | PROC_THERMAL_FEATURE_MBOX) },
> >         { },
> >  };
> > diff --git
> > a/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
> > b/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
> > index 92ed1213fe37..951a0869982a 100644
> > ---
> > a/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
> > +++
> > b/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
> > @@ -39,6 +39,25 @@ static const struct mmio_reg
> > tgl_fivr_mmio_regs[] = {
> >         { 1, 0x5A14, 2, 0x3, 1}, /* fivr_fffc_rev */
> >  };
> >
> > +static const char * const dlvr_strings[] = {
> > +       "dlvr_spread_spectrum_pct",
> > +       "dlvr_rfim_enable",
> > +       "dlvr_freq_select",
> > +       "dlvr_hardware_rev",
> > +       "dlvr_freq_mhz",
> > +       "dlvr_pll_busy",
> > +       NULL
> > +};
> > +
> > +static const struct mmio_reg dlvr_mmio_regs[] = {
> > +       { 0, 0x15A08, 5, 0x1F, 0}, /* dlvr_spread_spectrum_pct */
> > +       { 0, 0x15A08, 1, 0x1, 7}, /* dlvr_rfim_enable */
> > +       { 0, 0x15A08, 12, 0xFFF, 8}, /* dlvr_freq_select */
> > +       { 1, 0x15A10, 2, 0x3, 30}, /* dlvr_hardware_rev */
> > +       { 1, 0x15A10, 16, 0xFFFF, 0}, /* dlvr_freq_mhz */
> > +       { 1, 0x15A10, 1, 0x1, 16}, /* dlvr_pll_busy */
> > +};
> > +
> >  /* These will represent sysfs attribute names */
> >  static const char * const dvfs_strings[] = {
> >         "rfi_restriction_run_busy",
> > @@ -78,14 +97,16 @@ static ssize_t suffix##_show(struct device
> > *dev,\
> >         int ret;\
> >  \
> >         proc_priv = pci_get_drvdata(pdev);\
> > -       if (table) {\
> > +       if (table == 1) {\
> >                 match_strs = (const char **)dvfs_strings;\
> >                 mmio_regs = adl_dvfs_mmio_regs;\
> > -       } else { \
> > +       } else if (table == 2) { \
> > +               match_strs = (const char **)dlvr_strings;\
> > +               mmio_regs = dlvr_mmio_regs;\
> > +       } else {\
> >                 match_strs = (const char **)fivr_strings;\
> >                 mmio_regs = tgl_fivr_mmio_regs;\
> >         } \
> > -       \
> >         ret = match_string(match_strs, -1, attr->attr.name);\
> >         if (ret < 0)\
> >                 return ret;\
> > @@ -109,10 +130,13 @@ static ssize_t suffix##_store(struct device
> > *dev,\
> >         u32 mask;\
> >  \
> >         proc_priv = pci_get_drvdata(pdev);\
> > -       if (table) {\
> > +       if (table == 1) {\
> >                 match_strs = (const char **)dvfs_strings;\
> >                 mmio_regs = adl_dvfs_mmio_regs;\
> > -       } else { \
> > +       } else if (table == 2) { \
> > +               match_strs = (const char **)dlvr_strings;\
> > +               mmio_regs = dlvr_mmio_regs;\
> > +       } else {\
> >                 match_strs = (const char **)fivr_strings;\
> >                 mmio_regs = tgl_fivr_mmio_regs;\
> >         } \
> > @@ -147,6 +171,39 @@ RFIM_STORE(spread_spectrum_clk_enable, 0)
> >  RFIM_STORE(rfi_vco_ref_code, 0)
> >  RFIM_STORE(fivr_fffc_rev, 0)
> >
> > +RFIM_SHOW(dlvr_spread_spectrum_pct, 2)
> > +RFIM_SHOW(dlvr_hardware_rev, 2)
> > +RFIM_SHOW(dlvr_freq_mhz, 2)
> > +RFIM_SHOW(dlvr_pll_busy, 2)
> > +RFIM_SHOW(dlvr_freq_select, 2)
> > +RFIM_SHOW(dlvr_rfim_enable, 2)
> > +
> > +RFIM_STORE(dlvr_spread_spectrum_pct, 2)
> > +RFIM_STORE(dlvr_rfim_enable, 2)
> > +RFIM_STORE(dlvr_freq_select, 2)
> > +
> > +static DEVICE_ATTR_RW(dlvr_spread_spectrum_pct);
> > +static DEVICE_ATTR_RW(dlvr_freq_select);
> > +static DEVICE_ATTR_RO(dlvr_hardware_rev);
> > +static DEVICE_ATTR_RO(dlvr_freq_mhz);
> > +static DEVICE_ATTR_RO(dlvr_pll_busy);
> > +static DEVICE_ATTR_RW(dlvr_rfim_enable);
> > +
> > +static struct attribute *dlvr_attrs[] = {
> > +       &dev_attr_dlvr_spread_spectrum_pct.attr,
> > +       &dev_attr_dlvr_freq_select.attr,
> > +       &dev_attr_dlvr_hardware_rev.attr,
> > +       &dev_attr_dlvr_freq_mhz.attr,
> > +       &dev_attr_dlvr_pll_busy.attr,
> > +       &dev_attr_dlvr_rfim_enable.attr,
> > +       NULL
> > +};
> > +
> > +static const struct attribute_group dlvr_attribute_group = {
> > +       .attrs = dlvr_attrs,
> > +       .name = "dlvr"
> > +};
> > +
> >  static DEVICE_ATTR_RW(vco_ref_code_lo);
> >  static DEVICE_ATTR_RW(vco_ref_code_hi);
> >  static DEVICE_ATTR_RW(spread_spectrum_pct);
> > @@ -277,12 +334,22 @@ int proc_thermal_rfim_add(struct pci_dev
> > *pdev, struct proc_thermal_device *proc
> >                         return ret;
> >         }
> >
> > +       if (proc_priv->mmio_feature_mask &
> > PROC_THERMAL_FEATURE_DLVR) {
> > +               ret = sysfs_create_group(&pdev->dev.kobj,
> > &dlvr_attribute_group);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> >         if (proc_priv->mmio_feature_mask &
> > PROC_THERMAL_FEATURE_DVFS) {
> >                 ret = sysfs_create_group(&pdev->dev.kobj,
> > &dvfs_attribute_group);
> >                 if (ret && proc_priv->mmio_feature_mask &
> > PROC_THERMAL_FEATURE_FIVR) {
> >                         sysfs_remove_group(&pdev->dev.kobj,
> > &fivr_attribute_group);
> >                         return ret;
> >                 }
> > +               if (ret && proc_priv->mmio_feature_mask &
> > PROC_THERMAL_FEATURE_DLVR) {
> > +                       sysfs_remove_group(&pdev->dev.kobj,
> > &dlvr_attribute_group);
> > +                       return ret;
> > +               }
> >         }
> >
> >         return 0;
> > @@ -296,6 +363,9 @@ void proc_thermal_rfim_remove(struct pci_dev
> > *pdev)
> >         if (proc_priv->mmio_feature_mask &
> > PROC_THERMAL_FEATURE_FIVR)
> >                 sysfs_remove_group(&pdev->dev.kobj,
> > &fivr_attribute_group);
> >
> > +       if (proc_priv->mmio_feature_mask &
> > PROC_THERMAL_FEATURE_DLVR)
> > +               sysfs_remove_group(&pdev->dev.kobj,
> > &dlvr_attribute_group);
> > +
> >         if (proc_priv->mmio_feature_mask &
> > PROC_THERMAL_FEATURE_DVFS)
> >                 sysfs_remove_group(&pdev->dev.kobj,
> > &dvfs_attribute_group);
> >  }
> > --
> > 2.34.1
> >

2023-04-04 16:51:05

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH] thermal/drivers/intel/int340x: Add DLVR support

On Tue, Apr 4, 2023 at 6:39 PM srinivas pandruvada
<[email protected]> wrote:
>
> On Mon, 2023-04-03 at 20:37 +0200, Rafael J. Wysocki wrote:
> > On Fri, Mar 31, 2023 at 6:53 PM Srinivas Pandruvada
> > <[email protected]> wrote:
> > >
> > > Add support for DLVR (Digital Linear Voltage Regulator) attributes,
> > > which can be used to control RFIM.
> > > Here instead of "fivr" another directory "dlvr" is created with
> > > DLVR
> > > attributes:
> > >
> > > /sys/bus/pci/devices/0000:00:04.0/dlvr
> > > ├── dlvr_freq_mhz
> > > ├── dlvr_freq_select
> > > ├── dlvr_hardware_rev
> > > ├── dlvr_pll_busy
> > > ├── dlvr_rfim_enable
> > > └── dlvr_spread_spectrum_pct
> > >
> > > Attributes
> > > dlvr_freq_mhz (RO):
> > > Current DLVR PLL frequency in MHz.
> > >
> > > dlvr_freq_select (RW):
> > > Sets DLVR PLL clock frequency.
> > >
> > > dlvr_hardware_rev (RO):
> > > DLVR hardware revision.
> > >
> > > dlvr_pll_busy (RO):
> > > PLL can't accept frequency change when set.
> > >
> > > dlvr_rfim_enable (RW):
> > > 0: Disable RF frequency hopping, 1: Enable RF frequency hopping.
> > >
> > > dlvr_spread_spectrum_pct (RW)
> > > A write to this register updates the DLVR spread spectrum percent
> > > value.
> >
> > How is this attribute going to be used by user space in practice?
>
> Spread spectrum percent helps to reduce the DLVR clock noise to meet
> regulatory compliance. This spreading % increases bandwidth of signal
> transmission and hence reduces the effects of interference, noise, and
> signal fading.

The above information should be added to the documentation I think.

Still, I would like to know when user space is going to write to it
and how it is going to find out what value to write.

> > Also should it be split like the frequency one (for consistency)?
>
> This is a RW field and is applied immediately unlike frequency, where
> it is two step process. First you specify and enable and then see the
> effect. So they are two fields.

I was talking about dlvr_freq_mhz (RO) and dlvr_freq_select (RW). I'm
not sure how the above is related to them TBH.

2023-04-04 18:02:36

by srinivas pandruvada

[permalink] [raw]
Subject: Re: [PATCH] thermal/drivers/intel/int340x: Add DLVR support

On Tue, 2023-04-04 at 18:46 +0200, Rafael J. Wysocki wrote:
> On Tue, Apr 4, 2023 at 6:39 PM srinivas pandruvada
> <[email protected]> wrote:
> >
> > On Mon, 2023-04-03 at 20:37 +0200, Rafael J. Wysocki wrote:
> > > On Fri, Mar 31, 2023 at 6:53 PM Srinivas Pandruvada
> > > <[email protected]> wrote:
> > > >
> > > > Add support for DLVR (Digital Linear Voltage Regulator)
> > > > attributes,
> > > > which can be used to control RFIM.
> > > > Here instead of "fivr" another directory "dlvr" is created with
> > > > DLVR
> > > > attributes:
> > > >
> > > > /sys/bus/pci/devices/0000:00:04.0/dlvr
> > > > ├── dlvr_freq_mhz
> > > > ├── dlvr_freq_select
> > > > ├── dlvr_hardware_rev
> > > > ├── dlvr_pll_busy
> > > > ├── dlvr_rfim_enable
> > > > └── dlvr_spread_spectrum_pct
> > > >
> > > > Attributes
> > > > dlvr_freq_mhz (RO):
> > > > Current DLVR PLL frequency in MHz.
> > > >
> > > > dlvr_freq_select (RW):
> > > > Sets DLVR PLL clock frequency.
> > > >
> > > > dlvr_hardware_rev (RO):
> > > > DLVR hardware revision.
> > > >
> > > > dlvr_pll_busy (RO):
> > > > PLL can't accept frequency change when set.
> > > >
> > > > dlvr_rfim_enable (RW):
> > > > 0: Disable RF frequency hopping, 1: Enable RF frequency
> > > > hopping.
> > > >
> > > > dlvr_spread_spectrum_pct (RW)
> > > > A write to this register updates the DLVR spread spectrum
> > > > percent
> > > > value.
> > >
> > > How is this attribute going to be used by user space in practice?
> >
> > Spread spectrum percent helps to reduce the DLVR clock noise to
> > meet
> > regulatory compliance. This spreading % increases bandwidth of
> > signal
> > transmission and hence reduces the effects of interference, noise,
> > and
> > signal fading.
>
> The above information should be added to the documentation I think.
>
> Still, I would like to know when user space is going to write to it
> and how it is going to find out what value to write.
As specified in the
https://docs.kernel.org/driver-api/thermal/intel_dptf.html
This is all related to reduce interference with WiFi radio frequencies.

The algorithm should be read current dlvr_freq_mhz,
dlvr_spread_spectrum_pct, current WiFi frequency (channel has a fix
freq), find the error in WiFi frame error rates (From WiFi module), and
do small adjustment +- to dlvr_freq. While changing the dlvr
frequencies you may induce more interference so you spread the signal
to reduce S/N ratio using this percent knob.

>
> > > Also should it be split like the frequency one (for consistency)?
> >
> > This is a RW field and is applied immediately unlike frequency,
> > where
> > it is two step process. First you specify and enable and then see
> > the
> > effect. So they are two fields.
>
> I was talking about dlvr_freq_mhz (RO) and dlvr_freq_select (RW). 
> I'm
> not sure how the above is related to them TBH.
The frequency is the base signal to which interference must be reduced.
A good explanation for this technique
https://www.eetimes.com/tutorial-on-spread-spectrum-technology/

Thanks,
Srinivas

2023-04-06 19:02:27

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH] thermal/drivers/intel/int340x: Add DLVR support

On Tue, Apr 4, 2023 at 7:55 PM srinivas pandruvada
<[email protected]> wrote:
>
> On Tue, 2023-04-04 at 18:46 +0200, Rafael J. Wysocki wrote:
> > On Tue, Apr 4, 2023 at 6:39 PM srinivas pandruvada
> > <[email protected]> wrote:
> > >
> > > On Mon, 2023-04-03 at 20:37 +0200, Rafael J. Wysocki wrote:
> > > > On Fri, Mar 31, 2023 at 6:53 PM Srinivas Pandruvada
> > > > <[email protected]> wrote:
> > > > >
> > > > > Add support for DLVR (Digital Linear Voltage Regulator)
> > > > > attributes,
> > > > > which can be used to control RFIM.
> > > > > Here instead of "fivr" another directory "dlvr" is created with
> > > > > DLVR
> > > > > attributes:
> > > > >
> > > > > /sys/bus/pci/devices/0000:00:04.0/dlvr
> > > > > ├── dlvr_freq_mhz
> > > > > ├── dlvr_freq_select
> > > > > ├── dlvr_hardware_rev
> > > > > ├── dlvr_pll_busy
> > > > > ├── dlvr_rfim_enable
> > > > > └── dlvr_spread_spectrum_pct
> > > > >
> > > > > Attributes
> > > > > dlvr_freq_mhz (RO):
> > > > > Current DLVR PLL frequency in MHz.
> > > > >
> > > > > dlvr_freq_select (RW):
> > > > > Sets DLVR PLL clock frequency.
> > > > >
> > > > > dlvr_hardware_rev (RO):
> > > > > DLVR hardware revision.
> > > > >
> > > > > dlvr_pll_busy (RO):
> > > > > PLL can't accept frequency change when set.
> > > > >
> > > > > dlvr_rfim_enable (RW):
> > > > > 0: Disable RF frequency hopping, 1: Enable RF frequency
> > > > > hopping.
> > > > >
> > > > > dlvr_spread_spectrum_pct (RW)
> > > > > A write to this register updates the DLVR spread spectrum
> > > > > percent
> > > > > value.
> > > >
> > > > How is this attribute going to be used by user space in practice?
> > >
> > > Spread spectrum percent helps to reduce the DLVR clock noise to
> > > meet
> > > regulatory compliance. This spreading % increases bandwidth of
> > > signal
> > > transmission and hence reduces the effects of interference, noise,
> > > and
> > > signal fading.
> >
> > The above information should be added to the documentation I think.
> >
> > Still, I would like to know when user space is going to write to it
> > and how it is going to find out what value to write.
> As specified in the
> https://docs.kernel.org/driver-api/thermal/intel_dptf.html
> This is all related to reduce interference with WiFi radio frequencies.

So the first two paragraphs in the "DPTF Processor thermal RFIM
interface" section need to be updated to mention/cover DLVR. Then, it
will all be clear (at least AFAIAC).

> The algorithm should be read current dlvr_freq_mhz,
> dlvr_spread_spectrum_pct, current WiFi frequency (channel has a fix
> freq), find the error in WiFi frame error rates (From WiFi module), and
> do small adjustment +- to dlvr_freq. While changing the dlvr
> frequencies you may induce more interference so you spread the signal
> to reduce S/N ratio using this percent knob.
>
> >
> > > > Also should it be split like the frequency one (for consistency)?
> > >
> > > This is a RW field and is applied immediately unlike frequency,
> > > where
> > > it is two step process. First you specify and enable and then see
> > > the
> > > effect. So they are two fields.
> >
> > I was talking about dlvr_freq_mhz (RO) and dlvr_freq_select (RW).
> > I'm
> > not sure how the above is related to them TBH.
> The frequency is the base signal to which interference must be reduced.
> A good explanation for this technique
> https://www.eetimes.com/tutorial-on-spread-spectrum-technology/

Yes, I know what "spread spectrum" means.