Re: [Qemu-devel] [RFC PATCH v2 02/10] pci: Introduce function for PCI PM capability creation

[Dmitry Fleytman]

> On 19 Jan 2016, at 12:36 PM, Marcel Apfelbaum <address@hidden> wrote:
> 
> On 01/18/2016 07:35 PM, Leonid Bloch wrote:
>> From: Dmitry Fleytman <address@hidden>
>> 
>> Signed-off-by: Dmitry Fleytman <address@hidden>
>> Signed-off-by: Leonid Bloch <address@hidden>
>> ---
>>  hw/pci/pci.c              | 21 +++++++++++++++++++++
>>  include/hw/pci/pci.h      |  2 ++
>>  include/hw/pci/pci_regs.h |  1 +
>>  3 files changed, 24 insertions(+)
>> 
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index b3d5100..3aaf86c 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -2050,6 +2050,27 @@ static void pci_del_option_rom(PCIDevice *pdev)
>>      pdev->has_rom = false;
>>  }
>> 
>> +int pci_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
>> +{
>> +    int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, 
>> PCI_PM_SIZEOF);
>> +
>> +    if (ret >= 0) {
>> +        pci_set_word(pdev->config + offset + PCI_PM_PMC,
>> +                     PCI_PM_CAP_VER_1_1 |
> 
> Hi,
> 
> Why not ver 1.2 ? just wondering
> 
>> +                     pmc);
>> +
>> +        pci_set_word(pdev->wmask + offset + PCI_PM_CTRL,
>> +                     PCI_PM_CTRL_STATE_MASK |
>> +                     PCI_PM_CTRL_PME_ENABLE |
> 
> PME_ENABLE and PME_STATUS are writable only if the function supports PME# 
> generation from D3cold
> 
> 
>> +                     PCI_PM_CTRL_DATA_SEL_MASK
> 
> And Data_Select is writable only if the data register is implemented.
> 
> My point is this seems to be a standard capability function, but it depends
> on optional function features.
> 
> Thanks,
> Marcel

Hi Marcel,

I think it’s a good point, thanks.
Considering this, we’d better move this function into the device code, it’s not 
generic enough to be in the common code.

~Dmitry

> 
> );
>> +
>> +        pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
>> +                     PCI_PM_CTRL_PME_STATUS);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>  /*
>>   * if !offset
>>   * Reserve space and add capability to the linked list in pci config space
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index b97c295..cec7234 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -319,6 +319,8 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
>>                         uint8_t offset, uint8_t size,
>>                         Error **errp);
>> 
>> +int pci_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc);
>> +
>>  void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t 
>> cap_size);
>> 
>>  uint8_t pci_find_capability(PCIDevice *pci_dev, uint8_t cap_id);
>> diff --git a/include/hw/pci/pci_regs.h b/include/hw/pci/pci_regs.h
>> index 56a404b..2bd3ac9 100644
>> --- a/include/hw/pci/pci_regs.h
>> +++ b/include/hw/pci/pci_regs.h
>> @@ -221,6 +221,7 @@
>> 
>>  #define PCI_PM_PMC          2       /* PM Capabilities Register */
>>  #define  PCI_PM_CAP_VER_MASK        0x0007  /* Version */
>> +#define  PCI_PM_CAP_VER_1_1     0x0002  /* PCI PM spec ver. 1.1 */
>>  #define  PCI_PM_CAP_PME_CLOCK       0x0008  /* PME clock required */
>>  #define  PCI_PM_CAP_RESERVED    0x0010  /* Reserved field */
>>  #define  PCI_PM_CAP_DSI             0x0020  /* Device specific 
>> initialization */
>> 
>