Hi Lucas: Thanks for your comments. > -----Original Message----- > From: Lucas Stach [mailto:l.stach@xxxxxxxxxxxxxx] > Sent: Monday, September 22, 2014 8:07 PM > To: Zhu Richard-R65037 > Cc: linux-pci-owner@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Guo Shawn- > R65073; festevam@xxxxxxxxx > Subject: Re: [PATCH v1 1/4] PCI: imx6: enable pcie on imx6qdl sabreauto > > Am Montag, den 22.09.2014, 18:45 +0800 schrieb Richard Zhu: > > - enable pcie on imx6qdl sabreauto boards. > > - wait the clocks to stabilize after the pcie_ref_en > > (IMX6Q_GPR1_PCIE_REF_CLK_EN) is set. > > > > Those are two completely independent changes, even if you enable/fix a > specific board with those. So please split into DT changes (to be merged by > Shawn) and PCI changes (to be merged by Bjorn). > [Richard] Ok, no problem. Would be separated later. > > Signed-off-by: Richard Zhu <r65037@xxxxxxxxxxxxx> > > --- > > arch/arm/boot/dts/imx6qdl-sabreauto.dtsi | 4 ++++ > > drivers/pci/host/pci-imx6.c | 6 +++--- > > 2 files changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi > > b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi > > index 009abd6..d6040a5 100644 > > --- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi > > +++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi > > @@ -410,6 +410,10 @@ > > }; > > }; > > > > +&pcie { > > + status = "okay"; > > +}; > > + > > &pwm3 { > > pinctrl-names = "default"; > > pinctrl-0 = <&pinctrl_pwm3>; > > 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); > > Are you fixing a specific issue with this? Otherwise this change doesn't look > good to me. The reference manual states that the clock must be stable before > enabling the REF_CLK_EN bit in GPR1. So moving the delay without proof that it > doesn't have adverse effects and without a good justification seems wrong. > > If this fixes a specific issue please mention it in the commit message. > Also I would like at least a confirmation from Tim Harvey that this doesn't > regress his setup. So please take him on CC for the next version of this patch. [Richard] yes, it is used to fix one issue on imx6qdl ard boards. Actually, the 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. BTW, as I know that the clocks should be stable already after the "clk_prepare_enable" is return. Ok, I would CC Tim Harvey in the next version of this patch-set. Thanks. Best Regards Richard Zhu > > Regards, > Lucas > -- > Pengutronix e.K. | Lucas Stach | > Industrial Linux Solutions | http://www.pengutronix.de/ | ��.n��������+%������w��{.n�����{���"�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
