LinuxLists.cc - [PATCH V4 0/8] Support Wifi RFI interference mitigation feature

[permalink] [raw]

Subject: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

From: Mario Limonciello <[email protected]>

Due to electrical and mechanical constraints in certain platform designs
there may be likely interference of relatively high-powered harmonics of
the (G-)DDR memory clocks with local radio module frequency bands used
by Wifi 6/6e/7.

To mitigate this, AMD has introduced an ACPI based mechanism that
devices can use to notify active use of particular frequencies so
that devices can make relative internal adjustments as necessary
to avoid this resonance.

In order for a device to support this, the expected flow for device
driver or subsystems:

Drivers/subsystems contributing frequencies:

1) During probe, check `wbrf_supported_producer` to see if WBRF supported
for the device.
2) If adding frequencies, then call `wbrf_add_exclusion` with the
start and end ranges of the frequencies.
3) If removing frequencies, then call `wbrf_remove_exclusion` with
start and end ranges of the frequencies.

Drivers/subsystems responding to frequencies:

1) During probe, check `wbrf_supported_consumer` to see if WBRF is supported
for the device.
2) Call the `wbrf_retrieve_exclusions` to retrieve the current
exclusions on receiving an ACPI notification for a new frequency
change.

Signed-off-by: Mario Limonciello <[email protected]>
Co-developed-by: Evan Quan <[email protected]>
Signed-off-by: Evan Quan <[email protected]>
--
v1->v2:
- move those wlan specific implementations to net/mac80211(Mario)
---
drivers/acpi/Kconfig | 7 ++
drivers/acpi/Makefile | 2 +
drivers/acpi/acpi_wbrf.c | 215 +++++++++++++++++++++++++++++++++++++++
include/linux/wbrf.h | 55 ++++++++++
4 files changed, 279 insertions(+)
create mode 100644 drivers/acpi/acpi_wbrf.c
create mode 100644 include/linux/wbrf.h

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index ccbeab9500ec..0276c1487fa2 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -611,3 +611,10 @@ config X86_PM_TIMER

You should nearly always say Y here because many modern
systems require this timer.
+
+config ACPI_WBRF
+ bool "ACPI Wifi band RF mitigation mechanism"
+ help
+ Wifi band RF mitigation mechanism allows multiple drivers from
+ different domains to notify the frequencies in use so that hardware
+ can be reconfigured to avoid harmonic conflicts.
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index feb36c0b9446..14863b0c619f 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -131,3 +131,5 @@ obj-y += dptf/
obj-$(CONFIG_ARM64) += arm64/

obj-$(CONFIG_ACPI_VIOT) += viot.o
+
+obj-$(CONFIG_ACPI_WBRF) += acpi_wbrf.o
diff --git a/drivers/acpi/acpi_wbrf.c b/drivers/acpi/acpi_wbrf.c
new file mode 100644
index 000000000000..8c275998ac29
--- /dev/null
+++ b/drivers/acpi/acpi_wbrf.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD Wifi Band Exclusion Interface
+ * Copyright (C) 2023 Advanced Micro Devices
+ *
+ */
+
+#include <linux/wbrf.h>
+
+/* functions */
+#define WBRF_RECORD 0x1
+#define WBRF_RETRIEVE 0x2
+
+/* record actions */
+#define WBRF_RECORD_ADD 0x0
+#define WBRF_RECORD_REMOVE 0x1
+
+#define WBRF_REVISION 0x1
+
+static const guid_t wifi_acpi_dsm_guid =
+ GUID_INIT(0x7b7656cf, 0xdc3d, 0x4c1c,
+ 0x83, 0xe9, 0x66, 0xe7, 0x21, 0xde, 0x30, 0x70);
+
+static int wbrf_dsm(struct acpi_device *adev, u8 fn,
+ union acpi_object *argv4,
+ union acpi_object **out)
+{
+ union acpi_object *obj;
+ int rc;
+
+ obj = acpi_evaluate_dsm(adev->handle, &wifi_acpi_dsm_guid,
+ WBRF_REVISION, fn, argv4);
+ if (!obj)
+ return -ENXIO;
+
+ switch (obj->type) {
+ case ACPI_TYPE_BUFFER:
+ if (!*out) {
+ rc = -EINVAL;
+ break;
+ }
+ *out = obj;
+ return 0;
+
+ case ACPI_TYPE_INTEGER:
+ rc = obj->integer.value ? -EINVAL : 0;
+ break;
+ default:
+ rc = -EOPNOTSUPP;
+ }
+ ACPI_FREE(obj);
+
+ return rc;
+}
+
+static int wbrf_record(struct acpi_device *adev, uint8_t action,
+ struct wbrf_ranges_in *in)
+{
+ union acpi_object *argv4;
+ uint32_t num_of_ranges = 0;
+ uint32_t arg_idx = 0;
+ uint32_t loop_idx;
+ int ret;
+
+ if (!in)
+ return -EINVAL;
+
+ for (loop_idx = 0; loop_idx < ARRAY_SIZE(in->band_list);
+ loop_idx++)
+ if (in->band_list[loop_idx].start &&
+ in->band_list[loop_idx].end)
+ num_of_ranges++;
+
+ argv4 = kzalloc(sizeof(*argv4) * (2 * num_of_ranges + 2 + 1), GFP_KERNEL);
+ if (!argv4)
+ return -ENOMEM;
+
+ argv4[arg_idx].package.type = ACPI_TYPE_PACKAGE;
+ argv4[arg_idx].package.count = 2 + 2 * num_of_ranges;
+ argv4[arg_idx++].package.elements = &argv4[1];
+ argv4[arg_idx].integer.type = ACPI_TYPE_INTEGER;
+ argv4[arg_idx++].integer.value = num_of_ranges;
+ argv4[arg_idx].integer.type = ACPI_TYPE_INTEGER;
+ argv4[arg_idx++].integer.value = action;
+
+ for (loop_idx = 0; loop_idx < ARRAY_SIZE(in->band_list);
+ loop_idx++) {
+ if (!in->band_list[loop_idx].start ||
+ !in->band_list[loop_idx].end)
+ continue;
+
+ argv4[arg_idx].integer.type = ACPI_TYPE_INTEGER;
+ argv4[arg_idx++].integer.value = in->band_list[loop_idx].start;
+ argv4[arg_idx].integer.type = ACPI_TYPE_INTEGER;
+ argv4[arg_idx++].integer.value = in->band_list[loop_idx].end;
+ }
+
+ ret = wbrf_dsm(adev, WBRF_RECORD, argv4, NULL);
+
+ kfree(argv4);
+
+ return ret;
+}
+
+int wbrf_add_exclusion(struct acpi_device *adev,
+ struct wbrf_ranges_in *in)
+{
+ return wbrf_record(adev, WBRF_RECORD_ADD, in);
+}
+EXPORT_SYMBOL_GPL(wbrf_add_exclusion);
+
+int wbrf_remove_exclusion(struct acpi_device *adev,
+ struct wbrf_ranges_in *in)
+{
+ return wbrf_record(adev, WBRF_RECORD_REMOVE, in);
+}
+EXPORT_SYMBOL_GPL(wbrf_remove_exclusion);
+
+bool wbrf_supported_producer(struct acpi_device *adev)
+{
+ return acpi_check_dsm(adev->handle, &wifi_acpi_dsm_guid,
+ WBRF_REVISION,
+ (1ULL << WBRF_RECORD) | (1ULL << WBRF_RETRIEVE));
+}
+EXPORT_SYMBOL_GPL(wbrf_supported_producer);
+
+static union acpi_object *
+acpi_evaluate_wbrf(acpi_handle handle, u64 rev, u64 func)
+{
+ acpi_status ret;
+ struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
+ union acpi_object params[4];
+ struct acpi_object_list input = {
+ .count = 4,
+ .pointer = params,
+ };
+
+ params[0].type = ACPI_TYPE_INTEGER;
+ params[0].integer.value = rev;
+ params[1].type = ACPI_TYPE_INTEGER;
+ params[1].integer.value = func;
+ params[2].type = ACPI_TYPE_PACKAGE;
+ params[2].package.count = 0;
+ params[2].package.elements = NULL;
+ params[3].type = ACPI_TYPE_STRING;
+ params[3].string.length = 0;
+ params[3].string.pointer= NULL;
+
+ ret = acpi_evaluate_object(handle, "WBRF", &input, &buf);
+ if (ACPI_SUCCESS(ret))
+ return (union acpi_object *)buf.pointer;
+
+ if (ret != AE_NOT_FOUND)
+ acpi_handle_warn(handle,
+ "failed to evaluate WBRF(0x%x)\n", ret);
+
+ return NULL;
+}
+
+static bool check_acpi_wbrf(acpi_handle handle, u64 rev, u64 funcs)
+{
+ int i;
+ u64 mask = 0;
+ union acpi_object *obj;
+
+ if (funcs == 0)
+ return false;
+
+ obj = acpi_evaluate_wbrf(handle, rev, 0);
+ if (!obj)
+ return false;
+
+ if (obj->type != ACPI_TYPE_BUFFER)
+ return false;
+
+ for (i = 0; i < obj->buffer.length && i < 8; i++)
+ mask |= (((u64)obj->buffer.pointer[i]) << (i * 8));
+ ACPI_FREE(obj);
+
+ /*
+ * Bit 0 indicates whether there's support for any functions other than
+ * function 0.
+ */
+ if ((mask & 0x1) && (mask & funcs) == funcs)
+ return true;
+
+ return false;
+}
+
+bool wbrf_supported_consumer(struct acpi_device *adev)
+{
+ return check_acpi_wbrf(adev->handle,
+ WBRF_REVISION,
+ 1ULL << WBRF_RETRIEVE);
+}
+EXPORT_SYMBOL_GPL(wbrf_supported_consumer);
+
+int wbrf_retrieve_exclusions(struct acpi_device *adev,
+ struct wbrf_ranges_out *exclusions_out)
+{
+ union acpi_object *obj;
+
+ obj = acpi_evaluate_wbrf(adev->handle,
+ WBRF_REVISION,
+ WBRF_RETRIEVE);
+ if (!obj)
+ return -EINVAL;
+
+ memcpy(exclusions_out, obj->buffer.pointer, obj->buffer.length);
+
+ ACPI_FREE(obj);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(wbrf_retrieve_exclusions);
diff --git a/include/linux/wbrf.h b/include/linux/wbrf.h
new file mode 100644
index 000000000000..e4c99b69f1d2
--- /dev/null
+++ b/include/linux/wbrf.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * AMD Wifi Band Exclusion Interface
+ * Copyright (C) 2023 Advanced Micro Devices
+ */
+
+#ifndef _LINUX_WBRF_H
+#define _LINUX_WBRF_H
+
+#include <linux/acpi.h>
+
+/* Maximum number of wbrf ranges */
+#define MAX_NUM_OF_WBRF_RANGES 11
+
+struct exclusion_range {
+ /* start and end point of the frequency range in Hz */
+ uint64_t start;
+ uint64_t end;
+};
+
+struct wbrf_ranges_in {
+ /* valid entry: `start` and `end` filled with non-zero values */
+ struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES];
+};
+
+struct wbrf_ranges_out {
+ uint32_t num_of_ranges;
+ struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES];
+} __attribute__((packed));
+
+/**
+ * APIs needed by drivers/subsystems for contributing frequencies:
+ * During probe, check `wbrf_supported_producer` to see if WBRF is supported.
+ * If adding frequencies, then call `wbrf_add_exclusion` with the
+ * start and end points specified for the frequency ranges added.
+ * If removing frequencies, then call `wbrf_remove_exclusion` with
+ * start and end points specified for the frequency ranges added.
+ */
+bool wbrf_supported_producer(struct acpi_device *adev);
+int wbrf_add_exclusion(struct acpi_device *adev,
+ struct wbrf_ranges_in *in);
+int wbrf_remove_exclusion(struct acpi_device *adev,
+ struct wbrf_ranges_in *in);
+
+/**
+ * APIs needed by drivers/subsystems responding to frequencies:
+ * During probe, check `wbrf_supported_consumer` to see if WBRF is supported.
+ * When receiving an ACPI notification for some frequencies change, run
+ * `wbrf_retrieve_exclusions` to retrieve the latest frequencies ranges.
+ */
+int wbrf_retrieve_exclusions(struct acpi_device *adev,
+ struct wbrf_ranges_out *out);
+bool wbrf_supported_consumer(struct acpi_device *adev);
+
+#endif /* _LINUX_WBRF_H */
--
2.34.1

2023-06-21 05:51:01

[permalink] [raw]

Subject: [PATCH V4 2/8] cfg80211: expose nl80211_chan_width_to_mhz for wide sharing

The newly added WBRF feature needs this interface for channel
width calculation.

Signed-off-by: Evan Quan <[email protected]>
---
include/net/cfg80211.h | 8 ++++++++
net/wireless/chan.c | 3 ++-
2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 9e04f69712b1..c6dc337eafce 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -920,6 +920,14 @@ const struct cfg80211_chan_def *
cfg80211_chandef_compatible(const struct cfg80211_chan_def *chandef1,
const struct cfg80211_chan_def *chandef2);

+/**
+ * nl80211_chan_width_to_mhz - get the channel width in Mhz
+ * @chan_width: the channel width from &enum nl80211_chan_width
+ * Return: channel width in Mhz if the chan_width from &enum nl80211_chan_width
+ * is valid. -1 otherwise.
+ */
+int nl80211_chan_width_to_mhz(enum nl80211_chan_width chan_width);
+
/**
* cfg80211_chandef_valid - check if a channel definition is valid
* @chandef: the channel definition to check
diff --git a/net/wireless/chan.c b/net/wireless/chan.c
index 0b7e81db383d..227db04eac42 100644
--- a/net/wireless/chan.c
+++ b/net/wireless/chan.c
@@ -141,7 +141,7 @@ static bool cfg80211_edmg_chandef_valid(const struct cfg80211_chan_def *chandef)
return true;
}

-static int nl80211_chan_width_to_mhz(enum nl80211_chan_width chan_width)
+int nl80211_chan_width_to_mhz(enum nl80211_chan_width chan_width)
{
int mhz;

@@ -190,6 +190,7 @@ static int nl80211_chan_width_to_mhz(enum nl80211_chan_width chan_width)
}
return mhz;
}
+EXPORT_SYMBOL(nl80211_chan_width_to_mhz);

static int cfg80211_chandef_get_width(const struct cfg80211_chan_def *c)
{
--
2.34.1

2023-06-21 05:51:37

[permalink] [raw]

Subject: [PATCH V4 3/8] wifi: mac80211: Add support for ACPI WBRF

To support AMD's WBRF interference mitigation mechanism, Wifi adapters
utilized in the system must register the frequencies in use(or unregister
those frequencies no longer used) via the dedicated APCI calls. So that,
other drivers responding to the frequencies can take proper actions to
mitigate possible interference.

To make WBRF feature functional, the kernel needs to be configured with
CONFIG_ACPI_WBRF and the platform is equipped with WBRF support(from
BIOS and drivers).

Signed-off-by: Mario Limonciello <[email protected]>
Co-developed-by: Evan Quan <[email protected]>
Signed-off-by: Evan Quan <[email protected]>
--
v1->v2:
- place the new added member(`wbrf_supported`) in
ieee80211_local(Johannes)
- handle chandefs change scenario properly(Johannes)
- some minor fixes around code sharing and possible invalid input
checks(Johannes)
v2->v3:
- drop unnecessary input checks and intermediate APIs(Mario)
- Separate some mac80211 common code(Mario, Johannes)
---
include/linux/ieee80211.h | 1 +
net/mac80211/Makefile | 2 +
net/mac80211/chan.c | 11 ++++
net/mac80211/ieee80211_i.h | 19 +++++++
net/mac80211/main.c | 2 +
net/mac80211/wbrf.c | 111 +++++++++++++++++++++++++++++++++++++
6 files changed, 146 insertions(+)
create mode 100644 net/mac80211/wbrf.c

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index c4cf296e7eaf..0703921547f5 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -4319,6 +4319,7 @@ static inline int ieee80211_get_tdls_action(struct sk_buff *skb, u32 hdr_size)
/* convert frequencies */
#define MHZ_TO_KHZ(freq) ((freq) * 1000)
#define KHZ_TO_MHZ(freq) ((freq) / 1000)
+#define KHZ_TO_HZ(freq) ((freq) * 1000)
#define PR_KHZ(f) KHZ_TO_MHZ(f), f % 1000
#define KHZ_F "%d.%03d"

