Message-ID: <20190409132604.GA256045@google.com>
Date:   Tue, 9 Apr 2019 08:26:04 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Vidya Sagar <vidyas@...dia.com>
Cc:     robh+dt@...nel.org, mark.rutland@....com, thierry.reding@...il.com,
        jonathanh@...dia.com, kishon@...com, catalin.marinas@....com,
        will.deacon@....com, lorenzo.pieralisi@....com,
        jingoohan1@...il.com, gustavo.pimentel@...opsys.com,
        mperttunen@...dia.com, tiwai@...e.de, spujar@...dia.com,
        skomatineni@...dia.com, liviu.dudau@....com, krzk@...nel.org,
        heiko@...ech.de, horms+renesas@...ge.net.au, olof@...om.net,
        maxime.ripard@...tlin.com, andy.gross@...aro.org,
        bjorn.andersson@...aro.org, jagan@...rulasolutions.com,
        enric.balletbo@...labora.com, ezequiel@...labora.com,
        stefan.wahren@...e.com, marc.w.gonzalez@...e.fr,
        l.stach@...gutronix.de, tpiepho@...inj.com,
        hayashi.kunihiko@...ionext.com, yue.wang@...ogic.com,
        shawn.lin@...k-chips.com, xiaowei.bao@....com,
        devicetree@...r.kernel.org, mmaddireddy@...dia.com,
        kthota@...dia.com, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 09/10] PCI: tegra: Add Tegra194 PCIe support

On Tue, Apr 09, 2019 at 05:00:53PM +0530, Vidya Sagar wrote:
> 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.

> > 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.

This isn't a Tegra194 question; it's a question of whether this
behavior is covered by the PCIe spec.

> > 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)

Fair enough.

> > > .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

It makes sense that you should call pci_stop_root_bus() during
.remove(), i.e., when the host controller driver is being removed,
because the PCI bus will no longer be accessible.  I think you should
call it *directly* from tegra_pcie_dw_remove() because that will match
what other drivers do.

> 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.

I don't understand this part.  We should be able to runtime suspend
the host controller without detaching drivers for child devices.

If you shutdown the controller completely and detach the *host
controller driver*, sure, it makes sense to detach drivers from child
devices.  But that would be handled by the host controller .remove()
method, not by the runtime suspend method.

Bjorn

Powered by blists - more mailing lists