From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f52.google.com ([74.125.82.52]:39551 "EHLO mail-wg0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750755Ab3LLFKf (ORCPT ); Thu, 12 Dec 2013 00:10:35 -0500 Received: by mail-wg0-f52.google.com with SMTP id x13so7464062wgg.19 for ; Wed, 11 Dec 2013 21:10:33 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1386757818-5154-1-git-send-email-marex@denx.de> References: <1386757818-5154-1-git-send-email-marex@denx.de> Date: Wed, 11 Dec 2013 21:10:31 -0800 Message-ID: Subject: Re: [PATCH 1/7] PCI: imx6: Make reset-gpio optional From: Tim Harvey To: Marek Vasut Cc: "linux-pci@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Bjorn Helgaas , Frank Li , Harro Haan , Jingoo Han , Mohit KUMAR , Pratyush Anand , Richard Zhu , Sascha Hauer , Sean Cross , Shawn Guo , Siva Reddy Kallam , Srikanth T Shivanand , Troy Kisky , Yinghai Lu Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-pci-owner@vger.kernel.org List-ID: On Wed, Dec 11, 2013 at 2:30 AM, Marek Vasut wrote: > > Some boards do not have a PCIe reset GPIO. To avoid probe > failure on these boards, make the reset GPIO optional as > well. > > Signed-off-by: Marek Vasut > Cc: Bjorn Helgaas > Cc: Frank Li > Cc: Harro Haan > Cc: Jingoo Han > Cc: Mohit KUMAR > Cc: Pratyush Anand > Cc: Richard Zhu > Cc: Sascha Hauer > Cc: Sean Cross > Cc: Shawn Guo > Cc: Siva Reddy Kallam > Cc: Srikanth T Shivanand > Cc: Tim Harvey > Cc: Troy Kisky > Cc: Yinghai Lu > --- > .../devicetree/bindings/pci/designware-pcie.txt | 2 +- > drivers/pci/host/pci-imx6.c | 29 +++++++++++----------- > 2 files changed, 16 insertions(+), 15 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt > index d5d26d4..b7a2279 100644 > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt > @@ -19,9 +19,9 @@ Required properties: > to define the mapping of the PCIe interface to interrupt > numbers. > - num-lanes: number of lanes to use > -- reset-gpio: gpio pin number of power good signal > > Optional properties for fsl,imx6q-pcie > +- reset-gpio: gpio pin number of power good signal > - power-on-gpio: gpio pin number of power-enable signal > - wake-up-gpio: gpio pin number of incoming wakeup signal > - disable-gpio: gpio pin number of outgoing rfkill/endpoint disable signal > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index bd70af8..52027ad 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -214,9 +214,12 @@ static int imx6_pcie_assert_core_reset(struct pcie_port *pp) > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, > IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16); > > - gpio_set_value(imx6_pcie->reset_gpio, 0); > - msleep(100); > - gpio_set_value(imx6_pcie->reset_gpio, 1); > + /* Some boards don't have PCIe reset GPIO. */ > + if (gpio_is_valid(imx6_pcie->reset_gpio)) { > + gpio_set_value(imx6_pcie->reset_gpio, 0); > + msleep(100); > + gpio_set_value(imx6_pcie->reset_gpio, 1); > + } > > return 0; > } Marek, Though not the fault of your patch, I noticed while looking at this that the PCI Express specification is not being properly met with regards to PERST# and the reference clock. The spec states that PERST# must be kept asserted until after the reference clock is stable (I'm not entirely clear how long of a delay is needed for the clock to become stable but I think the value is typically the 100ms). I see in the current pci-imx6.c code that imx6_pcie_host_init calls imx6_pcie_assert_core_reset first, then imx6_pcie_init_phy, followed by imx6_pcie_deassert_core_reset. Despite the function names, imx6_pcie_assert_core_reset as shown above asserts then de-asserts PERST# before the clock is enabled in imx6_pcie_deassert_core_reset. This seems to me to be a violation of the spec and I believe the msleep(100) and de-assertion of the option reset_gpio should be done in imx6_pcie_deassert_core reset after the clock is brought up. If you agree with my assessment, would you mind resolving this issue at the same time? If not, I'm happy to follow-up with a patch to resolve it after your patch is accepted. Thanks, Tim > @@ -439,17 +442,15 @@ static int __init imx6_pcie_probe(struct platform_device *pdev) > > /* Fetch GPIOs */ > imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0); > - if (!gpio_is_valid(imx6_pcie->reset_gpio)) { > - dev_err(&pdev->dev, "no reset-gpio defined\n"); > - ret = -ENODEV; > - } > - ret = devm_gpio_request_one(&pdev->dev, > - imx6_pcie->reset_gpio, > - GPIOF_OUT_INIT_LOW, > - "PCIe reset"); > - if (ret) { > - dev_err(&pdev->dev, "unable to get reset gpio\n"); > - goto err; > + if (gpio_is_valid(imx6_pcie->reset_gpio)) { > + ret = devm_gpio_request_one(&pdev->dev, > + imx6_pcie->reset_gpio, > + GPIOF_OUT_INIT_LOW, > + "PCIe reset"); > + if (ret) { > + dev_err(&pdev->dev, "unable to get reset gpio\n"); > + goto err; > + } > } > > imx6_pcie->power_on_gpio = of_get_named_gpio(np, "power-on-gpio", 0); > -- > 1.8.4.3 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: tharvey@gateworks.com (Tim Harvey) Date: Wed, 11 Dec 2013 21:10:31 -0800 Subject: [PATCH 1/7] PCI: imx6: Make reset-gpio optional In-Reply-To: <1386757818-5154-1-git-send-email-marex@denx.de> References: <1386757818-5154-1-git-send-email-marex@denx.de> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Dec 11, 2013 at 2:30 AM, Marek Vasut wrote: > > Some boards do not have a PCIe reset GPIO. To avoid probe > failure on these boards, make the reset GPIO optional as > well. > > Signed-off-by: Marek Vasut > Cc: Bjorn Helgaas > Cc: Frank Li > Cc: Harro Haan > Cc: Jingoo Han > Cc: Mohit KUMAR > Cc: Pratyush Anand > Cc: Richard Zhu > Cc: Sascha Hauer > Cc: Sean Cross > Cc: Shawn Guo > Cc: Siva Reddy Kallam > Cc: Srikanth T Shivanand > Cc: Tim Harvey > Cc: Troy Kisky > Cc: Yinghai Lu > --- > .../devicetree/bindings/pci/designware-pcie.txt | 2 +- > drivers/pci/host/pci-imx6.c | 29 +++++++++++----------- > 2 files changed, 16 insertions(+), 15 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt > index d5d26d4..b7a2279 100644 > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt > @@ -19,9 +19,9 @@ Required properties: > to define the mapping of the PCIe interface to interrupt > numbers. > - num-lanes: number of lanes to use > -- reset-gpio: gpio pin number of power good signal > > Optional properties for fsl,imx6q-pcie > +- reset-gpio: gpio pin number of power good signal > - power-on-gpio: gpio pin number of power-enable signal > - wake-up-gpio: gpio pin number of incoming wakeup signal > - disable-gpio: gpio pin number of outgoing rfkill/endpoint disable signal > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index bd70af8..52027ad 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -214,9 +214,12 @@ static int imx6_pcie_assert_core_reset(struct pcie_port *pp) > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, > IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16); > > - gpio_set_value(imx6_pcie->reset_gpio, 0); > - msleep(100); > - gpio_set_value(imx6_pcie->reset_gpio, 1); > + /* Some boards don't have PCIe reset GPIO. */ > + if (gpio_is_valid(imx6_pcie->reset_gpio)) { > + gpio_set_value(imx6_pcie->reset_gpio, 0); > + msleep(100); > + gpio_set_value(imx6_pcie->reset_gpio, 1); > + } > > return 0; > } Marek, Though not the fault of your patch, I noticed while looking at this that the PCI Express specification is not being properly met with regards to PERST# and the reference clock. The spec states that PERST# must be kept asserted until after the reference clock is stable (I'm not entirely clear how long of a delay is needed for the clock to become stable but I think the value is typically the 100ms). I see in the current pci-imx6.c code that imx6_pcie_host_init calls imx6_pcie_assert_core_reset first, then imx6_pcie_init_phy, followed by imx6_pcie_deassert_core_reset. Despite the function names, imx6_pcie_assert_core_reset as shown above asserts then de-asserts PERST# before the clock is enabled in imx6_pcie_deassert_core_reset. This seems to me to be a violation of the spec and I believe the msleep(100) and de-assertion of the option reset_gpio should be done in imx6_pcie_deassert_core reset after the clock is brought up. If you agree with my assessment, would you mind resolving this issue at the same time? If not, I'm happy to follow-up with a patch to resolve it after your patch is accepted. Thanks, Tim > @@ -439,17 +442,15 @@ static int __init imx6_pcie_probe(struct platform_device *pdev) > > /* Fetch GPIOs */ > imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0); > - if (!gpio_is_valid(imx6_pcie->reset_gpio)) { > - dev_err(&pdev->dev, "no reset-gpio defined\n"); > - ret = -ENODEV; > - } > - ret = devm_gpio_request_one(&pdev->dev, > - imx6_pcie->reset_gpio, > - GPIOF_OUT_INIT_LOW, > - "PCIe reset"); > - if (ret) { > - dev_err(&pdev->dev, "unable to get reset gpio\n"); > - goto err; > + if (gpio_is_valid(imx6_pcie->reset_gpio)) { > + ret = devm_gpio_request_one(&pdev->dev, > + imx6_pcie->reset_gpio, > + GPIOF_OUT_INIT_LOW, > + "PCIe reset"); > + if (ret) { > + dev_err(&pdev->dev, "unable to get reset gpio\n"); > + goto err; > + } > } > > imx6_pcie->power_on_gpio = of_get_named_gpio(np, "power-on-gpio", 0); > -- > 1.8.4.3 >