On Thu, May 16, 2019 at 11:23:06AM +0530, Manikanta Maddireddy wrote:
> Add support for GPIO based PERST# signal. GPIO number comes from per port
> PCIe device tree node.
>
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@xxxxxxxxxx>
> ---
> V4: Using devm_gpiod_get_from_of_node() to get reset-gpios
>
> V3: Using helper function to get reset-gpios
>
> V2: Using standard "reset-gpio" property
>
> drivers/pci/controller/pci-tegra.c | 41 +++++++++++++++++++++++++-----
> 1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index 06b99fcbf382..09b4d384ba38 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -17,6 +17,7 @@
> #include <linux/debugfs.h>
> #include <linux/delay.h>
> #include <linux/export.h>
> +#include <linux/gpio.h>
> #include <linux/interrupt.h>
> #include <linux/iopoll.h>
> #include <linux/irq.h>
> @@ -400,6 +401,8 @@ struct tegra_pcie_port {
> unsigned int lanes;
>
> struct phy **phys;
> +
> + struct gpio_desc *reset_gpiod;
Nit: I'd leave away the "d" at the end there. Or perhaps even the _gpio
suffix entirely. But it's fine either way.
> };
>
> struct tegra_pcie_bus {
> @@ -583,15 +586,23 @@ static void tegra_pcie_port_reset(struct tegra_pcie_port *port)
> unsigned long value;
>
> /* pulse reset signal */
> - value = afi_readl(port->pcie, ctrl);
> - value &= ~AFI_PEX_CTRL_RST;
> - afi_writel(port->pcie, value, ctrl);
> + if (port->reset_gpiod) {
> + gpiod_set_value(port->reset_gpiod, 0);
So is this actually deasserting the reset pin, or is it asserting a
low-active reset? I think it's the latter, because ...
> + } else {
> + value = afi_readl(port->pcie, ctrl);
> + value &= ~AFI_PEX_CTRL_RST;
> + afi_writel(port->pcie, value, ctrl);
> + }
>
> usleep_range(1000, 2000);
>
> - value = afi_readl(port->pcie, ctrl);
> - value |= AFI_PEX_CTRL_RST;
> - afi_writel(port->pcie, value, ctrl);
> + if (port->reset_gpiod) {
> + gpiod_set_value(port->reset_gpiod, 1);
After this the port should be functional, right? I think it'd be better
to reverse the logic here and move the polarity of the GPIO into device
tree. gpiod_set_value() takes care of inverting the level internally if
the GPIO is marked as low-active in DT.
The end result is obviously the same, but it makes the usage much
clearer. If somebody want to write a DT for their board, they will look
at the schematics and see a low-active reset line and may be tempted to
describe it as such in DT, but with your current code that would be
exactly the wrong way around.
> + } else {
> + value = afi_readl(port->pcie, ctrl);
> + value |= AFI_PEX_CTRL_RST;
> + afi_writel(port->pcie, value, ctrl);
> + }
> }
>
> static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
> @@ -2238,6 +2249,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
> struct tegra_pcie_port *rp;
> unsigned int index;
> u32 value;
> + char *label;
>
> err = of_pci_get_devfn(port);
> if (err < 0) {
> @@ -2296,6 +2308,23 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
> if (IS_ERR(rp->base))
> return PTR_ERR(rp->base);
>
> + label = kasprintf(GFP_KERNEL, "pex-reset-%u", index);
devm_kasprintf()?
Thierry
> + if (!label) {
> + dev_err(dev, "failed to create reset GPIO label\n");
> + return -ENOMEM;
> + }
> +
> + rp->reset_gpiod = devm_gpiod_get_from_of_node(dev, port,
> + "reset-gpios", 0,
> + GPIOD_OUT_LOW,
> + label);
> + kfree(label);
> + if (IS_ERR(rp->reset_gpiod)) {
> + err = PTR_ERR(rp->reset_gpiod);
> + dev_err(dev, "failed to get reset GPIO: %d\n", err);
> + return err;
> + }
> +
> list_add_tail(&rp->list, &pcie->ports);
> }
>
> --
> 2.17.1
>
Attachment:
signature.asc
Description: PGP signature
