coreboot-gerrit May 2022

coreboot-gerrit@coreboot.org

1 participants
6166 discussions

Change in coreboot[master]: util/crossgcc: Update to GCC 11.3
by Felix Singer (Code Review) 02 May '22

02 May '22

Attention is currently required from: Martin L Roth, Elyes Haouas. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63807 ) Change subject: util/crossgcc: Update to GCC 11.3 ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > util/crossgcc/patches are missing That's what CB:63976 does. -- To view, visit https://review.coreboot.org/c/coreboot/+/63807 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1c0aa0952f85afee59a955593c63fcfac756cc60 Gerrit-Change-Number: 63807 Gerrit-PatchSet: 1 Gerrit-Owner: Martin L Roth <martinroth(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin L Roth <martinroth(a)google.com> Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Comment-Date: Mon, 02 May 2022 02:45:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: Documentation/internals: Add devicetree language documentation
by Martin L Roth (Code Review) 02 May '22

02 May '22

Martin L Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/63978 ) Change subject: Documentation/internals: Add devicetree language documentation ...................................................................... Documentation/internals: Add devicetree language documentation Signed-off-by: Martin Roth <gaumless(a)gmail.com> Change-Id: If868ad9a87cb2903bf144996fe3b87d29d4fc755 --- A Documentation/internals/devicetree_language.md 1 file changed, 1,287 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/63978/1 diff --git a/Documentation/internals/devicetree_language.md b/Documentation/internals/devicetree_language.md new file mode 100644 index 0000000..1b2d700 --- /dev/null +++ b/Documentation/internals/devicetree_language.md @@ -0,0 +1,1287 @@ +# Devicetree Language + +Devicetrees use what’s called a Domain Specific Language, so you need to +follow the rules of the language when you’re writing a devicetree. +Fortunately, it’s a relatively simple language, and because its use is +so limited, it can frequently be learned just by following existing +examples. + + +## Comments + +Comments in the devicetree are all single line style. They use the ‘#’ +character to start the comment, and go to the end of the line. + +### Comment Style + +* Put a space between the pound sign and the comment. + * \# Good + * \#Bad +* Sentence fragments are fine, but don’t end them with a period. + + +### When to use comments + +There are standard locations where comments are typically used and +expected. + + +#### Starting a group of settings + +Similar to commenting a function with the intent of the function, it can +be useful to describe at the top of a block of settings with what’s +being configured. + +Below is a good example. + +```text +# VR Settings Configuration for 4 Domains +# +----------------+-----------+-----------+-------------+----------+ +# | Domain/Setting | SA | IA | GT Unsliced | GT | +# +----------------+-----------+-----------+-------------+----------+ +# | Psi1Threshold | 20A | 20A | 20A | 20A | +# | Psi2Threshold | 4A | 5A | 5A | 5A | +# | Psi3Threshold | 1A | 1A | 1A | 1A | +# | Psi3Enable | 1 | 1 | 1 | 1 | +# | Psi4Enable | 1 | 1 | 1 | 1 | +# | ImonSlope | 0 | 0 | 0 | 0 | +# | ImonOffset | 0 | 0 | 0 | 0 | +# | IccMax | 7A | 34A | 35A | 35A | +# | VrVoltageLimit | 1.52V | 1.52V | 1.52V | 1.52V | +# | AC LoadLine | 15 mOhm | 5.7 mOhm | 5.2 mOhm | 5.2 mOhm | +# | DC LoadLine | 14.3 mOhm | 4.83 mOhm | 4.2 mOhm | 4.2 mOhm | +# +----------------+-----------+-----------+-------------+----------+ +``` + +#### End of keyword blocks + +If there are multiple end statements in a row, and it’s not instantly +obvious which end statement closes which keyword block, It’s useful to +add a comment to say what’s being ended. + +Below, we have three end statements in a row and the start of these +blocks are probably close to the top of the file. Identifying what the +‘end’ statement is closing can be determined by the spacing, but it’s +still useful to note it with a comment. + +```text + end # agesa northbridge + end # domain +end # northbridge/amd/agesa/family14/root_complex +``` + + +#### Identifying the reason for a setting + +Registers which don’t have a name that makes it obvious what is being +set, or don’t have a macro that identifies what it’s being set to should +be commented. + +The register setting and macro below both describe what the setting is, +but don’t describe what they’re being used for. Adding the comment of +“Camera”, gives the reader the extra information to know what’s on that +USB port, and understand that you (generally) don’t need overcurrent +protection for a camera. + +```text +register "usb2_ports[7]" = "USB2_PORT_MID(OC_SKIP)" # Camera +``` + +This below register is well named and tells you that there’s going to be +a delay of 12ms, but without the comment, you don’t know why. + +```text +register "stop_delay_ms" = "12" # NIC needs time to quiesce +``` + + +#### Devices without aliases + +It’s been common in the past to comment device identifiers with what the +device is, but this may be less needed now with aliases, which allow +devices to be named in a meaningful way. + +For older platforms which don’t have aliases set in the chipset.cb file, +or for non chipset devices, it’s still useful to add a comment after the +device is set. + + +### When not to use comments + +#### Magic numbers + +This overlaps somewhat with the next section, but if you find yourself +having to put in a comment to explain what’s being set, evaluate whether +it can be expressed as a macro instead. + +``` +register "PmConfigSlpS3MinAssert" = "2" # 50ms +``` + +Instead of using the magic number 2, use a macro, just as you’d be +expected to do in your code. It’s not obvious without the comment what +2 means, so use a macro instead. + +If there's some reason that a macro cannot be used, then sure, add a +comment, but this should be very unusual. + +#### Redundant comments + +With good naming, comments may not be needed. + +Example of redundant comments: + +```text +register "ipc1" = "0x00000000" # IPC1 +``` + +```text +register "link_freq[0]" = "360 * MHz" # 360 MHz +``` + +#### Devices with aliases + +For older chipset or SoC devices, or for non chipset devices that don’t +have a chipset.cb file, it’s still desirable to use comments on the +‘device’ line, but if the device has an alias, it should be used. + + +## Registers + +### Register Names + +Any registers which encode a value that has any sort of unit should +include the unit in the name instead of in a comment. + +The name “tFAW” in the register below is taken directly from the name in +the spec, but adding the units makes it a lot easier to understand. + +```text +register "tFAW" = "40000" # picoseconds +``` + +### Use Macros for encoded values + +If the value associated with the register encodes the unit, instead of +entering the value directly, add a macro for the value. + +```text +register "PchPmSlpS3MinAssert" = "3" # 50ms +``` + +This would be easier to understand if it had a macro, not just a magic +number, as seen below. + +```text +register "PchPmSlpS3MinAssert" = "ASSERT_50_MILLISEC" +``` + +## Devicetree Keywords + +alias, as, chip, cpu, cpu_cluster, device, domain, drq, end, field, +fw_config, generic, gpio, hidden, i2c, inherit, io, ioapic, ioapic_irq, +lapic, mandatory, mmio, off, on, option, pci, pnp, probe, ref, register, +smbios_dev_info, smbios_slot_desc, spi, subsystemid, usb, use + + +## Keyword Types + +The keywords are divided into a number of different categories. A few +keywords fall into more than one of these categories. + + +### Alias Types + +The alias type keywords are used to give human readable names to +devices, then later update that name for a specific usage. + +* alias, as, use + + +### Containers + +These keywords act to group information into logical units of different +sorts. + +* chip, device, domain, field, fw_config, end + + +### Device/Bus Types + +These keywords are used to identify the type of device being configured +and select the appropriate structure for configuration. + +* cpu, cpu_cluster, domain, generic, gpio, i2c, ioapic, lapic, mmio, + pci, pnp, ref, spi, usb + + +### Enumeration Status Modifiers + +The status modifiers determine how a device should be treated during +enumeration. + +* on, off, hidden, mandatory + + +### Firmware configuration + +* field, fw_config, option, probe, ref + + +### Resource allocation + +* drq, io, irq + + +### SMBIOS + +* smbios_dev_info, smbios_slot_desc + + +### Subsystem + +* inherit, subsystemid + + +### Register + +* register + + +### MPTABLE + +* ioapic_irq + + +## Keyword Definitions + +### alias + +* Keyword type: Alias +* Introduced in June of 2020, coreboot version 4.12 +* Usage: `device <device type> <device address> **alias** <ALIAS ID> <status modifier> … end` + +Sometimes, the driver of one device needs to know about another device +that can be anywhere in the device hierarchy. Current applications boil +down to EEPROMs that store information that is consumed by some code +(e.g. MAC address). + +The idea is to give device nodes in the `devicetree.cb` an alias that +can later be used to link it to a device driver's `config` structure. +The driver has to declare a field of type `struct device *`, e.g. + +Override devices can add an alias if it does not exist, but cannot +change the alias for a device that already exists. + +Alias names are checked for conflicts both in the base tree and in the +override tree. + +References are resolved after the tree is parsed so aliases and +references do not need to be in a specific order in the tree. + +Example: +```text +device i2c 0x50 alias my_eeprom on end +``` + +static.c: + +```C +struct some_chip_driver_config { + DEVTREE_CONST struct device *needed_eeprom; +}; +```` + +### as + +* Keyword type: Alias +* Introduced in June of 2020, coreboot version 4.12 +* * Usage: `use <Alias ID> as <Alias ID 2>` + +This keyword is used along with the ‘use’ keyword to give a more +specific name to a generic alias. + +The author of the devicetree is free to choose any alias name that is +unique in the devicetree. Later, when configuring the driver the alias +can be used to link the device with the field of a driver's config as in +the following example. + + +Example: + +```text +chip some/chip/driver + use my_eeprom as needed_eeprom +end +``` + + +### chip + +* Keyword type: Component type +* Introduced in initial sconfig implementation, pre coreboot 4.0 +* Usage: **chip** <path to chip.h file> … end + +The chip keyword defines a section for a device inside a hierarchy of +devices. + +Following the chip keyword is a path to an optional chip.h file. If +this file exists, it should contain a structure of all register settings +available for this device. + +The section started with "chip" is closed with the "end" keyword. + +Note that register commands are chip based, not device based. + +Example: +```text +chip some/chip/driver # comment as desired + # A chip section must have at least one device inside it + … +end # some/chip/driver +``` + + +### cpu + +* Keyword type: Device type +* Introduced in October of 2014, coreboot version 4.0 +* Usage: device **cpu** <Identifier> [alias <alias ID>] <status modifier> … end + +The cpu device type was introduced In order to enumerate CPU devices +that are non-x86 (read: no lapic). + +This device type is not used very much, as most non-x86 platforms just +handle all the CPU configuration at the cpu_cluster level. + +Example: + +```text +device cpu 0 on end +``` + + +### cpu_cluster + +* Keyword type: Device type +* Introduced in initial sconfig implementation, pre coreboot 4.0 as apic_cluster +* Updated in May of 2010 - renamed from apic_cluster to lapic_cluster +* Updated in February of 2013 - Renamed from from lapic_cluster to cpu_cluster +* Usage: `device cpu_cluster <Identifier> [alias <alias ID>] <status modifier> …. end` + +The CPU Cluster is typically one of the two top-level devices in a +devicetree, with the other being the PCI Domain. Like the domain +keyword, the cpu_cluster acts as a bridge device to contain any CPU and +lapic devices. + +On x86 type devices, the CPU Cluster contains CPU chip entries for each +socket which then contain any lapics on the system. + +On ARM or other non-x86 devices, the CPU Cluster can either contain +multiple CPUs or it can just be marked ‘on’, leaving all CPU +configuration to be handled at the CPU Cluster level. + +Example: + +```text +device cpu_cluster 0 on + device lapic 0 on end +end +``` + +### device + +* Keyword type: Component type +* Introduced in: Initial sconfig implementation, pre coreboot 4.0 +* Usage: `device <device type> <Identifier> [alias <alias ID>] <status modifier>…. end` + +A chip defines a collection of one or more devices, and as such, the +device is the fundamental building block of devicetree. + +The device block tells coreboot that a device is present on the +mainboard, gives some information on how to find, access, and work with +the device based on the device type. + +Examples: + +```text +device cpu_cluster 0 on + device lapic 0 on end +end +``` + +### domain + +* Keyword type: Device type +* Introduced in: Initial sconfig implementation, pre coreboot 4.0 +* Updated in: February of 2013: Renamed pci_domain to domain +* Usage: `device domain <identifier> [alias <alias ID>] <status modifier> … end` + +The domain type device acts as a container for one or more chips, defining a logical bus segment. + +As of 2022-04-29, the domain keyword is still used exclusively to define PCI bus segments. + +Example: + +```text +device domain 0 on + device generic 0 alias dptf_policy on end + device pci 1f.0 on # 8086 229c - LPC bridge + device pnp 2e.209 off end # GPIO 4 + end +end +``` + + +### drq + +* Keyword type: Resource type +* Introduced in: Initial sconfig implementation, pre coreboot 4.0 +* Usage: `drq 0x<register #> = <drq line>` + +Drq is used to configure a legacy DMA Request line register for a device +on a legacy bus - ISA/LPC/eSPI. + +The drq configuration is only allowed inside a pnp block. + +Example: + +```text +device pnp 2e.1 on # Parallel port + io 0x60 = 0x378 + irq 0x70 = 7 + drq 0x74 = 3 +end +``` + +### end + +* Keyword type: Component type +* Introduced in: Initial sconfig implementation, pre coreboot 4.0 +* Usage: `<device | chip | domain | field | fw_config> … end` + +End is used to close a chip, device, field, or fw_config block. + +Example: + +```text +chip northbridge/intel/x4x + device domain 0 on + chip southbridge/intel/i82801gx + device pci 1f.0 on # ISA bridge + chip superio/winbond/w83627dhg + device pnp 2e.1 on # LPT + io 0x60 = 0x378 + irq 0x70 = 7 + drq 0x74 = 3 + end # LPT + end # superio/winbond/w83627dhg + end # ISA Bridge + end # Southbridge + end # domain 0 +end # Northbridge +``` + +### field + +* Keyword type: Firmware config +* Introduced in: May of 2020, coreboot version 4.12 +* Usage: `field <Identifier string> <low bit> <high bit> [ | <low bit> <high bit> …] … end` + +Fields define the bits used in the Firmware config runtime options. +Note that the bits in a field can be discontiguous. + +All field definitions must be inside a fw_config block. + +Example: + +```text +field AUDIO 8 10 | 29 29 + option NONE 0 + option MAX98357_ALC5682I_I2S 1 + option MAX98373_ALC5682I_I2S 2 + option MAX98373_ALC5682_SNDW 3 + option MAX98373_ALC5682I_I2S_UP4 4 + option MAX98360_ALC5682I_I2S 5 + option RT1011_ALC5682I_I2S 6 + option AUDIO_FOO 7 + option AUDIO_BAR 8 + option AUDIO_QUUX 9 + option AUDIO_BLAH1 10 + option AUDIO_BLAH2 15 +end +``` + +static_fw_config.h: + +```C + FW_CONFIG_FIELD_AUDIO_MASK 0x20000700 + FW_CONFIG_FIELD_AUDIO_OPTION_NONE_VALUE 0x0 + FW_CONFIG_FIELD_AUDIO_OPTION_MAX98357_ALC5682I_I2S_VALUE 0x100 + FW_CONFIG_FIELD_AUDIO_OPTION_MAX98373_ALC5682I_I2S_VALUE 0x200 + FW_CONFIG_FIELD_AUDIO_OPTION_MAX98373_ALC5682_SNDW_VALUE 0x300 + FW_CONFIG_FIELD_AUDIO_OPTION_MAX98373_ALC5682I_I2S_UP4_VALUE 0x400 + FW_CONFIG_FIELD_AUDIO_OPTION_MAX98360_ALC5682I_I2S_VALUE 0x500 + FW_CONFIG_FIELD_AUDIO_OPTION_RT1011_ALC5682I_I2S_VALUE 0x600 + FW_CONFIG_FIELD_AUDIO_OPTION_AUDIO_FOO_VALUE 0x700 + FW_CONFIG_FIELD_AUDIO_OPTION_AUDIO_BAR_VALUE 0x20000000 + FW_CONFIG_FIELD_AUDIO_OPTION_AUDIO_QUUX_VALUE 0x20000100 + FW_CONFIG_FIELD_AUDIO_OPTION_AUDIO_BLAH1_VALUE 0x20000200 + FW_CONFIG_FIELD_AUDIO_OPTION_AUDIO_BLAH2_VALUE 0x20000700 +``` + +### fw_config + +* Keyword type: Firmware config +* Introduced in: May of 2020, coreboot version 4.12 +* Usage: fw_config + +Fw_config is the top level token for defining firmware config fields. +All field and option definitions must be inside the fw_config block. +Note that the table can be defined before any chips are configured. + +Example: +```text +fw_config + field USB_DB 0 1 + option USB_DB_A1_PS8811_C1_PS8818 0 + option USB_DB_A1_ANX7491_C1_ANX7451 1 + end + field FORM_FACTOR 2 + option FORM_FACTOR_CLAMSHELL 0 + option FORM_FACTOR_CONVERTIBLE 1 + end +end +``` + + +### generic + +* Keyword type: Device type +* Introduced in: May of 2016, coreboot version 4.4 +* Usage: `device generic <identifier> [alias <alias ID>] <status modifier> … end` + +This allows the devicetree to describe a device that does not have a +specific bus, but may need to be described in tables for the operating +system. For instance some chips may have various GPIO connections that +need to be described but do not fall under any other device. + +Example: +```text +device pci 0c.0 on # CNVi + chip drivers/wifi/generic + register "wake" = "GPE0A_CNVI_PME_STS" + device generic 0 on end + end +end +``` + + +### gpio +* Keyword type: Device type +* Introduced in: December 2020, coreboot version 4.13 +* Usage: device **gpio** <identifier> [alias <alias ID>] <status modifier> … end + +The general idea behind this is that every chip can have gpios that +shall be accessible in a very generic way by any driver through the +devicetree. + +The chip that implements the chip-specific gpio operations has to assign +them to the generic device operations struct, which then gets assigned +to the gpio device during device probing. See CB:48583 for how this gets +done for the SoCs using intel/blocks/gpio. + +The gpio device then can be added to the devicetree with an alias name +like in the following example: + +```text +chip soc/whateverlake + device gpio 0 alias soc_gpio on end + ... +end +``` + +Any driver that requires access to this gpio device needs to have a +device pointer (or multiple) and an option for specifying the gpio to be +used in its chip config like this: + +```C +struct drivers_ipmi_config { + ... + DEVTREE_CONST struct device *gpio_dev; + u16 post_complete_gpio; + ... +}; +``` + +The device `soc_gpio` can then be linked to the chip driver's `gpio_dev` +above by using the syntax `use ... as ...`, which was introduced in +commit 8e1ea52: + +```text +chip drivers/ipmi + use soc_gpio as gpio_dev + register "bmc_jumper_gpio" = "GPP_D22" + ... +end +```` + +The IPMI driver can then use the generic gpio operations without any knowlege of the chip's specifics: + +```C +unsigned int gpio_val; +const struct gpio_operations *gpio_ops; +gpio_ops = dev_get_gpio_ops(conf->gpio_dev); +gpio_val = gpio_ops->get(conf->bmc_jumper_gpio); +``` + +For a full example have a look at CB:48096 and CB:48095. + + +### hidden + +* Keyword type: Enumeration Status Modifier +* Introduced in: September 2018, coreboot version 4.8 +* Updated in: February of 2020, coreboot version 4.11 +* Usage: `device <device type> <identifier> [alias <alias ID>] hidden … end` + +The "Hidden" keyword was added to support devices sharing the same +driver to update their ACPI status with different values. If a device +is marked as Hidden, it omits the “SHOW_IN_UI” field, which prevents the +device from showing up in the OSPM user interface. This was added to +prevent ChromeOS specific devices from showing up in Windows, for +example. It does not look like this keyword was ever actually used in +this capacity, but the code for this is still present. + +In 2020, the “Hidden” keyword’s use was expanded to fix the problem of +PCI devices which have been hidden by disabling access to the config +space. Because the Device/Vendor IDs read back as 0xffffffff from the +hidden device, it appears that there is no PCI device located at that +Bus/Device/Function. This causes coreboot to remove the device at the +end of PCI enumeration. To fix this, if a device uses 'hidden' instead +of 'on', then it will be assumed during PCI enumeration that the device +indeed does exist, and it will not be removed. This allows coreboot to +manually assign resources to the device even though it doesn’t appear to +exist. + +Example: +```text +device pci 1f.2 alias pmc hidden end +``` + +static.c: + +``` +.enabled = 1, +.hidden = 1, +.mandatory = 0, +``` + + +### i2c + +* Keyword type: Device type +* Introduced in: Initial sconfig implementation, pre coreboot 4.0 +* Usage: `device I2c <identifier> [alias <alias ID>] <status modifier> … end` + +Example: + +```text +device i2c 2c on end +chip drivers/i2c/generic +register "hid" = ""ELAN0000"" + register "desc" = ""ELAN Touchpad"" + register "irq_gpio" = "ACPI_GPIO_IRQ_LEVEL_LOW(GPIO_9)" + register "wake" = "GEVENT_22" + register "probed" = "1" + device i2c 15 on end +end +``` + +### inherit + +* Keyword type: subsystem +* Introduced in: March of 2011, coreboot version 4.0 +* Usage: `subsystemid beee c001 inherit` + +Used to modify the subsystemid keyword, inherit makes the ID apply to +all subdevices and functions. + +Example: +```text +device pci 00.0 on + subsystemid 0xdead 0xbeef inherit +end +``` + + +### io + +* Keyword type: Resource +* Introduced in: Initial sconfig implementation, pre coreboot 4.0 +* Usage: `io 0x<address> = 0x<value>` + +The io command is used to set an legacy device's io register to a value. + +Example: + +```text +device pnp 4e.06 on # Keyboard + io 0x60 = 0x0060 + io 0x62 = 0x0064 + irq 0x70 = 1 +end +``` + +### ioapic + +* Keyword type: Device type +* Introduced in: +* Usage: `device ioapic <identifier> [alias <alias ID>] <status modifier> … end` + +Example: + + +### ioapic_irq + +* Keyword type: Mpinit type +* Introduced in: June of 2012, coreboot version 4.0 +* Usage: `ioapic_irq <APICID> <INTA|INTB|INTC|INTD> <INTPIN>` + +This keyword is used to support autogeneration of the MPTABLE from +devicetree.cb. This is done by a write_smp_table() declared weak in +mpspec.c. If the mainboard doesn't provide its own function, this +generic implementation is called. + +The ioapic_irq directive can be used in pci and pci_domain devices. If +there's no directive, the autogen code traverses the tree back to the +pci_domain and stops at the first device which sets such a directive, +and uses that information to generate the entry according to PCI IRQ +routing rules. + +Example: +```text +``` + + +### irq + +* Keyword type: Resource type +* Introduced in initial sconfig implementation, pre coreboot 4.0 +* Usage: `irq 0x<address> = 0x<value>` + +Irq is used to configure a legacy interrupt Request line register for a device on a legacy bus - ISA/LPC/eSPI. + +The irq configuration is only allowed inside a pnp block. + +Example: +```text +irq 0xc5 = 0x1f +``` + +```text +chip superio/fintek/f71808a + device pnp 4e.4 on # Hardware monitor + io 0x60 = 0x295 + irq 0x70 = 0 + end +end +``` + +### lapic + +* Keyword type: Device type +* Introduced in initial sconfig implementation, pre coreboot 4.0 +* Updated in May of 2010 - Renamed apic to lapic +* Usage: `device lapic <identifier> [alias <alias ID>] <status modifier> … end` + +Defines an lapic device on an x86 mainboard. + +Example: + +```text +device cpu_cluster 0 on # (L)APIC cluster + chip cpu/intel/slot_1 # CPU socket 0 + device lapic 0 on end # Local APIC of CPU 0 + end + chip cpu/intel/slot_1 # CPU socket 1 + device lapic 1 on end # Local APIC of CPU 1 + end +end +``` + +### mandatory + +* Keyword type: Enumeration Status Modifier +* Introduced in: February of 2020, coreboot version 4.11 +* Usage: `device <device type> <identifier> [alias <alias ID>] mandatory … end` + + +The Mandatory keyword allows for minimal PCI scanning which speeds up the boot process. Only devices marked "mandatory" get enabled and scanned during PCI enumeration when the MINIMAL_PCI_SCANNING Kconfig option is enabled. + +If MINIMAL_PCI_SCANNING is not enabled, this means the same as ‘on’ + + +static.c: +```C +.enabled = 1, +.hidden = 0, +.mandatory = 1, +``` + +example: +``` +device pci 1f.0 mandatory # LPC + chip drivers/pc80/tpm + device pnp 0c31.0 on end + end # tpm +end # LPC +``` + +### mmio + +* Keyword type: Device type +* Introduced in January of 2018, coreboot version 4.7 +* Usage: `device mmio 0x<address> [alias <alias ID>] <status modifier> … end` + +The mmio keyword allows fixed memory-mapped IO addressed devices to be assigned to given values. + +AMD platforms perform a significant amount of configuration through these MMIO addresses, including I2C bus configuration. + +example: +```text +device mmio 0xfedc9000 alias uart_0 off end +device mmio 0xfedca000 alias uart_1 off end +device mmio 0xfedc2000 on # I2C 0 + + chip drivers/generic/adau7002 + device generic 0.0 on end + end + + chip drivers/i2c/da7219 + register "irq_gpio" = "ACPI_GPIO_IRQ_EDGE_LOW(GPIO_14)" + end + + chip drivers/generic/max98357a + register "hid" = ""MX98357A"" + register "sdmode_delay" = "5" + device generic 0.1 on end + end + +end # I2C 0 +``` + + +### off + +* Keyword type: Enumeration Status Modifier +* Introduced in: Initial sconfig implementation, pre coreboot 4.0 +* Usage: `device <device type> <identifier> [alias <alias ID>] off … end` + +Indicates that the device should be disabled if possible and should not +be enumerated or configured. To disable a device, the code to disable it +must be present in the chipset/SoC code, though it isn’t always possible +to actually disable the device. + + +Note that If a device (especially PCI devices) is marked as ‘off’, but +does not get disabled, the OS or a driver may discover the device during +its enumeration sequence and assign it resources anyway. + + +static.c: + +```C +.enabled = 0 +.hidden = 0, +.mandatory = 0, +``` + +example: + +```text +device usb 2.6 off end +device pnp 2e.0 off end # FDC +device pci 15.2 off end +``` + +### on + +* Keyword type: Enumeration Status Modifier +* Introduced in: Initial sconfig implementation, pre coreboot 4.0 +* Usage: `device <device type> <identifier> [alias <alias ID>] on … end` + +Marks a device as present and enabled. This indicates that the device +should be enumerated and configured. + + +static.c: + +```C +.enabled = 1 +.hidden = 0, +.mandatory = 0, +``` + +Example: + +```text +device lapic 0 on end +device pci 00.0 on end # Host Bridge +device pnp 0c31.0 on end +``` + +### option + +* Keyword type: Firmware config +* Introduced in: +* Usage: `option <name> <value>` + +Options define the values which may be used in a firmware config field. The option keyword may only be used inside a field, which may only be used inside an fw_config block. + +Example: + +```text +fw_config + field OLED_SCREEN 28 + option OLED_NOT_PRESENT 0 + option OLED_PRESENT 1 + end +end +``` + +### pci + +* Keyword type: Device type +* Introduced in: Initial sconfig implementation, pre coreboot 4.0 +* Usage: `device pci <dev.func> [alias <alias ID>] <status modifier> …. end` + +The pci device type defines a PCI or PCIe device on the PCI logical bus. + +Resources for all PCI devices are assigned automatically, or must be +assigned in code if they’re non-standard. + +Currently, only a single segment is supported, but there is work to make +multiple different segments supported, each with a bus 0. Because the +bus is not specified, It’s assumed that all pci devices that are not +behind a pci bridge device are on bus 0. If there are additional pci +busses in a chip, they can be added behind their bridge device. + +Examples: + +```text +device pci 1a.0 on end # USB2 EHCI #2 +device pci 1b.0 on # High Definition Audio + subsystemid 0x1a86 0x4352 +end +``` + +Example - ISA bus under PCI to ISA bridge: + +```text +device pci 4.0 on # ISA bridge + chip superio/winbond/w83977tf # Super I/O + device pnp 3f0.2 on # COM1 + io 0x60 = 0x3f8 + irq 0x70 = 4 + end + end +end +``` + +Example - PCI Bus under a PCI to PCI bridge: + +```text +device pci 08.0 on end # Dummy Host Bridge, do not disable +device pci 08.1 alias gpp_bridge_a off # Internal GPP Bridge 0 to Bus A + device pci 0.0 alias gfx off end # Internal GPU (GFX) + device pci 0.1 alias gfx_hda off end # Display HDA (GFXAZ) +end +``` + +### pnp + +* Keyword type: Device type +* Introduced in: Initial sconfig implementation, pre coreboot 4.0 +* Usage: `device pnp <Identifier> [alias <alias ID>] <status modifier>…. end` + +pnp is used to define an IO addressed device within a chip on a legacy +bus - ISA, LPC, or eSPI. The identifier is the IO address used to +access the chip, followed by the identifier for the device within the +chip. + +The io, irq, and drq resource identifiers are allowed only inside a pnp +device. + +Example: + +```text +chip superio/ite/it8783ef + device pnp 2e.0 off end # Floppy + device pnp 2e.1 on # COM 1 + io 0x60 = 0x3f8 + irq 0x70 = 4 + end +end +``` + +### probe + +* Keyword type: Firmware config +* Introduced in May of 2020, coreboot version 4.12 +* Usage: `probe <Field Name> <Option Name>` + +The fields and options can be used to probe for a device and have that +device be disabled if it is not found at boot time. + +Example: + +```text +fw_config + field FEATURE 0 0 + option DISABLE 0 + option ENABLE 1 + end +end + +chip drivers/generic/feature + device generic 0 on + probe FEATURE ENABLE + end +end +``` + + +### ref + +* Keyword type: Device type (alias) +* Introduced in July of 2020, coreboot version 4.12 +* Usage: `device REF <Identifier> <status modifier>…. end` + +This allows the chipset to assign alias names to devices as well as set +default register values. This works for both the baseboard devicetree.cb +as well as variant overridetree.cb. + +Example: + +chipset.cb: + +```text +device pci 15.0 alias i2c0 off end +``` + +devicetree.cb: + +```text +device ref i2c0 on end +``` + +### register + +* Keyword type: Register +* Introduced in: Initial sconfig implementation, pre coreboot 4.0 +* Usage: `register "name" = "value"` + +The register keyword was initially intended to supply just that - +information for device register values. Over the years, its use has been +expanded to much more than this, becoming a general configuration +mechanism. + +All values set by the register keyword MUST be defined in the chip.h +file. The register keyword only works at the chip level, not at the +device level. + +Any register defined in the chip.h file that is not set in the +devicetree file gets set to zero. + +To double quote a value, use two sets of quotes: register "name" = +""string"" + +Examples: + +```text +register "desc" = ""USB2 Type-A Rear Lower"" +register "pcie_power_limits" = "{ { 10, 0 }, { 0, 0 }, { 0, 0 }, + { 0, 0 }, { 10, 0 }, { 0, 0 } }" +register "SataPortsEnable[0]" = "1" +register "generic.irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)" +register "common_soc_config" = "{ + // Touchpad I2C bus + .i2c[0] = { + .speed = I2C_SPEED_FAST, + .rise_time_ns = 80, + .fall_time_ns = 110, + }, + }" +``` + + +### smbios_dev_info + +* Keyword type: smbios +* Introduced in: May of 2019, coreboot version 4.9 +* Usage: `smbios_dev_info <Identifier>` + +Specify the instance ID and RefDes (Reference Designation) of onboard devices. + +The `SMBIOS_TYPE41_PROVIDED_BY_DEVTREE` Kconfig option enables using this syntax to control the generated Type 41 entries (Onboard Devices Extended Information). When this option is enabled, Type 41 entries are only autogenerated for devices with a defined instance ID. This avoids having to keep track of which instance IDs have been used for every device class. + +Using `smbios_dev_info` when `SMBIOS_TYPE41_PROVIDED_BY_DEVTREE` is not enabled will result in a build-time error, as the syntax is meaningless in this case. This is done with preprocessor guards around the Type 41 members in `struct device` and the code which uses the guarded members. Although the preprocessor usage isn't particularly elegant, adjusting the devicetree syntax and/or grammar depending on a Kconfig option is probably even worse. + + +Example: + +```text +device pci 1c.0 on # PCIe Port #1 + device pci 00.0 on + smbios_dev_info 6 + end +end +device pci 1c.1 on # PCIe Port #2 + device pci 00.0 on + smbios_dev_info 42 "PCIe-PCI Time Machine" + end +end +```` + +### smbios_slot_desc + +* Keyword type: smbios +* Introduced in: May of 2019, coreboot version 4.9 +* Usage: `smbios_slot_desc** <type> <length> [designation] [data width]` + +Add the new field 'smbios_slot_desc', which takes 2 to 4 arguments. +The field is only valid for PCI devices and only compiled if SMBIOS +table generation is enabled. + +smbios_slot_desc arguments - described in src/include/smbios.h: + +1. slot type: enum misc_slot_type +2. slot length: enum misc_slot_length +3. slot designation (optional): text string +4. slot data width (optional): enum slot_data_bus_bandwidth + +Example: + +```text +device pci 1c.1 on + smbios_slot_desc "SlotTypePciExpressGen3X16" "SlotLengthOther" "SLOT2" "SlotDataBusWidth8X" +end # PCIe Port #2 Integrated Wireless LAN +``` + +### spi + +* Keyword type: Device type +* Introduced in: February of 2017, coreboot version 4.5 +* Usage: device **SPI** <Identifier> [alias <alias ID>] <status modifier>…. end + +The SPI device takes only one parameter, the chip-select for the device on the SPI bus. + +Example: + +```text +device spi 0 alias spi_tpm on end +``` + + +### subsystemid + +* Keyword type: Subsystem +* Introduced in: March of 2011, coreboot version 4.0 +* Usage: `subsystemid <vendor> <device> [inherit]` + +Sets a PCI subsystem ID. + +If the user wants to have this ID inherited to all subdevices/functions, +they can add the keyword 'inherit' after the ID. + +If the user doesn’t want to inherit the subsystem value for a single +device, they can specify 'subsystemid 0 0' on this particular device. + + +Example: + +```text +device pci 00.0 on + subsystemid 0xdead 0xbeef +end +``` +or + +```text +device pci 00.0 on + subsystemid 0xdead 0xbeef inherit +end +``` + +### usb + +* Keyword type: Device type +* Introduced in: May of 2018, coreboot version 4.7 +* Usage: `device usb <Identifier> [alias <alias ID>] <status modifier> ... end` + +This supports describing USB ports in the devicetree. It allows a USB +port location to be described in the tree with configuration +information, and ACPI code to be generated that provides this +information to the OS. + +The usb symbol works with the scan_usb_bus() operation to scan bridges +for devices so a tree of ports and hubs can be created. + +The device address is computed with a 'port type' and a 'port id' which +is flexible for the SOC to handle depending on their specific USB setup +This also allows USB2 and USB3 ports to be described separately. + +For example a board may have devices on two ports, one with a USB2 +device and one with a USB3 device, both of which are connected to an +xHCI controller with a root hub: + +```text + xHCI + | + RootHub + | | + USB2[0] USB3[2] +``` + +This is described by the following example: + +```text +device pci 14.0 on + chip drivers/usb/acpi + register "name" = ""Root Hub"" + device usb 0.0 on + + chip drivers/usb/acpi + register "name" = ""USB 2.0 Port 0"" + device usb 2.0 on end + end + + chip drivers/usb/acpi + register "name" = ""USB 3.0 Port 2"" + device usb 3.2 on end + end + + end # device usb 0.0 + end # chip drivers/usb/acpi +end # device pci 14.0 +``` + +### use + +* Keyword type: Alias +* Introduced in September of 2020, coreboot version 4.12 +* Usage: `use <Alias ID> as <Alias ID2>` + +The author of the devicetree is free to choose any alias name that is +unique in the devicetree. Later, when configuring the driver for a +board, the alias can be used to link the device with the field of a +driver's config, giving it a specific name. + +Example: + +```text +chip some/chip/driver + use my_eeprom as needed_eeprom +end +``` -- To view, visit https://review.coreboot.org/c/coreboot/+/63978 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If868ad9a87cb2903bf144996fe3b87d29d4fc755 Gerrit-Change-Number: 63978 Gerrit-PatchSet: 1 Gerrit-Owner: Martin L Roth <martinroth(a)google.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: Documentation/internals: Add devicetree documentation
by Martin L Roth (Code Review) 02 May '22

