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

[Sherry Sun]

> On Tue, Mar 10, 2026 at 09:54:17AM +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.
> >
> > 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 a preparation patch for the upcoming changes that will parse
> > the
> 
> Nit: This is prepare for ...

Ok, will fix it.

> 
> > 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.
> >
> > Signed-off-by: Sherry Sun <sherry.sun@xxxxxxx>
> > ---
> ...
> >  static void imx_pcie_assert_perst(struct imx_pcie *imx_pcie, bool
> > assert)  {
> >  	if (assert) {
> > @@ -1240,6 +1249,29 @@ static int imx_pcie_host_init(struct dw_pcie_rp
> *pp)
> >  	struct imx_pcie *imx_pcie = to_imx_pcie(pci);
> >  	int ret;
> >
> > +	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;
> > +	}
> 
> are you sure need move this part? it is not related preset and vaux.

Yes, from code logic, no need to move this, I do this just to makes the code
"asser perst -> enable regulartor -> enable clk -> deassert perst" look cleaner.
If moving irrelevant code is not recommended, I can remove this part of the change.

> 
> > +
> > +	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) {
> 
> How about two pcie shared one vaux regulator?

Currently, both vpcie_aux and vpcie are enabled in imx_pcie_host_init(), this function
is called not only during probe but also during system resume. Since we need to keep
the 3.3Vaux power enabled for the entire PCIe controller lifecycle, we hope vpcie_aux
to be enabled only once during probe, so this flag is added to prevent it from being
touched during system resume.

> 
> ...
> >  		if (ret) {
> > @@ -1790,9 +1816,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);
> 
> keep old dev_err_probe(), just message change to "failed to get Vaux supply".

This is done to keep the same code style with vpcie and vph regulator in current code.

    imx_pcie->vpcie = devm_regulator_get_optional(&pdev->dev, "vpcie");
    if (IS_ERR(imx_pcie->vpcie)) {
        if (PTR_ERR(imx_pcie->vpcie) != -ENODEV)
            return PTR_ERR(imx_pcie->vpcie);
        imx_pcie->vpcie = NULL;
    }

    imx_pcie->vph = devm_regulator_get_optional(&pdev->dev, "vph");
    if (IS_ERR(imx_pcie->vph)) {
        if (PTR_ERR(imx_pcie->vph) != -ENODEV)
            return PTR_ERR(imx_pcie->vph);
        imx_pcie->vph = NULL;
    }

Best Regards
Sherry
> 
> Frnak
> > +		imx_pcie->vpcie_aux = NULL;
> > +	}
> >
> >  	imx_pcie->vpcie = devm_regulator_get_optional(&pdev->dev,
> "vpcie");
> >  	if (IS_ERR(imx_pcie->vpcie)) {
> > --
> > 2.37.1
> >