On Mon, Aug 14, 2023 at 09:50:49AM +, Quan, Evan wrote:
> [AMD Official Use Only - General]
>
> Hi Andrew,
>
> I sent out a new V8 series last week.
> A kernel parameter `wbrf` was introduced there to decide the policy.
> Please help to check whether that makes sense to you.
> Please share
> This comes back to the point that was mentioned by Johannes - you need to
> have deep design understanding of the hardware to know whether or not you
> will have producers that a consumer need to react to.
Yes, this is the policy is keep referring to. I would expect that
there is something
> > >> @@ -1395,6 +1395,8 @@ int ieee80211_register_hw(struct
> > ieee80211_hw *hw)
> > >>debugfs_hw_add(local);
> > >>rate_control_add_debugfs(local);
> > >>
> > >> + ieee80211_check_wbrf_support(local);
> > >> +
> > >>rtnl_lock();
> > >>wiphy_lock(hw->wiphy);
> > >>
> > >
> > >>
> @@ -1395,6 +1395,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> debugfs_hw_add(local);
> rate_control_add_debugfs(local);
>
> + ieee80211_check_wbrf_support(local);
> +
> rtnl_lock();
> wiphy_lock(hw->wiphy);
>
> +void
> The wbrf_supported_producer and wbrf_supported_consumer APIs seem
> unnecessary for the generic implementation.
I'm happy with these, once the description is corrected. As i said in
another comment, 'can' should be replaced with 'should'. The device
itself knows if it can, only the core knows
> +/**
> + * wbrf_supported_producer - Determine if the device can report frequencies
> + *
> + * @dev: device pointer
> + *
> + * WBRF is used to mitigate devices that cause harmonic interference.
> + * This function will determine if this device needs to report such
> frequencies.
How is the
> > What is the purpose of this stage? Why would it not be supported for this
> > device?
> This is needed for wbrf support via ACPI mechanism. If BIOS(AML code) does
> not support the wbrf adding/removing for some device,
> it should speak that out so that the device can be aware of that.
How
> Drivers/subsystems contributing frequencies:
>
> 1) During probe, check `wbrf_supported_producer` to see if WBRF supported
>for the device.
What is the purpose of this stage? Why would it not be supported for
this device?
> +#ifdef CONFIG_WBRF
> +bool wbrf_supported_producer(struct device
> Right now there are stubs for non CONFIG_WBRF as well as other patches are
> using #ifdef CONFIG_WBRF or having their own stubs. Like mac80211 patch
> looks for #ifdef CONFIG_WBRF.
>
> I think we should pick one or the other.
>
> Having other subsystems #ifdef CONFIG_WBRF will make the series
> + 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;
> +
> +static void get_chan_freq_boundary(u32 center_freq,
> +u32 bandwidth,
> +u64 *start,
> +u64 *end)
> +{
> + bandwidth = MHZ_TO_KHZ(bandwidth);
> + center_freq = MHZ_TO_KHZ(center_freq);
> +
>
> > 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
rf.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 doe
> 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
> 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
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
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
> 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
> 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
> 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
> 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
21 matches
Mail list logo
