[v2] PCI: mediatek-gen3: Avoid PCIe resetting via PCIE_RSTB for Airoha EN7581 SoC

Commit Message

Lorenzo Bianconi Nov. 4, 2024, 10 p.m. UTC

Airoha EN7581 has a hw bug asserting/releasing PCIE_PE_RSTB signal
causing occasional PCIe link down issues. In order to overcome the
problem, PCIE_RSTB signals are not asserted/released during device probe or
suspend/resume phase and the PCIe block is reset using REG_PCI_CONTROL
(0x88) and REG_RESET_CONTROL (0x834) registers available via the clock
module.
Introduce flags field in the mtk_gen3_pcie_pdata struct in order to
specify per-SoC capabilities.

Tested-by: Hui Ma <[e-mail]>
Signed-off-by: Lorenzo Bianconi <[e-mail]>
---
Changes in v2:
- introduce flags field in mtk_gen3_pcie_flags struct instead of adding
  reset callback
- fix the leftover case in mtk_pcie_suspend_noirq routine
- add more comments
- Link to v1: https://lore.kernel.org/r/[e-mail]
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 59 ++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 18 deletions(-)


---
base-commit: 3102ce10f3111e4c3b8fb233dc93f29e220adaf7
change-id: 20240920-pcie-en7581-rst-fix-8161658c13c4

Best regards,

Comments

Jianjun Wang (王建军) Nov. 5, 2024, 9:23 a.m. UTC | #1

On Mon, 2024-11-04 at 23:00 +0100, Lorenzo Bianconi wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Airoha EN7581 has a hw bug asserting/releasing PCIE_PE_RSTB signal
> causing occasional PCIe link down issues. In order to overcome the
> problem, PCIE_RSTB signals are not asserted/released during device
> probe or
> suspend/resume phase and the PCIe block is reset using
> REG_PCI_CONTROL
> (0x88) and REG_RESET_CONTROL (0x834) registers available via the
> clock
> module.
> Introduce flags field in the mtk_gen3_pcie_pdata struct in order to
> specify per-SoC capabilities.
> 
> Tested-by: Hui Ma <[e-mail]>
> Signed-off-by: Lorenzo Bianconi <[e-mail]>
> ---
> Changes in v2:
> - introduce flags field in mtk_gen3_pcie_flags struct instead of
> adding
>   reset callback
> - fix the leftover case in mtk_pcie_suspend_noirq routine
> - add more comments
> - Link to v1: 
> https://lore.kernel.org/r/[e-mail]
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 59
> ++++++++++++++++++++---------
>  1 file changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> b/drivers/pci/controller/pcie-mediatek-gen3.c
> index
> 66ce4b5d309bb6d64618c70ac5e0a529e0910511..8e4704ff3509867fc0ff799e9fb
> 99e71e46756cd 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -125,10 +125,18 @@
> 
>  struct mtk_gen3_pcie;
> 
> +enum mtk_gen3_pcie_flags {
> +       SKIP_PCIE_RSTB  = BIT(0), /* skip PCIE_RSTB signals
> configuration
> +                                  * during device probing or
> suspend/resume
> +                                  * phase in order to avoid hw
> bugs/issues.
> +                                  */
> +};
> +
>  /**
>   * struct mtk_gen3_pcie_pdata - differentiate between host
> generations
>   * @power_up: pcie power_up callback
>   * @phy_resets: phy reset lines SoC data.
> + * @flags: pcie device flags.
>   */
>  struct mtk_gen3_pcie_pdata {
>         int (*power_up)(struct mtk_gen3_pcie *pcie);
> @@ -136,6 +144,7 @@ struct mtk_gen3_pcie_pdata {
>                 const char *id[MAX_NUM_PHY_RESETS];
>                 int num_resets;
>         } phy_resets;
> +       u32 flags;
>  };
> 
>  /**
> @@ -402,22 +411,33 @@ static int mtk_pcie_startup_port(struct
> mtk_gen3_pcie *pcie)
>         val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
>         writel_relaxed(val, pcie->base + PCIE_MISC_CTRL_REG);
> 
> -       /* Assert all reset signals */
> -       val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> -       val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
> PCIE_PE_RSTB;
> -       writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> -
>         /*
> -        * Described in PCIe CEM specification sections 2.2 (PERST#
> Signal)
> -        * and 2.2.1 (Initial Power-Up (G3 to S0)).
> -        * The deassertion of PERST# should be delayed 100ms
> (TPVPERL)
> -        * for the power and clock to become stable.
> +        * Airoha EN7581 has a hw bug asserting/releasing
> PCIE_PE_RSTB signal
> +        * causing occasional PCIe link down. In order to overcome
> the issue,
> +        * PCIE_RSTB signals are not asserted/released at this stage
> and the
> +        * PCIe block is reset using REG_PCI_CONTROL (0x88) and
> +        * REG_RESET_CONTROL (0x834) registers available via the
> clock module.
>          */
> -       msleep(100);
> -
> -       /* De-assert reset signals */
> -       val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
> PCIE_PE_RSTB);
> -       writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);

What will happen if the EN7581 use this reset flow? Will it still work
after this link down?

