2017-10-12 12:50 GMT+02:00 Leif Lindholm <leif.lindh...@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.lindh...@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 <m...@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
