[+cc Hans, Dave, linux-pci]
On Thu, Sep 07, 2017 at 04:26:39PM +0800, rui_feng@xxxxxxxxxxxxxx wrote:
> From: Rui Feng <rui_feng@xxxxxxxxxxxxxx>
I wish this had been posted to linux-pci before being merged.
I'm concerned because some of this appears to overlap and
conflict with PCI core management of ASPM.
I assume these devices advertise ASPM support in their Link
Capabilites registers, right? If so, why isn't the existing
PCI core ASPM support sufficient?
> Enable power saving for RTS5250S as following steps:
> 1.Set 0xFE58 to enable clock power management.
Is this clock power management something specific to RTS5250S, or is
it standard PCIe architected stuff?
> 2.Check cfg space whether support L1SS or not.
This sounds like standard PCIe ASPM L1 Substates, right?
> 3.If support L1SS, set 0xFF03 to free clkreq.
> 4.When entering idle status, enable aspm
> and set parameters for L1SS and LTR.
> 5.Wnen entering run status, disable aspm
> and set parameters for L1SS and LTR.
In general, drivers should not configure ASPM, L1SS, and LTR
themselves; the PCI core should do that.
If a driver needs to tweak ASPM at run-time, it should use interfaces
exported by the PCI core to do so.
> If entering L1SS mode successfully,
> electric current will be below 2mA.
> Signed-off-by: Rui Feng <rui_feng@xxxxxxxxxxxxxx>
> ...
> +static void rts5249_init_from_cfg(struct rtsx_pcr *pcr)
> +{
> + struct rtsx_cr_option *option = &(pcr->option);
> + u32 lval;
> +
> + if (CHK_PCI_PID(pcr, PID_524A))
> + rtsx_pci_read_config_dword(pcr,
> + PCR_ASPM_SETTING_REG1, &lval);
> + else
> + rtsx_pci_read_config_dword(pcr,
> + PCR_ASPM_SETTING_REG2, &lval);
This looks like you're reading the ASPM L1 Substates capability, i.e.,
PCI_L1SS_CAP, using hard-coded offsets based on the Device ID. You
should be using pci_find_ext_capability() to locate it.
> + if (lval & ASPM_L1_1_EN_MASK)
> + rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
> +
> + if (lval & ASPM_L1_2_EN_MASK)
> + rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
> +
> + if (lval & PM_L1_1_EN_MASK)
> + rtsx_set_dev_flag(pcr, PM_L1_1_EN);
> +
> + if (lval & PM_L1_2_EN_MASK)
> + rtsx_set_dev_flag(pcr, PM_L1_2_EN);
You're adding these:
#define ASPM_L1_1_EN_MASK BIT(3)
#define ASPM_L1_2_EN_MASK BIT(2)
#define PM_L1_1_EN_MASK BIT(1)
#define PM_L1_2_EN_MASK BIT(0)
The PCI core already defines these and you should use them instead:
#define PCI_L1SS_CAP_PCIPM_L1_2 0x00000001 /* PCI-PM L1.2 Supported */
#define PCI_L1SS_CAP_PCIPM_L1_1 0x00000002 /* PCI-PM L1.1 Supported */
#define PCI_L1SS_CAP_ASPM_L1_2 0x00000004 /* ASPM L1.2 Supported */
#define PCI_L1SS_CAP_ASPM_L1_1 0x00000008 /* ASPM L1.1 Supported */
> + if (option->ltr_en) {
> + u16 val;
> +
> + pcie_capability_read_word(pcr->pci, PCI_EXP_DEVCTL2, &val);
> + if (val & PCI_EXP_DEVCTL2_LTR_EN) {
> + option->ltr_enabled = true;
> + option->ltr_active = true;
ltr_active is never actually used.
> + rtsx_set_ltr_latency(pcr, option->ltr_active_latency);
> + } else {
> + option->ltr_enabled = false;
> + }
> + }
> +}
> ...
> +static void rts5249_set_aspm(struct rtsx_pcr *pcr, bool enable)
> +{
> + struct rtsx_cr_option *option = &pcr->option;
> + u8 val = 0;
> +
> + if (pcr->aspm_enabled == enable)
> + return;
> +
> + if (option->dev_aspm_mode == DEV_ASPM_DYNAMIC) {
> + if (enable)
> + val = pcr->aspm_en;
> + rtsx_pci_update_cfg_byte(pcr,
> + pcr->pcie_cap + PCI_EXP_LNKCTL,
> + ASPM_MASK_NEG, val);
This stomps on whatever ASPM configuration the PCI core did.
> + } else if (option->dev_aspm_mode == DEV_ASPM_BACKDOOR) {
DEV_ASPM_BACKDOOR is never set, so this looks like dead code.
> + u8 mask = FORCE_ASPM_VAL_MASK | FORCE_ASPM_CTL0;
> + if (!enable)
> + val = FORCE_ASPM_CTL0;
> + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, mask, val);
> + }
> +
> + pcr->aspm_enabled = enable;
> +}
