Memory Down approach Error on intel Denverton board

[Ganesh Kumar C]

Hi ,

Greetings .

We are working on intel c508 intel denverton board . It has a soldered 4GB DDR4 connected to Channel 0 , Dimm 0 .

Below are the changes made :

1. We are using harcuvar board as a reference . I have selected * Enable Memory down option in menu config

2. Added spd values file in src/mainboard/intel/harcuvar/spd/spd_filename.hex

3. spd.bn getting generated in CBFS

FMAP REGION: COREBOOT
Name                           Offset     Type           Size   Comp
cbfs master header             0x0        cbfs header        32 none
fallback/romstage              0x80       stage           42084 none
cpu_microcode_blob.bin         0xa580     microcode       11264 none
fallback/ramstage              0xd200     stage           70701 none
config                         0x1e680    raw              1227 none
revision                       0x1ebc0    raw               691 none
spd.bin                        0x1eec0    spd               512 none
fallback/dsdt.aml              0x1f100    raw              7791 none
fallback/postcar               0x20fc0    stage           21832 none
fallback/payload               0x26540    simple elf     656810 none
(empty)                        0xc6b40    null          6656600 none
fspt.bin                       0x71fdc0   fsp              4096 none
(empty)                        0x720e00   null             3992 none
fspm.bin                       0x721dc0   fsp            589824 none
(empty)                        0x7b1e00   null             3992 none
fsps.bin                       0x7b2dc0   fsp            102400 none
(empty)                        0x7cbe00   null           114584 none
bootblock                      0x7e7dc0   bootblock       32768 none

4. Tried giving 256 and 512 values

CONFIG_DIMM_SPD_SIZE=256

We are using Fitc tool to generate full fw image,getting below error and got stuck after loading the build

UMA: ME UMA size set to 0. Isoc is Disabled.
SMBus Legacy: SPD Write Disable bit is locked now!
SMBus Host: SPD Write Disable bit is locked now!
MRC VERSION: 0x950441
MRCDATA Size: 115F1
MRC_SAVE_RESTORE Size: 6F2C
SocStepping: 16
Warning: MspData data structure hasn't been locked yet
Dunit Fuse Configuration
SCRAMBLER_SUPPORTED: 1
DDR_MAX_FREQ_LIMIT: 1
DDR_CURRENT_FREQ: 1
SINGLE_CHANNEL: 0
IPROCTRIM: 2
TIMING_1N_SUPPORTED: 1
X4_SUPPORTED: 1
X8_SUPPORTED: 1
DDR4_SUPPORTED: 1
DDR3_SUPPORTED: 1
DOUBLE_RANK_SUPPORTED: 1
POPULATE_2DPC_SUPPORTED: 1
ECC_SUPPORTED: 1
MAX_DEN_SUPPORTED: 3
MAX_MEM_SUPPORTED: 7
Warning: MspData data structure hasn't been locked yet
Dunit Fuse Configuration
SCRAMBLER_SUPPORTED: 1
DDR_MAX_FREQ_LIMIT: 1
DDR_CURRENT_FREQ: 1
SINGLE_CHANNEL: 0
IPROCTRIM: 2
TIMING_1N_SUPPORTED: 1
X4_SUPPORTED: 1
X8_SUPPORTED: 1
DDR4_SUPPORTED: 1
DDR3_SUPPORTED: 1
DOUBLE_RANK_SUPPORTED: 1
POPULATE_2DPC_SUPPORTED: 1
ECC_SUPPORTED: 1
MAX_DEN_SUPPORTED: 3
MAX_MEM_SUPPORTED: 7
GetSpdData(ch:0) Dimm:0 MemDn=1
Error! Unsuported DDR type: 255
Status          = 0x00000001

Any help on this is highly appreciated .

Thanks,

Ganesh kumar C

[Szafranski, MariuszX]

Hi,

