Change in coreboot[master]: [TESTONLY] devicetree: Remove weak declarations for ops

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: [TESTONLY] devicetree: Remove weak declarations for ops
......................................................................

[TESTONLY] devicetree: Remove weak declarations for ops

Change-Id: Ifb783e2f733d5c65c615e5c1879e3e4c7a83e049
Signed-off-by: Kyösti Mälkki 
---
M util/sconfig/main.c
1 file changed, 8 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/35086/1

diff --git a/util/sconfig/main.c b/util/sconfig/main.c
index 5e3a1d4..586de06 100644
--- a/util/sconfig/main.c
+++ b/util/sconfig/main.c
@@ -936,9 +936,14 @@

chip = tmp;
    while (chip) {
-		fprintf(fil,
-			"__attribute__((weak)) struct chip_operations %s_ops = {};\n",
-			chip->name_underscore);
+		if (strstr(chip->name_underscore, "cpu_") == chip->name_underscore) {
+			fprintf(fil,
+				"__attribute__((weak)) struct chip_operations %s_ops = {};\n",
+				chip->name_underscore);
+		} else {
+			fprintf(fil, "extern struct chip_operations %s_ops;\n",
+				chip->name_underscore);
+		}
    	chip = chip->next;
    }
    fprintf(fil, "#endif\n");

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: [TESTONLY] devicetree: Remove weak declarations for ops
......................................................................

Patch Set 1:

Arthur, you may want to look at the ibexpeak board errors here.

[Kyösti Mälkki (Code Review)]

Hello Arthur Heymans, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: [TESTONLY] devicetree: Remove weak declarations for ops
......................................................................

[TESTONLY] devicetree: Remove weak declarations for ops

Make it compulsory to build with all the drivers that are
visible in the board devicetree.cb file.

Change-Id: Ifb783e2f733d5c65c615e5c1879e3e4c7a83e049
Signed-off-by: Kyösti Mälkki 
---
M util/sconfig/main.c
1 file changed, 8 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/35086/3

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: [TESTONLY] devicetree: Remove weak declarations for ops
......................................................................

Patch Set 3:

I am not sure what to do about drivers/generic/generic. Nodody currently builds the ACPI namespace driver. Maybe declare empty generic_ops and rename the one that has an actual ACPI namespace implemenation on it?

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: [TESTONLY] devicetree: Remove weak declarations for ops
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35086/4/util/sconfig/main.c 
File util/sconfig/main.c:

https://review.coreboot.org/c/coreboot/+/35086/4/util/sconfig/main.c@951 
PS4, Line 951: 		if (strstr(chip->name_underscore, "cpu_") == chip->name_underscore) {
Could you please add a comment here about why cpu_ is special?

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Kyösti Mälkki (Code Review)]

Hello Aaron Durbin, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,

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

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

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

Change subject: devicetree: Remove weak declarations for ops
......................................................................

devicetree: Remove weak declarations for ops

Make it compulsory to build with all the drivers that are
visible in the board devicetree.cb file.

Change-Id: Ifb783e2f733d5c65c615e5c1879e3e4c7a83e049
Signed-off-by: Kyösti Mälkki 
---
M util/sconfig/main.c
1 file changed, 10 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/35086/5

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 5:

Need some advice with 'chip drivers/generic/generic'. CB:28796 added actual chip_operations but the path was already taken for (dummy) SPD and MAC eeprom devices.

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 5:
> Patch Set 5:
>
> Need some advice with 'chip drivers/generic/generic'. CB:28796 added actual chip_operations but the path was already taken for (dummy) SPD and MAC eeprom devices.

My guess is we'll have to carve out a new path to make everything work.  I *think* just a handful of devicetree.cb entries are using the ops (from a quick glance). We can just move some stuff around and should be good.

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 6:

Subrata, please suggest solution for drivers/i2c/max98373_ops and max98357. There are user visible options INCLUDE_SND_MAX98357_DA7219_NHLT and INCLUDE_SND_MAX98373_NHLT but the provided NHLT_BLOB_PATH is invalid.

I did not check individual boards, other errors seem to be on pc80/tpm and drivers/usb/acpi here.

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Subrata Banik (Code Review)]

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

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 6:
> Patch Set 6:
>
> Subrata, please suggest solution for drivers/i2c/max98373_ops and max98357. There are user visible options INCLUDE_SND_MAX98357_DA7219_NHLT and INCLUDE_SND_MAX98373_NHLT but the provided NHLT_BLOB_PATH is invalid.

Are you planning to use Non-HDA audio solution if yes then those blobs are required. i can check where does this blob exist for open source consumption. The same doesn't required for HDA solition
> I did not check individual boards, other errors seem to be on pc80/tpm and drivers/usb/acpi here.

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 6:
> Patch Set 6:
> > Patch Set 6:
> >
> > Subrata, please suggest solution for drivers/i2c/max98373_ops and max98357. There are user visible options INCLUDE_SND_MAX98357_DA7219_NHLT and INCLUDE_SND_MAX98373_NHLT but the provided NHLT_BLOB_PATH is invalid.
>
> Are you planning to use Non-HDA audio solution if yes then those blobs are required. i can check where does this blob exist for open source consumption. The same doesn't required for HDA solition

