Re: [edk2-devel] [edk2-platforms PATCH] Platform/Socionext/DeveloperBox: add SMBIOS type 17 table
On Tue, 11 Jun 2019 at 14:18, Masahisa Kojima wrote: > > This adds the SMBIOS type 17 table support for Developerbox platform. > The SPDs on a I2C bus is only accessible by the SCP, so SCP-firmware > stores SPDs in non-secure SRAM. > > This commit also reduces the uefi stack size to allocate space > for SPDs. It requires 2KB, 512bytes * 4 DIMMs. > > Signed-off-by: Masahisa Kojima So this patch depends on SCP fw changes that are not yet available, right? That is not a problem, but I will hold off on merging it until we have a new SCP firmware build in the tree. Some comments below. > --- > .../Socionext/DeveloperBox/DeveloperBox.dsc | 3 + > .../DeveloperBox/DeveloperBox.dsc.inc | 2 +- > .../SmbiosPlatformDxe/SmbiosPlatformDxe.c | 494 +- > .../SmbiosPlatformDxe/SmbiosPlatformDxe.inf | 2 + > Silicon/Socionext/SynQuacer/SynQuacer.dec | 3 + > 5 files changed, 381 insertions(+), 123 deletions(-) > > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc > b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc > index 97fb8c410c..f247370694 100644 > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc > @@ -147,6 +147,9 @@ > >gSynQuacerTokenSpaceGuid.PcdDramInfoBase|0x2E00FFC0 > > + # SCP-firmware stored SPD DDR4 data in non-secure SRAM > + gSynQuacerTokenSpaceGuid.PcdSmbiosStoredSpdDDR4Address|0x2E00F000 > + Please drop 'Smbios' from this PCD name - we may want to use the SPD data in a different way in the future. ># ># 96boards mezzanine support ># > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc > b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc > index a10e48ca07..abb113e858 100644 > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc > @@ -137,7 +137,7 @@ > ># non-secure SRAM >gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x2E00 > - gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0xFFC0 > + gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0xF000 > >gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|24 > > diff --git > a/Platform/Socionext/DeveloperBox/SmbiosPlatformDxe/SmbiosPlatformDxe.c > b/Platform/Socionext/DeveloperBox/SmbiosPlatformDxe/SmbiosPlatformDxe.c > index 6227b77877..da0fd2e6a5 100644 > --- a/Platform/Socionext/DeveloperBox/SmbiosPlatformDxe/SmbiosPlatformDxe.c > +++ b/Platform/Socionext/DeveloperBox/SmbiosPlatformDxe/SmbiosPlatformDxe.c > @@ -10,17 +10,48 @@ > **/ > > #include > +#include > #include > #include > #include > #include > #include > #include > +#include > #include > #include > > STATIC EFI_SMBIOS_PROTOCOL *mSmbios; > > +#define SPD4_MEM_BUS_WIDTH_8BIT(0x00) > +#define SPD4_MEM_BUS_WIDTH_16BIT (BIT0) > +#define SPD4_MEM_BUS_WIDTH_32BIT (BIT1) > +#define SPD4_MEM_BUS_WIDTH_64BIT (BIT0 | BIT1) > + > +#define SPD4_MEM_DEV_WIDTH_4BIT(0x00) > +#define SPD4_MEM_DEV_WIDTH_8BIT(BIT0) > +#define SPD4_MEM_DEV_WIDTH_16BIT (BIT1) > +#define SPD4_MEM_DEV_WIDTH_32BIT (BIT0 | BIT1) > + > +#define SPD4_MEM_MODULE_TYPE_RDIMM0x01 > +#define SPD4_MEM_MODULE_TYPE_UDIMM0x02 > +#define SPD4_MEM_MODULE_TYPE_SODIMM 0x03 > + > +#define TYPE17_DEVICE_LOCATOR_LEN (8 + 1) > +#define TYPE17_BANK_LOCATOR_LEN (20 + 1) > +#define TYPE17_MANUFACTURER_NAME_LEN (30 + 1) > +#define TYPE17_SERIAL_NUMBER_LEN (16 + 1) > +#define TYPE17_ASSETTAG_LEN (16 + 1) > +#define TYPE17_MODULE_PART_NUMBER_LEN (20 + 1) > + > +#define TYPE17_STRINGS_MAX_LEN(TYPE17_DEVICE_LOCATOR_LEN + \ > + TYPE17_BANK_LOCATOR_LEN + \ > + TYPE17_MANUFACTURER_NAME_LEN + \ > + TYPE17_SERIAL_NUMBER_LEN + \ > + TYPE17_ASSETTAG_LEN + \ > + TYPE17_MODULE_PART_NUMBER_LEN + \ > + 1/* null SMBIOS_TABLE_STRING > terminator */ ) > + > // > // Type definition and contents of the default SMBIOS table. > // This table covers only the minimum structures required by > @@ -69,7 +100,7 @@ typedef struct { > > typedef struct { >SMBIOS_TABLE_TYPE17 Base; > - CHAR8 Strings[]; > + CHAR8 Strings[TYPE17_STRINGS_MAX_LEN]; > } ARM_TYPE17; > > typedef struct { > @@ -94,6 +125,69 @@ enum { >SMBIOS_HANDLE_MEMORY, > }; > > +struct JEP106_MANUFACTURER_TABLE { > + UINT16 ManufacturerId; > + CHAR8 ManufacturerName[TYPE17_MANUFACTURER_NAME_LEN]; > +}; > + > +STATIC CONST struct JEP106_MANUFACTURER_TABLE Manufacturer[] = { > + {0x0010, "NEC\0"}, > + {0x002C, "Micron Technology\0"}, > + {0x003D, "Tektronix\0"}, > + {0x0097, "Texas Instruments\0"}, > + {0x00AD, "SK Hynix\0"}, > + {0x00B3, "IDT\0"}, > + {0x00C1, "Infineon\0"}, > + {0x00CE, "Samsung\0"}, > + {0x00DA
Re: [edk2-devel] [edk2-platforms PATCH] Platform/Socionext/DeveloperBox: add SMBIOS type 17 table
Hi Ard, On Tue, 11 Jun 2019 at 22:01, Ard Biesheuvel wrote: > > On Tue, 11 Jun 2019 at 14:18, Masahisa Kojima > wrote: > > > > This adds the SMBIOS type 17 table support for Developerbox platform. > > The SPDs on a I2C bus is only accessible by the SCP, so SCP-firmware > > stores SPDs in non-secure SRAM. > > > > This commit also reduces the uefi stack size to allocate space > > for SPDs. It requires 2KB, 512bytes * 4 DIMMs. > > > > Signed-off-by: Masahisa Kojima > > So this patch depends on SCP fw changes that are not yet available, > right? That is not a problem, but I will hold off on merging it until > we have a new SCP firmware build in the tree. Yes, this patch depends on SCP firmware changes. It is better to wait SCP fw available, thank you. > Some comments below. I will fix all comments. > > --- > > .../Socionext/DeveloperBox/DeveloperBox.dsc | 3 + > > .../DeveloperBox/DeveloperBox.dsc.inc | 2 +- > > .../SmbiosPlatformDxe/SmbiosPlatformDxe.c | 494 +- > > .../SmbiosPlatformDxe/SmbiosPlatformDxe.inf | 2 + > > Silicon/Socionext/SynQuacer/SynQuacer.dec | 3 + > > 5 files changed, 381 insertions(+), 123 deletions(-) > > > > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc > > b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc > > index 97fb8c410c..f247370694 100644 > > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc > > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc > > @@ -147,6 +147,9 @@ > > > >gSynQuacerTokenSpaceGuid.PcdDramInfoBase|0x2E00FFC0 > > > > + # SCP-firmware stored SPD DDR4 data in non-secure SRAM > > + gSynQuacerTokenSpaceGuid.PcdSmbiosStoredSpdDDR4Address|0x2E00F000 > > + > > Please drop 'Smbios' from this PCD name - we may want to use the SPD > data in a different way in the future. > > ># > ># 96boards mezzanine support > ># > > diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc > > b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc > > index a10e48ca07..abb113e858 100644 > > --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc > > +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc > > @@ -137,7 +137,7 @@ > > > ># non-secure SRAM > >gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x2E00 > > - gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0xFFC0 > > + gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0xF000 > > > >gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|24 > > > > diff --git > > a/Platform/Socionext/DeveloperBox/SmbiosPlatformDxe/SmbiosPlatformDxe.c > > b/Platform/Socionext/DeveloperBox/SmbiosPlatformDxe/SmbiosPlatformDxe.c > > index 6227b77877..da0fd2e6a5 100644 > > --- a/Platform/Socionext/DeveloperBox/SmbiosPlatformDxe/SmbiosPlatformDxe.c > > +++ b/Platform/Socionext/DeveloperBox/SmbiosPlatformDxe/SmbiosPlatformDxe.c > > @@ -10,17 +10,48 @@ > > **/ > > > > #include > > +#include > > #include > > #include > > #include > > #include > > #include > > #include > > +#include > > #include > > #include > > > > STATIC EFI_SMBIOS_PROTOCOL *mSmbios; > > > > +#define SPD4_MEM_BUS_WIDTH_8BIT(0x00) > > +#define SPD4_MEM_BUS_WIDTH_16BIT (BIT0) > > +#define SPD4_MEM_BUS_WIDTH_32BIT (BIT1) > > +#define SPD4_MEM_BUS_WIDTH_64BIT (BIT0 | BIT1) > > + > > +#define SPD4_MEM_DEV_WIDTH_4BIT(0x00) > > +#define SPD4_MEM_DEV_WIDTH_8BIT(BIT0) > > +#define SPD4_MEM_DEV_WIDTH_16BIT (BIT1) > > +#define SPD4_MEM_DEV_WIDTH_32BIT (BIT0 | BIT1) > > + > > +#define SPD4_MEM_MODULE_TYPE_RDIMM0x01 > > +#define SPD4_MEM_MODULE_TYPE_UDIMM0x02 > > +#define SPD4_MEM_MODULE_TYPE_SODIMM 0x03 > > + > > +#define TYPE17_DEVICE_LOCATOR_LEN (8 + 1) > > +#define TYPE17_BANK_LOCATOR_LEN (20 + 1) > > +#define TYPE17_MANUFACTURER_NAME_LEN (30 + 1) > > +#define TYPE17_SERIAL_NUMBER_LEN (16 + 1) > > +#define TYPE17_ASSETTAG_LEN (16 + 1) > > +#define TYPE17_MODULE_PART_NUMBER_LEN (20 + 1) > > + > > +#define TYPE17_STRINGS_MAX_LEN(TYPE17_DEVICE_LOCATOR_LEN + \ > > + TYPE17_BANK_LOCATOR_LEN + \ > > + TYPE17_MANUFACTURER_NAME_LEN + \ > > + TYPE17_SERIAL_NUMBER_LEN + \ > > + TYPE17_ASSETTAG_LEN + \ > > + TYPE17_MODULE_PART_NUMBER_LEN + \ > > + 1/* null SMBIOS_TABLE_STRING > > terminator */ ) > > + > > // > > // Type definition and contents of the default SMBIOS table. > > // This table covers only the minimum structures required by > > @@ -69,7 +100,7 @@ typedef struct { > > > > typedef struct { > >SMBIOS_TABLE_TYPE17 Base; > > - CHAR8 Strings[]; > > + CHAR8 Strings[TYPE17_STRINGS_MAX_LEN]; > > } ARM_TYPE17; > > > > typedef struct { > > @@ -94,6 +125,69 @@ enum { > >SMBIOS_HANDLE_MEMORY, > > }; > > > > +struct JEP10