Re: [RFC PATCH rtw-next 5/5] wifi: rtw88: pci: clear CLKREQ/ASPM enables when disable_aspm is set

["Luka Gejak" <luka.gejak@linux.dev>]

Hi Bob,

For the SteamOS test build this should do the job. Few notes for upstraming
though:

On Wed, 21 Oct 2026, Bob Beckett wrote:
> Clear both bits in rtw_pci_link_cfg() when disable_aspm is set, so the
> parameter does what it says from the moment the driver probes.

rtw_pci_link_cfg() runs near the end of probe. By then
rtw_chip_info_setup() has already powered the MAC on, downloaded the
firmware and powered it off again with the old bits still set:

        rtw_pci_probe()
          rtw_chip_info_setup()
            rtw_chip_efuse_info_setup()
              rtw_chip_efuse_enable()
                rtw_mac_power_on()
                rtw_download_firmware()
              rtw_chip_efuse_disable()
          rtw_pci_phy_cfg()
            rtw_pci_link_cfg()

If that power on fails, probe bails out before the bits are cleared.
Should the clear go before rtw_chip_info_setup(), or should the message
say it takes effect once probe reaches rtw_pci_phy_cfg()?

> Assisted-by: Claude:claude-opus-5-5

816d9992d9ed simplified this tag, it now just says LLM instead of the
agent and model. Or you could just drop the tag in whole as that tag
requirement was dropped(will be dropped) in linux 7.4.

[...]
> +static void rtw_pci_link_hw_disable(struct rtw_dev *rtwdev)
> +{
> +     u8 value;
> +     int ret;
> +
> +     ret = rtw_dbi_read8(rtwdev, RTK_PCIE_LINK_CFG, &value);

disable_aspm was added in 68aa716b7dd3 for platforms where this very read
fails and the device then drops off the bus, and the FA506II quirk
(b0034a2e4156) reports the same thing. With this patch every disable_aspm
user, and the five rtw_pci_quirks[] machines on every boot, now do the
read at probe.

The reports I found hit it when leaving power save or around suspend, for
example https://bugs.launchpad.net/bugs/1872984 where it fails in
rtw_leave_ips() with the MAC off and BIT_L1_SW_EN set. After a reload
your read runs in that same state. Is it known to be safe there?

If not, clearing the bits in rtw_pci_remove() on loads that set them
would keep disable_aspm users off this register. It would also cover a
reload without disable_aspm, which today starts with L1 still on from
the last rtw_enter_ips().

> +     if (ret) {
> +             rtw_err(rtwdev, "failed to read link config, ret=%d", ret);

Missing \n, the helpers above have the same problem.

> +             return;
> +     }
> +
> +     value &= ~(BIT_CLKREQ_SW_EN | BIT_L1_SW_EN);
> +     rtw_dbi_write8(rtwdev, RTK_PCIE_LINK_CFG, value);
> +}

This is the fourth copy of the same read, mask and write of
RTK_PCIE_LINK_CFG in pci.c, a small helper taking the bits to clear and
set could serve all of them. rtw_pci_link_hw_disable() also reads as if
it turned the link off, a name closer to rtw_pci_clkreq_set() and
rtw_pci_aspm_set() would fit better.

[...]
> +     /* The CLKREQ/ASPM enable bits live in the device and are not reset
> +      * by a driver reload, so a previous load may have left them set.
> +      * Clear them explicitly when ASPM is disabled.
> +      */
> +     if (rtw_pci_disable_aspm)
> +             rtw_pci_link_hw_disable(rtwdev);

Block comments should start with /* on its own line, as netdev dropped
that comment style. The last sentence only repeats the if below and can
go, and naming BIT_CLKREQ_SW_EN and BIT_L1_SW_EN says more than "the
CLKREQ/ASPM enable bits".

Best regards,
Luka Gejak