Linux-PCI Archive on lore.kernel.org
 help / Atom feed
From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Thierry Reding <thierry.reding@gmail.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: Fri, 14 Jun 2019 16:59:34 +0100
Message-ID: <20190614155934.GA28253@e121166-lin.cambridge.arm.com> (raw)
In-Reply-To: <20190614152304.GK15526@ulmo>

On Fri, Jun 14, 2019 at 05:23:04PM +0200, Thierry Reding wrote:
> On Fri, Jun 14, 2019 at 03:50:23PM +0100, Lorenzo Pieralisi wrote:
> > On Fri, Jun 14, 2019 at 08:08:26PM +0530, Manikanta Maddireddy wrote:
> > > 
> > > 
> > > On 14-Jun-19 8:02 PM, Lorenzo Pieralisi wrote:
> > > > On Fri, Jun 14, 2019 at 04:07:35PM +0530, Manikanta Maddireddy wrote:
> > > >>
> > > >> On 13-Jun-19 8:54 PM, Lorenzo Pieralisi wrote:
> > > >>> On Tue, Jun 04, 2019 at 03:22:33PM +0200, Thierry Reding wrote:
> > > >>>
> > > >>> [...]
> > > >>>
> > > >>>>> +	} 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.
> > > >>> I agree with Thierry here, you should change the logic.
> > > >>>
> > > >>> Question: what's the advantage of adding GPIO reset support if that's
> > > >>> architected already in port registers ? I am pretty sure there is a
> > > >>> reason behind it (and forgive me the dumb question) and I would like to
> > > >>> have it written in the commit log.
> > > >>>
> > > >>> Thanks,
> > > >>> Lorenzo
> > > >> Each PCIe controller has a dedicated SFIO pin to support PERST#
> > > >> signal. Port register can control only this particular SFIO pin.
> > > >> However, in one of the Nvidia platform, instead of using PCIe SFIO
> > > >> pin, different gpio is routed PCIe slot. This happened because of a
> > > >> confusion in IO ball naming convention. To support this particular
> > > >> platform, driver has provide gpio support. I will update the commit
> > > >> log in V5.
> > > > What happens on that platform where you trigger reset through a port
> > > > register with :
> > > >
> > > > value = afi_readl(port->pcie, ctrl);
> > > > value |= AFI_PEX_CTRL_RST;
> > > > afi_writel(port->pcie, value, ctrl);
> > > >
> > > > (imagine the DT is not updated for instance or on current
> > > > mainline) ?
> > > >
> > > > Lorenzo
> > > 
> > > Lets take an example of PCIe controller-0, SFIO ball name which is
> > > controlled by the port-0 register is PEX_L0_RST. It will deassert
> > > PEX_L0_RST SFIO line but it doesn't go to PCIe slot, so fundamental
> > > reset(PERST# deassert) is not applied to the endpoint connected to
> > > that slot.
> > 
> > That's the point I am making, if the reset is not applied nothing
> > will work (provided PEX_L0_RST does not do any damage either).
> > 
> > For the platform in question you should make reset-gpios mandatory and
> > fail if not present (instead of toggling the wrong reset line) there is
> > no chance the driver can work without that property AFAICS.
> 
> I'm not sure I understand what you're proposing here. Are you suggesting
> that we put a check in the driver to see if we're running on a specific
> board and then fail if the reset-gpios are not there?

I am just trying to understand what this patch does. By reading it again
it looks like it makes GPIO PERST# reset mandatory for all platforms
supported by this driver (because if the driver does not grab an handle
to the GPIO tegra_pcie_parse_dt() fails), if I read the code correctly,
apologies if not.

Which makes me question the check:

	if (port->reset_gpiod) {
		gpiod_set_value(port->reset_gpiod, 0);

in tegra_pcie_port_reset(), if we are there port->reset_gpiod can't be
NULL or I am missing something and also make:

	} else {
		value = afi_readl(port->pcie, ctrl);
		value &= ~AFI_PEX_CTRL_RST;
		afi_writel(port->pcie, value, ctrl);
	}

path dead code.

Is this GPIO based #PERST a per-platform requirement or you want
to update the driver to always use GPIO based #PERST ?

And if it is a per-platform requirement I assume that a missing
DT property describing the GPIO #PERST should cause a probe failure,
not a fallback to port registers reset (which may have unintended
consequences).

From the commit log it is not clear what this patch does and for what
reason it does it but it should be, let's define it here and update the
log accordingly for everyone's benefit.

Lorenzo

  reply index

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-16  5:52 [PATCH V4 00/28] Enable Tegra PCIe root port features Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 01/28] soc/tegra: pmc: Export tegra_powergate_power_on() Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 02/28] PCI: tegra: Handle failure cases in tegra_pcie_power_on() Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 03/28] PCI: tegra: Rearrange Tegra PCIe driver functions Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 04/28] PCI: tegra: Mask AFI_INTR in runtime suspend Manikanta Maddireddy
2019-06-04 13:08   ` Thierry Reding
2019-05-16  5:52 ` [PATCH V4 05/28] PCI: tegra: Fix PCIe host power up sequence Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 06/28] PCI: tegra: Add PCIe Gen2 link speed support Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 07/28] PCI: tegra: Advertise PCIe Advanced Error Reporting (AER) capability Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 08/28] PCI: tegra: Program UPHY electrical settings for Tegra210 Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 09/28] PCI: tegra: Enable opportunistic UpdateFC and ACK Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 10/28] PCI: tegra: Disable AFI dynamic clock gating Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 11/28] PCI: tegra: Process pending DLL transactions before entering L1 or L2 Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 12/28] PCI: tegra: Enable PCIe xclk clock clamping Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 13/28] PCI: tegra: Increase the deskew retry time Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 14/28] PCI: tegra: Add SW fixup for RAW violations Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 15/28] PCI: tegra: Update flow control timer frequency in Tegra210 Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 16/28] PCI: tegra: Set target speed as Gen1 before starting LTSSM Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 17/28] PCI: tegra: Fix PLLE power down issue due to CLKREQ# signal Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 18/28] PCI: tegra: Program AFI_CACHE* registers only for Tegra20 Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 19/28] PCI: tegra: Change PRSNT_SENSE IRQ log to debug Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 20/28] PCI: tegra: Use legacy IRQ for port service drivers Manikanta Maddireddy
2019-05-20 20:37   ` Bjorn Helgaas
2019-05-21  9:07     ` Manikanta Maddireddy
2019-05-16  5:53 ` [PATCH V4 21/28] PCI: tegra: Add AFI_PEX2_CTRL reg offset as part of soc struct Manikanta Maddireddy
2019-05-16  5:53 ` [PATCH V4 22/28] PCI: tegra: Access endpoint config only if PCIe link is up Manikanta Maddireddy
2019-06-04 13:14   ` Thierry Reding
2019-06-04 14:10     ` Manikanta Maddireddy
2019-06-10  4:38       ` Manikanta Maddireddy
2019-06-13 14:39         ` Lorenzo Pieralisi
2019-06-13 15:42           ` Thierry Reding
2019-05-16  5:53 ` [PATCH V4 23/28] dt-bindings: pci: tegra: Document PCIe DPD pinctrl optional prop Manikanta Maddireddy
2019-05-16  5:53 ` [PATCH V4 24/28] arm64: tegra: Add PEX DPD states as pinctrl properties Manikanta Maddireddy
2019-05-16  5:53 ` [PATCH V4 25/28] PCI: tegra: Put PEX CLK & BIAS pads in DPD mode Manikanta Maddireddy
2019-05-16  5:53 ` [PATCH V4 26/28] PCI: Add DT binding for "reset-gpios" property Manikanta Maddireddy
2019-05-16  5:53 ` [PATCH V4 27/28] PCI: tegra: Add support for GPIO based PERST# Manikanta Maddireddy
2019-06-04 13:22   ` Thierry Reding
2019-06-13 15:24     ` Lorenzo Pieralisi
2019-06-14 10:37       ` Manikanta Maddireddy
2019-06-14 14:32         ` Lorenzo Pieralisi
2019-06-14 14:38           ` Manikanta Maddireddy
2019-06-14 14:50             ` Lorenzo Pieralisi
2019-06-14 14:56               ` Manikanta Maddireddy
2019-06-14 15:23               ` Thierry Reding
2019-06-14 15:59                 ` Lorenzo Pieralisi [this message]
2019-06-14 16:30                   ` Manikanta Maddireddy
2019-06-14 16:53                     ` Lorenzo Pieralisi
2019-06-14 17:23                       ` Manikanta Maddireddy
2019-05-16  5:53 ` [PATCH V4 28/28] PCI: tegra: Change link retry log level to debug Manikanta Maddireddy
2019-06-04 13:22   ` Thierry Reding
2019-05-16 13:12 ` [PATCH V4 00/28] Enable Tegra PCIe root port features Bjorn Helgaas
2019-05-17  8:38   ` Manikanta Maddireddy
2019-06-10  4:45 ` Manikanta Maddireddy
2019-06-10 17:33   ` Lorenzo Pieralisi

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190614155934.GA28253@e121166-lin.cambridge.arm.com \
    --to=lorenzo.pieralisi@arm.com \
    --cc=bhelgaas@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jonathanh@nvidia.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mmaddireddy@nvidia.com \
    --cc=robh+dt@kernel.org \
    --cc=thierry.reding@gmail.com \
    --cc=vidyas@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org linux-pci@archiver.kernel.org
	public-inbox-index linux-pci


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/ public-inbox