Re: [PATCH 09/10] PCI: tegra: Add Tegra194 PCIe support

[Vidya Sagar]

On 4/6/2019 12:28 AM, Bjorn Helgaas wrote:
> On Fri, Apr 05, 2019 at 01:23:51AM +0530, Vidya Sagar wrote:
> > 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.
>
> There's something wrong here.  Either the question of how PME is
> signaled is generic and the spec provides a way for the OS to discover
> that method, or it's part of the device-specific architecture that
> each host bridge driver has to know about its device.  If the former,
> we need to make the PCI core smart enough to figure it out.  If the
> latter, we need a better interface than this ad hoc
> pcie_pme_disable_msi() thing.  But if it is truly the latter, your
> current code is sufficient and we can refine it over time.
In case of Tegra194, it is the latter case.

> > > > > > > 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.
>
> What I suspect should happen eventually is the DWC driver should call
> devm_pci_alloc_host_bridge() directly, as all the non-DWC drivers do.
> That would require a little reorganization of the DWC data structures,
> but it would be good to be more consistent.
>
> For now, I think stashing the pointer in pcie_port or dw_pcie would be
> OK.  I'm not 100% clear on the difference, but it looks like either
> should be common across all the DWC drivers, which is what we want.
Since dw_pcie is common for both root port and end point mode structures,
I think it makes sense to keep the pointer in pcie_port as this structure
is specific to root port mode of operation.
I'll make a note to reorganize code to have devm_pci_alloc_host_bridge()
used in the beginning itself to be inline with non-DWC implementations.
But, I'll take it up later (after I'm done with upstreaming current series)

> (Tangent, dw_pcie_host_init() currently uses pci_alloc_host_bridge(),
> not devm_pci_alloc_host_bridge(), even though it uses other devm
> interfaces.  This looks like a probable buglet.)
>
> (Tangent 2, devm_pci_alloc_host_bridge() doesn't initialize
> bridge->native_aer, etc, as pci_alloc_host_bridge() does.  This looks
> like my fault from 02bfeb484230 ("PCI/portdrv: Simplify PCIe feature
> permission checking"), and it probably means none of those PCIe
> services work correctly for these native host bridge drivers.)
>
> > > > > > > > +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.
>
> I think this is wrong.  pci_stop_root_bus() will detach all the
> drivers from all the devices.  We don't want to do that if we're
> merely runtime suspending the host bridge, do we?
In the current driver, the scenarios in which .runtime_suspend() is called
are
a) during .remove() call and
b) when there is no endpoint found and controller would be shutdown
In both cases, it is required to stop the root bus and remove all devices,
so, instead of having same call present in respective paths, I kept them
in .runtime_suspend() itself to avoid code duplication.

> Bjorn