[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Patel, Mayurkumar]

Hi Rajat


>On Thu, Apr 6, 2017 at 8:19 AM, Sinan Kaya <okaya at codeaurora.org> wrote:
>>
>> On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
>> > Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
>> > In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
>> > In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
>> > substates Configuration).
>> >
>> > @Bjorn:
>> > I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
>> > the spec.,
>> >
>> > The spec. says following and correct me If I am wrong or I misinterpret the spec. :
>> >
>> > 5.5.4. L1 PM Substates Configuration
>> >
>> > The Setting of any enable bit must be performed at the Downstream Port before the
>> > corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
>> > bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
>> > before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
>> >
>
>Thanks for bringing to attention. My understanding / interpretation of
>"Downstream port" was the port pointing downwards (from the "Upstream"
>component). E.g. when an EP connects to a hub port, PCIe text refers
>to the hub port as the "downstream port". Similarly "upstream port" is
>used for referring to a switch's port that "points" upwards towards
>the root port. Thus, I interpreted the text to mean that I need to
>enable it in the "downstream port" (in the root port / switch port)
>followed by the "upstream port" (in the device).
>
>It would have been great if the PCIe spec was as clear for L1
>substates as it was for L1:
>----------------------------
>ASPM L1 must be enabled by software in the Upstream
>component on a Link prior to enabling ASPM L1 in the
>Downstream component on that Link.
>-----------------------------
>For ASPM L1, the spec clearly says "Upstream component" which can only
>mean the switch's "downstream" port. I'm open to changing it if there
>is consensus that my interpretation is wrong.

In fact, Your understanding seems to be correct. It was my mistake that I raised it without
actually, keeping in mind or understanding port/component difference which I didn't notice.
My sincere apologies for that.


>
>I'm actually not sure if I understood what is the problem that is
>being seen with L1 PM substates. Can you please elaborate?
>


The actual problem, what Sinan and me were seeing with ASPM L1 with POLICY_DEFAULT
as described in https://patchwork.kernel.org/patch/9548321/ 


Sinan worked out patches to resolve the issue but on my platform now I am starting to see
that ASPM L1SS gets impacted by these patches. Basically, with his patches, when Policy is set to default,
and if no PCIe EP is connected to Root Port during boot up, BIOS does not configure L1SS for it, on my
platform. So the link->aspm_default in aspm.c stores configuration without L1SS. When the EP gets
connected afterwards, due to link->aspm_default set to non L1SS configuration, even if BIOS
sets ASPM L1SS for Root Port and Endpoint both, it gets cleared in the aspm driver.

Without the patches Sinan has worked, it was a different issue as described above, and L1/L1SS were
getting always enabled by the BIOS for endpoint and RP, but kernel enabled/disabled them alternatively.

So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
enable L1SS on Root Port no matter Endpoint is connected or not during boot?

Or I am not sure, whether could we should still update link->aspm_default partly for L1SS in the old patch way
https://patchwork.ozlabs.org/patch/736771/ 

Any feedback would be helpful then may be if required I can or if Sinan can work out the patches for L1SS also fine for me.


>
>>
>>
>> Thanks for testing.
>>
>> commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
>> Author: Rajat Jain <rajatja at google.com>
>> Date:   Mon Jan 2 22:34:15 2017 -0800
>>
>>     PCI/ASPM: Add comment about L1 substate latency
>>
>>     Since the exit latencies for L1 substates are not advertised by a device,
>>     it is not clear in spec how to do a L1 substate exit latency check.  We
>>     assume that the L1 exit latencies advertised by a device include L1
>>     substate latencies (and hence do not do any check).  If that is not true,
>>     we should do some sort of check here.
>>
>>     (I'm not clear about what that check should like currently. I'd be glad to
>>     take up any suggestions).
>>
>>     Signed-off-by: Rajat Jain <rajatja at google.com>
>>     Signed-off-by: Bjorn Helgaas <bhelgaas at google.com>
>>
>> I added Rajat for discussion on L1SS. I think this deserves its own discussion.
>
>Thanks, The above commit specifically adds a comment to
>pcie_aspm_check_latency(), because I wanted to leave a note
>highlighting that potentially, we could add a more stringent check for
>exit latency for L1SS. But that has nothing to do with how we are
>configuring or enabling / disabling the L1 substates.
>
>Thanks & Best Regards,
>
>Rajat
>
>> I'll look at the other part of your email and move things around a little bit
>> less aggressively for the aspm_default.
>>
>>
>> --
>> Sinan Kaya
>> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
>> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928