> +       if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
> +               /* Assert all reset signals */
> +               val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> +               val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB
> |
> +                      PCIE_PE_RSTB;
> +               writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +
> +               /*
> +                * Described in PCIe CEM specification sections 2.2
> (PERST# Signal)
> +                * and 2.2.1 (Initial Power-Up (G3 to S0)).
> +                * The deassertion of PERST# should be delayed 100ms
> (TPVPERL)
> +                * for the power and clock to become stable.
> +                */
> +               msleep(PCIE_T_PVPERL_MS);
> +
> +               /* De-assert reset signals */
> +               val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB |
> PCIE_BRG_RSTB |
> +                        PCIE_PE_RSTB);
> +               writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +       }
> 
>         /* Check if the link is up or not */
>         err = readl_poll_timeout(pcie->base + PCIE_LINK_STATUS_REG,
> val,
> @@ -1160,10 +1180,12 @@ static int mtk_pcie_suspend_noirq(struct
> device *dev)
>                 return err;
>         }
> 
> -       /* Pull down the PERST# pin */
> -       val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> -       val |= PCIE_PE_RSTB;
> -       writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +       if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
> +               /* Pull down the PERST# pin */
> +               val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> +               val |= PCIE_PE_RSTB;
> +               writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +       }
> 
>         dev_dbg(pcie->dev, "entered L2 states successfully");
> 
> @@ -1214,6 +1236,7 @@ static const struct mtk_gen3_pcie_pdata
> mtk_pcie_soc_en7581 = {
>                 .id[2] = "phy-lane2",
>                 .num_resets = 3,
>         },
> +       .flags = SKIP_PCIE_RSTB,
>  };
> 
>  static const struct of_device_id mtk_pcie_of_match[] = {
> 
> ---
> base-commit: 3102ce10f3111e4c3b8fb233dc93f29e220adaf7
> change-id: 20240920-pcie-en7581-rst-fix-8161658c13c4
> 
> Best regards,
> --
> Lorenzo Bianconi <[e-mail]>
> 
>

Lorenzo Bianconi Nov. 5, 2024, 9:30 a.m. UTC | #2

> On Mon, 2024-11-04 at 23:00 +0100, Lorenzo Bianconi wrote:
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > Airoha EN7581 has a hw bug asserting/releasing PCIE_PE_RSTB signal
> > causing occasional PCIe link down issues. In order to overcome the
> > problem, PCIE_RSTB signals are not asserted/released during device
> > probe or
> > suspend/resume phase and the PCIe block is reset using
> > REG_PCI_CONTROL
> > (0x88) and REG_RESET_CONTROL (0x834) registers available via the
> > clock
> > module.
> > Introduce flags field in the mtk_gen3_pcie_pdata struct in order to
> > specify per-SoC capabilities.
> > 
> > Tested-by: Hui Ma <[e-mail]>
> > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > ---
> > Changes in v2:
> > - introduce flags field in mtk_gen3_pcie_flags struct instead of
> > adding
> >   reset callback
> > - fix the leftover case in mtk_pcie_suspend_noirq routine
> > - add more comments
> > - Link to v1: 
> > https://lore.kernel.org/r/[e-mail]
> > ---
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 59
> > ++++++++++++++++++++---------
> >  1 file changed, 41 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index
> > 66ce4b5d309bb6d64618c70ac5e0a529e0910511..8e4704ff3509867fc0ff799e9fb
> > 99e71e46756cd 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -125,10 +125,18 @@
> > 
> >  struct mtk_gen3_pcie;
> > 
> > +enum mtk_gen3_pcie_flags {
> > +       SKIP_PCIE_RSTB  = BIT(0), /* skip PCIE_RSTB signals
> > configuration
> > +                                  * during device probing or
> > suspend/resume
> > +                                  * phase in order to avoid hw
> > bugs/issues.
> > +                                  */
> > +};
> > +
> >  /**
> >   * struct mtk_gen3_pcie_pdata - differentiate between host
> > generations
> >   * @power_up: pcie power_up callback
> >   * @phy_resets: phy reset lines SoC data.
> > + * @flags: pcie device flags.
> >   */
> >  struct mtk_gen3_pcie_pdata {
> >         int (*power_up)(struct mtk_gen3_pcie *pcie);
> > @@ -136,6 +144,7 @@ struct mtk_gen3_pcie_pdata {
> >                 const char *id[MAX_NUM_PHY_RESETS];
> >                 int num_resets;
> >         } phy_resets;
> > +       u32 flags;
> >  };
> > 
> >  /**
> > @@ -402,22 +411,33 @@ static int mtk_pcie_startup_port(struct
> > mtk_gen3_pcie *pcie)
> >         val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> >         writel_relaxed(val, pcie->base + PCIE_MISC_CTRL_REG);
> > 
> > -       /* Assert all reset signals */
> > -       val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> > -       val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
> > PCIE_PE_RSTB;
> > -       writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > -
> >         /*
> > -        * Described in PCIe CEM specification sections 2.2 (PERST#
> > Signal)
> > -        * and 2.2.1 (Initial Power-Up (G3 to S0)).
> > -        * The deassertion of PERST# should be delayed 100ms
> > (TPVPERL)
> > -        * for the power and clock to become stable.
> > +        * Airoha EN7581 has a hw bug asserting/releasing
> > PCIE_PE_RSTB signal
> > +        * causing occasional PCIe link down. In order to overcome
> > the issue,
> > +        * PCIE_RSTB signals are not asserted/released at this stage
> > and the
> > +        * PCIe block is reset using REG_PCI_CONTROL (0x88) and
> > +        * REG_RESET_CONTROL (0x834) registers available via the
> > clock module.
> >          */
> > -       msleep(100);
> > -
> > -       /* De-assert reset signals */
> > -       val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
> > PCIE_PE_RSTB);
> > -       writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> 
> What will happen if the EN7581 use this reset flow? Will it still work
> after this link down?

Hi Jianjun Wang,

This has been described here by Hui Ma:
https://lore.kernel.org/r/[e-mail]

Setting PCIE_PE_RSTB bit on EN7581 SoC during reset triggers occasional PCIe link
down issues caused by a hw problem.

Regards,
Lorenzo

