Re: [V9 2/9] drivers core: add ACPI based WBRF mechanism introduced by AMD
On Fri, Aug 18, 2023 at 5:27 AM Evan Quan wrote:
>
> AMD has introduced an ACPI based mechanism to support WBRF for some
> platforms with AMD dGPU + WLAN. This needs support from BIOS equipped
> with necessary AML implementations and dGPU firmwares.
This needs a problem statement in the first place: What exactly caused
AMD to come up with this design?
> For those systems without the ACPI mechanism and developing solutions,
> user can use/fall-back the generic WBRF solution for diagnosing potential
> interference issues.
>
> And for the platform which does not equip with the necessary AMD ACPI
> implementations but with CONFIG_WBRF_AMD_ACPI built as 'y', it will
> fall back to generic WBRF solution if the `wbrf` is set as "on".
OK, so I suppose that the patch implements support for the AMD WBRF
firmware interface? That needs to be stated somewhere.
>From patch reverse-engineering it looks like the generic WBRF code is
updated by it to hook up to the ACPI implementation if supported. If
my understanding is correct, it would be nice to state that in the
changelog too.
> Co-developed-by: Mario Limonciello
> Signed-off-by: Mario Limonciello
> Co-developed-by: Evan Quan
> Signed-off-by: Evan Quan
> --
> v4->v5:
> - promote this to be a more generic solution with input argument taking
> `struct device` and provide better scalability to support non-ACPI
> scenarios(Andrew)
> - update the APIs naming and some other minor fixes(Rafael)
> v5->v6:
> - make the code more readable and some other fixes(Andrew)
> v6->v8:
> - drop CONFIG_WBRF_GENERIC(Mario)
> - add `wbrf` kernel parameter for policy control(Mario)
> v8->v9:
> - correct some coding style(Simon)
> ---
> drivers/acpi/Makefile | 2 +
> drivers/acpi/amd_wbrf.c | 294 ++
> drivers/base/Kconfig | 20 +++
> drivers/base/wbrf.c | 135 +---
> include/linux/acpi_amd_wbrf.h | 25 +++
> 5 files changed, 452 insertions(+), 24 deletions(-)
> create mode 100644 drivers/acpi/amd_wbrf.c
> create mode 100644 include/linux/acpi_amd_wbrf.h
>
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 3fc5a0d54f6e..9185d16e4495 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -133,3 +133,5 @@ obj-$(CONFIG_ARM64) += arm64/
> obj-$(CONFIG_ACPI_VIOT)+= viot.o
>
> obj-$(CONFIG_RISCV)+= riscv/
> +
> +obj-$(CONFIG_WBRF_AMD_ACPI)+= amd_wbrf.o
> diff --git a/drivers/acpi/amd_wbrf.c b/drivers/acpi/amd_wbrf.c
> new file mode 100644
> index ..0e46de3dfac7
> --- /dev/null
> +++ b/drivers/acpi/amd_wbrf.c
> @@ -0,0 +1,294 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Wifi Band Exclusion Interface (AMD ACPI Implementation)
> + * Copyright (C) 2023 Advanced Micro Devices
> + *
It would be nice to have a description of the firmware interface here,
at least in general terms.
In particular, the terminology used throughout the code must be explained.
Without it, qualifying the validity and/or usefulness of the code is
rather hard.
> + */
> +
> +#include
> +#include
> +
> +#define ACPI_AMD_WBRF_METHOD "\\WBRF"
> +
> +/*
> + * Functions bit vector for WBRF method
> + *
> + * Bit 0: Supported for any functions other than function 0.
> + * 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
> +
> +/* record actions */
> +#define WBRF_RECORD_ADD0x0
> +#define WBRF_RECORD_REMOVE 0x1
> +
> +#define WBRF_REVISION 0x1
> +
> +/*
> + * The data structure used for WBRF_RETRIEVE is not natually aligned.
"naturally"
> + * And unfortunately the design has been settled down.
> + */
> +struct amd_wbrf_ranges_out {
> + u32 num_of_ranges;
> + struct exclusion_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);
> +
> +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, _acpi_dsm_guid,
> + WBRF_REVISION, fn, argv4);
> + if (!obj)
> + return -ENXIO;
> +
> + switch (obj->type) {
> + case ACPI_TYPE_BUFFER:
> + *out = obj;
> + return 0;
> +
> + case ACPI_TYPE_INTEGER:
> + rc = obj->integer.value ? -EINVAL : 0;
> + break;
> +
> + default:
> + rc = -EOPNOTSUPP;
> + }
> +
> + ACPI_FREE(obj);
> +
>
[V9 2/9] drivers core: add ACPI based WBRF mechanism introduced by AMD
AMD has introduced an ACPI based mechanism to support WBRF for some
platforms with AMD dGPU + WLAN. This needs support from BIOS equipped
with necessary AML implementations and dGPU firmwares.
For those systems without the ACPI mechanism and developing solutions,
user can use/fall-back the generic WBRF solution for diagnosing potential
interference issues.
And for the platform which does not equip with the necessary AMD ACPI
implementations but with CONFIG_WBRF_AMD_ACPI built as 'y', it will
fall back to generic WBRF solution if the `wbrf` is set as "on".
Co-developed-by: Mario Limonciello
Signed-off-by: Mario Limonciello
Co-developed-by: Evan Quan
Signed-off-by: Evan Quan
--
v4->v5:
- promote this to be a more generic solution with input argument taking
`struct device` and provide better scalability to support non-ACPI
scenarios(Andrew)
- update the APIs naming and some other minor fixes(Rafael)
v5->v6:
- make the code more readable and some other fixes(Andrew)
v6->v8:
- drop CONFIG_WBRF_GENERIC(Mario)
- add `wbrf` kernel parameter for policy control(Mario)
v8->v9:
- correct some coding style(Simon)
---
drivers/acpi/Makefile | 2 +
drivers/acpi/amd_wbrf.c | 294 ++
drivers/base/Kconfig | 20 +++
drivers/base/wbrf.c | 135 +---
include/linux/acpi_amd_wbrf.h | 25 +++
5 files changed, 452 insertions(+), 24 deletions(-)
create mode 100644 drivers/acpi/amd_wbrf.c
create mode 100644 include/linux/acpi_amd_wbrf.h
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 3fc5a0d54f6e..9185d16e4495 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -133,3 +133,5 @@ obj-$(CONFIG_ARM64) += arm64/
obj-$(CONFIG_ACPI_VIOT)+= viot.o
obj-$(CONFIG_RISCV)+= riscv/
+
+obj-$(CONFIG_WBRF_AMD_ACPI)+= amd_wbrf.o
diff --git a/drivers/acpi/amd_wbrf.c b/drivers/acpi/amd_wbrf.c
new file mode 100644
index ..0e46de3dfac7
--- /dev/null
+++ b/drivers/acpi/amd_wbrf.c
@@ -0,0 +1,294 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Wifi Band Exclusion Interface (AMD ACPI Implementation)
+ * Copyright (C) 2023 Advanced Micro Devices
+ *
+ */
+
+#include
+#include
+
+#define ACPI_AMD_WBRF_METHOD "\\WBRF"
+
+/*
+ * Functions bit vector for WBRF method
+ *
+ * Bit 0: Supported for any functions other than function 0.
+ * 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
+
+/* record actions */
+#define WBRF_RECORD_ADD0x0
+#define WBRF_RECORD_REMOVE 0x1
+
+#define WBRF_REVISION 0x1
+
+/*
+ * The data structure used for WBRF_RETRIEVE is not natually aligned.
+ * And unfortunately the design has been settled down.
+ */
+struct amd_wbrf_ranges_out {
+ u32 num_of_ranges;
+ struct exclusion_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);
+
+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, _acpi_dsm_guid,
+ WBRF_REVISION, fn, argv4);
+ if (!obj)
+ return -ENXIO;
+
+ switch (obj->type) {
+ case ACPI_TYPE_BUFFER:
+ *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;
+ union acpi_object *tmp;
+ u32 num_of_ranges = 0;
+ u32 num_of_elements;
+ u32 arg_idx = 0;
+ u32 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++;
+
+ /*
+* Every range comes with two end points(start and end) and
+* each of them 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.
+*/
+ num_of_elements = 2 * num_of_ranges + 1 + 1;
+
+ tmp = kcalloc(num_of_elements,
