From: Bjorn Helgaas <helgaas@kernel.org>
To: Marek Vasut <marek.vasut@gmail.com>
Cc: linux-pci@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>,
Geert Uytterhoeven <geert+renesas@glider.be>,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
Wolfram Sang <wsa@the-dreams.de>,
Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH] PCI: rcar: Add L1 link state fix into data abort hook
Date: Sun, 13 Sep 2020 23:01:40 -0500 [thread overview]
Message-ID: <20200914040140.GA1237003@bjorn-Precision-5520> (raw)
In-Reply-To: <67f2451d-720d-1bd9-7628-6a9171f799d3@gmail.com>
On Mon, Sep 14, 2020 at 03:36:08AM +0200, Marek Vasut wrote:
> On 9/13/20 7:22 PM, Bjorn Helgaas wrote:
>
> [...]
>
> >> The R-Car PCIe controller is capable of handling L0s/L1 link states.
> >> While the controller can enter and exit L0s link state, and exit L1
> >> link state, without any additional action from the driver, to enter
> >> L1 link state, the driver must complete the link state transition by
> >> issuing additional commands to the controller.
> >
> > So IIUC an R-Car Root Port can't enter L1 without help from the
> > driver? That *sounds* like a hardware defect, since ASPM is supposed
> > to be "hardware-autonomous" once configured.
>
> The driver needs to complete the switch from L0 to L1 link state, yes.
>
> >> The problem is, this transition is not atomic. The controller sets
> >> PMEL1RX bit in PMSR register upon reception of PM_ENTER_L1 DLLP from
> >> the PCIe card, but then the controller enters some sort of inbetween
> >> state. The driver must detect this condition and complete the link
> >> state transition, by setting L1IATN bit in PMCTLR and waiting for
> >> the link state transition to complete.
> >>
> >> If a PCIe access happens inside of this window, where the controller
> >> is inbetween L0 and L1 link states, the access generates a fault and
> >> the ARM 'imprecise external abort' handler is invoked.
> >
> > Let me see if I understand this.
> >
> > 1) Endpoint sends PM_ENTER_L1
> > 2) R-Car Root Port receives PM_ENTER_L1, sets PMEL1RX
> > 3) R-Car driver does something to complete transition to L1
> >
> > And if the Endpoint driver does an MMIO access between 2) and 3), it
> > causes an external abort?
>
> Yes, because the link is in this inbetween state. So you need 3) to
> complete that transition.
>
> > I couldn't find anything in the R-Car driver today that completes the
> > transition to L1.
>
> That's because this patch adds it.
So with this patch, we finish the transition to L1 and immediately
return to L0. That must mean that even in this in-between state we
save some power?
And the link may stay in that in-between state indefinitely, until the
CPU does an MMIO access or the device initiates a DMA? But it will
never spend any time in L1 because the link never actually *gets* to
L1 until some event that will take it back to L0 occurs?
> > And the MMIO access will immediately bring the link back to L0, won't
> > it?
>
> Yes
>
> > Is there any benefit to L1 at all for this Root Port? If it can't
> > enter L1 and just stays in the in-between state until some event that
> > will bring it *out* of L1, maybe we just need a quirk to stop
> > advertising support for L1 in the first place.
>
> Power saving I would say.
>
> >> Just like other PCI controller drivers, here we hook the fault handler,
> >> perform the fixup to help the controller enter L1 link state, and then
> >> restart the instruction which triggered the fault. Since the controller
> >> is in L1 link state now, the link can exit from L1 link state to L0 and
> >> successfully complete the access.
> >>
> >> Note that this fixup is applicable only to Aarch32 R-Car controllers,
> >> the Aarch64 R-Car perform the same fixup in TFA, see TFA commit
> >> 0969397f2 ("rcar_gen3: plat: Prevent PCIe hang during L1X config access")
> >
> > TFA? This doesn't seem to be an upstream Linux commit; can you give a
> > more specific reference?
>
> This is the Trusted Firmware from ARM, in this case it behaves similar
> to ACPI. See e.g.:
>
> https://github.com/ARM-software/arm-trusted-firmware/commit/0969397f295621aa26b3d14b76dd397d22be58bf
Thanks for that; please include it in the commit log for the next
version.
Bjorn
next prev parent reply other threads:[~2020-09-14 4:01 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-12 21:18 [PATCH] PCI: rcar: Add L1 link state fix into data abort hook marek.vasut
2020-09-13 17:22 ` Bjorn Helgaas
2020-09-14 1:36 ` Marek Vasut
2020-09-14 4:01 ` Bjorn Helgaas [this message]
2020-09-16 13:50 ` Marek Vasut
2020-11-19 16:25 ` Lorenzo Pieralisi
2020-11-29 13:02 ` Marek Vasut
2020-09-14 9:29 ` Geert Uytterhoeven
2020-09-14 10:11 ` Geert Uytterhoeven
Reply instructions:
You may reply publicly 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=20200914040140.GA1237003@bjorn-Precision-5520 \
--to=helgaas@kernel.org \
--cc=bhelgaas@google.com \
--cc=geert+renesas@glider.be \
--cc=linux-pci@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=marek.vasut@gmail.com \
--cc=wsa@the-dreams.de \
--cc=yoshihiro.shimoda.uh@renesas.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).