> 
> > +       if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
> > +               /* Assert all reset signals */
> > +               val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> > +               val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB
> > |
> > +                      PCIE_PE_RSTB;
> > +               writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > +
> > +               /*
> > +                * Described in PCIe CEM specification sections 2.2
> > (PERST# Signal)
> > +                * and 2.2.1 (Initial Power-Up (G3 to S0)).
> > +                * The deassertion of PERST# should be delayed 100ms
> > (TPVPERL)
> > +                * for the power and clock to become stable.
> > +                */
> > +               msleep(PCIE_T_PVPERL_MS);
> > +
> > +               /* De-assert reset signals */
> > +               val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB |
> > PCIE_BRG_RSTB |
> > +                        PCIE_PE_RSTB);
> > +               writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > +       }
> > 
> >         /* Check if the link is up or not */
> >         err = readl_poll_timeout(pcie->base + PCIE_LINK_STATUS_REG,
> > val,
> > @@ -1160,10 +1180,12 @@ static int mtk_pcie_suspend_noirq(struct
> > device *dev)
> >                 return err;
> >         }
> > 
> > -       /* Pull down the PERST# pin */
> > -       val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> > -       val |= PCIE_PE_RSTB;
> > -       writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > +       if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
> > +               /* Pull down the PERST# pin */
> > +               val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> > +               val |= PCIE_PE_RSTB;
> > +               writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > +       }
> > 
> >         dev_dbg(pcie->dev, "entered L2 states successfully");
> > 
> > @@ -1214,6 +1236,7 @@ static const struct mtk_gen3_pcie_pdata
> > mtk_pcie_soc_en7581 = {
> >                 .id[2] = "phy-lane2",
> >                 .num_resets = 3,
> >         },
> > +       .flags = SKIP_PCIE_RSTB,
> >  };
> > 
> >  static const struct of_device_id mtk_pcie_of_match[] = {
> > 
> > ---
> > base-commit: 3102ce10f3111e4c3b8fb233dc93f29e220adaf7
> > change-id: 20240920-pcie-en7581-rst-fix-8161658c13c4
> > 
> > Best regards,
> > --
> > Lorenzo Bianconi <[e-mail]>
> > 
> >

Bjorn Helgaas Nov. 5, 2024, 5:22 p.m. UTC | #3

On Mon, Nov 04, 2024 at 11:00:05PM +0100, Lorenzo Bianconi wrote:
> Airoha EN7581 has a hw bug asserting/releasing PCIE_PE_RSTB signal
> causing occasional PCIe link down issues. In order to overcome the
> problem, PCIE_RSTB signals are not asserted/released during device probe or
> suspend/resume phase and the PCIe block is reset using REG_PCI_CONTROL
> (0x88) and REG_RESET_CONTROL (0x834) registers available via the clock
> module.
> Introduce flags field in the mtk_gen3_pcie_pdata struct in order to
> specify per-SoC capabilities.

Add blank lines between paragraphs so we know where they end.

Where does this alternate way of doing reset (using REG_PCI_CONTROL
and REG_RESET_CONTROL) happen?  Why isn't there something in this
patch to use that alternate method at the same points where
PCIE_PE_RSTB is used?

If we don't need to assert reset for Airoha EN7581, why do we need to
do it for the other SoCs?

> Tested-by: Hui Ma <[e-mail]>
> Signed-off-by: Lorenzo Bianconi <[e-mail]>
> ---
> Changes in v2:
> - introduce flags field in mtk_gen3_pcie_flags struct instead of adding
>   reset callback
> - fix the leftover case in mtk_pcie_suspend_noirq routine
> - add more comments
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 59 ++++++++++++++++++++---------
>  1 file changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 66ce4b5d309bb6d64618c70ac5e0a529e0910511..8e4704ff3509867fc0ff799e9fb99e71e46756cd 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -125,10 +125,18 @@
>  
>  struct mtk_gen3_pcie;
>  
> +enum mtk_gen3_pcie_flags {
> +	SKIP_PCIE_RSTB	= BIT(0), /* skip PCIE_RSTB signals configuration
> +				   * during device probing or suspend/resume
> +				   * phase in order to avoid hw bugs/issues.
> +				   */
> +};
> +
>  /**
>   * struct mtk_gen3_pcie_pdata - differentiate between host generations
>   * @power_up: pcie power_up callback
>   * @phy_resets: phy reset lines SoC data.
> + * @flags: pcie device flags.
>   */
>  struct mtk_gen3_pcie_pdata {
>  	int (*power_up)(struct mtk_gen3_pcie *pcie);
> @@ -136,6 +144,7 @@ struct mtk_gen3_pcie_pdata {
>  		const char *id[MAX_NUM_PHY_RESETS];
>  		int num_resets;
>  	} phy_resets;
> +	u32 flags;
>  };
>  
>  /**
> @@ -402,22 +411,33 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>  	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
>  	writel_relaxed(val, pcie->base + PCIE_MISC_CTRL_REG);
>  
> -	/* Assert all reset signals */
> -	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> -	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> -	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> -
>  	/*
> -	 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> -	 * and 2.2.1 (Initial Power-Up (G3 to S0)).
> -	 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> -	 * for the power and clock to become stable.
> +	 * Airoha EN7581 has a hw bug asserting/releasing PCIE_PE_RSTB signal
> +	 * causing occasional PCIe link down. In order to overcome the issue,
> +	 * PCIE_RSTB signals are not asserted/released at this stage and the
> +	 * PCIe block is reset using REG_PCI_CONTROL (0x88) and
> +	 * REG_RESET_CONTROL (0x834) registers available via the clock module.
>  	 */
> -	msleep(100);
> -
> -	/* De-assert reset signals */
> -	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
> -	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +	if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
> +		/* Assert all reset signals */
> +		val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> +		val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
> +		       PCIE_PE_RSTB;
> +		writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +
> +		/*
> +		 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> +		 * and 2.2.1 (Initial Power-Up (G3 to S0)).
> +		 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> +		 * for the power and clock to become stable.

Blank line between paragraphs.

> +		 */
> +		msleep(PCIE_T_PVPERL_MS);
> +
> +		/* De-assert reset signals */
> +		val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
> +			 PCIE_PE_RSTB);
> +		writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +	}
>  
>  	/* Check if the link is up or not */
>  	err = readl_poll_timeout(pcie->base + PCIE_LINK_STATUS_REG, val,
> @@ -1160,10 +1180,12 @@ static int mtk_pcie_suspend_noirq(struct device *dev)
>  		return err;
>  	}
>  
> -	/* Pull down the PERST# pin */
> -	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> -	val |= PCIE_PE_RSTB;
> -	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +	if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
> +		/* Pull down the PERST# pin */
> +		val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> +		val |= PCIE_PE_RSTB;
> +		writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +	}
>  
>  	dev_dbg(pcie->dev, "entered L2 states successfully");
>  
> @@ -1214,6 +1236,7 @@ static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_en7581 = {
>  		.id[2] = "phy-lane2",
>  		.num_resets = 3,
>  	},
> +	.flags = SKIP_PCIE_RSTB,
>  };
>  
>  static const struct of_device_id mtk_pcie_of_match[] = {
> 
> ---
> base-commit: 3102ce10f3111e4c3b8fb233dc93f29e220adaf7
> change-id: 20240920-pcie-en7581-rst-fix-8161658c13c4
> 
> Best regards,
> -- 
> Lorenzo Bianconi <[e-mail]>
>

Lorenzo Bianconi Nov. 5, 2024, 6:13 p.m. UTC | #4

> On Mon, Nov 04, 2024 at 11:00:05PM +0100, Lorenzo Bianconi wrote:
> > Airoha EN7581 has a hw bug asserting/releasing PCIE_PE_RSTB signal
> > causing occasional PCIe link down issues. In order to overcome the
> > problem, PCIE_RSTB signals are not asserted/released during device probe or
> > suspend/resume phase and the PCIe block is reset using REG_PCI_CONTROL
> > (0x88) and REG_RESET_CONTROL (0x834) registers available via the clock
> > module.
> > Introduce flags field in the mtk_gen3_pcie_pdata struct in order to
> > specify per-SoC capabilities.
> 
> Add blank lines between paragraphs so we know where they end.

ack, I will fix it in v2.

> 
> Where does this alternate way of doing reset (using REG_PCI_CONTROL
> and REG_RESET_CONTROL) happen?  Why isn't there something in this
> patch to use that alternate method at the same points where
> PCIE_PE_RSTB is used?

REG_RESET_CONTROL (0x834) is already asserted/released in the following flow:

mtk_pcie_en7581_power_up() -> reset_control_bulk_deassert() -> en7523_reset_update()
https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L470

REG_PCI_CONTROL (0x88) is already asserted/released in the following flow:
mtk_pcie_en7581_power_up() -> clk_bulk_enable() -> en7581_pci_enable()
https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L385


> 
> If we don't need to assert reset for Airoha EN7581, why do we need to
> do it for the other SoCs?

I guess the other SoCs (mtk ones) do not have the same hw issue with
PCIE_PE_RSTB (but I am not sure about it).

Regards,
Lorenzo

> 
> > Tested-by: Hui Ma <[e-mail]>
> > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > ---
> > Changes in v2:
> > - introduce flags field in mtk_gen3_pcie_flags struct instead of adding
> >   reset callback
> > - fix the leftover case in mtk_pcie_suspend_noirq routine
> > - add more comments
> > - Link to v1: https://lore.kernel.org/r/[e-mail]
> > ---
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 59 ++++++++++++++++++++---------
> >  1 file changed, 41 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index 66ce4b5d309bb6d64618c70ac5e0a529e0910511..8e4704ff3509867fc0ff799e9fb99e71e46756cd 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -125,10 +125,18 @@
> >  
> >  struct mtk_gen3_pcie;
> >  
> > +enum mtk_gen3_pcie_flags {
> > +	SKIP_PCIE_RSTB	= BIT(0), /* skip PCIE_RSTB signals configuration
> > +				   * during device probing or suspend/resume
> > +				   * phase in order to avoid hw bugs/issues.
> > +				   */
> > +};
> > +
> >  /**
> >   * struct mtk_gen3_pcie_pdata - differentiate between host generations
> >   * @power_up: pcie power_up callback
> >   * @phy_resets: phy reset lines SoC data.
> > + * @flags: pcie device flags.
> >   */
> >  struct mtk_gen3_pcie_pdata {
> >  	int (*power_up)(struct mtk_gen3_pcie *pcie);
> > @@ -136,6 +144,7 @@ struct mtk_gen3_pcie_pdata {
> >  		const char *id[MAX_NUM_PHY_RESETS];
> >  		int num_resets;
> >  	} phy_resets;
> > +	u32 flags;
> >  };
> >  
> >  /**
> > @@ -402,22 +411,33 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
> >  	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> >  	writel_relaxed(val, pcie->base + PCIE_MISC_CTRL_REG);
> >  
> > -	/* Assert all reset signals */
> > -	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> > -	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> > -	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > -
> >  	/*
> > -	 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> > -	 * and 2.2.1 (Initial Power-Up (G3 to S0)).
> > -	 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> > -	 * for the power and clock to become stable.
> > +	 * Airoha EN7581 has a hw bug asserting/releasing PCIE_PE_RSTB signal
> > +	 * causing occasional PCIe link down. In order to overcome the issue,
> > +	 * PCIE_RSTB signals are not asserted/released at this stage and the
> > +	 * PCIe block is reset using REG_PCI_CONTROL (0x88) and
> > +	 * REG_RESET_CONTROL (0x834) registers available via the clock module.
> >  	 */
> > -	msleep(100);
> > -
> > -	/* De-assert reset signals */
> > -	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
> > -	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > +	if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
> > +		/* Assert all reset signals */
> > +		val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> > +		val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
> > +		       PCIE_PE_RSTB;
> > +		writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > +
> > +		/*
> > +		 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> > +		 * and 2.2.1 (Initial Power-Up (G3 to S0)).
> > +		 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> > +		 * for the power and clock to become stable.
> 
> Blank line between paragraphs.
> 
> > +		 */
> > +		msleep(PCIE_T_PVPERL_MS);
> > +
> > +		/* De-assert reset signals */
> > +		val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
> > +			 PCIE_PE_RSTB);
> > +		writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > +	}
> >  
> >  	/* Check if the link is up or not */
> >  	err = readl_poll_timeout(pcie->base + PCIE_LINK_STATUS_REG, val,
> > @@ -1160,10 +1180,12 @@ static int mtk_pcie_suspend_noirq(struct device *dev)
> >  		return err;
> >  	}
> >  
> > -	/* Pull down the PERST# pin */
> > -	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> > -	val |= PCIE_PE_RSTB;
> > -	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > +	if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
> > +		/* Pull down the PERST# pin */
> > +		val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> > +		val |= PCIE_PE_RSTB;
> > +		writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > +	}
> >  
> >  	dev_dbg(pcie->dev, "entered L2 states successfully");
> >  
> > @@ -1214,6 +1236,7 @@ static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_en7581 = {
> >  		.id[2] = "phy-lane2",
> >  		.num_resets = 3,
> >  	},
> > +	.flags = SKIP_PCIE_RSTB,
> >  };
> >  
> >  static const struct of_device_id mtk_pcie_of_match[] = {
> > 
> > ---
> > base-commit: 3102ce10f3111e4c3b8fb233dc93f29e220adaf7
> > change-id: 20240920-pcie-en7581-rst-fix-8161658c13c4
> > 
> > Best regards,
> > -- 
> > Lorenzo Bianconi <[e-mail]>
> >

Bjorn Helgaas Nov. 5, 2024, 8:57 p.m. UTC | #5

On Tue, Nov 05, 2024 at 07:13:52PM +0100, Lorenzo Bianconi wrote:
> > On Mon, Nov 04, 2024 at 11:00:05PM +0100, Lorenzo Bianconi wrote:
> > > Airoha EN7581 has a hw bug asserting/releasing PCIE_PE_RSTB signal
> > > causing occasional PCIe link down issues. In order to overcome the
> > > problem, PCIE_RSTB signals are not asserted/released during device probe or
> > > suspend/resume phase and the PCIe block is reset using REG_PCI_CONTROL
> > > (0x88) and REG_RESET_CONTROL (0x834) registers available via the clock
> > > module.
> > > Introduce flags field in the mtk_gen3_pcie_pdata struct in order to
> > > specify per-SoC capabilities.
> > 
> > Where does this alternate way of doing reset (using REG_PCI_CONTROL
> > and REG_RESET_CONTROL) happen?  Why isn't there something in this
> > patch to use that alternate method at the same points where
> > PCIE_PE_RSTB is used?
> 
> REG_RESET_CONTROL (0x834) is already asserted/released in the following flow:
> 
> mtk_pcie_en7581_power_up() -> reset_control_bulk_deassert() -> en7523_reset_update()
> https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L470
> 
> REG_PCI_CONTROL (0x88) is already asserted/released in the following flow:
> mtk_pcie_en7581_power_up() -> clk_bulk_enable() -> en7581_pci_enable()
> https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L385

So IIUC, you're saying that on EN7581, the PCI hierarchy is reset by
the soc->power_up() callback, mtk_pcie_en7581_power_up(), via
REG_PCI_CONTROL and REG_RESET_CONTROL.

I assume the hierarchy is also reset by the non-EN7581 .power_up()
callback, mtk_pcie_power_up()?

And prior to this patch, we reset the hierarchy *again* in
mtk_pcie_startup_port() via PCIE_RST_CTRL_REG, but this causes
occasional "link down" issues because of a EN7581 hardware defect.

So for EN7581, this patch skips the PCIE_RST_CTRL_REG reset in
mtk_pcie_startup_port().

.power_up() and mtk_pcie_startup_port() are used both at probe time
and in mtk_pcie_resume_noirq().  So after this patch, I assume:

  - EN7581 resets the hierarchy once at probe and resume instead of
    twice.

  - Non-EN7581 resets the hierarchy twice at probe and resume.

I assume I'm missing something (maybe mtk_pcie_power_up() doesn't
actually reset the hierarchy?) because I don't see why we would reset
the hierarchy twice for either controller.

