On Mon, Mar 20, 2017 at 05:15:41PM +0100, Marcin Wojtas wrote:
Hi Leif,
Thanks for your remarks.
+#ifndef __MVHWDESCLIB_H__ +#define __MVHWDESCLIB_H__
+#include <Library/NonDiscoverableDeviceRegistrationLib.h>
+// +// Platform storage description +// +typedef struct {
- // XHCI
- UINT8 XhciDevCount;
- UINTN XhciBaseAddresses[4];
- UINTN XhciMemSize[4];
- NON_DISCOVERABLE_DEVICE_DMA_TYPE XhciDmaType[4];
- // AHCI
- UINT8 AhciDevCount;
- UINTN AhciBaseAddresses[4];
- UINTN AhciMemSize[4];
- NON_DISCOVERABLE_DEVICE_DMA_TYPE AhciDmaType[4];
- // SDHCI
- UINT8 SdhciDevCount;
- UINTN SdhciBaseAddresses[4];
- UINTN SdhciMemSize[4];
- NON_DISCOVERABLE_DEVICE_DMA_TYPE SdhciDmaType[4];
+} MVHW_STORAGE_DESC;
What do these hardcoded array sizes signify? What makes 4 guaranteed future-proof?
Well, 4 is biggest available for all variants of Armada 7k and 8k. I have no idea for future plans, but would you like to go with some bigger, arbitrarily chosen number (like 8)?.
Preferably, I'd see a non-arbitrary number :)
But seeing as this is just a code shuffling exercise, could you at least add a #define of some descriptive name?
+#define DECLARE_A7K8K_STORAGE_TEMPLATE \
Is XHCI "storage"?
Well, the devices attached to XHCI controller can be called storage. I thought such name would be more meaningful than PCI_EMULATION whatsoever. Do you wish some other particular name here?
I think I'd be happier with PCI_EMULATION, but really we are talking about non-discoverable hardware, so maybe something reflecting that?
+STATIC MVHW_STORAGE_DESC mA7k8kStorageDescTemplate = {\
- 4, /* XHCI */\
- { 0xF2500000, 0xF2510000, 0xF4500000, 0xF4510000 },\
These addresses would be a lot nicer as #defines.
Ok. Could be in the same header?
Absolutely. Just gives the opportunity to add some description into it - like CTRL0_PORT1, ... And confuses the history less if we manage to merge a typo in an address, and fix it.
/ Leif
- { SIZE_16KB, SIZE_16KB, SIZE_16KB, SIZE_16KB },\
- { NonDiscoverableDeviceDmaTypeCoherent,
NonDiscoverableDeviceDmaTypeCoherent,\
- NonDiscoverableDeviceDmaTypeCoherent,
NonDiscoverableDeviceDmaTypeCoherent },\
- 2, /* AHCI */\
- { 0xF2540000, 0xF4540000 },\
- { SIZE_8KB, SIZE_8KB },\
- { NonDiscoverableDeviceDmaTypeCoherent,
NonDiscoverableDeviceDmaTypeCoherent },\
- 2, /* SDHCI */\
- { 0xF06E0000, 0xF2780000 },\
- { SIZE_1KB, SIZE_1KB },\
- { NonDiscoverableDeviceDmaTypeCoherent,
NonDiscoverableDeviceDmaTypeCoherent }\
+}
+#endif /* __MVHWDESCLIB_H__ */ diff --git a/Platforms/Marvell/PciEmulation/PciEmulation.c
b/Platforms/Marvell/PciEmulation/PciEmulation.c
index 491b886..ffefffb 100644 --- a/Platforms/Marvell/PciEmulation/PciEmulation.c +++ b/Platforms/Marvell/PciEmulation/PciEmulation.c @@ -34,47 +34,13 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
DAMAGE.
#include <PiDxe.h>
+#include <Library/MvHwDescLib.h>
Insert sorted, please.
Sure.
Thanks, Marcin