Change in coreboot[master]: mb/intel/adlrvp: Fix wrong comments and typo

[Subrata Banik (Code Review)]

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

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

mb/intel/adlrvp: Fix wrong comments and typo

1. CPU -> PCH
2. PCU -> PCH

Change-Id: I92123450bd7cfb2e70aae8de03053672a7772451
Signed-off-by: Subrata Banik 
---
M src/mainboard/intel/adlrvp/devicetree.cb
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/49136/1

diff --git a/src/mainboard/intel/adlrvp/devicetree.cb b/src/mainboard/intel/adlrvp/devicetree.cb
index 12cd475..d3111d8 100644
--- a/src/mainboard/intel/adlrvp/devicetree.cb
+++ b/src/mainboard/intel/adlrvp/devicetree.cb
@@ -68,10 +68,10 @@
    # Hybrid storage mode
    register "HybridStorageMode" = "1"

-	# Enable CPU PCIE RP 1 using PEG CLK 0
+	# Enable PCH PCIE RP 1 using PEG CLK 0
    register "PcieClkSrcUsage[0]" = "0x40"

-	# Enable PCU PCIE PEG Slot 1 and 2
+	# Enable PCH PCIE PEG Slot 1 and 2
    register "PcieClkSrcUsage[3]" = "0x41"
    register "PcieClkSrcUsage[4]" = "0x42"

[Angel Pons (Code Review)]

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

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/c/coreboot/+/49136/1/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/1/src/mainboard/intel/adlrvp/... 
PS1, Line 72: 	register "PcieClkSrcUsage[0]" = "0x40"
As far as I understand it, this configures PCH CLKSRC 0 to be used for CPU PCIe RP 1.

https://review.coreboot.org/c/coreboot/+/49136/1/src/mainboard/intel/adlrvp/... 
PS1, Line 75: 	register "PcieClkSrcUsage[3]" = "0x41"
Same here, as far as I understand it, this configures PCH CLKSRC 3 to be used for CPU PCIe RP 2.

https://review.coreboot.org/c/coreboot/+/49136/1/src/mainboard/intel/adlrvp/... 
PS1, Line 76: 	register "PcieClkSrcUsage[4]" = "0x42"
Same here, as far as I understand it, this configures PCH CLKSRC 4 to be used for CPU PCIe RP 3.

