Change in coreboot[master]: soc/intel/Kconfig: Avoid specify dedicated chipset name

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/Kconfig: Avoid specify dedicated chipset name
......................................................................

soc/intel/Kconfig: Avoid specify dedicated chipset name

This patch ensures all IA chipsets and common Kconfig files
are getting included without specifying dedicated chipset names.

TEST=Able to compile CML and TGL RVP.

Change-Id: Ic2d8a8ac1c4acfabd4ded1bfd4ff359e820e174b
Signed-off-by: Subrata Banik 
---
M src/soc/intel/Kconfig
1 file changed, 2 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39530/1

diff --git a/src/soc/intel/Kconfig b/src/soc/intel/Kconfig
index 47efc4d..0f8f0af 100644
--- a/src/soc/intel/Kconfig
+++ b/src/soc/intel/Kconfig
@@ -1,18 +1,5 @@
-# Load all chipsets
-source "src/soc/intel/apollolake/Kconfig"
-source "src/soc/intel/baytrail/Kconfig"
-source "src/soc/intel/braswell/Kconfig"
-source "src/soc/intel/broadwell/Kconfig"
-source "src/soc/intel/cannonlake/Kconfig"
-source "src/soc/intel/denverton_ns/Kconfig"
-source "src/soc/intel/quark/Kconfig"
-source "src/soc/intel/skylake/Kconfig"
-source "src/soc/intel/icelake/Kconfig"
-source "src/soc/intel/tigerlake/Kconfig"
-source "src/soc/intel/xeon_sp/Kconfig"
-
-# Load common config
-source "src/soc/intel/common/Kconfig"
+# Load all chipsets and common config
+source "src/soc/intel/*/Kconfig"

config INTEL_HAS_TOP_SWAP
    bool

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/Kconfig: Avoid specify dedicated chipset name
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39530/1/src/soc/intel/Kconfig 
File src/soc/intel/Kconfig:

https://review.coreboot.org/c/coreboot/+/39530/1/src/soc/intel/Kconfig@a15 
PS1, Line 15: source "src/soc/intel/common/Kconfig"
IIRC, it was done this way because we want SoC specific Kconfigs to be sourced before common Kconfig since the config system picks up the first value that it encounters for a Kconfig variable.

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39530 )

Change subject: soc/intel/Kconfig: Avoid specify dedicated chipset name
......................................................................

Patch Set 1:

(1 comment)

I had attempted this in past, had a review comment (below) on why order in required:

"
Order might matter here, that `common/Kconfig` is sourced last
is likely intentional.

Usually this is done so that the common code can provide a default
and SoCs can override it (first `default` line encountered by
Kconfig takes precedence)."

https://review.coreboot.org/c/coreboot/+/39530/1/src/soc/intel/Kconfig 
File src/soc/intel/Kconfig:

https://review.coreboot.org/c/coreboot/+/39530/1/src/soc/intel/Kconfig@a15 
PS1, Line 15: source "src/soc/intel/common/Kconfig"
> IIRC, it was done this way because we want SoC specific Kconfigs to be sourced before common Kconfig […]

Yes, had a similar comment here: https://review.coreboot.org/c/coreboot/+/37554
I had checked, the .config file had config changes with this implementation

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/Kconfig: Avoid specify dedicated chipset name
......................................................................

Patch Set 1:
> Patch Set 1:
>
> (1 comment)
>
> I had attempted this in past, had a review comment (below) on why order in required:
>
> "
> Order might matter here, that `common/Kconfig` is sourced last
> is likely intentional.
>
> Usually this is done so that the common code can provide a default
> and SoCs can override it (first `default` line encountered by
> Kconfig takes precedence)."

Correct. I think we can potentially do this:

Rename: src/soc/intel/common/Kconfig to src/soc/intel/common/Kconfig.common
Change: All specific SoC sources to "source src/soc/intel/*/Kconfig"
Change: Common source to "source src/soc/intel/common/Kconfig.common"

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/Kconfig: Avoid specify dedicated chipset name
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > (1 comment)
> >
> > I had attempted this in past, had a review comment (below) on why order in required:
> >
> > "
> > Order might matter here, that `common/Kconfig` is sourced last
> > is likely intentional.
> >
> > Usually this is done so that the common code can provide a default
> > and SoCs can override it (first `default` line encountered by
> > Kconfig takes precedence)."
>
> Correct. I think we can potentially do this:
>
> Rename: src/soc/intel/common/Kconfig to src/soc/intel/common/Kconfig.common
> Change: All specific SoC sources to "source src/soc/intel/*/Kconfig"
> Change: Common source to "source src/soc/intel/common/Kconfig.common"