diff --git a/net/mac80211/Makefile b/net/mac80211/Makefile
index b8de44da1fb8..709eb678f42a 100644
--- a/net/mac80211/Makefile
+++ b/net/mac80211/Makefile
@@ -65,4 +65,6 @@ rc80211_minstrel-$(CONFIG_MAC80211_DEBUGFS) += \

mac80211-$(CONFIG_MAC80211_RC_MINSTREL) += $(rc80211_minstrel-y)

+mac80211-$(CONFIG_ACPI_WBRF) += wbrf.o
+
ccflags-y += -DDEBUG
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 77c90ed8f5d7..0c5289a9aa6c 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -506,11 +506,16 @@ static void _ieee80211_change_chanctx(struct ieee80211_local *local,

WARN_ON(!cfg80211_chandef_compatible(&ctx->conf.def, chandef));

+ ieee80211_remove_wbrf(local, &ctx->conf.def);
+
ctx->conf.def = *chandef;

/* check if min chanctx also changed */
changed = IEEE80211_CHANCTX_CHANGE_WIDTH |
_ieee80211_recalc_chanctx_min_def(local, ctx, rsvd_for);
+
+ ieee80211_add_wbrf(local, &ctx->conf.def);
+
drv_change_chanctx(local, ctx, changed);

if (!local->use_chanctx) {
@@ -668,6 +673,10 @@ static int ieee80211_add_chanctx(struct ieee80211_local *local,
lockdep_assert_held(&local->mtx);
lockdep_assert_held(&local->chanctx_mtx);

+ err = ieee80211_add_wbrf(local, &ctx->conf.def);
+ if (err)
+ return err;
+
if (!local->use_chanctx)
local->hw.conf.radar_enabled = ctx->conf.radar_enabled;

@@ -748,6 +757,8 @@ static void ieee80211_del_chanctx(struct ieee80211_local *local,
}

ieee80211_recalc_idle(local);
+
+ ieee80211_remove_wbrf(local, &ctx->conf.def);
}

static void ieee80211_free_chanctx(struct ieee80211_local *local,
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index b0372e76f373..f832de16073b 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1591,6 +1591,10 @@ struct ieee80211_local {

/* extended capabilities provided by mac80211 */
u8 ext_capa[8];
+
+#ifdef CONFIG_ACPI_WBRF
+ bool wbrf_supported;
+#endif
};

static inline struct ieee80211_sub_if_data *
@@ -2615,4 +2619,19 @@ ieee80211_eht_cap_ie_to_sta_eht_cap(struct ieee80211_sub_if_data *sdata,
const struct ieee80211_eht_cap_elem *eht_cap_ie_elem,
u8 eht_cap_len,
struct link_sta_info *link_sta);
+
+#ifdef CONFIG_ACPI_WBRF
+void ieee80211_check_wbrf_support(struct ieee80211_local *local);
+int ieee80211_add_wbrf(struct ieee80211_local *local,
+ struct cfg80211_chan_def *chandef);
+void ieee80211_remove_wbrf(struct ieee80211_local *local,
+ struct cfg80211_chan_def *chandef);
+#else
+static inline void ieee80211_check_wbrf_support(struct ieee80211_local *local) { }
+static inline int ieee80211_add_wbrf(struct ieee80211_local *local,
+ struct cfg80211_chan_def *chandef) { return 0; }
+static inline void ieee80211_remove_wbrf(struct ieee80211_local *local,
+ struct cfg80211_chan_def *chandef) { }
+#endif /* CONFIG_ACPI_WBRF */
+
#endif /* IEEE80211_I_H */
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 55cdfaef0f5d..0a55626b1546 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1395,6 +1395,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
debugfs_hw_add(local);
rate_control_add_debugfs(local);

+ ieee80211_check_wbrf_support(local);
+
rtnl_lock();
wiphy_lock(hw->wiphy);

diff --git a/net/mac80211/wbrf.c b/net/mac80211/wbrf.c
new file mode 100644
index 000000000000..a96f74ef6584
--- /dev/null
+++ b/net/mac80211/wbrf.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD Wifi Band Exclusion Interface
+ * Copyright (C) 2023 Advanced Micro Devices
+ *
+ */
+
+#include <linux/wbrf.h>
+#include <net/cfg80211.h>
+#include "ieee80211_i.h"
+
+void ieee80211_check_wbrf_support(struct ieee80211_local *local)
+{
+ struct device *dev = local->hw.wiphy->dev.parent;
+ struct acpi_device *acpi_dev;
+
+ if (!dev)
+ return;
+
+ acpi_dev = ACPI_COMPANION(dev);
+ if (!acpi_dev) {
+ dev_dbg(dev, "ACPI companion not found\n");
+ return;
+ }
+
+ local->wbrf_supported = wbrf_supported_producer(acpi_dev);
+ dev_dbg(dev, "WBRF is %s supported\n",
+ local->wbrf_supported ? "" : "not");
+}
+
+static void get_chan_freq_boundary(u32 center_freq,
+ u32 bandwidth,
+ u64 *start,
+ u64 *end)
+{
+ bandwidth = MHZ_TO_KHZ(bandwidth);
+ center_freq = MHZ_TO_KHZ(center_freq);
+
+ *start = center_freq - bandwidth / 2;
+ *end = center_freq + bandwidth / 2;
+
+ /* Frequency in HZ is expected */
+ *start = KHZ_TO_HZ(*start);
+ *end = KHZ_TO_HZ(*end);
+}
+
+static int wbrf_get_ranges_from_chandef(struct cfg80211_chan_def *chandef,
+ struct wbrf_ranges_in *ranges_in)
+{
+ u64 start_freq1, end_freq1;
+ u64 start_freq2, end_freq2;
+ int bandwidth;
+
+ bandwidth = nl80211_chan_width_to_mhz(chandef->width);
+ if (bandwidth < 0)
+ return -EINVAL;
+
+ get_chan_freq_boundary(chandef->center_freq1,
+ bandwidth,
+ &start_freq1,
+ &end_freq1);
+
+ ranges_in->band_list[0].start = start_freq1;
+ ranges_in->band_list[0].end = end_freq1;
+
+ if (chandef->width == NL80211_CHAN_WIDTH_80P80) {
+ get_chan_freq_boundary(chandef->center_freq2,
+ bandwidth,
+ &start_freq2,
+ &end_freq2);
+
+ ranges_in->band_list[1].start = start_freq2;
+ ranges_in->band_list[1].end = end_freq2;
+ }
+
+ return 0;
+}
+
+int ieee80211_add_wbrf(struct ieee80211_local *local,
+ struct cfg80211_chan_def *chandef)
+{
+ struct device *dev = local->hw.wiphy->dev.parent;
+ struct wbrf_ranges_in ranges_in = {0};
+ int ret;
+
+ if (!local->wbrf_supported)
+ return 0;
+
+ ret = wbrf_get_ranges_from_chandef(chandef, &ranges_in);
+ if (ret)
+ return ret;
+
+ return wbrf_add_exclusion(ACPI_COMPANION(dev), &ranges_in);
+}
+
+void ieee80211_remove_wbrf(struct ieee80211_local *local,
+ struct cfg80211_chan_def *chandef)
+{
+ struct device *dev = local->hw.wiphy->dev.parent;
+ struct wbrf_ranges_in ranges_in = {0};
+ int ret;
+
+ if (!local->wbrf_supported)
+ return;
+
+ ret = wbrf_get_ranges_from_chandef(chandef, &ranges_in);
+ if (ret)
+ return;
+
+ wbrf_remove_exclusion(ACPI_COMPANION(dev), &ranges_in);
+}
--
2.34.1

2023-06-21 05:52:13

[permalink] [raw]

Subject: [PATCH V4 4/8] drm/amd/pm: update driver_if and ppsmc headers for coming wbrf feature

Add those data structures to support Wifi RFI mitigation feature.

Signed-off-by: Evan Quan <[email protected]>
Reviewed-by: Mario Limonciello <[email protected]>
---
.../pm/swsmu/inc/pmfw_if/smu13_driver_if_v13_0_0.h | 14 +++++++++++++-
.../pm/swsmu/inc/pmfw_if/smu13_driver_if_v13_0_7.h | 14 +++++++++++++-
.../amd/pm/swsmu/inc/pmfw_if/smu_v13_0_0_ppsmc.h | 3 ++-
.../amd/pm/swsmu/inc/pmfw_if/smu_v13_0_7_ppsmc.h | 3 ++-
4 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu13_driver_if_v13_0_0.h b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu13_driver_if_v13_0_0.h
index b686fb68a6e7..d64188fb5839 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu13_driver_if_v13_0_0.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu13_driver_if_v13_0_0.h
@@ -388,6 +388,17 @@ typedef struct {
EccInfo_t EccInfo[24];
} EccInfoTable_t;

+typedef struct {
+ uint16_t LowFreq;
+ uint16_t HighFreq;
+} WifiOneBand_t;
+
+typedef struct {
+ uint32_t WifiBandEntryNum;
+ WifiOneBand_t WifiBandEntry[11];
+ uint32_t MmHubPadding[8];
+} WifiBandEntryTable_t;
+
//D3HOT sequences
typedef enum {
BACO_SEQUENCE,
@@ -1592,7 +1603,8 @@ typedef struct {
#define TABLE_I2C_COMMANDS 9
#define TABLE_DRIVER_INFO 10
#define TABLE_ECCINFO 11
-#define TABLE_COUNT 12
+#define TABLE_WIFIBAND 12
+#define TABLE_COUNT 13

//IH Interupt ID
#define IH_INTERRUPT_ID_TO_DRIVER 0xFE
diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu13_driver_if_v13_0_7.h b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu13_driver_if_v13_0_7.h
index 4c46a0392451..77483e8485e7 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu13_driver_if_v13_0_7.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu13_driver_if_v13_0_7.h
@@ -392,6 +392,17 @@ typedef struct {
EccInfo_t EccInfo[24];
} EccInfoTable_t;

+typedef struct {
+ uint16_t LowFreq;
+ uint16_t HighFreq;
+} WifiOneBand_t;
+
+typedef struct {
+ uint32_t WifiBandEntryNum;
+ WifiOneBand_t WifiBandEntry[11];
+ uint32_t MmHubPadding[8];
+} WifiBandEntryTable_t;
+
//D3HOT sequences
typedef enum {
BACO_SEQUENCE,
@@ -1624,7 +1635,8 @@ typedef struct {
#define TABLE_I2C_COMMANDS 9
#define TABLE_DRIVER_INFO 10
#define TABLE_ECCINFO 11
-#define TABLE_COUNT 12
+#define TABLE_WIFIBAND 12
+#define TABLE_COUNT 13

//IH Interupt ID
#define IH_INTERRUPT_ID_TO_DRIVER 0xFE
diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v13_0_0_ppsmc.h b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v13_0_0_ppsmc.h
index 10cff75b44d5..c98cc32d11bd 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v13_0_0_ppsmc.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v13_0_0_ppsmc.h
@@ -138,7 +138,8 @@
#define PPSMC_MSG_SetBadMemoryPagesRetiredFlagsPerChannel 0x4A
#define PPSMC_MSG_SetPriorityDeltaGain 0x4B
#define PPSMC_MSG_AllowIHHostInterrupt 0x4C
-#define PPSMC_Message_Count 0x4D
+#define PPSMC_MSG_EnableUCLKShadow 0x51
+#define PPSMC_Message_Count 0x52

//Debug Dump Message
#define DEBUGSMC_MSG_TestMessage 0x1
diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v13_0_7_ppsmc.h b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v13_0_7_ppsmc.h
index 6aaefca9b595..a6bf9cdd130e 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v13_0_7_ppsmc.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v13_0_7_ppsmc.h
@@ -134,6 +134,7 @@
#define PPSMC_MSG_SetBadMemoryPagesRetiredFlagsPerChannel 0x4A
#define PPSMC_MSG_SetPriorityDeltaGain 0x4B
#define PPSMC_MSG_AllowIHHostInterrupt 0x4C
-#define PPSMC_Message_Count 0x4D
+#define PPSMC_MSG_EnableUCLKShadow 0x51
+#define PPSMC_Message_Count 0x52

#endif
--
2.34.1

2023-06-21 05:52:24

[permalink] [raw]

Subject: [PATCH V4 5/8] drm/amd/pm: setup the framework to support Wifi RFI mitigation feature

With WBRF feature supported, as a driver responding to the frequencies,
amdgpu driver is able to do shadow pstate switching to mitigate possible
interference(between its (G-)DDR memory clocks and local radio module
frequency bands used by Wifi 6/6e/7).

To make WBRF feature functional, the kernel needs to be configured with
CONFIG_ACPI_WBRF and the platform is equipped with necessary ACPI based
mechanism to get amdgpu driver notified.

Signed-off-by: Evan Quan <[email protected]>
Reviewed-by: Mario Limonciello <[email protected]>
--
v1->v2:
- update the prompt for feature support(Lijo)
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 26 +++
drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 63 ++++++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 19 ++
drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 184 ++++++++++++++++++
drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h | 20 ++
drivers/gpu/drm/amd/pm/swsmu/smu_internal.h | 3 +
6 files changed, 315 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 02b827785e39..2f2ec64ed1b2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -50,6 +50,7 @@
#include <linux/hashtable.h>
#include <linux/dma-fence.h>
#include <linux/pci.h>
+#include <linux/wbrf.h>

#include <drm/ttm/ttm_bo.h>
#include <drm/ttm/ttm_placement.h>
@@ -241,6 +242,7 @@ extern int amdgpu_num_kcq;
#define AMDGPU_VCNFW_LOG_SIZE (32 * 1024)
extern int amdgpu_vcnfw_log;
extern int amdgpu_sg_display;
+extern int amdgpu_wbrf;

#define AMDGPU_VM_MAX_NUM_CTX 4096
#define AMDGPU_SG_THRESHOLD (256*1024*1024)
@@ -741,6 +743,9 @@ struct amdgpu_reset_domain;
*/
#define AMDGPU_HAS_VRAM(_adev) ((_adev)->gmc.real_vram_size)

+typedef
+void (*wbrf_notify_handler) (struct amdgpu_device *adev);
+
struct amdgpu_device {
struct device *dev;
struct pci_dev *pdev;
@@ -1050,6 +1055,8 @@ struct amdgpu_device {

bool job_hang;
bool dc_enabled;
+
+ wbrf_notify_handler wbrf_event_handler;
};

static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev)
@@ -1381,6 +1388,25 @@ static inline int amdgpu_acpi_smart_shift_update(struct drm_device *dev,
enum amdgpu_ss ss_state) { return 0; }
#endif

+#if defined(CONFIG_ACPI_WBRF)
+bool amdgpu_acpi_is_wbrf_supported(struct amdgpu_device *adev);
+int amdgpu_acpi_wbrf_retrieve_exclusions(struct amdgpu_device *adev,
+ struct wbrf_ranges_out *exclusions_out);
+int amdgpu_acpi_register_wbrf_notify_handler(struct amdgpu_device *adev,
+ wbrf_notify_handler handler);
+int amdgpu_acpi_unregister_wbrf_notify_handler(struct amdgpu_device *adev);
+#else
+static inline bool amdgpu_acpi_is_wbrf_supported(struct amdgpu_device *adev) { return false; }
+static inline
+int amdgpu_acpi_wbrf_retrieve_exclusions(struct amdgpu_device *adev,
+ struct wbrf_ranges_out *exclusions_out) { return 0; }
+static inline
+int amdgpu_acpi_register_wbrf_notify_handler(struct amdgpu_device *adev,
+ wbrf_notify_handler handler) { return 0; }
+static inline
+int amdgpu_acpi_unregister_wbrf_notify_handler(struct amdgpu_device *adev) { return 0; }
+#endif
+
#if defined(CONFIG_ACPI) && defined(CONFIG_SUSPEND)
bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev);
bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index aeeec211861c..efbe6dd91d1a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -1105,3 +1105,66 @@ bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)
}

#endif /* CONFIG_SUSPEND */
+
+#ifdef CONFIG_ACPI_WBRF
+bool amdgpu_acpi_is_wbrf_supported(struct amdgpu_device *adev)
+{
+ struct acpi_device *acpi_dev = ACPI_COMPANION(adev->dev);
+
+ if (!acpi_dev)
+ return false;
+
+ return wbrf_supported_consumer(acpi_dev);
+}
+
+int amdgpu_acpi_wbrf_retrieve_exclusions(struct amdgpu_device *adev,
+ struct wbrf_ranges_out *exclusions_out)
+{
+ struct acpi_device *acpi_dev = ACPI_COMPANION(adev->dev);
+
+ if (!acpi_dev)
+ return -ENODEV;
+
+ return wbrf_retrieve_exclusions(acpi_dev, exclusions_out);
+}
+
+#define CPM_GPU_NOTIFY_COMMAND 0x55
+static void amdgpu_acpi_wbrf_event(acpi_handle handle, u32 event, void *data)
+{
+ struct amdgpu_device *adev = (struct amdgpu_device *)data;
+
+ if (event == CPM_GPU_NOTIFY_COMMAND &&
+ adev->wbrf_event_handler)
+ adev->wbrf_event_handler(adev);
+}
+
+int amdgpu_acpi_register_wbrf_notify_handler(struct amdgpu_device *adev,
+ wbrf_notify_handler handler)
+{
+ struct acpi_handle *acpi_hdler = ACPI_HANDLE(adev->dev);
+
+ if (!acpi_hdler)
+ return -ENODEV;
+
+ adev->wbrf_event_handler = handler;
+
+ return acpi_install_notify_handler(acpi_hdler,
+ ACPI_ALL_NOTIFY,
+ amdgpu_acpi_wbrf_event,
+ adev);
+}
+
+int amdgpu_acpi_unregister_wbrf_notify_handler(struct amdgpu_device *adev)
+{
+ struct acpi_handle *acpi_hdler = ACPI_HANDLE(adev->dev);
+
+ if (!acpi_hdler)
+ return -ENODEV;
+
+ adev->wbrf_event_handler = NULL;
+
+ return acpi_remove_notify_handler(acpi_hdler,
+ ACPI_ALL_NOTIFY,
+ amdgpu_acpi_wbrf_event);
+}
+#endif
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index b1ca1ab6d6ad..bf82cc192153 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -191,6 +191,7 @@ int amdgpu_smartshift_bias;
int amdgpu_use_xgmi_p2p = 1;
int amdgpu_vcnfw_log;
int amdgpu_sg_display = -1; /* auto */
+int amdgpu_wbrf = -1;

static void amdgpu_drv_delayed_reset_work_handler(struct work_struct *work);

@@ -948,6 +949,24 @@ MODULE_PARM_DESC(smu_pptable_id,
"specify pptable id to be used (-1 = auto(default) value, 0 = use pptable from vbios, > 0 = soft pptable id)");
module_param_named(smu_pptable_id, amdgpu_smu_pptable_id, int, 0444);

+#ifdef CONFIG_ACPI_WBRF
+/**
+ * DOC: wbrf (int)
+ * Enable Wifi RFI interference mitigation feature.
+ * Due to electrical and mechanical constraints there may be likely interference of
+ * relatively high-powered harmonics of the (G-)DDR memory clocks with local radio
+ * module frequency bands used by Wifi 6/6e/7. To mitigate the possible RFI interference,
+ * with this feature enabled, PMFW will use either “shadowed P-State” or “P-State” based
+ * on active list of frequencies in-use (to be avoided) as part of initial setting or
+ * P-state transition. However, there may be potential performance impact with this
+ * feature enabled.
+ * (0 = disabled, 1 = enabled, -1 = auto (default setting, will be enabled if supported))
+ */
+MODULE_PARM_DESC(wbrf,
+ "Enable Wifi RFI interference mitigation (0 = disabled, 1 = enabled, -1 = auto(default)");
+module_param_named(wbrf, amdgpu_wbrf, int, 0444);
+#endif
+
/* These devices are not supported by amdgpu.
* They are supported by the mach64, r128, radeon drivers
*/
diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index 2ddf5198e5c4..a8efdce4607c 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -1188,6 +1188,163 @@ static int smu_get_thermal_temperature_range(struct smu_context *smu)
return ret;
}

+/**
+ * smu_wbrf_handle_exclusion_ranges - consume the wbrf exclusion ranges
+ *
+ * @smu: smu_context pointer
+ *
+ * Retrieve the wbrf exclusion ranges and send them to PMFW for proper handling.
+ * Returns 0 on success, error on failure.
+ */
+static int smu_wbrf_handle_exclusion_ranges(struct smu_context *smu)
+{
+ struct wbrf_ranges_out wbrf_exclusion = {0};
+ struct exclusion_range *wifi_bands = wbrf_exclusion.band_list;
+ struct amdgpu_device *adev = smu->adev;
+ uint64_t start, end;
+ int ret, i, j;
+
+ ret = amdgpu_acpi_wbrf_retrieve_exclusions(adev, &wbrf_exclusion);
+ if (ret) {
+ dev_err(adev->dev, "Failed to retrieve exclusion ranges!\n");
+ return ret;
+ }
+
+ /*
+ * The exclusion ranges array we got might be filled with holes and duplicate
+ * entries. For example:
+ * {(2400, 2500), (0, 0), (6882, 6962), (2400, 2500), (0, 0), (6117, 6189), (0, 0)...}
+ * We need to do some sortups to eliminate those holes and duplicate entries.
+ * Expected output: {(2400, 2500), (6117, 6189), (6882, 6962), (0, 0)...}
+ */
+ for (i = 0; i < MAX_NUM_OF_WBRF_RANGES; i++) {
+ start = wifi_bands[i].start;
+ end = wifi_bands[i].end;
+
+ /* get the last valid entry to fill the intermediate hole */
+ if (!start && !end) {
+ for (j = MAX_NUM_OF_WBRF_RANGES - 1; j > i; j--)
+ if (wifi_bands[j].start &&
+ wifi_bands[j].end)
+ break;
+
+ if (j > i) {
+ wifi_bands[i].start = wifi_bands[j].start;
+ wifi_bands[i].end = wifi_bands[j].end;
+ wifi_bands[j].start = 0;
+ wifi_bands[j].end = 0;
+ }
+
+ continue;
+ }
+
+ /* eliminate duplicate entries */
+ for (j = i + 1; j < MAX_NUM_OF_WBRF_RANGES; j++) {
+ if ((wifi_bands[j].start == start) &&
+ (wifi_bands[j].end == end)) {
+ wifi_bands[j].start = 0;
+ wifi_bands[j].end = 0;
+ continue;
+ }
+ }
+ }
+
+ /* Send the sorted wifi_bands to PMFW */
+ ret = smu_set_wbrf_exclusion_ranges(smu, wifi_bands);
+ /* Give it another chance */
+ if (unlikely(ret == -EBUSY)) {
+ mdelay(5);
+ ret = smu_set_wbrf_exclusion_ranges(smu, wifi_bands);
+ }
+
+ return ret;
+}
+
+/**
+ * smu_wbrf_event_handler - handle notify events
+ *
+ * @adev: struct amdgpu_device pointer
+ *
+ * Calls relevant amdgpu function in response to wbrf event
+ * notification from BIOS.
+ */
+static void smu_wbrf_event_handler(struct amdgpu_device *adev)
+{
+ struct smu_context *smu = adev->powerplay.pp_handle;
+
+ smu_wbrf_handle_exclusion_ranges(smu);
+}
+
+/**
+ * smu_wbrf_support_check - check wbrf support
+ *
+ * @smu: smu_context pointer
+ *
+ * Verifies the ACPI interface whether wbrf is supported.
+ */
+static void smu_wbrf_support_check(struct smu_context *smu)
+{
+ struct amdgpu_device *adev = smu->adev;
+
+ smu->wbrf_supported = smu_is_asic_wbrf_supported(smu) &&
+ !!amdgpu_wbrf &&
+ amdgpu_acpi_is_wbrf_supported(adev);
+
+ if (smu->wbrf_supported)
+ dev_info(adev->dev, "RF interference mitigation is supported\n");
+}
+
+/**
+ * smu_wbrf_init - init driver wbrf support
+ *
+ * @smu: smu_context pointer
+ *
+ * Verifies the AMD ACPI interfaces and registers with the wbrf
+ * notifier chain if wbrf feature is supported.
+ * Returns 0 on success, error on failure.
+ */
+static int smu_wbrf_init(struct smu_context *smu)
+{
+ struct amdgpu_device *adev = smu->adev;
+ int ret;
+
+ if (!smu->wbrf_supported)
+ return 0;
+
+ ret = amdgpu_acpi_register_wbrf_notify_handler(adev,
+ smu_wbrf_event_handler);
+ if (ret)
+ return ret;
+
+ /*
+ * Some wifiband exclusion ranges may be already there
+ * before our driver loaded. To make sure our driver
+ * is awared of those exclusion ranges.
+ */
+ ret = smu_wbrf_handle_exclusion_ranges(smu);
+ if (ret)
+ dev_err(adev->dev, "Failed to handle wbrf exclusion ranges\n");
+
+ return ret;
+}
+
+/**
+ * smu_wbrf_fini - tear down driver wbrf support
+ *
+ * @smu: smu_context pointer
+ *
+ * Unregisters with the wbrf notifier chain.
+ */
+static void smu_wbrf_fini(struct smu_context *smu)
+{
+ struct amdgpu_device *adev = smu->adev;
+
+ if (!smu->wbrf_supported)
+ return;
+
+ amdgpu_acpi_unregister_wbrf_notify_handler(adev);
+}
+
static int smu_smc_hw_setup(struct smu_context *smu)
{
struct smu_feature *feature = &smu->smu_feature;
@@ -1280,6 +1437,15 @@ static int smu_smc_hw_setup(struct smu_context *smu)
if (ret)
return ret;

+ /* Enable UclkShadow on wbrf supported */
+ if (smu->wbrf_supported) {
+ ret = smu_enable_uclk_shadow(smu, true);
+ if (ret) {
+ dev_err(adev->dev, "Failed to enable UclkShadow feature to support wbrf!\n");
+ return ret;
+ }
+ }
+
/*
* With SCPM enabled, these actions(and relevant messages) are
* not needed and permitted.
@@ -1376,6 +1542,15 @@ static int smu_smc_hw_setup(struct smu_context *smu)
*/
ret = smu_set_min_dcef_deep_sleep(smu,
smu->smu_table.boot_values.dcefclk / 100);
+ if (ret) {
+ dev_err(adev->dev, "Error setting min deepsleep dcefclk\n");
+ return ret;
+ }
+
+ /* Init wbrf support. Properly setup the notifier */
+ ret = smu_wbrf_init(smu);
+ if (ret)
+ dev_err(adev->dev, "Error during wbrf init call\n");

return ret;
}
@@ -1431,6 +1606,13 @@ static int smu_hw_init(void *handle)
return ret;
}

+ /*
+ * Check whether wbrf is supported. This needs to be done
+ * before SMU setup starts since part of SMU configuration
+ * relies on this.
+ */
+ smu_wbrf_support_check(smu);
+
if (smu->is_apu) {
ret = smu_set_gfx_imu_enable(smu);
if (ret)
@@ -1583,6 +1765,8 @@ static int smu_smc_hw_cleanup(struct smu_context *smu)
struct amdgpu_device *adev = smu->adev;
int ret = 0;

+ smu_wbrf_fini(smu);
+
cancel_work_sync(&smu->throttling_logging_work);
cancel_work_sync(&smu->interrupt_work);

diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
index 09469c750a96..ff0af3da0be2 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
@@ -573,6 +573,9 @@ struct smu_context
u32 debug_param_reg;
u32 debug_msg_reg;
u32 debug_resp_reg;
+
+ /* data structures for wbrf feature support */
+ bool wbrf_supported;
};

struct i2c_adapter;
@@ -1354,6 +1357,23 @@ struct pptable_funcs {
* @init_pptable_microcode: Prepare the pptable microcode to upload via PSP
*/
int (*init_pptable_microcode)(struct smu_context *smu);
+
+ /**
+ * @is_asic_wbrf_supported: check whether PMFW supports the wbrf feature
+ */
+ bool (*is_asic_wbrf_supported)(struct smu_context *smu);
+
+ /**
+ * @enable_uclk_shadow: Enable the uclk shadow feature on wbrf supported
+ */
+ int (*enable_uclk_shadow)(struct smu_context *smu,
+ bool enablement);
+
+ /**
+ * @set_wbrf_exclusion_ranges: notify SMU the wifi bands occupied
+ */
+ int (*set_wbrf_exclusion_ranges)(struct smu_context *smu,
+ struct exclusion_range *exclusion_ranges);
};

typedef enum {
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_internal.h b/drivers/gpu/drm/amd/pm/swsmu/smu_internal.h
index ceb13c838067..67d7495ab49e 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu_internal.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu_internal.h
@@ -97,6 +97,9 @@
#define smu_get_default_config_table_settings(smu, config_table) smu_ppt_funcs(get_default_config_table_settings, -EOPNOTSUPP, smu, config_table)
#define smu_set_config_table(smu, config_table) smu_ppt_funcs(set_config_table, -EOPNOTSUPP, smu, config_table)
#define smu_init_pptable_microcode(smu) smu_ppt_funcs(init_pptable_microcode, 0, smu)
+#define smu_is_asic_wbrf_supported(smu) smu_ppt_funcs(is_asic_wbrf_supported, false, smu)
+#define smu_enable_uclk_shadow(smu, enablement) smu_ppt_funcs(enable_uclk_shadow, 0, smu, enablement)
+#define smu_set_wbrf_exclusion_ranges(smu, exclusion_ranges) smu_ppt_funcs(set_wbrf_exclusion_ranges, -EOPNOTSUPP, smu, exclusion_ranges)

#endif
#endif
--
2.34.1

2023-06-21 05:52:44

[permalink] [raw]

Subject: [PATCH V4 6/8] drm/amd/pm: add flood detection for wbrf events

To protect PMFW from being overloaded.

Signed-off-by: Evan Quan <[email protected]>
Reviewed-by: Mario Limonciello <[email protected]>
---
drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 28 ++++++++++++++++---
drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h | 7 +++++
2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index a8efdce4607c..75d6e654b2a6 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -1272,6 +1272,22 @@ static void smu_wbrf_event_handler(struct amdgpu_device *adev)
{
struct smu_context *smu = adev->powerplay.pp_handle;

+ schedule_delayed_work(&smu->wbrf_delayed_work,
+ msecs_to_jiffies(SMU_WBRF_EVENT_HANDLING_PACE));
+}
+
+/**
+ * smu_wbrf_delayed_work_handler - callback on delayed work timer expired
+ *
+ * @work: struct work_struct pointer
+ *
+ * Flood is over and driver will consume the latest exclusion ranges.
+ */
+static void smu_wbrf_delayed_work_handler(struct work_struct *work)
+{
+ struct smu_context *smu =
+ container_of(work, struct smu_context, wbrf_delayed_work.work);
+
smu_wbrf_handle_exclusion_ranges(smu);
}

@@ -1311,6 +1327,9 @@ static int smu_wbrf_init(struct smu_context *smu)
if (!smu->wbrf_supported)
return 0;

+ INIT_DELAYED_WORK(&smu->wbrf_delayed_work,
+ smu_wbrf_delayed_work_handler);
+
ret = amdgpu_acpi_register_wbrf_notify_handler(adev,
smu_wbrf_event_handler);
if (ret)
@@ -1321,11 +1340,10 @@ static int smu_wbrf_init(struct smu_context *smu)
* before our driver loaded. To make sure our driver
* is awared of those exclusion ranges.
*/
- ret = smu_wbrf_handle_exclusion_ranges(smu);
- if (ret)
- dev_err(adev->dev, "Failed to handle wbrf exclusion ranges\n");
+ schedule_delayed_work(&smu->wbrf_delayed_work,
+ msecs_to_jiffies(SMU_WBRF_EVENT_HANDLING_PACE));

- return ret;
+ return 0;
}

/**
@@ -1343,6 +1361,8 @@ static void smu_wbrf_fini(struct smu_context *smu)
return;

amdgpu_acpi_unregister_wbrf_notify_handler(adev);
+
+ cancel_delayed_work_sync(&smu->wbrf_delayed_work);
}

static int smu_smc_hw_setup(struct smu_context *smu)
diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
index ff0af3da0be2..aa63cc43d41c 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
@@ -478,6 +478,12 @@ struct stb_context {

#define WORKLOAD_POLICY_MAX 7

+/*
+ * Configure wbrf event handling pace as there can be only one
+ * event processed every SMU_WBRF_EVENT_HANDLING_PACE ms.
+ */
+#define SMU_WBRF_EVENT_HANDLING_PACE 10
+
struct smu_context
{
struct amdgpu_device *adev;
@@ -576,6 +582,7 @@ struct smu_context

/* data structures for wbrf feature support */
bool wbrf_supported;
+ struct delayed_work wbrf_delayed_work;
};

struct i2c_adapter;
--
2.34.1

2023-06-21 05:53:45

[permalink] [raw]

Subject: [PATCH V4 7/8] drm/amd/pm: enable Wifi RFI mitigation feature support for SMU13.0.0

Fulfill the SMU13.0.0 support for Wifi RFI mitigation feature.

Signed-off-by: Evan Quan <[email protected]>
Reviewed-by: Mario Limonciello <[email protected]>
---
drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h | 3 +
drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h | 3 +-
drivers/gpu/drm/amd/pm/swsmu/inc/smu_v13_0.h | 3 +
.../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c | 9 +++
.../drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c | 60 +++++++++++++++++++
5 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
index aa63cc43d41c..a8a4be32cc59 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
@@ -323,6 +323,7 @@ enum smu_table_id
SMU_TABLE_PACE,
SMU_TABLE_ECCINFO,
SMU_TABLE_COMBO_PPTABLE,
+ SMU_TABLE_WIFIBAND,
SMU_TABLE_COUNT,
};

@@ -1496,6 +1497,8 @@ enum smu_baco_seq {
__dst_size); \
})

