Re: [PATCH V4 27/28] PCI: tegra: Add support for GPIO based PERST#

From: Thierry Reding <thierry.reding@gmail.com>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Manikanta Maddireddy <mmaddireddy@nvidia.com>,
	bhelgaas@google.com, robh+dt@kernel.org, mark.rutland@arm.com,
	jonathanh@nvidia.com, vidyas@nvidia.com,
	linux-tegra@vger.kernel.org, linux-pci@vger.kernel.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH V4 27/28] PCI: tegra: Add support for GPIO based PERST#
Date: Mon, 17 Jun 2019 13:26:46 +0200	[thread overview]
Message-ID: <20190617112646.GI508@ulmo> (raw)
In-Reply-To: <20190614165353.GB30511@e121166-lin.cambridge.arm.com>

[-- Attachment #1: Type: text/plain, Size: 3321 bytes --]

On Fri, Jun 14, 2019 at 05:53:53PM +0100, Lorenzo Pieralisi wrote:
> On Fri, Jun 14, 2019 at 10:00:49PM +0530, Manikanta Maddireddy wrote:
> 
> [...]
> 
> > GPIO based PERST# is per-platform requirement.
> > If DT prop is not present, then devm_gpiod_get_from_of_node() returns
> > NULL gpio_desc.
> > 
> > struct gpio_desc *gpiod_get_from_of_node(struct device_node *node,
> >                                          const char *propname, int index,
> >                                          enum gpiod_flags dflags,
> >                                          const char *label)
> > {
> >         struct gpio_desc *desc;
> >         unsigned long lflags = 0;
> >         enum of_gpio_flags flags;
> >         bool active_low = false;
> >         bool single_ended = false;
> >         bool open_drain = false;
> >         bool transitory = false;
> >         int ret;
> > 
> >         desc = of_get_named_gpiod_flags(node, propname,
> >                                         index, &flags);
> > 
> >         if (!desc || IS_ERR(desc)) {
> > */* If it is not there, just return NULL */****if (PTR_ERR(desc) == -ENOENT)****return NULL;*
> >                 return desc;
> >         }
> > 	...
> > 
> > }
> 
> Ok. My point then is that you have no way to enforce this requirement on
> platforms that actually need it, I do not even know if there is a
> way you can do it (I was thinking along the lines of using a
> compatible string to detect whether the GPIO #PERST reset is mandatory)
> but maybe this is not even a SOC property.

So this is definitely not an SoC property. From what Manikanta said, the
only reason why we need this is because on one particular design (that
we know of), the PCIe #PERST signal was connected to a pin that does not
originate from the PCIe controller. This happened by accident.

> Maybe what I am asking is overkill, I just wanted to understand.
> 
> I was just asking a question to understand how you handle the case
> where a GPIO pin definition is missing in DT for a platform that
> actually needs it, the driver will probe but nothing will work.

I think we should handle this the way we handle other GPIOs as well. If
some device requires a GPIO to be toggled to put it into or take it out
of reset, then that's something that needs to be described in DT. In
this case it just so happens that typically we don't need to worry about
it because the signals are properly connected and then the PCIe
controller will do the right with the reset. For the one design where
it doesn't work the reset GPIO is a workaround and it's board-specific
knowledge that the engineer writing the DT will have to know. I suspect
that if the same accident happened on another board, then as part of
writing the DT somebody would have noticed that we need an external pin
to be hooked up because the SFIO doesn't work.

> It would be good to describe this and capture it in the commit log.

Agreed. Let's be more verbose about the situation where this is required
and make it very clear that this is a workaround for a board design
mistake that shouldn't be necessary if best practices are followed.

Maybe add that to both the commit message and the device tree bindings
to make it difficult for people to miss.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]