On Thu, Feb 18, 2016 at 12:35 PM, Michael S. Tsirkin <m...@redhat.com> wrote:
> On Thu, Feb 18, 2016 at 12:07:24PM +0200, Leonid Bloch wrote:
>> From: Dmitry Fleytman <dmitry.fleyt...@ravellosystems.com>
>>
>
> A bit of text here to explain what's different
> from the existing APIs.
>
>> Signed-off-by: Dmitry Fleytman <dmitry.fleyt...@ravellosystems.com>
>> Signed-off-by: Leonid Bloch <leonid.bl...@ravellosystems.com>
>> ---
>> hw/pci/pcie.c | 80
>> +++++++++++++++++++++++++++++++++++-----------
>> include/hw/pci/pcie.h | 4 +++
>> include/hw/pci/pcie_regs.h | 5 +--
>> 3 files changed, 69 insertions(+), 20 deletions(-)
>>
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 435a6cf..be3a318 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -42,26 +42,15 @@
>> /***************************************************************************
>> * pci express capability helper functions
>> */
>> -int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t
>> port)
>> -{
>> - int pos;
>> - uint8_t *exp_cap;
>> -
>> - assert(pci_is_express(dev));
>> -
>> - pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
>> - PCI_EXP_VER2_SIZEOF);
>> - if (pos < 0) {
>> - return pos;
>> - }
>> - dev->exp.exp_cap = pos;
>> - exp_cap = dev->config + pos;
>>
>> +static void
>> +_pcie_cap_v1_fill(uint8_t *exp_cap, uint8_t port, uint8_t type, uint8_t
>> version)
>
> Names are rather confusing. There's init, there's fill, there's
> a prefix and a non prefix versions ...
> Please add some comments for each function, saying what it does.
Thanks, the naming was changed in v2. Some comments were added as well.
>
> Also this one is really both for v1 and v2, correct?
This one fills the v1 values, and the values common with v1 in v2.
>
> Don't use _ prefix in QEMU please.
Thanks, got rid of leading underscores in this patch, and in the others as well.
>
>> +{
>> /* capability register
>> - interrupt message number defaults to 0 */
>> + interrupt message number defaults to 0 */
>> pci_set_word(exp_cap + PCI_EXP_FLAGS,
>> ((type << PCI_EXP_FLAGS_TYPE_SHIFT) & PCI_EXP_FLAGS_TYPE) |
>> - PCI_EXP_FLAGS_VER2);
>> + version);
>>
>> /* device capability register
>> * table 7-12:
>> @@ -80,6 +69,23 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t
>> type, uint8_t port)
>>
>> pci_set_word(exp_cap + PCI_EXP_LNKSTA,
>> PCI_EXP_LNK_MLW_1 | PCI_EXP_LNK_LS_25
>> |PCI_EXP_LNKSTA_DLLLA);
>> +}
>> +
>> +int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t
>> port)
>> +{
>> + int pos;
>> + uint8_t *exp_cap;
>> +
>> + assert(pci_is_express(dev));
>> +
>> + pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
>> PCI_EXP_VER2_SIZEOF);
>> + if (pos < 0) {
>> + return pos;
>> + }
>> + dev->exp.exp_cap = pos;
>> + exp_cap = dev->config + pos;
>> +
>> + _pcie_cap_v1_fill(exp_cap, port, type, PCI_EXP_FLAGS_VER2);
>>
>> pci_set_long(exp_cap + PCI_EXP_DEVCAP2,
>> PCI_EXP_DEVCAP2_EFF | PCI_EXP_DEVCAP2_EETLPP);
>> @@ -88,7 +94,28 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t
>> type, uint8_t port)
>> return pos;
>> }
>>
>> -int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset)
>> +int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t
>> port)
>> +{
>> + int pos;
>> + uint8_t *exp_cap;
>> +
>> + assert(pci_is_express(dev));
>> +
>> + pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
>> + PCI_EXP_VER1_SIZEOF);
>> + if (pos < 0) {
>> + return pos;
>> + }
>> + dev->exp.exp_cap = pos;
>> + exp_cap = dev->config + pos;
>> +
>> + _pcie_cap_v1_fill(exp_cap, port, type, PCI_EXP_FLAGS_VER1);
>> +
>> + return pos;
>> +}
>> +
>> +static int
>> +_pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size)
>> {
>> uint8_t type = PCI_EXP_TYPE_ENDPOINT;
>>
>> @@ -101,7 +128,19 @@ int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t
>> offset)
>> type = PCI_EXP_TYPE_RC_END;
>> }
>>
>> - return pcie_cap_init(dev, offset, type, 0);
>> + return (cap_size == PCI_EXP_VER1_SIZEOF)
>> + ? pcie_cap_v1_init(dev, offset, type, 0)
>> + : pcie_cap_init(dev, offset, type, 0);
>> +}
>> +
>> +int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset)
>> +{
>> + return _pcie_endpoint_cap_init(dev, offset, PCI_EXP_VER2_SIZEOF);
>> +}
>> +
>> +int pcie_endpoint_cap_v1_init(PCIDevice *dev, uint8_t offset)
>> +{
>> + return _pcie_endpoint_cap_init(dev, offset, PCI_EXP_VER1_SIZEOF);
>> }
>>
>> void pcie_cap_exit(PCIDevice *dev)
>> @@ -109,6 +148,11 @@ void pcie_cap_exit(PCIDevice *dev)
>> pci_del_capability(dev, PCI_CAP_ID_EXP, PCI_EXP_VER2_SIZEOF);
>> }
>>
>> +void pcie_cap_v1_exit(PCIDevice *dev)
>> +{
>> + pci_del_capability(dev, PCI_CAP_ID_EXP, PCI_EXP_VER1_SIZEOF);
>> +}
>> +
>> uint8_t pcie_cap_get_type(const PCIDevice *dev)
>> {
>> uint32_t pos = dev->exp.exp_cap;
>> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
>> index b48a7a2..cbbf0c5 100644
>> --- a/include/hw/pci/pcie.h
>> +++ b/include/hw/pci/pcie.h
>> @@ -80,8 +80,12 @@ struct PCIExpressDevice {
>>
>> /* PCI express capability helper functions */
>> int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t
>> port);
>> +int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset,
>> + uint8_t type, uint8_t port);
>> int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset);
>> void pcie_cap_exit(PCIDevice *dev);
>> +int pcie_endpoint_cap_v1_init(PCIDevice *dev, uint8_t offset);
>> +void pcie_cap_v1_exit(PCIDevice *dev);
>> uint8_t pcie_cap_get_type(const PCIDevice *dev);
>> void pcie_cap_flags_set_vector(PCIDevice *dev, uint8_t vector);
>> uint8_t pcie_cap_flags_get_vector(PCIDevice *dev);
>> diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
>> index 6a28b33..a95522a 100644
>> --- a/include/hw/pci/pcie_regs.h
>> +++ b/include/hw/pci/pcie_regs.h
>> @@ -11,6 +11,7 @@
>>
>> /* express capability */
>>
>> +#define PCI_EXP_VER1_SIZEOF 0x14 /* express capability of ver.
>> 1 */
>> #define PCI_EXP_VER2_SIZEOF 0x3c /* express capability of ver.
>> 2 */
>> #define PCI_EXT_CAP_VER_SHIFT 16
>> #define PCI_EXT_CAP_NEXT_SHIFT 20
>> @@ -26,11 +27,11 @@
>> (((x) + PCI_EXT_CAP_ALIGN - 1) & ~(PCI_EXT_CAP_ALIGN - 1))
>>
>> /* PCI_EXP_FLAGS */
>> -#define PCI_EXP_FLAGS_VER2 2 /* for now, supports only ver. 2
>> */
>> +#define PCI_EXP_FLAGS_VER1 1
>> +#define PCI_EXP_FLAGS_VER2 2
>> #define PCI_EXP_FLAGS_IRQ_SHIFT ctz32(PCI_EXP_FLAGS_IRQ)
>> #define PCI_EXP_FLAGS_TYPE_SHIFT ctz32(PCI_EXP_FLAGS_TYPE)
>>
>> -
>> /* PCI_EXP_LINK{CAP, STA} */
>> /* link speed */
>> #define PCI_EXP_LNK_LS_25 1
>> --
>> 2.5.0