+#define HZ_IN_MHZ 1000000U
+
#if !defined(SWSMU_CODE_LAYER_L2) && !defined(SWSMU_CODE_LAYER_L3) && !defined(SWSMU_CODE_LAYER_L4)
int smu_get_power_limit(void *handle,
uint32_t *limit,
diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h b/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
index 297b70b9388f..5bbb60289a79 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
@@ -245,7 +245,8 @@
__SMU_DUMMY_MAP(AllowGpo), \
__SMU_DUMMY_MAP(Mode2Reset), \
__SMU_DUMMY_MAP(RequestI2cTransaction), \
- __SMU_DUMMY_MAP(GetMetricsTable),
+ __SMU_DUMMY_MAP(GetMetricsTable), \
+ __SMU_DUMMY_MAP(EnableUCLKShadow),

#undef __SMU_DUMMY_MAP
#define __SMU_DUMMY_MAP(type) SMU_MSG_##type
diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/smu_v13_0.h b/drivers/gpu/drm/amd/pm/swsmu/inc/smu_v13_0.h
index df3baaab0037..b6fae9b92303 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/inc/smu_v13_0.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/inc/smu_v13_0.h
@@ -303,5 +303,8 @@ int smu_v13_0_get_pptable_from_firmware(struct smu_context *smu,
uint32_t *size,
uint32_t pptable_id);

+int smu_v13_0_enable_uclk_shadow(struct smu_context *smu,
+ bool enablement);
+
#endif
#endif
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
index 393c6a7b9609..8c2230d1d862 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
@@ -2453,3 +2453,12 @@ int smu_v13_0_mode1_reset(struct smu_context *smu)

return ret;
}
+
+int smu_v13_0_enable_uclk_shadow(struct smu_context *smu,
+ bool enablement)
+{
+ return smu_cmn_send_smc_msg_with_param(smu,
+ SMU_MSG_EnableUCLKShadow,
+ enablement,
+ NULL);
+}
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c
index 09405ef1e3c8..cf75feaee779 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c
@@ -155,6 +155,7 @@ static struct cmn2asic_msg_mapping smu_v13_0_0_message_map[SMU_MSG_MAX_COUNT] =
MSG_MAP(AllowGpo, PPSMC_MSG_SetGpoAllow, 0),
MSG_MAP(AllowIHHostInterrupt, PPSMC_MSG_AllowIHHostInterrupt, 0),
MSG_MAP(ReenableAcDcInterrupt, PPSMC_MSG_ReenableAcDcInterrupt, 0),
+ MSG_MAP(EnableUCLKShadow, PPSMC_MSG_EnableUCLKShadow, 0),
};