Bjorn

Jianjun Wang (王建军) Nov. 6, 2024, 1:18 a.m. UTC | #6

Hi Bjorn,

On Tue, 2024-11-05 at 14:57 -0600, Bjorn Helgaas wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Tue, Nov 05, 2024 at 07:13:52PM +0100, Lorenzo Bianconi wrote:
> > > On Mon, Nov 04, 2024 at 11:00:05PM +0100, Lorenzo Bianconi wrote:
> > > > Airoha EN7581 has a hw bug asserting/releasing PCIE_PE_RSTB
> > > > signal
> > > > causing occasional PCIe link down issues. In order to overcome
> > > > the
> > > > problem, PCIE_RSTB signals are not asserted/released during
> > > > device probe or
> > > > suspend/resume phase and the PCIe block is reset using
> > > > REG_PCI_CONTROL
> > > > (0x88) and REG_RESET_CONTROL (0x834) registers available via
> > > > the clock
> > > > module.
> > > > Introduce flags field in the mtk_gen3_pcie_pdata struct in
> > > > order to
> > > > specify per-SoC capabilities.
> > > 
> > > Where does this alternate way of doing reset (using
> > > REG_PCI_CONTROL
> > > and REG_RESET_CONTROL) happen?  Why isn't there something in this
> > > patch to use that alternate method at the same points where
> > > PCIE_PE_RSTB is used?
> > 
> > REG_RESET_CONTROL (0x834) is already asserted/released in the
> > following flow:
> > 
> > mtk_pcie_en7581_power_up() -> reset_control_bulk_deassert() ->
> > en7523_reset_update()
> > 
https://urldefense.com/v3/__https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c*L470__;Iw!!CTRNKA9wMg0ARbw!mB58-w-wCBlwng2b0U_dcXijrb0y9Z9T5Xb0uNqMeNLcNZkN4SkyGK8cXfcW0bSE5RX3wEfvYITZ3CFhgTZ_$
> > 
> > REG_PCI_CONTROL (0x88) is already asserted/released in the
> > following flow:
> > mtk_pcie_en7581_power_up() -> clk_bulk_enable() ->
> > en7581_pci_enable()
> > 
https://urldefense.com/v3/__https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c*L385__;Iw!!CTRNKA9wMg0ARbw!mB58-w-wCBlwng2b0U_dcXijrb0y9Z9T5Xb0uNqMeNLcNZkN4SkyGK8cXfcW0bSE5RX3wEfvYITZ3L8Llrnw$
> 
> So IIUC, you're saying that on EN7581, the PCI hierarchy is reset by
> the soc->power_up() callback, mtk_pcie_en7581_power_up(), via
> REG_PCI_CONTROL and REG_RESET_CONTROL.
> 
> I assume the hierarchy is also reset by the non-EN7581 .power_up()
> callback, mtk_pcie_power_up()?
> 
> And prior to this patch, we reset the hierarchy *again* in
> mtk_pcie_startup_port() via PCIE_RST_CTRL_REG, but this causes
> occasional "link down" issues because of a EN7581 hardware defect.
> 
> So for EN7581, this patch skips the PCIE_RST_CTRL_REG reset in
> mtk_pcie_startup_port().
> 
> .power_up() and mtk_pcie_startup_port() are used both at probe time
> and in mtk_pcie_resume_noirq().  So after this patch, I assume:
> 
>   - EN7581 resets the hierarchy once at probe and resume instead of
>     twice.
> 
>   - Non-EN7581 resets the hierarchy twice at probe and resume.
> 
> I assume I'm missing something (maybe mtk_pcie_power_up() doesn't
> actually reset the hierarchy?) because I don't see why we would reset
> the hierarchy twice for either controller.

