[PATCH] imx6 PCI Host initialisation order

[PATCH] imx6 PCI Host initialisation order

[Raymond van der Rots]

Hi,

The imx6dl on our hardware board frequently had problems bringing up the PCI link, with or without peripherals connected. I found these issues to be due to the initialisation order of the PCI Host.

The host driver first enables the phy, and then enables its clocks. However, according to the reference manual (IMX6SDLRM, page 2033):
> The phy_ref_ssp_en signal must remain deasserted until the reference clock is running at the appropriate frequency, at which point phy_ref_ssp_en can be asserted.


Which implies that the clocks should be brought up first, after which the peripheral should be enabled.
This patch changes that initialisation order.

I do not have other hardware with an imx6dl, so this patch has only been tested on our board. Could someone confirm that this is more technically correct or improves behaviour?

Signed-off-by: Raymond van der Rots <raymond@electrocompaniet.no>
---
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index a568efa..17c35b4 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -228,11 +228,6 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
 	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
 	int ret;
 
-	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);
-
 	ret = clk_prepare_enable(imx6_pcie->pcie_phy);
 	if (ret) {
 		dev_err(pp->dev, "unable to enable pcie_phy clock\n");
@@ -254,6 +249,11 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
 	/* allow the clocks to stabilize */
 	usleep_range(200, 500);
 
+	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);
+
 	/* Some boards don't have PCIe reset GPIO. */
 	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
 		gpio_set_value(imx6_pcie->reset_gpio, 0);

Re: [PATCH] imx6 PCI Host initialisation order

[Shawn Guo]

Hi Raymond,

It seems that there is already a similar patch [1] from Tim floating
on the list.

Shawn

[1] http://www.spinics.net/lists/linux-pci/msg33520.html

On Mon, Sep 08, 2014 at 11:10:37AM +0200, Raymond van der Rots wrote:
> Hi,
> 
> The imx6dl on our hardware board frequently had problems bringing up the PCI link, with or without peripherals connected. I found these issues to be due to the initialisation order of the PCI Host.
> 
> The host driver first enables the phy, and then enables its clocks. However, according to the reference manual (IMX6SDLRM, page 2033):
> > The phy_ref_ssp_en signal must remain deasserted until the reference clock is running at the appropriate frequency, at which point phy_ref_ssp_en can be asserted.
> 
> 
> Which implies that the clocks should be brought up first, after which the peripheral should be enabled.
> This patch changes that initialisation order.
> 
> I do not have other hardware with an imx6dl, so this patch has only been tested on our board. Could someone confirm that this is more technically correct or improves behaviour?
> 
> Signed-off-by: Raymond van der Rots <raymond@electrocompaniet.no>
> ---
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index a568efa..17c35b4 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -228,11 +228,6 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
>  	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
>  	int ret;
>  
> -	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);
> -
>  	ret = clk_prepare_enable(imx6_pcie->pcie_phy);
>  	if (ret) {
>  		dev_err(pp->dev, "unable to enable pcie_phy clock\n");
> @@ -254,6 +249,11 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
>  	/* allow the clocks to stabilize */
>  	usleep_range(200, 500);
>  
> +	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);
> +
>  	/* Some boards don't have PCIe reset GPIO. */
>  	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
>  		gpio_set_value(imx6_pcie->reset_gpio, 0);
> 
> 

Re: [PATCH] imx6 PCI Host initialisation order

[Bjorn Helgaas]

On Mon, Sep 8, 2014 at 5:28 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> Hi Raymond,
>
> It seems that there is already a similar patch [1] from Tim floating
> on the list.
>
> Shawn
>
> [1] http://www.spinics.net/lists/linux-pci/msg33520.html

Yes.  MAINTAINERS lists Richard Zhu <r65037@freescale.com> and Shawn
Guo <shawn.guo@freescale.com> (hi Shawn :)) as the caretakers for
pci-imx6.c.  Neither has acked it yet, and Richard asked for a comment
update, which I haven't seen yet.  So I haven't applied it yet.