static struct cmn2asic_mapping smu_v13_0_0_clk_map[SMU_CLK_COUNT] = {
@@ -235,6 +236,7 @@ static struct cmn2asic_mapping smu_v13_0_0_table_map[SMU_TABLE_COUNT] = {
TAB_MAP(DRIVER_SMU_CONFIG),
TAB_MAP(ACTIVITY_MONITOR_COEFF),
[SMU_TABLE_COMBO_PPTABLE] = {1, TABLE_COMBO_PPTABLE},
+ TAB_MAP(WIFIBAND),
TAB_MAP(I2C_COMMANDS),
TAB_MAP(ECCINFO),
};
@@ -472,6 +474,9 @@ static int smu_v13_0_0_tables_init(struct smu_context *smu)
PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
SMU_TABLE_INIT(tables, SMU_TABLE_ECCINFO, sizeof(EccInfoTable_t),
PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
+ SMU_TABLE_INIT(tables, SMU_TABLE_WIFIBAND,
+ sizeof(WifiBandEntryTable_t), PAGE_SIZE,
+ AMDGPU_GEM_DOMAIN_VRAM);

smu_table->metrics_table = kzalloc(sizeof(SmuMetricsExternal_t), GFP_KERNEL);
if (!smu_table->metrics_table)
@@ -2112,6 +2117,58 @@ static ssize_t smu_v13_0_0_get_ecc_info(struct smu_context *smu,
return ret;
}

+static bool smu_v13_0_0_wbrf_support_check(struct smu_context *smu)
+{
+ struct amdgpu_device *adev = smu->adev;
+
+ switch (adev->ip_versions[MP1_HWIP][0]) {
+ case IP_VERSION(13, 0, 0):
+ return smu->smc_fw_version >= 0x004e5f00;
+ case IP_VERSION(13, 0, 10):
+ return smu->smc_fw_version >= 0x00503000;
+ default:
+ return false;
+ }
+}
+
+static int smu_v13_0_0_set_wbrf_exclusion_ranges(struct smu_context *smu,
+ struct exclusion_range *exclusion_ranges)
+{
+ WifiBandEntryTable_t wifi_bands;
+ int valid_entries = 0;
+ int ret, i;
+
+ memset(&wifi_bands, 0, sizeof(wifi_bands));
+ for (i = 0; i < ARRAY_SIZE(wifi_bands.WifiBandEntry); i++) {
+ if (!exclusion_ranges[i].start &&
+ !exclusion_ranges[i].end)
+ break;
+
+ /* PMFW expects the inputs to be in Mhz unit */
+ wifi_bands.WifiBandEntry[valid_entries].LowFreq =
+ DIV_ROUND_DOWN_ULL(exclusion_ranges[i].start, HZ_IN_MHZ);
+ wifi_bands.WifiBandEntry[valid_entries++].HighFreq =
+ DIV_ROUND_UP_ULL(exclusion_ranges[i].end, HZ_IN_MHZ);
+ }
+ wifi_bands.WifiBandEntryNum = valid_entries;
+
+ /*
+ * Per confirm with PMFW team, WifiBandEntryNum = 0
+ * is a valid setting. So, there should be no direct
+ * return on that.
+ */
+
+ ret = smu_cmn_update_table(smu,
+ SMU_TABLE_WIFIBAND,
+ 0,
+ (void *)(&wifi_bands),
+ true);
+ if (ret)
+ dev_err(smu->adev->dev, "Failed to set wifiband!");
+
+ return ret;
+}
+
static const struct pptable_funcs smu_v13_0_0_ppt_funcs = {
.get_allowed_feature_mask = smu_v13_0_0_get_allowed_feature_mask,
.set_default_dpm_table = smu_v13_0_0_set_default_dpm_table,
@@ -2188,6 +2245,9 @@ static const struct pptable_funcs smu_v13_0_0_ppt_funcs = {
.send_hbm_bad_channel_flag = smu_v13_0_0_send_bad_mem_channel_flag,
.gpo_control = smu_v13_0_gpo_control,
.get_ecc_info = smu_v13_0_0_get_ecc_info,
+ .is_asic_wbrf_supported = smu_v13_0_0_wbrf_support_check,
+ .enable_uclk_shadow = smu_v13_0_enable_uclk_shadow,
+ .set_wbrf_exclusion_ranges = smu_v13_0_0_set_wbrf_exclusion_ranges,
};

void smu_v13_0_0_set_ppt_funcs(struct smu_context *smu)
--
2.34.1

2023-06-21 06:02:43

[permalink] [raw]

Subject: [PATCH V4 8/8] drm/amd/pm: enable Wifi RFI mitigation feature support for SMU13.0.7

Fulfill the SMU13.0.7 support for Wifi RFI mitigation feature.

Signed-off-by: Evan Quan <[email protected]>
Reviewed-by: Mario Limonciello <[email protected]>
---
.../drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c | 59 +++++++++++++++++++
1 file changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c
index 98a33f8ee209..16c1c04e2034 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c
@@ -125,6 +125,7 @@ static struct cmn2asic_msg_mapping smu_v13_0_7_message_map[SMU_MSG_MAX_COUNT] =
MSG_MAP(ArmD3, PPSMC_MSG_ArmD3, 0),
MSG_MAP(AllowGpo, PPSMC_MSG_SetGpoAllow, 0),
MSG_MAP(GetPptLimit, PPSMC_MSG_GetPptLimit, 0),
+ MSG_MAP(EnableUCLKShadow, PPSMC_MSG_EnableUCLKShadow, 0),
};

static struct cmn2asic_mapping smu_v13_0_7_clk_map[SMU_CLK_COUNT] = {
@@ -205,6 +206,7 @@ static struct cmn2asic_mapping smu_v13_0_7_table_map[SMU_TABLE_COUNT] = {
TAB_MAP(DRIVER_SMU_CONFIG),
TAB_MAP(ACTIVITY_MONITOR_COEFF),
[SMU_TABLE_COMBO_PPTABLE] = {1, TABLE_COMBO_PPTABLE},
+ TAB_MAP(WIFIBAND),
};

static struct cmn2asic_mapping smu_v13_0_7_pwr_src_map[SMU_POWER_SOURCE_COUNT] = {
@@ -487,6 +489,9 @@ static int smu_v13_0_7_tables_init(struct smu_context *smu)
AMDGPU_GEM_DOMAIN_VRAM);
SMU_TABLE_INIT(tables, SMU_TABLE_COMBO_PPTABLE, MP0_MP1_DATA_REGION_SIZE_COMBOPPTABLE,
PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
+ SMU_TABLE_INIT(tables, SMU_TABLE_WIFIBAND,
+ sizeof(WifiBandEntryTable_t), PAGE_SIZE,
+ AMDGPU_GEM_DOMAIN_VRAM);

smu_table->metrics_table = kzalloc(sizeof(SmuMetricsExternal_t), GFP_KERNEL);
if (!smu_table->metrics_table)
@@ -1721,6 +1726,57 @@ static int smu_v13_0_7_set_df_cstate(struct smu_context *smu,
NULL);
}

+static bool smu_v13_0_7_wbrf_support_check(struct smu_context *smu)
+{
+ return smu->smc_fw_version > 0x00524600;
+}
+
+static int smu_v13_0_7_set_wbrf_exclusion_ranges(struct smu_context *smu,
+ struct exclusion_range *exclusion_ranges)
+{
+ WifiBandEntryTable_t wifi_bands;
+ int valid_entries = 0;
+ int ret, i;
+
+ memset(&wifi_bands, 0, sizeof(wifi_bands));
+ for (i = 0; i < ARRAY_SIZE(wifi_bands.WifiBandEntry); i++) {
+ if (!exclusion_ranges[i].start &&
+ !exclusion_ranges[i].end)
+ break;
+
+ /* PMFW expects the inputs to be in Mhz unit */
+ wifi_bands.WifiBandEntry[valid_entries].LowFreq =
+ DIV_ROUND_DOWN_ULL(exclusion_ranges[i].start, HZ_IN_MHZ);
+ wifi_bands.WifiBandEntry[valid_entries++].HighFreq =
+ DIV_ROUND_UP_ULL(exclusion_ranges[i].end, HZ_IN_MHZ);
+ }
+ wifi_bands.WifiBandEntryNum = valid_entries;
+
+ /*
+ * Per confirm with PMFW team, WifiBandEntryNum = 0 is a valid setting.
+ * Considering the scenarios below:
+ * - At first the wifi device adds an exclusion range e.g. (2400,2500) to
+ * BIOS and our driver gets notified. We will set WifiBandEntryNum = 1
+ * and pass the WifiBandEntry (2400, 2500) to PMFW.
+ *
+ * - Later the wifi device removes the wifiband list added above and
+ * our driver gets notified again. At this time, driver will set
+ * WifiBandEntryNum = 0 and pass an empty WifiBandEntry list to PMFW.
+ * - PMFW may still need to do some uclk shadow update(e.g. switching
+ * from shadow clock back to primary clock) on receiving this.
+ */
+
+ ret = smu_cmn_update_table(smu,
+ SMU_TABLE_WIFIBAND,
+ 0,
+ (void *)(&wifi_bands),
+ true);
+ if (ret)
+ dev_err(smu->adev->dev, "Failed to set wifiband!");
+
+ return ret;
+}
+
static const struct pptable_funcs smu_v13_0_7_ppt_funcs = {
.get_allowed_feature_mask = smu_v13_0_7_get_allowed_feature_mask,
.set_default_dpm_table = smu_v13_0_7_set_default_dpm_table,
@@ -1786,6 +1842,9 @@ static const struct pptable_funcs smu_v13_0_7_ppt_funcs = {
.set_mp1_state = smu_v13_0_7_set_mp1_state,
.set_df_cstate = smu_v13_0_7_set_df_cstate,
.gpo_control = smu_v13_0_gpo_control,
+ .is_asic_wbrf_supported = smu_v13_0_7_wbrf_support_check,
+ .enable_uclk_shadow = smu_v13_0_enable_uclk_shadow,
+ .set_wbrf_exclusion_ranges = smu_v13_0_7_set_wbrf_exclusion_ranges,
};

void smu_v13_0_7_set_ppt_funcs(struct smu_context *smu)
--
2.34.1

2023-06-21 10:31:18

[permalink] [raw]

Subject: Re: [PATCH V4 3/8] wifi: mac80211: Add support for ACPI WBRF

On Wed, 2023-06-21 at 13:45 +0800, Evan Quan wrote:
> To support AMD's WBRF interference mitigation mechanism, Wifi adapters
> utilized in the system must register the frequencies in use(or unregister
> those frequencies no longer used) via the dedicated APCI calls. So that,
> other drivers responding to the frequencies can take proper actions to
> mitigate possible interference.
>
> To make WBRF feature functional, the kernel needs to be configured with
> CONFIG_ACPI_WBRF and the platform is equipped with WBRF support(from
> BIOS and drivers).
>
> Signed-off-by: Mario Limonciello <[email protected]>
> Co-developed-by: Evan Quan <[email protected]>
> Signed-off-by: Evan Quan <[email protected]>

I was going to say this looks good ... but still have a few nits, sorry.

But then the next question anyway is how we merge this? The wifi parts
sort of depend on the first patch, although technically I guess I could
merge them since it's all hidden behind the CONFIG_ symbol, assuming you
get that in via some other tree it can combine upstream.

I'd also say you can merge those parts elsewhere but I'm planning to
also land some locking rework that I've been working on, so it will
probably conflict somewhere.

> +++ b/net/mac80211/chan.c
> @@ -506,11 +506,16 @@ static void _ieee80211_change_chanctx(struct ieee80211_local *local,
>
> WARN_ON(!cfg80211_chandef_compatible(&ctx->conf.def, chandef));
>
> + ieee80211_remove_wbrf(local, &ctx->conf.def);
> +
> ctx->conf.def = *chandef;
>
> /* check if min chanctx also changed */
> changed = IEEE80211_CHANCTX_CHANGE_WIDTH |
> _ieee80211_recalc_chanctx_min_def(local, ctx, rsvd_for);
> +
> + ieee80211_add_wbrf(local, &ctx->conf.def);

You ignore the return value here.

> @@ -668,6 +673,10 @@ static int ieee80211_add_chanctx(struct ieee80211_local *local,
> lockdep_assert_held(&local->mtx);
> lockdep_assert_held(&local->chanctx_mtx);
>
> + err = ieee80211_add_wbrf(local, &ctx->conf.def);
> + if (err)
> + return err;

But not here.

In the code, there are basically two error paths:

> +int ieee80211_add_wbrf(struct ieee80211_local *local,
> + struct cfg80211_chan_def *chandef)
> +{
> + struct device *dev = local->hw.wiphy->dev.parent;
> + struct wbrf_ranges_in ranges_in = {0};
> + int ret;
> +
> + if (!local->wbrf_supported)
> + return 0;
> +
> + ret = wbrf_get_ranges_from_chandef(chandef, &ranges_in);
> + if (ret)
> + return ret;

This really won't fail, just if the bandwidth calculation was bad, but
that's an internal error that WARNs anyway and we can ignore it.

> + return wbrf_add_exclusion(ACPI_COMPANION(dev), &ranges_in);

This I find a bit confusing, why do we even propagate the error? If the
platform has some issue with it, should we really fail the connection?

I think it seems better to me to just make this void, and have it be
only a notification interface?

johannes

2023-06-21 14:35:50

[permalink] [raw]

Subject: Re: [PATCH V4 3/8] wifi: mac80211: Add support for ACPI WBRF

On 6/21/2023 5:22 AM, Johannes Berg wrote:
> On Wed, 2023-06-21 at 13:45 +0800, Evan Quan wrote:
>> To support AMD's WBRF interference mitigation mechanism, Wifi adapters
>> utilized in the system must register the frequencies in use(or unregister
>> those frequencies no longer used) via the dedicated APCI calls. So that,
>> other drivers responding to the frequencies can take proper actions to
>> mitigate possible interference.
>>
>> To make WBRF feature functional, the kernel needs to be configured with
>> CONFIG_ACPI_WBRF and the platform is equipped with WBRF support(from
>> BIOS and drivers).
>>
>> Signed-off-by: Mario Limonciello <[email protected]>
>> Co-developed-by: Evan Quan <[email protected]>
>> Signed-off-by: Evan Quan <[email protected]>
> I was going to say this looks good ... but still have a few nits, sorry.
>
> But then the next question anyway is how we merge this? The wifi parts
> sort of depend on the first patch, although technically I guess I could
> merge them since it's all hidden behind the CONFIG_ symbol, assuming you
> get that in via some other tree it can combine upstream.
>
> I'd also say you can merge those parts elsewhere but I'm planning to
> also land some locking rework that I've been working on, so it will
> probably conflict somewhere.
Since it's all gated by CONFIG_ACPI_WBRF for each subsystem that it touches,
my take is that we should merge like this:

1) Get A-b/R-b on patch 1 (ACPI patch) from Rafael.
2) Merge mac80211 bits through WLAN trees
3) Merge AMDGPU bits *and* ACPI bits through amd-staging-drm-next
followed by drm tree

Since WLAN and AMDGPU bits are using the exported ACPI functions from
patch 1, we need to make sure that it is accepted and won't change
interface before merging other bits.

Everything can come together in the upstream tree and the bots
will be able to test linux-next as well this way.

By bringing ACPI bits through amd-staging-drm-next we can also enable
the new Kconfig
option in AMD's CI system to make sure that all the amdgpu bits are
going through CI
testing too earlier before it even hits linux-next.

>> +++ b/net/mac80211/chan.c
>> @@ -506,11 +506,16 @@ static void _ieee80211_change_chanctx(struct ieee80211_local *local,
>>
>> WARN_ON(!cfg80211_chandef_compatible(&ctx->conf.def, chandef));
>>
>> + ieee80211_remove_wbrf(local, &ctx->conf.def);
>> +
>> ctx->conf.def = *chandef;
>>
>> /* check if min chanctx also changed */
>> changed = IEEE80211_CHANCTX_CHANGE_WIDTH |
>> _ieee80211_recalc_chanctx_min_def(local, ctx, rsvd_for);
>> +
>> + ieee80211_add_wbrf(local, &ctx->conf.def);
> You ignore the return value here.
>
>
>> @@ -668,6 +673,10 @@ static int ieee80211_add_chanctx(struct ieee80211_local *local,
>> lockdep_assert_held(&local->mtx);
>> lockdep_assert_held(&local->chanctx_mtx);
>>
>> + err = ieee80211_add_wbrf(local, &ctx->conf.def);
>> + if (err)
>> + return err;
> But not here.
>
> In the code, there are basically two error paths:
>
>> +int ieee80211_add_wbrf(struct ieee80211_local *local,
>> + struct cfg80211_chan_def *chandef)
>> +{
>> + struct device *dev = local->hw.wiphy->dev.parent;
>> + struct wbrf_ranges_in ranges_in = {0};
>> + int ret;
>> +
>> + if (!local->wbrf_supported)
>> + return 0;
>> +
>> + ret = wbrf_get_ranges_from_chandef(chandef, &ranges_in);
>> + if (ret)
>> + return ret;
> This really won't fail, just if the bandwidth calculation was bad, but
> that's an internal error that WARNs anyway and we can ignore it.
>
>> + return wbrf_add_exclusion(ACPI_COMPANION(dev), &ranges_in);
> This I find a bit confusing, why do we even propagate the error? If the
> platform has some issue with it, should we really fail the connection?
>
>
> I think it seems better to me to just make this void, and have it be
> only a notification interface?
>
> johannes

2023-06-21 15:49:24

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

On Wed, Jun 21, 2023 at 01:45:56PM +0800, Evan Quan wrote:
> From: Mario Limonciello <[email protected]>
>
> Due to electrical and mechanical constraints in certain platform designs
> there may be likely interference of relatively high-powered harmonics of
> the (G-)DDR memory clocks with local radio module frequency bands used
> by Wifi 6/6e/7.
>
> To mitigate this, AMD has introduced an ACPI based mechanism that
> devices can use to notify active use of particular frequencies so
> that devices can make relative internal adjustments as necessary
> to avoid this resonance.

Do only ACPI based systems have:

interference of relatively high-powered harmonics of the (G-)DDR
memory clocks with local radio module frequency bands used by
Wifi 6/6e/7."

Could Device Tree based systems not experience this problem?

> +/**
> + * APIs needed by drivers/subsystems for contributing frequencies:
> + * During probe, check `wbrf_supported_producer` to see if WBRF is supported.
> + * If adding frequencies, then call `wbrf_add_exclusion` with the
> + * start and end points specified for the frequency ranges added.
> + * If removing frequencies, then call `wbrf_remove_exclusion` with
> + * start and end points specified for the frequency ranges added.
> + */
> +bool wbrf_supported_producer(struct acpi_device *adev);
> +int wbrf_add_exclusion(struct acpi_device *adev,
> + struct wbrf_ranges_in *in);
> +int wbrf_remove_exclusion(struct acpi_device *adev,
> + struct wbrf_ranges_in *in);

Could struct device be used here, to make the API agnostic to where
the information is coming from? That would then allow somebody in the
future to implement a device tree based information provider.

Andrew

2023-06-21 15:59:43

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

On Wed, 2023-06-21 at 17:36 +0200, Andrew Lunn wrote:
> On Wed, Jun 21, 2023 at 01:45:56PM +0800, Evan Quan wrote:
> > From: Mario Limonciello <[email protected]>
> >
> > Due to electrical and mechanical constraints in certain platform designs
> > there may be likely interference of relatively high-powered harmonics of
> > the (G-)DDR memory clocks with local radio module frequency bands used
> > by Wifi 6/6e/7.
> >
> > To mitigate this, AMD has introduced an ACPI based mechanism that
> > devices can use to notify active use of particular frequencies so
> > that devices can make relative internal adjustments as necessary
> > to avoid this resonance.
>
> Do only ACPI based systems have:
>
> interference of relatively high-powered harmonics of the (G-)DDR
> memory clocks with local radio module frequency bands used by
> Wifi 6/6e/7."
>
> Could Device Tree based systems not experience this problem?

They could, of course, but they'd need some other driver to change
_something_ in the system? I don't even know what this is doing
precisely under the hood in the ACPI BIOS, perhaps it adjusts the DDR
memory clock frequency in response to WiFi using a frequency that will
cause interference with harmonics.

> > +/**
> > + * APIs needed by drivers/subsystems for contributing frequencies:
> > + * During probe, check `wbrf_supported_producer` to see if WBRF is supported.
> > + * If adding frequencies, then call `wbrf_add_exclusion` with the
> > + * start and end points specified for the frequency ranges added.
> > + * If removing frequencies, then call `wbrf_remove_exclusion` with
> > + * start and end points specified for the frequency ranges added.
> > + */
> > +bool wbrf_supported_producer(struct acpi_device *adev);
> > +int wbrf_add_exclusion(struct acpi_device *adev,
> > + struct wbrf_ranges_in *in);
> > +int wbrf_remove_exclusion(struct acpi_device *adev,
> > + struct wbrf_ranges_in *in);
>
> Could struct device be used here, to make the API agnostic to where
> the information is coming from? That would then allow somebody in the
> future to implement a device tree based information provider.

That does make sense, and it wouldn't even be that much harder if we
assume in a given platform there's only one provider - but once you go
beyond that these would need to call function pointers I guess? Though
that could be left for "future improvement" too.

johannes

2023-06-21 16:20:39

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

> > Do only ACPI based systems have:
> >
> > interference of relatively high-powered harmonics of the (G-)DDR
> > memory clocks with local radio module frequency bands used by
> > Wifi 6/6e/7."
> >
> > Could Device Tree based systems not experience this problem?
>
> They could, of course, but they'd need some other driver to change
> _something_ in the system? I don't even know what this is doing
> precisely under the hood in the ACPI BIOS

If you don't know what it is actually doing, it suggests the API is
not very well defined. Is there even enough details that ARM64 ACPI
BIOS could implement this?

> > > +/**
> > > + * APIs needed by drivers/subsystems for contributing frequencies:
> > > + * During probe, check `wbrf_supported_producer` to see if WBRF is supported.
> > > + * If adding frequencies, then call `wbrf_add_exclusion` with the
> > > + * start and end points specified for the frequency ranges added.
> > > + * If removing frequencies, then call `wbrf_remove_exclusion` with
> > > + * start and end points specified for the frequency ranges added.
> > > + */
> > > +bool wbrf_supported_producer(struct acpi_device *adev);
> > > +int wbrf_add_exclusion(struct acpi_device *adev,
> > > + struct wbrf_ranges_in *in);
> > > +int wbrf_remove_exclusion(struct acpi_device *adev,
> > > + struct wbrf_ranges_in *in);
> >
> > Could struct device be used here, to make the API agnostic to where
> > the information is coming from? That would then allow somebody in the
> > future to implement a device tree based information provider.
>
> That does make sense, and it wouldn't even be that much harder if we
> assume in a given platform there's only one provider

That seems like a very reasonable assumption. It is theoretically
possible to build an ACPI + DT hybrid, but i've never seen it actually
done.

If an ARM64 ACPI BIOS could implement this, then i would guess the low
level bits would be solved, i guess jumping into the EL1
firmware. Putting DT on top instead should not be too hard.

Andrew

2023-06-21 16:20:39

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

On 6/21/2023 10:39 AM, Johannes Berg wrote:
> On Wed, 2023-06-21 at 17:36 +0200, Andrew Lunn wrote:
>> On Wed, Jun 21, 2023 at 01:45:56PM +0800, Evan Quan wrote:
>>> From: Mario Limonciello <[email protected]>
>>>
>>> Due to electrical and mechanical constraints in certain platform designs
>>> there may be likely interference of relatively high-powered harmonics of
>>> the (G-)DDR memory clocks with local radio module frequency bands used
>>> by Wifi 6/6e/7.
>>>
>>> To mitigate this, AMD has introduced an ACPI based mechanism that
>>> devices can use to notify active use of particular frequencies so
>>> that devices can make relative internal adjustments as necessary
>>> to avoid this resonance.
>> Do only ACPI based systems have:
>>
>> interference of relatively high-powered harmonics of the (G-)DDR
>> memory clocks with local radio module frequency bands used by
>> Wifi 6/6e/7."
>>
>> Could Device Tree based systems not experience this problem?
> They could, of course, but they'd need some other driver to change
> _something_ in the system? I don't even know what this is doing
> precisely under the hood in the ACPI BIOS, perhaps it adjusts the DDR
> memory clock frequency in response to WiFi using a frequency that will
> cause interference with harmonics.

The way that WBRF has been architected, it's intended to be able
to scale to any type of device pair that has harmonic issues.

In the first use (Wifi 6e + specific AMD dGPUs) that matches this
series BIOS has the following purposes:

1) The existence of _DSM indicates that the system may not have
adequate shielding and should be using these mitigations.

2) Notification mechanism of frequency use.

For the first problematic devices we *could* have done notifications
entirely in native Linux kernel code with notifier chains.
However that still means you need a hint from the platform that the
functionality is needed like a _DSD bit.

It's also done this way so that AML could do some of the notifications
directly to applicable devices in the future without needing "consumer"
driver participation.

>>> +/**
>>> + * APIs needed by drivers/subsystems for contributing frequencies:
>>> + * During probe, check `wbrf_supported_producer` to see if WBRF is supported.
>>> + * If adding frequencies, then call `wbrf_add_exclusion` with the
>>> + * start and end points specified for the frequency ranges added.
>>> + * If removing frequencies, then call `wbrf_remove_exclusion` with
>>> + * start and end points specified for the frequency ranges added.
>>> + */
>>> +bool wbrf_supported_producer(struct acpi_device *adev);
>>> +int wbrf_add_exclusion(struct acpi_device *adev,
>>> + struct wbrf_ranges_in *in);
>>> +int wbrf_remove_exclusion(struct acpi_device *adev,
>>> + struct wbrf_ranges_in *in);
>> Could struct device be used here, to make the API agnostic to where
>> the information is coming from? That would then allow somebody in the
>> future to implement a device tree based information provider.
> That does make sense, and it wouldn't even be that much harder if we
> assume in a given platform there's only one provider - but once you go
> beyond that these would need to call function pointers I guess? Though
> that could be left for "future improvement" too.
>
> johannes

There's more to it than just sending in the frequency that is
added or removed. The notification path comes from ACPI as well.

This first implementation only has one provider and consumer
but yes, we envision that there could be multiple of each party
and that AML may be the mechanism for some consumers to react.

2023-06-21 16:43:07

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

> I think there is enough details for this to happen. It's done
> so that either the AML can natively behave as a consumer or a
> driver can behave as a consumer.

> > > > > +/**
> > > > > + * APIs needed by drivers/subsystems for contributing frequencies:
> > > > > + * During probe, check `wbrf_supported_producer` to see if WBRF is supported.
> > > > > + * If adding frequencies, then call `wbrf_add_exclusion` with the
> > > > > + * start and end points specified for the frequency ranges added.
> > > > > + * If removing frequencies, then call `wbrf_remove_exclusion` with
> > > > > + * start and end points specified for the frequency ranges added.
> > > > > + */
> > > > > +bool wbrf_supported_producer(struct acpi_device *adev);
> > > > > +int wbrf_add_exclusion(struct acpi_device *adev,
> > > > > + struct wbrf_ranges_in *in);
> > > > > +int wbrf_remove_exclusion(struct acpi_device *adev,
> > > > > + struct wbrf_ranges_in *in);
> > > > Could struct device be used here, to make the API agnostic to where
> > > > the information is coming from? That would then allow somebody in the
> > > > future to implement a device tree based information provider.
> > > That does make sense, and it wouldn't even be that much harder if we
> > > assume in a given platform there's only one provider
> > That seems like a very reasonable assumption. It is theoretically
> > possible to build an ACPI + DT hybrid, but i've never seen it actually
> > done.
> >
> > If an ARM64 ACPI BIOS could implement this, then i would guess the low
> > level bits would be solved, i guess jumping into the EL1
> > firmware. Putting DT on top instead should not be too hard.
> >
> > Andrew
>
> To make life easier I'll ask whether we can include snippets of
> the matching ASL for this first implementation as part of the
> public ACPI spec that matches this code when we release it.

So it sounds like you are pretty open about this, there should be
enough information for independent implementations. So please do make
the APIs between the providers and the consumers abstract, struct
device, not an ACPI object.

Andrew

2023-06-21 16:45:24

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

On Wed, 2023-06-21 at 18:14 +0200, Andrew Lunn wrote:
> > > Do only ACPI based systems have:
> > >
> > >    interference of relatively high-powered harmonics of the (G-)DDR
> > >    memory clocks with local radio module frequency bands used by
> > >    Wifi 6/6e/7."
> > >
> > > Could Device Tree based systems not experience this problem?
> >
> > They could, of course, but they'd need some other driver to change
> > _something_ in the system? I don't even know what this is doing
> > precisely under the hood in the ACPI BIOS
>
> If you don't know what it is actually doing, it suggests the API is
> not very well defined.

I wouldn't say that. At the level it's defined now, the API is very
clear: the wifi subsystem tells the other side what channels it's
operating on right now.

> Is there even enough details that ARM64 ACPI
> BIOS could implement this?

This, in itself? No. You'd have to know about the physical
characteristics of the system, what is actually causing interference and
at what frequencies and of course what you can actually do to mitigate
(such as adjusting clock frequencies.)

But as an API? I'd think yes, since WiFi can't really move off a
frequency, other than disconnect, anyway.

> > > > +bool wbrf_supported_producer(struct acpi_device *adev);
> > > > +int wbrf_add_exclusion(struct acpi_device *adev,
> > > > + struct wbrf_ranges_in *in);
> > > > +int wbrf_remove_exclusion(struct acpi_device *adev,
> > > > + struct wbrf_ranges_in *in);
> > >
> > > Could struct device be used here, to make the API agnostic to where
> > > the information is coming from? That would then allow somebody in the
> > > future to implement a device tree based information provider.
> >
> > That does make sense, and it wouldn't even be that much harder if we
> > assume in a given platform there's only one provider
>
> That seems like a very reasonable assumption. It is theoretically
> possible to build an ACPI + DT hybrid, but i've never seen it actually
> done.

OK.

> If an ARM64 ACPI BIOS could implement this, then i would guess the low
> level bits would be solved, i guess jumping into the EL1
> firmware. Putting DT on top instead should not be too hard.

Right.

Maybe then this really shouldn't be called "wbrf", but maybe naming
doesn't matter that much :)

johannes

2023-06-21 17:10:22

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

On Wed, Jun 21, 2023 at 11:15:00AM -0500, Limonciello, Mario wrote:
>
> On 6/21/2023 10:39 AM, Johannes Berg wrote:
> > On Wed, 2023-06-21 at 17:36 +0200, Andrew Lunn wrote:
> > > On Wed, Jun 21, 2023 at 01:45:56PM +0800, Evan Quan wrote:
> > > > From: Mario Limonciello <[email protected]>
> > > >
> > > > Due to electrical and mechanical constraints in certain platform designs
> > > > there may be likely interference of relatively high-powered harmonics of
> > > > the (G-)DDR memory clocks with local radio module frequency bands used
> > > > by Wifi 6/6e/7.
> > > >
> > > > To mitigate this, AMD has introduced an ACPI based mechanism that
> > > > devices can use to notify active use of particular frequencies so
> > > > that devices can make relative internal adjustments as necessary
> > > > to avoid this resonance.
> > > Do only ACPI based systems have:
> > >
> > > interference of relatively high-powered harmonics of the (G-)DDR
> > > memory clocks with local radio module frequency bands used by
> > > Wifi 6/6e/7."
> > >
> > > Could Device Tree based systems not experience this problem?
> > They could, of course, but they'd need some other driver to change
> > _something_ in the system? I don't even know what this is doing
> > precisely under the hood in the ACPI BIOS, perhaps it adjusts the DDR
> > memory clock frequency in response to WiFi using a frequency that will
> > cause interference with harmonics.
>
> The way that WBRF has been architected, it's intended to be able
> to scale to any type of device pair that has harmonic issues.

So you set out to make something generic...

> In the first use (Wifi 6e + specific AMD dGPUs) that matches this
> series BIOS has the following purposes:
>
> 1) The existence of _DSM indicates that the system may not have
> adequate shielding and should be using these mitigations.
>
> 2) Notification mechanism of frequency use.
>
> For the first problematic devices we *could* have done notifications
> entirely in native Linux kernel code with notifier chains.
> However that still means you need a hint from the platform that the
> functionality is needed like a _DSD bit.
>
> It's also done this way so that AML could do some of the notifications
> directly to applicable devices in the future without needing "consumer"
> driver participation.

And then tie is very closely to ACPI.

Now, you are AMD, i get that ACPI is what you have. But i think as
kernel Maintainers, we need to consider that ACPI is not the only
thing used. Do we want the APIs to be agnostic? I think APIs used by
drivers should be agnostic.

Andrew

2023-06-21 17:34:58

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

On 6/21/2023 11:52 AM, Andrew Lunn wrote:
> On Wed, Jun 21, 2023 at 11:15:00AM -0500, Limonciello, Mario wrote:
>> On 6/21/2023 10:39 AM, Johannes Berg wrote:
>>> On Wed, 2023-06-21 at 17:36 +0200, Andrew Lunn wrote:
>>>> On Wed, Jun 21, 2023 at 01:45:56PM +0800, Evan Quan wrote:
>>>>> From: Mario Limonciello <[email protected]>
>>>>>
>>>>> Due to electrical and mechanical constraints in certain platform designs
>>>>> there may be likely interference of relatively high-powered harmonics of
>>>>> the (G-)DDR memory clocks with local radio module frequency bands used
>>>>> by Wifi 6/6e/7.
>>>>>
>>>>> To mitigate this, AMD has introduced an ACPI based mechanism that
>>>>> devices can use to notify active use of particular frequencies so
>>>>> that devices can make relative internal adjustments as necessary
>>>>> to avoid this resonance.
>>>> Do only ACPI based systems have:
>>>>
>>>> interference of relatively high-powered harmonics of the (G-)DDR
>>>> memory clocks with local radio module frequency bands used by
>>>> Wifi 6/6e/7."
>>>>
>>>> Could Device Tree based systems not experience this problem?
>>> They could, of course, but they'd need some other driver to change
>>> _something_ in the system? I don't even know what this is doing
>>> precisely under the hood in the ACPI BIOS, perhaps it adjusts the DDR
>>> memory clock frequency in response to WiFi using a frequency that will
>>> cause interference with harmonics.
>> The way that WBRF has been architected, it's intended to be able
>> to scale to any type of device pair that has harmonic issues.
> So you set out to make something generic...
>
>> In the first use (Wifi 6e + specific AMD dGPUs) that matches this
>> series BIOS has the following purposes:
>>
>> 1) The existence of _DSM indicates that the system may not have
>> adequate shielding and should be using these mitigations.
>>
>> 2) Notification mechanism of frequency use.
>>
>> For the first problematic devices we *could* have done notifications
>> entirely in native Linux kernel code with notifier chains.
>> However that still means you need a hint from the platform that the
>> functionality is needed like a _DSD bit.
>>
>> It's also done this way so that AML could do some of the notifications
>> directly to applicable devices in the future without needing "consumer"
>> driver participation.
> And then tie is very closely to ACPI.
>
> Now, you are AMD, i get that ACPI is what you have. But i think as
> kernel Maintainers, we need to consider that ACPI is not the only
> thing used. Do we want the APIs to be agnostic? I think APIs used by
> drivers should be agnostic.
>
> Andrew
I think what you're asking for is another layer of indirection
like CONFIG_WBRF in addition to CONFIG_ACPI_WBRF.

Producers would call functions like wbrf_supported_producer()
where the source file is not guarded behind CONFIG_ACPI_WBRF,
but instead by CONFIG_WBRF and locally use CONFIG_ACPI_WBRF within
it. So a producer could look like this:

bool wbrf_supported_producer(struct device *dev)
{
#ifdef CONFIG_ACPI_WBRF
    struct acpi_device *adev = ACPI_COMPANION(dev);

    if (adev)
        return check_acpi_wbrf(adev->handle,
                     WBRF_REVISION,
                     1ULL << WBRF_RECORD);
#endif
    return -ENODEV;

}
EXPORT_SYMBOL_GPL(wbrf_supported_producer);

And then adding/removing could look something like this

int wbrf_add_exclusion(struct device *dev,
               struct wbrf_ranges_in *in)
{
#ifdef CONFIG_ACPI_WBRF
    struct acpi_device *adev = ACPI_COMPANION(dev);

