coreboot-gerrit June 2024

coreboot-gerrit@coreboot.org

1 participants
3527 discussions

[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow package power limit clamping
by Anastasios Koutian (Code Review) 30 Jun '24

30 Jun '24

Attention is currently required from: Angel Pons. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/83270?usp=email to look at the new patch set (#4). Change subject: cpu/intel/model_206ax: Allow package power limit clamping ...................................................................... cpu/intel/model_206ax: Allow package power limit clamping Tested on ThinkPad T420 with the i7-3940XM. Change-Id: Id0c0aedc29aca121d0fd1d8f8826089e13a026be Signed-off-by: Anastasios Koutian <akoutian2(a)gmail.com> --- M src/cpu/intel/model_206ax/chip.h M src/cpu/intel/model_206ax/model_206ax_init.c 2 files changed, 12 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/83270/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/83270?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Id0c0aedc29aca121d0fd1d8f8826089e13a026be Gerrit-Change-Number: 83270 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>

1 0

[M] Change in coreboot[main]: RFC: util/autoport: Utilize text/template module
by Nicholas Chin (Code Review) 30 Jun '24

30 Jun '24

Nicholas Chin has posted comments on this change by Nicholas Chin. ( https://review.coreboot.org/c/coreboot/+/83279?usp=email ) Change subject: RFC: util/autoport: Utilize text/template module ...................................................................... Patch Set 3: (1 comment) File util/autoport/templates/hda_verb.c: https://review.coreboot.org/c/coreboot/+/83279/comment/ca672be0_87153fe8?us… : PS3, Line 9: len .PinConfig This is incorrect, as the original code used the length of PinConfig + 1 to account for the AZALIA_SUBVENDOR entry. Sadly, it doesn't seem like you can do arithmetic in Go templates. -- To view, visit https://review.coreboot.org/c/coreboot/+/83279?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I15934c6ebc8a599dbe1c578481de48f135d97232 Gerrit-Change-Number: 83279 Gerrit-PatchSet: 3 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sun, 30 Jun 2024 17:31:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow package power limit clamping
by Anastasios Koutian (Code Review) 30 Jun '24

30 Jun '24

Attention is currently required from: Angel Pons. Anastasios Koutian has posted comments on this change by Anastasios Koutian. ( https://review.coreboot.org/c/coreboot/+/83270?usp=email ) Change subject: cpu/intel/model_206ax: Allow package power limit clamping ...................................................................... Patch Set 3: (3 comments) File src/cpu/intel/model_206ax/model_206ax_init.c: https://review.coreboot.org/c/coreboot/+/83270/comment/e6c379c7_befd65ac?us… : PS2, Line 144: if(conf->pl2_clamp) { > ```suggestion […] Done https://review.coreboot.org/c/coreboot/+/83270/comment/816fa2d2_21982d5c?us… : PS2, Line 153: if (conf->pl2){ > ```suggestion […] Done https://review.coreboot.org/c/coreboot/+/83270/comment/ef977d3f_e498817f?us… : PS2, Line 160: if(conf->pl2_clamp) { > I guess you can run checkpatch locally and fix up the warnings before pushing. […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/83270?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Id0c0aedc29aca121d0fd1d8f8826089e13a026be Gerrit-Change-Number: 83270 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 30 Jun 2024 17:30:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasios Koutian <akoutian2(a)gmail.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>

1 0

[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow PL1/PL2 configuration
by Anastasios Koutian (Code Review) 30 Jun '24

30 Jun '24

Attention is currently required from: Angel Pons, Patrick Rudolph. Anastasios Koutian has posted comments on this change by Anastasios Koutian. ( https://review.coreboot.org/c/coreboot/+/83269?usp=email ) Change subject: cpu/intel/model_206ax: Allow PL1/PL2 configuration ...................................................................... Patch Set 2: (5 comments) File src/cpu/intel/model_206ax/chip.h: https://review.coreboot.org/c/coreboot/+/83269/comment/60b525e7_04212853?us… : PS1, Line 46: int pl1; /* Long-term power limit*/ : int pl2; /* Short-term power limit*/ > I chose this in order to be consistent with the linux kernel's intel-rapl-msr interface (/sys/device […] Done File src/cpu/intel/model_206ax/model_206ax.h: https://review.coreboot.org/c/coreboot/+/83269/comment/7152cb72_dfb1c1c5?us… : PS1, Line 132: void set_power_limits(u8 power_limit_1_time, struct device *); > ```suggestion […] Done File src/cpu/intel/model_206ax/model_206ax_init.c: https://review.coreboot.org/c/coreboot/+/83269/comment/bbc9a138_e1eb2f5c?us… : PS1, Line 137: if(conf->pl1) { > ```suggestion […] Done https://review.coreboot.org/c/coreboot/+/83269/comment/04ccae0c_7f811f98?us… : PS1, Line 138: dev_path(dev) > Ack, will remove Done https://review.coreboot.org/c/coreboot/+/83269/comment/8dbb6a63_82bd74e2?us… : PS1, Line 149: if(conf->pl2){ > ```suggestion […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/83269?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I064af25ec4805fae755eea52c4c9c6d4386c0aee Gerrit-Change-Number: 83269 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 30 Jun 2024 17:29:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasios Koutian <akoutian2(a)gmail.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>

1 0

[XS] Change in coreboot[main]: mb/lenovo/t420: Use vendor default power limits
by Anastasios Koutian (Code Review) 30 Jun '24

30 Jun '24

Attention is currently required from: Alexander Couzens. Anastasios Koutian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/83280?usp=email ) Change subject: mb/lenovo/t420: Use vendor default power limits ...................................................................... mb/lenovo/t420: Use vendor default power limits Also set the vendor default TCC offset temperature Change-Id: Ia187b67ae28fbcda7d5d0e35ec64a3b21d97a21b Signed-off-by: Anastasios Koutian <akoutian2(a)gmail.com> --- M src/mainboard/lenovo/t420/devicetree.cb 1 file changed, 5 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/83280/1 diff --git a/src/mainboard/lenovo/t420/devicetree.cb b/src/mainboard/lenovo/t420/devicetree.cb index 6b721f3..8b6a654 100644 --- a/src/mainboard/lenovo/t420/devicetree.cb +++ b/src/mainboard/lenovo/t420/devicetree.cb @@ -18,6 +18,11 @@ register "spd_addresses" = "{0x50, 0, 0x51, 0}" chip cpu/intel/model_206ax # Values obtained from vendor BIOS + register "tcc_offset" = "3" + register "pl1_uw" = "35000000" + register "pl2_uw" = "43750000" + register "pp0_current_limit" = "97" + register "pp1_current_limit" = "32" register "pp0_psi[VR12_PSI2]" = "{VR12_ALL_PHASES, 5}" register "pp0_psi[VR12_PSI3]" = "{VR12_ALL_PHASES, 1}" register "pp1_psi[VR12_PSI2]" = "{VR12_ALL_PHASES, 5}" -- To view, visit https://review.coreboot.org/c/coreboot/+/83280?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ia187b67ae28fbcda7d5d0e35ec64a3b21d97a21b Gerrit-Change-Number: 83280 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Attention: Alexander Couzens <lynxis(a)fe80.eu>

1 0

[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow PL1/PL2 configuration
by Anastasios Koutian (Code Review) 30 Jun '24

30 Jun '24

Attention is currently required from: Angel Pons, Patrick Rudolph. Hello Angel Pons, Nico Huber, Patrick Rudolph, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/83269?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: cpu/intel/model_206ax: Allow PL1/PL2 configuration ...................................................................... cpu/intel/model_206ax: Allow PL1/PL2 configuration Tested on ThinkPad T420 with the i7-3940XM. Change-Id: I064af25ec4805fae755eea52c4c9c6d4386c0aee Signed-off-by: Anastasios Koutian <akoutian2(a)gmail.com> --- M src/cpu/intel/model_206ax/chip.h M src/cpu/intel/model_206ax/model_206ax.h M src/cpu/intel/model_206ax/model_206ax_init.c M src/northbridge/intel/sandybridge/northbridge.c 4 files changed, 22 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/83269/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/83269?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I064af25ec4805fae755eea52c4c9c6d4386c0aee Gerrit-Change-Number: 83269 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>

1 0

[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow package power limit clamping
by Anastasios Koutian (Code Review) 30 Jun '24

30 Jun '24

Attention is currently required from: Anastasios Koutian. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/83270?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: cpu/intel/model_206ax: Allow package power limit clamping ...................................................................... cpu/intel/model_206ax: Allow package power limit clamping Tested on ThinkPad T420 with the i7-3940XM. Change-Id: Id0c0aedc29aca121d0fd1d8f8826089e13a026be Signed-off-by: Anastasios Koutian <akoutian2(a)gmail.com> --- M src/cpu/intel/model_206ax/chip.h M src/cpu/intel/model_206ax/model_206ax_init.c 2 files changed, 12 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/83270/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/83270?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Id0c0aedc29aca121d0fd1d8f8826089e13a026be Gerrit-Change-Number: 83270 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasios Koutian <akoutian2(a)gmail.com>

1 0

[M] Change in coreboot[main]: RFC: util/autoport: Utilize text/template module
by Nicholas Chin (Code Review) 30 Jun '24

30 Jun '24

Nicholas Chin has uploaded a new patch set (#3). ( https://review.coreboot.org/c/coreboot/+/83279?usp=email ) Change subject: RFC: util/autoport: Utilize text/template module ...................................................................... RFC: util/autoport: Utilize text/template module Go has a template system for generating textual output [0]. One annoyance I find with autoport is that multiline blocks of text in between backticks that are intended to be copied verbatim break the indentation of the main code, as any indents to match the outer code will also show up in the code. For example, the code generating gma-mainboard.ads at the bottom of main.go has the first line immediately inside the call to gma.WriteString, which is indented twice, but the subsequent lines must have no indents so that two extra indents do not appear in the output. Template files make the format of the output more obvious as it does not have to work around the indents of surrounding code. Autoport can also make it less obvious about how a certain line in the output was generated, as various lines may be spread across multiple functions in multiple files. By using templates, the exact field or function call that generates a string in a particular file is made more obvious, which may be useful for those wanting to understand how logs correspond to generated code as well as find sections that need to be updated to account for changes in the coreboot src tree. This replaces the output of certain files with a template driven implementation to give an idea of what is possible and some pros and cons of the approach. - gma-mainboard.ads: Previously, the entire content of this file was generated by copying a block of text verbatim with no dependencies on the values of certain variables. Thus, the new template for this can simply be the generated file as is, which is essentially just copied to the new mainboard directory. This represents the simplest case of using go templates. In these cases, templates have the advantage of being identical in formatting to the output, and can be trivially edited to account for changes needed to the generated file. - Kconfig.name: Previously, the context of this was simply used Fprintf with members of the ctx struct to generate the content of this file. The new template is essentially the same as the generated file, but uses field arguments [1] to use the values of the ctx fields directly. This is a bit more complex compared to the previous example, but is still rather simple compared to other usages of templates. This represents cases where the values of various text in the output is only dependent on fields in a structure or map, and doesn't need special formatting. This has the advantage of being still being relatively simple to update, and you doesn't need to dig through code to determine where the contents of a file are generated. The formatting of the output is also more obvious compared to format strings or blocks of verbatim text in backticks, as noted earlier. - hda_verb.c: Previously, the contents of this file were generated using several loops to iterate through the codecs and nodes. This is a more complex example, utilizing a range action [2] to loop through the codecs array instead of loops in the code. It also uses the printf template function [3] to format some of the values. This particular approach has the disadvantage of using the more complex template syntax which might be less familiar to developers who are used to more traditional fprintf calls and loops, and is less flexible compared to the full capabilities of normal Go code. For this reason the example here does not yet generate the AZALIA_PIN_CFG lines, as I have not yet figured out how to use the template syntax to implement this. In any case, it may not be possible at all, or require such a complex template that any benefit would be negligible. However, it does seem like arbitrary Go functions can be added to a template's function map, allowing more complex output to be defined using standard Go code. This is probably a better approach for this sort of file. TEST=Generated output is identical to the output before this change (except hda_verb.c for reasons noted above) [0]: https://pkg.go.dev/text/template [1]: https://pkg.go.dev/text/template#hdr-Arguments [2]: https://pkg.go.dev/text/template#hdr-Actions [3]: https://pkg.go.dev/text/template#hde-Functions [4]: https://pkg.go.dev/text/template#Template.Funcs Change-Id: I15934c6ebc8a599dbe1c578481de48f135d97232 Signed-off-by: Nicholas Chin <nic.c3.14(a)gmail.com> --- M util/autoport/azalia.go M util/autoport/main.go A util/autoport/templates/Kconfig.name A util/autoport/templates/gma-mainboard.ads A util/autoport/templates/hda_verb.c 5 files changed, 58 insertions(+), 62 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/83279/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/83279?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I15934c6ebc8a599dbe1c578481de48f135d97232 Gerrit-Change-Number: 83279 Gerrit-PatchSet: 3 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[M] Change in coreboot[main]: RFC: util/autoport: Utilize text/template module
by Nicholas Chin (Code Review) 30 Jun '24

30 Jun '24

Nicholas Chin has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/83279?usp=email ) Change subject: RFC: util/autoport: Utilize text/template module ...................................................................... RFC: util/autoport: Utilize text/template module Go has a template system for generating textual output [0]. One annoyance I find with autoport is that multiline blocks of text in between backticks that are intended to be copied verbatim break the indentation of the main code, as any indents to match the outer code will also show up in the code. For example, the code generating gma-mainboard.ads at the bottom of main.go has the first line immediately inside the call to gma.WriteString, which is indended twice, but the subsequent lines must have no indents so that two extra indents do not appear in the output. Template files make the format of the output more obvious as it does not have to work around the indents of surrounding code. Autoport can also make it less obvious about how a certain line in the output was generated, as various lines may be spread across multiple functions in multiple files. By using templates, the exact field or function call that generates a string in a particular file is made more obvious, which may be useful for those wanting to understand how logs correspond to generated code as well as find sections that need to be updated to account for changes in the coreboot src tree. This replaces the output of certain files with a template driven implementation to give an idea of what is possible and some pros and cons of the approach. - gma-mainboard.ads: Previously, the entire content of this file was generated by copying a block of text verbatim with dependancies on the values of certain variables. Thus, the new template for this can simply be the generated file as is, which is essentially just copied to the new mainboard directory. This represents the simplest case of using go templates. In these cases, templates have the advantage of being identical in formatting to the output, and can be trivially edited to account for changes needed to the generated file. - Kconfig.name: Previously, the context of this was simply used Fprintf with members of the ctx struct to generate the content of this file. The new template is essentially the same as the generated file, but uses field arguments [1] to use the values of the ctx fields directly. This is a bit more complex compared to the previous example, but is still rather simple compared to other usages of templates. This represents cases where the values of various text in the output is only dependent on fields in a structure or map, and doesn't need special formatting. This has the advantage of being still being relatively simple to update, and you doesn't need to dig through code to determine where the contents of a file are generated. The formatting of the output is also more obvious compared to format strings or blocks of verbatim text in backticks, as noted earlier. - hda_verb.c: Previously, the contents of this file were generated using several loops to iterate through the codecs and nodes. This is a more complex example, utilizing a range action [2] to loop through the codecs array instead of loops in the code. It also uses the printf template function [3] to format some of the values. This particular approach has the disadvantage of using the more complex template syntax which might be less familiar to developers who are used to more traditional fprintf calls and loops, and is less flexible compared to the full capabilities of normal Go code. For this reason the example here does not yet generate the AZALIA_PIN_CFG lines, as I have not yet figured out how to use the template syntax to implement this. In any case, it may not be possible at all, or require such a complex template that any benefit would be negligible. However, it does seem like arbitrary Go functions can be added to a template's function map, allowing more complex output to be defined using standard Go code. This is probably a better approach for this sort of file. TEST=Generated output is identical to the output before this change (except hda_verb.c for reasons noted above) [0]: https://pkg.go.dev/text/template [1]: https://pkg.go.dev/text/template#hdr-Arguments [2]: https://pkg.go.dev/text/template#hdr-Actions [3]: https://pkg.go.dev/text/template#hde-Functions [4]: https://pkg.go.dev/text/template#Template.Funcs Change-Id: I15934c6ebc8a599dbe1c578481de48f135d97232 Signed-off-by: Nicholas Chin <nic.c3.14(a)gmail.com> --- M util/autoport/azalia.go M util/autoport/main.go A util/autoport/templates/Kconfig.name A util/autoport/templates/gma-mainboard.ads A util/autoport/templates/hda_verb.c 5 files changed, 58 insertions(+), 62 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/83279/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/83279?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I15934c6ebc8a599dbe1c578481de48f135d97232 Gerrit-Change-Number: 83279 Gerrit-PatchSet: 2 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[M] Change in coreboot[main]: RFC: util/autoport: Utilize text/template module
by Nicholas Chin (Code Review) 30 Jun '24

30 Jun '24

Nicholas Chin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/83279?usp=email ) Change subject: RFC: util/autoport: Utilize text/template module ...................................................................... RFC: util/autoport: Utilize text/template module Go has a template system for generating textual output [0]. One annoyance I find with autoport is that multiline blocks of text in between backticks that are intended to be copied verbatim break the indentation of the main code, as any indents to match the outer code will also show up in the code. For example, the code generating gma-mainboard.ads at the bottom of main.go has the first line immediately inside the call to gma.WriteString, which is indended twice, but the subsequent lines must have no indents so that two extra indents do not appear in the output. Template files make the format of the output more obvious as it does not have to work around the indents of surrounding code. Autoport can also make it less obvious about how a certain line in the output was generated, as various lines may be spread across multiple functions in multiple files. By using templates, the exact field or function call that generates a string in a particular file is made more obvious, which may be useful for those wanting to understand how logs correspond to generated code as well as find sections that need to be updated to account for changes in the coreboot src tree. This replaces the output of certain files with a template driven implementation to give an idea of what is possible and some pros and cons of the approach. - gma-mainboard.ads: Previously, the entire content of this file was generated by copying a block of text verbatim with dependancies on the values of certain variables. Thus, the new template for this can simply be the generated file as is, which is essentially just copied to the new mainboard directory. This represents the simplest case of using go templates. In these cases, templates have the advantage of being identical in formatting to the output, and can be trivially edited to account for changes needed to the generated file. - Kconfig.name: Previously, the context of this was simply used Fprintf with members of the ctx struct to generate the content of this file. The new template is essentially the same as the generated file, but uses field arguments [1] to use the values of the ctx fields directly. This is a bit more complex compared to the previous example, but is still rather simple compared to other usages of templates. This represents cases where the values of various text in the output is only dependent on fields in a structure or map, and doesn't need special formatting. This has the advantage of being still being relatively simple to update, and you doesn't need to dig through code to determine where the contents of a file are generated. The formatting of the output is also more obvious compared to format strings or blocks of verbatim text in backticks, as noted earlier. - hda_verb.c: Previously, the contents of this file were generated using several loops to iterate through the codecs and nodes. This is a more complex example, utilizing a range action [2] to loop through the codecs array instead of loops in the code. It also uses the printf template function [3] to format some of the values. This particular approach has the disadvantage of using the more complex template syntax which might be less familiar to developers who are used to more traditional fprintf calls and loops, and is less flexible compared to the full capabilities of normal Go code. For this reason the example here does not yet generate the AZALIA_PIN_CFG lines, as I have not yet figured out how to use the template syntax to implement this. In any case, it may not be possible at all, or require such a complex template that any benefit would be negligible. However, it does seem like arbitrary Go functions can be added to a template's function map, allowing more complex output to be defined using standard Go code. This is probably a better approach for this sort of file. TEST=Generated output is identical to the output before this change (except hda_verb.c for reasons noted above) [0]: https://pkg.go.dev/text/template [1]: https://pkg.go.dev/text/template#hdr-Arguments [2]: https://pkg.go.dev/text/template#hdr-Actions [3]: https://pkg.go.dev/text/template#hde-Functions [4]: https://pkg.go.dev/text/template#Template.Funcs Change-Id: I15934c6ebc8a599dbe1c578481de48f135d97232 Signed-off-by: Nicholas Chin <nic.c3.14(a)gmail.com> --- M util/autoport/azalia.go M util/autoport/main.go A util/autoport/templates/gma-mainboard.ads A util/autoport/templates/hda_verb.c 4 files changed, 56 insertions(+), 62 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/83279/1 diff --git a/util/autoport/azalia.go b/util/autoport/azalia.go index c98b03c..b61dd80 100644 --- a/util/autoport/azalia.go +++ b/util/autoport/azalia.go @@ -1,33 +1,12 @@ package main -import ( - "fmt" - "sort" -) - type azalia struct { } func (i azalia) Scan(ctx Context, addr PCIDevData) { - az := Create(ctx, "hda_verb.c") - defer az.Close() - - Add_gpl(az) - az.WriteString( - `#include <device/azalia_device.h> - -const u32 cim_verb_data[] = { -`) - + createFromTemplate(ctx, "hda_verb.c", ctx) + /* for _, codec := range ctx.InfoSource.GetAzaliaCodecs() { - fmt.Fprintf(az, "\t0x%08x,\t/* Codec Vendor / Device ID: %s */\n", - codec.VendorID, codec.Name) - fmt.Fprintf(az, "\t0x%08x,\t/* Subsystem ID */\n", - codec.SubsystemID) - fmt.Fprintf(az, "\t%d,\t\t/* Number of 4 dword sets */\n", - len(codec.PinConfig)+1) - fmt.Fprintf(az, "\tAZALIA_SUBVENDOR(%d, 0x%08x),\n", - codec.CodecNo, codec.SubsystemID) keys := []int{} for nid, _ := range codec.PinConfig { @@ -42,14 +21,7 @@ } az.WriteString("\n"); } - - az.WriteString( - `}; - -const u32 pc_beep_verbs[0] = {}; - -AZALIA_ARRAY_SIZES; -`) + */ PutPCIDev(addr, "") } diff --git a/util/autoport/main.go b/util/autoport/main.go index b7130b2..0723fef 100644 --- a/util/autoport/main.go +++ b/util/autoport/main.go @@ -10,6 +10,7 @@ "os" "sort" "strings" + "text/template" ) type PCIAddr struct { @@ -154,6 +155,17 @@ return result } +func createFromTemplate(ctx Context, name string, data any) { + f := Create(ctx, name) + defer f.Close() + t := template.Must(template.ParseFiles("templates/" + name)) + err := t.Execute(f, data) + if err != nil { + fmt.Printf("Error: %s", err) + } + +} + func AddBootBlockFile(Name string, Condition string) { BootBlockFiles[Name] = Condition } @@ -527,10 +539,7 @@ } func makeKconfigName(ctx Context) { - kn := Create(ctx, "Kconfig.name") - defer kn.Close() - - fmt.Fprintf(kn, "config %s\n\tbool \"%s\"\n", ctx.KconfigName, ctx.Model) + createFromTemplate(ctx, "Kconfig.name", ctx) } func makeComment(name string) string { @@ -875,32 +884,6 @@ `) if IGDEnabled { - gma := Create(ctx, "gma-mainboard.ads") - defer gma.Close() - - gma.WriteString(`-- SPDX-License-Identifier: GPL-2.0-or-later - -with HW.GFX.GMA; -with HW.GFX.GMA.Display_Probing; - -use HW.GFX.GMA; -use HW.GFX.GMA.Display_Probing; - -private package GMA.Mainboard is - - -- FIXME: check this - ports : constant Port_List := - (DP1, - DP2, - DP3, - HDMI1, - HDMI2, - HDMI3, - Analog, - LVDS, - eDP); - -end GMA.Mainboard; -`) + createFromTemplate(ctx, "gma-mainboard.ads", nil) } } diff --git a/util/autoport/templates/gma-mainboard.ads b/util/autoport/templates/gma-mainboard.ads new file mode 100644 index 0000000..133fde5 --- /dev/null +++ b/util/autoport/templates/gma-mainboard.ads @@ -0,0 +1,23 @@ +-- SPDX-License-Identifier: GPL-2.0-or-later + +with HW.GFX.GMA; +with HW.GFX.GMA.Display_Probing; + +use HW.GFX.GMA; +use HW.GFX.GMA.Display_Probing; + +private package GMA.Mainboard is + + -- FIXME: check this + ports : constant Port_List := + (DP1, + DP2, + DP3, + HDMI1, + HDMI2, + HDMI3, + Analog, + LVDS, + eDP); + +end GMA.Mainboard; diff --git a/util/autoport/templates/hda_verb.c b/util/autoport/templates/hda_verb.c new file mode 100644 index 0000000..7d7f971 --- /dev/null +++ b/util/autoport/templates/hda_verb.c @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <device/azalia_device.h> + +const u32 cim_verb_data[] = { + {{range .InfoSource.GetAzaliaCodecs}} + {{printf "0x%08x" .VendorID}}, /* Codec Vendor / Device ID: {{.Name}} */ + {{printf "0x%08x" .SubsystemID }}, /* Subsystem ID */ + {{len .PinConfig | printf "%d"}}, /* Number of 4 dword sets */ + AZALIA_SUBVENDOR({{printf "%d, 0x%08x" .CodecNo .SubsystemID}}), + {{end}} +}; + +const u32 pc_beep_verbs[0] = {}; + +AZALIA_ARRAY_SIZES; -- To view, visit https://review.coreboot.org/c/coreboot/+/83279?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I15934c6ebc8a599dbe1c578481de48f135d97232 Gerrit-Change-Number: 83279 Gerrit-PatchSet: 1 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com>

1 0

← Newer
1
2
3
4
5
6
7
...
353
Older →

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit June 2024