On 4/3/2019 11:06 PM, Bjorn Helgaas wrote:
On Wed, Apr 03, 2019 at 03:13:09PM +0530, Vidya Sagar wrote:
On 4/3/2019 12:01 AM, Bjorn Helgaas wrote:
On Tue, Apr 02, 2019 at 12:47:48PM +0530, Vidya Sagar wrote:
On 3/30/2019 2:22 AM, Bjorn Helgaas wrote:
On Tue, Mar 26, 2019 at 08:43:26PM +0530, Vidya Sagar wrote:
Add support for Synopsys DesignWare core IP based PCIe host controller
present in Tegra194 SoC.
- Why does this chip require pcie_pme_disable_msi()? The only other
use is a DMI quirk for "MSI Wind U-100", added by c39fae1416d5
("PCI PM: Make it possible to force using INTx for PCIe PME
signaling").
Because Tegra194 doesn't support raising PME interrupts through MSI line.
What does the spec say about this? Is hardware supposed to support
MSI for PME? Given that MSI Wind U-100 and Tegra194 are the only two
cases we know about where PME via MSI isn't supported, it seems like
there must be either a requirement for that or some mechanism for the
OS to figure this out, e.g., a capability bit.
AFAIU, Spec doesn't say anything about whether PME interrupt should be through MSI
or by other means. As far as Tegra194 is concerned, there are only two
interrupt lanes that go from PCIe IP to GIC, one being legacy interrupt (that
represents legacy interrupts coming over PCIe bus from downstream devices and
also the events happening internal to root port) and the other being MSI
interrupt (that represents MSI interrupts coming over PCIe bus from downstream
devices). Since hardware folks had a choice to route PME interrupts either
through legacy interrupt line or through MSI interrupt line and legacy interrupt
line was chosen as a design choice. That being the case at hardware level, I tried
to inform the same through pcie_pme_disable_msi() to PCIe sub-system that
PME interrupts are not expected over MSI.
I see that an earlier patch added "bus" to struct pcie_port.
I think it would be better to somehow connect to the
pci_host_bridge struct. Several other drivers already do
this; see uses of pci_host_bridge_from_priv().
All non-DesignWare based implementations save their private data
structure in 'private' pointer of struct pci_host_bridge and use
pci_host_bridge_from_priv() to get it back. But, DesignWare
based implementations save pcie_port in 'sysdata' and nothing in
'private' pointer. So, I'm not sure if
pci_host_bridge_from_priv() can be used in this case. Please do
let me know if you think otherwise.
DesignWare-based drivers should have a way to retrieve the
pci_host_bridge pointer. It doesn't have to be *exactly* the same
as non-DesignWare drivers, but it should be similar.
I gave my reasoning as to why with the current code, it is not
possible to get the pci_host_bridge structure pointer from struct
pcie_port pointer in another thread as a reply to Thierry Reding's
comments. Since Jishen'g changes to support remove functionality are
accepted, I think using bus pointer saved in struct pcie_port
pointer shouldn't be any issue now. Please do let me know if that is
something not acceptable.
That would give you the bus, as well as flags like
no_ext_tags, native_aer, etc, which this driver, being a host
bridge driver that's responsible for this part of the
firmware/OS interface, may conceivably need.
I think saving the pp->root_bus pointer as Jisheng's patch does is a
sub-optimal solution. If we figure out how to save the
pci_host_bridge pointer, we automatically get the root bus pointer as
well.
It may require some restructuring to save the pci_host_bridge pointer,
but I doubt it's really *impossible*.
Is it OK to save pci_host_bridge pointer in pcie_port structure directly? I see
that as another way to get pci_host_bridge pointer from pcie_port
structure. My understanding is that, to get pci_host_bridge pointer, either
pcie_port structure should be part of pci_host_bridge structure (which is the
case with all non-DW implementations) or pcie_port should have a pointer to
some structure which is directly (and not by means of a pointer) part of
pci_host_bridge structure so that container_of() can be used to get pci_host_bridge.
As I see, there is no data object of pci_host_bridge whose pointer is saved in
pcie_port structure. In fact, in reverse, pcie_port's struct dev pointer is saved
as parent to pci_host_bridge's struct dev. So, another way would be to iterate over
the children of pcie_port's struct dev pointer to find pci_host_bridge's dev pointer
and from there get pci_host_bridge through container_of. But, I think is complicating
it more than using bus pointer from pcie_port. I'm not sure if I'm able to convey the
issue I'm facing here to get pci_host_bridge from pcie_port, but doing any other thing seems complicating it even more.
+static int tegra_pcie_dw_runtime_suspend(struct device *dev)
+{
+ struct tegra_pcie_dw *pcie = dev_get_drvdata(dev);
+
+ tegra_pcie_downstream_dev_to_D0(pcie);
+
+ pci_stop_root_bus(pcie->pci.pp.bus);
+ pci_remove_root_bus(pcie->pci.pp.bus);
Why are you calling these? No other drivers do this except in
their .remove() methods. Is there something special about
Tegra, or is this something the other drivers *should* be
doing?
Since this API is called by remove, I'm removing the hierarchy
to safely bring down all the devices. I'll have to re-visit this
part as Jisheng Zhang's patches
https://patchwork.kernel.org/project/linux-pci/list/?series=98559
are now approved and I need to verify this part after
cherry-picking Jisheng's changes.
Tegra194 should do this the same way as other drivers, independent
of Jisheng's changes.
When other Designware implementations add remove functionality, even
they should be calling these APIs (Jisheng also mentioned the same
in his commit message)
My point is that these APIs should be called from driver .remove()
methods, not from .runtime_suspend() methods.
.remove() internally calls pm_runtime_put_sync() API which calls
.runtime_suspend(). I made a new patch to add a host_deinit() call
which make all these calls. Since host_init() is called from inside
.runtime_resume() of this driver, to be in sync, I'm now calling
host_deinit() from inside .runtime_suspend() API.
Bjorn
