Change in ...coreboot[master]: soc/intel/cannonlake: Pass coreboot debug interface info to FSP

[Maulik V Vaghela (Code Review)]

Maulik V Vaghela has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31884

Change subject: soc/intel/cannonlake: Pass coreboot debug interface info to FSP
......................................................................

soc/intel/cannonlake: Pass coreboot debug interface info to FSP

coreboot have an option to use legacy UART or LPSS UART. FSP will use the
UART initialized by coreboot and we can choose an option to skip Uart
initialization by FSP.
For this, we need to pass correct debug interface flag to FSP through
which FSP will know which UART port to use. If we don't pass correct
interface information, FSP may try to dump logs on that port and it may
slow down the system.

BUG=none
BRANCH=none
TEST=Compile and boot with coreboot. Check FSP and coreboot logs are
coming on serial port.

Change-Id: I1ebb20c93e2c15ec085538509099de72bc9dd62c
Signed-off-by: Maulik V Vaghela 
---
M src/soc/intel/cannonlake/Kconfig
M src/soc/intel/cannonlake/romstage/fsp_params.c
2 files changed, 13 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31884/1

diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig
index 9796546..9ffb47b 100644
--- a/src/soc/intel/cannonlake/Kconfig
+++ b/src/soc/intel/cannonlake/Kconfig
@@ -120,6 +120,17 @@
      The amount of anticipated stack usage in CAR by bootblock and
      other stages.

+config COREBOOT_DEBUG_INTERFACE
+	hex
+	default 0x02 if DRIVERS_UART_8250IO
+	default 0x10
+	help
+	  This config will allow coreboot to pass information to the FSP
+	  regarding which debug interface is being used. This value will be
+	  filled into PcdDebugInterfaceFlags.
+	  Debug Interfaces. BIT0-RAM, BIT1-Legacy Uart BIT3-USB3, BIT4-LPSS Uart,
+	  BIT5-TraceHub, BIT2 - Not used.
+
 config IFD_CHIPSET
    string
    default "cnl"
diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c
index 5597c4f..a59a438 100644
--- a/src/soc/intel/cannonlake/romstage/fsp_params.c
+++ b/src/soc/intel/cannonlake/romstage/fsp_params.c
@@ -48,6 +48,8 @@
    m_cfg->PrmrrSize = config->PrmrrSize;
    m_cfg->EnableC6Dram = config->enable_c6dram;
    m_cfg->PcdSerialIoUartNumber = CONFIG_UART_FOR_CONSOLE;
+	m_cfg->PcdDebugInterfaceFlags = CONFIG_COREBOOT_DEBUG_INTERFACE;
+
    /* Disable Vmx if Vt-d is already disabled */
    if (config->VtdDisable)
    	m_cfg->VmxEnable = 0;

[Ronak Kanabar (Code Review)]

Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31884 )

Change subject: soc/intel/cannonlake: Pass coreboot debug interface info to FSP
......................................................................

Patch Set 1: Code-Review+1

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/cannonlake: Pass coreboot debug interface info to FSP
......................................................................

Patch Set 1: Code-Review+2

[V Sowmya (Code Review)]

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

Change subject: soc/intel/cannonlake: Pass coreboot debug interface info to FSP
......................................................................

Patch Set 1: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31884 )

Change subject: soc/intel/cannonlake: Pass coreboot debug interface info to FSP
......................................................................

Patch Set 1:

given that the encoding is FSP specific, would it make sense to keep it out of Kconfig and state it like this instead?

m_cfg->PcdDebugInterfaceFlags = CONFIG(DRIVERS_UART_8250IO)?0x02:0x10;

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Pass coreboot debug interface info to FSP
......................................................................

Patch Set 1:
> Patch Set 1:
>
> given that the encoding is FSP specific, would it make sense to keep it out of Kconfig and state it like this instead?
>
> m_cfg->PcdDebugInterfaceFlags = CONFIG(DRIVERS_UART_8250IO)?0x02:0x10;

+1 to what Patrick said.