Sure, let me do that as mentioned here

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/Kconfig: Avoid specify dedicated chipset name
......................................................................

Patch Set 1:
> Patch Set 1:
>
> (1 comment)
>
> I had attempted this in past, had a review comment (below) on why order in required:
>
> "
> Order might matter here, that `common/Kconfig` is sourced last
> is likely intentional.
>
> Usually this is done so that the common code can provide a default
> and SoCs can override it (first `default` line encountered by
> Kconfig takes precedence)."

Thanks Aamir for pointers

[Subrata Banik (Code Review)]

Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Wonkyu Kim, Angel Pons, Aamir Bohra, Patrick Rudolph,

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

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

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

Change subject: soc/intel/Kconfig: Avoid specify dedicated chipset name
......................................................................

soc/intel/Kconfig: Avoid specify dedicated chipset name

This patch ensures all IA chipsets and common Kconfig files
are getting included without specifying dedicated chipset names.

Details below:
Rename: src/soc/intel/common/Kconfig to src/soc/intel/common/Kconfig.common
Change: All specific SoC sources to "source src/soc/intel/*/Kconfig"
Change: Common source to "source src/soc/intel/common/Kconfig.common"

TEST=Able to compile CML and TGL RVP.

Change-Id: Ic2d8a8ac1c4acfabd4ded1bfd4ff359e820e174b
Signed-off-by: Subrata Banik 
---
M src/soc/intel/Kconfig
R src/soc/intel/common/Kconfig.common
2 files changed, 2 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39530/2

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/Kconfig: Avoid specify dedicated chipset name
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39530/1/src/soc/intel/Kconfig 
File src/soc/intel/Kconfig:

https://review.coreboot.org/c/coreboot/+/39530/1/src/soc/intel/Kconfig@a15 
PS1, Line 15: source "src/soc/intel/common/Kconfig"
> Yes, had a similar comment here: https://review.coreboot.org/c/coreboot/+/37554 […]

Ack

[Angel Pons (Code Review)]

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

Change subject: soc/intel/Kconfig: Avoid specify dedicated chipset name
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/39530/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39530/2//COMMIT_MSG@7 
PS2, Line 7: specify
Nit: Use the gerund tense: specifying

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/Kconfig: Avoid specify dedicated chipset name
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39530/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39530/2//COMMIT_MSG@7 
PS2, Line 7: specify
> Nit: Use the gerund tense: specifying

Ack

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39530 )

Change subject: soc/intel/Kconfig: Avoid specify dedicated chipset name
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: soc/intel/Kconfig: Avoid specifying dedicated chipset name

[Subrata Banik (Code Review)]

Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Wonkyu Kim, Angel Pons, Aamir Bohra, Patrick Rudolph,

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

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

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

Change subject: soc/intel/Kconfig: Avoid specifying dedicated chipset name
......................................................................

soc/intel/Kconfig: Avoid specifying dedicated chipset name

This patch ensures all IA chipsets and common Kconfig files
are getting included without specifying dedicated chipset names.

Details below:
Rename: src/soc/intel/common/Kconfig to src/soc/intel/common/Kconfig.common
Change: All specific SoC sources to "source src/soc/intel/*/Kconfig"
Change: Common source to "source src/soc/intel/common/Kconfig.common"

TEST=Able to compile CML and TGL RVP.

Change-Id: Ic2d8a8ac1c4acfabd4ded1bfd4ff359e820e174b
Signed-off-by: Subrata Banik 
---
M src/soc/intel/Kconfig
R src/soc/intel/common/Kconfig.common
2 files changed, 2 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39530/3

Change in coreboot[master]: soc/intel/Kconfig: Avoid specifying dedicated chipset name

[Angel Pons (Code Review)]

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

