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

[Frank Li]

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 ...

> 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.

> +
> +	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?

...
>  		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".

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
>