coreboot-gerrit October 2020

coreboot-gerrit@coreboot.org

1 participants
3474 discussions

Change in coreboot[master]: mb/clevo/l140cu: add CMOS layout and defaults
by Michael Niewöhner (Code Review) 12 Oct '20

12 Oct '20

Hello Felix Singer, Paul Menzel, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/46311 to look at the new patch set (#2). Change subject: mb/clevo/l140cu: add CMOS layout and defaults ...................................................................... mb/clevo/l140cu: add CMOS layout and defaults Add CMOS layout and defaults files and enable CMOS options in Kconfig. Change-Id: Ia1a27818b2d12fb7578189e5748b8073c8f928e3 Signed-off-by: Michael Niewöhner <foss(a)mniewoehner.de> --- M src/mainboard/clevo/cml-u/Kconfig A src/mainboard/clevo/cml-u/variants/l140cu/cmos.default A src/mainboard/clevo/cml-u/variants/l140cu/cmos.layout 3 files changed, 66 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/46311/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/46311 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia1a27818b2d12fb7578189e5748b8073c8f928e3 Gerrit-Change-Number: 46311 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/cannonlake: Align cosmetics with Ice Lake
by Angel Pons (Code Review) 12 Oct '20

12 Oct '20

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45778 ) Change subject: soc/intel/cannonlake: Align cosmetics with Ice Lake ...................................................................... soc/intel/cannonlake: Align cosmetics with Ice Lake Tested with BUILD_TIMELESS=1, Prodrive Hermes remains identical. Change-Id: I4d9f882f9f8af1245e937b0d47bc7e993547365f Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/soc/intel/cannonlake/acpi/pch_hda.asl M src/soc/intel/cannonlake/cpu.c M src/soc/intel/cannonlake/me.c M src/soc/intel/cannonlake/sd.c 4 files changed, 46 insertions(+), 50 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/45778/1 diff --git a/src/soc/intel/cannonlake/acpi/pch_hda.asl b/src/soc/intel/cannonlake/acpi/pch_hda.asl index 04e974f..f292901 100644 --- a/src/soc/intel/cannonlake/acpi/pch_hda.asl +++ b/src/soc/intel/cannonlake/acpi/pch_hda.asl @@ -4,7 +4,7 @@ Device (HDAS) { - Name (_ADR, 0x001F0003) + Name (_ADR, 0x001f0003) Name (_DDN, "Audio Controller") Name (UUID, ToUUID ("A69F886E-6CEB-4594-A41F-7B5DCE24C553")) diff --git a/src/soc/intel/cannonlake/cpu.c b/src/soc/intel/cannonlake/cpu.c index 36d252e..0c24535 100644 --- a/src/soc/intel/cannonlake/cpu.c +++ b/src/soc/intel/cannonlake/cpu.c @@ -54,9 +54,10 @@ static void configure_misc(void) { - config_t *conf = config_of_soc(); msr_t msr; + config_t *conf = config_of_soc(); + msr = rdmsr(IA32_MISC_ENABLE); msr.lo |= (1 << 0); /* Fast String enable */ msr.lo |= (1 << 3); /* TM1/TM2/EMTTM enable */ @@ -105,6 +106,33 @@ } } +/* + * The emulated ACPI timer allows replacing of the ACPI timer + * (PM1_TMR) to have no impart on the system. + */ +static void enable_pm_timer_emulation(void) +{ + const struct soc_intel_cannonlake_config *config; + msr_t msr; + + config = config_of_soc(); + + /* Enable PM timer emulation only if ACPI PM timer is disabled */ + if (!config->PmTimerDisabled) + return; + /* + * The derived frequency is calculated as follows: + * (CTC_FREQ * msr[63:32]) >> 32 = target frequency. + * Back solve the multiplier so the 3.579545MHz ACPI timer + * frequency is used. + */ + msr.hi = (3579545ULL << 32) / CTC_FREQ; + /* Set PM1 timer IO port and enable */ + msr.lo = (EMULATE_DELAY_VALUE << EMULATE_DELAY_OFFSET_VALUE) | + EMULATE_PM_TMR_EN | (ACPI_BASE_ADDRESS + PM1_TMR); + wrmsr(MSR_EMULATE_PM_TIMER, msr); +} + static void set_energy_perf_bias(u8 policy) { msr_t msr; @@ -155,33 +183,6 @@ wrmsr(MSR_C_STATE_LATENCY_CONTROL_5, msr); } -/* - * The emulated ACPI timer allows replacing of the ACPI timer - * (PM1_TMR) to have no impart on the system. - */ -static void enable_pm_timer_emulation(void) -{ - const struct soc_intel_cannonlake_config *config; - msr_t msr; - - config = config_of_soc(); - - /* Enable PM timer emulation only if ACPI PM timer is disabled */ - if (!config->PmTimerDisabled) - return; - /* - * The derived frequency is calculated as follows: - * (CTC_FREQ * msr[63:32]) >> 32 = target frequency. - * Back solve the multiplier so the 3.579545MHz ACPI timer - * frequency is used. - */ - msr.hi = (3579545ULL << 32) / CTC_FREQ; - /* Set PM1 timer IO port and enable */ - msr.lo = (EMULATE_DELAY_VALUE << EMULATE_DELAY_OFFSET_VALUE) | - EMULATE_PM_TMR_EN | (ACPI_BASE_ADDRESS + PM1_TMR); - wrmsr(MSR_EMULATE_PM_TIMER, msr); -} - /* All CPUs including BSP will run the following function. */ void soc_core_init(struct device *cpu) { diff --git a/src/soc/intel/cannonlake/me.c b/src/soc/intel/cannonlake/me.c index a51b702..7bbe1ae 100644 --- a/src/soc/intel/cannonlake/me.c +++ b/src/soc/intel/cannonlake/me.c @@ -12,7 +12,7 @@ /* Host Firmware Status Register 2 */ union me_hfsts2 { - uint32_t raw; + uint32_t data; struct { uint32_t nftp_load_failure : 1; uint32_t icc_prog_status : 2; @@ -36,7 +36,7 @@ /* Host Firmware Status Register 4 */ union me_hfsts4 { - uint32_t raw; + uint32_t data; struct { uint32_t rsvd0 : 9; uint32_t enforcement_flow : 1; @@ -52,7 +52,7 @@ /* Host Firmware Status Register 5 */ union me_hfsts5 { - uint32_t raw; + uint32_t data; struct { uint32_t acm_active : 1; uint32_t valid : 1; @@ -71,7 +71,7 @@ /* Host Firmware Status Register 6 */ union me_hfsts6 { - uint32_t raw; + uint32_t data; struct { uint32_t force_boot_guard_acm : 1; uint32_t cpu_debug_disable : 1; @@ -107,24 +107,18 @@ return; hfsts1.data = me_read_config32(PCI_ME_HFSTS1); - hfsts2.raw = me_read_config32(PCI_ME_HFSTS2); + hfsts2.data = me_read_config32(PCI_ME_HFSTS2); hfsts3.data = me_read_config32(PCI_ME_HFSTS3); - hfsts4.raw = me_read_config32(PCI_ME_HFSTS4); - hfsts5.raw = me_read_config32(PCI_ME_HFSTS5); - hfsts6.raw = me_read_config32(PCI_ME_HFSTS6); + hfsts4.data = me_read_config32(PCI_ME_HFSTS4); + hfsts5.data = me_read_config32(PCI_ME_HFSTS5); + hfsts6.data = me_read_config32(PCI_ME_HFSTS6); - printk(BIOS_DEBUG, "ME: HFSTS1 : 0x%08X\n", - hfsts1.data); - printk(BIOS_DEBUG, "ME: HFSTS2 : 0x%08X\n", - hfsts2.raw); - printk(BIOS_DEBUG, "ME: HFSTS3 : 0x%08X\n", - hfsts3.data); - printk(BIOS_DEBUG, "ME: HFSTS4 : 0x%08X\n", - hfsts4.raw); - printk(BIOS_DEBUG, "ME: HFSTS5 : 0x%08X\n", - hfsts5.raw); - printk(BIOS_DEBUG, "ME: HFSTS6 : 0x%08X\n", - hfsts6.raw); + printk(BIOS_DEBUG, "ME: HFSTS1 : 0x%08X\n", hfsts1.data); + printk(BIOS_DEBUG, "ME: HFSTS2 : 0x%08X\n", hfsts2.data); + printk(BIOS_DEBUG, "ME: HFSTS3 : 0x%08X\n", hfsts3.data); + printk(BIOS_DEBUG, "ME: HFSTS4 : 0x%08X\n", hfsts4.data); + printk(BIOS_DEBUG, "ME: HFSTS5 : 0x%08X\n", hfsts5.data); + printk(BIOS_DEBUG, "ME: HFSTS6 : 0x%08X\n", hfsts6.data); printk(BIOS_DEBUG, "ME: Manufacturing Mode : %s\n", hfsts1.fields.mfg_mode ? "YES" : "NO"); @@ -159,5 +153,6 @@ printk(BIOS_DEBUG, "ME: TXT Support : %s\n", hfsts6.fields.txt_support ? "YES" : "NO"); } + BOOT_STATE_INIT_ENTRY(BS_DEV_ENABLE, BS_ON_EXIT, print_me_fw_version, NULL); BOOT_STATE_INIT_ENTRY(BS_OS_RESUME_CHECK, BS_ON_EXIT, dump_me_status, NULL); diff --git a/src/soc/intel/cannonlake/sd.c b/src/soc/intel/cannonlake/sd.c index 7663b2f..46d3852 100644 --- a/src/soc/intel/cannonlake/sd.c +++ b/src/soc/intel/cannonlake/sd.c @@ -3,7 +3,7 @@ #include <intelblocks/sd.h> #include "chip.h" -int sd_fill_soc_gpio_info(struct acpi_gpio* gpio, const struct device *dev) +int sd_fill_soc_gpio_info(struct acpi_gpio *gpio, const struct device *dev) { config_t *config = config_of(dev); -- To view, visit https://review.coreboot.org/c/coreboot/+/45778 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4d9f882f9f8af1245e937b0d47bc7e993547365f Gerrit-Change-Number: 45778 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

3 6

Change in coreboot[master]: mb/google/poppy/variant/atlas: Reset bluetooth on boot
by Caveh Jalali (Code Review) 12 Oct '20

12 Oct '20

Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46120 ) Change subject: mb/google/poppy/variant/atlas: Reset bluetooth on boot ...................................................................... Patch Set 2: (7 comments) https://review.coreboot.org/c/coreboot/+/46120/1//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/46120/1//COMMIT_MSG@7 PS1, Line 7: mb/google/poppy/variant/atlas: Reset bluetooth in BIOS > Please avoid the term BIOS at least in summaries. ;-) […] Done https://review.coreboot.org/c/coreboot/+/46120/1//COMMIT_MSG@13 PS1, Line 13: down-rev > older firmware, as in the opposite of to "up-rev" firmware Done https://review.coreboot.org/c/coreboot/+/46120/1//COMMIT_MSG@14 PS1, Line 14: makes no attempt update > *to* update Done https://review.coreboot.org/c/coreboot/+/46120/1//COMMIT_MSG@12 PS1, Line 12: If the BT chip is : already running down-rev firmware from a previous boot, the driver : makes no attempt update the BT firmware by resetting it. This can lead : to unexpected behavior across system upgrades. > It's not Linux-specific, it's specific to the Bluetooth module. b:168757348 captures this behavior. https://review.coreboot.org/c/coreboot/+/46120/1//COMMIT_MSG@16 PS1, Line 16: BIOS > coreboot 😋 Done https://review.coreboot.org/c/coreboot/+/46120/1//COMMIT_MSG@17 PS1, Line 17: > Please describe the change. […] line 9 says it all. force reset. https://review.coreboot.org/c/coreboot/+/46120/1//COMMIT_MSG@20 PS1, Line 20: > For sure :) I just mean to tag this with BRANCH=poppy or something it would be BRANCH=atlas, but i think this is meaningless. this change is relevant on ToT and we will need to cherry-pick it to the atlas firmware branch and up-rev the firmware updater in the next shipping chrome os image to close the bug. -- To view, visit https://review.coreboot.org/c/coreboot/+/46120 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I361caa49e82aa086c3c35962164fb078fa30139e Gerrit-Change-Number: 46120 Gerrit-PatchSet: 2 Gerrit-Owner: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 12 Oct 2020 20:43:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)users.sourceforge.net> Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Comment-In-Reply-To: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/clevo/l140cu: add CMOS layout and defaults
by Michael Niewöhner (Code Review) 12 Oct '20

12 Oct '20

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46311 ) Change subject: mb/clevo/l140cu: add CMOS layout and defaults ...................................................................... Patch Set 1: This change is ready for review. -- To view, visit https://review.coreboot.org/c/coreboot/+/46311 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia1a27818b2d12fb7578189e5748b8073c8f928e3 Gerrit-Change-Number: 46311 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 12 Oct 2020 20:42:01 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/poppy/variant/atlas: Reset bluetooth on boot
by Caveh Jalali (Code Review) 12 Oct '20

12 Oct '20

Hello build bot (Jenkins), Tim Wawrzynczak, Nick Vaccaro, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/46120 to look at the new patch set (#2). Change subject: mb/google/poppy/variant/atlas: Reset bluetooth on boot ...................................................................... mb/google/poppy/variant/atlas: Reset bluetooth on boot This forces a reset of the integrated bluetooth device on AP reset and re-enables it before handing control to the kernel. We advertise a reset_gpio to the kernel over ACPI, but the kernel BT driver only uses this to reset BT when there are command timeouts. If the BT chip is already running firmware from a previous boot, the driver makes no attempt to update the BT firmware by resetting it, even when it does not match the current version. This can lead to unexpected behavior across system upgrades. So, we need to reset the BT chip in coreboot to make sure the BT driver loads current firmware. BUG=b:168757348 TEST=boot on atlas Signed-off-by: Caveh Jalali <caveh(a)chromium.org> Change-Id: I361caa49e82aa086c3c35962164fb078fa30139e --- M src/mainboard/google/poppy/variants/atlas/gpio.c 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/46120/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/46120 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I361caa49e82aa086c3c35962164fb078fa30139e Gerrit-Change-Number: 46120 Gerrit-PatchSet: 2 Gerrit-Owner: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: libpayload/x86: Try to discover invariant TSC rate
by Nico Huber (Code Review) 12 Oct '20

12 Oct '20

Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, Tim Wawrzynczak, Angel Pons, Julius Werner, Arthur Heymans, Kyösti Mälkki, Aaron Durbin, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/34170 to look at the new patch set (#4). Change subject: libpayload/x86: Try to discover invariant TSC rate ...................................................................... libpayload/x86: Try to discover invariant TSC rate We can skip the PIT-based TSC calibration if we can derive the invariant TSC rate from CPUID/MSR data. This is necessary if the PIT is disabled, which is the default, for instance, on Coffee Lake CPUs. This implementation should cover all Intel Core i processors at least. For older processors, we fall back to the PIT calibration. Change-Id: Ic6607ee2a8b41c2be9dc1bb4f1e23e652bb33889 Signed-off-by: Nico Huber <nico.huber(a)secunet.com> --- M payloads/libpayload/arch/x86/timer.c 1 file changed, 115 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/34170/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/34170 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic6607ee2a8b41c2be9dc1bb4f1e23e652bb33889 Gerrit-Change-Number: 34170 Gerrit-PatchSet: 4 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.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)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: drivers/i2c: Add chip driver for GPIO based I2C multiplexer
by Karthik Ramasubramanian (Code Review) 12 Oct '20

12 Oct '20

1 0

Change in coreboot[master]: drivers/i2c: Add chip driver for GPIO based I2C multiplexer
by Karthik Ramasubramanian (Code Review) 12 Oct '20

12 Oct '20

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45911 ) Change subject: drivers/i2c: Add chip driver for GPIO based I2C multiplexer ...................................................................... Patch Set 7: (14 comments) https://review.coreboot.org/c/coreboot/+/45911/4//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/45911/4//COMMIT_MSG@8 PS4, Line 8: > Can you mention the kernel driver this is intended to be used with? Added the kernel driver for both mux and bus chip drivers in the commit message. https://review.coreboot.org/c/coreboot/+/45911/6//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/45911/6//COMMIT_MSG@48 PS6, Line 48: Package (0x02) : { : "mux-gpios", : Package (0x04) : { : \_SB.PCI0.I2C3.MUX0, : Zero, : Zero, : Zero : } : } : } > Where is this used? Added the kernel driver with a link to its doc in the commit summary. https://review.coreboot.org/c/coreboot/+/45911/5/src/drivers/i2c/gpio_mux/c… File src/drivers/i2c/gpio_mux/chip.h: https://review.coreboot.org/c/coreboot/+/45911/5/src/drivers/i2c/gpio_mux/c… PS5, Line 16: char > const This has been done for the bus chip info here: https://review.coreboot.org/c/coreboot/+/46144/2/src/drivers/i2c/gpiomux/bu… The mux chip info does not need a name field now. https://review.coreboot.org/c/coreboot/+/45911/5/src/drivers/i2c/gpio_mux/c… PS5, Line 17: acpi_hid > I don't think we need to take in acpi_hid as a parameter from the mainboard. […] Done https://review.coreboot.org/c/coreboot/+/45911/5/src/drivers/i2c/gpio_mux/c… PS5, Line 19: /* GPIOs used to select the mux lines */ : uint32_t mux_gpio_count; : struct acpi_gpio mux_gpio[MAX_NUM_MUX_GPIOS]; > Should we put these in a structure just to make it clear that they are applicable only to i2c_mux? Now that the bus and mux chip drivers are split, I have not created a nested structure. https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/g… File src/drivers/i2c/gpio_mux/gpio_mux.c: https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/g… PS3, Line 34: i2c_gpio_mux_adapter_fill_ssdt > suggestion: i2c_gpio_mux_adapter_fill Since the bus and mux drivers are split, no need for these helper functions. https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/g… PS3, Line 37: acpigen_write_name_integer > acpigen_write_ADR ? Done here: https://review.coreboot.org/c/coreboot/+/46144/2/src/drivers/i2c/gpiomux/bu… https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/g… PS3, Line 40: i2c_gpio_mux_fill_ssdt > suggestion: i2c_gpio_mux_write_gpios The drivers are split and hence no need for this helper function. https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/g… PS3, Line 40: static void i2c_gpio_mux_fill_ssdt(struct drivers_i2c_gpio_mux_config *config) : { : /* Resources */ : acpigen_write_name("_CRS"); : acpigen_write_resourcetemplate_header(); : acpi_device_write_gpio(&config->mux_gpio); : acpigen_write_resourcetemplate_footer(); > All this function uses from `config` is the mux_gpio field, why not pass that in instead (const stru […] The drivers are split and hence no need for this helper function. https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/g… PS3, Line 50: i2c_gpio_mux_fill_ssdt_generator > note: we removed the 'generator' names a while ago. Done https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/g… PS3, Line 51: { : const char *scope = acpi_device_scope(dev); : const char *path = acpi_device_path(dev); : struct drivers_i2c_gpio_mux_config *config = dev->chip_info; : : if (!dev->enabled || !scope) : return; : > Could you restructure this so that dev is checked for NULL first? That will make coverity happy. acpi_device_scope returns NULL if dev is NULL acpi_device_path returns NULL if dev is NULL config_of(dev) is being used to get the chip information. All these things ensure NULL pointer access is not performed. Should I still restructure the code to satisfy coverity. https://review.coreboot.org/c/coreboot/+/45911/4/src/drivers/i2c/gpio_mux/g… File src/drivers/i2c/gpio_mux/gpio_mux.c: PS4: > Exactly what I was thinking Done https://review.coreboot.org/c/coreboot/+/45911/6/src/drivers/i2c/gpiomux/mu… File src/drivers/i2c/gpiomux/mux/mux.c: https://review.coreboot.org/c/coreboot/+/45911/6/src/drivers/i2c/gpiomux/mu… PS6, Line 55: > space instead of tab. Done https://review.coreboot.org/c/coreboot/+/45911/6/src/drivers/i2c/gpiomux/mu… PS6, Line 76: if (!config) : return; > Why is this check required? My bad. Forgot to remove the dead code. The current check for NULL dev is to satisfy coverity. -- To view, visit https://review.coreboot.org/c/coreboot/+/45911 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib371108cc6043c133681066bf7bf4b2e00771e8b Gerrit-Change-Number: 45911 Gerrit-PatchSet: 7 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Evan Green <evgreen(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Justin TerAvest <teravest(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 12 Oct 2020 20:22:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/i2c: Add chip driver for GPIO based I2C multiplexer
by Karthik Ramasubramanian (Code Review) 12 Oct '20

12 Oct '20

Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Evan Green, Justin TerAvest, Tim Wawrzynczak, Duncan Laurie, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/45911 to look at the new patch set (#7). Change subject: drivers/i2c: Add chip driver for GPIO based I2C multiplexer ...................................................................... drivers/i2c: Add chip driver for GPIO based I2C multiplexer Add identifiers in ACPI tables for GPIO based I2C multiplexer. The multiplexer device defines the GPIO resource used to select the adapter/bus lines. The multiplexer adapter device defines the address to select the adapter/client lines. These ACPI identifiers are consumed by the i2c-mux-gpio kernel driver: https://www.kernel.org/doc/html/latest/i2c/muxes/i2c-mux-gpio.html BUG=b:169444894 TEST=Build and boot waddledee to OS. Ensure that the ACPI identifiers are added for I2C devices multiplexed using I2C MUX under the appropriate scope. Here is the output SSDT: Scope (\_SB.PCI0.I2C3) { Device (MUX0) { Name (_HID, "PRP0001") // _HID: Hardware ID Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) } Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\\_SB.PCI0.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x0125 } }) Name (_DSD, Package (0x02) // _DSD: Device-Specific Data { ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */, Package (0x02) { Package (0x02) { "compatible", "i2c-mux-gpio" }, Package (0x02) { "mux-gpios", Package (0x04) { \_SB.PCI0.I2C3.MUX0, Zero, Zero, Zero } } } }) } } Change-Id: Ib371108cc6043c133681066bf7bf4b2e00771e8b Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> --- A src/drivers/i2c/gpiomux/Kconfig A src/drivers/i2c/gpiomux/Makefile.inc A src/drivers/i2c/gpiomux/mux/Makefile.inc A src/drivers/i2c/gpiomux/mux/chip.h A src/drivers/i2c/gpiomux/mux/mux.c 5 files changed, 106 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/45911/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/45911 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib371108cc6043c133681066bf7bf4b2e00771e8b Gerrit-Change-Number: 45911 Gerrit-PatchSet: 7 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Evan Green <evgreen(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Justin TerAvest <teravest(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/xeon_sp: Move function debug macros
by Marc Jones (Code Review) 12 Oct '20

12 Oct '20

Marc Jones has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46305 ) Change subject: soc/intel/xeon_sp: Move function debug macros ...................................................................... soc/intel/xeon_sp: Move function debug macros Move the macros for printing debug information to debug.h in the common console include directory and device include file. These are added if the platform selects HAVE_DEBUG_FUNC and DEFAULT_CONSOLE_LOGLEVEL_8. The macros could be used by any platform. Change-Id: Ie237bdf8cdc42c76f38a0c820fdc92e81095f47c Signed-off-by: Marc Jones <marcjones(a)sysproconsulting.com> fixup funcs Change-Id: I834af2d3d1aca2bbf4c17608578a15f6d1f1a84a Signed-off-by: Marc Jones <marcjones(a)sysproconsulting.com> --- M src/Kconfig A src/include/console/debug.h M src/include/device/device.h M src/soc/intel/xeon_sp/Kconfig M src/soc/intel/xeon_sp/cpx/chip.c M src/soc/intel/xeon_sp/cpx/cpu.c M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/soc/intel/xeon_sp/include/soc/util.h M src/soc/intel/xeon_sp/skx/chip.c M src/soc/intel/xeon_sp/skx/cpu.c 10 files changed, 57 insertions(+), 31 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/46305/1 diff --git a/src/Kconfig b/src/Kconfig index 352a1e1..8ed97f0 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -1117,6 +1117,21 @@ of calling function. Please note some printk related functions are omitted from trace to have good looking console dumps. +config HAVE_DEBUG_FUNC + def_bool n + +config DEBUG_FUNC + bool "Enable function entry and exit reporting macros" if DEFAULT_CONSOLE_LOGLEVEL_8 + default n + help + This option enables additional function entry and exit debug messages + for select functions. If supported, this is less output than + the TRACE option. + + Note: This option will increase the size of the coreboot image. + + If unsure, say N. + config DEBUG_COVERAGE bool "Debug code coverage" default n diff --git a/src/include/console/debug.h b/src/include/console/debug.h new file mode 100644 index 0000000..174c287 --- /dev/null +++ b/src/include/console/debug.h @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#ifndef _CONSOLE_DEBUG_H_ +#define _CONSOLE_DEBUG_H_ + +#if CONFIG(DEBUG_FUNC) +#include <console/console.h> + +#define FUNC_ENTER() \ + printk(BIOS_SPEW, "%s:%s:%d: ENTER\n", __FILE__, __func__, __LINE__) + +#define FUNC_EXIT() \ + printk(BIOS_SPEW, "%s:%s:%d: EXIT\n", __FILE__, __func__, __LINE__) + +#else /* FUNC_DEBUG */ + +#define FUNC_ENTER() +#define FUNC_EXIT() + +#endif /* FUNC_DEBUG */ + +#endif diff --git a/src/include/device/device.h b/src/include/device/device.h index 0d4205d..542a305 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -279,6 +279,20 @@ #define LOG_IO_RESOURCE(type, dev, index, base, size) #endif /* DEBUG_RESOURCES*/ +#if CONFIG(DEBUG_FUNC) +#include <console/console.h> +#define DEV_FUNC_ENTER(dev) \ + printk(BIOS_SPEW, "%s:%s:%d: ENTER (dev: %s)\n", \ + __FILE__, __func__, __LINE__, dev_path(dev)) + +#define DEV_FUNC_EXIT(dev) \ + printk(BIOS_SPEW, "%s:%s:%d: EXIT (dev: %s)\n", __FILE__, \ + __func__, __LINE__, dev_path(dev)) +#else /* DEBUG_FUNC */ +#define DEV_FUNC_ENTER(dev) +#define DEV_FUNC_EXIT(dev) +#endif /* DEBUG_FUNC */ + /* Rounding for boundaries. * Due to some chip bugs, go ahead and round IO to 16 */ diff --git a/src/soc/intel/xeon_sp/Kconfig b/src/soc/intel/xeon_sp/Kconfig index d3e137b..01d5d11 100644 --- a/src/soc/intel/xeon_sp/Kconfig +++ b/src/soc/intel/xeon_sp/Kconfig @@ -57,6 +57,7 @@ select CPU_INTEL_FIRMWARE_INTERFACE_TABLE select FSP_CAR select HAVE_DEBUG_RESOURCES + select HAVE_DEBUG_FUNC config MAINBOARD_USES_FSP2_0 bool diff --git a/src/soc/intel/xeon_sp/cpx/chip.c b/src/soc/intel/xeon_sp/cpx/chip.c index 11fe44b..f69797a 100644 --- a/src/soc/intel/xeon_sp/cpx/chip.c +++ b/src/soc/intel/xeon_sp/cpx/chip.c @@ -3,6 +3,7 @@ #include <arch/ioapic.h> #include <assert.h> #include <console/console.h> +#include <console/debug.h> #include <cpu/x86/lapic.h> #include <device/pci.h> #include <fsp/api.h> diff --git a/src/soc/intel/xeon_sp/cpx/cpu.c b/src/soc/intel/xeon_sp/cpx/cpu.c index 0999f6d..4afe47c 100644 --- a/src/soc/intel/xeon_sp/cpx/cpu.c +++ b/src/soc/intel/xeon_sp/cpx/cpu.c @@ -4,6 +4,7 @@ #include <acpi/acpi.h> #include <assert.h> #include <console/console.h> +#include <console/debug.h> #include <cpu/cpu.h> #include <cpu/intel/microcode.h> #include <cpu/intel/turbo.h> diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h index 412730b..3e19bac 100644 --- a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h +++ b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h @@ -3,24 +3,8 @@ #ifndef _SOC_UTIL_H_ #define _SOC_UTIL_H_ -#include <console/console.h> #include <hob_iiouds.h> #include <hob_memmap.h> -#include <stdint.h> - -#define DEV_FUNC_ENTER(dev) \ - printk(BIOS_SPEW, "%s:%s:%d: ENTER (dev: %s)\n", \ - __FILE__, __func__, __LINE__, dev_path(dev)) - -#define DEV_FUNC_EXIT(dev) \ - printk(BIOS_SPEW, "%s:%s:%d: EXIT (dev: %s)\n", __FILE__, \ - __func__, __LINE__, dev_path(dev)) - -#define FUNC_ENTER() \ - printk(BIOS_SPEW, "%s:%s:%d: ENTER\n", __FILE__, __func__, __LINE__) - -#define FUNC_EXIT() \ - printk(BIOS_SPEW, "%s:%s:%d: EXIT\n", __FILE__, __func__, __LINE__) struct iiostack_resource { uint8_t no_of_stacks; diff --git a/src/soc/intel/xeon_sp/include/soc/util.h b/src/soc/intel/xeon_sp/include/soc/util.h index f223efa..8c2b597 100644 --- a/src/soc/intel/xeon_sp/include/soc/util.h +++ b/src/soc/intel/xeon_sp/include/soc/util.h @@ -3,25 +3,10 @@ #ifndef _XEON_SP_SOC_UTIL_H_ #define _XEON_SP_SOC_UTIL_H_ -#include <console/console.h> #include <hob_iiouds.h> void get_cpubusnos(uint32_t *bus0, uint32_t *bus1, uint32_t *bus2, uint32_t *bus3); void unlock_pam_regions(void); void get_stack_busnos(uint32_t *bus); -#define DEV_FUNC_ENTER(dev) \ - printk(BIOS_SPEW, "%s:%s:%d: ENTER (dev: %s)\n", \ - __FILE__, __func__, __LINE__, dev_path(dev)) - -#define DEV_FUNC_EXIT(dev) \ - printk(BIOS_SPEW, "%s:%s:%d: EXIT (dev: %s)\n", __FILE__, \ - __func__, __LINE__, dev_path(dev)) - -#define FUNC_ENTER() \ - printk(BIOS_SPEW, "%s:%s:%d: ENTER\n", __FILE__, __func__, __LINE__) - -#define FUNC_EXIT() \ - printk(BIOS_SPEW, "%s:%s:%d: EXIT\n", __FILE__, __func__, __LINE__) - #endif diff --git a/src/soc/intel/xeon_sp/skx/chip.c b/src/soc/intel/xeon_sp/skx/chip.c index 845d7cb..faa455a 100644 --- a/src/soc/intel/xeon_sp/skx/chip.c +++ b/src/soc/intel/xeon_sp/skx/chip.c @@ -2,6 +2,8 @@ #include <cbfs.h> #include <assert.h> +#include <console/console.h> +#include <console/debug.h> #include <post.h> #include <device/pci.h> #include <soc/acpi.h> diff --git a/src/soc/intel/xeon_sp/skx/cpu.c b/src/soc/intel/xeon_sp/skx/cpu.c index ea9f531..73d7a73 100644 --- a/src/soc/intel/xeon_sp/skx/cpu.c +++ b/src/soc/intel/xeon_sp/skx/cpu.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */ #include <console/console.h> +#include <console/debug.h> #include <intelblocks/cpulib.h> #include <cpu/cpu.h> #include <cpu/x86/mtrr.h> -- To view, visit https://review.coreboot.org/c/coreboot/+/46305 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I834af2d3d1aca2bbf4c17608578a15f6d1f1a84a Gerrit-Change-Number: 46305 Gerrit-PatchSet: 1 Gerrit-Owner: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

1 1

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit October 2020