Change subject: soc/intel/Kconfig: Avoid specifying dedicated chipset name
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: soc/intel/Kconfig: Avoid specifying dedicated chipset name

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39530 )

Change subject: soc/intel/Kconfig: Avoid specifying dedicated chipset name
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/39530/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39530/3//COMMIT_MSG@13 
PS3, Line 13: Rename: src/soc/intel/common/Kconfig to src/soc/intel/common/Kconfig.common
I do not see why the renaming is needed. Please elaborate why the renaming is needed.

https://review.coreboot.org/c/coreboot/+/39530/3//COMMIT_MSG@13 
PS3, Line 13: Rename: src/soc/intel/common/Kconfig to src/soc/intel/common/Kconfig.common
            : Change: All specific SoC sources to "source src/soc/intel/*/Kconfig"
            : Change: Common source to "source src/soc/intel/common/Kconfig.common"
These prefixes (Rename, Change) look quite strange. Is that used internally by Intel?

Change in coreboot[master]: soc/intel/Kconfig: Avoid specifying dedicated chipset name

[Subrata Banik (Code Review)]

Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Wonkyu Kim, Angel Pons, Aamir Bohra, Patrick Rudolph,

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

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

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

Change subject: soc/intel/Kconfig: Avoid specifying dedicated chipset name
......................................................................

soc/intel/Kconfig: Avoid specifying dedicated chipset name

This patch ensures all IA chipsets and common Kconfig files
are getting included without specifying dedicated chipset names.

TEST=Able to compile CML and TGL RVP.

Change-Id: Ic2d8a8ac1c4acfabd4ded1bfd4ff359e820e174b
Signed-off-by: Subrata Banik 
---
M src/soc/intel/Kconfig
R src/soc/intel/common/Kconfig.common
2 files changed, 2 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/39530/4

Change in coreboot[master]: soc/intel/Kconfig: Avoid specifying dedicated chipset name

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/Kconfig: Avoid specifying dedicated chipset name
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/39530/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39530/3//COMMIT_MSG@13 
PS3, Line 13: Rename: src/soc/intel/common/Kconfig to src/soc/intel/common/Kconfig.common
> I do not see why the renaming is needed. Please elaborate why the renaming is needed.

Ack

https://review.coreboot.org/c/coreboot/+/39530/3//COMMIT_MSG@13 
PS3, Line 13: Rename: src/soc/intel/common/Kconfig to src/soc/intel/common/Kconfig.common
            : Change: All specific SoC sources to "source src/soc/intel/*/Kconfig"
            : Change: Common source to "source src/soc/intel/common/Kconfig.common"
> These prefixes (Rename, Change) look quite strange. […]

Ack

Change in coreboot[master]: soc/intel/Kconfig: Avoid specifying dedicated chipset name

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39530 )

Change subject: soc/intel/Kconfig: Avoid specifying dedicated chipset name
......................................................................

Patch Set 4: Code-Review+1

Change in coreboot[master]: soc/intel/Kconfig: Avoid specifying dedicated chipset name

[Angel Pons (Code Review)]

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

Change subject: soc/intel/Kconfig: Avoid specifying dedicated chipset name
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39530/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39530/3//COMMIT_MSG@13 
PS3, Line 13: Rename: src/soc/intel/common/Kconfig to src/soc/intel/common/Kconfig.common
> Ack

The rename to Kconfig.common is to avoid being included by the wildcard, as common code must be the last one for Kconfig fallbacks to work properly.

Change in coreboot[master]: soc/intel/Kconfig: Avoid specifying dedicated chipset name

[Nico Huber (Code Review)]

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

Change subject: soc/intel/Kconfig: Avoid specifying dedicated chipset name
......................................................................

Patch Set 4: Code-Review+1

Change in coreboot[master]: soc/intel/Kconfig: Avoid specifying dedicated chipset name

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/Kconfig: Avoid specifying dedicated chipset name
......................................................................

soc/intel/Kconfig: Avoid specifying dedicated chipset name

This patch ensures all IA chipsets and common Kconfig files
are getting included without specifying dedicated chipset names.

TEST=Able to compile CML and TGL RVP.

