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

[Rafael J. Wysocki]

On Fri, Jun 23, 2023 at 6:48 PM Limonciello, Mario
<mario.limonciello@xxxxxxx> wrote:
>
>
> On 6/23/2023 11:28 AM, Rafael J. Wysocki wrote:
> > On Fri, Jun 23, 2023 at 5:57 PM Limonciello, Mario
> > <mario.limonciello@xxxxxxx> wrote:
> >>
> >> On 6/23/2023 9:52 AM, Rafael J. Wysocki wrote:
> >>> On Wed, Jun 21, 2023 at 7:47 AM Evan Quan <evan.quan@xxxxxxx> 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.
> >>>>
> >>>> 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).

_DSM is an interface to the platform like any other AML, so I'm not
really sure what you mean.

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

Absolutely, drivers should not be bothered with having to use _DSM in
any case.  However, they may not even realize that they are running on
a system using ACPI and I'm not sure if they really should care.

> >
> > 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.

Either of the above has problems IMO.

The problem with the wbrf_ helper approach is that it adds
(potentially) several pieces of interaction with the platform,
potentially for every driver, in places where drivers don't do such
things as a rule.

The problem with the other approach is that the drivers in question
now need to be aware of ACPI in general and the AMD WBRF interface in
particular and if other similar interfaces are added by other vendors,
they will have to learn about those as well.

I think that we need to start over with a general problem statement
that in some cases the platform needs to be consulted regarding radio
frequencies that drivers would like to use, because it may need to
adjust or simply say which ranges are "noisy" (or even completely
unusable for that matter).  That should allow us to figure out how the
interface should look like from the driver side and it should be
possible to hook up the existing platform interface to that.