[Subrata Banik (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons,

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

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

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

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

mb/intel/adlrvp: Fix wrong comments and typo

PCU -> CPU

Change-Id: I92123450bd7cfb2e70aae8de03053672a7772451
Signed-off-by: Subrata Banik 
---
M src/mainboard/intel/adlrvp/devicetree.cb
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/49136/2

[Subrata Banik (Code Review)]

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

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/49136/1/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/1/src/mainboard/intel/adlrvp/... 
PS1, Line 72: 	register "PcieClkSrcUsage[0]" = "0x40"
> As far as I understand it, this configures PCH CLKSRC 0 to be used for CPU PCIe RP 1.

you are right, this is all due to wrong naming in FSP, ideally this is CPU RP 1 getting PEG clk 0

[Furquan Shaikh (Code Review)]

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

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
PS2, Line 74:  Enable CPU PCIE PEG Slot 1 and 2
But, you are not setting CpuPcieRpEnableMask for ports 1 and 2. Do these configurations for PcieClkSrcUsage actually do anything at all?

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
PS2, Line 74:  Enable CPU PCIE PEG Slot 1 and 2
> But, you are not setting CpuPcieRpEnableMask for ports 1 and 2. […]

IIRC, using a value with bit 6 ORed in (0x40, etc.) means that it's for a CPU PCIe port; however, the only configuration I see enabled for CPU PCIe is 00:06.0.

[Furquan Shaikh (Code Review)]

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

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
PS2, Line 74:  Enable CPU PCIE PEG Slot 1 and 2
> IIRC, using a value with bit 6 ORed in (0x40, etc. […]

Exactly. I don't really understand this configuration for CLKSRC3 and CLKSRC4.

[Subrata Banik (Code Review)]

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

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
PS2, Line 74:  Enable CPU PCIE PEG Slot 1 and 2
> Exactly. I don't really understand this configuration for CLKSRC3 and CLKSRC4.

There are 3 CPU PCIE port, below is CLKREQ->CLKSRC for those ports 
1. PEG 0:6:0 => 0x40
2. PEG 0:1:0 => 0x41
3. PEG 0.6.2 => 0x42

[Furquan Shaikh (Code Review)]

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

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
PS2, Line 74:  Enable CPU PCIE PEG Slot 1 and 2
> There are 3 CPU PCIE port, below is CLKREQ->CLKSRC for those ports  […]

But, 0/1/0 and 0/6/2 are never enabled in devicetree or in the CpuPcieRpEnableMask?

[Subrata Banik (Code Review)]

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

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
PS2, Line 74:  Enable CPU PCIE PEG Slot 1 and 2
> But, 0/1/0 and 0/6/2 are never enabled in devicetree or in the CpuPcieRpEnableMask?

We need to enable incrementally, i have to clean up CpuPciRpMask parameter as well.

[Furquan Shaikh (Code Review)]

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

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
PS2, Line 74:  Enable CPU PCIE PEG Slot 1 and 2
> > But, 0/1/0 and 0/6/2 are never enabled in devicetree or in the CpuPcieRpEnableMask? […]

I think Eric has done most of the work here: https://review.coreboot.org/c/coreboot/+/48340. He is just waiting for confirmation from you.

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/2/src/mainboard/intel/adlrvp/... 
PS2, Line 74:  Enable CPU PCIE PEG Slot 1 and 2
> I think Eric has done most of the work here: https://review.coreboot.org/c/coreboot/+/48340. […]

Yes, maybe enable 0/1/0 and 0/6/2 in this CL. And we can do the reset :)

[Subrata Banik (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, EricR Lai.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

Patch Set 3:

(1 comment)

File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/comment/adfb3d98_3d3aaedb 
PS2, Line 74:  Enable CPU PCIE PEG Slot 1 and 2
> Yes, maybe enable 0/1/0 and 0/6/2 in this CL. […]

can we use this CL just to fix the typo ? and another one to enable PEG 10 and PEG62 port ?

[Angel Pons (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, EricR Lai.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

Patch Set 3:

(1 comment)

File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/comment/b964eb58_fecc7525 
PS2, Line 74:  Enable CPU PCIE PEG Slot 1 and 2
> can we use this CL just to fix the typo ? and another one to enable PEG 10 and PEG62 port ?

IMHO, it would make more sense to change the comments in the same patch that enables the corresponding PEG ports

[Subrata Banik (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Angel Pons, EricR Lai.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: mb/intel/adlrvp: Fix wrong comments and typo
......................................................................

Patch Set 3:

(1 comment)

File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/comment/4a055a81_40f44fb8 
PS2, Line 74:  Enable CPU PCIE PEG Slot 1 and 2
> IMHO, it would make more sense to change the comments in the same patch that enables the correspondi […]

Sure on it. let me try to update the CL by EOD

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Subrata Banik (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Angel Pons, EricR Lai.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, EricR Lai,

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

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

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

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

List of changes:
1. Create new Kconfig MAX_CPU_ROOT_PORTS and MAX_PCH_ROOT_PORTS as per EDS.
2. Add new chip variable to enable/disable CPU PCIE RPs from mainboards.
3. Rename PcieRpEnable to PchPcieRpEnable.
4. Enable PEG10 and CPU SSD2 slot(PEG62) in mainboard devicetree.cb

Change-Id: I92123450bd7cfb2e70aae8de03053672a7772451
Signed-off-by: Subrata Banik 
---
M src/mainboard/intel/adlrvp/devicetree.cb
M src/soc/intel/alderlake/Kconfig
M src/soc/intel/alderlake/chip.h
M src/soc/intel/alderlake/fsp_params.c
M src/soc/intel/alderlake/romstage/fsp_params.c
5 files changed, 44 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/49136/4

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Subrata Banik (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph, EricR Lai.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph, EricR Lai,

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

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

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

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

List of changes:
1. Create new Kconfig MAX_CPU_ROOT_PORTS and MAX_PCH_ROOT_PORTS as per EDS.
2. Add new chip variable to enable/disable CPU PCIE RPs from mainboards.
3. Rename PcieRpEnable to PchPcieRpEnable.
4. Enable PEG10 and CPU SSD2 slot(PEG62) in mainboard devicetree.cb.

Change-Id: I92123450bd7cfb2e70aae8de03053672a7772451
Signed-off-by: Subrata Banik 
---
M src/mainboard/intel/adlrvp/devicetree.cb
M src/soc/intel/alderlake/Kconfig
M src/soc/intel/alderlake/chip.h
M src/soc/intel/alderlake/fsp_params.c
M src/soc/intel/alderlake/romstage/fsp_params.c
5 files changed, 41 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/49136/5

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[EricR Lai (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, Angel Pons, Patrick Rudolph.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 4: Code-Review+1

(1 comment)

Patchset:

PS4: 
Could you add comment like mine to describe RP1/2/3 for better reference 😊
RP1: 0/6/0
RP2: 0/1/0 (PCIE 5.0)
RP3: 0/6/2

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Subrata Banik (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph, EricR Lai.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 5:

(4 comments)

File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/comment/a951de51_4fc8df5e 
PS1, Line 72: 	register "PcieClkSrcUsage[0]" = "0x40"
> you are right, this is all due to wrong naming in FSP, ideally this is CPU RP 1 getting PEG clk 0

Ack

https://review.coreboot.org/c/coreboot/+/49136/comment/a4b50201_0d9bf664 
PS1, Line 75: 	register "PcieClkSrcUsage[3]" = "0x41"
> Same here, as far as I understand it, this configures PCH CLKSRC 3 to be used for CPU PCIe RP 2.

Ack

https://review.coreboot.org/c/coreboot/+/49136/comment/06b46da8_ec9c2f74 
PS1, Line 76: 	register "PcieClkSrcUsage[4]" = "0x42"
> Same here, as far as I understand it, this configures PCH CLKSRC 4 to be used for CPU PCIe RP 3.

Ack

File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/comment/75f1fb3f_2997f58a 
PS2, Line 74:  Enable CPU PCIE PEG Slot 1 and 2
> Sure on it. […]

Ack

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Angel Pons (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Patrick Rudolph, EricR Lai.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 5: Code-Review+1

(2 comments)

File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/comment/feedf21f_90b5ad61 
PS5, Line 75: 1
Code says CLK 3. Also, I would not say `PEG CLK` since the clocks come from the PCH, just `CLK` is enough.

https://review.coreboot.org/c/coreboot/+/49136/comment/9c456168_2919d342 
PS5, Line 79: 2
Code says CLK 4

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Subrata Banik (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Patrick Rudolph, EricR Lai.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph, EricR Lai,

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

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

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

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

List of changes:
1. Create new Kconfig MAX_CPU_ROOT_PORTS and MAX_PCH_ROOT_PORTS as per EDS.
2. Add new chip variable to enable/disable CPU PCIE RPs from mainboards.
3. Rename PcieRpEnable to PchPcieRpEnable.
4. Enable PEG10 and CPU SSD2 slot(PEG62) in mainboard devicetree.cb.

Change-Id: I92123450bd7cfb2e70aae8de03053672a7772451
Signed-off-by: Subrata Banik 
---
M src/mainboard/intel/adlrvp/devicetree.cb
M src/soc/intel/alderlake/Kconfig
M src/soc/intel/alderlake/chip.h
M src/soc/intel/alderlake/fsp_params.c
M src/soc/intel/alderlake/romstage/fsp_params.c
5 files changed, 42 insertions(+), 21 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/49136/6

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Subrata Banik (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph, EricR Lai.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 6:

(2 comments)

File src/mainboard/intel/adlrvp/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/49136/comment/96ae088b_15237534 
PS5, Line 75: 1
> Code says CLK 3. […]

Ack

https://review.coreboot.org/c/coreboot/+/49136/comment/3ce3f2c6_8be50b42 
PS5, Line 79: 2
> Code says CLK 4

Ack

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[EricR Lai (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, Angel Pons, Patrick Rudolph.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 6:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/49136/comment/7c3ab6bd_2702bc35 
PS6, Line 14: 
Maybe add some comment for CPU ports as well. If I was wrong you can correct it.
RP1: 0/6/0
RP2: 0/1/0 (PCIE 5.0)
RP3: 0/6/2

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[EricR Lai (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, Angel Pons, Patrick Rudolph.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 6: Code-Review+2

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Subrata Banik (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, Angel Pons, Patrick Rudolph.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph, EricR Lai,

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

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

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

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

List of changes:
1. Create new Kconfig MAX_CPU_ROOT_PORTS and MAX_PCH_ROOT_PORTS as per
EDS.
2. Add new chip variable to enable/disable CPU PCIE RPs from mainboards.
3. Rename PcieRpEnable to PchPcieRpEnable.
4. Enable CPU RPs as below in mainboard devicetree.cb

RP1: PEG60 : 0:6:0 : CPU SSD1
RP2: PEG10 : 0:1:0 : x8 CPU Slot
RP3: PEG62 : 0:6:2 : CPU SSD2

Change-Id: I92123450bd7cfb2e70aae8de03053672a7772451
Signed-off-by: Subrata Banik 
---
M src/mainboard/intel/adlrvp/devicetree.cb
M src/soc/intel/alderlake/Kconfig
M src/soc/intel/alderlake/chip.h
M src/soc/intel/alderlake/fsp_params.c
M src/soc/intel/alderlake/romstage/fsp_params.c
5 files changed, 42 insertions(+), 21 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/49136/7

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Subrata Banik (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 7:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/49136/comment/4881049f_dd620ff8 
PS6, Line 14:
> Maybe add some comment for CPU ports as well. If I was wrong you can correct it. […]

Ack

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Nico Huber (Code Review)]

Attention is currently required from:  Felix Singer, Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, Angel Pons, Patrick Rudolph.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 7:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/49136/comment/1a0c646f_4df447de 
PS7, Line 12: 2. Add new chip variable to enable/disable CPU PCIE RPs from mainboards.
Any reason to not use the devicetree on/off state instead? e.g. is_dev_enabled()

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Subrata Banik (Code Review)]

Attention is currently required from:  Felix Singer, Nico Huber, Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 7:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/49136/comment/259591dd_c10565a3 
PS7, Line 12: 2. Add new chip variable to enable/disable CPU PCIE RPs from mainboards.
> Any reason to not use the devicetree on/off state instead? e.g. […]

Just because of this code Nico, if we use is_dev_enabled() then iterative case is not possible.

mask = 0;
    for (i = 0; i < ARRAY_SIZE(config->CpuPcieRpEnable); i++) {
    	if (config->CpuPcieRpEnable[i])
    		mask |= (1 << i);
    }
    m_cfg->CpuPcieRpEnableMask = mask;

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Nico Huber (Code Review)]

Attention is currently required from:  Felix Singer, Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, Angel Pons, Patrick Rudolph.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 7:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/49136/comment/901720df_21f29341 
PS7, Line 12: 2. Add new chip variable to enable/disable CPU PCIE RPs from mainboards.
> Just because of this code Nico, if we use is_dev_enabled() then iterative case is not possible.   […]

I don't follow. You can loop over device and function numbers as well and look
the device nodes up.

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Subrata Banik (Code Review)]

Attention is currently required from:  Felix Singer, Nico Huber, Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 7:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/49136/comment/8efe09f6_1895a9d4 
PS7, Line 12: 2. Add new chip variable to enable/disable CPU PCIE RPs from mainboards.
> I don't follow. You can loop over device and function numbers as well and look […]

let me try something

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[EricR Lai (Code Review)]

Attention is currently required from:  Felix Singer, Nico Huber, Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, Angel Pons, Patrick Rudolph.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 7:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/49136/comment/3f10ff17_1ae2e7cd 
PS7, Line 12: 2. Add new chip variable to enable/disable CPU PCIE RPs from mainboards.
> let me try something

@Nico, this will deal in my patch :)

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[EricR Lai (Code Review)]

Attention is currently required from:  Felix Singer, Nico Huber, Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, Angel Pons, Patrick Rudolph.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 7:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/49136/comment/6a0c3a6e_09c39e88 
PS7, Line 12: 2. Add new chip variable to enable/disable CPU PCIE RPs from mainboards.
> @Nico, this will deal in my patch :)

@Subrata, Furquan and I are working on it. So don't worry :p

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Nico Huber (Code Review)]

Attention is currently required from:  Felix Singer, Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, Angel Pons, Patrick Rudolph.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 7:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/49136/comment/4c948947_2bd51646 
PS7, Line 12: 2. Add new chip variable to enable/disable CPU PCIE RPs from mainboards.
> @Subrata, Furquan and I are working on it. […]

Ack. Thanks for taking care of it.

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Nico Huber (Code Review)]

Attention is currently required from:  Felix Singer, Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, Angel Pons, Patrick Rudolph.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 7:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/49136/comment/ef70c9d3_664b5002 
PS7, Line 16: RP1: PEG60 : 0:6:0 : CPU SSD1
            : RP2: PEG10 : 0:1:0 : x8 CPU Slot
            : RP3: PEG62 : 0:6:2 : CPU SSD2
This seems to match ADL-P only. Is ADL-S generally not supported?

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Subrata Banik (Code Review)]

Attention is currently required from:  Felix Singer, Nico Huber, Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49136 )

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

Patch Set 7:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/49136/comment/f0b5b6eb_4ce4dfe5 
PS7, Line 16: RP1: PEG60 : 0:6:0 : CPU SSD1
            : RP2: PEG10 : 0:1:0 : x8 CPU Slot
            : RP3: PEG62 : 0:6:2 : CPU SSD2
> This seems to match ADL-P only. […]

ADL-S has one extra PEG11, will add ADL-S later in year

Change in coreboot[master]: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs
......................................................................

soc/intel/alderlake: Refactor SoC code to maintain CPU and PCH PCIE RPs

List of changes:
1. Create new Kconfig MAX_CPU_ROOT_PORTS and MAX_PCH_ROOT_PORTS as per
EDS.
2. Add new chip variable to enable/disable CPU PCIE RPs from mainboards.
3. Rename PcieRpEnable to PchPcieRpEnable.
4. Enable CPU RPs as below in mainboard devicetree.cb

RP1: PEG60 : 0:6:0 : CPU SSD1
RP2: PEG10 : 0:1:0 : x8 CPU Slot
RP3: PEG62 : 0:6:2 : CPU SSD2

Change-Id: I92123450bd7cfb2e70aae8de03053672a7772451
Signed-off-by: Subrata Banik 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/49136
Reviewed-by: EricR Lai 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/intel/adlrvp/devicetree.cb
M src/soc/intel/alderlake/Kconfig
M src/soc/intel/alderlake/chip.h
M src/soc/intel/alderlake/fsp_params.c
M src/soc/intel/alderlake/romstage/fsp_params.c
5 files changed, 42 insertions(+), 21 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  EricR Lai: Looks good to me, approved

diff --git a/src/mainboard/intel/adlrvp/devicetree.cb b/src/mainboard/intel/adlrvp/devicetree.cb
index 12cd475..51b781c 100644
--- a/src/mainboard/intel/adlrvp/devicetree.cb
+++ b/src/mainboard/intel/adlrvp/devicetree.cb
@@ -40,39 +40,44 @@
    register "PrmrrSize" = "0"

# Enable PCH PCIE RP 5 using CLK 2
-	register "PcieRpEnable[4]" = "1"
+	register "PchPcieRpEnable[4]" = "1"
    register "PcieClkSrcClkReq[2]" = "2"
    register "PcieClkSrcUsage[2]" = "0x4"
    register "PcieRpClkReqDetect[4]" = "1"

# Enable PCH PCIE RP 6 using CLK 5
-	register "PcieRpEnable[5]" = "1"
+	register "PchPcieRpEnable[5]" = "1"
    register "PcieClkSrcClkReq[5]" = "5"
    register "PcieClkSrcUsage[5]" = "0x5"
    register "PcieRpClkReqDetect[5]" = "1"

# Enable PCH PCIE RP 8 using CLK 6
-	register "PcieRpEnable[7]" = "1"
+	register "PchPcieRpEnable[7]" = "1"
    register "PcieClkSrcClkReq[7]" = "6" # CLKSRC -> 7 and CLKREQ -> 6
    register "PcieClkSrcUsage[6]" = "PCIE_CLK_FREE" # CLK 6 is using free running CLK
    register "PcieRpClkReqDetect[6]" = "1"

# Enable PCH PCIE RP 9 using CLK 1
-	register "PcieRpEnable[8]" = "1"
+	register "PchPcieRpEnable[8]" = "1"
    register "PcieClkSrcClkReq[1]" = "1"
    register "PcieClkSrcUsage[1]" = "0x8"
    register "PcieRpClkReqDetect[8]" = "1"

# Enable PCH PCIE RP 11 for optane
-	register "PcieRpEnable[10]" = "1"
+	register "PchPcieRpEnable[10]" = "1"
    # Hybrid storage mode
    register "HybridStorageMode" = "1"

-	# Enable CPU PCIE RP 1 using PEG CLK 0
+	# Enable CPU PCIE RP 1 using CLK 0
+	register "CpuPcieRpEnable[0]" = "1"
    register "PcieClkSrcUsage[0]" = "0x40"

-	# Enable PCU PCIE PEG Slot 1 and 2
+	# Enable CPU PCIE RP 2 using CLK 3
+	register "CpuPcieRpEnable[1]" = "1"
    register "PcieClkSrcUsage[3]" = "0x41"
+
+	# Enable CPU PCIE RP 3 using CLK 4
+	register "CpuPcieRpEnable[2]" = "1"
    register "PcieClkSrcUsage[4]" = "0x42"

# Mark LAN CLK pins as unused as GbE 0:0x1f.6 is disabled below
@@ -177,10 +182,12 @@

device domain 0 on
    	device pci 00.0 on  end # Host Bridge
+		device pci 01.0 on  end # PEG10
    	device pci 02.0 on  end # Graphics
    	device pci 04.0 on  end # DPTF
    	device pci 05.0 on  end # IPU
    	device pci 06.0 on  end # PEG60
+		device pci 06.2 on  end # PEG62
    	device pci 07.0 on  end # TBT_PCIe0
    	device pci 07.1 on  end # TBT_PCIe1
    	device pci 07.2 on  end # TBT_PCIe2
diff --git a/src/soc/intel/alderlake/Kconfig b/src/soc/intel/alderlake/Kconfig
index c73df50..8009a42 100644
--- a/src/soc/intel/alderlake/Kconfig
+++ b/src/soc/intel/alderlake/Kconfig
@@ -119,10 +119,18 @@
    hex
    default 0x10000

-config MAX_ROOT_PORTS
+config MAX_PCH_ROOT_PORTS
    int
    default 12

+config MAX_CPU_ROOT_PORTS
+	int
+	default 3
+
+config MAX_ROOT_PORTS
+	int
+	default MAX_PCH_ROOT_PORTS
+
 config MAX_PCIE_CLOCKS
    int
    default 12
diff --git a/src/soc/intel/alderlake/chip.h b/src/soc/intel/alderlake/chip.h
index 38d9671..8e59c9a 100644
--- a/src/soc/intel/alderlake/chip.h
+++ b/src/soc/intel/alderlake/chip.h
@@ -118,9 +118,12 @@
    uint8_t PchHdaIDispLinkFrequency;
    uint8_t PchHdaIDispCodecDisconnect;

-	/* PCIe Root Ports */
-	uint8_t PcieRpEnable[CONFIG_MAX_ROOT_PORTS];
-	uint8_t PcieRpHotPlug[CONFIG_MAX_ROOT_PORTS];
+	/* CPU PCIe Root Ports */
+	uint8_t CpuPcieRpEnable[CONFIG_MAX_CPU_ROOT_PORTS];
+
+	/* PCH PCIe Root Ports */
+	uint8_t PchPcieRpEnable[CONFIG_MAX_PCH_ROOT_PORTS];
+	uint8_t PcieRpHotPlug[CONFIG_MAX_PCH_ROOT_PORTS];
    /* PCIe output clocks type to PCIe devices.
     * 0-23: PCH rootport, 0x70: LAN, 0x80: unspecified but in use,
     * 0xFF: not used */
@@ -130,7 +133,7 @@
    uint8_t PcieClkSrcClkReq[CONFIG_MAX_PCIE_CLOCKS];

/* Probe CLKREQ# signal before enabling CLKREQ# based power management.*/
-	uint8_t PcieRpClkReqDetect[CONFIG_MAX_ROOT_PORTS];
+	uint8_t PcieRpClkReqDetect[CONFIG_MAX_PCH_ROOT_PORTS];

/* PCIe RP L1 substate */
    enum L1_substates_control {
@@ -138,13 +141,13 @@
    	L1_SS_DISABLED,
    	L1_SS_L1_1,
    	L1_SS_L1_2,
-	} PcieRpL1Substates[CONFIG_MAX_ROOT_PORTS];
+	} PcieRpL1Substates[CONFIG_MAX_PCH_ROOT_PORTS];

/* PCIe LTR: Enable (1) / Disable (0) */
-	uint8_t PcieRpLtrEnable[CONFIG_MAX_ROOT_PORTS];
+	uint8_t PcieRpLtrEnable[CONFIG_MAX_PCH_ROOT_PORTS];

/* PCIE RP Advanced Error Report: Enable (1) / Disable (0) */
-	uint8_t PcieRpAdvancedErrorReporting[CONFIG_MAX_ROOT_PORTS];
+	uint8_t PcieRpAdvancedErrorReporting[CONFIG_MAX_PCH_ROOT_PORTS];

/* Gfx related */
    enum {
diff --git a/src/soc/intel/alderlake/fsp_params.c b/src/soc/intel/alderlake/fsp_params.c
index 6de8649..35f7a3c 100644
--- a/src/soc/intel/alderlake/fsp_params.c
+++ b/src/soc/intel/alderlake/fsp_params.c
@@ -270,7 +270,7 @@
    /* Enable Hybrid storage auto detection */
    params->HybridStorageMode = config->HybridStorageMode;

-	for (i = 0; i < CONFIG_MAX_ROOT_PORTS; i++) {
+	for (i = 0; i < CONFIG_MAX_PCH_ROOT_PORTS; i++) {
    	params->PcieRpL1Substates[i] =
    		get_l1_substate_control(config->PcieRpL1Substates[i]);
    	params->PcieRpLtrEnable[i] = config->PcieRpLtrEnable[i];
diff --git a/src/soc/intel/alderlake/romstage/fsp_params.c b/src/soc/intel/alderlake/romstage/fsp_params.c
index 7e842a2..a615f0b 100644
--- a/src/soc/intel/alderlake/romstage/fsp_params.c
+++ b/src/soc/intel/alderlake/romstage/fsp_params.c
@@ -41,8 +41,8 @@
    	/* Set CpuRatio to match existing MSR value */
    	m_cfg->CpuRatio = (rdmsr(MSR_FLEX_RATIO).lo >> 8) & 0xff;

-	for (i = 0; i < ARRAY_SIZE(config->PcieRpEnable); i++) {
-		if (config->PcieRpEnable[i])
+	for (i = 0; i < ARRAY_SIZE(config->PchPcieRpEnable); i++) {
+		if (config->PchPcieRpEnable[i])
    		mask |= (1 << i);
    }
    m_cfg->PcieRpEnableMask = mask;
@@ -155,9 +155,12 @@
    /* Skip CPU replacement check */
    m_cfg->SkipCpuReplacementCheck = !config->CpuReplacementCheck;

-	/* Skip CPU side PCIe enablement in FSP if device is disabled in devicetree */
-	dev = pcidev_path_on_root(SA_DEVFN_CPU_PCIE);
-	m_cfg->CpuPcieRpEnableMask = is_dev_enabled(dev);
+	mask = 0;
+	for (i = 0; i < ARRAY_SIZE(config->CpuPcieRpEnable); i++) {
+		if (config->CpuPcieRpEnable[i])
+			mask |= (1 << i);
+	}
+	m_cfg->CpuPcieRpEnableMask = mask;

m_cfg->TmeEnable = CONFIG(INTEL_TME);