Re: [PATCH v5 1/6] PCI/bwctrl: Set host bridge OPP and optionally disable ASPM around link retraining

[Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>]

On 10/7/2026 2:41 PM, Val Packett wrote:
>
> On 8/19/26 10:25 AM, Krishna Chaitanya Chundru wrote:
>> PCIe host bridge controllers may need their operating point raised before
>> retraining to a higher link speed so that hardware resources (e.g., RPMh
>> votes on Qualcomm platforms) are available at the requested data rate.
>> After retraining, the operating point must be updated to reflect the
>> actual negotiated speed.
>>
>> Add pcie_set_opp() to look up an OPP on the host bridge parent device
>> using a key of (per-lane frequency in kHz, LNKCTL2 Target Link Speed
>> level).  Keying by generation rather than total bandwidth lets OPP tables
>> remain width-independent.
>>
>> In pcie_set_target_speed(), call pcie_set_opp() before retraining only
>> when upscaling (speed_req > cur_bus_speed), since only raising the
>> operating point requires pre-staging hardware.  After retraining, call
>> pcie_set_opp() unconditionally with the actual cur_bus_speed to settle
>> the votes.  Both calls are skipped for downstream ports of PCIe switches,
>> as those are outside the host controller's scope.
>>
>> Some controllers also require ASPM to be disabled around link retraining.
>> Add a disable_aspm_for_retrain flag to pci_host_bridge; when set,
>> pcie_set_target_speed() saves the child device's ASPM state, disables all
>> ASPM link states before retraining, and restores them afterward.
>>
>> Signed-off-by: Krishna Chaitanya Chundru<krishna.chundru@oss.qualcomm.com>
>> ---
>> [..]
>> @@ -176,6 +231,12 @@ int pcie_set_target_speed(struct pci_dev *port, enum
>> pci_bus_speed speed_req,
>>           !list_empty(&bus->devices))
>>           ret = -EAGAIN;
>>   +    if (bus && is_rootbus && host) {
>> +        if (child && host->disable_aspm_for_retrain)
>> +            pci_enable_link_state_locked(child, aspm_state);
>> +        pcie_set_opp(port, host, bus->cur_bus_speed);
>> +    }
>> +
>>       return ret;
>>   }
>
> ASPM is not actually reenabled here:
>
> LnkCap: Port #0, Speed 16GT/s, Width x4, ASPM L1, Exit Latency L1 <8us
>         ClockPM- Surprise- LLActRep- BwNot- ASPMOptComp+
> LnkCtl: ASPM Disabled; RCB 128 bytes, LnkDisable- CommClk+
>         ExtSynch+ ClockPM- AutWidDis- BWInt- AutBWInt- FltModeDis-
> LnkSta: Speed 2.5GT/s (downgraded), Width x4
>         TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
>
>
> Because as the comment for pci_enable_link_state(_locked) says, "note that
> this does not enable states disabled by pci_disable_link_state(). Use
> pci_force_enable_link_state() for that"!
>
> This needs something like:
>
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 4bad311dc7..c54f2658e0 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1709,6 +1709,12 @@
>  }
>  EXPORT_SYMBOL(pci_force_enable_link_state);
>
> +int pci_force_enable_link_state_locked(struct pci_dev *pdev, int state)
> +{
> +    return __pci_enable_link_state(pdev, state, true, true);
> +}
> +EXPORT_SYMBOL(pci_force_enable_link_state_locked);
> +
>  void pcie_aspm_remove_cap(struct pci_dev *pdev, u32 lnkcap)
>  {
>      if (lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
> diff --git a/drivers/pci/pcie/bwctrl.c b/drivers/pci/pcie/bwctrl.c
> index 623731b96c..530b9047a0 100644
> --- a/drivers/pci/pcie/bwctrl.c
> +++ b/drivers/pci/pcie/bwctrl.c
> @@ -211,7 +211,7 @@
>
>      if (bus && is_rootbus && host) {
>          if (child && host->disable_aspm_for_retrain)
> -            pci_enable_link_state_locked(child, aspm_state);
> +            pci_force_enable_link_state_locked(child, aspm_state);
>          pcie_set_opp(port, host, bus->cur_bus_speed);
>      }
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 9f20bae6d7..59d7b67c9d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1960,6 +1960,7 @@
>  int pci_enable_link_state(struct pci_dev *pdev, int state);
>  int pci_enable_link_state_locked(struct pci_dev *pdev, int state);
>  int pci_force_enable_link_state(struct pci_dev *pdev, int state);
> +int pci_force_enable_link_state_locked(struct pci_dev *pdev, int state);
>  void pcie_no_aspm(void);
>  bool pcie_aspm_support_enabled(void);
>  u32 pcie_aspm_enabled(struct pci_dev *pdev);
> @@ -1974,6 +1975,8 @@
>  { return 0; }
>  static inline int pci_force_enable_link_state(struct pci_dev *pdev, int state)
>  { return 0; }
> +static inline int pci_force_enable_link_state_locked(struct pci_dev *pdev,
> int state)
> +{ return 0; }
>  static inline void pcie_no_aspm(void) { }
>  static inline bool pcie_aspm_support_enabled(void) { return false; }
>  static inline u32 pcie_aspm_enabled(struct pci_dev *pdev) { return 0; }
>
> (the addition of the new force+locked variant should go as a separate
> commit, but well)
>
> With that,
>
> Tested-by: Val Packett <val@packett.cool> # x1e80100-dell-latitude-7455
Thanks Val for testing and pointing the ASPM issue, there is actually patch
for this[1], I missed this new API change, I will fix in the next series.

[1] [PATCH v3 2/8] PCI/ASPM: Add pci_force_enable_link_state() API -
Manivannan Sadhasivam
<https://lore.kernel.org/all/20260708-pci-aspm-fix-v3-2-6bd72451746e@kernel.org/>

- Krishna Chaitanya.
>
> Well, tested without that too, but losing ASPM is not good :)
>
>
> Thanks,
> ~val
>