Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Johannes Berg <johannes@sipsolutions.net>
To: Thierry Reding <thierry.reding@gmail.com>,
	Manikanta Maddireddy <mmaddireddy@nvidia.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.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,
	linux-pm@vger.kernel.org, "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	linux-wireless@vger.kernel.org
Subject: Re: [PATCH V4 22/28] PCI: tegra: Access endpoint config only if PCIe link is up
Date: Tue, 18 Jun 2019 14:32:59 +0200
Message-ID: <9c0fb01f0dc6a193265297eaa100a35ff25413e7.camel@sipsolutions.net> (raw)
In-Reply-To: <20190618104918.GA28892@ulmo>

I got to this thread really late I guess :-)

On Tue, 2019-06-18 at 12:49 +0200, Thierry Reding wrote:

> > > > > > > > > 1. WiFi devices provides power-off feature for power saving
> > > > > > > > > in mobiles.  When WiFi is turned off we shouldn't power on
> > > > > > > > > the HW back without user turning it back on.

But why would you disconnect the PCIe device just to power it down?!

> > > > > > The problem that Manikanta is trying to solve here occurs in
> > > > > > this situation (Manikanta, correct me if I've got this wrong):
> > > > > > on some setups, a WiFi module connected over PCI will toggle a
> > > > > > power GPIO as part of runtime suspend. This effectively causes
> > > > > > the module to disappear from the PCI bus (i.e. it can no longer
> > > > > > be accessed until the power GPIO is toggled again).
> > > > > 
> > > > > GPIO is toggled as part of WiFi on/off, can be triggered from
> > > > > network manager UI.

That's kinda icky, IMHO.

> > > > > Correct, rfkill switch should handle the GPIO.
> > > > > Sequence will be,
> > > > >  - WiFi ON
> > > > >    - rfkill switch enables the WiFi GPIO
> > > > >    - Tegra PCIe receives hot plug event
> > > > >    - Tegra PCIe hot plug driver rescans PCI bus and enumerates the device
> > > > >    - PCI client driver is probed, which will create network interface
> > > > >  - WiFi OFF
> > > > >    - rfkill switch disables the WiFi GPIO
> > > > >    - Tegra PCIe receives hot unplug event
> > > > >    - Tegra PCIe hot plug driver removes PCI devices under the bus
> > > > >    - PCI client driver remove is executed, which will remove
> > > > >      network interface
> > > > > We don't need current patch in this case because PCI device is not
> > > > > present in the PCI hierarchy, so there cannot be EP config access
> > > > > with link down.  However Tegra doesn't support hot plug and unplug
> > > > > events. I am not sure if we have any software based hot plug event
> > > > > trigger.

Looks reasonable to me.

I guess if you absolutely know in software when the device is present or
not, you don't need "real" PCIe hotplug, just need to tickle the
software right?

> > > How does rfkill work?  It sounds like it completely removes power from
> > > the wifi device, putting it in D3cold.  Is there any software
> > > notification other than the "Slot present pin change" (which looks
> > > like a Tegra-specific thing)?

Well, they said above it's a GPIO that controls it, so the software
already knows and doesn't really need an event?

> > The rfkill subsystem provides a generic interface for disabling any radio
> > transmitter in the system. WiFi M.2 form factor cards provide W_DISABLE
> > GPIO to control the radio transmitter

But it depends on the hardware how this is handled, Intel NICs for
example just trigger an IRQ to the host and don't turn off much, for
them the W_DISABLE pin is just a GPIO in input mode, with edge triggered
interrupt to the driver.

> > and I have seen some cards provide
> > control to turn off complete chip through this GPIO. 

I never heard of this. Which NICs are we talking about?

> Perhaps what we need here is some sort of mechanism to make rfkill and
> the PCI host controller interoperate? I could imagine for example that
> the PCI host controller would get a new "rfkill" property in device
> tree that points at the rfkill device via phandle.

But you don't know which the rfkill device is, do you?

I mean, fundamentally, you just have a GPIO that turns on and off the
W_DISABLE pin. NICs will not generally disappear from the bus when
that's turned on, so you need a NIC driver integration.

I guess you also have an rfkill-gpio driver assigned to this GPIO, which
gets assigned there via DT/platform code?

Ah, but then I guess you could have a phandle in the DT or so that ties
the W_DISABLE-GPIO with the PCIe slot that it controls.

> The driver could then get a reference to it using something like:
> 
> 	rfkill = rfkill_get(dev);
> 	if (IS_ERR(rfkill)) {
> 		...
> 	}
> 
> and register for notification:
> 
> 	err = rfkill_subscribe(rfkill, callback);
> 	if (err < 0) {
> 		...
> 	}
> 
> rfkill_unsubscribe() and rfkill_put() would then be used upon driver
> unload to detach from the rfkill.

This I don't understand.

> I noticed that there's an rfkill-gpio driver (net/rfkill/rfkill-gpio.c)
> that already does pretty much everything that we need, except that it
> doesn't support DT yet, but I suspect that that's pretty easy to add.

Oh, good point, no DT support here - so how *do* you actually
instantiate the rfkill today??

> Johannes, any thoughts on this. In a nutshell what we're trying to solve
> here is devices that get removed from/added to PCI based on an rfkill-
> type of device. The difference to other implementations is that we have
> no way of detecting when the device has gone away (PCI hotplug does not
> work). So we'd need some software-triggered mechanism to let the PCI
> host controller know when the device is presumably going away or being
> added back, so that the PCI bus can be rescanned and the PCI device
> removed or added at that point).

