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 <mario.limonciello@xxxxxxx>
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?
