Change in coreboot[master]: mb/intel/adlrvp: Refactor lpddr4_mem_config structure

[Subrata Banik (Code Review)]

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

mb/intel/adlrvp: Refactor lpddr4_mem_config structure

List of changes:
1. Initialize dq_map array in a single line
2. Make dqs_map array also in a single line

TEST=Able to build and boot ADLRVP LP4 SKU.

Change-Id: I64f2b38492934c8ede301f4b252c8700060ed4ac
Signed-off-by: Subrata Banik 
---
M src/mainboard/intel/adlrvp/memory.c
1 file changed, 9 insertions(+), 19 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/48077/1

diff --git a/src/mainboard/intel/adlrvp/memory.c b/src/mainboard/intel/adlrvp/memory.c
index cab4ef9..c25b245 100644
--- a/src/mainboard/intel/adlrvp/memory.c
+++ b/src/mainboard/intel/adlrvp/memory.c
@@ -22,29 +22,19 @@
 static const struct mb_cfg lpddr4_mem_config = {
    /* DQ byte map */
    .dq_map = {
-	{   0,  2,   3,  1,   6,   7,   5,   4,    /* Byte 0 */
-	   10,  8,  11,  9,  14,  12,  13,  15 },  /* Byte 1 */
-	{  12,  8,  14, 10,  11,  13,  15,   9,    /* Byte 2 */
-	    5,  0,   7,  3,   6,   2,   1,   4 },  /* Byte 3 */
-	{   3,  0,   2,  1,   6,   5,   4,   7,    /* Byte 4 */
-	   12, 13,  14, 15,  10,   9,   8,  11 },  /* Byte 5 */
-	{   2,  6,   7,  1,   3,   4,   0,   5,    /* Byte 6 */
-	    9, 13,   8, 15,  14,  11,  12,  10 },  /* Byte 7 */
-	{   3,  0,   1,  2,   7,   4,   6,   5,    /* Byte 0 */
-	   10,  8,  11,  9,  14,  13,  12,  15 },  /* Byte 1 */
-	{  10, 12,  14,  8,   9,  13,  15,  11,    /* Byte 2 */
-	    3,  7,   6,  2,   0,   4,   5,  1 },   /* Byte 3 */
-	{  12, 15,  14, 13,   9,  10,  11,  8,     /* Byte 4 */
-	    7,  4,   6,  5,   0,   1,   3,  2 },   /* Byte 5 */
-	{   0,  2,   4,  3,   1,   6,   7,  5,     /* Byte 6 */
-	   13,  9,  10, 11,   8,  12,  14, 15 },   /* Byte 7 */
+		{ 0, 2, 3, 1, 6, 7, 5, 4, 10, 8, 11, 9, 14, 12, 13, 15 },
+		{ 12, 8, 14, 10, 11, 13, 15, 9, 5, 0, 7, 3, 6, 2, 1, 4 },
+		{ 3, 0, 2, 1, 6, 5, 4, 7, 12, 13, 14, 15, 10, 9, 8, 11 },
+		{ 2, 6, 7, 1, 3, 4, 0, 5, 9, 13, 8, 15, 14, 11, 12, 10 },
+		{ 3, 0, 1, 2, 7, 4, 6, 5, 10, 8, 11, 9, 14, 13, 12, 15 },
+		{ 10, 12, 14, 8, 9, 13, 15, 11, 3, 7, 6, 2, 0, 4, 5, 1 },
+		{ 12, 15, 14, 13, 9, 10, 11, 8, 7, 4, 6, 5, 0, 1, 3, 2 },
+		{ 0, 2, 4, 3, 1, 6, 7, 5, 13, 9, 10, 11, 8, 12, 14, 15 },
    },

/* DQS CPU<>DRAM map */
    .dqs_map = {
-	/* Ch 0     1         2         3  */
-	{ 0, 1 }, { 1, 0 }, { 0, 1 }, { 0, 1 },
-	{ 0, 1 }, { 1, 0 }, { 1, 0 }, { 0, 1 }
+		{ 0, 1 }, { 1, 0 }, { 0, 1 }, { 0, 1 }, { 0, 1 }, { 1, 0 }, { 1, 0 }, { 0, 1 }
    },

.dq_pins_interleaved = false,

[Subrata Banik (Code Review)]

Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Meera Ravindranath, Aamir Bohra,

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

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

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

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

mb/intel/adlrvp: Refactor lpddr4_mem_config structure

List of changes:
1. Initialize dq_map array in a single line
2. Make dqs_map array also in a single line

TEST=Able to build and boot ADLRVP LP4 SKU.

Change-Id: I64f2b38492934c8ede301f4b252c8700060ed4ac
Signed-off-by: Subrata Banik 
---
M src/mainboard/intel/adlrvp/memory.c
1 file changed, 9 insertions(+), 19 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/48077/2

[Sridhar Siricilla (Code Review)]

Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 2: Code-Review+1

[V Sowmya (Code Review)]

V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 2: Code-Review+2

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 2:

@angel if you can take a look into this patchset. waiting for your +2 before i merge this entire tree

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 2: Code-Review+2

I've never dealt with LPDDR DQ/DQS mapping myself, so I'm not sure which is the best way to describe them in code. I'm fine with this change.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48077/2/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/memory.c:

https://review.coreboot.org/c/coreboot/+/48077/2/src/mainboard/intel/adlrvp/... 
PS2, Line 32: 		{ 0, 2, 4, 3, 1, 6, 7, 5, 13, 9, 10, 11, 8, 12, 14, 15 },
Uh, just noticed. Mind adding a space to the left of single-digit numbers, please? The resulting alignment should be much more readable.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48077/2/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/memory.c:

https://review.coreboot.org/c/coreboot/+/48077/2/src/mainboard/intel/adlrvp/... 
PS2, Line 32: 		{ 0, 2, 4, 3, 1, 6, 7, 5, 13, 9, 10, 11, 8, 12, 14, 15 },
> Uh, just noticed. […]

Ack

[Subrata Banik (Code Review)]

Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Sridhar Siricilla, Meera Ravindranath, Aamir Bohra,

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

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

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

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

mb/intel/adlrvp: Refactor lpddr4_mem_config structure

List of changes:
1. Initialize dq_map array in a single line
2. Make dqs_map array also in a single line

TEST=Able to build and boot ADLRVP LP4 SKU.

Change-Id: I64f2b38492934c8ede301f4b252c8700060ed4ac
Signed-off-by: Subrata Banik 
---
M src/mainboard/intel/adlrvp/memory.c
1 file changed, 9 insertions(+), 19 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/48077/3

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 4: Code-Review+2

[V Sowmya (Code Review)]

V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 4: Code-Review+2

[Furquan Shaikh (Code Review)]

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

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48077/4/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/memory.c:

https://review.coreboot.org/c/coreboot/+/48077/4/src/mainboard/intel/adlrvp/... 
PS4, Line 25: {  0,  2,  3,  1,  6,  7,  5,  4, 10,  8, 11,  9, 14, 12, 13, 15 },
What is the reason behind this? I think it makes it easier to read and understand if associated with the hardware names that explains what these numbers really mean. Example: https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/volt...

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48077/4/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/memory.c:

https://review.coreboot.org/c/coreboot/+/48077/4/src/mainboard/intel/adlrvp/... 
PS4, Line 25: {  0,  2,  3,  1,  6,  7,  5,  4, 10,  8, 11,  9, 14, 12, 13, 15 },
> What is the reason behind this? I think it makes it easier to read and understand if associated with […]

I believe Paul had requested for this to make sure it act as an array and nothing else.
but i would believe that doesn't crate any problem to represent as earlier format and new format, we have to assign DQ byte, thats only matter most.

[Furquan Shaikh (Code Review)]

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

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48077/4/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/memory.c:

https://review.coreboot.org/c/coreboot/+/48077/4/src/mainboard/intel/adlrvp/... 
PS4, Line 25: {  0,  2,  3,  1,  6,  7,  5,  4, 10,  8, 11,  9, 14, 12, 13, 15 },
> I believe Paul had requested for this to make sure it act as an array and nothing else. […]

Yes, it doesn't make sense from a functional standpoint. Maybe not for adlrvp, but for Chrome OS board, it is very important to have this structured such that it is easy to relate it to the hardware schematics. It is helpful for partners as they add their own variants.

[Subrata Banik (Code Review)]

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

mb/intel/adlrvp: Refactor lpddr4_mem_config structure

List of changes:
1. Initialize dq_map array in a single line
2. Make dqs_map array also in a single line

TEST=Able to build and boot ADLRVP LP4 SKU.

Change-Id: I64f2b38492934c8ede301f4b252c8700060ed4ac
Signed-off-by: Subrata Banik 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48077
Reviewed-by: Angel Pons 
Reviewed-by: V Sowmya 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/intel/adlrvp/memory.c
1 file changed, 9 insertions(+), 19 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  V Sowmya: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/mainboard/intel/adlrvp/memory.c b/src/mainboard/intel/adlrvp/memory.c
index 5d374db..b203f69 100644
--- a/src/mainboard/intel/adlrvp/memory.c
+++ b/src/mainboard/intel/adlrvp/memory.c
@@ -22,29 +22,19 @@
 static const struct mb_cfg lpddr4_mem_config = {
    /* DQ byte map */
    .dq_map = {
-	{   0,  2,   3,  1,   6,   7,   5,   4,    /* Byte 0 */
-	   10,  8,  11,  9,  14,  12,  13,  15 },  /* Byte 1 */
-	{  12,  8,  14, 10,  11,  13,  15,   9,    /* Byte 2 */
-	    5,  0,   7,  3,   6,   2,   1,   4 },  /* Byte 3 */
-	{   3,  0,   2,  1,   6,   5,   4,   7,    /* Byte 4 */
-	   12, 13,  14, 15,  10,   9,   8,  11 },  /* Byte 5 */
-	{   2,  6,   7,  1,   3,   4,   0,   5,    /* Byte 6 */
-	    9, 13,   8, 15,  14,  11,  12,  10 },  /* Byte 7 */
-	{   3,  0,   1,  2,   7,   4,   6,   5,    /* Byte 0 */
-	   10,  8,  11,  9,  14,  13,  12,  15 },  /* Byte 1 */
-	{  10, 12,  14,  8,   9,  13,  15,  11,    /* Byte 2 */
-	    3,  7,   6,  2,   0,   4,   5,  1 },   /* Byte 3 */
-	{  12, 15,  14, 13,   9,  10,  11,  8,     /* Byte 4 */
-	    7,  4,   6,  5,   0,   1,   3,  2 },   /* Byte 5 */
-	{   0,  2,   4,  3,   1,   6,   7,  5,     /* Byte 6 */
-	   13,  9,  10, 11,   8,  12,  14, 15 },   /* Byte 7 */
+		{  0,  2,  3,  1,  6,  7,  5,  4, 10,  8, 11,  9, 14, 12, 13, 15 },
+		{ 12,  8, 14, 10, 11, 13, 15,  9,  5,  0,  7,  3,  6,  2,  1,  4 },
+		{  3,  0,  2,  1,  6,  5,  4,  7, 12, 13, 14, 15, 10,  9,  8, 11 },
+		{  2,  6,  7,  1,  3,  4,  0,  5,  9, 13,  8, 15, 14, 11, 12, 10 },
+		{  3,  0,  1,  2,  7,  4,  6,  5, 10,  8, 11,  9, 14, 13, 12, 15 },
+		{ 10, 12, 14,  8,  9, 13, 15, 11,  3,  7,  6,  2,  0,  4,  5,  1 },
+		{ 12, 15, 14, 13,  9, 10, 11,  8,  7,  4,  6,  5,  0,  1,  3,  2 },
+		{  0,  2,  4,  3,  1,  6,  7,  5, 13,  9, 10, 11,  8, 12, 14, 15 },
    },

/* DQS CPU<>DRAM map */
    .dqs_map = {
-	/* Ch 0     1         2         3  */
-	{ 0, 1 }, { 1, 0 }, { 0, 1 }, { 0, 1 },
-	{ 0, 1 }, { 1, 0 }, { 1, 0 }, { 0, 1 }
+		{ 0, 1 }, { 1, 0 }, { 0, 1 }, { 0, 1 }, { 0, 1 }, { 1, 0 }, { 1, 0 }, { 0, 1 }
    },

.dq_pins_interleaved = false,

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48077/4/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/memory.c:

https://review.coreboot.org/c/coreboot/+/48077/4/src/mainboard/intel/adlrvp/... 
PS4, Line 25: {  0,  2,  3,  1,  6,  7,  5,  4, 10,  8, 11,  9, 14, 12, 13, 15 },
> Yes, it doesn't make sense from a functional standpoint. […]

For volteer, how about using a struct instead? This would allow each group of 8 numbers to be labelled .dq0 or .dq1 without comments.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 5:

Why was this merged? Furquan has a point.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 5:
> Patch Set 5:
>
> Why was this merged? Furquan has a point.

oops, i thought Furquan said, lets not replicate this for Google ADL boards where partners need more configurability

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 5:
> Patch Set 5:
> > Patch Set 5:
> >
> > Why was this merged? Furquan has a point.
>
> oops, i thought Furquan said, lets not replicate this for Google ADL boards where partners need more configurability

Alright. I think we can wait until some Google ADL boards are in the tree, then restructure memcfg to better reflect LPDDR byte mapping logic.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48077 )

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 5:
> Patch Set 5:
> > Patch Set 5:
> > > Patch Set 5:
> > >
> > > Why was this merged? Furquan has a point.
> >
> > oops, i thought Furquan said, lets not replicate this for Google ADL boards where partners need more configurability
>
> Alright. I think we can wait until some Google ADL boards are in the tree, then restructure memcfg to better reflect LPDDR byte mapping logic.

SG to me 👍

[Furquan Shaikh (Code Review)]

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

Change subject: mb/intel/adlrvp: Refactor lpddr4_mem_config structure
......................................................................

Patch Set 5:

(1 comment)
> Patch Set 5:
> > Patch Set 5:
> > > Patch Set 5:
> > >
> > > Why was this merged? Furquan has a point.
> >
> > oops, i thought Furquan said, lets not replicate this for Google ADL boards where partners need more configurability
>
> Alright. I think we can wait until some Google ADL boards are in the tree, then restructure memcfg to better reflect LPDDR byte mapping logic.

Yes, let's do that. I think it is important to keep the translation from schematics to mainboard description as simple and straightfoward as possible. The complexities can be hidden inside SoC code to translate into appropriate UPDs, etc. This makes life easier for mainboard developers.

https://review.coreboot.org/c/coreboot/+/48077/4/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/memory.c:

https://review.coreboot.org/c/coreboot/+/48077/4/src/mainboard/intel/adlrvp/... 
PS4, Line 25: {  0,  2,  3,  1,  6,  7,  5,  4, 10,  8, 11,  9, 14, 12, 13, 15 },
> For volteer, how about using a struct instead? This would allow each group of 8 numbers to be labell […]

Yeah, that would make sense. We will have to go back and clean things up as new boards get added.