Non-EN7581 only reset once, the original intention of
mtk_pcie_power_up() is to perform some power-up things:
1. Release hardware resets(this is not PERST#, but the hardware IP's
reset).
2. Initialize the PHY through PHY APIs.
3. Power on and enable the MAC's clocks.

After these steps, we initialize the PCIe interface in
mtk_pcie_startup_port() and toggle PERST# to reset the hierarchy as
required by the PCIe Spec. At this point, the PCIe link should be
ready.

Thanks.

> 
> Bjorn
>

Jianjun Wang (王建军) Nov. 6, 2024, 8:25 a.m. UTC | #7

On Tue, 2024-11-05 at 10:30 +0100, [e-mail] wrote:
> > On Mon, 2024-11-04 at 23:00 +0100, Lorenzo Bianconi wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > Airoha EN7581 has a hw bug asserting/releasing PCIE_PE_RSTB
> > > signal
> > > causing occasional PCIe link down issues. In order to overcome
> > > the
> > > problem, PCIE_RSTB signals are not asserted/released during
> > > device
> > > probe or
> > > suspend/resume phase and the PCIe block is reset using
> > > REG_PCI_CONTROL
> > > (0x88) and REG_RESET_CONTROL (0x834) registers available via the
> > > clock
> > > module.
> > > Introduce flags field in the mtk_gen3_pcie_pdata struct in order
> > > to
> > > specify per-SoC capabilities.
> > > 
> > > Tested-by: Hui Ma <[e-mail]>
> > > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > > ---
> > > Changes in v2:
> > > - introduce flags field in mtk_gen3_pcie_flags struct instead of
> > > adding
> > >   reset callback
> > > - fix the leftover case in mtk_pcie_suspend_noirq routine
> > > - add more comments
> > > - Link to v1: 
> > > 
https://lore.kernel.org/r/[e-mail]
> > > ---
> > >  drivers/pci/controller/pcie-mediatek-gen3.c | 59
> > > ++++++++++++++++++++---------
> > >  1 file changed, 41 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > index
> > > 66ce4b5d309bb6d64618c70ac5e0a529e0910511..8e4704ff3509867fc0ff799
> > > e9fb
> > > 99e71e46756cd 100644
> > > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > @@ -125,10 +125,18 @@
> > > 
> > >  struct mtk_gen3_pcie;
> > > 
> > > +enum mtk_gen3_pcie_flags {
> > > +       SKIP_PCIE_RSTB  = BIT(0), /* skip PCIE_RSTB signals
> > > configuration
> > > +                                  * during device probing or
> > > suspend/resume
> > > +                                  * phase in order to avoid hw
> > > bugs/issues.
> > > +                                  */
> > > +};
> > > +
> > >  /**
> > >   * struct mtk_gen3_pcie_pdata - differentiate between host
> > > generations
> > >   * @power_up: pcie power_up callback
> > >   * @phy_resets: phy reset lines SoC data.
> > > + * @flags: pcie device flags.
> > >   */
> > >  struct mtk_gen3_pcie_pdata {
> > >         int (*power_up)(struct mtk_gen3_pcie *pcie);
> > > @@ -136,6 +144,7 @@ struct mtk_gen3_pcie_pdata {
> > >                 const char *id[MAX_NUM_PHY_RESETS];
> > >                 int num_resets;
> > >         } phy_resets;
> > > +       u32 flags;
> > >  };
> > > 
> > >  /**
> > > @@ -402,22 +411,33 @@ static int mtk_pcie_startup_port(struct
> > > mtk_gen3_pcie *pcie)
> > >         val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> > >         writel_relaxed(val, pcie->base + PCIE_MISC_CTRL_REG);
> > > 
> > > -       /* Assert all reset signals */
> > > -       val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> > > -       val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
> > > PCIE_PE_RSTB;
> > > -       writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > > -
> > >         /*
> > > -        * Described in PCIe CEM specification sections 2.2
> > > (PERST#
> > > Signal)
> > > -        * and 2.2.1 (Initial Power-Up (G3 to S0)).
> > > -        * The deassertion of PERST# should be delayed 100ms
> > > (TPVPERL)
> > > -        * for the power and clock to become stable.
> > > +        * Airoha EN7581 has a hw bug asserting/releasing
> > > PCIE_PE_RSTB signal
> > > +        * causing occasional PCIe link down. In order to
> > > overcome
> > > the issue,
> > > +        * PCIE_RSTB signals are not asserted/released at this
> > > stage
> > > and the
> > > +        * PCIe block is reset using REG_PCI_CONTROL (0x88) and
> > > +        * REG_RESET_CONTROL (0x834) registers available via the
> > > clock module.
> > >          */
> > > -       msleep(100);
> > > -
> > > -       /* De-assert reset signals */
> > > -       val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
> > > PCIE_PE_RSTB);
> > > -       writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > 
> > What will happen if the EN7581 use this reset flow? Will it still
> > work
> > after this link down?
> 
> Hi Jianjun Wang,
> 
> This has been described here by Hui Ma:
> 
https://lore.kernel.org/r/[e-mail]
> 
> Setting PCIE_PE_RSTB bit on EN7581 SoC during reset triggers
> occasional PCIe link
> down issues caused by a hw problem.

