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

[Quan, Evan]

[AMD Official Use Only - General]

Hi Rafael & Andrew,

I just posted a new V5 series based on the discussions here and offline discussions with Mario.
Please share your comments/insights there.

Thanks,
Evan
> -----Original Message-----
> From: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> Sent: Saturday, June 24, 2023 1:16 AM
> To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
> Cc: Rafael J. Wysocki <rafael@xxxxxxxxxx>; Quan, Evan
> <Evan.Quan@xxxxxxx>; lenb@xxxxxxxxxx; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; Koenig, Christian
> <Christian.Koenig@xxxxxxx>; Pan, Xinhui <Xinhui.Pan@xxxxxxx>;
> airlied@xxxxxxxxx; daniel@xxxxxxxx; johannes@xxxxxxxxxxxxxxxx;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; mdaenzer@xxxxxxxxxx;
> maarten.lankhorst@xxxxxxxxxxxxxxx; tzimmermann@xxxxxxx;
> hdegoede@xxxxxxxxxx; jingyuwang_vip@xxxxxxx; Lazar, Lijo
> <Lijo.Lazar@xxxxxxx>; jim.cromie@xxxxxxxxx; bellosilicio@xxxxxxxxx;
> andrealmeid@xxxxxxxxxx; trix@xxxxxxxxxx; jsg@xxxxxxxxx; arnd@xxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; amd-
> gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-
> wireless@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF
> mitigations
>
> 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.