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

["lorenzo@kernel.org" <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 8254 bytes --]

> On Tue, 2024-11-05 at 10:30 +0100, lorenzo@kernel.org 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 <hui.ma@airoha.com>
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > > 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/20240920-pcie-en7581-rst-fix-v1-1-1043fb63ffc9@kernel.org
> > > > ---
> > > >  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/20240920-pcie-en7581-rst-fix-v1-1-1043fb63ffc9@kernel.org
> > 
> > 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 <lorenzo@kernel.org>
> > > > 
> > > > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]