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