Did you adjusted mMemoryDownConfig structure in mainboard`s romstage.c file to much your memory down configuration?
Refer to commented out example just above structure definition.
Also double check if .SpdDataPtr structure member for memory down slot has correct pointer to spd.bin content.

Mariusz
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263

This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.

[Ganesh Kumar C]

Hi MariuszX,

Thanks for you time .

Yes . I have added the below  memoryDownConfig struct in

src/mainboard/intel/harcuvar/romstage.c file .

const MEMORY_DOWN_CONFIG mMemoryDownConfig = {
        .SlotState = {
                {STATE_MEMORY_DOWN, STATE_MEMORY_SLOT},
                {STATE_MEMORY_SLOT, STATE_MEMORY_SLOT}
        },
        .SpdDataLen = MAX_SPD_BYTES, //512
        .SpdDataPtr = {
                {(void *)CONFIG_SPD_LOC, (void *)NULL},
                {(void *)NULL, (void *)NULL}
        }
};

Regards

Ganeshc

________________________________
From: Szafranski, MariuszX 
Sent: Thursday, January 20, 2022 6:55 PM
To: Ganesh Kumar C; Coreboot
Subject: RE: Memory Down approach Error on intel Denverton board

Hi,

Did you adjusted mMemoryDownConfig structure in mainboard`s romstage.c file to much your memory down configuration?
Refer to commented out example just above structure definition.
Also double check if .SpdDataPtr structure member for memory down slot has correct pointer to spd.bin content.

Mariusz

--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.

CAUTION: This email originated from outside your organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

[Ganesh Kumar C]

Also double check if .SpdDataPtr structure member for memory down slot has correct pointer to spd.bin content.
> >

Yes , I have verified this part and also printed the spd values getting from spd.bin .

[Angel Pons]

Hi,

On Mon, Jan 24, 2022 at 8:26 AM Ganesh Kumar C via coreboot
 wrote:
> Hi MariuszX,
>
> Thanks for you time .
>
> Yes . I have added the below  memoryDownConfig struct in
>
> src/mainboard/intel/harcuvar/romstage.c file .
>
> const MEMORY_DOWN_CONFIG mMemoryDownConfig = {
>         .SlotState = {
>                 {STATE_MEMORY_DOWN, STATE_MEMORY_SLOT},
>                 {STATE_MEMORY_SLOT, STATE_MEMORY_SLOT}
>         },
>         .SpdDataLen = MAX_SPD_BYTES, //512
>         .SpdDataPtr = {
>                 {(void *)CONFIG_SPD_LOC, (void *)NULL},
>                 {(void *)NULL, (void *)NULL}
>         }
> };

Looks like the Harcuvar memory down code has never been tested,
there's no way it can work as-is. Moreover, documenting code changes
in comments is a terrible idea, because comments aren't compiled.

