Re: [PATCH v8 13/14] PCI: tegra194: Disable L1.2 capability of Tegra234 EP

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Endpoint hardware bug workaround is incorrectly conditionalized on CONFIG_PCIEASPM.
--

--- Patch [13]: [PATCH v8 13/14] PCI: tegra194: Disable L1.2 capability of Tegra234 EP ---
commit 565b1a8d658f7e3ba31d5fa1ddf41455dcfe18ed
Author: Vidya Sagar <[email protected]>

    PCI: tegra194: Disable L1.2 capability of Tegra234 EP
    
    When Tegra234 is operating in the Endpoint mode with L1.2 enabled, PCIe
    link goes down during L1.2 exit. This is because Tegra234 is powering up
    UPHY PLL immediately without making sure that the REFCLK is stable.
    This is causing UPHY PLL to not lock to the correct frequency and leading
    to link going down. There is no hardware fix for this, hence do not
    advertise the L1.2 capability in the Endpoint mode.

> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index 47df4c6b5fe0f..b159836b7e9a0 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c

[ ... ]

> @@ -679,6 +680,22 @@ static void init_host_aspm(struct tegra_pcie_dw *pcie)
>  	if (pcie->supports_clkreq)
>  		pci->l1ss_support = true;
>  
> +	/*
> +	 * Disable L1.2 capability advertisement for Tegra234 Endpoint mode.
> +	 * Tegra234 has a hardware bug where during L1.2 exit, the UPHY PLL is
> +	 * powered up immediately without waiting for REFCLK to stabilize. This
> +	 * causes the PLL to fail to lock to the correct frequency, resulting in
> +	 * PCIe link loss. Since there is no hardware fix available, we prevent
> +	 * the Endpoint from advertising L1.2 support by clearing the L1.2 bits
> +	 * in the L1 PM Substates Capabilities register. This ensures the host
> +	 * will not attempt to enter L1.2 state with this Endpoint.
> +	 */
> +	if (pcie->of_data->disable_l1_2 && pcie->of_data->mode == DW_PCIE_EP_TYPE) {
> +		val = dw_pcie_readl_dbi(pci, l1ss + PCI_L1SS_CAP);
> +		val &= ~(PCI_L1SS_CAP_PCIPM_L1_2 | PCI_L1SS_CAP_ASPM_L1_2);
> +		dw_pcie_writel_dbi(pci, l1ss + PCI_L1SS_CAP, val);
> +	}

[Severity: High]
Does this conditionally compile out the endpoint hardware bug workaround if
CONFIG_PCIEASPM is disabled?

Looking at drivers/pci/controller/dwc/pcie-tegra194.c, init_host_aspm() is
guarded by a configuration check:

#if defined(CONFIG_PCIEASPM)
static void init_host_aspm(struct tegra_pcie_dw *pcie)
{
    ...
}
#else
static inline void init_host_aspm(struct tegra_pcie_dw *pcie) { return; }
#endif

Since ASPM configuration on the root complex (host) can be entirely
independent of the endpoint's kernel configuration, would this allow the host
to still enable L1.2 based on the endpoint's hardware advertisement if the
endpoint kernel is built without CONFIG_PCIEASPM?

> +
>  	/* Program L0s and L1 entrance latencies */
>  	val = dw_pcie_readl_dbi(pci, PCIE_PORT_AFR);
>  	val &= ~PORT_AFR_L0S_ENTRANCE_LAT_MASK;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13