| Message ID | 1376396724-32048-5-git-send-email-sebastian.hesselbarth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Dear Sebastian Hesselbarth, On Tue, 13 Aug 2013 14:25:23 +0200, Sebastian Hesselbarth wrote: > + port->reset_gpio = of_get_named_gpio_flags(child, > + "reset-gpios", 0, &flags); > + if (gpio_is_valid(port->reset_gpio)) { > + u32 reset_udelay = 20000; > + > + port->reset_active_low = flags & OF_GPIO_ACTIVE_LOW; > + port->reset_name = kasprintf(GFP_KERNEL, > + "pcie%d.%d-reset", port->port, port->lane); > + of_property_read_u32(child, "reset-delay-us", > + &reset_udelay); > + > + ret = devm_gpio_request_one(&pdev->dev, > + port->reset_gpio, GPIOF_DIR_OUT, port->reset_name); > + if (ret) { > + if (ret == -EPROBE_DEFER) > + return ret; > + continue; > + } > + > + gpio_set_value(port->reset_gpio, > + (port->reset_active_low) ? 1 : 0); > + udelay(reset_udelay); > + } Sorry for raising this only now, but I think I would have preferred to see this reset-gpios handling be moved into a separate sub-function. The loop initializing each PCIe interface is already quite large, and I believe moving this reset-gpios thing to a sub-function would have made sense. But well, the patches have been applied, and we can always adjust this with a followup patch. Jason, have you re-created your for-next branch with all those patches? I'd like to give them a test if possible. Thanks! Thomas
On Tue, Aug 13, 2013 at 10:29:43PM +0200, Thomas Petazzoni wrote: > Dear Sebastian Hesselbarth, > > On Tue, 13 Aug 2013 14:25:23 +0200, Sebastian Hesselbarth wrote: > > > + port->reset_gpio = of_get_named_gpio_flags(child, > > + "reset-gpios", 0, &flags); > > + if (gpio_is_valid(port->reset_gpio)) { > > + u32 reset_udelay = 20000; > > + > > + port->reset_active_low = flags & OF_GPIO_ACTIVE_LOW; > > + port->reset_name = kasprintf(GFP_KERNEL, > > + "pcie%d.%d-reset", port->port, port->lane); > > + of_property_read_u32(child, "reset-delay-us", > > + &reset_udelay); > > + > > + ret = devm_gpio_request_one(&pdev->dev, > > + port->reset_gpio, GPIOF_DIR_OUT, port->reset_name); > > + if (ret) { > > + if (ret == -EPROBE_DEFER) > > + return ret; > > + continue; > > + } > > + > > + gpio_set_value(port->reset_gpio, > > + (port->reset_active_low) ? 1 : 0); > > + udelay(reset_udelay); > > + } > > Sorry for raising this only now, but I think I would have preferred to > see this reset-gpios handling be moved into a separate sub-function. > The loop initializing each PCIe interface is already quite large, and I > believe moving this reset-gpios thing to a sub-function would have made > sense. > > But well, the patches have been applied, and we can always adjust this > with a followup patch. The branch this is in will be the last PR, so if the patch is reworked by tomorrow, I'll just replace it. > Jason, have you re-created your for-next branch with all those patches? > I'd like to give them a test if possible. Yep. Give it a go. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 13, 2013 at 02:25:23PM +0200, Sebastian Hesselbarth wrote: [...] > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c [...] > @@ -897,6 +904,30 @@ static int mvebu_pcie_probe(struct platform_device *pdev) [...] > + u32 reset_udelay = 20000; [...] > + udelay(reset_udelay); udelay(20000) is probably not a good idea. You should probably use something like usleep_range() or msleep() here. Also see: Documentation/timers/timers-howto.txt Thierry
On 08/14/2013 11:07 AM, Thierry Reding wrote: > On Tue, Aug 13, 2013 at 02:25:23PM +0200, Sebastian Hesselbarth wrote: > [...] >> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c > [...] >> @@ -897,6 +904,30 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > [...] >> + u32 reset_udelay = 20000; > [...] >> + udelay(reset_udelay); > > udelay(20000) is probably not a good idea. You should probably use > something like usleep_range() or msleep() here. Also see: > > Documentation/timers/timers-howto.txt Thierry, thanks for mentioning this. I clearly got distracted from ms to us switch due to the different reset API convention. As the delay will be most likely 10ms+, we should change the above to msleep(reset_udelay/1000) instead. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 14, 2013 at 11:25:02AM +0200, Sebastian Hesselbarth wrote: > On 08/14/2013 11:07 AM, Thierry Reding wrote: > >On Tue, Aug 13, 2013 at 02:25:23PM +0200, Sebastian Hesselbarth wrote: > >[...] > >>diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c > >[...] > >>@@ -897,6 +904,30 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > >[...] > >>+ u32 reset_udelay = 20000; > >[...] > >>+ udelay(reset_udelay); > > > >udelay(20000) is probably not a good idea. You should probably use > >something like usleep_range() or msleep() here. Also see: > > > > Documentation/timers/timers-howto.txt > > Thierry, > > thanks for mentioning this. I clearly got distracted from ms to us > switch due to the different reset API convention. As the delay will > be most likely 10ms+, we should change the above to > msleep(reset_udelay/1000) instead. I'll just go ahead and fix this up locally. No need to resend. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/pci/mvebu-pci.txt b/Documentation/devicetree/bindings/pci/mvebu-pci.txt index 638673a..8bb3245 100644 --- a/Documentation/devicetree/bindings/pci/mvebu-pci.txt +++ b/Documentation/devicetree/bindings/pci/mvebu-pci.txt @@ -76,6 +76,8 @@ and the following optional properties: - marvell,pcie-lane: the physical PCIe lane number, for ports having multiple lanes. If this property is not found, we assume that the value is 0. +- reset-gpios: optional gpio to PERST# +- reset-delay-us: delay in us to wait after reset de-assertion Example: @@ -138,6 +140,10 @@ pcie-controller { interrupt-map = <0 0 0 0 &mpic 58>; marvell,pcie-port = <0>; marvell,pcie-lane = <0>; + /* low-active PERST# reset on GPIO 25 */ + reset-gpios = <&gpio0 25 1>; + /* wait 20ms for device settle after reset deassertion */ + reset-delay-us = <20000>; clocks = <&gateclk 5>; status = "disabled"; }; diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c index bd7092a..a9ad4b3 100644 --- a/drivers/pci/host/pci-mvebu.c +++ b/drivers/pci/host/pci-mvebu.c @@ -9,14 +9,17 @@ #include <linux/kernel.h> #include <linux/pci.h> #include <linux/clk.h> +#include <linux/delay.h> +#include <linux/gpio.h> #include <linux/module.h> #include <linux/mbus.h> #include <linux/msi.h> #include <linux/slab.h> #include <linux/platform_device.h> #include <linux/of_address.h> -#include <linux/of_pci.h> #include <linux/of_irq.h> +#include <linux/of_gpio.h> +#include <linux/of_pci.h> #include <linux/of_platform.h> /* @@ -126,6 +129,9 @@ struct mvebu_pcie_port { unsigned int io_target; unsigned int io_attr; struct clk *clk; + int reset_gpio; + int reset_active_low; + char *reset_name; struct mvebu_sw_pci_bridge bridge; struct device_node *dn; struct mvebu_pcie *pcie; @@ -857,6 +863,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev) i = 0; for_each_child_of_node(pdev->dev.of_node, child) { struct mvebu_pcie_port *port = &pcie->ports[i]; + enum of_gpio_flags flags; if (!of_device_is_available(child)) continue; @@ -897,6 +904,30 @@ static int mvebu_pcie_probe(struct platform_device *pdev) continue; } + port->reset_gpio = of_get_named_gpio_flags(child, + "reset-gpios", 0, &flags); + if (gpio_is_valid(port->reset_gpio)) { + u32 reset_udelay = 20000; + + port->reset_active_low = flags & OF_GPIO_ACTIVE_LOW; + port->reset_name = kasprintf(GFP_KERNEL, + "pcie%d.%d-reset", port->port, port->lane); + of_property_read_u32(child, "reset-delay-us", + &reset_udelay); + + ret = devm_gpio_request_one(&pdev->dev, + port->reset_gpio, GPIOF_DIR_OUT, port->reset_name); + if (ret) { + if (ret == -EPROBE_DEFER) + return ret; + continue; + } + + gpio_set_value(port->reset_gpio, + (port->reset_active_low) ? 1 : 0); + udelay(reset_udelay); + } + port->clk = of_clk_get_by_name(child, NULL); if (IS_ERR(port->clk)) { dev_err(&pdev->dev, "PCIe%d.%d: cannot get clock\n",
This patch adds a check for DT passed reset-gpios property and deasserts/ asserts reset pin on probe/remove with configurable delay. Corresponding binding documentation is also updated. Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> --- Changelog: v1->v2: - use reset API compatible bindings (Reported by Kumar Gala and Thierry Reding) - add gpio properties to DT examples (Reported by Kumar Gala) - only fail (return) on EPROBE_DEFER and skip port parsing otherwise - use us delay instead of ms delay Cc: Russell King <linux@arm.linux.org.uk> Cc: Jason Cooper <jason@lakedaemon.net> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Cc: Thierry Reding <thierry.reding@gmail.com> Cc: Stephen Warren <swarren@wwwdotorg.org> Cc: devicetree@vger.kernel.org Cc: linux-doc@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-pci@vger.kernel.org --- .../devicetree/bindings/pci/mvebu-pci.txt | 6 ++++ drivers/pci/host/pci-mvebu.c | 33 +++++++++++++++++++- 2 files changed, 38 insertions(+), 1 deletion(-)