Re: [PATCH v2 2/5] PCI: imx6: wait the clocks to stabilize after ref_en

[Lucas Stach <l.stach@pengutronix.de>]

Am Dienstag, den 23.09.2014, 12:11 +0800 schrieb Richard Zhu:
> - a while delay is mandatory required after pcie_ref_clk_en
> is set. Otherwise, the system would be hang on imx6qdl ard
> boards, because that imx6qdl boards don't have the reset_gpio.
> - the clocks should be stable already after the
> "clk_prepare_enable" is return. So I think it's ok to move the
> usleep delay after the pcie_ref_en is set.
> 

You are describing a lot of the conditions around the issue, but not the
issue itself, which makes it hard to follow your commit message. After
looking at the code I think the problem is this (and should be described
accordingly):

For boards without a reset gpio we skip the delay between enabling the
pcie_ref_clk and touching the RC registers for configuration. Apparently
this hangs when the clocks are not yet settled in the DW PCIe core. So
we need to make sure that there is always an appropriate delay between
those two actions.

I have not found this constraint anywhere in the i.MX6 Reference Manual,
nor in the DW PCIe documents I have access to, which makes me a bit feel
a bit unhappy about this. Richard, do you have better info on why this
delay is needed and how long it needs to be? Or is this just empirical?

In general I'm ok with this patch, but still want a confirmation from
Tim that this doesn't break anything.

> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  drivers/pci/host/pci-imx6.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 233fe8a..bc4222b 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -275,15 +275,15 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
>  		goto err_pcie;
>  	}
>  
> -	/* allow the clocks to stabilize */
> -	usleep_range(200, 500);
> -
>  	/* power up core phy and enable ref clock */
>  	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
>  			IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18);
>  	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
>  			IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
>  
> +	/* allow the clocks to stabilize */
> +	usleep_range(200, 500);
> +
>  	/* Some boards don't have PCIe reset GPIO. */
>  	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
>  		gpio_set_value(imx6_pcie->reset_gpio, 0);

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |