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@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;
-
Nicholas Chin (Code Review)