Re: [PATCH v2 1/2] PCI/ASPM: Override the ASPM and Clock PM states set by BIOS for devicetree platforms

[Niklas Cassel <cassel@kernel.org>]

On Wed, Oct 15, 2025 at 04:03:53PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Oct 15, 2025 at 11:46:02AM +0200, Niklas Cassel wrote:
> > Hello Shawn,
> > 
> > On Wed, Oct 15, 2025 at 05:11:39PM +0800, Shawn Lin wrote:
> > > > 
> > > > Thanks! Could you please try the below diff with f3ac2ff14834 applied?
> > > > 
> > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > > index 214ed060ca1b..0069d06c282d 100644
> > > > --- a/drivers/pci/quirks.c
> > > > +++ b/drivers/pci/quirks.c
> > > > @@ -2525,6 +2525,15 @@ static void quirk_disable_aspm_l0s_l1(struct pci_dev *dev)
> > > >    */
> > > >   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ASMEDIA, 0x1080, quirk_disable_aspm_l0s_l1);
> > > > 
> > > > +
> > > > +static void quirk_disable_aspm_all(struct pci_dev *dev)
> > > > +{
> > > > +       pci_info(dev, "Disabling ASPM\n");
> > > > +       pci_disable_link_state(dev, PCIE_LINK_STATE_ALL);
> > > > +}
> > > > +
> > > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ROCKCHIP, 0x3588, quirk_disable_aspm_all);
> > > 
> > > That's not true from my POV. Rockchip platform supports all ASPM policy
> > > after mass production verification. I also verified current upstream
> > > code this morning with RK3588-EVB and can check L0s/L1/L1ss work fine.
> > > 
> > > The log and lspci output could be found here:
> > > https://pastebin.com/qizeYED7
> > > 
> > > Moreover, I disscussed this issue with FUKAUMI today off-list and his
> > > board seems to work when only disable L1ss by patching:
> > > 
> > > --- a/drivers/pci/pcie/aspm.c
> > > +++ b/drivers/pci/pcie/aspm.c
> > > @@ -813,7 +813,7 @@ static void pcie_aspm_override_default_link_state(struct
> > > pcie_link_state *link)
> > > 
> > >         /* For devicetree platforms, enable all ASPM states by default */
> > >         if (of_have_populated_dt()) {
> > > -               link->aspm_default = PCIE_LINK_STATE_ASPM_ALL;
> > > +               link->aspm_default = PCIE_LINK_STATE_L0S |
> > > PCIE_LINK_STATE_L1;
> > >                 override = link->aspm_default & ~link->aspm_enabled;
> > >                 if (override)
> > >                         pci_info(pdev, "ASPM: DT platform,
> > > 
> > > 
> > > So, is there a proper way to just disable this feature for spec boards
> > > instead of this Soc?
> > 
> > This fix seems do the trick, without needing to patch common code (aspm.c):
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > index 3e2752c7dd09..f5e1aaa97719 100644
> > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > @@ -200,6 +200,19 @@ static bool rockchip_pcie_link_up(struct dw_pcie *pci)
> >  	return FIELD_GET(PCIE_LINKUP_MASK, val) == PCIE_LINKUP;
> >  }
> >  
> > +static void rockchip_pcie_disable_l1sub(struct dw_pcie *pci)
> > +{
> > +	u32 cap, l1subcap;
> > +
> > +	cap = dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS);
> > +	if (cap) {
> > +		l1subcap = dw_pcie_readl_dbi(pci, cap + PCI_L1SS_CAP);
> > +		l1subcap &= ~(PCI_L1SS_CAP_ASPM_L1_1 | PCI_L1SS_CAP_ASPM_L1_2 | PCI_L1SS_CAP_L1_PM_SS);
> > +		dw_pcie_writel_dbi(pci, cap + PCI_L1SS_CAP, l1subcap);
> > +		l1subcap = dw_pcie_readl_dbi(pci, cap + PCI_L1SS_CAP);
> > +	}
> > +}
> > +
> >  static void rockchip_pcie_enable_l0s(struct dw_pcie *pci)
> >  {
> >  	u32 cap, lnkcap;
> > @@ -264,6 +277,7 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
> >  	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
> >  					rockchip);
> >  
> > +	rockchip_pcie_disable_l1sub(pci);
> >  	rockchip_pcie_enable_l0s(pci);
> >  
> >  	return 0;
> > @@ -301,6 +315,7 @@ static void rockchip_pcie_ep_init(struct dw_pcie_ep *ep)
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >  	enum pci_barno bar;
> >  
> > +	rockchip_pcie_disable_l1sub(pci);
> >  	rockchip_pcie_enable_l0s(pci);
> >  	rockchip_pcie_ep_hide_broken_ats_cap_rk3588(ep);
> > 
> 
> But this patch removes the L1SS CAP for all boards, isn't it?

Yes, all boards supported by pcie-dw-rockchip.c, which matches what their
downstream driver does.

(Their downstream driver disables L1 substates for all boards that have
not defined 'supports-clkreq', and a grep through their downstream tree,
for all their all their different branches, shows that not a since rockchip
DTS has this property specified.)

So, let me submit a real patch with the above.


Kind regards,
Niklas