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

Manivannan Sadhasivam
2025-11-06T07:07:16Z

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

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