Hi,
I send v3 of PciEmulation, which base on NonDiscoverablePciDeviceDxe driver: [PATCH v2 0/5] MdeModulePkg: add support for non-discoverable devices
Code is also available in the github: https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/opp-...
According to review comments, PCD usage is improved, as well as redundant includes or packages were removed.
We still do not have boards with new SATA IP, but if this version of PciEmulation is accepted, let's keep it in hand until NonDiscoverablePciDeviceDxe gets merged or sort out some solution in the end of November.
I'm looking forward to your review.
Best regards, Marcin
Changelog v2 -> v3
* Use static globals for PCD-related variables * Check with 'if' in runtime if they are correct * Return error on each kind of failure * Don't use arrays of DmaTypes/DevTypes for registering NonDiscoverableDevices * Remove redundant protocols, includes, libraries * Add reviewed-by in patches 2-6
v1 -> v2
* Move to NonDiscoverablePciDeviceDxe
Jan Dąbroś (4): Platforms/Marvell: Enable PciEmulation driver for Armada70x0 platform Platforms/Marvell: Enable USB stack for Armada70x0 platform Platforms/Marvell: Enable two xHCI ports for Armada70x0 board Platforms/Marvell: Enable SATA stack for Armada70x0 platform
Marcin Wojtas (2): Platforms/Marvell: Add PciEmulation driver Platforms/Marvell: Enable SATA port for Armada70x0 board
.../Marvell/PortingGuide/PciEmulation.txt | 46 ++++++++ Platforms/Marvell/Armada/Armada.dsc.inc | 19 ++++ Platforms/Marvell/Armada/Armada70x0.dsc | 10 ++ Platforms/Marvell/Armada/Armada70x0.fdf | 18 ++++ Platforms/Marvell/Marvell.dec | 6 ++ Platforms/Marvell/PciEmulation/PciEmulation.c | 119 +++++++++++++++++++++ Platforms/Marvell/PciEmulation/PciEmulation.inf | 46 ++++++++ 7 files changed, 264 insertions(+) create mode 100644 Documentation/Marvell/PortingGuide/PciEmulation.txt create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.c create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.inf
In order to use numerous UEFI drivers based on PCI bus, PciEmulation driver is implemented. It enables proper registration of platform devices as NonDiscoverableDevices and use generic EDK2 PciEmulation solution.
Configuration and setting amount of devices is enabled via PCD.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Marcin Wojtas mw@semihalf.com Signed-off-by: Jan Dabros jsd@semihalf.com --- .../Marvell/PortingGuide/PciEmulation.txt | 46 ++++++++ Platforms/Marvell/Marvell.dec | 6 ++ Platforms/Marvell/PciEmulation/PciEmulation.c | 119 +++++++++++++++++++++ Platforms/Marvell/PciEmulation/PciEmulation.inf | 46 ++++++++ 4 files changed, 217 insertions(+) create mode 100644 Documentation/Marvell/PortingGuide/PciEmulation.txt create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.c create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.inf
diff --git a/Documentation/Marvell/PortingGuide/PciEmulation.txt b/Documentation/Marvell/PortingGuide/PciEmulation.txt new file mode 100644 index 0000000..14dc9dc --- /dev/null +++ b/Documentation/Marvell/PortingGuide/PciEmulation.txt @@ -0,0 +1,46 @@ +PciEmulation configuration +-------------------------- +Installation of various NonDiscoverable devices via PciEmulation driver is performed +via set of PCDs. Following are available: + + gMarvellTokenSpaceGuid.PcdPciEDevCount + +Indicates amount of used devices. + +Below PCD is an unicode string format, containing settings for all devices +separated with semicolon. It indicates base address of register space: + + gMarvellTokenSpaceGuid.PcdPciEDevBaseAddress + +Devices' types are represented in an array comprising hexadecimal values: + + gMarvellTokenSpaceGuid.PcdPciEDevType + +They correspond to DEV_TYPE enum: + typedef enum { + 0 DEV_XHCI, + 1 DEV_AHCI, + 2 DEV_SDHCI, + 3 DEV_MAX + } DEV_TYPE; + +Devices' DMA types are represented in an array comprising hexadecimal values: + + gMarvellTokenSpaceGuid.PcdPciEDmaType + +They correspond to DMA_TYPE enum: + typedef enum { + 0 DMA_COHERENT, + 1 DMA_NONCOHERENT, + 2 DMA_TYPE_MAX + } DMA_TYPE; + +Examples +-------- +Assuming that there are two XHCI controllers with register space starting at +0xF2500000 and 0xF2510000, with coherent DMA, following PCD values should be set: + + gMarvellTokenSpaceGuid.PcdPciEDevCount|2 + gMarvellTokenSpaceGuid.PcdPciEDevBaseAddress|L"0xF2500000;0xF2510000" + gMarvellTokenSpaceGuid.PcdPciEDevType|{ 0x0, 0x0 } + gMarvellTokenSpaceGuid.PcdPciEDmaType|{ 0x0, 0x0 } diff --git a/Platforms/Marvell/Marvell.dec b/Platforms/Marvell/Marvell.dec index db99230..13fffbb 100644 --- a/Platforms/Marvell/Marvell.dec +++ b/Platforms/Marvell/Marvell.dec @@ -209,6 +209,12 @@ gMarvellTokenSpaceGuid.PcdPp2XlgBaseAddress|0|UINT64|0x3000031 gMarvellTokenSpaceGuid.PcdPp2XlgDevSize|0|UINT32|0x3000032
+#PciEmulation + gMarvellTokenSpaceGuid.PcdPciEDevBaseAddress|{ 0x0 }|VOID*|0x30000058 + gMarvellTokenSpaceGuid.PcdPciEDevCount|0|UINT32|0x30000059 + gMarvellTokenSpaceGuid.PcdPciEDevType|{ 0x0 }|VOID*|0x3000005A + gMarvellTokenSpaceGuid.PcdPciEDmaType|{ 0x0 }|VOID*|0x3000005B + #ResetLib gMarvellTokenSpaceGuid.PcdResetRegAddress|0|UINT64|0x40000050 gMarvellTokenSpaceGuid.PcdResetRegMask|0|UINT32|0x4000051 diff --git a/Platforms/Marvell/PciEmulation/PciEmulation.c b/Platforms/Marvell/PciEmulation/PciEmulation.c new file mode 100644 index 0000000..00553b8 --- /dev/null +++ b/Platforms/Marvell/PciEmulation/PciEmulation.c @@ -0,0 +1,119 @@ +/** @file + + Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR> + Copyright (c) 2016, Marvell. All rights reserved. + + 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/NonDiscoverableDeviceRegistrationLib.h> +#include <Library/ParsePcdLib.h> +#include <Library/UefiBootServicesTableLib.h> + +#include <Protocol/EmbeddedExternalDevice.h> + +typedef enum { + DEV_XHCI, + DEV_AHCI, + DEV_SDHCI, + DEV_MAX +} DEV_TYPE; + +typedef enum { + DMA_COHERENT, + DMA_NONCOHERENT, + DMA_TYPE_MAX, +} DMA_TYPE; + +STATIC CONST UINT8 DevCount = FixedPcdGet8 (PcdPciEDevCount); +STATIC UINT8 * CONST DeviceTypeTable = FixedPcdGetPtr (PcdPciEDevType); +STATIC UINT8 * CONST DmaTypeTable = FixedPcdGetPtr (PcdPciEDmaType); + +// +// Below function is used to parse devices information from PCD strings. +// Once obtained, the resources are used for registration of +// NonDiscoverable devices. +// +EFI_STATUS +EFIAPI +PciEmulationEntryPoint ( + IN EFI_HANDLE ImageHandle, + IN EFI_SYSTEM_TABLE *SystemTable + ) +{ + EFI_STATUS Status; + UINT8 i; + UINTN BaseAddrTable[DevCount]; + NON_DISCOVERABLE_DEVICE_TYPE DeviceType; + NON_DISCOVERABLE_DEVICE_DMA_TYPE DmaType; + + if (PcdGetSize (PcdPciEDevType) != DevCount) { + DEBUG((DEBUG_ERROR, "PciEmulation: Wrong PcdPciEDevType format\n")); + return EFI_INVALID_PARAMETER; + } + + if (PcdGetSize (PcdPciEDmaType) != DevCount) { + DEBUG((DEBUG_ERROR, "PciEmulation: Wrong PcdPciEDmaType format\n")); + return EFI_INVALID_PARAMETER; + } + + Status = ParsePcdString ((CHAR16 *) PcdGetPtr (PcdPciEDevBaseAddress), DevCount, BaseAddrTable, NULL); + if (EFI_ERROR(Status)) { + DEBUG((DEBUG_ERROR, "PciEmulation: Wrong PcdPciEDevBaseAddress format\n")); + return EFI_INVALID_PARAMETER; + } + + for (i = 0; i < DevCount; i++) { + switch (DeviceTypeTable[i]) { + case DEV_XHCI: + DeviceType = NonDiscoverableDeviceTypeXhci; + break; + case DEV_AHCI: + DeviceType = NonDiscoverableDeviceTypeAhci; + break; + case DEV_SDHCI: + DeviceType = NonDiscoverableDeviceTypeSdhci; + break; + default: + DEBUG((DEBUG_ERROR, "PciEmulation: Unsupported device type with ID=%d\n", i)); + return EFI_INVALID_PARAMETER; + } + + switch (DmaTypeTable[i]) { + case DMA_COHERENT: + DmaType = NonDiscoverableDeviceDmaTypeCoherent; + break; + case DMA_NONCOHERENT: + DmaType = NonDiscoverableDeviceDmaTypeNonCoherent; + break; + default: + DEBUG((DEBUG_ERROR, "PciEmulation: Unsupported DMA type with ID=%d\n", i)); + return EFI_INVALID_PARAMETER; + } + + Status = RegisterNonDiscoverableDevice ( + BaseAddrTable[i], + DeviceType, + DmaType, + NULL, + NULL + ); + + if (EFI_ERROR(Status)) { + DEBUG((DEBUG_ERROR, "PciEmulation: Cannot install device with ID=%d\n", i)); + return Status; + } + } + + return EFI_SUCCESS; +} diff --git a/Platforms/Marvell/PciEmulation/PciEmulation.inf b/Platforms/Marvell/PciEmulation/PciEmulation.inf new file mode 100644 index 0000000..75450a3 --- /dev/null +++ b/Platforms/Marvell/PciEmulation/PciEmulation.inf @@ -0,0 +1,46 @@ +/** @file + + Copyright (c) 2009, Apple Inc. All rights reserved.<BR> + Copyright (c) 2016, Marvell. All rights reserved. + + 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 = 0x00010019 + BASE_NAME = PciEmulation + FILE_GUID = 3dfa08da-923b-4841-9435-c77a604d7493 + MODULE_TYPE = DXE_DRIVER + VERSION_STRING = 1.0 + + ENTRY_POINT = PciEmulationEntryPoint + +[Sources.common] + PciEmulation.c + +[Packages] + EmbeddedPkg/EmbeddedPkg.dec + MdeModulePkg/MdeModulePkg.dec + MdePkg/MdePkg.dec + OpenPlatformPkg/Platforms/Marvell/Marvell.dec + +[LibraryClasses] + NonDiscoverableDeviceRegistrationLib + ParsePcdLib + UefiDriverEntryPoint + +[Pcd] + gMarvellTokenSpaceGuid.PcdPciEDevBaseAddress + gMarvellTokenSpaceGuid.PcdPciEDevCount + gMarvellTokenSpaceGuid.PcdPciEDevType + gMarvellTokenSpaceGuid.PcdPciEDmaType + +[Depex] + TRUE
From: Jan Dąbroś jsd@semihalf.com
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jan Dabros jsd@semihalf.com Signed-off-by: Marcin Wojtas mw@semihalf.com Reviewed-by: Ard Biesheuvel ard.biesheuvel@linaro.org --- Platforms/Marvell/Armada/Armada.dsc.inc | 4 ++++ Platforms/Marvell/Armada/Armada70x0.fdf | 4 ++++ 2 files changed, 8 insertions(+)
diff --git a/Platforms/Marvell/Armada/Armada.dsc.inc b/Platforms/Marvell/Armada/Armada.dsc.inc index 76c5d6d..3ebee0d 100644 --- a/Platforms/Marvell/Armada/Armada.dsc.inc +++ b/Platforms/Marvell/Armada/Armada.dsc.inc @@ -170,6 +170,7 @@ SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf + NonDiscoverableDeviceRegistrationLib|MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf
[LibraryClasses.common.UEFI_APPLICATION] UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.inf @@ -417,6 +418,9 @@ EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf
+ OpenPlatformPkg/Platforms/Marvell/PciEmulation/PciEmulation.inf + MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf + # Console packages MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf diff --git a/Platforms/Marvell/Armada/Armada70x0.fdf b/Platforms/Marvell/Armada/Armada70x0.fdf index 64c3440..2403b71 100644 --- a/Platforms/Marvell/Armada/Armada70x0.fdf +++ b/Platforms/Marvell/Armada/Armada70x0.fdf @@ -123,6 +123,10 @@ FvNameGuid = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c INF OpenPlatformPkg/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.inf INF OpenPlatformPkg/Drivers/Net/Pp2Dxe/Pp2Dxe.inf
+ # PciEmulation + INF OpenPlatformPkg/Platforms/Marvell/PciEmulation/PciEmulation.inf + INF MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf + # Multiple Console IO support INF MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf INF MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf
From: Jan Dąbroś jsd@semihalf.com
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jan Dabros jsd@semihalf.com Signed-off-by: Marcin Wojtas mw@semihalf.com Reviewed-by: Ard Biesheuvel ard.biesheuvel@linaro.org --- Platforms/Marvell/Armada/Armada.dsc.inc | 5 +++++ Platforms/Marvell/Armada/Armada70x0.fdf | 5 +++++ 2 files changed, 10 insertions(+)
diff --git a/Platforms/Marvell/Armada/Armada.dsc.inc b/Platforms/Marvell/Armada/Armada.dsc.inc index 3ebee0d..470cf81 100644 --- a/Platforms/Marvell/Armada/Armada.dsc.inc +++ b/Platforms/Marvell/Armada/Armada.dsc.inc @@ -421,6 +421,11 @@ OpenPlatformPkg/Platforms/Marvell/PciEmulation/PciEmulation.inf MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
+ # USB + MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf + MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBusDxe.inf + MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf + # Console packages MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf diff --git a/Platforms/Marvell/Armada/Armada70x0.fdf b/Platforms/Marvell/Armada/Armada70x0.fdf index 2403b71..2c4f833 100644 --- a/Platforms/Marvell/Armada/Armada70x0.fdf +++ b/Platforms/Marvell/Armada/Armada70x0.fdf @@ -127,6 +127,11 @@ FvNameGuid = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c INF OpenPlatformPkg/Platforms/Marvell/PciEmulation/PciEmulation.inf INF MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
+ # USB + INF MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf + INF MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBusDxe.inf + INF MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf + # Multiple Console IO support INF MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf INF MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf
From: Jan Dąbroś jsd@semihalf.com
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jan Dabros jsd@semihalf.com Signed-off-by: Marcin Wojtas mw@semihalf.com Reviewed-by: Ard Biesheuvel ard.biesheuvel@linaro.org --- Platforms/Marvell/Armada/Armada70x0.dsc | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/Platforms/Marvell/Armada/Armada70x0.dsc b/Platforms/Marvell/Armada/Armada70x0.dsc index 2f2b278..5e07161 100644 --- a/Platforms/Marvell/Armada/Armada70x0.dsc +++ b/Platforms/Marvell/Armada/Armada70x0.dsc @@ -139,6 +139,13 @@ gMarvellTokenSpaceGuid.PcdPp2XlgBaseAddress|0xf2130f00 gMarvellTokenSpaceGuid.PcdPp2XlgDevSize|0x1000
+ #PciEmulation + gMarvellTokenSpaceGuid.PcdPciEDevCount|2 + ## XHCI1 XHCI2 + gMarvellTokenSpaceGuid.PcdPciEDevBaseAddress|L"0xF2500000;0xF2510000" + gMarvellTokenSpaceGuid.PcdPciEDevType|{ 0x0, 0x0 } + gMarvellTokenSpaceGuid.PcdPciEDmaType|{ 0x0, 0x0 } + #ResetLib gMarvellTokenSpaceGuid.PcdResetRegAddress|0xf06f0084 gMarvellTokenSpaceGuid.PcdResetRegMask|0x1
From: Jan Dąbroś jsd@semihalf.com
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jan Dabros jsd@semihalf.com Signed-off-by: Marcin Wojtas mw@semihalf.com Reviewed-by: Ard Biesheuvel ard.biesheuvel@linaro.org --- Platforms/Marvell/Armada/Armada.dsc.inc | 10 ++++++++++ Platforms/Marvell/Armada/Armada70x0.fdf | 9 +++++++++ 2 files changed, 19 insertions(+)
diff --git a/Platforms/Marvell/Armada/Armada.dsc.inc b/Platforms/Marvell/Armada/Armada.dsc.inc index 470cf81..0b1c50d 100644 --- a/Platforms/Marvell/Armada/Armada.dsc.inc +++ b/Platforms/Marvell/Armada/Armada.dsc.inc @@ -82,6 +82,7 @@ ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf + UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
# Serial port libraries SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf @@ -421,6 +422,15 @@ OpenPlatformPkg/Platforms/Marvell/PciEmulation/PciEmulation.inf MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
+ # SCSI + MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf + MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf + + # SATA + MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf + MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf + OvmfPkg/SataControllerDxe/SataControllerDxe.inf + # USB MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBusDxe.inf diff --git a/Platforms/Marvell/Armada/Armada70x0.fdf b/Platforms/Marvell/Armada/Armada70x0.fdf index 2c4f833..b420153 100644 --- a/Platforms/Marvell/Armada/Armada70x0.fdf +++ b/Platforms/Marvell/Armada/Armada70x0.fdf @@ -127,6 +127,15 @@ FvNameGuid = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c INF OpenPlatformPkg/Platforms/Marvell/PciEmulation/PciEmulation.inf INF MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
+ # SCSI + INF MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf + INF MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf + + # SATA + INF MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf + INF MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf + INF OvmfPkg/SataControllerDxe/SataControllerDxe.inf + # USB INF MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf INF MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBusDxe.inf
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jan Dabros jsd@semihalf.com Signed-off-by: Marcin Wojtas mw@semihalf.com Reviewed-by: Ard Biesheuvel ard.biesheuvel@linaro.org --- Platforms/Marvell/Armada/Armada70x0.dsc | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/Platforms/Marvell/Armada/Armada70x0.dsc b/Platforms/Marvell/Armada/Armada70x0.dsc index 5e07161..60a86a0 100644 --- a/Platforms/Marvell/Armada/Armada70x0.dsc +++ b/Platforms/Marvell/Armada/Armada70x0.dsc @@ -140,12 +140,15 @@ gMarvellTokenSpaceGuid.PcdPp2XlgDevSize|0x1000
#PciEmulation - gMarvellTokenSpaceGuid.PcdPciEDevCount|2 - ## XHCI1 XHCI2 - gMarvellTokenSpaceGuid.PcdPciEDevBaseAddress|L"0xF2500000;0xF2510000" - gMarvellTokenSpaceGuid.PcdPciEDevType|{ 0x0, 0x0 } - gMarvellTokenSpaceGuid.PcdPciEDmaType|{ 0x0, 0x0 } + gMarvellTokenSpaceGuid.PcdPciEDevCount|3 + ## XHCI1 XHCI2 SATA + gMarvellTokenSpaceGuid.PcdPciEDevBaseAddress|L"0xF2500000;0xF2510000;0xF2540000" + gMarvellTokenSpaceGuid.PcdPciEDevType|{ 0x0, 0x0, 0x1 } + gMarvellTokenSpaceGuid.PcdPciEDmaType|{ 0x0, 0x0, 0x0 }
#ResetLib gMarvellTokenSpaceGuid.PcdResetRegAddress|0xf06f0084 gMarvellTokenSpaceGuid.PcdResetRegMask|0x1 + + #SATA + gMarvellTokenSpaceGuid.PcdSataBaseAddress|0xF2540000
So, I have no strong objection to this set, but I do have a question/comment:
The semicolon-separated-arrays-in-Pcds mechanism is now by far the clunkiest aspect of this set.
I can well see a near future in which we add a core library to consume a static struct from a platform library and do the registration on. At that point, I would like this code to move over to that.
So is there any way we could delete all the Pcd juggling, stick a small static struct and a much simpler iterator into Armada70x0Lib from day one instead?
Regards,
Leif
On Tue, Nov 15, 2016 at 05:29:56PM +0100, Marcin Wojtas wrote:
Hi,
I send v3 of PciEmulation, which base on NonDiscoverablePciDeviceDxe driver: [PATCH v2 0/5] MdeModulePkg: add support for non-discoverable devices
Code is also available in the github: https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/opp-...
According to review comments, PCD usage is improved, as well as redundant includes or packages were removed.
We still do not have boards with new SATA IP, but if this version of PciEmulation is accepted, let's keep it in hand until NonDiscoverablePciDeviceDxe gets merged or sort out some solution in the end of November.
I'm looking forward to your review.
Best regards, Marcin
Changelog v2 -> v3
- Use static globals for PCD-related variables
- Check with 'if' in runtime if they are correct
- Return error on each kind of failure
- Don't use arrays of DmaTypes/DevTypes for registering NonDiscoverableDevices
- Remove redundant protocols, includes, libraries
- Add reviewed-by in patches 2-6
v1 -> v2
- Move to NonDiscoverablePciDeviceDxe
Jan Dąbroś (4): Platforms/Marvell: Enable PciEmulation driver for Armada70x0 platform Platforms/Marvell: Enable USB stack for Armada70x0 platform Platforms/Marvell: Enable two xHCI ports for Armada70x0 board Platforms/Marvell: Enable SATA stack for Armada70x0 platform
Marcin Wojtas (2): Platforms/Marvell: Add PciEmulation driver Platforms/Marvell: Enable SATA port for Armada70x0 board
.../Marvell/PortingGuide/PciEmulation.txt | 46 ++++++++ Platforms/Marvell/Armada/Armada.dsc.inc | 19 ++++ Platforms/Marvell/Armada/Armada70x0.dsc | 10 ++ Platforms/Marvell/Armada/Armada70x0.fdf | 18 ++++ Platforms/Marvell/Marvell.dec | 6 ++ Platforms/Marvell/PciEmulation/PciEmulation.c | 119 +++++++++++++++++++++ Platforms/Marvell/PciEmulation/PciEmulation.inf | 46 ++++++++ 7 files changed, 264 insertions(+) create mode 100644 Documentation/Marvell/PortingGuide/PciEmulation.txt create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.c create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.inf
-- 1.8.3.1
Hi Leif,
How about this for now: 1. In PciEmulation: - Static struct comprising all possible Armada 70x0 XHCI/AHCI/SDHCI base addresses + dev count of each + information if they are dma-coherent. - Iterate for each device type separately
2. In dsc - for each device type separate PCD, e.g. for USB: gMarvellTokenSpaceGuid.PcdPciEXhci|{ 0x0, 0x1 } It means that port0 is disabled and port1 - enabled.
Imo it would be flexible for various boards, also easy for next possible SoC supported in OPP - Armada 80x0 can fit into it, as it's a superset of A70x0. Actually I think in the structure there could be even more SoC's described this way.
Please let me know your opinion.
Best regards, Marcin
2016-11-15 22:36 GMT+01:00 Leif Lindholm leif.lindholm@linaro.org:
So, I have no strong objection to this set, but I do have a question/comment:
The semicolon-separated-arrays-in-Pcds mechanism is now by far the clunkiest aspect of this set.
I can well see a near future in which we add a core library to consume a static struct from a platform library and do the registration on. At that point, I would like this code to move over to that.
So is there any way we could delete all the Pcd juggling, stick a small static struct and a much simpler iterator into Armada70x0Lib from day one instead?
Regards,
Leif
On Tue, Nov 15, 2016 at 05:29:56PM +0100, Marcin Wojtas wrote:
Hi,
I send v3 of PciEmulation, which base on NonDiscoverablePciDeviceDxe driver: [PATCH v2 0/5] MdeModulePkg: add support for non-discoverable devices
Code is also available in the github: https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/opp-...
According to review comments, PCD usage is improved, as well as redundant includes or packages were removed.
We still do not have boards with new SATA IP, but if this version of PciEmulation is accepted, let's keep it in hand until NonDiscoverablePciDeviceDxe gets merged or sort out some solution in the end of November.
I'm looking forward to your review.
Best regards, Marcin
Changelog v2 -> v3
- Use static globals for PCD-related variables
- Check with 'if' in runtime if they are correct
- Return error on each kind of failure
- Don't use arrays of DmaTypes/DevTypes for registering NonDiscoverableDevices
- Remove redundant protocols, includes, libraries
- Add reviewed-by in patches 2-6
v1 -> v2
- Move to NonDiscoverablePciDeviceDxe
Jan Dąbroś (4): Platforms/Marvell: Enable PciEmulation driver for Armada70x0 platform Platforms/Marvell: Enable USB stack for Armada70x0 platform Platforms/Marvell: Enable two xHCI ports for Armada70x0 board Platforms/Marvell: Enable SATA stack for Armada70x0 platform
Marcin Wojtas (2): Platforms/Marvell: Add PciEmulation driver Platforms/Marvell: Enable SATA port for Armada70x0 board
.../Marvell/PortingGuide/PciEmulation.txt | 46 ++++++++ Platforms/Marvell/Armada/Armada.dsc.inc | 19 ++++ Platforms/Marvell/Armada/Armada70x0.dsc | 10 ++ Platforms/Marvell/Armada/Armada70x0.fdf | 18 ++++ Platforms/Marvell/Marvell.dec | 6 ++ Platforms/Marvell/PciEmulation/PciEmulation.c | 119 +++++++++++++++++++++ Platforms/Marvell/PciEmulation/PciEmulation.inf | 46 ++++++++ 7 files changed, 264 insertions(+) create mode 100644 Documentation/Marvell/PortingGuide/PciEmulation.txt create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.c create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.inf
-- 1.8.3.1
I think this is where it would have been useful to separate platform and SoC (which really only Hisilicon have done so far). And that's not a criticism of your port (hey, I've been reviewing it all the way, we're all learning here).
At which point it's probably better to merge something clunky, rather than trying to improve it incrementally.
So, long-term, what I think it would make sense to move towards would be: - An SoC library(/driver) (Armada70x0) - Defining all of the possible components and possible base addresses and dev count and coherency information. - A .dec containing per-port boolean Pcds. - A platform port that only sets the Pcds to enable the components available to it.
But for this pass, let's just go with what we have: Reviewed-by: Leif Lindholm leif.lindholm@linaro.org
(But we're waiting for Ard's next spin, with a version to go into OpenPlatformPkg, and you verifying that it works as expected.)
/ Leif
On Tue, Nov 15, 2016 at 11:04:32PM +0100, Marcin Wojtas wrote:
Hi Leif,
How about this for now:
- In PciEmulation:
- Static struct comprising all possible Armada 70x0 XHCI/AHCI/SDHCI
base addresses + dev count of each + information if they are dma-coherent.
- Iterate for each device type separately
- In dsc - for each device type separate PCD, e.g. for USB:
gMarvellTokenSpaceGuid.PcdPciEXhci|{ 0x0, 0x1 } It means that port0 is disabled and port1 - enabled.
Imo it would be flexible for various boards, also easy for next possible SoC supported in OPP - Armada 80x0 can fit into it, as it's a superset of A70x0. Actually I think in the structure there could be even more SoC's described this way.
Please let me know your opinion.
Best regards, Marcin
2016-11-15 22:36 GMT+01:00 Leif Lindholm leif.lindholm@linaro.org:
So, I have no strong objection to this set, but I do have a question/comment:
The semicolon-separated-arrays-in-Pcds mechanism is now by far the clunkiest aspect of this set.
I can well see a near future in which we add a core library to consume a static struct from a platform library and do the registration on. At that point, I would like this code to move over to that.
So is there any way we could delete all the Pcd juggling, stick a small static struct and a much simpler iterator into Armada70x0Lib from day one instead?
Regards,
Leif
On Tue, Nov 15, 2016 at 05:29:56PM +0100, Marcin Wojtas wrote:
Hi,
I send v3 of PciEmulation, which base on NonDiscoverablePciDeviceDxe driver: [PATCH v2 0/5] MdeModulePkg: add support for non-discoverable devices
Code is also available in the github: https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/opp-...
According to review comments, PCD usage is improved, as well as redundant includes or packages were removed.
We still do not have boards with new SATA IP, but if this version of PciEmulation is accepted, let's keep it in hand until NonDiscoverablePciDeviceDxe gets merged or sort out some solution in the end of November.
I'm looking forward to your review.
Best regards, Marcin
Changelog v2 -> v3
- Use static globals for PCD-related variables
- Check with 'if' in runtime if they are correct
- Return error on each kind of failure
- Don't use arrays of DmaTypes/DevTypes for registering NonDiscoverableDevices
- Remove redundant protocols, includes, libraries
- Add reviewed-by in patches 2-6
v1 -> v2
- Move to NonDiscoverablePciDeviceDxe
Jan Dąbroś (4): Platforms/Marvell: Enable PciEmulation driver for Armada70x0 platform Platforms/Marvell: Enable USB stack for Armada70x0 platform Platforms/Marvell: Enable two xHCI ports for Armada70x0 board Platforms/Marvell: Enable SATA stack for Armada70x0 platform
Marcin Wojtas (2): Platforms/Marvell: Add PciEmulation driver Platforms/Marvell: Enable SATA port for Armada70x0 board
.../Marvell/PortingGuide/PciEmulation.txt | 46 ++++++++ Platforms/Marvell/Armada/Armada.dsc.inc | 19 ++++ Platforms/Marvell/Armada/Armada70x0.dsc | 10 ++ Platforms/Marvell/Armada/Armada70x0.fdf | 18 ++++ Platforms/Marvell/Marvell.dec | 6 ++ Platforms/Marvell/PciEmulation/PciEmulation.c | 119 +++++++++++++++++++++ Platforms/Marvell/PciEmulation/PciEmulation.inf | 46 ++++++++ 7 files changed, 264 insertions(+) create mode 100644 Documentation/Marvell/PortingGuide/PciEmulation.txt create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.c create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.inf
-- 1.8.3.1
Hi Leif,
Thanks. I'd be happy to do it after November, once the pressure gets smaller. Can you please point me the exact soltion example from Hisilicon?
As for the status, still to go: - verify sata with new silicon (the board is still on its way to our office) - it is supposed to be quirkless, I keep fingers crossed it will work with generic driver - submit sd/mmc driver- Jan is preparing patchset at the moment - try fupdate with ShellDynamic protocol
And that's all. We'll be in touch.
Thanks, Marcin
-----Wiadomość oryginalna----- Od: "Leif Lindholm" leif.lindholm@linaro.org Wysłano: 2016-11-16 10:00 Do: "Marcin Wojtas" mw@semihalf.com DW: "Linaro UEFI Mailman List" linaro-uefi@lists.linaro.org; "Ard Biesheuvel" ard.biesheuvel@linaro.org; "Neta Zur Hershkovits" neta@marvell.com; "Yehuda Yitschak" yehuday@marvell.com; "Haim Boot" hayim@marvell.com; "Nitzan Zorea" nzorea@marvell.com; "semihalf-dabros-jan" jsd@semihalf.com; "Rafal Jaworowski" raj@semihalf.com Temat: Re: [PATCH v3 0/6] Armada 7040 PciEmulation, SATA, USB
I think this is where it would have been useful to separate platform and SoC (which really only Hisilicon have done so far). And that's not a criticism of your port (hey, I've been reviewing it all the way, we're all learning here).
At which point it's probably better to merge something clunky, rather than trying to improve it incrementally.
So, long-term, what I think it would make sense to move towards would be: - An SoC library(/driver) (Armada70x0) - Defining all of the possible components and possible base addresses and dev count and coherency information. - A .dec containing per-port boolean Pcds. - A platform port that only sets the Pcds to enable the components available to it.
But for this pass, let's just go with what we have: Reviewed-by: Leif Lindholm leif.lindholm@linaro.org
(But we're waiting for Ard's next spin, with a version to go into OpenPlatformPkg, and you verifying that it works as expected.)
/ Leif
On Tue, Nov 15, 2016 at 11:04:32PM +0100, Marcin Wojtas wrote:
Hi Leif,
How about this for now:
- In PciEmulation:
- Static struct comprising all possible Armada 70x0 XHCI/AHCI/SDHCI
base addresses + dev count of each + information if they are dma-coherent.
- Iterate for each device type separately
- In dsc - for each device type separate PCD, e.g. for USB:
gMarvellTokenSpaceGuid.PcdPciEXhci|{ 0x0, 0x1 } It means that port0 is disabled and port1 - enabled.
Imo it would be flexible for various boards, also easy for next possible SoC supported in OPP - Armada 80x0 can fit into it, as it's a superset of A70x0. Actually I think in the structure there could be even more SoC's described this way.
Please let me know your opinion.
Best regards, Marcin
2016-11-15 22:36 GMT+01:00 Leif Lindholm leif.lindholm@linaro.org:
So, I have no strong objection to this set, but I do have a question/comment:
The semicolon-separated-arrays-in-Pcds mechanism is now by far the clunkiest aspect of this set.
I can well see a near future in which we add a core library to consume a static struct from a platform library and do the registration on. At that point, I would like this code to move over to that.
So is there any way we could delete all the Pcd juggling, stick a small static struct and a much simpler iterator into Armada70x0Lib from day one instead?
Regards,
Leif
On Tue, Nov 15, 2016 at 05:29:56PM +0100, Marcin Wojtas wrote:
Hi,
I send v3 of PciEmulation, which base on NonDiscoverablePciDeviceDxe driver: [PATCH v2 0/5] MdeModulePkg: add support for non-discoverable devices
Code is also available in the github: https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/opp-...
According to review comments, PCD usage is improved, as well as redundant includes or packages were removed.
We still do not have boards with new SATA IP, but if this version of PciEmulation is accepted, let's keep it in hand until NonDiscoverablePciDeviceDxe gets merged or sort out some solution in the end of November.
I'm looking forward to your review.
Best regards, Marcin
Changelog v2 -> v3
- Use static globals for PCD-related variables
- Check with 'if' in runtime if they are correct
- Return error on each kind of failure
- Don't use arrays of DmaTypes/DevTypes for registering NonDiscoverableDevices
- Remove redundant protocols, includes, libraries
- Add reviewed-by in patches 2-6
v1 -> v2
- Move to NonDiscoverablePciDeviceDxe
Jan Dąbroś (4): Platforms/Marvell: Enable PciEmulation driver for Armada70x0 platform Platforms/Marvell: Enable USB stack for Armada70x0 platform Platforms/Marvell: Enable two xHCI ports for Armada70x0 board Platforms/Marvell: Enable SATA stack for Armada70x0 platform
Marcin Wojtas (2): Platforms/Marvell: Add PciEmulation driver Platforms/Marvell: Enable SATA port for Armada70x0 board
.../Marvell/PortingGuide/PciEmulation.txt | 46 ++++++++ Platforms/Marvell/Armada/Armada.dsc.inc | 19 ++++ Platforms/Marvell/Armada/Armada70x0.dsc | 10 ++ Platforms/Marvell/Armada/Armada70x0.fdf | 18 ++++ Platforms/Marvell/Marvell.dec | 6 ++ Platforms/Marvell/PciEmulation/PciEmulation.c | 119 +++++++++++++++++++++ Platforms/Marvell/PciEmulation/PciEmulation.inf | 46 ++++++++ 7 files changed, 264 insertions(+) create mode 100644 Documentation/Marvell/PortingGuide/PciEmulation.txt create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.c create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.inf
-- 1.8.3.1
On Wed, Nov 16, 2016 at 10:37:03AM +0100, marcin wojtas wrote:
Hi Leif,
Thanks. I'd be happy to do it after November, once the pressure gets smaller.
Great, thanks.
Can you please point me the exact soltion example from Hisilicon?
Well, the solution is simply that they split the SoC code apart from the platform code: https://git.linaro.org/uefi/OpenPlatformPkg.git/tree/Chips/Hisilicon vs https://git.linaro.org/uefi/OpenPlatformPkg.git/tree/Platforms/Hisilicon
You guys are still the trailblazers on the non-discoverable side :)
As for the status, still to go:
- verify sata with new silicon (the board is still on its way to our office) - it is supposed to be quirkless, I keep fingers crossed it will work with generic driver
If not, it'd be useful to see the quirk as a standaloen patch, to see what could be do abot integrating it in one of the more reusable solutions.
- submit sd/mmc driver- Jan is preparing patchset at the moment
- try fupdate with ShellDynamic protocol
And that's all. We'll be in touch.
Sounds good - thanks!
/ Leif
Thanks, Marcin
-----Wiadomość oryginalna----- Od: "Leif Lindholm" leif.lindholm@linaro.org Wysłano: 2016-11-16 10:00 Do: "Marcin Wojtas" mw@semihalf.com DW: "Linaro UEFI Mailman List" linaro-uefi@lists.linaro.org; "Ard Biesheuvel" ard.biesheuvel@linaro.org; "Neta Zur Hershkovits" neta@marvell.com; "Yehuda Yitschak" yehuday@marvell.com; "Haim Boot" hayim@marvell.com; "Nitzan Zorea" nzorea@marvell.com; "semihalf-dabros-jan" jsd@semihalf.com; "Rafal Jaworowski" raj@semihalf.com Temat: Re: [PATCH v3 0/6] Armada 7040 PciEmulation, SATA, USB
I think this is where it would have been useful to separate platform and SoC (which really only Hisilicon have done so far). And that's not a criticism of your port (hey, I've been reviewing it all the way, we're all learning here).
At which point it's probably better to merge something clunky, rather than trying to improve it incrementally.
So, long-term, what I think it would make sense to move towards would be:
- An SoC library(/driver) (Armada70x0)
- Defining all of the possible components and possible base addresses and dev count and coherency information.
- A .dec containing per-port boolean Pcds.
- A platform port that only sets the Pcds to enable the components available to it.
But for this pass, let's just go with what we have: Reviewed-by: Leif Lindholm leif.lindholm@linaro.org
(But we're waiting for Ard's next spin, with a version to go into OpenPlatformPkg, and you verifying that it works as expected.)
/ Leif
On Tue, Nov 15, 2016 at 11:04:32PM +0100, Marcin Wojtas wrote:
Hi Leif,
How about this for now:
- In PciEmulation:
- Static struct comprising all possible Armada 70x0 XHCI/AHCI/SDHCI
base addresses + dev count of each + information if they are dma-coherent.
- Iterate for each device type separately
- In dsc - for each device type separate PCD, e.g. for USB:
gMarvellTokenSpaceGuid.PcdPciEXhci|{ 0x0, 0x1 } It means that port0 is disabled and port1 - enabled.
Imo it would be flexible for various boards, also easy for next possible SoC supported in OPP - Armada 80x0 can fit into it, as it's a superset of A70x0. Actually I think in the structure there could be even more SoC's described this way.
Please let me know your opinion.
Best regards, Marcin
2016-11-15 22:36 GMT+01:00 Leif Lindholm leif.lindholm@linaro.org:
So, I have no strong objection to this set, but I do have a question/comment:
The semicolon-separated-arrays-in-Pcds mechanism is now by far the clunkiest aspect of this set.
I can well see a near future in which we add a core library to consume a static struct from a platform library and do the registration on. At that point, I would like this code to move over to that.
So is there any way we could delete all the Pcd juggling, stick a small static struct and a much simpler iterator into Armada70x0Lib from day one instead?
Regards,
Leif
On Tue, Nov 15, 2016 at 05:29:56PM +0100, Marcin Wojtas wrote:
Hi,
I send v3 of PciEmulation, which base on NonDiscoverablePciDeviceDxe driver: [PATCH v2 0/5] MdeModulePkg: add support for non-discoverable devices
Code is also available in the github: https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/opp-...
According to review comments, PCD usage is improved, as well as redundant includes or packages were removed.
We still do not have boards with new SATA IP, but if this version of PciEmulation is accepted, let's keep it in hand until NonDiscoverablePciDeviceDxe gets merged or sort out some solution in the end of November.
I'm looking forward to your review.
Best regards, Marcin
Changelog v2 -> v3
- Use static globals for PCD-related variables
- Check with 'if' in runtime if they are correct
- Return error on each kind of failure
- Don't use arrays of DmaTypes/DevTypes for registering NonDiscoverableDevices
- Remove redundant protocols, includes, libraries
- Add reviewed-by in patches 2-6
v1 -> v2
- Move to NonDiscoverablePciDeviceDxe
Jan Dąbroś (4): Platforms/Marvell: Enable PciEmulation driver for Armada70x0 platform Platforms/Marvell: Enable USB stack for Armada70x0 platform Platforms/Marvell: Enable two xHCI ports for Armada70x0 board Platforms/Marvell: Enable SATA stack for Armada70x0 platform
Marcin Wojtas (2): Platforms/Marvell: Add PciEmulation driver Platforms/Marvell: Enable SATA port for Armada70x0 board
.../Marvell/PortingGuide/PciEmulation.txt | 46 ++++++++ Platforms/Marvell/Armada/Armada.dsc.inc | 19 ++++ Platforms/Marvell/Armada/Armada70x0.dsc | 10 ++ Platforms/Marvell/Armada/Armada70x0.fdf | 18 ++++ Platforms/Marvell/Marvell.dec | 6 ++ Platforms/Marvell/PciEmulation/PciEmulation.c | 119 +++++++++++++++++++++ Platforms/Marvell/PciEmulation/PciEmulation.inf | 46 ++++++++ 7 files changed, 264 insertions(+) create mode 100644 Documentation/Marvell/PortingGuide/PciEmulation.txt create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.c create mode 100644 Platforms/Marvell/PciEmulation/PciEmulation.inf
-- 1.8.3.1
-
Leif Lindholm -
Marcin Wojtas -
marcin wojtas