[+cc Puranjay, for LTR issues, original posting at https://lore.kernel.org/r/20200706070259.32565-1-ricky_wu@xxxxxxxxxxx] I've complained about some of this stuff before, but we haven't really made any progress yet: https://lore.kernel.org/lkml/20171214222522.GL30595@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ On Mon, Jul 06, 2020 at 03:02:59PM +0800, ricky_wu@xxxxxxxxxxx wrote: > From: Ricky Wu <ricky_wu@xxxxxxxxxxx> > > In order to support new chip rts5228, the definitions of some internal > registers and workflow have to be modified. > Added rts5228.c rts5228.h for independent functions of the new chip rts5228 > +static void rts5228_init_from_cfg(struct rtsx_pcr *pcr) > +{ > + u32 lval; > + struct rtsx_cr_option *option = &pcr->option; > + > + rtsx_pci_read_config_dword(pcr, PCR_ASPM_SETTING_REG1, &lval); > + > + > + if (0 == (lval & 0x0F)) > + rtsx_pci_enable_oobs_polling(pcr); > + else > + rtsx_pci_disable_oobs_polling(pcr); > + > + if (lval & ASPM_L1_1_EN_MASK) > + rtsx_set_dev_flag(pcr, ASPM_L1_1_EN); > + else > + rtsx_clear_dev_flag(pcr, ASPM_L1_1_EN); > + > + if (lval & ASPM_L1_2_EN_MASK) > + rtsx_set_dev_flag(pcr, ASPM_L1_2_EN); > + else > + rtsx_clear_dev_flag(pcr, ASPM_L1_2_EN); > + > + if (lval & PM_L1_1_EN_MASK) > + rtsx_set_dev_flag(pcr, PM_L1_1_EN); > + else > + rtsx_clear_dev_flag(pcr, PM_L1_1_EN); > + > + if (lval & PM_L1_2_EN_MASK) > + rtsx_set_dev_flag(pcr, PM_L1_2_EN); > + else > + rtsx_clear_dev_flag(pcr, PM_L1_2_EN); This looks like a bunch of driver-specific #defines that should be using the PCI core #defines instead (PCI_L1SS_CTL1_ASPM_L1_1, PCI_L1SS_CTL1_ASPM_L1_2, etc). rtsx_pci_read_config_dword() adds very little value and obscures the code unnecessarily. PCR_ASPM_SETTING_REG1 probably should be removed and replaced with something like pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS). > + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, 0xFF, 0); > + 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; > + rtsx_set_ltr_latency(pcr, option->ltr_active_latency); I do not believe this LTR programming is correct. But I'd be glad to be corrected with specific references to the spec. One reason I don't think it's correct is because PCIe r5.0, sec 6.18, says LTR must not be enabled unless the Root Complex and all intermediate Switches indicate support for LTR. I don't see any checking for that here. I *assume* that rtsx_set_ltr_latency() sets values in the LTR Extended Capability. That should be done with pci_write_config_word(), not with rtsx_pci_write_register() as is done in rtsx_comm_set_ltr_latency(). But maybe rtsx_set_ltr_latency() isn't doing what I think it is. I think the values programmed into the LTR Capability depend on some platform-specific values that can only be learned from an ACPI _DSM; see the PCI Firmware spec, v3.2, sec 4.6.6. It looks like option->ltr_active_latency is always LTR_ACTIVE_LATENCY_DEF (0x883C). How did you derive that value? All the LTR programming should be done by the PCI core. The PCI core does some of that, but not all. We should work on getting support done instead of spreading it around in drivers. > +static void rts5228_enable_aspm(struct rtsx_pcr *pcr, bool enable) > +{ > + u8 mask, val; > + > + if (pcr->aspm_enabled == enable) > + return; > + > + mask = FORCE_ASPM_VAL_MASK | FORCE_ASPM_CTL0 | FORCE_ASPM_CTL1; > + val = FORCE_ASPM_CTL0 | FORCE_ASPM_CTL1; > + val |= (pcr->aspm_en & 0x02); > + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, mask, val); > + pcie_capability_clear_and_set_word(pcr->pci, PCI_EXP_LNKCTL, > + PCI_EXP_LNKCTL_ASPMC, pcr->aspm_en); > + pcr->aspm_enabled = enable; ASPM configuration should also be done by the PCI core. Correct ASPM configuration can only be done by looking at *both* ends of the link. The PCI core is in a position to do that, but individual drivers really are not. Bjorn
