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

  From: Mario Limonciello <mario.limonciello@amd.com>

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 <mario.limonciello@amd.com>
Co-developed-by: Evan Quan <evan.quan@amd.com>
Signed-off-by: Evan Quan <evan.quan@amd.com>
--
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
  

On Wed, Jun 21, 2023 at 01:45:56PM +0800, Evan Quan wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> 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
  

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@amd.com>
> > 
> > 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
  

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

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@amd.com>
>>>
>>> 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.
  

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.
  

> 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
  

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
  

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.
  

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@amd.com>
> > > > 
> > > > 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
  

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@amd.com>
>>>>>
>>>>> 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?
  

> 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
  

> 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
  

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.
  

> 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
  

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.
  

> 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
  

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
  

> 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
  

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.
  

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
  

On Wed, Jun 21, 2023 at 7:47 AM Evan Quan <evan.quan@amd.com> wrote:
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> 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.
  

On 6/23/2023 9:52 AM, Rafael J. Wysocki wrote:
> On Wed, Jun 21, 2023 at 7:47 AM Evan Quan <evan.quan@amd.com> wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> 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.
  

On Fri, Jun 23, 2023 at 5:57 PM Limonciello, Mario
<mario.limonciello@amd.com> 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@amd.com> wrote:
> >> From: Mario Limonciello <mario.limonciello@amd.com>
> >>
> >> 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.
  

