Change in coreboot[master]: mb/google/volteer: add new generic SPD file

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Nick Vaccaro (Code Review)

3 Apr 2020 3 Apr '20

3:03 a.m.

Nick Vaccaro has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40108 )

Change subject: mb/google/volteer: add new generic SPD file ......................................................................

mb/google/volteer: add new generic SPD file

The Micron MT53E512M32D2NP-046 part needs a new generic SPD file as it doesn't match any of the existing generic SPD file's chip architecture.

BUG=b:152827558 TEST=none

Change-Id: I4bda759a88d7d3ffd10ed120cc8e7f0e5d289a34 Signed-off-by: Nick Vaccaro nvaccaro@google.com --- A src/mainboard/google/volteer/spd/SPD_LPDDR4X_200b_8bank_1Rx16_16Gb_rc21_DDP_4267.spd.hex 1 file changed, 32 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/40108/1

diff --git a/src/mainboard/google/volteer/spd/SPD_LPDDR4X_200b_8bank_1Rx16_16Gb_rc21_DDP_4267.spd.hex b/src/mainboard/google/volteer/spd/SPD_LPDDR4X_200b_8bank_1Rx16_16Gb_rc21_DDP_4267.spd.hex new file mode 100644 index 0000000..5fbb943 --- /dev/null +++ b/src/mainboard/google/volteer/spd/SPD_LPDDR4X_200b_8bank_1Rx16_16Gb_rc21_DDP_4267.spd.hex @@ -0,0 +1,32 @@ +23 11 11 0E 15 21 95 08 00 00 00 00 02 01 00 00 +00 00 04 00 92 54 05 00 87 00 90 A8 90 C0 08 60 +04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 E5 00 E1 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 +00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Attachments:

attachment.htm (text/html — 5.5 KB)

Show replies by date

Nick Vaccaro (Code Review)

3 Apr 3 Apr

3:07 a.m.

Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Jes Klinke,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40108

to look at the new patch set (#3).

Change subject: mb/google/volteer: add new generic SPD file ......................................................................

mb/google/volteer: add new generic SPD file

The Micron MT53E512M32D2NP-046 part needs a new generic SPD file as it doesn't match any of the existing generic SPD files' chip architectures.

BUG=b:152827558 TEST=none

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/40108/3

-- To view, visit https://review.coreboot.org/c/coreboot/+/40108 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4bda759a88d7d3ffd10ed120cc8e7f0e5d289a34 Gerrit-Change-Number: 40108 Gerrit-PatchSet: 3 Gerrit-Owner: Nick Vaccaro nvaccaro@google.com Gerrit-Reviewer: Caveh Jalali caveh@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jes Klinke jbk@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: newpatchset

Caveh Jalali (Code Review)

4 a.m.

Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40108 )

Change subject: mb/google/volteer: add new generic SPD file ......................................................................

Patch Set 3:

do we have these parameters in a spreadsheet somewhere so we can review & compare with other SPDs?

-- To view, visit https://review.coreboot.org/c/coreboot/+/40108 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4bda759a88d7d3ffd10ed120cc8e7f0e5d289a34 Gerrit-Change-Number: 40108 Gerrit-PatchSet: 3 Gerrit-Owner: Nick Vaccaro nvaccaro@google.com Gerrit-Reviewer: Caveh Jalali caveh@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jes Klinke jbk@google.com Gerrit-Reviewer: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 03 Apr 2020 04:00:01 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Nick Vaccaro (Code Review)

5:07 a.m.

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40108 )

Change subject: mb/google/volteer: add new generic SPD file ......................................................................

Patch Set 3:

...

Patch Set 3:

do we have these parameters in a spreadsheet somewhere so we can review & compare with other SPDs?

Yes, it's in the "Volteer DRAM Parts ID" document.

-- To view, visit https://review.coreboot.org/c/coreboot/+/40108 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4bda759a88d7d3ffd10ed120cc8e7f0e5d289a34 Gerrit-Change-Number: 40108 Gerrit-PatchSet: 3 Gerrit-Owner: Nick Vaccaro nvaccaro@google.com Gerrit-Reviewer: Caveh Jalali caveh@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jes Klinke jbk@google.com Gerrit-Reviewer: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 03 Apr 2020 05:07:18 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

6:20 a.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40108 )

Change subject: mb/google/volteer: add new generic SPD file ......................................................................

Patch Set 3: Code-Review-2

This is not correct. We should be using https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/volt.... Please see my comment: https://b.corp.google.com/issues/152827558#comment10

