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

[Val Packett]

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@xxxxxxxxxxxxxxxx>
> ---
> [..]
> @@ -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@xxxxxxxxxxxx> # x1e80100-dell-latitude-7455

Well, tested without that too, but losing ASPM is not good :)


Thanks,
~val