On Fri, Oct 01, 2021 at 09:58:55PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@xxxxxxxxxx>
>
> Current implementation of advk_pcie_link_up() is wrong as it marks also
> link disabled or hot reset states as link up.
>
> Fix it by marking link up only to those states which are defined in PCIe
> Base specification 3.0, Table 4-14: Link Status Mapped to the LTSSM.
>
> To simplify implementation, Define macros for every LTSSM state which
> aardvark hardware can return in CFG_REG register.
>
> Fix also checking for link training according to the same Table 4-14.
> Define a new function advk_pcie_link_training() for this purpose.
>
> Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
> Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>
> Reviewed-by: Marek Behún <kabel@xxxxxxxxxx>
> Signed-off-by: Marek Behún <kabel@xxxxxxxxxx>
> Cc: Remi Pommarel <repk@xxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> drivers/pci/controller/pci-aardvark.c | 69 +++++++++++++++++++++++++--
> 1 file changed, 64 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 5387d9cc3eba..9465b630cede 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -165,7 +165,44 @@
> #define CFG_REG (LMI_BASE_ADDR + 0x0)
> #define LTSSM_SHIFT 24
> #define LTSSM_MASK 0x3f
> +#define LTSSM_DETECT_QUIET 0x0
> +#define LTSSM_DETECT_ACTIVE 0x1
> +#define LTSSM_POLLING_ACTIVE 0x2
> +#define LTSSM_POLLING_COMPLIANCE 0x3
> +#define LTSSM_POLLING_CONFIGURATION 0x4
> +#define LTSSM_CONFIG_LINKWIDTH_START 0x5
> +#define LTSSM_CONFIG_LINKWIDTH_ACCEPT 0x6
> +#define LTSSM_CONFIG_LANENUM_ACCEPT 0x7
> +#define LTSSM_CONFIG_LANENUM_WAIT 0x8
> +#define LTSSM_CONFIG_COMPLETE 0x9
> +#define LTSSM_CONFIG_IDLE 0xa
> +#define LTSSM_RECOVERY_RCVR_LOCK 0xb
> +#define LTSSM_RECOVERY_SPEED 0xc
> +#define LTSSM_RECOVERY_RCVR_CFG 0xd
> +#define LTSSM_RECOVERY_IDLE 0xe
> #define LTSSM_L0 0x10
> +#define LTSSM_RX_L0S_ENTRY 0x11
> +#define LTSSM_RX_L0S_IDLE 0x12
> +#define LTSSM_RX_L0S_FTS 0x13
> +#define LTSSM_TX_L0S_ENTRY 0x14
> +#define LTSSM_TX_L0S_IDLE 0x15
> +#define LTSSM_TX_L0S_FTS 0x16
> +#define LTSSM_L1_ENTRY 0x17
> +#define LTSSM_L1_IDLE 0x18
> +#define LTSSM_L2_IDLE 0x19
> +#define LTSSM_L2_TRANSMIT_WAKE 0x1a
> +#define LTSSM_DISABLED 0x20
> +#define LTSSM_LOOPBACK_ENTRY_MASTER 0x21
> +#define LTSSM_LOOPBACK_ACTIVE_MASTER 0x22
> +#define LTSSM_LOOPBACK_EXIT_MASTER 0x23
> +#define LTSSM_LOOPBACK_ENTRY_SLAVE 0x24
> +#define LTSSM_LOOPBACK_ACTIVE_SLAVE 0x25
> +#define LTSSM_LOOPBACK_EXIT_SLAVE 0x26
> +#define LTSSM_HOT_RESET 0x27
> +#define LTSSM_RECOVERY_EQUALIZATION_PHASE0 0x28
> +#define LTSSM_RECOVERY_EQUALIZATION_PHASE1 0x29
> +#define LTSSM_RECOVERY_EQUALIZATION_PHASE2 0x2a
> +#define LTSSM_RECOVERY_EQUALIZATION_PHASE3 0x2b
An enum would be nicer IMO, for readability. Don't repost the series
yet.
Lorenzo
> #define RC_BAR_CONFIG 0x300
> #define VENDOR_ID_REG (LMI_BASE_ADDR + 0x44)
>
> @@ -258,13 +295,35 @@ static inline u32 advk_readl(struct advk_pcie *pcie, u64 reg)
> return readl(pcie->base + reg);
> }
>
> -static int advk_pcie_link_up(struct advk_pcie *pcie)
> +static u8 advk_pcie_ltssm_state(struct advk_pcie *pcie)
> {
> - u32 val, ltssm_state;
> + u32 val;
> + u8 ltssm_state;
>
> val = advk_readl(pcie, CFG_REG);
> ltssm_state = (val >> LTSSM_SHIFT) & LTSSM_MASK;
> - return ltssm_state >= LTSSM_L0;
> + return ltssm_state;
> +}
> +
> +static inline bool advk_pcie_link_up(struct advk_pcie *pcie)
> +{
> + /* check if LTSSM is in normal operation - some L* state */
> + u8 ltssm_state = advk_pcie_ltssm_state(pcie);
> + return ltssm_state >= LTSSM_L0 && ltssm_state < LTSSM_DISABLED;
> +}
> +
> +static inline bool advk_pcie_link_training(struct advk_pcie *pcie)
> +{
> + /*
> + * According to PCIe Base specification 3.0, Table 4-14: Link
> + * Status Mapped to the LTSSM is Link Training mapped to LTSSM
> + * Configuration and Recovery states.
> + */
> + u8 ltssm_state = advk_pcie_ltssm_state(pcie);
> + return ((ltssm_state >= LTSSM_CONFIG_LINKWIDTH_START &&
> + ltssm_state < LTSSM_L0) ||
> + (ltssm_state >= LTSSM_RECOVERY_EQUALIZATION_PHASE0 &&
> + ltssm_state <= LTSSM_RECOVERY_EQUALIZATION_PHASE3));
> }
>
> static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
> @@ -287,7 +346,7 @@ static void advk_pcie_wait_for_retrain(struct advk_pcie *pcie)
> size_t retries;
>
> for (retries = 0; retries < RETRAIN_WAIT_MAX_RETRIES; ++retries) {
> - if (!advk_pcie_link_up(pcie))
> + if (advk_pcie_link_training(pcie))
> break;
> udelay(RETRAIN_WAIT_USLEEP_US);
> }
> @@ -706,7 +765,7 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
> /* u32 contains both PCI_EXP_LNKCTL and PCI_EXP_LNKSTA */
> u32 val = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + reg) &
> ~(PCI_EXP_LNKSTA_LT << 16);
> - if (!advk_pcie_link_up(pcie))
> + if (advk_pcie_link_training(pcie))
> val |= (PCI_EXP_LNKSTA_LT << 16);
> *value = val;
> return PCI_BRIDGE_EMUL_HANDLED;
> --
> 2.32.0
>
