[PATCH v3] PCI: dw-rockchip: Prevent advertising L1 Substates support

[Manivannan Sadhasivam]

On Tue, Nov 04, 2025 at 04:17:24PM -0600, Bjorn Helgaas wrote:
> On Tue, Nov 04, 2025 at 08:58:02AM +0800, Shawn Lin wrote:
> > 在 2025/11/04 星期二 5:32, Bjorn Helgaas 写道:
> > > On Tue, Oct 28, 2025 at 02:02:18PM -0500, Bjorn Helgaas wrote:
> > > > On Fri, Oct 17, 2025 at 06:32:53PM +0200, Niklas Cassel wrote:
> > > > > The L1 substates support requires additional steps to work, namely:
> > > > > -Proper handling of the CLKREQ# sideband signal. (It is mostly handled by
> > > > >   hardware, but software still needs to set the clkreq fields in the
> > > > >   PCIE_CLIENT_POWER_CON register to match the hardware implementation.)
> > > > > -Program the frequency of the aux clock into the
> > > > >   DSP_PCIE_PL_AUX_CLK_FREQ_OFF register. (During L1 substates the core_clk
> > > > >   is turned off and the aux_clk is used instead.)
> > > > ...
> > > 
> > > > > +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_L1_PM_SS | PCI_L1SS_CAP_ASPM_L1_1 |
> > > > > +			      PCI_L1SS_CAP_ASPM_L1_2 | PCI_L1SS_CAP_PCIPM_L1_1 |
> > > > > +			      PCI_L1SS_CAP_PCIPM_L1_2);
> > > > > +		dw_pcie_writel_dbi(pci, cap + PCI_L1SS_CAP, l1subcap);
> > > > > +	}
> > > > > +}
> > > > 
> > > > I like this.  But why should we do it just for dw-rockchip?  Is there
> > > > something special about dw-rockchip that makes this a problem?  Maybe
> > > > we should consider doing this in the dwc, cadence, mobiveil, and plda
> > > > cores instead of trying to do it for every driver individually?
> > > > 
> > > > Advertising L1SS support via PCI_EXT_CAP_ID_L1SS means users can
> > > > enable L1SS via CONFIG_PCIEASPM_POWER_SUPERSAVE=y or sysfs, and that
> > > > seems likely to cause problems unless CLKREQ# is supported.
> > > 
> > > Any thoughts on this?  There's nothing rockchip-specific in this
> > > patch.  What I'm proposing is something like this:
> > 
> > I like your idea, though. But could it be another form of regression
> > that we may breaks the platform which have already support L1SS
> > properly? It's even harder to detect because a functional break is easier to
> > notice than increased power consumption. 
> 
> True, but I think it's unlikely because the PCI core never enabled
> L1SS (except for CONFIG_PCIEASPM_POWER_SUPERSAVE=y or sysfs, which I
> doubt anybody really uses).
> 
> Devicetree platforms that use L1SS should have explicit code to enable
> it, like qcom does, so we should be able to find them and make sure
> they do what's needed to prevent the regression.
> 
> > Or maybe we could
> > just export dw_pcie_clear_l1ss_advert() in dwc for host drivers to
> > call it?
> 
> I don't like the idea of host drivers having to opt in for this
> because that requires changes to all of them, not just changes to
> drivers that have done the work to actually support L1SS.
> 

Otherwise, we may have to introduce a flag for the controller drivers to opt-out
of this disablement. Like, dw_pcie_rp::native_clkreq and bailing out early if
this flag is set.

- Mani

-- 
மணிவண்ணன் சதாசிவம்