It's not a good idea to directly access files inside CBFS (for
example, spd.bin is in CBFS) via a hardcoded address (`CONFIG_SPD_LOC`
in this case), as it completely bypasses the CBFS API: nothing
guarantees that the expected data will always be at that address,
there's no way to know the size of the file at runtime and prevents
making use of CBFS security features such as file measurement and
TOCTOU safety (IIRC, it's still work in progress). The right way to
fetch the SPD data using the CBFS API is already implemented in
`src/mainboard/intel/harcuvar/spd/spd.c` function
`mainboard_find_spd_data()`, but the returned pointer is not used in
the code.

I just made https://review.coreboot.org/61341 to show how to do it The
Right Way. Let me know if the implementation from my change works for
you.
> Regards
>
> Ganeshc

Best regards,
Angel

[Ganesh Kumar C]

Sure, i will try these changes and let you know shortly .

Thanks,
Ganesh
________________________________________
From: Angel Pons 
Sent: Monday, January 24, 2022 4:09 PM
To: Ganesh Kumar C
Cc: Szafranski, MariuszX; Coreboot
Subject: Re: [coreboot] Re: Memory Down approach Error on intel Denverton board

Hi,

On Mon, Jan 24, 2022 at 8:26 AM Ganesh Kumar C via coreboot
 wrote:
> Hi MariuszX,
>
> Thanks for you time .
>
> Yes . I have added the below  memoryDownConfig struct in
>
> src/mainboard/intel/harcuvar/romstage.c file .
>
> const MEMORY_DOWN_CONFIG mMemoryDownConfig = {
>         .SlotState = {
>                 {STATE_MEMORY_DOWN, STATE_MEMORY_SLOT},
>                 {STATE_MEMORY_SLOT, STATE_MEMORY_SLOT}
>         },
>         .SpdDataLen = MAX_SPD_BYTES, //512
>         .SpdDataPtr = {
>                 {(void *)CONFIG_SPD_LOC, (void *)NULL},
>                 {(void *)NULL, (void *)NULL}
>         }
> };

Looks like the Harcuvar memory down code has never been tested,
there's no way it can work as-is. Moreover, documenting code changes
in comments is a terrible idea, because comments aren't compiled.

It's not a good idea to directly access files inside CBFS (for
example, spd.bin is in CBFS) via a hardcoded address (`CONFIG_SPD_LOC`
in this case), as it completely bypasses the CBFS API: nothing
guarantees that the expected data will always be at that address,
there's no way to know the size of the file at runtime and prevents
making use of CBFS security features such as file measurement and
TOCTOU safety (IIRC, it's still work in progress). The right way to
fetch the SPD data using the CBFS API is already implemented in
`src/mainboard/intel/harcuvar/spd/spd.c` function
`mainboard_find_spd_data()`, but the returned pointer is not used in
the code.

I just made https://review.coreboot.org/61341 to show how to do it The
Right Way. Let me know if the implementation from my change works for
you.
> Regards
>
> Ganeshc

Best regards,
Angel
CAUTION: This email originated from outside your organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

[Ganesh Kumar C]

Hi Angel Pons,

Thanks for suggesting the changes . I have removed spd eeprom and verified the changes . It is working .

Thank you all for the quick support .

Regards,
GaneshC
________________________________________
From: Ganesh Kumar C
Sent: Monday, January 24, 2022 6:13 PM
To: Angel Pons
Cc: Szafranski, MariuszX; Coreboot
Subject: Re: [coreboot] Re: Memory Down approach Error on intel Denverton board

Sure, i will try these changes and let you know shortly .

Thanks,
Ganesh
________________________________________
From: Angel Pons 
Sent: Monday, January 24, 2022 4:09 PM
To: Ganesh Kumar C
Cc: Szafranski, MariuszX; Coreboot
Subject: Re: [coreboot] Re: Memory Down approach Error on intel Denverton board

Hi,

On Mon, Jan 24, 2022 at 8:26 AM Ganesh Kumar C via coreboot
 wrote:
> Hi MariuszX,
>
> Thanks for you time .
>
> Yes . I have added the below  memoryDownConfig struct in
>
> src/mainboard/intel/harcuvar/romstage.c file .
>
> const MEMORY_DOWN_CONFIG mMemoryDownConfig = {
>         .SlotState = {
>                 {STATE_MEMORY_DOWN, STATE_MEMORY_SLOT},
>                 {STATE_MEMORY_SLOT, STATE_MEMORY_SLOT}
>         },
>         .SpdDataLen = MAX_SPD_BYTES, //512
>         .SpdDataPtr = {
>                 {(void *)CONFIG_SPD_LOC, (void *)NULL},
>                 {(void *)NULL, (void *)NULL}
>         }
> };

Looks like the Harcuvar memory down code has never been tested,
there's no way it can work as-is. Moreover, documenting code changes
in comments is a terrible idea, because comments aren't compiled.

It's not a good idea to directly access files inside CBFS (for
example, spd.bin is in CBFS) via a hardcoded address (`CONFIG_SPD_LOC`
in this case), as it completely bypasses the CBFS API: nothing
guarantees that the expected data will always be at that address,
there's no way to know the size of the file at runtime and prevents
making use of CBFS security features such as file measurement and
TOCTOU safety (IIRC, it's still work in progress). The right way to
fetch the SPD data using the CBFS API is already implemented in
`src/mainboard/intel/harcuvar/spd/spd.c` function
`mainboard_find_spd_data()`, but the returned pointer is not used in
the code.

I just made https://review.coreboot.org/61341 to show how to do it The
Right Way. Let me know if the implementation from my change works for
you.
> Regards
>
> Ganeshc

Best regards,
Angel
CAUTION: This email originated from outside your organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.