On Tue, Sep 23, 2014 at 5:28 AM, Tim Harvey <tharvey@xxxxxxxxxxxxx> wrote:
> On Tue, Sep 23, 2014 at 2:56 AM, Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote:
>> 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.
>
> I agree with Lucas' comments and also agree that this can use some
> testing. Based on my previous findings PCI link is very fragile. It
> will take me a few days to get a proper test setup in a thermal
> chamber with a host of boards but I will report back when I have
> findings.
>
> Tim
>
>>
>>> Signed-off-by: Richard Zhu <r65037@xxxxxxxxxxxxx>
>>> ---
>>> 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);
>>
I tested this across temperature over 300+ boots each on several IMX6
based boards with switches and did not encounter any link failures.
Tested-by: Tim Harvey <tharvey@xxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