Hi Lorenzo,

I'm wondering if we can ignore the previous reset and take this one as
the initial reset?

Thanks.

> 
> Regards,
> Lorenzo
> 
> > 
> > > +       if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
> > > +               /* Assert all reset signals */
> > > +               val = readl_relaxed(pcie->base +
> > > PCIE_RST_CTRL_REG);
> > > +               val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB |
> > > PCIE_BRG_RSTB
> > > > 
> > > 
> > > +                      PCIE_PE_RSTB;
> > > +               writel_relaxed(val, pcie->base +
> > > PCIE_RST_CTRL_REG);
> > > +
> > > +               /*
> > > +                * Described in PCIe CEM specification sections
> > > 2.2
> > > (PERST# Signal)
> > > +                * and 2.2.1 (Initial Power-Up (G3 to S0)).
> > > +                * The deassertion of PERST# should be delayed
> > > 100ms
> > > (TPVPERL)
> > > +                * for the power and clock to become stable.
> > > +                */
> > > +               msleep(PCIE_T_PVPERL_MS);
> > > +
> > > +               /* De-assert reset signals */
> > > +               val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB |
> > > PCIE_BRG_RSTB |
> > > +                        PCIE_PE_RSTB);
> > > +               writel_relaxed(val, pcie->base +
> > > PCIE_RST_CTRL_REG);
> > > +       }
> > > 
> > >         /* Check if the link is up or not */
> > >         err = readl_poll_timeout(pcie->base +
> > > PCIE_LINK_STATUS_REG,
> > > val,
> > > @@ -1160,10 +1180,12 @@ static int mtk_pcie_suspend_noirq(struct
> > > device *dev)
> > >                 return err;
> > >         }
> > > 
> > > -       /* Pull down the PERST# pin */
> > > -       val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> > > -       val |= PCIE_PE_RSTB;
> > > -       writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > > +       if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
> > > +               /* Pull down the PERST# pin */
> > > +               val = readl_relaxed(pcie->base +
> > > PCIE_RST_CTRL_REG);
> > > +               val |= PCIE_PE_RSTB;
> > > +               writel_relaxed(val, pcie->base +
> > > PCIE_RST_CTRL_REG);
> > > +       }
> > > 
> > >         dev_dbg(pcie->dev, "entered L2 states successfully");
> > > 
> > > @@ -1214,6 +1236,7 @@ static const struct mtk_gen3_pcie_pdata
> > > mtk_pcie_soc_en7581 = {
> > >                 .id[2] = "phy-lane2",
> > >                 .num_resets = 3,
> > >         },
> > > +       .flags = SKIP_PCIE_RSTB,
> > >  };
> > > 
> > >  static const struct of_device_id mtk_pcie_of_match[] = {
> > > 
> > > ---
> > > base-commit: 3102ce10f3111e4c3b8fb233dc93f29e220adaf7
> > > change-id: 20240920-pcie-en7581-rst-fix-8161658c13c4
> > > 
> > > Best regards,
> > > --
> > > Lorenzo Bianconi <[e-mail]>
> > > 
> > >

Lorenzo Bianconi Nov. 6, 2024, 9 a.m. UTC | #8

> On Tue, 2024-11-05 at 10:30 +0100, [e-mail] wrote:
> > > On Mon, 2024-11-04 at 23:00 +0100, Lorenzo Bianconi wrote:
> > > > External email : Please do not click links or open attachments
> > > > until
> > > > you have verified the sender or the content.
> > > > 
> > > > 
> > > > Airoha EN7581 has a hw bug asserting/releasing PCIE_PE_RSTB
> > > > signal
> > > > causing occasional PCIe link down issues. In order to overcome
> > > > the
> > > > problem, PCIE_RSTB signals are not asserted/released during
> > > > device
> > > > probe or
> > > > suspend/resume phase and the PCIe block is reset using
> > > > REG_PCI_CONTROL
> > > > (0x88) and REG_RESET_CONTROL (0x834) registers available via the
> > > > clock
> > > > module.
> > > > Introduce flags field in the mtk_gen3_pcie_pdata struct in order
> > > > to
> > > > specify per-SoC capabilities.
> > > > 
> > > > Tested-by: Hui Ma <[e-mail]>
> > > > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > > > ---
> > > > Changes in v2:
> > > > - introduce flags field in mtk_gen3_pcie_flags struct instead of
> > > > adding
> > > >   reset callback
> > > > - fix the leftover case in mtk_pcie_suspend_noirq routine
> > > > - add more comments
> > > > - Link to v1: 
> > > > 
> https://lore.kernel.org/r/[e-mail]
> > > > ---
> > > >  drivers/pci/controller/pcie-mediatek-gen3.c | 59
> > > > ++++++++++++++++++++---------
> > > >  1 file changed, 41 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > index
> > > > 66ce4b5d309bb6d64618c70ac5e0a529e0910511..8e4704ff3509867fc0ff799
> > > > e9fb
> > > > 99e71e46756cd 100644
> > > > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > @@ -125,10 +125,18 @@
> > > > 
> > > >  struct mtk_gen3_pcie;
> > > > 
> > > > +enum mtk_gen3_pcie_flags {
> > > > +       SKIP_PCIE_RSTB  = BIT(0), /* skip PCIE_RSTB signals
> > > > configuration
> > > > +                                  * during device probing or
> > > > suspend/resume
> > > > +                                  * phase in order to avoid hw
> > > > bugs/issues.
> > > > +                                  */
> > > > +};
> > > > +
> > > >  /**
> > > >   * struct mtk_gen3_pcie_pdata - differentiate between host
> > > > generations
> > > >   * @power_up: pcie power_up callback
> > > >   * @phy_resets: phy reset lines SoC data.
> > > > + * @flags: pcie device flags.
> > > >   */
> > > >  struct mtk_gen3_pcie_pdata {
> > > >         int (*power_up)(struct mtk_gen3_pcie *pcie);
> > > > @@ -136,6 +144,7 @@ struct mtk_gen3_pcie_pdata {
> > > >                 const char *id[MAX_NUM_PHY_RESETS];
> > > >                 int num_resets;
> > > >         } phy_resets;
> > > > +       u32 flags;
> > > >  };
> > > > 
> > > >  /**
> > > > @@ -402,22 +411,33 @@ static int mtk_pcie_startup_port(struct
> > > > mtk_gen3_pcie *pcie)
> > > >         val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> > > >         writel_relaxed(val, pcie->base + PCIE_MISC_CTRL_REG);
> > > > 
> > > > -       /* Assert all reset signals */
> > > > -       val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> > > > -       val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
> > > > PCIE_PE_RSTB;
> > > > -       writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > > > -
> > > >         /*
> > > > -        * Described in PCIe CEM specification sections 2.2
> > > > (PERST#
> > > > Signal)
> > > > -        * and 2.2.1 (Initial Power-Up (G3 to S0)).
> > > > -        * The deassertion of PERST# should be delayed 100ms
> > > > (TPVPERL)
> > > > -        * for the power and clock to become stable.
> > > > +        * Airoha EN7581 has a hw bug asserting/releasing
> > > > PCIE_PE_RSTB signal
> > > > +        * causing occasional PCIe link down. In order to
> > > > overcome
> > > > the issue,
> > > > +        * PCIE_RSTB signals are not asserted/released at this
> > > > stage
> > > > and the
> > > > +        * PCIe block is reset using REG_PCI_CONTROL (0x88) and
> > > > +        * REG_RESET_CONTROL (0x834) registers available via the
> > > > clock module.
> > > >          */
> > > > -       msleep(100);
> > > > -
> > > > -       /* De-assert reset signals */
> > > > -       val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
> > > > PCIE_PE_RSTB);
> > > > -       writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > > 
> > > What will happen if the EN7581 use this reset flow? Will it still
> > > work
> > > after this link down?
> > 
> > Hi Jianjun Wang,
> > 
> > This has been described here by Hui Ma:
> > 
> https://lore.kernel.org/r/[e-mail]
> > 
> > Setting PCIE_PE_RSTB bit on EN7581 SoC during reset triggers
> > occasional PCIe link
> > down issues caused by a hw problem.
> 
> Hi Lorenzo,
> 
> I'm wondering if we can ignore the previous reset and take this one as
> the initial reset?

Hi Jianjun Wang,

according to my understanding from Hui Ma's description, EN7581 has a hw issue
with PCIE_PE_RSTB and it can't rely on it.

Regards,
Lorenzo

> 
> Thanks.
> 
> > 
> > Regards,
> > Lorenzo
> > 
> > > 
> > > > +       if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
> > > > +               /* Assert all reset signals */
> > > > +               val = readl_relaxed(pcie->base +
> > > > PCIE_RST_CTRL_REG);
> > > > +               val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB |
> > > > PCIE_BRG_RSTB
> > > > > 
> > > > 
> > > > +                      PCIE_PE_RSTB;
> > > > +               writel_relaxed(val, pcie->base +
> > > > PCIE_RST_CTRL_REG);
> > > > +
> > > > +               /*
> > > > +                * Described in PCIe CEM specification sections
> > > > 2.2
> > > > (PERST# Signal)
> > > > +                * and 2.2.1 (Initial Power-Up (G3 to S0)).
> > > > +                * The deassertion of PERST# should be delayed
> > > > 100ms
> > > > (TPVPERL)
> > > > +                * for the power and clock to become stable.
> > > > +                */
> > > > +               msleep(PCIE_T_PVPERL_MS);
> > > > +
> > > > +               /* De-assert reset signals */
> > > > +               val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB |
> > > > PCIE_BRG_RSTB |
> > > > +                        PCIE_PE_RSTB);
> > > > +               writel_relaxed(val, pcie->base +
> > > > PCIE_RST_CTRL_REG);
> > > > +       }
> > > > 
> > > >         /* Check if the link is up or not */
> > > >         err = readl_poll_timeout(pcie->base +
> > > > PCIE_LINK_STATUS_REG,
> > > > val,
> > > > @@ -1160,10 +1180,12 @@ static int mtk_pcie_suspend_noirq(struct
> > > > device *dev)
> > > >                 return err;
> > > >         }
> > > > 
> > > > -       /* Pull down the PERST# pin */
> > > > -       val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> > > > -       val |= PCIE_PE_RSTB;
> > > > -       writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > > > +       if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
> > > > +               /* Pull down the PERST# pin */
> > > > +               val = readl_relaxed(pcie->base +
> > > > PCIE_RST_CTRL_REG);
> > > > +               val |= PCIE_PE_RSTB;
> > > > +               writel_relaxed(val, pcie->base +
> > > > PCIE_RST_CTRL_REG);
> > > > +       }
> > > > 
> > > >         dev_dbg(pcie->dev, "entered L2 states successfully");
> > > > 
> > > > @@ -1214,6 +1236,7 @@ static const struct mtk_gen3_pcie_pdata
> > > > mtk_pcie_soc_en7581 = {
> > > >                 .id[2] = "phy-lane2",
> > > >                 .num_resets = 3,
> > > >         },
> > > > +       .flags = SKIP_PCIE_RSTB,
> > > >  };
> > > > 
> > > >  static const struct of_device_id mtk_pcie_of_match[] = {
> > > > 
> > > > ---
> > > > base-commit: 3102ce10f3111e4c3b8fb233dc93f29e220adaf7
> > > > change-id: 20240920-pcie-en7581-rst-fix-8161658c13c4
> > > > 
> > > > Best regards,
> > > > --
> > > > Lorenzo Bianconi <[e-mail]>
> > > > 
> > > >

Lorenzo Bianconi Nov. 6, 2024, 9:18 a.m. UTC | #9

> On Tue, Nov 05, 2024 at 07:13:52PM +0100, Lorenzo Bianconi wrote:
> > > On Mon, Nov 04, 2024 at 11:00:05PM +0100, Lorenzo Bianconi wrote:
> > > > Airoha EN7581 has a hw bug asserting/releasing PCIE_PE_RSTB signal
> > > > causing occasional PCIe link down issues. In order to overcome the
> > > > problem, PCIE_RSTB signals are not asserted/released during device probe or
> > > > suspend/resume phase and the PCIe block is reset using REG_PCI_CONTROL
> > > > (0x88) and REG_RESET_CONTROL (0x834) registers available via the clock
> > > > module.
> > > > Introduce flags field in the mtk_gen3_pcie_pdata struct in order to
> > > > specify per-SoC capabilities.
> > > 
> > > Where does this alternate way of doing reset (using REG_PCI_CONTROL
> > > and REG_RESET_CONTROL) happen?  Why isn't there something in this
> > > patch to use that alternate method at the same points where
> > > PCIE_PE_RSTB is used?
> > 
> > REG_RESET_CONTROL (0x834) is already asserted/released in the following flow:
> > 
> > mtk_pcie_en7581_power_up() -> reset_control_bulk_deassert() -> en7523_reset_update()
> > https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L470
> > 
> > REG_PCI_CONTROL (0x88) is already asserted/released in the following flow:
> > mtk_pcie_en7581_power_up() -> clk_bulk_enable() -> en7581_pci_enable()
> > https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L385
> 
> So IIUC, you're saying that on EN7581, the PCI hierarchy is reset by
> the soc->power_up() callback, mtk_pcie_en7581_power_up(), via
> REG_PCI_CONTROL and REG_RESET_CONTROL.

yes, correct.

> 
> I assume the hierarchy is also reset by the non-EN7581 .power_up()
> callback, mtk_pcie_power_up()?

as pointed out by Jianjun Wang, non-EN7581 family is reset via PCIE_RSTB
signals and .power_up() callback is used just for initialization.
For EN7581 family we need to reset the device via .power_up() callback
since we have a hw issue with PCIE_PE_RSTB signal (at least this is my
take-away :))