02 May '22

Martin L Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/63977 ) Change subject: Documentation/internals: Add devicetree documentation ...................................................................... Documentation/internals: Add devicetree documentation Signed-off-by: Martin Roth <gaumless(a)gmail.com> Change-Id: I2a43a96911844bd2b682004d5423126ad00a4bf3 --- A Documentation/internals/devicetree.md 1 file changed, 621 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/63977/1 diff --git a/Documentation/internals/devicetree.md b/Documentation/internals/devicetree.md new file mode 100644 index 0000000..c1c7cd5 --- /dev/null +++ b/Documentation/internals/devicetree.md @@ -0,0 +1,621 @@ + +# Devicetree + +## Introduction to the coreboot devicetree + +The first thing that may come to mind when one hears “DeviceTree" is a +different sort of description file that is generally passed to the Linux +kernel to describe a system’s components. Both that devicetree and +coreboot’s devicetree serve fundamentally the same purpose, but are +otherwise unrelated and have completely different syntax. The term +devicetree was used long before either version was created, and was +initially used in coreboot as a generic term. + +coreboot's devicetree’s main use is to define and describe the runtime +configuration and settings of the hardware on a board, chip or device +level. It defines which of the functions of the chips on the board are +enabled, and how they’re configured. + +The devicetree file is parsed during the build process by a utility +named sconfig, which translates the devicetree into a tree of C +structures containing the included devices. This code is placed in the +file static.c and a couple of header files all under the build +directory. This file is then built into the binaries for the various +coreboot stages and is referred to during the coreboot boot process. + +For the early stages of the coreboot boot process, the data that is +generated by sconfig is a useful resource, but this structure is the +critical architectural glue of ramstage. This structure gets filled in +with pointers to every chip's initialization code, allowing ramstage to +find and initialize those devices through the chip_operations +structures. + + +### History of coreboot’s devicetree + +The initial devicetree in coreboot was introduced in 2003 by Ron Minnich +as a part of the linuxbios config file, 'config.lb'. At this point both +the devicetree and config options were in the same file. In 2009, +when Kconfig was added into the coreboot build, devicetree was split +out into its own file for each mainboard in a commit with this message: + +```text +devicetree.cb + +The devicetree that formerly resided in src/mainboard/*/*/Config.lb. + +Just without the build system crap +``` + +The devicetree structure was initially mainly used only in ramstage for +PCI device enumeration, configuration and resource allocation. It has +since expanded for use in the pre-ram stages as a read-only structure. + +The language used in the devicetree has been expanded greatly since it +was first introduced as well, adding new features every year or so. + + +### Devicetree Registers + +In coreboot, the devicetree register setting is one of the two main +methods used to configure a board’s properties. In this way, devicetree +is similar in function to Kconfig. It’s more flexible in many ways as +it can specify not only single values, but also arrays or structures. +It's also even more static than Kconfig because there’s no update +mechanism for it other than editing the devicetree files. + + +### Adding new static configuration options: Devicetree or Kconfig + +When adding options for a new board or chip, there is frequently a +decision that needs to be made in terms of how the option should be +added. Using the devicetree or Kconfig are the two typical methods of +build-time configuration. Below are some general rules of when each one +should be used. + +Kconfig should be used if the option is used to configure the build in a +Makefile, or if the option is something that should be user selectable. +Kconfig is also preferred if the configuration is a global option and +not limited to a single chip. Another thing that Kconfig is good at is +handling decisions based on other configuration options, which isn’t +something that devicetree can really do. + +Devicetree should obviously be used to define the configuration of the +hardware hierarchy, but it's also preferred if the option is only used +in the C code and is static for a mainboard. or if the option is chip +specific. As mentioned earlier, Devicetree registers can also be used +to create structures or arrays that Kconfig can't. + +Both Kconfig and devicetree can be used in the C code for runtime +configuration, but there’s a huge difference on the back-end about how +these are handled. Because Kconfig generates a #define of the choice, +the compiler can eliminate any code paths not used by the option. +Devicetree options, on the other hand, are actual runtime selections and +the code for any and all choices will remain in the final build. + + +## Three levels of devicetree files + +There are currently three different levels of devicetrees used to build +up the structure of components and register values in coreboot. From +the lowest, most general level to the highest and most specific, they +are chipset.cb, devicetree.cb and overridetree.cb. + +Unless there’s a specific reason to name them something other than these +names, they are the names that should be used. + +For newer SoCs and chipset, there will generally be a chipset.cb file, +and every mainboard is required to have a devicetree.cb file, although +it can be empty if somehow everything is the same as the SoC level file. +An overridetree.cb file is only required if variants have differences +from the devicetree at the primary mainboard level. + + +### SoC / chipset level, chipset.cb + +The chipset.cb file was added in October of 2020 allowing a single +chipset or SoC to provide a "base level" devicetree, cutting down on +duplication between mainboards. + +The chipset.cb file also typically will define human readable "aliases" +for particular devices so that mainboards can use those instead of just +the PCI routing numbers and the like. + +The use of the chipset.cb file is specified in Kconfig by the symbol +CONFIG_CHIPSET_DEVICETREE. + +In chipset.cb file, you might see a couple of lines like this: + +```text +device pci 17.0 alias sata off end +device pci 1e.0 alias uart0 off end +``` + +This sets the aliases for these PCI devices and default them to being +disabled. + + +### Primary mainboard level, devicetree.cb + +Each mainboard must have a devicetree.cb file. The filename and path is +set to a default value of `src/mainboard/<VENDOR><MBNAME>/devicetree` by +the CONFIG_DEVICETREE symbol in Kconfig. + +If a mainboard and variants typically wanted both devices enabled, +you’d see the following in devicetree.cb: + +```text +device ref sata on end +device ref uart0 on end +``` + +### Mainboard variant level, overrridetree.cb + +Introduced in 2018 to reduce duplication and maintenance of variant +boards, the overridetree.cb file is the most specific level of +devicetree file. + +This allows for a base devicetree at the top mainboard level that is +shared by all variants, and each variant only needs to update for their +specific values + +The override tree filename is set in Kconfig with the +OVERRIDE_DEVICETREE symbol and is typically named "overridetree.cb". + +Finally, if one of the variants of a mainboard came without a sata +connector, it could again disable sata with the following in +overridetree.cb: + +```text + device ref sata on end +``` + +## Additional files + + +### chip.h files + +coreboot looks at the chip as a collection of devices. That collection +can be a single device or multiple different devices. The ‘chip’ +keyword which will be described more later, starts this collection of +devices. Following the ‘chip’ keyword is a directory name which +contains the code to deal with that collection of devices. There may +optionally be a chip.h file in that directory which will also be parsed +to create the devicetree data structures. + +If the chip.h file is present in that directory, it is used to define a +structure containing the "register definitions" for the chip. The +values for this structure's members get set in one of the devicetree +files. If not specifically set in one of the devicetree files, the +value of the register defaults to 0. The chip.h file frequently also +contains macros, enums and sub-structures used to set the register +structure's members. + +The structure for the chip’s register definition is named after the +directory that would contains the chip.h file, with the slashes changed +to underscores, then appended by ‘_config’. The src/ directory is +omitted. + +This means that a line in a devicetree file containing +`chip drivers/i2c/hid` would use `src/drivers/i2c/hid/chip.h`, and the +register definition structure it contains would be named +`drivers_i2c_hid_config`. + + +Here is the contents of that chip.h file: + +```text +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __DRIVERS_I2C_HID_CHIP_H__ +#define __DRIVERS_I2C_HID_CHIP_H__ +#include <drivers/i2c/generic/chip.h> +#define I2C_HID_CID "PNP0C50" + +struct drivers_i2c_hid_config { + struct drivers_i2c_generic_config generic; + uint8_t hid_desc_reg_offset; +}; + +#endif /* __I2C_HID_CHIP_H__ */ +``` + +## The sconfig utility and generated files + + +### coreboot/util/sconfig + +Sconfig is the tool that parses the coreboot devicetrees and turns them +into a collection of C style structures. This is a coreboot specific +tool, compiled with flex & bison to define and parse the domain specific +language used by coreboot’s device + +Sconfig is called by the makefile during the build process, and +shouldn’t generally need to be run directly. Despite that, it obviously +can be run manually if required, and gives the basic command line +options used to control it. + +```text +usage: sconfig <options> + + -c | --output_c : Path to output static.c file (required) + -r | --output_h : Path to header static.h file (required) + -d | --output_d : Path to header static_devices.h file (required) + -f | --output_f : Path to header static_fw_config.h file (required) + -m | --mainboard_devtree : Path to mainboard devicetree file (required) + -o | --override_devtree : Path to override devicetree file (optional) + -p | --chipset_devtree : Path to chipset/SOC devicetree file (optional) +``` + +### sconfig inputs + +The sconfig input files chip.h, chipset.cb, devicetree.cb and +overridetree.cb were discussed previously. As the usage above shows, +the only required input file is the mainboard devicetree. The additional +devicetree files, chipset.cb and overridetree.cb are optional, and the +chip.h files do not need to be specified, because their location is +contained inside the files + +Constructing the devicetree input files will be discussed later. + + +### sconfig outputs + +#### static.c + +This file is the primary file generated by sconfig. It contains the +main data about the component busses and interfaces. + +For historic reasons, static.c is generated in the +`build/mainboard/<VENDOR>/<MBNAME> directory` + + +#### static.h + +The static.h file is the main header file included by most coreboot +files that need to use the devicetree data. This is included by +src/include/device/device.h, which contains all of the definitions, +structures, and function prototypes for dealing with the devicetree +generated output. + +Static.h used to contain all of the output directly, but as of October +2020, simply contains include statements bringing in the other two +generated header files. This allows the two headers to be included +separately so that the firmware config options can be used in a payload. + + +#### static_devices.h + +The file static_devices.h contains extern declarations of all of the +device structures defined in static.c. + + +#### static_fw_config.h + +static_fw_config.h contains the FW_CONFIG_FIELD_* macros only, which +makes it easily consumable by a payload or anything else which wishes to +use the platform’s FW_CONFIG fields. + +## Devicetree Example + + +### A very simple devicetree + +This is the devicetree.cb file in src/mainboard/sifive/hifive-unleashed +with line numbers added + +Non-X86 devicetree files tend to be very simple like this. + +```text + 1 # SPDX-License-Identifier: GPL-2.0-only + 2 chip soc/sifive/fu540 + 3 device cpu_cluster 0 on end + 4 end +``` + +This can be broken down as follows: + +Line 1: Comments start with the '#' character. This line is what’s +known as the SPDX header, which identifies how this particular file is +licensed. + +Line 2: "chip" starts a section for a collection of devices. It is +followed by a directory which contains the code to an optional chip.h +file. + +Line 3: "device" starts a device block, but needs to be followed by a +device type. "cpu_cluster" is the type, and this needs to be followed +by an identifier - "0". + +The device can be marked enabled "on", or disabled "off", then the device +block is closed with the "end" keyword. + +Line 4: Finally "end" closes the block started by the "chip" keyword. + + +### Generated files + +Continuing with the simple sifive/hifive-unleashed mainboard’s +devicetree, these are the files generated from the devicetree as of +2022-05-01. Because the devicetree is so simple, there’s almost nothing +in the header files + + +#### build/static.h + +```C +#ifndef __STATIC_DEVICE_TREE_H +#define __STATIC_DEVICE_TREE_H + +#include <static_fw_config.h> +#include <static_devices.h> + +#endif /* __STATIC_DEVICE_TREE_H */ +``` + + + +#### build/static_devices.h + +```C +#ifndef __STATIC_DEVICES_H +#define __STATIC_DEVICES_H +#include <device/device.h> +/* expose_device_names */ +#endif /* __STATIC_DEVICE_NAMES_H */ +``` + +#### build/static_fw_config.h + +Because this file doesn’t use any firmware config definitions in the +devicetree, there are also none here. + +```C +#ifndef __STATIC_FW_CONFIG_H +#define __STATIC_FW_CONFIG_H +#endif /* __STATIC_FW_CONFIG_H */ +``` + +#### build/mainboard/sifive/hifive-unleashed/static.c + + +##### Includes + +```text +1 #include <boot/coreboot_tables.h> +2 #include <device/device.h> +3 #include <device/pci.h> +4 #include <fw_config.h> +5 #include <static.h> +``` + +Lines 1 - 5: Includes of header files required for the following +structures. + + +##### Declarations for chip-ops + +```text +6 +7 #if !DEVTREE_EARLY +8 __attribute__((weak)) struct chip_operations mainboard_ops = {}; +9 extern struct chip_operations soc_sifive_fu540_ops; +10 #endif +``` + +Lines 7 & 10: The ops structures inside this IF block are only used in +ramstage. + +Lines 8 - 9: Declarations for chip ops structures. This section will +expand as more chips are added to the device tree. + +* Line 8, for mainboard_ops, is always present, and doesn’t change between +boards. It’s defined as a weak function because the mainboard may or may +not define a chip_operations structure. + +* Line 9 is created by the chip declaration in the devicetree file. There +will be a similar line for every chip declared in the devicetree. + + +##### STORAGE definition + +```text +11 +12 #define STORAGE static __unused DEVTREE_CONST +``` + +Line 12: This macro resolves to ‘static __unused const’ in early stages +and ‘static __unused’ in ramstage. + + +##### Structure definitions + +```text +13 +14 +15 /* pass 0 */ +16 STORAGE struct bus dev_root_links[]; +17 STORAGE struct device _dev_0; +18 DEVTREE_CONST struct device * DEVTREE_CONST last_dev = &_dev_0; +``` + +Lines 16 - 18 declare all of the structures used in static.c + + +##### Register Structures + +```text +19 +20 /* chip configs */ +``` + +Line 20 is the start of an empty section for this mainboard, but it +would contain any chip register structures defined in any of the chip.h +files. + + +##### Dev_root structure + +Line 21 - Line 44: dev_root: This is the mainboard structure, and the +head of the linked list to all devices. This structure is created for +every mainboard regardless of anything else in the devicetree. Though +the mainboard is frequently listed in the devicetree, it does not need +to be specified to generate this structure. + +```text +21 +22 /* pass 1 */ +23 DEVTREE_CONST struct device dev_root = { +24 #if !DEVTREE_EARLY +25 .ops = &default_dev_ops_root, +26 #endif +27 .bus = &dev_root_links[0], +28 .path = { .type = DEVICE_PATH_ROOT }, +29 .enabled = 1, +30 .hidden = 0, +31 .mandatory = 0, +32 .on_mainboard = 1, +33 .link_list = &dev_root_links[0], +34 .sibling = NULL, +35 #if !DEVTREE_EARLY +36 .chip_ops = &mainboard_ops, +37 .name = mainboard_name, +38 #endif +39 .next=&_dev_0, +40 #if !DEVTREE_EARLY +41 #if CONFIG(GENERATE_SMBIOS_TABLES) +42 #endif +43 #endif +44 }; +``` + +Lines 24 - 26: This points to a default ramstage device_operation +structure (described later) in the file src/device/root_device.c which +does nothing by default. If you wish to use this device structure for +something (like generating mainboard specific acpi tables), it can be +updated early in ramstage in the mainboard’s +chip_operations::enable_dev() function, also described later. + +Line 27: The pointer to the root link of this bus. Because this device +is the root link of this bus, it’s pointing at its own bus structure. + +Lines 28 - 32: Enumeration Status. + +* The hidden and mandatory are only ever set by those keywords in the + devicetree. +* The enabled status is initially set by the devicetree, but can be + modified in ramstage if the device is not detected. +* The on_mainboard status flag is set for anything that is specified in + devicetree. If the device were found during enumeration, this flag + would not be set. + +Line 33: The pointer to the next link on this bus in the devicetree +list. In this case, the next link and the root link are the same. This +is described more in the next section. + +Line 34: Sibling devices or chips are those that are defined at the same +level in the devicetree file. Examples would be northbridge and +southbridge chips, or pci devices and their functions within a chip. +Nothing should be at the same level as the mainboard chip, so this +should always be NULL. + +Line 36: The pointer to the mainboard’s chip_operations structure. The +mainboard is special in that although it’s not actually a chip, it still +gets a chip_operations structure. This allows the mainboard to perform +any required actions at the same points in the boot flow that a chip +would be able to do. + +Line 37: The mainboard name. This is a global variable set at build +time, also in the file src/device/root_device.c. + + +##### Dev_root_links + +lines 45 - 52: The dev_root bus structure + +This is the structure that is linked to on lines 27 & 33 above. It +provides pointers to the current device, and to the next device in the +linked list. + +A new bus structure is created for each different device type in the +devicetree. The same bus struct can be shared for the same device +types, even in different chips so long as the bus is contained within +the same cpu_cluster or domain. + +Typically, single socket x86 boards will have four of these structures: + +* Mainboard - always dev_root_links +* CPU_Cluster - typically _dev_0_links +* PCI Bus, Domain 0 - typically _dev_1_links +* ISA/LPC/eSPI bus - _dev_X_links, where X is the bridge device on PCI + +```text +45 STORAGE struct bus dev_root_links[] = { +46 [0] = { +47 .link_num = 0, +48 .dev = &dev_root, +49 .children = &_dev_0, +50 .next = NULL, +51 }, +52 }; +``` + +Line 47: .link: The index of this link. + +Line 48: .dev: Pointer to the current bridge device structure. + +Line 49: .children: The top device structure on the bus behind this +bridge device. + +Line 50: .next: The next bridge device on the current bus. + + +##### _dev_0 + +Line 53 - Line 72: The cpu_cluster device + +```text +53 STORAGE struct device _dev_0 = { +54 #if !DEVTREE_EARLY +55 .ops = NULL, +56 #endif +57 .bus = &dev_root_links[0], +58 .path = {.type=DEVICE_PATH_CPU_CLUSTER,{.cpu_cluster={ .cluster = 0x0 }}}, +59 .enabled = 1, +60 .hidden = 0, +61 .mandatory = 0, +62 .on_mainboard = 1, +63 .link_list = NULL, +64 .sibling = NULL, +65 #if !DEVTREE_EARLY +66 .chip_ops = &soc_sifive_fu540_ops, +67 #endif +68 #if !DEVTREE_EARLY +69 #if CONFIG(GENERATE_SMBIOS_TABLES) +70 #endif +71 #endif +72 }; +``` + +Lines 54-56: Pointer to the device_operations structure. Since this is +a chip, not a device, this pointer is NULL. + +Line 57: Pointer to the root link of this bus. Because this device is +directly under the mainboard, this points to the mainboard’s bus link. + +Line 58: The device_path structure, defined in src/include/device/path.h +is a path-type-specific structure detailing the identity of the device. +These should be unique in the devicetree. This is the field that’s +referenced when searching the devicetree for a device. + +Lines 59 - 62: Enumeration Status. Same as in dev_root + +Lines 63 & 64: Link list pointers to other devices in the tree. These +are NULL in this case because the only device in the devicetree that +generated this structure is the SoC chip. + +Lines 65 - 67: Pointer to the processor chip_ops structure. This is +only used in ramstage. + +Lines 68 - 71: Empty here, but this is where anything defined by +smbios_dev_info or smbios_slot_desc would go. -- To view, visit https://review.coreboot.org/c/coreboot/+/63977 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a43a96911844bd2b682004d5423126ad00a4bf3 Gerrit-Change-Number: 63977 Gerrit-PatchSet: 1 Gerrit-Owner: Martin L Roth <martinroth(a)google.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: util/crossgcc: Update to GCC 11.3
by Elyes Haouas (Code Review) 02 May '22

