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 <quanliangl@xxxxxxxxxxx>
> Signed-off-by: Evan Quan <quanliangl@xxxxxxxxxxx>
> Signed-off-by: Ma Jun <Jun.Ma2@xxxxxxx>
<snip>
> +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(struct device *dev,
> + struct wbrf_ranges_in_out *out)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> + struct amd_wbrf_ranges_out acpi_out = {0};
> + union acpi_object *obj;
> + int ret = 0;
> +
> + if (!adev)
> + return -ENODEV;
> +
> + obj = acpi_evaluate_wbrf(adev->handle,
> + WBRF_REVISION,
> + WBRF_RETRIEVE);
> + if (!obj)
> + return -EINVAL;
And I would expect this to use acpi_evaluate_dsm(), or
preferably if possible acpi_evaluate_dsm_typed().
Is there any reason why the code is directly calling
a method called WBRF here instead of going through
the _DSM method ?
And if there is such a reason then please update
the documentation to say so, instead of having
the docs clam that consumers also use the _DSM method.
Regards,
Hans