In the case we want to merge this, we need some solution to the build errors you see here with jenkins.

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 7:

Patrick (G.) can you push the question about invalid 3rdparty/ blobs to Intel agenda. It is somewhat blocking this from progressing.

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Abandoned

Insufficient vendor feedback here and CB:35103 to get this done.

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has restored this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Restored

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Angel Pons (Code Review)]

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

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 8: Code-Review+1

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Nico Huber (Code Review)]

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

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 8: Code-Review+2

+2 +2 +2 +2 the errors show how much this is needed!

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Paul Menzel (Code Review)]

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

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 8:

I am adding some folks to look at the build errors, so maybe they can fix their devices.

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 8: Code-Review+1

I agree this would catch issues, it can be frustrating to debug if you don't know what to look for.

I sent a patch for volteer boards at https://review.coreboot.org/c/coreboot/+/41700

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35086/4/util/sconfig/main.c 
File util/sconfig/main.c:

https://review.coreboot.org/c/coreboot/+/35086/4/util/sconfig/main.c@951 
PS4, Line 951: 		if (strstr(chip->name_underscore, "cpu_") == chip->name_underscore) {
> Could you please add a comment here about why cpu_ is special?

Done

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Furquan Shaikh (Code Review)]

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

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 14: Code-Review+2

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 14:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35086/14//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35086/14//COMMIT_MSG@7 
PS14, Line 7: devicetree: Remove weak declarations for ops
Just an unresolved comment to prevent premature submit.

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 14:
> Patch Set 6:
> > Patch Set 6:
> >
> > Subrata, please suggest solution for drivers/i2c/max98373_ops and max98357. There are user visible options INCLUDE_SND_MAX98357_DA7219_NHLT and INCLUDE_SND_MAX98373_NHLT but the provided NHLT_BLOB_PATH is invalid.
>
> Are you planning to use Non-HDA audio solution if yes then those blobs are required. i can check where does this blob exist for open source consumption. The same doesn't required for HDA solition

Subrata, any updates about the invalid blob paths?

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Kyösti Mälkki (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Patrick Rudolph, Jonathan Zhang, Matt DeVillier, Duncan Laurie, Angel Pons, Subrata Banik, Arthur Heymans, Andrey Petrov, Aaron Durbin, Piotr Król, Philipp Deppenwiese, Nico Huber, Michał Żygowski, Martin Roth, Maxim Polyakov, Jonathan Kollasch,

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

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

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

Change subject: devicetree: Remove weak declarations for ops
......................................................................

devicetree: Remove weak declarations for ops

Make it compulsory to build with all the drivers that are
visible in the board devicetree.cb file.

Change-Id: Ifb783e2f733d5c65c615e5c1879e3e4c7a83e049
Signed-off-by: Kyösti Mälkki 
---
M util/sconfig/main.c
1 file changed, 10 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/35086/20

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 21: Code-Review+2

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: devicetree: Remove weak declarations for ops
......................................................................

Patch Set 22:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35086/14//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35086/14//COMMIT_MSG@7 
PS14, Line 7: devicetree: Remove weak declarations for ops
> Just an unresolved comment to prevent premature submit.

Ack

Change in coreboot[master]: devicetree: Remove weak declarations for ops

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35086 )

Change subject: devicetree: Remove weak declarations for ops
......................................................................

devicetree: Remove weak declarations for ops

Make it compulsory to build with all the drivers that are
visible in the board devicetree.cb file.

Change-Id: Ifb783e2f733d5c65c615e5c1879e3e4c7a83e049
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35086
Tested-by: build bot (Jenkins) 
Reviewed-by: Felix Held 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Angel Pons 
Reviewed-by: Nico Huber 
Reviewed-by: Duncan Laurie 
---
M util/sconfig/main.c
1 file changed, 10 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Duncan Laurie: Looks good to me, but someone else must approve
  Nico Huber: Looks good to me, approved
  Felix Held: Looks good to me, approved
  Furquan Shaikh: Looks good to me, approved
  Angel Pons: Looks good to me, but someone else must approve

diff --git a/util/sconfig/main.c b/util/sconfig/main.c
index 6752b61c..b0c32f6 100644
--- a/util/sconfig/main.c
+++ b/util/sconfig/main.c
@@ -1170,9 +1170,16 @@

chip = tmp;
    while (chip) {
-		fprintf(fil,
-			"__attribute__((weak)) struct chip_operations %s_ops = {};\n",
-			chip->name_underscore);
+		/* A lot of cpus do not define chip_operations at all, and the ones
+		   that do only initialise .name. */
+		if (strstr(chip->name_underscore, "cpu_") == chip->name_underscore) {
+			fprintf(fil,
+				"__attribute__((weak)) struct chip_operations %s_ops = {};\n",
+				chip->name_underscore);
+		} else {
+			fprintf(fil, "extern struct chip_operations %s_ops;\n",
+				chip->name_underscore);
+		}
    	chip = chip->next;
    }
    fprintf(fil, "#endif\n");