On Wed, Jun 21, 2023 at 7:47 AM Evan Quan <evan.quan@amd.com> wrote:
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> 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 <mario.limonciello@amd.com>
> Co-developed-by: Evan Quan <evan.quan@amd.com>
> Signed-off-by: Evan Quan <evan.quan@amd.com>
> --
> 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 000000000000..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 <linux/wbrf.h>
> +
> +/* functions */
> +#define WBRF_RECORD            0x1
> +#define WBRF_RETRIEVE          0x2
> +
> +/* record actions */
> +#define WBRF_RECORD_ADD                0x0
> +#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)
> +{
> +       union acpi_object *argv4;
> +       uint32_t num_of_ranges = 0;
> +       uint32_t arg_idx = 0;
> +       uint32_t loop_idx;
> +       int ret;
> +
> +       if (!in)
> +               return -EINVAL;
> +
> +       for (loop_idx = 0; loop_idx < ARRAY_SIZE(in->band_list);
> +            loop_idx++)
> +               if (in->band_list[loop_idx].start &&
> +                   in->band_list[loop_idx].end)
> +                       num_of_ranges++;
> +
> +       argv4 = kzalloc(sizeof(*argv4) * (2 * num_of_ranges + 2 + 1), GFP_KERNEL);
> +       if (!argv4)
> +               return -ENOMEM;
> +
> +       argv4[arg_idx].package.type = ACPI_TYPE_PACKAGE;
> +       argv4[arg_idx].package.count = 2 + 2 * num_of_ranges;
> +       argv4[arg_idx++].package.elements = &argv4[1];
> +       argv4[arg_idx].integer.type = ACPI_TYPE_INTEGER;
> +       argv4[arg_idx++].integer.value = num_of_ranges;
> +       argv4[arg_idx].integer.type = ACPI_TYPE_INTEGER;
> +       argv4[arg_idx++].integer.value = action;
> +
> +       for (loop_idx = 0; loop_idx < ARRAY_SIZE(in->band_list);
> +            loop_idx++) {
> +               if (!in->band_list[loop_idx].start ||
> +                   !in->band_list[loop_idx].end)
> +                       continue;
> +
> +               argv4[arg_idx].integer.type = ACPI_TYPE_INTEGER;
> +               argv4[arg_idx++].integer.value = in->band_list[loop_idx].start;
> +               argv4[arg_idx].integer.type = ACPI_TYPE_INTEGER;
> +               argv4[arg_idx++].integer.value = in->band_list[loop_idx].end;
> +       }
> +
> +       ret = wbrf_dsm(adev, WBRF_RECORD, argv4, NULL);
> +
> +       kfree(argv4);
> +
> +       return ret;
> +}
> +
> +int wbrf_add_exclusion(struct acpi_device *adev,
> +                      struct wbrf_ranges_in *in)
> +{
> +       return wbrf_record(adev, WBRF_RECORD_ADD, in);
> +}
> +EXPORT_SYMBOL_GPL(wbrf_add_exclusion);
> +
> +int wbrf_remove_exclusion(struct acpi_device *adev,
> +                         struct wbrf_ranges_in *in)
> +{
> +       return wbrf_record(adev, WBRF_RECORD_REMOVE, in);
> +}
> +EXPORT_SYMBOL_GPL(wbrf_remove_exclusion);
> +
> +bool wbrf_supported_producer(struct acpi_device *adev)
> +{
> +       return acpi_check_dsm(adev->handle, &wifi_acpi_dsm_guid,
> +                             WBRF_REVISION,
> +                             (1ULL << WBRF_RECORD) | (1ULL << WBRF_RETRIEVE));

I'm wondering if the BIT() macro would work here (and below).

> +}
> +EXPORT_SYMBOL_GPL(wbrf_supported_producer);
> +
> +static union acpi_object *
> +acpi_evaluate_wbrf(acpi_handle handle, u64 rev, u64 func)
> +{
> +       acpi_status ret;
> +       struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
> +       union acpi_object params[4];
> +       struct acpi_object_list input = {
> +               .count = 4,
> +               .pointer = params,
> +       };
> +
> +       params[0].type = ACPI_TYPE_INTEGER;
> +       params[0].integer.value = rev;
> +       params[1].type = ACPI_TYPE_INTEGER;
> +       params[1].integer.value = func;
> +       params[2].type = ACPI_TYPE_PACKAGE;
> +       params[2].package.count = 0;
> +       params[2].package.elements = NULL;
> +       params[3].type = ACPI_TYPE_STRING;
> +       params[3].string.length = 0;
> +       params[3].string.pointer= NULL;
> +
> +       ret = acpi_evaluate_object(handle, "WBRF", &input, &buf);
> +       if (ACPI_SUCCESS(ret))
> +               return (union acpi_object *)buf.pointer;
> +
> +       if (ret != AE_NOT_FOUND)
> +               acpi_handle_warn(handle,
> +                                "failed to evaluate WBRF(0x%x)\n", ret);

Why _warn()?

> +
> +       return NULL;
> +}
> +
> +static bool check_acpi_wbrf(acpi_handle handle, u64 rev, u64 funcs)
> +{
> +       int i;
> +       u64 mask = 0;
> +       union acpi_object *obj;
> +
> +       if (funcs == 0)
> +               return false;
> +
> +       obj = acpi_evaluate_wbrf(handle, rev, 0);
> +       if (!obj)
> +               return false;
> +
> +       if (obj->type != ACPI_TYPE_BUFFER)
> +               return false;
> +
> +       for (i = 0; i < obj->buffer.length && i < 8; i++)
> +               mask |= (((u64)obj->buffer.pointer[i]) << (i * 8));

What is going on here?  Any comment to explain it?

> +       ACPI_FREE(obj);
> +
> +       /*
> +        * Bit 0 indicates whether there's support for any functions other than
> +        * function 0.
> +        */
> +       if ((mask & 0x1) && (mask & funcs) == funcs)
> +               return true;
> +
> +       return false;
> +}
> +
> +bool wbrf_supported_consumer(struct acpi_device *adev)
> +{
> +       return check_acpi_wbrf(adev->handle,
> +                              WBRF_REVISION,
> +                              1ULL << WBRF_RETRIEVE);
> +}
> +EXPORT_SYMBOL_GPL(wbrf_supported_consumer);
> +
> +int wbrf_retrieve_exclusions(struct acpi_device *adev,
> +                            struct wbrf_ranges_out *exclusions_out)
> +{
> +       union acpi_object *obj;
> +
> +       obj = acpi_evaluate_wbrf(adev->handle,
> +                                WBRF_REVISION,
> +                                WBRF_RETRIEVE);
> +       if (!obj)
> +               return -EINVAL;
> +
> +       memcpy(exclusions_out, obj->buffer.pointer, obj->buffer.length);

How is it guaranteed that the length of the buffer will not exceed the
size of memory allocated by the caller for the data?

> +
> +       ACPI_FREE(obj);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(wbrf_retrieve_exclusions);
> diff --git a/include/linux/wbrf.h b/include/linux/wbrf.h
> new file mode 100644
> index 000000000000..e4c99b69f1d2
> --- /dev/null
> +++ b/include/linux/wbrf.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * AMD Wifi Band Exclusion Interface
> + * Copyright (C) 2023 Advanced Micro Devices
> + */
> +
> +#ifndef _LINUX_WBRF_H
> +#define _LINUX_WBRF_H
> +
> +#include <linux/acpi.h>
> +
> +/* Maximum number of wbrf ranges */
> +#define MAX_NUM_OF_WBRF_RANGES         11
> +
> +struct exclusion_range {
> +       /* start and end point of the frequency range in Hz */
> +       uint64_t        start;
> +       uint64_t        end;
> +};
> +
> +struct wbrf_ranges_in {
> +       /* valid entry: `start` and `end` filled with non-zero values */
> +       struct exclusion_range  band_list[MAX_NUM_OF_WBRF_RANGES];
> +};
> +
> +struct wbrf_ranges_out {
> +       uint32_t                num_of_ranges;
> +       struct exclusion_range  band_list[MAX_NUM_OF_WBRF_RANGES];
> +} __attribute__((packed));
> +
> +/**
> + * 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);
> +
> +/**
> + * APIs needed by drivers/subsystems responding to frequencies:
> + * During probe, check `wbrf_supported_consumer` to see if WBRF is supported.
> + * When receiving an ACPI notification for some frequencies change, run
> + * `wbrf_retrieve_exclusions` to retrieve the latest frequencies ranges.
> + */
> +int wbrf_retrieve_exclusions(struct acpi_device *adev,
> +                            struct wbrf_ranges_out *out);
> +bool wbrf_supported_consumer(struct acpi_device *adev);
> +
> +#endif /* _LINUX_WBRF_H */
> --
> 2.34.1
>
  

On 6/23/2023 11:28 AM, Rafael J. Wysocki wrote:
> On Fri, Jun 23, 2023 at 5:57 PM Limonciello, Mario
> <mario.limonciello@amd.com> 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@amd.com> wrote:
>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>
>>>> 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.
  

On Fri, Jun 23, 2023 at 6:48 PM Limonciello, Mario
<mario.limonciello@amd.com> 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@amd.com> 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@amd.com> wrote:
> >>>> From: Mario Limonciello <mario.limonciello@amd.com>
> >>>>
> >>>> 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.
  

[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@kernel.org>
> Sent: Saturday, June 24, 2023 1:16 AM
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>; Quan, Evan
> <Evan.Quan@amd.com>; lenb@kernel.org; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Koenig, Christian
> <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>;
> airlied@gmail.com; daniel@ffwll.ch; johannes@sipsolutions.net;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; mdaenzer@redhat.com;
> maarten.lankhorst@linux.intel.com; tzimmermann@suse.de;
> hdegoede@redhat.com; jingyuwang_vip@163.com; Lazar, Lijo
> <Lijo.Lazar@amd.com>; jim.cromie@gmail.com; bellosilicio@gmail.com;
> andrealmeid@igalia.com; trix@redhat.com; jsg@jsg.id.au; arnd@arndb.de;
> linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org; amd-
> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> wireless@vger.kernel.org; netdev@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
> <mario.limonciello@amd.com> 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@amd.com> 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@amd.com>
> wrote:
> > >>>> From: Mario Limonciello <mario.limonciello@amd.com>
> > >>>>
> > >>>> 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.