Change-Id: Ic2d8a8ac1c4acfabd4ded1bfd4ff359e820e174b
Signed-off-by: Subrata Banik 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39530
Reviewed-by: HAOUAS Elyes 
Reviewed-by: Nico Huber 
Reviewed-by: Angel Pons 
Reviewed-by: Aamir Bohra 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/Kconfig
R src/soc/intel/common/Kconfig.common
2 files changed, 2 insertions(+), 12 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, but someone else must approve
  HAOUAS Elyes: Looks good to me, but someone else must approve
  Aamir Bohra: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/Kconfig b/src/soc/intel/Kconfig
index 47efc4d..d519068 100644
--- a/src/soc/intel/Kconfig
+++ b/src/soc/intel/Kconfig
@@ -1,18 +1,8 @@
 # Load all chipsets
-source "src/soc/intel/apollolake/Kconfig"
-source "src/soc/intel/baytrail/Kconfig"
-source "src/soc/intel/braswell/Kconfig"
-source "src/soc/intel/broadwell/Kconfig"
-source "src/soc/intel/cannonlake/Kconfig"
-source "src/soc/intel/denverton_ns/Kconfig"
-source "src/soc/intel/quark/Kconfig"
-source "src/soc/intel/skylake/Kconfig"
-source "src/soc/intel/icelake/Kconfig"
-source "src/soc/intel/tigerlake/Kconfig"
-source "src/soc/intel/xeon_sp/Kconfig"
+source "src/soc/intel/*/Kconfig"

# Load common config
-source "src/soc/intel/common/Kconfig"
+source "src/soc/intel/common/Kconfig.common"

config INTEL_HAS_TOP_SWAP
    bool
diff --git a/src/soc/intel/common/Kconfig b/src/soc/intel/common/Kconfig.common
similarity index 100%
rename from src/soc/intel/common/Kconfig
rename to src/soc/intel/common/Kconfig.common

Change in coreboot[master]: soc/intel/Kconfig: Avoid specifying dedicated chipset name

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39530 )

Change subject: soc/intel/Kconfig: Avoid specifying dedicated chipset name
......................................................................

Patch Set 5:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1356
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1355
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1354

Please note: This test is under development and might not be accurate at all!

Change in coreboot[master]: soc/intel/Kconfig: Avoid specifying dedicated chipset name

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39530 )

Change subject: soc/intel/Kconfig: Avoid specifying dedicated chipset name
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39530/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39530/3//COMMIT_MSG@13 
PS3, Line 13: Rename: src/soc/intel/common/Kconfig to src/soc/intel/common/Kconfig.common
> The rename to Kconfig. […]

Subrata, what does *Ack* mean.

Angel, thanks, but the reasoning belongs into the commit message.

Change in coreboot[master]: soc/intel/Kconfig: Avoid specifying dedicated chipset name

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/Kconfig: Avoid specifying dedicated chipset name
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39530/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39530/3//COMMIT_MSG@13 
PS3, Line 13: Rename: src/soc/intel/common/Kconfig to src/soc/intel/common/Kconfig.common
            : Change: All specific SoC sources to "source src/soc/intel/*/Kconfig"
            : Change: Common source to "source src/soc/intel/common/Kconfig.common"
> Ack

i thought you have concern in duplicating commit msg. hence i have removed those from commit msg and ACK it

renaming is needed to allow override as Angel has mentioned.

Change in coreboot[master]: soc/intel/Kconfig: Avoid specifying dedicated chipset name

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39530 )

Change subject: soc/intel/Kconfig: Avoid specifying dedicated chipset name
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39530/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39530/3//COMMIT_MSG@13 
PS3, Line 13: Rename: src/soc/intel/common/Kconfig to src/soc/intel/common/Kconfig.common
            : Change: All specific SoC sources to "source src/soc/intel/*/Kconfig"
            : Change: Common source to "source src/soc/intel/common/Kconfig.common"
> i thought you have concern in duplicating commit msg. […]

Ok, looks like a misunderstanding. Please tell, how I can phrase it differently in the future, so it’s not missed.

It was about the strange way to formulating these sentences.

Additionally, if some discussion comes up during review, which is clarified in the review comments or even new patchsets, it makes sure that more reasoning/detail is needed in the commit message.