> 
> And prior to this patch, we reset the hierarchy *again* in
> mtk_pcie_startup_port() via PCIE_RST_CTRL_REG, but this causes
> occasional "link down" issues because of a EN7581 hardware defect.

yes, correct

> 
> So for EN7581, this patch skips the PCIE_RST_CTRL_REG reset in
> mtk_pcie_startup_port().

yes, correct

> 
> .power_up() and mtk_pcie_startup_port() are used both at probe time
> and in mtk_pcie_resume_noirq().  So after this patch, I assume:
> 
>   - EN7581 resets the hierarchy once at probe and resume instead of
>     twice.

yes, correct

> 
>   - Non-EN7581 resets the hierarchy twice at probe and resume.

nope, just once since .power_up() does not reset the device.

Regards,
Lorenzo

> 
> I assume I'm missing something (maybe mtk_pcie_power_up() doesn't
> actually reset the hierarchy?) because I don't see why we would reset
> the hierarchy twice for either controller.
> 
> Bjorn

Patch

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 66ce4b5d309bb6d64618c70ac5e0a529e0910511..8e4704ff3509867fc0ff799e9fb99e71e46756cd 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -125,10 +125,18 @@ 
 
 struct mtk_gen3_pcie;
 
+enum mtk_gen3_pcie_flags {
+	SKIP_PCIE_RSTB	= BIT(0), /* skip PCIE_RSTB signals configuration
+				   * during device probing or suspend/resume
+				   * phase in order to avoid hw bugs/issues.
+				   */
+};
+
 /**
  * struct mtk_gen3_pcie_pdata - differentiate between host generations
  * @power_up: pcie power_up callback
  * @phy_resets: phy reset lines SoC data.
+ * @flags: pcie device flags.
  */
 struct mtk_gen3_pcie_pdata {
 	int (*power_up)(struct mtk_gen3_pcie *pcie);
@@ -136,6 +144,7 @@  struct mtk_gen3_pcie_pdata {
 		const char *id[MAX_NUM_PHY_RESETS];
 		int num_resets;
 	} phy_resets;
+	u32 flags;
 };
 
 /**
@@ -402,22 +411,33 @@  static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
 	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
 	writel_relaxed(val, pcie->base + PCIE_MISC_CTRL_REG);
 
-	/* Assert all reset signals */
-	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
-	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
-	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
-
 	/*
-	 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
-	 * and 2.2.1 (Initial Power-Up (G3 to S0)).
-	 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
-	 * for the power and clock to become stable.
+	 * Airoha EN7581 has a hw bug asserting/releasing PCIE_PE_RSTB signal
+	 * causing occasional PCIe link down. In order to overcome the issue,
+	 * PCIE_RSTB signals are not asserted/released at this stage and the
+	 * PCIe block is reset using REG_PCI_CONTROL (0x88) and
+	 * REG_RESET_CONTROL (0x834) registers available via the clock module.
 	 */
-	msleep(100);
-
-	/* De-assert reset signals */
-	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
-	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
+	if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
+		/* Assert all reset signals */
+		val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
+		val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
+		       PCIE_PE_RSTB;
+		writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
+
+		/*
+		 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
+		 * and 2.2.1 (Initial Power-Up (G3 to S0)).
+		 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
+		 * for the power and clock to become stable.
+		 */
+		msleep(PCIE_T_PVPERL_MS);
+
+		/* De-assert reset signals */
+		val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
+			 PCIE_PE_RSTB);
+		writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
+	}
 
 	/* Check if the link is up or not */
 	err = readl_poll_timeout(pcie->base + PCIE_LINK_STATUS_REG, val,
@@ -1160,10 +1180,12 @@  static int mtk_pcie_suspend_noirq(struct device *dev)
 		return err;
 	}
 
-	/* Pull down the PERST# pin */
-	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
-	val |= PCIE_PE_RSTB;
-	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
+	if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
+		/* Pull down the PERST# pin */
+		val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
+		val |= PCIE_PE_RSTB;
+		writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
+	}
 
 	dev_dbg(pcie->dev, "entered L2 states successfully");
 
@@ -1214,6 +1236,7 @@  static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_en7581 = {
 		.id[2] = "phy-lane2",
 		.num_resets = 3,
 	},
+	.flags = SKIP_PCIE_RSTB,
 };
 
 static const struct of_device_id mtk_pcie_of_match[] = {