02 May '22

Attention is currently required from: Martin L Roth. Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63807 ) Change subject: util/crossgcc: Update to GCC 11.3 ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: util/crossgcc/patches are missing -- To view, visit https://review.coreboot.org/c/coreboot/+/63807 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1c0aa0952f85afee59a955593c63fcfac756cc60 Gerrit-Change-Number: 63807 Gerrit-PatchSet: 1 Gerrit-Owner: Martin L Roth <martinroth(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin L Roth <martinroth(a)google.com> Gerrit-Comment-Date: Mon, 02 May 2022 01:53:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/crossgcc: Update to GCC 11.3
by Felix Singer (Code Review) 02 May '22

02 May '22

Attention is currently required from: Martin L Roth. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63807 ) Change subject: util/crossgcc: Update to GCC 11.3 ...................................................................... Patch Set 1: Code-Review+1 (2 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/63807/comment/b70e5502_1c711480 PS1, Line 11: important Fix line length https://review.coreboot.org/c/coreboot/+/63807/comment/3d3b1fbb_8738ee5a PS1, Line 17: Builds with CB:63976 applied. -- To view, visit https://review.coreboot.org/c/coreboot/+/63807 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1c0aa0952f85afee59a955593c63fcfac756cc60 Gerrit-Change-Number: 63807 Gerrit-PatchSet: 1 Gerrit-Owner: Martin L Roth <martinroth(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin L Roth <martinroth(a)google.com> Gerrit-Comment-Date: Sun, 01 May 2022 22:34:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd/common/block/psp: Add platform secure boot support
by Angel Pons (Code Review) 01 May '22

01 May '22

Attention is currently required from: Jason Glenesk, Raul Rangel, ritul guru, Marshall Dawson, Paul Menzel, Aamir Bohra, Fred Reitberger, Felix Held. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60968 ) Change subject: soc/amd/common/block/psp: Add platform secure boot support ...................................................................... Patch Set 17: (1 comment) File src/soc/amd/common/block/psp/psb.c: https://review.coreboot.org/c/coreboot/+/60968/comment/4c259f59_9136bbf8 PS14, Line 12: #define PSB_HSTI_STATUS_OFFSET 0x10998 > Ack Would it make sense to rename those elements (in a separate commit) to have a more descriptive name? `CORE_2_PSP_MSG_38` doesn't seem too informative. -- To view, visit https://review.coreboot.org/c/coreboot/+/60968 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I30aac29a22a5800d5995a78c50fdecd660a3d4eb Gerrit-Change-Number: 60968 Gerrit-PatchSet: 17 Gerrit-Owner: ritul guru <ritul.bits(a)gmail.com> Gerrit-Reviewer: Aamir Bohra <aamirbohra(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: ritul guru <ritul.bits(a)gmail.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Aamir Bohra <aamirbohra(a)gmail.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Sun, 01 May 2022 21:14:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: ritul guru <ritul.bits(a)gmail.com> Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/skylake: Move FSP_HYPERTHREADING to common Intel Kconfig
by Felix Singer (Code Review) 01 May '22

