RE: [PATCH V10 04/13] PCI: imx6: Assert PERST# before enabling regulators

[Sherry Sun]

> On Thu, Apr 02, 2026 at 05:50:58PM +0800, Sherry Sun wrote:
> > According to the PCIe initialization requirements, PERST# signal
> > should be asserted before applying power to the PCIe device, and
> > deasserted after power and reference clock are stable.
> >
> 
> Spec wording is not quite like this. Spec mandates asserting PERST# *before*
> stopping refclk and powering down the device and deasserting it *after*
> applying power and refclk stable.
> 
> I believe you want to assert PERST# before enabling regulator to prevent the
> endpoint from functioning? If so, is it due to refclk not available yet or some
> other reason?

You are right. My commit message wording was not that precise.
The PCIe endpoint may start responding or driving signals as
soon as its supply is enabled, even before the reference clock is stable.
Asserting PERST# before enabling the regulator ensures that the endpoint
remains in reset throughout the entire power-up sequence, until both
power and refclk are known to be stable and link initialization can safely
begin. This is mainly to avoid undefined behavior during early power-up.

I will update the commit message to better reflect this.

> 
> > Currently, the driver enables the vpcie3v3aux regulator in
> > imx_pcie_probe() before PERST# is asserted in imx_pcie_host_init(),
> > which violates the PCIe power sequencing requirements. However, there
> > is no issue so far because PERST# is requested as GPIOD_OUT_HIGH in
> > imx_pcie_probe(), which guarantees that PERST# is asserted before
> > enabling the vpcie3v3aux regulator.
> >
> > This is prepare for the upcoming changes that will parse the reset
> > property using the new Root Port binding, which will use GPIOD_ASIS
> > when requesting the reset GPIO. With GPIOD_ASIS, the GPIO state is not
> > guaranteed, so explicit sequencing is required.
> >
> > Fix the power sequencing by:
> > 1. Moving vpcie3v3aux regulator enable from probe to
> >    imx_pcie_host_init(), where it can be properly sequenced with PERST#.
> > 2. Moving imx_pcie_assert_perst() before regulator and clock enable to
> >    ensure correct ordering.
> >
> > The vpcie3v3aux regulator is kept enabled for the entire PCIe
> > controller lifecycle and automatically disabled on device removal via devm
> cleanup.
> >
> 
> vpcie3v3aux handling should be in a separate patch.

Actually the handling of vpcie3v3aux itself remains unchanged, I just adjust the
sequence of regulator/clock enable and perst#.
Previously, the imx driver enabled the vpcie3v3aux regulator in imx_pcie_probe()
before PERST# is asserted in imx_pcie_host_init(), which violates the PCIe power
sequencing requirements.
This patch moves vpcie3v3aux regulator enable from probe to  imx_pcie_host_init(),
where it can be properly sequenced with PERST#.
Perhaps I should just remove this description to avoid confusion.

