On Fri, Aug 19, 2022 at 02:48:10PM +0800, Ping-Ke Shih wrote:
> From: Chin-Yen Lee <timlee@xxxxxxxxxxx>
>
> 8852CE controls CLKREQ, ASPM L1, L1ss via wifi registers
> instead, so change them accordingly.
> ...
> static void rtw89_pci_l1ss_set(struct rtw89_dev *rtwdev, bool enable)
> {
> + enum rtw89_core_chip_id chip_id = rtwdev->chip->chip_id;
> int ret;
>
> - if (enable)
> - ret = rtw89_pci_config_byte_set(rtwdev, RTW89_PCIE_TIMER_CTRL,
> - RTW89_PCIE_BIT_L1SUB);
> - else
> - ret = rtw89_pci_config_byte_clr(rtwdev, RTW89_PCIE_TIMER_CTRL,
> - RTW89_PCIE_BIT_L1SUB);
> - if (ret)
> - rtw89_err(rtwdev, "failed to %s L1SS, ret=%d",
> - enable ? "set" : "unset", ret);
> + if (chip_id == RTL8852A || chip_id == RTL8852B) {
> + if (enable)
> + ret = rtw89_pci_config_byte_set(rtwdev,
> + RTW89_PCIE_TIMER_CTRL,
> + RTW89_PCIE_BIT_L1SUB);
> + else
> + ret = rtw89_pci_config_byte_clr(rtwdev,
> + RTW89_PCIE_TIMER_CTRL,
> + RTW89_PCIE_BIT_L1SUB);
> + if (ret)
> + rtw89_err(rtwdev, "failed to %s L1SS, ret=%d",
> + enable ? "set" : "unset", ret);
> + } else if (chip_id == RTL8852C) {
> + ret = rtw89_pci_config_byte_clr(rtwdev, RTW89_PCIE_L1SS_STS_V1,
> + RTW89_PCIE_BIT_ASPM_L11 |
> + RTW89_PCIE_BIT_PCI_L11);
> + if (ret)
> + rtw89_warn(rtwdev, "failed to unset ASPM L1.1, ret=%d", ret);
> + if (enable)
> + rtw89_write32_clr(rtwdev, R_AX_PCIE_MIX_CFG_V1,
> + B_AX_L1SUB_DISABLE);
> + else
> + rtw89_write32_set(rtwdev, R_AX_PCIE_MIX_CFG_V1,
> + B_AX_L1SUB_DISABLE);
> + }
> }
We get here via this path:
rtw89_pci_probe
rtw89_pci_l1ss_cfg
pci_read_config_dword(pdev, l1ss_cap_ptr + PCI_L1SS_CTL1, &l1ss_ctrl);
if (l1ss_ctrl & PCI_L1SS_CTL1_L1SS_MASK)
rtw89_pci_l1ss_set(rtwdev, true);
This looks like it might be a problem because L1SS configuration is
owned by the PCI core, not by the device driver. The PCI core
provides sysfs user interfaces that can enable and disable L1SS at
run-time without notification to the driver (see [1]).
The user may enable or disable L1SS using those sysfs interfaces, and
this code in the rtw89 driver will not be called.
Bjorn
P.S. rtw89_pci_l1ss_set() is only called from rtw89_pci_l1ss_cfg()
which always supplies "enable == true", so it looks like that
parameter is not needed.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-bus-pci?id=v6.1#n410
