Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <936cc917828bfa3c089a33f0af219fecdc2a778a.camel@pengutronix.de>
Subject: Re: [RFC 1/2] ARM: dts: imx6qp-sabresd: Enable pcie support
From:   Lucas Stach <l.stach@pengutronix.de>
To:     Richard Zhu <hongxing.zhu@nxp.com>, bhelgaas@google.com,
        lorenzo.pieralisi@arm.com, shawnguo@kernel.org
Cc:     linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, kernel@pengutronix.de,
        linux-imx@nxp.com
Date:   Wed, 09 Feb 2022 10:05:16 +0100
In-Reply-To: <1644390156-5940-1-git-send-email-hongxing.zhu@nxp.com>
References: <1644390156-5940-1-git-send-email-hongxing.zhu@nxp.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.40.4 (3.40.4-1.fc34) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Richard,

Am Mittwoch, dem 09.02.2022 um 15:02 +0800 schrieb Richard Zhu:
> In the i.MX6QP sabresd board(sch-28857) design, one external oscillator
> is used as the PCIe reference clock source by the endpoint device.
> 
> If RC uses this oscillator as reference clock too, PLL6(ENET PLL) would
> has to be in bypass mode, and ENET clocks would be messed up.
> 
> To keep things simple, let RC use the internal PLL as reference clock
> and always enable the external oscillator for endpoint device on
> i.MX6QP sabresd board.
> 
The commit message doesn't really match what's being done in the patch.
Maybe you meant to say that even though the HW design is different you
are enabling the PCIe controller in the same way as on the 6Q sabresd?

Also, is this configuration stable for you? We've had some issues with
this kind of split clocking setup in a customer design, where it was
enabled by accident, due to PLL6 no being bypassed. In this design it
caused the link to randomly drop under load and causing aborts on the
CPU side, due to completion timeouts. I think it at least warrants a
comment somewhere that this isn't a recommended setup.

Regards,
Lucas

> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  arch/arm/boot/dts/imx6qp-sabresd.dts | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/imx6qp-sabresd.dts b/arch/arm/boot/dts/imx6qp-sabresd.dts
> index 480e73183f6b..083cf90bcab5 100644
> --- a/arch/arm/boot/dts/imx6qp-sabresd.dts
> +++ b/arch/arm/boot/dts/imx6qp-sabresd.dts
> @@ -50,8 +50,14 @@ MX6QDL_PAD_SD3_DAT7__SD3_DATA7		0x17059
>  	};
>  };
>  
> +&vgen3_reg {
> +	regulator-min-microvolt = <1800000>;
> +	regulator-max-microvolt = <3300000>;
> +	regulator-always-on;
> +};
> +
>  &pcie {
> -	status = "disabled";
> +	status = "okay";
>  };
>  
>  &sata {