Re: [Qemu-devel] [PATCH] pcie: Add more ASPM support

[Bjorn Helgaas]

On Sun, May 19, 2013 at 5:54 AM, Michael S. Tsirkin <address@hidden> wrote:
> On Wed, May 15, 2013 at 04:33:08PM -0600, Bjorn Helgaas wrote:
>> Indicate ASPM L0s and L1 support in Link Capabilities and make the ASPM
>> bits in Link Control writable.  These Link Control bits don't do anything
>> in qemu, but having them writable means the BIOS or OS can write them as
>> on real hardware.
>>
>> Signed-off-by: Bjorn Helgaas <address@hidden>
>
> Out of curiousity - L1 is optional on real hardware, isn't it?

Yes.  Per PCIe spec r3.0, sec 5.4.1, "ASPM support for the L1 Link
state is optional unless specifically required by a particular form
factor."

> Anyway, went over spec and L1 is just like L0s
> but with higher latency, so I think this is a safe change.
> A minor comment below - would appreciate if you can address it.
>
>> ---
>>  hw/pci/pcie.c              |    4 +++-
>>  include/hw/pci/pcie_regs.h |    5 ++++-
>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 62bd0b8..7336054 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -73,13 +73,15 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, 
>> uint8_t type, uint8_t port)
>>
>>      pci_set_long(exp_cap + PCI_EXP_LNKCAP,
>>                   (port << PCI_EXP_LNKCAP_PN_SHIFT) |
>> -                 PCI_EXP_LNKCAP_ASPMS_0S |
>> +                 PCI_EXP_LNKCAP_ASPMS_0S | PCI_EXP_LNKCAP_ASPMS_1 |
>>                   PCI_EXP_LNK_MLW_1 |
>>                   PCI_EXP_LNK_LS_25);
>>
>>      pci_set_word(exp_cap + PCI_EXP_LNKSTA,
>>                   PCI_EXP_LNK_MLW_1 | PCI_EXP_LNK_LS_25);
>>
>> +    pci_set_word(dev->wmask + pos + PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_ASPM);
>> +
>>      pci_set_long(exp_cap + PCI_EXP_DEVCAP2,
>>                   PCI_EXP_DEVCAP2_EFF | PCI_EXP_DEVCAP2_EETLPP);
>>
>> diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
>> index 4d123d9..6b80a8d 100644
>> --- a/include/hw/pci/pcie_regs.h
>> +++ b/include/hw/pci/pcie_regs.h
>> @@ -31,7 +31,7 @@
>>  #define PCI_EXP_FLAGS_TYPE_SHIFT        (ffs(PCI_EXP_FLAGS_TYPE) - 1)
>>
>>
>> -/* PCI_EXP_LINK{CAP, STA} */
>> +/* PCI_EXP_LINK{CAP, STA, CTL} */
>>  /* link speed */
>>  #define PCI_EXP_LNK_LS_25               1
>>
>> @@ -41,9 +41,12 @@
>>  /* PCI_EXP_LINKCAP */
>>  #define PCI_EXP_LNKCAP_ASPMS_SHIFT      (ffs(PCI_EXP_LNKCAP_ASPMS) - 1)
>>  #define PCI_EXP_LNKCAP_ASPMS_0S         (1 << PCI_EXP_LNKCAP_ASPMS_SHIFT)
>> +#define PCI_EXP_LNKCAP_ASPMS_1          (2 << PCI_EXP_LNKCAP_ASPMS_SHIFT)
>>
>>  #define PCI_EXP_LNKCAP_PN_SHIFT         (ffs(PCI_EXP_LNKCAP_PN) - 1)
>>
>> +#define PCI_EXP_LNKCTL_ASPM          0x0003
>> +
>>  #define PCI_EXP_SLTCAP_PSN_SHIFT        (ffs(PCI_EXP_SLTCAP_PSN) - 1)
>>
>>  #define PCI_EXP_SLTCTL_IND_RESERVED     0x0
>
>
> I'd like to name them the same as you did in linux/pci_regs.h
> Let's rename PCI_EXP_LNKCAP_ASPMS_0S/PCI_EXP_LNKCAP_ASPMS_1 as well.

Will do.  I'll repost this shortly.

Bjorn