01 May '22

Attention is currently required from: Nico Huber, Tim Wawrzynczak, Michael Niewöhner. Hello build bot (Jenkins), Nico Huber, Tim Wawrzynczak, Michael Niewöhner, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/60543 to look at the new patch set (#6). Change subject: soc/intel/skylake: Move FSP_HYPERTHREADING to common Intel Kconfig ...................................................................... soc/intel/skylake: Move FSP_HYPERTHREADING to common Intel Kconfig Move the Kconfig option `FSP_HYPERTHREADING` to common Intel Kconfig so that it can be reused by other SoCs. Since not all SoCs support hyperthreading, make it conditional on `HAVE_HYPERTHREADING`. SoCs supporting hyperthreading need to select it so that `FSP_HYPERTHREADING` is available. Also, select `HAVE_HYPERTHREADING` on the following Intel platforms: * Alderlake * Cannon Lake * Ice Lake * Skylake * Tiger Lake * Xeon SP Change-Id: I892d48b488cbf828057f0e9be9edc4352c58bbe7 Signed-off-by: Felix Singer <felixsinger(a)posteo.net> --- M src/soc/intel/alderlake/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/common/block/cpu/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig M src/soc/intel/tigerlake/Kconfig M src/soc/intel/xeon_sp/Kconfig 7 files changed, 18 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/60543/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/60543 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I892d48b488cbf828057f0e9be9edc4352c58bbe7 Gerrit-Change-Number: 60543 Gerrit-PatchSet: 6 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: ec/clevo/it5570: Add Clevo EC implementation found on Clevo laptops
by Angel Pons (Code Review) 01 May '22

01 May '22

Attention is currently required from: Michał Kopeć, Michael Niewöhner. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62495 ) Change subject: ec/clevo/it5570: Add Clevo EC implementation found on Clevo laptops ...................................................................... Patch Set 4: Code-Review+1 (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/62495/comment/5295c5f9_bd6f6489 PS4, Line 17: enabels typo: enables -- To view, visit https://review.coreboot.org/c/coreboot/+/62495 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I07c7d1d4f86d764bd10cfa3b0da23d1116281f3b Gerrit-Change-Number: 62495 Gerrit-PatchSet: 4 Gerrit-Owner: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Comment-Date: Sun, 01 May 2022 20:48:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: intelblocks/pep: Add display on/off notifications and handle TBT disp...
by Angel Pons (Code Review) 01 May '22

01 May '22

Attention is currently required from: Tim Wawrzynczak, Michał Kopeć, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62494 ) Change subject: intelblocks/pep: Add display on/off notifications and handle TBT displays ...................................................................... Patch Set 4: (2 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/62494/comment/51133bed_ea56b36c PS3, Line 7: intelblocks/pep: Add display on/off notifications and handle TBT displays > nit: 72 characters wide for commit msgs Actually, it doesn't fit because this commit does too much. Whenever `and` appears in the commit summary, it's most likely possible to split into multiple commits. https://review.coreboot.org/c/coreboot/+/62494/comment/abb7c514_e6e993f9 PS3, Line 8: : Add display on and off notifications which call mainboard hooks if : present. This allows to handle some board specific functions in user : absence or presence (whe ndisplay goes off from inactivity or on from : activity). : : Additionally notify IOM to enable or disable TBT displays on S0ix : exit and entry respectively. > These are kind of unrelated changes, mind splitting this into two patches? +1 -- To view, visit https://review.coreboot.org/c/coreboot/+/62494 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie80f631ecffa74467ab6d6162e552ba977f7e3f4 Gerrit-Change-Number: 62494 Gerrit-PatchSet: 4 Gerrit-Owner: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Sun, 01 May 2022 20:47:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/clevo/tgl-u: Add Clevo NV41 Tiger Lake laptop support
by Angel Pons (Code Review) 01 May '22

01 May '22

Attention is currently required from: Felix Singer, Michał Żygowski, Michał Kopeć, Michael Niewöhner. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62498 ) Change subject: mb/clevo/tgl-u: Add Clevo NV41 Tiger Lake laptop support ...................................................................... Patch Set 3: (38 comments) File src/mainboard/clevo/tgl-u/Kconfig: https://review.coreboot.org/c/coreboot/+/62498/comment/027038c3_3a170c7b PS3, Line 13: MAINBOARD_HAS_LPC_TPM MEMORY_MAPPED_TPM https://review.coreboot.org/c/coreboot/+/62498/comment/8895ab1c_7486d33e PS3, Line 14: select NO_UART_ON_SUPERIO Shouldn't be needed, selecting `INTEL_LPSS_UART_FOR_CONSOLE` should already provide the necessary implications. https://review.coreboot.org/c/coreboot/+/62498/comment/f3880664_238a95ff PS3, Line 20: select HAVE_SMI_HANDLER SoC already selects this https://review.coreboot.org/c/coreboot/+/62498/comment/49137e6e_8f55dd3c PS3, Line 22: select TPM_MEASURED_BOOT Why is it necessary to force people to use measured boot? This option is user-configurable, selecting it prevents user configuration. https://review.coreboot.org/c/coreboot/+/62498/comment/450defdb_0fa82471 PS3, Line 25: bool "NV4x" Why does this have a prompt? You can simply omit the line, the type only needs to be specified once (see my other comments). https://review.coreboot.org/c/coreboot/+/62498/comment/ddb3e919_622880ee PS3, Line 32: string Type not needed https://review.coreboot.org/c/coreboot/+/62498/comment/8730522f_a7c23733 PS3, Line 36: string Type not needed https://review.coreboot.org/c/coreboot/+/62498/comment/b3de50f7_bfccae5d PS3, Line 40: string Type not needed https://review.coreboot.org/c/coreboot/+/62498/comment/234343e0_0f8d744e PS3, Line 43: config MAX_CPUS : int : default 8 Is this actually needed? https://review.coreboot.org/c/coreboot/+/62498/comment/41b1cb98_a19573f6 PS3, Line 47: config PCIEXP_HOTPLUG : default y Redundant, given the select statement at the start of the file. https://review.coreboot.org/c/coreboot/+/62498/comment/ea91a212_626f90cf PS3, Line 50: config PCIEXP_HOTPLUG_BUSES : int : default 42 : : config PCIEXP_HOTPLUG_MEM : hex : default 0xc200000 # 194 MiB : : config PCIEXP_HOTPLUG_PREFETCH_MEM : hex : default 0x1c00000 # 448 MiB Where do these numbers come from? https://review.coreboot.org/c/coreboot/+/62498/comment/efbded88_a5933753 PS3, Line 63: string Type not needed https://review.coreboot.org/c/coreboot/+/62498/comment/87b99ce5_104ed262 PS3, Line 67: string Type not needed https://review.coreboot.org/c/coreboot/+/62498/comment/44731b42_573a3d7b PS3, Line 70: config DIMM_MAX : int : default 4 # Hack to make soc code work Huh? https://review.coreboot.org/c/coreboot/+/62498/comment/8d831baa_8c6a61b5 PS3, Line 75: int Type probably not needed https://review.coreboot.org/c/coreboot/+/62498/comment/7774625d_ddf4a74e PS3, Line 90: string Type not needed File src/mainboard/clevo/tgl-u/acpi/mainboard.asl: https://review.coreboot.org/c/coreboot/+/62498/comment/77f2ca49_28b9161c PS3, Line 8: // echo '\_SB.SLPB._PRW' | sudo tee /proc/acpi/call; sudo cat /proc/acpi/call : // echo '\_SB.PC00.LPCB.EC._GPE' | sudo tee /proc/acpi/call; sudo cat /proc/acpi/call What is this for? File src/mainboard/clevo/tgl-u/acpi/sleep.asl: https://review.coreboot.org/c/coreboot/+/62498/comment/ed1bdc28_fa2687ec PS3, Line 20: PGPM (MISCCFG_GPIO_PM_CONFIG_BITS) Why doesn't SoC code handle this? File src/mainboard/clevo/tgl-u/bootblock.c: https://review.coreboot.org/c/coreboot/+/62498/comment/9f38896f_3b36bc76 PS1, Line 16: if (onoff) { : gpio_set(DGPU_RST_N, 0); : mdelay(4); : gpio_set(DGPU_PWR_EN, 1); : mdelay(4); : gpio_set(DGPU_RST_N, 1); : } else { : gpio_set(DGPU_RST_N, 0); : mdelay(4); : gpio_set(DGPU_PWR_EN, 0); > Aren't the power sequence GPIOs mainboard-specific? The sequence may also depend on other stuff. […] Also, no polling for PWROK/PG to assert before releasing dGPU reset? File src/mainboard/clevo/tgl-u/bootblock.c: https://review.coreboot.org/c/coreboot/+/62498/comment/2da07e5f_15b8a618 PS3, Line 14: int Why not `bool`? https://review.coreboot.org/c/coreboot/+/62498/comment/c92eb27d_a14e9d07 PS3, Line 14: static void dgpu_power_enable(int onoff) { > open brace '{' following function definitions go on the next line Please fix. https://review.coreboot.org/c/coreboot/+/62498/comment/fa065b62_3d259c40 PS3, Line 15: %d I'd use `%s` and print `onoff ? "on" : "off"` File src/mainboard/clevo/tgl-u/cmos.layout: https://review.coreboot.org/c/coreboot/+/62498/comment/4b42e582_599dca13 PS3, Line 11: #395 4 e 3 debug_level Why is this commented out? https://review.coreboot.org/c/coreboot/+/62498/comment/84e32ba1_36277f49 PS3, Line 12: 408 1 h 1 preserve_smmstore What does this do? File src/mainboard/clevo/tgl-u/dsdt.asl: https://review.coreboot.org/c/coreboot/+/62498/comment/cc871af9_526d2631 PS3, Line 4: #include <baseboard/gpio.h> Is it possible to include this from the file that needs it? https://review.coreboot.org/c/coreboot/+/62498/comment/6508aeca_0c6a98cb PS3, Line 12: /* OEM revision */ Is it, though? https://review.coreboot.org/c/coreboot/+/62498/comment/ad9db19d_97084e2e PS3, Line 36: // Mainboard specific Redundant comment, please kill with fire File src/mainboard/clevo/tgl-u/romstage.c: https://review.coreboot.org/c/coreboot/+/62498/comment/71683ab9_dccfd703 PS3, Line 33: const uint8_t vtd = get_uint_option("vtd", 1); : memupd->FspmConfig.VtdDisable = !vtd; : : const uint8_t ht = get_uint_option("hyper_threading", : memupd->FspmConfig.HyperThreading); : memupd->FspmConfig.HyperThreading = ht; I don't see these in cmos.layout File src/mainboard/clevo/tgl-u/variants/baseboard/devicetree.cb: PS1: > We don't use a baseboard directory in the clevo mainboards. […] +1, the concept of `variants/baseboard` gives me aneurysms. File src/mainboard/clevo/tgl-u/variants/baseboard/devicetree.cb: https://review.coreboot.org/c/coreboot/+/62498/comment/0671bdb6_ad22956f PS3, Line 3: .chipset_lockdown = CHIPSET_LOCKDOWN_COREBOOT, Should be the default already? https://review.coreboot.org/c/coreboot/+/62498/comment/7ee5e115_4bc3fe91 PS3, Line 13: # Disable DPTF : register "dptf_enable" = "0" Just omit it then? https://review.coreboot.org/c/coreboot/+/62498/comment/b2b5ba0e_0ec3dffc PS3, Line 22: required Really? https://review.coreboot.org/c/coreboot/+/62498/comment/f1749506_4f52313e PS3, Line 26: # Enable C6 DRAM Please don't restate the obvious https://review.coreboot.org/c/coreboot/+/62498/comment/8507d58d_08ee5607 PS3, Line 33: # Acoustic settings This is not useful, please drop. File src/mainboard/clevo/tgl-u/variants/nv4x/gpio_early.c: https://review.coreboot.org/c/coreboot/+/62498/comment/9f71917f_889c5239 PS3, Line 11: PAD_CFG_TERM_GPO(GPP_U4, 0, NONE, DEEP), /* DGPU_RST#_PCH */ : PAD_CFG_TERM_GPO(GPP_U5, 0, NONE, DEEP), /* DGPU_PWR_EN */ Why not use `PAD_CFG_GPO` instead? Also, I guess unconditionally configuring these pads is not a problem because HAVE_ACPI_RESUME is not selected. File src/mainboard/clevo/tgl-u/variants/nv4x/overridetree.cb: PS3: Are any PCIe RPs wired to a slot? https://review.coreboot.org/c/coreboot/+/62498/comment/5d515dbf_4478ee0e PS3, Line 3: # Power limits : # TODO: Check if this is correct : register "power_limits_config[POWER_LIMITS_U_4_CORE]" = "{ : .tdp_pl1_override = 10, : .tdp_pl2_override = 20, : }" : register "power_limits_config[POWER_LIMITS_U_2_CORE]" = "{ : .tdp_pl1_override = 10, : .tdp_pl2_override = 20, : }" Why not just drop it? https://review.coreboot.org/c/coreboot/+/62498/comment/2c3a3abf_3f68537a PS3, Line 163: # Pantone ROM W-what is this for? -- To view, visit https://review.coreboot.org/c/coreboot/+/62498 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib373d62d9d18bafdfde2e1acb4e00e3a20ae09bc Gerrit-Change-Number: 62498 Gerrit-PatchSet: 3 Gerrit-Owner: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Comment-Date: Sun, 01 May 2022 20:45:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit May 2022