Re: [PATCH 2/4] PCI: tegra194: Remove unnecessary L1SS disable code

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Nov 18, 2025 at 09:06:08PM +0100, Niklas Cassel wrote:
> On Tue, Nov 18, 2025 at 12:59:17PM -0600, Bjorn Helgaas wrote:
> > On Wed, Nov 12, 2025 at 09:29:20AM +0100, Niklas Cassel wrote:
> > > On Tue, Nov 11, 2025 at 04:16:09PM -0600, Bjorn Helgaas wrote:
> > > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > >
> > > > The DWC core clears the L1 Substates Supported bits unless the driver sets
> > > > the "dw_pcie.l1ss_support" flag.
> > > >
> > > > The tegra194 init_host_aspm() sets "dw_pcie.l1ss_support" if the platform
> > > > has the "supports-clkreq" DT property.  If "supports-clkreq" is absent,
> > > > "dw_pcie.l1ss_support" is not set, and the DWC core will clear the L1
> > > > Substates Supported bits.
> > > >
> > > > The tegra194 code to clear the L1 Substates Supported bits is unnecessary,
> > > > so remove it.
> > > >
> > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > ---
> > >
> > > Since init_host_aspm() is now the only place using struct tegra_pcie_dw
> > > struct member cfg_link_cap_l1sub, I think that you can remove this struct
> > > member, and instead make this a local variable in init_host_aspm().
> >
> > It looks like tegra_pcie_ep_irq_thread() also uses it, although I'm
> > dubious about that.
> >
> > It's odd that software would be responsible for sending LTR messages,
> > but I guess this only happens for tegra194_pcie_dw_ep_of_data, and
> > apparently it's fixed (".has_ltr_req_fix" for tegra234.
> >
> > And odd that we would read the capability register on every interrupt
> > even though this driver is the only thing that can change it, so we
> > should be able to cache the value in init_host_aspm().
> 
> Yes, sorry, my thinking was that this code in tegra_pcie_ep_irq_thread():
> 
> 	/* If EP doesn't advertise L1SS, just return */
> 	val = dw_pcie_readl_dbi(pci, pcie->cfg_link_cap_l1sub);
> 	if (!(val & (PCI_L1SS_CAP_ASPM_L1_1 | PCI_L1SS_CAP_ASPM_L1_2)))
> 		return IRQ_HANDLED;
> 
> 
> after your patch can be replaced with:
> 
> 	/* If EP doesn't advertise L1SS, just return */
> 	if (!pci->l1ss_support)
> 		return IRQ_HANDLED;
> 
> 
> Since we already know that if !pci->l1ss_support,
> these bits will be cleared by DWC common code.
> 
> 
> So, after that, we could replace the struct tegra_pcie_dw
> struct member cfg_link_cap_l1sub with a local variable in
> init_host_aspm().

Oh, of course, thanks!