    if (adev)
        return wbrf_record(adev, WBRF_RECORD_ADD, in);
#endif
    return -ENODEV;
}
EXPORT_SYMBOL_GPL(wbrf_add_exclusion);

int wbrf_remove_exclusion(struct device *dev,
               struct wbrf_ranges_in *in)
{
#ifdef CONFIG_ACPI_WBRF
    struct acpi_device *adev = ACPI_COMPANION(dev);

    if (adev)
        return wbrf_record(adev, WBRF_RECORD_REMOVE, in);
#endif
    return -ENODEV;
}
EXPORT_SYMBOL_GPL(wbrf_remove_exclusion);

This would allow anyone interested in making a non-ACPI implementation
be able to slide it into those functions.

How does that sound?

2023-06-21 17:35:55

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

> Think a little more about what a non-ACPI implementation
> would look like:
>
> 1) Would producers and consumers still need you to set CONFIG_ACPI_WBRF?

I would expect there to be an CONFIG_WBRF, and then under that a
CONFIG_WBRF_ACPI, CONFIG_WBRF_DT, CONFIG_WBRF_FOOBAR, each indicating
they are mutual exclusive to the other.

> 2) How would you indicate you need WBRF support?

As far as i understand it, you have something in ACPI which indicates
it? Could that not also be a DT property?

