Instead of using hardcoded value in PcdSystemMemorySize PCD,
obtain DRAM size directly from SoC registers, which are filled
by firmware during early initialization stage.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 111 +++++++++++++++++++-
Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h | 49 +++++++++
2 files changed, 159 insertions(+), 1 deletion(-)
diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
index 2cb2e15..22cbe47 100644
--- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
+++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
@@ -36,8 +36,11 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include <Library/ArmPlatformLib.h>
#include <Library/DebugLib.h>
#include <Library/HobLib.h>
+#include <Library/IoLib.h>
#include <Library/MemoryAllocationLib.h>
+#include "Armada70x0LibMem.h"
+
// The total number of descriptors, including the final "end-of-table" descriptor.
#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 16
@@ -47,6 +50,105 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
+// Obtain DRAM size basing on register values filled by early firmware.
+STATIC
+UINT64
+DramSizeGet (
+ UINT64 *MemSize
+ )
+{
+ UINT64 BaseAddr;
+ UINT32 RegVal;
+ UINT8 AreaLengthMap;
+ UINT8 Cs;
+
+ *MemSize = 0;
+
+ for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) {
+
+ RegVal = MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs));
+
+ /* Exit loop on first disabled DRAM CS */
+ if (!(RegVal & DRAM_CS_VALID_ENABLED_MASK)) {
+ break;
+ }
+
+ /*
+ * Sanity check for base address of next DRAM block.
+ * Only continuous space will be used.
+ */
+ BaseAddr = ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG (Cs)) <<
+ DRAM_START_ADDR_HTOL_OFFS) | (RegVal & DRAM_START_ADDRESS_L_MASK);
+ if (BaseAddr != *MemSize) {
+ DEBUG ((DEBUG_ERROR,
+ "DramSizeGet: DRAM blocks are not contiguous, limit size to 0x%llx\n",
+ *MemSize));
+ return EFI_SUCCESS;
+ }
+
+ /* Decode area length for current CS from register value */
+ AreaLengthMap = ((RegVal & DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS);
+ switch (AreaLengthMap) {
+ case 0x0:
+ *MemSize += 0x18000000;
+ break;
+ case 0x1:
+ *MemSize += 0x30000000;
+ break;
+ case 0x2:
+ *MemSize += 0x60000000;
+ break;
+ case 0x3:
+ *MemSize += 0xC0000000;
+ break;
+ case 0x7:
+ *MemSize += 0x00800000;
+ break;
+ case 0x8:
+ *MemSize += 0x01000000;
+ break;
+ case 0x9:
+ *MemSize += 0x02000000;
+ break;
+ case 0xA:
+ *MemSize += 0x04000000;
+ break;
+ case 0xB:
+ *MemSize += 0x08000000;
+ break;
+ case 0xC:
+ *MemSize += 0x10000000;
+ break;
+ case 0xD:
+ *MemSize += 0x20000000;
+ break;
+ case 0xE:
+ *MemSize += 0x40000000;
+ break;
+ case 0xF:
+ *MemSize += 0x80000000;
+ break;
+ case 0x10:
+ *MemSize += 0x100000000;
+ break;
+ case 0x11:
+ *MemSize += 0x200000000;
+ break;
+ case 0x12:
+ *MemSize += 0x400000000;
+ break;
+ case 0x13:
+ *MemSize += 0x800000000;
+ break;
+ default:
+ DEBUG ((DEBUG_ERROR, "Invalid area length (0x%x) for CS#%d\n", AreaLengthMap, Cs));
+ return EFI_INVALID_PARAMETER;
+ }
+ }
+
+ return EFI_SUCCESS;
+}
+
/**
Return the Virtual Memory Map of your platform
@@ -68,10 +170,17 @@ ArmPlatformGetVirtualMemoryMap (
UINT64 MemHighStart;
UINT64 MemHighSize;
EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
+ EFI_STATUS Status;
ASSERT (VirtualMemoryMap != NULL);
- MemSize = FixedPcdGet64 (PcdSystemMemorySize);
+ // Obtain total memory size from the hardware.
+ Status = DramSizeGet (&MemSize);
+ if (EFI_ERROR (Status)) {
+ MemSize = FixedPcdGet64 (PcdSystemMemorySize);
+ DEBUG ((DEBUG_ERROR, "Limit total memory size to %d MB\n", MemSize / 1024 / 1024));
+ }
+
MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize);
MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) +
FixedPcdGet32 (PcdDramRemapSize);
diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h
new file mode 100644
index 0000000..b81fd1d
--- /dev/null
+++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h
@@ -0,0 +1,49 @@
+/*******************************************************************************
+Copyright (C) 2017 Marvell International Ltd.
+
+Marvell BSD License Option
+
+If you received this File from Marvell, you may opt to use, redistribute and/or
+modify this File under the following licensing terms.
+Redistribution and use in source and binary forms, with or without modification,
+are permitted provided that the following conditions are met:
+
+* Redistributions of source code must retain the above copyright notice,
+ this list of conditions and the following disclaimer.
+
+* Redistributions in binary form must reproduce the above copyright
+ notice, this list of conditions and the following disclaimer in the
+ documentation and/or other materials provided with the distribution.
+
+* Neither the name of Marvell nor the names of its contributors may be
+ used to endorse or promote products derived from this software without
+ specific prior written permission.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
+ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+*******************************************************************************/
+
+#define DRAM_CONF_BASE 0xf0020000
+
+#define DRAM_CH0_MMAP_LOW_BASE (DRAM_CONF_BASE + 0x200)
+#define DRAM_CH0_MMAP_LOW_REG(cs) (DRAM_CH0_MMAP_LOW_BASE + (cs) * 0x8)
+#define DRAM_CS_VALID_ENABLED_MASK 0x1
+#define DRAM_AREA_LENGTH_OFFS 16
+#define DRAM_AREA_LENGTH_MASK (0x1f << DRAM_AREA_LENGTH_OFFS)
+#define DRAM_START_ADDRESS_L_OFFS 23
+#define DRAM_START_ADDRESS_L_MASK (0x1ff << DRAM_START_ADDRESS_L_OFFS)
+
+#define DRAM_CH0_MMAP_HIGH_BASE (DRAM_CONF_BASE + 0x204)
+#define DRAM_CH0_MMAP_HIGH_REG(cs) (DRAM_CH0_MMAP_HIGH_BASE + (cs) * 0x8)
+#define DRAM_START_ADDR_HTOL_OFFS 32
+
+#define DRAM_MAX_CS_NUM 8
--
2.7.4
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On Wed, Oct 11, 2017 at 05:40:47PM +0200, Marcin Wojtas wrote:
> Instead of using hardcoded value in PcdSystemMemorySize PCD,
> obtain DRAM size directly from SoC registers, which are filled
> by firmware during early initialization stage.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 111 +++++++++++++++++++-
> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h | 49 +++++++++
> 2 files changed, 159 insertions(+), 1 deletion(-)
>
> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> index 2cb2e15..22cbe47 100644
> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> @@ -36,8 +36,11 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> #include <Library/ArmPlatformLib.h>
> #include <Library/DebugLib.h>
> #include <Library/HobLib.h>
> +#include <Library/IoLib.h>
> #include <Library/MemoryAllocationLib.h>
>
> +#include "Armada70x0LibMem.h"
> +
> // The total number of descriptors, including the final "end-of-table" descriptor.
> #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 16
>
> @@ -47,6 +50,105 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>
> STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
>
> +// Obtain DRAM size basing on register values filled by early firmware.
> +STATIC
> +UINT64
> +DramSizeGet (
GetDramSize?
> + UINT64 *MemSize
IN, OUT?
> + )
> +{
> + UINT64 BaseAddr;
> + UINT32 RegVal;
> + UINT8 AreaLengthMap;
> + UINT8 Cs;
> +
> + *MemSize = 0;
> +
> + for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) {
> +
> + RegVal = MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs));
> +
> + /* Exit loop on first disabled DRAM CS */
> + if (!(RegVal & DRAM_CS_VALID_ENABLED_MASK)) {
> + break;
> + }
> +
> + /*
> + * Sanity check for base address of next DRAM block.
> + * Only continuous space will be used.
> + */
> + BaseAddr = ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG (Cs)) <<
> + DRAM_START_ADDR_HTOL_OFFS) | (RegVal & DRAM_START_ADDRESS_L_MASK);
Please use macros, temporary variables, more parentheses or whatever
to help make the above operation readable.
> + if (BaseAddr != *MemSize) {
> + DEBUG ((DEBUG_ERROR,
> + "DramSizeGet: DRAM blocks are not contiguous, limit size to 0x%llx\n",
> + *MemSize));
> + return EFI_SUCCESS;
> + }
> +
> + /* Decode area length for current CS from register value */
> + AreaLengthMap = ((RegVal & DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS);
> + switch (AreaLengthMap) {
Why Map?
> + case 0x0:
> + *MemSize += 0x18000000;
> + break;
> + case 0x1:
> + *MemSize += 0x30000000;
> + break;
> + case 0x2:
> + *MemSize += 0x60000000;
> + break;
> + case 0x3:
> + *MemSize += 0xC0000000;
> + break;
The above ones look if not devoid of pattern, at least a bit
unexpected - and 4-6 are comlpetely missing. Is there any
documentation available for me to read to try to understand what is
going on?
The below all look predictably formatted and possible to calculate
rather than list one by one.
(1 << ((AreaLengthMap + 16))) if a quick calculation serves me right.
> + case 0x7:
> + *MemSize += 0x00800000;
> + break;
> + case 0x8:
> + *MemSize += 0x01000000;
> + break;
> + case 0x9:
> + *MemSize += 0x02000000;
> + break;
> + case 0xA:
> + *MemSize += 0x04000000;
> + break;
> + case 0xB:
> + *MemSize += 0x08000000;
> + break;
> + case 0xC:
> + *MemSize += 0x10000000;
> + break;
> + case 0xD:
> + *MemSize += 0x20000000;
> + break;
> + case 0xE:
> + *MemSize += 0x40000000;
> + break;
> + case 0xF:
> + *MemSize += 0x80000000;
> + break;
> + case 0x10:
> + *MemSize += 0x100000000;
> + break;
> + case 0x11:
> + *MemSize += 0x200000000;
> + break;
> + case 0x12:
> + *MemSize += 0x400000000;
> + break;
> + case 0x13:
> + *MemSize += 0x800000000;
> + break;
> + default:
> + DEBUG ((DEBUG_ERROR, "Invalid area length (0x%x) for CS#%d\n", AreaLengthMap, Cs));
Area length isn't really a helpful debug message.
"memory module size"?
/
Leif
> + return EFI_INVALID_PARAMETER;
> + }
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> Return the Virtual Memory Map of your platform
>
> @@ -68,10 +170,17 @@ ArmPlatformGetVirtualMemoryMap (
> UINT64 MemHighStart;
> UINT64 MemHighSize;
> EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
> + EFI_STATUS Status;
>
> ASSERT (VirtualMemoryMap != NULL);
>
> - MemSize = FixedPcdGet64 (PcdSystemMemorySize);
> + // Obtain total memory size from the hardware.
> + Status = DramSizeGet (&MemSize);
> + if (EFI_ERROR (Status)) {
> + MemSize = FixedPcdGet64 (PcdSystemMemorySize);
> + DEBUG ((DEBUG_ERROR, "Limit total memory size to %d MB\n", MemSize / 1024 / 1024));
> + }
> +
> MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize);
> MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) +
> FixedPcdGet32 (PcdDramRemapSize);
> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h
> new file mode 100644
> index 0000000..b81fd1d
> --- /dev/null
> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h
> @@ -0,0 +1,49 @@
> +/*******************************************************************************
> +Copyright (C) 2017 Marvell International Ltd.
> +
> +Marvell BSD License Option
> +
> +If you received this File from Marvell, you may opt to use, redistribute and/or
> +modify this File under the following licensing terms.
> +Redistribution and use in source and binary forms, with or without modification,
> +are permitted provided that the following conditions are met:
> +
> +* Redistributions of source code must retain the above copyright notice,
> + this list of conditions and the following disclaimer.
> +
> +* Redistributions in binary form must reproduce the above copyright
> + notice, this list of conditions and the following disclaimer in the
> + documentation and/or other materials provided with the distribution.
> +
> +* Neither the name of Marvell nor the names of its contributors may be
> + used to endorse or promote products derived from this software without
> + specific prior written permission.
> +
> +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
> +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> +WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
> +ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> +(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> +ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> +SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +*******************************************************************************/
> +
> +#define DRAM_CONF_BASE 0xf0020000
> +
> +#define DRAM_CH0_MMAP_LOW_BASE (DRAM_CONF_BASE + 0x200)
> +#define DRAM_CH0_MMAP_LOW_REG(cs) (DRAM_CH0_MMAP_LOW_BASE + (cs) * 0x8)
> +#define DRAM_CS_VALID_ENABLED_MASK 0x1
> +#define DRAM_AREA_LENGTH_OFFS 16
> +#define DRAM_AREA_LENGTH_MASK (0x1f << DRAM_AREA_LENGTH_OFFS)
> +#define DRAM_START_ADDRESS_L_OFFS 23
> +#define DRAM_START_ADDRESS_L_MASK (0x1ff << DRAM_START_ADDRESS_L_OFFS)
> +
> +#define DRAM_CH0_MMAP_HIGH_BASE (DRAM_CONF_BASE + 0x204)
> +#define DRAM_CH0_MMAP_HIGH_REG(cs) (DRAM_CH0_MMAP_HIGH_BASE + (cs) * 0x8)
> +#define DRAM_START_ADDR_HTOL_OFFS 32
> +
> +#define DRAM_MAX_CS_NUM 8
> --
> 2.7.4
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
2017-10-11 19:56 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Wed, Oct 11, 2017 at 05:40:47PM +0200, Marcin Wojtas wrote:
>> Instead of using hardcoded value in PcdSystemMemorySize PCD,
>> obtain DRAM size directly from SoC registers, which are filled
>> by firmware during early initialization stage.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 111 +++++++++++++++++++-
>> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h | 49 +++++++++
>> 2 files changed, 159 insertions(+), 1 deletion(-)
>>
>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> index 2cb2e15..22cbe47 100644
>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> @@ -36,8 +36,11 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> #include <Library/ArmPlatformLib.h>
>> #include <Library/DebugLib.h>
>> #include <Library/HobLib.h>
>> +#include <Library/IoLib.h>
>> #include <Library/MemoryAllocationLib.h>
>>
>> +#include "Armada70x0LibMem.h"
>> +
>> // The total number of descriptors, including the final "end-of-table" descriptor.
>> #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 16
>>
>> @@ -47,6 +50,105 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>
>> STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
>>
>> +// Obtain DRAM size basing on register values filled by early firmware.
>> +STATIC
>> +UINT64
>> +DramSizeGet (
>
> GetDramSize?
>
>> + UINT64 *MemSize
>
> IN, OUT?
>
2x OK.
>> + )
>> +{
>> + UINT64 BaseAddr;
>> + UINT32 RegVal;
>> + UINT8 AreaLengthMap;
>> + UINT8 Cs;
>> +
>> + *MemSize = 0;
>> +
>> + for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) {
>> +
>> + RegVal = MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs));
>> +
>> + /* Exit loop on first disabled DRAM CS */
>> + if (!(RegVal & DRAM_CS_VALID_ENABLED_MASK)) {
>> + break;
>> + }
>> +
>> + /*
>> + * Sanity check for base address of next DRAM block.
>> + * Only continuous space will be used.
>> + */
>> + BaseAddr = ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG (Cs)) <<
>> + DRAM_START_ADDR_HTOL_OFFS) | (RegVal & DRAM_START_ADDRESS_L_MASK);
>
> Please use macros, temporary variables, more parentheses or whatever
> to help make the above operation readable.
>
Sure.
>> + if (BaseAddr != *MemSize) {
>> + DEBUG ((DEBUG_ERROR,
>> + "DramSizeGet: DRAM blocks are not contiguous, limit size to 0x%llx\n",
>> + *MemSize));
>> + return EFI_SUCCESS;
>> + }
>> +
>> + /* Decode area length for current CS from register value */
>> + AreaLengthMap = ((RegVal & DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS);
>> + switch (AreaLengthMap) {
>
> Why Map?
>
I stuck to mv-ddr convention, but I agree, 'AreaLength' will be better.
>> + case 0x0:
>> + *MemSize += 0x18000000;
>> + break;
>> + case 0x1:
>> + *MemSize += 0x30000000;
>> + break;
>> + case 0x2:
>> + *MemSize += 0x60000000;
>> + break;
>> + case 0x3:
>> + *MemSize += 0xC0000000;
>> + break;
>
> The above ones look if not devoid of pattern, at least a bit
> unexpected - and 4-6 are comlpetely missing. Is there any
> documentation available for me to read to try to understand what is
> going on?
Do you have a docs for armada 7k or 8k? Please check 0xf0020200 table,
bits 20:16. You are right, I missed '4' (6GB), but 5 and 6 are
reserved (no value possible).
>
> The below all look predictably formatted and possible to calculate
> rather than list one by one.
>
> (1 << ((AreaLengthMap + 16))) if a quick calculation serves me right.
I think we can reduce the switch-case to this:
if (AreaLength =< 0x4) {
*MemSize = 0x18000000 << AreaLength;
} else if (AreaLength >= 0x7 && AreaLength < 0x1B)
*MemSize = 1 << (AreaLengthMap + 16);
} else {
DEBUG ((DEBUG_ERROR, "Invalid memory module size (0x%x) for
CS#%d\n", AreaLength, Cs));
}
Is above ok for you?
>
>> + case 0x7:
>> + *MemSize += 0x00800000;
>> + break;
>> + case 0x8:
>> + *MemSize += 0x01000000;
>> + break;
>> + case 0x9:
>> + *MemSize += 0x02000000;
>> + break;
>> + case 0xA:
>> + *MemSize += 0x04000000;
>> + break;
>> + case 0xB:
>> + *MemSize += 0x08000000;
>> + break;
>> + case 0xC:
>> + *MemSize += 0x10000000;
>> + break;
>> + case 0xD:
>> + *MemSize += 0x20000000;
>> + break;
>> + case 0xE:
>> + *MemSize += 0x40000000;
>> + break;
>> + case 0xF:
>> + *MemSize += 0x80000000;
>> + break;
>> + case 0x10:
>> + *MemSize += 0x100000000;
>> + break;
>> + case 0x11:
>> + *MemSize += 0x200000000;
>> + break;
>> + case 0x12:
>> + *MemSize += 0x400000000;
>> + break;
>> + case 0x13:
>> + *MemSize += 0x800000000;
>> + break;
>> + default:
>> + DEBUG ((DEBUG_ERROR, "Invalid area length (0x%x) for CS#%d\n", AreaLengthMap, Cs));
>
> Area length isn't really a helpful debug message.
> "memory module size"?
>
Ok.
Thanks,
Marcin
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On Thu, Oct 12, 2017 at 07:47:52AM +0200, Marcin Wojtas wrote:
> 2017-10-11 19:56 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > On Wed, Oct 11, 2017 at 05:40:47PM +0200, Marcin Wojtas wrote:
> >> Instead of using hardcoded value in PcdSystemMemorySize PCD,
> >> obtain DRAM size directly from SoC registers, which are filled
> >> by firmware during early initialization stage.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >> ---
> >> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 111 +++++++++++++++++++-
> >> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h | 49 +++++++++
> >> 2 files changed, 159 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> >> index 2cb2e15..22cbe47 100644
> >> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> >> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> >> @@ -36,8 +36,11 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >> #include <Library/ArmPlatformLib.h>
> >> #include <Library/DebugLib.h>
> >> #include <Library/HobLib.h>
> >> +#include <Library/IoLib.h>
> >> #include <Library/MemoryAllocationLib.h>
> >>
> >> +#include "Armada70x0LibMem.h"
> >> +
> >> // The total number of descriptors, including the final "end-of-table" descriptor.
> >> #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 16
> >>
> >> @@ -47,6 +50,105 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >>
> >> STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
> >>
> >> +// Obtain DRAM size basing on register values filled by early firmware.
> >> +STATIC
> >> +UINT64
> >> +DramSizeGet (
> >
> > GetDramSize?
> >
> >> + UINT64 *MemSize
> >
> > IN, OUT?
> >
>
> 2x OK.
>
> >> + )
> >> +{
> >> + UINT64 BaseAddr;
> >> + UINT32 RegVal;
> >> + UINT8 AreaLengthMap;
> >> + UINT8 Cs;
> >> +
> >> + *MemSize = 0;
> >> +
> >> + for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) {
> >> +
> >> + RegVal = MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs));
> >> +
> >> + /* Exit loop on first disabled DRAM CS */
> >> + if (!(RegVal & DRAM_CS_VALID_ENABLED_MASK)) {
> >> + break;
> >> + }
> >> +
> >> + /*
> >> + * Sanity check for base address of next DRAM block.
> >> + * Only continuous space will be used.
> >> + */
> >> + BaseAddr = ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG (Cs)) <<
> >> + DRAM_START_ADDR_HTOL_OFFS) | (RegVal & DRAM_START_ADDRESS_L_MASK);
> >
> > Please use macros, temporary variables, more parentheses or whatever
> > to help make the above operation readable.
> >
>
> Sure.
>
> >> + if (BaseAddr != *MemSize) {
> >> + DEBUG ((DEBUG_ERROR,
> >> + "DramSizeGet: DRAM blocks are not contiguous, limit size to 0x%llx\n",
> >> + *MemSize));
> >> + return EFI_SUCCESS;
> >> + }
> >> +
> >> + /* Decode area length for current CS from register value */
> >> + AreaLengthMap = ((RegVal & DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS);
> >> + switch (AreaLengthMap) {
> >
> > Why Map?
> >
>
> I stuck to mv-ddr convention, but I agree, 'AreaLength' will be better.
>
> >> + case 0x0:
> >> + *MemSize += 0x18000000;
> >> + break;
> >> + case 0x1:
> >> + *MemSize += 0x30000000;
> >> + break;
> >> + case 0x2:
> >> + *MemSize += 0x60000000;
> >> + break;
> >> + case 0x3:
> >> + *MemSize += 0xC0000000;
> >> + break;
> >
> > The above ones look if not devoid of pattern, at least a bit
> > unexpected - and 4-6 are comlpetely missing. Is there any
> > documentation available for me to read to try to understand what is
> > going on?
>
> Do you have a docs for armada 7k or 8k? Please check 0xf0020200 table,
> bits 20:16. You are right, I missed '4' (6GB), but 5 and 6 are
> reserved (no value possible).
(Where do I find .75G, 1.5G, 3G and 6G DIMMs?)
> >
> > The below all look predictably formatted and possible to calculate
> > rather than list one by one.
> >
> > (1 << ((AreaLengthMap + 16))) if a quick calculation serves me right.
>
> I think we can reduce the switch-case to this:
>
> if (AreaLength =< 0x4) {
> *MemSize = 0x18000000 << AreaLength;
> } else if (AreaLength >= 0x7 && AreaLength < 0x1B)
> *MemSize = 1 << (AreaLengthMap + 16);
I would prefer for the arithmetic to be hidden away in a macro.
Both cases.
Also, the tests themselves: IS_VALID_xxx_AREA(), IS_VALID_yyy_AREA().
Sorry, can't think of good real names - if the documentation has it,
use that.
> } else {
> DEBUG ((DEBUG_ERROR, "Invalid memory module size (0x%x) for
> CS#%d\n", AreaLength, Cs));
> }
>
> Is above ok for you?
Yeah, it's a big improvement over listing each individual option.
/
Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
2017-10-12 12:50 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
>
> On Thu, Oct 12, 2017 at 07:47:52AM +0200, Marcin Wojtas wrote:
> > 2017-10-11 19:56 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > > On Wed, Oct 11, 2017 at 05:40:47PM +0200, Marcin Wojtas wrote:
> > >> Instead of using hardcoded value in PcdSystemMemorySize PCD,
> > >> obtain DRAM size directly from SoC registers, which are filled
> > >> by firmware during early initialization stage.
> > >>
> > >> Contributed-under: TianoCore Contribution Agreement 1.1
> > >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > >> ---
> > >> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 111 +++++++++++++++++++-
> > >> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h | 49 +++++++++
> > >> 2 files changed, 159 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> > >> index 2cb2e15..22cbe47 100644
> > >> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> > >> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> > >> @@ -36,8 +36,11 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > >> #include <Library/ArmPlatformLib.h>
> > >> #include <Library/DebugLib.h>
> > >> #include <Library/HobLib.h>
> > >> +#include <Library/IoLib.h>
> > >> #include <Library/MemoryAllocationLib.h>
> > >>
> > >> +#include "Armada70x0LibMem.h"
> > >> +
> > >> // The total number of descriptors, including the final "end-of-table" descriptor.
> > >> #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 16
> > >>
> > >> @@ -47,6 +50,105 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > >>
> > >> STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
> > >>
> > >> +// Obtain DRAM size basing on register values filled by early firmware.
> > >> +STATIC
> > >> +UINT64
> > >> +DramSizeGet (
> > >
> > > GetDramSize?
> > >
> > >> + UINT64 *MemSize
> > >
> > > IN, OUT?
> > >
> >
> > 2x OK.
> >
> > >> + )
> > >> +{
> > >> + UINT64 BaseAddr;
> > >> + UINT32 RegVal;
> > >> + UINT8 AreaLengthMap;
> > >> + UINT8 Cs;
> > >> +
> > >> + *MemSize = 0;
> > >> +
> > >> + for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) {
> > >> +
> > >> + RegVal = MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs));
> > >> +
> > >> + /* Exit loop on first disabled DRAM CS */
> > >> + if (!(RegVal & DRAM_CS_VALID_ENABLED_MASK)) {
> > >> + break;
> > >> + }
> > >> +
> > >> + /*
> > >> + * Sanity check for base address of next DRAM block.
> > >> + * Only continuous space will be used.
> > >> + */
> > >> + BaseAddr = ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG (Cs)) <<
> > >> + DRAM_START_ADDR_HTOL_OFFS) | (RegVal & DRAM_START_ADDRESS_L_MASK);
> > >
> > > Please use macros, temporary variables, more parentheses or whatever
> > > to help make the above operation readable.
> > >
> >
> > Sure.
> >
> > >> + if (BaseAddr != *MemSize) {
> > >> + DEBUG ((DEBUG_ERROR,
> > >> + "DramSizeGet: DRAM blocks are not contiguous, limit size to 0x%llx\n",
> > >> + *MemSize));
> > >> + return EFI_SUCCESS;
> > >> + }
> > >> +
> > >> + /* Decode area length for current CS from register value */
> > >> + AreaLengthMap = ((RegVal & DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS);
> > >> + switch (AreaLengthMap) {
> > >
> > > Why Map?
> > >
> >
> > I stuck to mv-ddr convention, but I agree, 'AreaLength' will be better.
> >
> > >> + case 0x0:
> > >> + *MemSize += 0x18000000;
> > >> + break;
> > >> + case 0x1:
> > >> + *MemSize += 0x30000000;
> > >> + break;
> > >> + case 0x2:
> > >> + *MemSize += 0x60000000;
> > >> + break;
> > >> + case 0x3:
> > >> + *MemSize += 0xC0000000;
> > >> + break;
> > >
> > > The above ones look if not devoid of pattern, at least a bit
> > > unexpected - and 4-6 are comlpetely missing. Is there any
> > > documentation available for me to read to try to understand what is
> > > going on?
> >
> > Do you have a docs for armada 7k or 8k? Please check 0xf0020200 table,
> > bits 20:16. You are right, I missed '4' (6GB), but 5 and 6 are
> > reserved (no value possible).
>
> (Where do I find .75G, 1.5G, 3G and 6G DIMMs?)
I guess nowhere, but please bear in mind, there are also custom
modules (Armada-7040-DB has one) and on-board DRAMs - in such
circumstances it's up to the designer.
>
>
> > >
> > > The below all look predictably formatted and possible to calculate
> > > rather than list one by one.
> > >
> > > (1 << ((AreaLengthMap + 16))) if a quick calculation serves me right.
> >
> > I think we can reduce the switch-case to this:
> >
> > if (AreaLength =< 0x4) {
> > *MemSize = 0x18000000 << AreaLength;
> > } else if (AreaLength >= 0x7 && AreaLength < 0x1B)
> > *MemSize = 1 << (AreaLengthMap + 16);
>
> I would prefer for the arithmetic to be hidden away in a macro.
> Both cases.
>
> Also, the tests themselves: IS_VALID_xxx_AREA(), IS_VALID_yyy_AREA().
> Sorry, can't think of good real names - if the documentation has it,
> use that.
>
Ok, I'll try to come up with something readable.
> > } else {
> > DEBUG ((DEBUG_ERROR, "Invalid memory module size (0x%x) for
> > CS#%d\n", AreaLength, Cs));
> > }
> >
> > Is above ok for you?
>
> Yeah, it's a big improvement over listing each individual option.
>
Ok, thanks.
Marcin
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.