Change in ...coreboot[master]: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINU...

[Jeremy Soller (Code Review)]

Jeremy Soller has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31536

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE

Change-Id: I9ad4f5a6c7391fc6e813ec1306c708f449a69f59
---
M src/soc/intel/cannonlake/Kconfig
M src/soc/intel/cannonlake/fsp_params.c
2 files changed, 13 insertions(+), 0 deletions(-)

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

diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig
index cd8819d..3716640 100644
--- a/src/soc/intel/cannonlake/Kconfig
+++ b/src/soc/intel/cannonlake/Kconfig
@@ -172,6 +172,13 @@
    default 24 if SOC_INTEL_CANNONLAKE_PCH_H
    default 16

+config SERIRQ_CONTINUOUS_MODE
+	bool
+	default n
+	help
+	  If you set this option to y, the serial IRQ machine will be
+	  operated in continuous mode.
+
 config SMM_TSEG_SIZE
    hex
    default 0x800000
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index c276c86..c95a68f 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -246,6 +246,12 @@

/* Set TccActivationOffset */
    tconfig->TccActivationOffset = config->tcc_offset;
+
+	/* Set correct Sirq mode based on config */
+	if (IS_ENABLED(CONFIG_SERIRQ_CONTINUOUS_MODE))
+		params->PchSirqMode = 1;
+	else
+		params->PchSirqMode = 0;
 }

/* Mainboard GPIO Configuration */

[Jeremy Soller (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

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

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

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

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE

Tested on system76 galp3-c

Signed-off-by: Jeremy Soller 
Change-Id: I9ad4f5a6c7391fc6e813ec1306c708f449a69f59
---
M src/soc/intel/cannonlake/Kconfig
M src/soc/intel/cannonlake/fsp_params.c
2 files changed, 13 insertions(+), 0 deletions(-)

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

[Patrick Rudolph (Code Review)]

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

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/#/c/31536/2/src/soc/intel/cannonlake/fsp_params.... 
PS2, Line 250: 	/* Set correct Sirq mode based on config */
That's already done in lpc_soc_init().
If FSP overwrites that, it's a bug and should be fixed in FSP.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31536 )

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

Thanks for the fix. I wouldn't mind merging it in this
state, though maybe see if we can settle on the Kconfig
vs `chip.h` thing, first.

NB. There may be more unset UPDs related to serial irqs.
Maybe we should set them all at once.

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

https://review.coreboot.org/#/c/31536/2/src/soc/intel/cannonlake/fsp_params.... 
PS2, Line 250: 	/* Set correct Sirq mode based on config */
> That's already done in lpc_soc_init(). […]

For once, I wouldn't say that. FSP is full of these
options (fixing that would imply redesigning FSP entire-
ly; I wouldn't mind, though). coreboot should never
rely on UPD defaults, every UPD must be set according
to coreboot's expectations, and that is what is done
here (at least for one more option).

That we have so much redundant code in coreboot, IMHO,
is a documentation problem. Nobody knows what FSP al-
ready does exactly, so people add more and more con-
flicting code... that is something that can be fixed
on the FSP side. Please, somebody document it!

The coreboot code around this is rather messy and needs
a clean-up, IMHO. For instance, Skylake provides both a
Kconfig and a `chip.h` setting for this :-/

It was a Kconfig historically, but I wouldn't mind a
`chip.h` setting in this case. Simply because we decide
it once per board and don't have any complex require-
ments, neither do we want it user configurable with a
Kconfig prompt.

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31536/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31536/2//COMMIT_MSG@9 
PS2, Line 9: Tested on system76 galp3-c
can you please explain the issue ?

[Jeremy Soller (Code Review)]

Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31536 )

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

Patch Set 2:
> Patch Set 2:
>
> (1 comment)

The issue is that the FSP-S will set the Sirq mode to quiet no matter what. This is not overwritten because without the change I made to KConfig, there is no way to set continuous mode in coreboot after FSP-S.

[Jeremy Soller (Code Review)]

Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31536 )

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review+1
>
> (1 comment)
>
> Thanks for the fix. I wouldn't mind merging it in this
> state, though maybe see if we can settle on the Kconfig
> vs `chip.h` thing, first.
>
> NB. There may be more unset UPDs related to serial irqs.
> Maybe we should set them all at once.

The issue is that the FSP-S will set the Sirq mode to quiet no matter what. This is not overwritten because without the change I made to KConfig, there is no way to set continuous mode in coreboot after FSP-S.

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

Patch Set 2:
> > Patch Set 2: Code-Review+1
> >
> > (1 comment)
> >
> > Thanks for the fix. I wouldn't mind merging it in this
> > state, though maybe see if we can settle on the Kconfig
> > vs `chip.h` thing, first.
> >
> > NB. There may be more unset UPDs related to serial irqs.
> > Maybe we should set them all at once.
>
> The issue is that the FSP-S will set the Sirq mode to quiet no
> matter what. This is not overwritten because without the change I
> made to KConfig, there is no way to set continuous mode in coreboot
> after FSP-S.

question is why u need to set IRQ into continuous mode ?

[Jeremy Soller (Code Review)]

Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31536 )

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

