Change in coreboot[master]: mainboard/intel/common: Add config for intel common mainboard support
Aamir Bohra has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37705 )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
mainboard/intel/common: Add config for intel common mainboard support
Add config to support intel mainboard common code. The intend is to create common placeholder for APIs common across the mainboard.
Change-Id: Idc6f9bd298520f5e929534c64f1b038a4c9684a9 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- A src/mainboard/intel/common/Kconfig 1 file changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/37705/1
diff --git a/src/mainboard/intel/common/Kconfig b/src/mainboard/intel/common/Kconfig new file mode 100644 index 0000000..e96d943 --- /dev/null +++ b/src/mainboard/intel/common/Kconfig @@ -0,0 +1,5 @@ +config MAINBOARD_INTEL_COMMON + bool + help + common code for Intel Mainboards +
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37705
to look at the new patch set (#2).
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
mainboard/intel/common: Add config for intel common mainboard support
Add config to support intel mainboard common code. The intend is to create common placeholder for APIs common across the mainboard.
Also adding a Kconfig.name and a board_info.txt as checkpatch looks for it.
Change-Id: Idc6f9bd298520f5e929534c64f1b038a4c9684a9 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- A src/mainboard/intel/common/Kconfig A src/mainboard/intel/common/Kconfig.name A src/mainboard/intel/common/board_info.txt 3 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/37705/2
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37705 )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
Patch Set 4: Code-Review+2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37705 )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37705/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37705/4//COMMIT_MSG@12 PS4, Line 12: Also adding a Kconfig.name and a board_info.txt as checkpatch : looks for it. I am not a big fan of dummy files. I have added a CL to provide exemption for common definitions - https://review.coreboot.org/c/coreboot/+/37807. Let us see if the community likes the exemption.
Hello Karthik Ramasubramanian, Subrata Banik, Arthur Heymans, Maulik V Vaghela, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37705
to look at the new patch set (#5).
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
mainboard/intel/common: Add config for intel common mainboard support
Add config to support intel mainboard common code. The intent is to create common placeholder for APIs common across the mainboard.
Change-Id: Idc6f9bd298520f5e929534c64f1b038a4c9684a9 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- A src/mainboard/intel/common/Kconfig 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/37705/5
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37705 )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
Patch Set 5:
Random thought: Couldn't this be factored out into drivers instead?
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37705 )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
Patch Set 5:
Patch Set 5:
Random thought: Couldn't this be factored out into drivers instead?
I initially had that thought. But some of the things here are more APIs than hardware management/configuration. I can see that the drivers enable configuring the device through device tree and export ACPI objects for use by kernel. I also thought about factoring into lib. But again the things here are more vendor/mainboard specific. Just my thoughts.
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37705 )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
Patch Set 6:
Patch Set 5:
Patch Set 5:
Random thought: Couldn't this be factored out into drivers instead?
I initially had that thought. But some of the things here are more APIs than hardware management/configuration. I can see that the drivers enable configuring the device through device tree and export ACPI objects for use by kernel. I also thought about factoring into lib. But again the things here are more vendor/mainboard specific. Just my thoughts.
Lint check still seems to fail for mb/intel/common CLs, can I keep the Kconfig.name and board_info.txt for now? and remove once we have this CL merged?
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37705 )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 5:
Patch Set 5:
Random thought: Couldn't this be factored out into drivers instead?
I initially had that thought. But some of the things here are more APIs than hardware management/configuration. I can see that the drivers enable configuring the device through device tree and export ACPI objects for use by kernel. I also thought about factoring into lib. But again the things here are more vendor/mainboard specific. Just my thoughts.
Lint check still seems to fail for mb/intel/common CLs, can I keep the Kconfig.name and board_info.txt for now? and remove once we have this CL merged?
sorry wanted to comment on https://review.coreboot.org/c/coreboot/+/37807
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37705 )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
Patch Set 6:
When you want to make such changes to general tree topology, please announce your ideas on the mailing list first. Some things are wired in strange ways and hooked up into automatic generation of tables that used to be presented in the (now obsoleted) wiki.
There is rarely the need to have something tied to "mainboard vendor". These things usually end up being so generic others will want them too, making mb/intel/common somewhat awkward to use. I need to say I did not do thorough review of this, but I am speculating drivers/ might suite us better.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37705 )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
Patch Set 6:
Patch Set 6:
When you want to make such changes to general tree topology, please announce your ideas on the mailing list first. Some things are wired in strange ways and hooked up into automatic generation of tables that used to be presented in the (now obsoleted) wiki.
There is rarely the need to have something tied to "mainboard vendor". These things usually end up being so generic others will want them too, making mb/intel/common somewhat awkward to use. I need to say I did not do thorough review of this, but I am speculating drivers/ might suite us better.
Or even a library in vendorcode? If it really is just related to intel mainboards, why a lib in vendorcode can't serve the needs?
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37705 )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
When you want to make such changes to general tree topology, please announce your ideas on the mailing list first. Some things are wired in strange ways and hooked up into automatic generation of tables that used to be presented in the (now obsoleted) wiki.
There is rarely the need to have something tied to "mainboard vendor". These things usually end up being so generic others will want them too, making mb/intel/common somewhat awkward to use. I need to say I did not do thorough review of this, but I am speculating drivers/ might suite us better.
Or even a library in vendorcode? If it really is just related to intel mainboards, why a lib in vendorcode can't serve the needs?
I had put it in mainboard with a thought that the code added to intel/mb/common would be used by the intel mainboards and in future we can pull in more common pieces from Intel mainboards in common directy , like fmap files, hda verb tables , spd files for common codec and memory modules commonly used on Intel RVPs. Which could make it more Intel mb specific, the vendorcode might be very generic location to hold these mb specific pieces.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37705 )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
When you want to make such changes to general tree topology, please announce your ideas on the mailing list first. Some things are wired in strange ways and hooked up into automatic generation of tables that used to be presented in the (now obsoleted) wiki.
There is rarely the need to have something tied to "mainboard vendor". These things usually end up being so generic others will want them too, making mb/intel/common somewhat awkward to use. I need to say I did not do thorough review of this, but I am speculating drivers/ might suite us better.
Or even a library in vendorcode? If it really is just related to intel mainboards, why a lib in vendorcode can't serve the needs?
I had put it in mainboard with a thought that the code added to intel/mb/common would be used by the intel mainboards and in future we can pull in more common pieces from Intel mainboards in common directy , like fmap files, hda verb tables , spd files for common codec and memory modules commonly used on Intel RVPs. Which could make it more Intel mb specific, the vendorcode might be very generic location to hold these mb specific pieces.
SPD files aren't specific to Intel mainboards. I would suggest having all SPD files in a common folder structure, so that each mainboard's Makefile.inc can pull in the ones it needs from there. What's more: this common folder can be categorized by memory type, module size and vendor! I like ordered things :)
About HDA codec verb tables, I think codec drivers would make sense. That way, verb tables could be replaced with a device on the devicetree. In addition, the devicetree could contain high-level settings instead of raw verbs:
device pci 1b.0 on # HDA controller chip codec/realtek/alc269 # Codec pin configurations would go here. For example: # - Port D (0x14): Internal Speaker # - Port A (0x21): Headphone (SenseA) # - Port F (0x19): Mic2 (SenseB) # - Port E (0x1b): Line2 (internal Mic) # PCBeep is enabled end end
Note that this is just an idea. Before implementing anything, I would bring this up on the mailing list so that we can all brainstorm and choose the best approach.
About fmap files, I'm afraid I don't know enough about them to say.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37705 )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
Patch Set 6:
Note that this is just an idea. Before implementing anything, I would bring this up on the mailing list so that we can all brainstorm and choose the best approach.
Aamir, Can you please initiate a discussion regarding this in the coreboot mailing list?
Also until the discussion is going on in the mailing list, is it ok to continue with the old way of doing things i.e. continue development in the relevant mainboard?
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37705 )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
Patch Set 6:
Patch Set 6:
Note that this is just an idea. Before implementing anything, I would bring this up on the mailing list so that we can all brainstorm and choose the best approach.
Aamir, Can you please initiate a discussion regarding this in the coreboot mailing list?
Also until the discussion is going on in the mailing list, is it ok to continue with the old way of doing things i.e. continue development in the relevant mainboard?
yes, I was thinking of having the board id APIs included in mainboard CL, until we have scope and idea finalized and intercept it later. will revise the CL.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37705 )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
Patch Set 6:
Patch Set 6:
Note that this is just an idea. Before implementing anything, I would bring this up on the mailing list so that we can all brainstorm and choose the best approach.
Aamir, Can you please initiate a discussion regarding this in the coreboot mailing list?
Also until the discussion is going on in the mailing list, is it ok to continue with the old way of doing things i.e. continue development in the relevant mainboard?
Problems mostly arise when we start to fork unnecessary copies of sources. In my opinion you can do development within one mainboard and identify in the Makefile what parts you consider should go into shared location instead.
Consider that board as a proof-of-concept. Depending of how the discussion mailing list goes you get either a quick solution and agreement, or plethora of different opinions.
Personally I don't like cases where common code has dependency of headerfiles located under platform directories. Like <soc/pci_devs.h>. You may want to specifically ask if such approach like <mb/board-specific.h> should be avoided.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37705?usp=email )
Change subject: mainboard/intel/common: Add config for intel common mainboard support ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Aamir Bohra (Code Review) -
Angel Pons (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin L Roth (Code Review)
-
Maulik V Vaghela (Code Review)
-
Werner Zeh (Code Review)