> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Wednesday, February 8, 2023 4:50 AM
> To: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> Cc: Kalle Valo <kvalo@xxxxxxxxxx>; Leo.Li <leo.li@xxxxxxxxxxx>; Timlee <timlee@xxxxxxxxxxx>; Bernie Huang
> <phhuang@xxxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 4/5] wifi: rtw89: pci: enable CLK_REQ, ASPM, L1 and L1ss for 8852c
>
> 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.
The chunk of code is to configure L1SS of chip specific setting along with
standard PCI capability, and normally the setting and capability are consistent.
An exception is that PCI capability is enabled but chip specific setting is
disabled, when we want to use module parameter to disable chip specific
setting experimentally to resolve interoperability problem on some platforms.
We don't suggest the use case that L1SS of PCI capability is disabled but
chip specific setting is enabled, because hardware could get abnormal
occasionally. Also, it could also get unexpected behavior suddenly if we change
L1SS dynamically.
Summary:
PCI capability chip specific setting comment
-------------- --------------------- -------
enabled enabled ok, currently support
disabled disabled ok, currently support
enabled disabled experimental case via module parameter
disabled enabled don't suggest
With above reasons, if users meet problem or unexpected result after changing
L1SS, we may tell them this hardware can't dynamically configure L1SS via
sysfs interfaces.
Ping-Ke
