Hello, Sorry for the delay. On 08 August 2018 14:30, Marek Vasut wrote: > On 07/25/2018 11:08 PM, Marek Vasut wrote: > > On 06/13/2018 07:25 PM, Bjorn Helgaas wrote: > >> On Wed, Jun 13, 2018 at 04:52:52PM +0100, Lorenzo Pieralisi wrote: > >>> On Wed, Jun 13, 2018 at 08:53:08AM -0500, Bjorn Helgaas wrote: > >>>> On Wed, Jun 13, 2018 at 01:54:51AM +0200, Marek Vasut wrote: > >>>>> On 06/11/2018 03:59 PM, Bjorn Helgaas wrote: > >>>>>> On Sun, Jun 10, 2018 at 03:57:10PM +0200, Marek Vasut wrote: > >>>>>>> On 11/17/2017 06:49 PM, Lorenzo Pieralisi wrote: > >>>>>>>> On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote: > >>>>>>>>> From: Phil Edworthy <phil.edworthy@xxxxxxxxxxx> > >>>>>>>>> > >>>>>>>>> Most PCIe host controllers support L0s and L1 power states via > ASPM. > >>>>>>>>> The R-Car hardware only supports L0s, so when the system > >>>>>>>>> suspends and resumes we have to manually handle L1. > >>>>>>>>> When the system suspends, cards can put themselves into L1 > and > >>>>>>>>> send a > >>>>>>>> > >>>>>>>> I assumed L1 entry has to be negotiated depending upon the PCIe > >>>>>>>> hierarchy capabilities, I would appreciate if you can explain > >>>>>>>> to me what's the root cause of the issue please. > >>>>>>> > >>>>>>> You should probably ignore the suspend/resume part altogether. > >>>>>>> The issue here is that the cards can enter L1 state, while the > >>>>>>> controller won't do that automatically, it can only detect that the > link went into L1 state. > >>>>>>> If that happens,the driver must manually put the controller to L1 > state. > >>>>>>> The controller can transition out of L1 state automatically though. > >>>>>> > >>>>>> From earlier discussion I thought the R-Car root port did not > >>>>>> advertise L1 support. > >>>>> > >>>>> Which discussion ? This one or somewhere else ? > >>>> > >>>> > https://lkml.kernel.org/r/HK2PR0601MB1393D917D343E6363484CA68F5CB0 > @ > >>>> HK2PR0601MB1393.apcprd06.prod.outlook.com > >>>> > >>>> Re-reading that, I think I see my misunderstanding. I was only > >>>> considering L1 in the ASPM context. I didn't realize the L1 > >>>> implications of devices being in states other than D0. > >>>> > >>>> Obviously L1 support for ASPM is optional and advertised via Link > >>>> Capabilities. But per PCIe r4.0, sec 5.2, L1 support is required > >>>> for PCI-PM compatible power management, and is entered "whenever > >>>> all Functions ... are programmed to a D-state other than D0." > >>>> > >>>> So I guess this means *every* device is supposed to support L1 when > >>>> it is in a non-D0 power state. I think *this* is the case you're > >>>> solving. > >>>> > >>>> A little more of this detail, e.g., that this issue has nothing to > >>>> do with ASPM, it's probably an R-Car erratum that the RC can't > >>>> transition from L1 to L0, etc., in the changelog would really help > >>>> clear things up for me. > >>> > >>> I think that the issue is related to the L0->L1 transition upon > >>> system suspend (ie the kernel must force the controller into L1 when > >>> all devices are in a sleep state) and for this specific reason I > >>> still think that checking for a PM_Enter_L1 DLLP reception and doing > >>> the L0->L1 transition within a config access is wrong and prone to > >>> error (what's the rationale behind that ?), this ought to be done > >>> using PM methods in the host controller driver. > >> > >> But doesn't the problem happen whenever the link goes to L1, for any > >> reason? E.g., runtime power management might put an endpoint in D3 > >> even if we're not doing a whole system suspend. A user could even > >> force the endpoint to D3 by writing to PCI_PM_CTRL with "setpci". If > >> that's the case, I don't think the host controller PM methods will be > >> enough to work around the issue. > > > > I think so, it's the link that goes into L1 state and this can happen > > without any action from the controller side. > > > >> The comment in the patch ("If we are not in L1 link state and we have > >> received PM_ENTER_L1 DLLP, transition to L1 link state") suggests > >> that the R-Car host doesn't handle step 10 in PCIe r4.0, sec 5.3.2.1 > >> correctly, i.e., it doesn't complete the transition of the link to L1. > >> > >> Putting this workaround in the config accessor makes sense to me > >> because in this situation the endpoint thinks it's in L1 and it won't > >> receive TLPs for config accesses. Apparently forcing the RP to L1 > >> completes the L1 entry, and the RP correctly handles the "Exit from > >> L1 State" (sec 5.3.2.2) that's required when the RP needs to send a > >> TLP to the endpoint. That my understanding. > >> I think there's still a potential issue if the endpoint goes to a > >> non-D0 state, the link is stuck in this transitional state (endpoint > >> thinks it's L1, RP thinks it's L0), and the *endpoint* wants to exit > >> L1, e.g., so it can send a PME message for a wakeup. I don't know > >> what happens then. > > > > Is there some hardware which I can use to simulate this situation ? > > > >> If there were a real erratum writeup for this, it would probably > >> discuss this situation. > > > > I went through the latest errata sheet and don't see anything. The > > datasheet only mentions that L0/L0s/L1 is supported and L2 is not > supported. > > > Maybe Phil can comment on this too ? There is no errata for this that I know of. The rcar RP supports L0/L0s/L1 in hardware, but only supports L0s via ASPM, i.e. you need software to poke some registers to make the RP transition to L1. However, both before and after this patch, the RP does not transition L1 when the endpoints change to L1. This patch only transitions the RP to L1 during accessing a card's config registers, if the RP is not in L1 link state and has received PM_ENTER_L1 DLLP (e.g. resume). After this, the hardware will handle the transition out of L1. The relevant part of the rcar manual says: "After a recovery to L0, if the device is in the Non-D0 state and PM_Enter_L1 DLLP is transmitted from the downstream device, software should confirm that hardware is in the L0 state (PMSR.PMSTATE = L0) and initiate the L1 transition sequence again (write 1 to PMCTLR.L1IATN). In this case, the sequence is: L0 → L1 → L0 recovery → L1 again." I don’t think the potential issue that Bjorn talked about can happen because the RP does go into L1. I could be wrong though... The driver should also have a runtime-PM hook to transition to L1 on suspend in order to save power. However, that is somewhat separate to the problem the patch fixes. Phil