Patch Set 2:
> Patch Set 2:
> > > Patch Set 2: Code-Review+1
> > >
> > > (1 comment)
> > >
> > > Thanks for the fix. I wouldn't mind merging it in this
> > > state, though maybe see if we can settle on the Kconfig
> > > vs `chip.h` thing, first.
> > >
> > > NB. There may be more unset UPDs related to serial irqs.
> > > Maybe we should set them all at once.
> >
> > The issue is that the FSP-S will set the Sirq mode to quiet no
> > matter what. This is not overwritten because without the change I
> > made to KConfig, there is no way to set continuous mode in coreboot
> > after FSP-S.
>
> question is why u need to set IRQ into continuous mode ?

Ah. The System76 galp3-c EC does not function with the quiet setting. Interrupts are missed. It was done in some skylake and kabylake models, so I utilized it on our whiskeylake model with success.

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

Patch Set 2: Code-Review+1

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31536 )

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/#/c/31536/2/src/soc/intel/cannonlake/fsp_params.... 
PS2, Line 250: 	/* Set correct Sirq mode based on config */
> For once, I wouldn't say that. FSP is full of these […]

Subrata, what do you think about the Kconfig vs.
`chip.h` setting? If we agree to use Kconfig for
this, I can write a patch that removes the `chip.h`
entry in skylake/.

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/#/c/31536/2/src/soc/intel/cannonlake/fsp_params.... 
PS2, Line 250: 	/* Set correct Sirq mode based on config */
> Subrata, what do you think about the Kconfig vs. […]

@Nico: one standard should be fine, if its chip.h or Kconfig. chip.h is more readable in my view.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31536 )

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/#/c/31536/2/src/soc/intel/cannonlake/fsp_params.... 
PS2, Line 250: 	/* Set correct Sirq mode based on config */
> @Nico: one standard should be fine, if its chip.h or Kconfig. chip.h is more readable in my view.

I agree. Though, this is more work than I expected.
It started with Skylake where we had both (devicetree
and Kconfig): CB:31596. Apollo Lake already does it
like that.

[Jeremy Soller (Code Review)]

Hello Patrick Rudolph, Subrata Banik, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE

Tested on system76 galp3-c

Signed-off-by: Jeremy Soller 
Change-Id: I9ad4f5a6c7391fc6e813ec1306c708f449a69f59
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
M src/soc/intel/cannonlake/lpc.c
3 files changed, 14 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/31536/3

[Jeremy Soller (Code Review)]

Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31536 )

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

Patch Set 3:
> Patch Set 2: Code-Review+1
>
> (1 comment)
>
> Thanks for the fix. I wouldn't mind merging it in this
> state, though maybe see if we can settle on the Kconfig
> vs `chip.h` thing, first.
>
> NB. There may be more unset UPDs related to serial irqs.
> Maybe we should set them all at once.

I changed it to be done in chip.h

[Jeremy Soller (Code Review)]

Hello Patrick Rudolph, Subrata Banik, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE

Tested on system76 galp3-c

Signed-off-by: Jeremy Soller 
Change-Id: I9ad4f5a6c7391fc6e813ec1306c708f449a69f59
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
M src/soc/intel/cannonlake/lpc.c
3 files changed, 11 insertions(+), 1 deletion(-)

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

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31536 )

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

Thanks for the update. In the long run, I'd prefer to
use the common `enum serirq_mode`, though.

https://review.coreboot.org/#/c/31536/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31536/4//COMMIT_MSG@7 
PS4, Line 7: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
This line is too long and needs an update.

[Nathaniel L Desimone (Code Review)]

Nathaniel L Desimone has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31536 )

Change subject: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
......................................................................

Patch Set 4: Code-Review+1

Change in coreboot[master]: soc/intel/cannonlake: Set correct serirq mode

[Matt DeVillier (Code Review)]

Matt DeVillier has uploaded a new patch set (#5) to the change originally created by Jeremy Soller. ( https://review.coreboot.org/c/coreboot/+/31536 )

Change subject: soc/intel/cannonlake: Set correct serirq mode
......................................................................

soc/intel/cannonlake: Set correct serirq mode

Set FSP params PchSirqEnable/PchSirqMode based on board
setting of serirq_mode. Matches implementation on Skylake.

This is a no-change for existing boards since the default
remains SERIRQ_QUIET mode.

Tested on system76 galp3-c, out-of-tree WHL-U board

Change-Id: I9ad4f5a6c7391fc6e813ec1306c708f449a69f59
Signed-off-by: Jeremy Soller 
Signed-off-by: Matt DeVillier 
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
M src/soc/intel/cannonlake/lpc.c
3 files changed, 10 insertions(+), 4 deletions(-)

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

Change in coreboot[master]: soc/intel/cannonlake: Set correct serirq mode

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31536 )