Right.

So, I'm not even sure we need the *driver* to do anything other than say
"I know the device will drop off the bus when rfkill is enabled", right?


But do we actually need rfkill to be involved here?

I mean, let's say first we make rfkill-gpio DT-aware, rather than just
ACPI. This should be simple. Then it drives a GPIO (it can actually
drive two and a clock, not sure I know why).

Now, next we need something that says that the device should be treated
as hotplug/unplug. We could make this in the driver somehow like you
suggested, but that seems like a lot of effort?

Couldn't we put this into the *GPIO* subsystem instead?

I mean - conceivably there could be GPIOs that just power down a device
for example. Not even through something like W_DISABLE, but just having
a GPIO hooked up to a transistor on the voltage pin of the device. That
would have very similar semantics?

So why not just attach the PCIe device/port to the GPIO, and have the
GPIO implementation here call the detach/attach (or detach/rescan?) when
they are toggled?

Not that I'd mind having it in rfkill! But it seems like a special case
to have it there, when you can do so much more with GPIOs.

johannes


  reply index

Thread overview: 72+ 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-06-17 10:01             ` Manikanta Maddireddy
2019-06-17 11:47               ` Thierry Reding
2019-06-17 19:30                 ` Bjorn Helgaas
2019-06-18  5:36                   ` Manikanta Maddireddy
2019-06-18 10:49                     ` Thierry Reding
2019-06-18 12:32                       ` Johannes Berg [this message]
2019-06-18 13:40                         ` Thierry Reding
2019-06-18 14:48                           ` Johannes Berg
2019-06-19 13:38                         ` Bjorn Helgaas
2019-06-19 13:40                           ` Johannes Berg
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-06-17 11:30   ` Thierry Reding
2019-06-17 11:38     ` Manikanta Maddireddy
2019-06-17 11:48       ` Thierry Reding
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
2019-06-14 16:30                   ` Manikanta Maddireddy
2019-06-14 16:53                     ` Lorenzo Pieralisi
2019-06-14 17:23                       ` Manikanta Maddireddy
2019-06-17  9:48                         ` Lorenzo Pieralisi
2019-06-17 10:27                           ` Manikanta Maddireddy
2019-06-17 10:39                             ` Lorenzo Pieralisi
2019-06-17 11:29                         ` Thierry Reding
2019-06-17 11:26                       ` Thierry Reding
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=9c0fb01f0dc6a193265297eaa100a35ff25413e7.camel@sipsolutions.net \
    --to=johannes@sipsolutions.net \
    --cc=devicetree@vger.kernel.org \
    --cc=helgaas@kernel.org \
    --cc=jonathanh@nvidia.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=mmaddireddy@nvidia.com \
    --cc=rjw@rjwysocki.net \
    --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