RE: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
[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 > Sent: Saturday, June 24, 2023 1:16 AM > To: Limonciello, Mario > Cc: Rafael J. Wysocki ; Quan, Evan > ; l...@kernel.org; Deucher, Alexander > ; Koenig, Christian > ; Pan, Xinhui ; > airl...@gmail.com; dan...@ffwll.ch; johan...@sipsolutions.net; > da...@davemloft.net; eduma...@google.com; k...@kernel.org; > pab...@redhat.com; mdaen...@redhat.com; > maarten.lankho...@linux.intel.com; tzimmerm...@suse.de; > hdego...@redhat.com; jingyuwang_...@163.com; Lazar, Lijo > ; jim.cro...@gmail.com; bellosili...@gmail.com; > andrealm...@igalia.com; t...@redhat.com; j...@jsg.id.au; a...@arndb.de; > linux-ker...@vger.kernel.org; linux-a...@vger.kernel.org; amd- > g...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- > wirel...@vger.kernel.org; net...@vger.kernel.org > 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 > wrote: > > > > > > On 6/23/2023 11:28 AM, Rafael J. Wysocki wrote: > > > On Fri, Jun 23, 2023 at 5:57 PM Limonciello, Mario > > > wrote: > > >> > > >> On 6/23/2023 9:52 AM, Rafael J. Wysocki wrote: > > >>> On Wed, Jun 21, 2023 at 7:47 AM Evan Quan > wrote: > > >>>> From: Mario Limonciello > > >>>> > > >>>> 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 >
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 wrote: > > > On 6/23/2023 11:28 AM, Rafael J. Wysocki wrote: > > On Fri, Jun 23, 2023 at 5:57 PM Limonciello, Mario > > wrote: > >> > >> On 6/23/2023 9:52 AM, Rafael J. Wysocki wrote: > >>> On Wed, Jun 21, 2023 at 7:47 AM Evan Quan wrote: > From: Mario Limonciello > > 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.
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On 6/23/2023 11:28 AM, Rafael J. Wysocki wrote: On Fri, Jun 23, 2023 at 5:57 PM Limonciello, Mario wrote: On 6/23/2023 9:52 AM, Rafael J. Wysocki wrote: On Wed, Jun 21, 2023 at 7:47 AM Evan Quan wrote: From: Mario Limonciello 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). The reason for having acpi_wbrf.c in the first place is to avoid the boilerplate of the _DSM implementation across multiple drivers. 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.
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On Wed, Jun 21, 2023 at 7:47 AM Evan Quan wrote:
>
> From: Mario Limonciello
>
> 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
>for the device.
> 2) If adding frequencies, then call `wbrf_add_exclusion` with the
>start and end ranges of the frequencies.
> 3) If removing frequencies, then call `wbrf_remove_exclusion` with
>start and end ranges of the frequencies.
>
> Drivers/subsystems responding to frequencies:
>
> 1) During probe, check `wbrf_supported_consumer` to see if WBRF is supported
>for the device.
> 2) Call the `wbrf_retrieve_exclusions` to retrieve the current
>exclusions on receiving an ACPI notification for a new frequency
>change.
>
> Signed-off-by: Mario Limonciello
> Co-developed-by: Evan Quan
> Signed-off-by: Evan Quan
> --
> v1->v2:
> - move those wlan specific implementations to net/mac80211(Mario)
> ---
> drivers/acpi/Kconfig | 7 ++
> drivers/acpi/Makefile| 2 +
> drivers/acpi/acpi_wbrf.c | 215 +++
> include/linux/wbrf.h | 55 ++
> 4 files changed, 279 insertions(+)
> create mode 100644 drivers/acpi/acpi_wbrf.c
> create mode 100644 include/linux/wbrf.h
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index ccbeab9500ec..0276c1487fa2 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -611,3 +611,10 @@ config X86_PM_TIMER
>
> You should nearly always say Y here because many modern
> systems require this timer.
> +
> +config ACPI_WBRF
> + bool "ACPI Wifi band RF mitigation mechanism"
> + help
> + Wifi band RF mitigation mechanism allows multiple drivers from
> + different domains to notify the frequencies in use so that hardware
> + can be reconfigured to avoid harmonic conflicts.
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index feb36c0b9446..14863b0c619f 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -131,3 +131,5 @@ obj-y += dptf/
> obj-$(CONFIG_ARM64)+= arm64/
>
> obj-$(CONFIG_ACPI_VIOT)+= viot.o
> +
> +obj-$(CONFIG_ACPI_WBRF)+= acpi_wbrf.o
> diff --git a/drivers/acpi/acpi_wbrf.c b/drivers/acpi/acpi_wbrf.c
> new file mode 100644
> index ..8c275998ac29
> --- /dev/null
> +++ b/drivers/acpi/acpi_wbrf.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Wifi Band Exclusion Interface
Where is the AML interface for this defined and how does it work?
> + * Copyright (C) 2023 Advanced Micro Devices
> + *
> + */
> +
> +#include
> +
> +/* functions */
> +#define WBRF_RECORD0x1
> +#define WBRF_RETRIEVE 0x2
> +
> +/* record actions */
> +#define WBRF_RECORD_ADD0x0
> +#define WBRF_RECORD_REMOVE 0x1
> +
> +#define WBRF_REVISION 0x1
> +
> +static const guid_t wifi_acpi_dsm_guid =
> + GUID_INIT(0x7b7656cf, 0xdc3d, 0x4c1c,
> + 0x83, 0xe9, 0x66, 0xe7, 0x21, 0xde, 0x30, 0x70);
> +
> +static int wbrf_dsm(struct acpi_device *adev, u8 fn,
> + union acpi_object *argv4,
> + union acpi_object **out)
> +{
> + union acpi_object *obj;
> + int rc;
> +
> + obj = acpi_evaluate_dsm(adev->handle, &wifi_acpi_dsm_guid,
> + WBRF_REVISION, fn, argv4);
> + if (!obj)
> + return -ENXIO;
> +
> + switch (obj->type) {
> + case ACPI_TYPE_BUFFER:
> + if (!*out) {
> + rc = -EINVAL;
> + break;
I'm not sure why you want to return an error in this case. Did you
really mean !out?
> + }
> + *out = obj;
> + return 0;
> +
> + case ACPI_TYPE_INTEGER:
> + rc = obj->integer.value ? -EINVAL : 0;
> + break;
An empty line here, please, as you added one after the return statement above.
> + default:
> + rc = -EOPNOTSUPP;
> + }
> + ACPI_FREE(obj);
> +
> + return rc;
How does the caller know whether or not they need to free the out
object after calling this function?
> +}
> +
> +static int wbrf_record(struct acpi_device *adev, uint8_t action,
> + struct wbrf_ranges_in *in)
> +{
> + uni
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On Fri, Jun 23, 2023 at 5:57 PM Limonciello, Mario wrote: > > > On 6/23/2023 9:52 AM, Rafael J. Wysocki wrote: > > On Wed, Jun 21, 2023 at 7:47 AM Evan Quan wrote: > >> From: Mario Limonciello > >> > >> 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. 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. Also if the given interface is specified somewhere, it would be good to have a pointer to that place. > > > > 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. Also whoever uses it, would first need to check if the device in question has an ACPI companion.
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On 6/23/2023 9:52 AM, Rafael J. Wysocki wrote: On Wed, Jun 21, 2023 at 7:47 AM Evan Quan wrote: From: Mario Limonciello 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. Whether or not there needs to be an intermediate library wrapped around this is a different matter.
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On Wed, Jun 21, 2023 at 7:47 AM Evan Quan wrote: > > From: Mario Limonciello > > 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. Whether or not there needs to be an intermediate library wrapped around this is a different matter.
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On Wed, Jun 21, 2023 at 01:50:34PM -0500, Limonciello, Mario wrote:
> So if we go down this path of CONFIG_WBRF and CONFIG_WBRF_ACPI, another
> question would be where should the new "wbrf.c" be stored? The ACPI only
> version most certainly made sense in drivers/acpi/wbrf.c, but a generic
> version that only has an ACPI implementation right now not so much.
>
> On 6/21/2023 1:30 PM, Andrew Lunn wrote:
> > > And consumer would need to call it, but only if CONFIG_WBRF_ACPI isn't
> > > set.
> > Why? How is ACPI special that it does not need notifiers?
> ACPI core does has notifiers that are used, but they don't work the same.
> If you look at patch 4, you'll see amdgpu registers and unregisters using
> both
>
> acpi_install_notify_handler()
> and
> acpi_remove_notify_handler()
>
> If we supported both ACPI notifications and non-ACPI notifications
> all consumers would have to have support to register and use both types.
I took a quick look at this:
#define CPM_GPU_NOTIFY_COMMAND 0x55
+static void amdgpu_acpi_wbrf_event(acpi_handle handle, u32 event, void *data)
+{
+ struct amdgpu_device *adev = (struct amdgpu_device *)data;
+
+ if (event == CPM_GPU_NOTIFY_COMMAND &&
+ adev->wbrf_event_handler)
+ adev->wbrf_event_handler(adev);
+}
handle is ignored, All you need is the void * data to link back to
your private data.
I find it interesting that CPM_GPU_NOTIFY_COMMAND is defined here. So
nothing else can use it. Should it maybe be called
CPM_AMDGPU_NOTIFY_COMMAND?
Overall, looking at this, i don't see anything which could not be made
abstract:
static void amdgpu_wbrf_event(u32 event, void *data)
{
struct amdgpu_device *adev = (struct amdgpu_device *)data;
if (event == WBRF_GPU_NOTIFY_COMMAND &&
adev->wbrf_event_handler)
adev->wbrf_event_handler(adev);
}
int amdgpu_register_wbrf_notify_handler(struct amdgpu_device *adev,
wbrf_notify_handler handler)
{
struct device *dev = adev->dev);
adev->wbrf_event_handler = handler;
return wbrf_install_notify_handler(dev, amdgpu_wbrf_event, adev);
}
Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On 6/21/2023 8:55 PM, Andrew Lunn wrote: Honestly I'm not sure though we need this complexity right now? I mean, it'd be really easy to replace the calls in mac80211 with some other more generalised calls in the future? You need some really deep platform/hardware level knowledge and involvement to do this, so I don't think it's something that someone will come up with very easily for a DT-based platform... What is this API about? It is a struct device says, i'm badly designed and make a mess of the following frequency bands. Optionally, if you ask me nicely, i might be able to tweak what i'm doing to avoid interfering with you. And it is about a struct device say, i'm using this particular frequency. If you can reduce the noise you make, i would be thankful. Hey now - you're making assumptions about what's badly designed. At it's core the issue here that prompts all of this is a "platform" issue with the tiny Z heights laptops these days strive for causing implied limitations for shielding. Independently both components work just fine. The one generating the noise could be anything. The PWM driving my laptop display back light?, What is being interfered with? The 3.5mm audio jack? How much deep system knowledge is needed to call pwm_set_state() to move the base frequency up above 20Khz so only my dog will hear it? But at the cost of a loss of efficiency and my battery going flatter faster? Is the DDR memory really the only badly designed component, when you think of the range of systems Linux is used on from PHC to tiny embedded systems? Ideally we want any sort of receiver with a low noise amplifier to just unconditionally use this API to let rest of the system know about it. And ideally we want anything which is a source of noise to declare itself. What happens after that should be up to the struct device causing the interference. I do get your point here - but the problem with a PWM on your laptop display interfering with the 3.5mm audio jack would likely be localized to your specific model. If you have the 16" version of the laptop and I have the 13" version I might have the 3.5mm audio jack in another location, that is better shielded and so making that assumption that we both have the same components so need to make changes could be totally wrong. If you have EVERYTHING with an amplifier advertising frequencies in use without any extra information about the location of the component or the impacts that component can have you're going to have a useless interface that is just a bunch of garbage data. I really think the application of this type of software mitigation should be reserved for system designers that made those design decisions and know they are going to run into problems. Mario did say: 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. Andrew The types of things that we envisioned were high frequency devices with larger power emissions. For example WWAN or USB4 devices. These fit well into the ACPI device model. When Evan gets back from holiday I'll discuss with him the ideas from this thread. However before then I would really appreciate if Rafael can provide some comments on patch 1 as it stands today.
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
> Honestly I'm not sure though we need this complexity right now? I mean, > it'd be really easy to replace the calls in mac80211 with some other > more generalised calls in the future? > > You need some really deep platform/hardware level knowledge and > involvement to do this, so I don't think it's something that someone > will come up with very easily for a DT-based platform... What is this API about? It is a struct device says, i'm badly designed and make a mess of the following frequency bands. Optionally, if you ask me nicely, i might be able to tweak what i'm doing to avoid interfering with you. And it is about a struct device say, i'm using this particular frequency. If you can reduce the noise you make, i would be thankful. The one generating the noise could be anything. The PWM driving my laptop display back light?, What is being interfered with? The 3.5mm audio jack? How much deep system knowledge is needed to call pwm_set_state() to move the base frequency up above 20Khz so only my dog will hear it? But at the cost of a loss of efficiency and my battery going flatter faster? Is the DDR memory really the only badly designed component, when you think of the range of systems Linux is used on from PHC to tiny embedded systems? Ideally we want any sort of receiver with a low noise amplifier to just unconditionally use this API to let rest of the system know about it. And ideally we want anything which is a source of noise to declare itself. What happens after that should be up to the struct device causing the interference. Mario did say: 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. Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On Wed, 2023-06-21 at 21:25 +0200, Andrew Lunn wrote: > > ACPI core does has notifiers that are used, but they don't work the same. > > If you look at patch 4, you'll see amdgpu registers and unregisters using > > both > > > > acpi_install_notify_handler() > > and > > acpi_remove_notify_handler() > > > > If we supported both ACPI notifications and non-ACPI notifications > > all consumers would have to have support to register and use both types. > > Why would you want to support ACPI notifications and non-ACPI > notifications? All you need is wbrf notification. > > The new wbrf.c should implement wbrf_install_notify_handler() and > wbrf_remove_notify_handler(). > > As to where to put wbrf.c? I guess either drivers/base/ or > drivers/wbrf/. Maybe ask GregKH? Not sure it should even be called WBRF at that point, but hey :) Honestly I'm not sure though we need this complexity right now? I mean, it'd be really easy to replace the calls in mac80211 with some other more generalised calls in the future? You need some really deep platform/hardware level knowledge and involvement to do this, so I don't think it's something that someone will come up with very easily for a DT-based platform... If we do something with a notifier chain in the future, we can just install one in the ACPI code too, and react indirectly rather than calling from wifi to the ACPI directly. johannes
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
> ACPI core does has notifiers that are used, but they don't work the same. > If you look at patch 4, you'll see amdgpu registers and unregisters using > both > > acpi_install_notify_handler() > and > acpi_remove_notify_handler() > > If we supported both ACPI notifications and non-ACPI notifications > all consumers would have to have support to register and use both types. Why would you want to support ACPI notifications and non-ACPI notifications? All you need is wbrf notification. The new wbrf.c should implement wbrf_install_notify_handler() and wbrf_remove_notify_handler(). As to where to put wbrf.c? I guess either drivers/base/ or drivers/wbrf/. Maybe ask GregKH? Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
So if we go down this path of CONFIG_WBRF and CONFIG_WBRF_ACPI, another question would be where should the new "wbrf.c" be stored? The ACPI only version most certainly made sense in drivers/acpi/wbrf.c, but a generic version that only has an ACPI implementation right now not so much. On 6/21/2023 1:30 PM, Andrew Lunn wrote: And consumer would need to call it, but only if CONFIG_WBRF_ACPI isn't set. Why? How is ACPI special that it does not need notifiers? ACPI core does has notifiers that are used, but they don't work the same. If you look at patch 4, you'll see amdgpu registers and unregisters using both acpi_install_notify_handler() and acpi_remove_notify_handler() If we supported both ACPI notifications and non-ACPI notifications all consumers would have to have support to register and use both types. I don't see why it couldn't be a DT/ACPI hybrid solution for ARM64. As said somewhere else, nobody does hybrid. In fact, turn it around. Why not implement all this in DT, and make X86 hybrid? That will make arm, powerpc, risc-v and mips much simpler :-) Andrew Doesn't coreboot do something hybrid with device tree? I thought they generate their ACPI tables from a combination of DT and some static ASL.
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
> And consumer would need to call it, but only if CONFIG_WBRF_ACPI isn't set. Why? How is ACPI special that it does not need notifiers? > I don't see why it couldn't be a DT/ACPI hybrid solution for ARM64. As said somewhere else, nobody does hybrid. In fact, turn it around. Why not implement all this in DT, and make X86 hybrid? That will make arm, powerpc, risc-v and mips much simpler :-) Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On 6/21/2023 12:26 PM, Andrew Lunn wrote:
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);
Yes, this looks a lot better.
But what about notifications?
Once you implement this it gets a lot more complex and the driver
consumers would need
to know more about the kernel's implementation. For example consumers
need a
notifier block like:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index e3e2e6e3b485..146fe3c43343 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1066,6 +1066,8 @@ struct amdgpu_device {
bool job_hang;
bool dc_enabled;
+
+ struct notifier_block wbrf_notifier;
};
static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev)
And then would need matching notifier functions like:
static int amdgpu_wbrf_frequencies_notifier(struct notifier_block *nb,
unsigned long action, void *_arg)
And we'd need to set up a chain to be used in this case in the WBRF code:
static BLOCKING_NOTIFIER_HEAD(wbrf_chain_head);
int wbrf_register_notifier(struct notifier_block *nb)
{
return blocking_notifier_chain_register(&wbrf_chain_head, nb);
}
EXPORT_SYMBOL_GPL(wbrf_register_notifier);
int wbrf_unregister_notifier(struct notifier_block *nb)
{
return blocking_notifier_chain_unregister(&wbrf_chain_head, nb);
}
EXPORT_SYMBOL_GPL(wbrf_unregister_notifier);
And consumer would need to call it, but only if CONFIG_WBRF_ACPI isn't set.
Add/remove functions can easily call something like:
blocking_notifier_call_chain(&wbrf_chain_head, action, data);
With all of this complexity and (effectively) dead code for ACPI vs non-ACPI
path I really have to ask why wouldn't a non-AMD implementation be able to
do this as ACPI?
I don't see why it couldn't be a DT/ACPI hybrid solution for ARM64.
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
> 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);
Yes, this looks a lot better.
But what about notifications?
Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
> Think a little more about what a non-ACPI implementation > would look like: > > 1) Would producers and consumers still need you to set CONFIG_ACPI_WBRF? I would expect there to be an CONFIG_WBRF, and then under that a CONFIG_WBRF_ACPI, CONFIG_WBRF_DT, CONFIG_WBRF_FOOBAR, each indicating they are mutual exclusive to the other. > 2) How would you indicate you need WBRF support? As far as i understand it, you have something in ACPI which indicates it? Could that not also be a DT property? > 3) How would notifications from one device to another work? Linux notified chains? This is something which happens a lot in networking. A physical interface goes down and needs to tell team/bonding interface stack on top of it that its status has changed. It just calls a notification chain. > I don't think those are trivial problems that can be solved by > just making the pointer 'struct device' particularly as with the > ACPI implementation consumers are expecting the notification from > ACPI. Do consumers really care who the notification is from? Isn't the event and its content what matters, not who sent it? But I agree, i don't expect it is trivial. But it is going to get harder and harder to make abstract as more and more users are added. So we need to consider, do you want to do that work now, or later? Do we want to merge something we know is limited and not using the generic kernel abstractions? Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
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
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?
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
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 > > > > > > > > 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
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On 6/21/2023 11:31 AM, Andrew Lunn wrote: I think there is enough details for this to happen. It's done so that either the AML can natively behave as a consumer or a driver can behave as a consumer. +/** + * APIs needed by drivers/subsystems for contributing frequencies: + * During probe, check `wbrf_supported_producer` to see if WBRF is supported. + * If adding frequencies, then call `wbrf_add_exclusion` with the + * start and end points specified for the frequency ranges added. + * If removing frequencies, then call `wbrf_remove_exclusion` with + * start and end points specified for the frequency ranges added. + */ +bool wbrf_supported_producer(struct acpi_device *adev); +int wbrf_add_exclusion(struct acpi_device *adev, + struct wbrf_ranges_in *in); +int wbrf_remove_exclusion(struct acpi_device *adev, + struct wbrf_ranges_in *in); Could struct device be used here, to make the API agnostic to where the information is coming from? That would then allow somebody in the future to implement a device tree based information provider. That does make sense, and it wouldn't even be that much harder if we assume in a given platform there's only one provider That seems like a very reasonable assumption. It is theoretically possible to build an ACPI + DT hybrid, but i've never seen it actually done. If an ARM64 ACPI BIOS could implement this, then i would guess the low level bits would be solved, i guess jumping into the EL1 firmware. Putting DT on top instead should not be too hard. Andrew To make life easier I'll ask whether we can include snippets of the matching ASL for this first implementation as part of the public ACPI spec that matches this code when we release it. So it sounds like you are pretty open about this, there should be enough information for independent implementations. So please do make the APIs between the providers and the consumers abstract, struct device, not an ACPI object. Andrew Think a little more about what a non-ACPI implementation would look like: 1) Would producers and consumers still need you to set CONFIG_ACPI_WBRF? 2) How would you indicate you need WBRF support? 3) How would notifications from one device to another work? I don't think those are trivial problems that can be solved by just making the pointer 'struct device' particularly as with the ACPI implementation consumers are expecting the notification from ACPI.
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On Wed, 2023-06-21 at 18:14 +0200, Andrew Lunn wrote: > > > 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 > > If you don't know what it is actually doing, it suggests the API is > not very well defined. I wouldn't say that. At the level it's defined now, the API is very clear: the wifi subsystem tells the other side what channels it's operating on right now. > Is there even enough details that ARM64 ACPI > BIOS could implement this? This, in itself? No. You'd have to know about the physical characteristics of the system, what is actually causing interference and at what frequencies and of course what you can actually do to mitigate (such as adjusting clock frequencies.) But as an API? I'd think yes, since WiFi can't really move off a frequency, other than disconnect, anyway. > > > > +bool wbrf_supported_producer(struct acpi_device *adev); > > > > +int wbrf_add_exclusion(struct acpi_device *adev, > > > > + struct wbrf_ranges_in *in); > > > > +int wbrf_remove_exclusion(struct acpi_device *adev, > > > > + struct wbrf_ranges_in *in); > > > > > > Could struct device be used here, to make the API agnostic to where > > > the information is coming from? That would then allow somebody in the > > > future to implement a device tree based information provider. > > > > That does make sense, and it wouldn't even be that much harder if we > > assume in a given platform there's only one provider > > That seems like a very reasonable assumption. It is theoretically > possible to build an ACPI + DT hybrid, but i've never seen it actually > done. OK. > If an ARM64 ACPI BIOS could implement this, then i would guess the low > level bits would be solved, i guess jumping into the EL1 > firmware. Putting DT on top instead should not be too hard. Right. Maybe then this really shouldn't be called "wbrf", but maybe naming doesn't matter that much :) johannes
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
> I think there is enough details for this to happen. It's done > so that either the AML can natively behave as a consumer or a > driver can behave as a consumer. > > > > > +/** > > > > > + * APIs needed by drivers/subsystems for contributing frequencies: > > > > > + * During probe, check `wbrf_supported_producer` to see if WBRF is > > > > > supported. > > > > > + * If adding frequencies, then call `wbrf_add_exclusion` with the > > > > > + * start and end points specified for the frequency ranges added. > > > > > + * If removing frequencies, then call `wbrf_remove_exclusion` with > > > > > + * start and end points specified for the frequency ranges added. > > > > > + */ > > > > > +bool wbrf_supported_producer(struct acpi_device *adev); > > > > > +int wbrf_add_exclusion(struct acpi_device *adev, > > > > > +struct wbrf_ranges_in *in); > > > > > +int wbrf_remove_exclusion(struct acpi_device *adev, > > > > > + struct wbrf_ranges_in *in); > > > > Could struct device be used here, to make the API agnostic to where > > > > the information is coming from? That would then allow somebody in the > > > > future to implement a device tree based information provider. > > > That does make sense, and it wouldn't even be that much harder if we > > > assume in a given platform there's only one provider > > That seems like a very reasonable assumption. It is theoretically > > possible to build an ACPI + DT hybrid, but i've never seen it actually > > done. > > > > If an ARM64 ACPI BIOS could implement this, then i would guess the low > > level bits would be solved, i guess jumping into the EL1 > > firmware. Putting DT on top instead should not be too hard. > > > > Andrew > > To make life easier I'll ask whether we can include snippets of > the matching ASL for this first implementation as part of the > public ACPI spec that matches this code when we release it. So it sounds like you are pretty open about this, there should be enough information for independent implementations. So please do make the APIs between the providers and the consumers abstract, struct device, not an ACPI object. Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On 6/21/2023 11:14 AM, Andrew Lunn wrote: 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 If you don't know what it is actually doing, it suggests the API is not very well defined. Is there even enough details that ARM64 ACPI BIOS could implement this? I think there is enough details for this to happen. It's done so that either the AML can natively behave as a consumer or a driver can behave as a consumer. +/** + * APIs needed by drivers/subsystems for contributing frequencies: + * During probe, check `wbrf_supported_producer` to see if WBRF is supported. + * If adding frequencies, then call `wbrf_add_exclusion` with the + * start and end points specified for the frequency ranges added. + * If removing frequencies, then call `wbrf_remove_exclusion` with + * start and end points specified for the frequency ranges added. + */ +bool wbrf_supported_producer(struct acpi_device *adev); +int wbrf_add_exclusion(struct acpi_device *adev, + struct wbrf_ranges_in *in); +int wbrf_remove_exclusion(struct acpi_device *adev, + struct wbrf_ranges_in *in); Could struct device be used here, to make the API agnostic to where the information is coming from? That would then allow somebody in the future to implement a device tree based information provider. That does make sense, and it wouldn't even be that much harder if we assume in a given platform there's only one provider That seems like a very reasonable assumption. It is theoretically possible to build an ACPI + DT hybrid, but i've never seen it actually done. If an ARM64 ACPI BIOS could implement this, then i would guess the low level bits would be solved, i guess jumping into the EL1 firmware. Putting DT on top instead should not be too hard. Andrew To make life easier I'll ask whether we can include snippets of the matching ASL for this first implementation as part of the public ACPI spec that matches this code when we release it.
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
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 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. 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. +/** + * APIs needed by drivers/subsystems for contributing frequencies: + * During probe, check `wbrf_supported_producer` to see if WBRF is supported. + * If adding frequencies, then call `wbrf_add_exclusion` with the + * start and end points specified for the frequency ranges added. + * If removing frequencies, then call `wbrf_remove_exclusion` with + * start and end points specified for the frequency ranges added. + */ +bool wbrf_supported_producer(struct acpi_device *adev); +int wbrf_add_exclusion(struct acpi_device *adev, + struct wbrf_ranges_in *in); +int wbrf_remove_exclusion(struct acpi_device *adev, + struct wbrf_ranges_in *in); Could struct device be used here, to make the API agnostic to where the information is coming from? That would then allow somebody in the future to implement a device tree based information provider. That does make sense, and it wouldn't even be that much harder if we assume in a given platform there's only one provider - but once you go beyond that these would need to call function pointers I guess? Though that could be left for "future improvement" too. johannes There's more to it than just sending in the frequency that is added or removed. The notification path comes from ACPI as well. This first implementation only has one provider and consumer but yes, we envision that there could be multiple of each party and that AML may be the mechanism for some consumers to react.
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
> > 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 If you don't know what it is actually doing, it suggests the API is not very well defined. Is there even enough details that ARM64 ACPI BIOS could implement this? > > > +/** > > > + * APIs needed by drivers/subsystems for contributing frequencies: > > > + * During probe, check `wbrf_supported_producer` to see if WBRF is > > > supported. > > > + * If adding frequencies, then call `wbrf_add_exclusion` with the > > > + * start and end points specified for the frequency ranges added. > > > + * If removing frequencies, then call `wbrf_remove_exclusion` with > > > + * start and end points specified for the frequency ranges added. > > > + */ > > > +bool wbrf_supported_producer(struct acpi_device *adev); > > > +int wbrf_add_exclusion(struct acpi_device *adev, > > > +struct wbrf_ranges_in *in); > > > +int wbrf_remove_exclusion(struct acpi_device *adev, > > > + struct wbrf_ranges_in *in); > > > > Could struct device be used here, to make the API agnostic to where > > the information is coming from? That would then allow somebody in the > > future to implement a device tree based information provider. > > That does make sense, and it wouldn't even be that much harder if we > assume in a given platform there's only one provider That seems like a very reasonable assumption. It is theoretically possible to build an ACPI + DT hybrid, but i've never seen it actually done. If an ARM64 ACPI BIOS could implement this, then i would guess the low level bits would be solved, i guess jumping into the EL1 firmware. Putting DT on top instead should not be too hard. Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
On Wed, Jun 21, 2023 at 01:45:56PM +0800, Evan Quan wrote: > From: Mario Limonciello > > 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? > +/** > + * APIs needed by drivers/subsystems for contributing frequencies: > + * During probe, check `wbrf_supported_producer` to see if WBRF is supported. > + * If adding frequencies, then call `wbrf_add_exclusion` with the > + * start and end points specified for the frequency ranges added. > + * If removing frequencies, then call `wbrf_remove_exclusion` with > + * start and end points specified for the frequency ranges added. > + */ > +bool wbrf_supported_producer(struct acpi_device *adev); > +int wbrf_add_exclusion(struct acpi_device *adev, > +struct wbrf_ranges_in *in); > +int wbrf_remove_exclusion(struct acpi_device *adev, > + struct wbrf_ranges_in *in); Could struct device be used here, to make the API agnostic to where the information is coming from? That would then allow somebody in the future to implement a device tree based information provider. Andrew
Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations
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 > > > > 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. > > +/** > > + * APIs needed by drivers/subsystems for contributing frequencies: > > + * During probe, check `wbrf_supported_producer` to see if WBRF is > > supported. > > + * If adding frequencies, then call `wbrf_add_exclusion` with the > > + * start and end points specified for the frequency ranges added. > > + * If removing frequencies, then call `wbrf_remove_exclusion` with > > + * start and end points specified for the frequency ranges added. > > + */ > > +bool wbrf_supported_producer(struct acpi_device *adev); > > +int wbrf_add_exclusion(struct acpi_device *adev, > > + struct wbrf_ranges_in *in); > > +int wbrf_remove_exclusion(struct acpi_device *adev, > > + struct wbrf_ranges_in *in); > > Could struct device be used here, to make the API agnostic to where > the information is coming from? That would then allow somebody in the > future to implement a device tree based information provider. That does make sense, and it wouldn't even be that much harder if we assume in a given platform there's only one provider - but once you go beyond that these would need to call function pointers I guess? Though that could be left for "future improvement" too. johannes