-- To view, visit https://review.coreboot.org/c/coreboot/+/40108 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4bda759a88d7d3ffd10ed120cc8e7f0e5d289a34 Gerrit-Change-Number: 40108 Gerrit-PatchSet: 3 Gerrit-Owner: Nick Vaccaro nvaccaro@google.com Gerrit-Reviewer: Caveh Jalali caveh@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jes Klinke jbk@google.com Gerrit-Reviewer: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 03 Apr 2020 06:20:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Nick Vaccaro (Code Review)

4 Apr 4 Apr

2:56 a.m.

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40108 )

Change subject: mb/google/volteer: add new generic SPD file ......................................................................

Patch Set 3:

...

Patch Set 3: Code-Review-2

This is not correct. We should be using https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/volt.... Please see my comment: https://b.corp.google.com/issues/152827558#comment10

Thanks, Furquan. That comment says that data in the SPD is incorrect. "As per FSP code, this should be 0x21 (i.e. 16 rows, 10 columns). Currently checked in coreboot file is wrong."

But it also says that the currently checked in SPD file is the incorrect one, which would mean that this is the correct one, no? If so, I will try out this SPD and then fix the currently checked in one accordingly and abandon this CL.

-- To view, visit https://review.coreboot.org/c/coreboot/+/40108 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4bda759a88d7d3ffd10ed120cc8e7f0e5d289a34 Gerrit-Change-Number: 40108 Gerrit-PatchSet: 3 Gerrit-Owner: Nick Vaccaro nvaccaro@google.com Gerrit-Reviewer: Caveh Jalali caveh@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jes Klinke jbk@google.com Gerrit-Reviewer: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sat, 04 Apr 2020 02:56:48 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Nick Vaccaro (Code Review)

2:59 a.m.

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40108 )

Change subject: mb/google/volteer: add new generic SPD file ......................................................................

Patch Set 3:

...

Patch Set 3:

...
Patch Set 3: Code-Review-2

This is not correct. We should be using https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/volt.... Please see my comment: https://b.corp.google.com/issues/152827558#comment10

Thanks, Furquan. That comment says that data in the SPD is incorrect. "As per FSP code, this should be 0x21 (i.e. 16 rows, 10 columns). Currently checked in coreboot file is wrong."

But it also says that the currently checked in SPD file is the incorrect one, which would mean that this is the correct one, no? If so, I will try out this SPD and then fix the currently checked in one accordingly and abandon this CL.

It's surprising that bus width could be wrong and it still work (for the checked-in SPD).

-- To view, visit https://review.coreboot.org/c/coreboot/+/40108 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4bda759a88d7d3ffd10ed120cc8e7f0e5d289a34 Gerrit-Change-Number: 40108 Gerrit-PatchSet: 3 Gerrit-Owner: Nick Vaccaro nvaccaro@google.com Gerrit-Reviewer: Caveh Jalali caveh@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jes Klinke jbk@google.com Gerrit-Reviewer: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sat, 04 Apr 2020 02:59:19 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

5 Apr 5 Apr

8:58 a.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40108 )

Change subject: mb/google/volteer: add new generic SPD file ......................................................................

Patch Set 3:

...

Patch Set 3:

...
Patch Set 3: Code-Review-2

This is not correct. We should be using https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/volt.... Please see my comment: https://b.corp.google.com/issues/152827558#comment10

Thanks, Furquan. That comment says that data in the SPD is incorrect. "As per FSP code, this should be 0x21 (i.e. 16 rows, 10 columns). Currently checked in coreboot file is wrong."

But it also says that the currently checked in SPD file is the incorrect one, which would mean that this is the correct one, no? If so, I will try out this SPD and then fix the currently checked in one accordingly and abandon this CL.

Yes, there are some bytes as mentioned on the bug that you should raise a bug against Intel to check the reason behind using those values.

-- To view, visit https://review.coreboot.org/c/coreboot/+/40108 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4bda759a88d7d3ffd10ed120cc8e7f0e5d289a34 Gerrit-Change-Number: 40108 Gerrit-PatchSet: 3 Gerrit-Owner: Nick Vaccaro nvaccaro@google.com Gerrit-Reviewer: Caveh Jalali caveh@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jes Klinke jbk@google.com Gerrit-Reviewer: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sun, 05 Apr 2020 08:58:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

8:58 a.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40108 )

Change subject: mb/google/volteer: add new generic SPD file ......................................................................

Patch Set 3:

...

Patch Set 3:

...
Patch Set 3:

...
Patch Set 3: Code-Review-2