> 3) How would notifications from one device to another work?

Linux notified chains? This is something which happens a lot in
networking. A physical interface goes down and needs to tell
team/bonding interface stack on top of it that its status has
changed. It just calls a notification chain.

> I don't think those are trivial problems that can be solved by
> just making the pointer 'struct device' particularly as with the
> ACPI implementation consumers are expecting the notification from
> ACPI.

Do consumers really care who the notification is from? Isn't the event
and its content what matters, not who sent it?

But I agree, i don't expect it is trivial. But it is going to get
harder and harder to make abstract as more and more users are
added. So we need to consider, do you want to do that work now, or
later? Do we want to merge something we know is limited and not using
the generic kernel abstractions?

Andrew

2023-06-21 17:40:30

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

> I think what you're asking for is another layer of indirection
> like CONFIG_WBRF in addition to CONFIG_ACPI_WBRF.
>
> Producers would call functions like wbrf_supported_producer()
> where the source file is not guarded behind CONFIG_ACPI_WBRF,
> but instead by CONFIG_WBRF and locally use CONFIG_ACPI_WBRF within
> it.? So a producer could look like this:
>
> bool wbrf_supported_producer(struct device *dev)
> {
> #ifdef CONFIG_ACPI_WBRF
> ??? struct acpi_device *adev = ACPI_COMPANION(dev);
>
> ??? if (adev)
> ?? ???? return check_acpi_wbrf(adev->handle,
> ??? ?? ? ?? ??? ?????? WBRF_REVISION,
> ??? ??? ?? ? ?? ?????? 1ULL << WBRF_RECORD);
> #endif
> ??? return -ENODEV;
>
> }
> EXPORT_SYMBOL_GPL(wbrf_supported_producer);
>
> And then adding/removing could look something like this
>
> int wbrf_add_exclusion(struct device *dev,
> ??? ??? ?????? struct wbrf_ranges_in *in)
> {
> #ifdef CONFIG_ACPI_WBRF
> ??? struct acpi_device *adev = ACPI_COMPANION(dev);
>
> ??? if (adev)
> ?? ???? return wbrf_record(adev, WBRF_RECORD_ADD, in);
> #endif
> ??? return -ENODEV;
> }
> EXPORT_SYMBOL_GPL(wbrf_add_exclusion);
>
> int wbrf_remove_exclusion(struct device *dev,
> ??? ??? ?????? struct wbrf_ranges_in *in)
> {
> #ifdef CONFIG_ACPI_WBRF
> ??? struct acpi_device *adev = ACPI_COMPANION(dev);
>
> ??? if (adev)
> ?? ???? return wbrf_record(adev, WBRF_RECORD_REMOVE, in);
> #endif
> ??? return -ENODEV;
> }
> EXPORT_SYMBOL_GPL(wbrf_remove_exclusion);

Yes, this looks a lot better.

But what about notifications?

Andrew

2023-06-21 17:45:29

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

On 6/21/2023 12:26 PM, Andrew Lunn wrote:
>> I think what you're asking for is another layer of indirection
>> like CONFIG_WBRF in addition to CONFIG_ACPI_WBRF.
>>
>> Producers would call functions like wbrf_supported_producer()
>> where the source file is not guarded behind CONFIG_ACPI_WBRF,
>> but instead by CONFIG_WBRF and locally use CONFIG_ACPI_WBRF within
>> it. So a producer could look like this:
>>
>> bool wbrf_supported_producer(struct device *dev)
>> {
>> #ifdef CONFIG_ACPI_WBRF
>>     struct acpi_device *adev = ACPI_COMPANION(dev);
>>
>>     if (adev)
>>         return check_acpi_wbrf(adev->handle,
>>                      WBRF_REVISION,
>>                      1ULL << WBRF_RECORD);
>> #endif
>>     return -ENODEV;
>>
>> }
>> EXPORT_SYMBOL_GPL(wbrf_supported_producer);
>>
>> And then adding/removing could look something like this
>>
>> int wbrf_add_exclusion(struct device *dev,
>>                struct wbrf_ranges_in *in)
>> {
>> #ifdef CONFIG_ACPI_WBRF
>>     struct acpi_device *adev = ACPI_COMPANION(dev);
>>
>>     if (adev)
>>         return wbrf_record(adev, WBRF_RECORD_ADD, in);
>> #endif
>>     return -ENODEV;
>> }
>> EXPORT_SYMBOL_GPL(wbrf_add_exclusion);
>>
>> int wbrf_remove_exclusion(struct device *dev,
>>                struct wbrf_ranges_in *in)
>> {
>> #ifdef CONFIG_ACPI_WBRF
>>     struct acpi_device *adev = ACPI_COMPANION(dev);
>>
>>     if (adev)
>>         return wbrf_record(adev, WBRF_RECORD_REMOVE, in);
>> #endif
>>     return -ENODEV;
>> }
>> EXPORT_SYMBOL_GPL(wbrf_remove_exclusion);
> Yes, this looks a lot better.
>
> But what about notifications?
Once you implement this it gets a lot more complex and the driver
consumers would need
to know more about the kernel's implementation. For example consumers
need a
notifier block like:

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index e3e2e6e3b485..146fe3c43343 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1066,6 +1066,8 @@ struct amdgpu_device {

        bool                            job_hang;
        bool                            dc_enabled;
+
+       struct notifier_block           wbrf_notifier;
};