[Maulik V Vaghela (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Ronak Kanabar, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Pass coreboot debug interface info to FSP
......................................................................

soc/intel/cannonlake: Pass coreboot debug interface info to FSP

coreboot have an option to use legacy UART or LPSS UART. FSP will use the
UART initialized by coreboot and we can choose an option to skip Uart
initialization by FSP.
For this, we need to pass correct debug interface flag to FSP through
which FSP will know which UART port to use. If we don't pass correct
interface information, FSP may try to dump logs on that port and it may
slow down the system.

BUG=none
BRANCH=none
TEST=Compile and boot with coreboot. Check FSP and coreboot logs are
coming on serial port.

Change-Id: I1ebb20c93e2c15ec085538509099de72bc9dd62c
Signed-off-by: Maulik V Vaghela 
---
M src/soc/intel/cannonlake/romstage/fsp_params.c
1 file changed, 10 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31884/2

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31884 )

Change subject: soc/intel/cannonlake: Pass coreboot debug interface info to FSP
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/31884/2/src/soc/intel/cannonlake/romstage/fs... 
File src/soc/intel/cannonlake/romstage/fsp_params.c:

https://review.coreboot.org/#/c/31884/2/src/soc/intel/cannonlake/romstage/fs... 
PS2, Line 59: 	m_cfg->PcdDebugInterfaceFlags = CONFIG(DRIVERS_UART_8250IO)? 0x02: 0x10;
spaces required around that '?' (ctx:VxW)

https://review.coreboot.org/#/c/31884/2/src/soc/intel/cannonlake/romstage/fs... 
PS2, Line 59: 	m_cfg->PcdDebugInterfaceFlags = CONFIG(DRIVERS_UART_8250IO)? 0x02: 0x10;
spaces required around that ':' (ctx:VxW)

[Maulik V Vaghela (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Ronak Kanabar, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Pass coreboot debug interface info to FSP
......................................................................

soc/intel/cannonlake: Pass coreboot debug interface info to FSP

coreboot have an option to use legacy UART or LPSS UART. FSP will use the
UART initialized by coreboot and we can choose an option to skip Uart
initialization by FSP.
For this, we need to pass correct debug interface flag to FSP through
which FSP will know which UART port to use. If we don't pass correct
interface information, FSP may try to dump logs on that port and it may
slow down the system.

BUG=none
BRANCH=none
TEST=Compile and boot with coreboot. Check FSP and coreboot logs are
coming on serial port.

Change-Id: I1ebb20c93e2c15ec085538509099de72bc9dd62c
Signed-off-by: Maulik V Vaghela 
---
M src/soc/intel/cannonlake/romstage/fsp_params.c
1 file changed, 11 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31884/3

[Maulik V Vaghela (Code Review)]

Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31884 )

Change subject: soc/intel/cannonlake: Pass coreboot debug interface info to FSP
......................................................................

Patch Set 3:

Thank you for review Patrick.I have updated CL

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31884 )

Change subject: soc/intel/cannonlake: Pass coreboot debug interface info to FSP
......................................................................

Patch Set 3:

Can you make use of BIT0 and store it in cbmem?

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Pass coreboot debug interface info to FSP
......................................................................

Patch Set 3: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31884 )

Change subject: soc/intel/cannonlake: Pass coreboot debug interface info to FSP
......................................................................

soc/intel/cannonlake: Pass coreboot debug interface info to FSP

coreboot have an option to use legacy UART or LPSS UART. FSP will use the
UART initialized by coreboot and we can choose an option to skip Uart
initialization by FSP.
For this, we need to pass correct debug interface flag to FSP through
which FSP will know which UART port to use. If we don't pass correct
interface information, FSP may try to dump logs on that port and it may
slow down the system.

BUG=none
BRANCH=none
TEST=Compile and boot with coreboot. Check FSP and coreboot logs are
coming on serial port.

Change-Id: I1ebb20c93e2c15ec085538509099de72bc9dd62c
Signed-off-by: Maulik V Vaghela 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31884
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
---
M src/soc/intel/cannonlake/romstage/fsp_params.c
1 file changed, 11 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved

diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c
index 5597c4f..791a6c8 100644
--- a/src/soc/intel/cannonlake/romstage/fsp_params.c
+++ b/src/soc/intel/cannonlake/romstage/fsp_params.c
@@ -48,6 +48,17 @@
    m_cfg->PrmrrSize = config->PrmrrSize;
    m_cfg->EnableC6Dram = config->enable_c6dram;
    m_cfg->PcdSerialIoUartNumber = CONFIG_UART_FOR_CONSOLE;
+	/*
+	 * PcdDebugInterfaceFlags
+	 * This config will allow coreboot to pass information to the FSP
+	 * regarding which debug interface is being used.
+	 * Debug Interfaces:
+	 * BIT0-RAM, BIT1-Legacy Uart BIT3-USB3, BIT4-LPSS Uart, BIT5-TraceHub
+	 * BIT2 - Not used.
+	 */
+	m_cfg->PcdDebugInterfaceFlags =
+				CONFIG(DRIVERS_UART_8250IO) ? 0x02 : 0x10;
+
    /* Disable Vmx if Vt-d is already disabled */
    if (config->VtdDisable)
    	m_cfg->VmxEnable = 0;