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.hu...@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.hu...@linaro.org> >>> Reviewed-by: Leif Lindholm <leif.lindh...@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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