static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev)

And then would need matching notifier functions like:

static int amdgpu_wbrf_frequencies_notifier(struct notifier_block *nb,
                                    unsigned long action, void *_arg)

And we'd need to set up a chain to be used in this case in the WBRF code:

static BLOCKING_NOTIFIER_HEAD(wbrf_chain_head);

int wbrf_register_notifier(struct notifier_block *nb)
{
   return blocking_notifier_chain_register(&wbrf_chain_head, nb);
}
EXPORT_SYMBOL_GPL(wbrf_register_notifier);

int wbrf_unregister_notifier(struct notifier_block *nb)
{
   return blocking_notifier_chain_unregister(&wbrf_chain_head, nb);
}
EXPORT_SYMBOL_GPL(wbrf_unregister_notifier);

And consumer would need to call it, but only if CONFIG_WBRF_ACPI isn't set.

Add/remove functions can easily call something like:

blocking_notifier_call_chain(&wbrf_chain_head, action, data);

With all of this complexity and (effectively) dead code for ACPI vs non-ACPI
path I really have to ask why wouldn't a non-AMD implementation be able to
do this as ACPI?

I don't see why it couldn't be a DT/ACPI hybrid solution for ARM64.

2023-06-21 18:41:02

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

> And consumer would need to call it, but only if CONFIG_WBRF_ACPI isn't set.

Why? How is ACPI special that it does not need notifiers?

> I don't see why it couldn't be a DT/ACPI hybrid solution for ARM64.

As said somewhere else, nobody does hybrid. In fact, turn it
around. Why not implement all this in DT, and make X86 hybrid? That
will make arm, powerpc, risc-v and mips much simpler :-)

Andrew

2023-06-21 19:00:21

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

So if we go down this path of CONFIG_WBRF and CONFIG_WBRF_ACPI, another
question would be where should the new "wbrf.c" be stored? The ACPI only
version most certainly made sense in drivers/acpi/wbrf.c, but a generic
version that only has an ACPI implementation right now not so much.

On 6/21/2023 1:30 PM, Andrew Lunn wrote:
>> And consumer would need to call it, but only if CONFIG_WBRF_ACPI isn't set.
> Why? How is ACPI special that it does not need notifiers?
ACPI core does has notifiers that are used, but they don't work the same.
If you look at patch 4, you'll see amdgpu registers and unregisters using
both

acpi_install_notify_handler()
and
acpi_remove_notify_handler()

If we supported both ACPI notifications and non-ACPI notifications
all consumers would have to have support to register and use both types.

>
>> I don't see why it couldn't be a DT/ACPI hybrid solution for ARM64.
> As said somewhere else, nobody does hybrid. In fact, turn it
> around. Why not implement all this in DT, and make X86 hybrid? That
> will make arm, powerpc, risc-v and mips much simpler :-)
>
> Andrew
Doesn't coreboot do something hybrid with device tree? I thought they
generate their ACPI tables from a combination of DT and some static ASL.

2023-06-21 19:38:41

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

> ACPI core does has notifiers that are used, but they don't work the same.
> If you look at patch 4, you'll see amdgpu registers and unregisters using
> both
>
> acpi_install_notify_handler()
> and
> acpi_remove_notify_handler()
>
> If we supported both ACPI notifications and non-ACPI notifications
> all consumers would have to have support to register and use both types.

Why would you want to support ACPI notifications and non-ACPI
notifications? All you need is wbrf notification.

The new wbrf.c should implement wbrf_install_notify_handler() and
wbrf_remove_notify_handler().

As to where to put wbrf.c? I guess either drivers/base/ or
drivers/wbrf/. Maybe ask GregKH?

Andrew

2023-06-21 22:35:56

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

On Wed, 2023-06-21 at 21:25 +0200, Andrew Lunn wrote:
> > ACPI core does has notifiers that are used, but they don't work the same.
> > If you look at patch 4, you'll see amdgpu registers and unregisters using
> > both
> >
> > acpi_install_notify_handler()
> > and
> > acpi_remove_notify_handler()
> >
> > If we supported both ACPI notifications and non-ACPI notifications
> > all consumers would have to have support to register and use both types.
>
> Why would you want to support ACPI notifications and non-ACPI
> notifications? All you need is wbrf notification.
>
> The new wbrf.c should implement wbrf_install_notify_handler() and
> wbrf_remove_notify_handler().
>
> As to where to put wbrf.c? I guess either drivers/base/ or
> drivers/wbrf/. Maybe ask GregKH?

Not sure it should even be called WBRF at that point, but hey :)

Honestly I'm not sure though we need this complexity right now? I mean,
it'd be really easy to replace the calls in mac80211 with some other
more generalised calls in the future?

You need some really deep platform/hardware level knowledge and
involvement to do this, so I don't think it's something that someone
will come up with very easily for a DT-based platform...

If we do something with a notifier chain in the future, we can just
install one in the ACPI code too, and react indirectly rather than
calling from wifi to the ACPI directly.

johannes

2023-06-22 02:01:53

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

> Honestly I'm not sure though we need this complexity right now? I mean,
> it'd be really easy to replace the calls in mac80211 with some other
> more generalised calls in the future?
>
> You need some really deep platform/hardware level knowledge and
> involvement to do this, so I don't think it's something that someone
> will come up with very easily for a DT-based platform...

What is this API about?

It is a struct device says, i'm badly designed and make a mess of the
following frequency bands. Optionally, if you ask me nicely, i might
be able to tweak what i'm doing to avoid interfering with you.

And it is about a struct device say, i'm using this particular
frequency. If you can reduce the noise you make, i would be thankful.

The one generating the noise could be anything. The PWM driving my
laptop display back light?, What is being interfered with? The 3.5mm
audio jack?

How much deep system knowledge is needed to call pwm_set_state() to
move the base frequency up above 20Khz so only my dog will hear it?
But at the cost of a loss of efficiency and my battery going flatter
faster?

Is the DDR memory really the only badly designed component, when you
think of the range of systems Linux is used on from PHC to tiny
embedded systems?

Ideally we want any sort of receiver with a low noise amplifier to
just unconditionally use this API to let rest of the system know about
it. And ideally we want anything which is a source of noise to declare
itself. What happens after that should be up to the struct device
causing the interference.

Mario did say:

The way that WBRF has been architected, it's intended to be able to
scale to any type of device pair that has harmonic issues.

Andrew

2023-06-22 20:38:57

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

On 6/21/2023 8:55 PM, Andrew Lunn wrote:
>> Honestly I'm not sure though we need this complexity right now? I mean,
>> it'd be really easy to replace the calls in mac80211 with some other
>> more generalised calls in the future?
>>
>> You need some really deep platform/hardware level knowledge and
>> involvement to do this, so I don't think it's something that someone
>> will come up with very easily for a DT-based platform...
> What is this API about?
>
> It is a struct device says, i'm badly designed and make a mess of the
> following frequency bands. Optionally, if you ask me nicely, i might
> be able to tweak what i'm doing to avoid interfering with you.
>
> And it is about a struct device say, i'm using this particular
> frequency. If you can reduce the noise you make, i would be thankful.
Hey now - you're making assumptions about what's badly designed.

At it's core the issue here that prompts all of this is a
"platform" issue with the tiny Z heights laptops these days
strive for causing implied limitations for shielding.

Independently both components work just fine.

>
> The one generating the noise could be anything. The PWM driving my
> laptop display back light?, What is being interfered with? The 3.5mm
> audio jack?
>
> How much deep system knowledge is needed to call pwm_set_state() to
> move the base frequency up above 20Khz so only my dog will hear it?
> But at the cost of a loss of efficiency and my battery going flatter
> faster?
>
> Is the DDR memory really the only badly designed component, when you
> think of the range of systems Linux is used on from PHC to tiny
> embedded systems?
>
> Ideally we want any sort of receiver with a low noise amplifier to
> just unconditionally use this API to let rest of the system know about
> it. And ideally we want anything which is a source of noise to declare
> itself. What happens after that should be up to the struct device
> causing the interference.
I do get your point here - but the problem with a PWM on your
laptop display interfering with the 3.5mm audio jack would
likely be localized to your specific model.

If you have the 16" version of the laptop and I have the 13"
version I might have the 3.5mm audio jack in another location,
that is better shielded and so making that assumption that we
both have the same components so need to make changes could be
totally wrong.

If you have EVERYTHING with an amplifier advertising frequencies
in use without any extra information about the location of the
component or the impacts that component can have you're going
to have a useless interface that is just a bunch of garbage data.

I really think the application of this type of software
mitigation should be reserved for system designers that made
those design decisions and know they are going to run into problems.

> Mario did say:
>
> The way that WBRF has been architected, it's intended to be able to
> scale to any type of device pair that has harmonic issues.
>
> Andrew
The types of things that we envisioned were high frequency devices
with larger power emissions. For example WWAN or USB4 devices.
These fit well into the ACPI device model.

When Evan gets back from holiday I'll discuss with him the ideas from
this thread.

However before then I would really appreciate if Rafael can provide
some comments on patch 1 as it stands today.

2023-06-22 21:41:29

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

On Wed, Jun 21, 2023 at 01:50:34PM -0500, Limonciello, Mario wrote:
> So if we go down this path of CONFIG_WBRF and CONFIG_WBRF_ACPI, another
> question would be where should the new "wbrf.c" be stored?? The ACPI only
> version most certainly made sense in drivers/acpi/wbrf.c, but a generic
> version that only has an ACPI implementation right now not so much.
>
> On 6/21/2023 1:30 PM, Andrew Lunn wrote:
> > > And consumer would need to call it, but only if CONFIG_WBRF_ACPI isn't set.
> > Why? How is ACPI special that it does not need notifiers?
> ACPI core does has notifiers that are used, but they don't work the same.
> If you look at patch 4, you'll see amdgpu registers and unregisters using
> both
>
> acpi_install_notify_handler()
> and
> acpi_remove_notify_handler()
>
> If we supported both ACPI notifications and non-ACPI notifications
> all consumers would have to have support to register and use both types.

I took a quick look at this:

#define CPM_GPU_NOTIFY_COMMAND 0x55
+static void amdgpu_acpi_wbrf_event(acpi_handle handle, u32 event, void *data)
+{
+ struct amdgpu_device *adev = (struct amdgpu_device *)data;
+
+ if (event == CPM_GPU_NOTIFY_COMMAND &&
+ adev->wbrf_event_handler)
+ adev->wbrf_event_handler(adev);
+}

handle is ignored, All you need is the void * data to link back to
your private data.

I find it interesting that CPM_GPU_NOTIFY_COMMAND is defined here. So
nothing else can use it. Should it maybe be called
CPM_AMDGPU_NOTIFY_COMMAND?

Overall, looking at this, i don't see anything which could not be made
abstract:

static void amdgpu_wbrf_event(u32 event, void *data)
{
struct amdgpu_device *adev = (struct amdgpu_device *)data;

if (event == WBRF_GPU_NOTIFY_COMMAND &&
adev->wbrf_event_handler)
adev->wbrf_event_handler(adev);
}

int amdgpu_register_wbrf_notify_handler(struct amdgpu_device *adev,
wbrf_notify_handler handler)
{
struct device *dev = adev->dev);

adev->wbrf_event_handler = handler;

return wbrf_install_notify_handler(dev, amdgpu_wbrf_event, adev);
}

Andrew

2023-06-23 14:58:52

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

2023-06-23 16:42:56

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

On 6/23/2023 9:52 AM, Rafael J. Wysocki wrote:
> On Wed, Jun 21, 2023 at 7:47 AM Evan Quan <[email protected]> wrote:
>> From: Mario Limonciello <[email protected]>
>>
>> Due to electrical and mechanical constraints in certain platform designs
>> there may be likely interference of relatively high-powered harmonics of
>> the (G-)DDR memory clocks with local radio module frequency bands used
>> by Wifi 6/6e/7.
>>
>> To mitigate this, AMD has introduced an ACPI based mechanism that
>> devices can use to notify active use of particular frequencies so
>> that devices can make relative internal adjustments as necessary
>> to avoid this resonance.
>>
>> In order for a device to support this, the expected flow for device
>> driver or subsystems:
>>
>> Drivers/subsystems contributing frequencies:
>>
>> 1) During probe, check `wbrf_supported_producer` to see if WBRF supported
> The prefix should be acpi_wbrf_ or acpi_amd_wbrf_ even, so it is clear
> that this uses ACPI and is AMD-specific.

I guess if we end up with an intermediary library approach
wbrf_supported_producer makes sense and that could call acpi_wbrf_*.

But with no intermediate library your suggestion makes sense.

I would prefer not to make it acpi_amd as there is no reason that
this exact same problem couldn't happen on an
Wifi 6e + Intel SOC + AMD dGPU design too and OEMs could use the
same mitigation mechanism as Wifi6e + AMD SOC + AMD dGPU too.

>
> Whether or not there needs to be an intermediate library wrapped
> around this is a different matter.

2023-06-23 16:43:18

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

On Fri, Jun 23, 2023 at 5:57 PM Limonciello, Mario
<[email protected]> wrote:
>
>
> On 6/23/2023 9:52 AM, Rafael J. Wysocki wrote:
> > On Wed, Jun 21, 2023 at 7:47 AM Evan Quan <[email protected]> wrote:
> >> From: Mario Limonciello <[email protected]>
> >>
> >> Due to electrical and mechanical constraints in certain platform designs
> >> there may be likely interference of relatively high-powered harmonics of
> >> the (G-)DDR memory clocks with local radio module frequency bands used
> >> by Wifi 6/6e/7.
> >>
> >> To mitigate this, AMD has introduced an ACPI based mechanism that
> >> devices can use to notify active use of particular frequencies so
> >> that devices can make relative internal adjustments as necessary
> >> to avoid this resonance.
> >>
> >> In order for a device to support this, the expected flow for device
> >> driver or subsystems:
> >>
> >> Drivers/subsystems contributing frequencies:
> >>
> >> 1) During probe, check `wbrf_supported_producer` to see if WBRF supported
> > The prefix should be acpi_wbrf_ or acpi_amd_wbrf_ even, so it is clear
> > that this uses ACPI and is AMD-specific.
>
> I guess if we end up with an intermediary library approach
> wbrf_supported_producer makes sense and that could call acpi_wbrf_*.
>
> But with no intermediate library your suggestion makes sense.
>
> I would prefer not to make it acpi_amd as there is no reason that
> this exact same problem couldn't happen on an
> Wifi 6e + Intel SOC + AMD dGPU design too and OEMs could use the
> same mitigation mechanism as Wifi6e + AMD SOC + AMD dGPU too.

The mitigation mechanism might be the same, but the AML interface very
well may be different.

My point is that this particular interface is AMD-specific ATM and I'm
not aware of any plans to make it "standard" in some way.

Also if the given interface is specified somewhere, it would be good
to have a pointer to that place.

> >
> > Whether or not there needs to be an intermediate library wrapped
> > around this is a different matter.

IMO individual drivers should not be expected to use this interface
directly, as that would add to boilerplate code and overall bloat.

Also whoever uses it, would first need to check if the device in
question has an ACPI companion.

2023-06-23 16:46:02

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