Best Regards
Sherry
> 
> - Mani
> 
> > Signed-off-by: Sherry Sun <sherry.sun@xxxxxxx>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 49
> > +++++++++++++++++++++------
> >  1 file changed, 39 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 45d70ae7e04f..948ffb75d122 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -166,6 +166,8 @@ struct imx_pcie {
> >  	u32			tx_swing_full;
> >  	u32			tx_swing_low;
> >  	struct regulator	*vpcie;
> > +	struct regulator	*vpcie_aux;
> > +	bool			vpcie_aux_enabled;
> >  	struct regulator	*vph;
> >  	void __iomem		*phy_base;
> >
> > @@ -1220,6 +1222,13 @@ static void imx_pcie_disable_device(struct
> pci_host_bridge *bridge,
> >  	imx_pcie_remove_lut(imx_pcie, pci_dev_id(pdev));  }
> >
> > +static void imx_pcie_vpcie_aux_disable(void *data) {
> > +	struct regulator *vpcie_aux = data;
> > +
> > +	regulator_disable(vpcie_aux);
> > +}
> > +
> >  static void imx_pcie_assert_perst(struct imx_pcie *imx_pcie, bool
> > assert)  {
> >  	if (assert) {
> > @@ -1240,6 +1249,24 @@ static int imx_pcie_host_init(struct dw_pcie_rp
> *pp)
> >  	struct imx_pcie *imx_pcie = to_imx_pcie(pci);
> >  	int ret;
> >
> > +	imx_pcie_assert_perst(imx_pcie, true);
> > +
> > +	/* Keep 3.3Vaux supply enabled for the entire PCIe controller lifecycle
> */
> > +	if (imx_pcie->vpcie_aux && !imx_pcie->vpcie_aux_enabled) {
> > +		ret = regulator_enable(imx_pcie->vpcie_aux);
> > +		if (ret) {
> > +			dev_err(dev, "failed to enable vpcie_aux
> regulator: %d\n",
> > +				ret);
> > +			return ret;
> > +		}
> > +		imx_pcie->vpcie_aux_enabled = true;
> > +
> > +		ret = devm_add_action_or_reset(dev,
> imx_pcie_vpcie_aux_disable,
> > +					       imx_pcie->vpcie_aux);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	if (imx_pcie->vpcie) {
> >  		ret = regulator_enable(imx_pcie->vpcie);
> >  		if (ret) {
> > @@ -1249,25 +1276,24 @@ static int imx_pcie_host_init(struct dw_pcie_rp
> *pp)
> >  		}
> >  	}
> >
> > +	ret = imx_pcie_clk_enable(imx_pcie);
> > +	if (ret) {
> > +		dev_err(dev, "unable to enable pcie clocks: %d\n", ret);
> > +		goto err_reg_disable;
> > +	}
> > +
> >  	if (pp->bridge && imx_check_flag(imx_pcie,
> IMX_PCIE_FLAG_HAS_LUT)) {
> >  		pp->bridge->enable_device = imx_pcie_enable_device;
> >  		pp->bridge->disable_device = imx_pcie_disable_device;
> >  	}
> >
> >  	imx_pcie_assert_core_reset(imx_pcie);
> > -	imx_pcie_assert_perst(imx_pcie, true);
> >
> >  	if (imx_pcie->drvdata->init_phy)
> >  		imx_pcie->drvdata->init_phy(imx_pcie);
> >
> >  	imx_pcie_configure_type(imx_pcie);
> >
> > -	ret = imx_pcie_clk_enable(imx_pcie);
> > -	if (ret) {
> > -		dev_err(dev, "unable to enable pcie clocks: %d\n", ret);
> > -		goto err_reg_disable;
> > -	}
> > -
> >  	if (imx_pcie->phy) {
> >  		ret = phy_init(imx_pcie->phy);
> >  		if (ret) {
> > @@ -1780,9 +1806,12 @@ static int imx_pcie_probe(struct platform_device
> *pdev)
> >  	of_property_read_u32(node, "fsl,max-link-speed", &pci-
> >max_link_speed);
> >  	imx_pcie->supports_clkreq = of_property_read_bool(node,
> > "supports-clkreq");
> >
> > -	ret = devm_regulator_get_enable_optional(&pdev->dev,
> "vpcie3v3aux");
> > -	if (ret < 0 && ret != -ENODEV)
> > -		return dev_err_probe(dev, ret, "failed to enable Vaux
> supply\n");
> > +	imx_pcie->vpcie_aux = devm_regulator_get_optional(&pdev->dev,
> "vpcie3v3aux");
> > +	if (IS_ERR(imx_pcie->vpcie_aux)) {
> > +		if (PTR_ERR(imx_pcie->vpcie_aux) != -ENODEV)
> > +			return PTR_ERR(imx_pcie->vpcie_aux);
> > +		imx_pcie->vpcie_aux = NULL;
> > +	}
> >
> >  	imx_pcie->vpcie = devm_regulator_get_optional(&pdev->dev,
> "vpcie");
> >  	if (IS_ERR(imx_pcie->vpcie)) {
> > --
> > 2.37.1
> >
> 
> --
> மணிவண்ணன் சதாசிவம்