Change subject: soc/intel/cannonlake: Set correct serirq mode
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/coreboot/+/31536/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/31536/4//COMMIT_MSG@7 
PS4, Line 7: soc/intel/cannonlake: Set correct serirq mode based on SERIRQ_CONTINUOUS_MODE
> This line is too long and needs an update.

Done

https://review.coreboot.org/c/coreboot/+/31536/2/src/soc/intel/cannonlake/fs... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/31536/2/src/soc/intel/cannonlake/fs... 
PS2, Line 250: 	/* Set correct Sirq mode based on config */
> I agree. Though, this is more work than I expected. […]

Done

Change in coreboot[master]: soc/intel/cannonlake: Set correct serirq mode

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31536 )

Change subject: soc/intel/cannonlake: Set correct serirq mode
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: soc/intel/cannonlake: Set correct serirq mode

[Nathaniel L Desimone (Code Review)]

Nathaniel L Desimone has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31536 )

Change subject: soc/intel/cannonlake: Set correct serirq mode
......................................................................

Patch Set 5: Code-Review+1

Change in coreboot[master]: soc/intel/cannonlake: Set correct serirq mode

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/31536 )

Change subject: soc/intel/cannonlake: Set correct serirq mode
......................................................................

soc/intel/cannonlake: Set correct serirq mode

Set FSP params PchSirqEnable/PchSirqMode based on board
setting of serirq_mode. Matches implementation on Skylake.

This is a no-change for existing boards since the default
remains SERIRQ_QUIET mode.

Tested on system76 galp3-c, out-of-tree WHL-U board

Change-Id: I9ad4f5a6c7391fc6e813ec1306c708f449a69f59
Signed-off-by: Jeremy Soller 
Signed-off-by: Matt DeVillier 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31536
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
Reviewed-by: Nathaniel L Desimone 
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
M src/soc/intel/cannonlake/lpc.c
3 files changed, 10 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  Nathaniel L Desimone: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h
index 330555c..b14c3c5 100644
--- a/src/soc/intel/cannonlake/chip.h
+++ b/src/soc/intel/cannonlake/chip.h
@@ -22,6 +22,7 @@
 #include <drivers/i2c/designware/dw_i2c.h>
 #include <intelblocks/gpio.h>
 #include <intelblocks/gspi.h>
+#include <intelblocks/lpc_lib.h>
 #include <smbios.h>
 #include <stdint.h>
 #include <soc/gpio.h>
@@ -360,6 +361,8 @@
     */
    uint8_t SerialIoDevMode[PchSerialIoIndexMAX];

+	enum serirq_mode serirq_mode;
+
    /* GPIO SD card detect pin */
    unsigned int sdcard_cd_gpio;

diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index 80918f1..9d6ed38 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -416,6 +416,10 @@
    /* Unlock all GPIO pads */
    tconfig->PchUnlockGpioPads = config->PchUnlockGpioPads;

+	/* Set correct Sirq mode based on config */
+	params->PchSirqEnable = config->serirq_mode != SERIRQ_OFF;
+	params->PchSirqMode = config->serirq_mode == SERIRQ_CONTINUOUS;
+
    /*
     * GSPI Chip Select parameters
     * The GSPI driver assumes that CS0 is the used chip-select line,
diff --git a/src/soc/intel/cannonlake/lpc.c b/src/soc/intel/cannonlake/lpc.c
index c4eb884..8b98022 100644
--- a/src/soc/intel/cannonlake/lpc.c
+++ b/src/soc/intel/cannonlake/lpc.c
@@ -210,6 +210,8 @@

void lpc_soc_init(struct device *dev)
 {
+	const config_t *config = dev->chip_info;
+
    /* Legacy initialization */
    isa_dma_init();
    pch_misc_init();
@@ -218,10 +220,7 @@
    lpc_enable_pci_clk_cntl();

/* Set LPC Serial IRQ mode */
-	if (CONFIG(SERIRQ_CONTINUOUS_MODE))
-		lpc_set_serirq_mode(SERIRQ_CONTINUOUS);
-	else
-		lpc_set_serirq_mode(SERIRQ_QUIET);
+	lpc_set_serirq_mode(config->serirq_mode);

/* Interrupt configuration */
    pch_enable_ioapic(dev);