Re: [PATCH v8 12/14] PCI: dwc: Apply ECRC workaround to DesignWare 5.00a as well

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The ECRC workaround is not applied to the Tegra234 SoC due to an incomplete hardware version check, and the updated code comments severely mismatch the implementation.
--

--- Patch [12]: [PATCH v8 12/14] PCI: dwc: Apply ECRC workaround to DesignWare 5.00a as well ---
commit b8f7e871ae3ebf76088785914137c4a00e66a2a1
Author: Manikanta Maddireddy <[email protected]>

PCI: dwc: Apply ECRC workaround to DesignWare 5.00a as well

This commit extends the ECRC (TLP digest) workaround, originally implemented
for DesignWare version 4.90a, to version 5.00a. It updates the ATU outbound
region programming to enable the workaround for the Tegra234 SoC.

> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 5741c09dde7f4..d69db0ab3b14d 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -487,7 +487,7 @@ static inline void dw_pcie_writel_atu_ob(struct dw_pcie *pci, u32 index, u32 reg
>  static inline u32 dw_pcie_enable_ecrc(u32 val)
>  {
>  	/*
> -	 * DesignWare core version 4.90A has a design issue where the 'TD'
> +	 * DWC versions 0x3530302a and 0x3536322a has a design issue where the 'TD'

[Severity: Medium]
Does this comment accurately reflect the versions that need the workaround?
The comment lists 0x3530302a (5.00a) and 0x3536322a (5.62a), but removes
the mention of 4.90a.

This creates a discrepancy with the implementation in
dw_pcie_prog_outbound_atu() below, which still checks for 4.90a but does
not check for 5.62a.

[ ... ]

> @@ -563,7 +563,7 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
>  	if (upper_32_bits(limit_addr) > upper_32_bits(parent_bus_addr) &&
>  	    dw_pcie_ver_is_ge(pci, 460A))
>  		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> -	if (dw_pcie_ver_is(pci, 490A))
> +	if (dw_pcie_ver_is(pci, 490A) || dw_pcie_ver_is(pci, 500A))
>  		val = dw_pcie_enable_ecrc(val);

[Severity: Medium]
Does this version check correctly apply the workaround for the Tegra234 SoC?

The commit message states that Tegra234 uses the 5.00a IP version, but
previous patches set TEGRA234_DWC_IP_VER to DW_PCIE_VER_562A (0x3536322a).

If Tegra234 is actually 5.62a, the explicit check for 500A here means the
workaround is entirely skipped for Tegra234, defeating the purpose of the
patch. Should this check also include dw_pcie_ver_is(pci, 562A) as suggested
by the updated comment above?

>  	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL1, val);

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