This is not correct. We should be using https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/volt.... Please see my comment: https://b.corp.google.com/issues/152827558#comment10

Thanks, Furquan. That comment says that data in the SPD is incorrect. "As per FSP code, this should be 0x21 (i.e. 16 rows, 10 columns). Currently checked in coreboot file is wrong."

But it also says that the currently checked in SPD file is the incorrect one, which would mean that this is the correct one, no? If so, I will try out this SPD and then fix the currently checked in one accordingly and abandon this CL.

It's surprising that bus width could be wrong and it still work (for the checked-in SPD).

I think it is probably because FSP hardcodes some of the bytes as per its expectations.

-- To view, visit https://review.coreboot.org/c/coreboot/+/40108 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4bda759a88d7d3ffd10ed120cc8e7f0e5d289a34 Gerrit-Change-Number: 40108 Gerrit-PatchSet: 3 Gerrit-Owner: Nick Vaccaro nvaccaro@google.com Gerrit-Reviewer: Caveh Jalali caveh@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jes Klinke jbk@google.com Gerrit-Reviewer: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sun, 05 Apr 2020 08:58:56 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Nick Vaccaro (Code Review)

6 Apr 6 Apr

4:14 p.m.

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40108 )

Change subject: mb/google/volteer: add new generic SPD file ......................................................................

Patch Set 3:

...

Patch Set 3:

...
Patch Set 3:

...
Patch Set 3:

...
Patch Set 3: Code-Review-2

This is not correct. We should be using https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/volt.... Please see my comment: https://b.corp.google.com/issues/152827558#comment10

Thanks, Furquan. That comment says that data in the SPD is incorrect. "As per FSP code, this should be 0x21 (i.e. 16 rows, 10 columns). Currently checked in coreboot file is wrong."

But it also says that the currently checked in SPD file is the incorrect one, which would mean that this is the correct one, no? If so, I will try out this SPD and then fix the currently checked in one accordingly and abandon this CL.

It's surprising that bus width could be wrong and it still work (for the checked-in SPD).

I think it is probably because FSP hardcodes some of the bytes as per its expectations.

I asked the memory team for a list of important data that was used from the SPD for determining generic SPD fields and that field was on the "needs" list.

Did you note that the comment says that the SPD file that is currently checked in is the incorrect one. Do you concur?

-- To view, visit https://review.coreboot.org/c/coreboot/+/40108 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4bda759a88d7d3ffd10ed120cc8e7f0e5d289a34 Gerrit-Change-Number: 40108 Gerrit-PatchSet: 3 Gerrit-Owner: Nick Vaccaro nvaccaro@google.com Gerrit-Reviewer: Caveh Jalali caveh@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jes Klinke jbk@google.com Gerrit-Reviewer: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 06 Apr 2020 16:14:33 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Nick Vaccaro (Code Review)

5:56 p.m.

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40108 )

Change subject: mb/google/volteer: add new generic SPD file ......................................................................

Patch Set 3:

Field 13 is incorrect both in this SPD as well as the checked-in SPD, which made this SPD look different than the DRAM ID 0 spd currently checked in for volteer.

I will fix the bug in the currently checked in SPD and abandon this CL as not needed.

-- To view, visit https://review.coreboot.org/c/coreboot/+/40108 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4bda759a88d7d3ffd10ed120cc8e7f0e5d289a34 Gerrit-Change-Number: 40108 Gerrit-PatchSet: 3 Gerrit-Owner: Nick Vaccaro nvaccaro@google.com Gerrit-Reviewer: Caveh Jalali caveh@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jes Klinke jbk@google.com Gerrit-Reviewer: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 06 Apr 2020 17:56:20 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Nick Vaccaro (Code Review)

5:56 p.m.

Nick Vaccaro has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40108 )

Change subject: mb/google/volteer: add new generic SPD file ......................................................................

Abandoned

-- To view, visit https://review.coreboot.org/c/coreboot/+/40108 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4bda759a88d7d3ffd10ed120cc8e7f0e5d289a34 Gerrit-Change-Number: 40108 Gerrit-PatchSet: 3 Gerrit-Owner: Nick Vaccaro nvaccaro@google.com Gerrit-Reviewer: Caveh Jalali caveh@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jes Klinke jbk@google.com Gerrit-Reviewer: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: abandon

1507

days inactive

1510

days old

coreboot-gerrit@coreboot.org

11 comments

3 participants

tags (0)

participants (3)

Caveh Jalali (Code Review)
Furquan Shaikh (Code Review)
Nick Vaccaro (Code Review)