> On Mon, Sep 08, 2014 at 11:10:37AM +0200, Raymond van der Rots wrote:
>> Hi,
>>
>> The imx6dl on our hardware board frequently had problems bringing up the PCI link, with or without peripherals connected. I found these issues to be due to the initialisation order of the PCI Host.
>>
>> The host driver first enables the phy, and then enables its clocks. However, according to the reference manual (IMX6SDLRM, page 2033):
>> > The phy_ref_ssp_en signal must remain deasserted until the reference clock is running at the appropriate frequency, at which point phy_ref_ssp_en can be asserted.
>>
>>
>> Which implies that the clocks should be brought up first, after which the peripheral should be enabled.
>> This patch changes that initialisation order.
>>
>> I do not have other hardware with an imx6dl, so this patch has only been tested on our board. Could someone confirm that this is more technically correct or improves behaviour?
>>
>> Signed-off-by: Raymond van der Rots <raymond@electrocompaniet.no>
>> ---
>> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
>> index a568efa..17c35b4 100644
>> --- a/drivers/pci/host/pci-imx6.c
>> +++ b/drivers/pci/host/pci-imx6.c
>> @@ -228,11 +228,6 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
>>       struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
>>       int ret;
>>
>> -     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);
>> -
>>       ret = clk_prepare_enable(imx6_pcie->pcie_phy);
>>       if (ret) {
>>               dev_err(pp->dev, "unable to enable pcie_phy clock\n");
>> @@ -254,6 +249,11 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
>>       /* allow the clocks to stabilize */
>>       usleep_range(200, 500);
>>
>> +     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);
>> +
>>       /* Some boards don't have PCIe reset GPIO. */
>>       if (gpio_is_valid(imx6_pcie->reset_gpio)) {
>>               gpio_set_value(imx6_pcie->reset_gpio, 0);
>>
>>

RE: [PATCH] imx6 PCI Host initialisation order

[Hong-Xing.Zhu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3631 bytes --]


> -----Original Message-----
> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> Sent: Tuesday, September 09, 2014 12:51 AM
> To: Guo Shawn-R65073
> Cc: Raymond van der Rots; Zhu Richard-R65037; linux-kernel@vger.kernel.org;
> Lucas Stach; Tim Harvey
> Subject: Re: [PATCH] imx6 PCI Host initialisation order
> 
> On Mon, Sep 8, 2014 at 5:28 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> > Hi Raymond,
> >
> > It seems that there is already a similar patch [1] from Tim floating
> > on the list.
> >
> > Shawn
> >
> > [1] http://www.spinics.net/lists/linux-pci/msg33520.html
> 
> Yes.  MAINTAINERS lists Richard Zhu <r65037@freescale.com> and Shawn Guo
> <shawn.guo@freescale.com> (hi Shawn :)) as the caretakers for pci-imx6.c.
> Neither has acked it yet, and Richard asked for a comment update, which I
> haven't seen yet.  So I haven't applied it yet.
> 
[Richard] Yes it is. I recommended to add one comment used to describe why
the modification is required.

> > On Mon, Sep 08, 2014 at 11:10:37AM +0200, Raymond van der Rots wrote:
> >> Hi,
> >>
> >> The imx6dl on our hardware board frequently had problems bringing up the
> PCI link, with or without peripherals connected. I found these issues to be
> due to the initialisation order of the PCI Host.
> >>
> >> The host driver first enables the phy, and then enables its clocks. However,
> according to the reference manual (IMX6SDLRM, page 2033):
> >> > The phy_ref_ssp_en signal must remain deasserted until the reference
> clock is running at the appropriate frequency, at which point phy_ref_ssp_en
> can be asserted.
> >>
> >>
> >> Which implies that the clocks should be brought up first, after which the
> peripheral should be enabled.
> >> This patch changes that initialisation order.
> >>
> >> I do not have other hardware with an imx6dl, so this patch has only been
> tested on our board. Could someone confirm that this is more technically
> correct or improves behaviour?
> >>
> >> Signed-off-by: Raymond van der Rots <raymond@electrocompaniet.no>
> >> ---
> >> diff --git a/drivers/pci/host/pci-imx6.c
> >> b/drivers/pci/host/pci-imx6.c index a568efa..17c35b4 100644
> >> --- a/drivers/pci/host/pci-imx6.c
> >> +++ b/drivers/pci/host/pci-imx6.c
> >> @@ -228,11 +228,6 @@ static int imx6_pcie_deassert_core_reset(struct
> pcie_port *pp)
> >>       struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
> >>       int ret;
> >>
> >> -     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);
> >> -
> >>       ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> >>       if (ret) {
> >>               dev_err(pp->dev, "unable to enable pcie_phy clock\n");
> >> @@ -254,6 +249,11 @@ static int imx6_pcie_deassert_core_reset(struct
> pcie_port *pp)
> >>       /* allow the clocks to stabilize */
> >>       usleep_range(200, 500);
> >>
> >> +     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);
> >> +
> >>       /* Some boards don't have PCIe reset GPIO. */
> >>       if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> >>               gpio_set_value(imx6_pcie->reset_gpio, 0);
> >>
> >>

Best Regards
Richard Zhu

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