On Wed, Jun 21, 2023 at 7:47 AM Evan Quan <[email protected]> wrote:
>
> From: Mario Limonciello <[email protected]>
>
> Due to electrical and mechanical constraints in certain platform designs
> there may be likely interference of relatively high-powered harmonics of
> the (G-)DDR memory clocks with local radio module frequency bands used
> by Wifi 6/6e/7.
>
> To mitigate this, AMD has introduced an ACPI based mechanism that
> devices can use to notify active use of particular frequencies so
> that devices can make relative internal adjustments as necessary
> to avoid this resonance.
>
> In order for a device to support this, the expected flow for device
> driver or subsystems:
>
> Drivers/subsystems contributing frequencies:
>
> 1) During probe, check `wbrf_supported_producer` to see if WBRF supported
> for the device.
> 2) If adding frequencies, then call `wbrf_add_exclusion` with the
> start and end ranges of the frequencies.
> 3) If removing frequencies, then call `wbrf_remove_exclusion` with
> start and end ranges of the frequencies.
>
> Drivers/subsystems responding to frequencies:
>
> 1) During probe, check `wbrf_supported_consumer` to see if WBRF is supported
> for the device.
> 2) Call the `wbrf_retrieve_exclusions` to retrieve the current
> exclusions on receiving an ACPI notification for a new frequency
> change.
>
> Signed-off-by: Mario Limonciello <[email protected]>
> Co-developed-by: Evan Quan <[email protected]>
> Signed-off-by: Evan Quan <[email protected]>
> --
> v1->v2:
> - move those wlan specific implementations to net/mac80211(Mario)
> ---
> drivers/acpi/Kconfig | 7 ++
> drivers/acpi/Makefile | 2 +
> drivers/acpi/acpi_wbrf.c | 215 +++++++++++++++++++++++++++++++++++++++
> include/linux/wbrf.h | 55 ++++++++++
> 4 files changed, 279 insertions(+)
> create mode 100644 drivers/acpi/acpi_wbrf.c
> create mode 100644 include/linux/wbrf.h
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index ccbeab9500ec..0276c1487fa2 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -611,3 +611,10 @@ config X86_PM_TIMER
>
> You should nearly always say Y here because many modern
> systems require this timer.
> +
> +config ACPI_WBRF
> + bool "ACPI Wifi band RF mitigation mechanism"
> + help
> + Wifi band RF mitigation mechanism allows multiple drivers from
> + different domains to notify the frequencies in use so that hardware
> + can be reconfigured to avoid harmonic conflicts.
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index feb36c0b9446..14863b0c619f 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -131,3 +131,5 @@ obj-y += dptf/
> obj-$(CONFIG_ARM64) += arm64/
>
> obj-$(CONFIG_ACPI_VIOT) += viot.o
> +
> +obj-$(CONFIG_ACPI_WBRF) += acpi_wbrf.o
> diff --git a/drivers/acpi/acpi_wbrf.c b/drivers/acpi/acpi_wbrf.c
> new file mode 100644
> index 000000000000..8c275998ac29
> --- /dev/null
> +++ b/drivers/acpi/acpi_wbrf.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Wifi Band Exclusion Interface

Where is the AML interface for this defined and how does it work?

> + * Copyright (C) 2023 Advanced Micro Devices
> + *
> + */
> +
> +#include <linux/wbrf.h>
> +
> +/* functions */
> +#define WBRF_RECORD 0x1
> +#define WBRF_RETRIEVE 0x2
> +
> +/* record actions */
> +#define WBRF_RECORD_ADD 0x0
> +#define WBRF_RECORD_REMOVE 0x1
> +
> +#define WBRF_REVISION 0x1
> +
> +static const guid_t wifi_acpi_dsm_guid =
> + GUID_INIT(0x7b7656cf, 0xdc3d, 0x4c1c,
> + 0x83, 0xe9, 0x66, 0xe7, 0x21, 0xde, 0x30, 0x70);
> +
> +static int wbrf_dsm(struct acpi_device *adev, u8 fn,
> + union acpi_object *argv4,
> + union acpi_object **out)
> +{
> + union acpi_object *obj;
> + int rc;
> +
> + obj = acpi_evaluate_dsm(adev->handle, &wifi_acpi_dsm_guid,
> + WBRF_REVISION, fn, argv4);
> + if (!obj)
> + return -ENXIO;
> +
> + switch (obj->type) {
> + case ACPI_TYPE_BUFFER:
> + if (!*out) {
> + rc = -EINVAL;
> + break;

I'm not sure why you want to return an error in this case. Did you
really mean !out?

> + }
> + *out = obj;
> + return 0;
> +
> + case ACPI_TYPE_INTEGER:
> + rc = obj->integer.value ? -EINVAL : 0;
> + break;

An empty line here, please, as you added one after the return statement above.

> + default:
> + rc = -EOPNOTSUPP;
> + }
> + ACPI_FREE(obj);
> +
> + return rc;

How does the caller know whether or not they need to free the out
object after calling this function?

> +}
> +
> +static int wbrf_record(struct acpi_device *adev, uint8_t action,
> + struct wbrf_ranges_in *in)
> +{
> + union acpi_object *argv4;
> + uint32_t num_of_ranges = 0;
> + uint32_t arg_idx = 0;
> + uint32_t loop_idx;
> + int ret;
> +
> + if (!in)
> + return -EINVAL;
> +
> + for (loop_idx = 0; loop_idx < ARRAY_SIZE(in->band_list);
> + loop_idx++)
> + if (in->band_list[loop_idx].start &&
> + in->band_list[loop_idx].end)
> + num_of_ranges++;
> +
> + argv4 = kzalloc(sizeof(*argv4) * (2 * num_of_ranges + 2 + 1), GFP_KERNEL);
> + if (!argv4)
> + return -ENOMEM;
> +
> + argv4[arg_idx].package.type = ACPI_TYPE_PACKAGE;
> + argv4[arg_idx].package.count = 2 + 2 * num_of_ranges;
> + argv4[arg_idx++].package.elements = &argv4[1];
> + argv4[arg_idx].integer.type = ACPI_TYPE_INTEGER;
> + argv4[arg_idx++].integer.value = num_of_ranges;
> + argv4[arg_idx].integer.type = ACPI_TYPE_INTEGER;
> + argv4[arg_idx++].integer.value = action;
> +
> + for (loop_idx = 0; loop_idx < ARRAY_SIZE(in->band_list);
> + loop_idx++) {
> + if (!in->band_list[loop_idx].start ||
> + !in->band_list[loop_idx].end)
> + continue;
> +
> + argv4[arg_idx].integer.type = ACPI_TYPE_INTEGER;
> + argv4[arg_idx++].integer.value = in->band_list[loop_idx].start;
> + argv4[arg_idx].integer.type = ACPI_TYPE_INTEGER;
> + argv4[arg_idx++].integer.value = in->band_list[loop_idx].end;
> + }
> +
> + ret = wbrf_dsm(adev, WBRF_RECORD, argv4, NULL);
> +
> + kfree(argv4);
> +
> + return ret;
> +}
> +
> +int wbrf_add_exclusion(struct acpi_device *adev,
> + struct wbrf_ranges_in *in)
> +{
> + return wbrf_record(adev, WBRF_RECORD_ADD, in);
> +}
> +EXPORT_SYMBOL_GPL(wbrf_add_exclusion);
> +
> +int wbrf_remove_exclusion(struct acpi_device *adev,
> + struct wbrf_ranges_in *in)
> +{
> + return wbrf_record(adev, WBRF_RECORD_REMOVE, in);
> +}
> +EXPORT_SYMBOL_GPL(wbrf_remove_exclusion);
> +
> +bool wbrf_supported_producer(struct acpi_device *adev)
> +{
> + return acpi_check_dsm(adev->handle, &wifi_acpi_dsm_guid,
> + WBRF_REVISION,
> + (1ULL << WBRF_RECORD) | (1ULL << WBRF_RETRIEVE));

I'm wondering if the BIT() macro would work here (and below).

> +}
> +EXPORT_SYMBOL_GPL(wbrf_supported_producer);
> +
> +static union acpi_object *
> +acpi_evaluate_wbrf(acpi_handle handle, u64 rev, u64 func)
> +{
> + acpi_status ret;
> + struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
> + union acpi_object params[4];
> + struct acpi_object_list input = {
> + .count = 4,
> + .pointer = params,
> + };
> +
> + params[0].type = ACPI_TYPE_INTEGER;
> + params[0].integer.value = rev;
> + params[1].type = ACPI_TYPE_INTEGER;
> + params[1].integer.value = func;
> + params[2].type = ACPI_TYPE_PACKAGE;
> + params[2].package.count = 0;
> + params[2].package.elements = NULL;
> + params[3].type = ACPI_TYPE_STRING;
> + params[3].string.length = 0;
> + params[3].string.pointer= NULL;
> +
> + ret = acpi_evaluate_object(handle, "WBRF", &input, &buf);
> + if (ACPI_SUCCESS(ret))
> + return (union acpi_object *)buf.pointer;
> +
> + if (ret != AE_NOT_FOUND)
> + acpi_handle_warn(handle,
> + "failed to evaluate WBRF(0x%x)\n", ret);

Why _warn()?

> +
> + return NULL;
> +}
> +
> +static bool check_acpi_wbrf(acpi_handle handle, u64 rev, u64 funcs)
> +{
> + int i;
> + u64 mask = 0;
> + union acpi_object *obj;
> +
> + if (funcs == 0)
> + return false;
> +
> + obj = acpi_evaluate_wbrf(handle, rev, 0);
> + if (!obj)
> + return false;
> +
> + if (obj->type != ACPI_TYPE_BUFFER)
> + return false;
> +
> + for (i = 0; i < obj->buffer.length && i < 8; i++)
> + mask |= (((u64)obj->buffer.pointer[i]) << (i * 8));

What is going on here? Any comment to explain it?

> + ACPI_FREE(obj);
> +
> + /*
> + * Bit 0 indicates whether there's support for any functions other than
> + * function 0.
> + */
> + if ((mask & 0x1) && (mask & funcs) == funcs)
> + return true;
> +
> + return false;
> +}
> +
> +bool wbrf_supported_consumer(struct acpi_device *adev)
> +{
> + return check_acpi_wbrf(adev->handle,
> + WBRF_REVISION,
> + 1ULL << WBRF_RETRIEVE);
> +}
> +EXPORT_SYMBOL_GPL(wbrf_supported_consumer);
> +
> +int wbrf_retrieve_exclusions(struct acpi_device *adev,
> + struct wbrf_ranges_out *exclusions_out)
> +{
> + union acpi_object *obj;
> +
> + obj = acpi_evaluate_wbrf(adev->handle,
> + WBRF_REVISION,
> + WBRF_RETRIEVE);
> + if (!obj)
> + return -EINVAL;
> +
> + memcpy(exclusions_out, obj->buffer.pointer, obj->buffer.length);

How is it guaranteed that the length of the buffer will not exceed the
size of memory allocated by the caller for the data?

> +
> + ACPI_FREE(obj);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(wbrf_retrieve_exclusions);
> diff --git a/include/linux/wbrf.h b/include/linux/wbrf.h
> new file mode 100644
> index 000000000000..e4c99b69f1d2
> --- /dev/null
> +++ b/include/linux/wbrf.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * AMD Wifi Band Exclusion Interface
> + * Copyright (C) 2023 Advanced Micro Devices
> + */
> +
> +#ifndef _LINUX_WBRF_H
> +#define _LINUX_WBRF_H
> +
> +#include <linux/acpi.h>
> +
> +/* Maximum number of wbrf ranges */
> +#define MAX_NUM_OF_WBRF_RANGES 11
> +
> +struct exclusion_range {
> + /* start and end point of the frequency range in Hz */
> + uint64_t start;
> + uint64_t end;
> +};
> +
> +struct wbrf_ranges_in {
> + /* valid entry: `start` and `end` filled with non-zero values */
> + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES];
> +};
> +
> +struct wbrf_ranges_out {
> + uint32_t num_of_ranges;
> + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES];
> +} __attribute__((packed));
> +
> +/**
> + * APIs needed by drivers/subsystems for contributing frequencies:
> + * During probe, check `wbrf_supported_producer` to see if WBRF is supported.
> + * If adding frequencies, then call `wbrf_add_exclusion` with the
> + * start and end points specified for the frequency ranges added.
> + * If removing frequencies, then call `wbrf_remove_exclusion` with
> + * start and end points specified for the frequency ranges added.
> + */
> +bool wbrf_supported_producer(struct acpi_device *adev);
> +int wbrf_add_exclusion(struct acpi_device *adev,
> + struct wbrf_ranges_in *in);
> +int wbrf_remove_exclusion(struct acpi_device *adev,
> + struct wbrf_ranges_in *in);
> +
> +/**
> + * APIs needed by drivers/subsystems responding to frequencies:
> + * During probe, check `wbrf_supported_consumer` to see if WBRF is supported.
> + * When receiving an ACPI notification for some frequencies change, run
> + * `wbrf_retrieve_exclusions` to retrieve the latest frequencies ranges.
> + */
> +int wbrf_retrieve_exclusions(struct acpi_device *adev,
> + struct wbrf_ranges_out *out);
> +bool wbrf_supported_consumer(struct acpi_device *adev);
> +
> +#endif /* _LINUX_WBRF_H */
> --
> 2.34.1
>

2023-06-23 16:51:52

[permalink] [raw]

Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

On 6/23/2023 11:28 AM, Rafael J. Wysocki wrote:
> On Fri, Jun 23, 2023 at 5:57 PM Limonciello, Mario
> <[email protected]> wrote:
>>
>> On 6/23/2023 9:52 AM, Rafael J. Wysocki wrote:
>>> On Wed, Jun 21, 2023 at 7:47 AM Evan Quan <[email protected]> wrote:
>>>> From: Mario Limonciello <[email protected]>
>>>>
>>>> Due to electrical and mechanical constraints in certain platform designs
>>>> there may be likely interference of relatively high-powered harmonics of
>>>> the (G-)DDR memory clocks with local radio module frequency bands used
>>>> by Wifi 6/6e/7.
>>>>
>>>> To mitigate this, AMD has introduced an ACPI based mechanism that
>>>> devices can use to notify active use of particular frequencies so
>>>> that devices can make relative internal adjustments as necessary
>>>> to avoid this resonance.
>>>>
>>>> In order for a device to support this, the expected flow for device
>>>> driver or subsystems:
>>>>
>>>> Drivers/subsystems contributing frequencies:
>>>>
>>>> 1) During probe, check `wbrf_supported_producer` to see if WBRF supported
>>> The prefix should be acpi_wbrf_ or acpi_amd_wbrf_ even, so it is clear
>>> that this uses ACPI and is AMD-specific.
>> I guess if we end up with an intermediary library approach
>> wbrf_supported_producer makes sense and that could call acpi_wbrf_*.
>>
>> But with no intermediate library your suggestion makes sense.
>>
>> I would prefer not to make it acpi_amd as there is no reason that
>> this exact same problem couldn't happen on an
>> Wifi 6e + Intel SOC + AMD dGPU design too and OEMs could use the
>> same mitigation mechanism as Wifi6e + AMD SOC + AMD dGPU too.
> The mitigation mechanism might be the same, but the AML interface very
> well may be different.

Right. I suppose right now we should keep it prefixed as "amd",
and if it later is promoted as a standard it can be renamed.

>
> My point is that this particular interface is AMD-specific ATM and I'm
> not aware of any plans to make it "standard" in some way.

Yeah; this implementation is currently AMD specific AML, but I
expect the exact same AML would be delivered to OEMs using the
dGPUs.

>
> Also if the given interface is specified somewhere, it would be good
> to have a pointer to that place.

It's a code first implementation. I'm discussing with the
owners when they will release it.

>
>>> Whether or not there needs to be an intermediate library wrapped
>>> around this is a different matter.
> IMO individual drivers should not be expected to use this interface
> directly, as that would add to boilerplate code and overall bloat.

The thing is the ACPI method is not a platform method. It's
a function of the device (_DSM).

The reason for having acpi_wbrf.c in the first place is to
avoid the boilerplate of the _DSM implementation across multiple
drivers.

>
> Also whoever uses it, would first need to check if the device in
> question has an ACPI companion.

Which comes back to Andrew's point.
Either we:

Have a generic wbrf_ helper that takes struct *device and
internally checks if there is an ACPI companion and support.

or

Do the check for support in mac80211 + applicable drivers
and only call the AMD WBRF ACPI method in those drivers in
those cases.

2023-06-30 10:53:09