Re: [PATCH v12 2/9] platform/x86/amd: Add support for AMD ACPI based Wifi band RFI mitigation feature
Hi,
On 10/17/23 04:53, Ma Jun wrote:
> 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 a mechanism that devices can use to
> notify active use of particular frequencies so that other devices can make
> relative internal adjustments as necessary to avoid this resonance.
>
> Co-Developed-by: Evan Quan
> Signed-off-by: Evan Quan
> Signed-off-by: Ma Jun
> +bool acpi_amd_wbrf_supported_producer(struct device *dev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> + if (!adev)
> + return false;
> +
> + if (!acpi_amd_wbrf_supported_system())
> + return false;
> +
> +
> + return acpi_check_dsm(adev->handle, &wifi_acpi_dsm_guid,
> + WBRF_REVISION,
> + BIT(WBRF_RECORD));
> +}
> +EXPORT_SYMBOL_GPL(acpi_amd_wbrf_supported_producer);
So until here you use acpi_dsm methods (1), which matches
with patch 1/9 which says that both producers and consumers
use a _DSM for WBRF.
1) With the exception of the weird acpi_amd_wbrf_supported_system()
helper.
> +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_FAILURE(ret))
> + return NULL;
> +
> + return buf.pointer;
> +}
But now all of a sudden you start calling a WBRF method
directly instead of calling a _DSM by GUID, which seems
to be intended for consumers.
This contradicts with the documentation which says that
consumers also use the _DSM.
And this looks a lot like acpi_evaluate_dsm and
... (continued below)
> +
> +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;
> +
> + /*
> + * Bit vector providing supported functions information.
> + * Each bit marks support for one specific function of the WBRF method.
> + */
> + for (i = 0; i < obj->buffer.length && i < 8; i++)
> + mask |= (u64)obj->buffer.pointer[i] << i * 8;
> +
> + ACPI_FREE(obj);
> +
> + if ((mask & BIT(WBRF_ENABLED)) && (mask & funcs) == funcs)
> + return true;
> +
> + return false;
> +}
This looks exactly like acpi_check_dsm().
> +
> +/**
> + * acpi_amd_wbrf_supported_consumer - determine if the WBRF can be enabled
> + *for the device as a consumer
> + *
> + * @dev: device pointer
> + *
> + * Determine if the platform equipped with necessary implementations to
> + * support WBRF for the device as a consumer.
> + *
> + * Return:
> + * true if WBRF is supported, otherwise returns false.
> + */
> +bool acpi_amd_wbrf_supported_consumer(struct device *dev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> + if (!adev)
> + return false;
> +
> + if (!acpi_amd_wbrf_supported_system())
> + return false;
> +
> + return check_acpi_wbrf(adev->handle,
> +WBRF_REVISION,
> +BIT(WBRF_RETRIEVE));
> +}
> +EXPORT_SYMBOL_GPL(acpi_amd_wbrf_supported_consumer);
So I would expect this to just use acpi_check_dsm like
is done for the producers.
> +
> +/**
> + * amd_wbrf_retrieve_freq_band - retrieve current active frequency
> + * bands
> + *
> + * @dev: device pointer
> + * @out: output structure containing all the active frequency bands
> + *
> + * Retrieve the current active frequency bands which were broadcasted
> + * by other producers. The consumer who calls this API should take
> + * proper actions if any of the frequency band may cause RFI with its
> + * own frequency band used.
> + *
> + * Return:
> + * 0 for getting wifi freq band successfully.
> + * Returns a negative error code for failure.
> + */
> +int amd_wbrf_retrieve_freq_band(s
Re: [PATCH v12 2/9] platform/x86/amd: Add support for AMD ACPI based Wifi band RFI mitigation feature
On Tue, 17 Oct 2023, Ma Jun wrote:
> 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 a mechanism that devices can use to
> notify active use of particular frequencies so that other devices can make
> relative internal adjustments as necessary to avoid this resonance.
>
> Co-Developed-by: Evan Quan
> Signed-off-by: Evan Quan
> Signed-off-by: Ma Jun
>
> --
> v11:
> - fix typo(Simon)
> v12:
> - Fix the code (Rafael)
> - Move amd_wbrf.c to drivers/platform/x86/amd/wbrf.c
> - Updated Evan's email because he's no longer at AMD.Thanks
> for his work in earlier versions.
> ---
> drivers/platform/x86/amd/Kconfig | 15 ++
> drivers/platform/x86/amd/Makefile | 1 +
> drivers/platform/x86/amd/wbrf.c | 402 ++
> include/linux/acpi_amd_wbrf.h | 101
> 4 files changed, 519 insertions(+)
> create mode 100644 drivers/platform/x86/amd/wbrf.c
> create mode 100644 include/linux/acpi_amd_wbrf.h
>
> diff --git a/drivers/platform/x86/amd/Kconfig
> b/drivers/platform/x86/amd/Kconfig
> index d9685aef0887..fa5a978a2d22 100644
> --- a/drivers/platform/x86/amd/Kconfig
> +++ b/drivers/platform/x86/amd/Kconfig
> @@ -32,3 +32,18 @@ config AMD_HSMP
>
> If you choose to compile this driver as a module the module will be
> called amd_hsmp.
> +
> +config AMD_WBRF
> + bool "AMD Wifi RF Band mitigations (WBRF)"
> + depends on ACPI
> + default n
> + help
> + WBRF(Wifi Band RFI mitigation) mechanism allows Wifi drivers
> + to notify the frequencies they are using so that other hardware
> + can be reconfigured to avoid harmonic conflicts.
> +
> + AMD provides an ACPI based mechanism to support WBRF on platform with
> + appropriate underlying support.
> +
> + This mechanism will only be activated on platforms that advertise a
> + need for it.
> diff --git a/drivers/platform/x86/amd/Makefile
> b/drivers/platform/x86/amd/Makefile
> index 65732f0a3913..62b98b048b17 100644
> --- a/drivers/platform/x86/amd/Makefile
> +++ b/drivers/platform/x86/amd/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_AMD_PMC) += amd-pmc.o
> amd_hsmp-y := hsmp.o
> obj-$(CONFIG_AMD_HSMP) += amd_hsmp.o
> obj-$(CONFIG_AMD_PMF)+= pmf/
> +obj-$(CONFIG_AMD_WBRF) += wbrf.o
> diff --git a/drivers/platform/x86/amd/wbrf.c b/drivers/platform/x86/amd/wbrf.c
> new file mode 100644
> index ..fb414564f576
> --- /dev/null
> +++ b/drivers/platform/x86/amd/wbrf.c
> @@ -0,0 +1,402 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Wifi Frequency Band Manage Interface
> + * Copyright (C) 2023 Advanced Micro Devices
> + */
> +
> +#include
> +#include
> +
> +#define ACPI_AMD_WBRF_METHOD "\\WBRF"
> +
> +/*
> + * Functions bit vector for WBRF method
> + *
> + * Bit 0: WBRF supported.
> + * Bit 1: Function 1 (Add / Remove frequency) is supported.
> + * Bit 2: Function 2 (Get frequency list) is supported.
> + */
> +#define WBRF_ENABLED 0x0
> +#define WBRF_RECORD 0x1
> +#define WBRF_RETRIEVE0x2
> +
> +#define WBRF_REVISION0x1
> +
> +/*
> + * The data structure used for WBRF_RETRIEVE is not naturally aligned.
> + * And unfortunately the design has been settled down.
> + */
> +struct amd_wbrf_ranges_out {
> + u32 num_of_ranges;
> + struct freq_band_range band_list[MAX_NUM_OF_WBRF_RANGES];
> +} __packed;
> +
> +static const guid_t wifi_acpi_dsm_guid =
> + GUID_INIT(0x7b7656cf, 0xdc3d, 0x4c1c,
> + 0x83, 0xe9, 0x66, 0xe7, 0x21, 0xde, 0x30, 0x70);
> +
> +/*
> + * Used to notify consumer (amdgpu driver currently) about
> + * the wifi frequency is change.
> + */
> +static BLOCKING_NOTIFIER_HEAD(wbrf_chain_head);
> +
> +static int wbrf_record(struct acpi_device *adev, uint8_t action,
> +struct wbrf_ranges_in_out *in)
> +{
> + union acpi_object argv4;
> + union acpi_object *tmp;
> + union acpi_object *obj;
> + u32 num_of_ranges = 0;
> + u32 num_of_elements;
> + u32 arg_idx = 0;
> + u32 loop_idx;
> + int ret;
> +
> + if (!in)
> + return -EINVAL;
> +
> + /*
> + * The num_of_ranges value in the "in" object supplied by
> + * the caller is required to be equal to the number of
> + * entries in the band_list array in there.
> + */
> + for (loop_idx = 0; loop_idx < ARRAY_SIZE(in->band_list);
> + loop_idx++)
This fits easily to one line.
What extra information loop_idx provides over the usual i? I see zero
extra value, only extra characters.
> + if (in->band_list[loop_idx].start &&
> + in->band_list[loop_idx].end)
One line.
> +
[PATCH v12 2/9] platform/x86/amd: Add support for AMD ACPI based Wifi band RFI mitigation feature
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 a mechanism that devices can use to
notify active use of particular frequencies so that other devices can make
relative internal adjustments as necessary to avoid this resonance.
Co-Developed-by: Evan Quan
Signed-off-by: Evan Quan
Signed-off-by: Ma Jun
--
v11:
- fix typo(Simon)
v12:
- Fix the code (Rafael)
- Move amd_wbrf.c to drivers/platform/x86/amd/wbrf.c
- Updated Evan's email because he's no longer at AMD.Thanks
for his work in earlier versions.
---
drivers/platform/x86/amd/Kconfig | 15 ++
drivers/platform/x86/amd/Makefile | 1 +
drivers/platform/x86/amd/wbrf.c | 402 ++
include/linux/acpi_amd_wbrf.h | 101
4 files changed, 519 insertions(+)
create mode 100644 drivers/platform/x86/amd/wbrf.c
create mode 100644 include/linux/acpi_amd_wbrf.h
diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
index d9685aef0887..fa5a978a2d22 100644
--- a/drivers/platform/x86/amd/Kconfig
+++ b/drivers/platform/x86/amd/Kconfig
@@ -32,3 +32,18 @@ config AMD_HSMP
If you choose to compile this driver as a module the module will be
called amd_hsmp.
+
+config AMD_WBRF
+ bool "AMD Wifi RF Band mitigations (WBRF)"
+ depends on ACPI
+ default n
+ help
+ WBRF(Wifi Band RFI mitigation) mechanism allows Wifi drivers
+ to notify the frequencies they are using so that other hardware
+ can be reconfigured to avoid harmonic conflicts.
+
+ AMD provides an ACPI based mechanism to support WBRF on platform with
+ appropriate underlying support.
+
+ This mechanism will only be activated on platforms that advertise a
+ need for it.
diff --git a/drivers/platform/x86/amd/Makefile
b/drivers/platform/x86/amd/Makefile
index 65732f0a3913..62b98b048b17 100644
--- a/drivers/platform/x86/amd/Makefile
+++ b/drivers/platform/x86/amd/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_AMD_PMC) += amd-pmc.o
amd_hsmp-y := hsmp.o
obj-$(CONFIG_AMD_HSMP) += amd_hsmp.o
obj-$(CONFIG_AMD_PMF) += pmf/
+obj-$(CONFIG_AMD_WBRF) += wbrf.o
diff --git a/drivers/platform/x86/amd/wbrf.c b/drivers/platform/x86/amd/wbrf.c
new file mode 100644
index ..fb414564f576
--- /dev/null
+++ b/drivers/platform/x86/amd/wbrf.c
@@ -0,0 +1,402 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Wifi Frequency Band Manage Interface
+ * Copyright (C) 2023 Advanced Micro Devices
+ */
+
+#include
+#include
+
+#define ACPI_AMD_WBRF_METHOD "\\WBRF"
+
+/*
+ * Functions bit vector for WBRF method
+ *
+ * Bit 0: WBRF supported.
+ * Bit 1: Function 1 (Add / Remove frequency) is supported.
+ * Bit 2: Function 2 (Get frequency list) is supported.
+ */
+#define WBRF_ENABLED 0x0
+#define WBRF_RECORD0x1
+#define WBRF_RETRIEVE 0x2
+
+#define WBRF_REVISION 0x1
+
+/*
+ * The data structure used for WBRF_RETRIEVE is not naturally aligned.
+ * And unfortunately the design has been settled down.
+ */
+struct amd_wbrf_ranges_out {
+ u32 num_of_ranges;
+ struct freq_band_range band_list[MAX_NUM_OF_WBRF_RANGES];
+} __packed;
+
+static const guid_t wifi_acpi_dsm_guid =
+ GUID_INIT(0x7b7656cf, 0xdc3d, 0x4c1c,
+ 0x83, 0xe9, 0x66, 0xe7, 0x21, 0xde, 0x30, 0x70);
+
+/*
+ * Used to notify consumer (amdgpu driver currently) about
+ * the wifi frequency is change.
+ */
+static BLOCKING_NOTIFIER_HEAD(wbrf_chain_head);
+
+static int wbrf_record(struct acpi_device *adev, uint8_t action,
+ struct wbrf_ranges_in_out *in)
+{
+ union acpi_object argv4;
+ union acpi_object *tmp;
+ union acpi_object *obj;
+ u32 num_of_ranges = 0;
+ u32 num_of_elements;
+ u32 arg_idx = 0;
+ u32 loop_idx;
+ int ret;
+
+ if (!in)
+ return -EINVAL;
+
+ /*
+* The num_of_ranges value in the "in" object supplied by
+* the caller is required to be equal to the number of
+* entries in the band_list array in there.
+*/
+ 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++;
+
+ if (num_of_ranges != in->num_of_ranges)
+ return -EINVAL;
+
+ /*
+* Every input frequency band comes with two end points(start/end)
+* and each is accounted as an element. Meanwhile the range count
+* and action type are accounted as an element each.
+* So, the total element count = 2 * num_of_ranges + 1 + 1.
+
