Re: [PATCH v1] PCI: mediatek-gen3: Align PERST# sequence with PCIe CEM specification

[Jian Yang (杨戬)]

Hi Mani,

On Wed, 2026-04-01 at 17:49 +0530, Manivannan Sadhasivam wrote:
> On Wed, Apr 01, 2026 at 11:16:42AM +0800, Jian Yang wrote:
> > Fix the control sequence of PERST# during system bootup/shutdown to
> > meet the requirement from PCIe CEM specification. There are two
> > major
> > changes in this patch:
> > 
> 
> You are fixing multiple issues in one patch, which is not acceptable.
> 
> > 1. Some of MediaTek's chip will stop generating REFCLK if the
> > PCIE_PHY_RSTB signal of PCIe controller is asserted. We have to
> > adjust the startup sequence as follows to ensure that PERST# will
> > be
> > de-asserted after the REFCLK is stable:
> > Assert PHY reset and PERST# -> delay 10ms -> De-assert PHY reset ->
> > delay 100ms -> De-assert PERST#
> > 
> 
> This issue is separate and not related to the below two issues.
> 
> > 2. Add 'shutdown' callback to control the timing of PERST# and
> > power
> > during the system shutdown phase, ensuring that PERST# is active
> > before the power on connector is removed.
> > 
> 
> Adding 'shutdown()' callback should belong to a separate patch with
> its own
> motivation.

Thanks for your comments. I will send a new patch series.

> 
> > Signed-off-by: Jian Yang <jian.yang@xxxxxxxxxxxx>
> > ---
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 39
> > +++++++++++++++++++--
> >  1 file changed, 36 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index a94fdbaf47fe..66d177918565 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -62,6 +62,7 @@
> >  #define PCIE_PHY_RSTB			BIT(1)
> >  #define PCIE_BRG_RSTB			BIT(2)
> >  #define PCIE_PE_RSTB			BIT(3)
> > +#define PCIE_BRG_RST_RDY_MS		10
> >  
> >  #define PCIE_LTSSM_STATUS_REG		0x150
> >  #define PCIE_LTSSM_STATE_MASK		GENMASK(28, 24)
> > @@ -133,6 +134,7 @@
> >  #define MAX_NUM_PHY_RESETS		3
> >  
> >  #define PCIE_MTK_RESET_TIME_US		10
> > +#define PCIE_MTK_PDN_PERST_TIME_MS	5
> >  
> >  /* Time in ms needed to complete PCIe reset on EN7581 SoC */
> >  #define PCIE_EN7581_RESET_TIME_MS	100
> > @@ -430,6 +432,21 @@ static int mtk_pcie_devices_power_up(struct
> > mtk_gen3_pcie *pcie)
> >  		return err;
> >  	}
> >  
> > +	/*
> > +	 * Some of MediaTek's chips won't output REFCLK when
> > PCIE_PHY_RSTB is
> > +	 * asserted, we have to de-assert MAC & PHY & BRG reset signals
> > first
> > +	 * to allow the REFCLK to be stable. While PCIE_BRG_RSTB is
> > asserted,
> > +	 * there is a short period during which the PCIe internal
> > register
> > +	 * cannot be accessed, so we need to wait 10ms here.
> > +	 */
> > +	msleep(PCIE_BRG_RST_RDY_MS);
> > +
> > +	if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
> > +		/* De-assert MAC, PHY and BRG reset signals */
> > +		val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB |
> > PCIE_BRG_RSTB);
> > +		writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > +	}
> > +
> >  	/*
> >  	 * Described in PCIe CEM specification revision 6.0.
> >  	 *
> > @@ -439,9 +456,8 @@ static int mtk_pcie_devices_power_up(struct
> > mtk_gen3_pcie *pcie)
> >  	msleep(PCIE_T_PVPERL_MS);
> >  
> >  	if (!(pcie->soc->flags & SKIP_PCIE_RSTB)) {
> > -		/* De-assert reset signals */
> > -		val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB
> > |
> > -			 PCIE_PE_RSTB);
> > +		/* De-assert PERST# signal */
> > +		val &= ~PCIE_PE_RSTB;
> >  		writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> >  	}
> >  
> > @@ -459,6 +475,14 @@ static void mtk_pcie_devices_power_down(struct
> > mtk_gen3_pcie *pcie)
> >  		writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> >  	}
> >  
> > +	/*
> > +	 * Described in PCIe CEM specification revision 6.0.
> > +	 *
> > +	 * The PERST# gose active before the power on the connector is
> > removed.
> > +	 * Wait a while to ensure the voltage transition of PERST# is
> > completed.
> > +	 */
> > +	msleep(PCIE_MTK_PDN_PERST_TIME_MS);
> 
> PERST# is asserted by setting the 'PCIE_PE_RSTB' bit. So if that
> controls the
> actual voltage transition of PERST# signal, will reading it back
> ensure that the
> transition has been completed without waiting for an arbitrary delay?

You're right. I believe this delay is unnecessary. I will remove it in
the next version.

> 
> Nevertheless, this also belongs to a separate patch.
> 
> - Mani
> 

Best regards,
Jian Yang