Re: [PATCH edk2-platforms v5 21/28] Platform/Hisilicon/D06: Add PciHostBridgeLib

[Ming Huang <ming.huang@linaro.org>]

Hi Ard & Laszlo,

Sorry for delay reply.
Should all host bridges use EISA_PNP_ID(0x0A03)?

Ming

On 10/12/2018 4:08 PM, Laszlo Ersek wrote:
> On 10/12/18 09:29, Ard Biesheuvel wrote:
>> Hello all,
>>
>> While grepping through the code in edk2-platforms, I noticed an issue
>> with this commit. Apologies for not spotting it earlier.
>>
>> On 31 August 2018 at 15:27, Ming Huang <ming.huang@linaro.org> wrote:
>>> PciHostBridgeLib which is need by PciHostBridgeDxe, provide
>>> root bridges and deal with resource conflict.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ming Huang <ming.huang@linaro.org>
>>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>> ---
>>>  Platform/Hisilicon/D06/D06.dsc                                       |   2 +-
>>>  Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.inf |  36 ++
>>>  Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.c   | 635 ++++++++++++++++++++
>>>  3 files changed, 672 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Platform/Hisilicon/D06/D06.dsc b/Platform/Hisilicon/D06/D06.dsc
>>> index 2659cb7e37..83dcbab6c4 100644
>>> --- a/Platform/Hisilicon/D06/D06.dsc
>>> +++ b/Platform/Hisilicon/D06/D06.dsc
>>> @@ -419,7 +419,7 @@
>>>      <LibraryClasses>
>>>        PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
>>>        PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
>>> -      PciHostBridgeLib|MdeModulePkg/Library/PciHostBridgeLibNull/PciHostBridgeLibNull.inf
>>> +      PciHostBridgeLib|Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>>>    }
>>>
>>>    MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>>> diff --git a/Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>>> new file mode 100644
>>> index 0000000000..8a998681a3
>>> --- /dev/null
>>> +++ b/Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>>> @@ -0,0 +1,36 @@
>>> +## @file
>>> +#
>>> +#  Copyright (c) 2018, Hisilicon Limited. All rights reserved.<BR>
>>> +#  Copyright (c) 2018, Linaro Limited. All rights reserved.<BR>
>>> +#
>>> +#  This program and the accompanying materials
>>> +#  are licensed and made available under the terms and conditions of the BSD License
>>> +#  which accompanies this distribution. The full text of the license may be found at
>>> +#  http://opensource.org/licenses/bsd-license.php
>>> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>>> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>> +#
>>> +#
>>> +##
>>> +
>>> +[Defines]
>>> +  INF_VERSION                    = 0x0001001A
>>> +  BASE_NAME                      = PciHostBridgeLib
>>> +  FILE_GUID                      = 61b7276a-fc67-11e5-82fd-47ea9896dd5d
>>> +  MODULE_TYPE                    = DXE_DRIVER
>>> +  VERSION_STRING                 = 1.0
>>> +  LIBRARY_CLASS                  = PciHostBridgeLib|DXE_DRIVER
>>> +
>>> +[Sources]
>>> +  PciHostBridgeLib.c
>>> +
>>> +[Packages]
>>> +  MdeModulePkg/MdeModulePkg.dec
>>> +  MdePkg/MdePkg.dec
>>> +
>>> +[LibraryClasses]
>>> +  BaseLib
>>> +  DebugLib
>>> +  DevicePathLib
>>> +  MemoryAllocationLib
>>> +  UefiBootServicesTableLib
>>> diff --git a/Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.c b/Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.c
>>> new file mode 100644
>>> index 0000000000..d1a436d9bc
>>> --- /dev/null
>>> +++ b/Platform/Hisilicon/D06/Library/PciHostBridgeLib/PciHostBridgeLib.c
>>> @@ -0,0 +1,635 @@
>>> +/** @file
>>> +
>>> +  Copyright (c) 2018, Hisilicon Limited. All rights reserved.<BR>
>>> +  Copyright (c) 2018, Linaro Limited. All rights reserved.<BR>
>>> +
>>> +  This program and the accompanying materials
>>> +  are licensed and made available under the terms and conditions of the BSD License
>>> +  which accompanies this distribution.  The full text of the license may be found at
>>> +  http://opensource.org/licenses/bsd-license.php
>>> +
>>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>> +
>>> +**/
>>> +#include <PiDxe.h>
>>> +#include <Library/DebugLib.h>
>>> +#include <Library/DevicePathLib.h>
>>> +#include <Library/MemoryAllocationLib.h>
>>> +#include <Library/PciHostBridgeLib.h>
>>> +#include <Protocol/PciHostBridgeResourceAllocation.h>
>>> +#include <Protocol/PciRootBridgeIo.h>
>>> +
>>> +#define ENUM_HB_NUM 8
>>> +
>>> +#define EFI_PCI_SUPPORT   (EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO | \
>>> +                           EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO | \
>>> +                           EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | \
>>> +                           EFI_PCI_ATTRIBUTE_ISA_IO_16  | \
>>> +                           EFI_PCI_ATTRIBUTE_VGA_MEMORY | \
>>> +                           EFI_PCI_ATTRIBUTE_VGA_IO_16  | \
>>> +                           EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16)
>>> +
>>> +#define EFI_PCI_ATTRIBUTE  EFI_PCI_SUPPORT
>>> +
>>> +#pragma pack(1)
>>> +typedef struct {
>>> +  ACPI_HID_DEVICE_PATH     AcpiDevicePath;
>>> +  EFI_DEVICE_PATH_PROTOCOL EndDevicePath;
>>> +} EFI_PCI_ROOT_BRIDGE_DEVICE_PATH;
>>> +#pragma pack ()
>>> +
>>> +STATIC EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath [ENUM_HB_NUM] = {
>>> +//Host Bridge 0
>>> +  {
>>> +    {
>>> +      {
>>> +        ACPI_DEVICE_PATH,
>>> +        ACPI_DP,
>>> +        {
>>> +          (UINT8)sizeof (ACPI_HID_DEVICE_PATH),
>>> +          (UINT8)(sizeof (ACPI_HID_DEVICE_PATH) >> 8)
>>> +        }
>>> +      },
>>> +      EISA_PNP_ID(0x0A03), // PCI
>>> +      0
>> ...
>>> +//Host Bridge 2
>> ...
>>> +      EISA_PNP_ID(0x0A04), // PCI
>>> +      0
>> ..
>>> +//Host Bridge 4
>> ...
>>> +      EISA_PNP_ID(0x0A05), // PCI
>>> +      0
>> ...
>>> +//Host Bridge 5
>> ...
>>> +      EISA_PNP_ID(0x0A06), // PCI
>>> +      0
>> ...
>>> +//Host Bridge 6
>> ...
>>> +      EISA_PNP_ID(0x0A07), // PCI
>>> +      0
>> ...
>>> +//Host Bridge 8
>> ...
>>> +      EISA_PNP_ID(0x0A08), // PCI
>>> +      0
>> ...
>>> +//Host Bridge 10
>> ...
>>> +      EISA_PNP_ID(0x0A09), // PCI
>>> +      0
>> ...
>>> +//Host Bridge 11
>> ...
>>> +      EISA_PNP_ID(0x0A0A), // PCI
>>> +      0
>>
>> This is *not* how it works. You cannot invent your own ACPI HIDs like
>> that. If you have multiple instances of a device, you increment the
>> UID.
> 
> I agree.
> 
>> Please synchronize these definitions with the HIDs/UIDs used in the
>> DSDT/SSDT for PCIe. And please make sure all host bridges are
>> accounted for.
> 
> I agree again; this is the cleanest approach, by the book.
> 
> (
> 
> In order to be completely honest, I have to point out that we're
> currently cutting some corners on the PciRoot() / PcieRoot() difference,
> in ArmVirtQemu. (Which translates to a PNPID difference, between 0x0A03
> / 0x0A08.) Please refer to the following discussions (search them for
> "EISA_PNP_ID"):
> 
> - http://mid.mail-archive.com/8ba58ec8-9360-4805-c9a5-b9d5c193fdb0@redhat.com
> - http://mid.mail-archive.com/b87c8a0e-e4c9-f9d3-3d53-f7bb1e27cc1c@redhat.com
> - http://mid.mail-archive.com/1472840159-28957-4-git-send-email-ard.biesheuvel@linaro.org
>   (and this was pushed ultimately)
> 
> )
> 
> Thanks
> Laszlo
>