coreboot-gerrit February 2021

coreboot-gerrit@coreboot.org

1 participants
7192 discussions

Change in coreboot[master]: soc/intel/skylake/acpi: Add PEP table
by Benjamin Doron (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Michael Niewöhner. Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49140 ) Change subject: soc/intel/skylake/acpi: Add PEP table ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/49140/comment/a271556e_53f686fd PS3, Line 11: > > > > > > > > > Awesome, then at least S0ix states work just fine. > > > > > > > > > > > > > > > > > > That error message comes from the kernel SlpS0 self-test. So, let's test something. I assume `cat /sys/power/mem_sleep` has `s2idle` selected? (With s0ix_enable=1 it should. If not, we have another problem.) > > > > > > > > > > > > > > > > Yes. (I think that's based on the FADT) > > > > > > > > > > > > > > Right, there is some bit in FADT for that. > > > > > > > > > > > > > > > > > > > > > > > > Does your system stay in sleep after executing `echo freeze >/sys/power/state`? > > > > > > > > > > > > > > > > Is the alternative that it wakes immediately? Then yes, it stays in sleep. However, when it does wake, it has always been unlocked. I don't know if that's relevant. > > > > > > > > > > > > > > Unlocked vs. locked is user-space and depends on the desktop environment. XFCE has an option for locking before going to sleep but that only works when sleep was initiated from the DE. > > > > > > > > > > > > That makes sense; I thought it might have been a possibility. > > > > > > > > > > > > > If it wakes immediately with that error, the self-test failed. I'm surprised to see the self-test fail *without* immediate wake, though.... Maybe we have to exclude the SlpS0 state from the table LPIT, when s0ix is not supported on a board :S Could you test this, please? Insert a "return" right before the comment "System (Slp_S0) residency counter" in `src/soc/intel/common/block/acpi/lpit.c` and see if the error disappears. > > > > > > > > > > > > Based on https://01.org/blogs/qwang59/2020/linux-s0ix-troubleshooting, PC10 without S0ix residency is a possible case. If I understand correctly, that situation would be what I see with this board: No S0ix/failed self-test, but PC10 residency/no immediate wake. I will attempt to troubleshoot that sometime by disabling devices, I suppose, following that link. > > > > > > > > > > > > I'm also not certain what you mean. If S0ix is disabled, would LPIT factor? Besides, if S0ix does relate to hardware state, but not board-design, shouldn't LPIT always advertise both PC10 residency and system residency to warn about broken S0ix support for a board? > > > > > > > > > > Well, yes PC10 without S0ix is possible. s0ix_enable means means "PC10 and/or SlpS0 possible". Note that SlpS0 is a substate of s0ix but s0ix != SlpS0. > > > > > > > > > > The LPIT table contains two entries, one for PC10 (the first one) and SlpS0 (the second one. Based on these entries, the OS decides what *actually* is possible. When PC10 and SlpS0 is advertised but SlpS0 doesn't work, that error triggers. So, I want to test what happens, when SlpS0 is not advertised, because it doesn't apply to your board. > > > > > > > > I see the same error. > > > > > > Strange.... > > > > I can double-check. > > > > > > > > > > Additionally, I noticed while looking over the vendor firmware's LPIT table that it defines the PC10 subtable twice, if I'm reading it correctly (two copies of address 0x632 as "FunctionalFixedHW" - the PKG_C10_RESIDENCY MSR?). Even so, the kernel warns that the system was unable to enter PC10. > > > > > > Oh yeah, I've seen that too somewhere but I doubt that's correct. > > > > > > > > > > > So, while I was initially surprised to hear that SlpS0 might not apply to my board, perhaps that is the case. I can always try to troubleshoot some other time. > > > > > > > > > > But regardless: `return` or `return current`? The PC10 residency counter portion does increment "current" > > > > > > > > > > Yes, you're right, it should be `return current` ofc :-) > > > > > > > > Was this only for testing, or do you think that the S0ix subtable should only be advertised if S0ix is known to work? > > > > > > Very good question. S0ix isn't documented very good but I wouldn't advertise something that isn't usable. However, that will require us to introduce a dt option `slps0_supported`. That's already planned but I currently have higher-prio stuff. I'll come back to that asap. If you're curious, have a look on my WIP patches on s0ix. > > > > > > Since it doesn't seem to make a difference irt that error/warning, let's keep it as-is for now and resolve that as soon as I get back to these patches. Wdyt? > > > > The warning is informative and not harmful, so that's fine by me. > > > > > > If S0ix can be temperamental, I think that the OS should always be able to warn about errors. > > > > > > Well, when the board does not support SlpS0 (= SlpS0 is unconnected) I wouldn't want the OS to warn about it. That warning/error is triggered by the S0ix self-test, which may also be buggy btw. > > > > Ah. If the pad being disconnected means that lower S0ix states aren't possible, then there might not be anything to debug. > > Wait... I had some knot in my brain. SlpS0 wouldn't *do* anything (b/c it's nc) but it should still be asserted and that error is telling us that it isn't. I have `PAD_CFG_TERM_GPO(GPP_B12, 1, DN_20K, DEEP),` for SLP_S0# in the gpio.c for my board, I can change it to "UP_20K" if it won't cause problems with the PMC. > We just don't know yet, why the error is triggered, when SlpS0 is not advertised... *That* could be the bug here, which would be in the kernel. So, we need to check if Linux only checks the FADT flag or differentiates C10-only and SlpS0 devices. I'll double-check this. Regarding the kernel, it appears it uses the default address if LPIT doesn't advertise one (https://github.com/torvalds/linux/blob/master/drivers/platform/x86/intel_pm…). > > > > Sorry it took me so long to get back to you. > > > > > > No problem :-) > > > > > > > > > So... last question - I hope I didn't ask that already, but I haven't seen it above. Is this patch the cause for that error message or does the error message appear even without it? > > > > The error is produced by the intel_pmc_core driver, which is only loaded if the PEP table is present? > > Oh right... that was it :/ -- To view, visit https://review.coreboot.org/c/coreboot/+/49140 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I08d8c1fde4f447e9292a0508649f802fdc2721e1 Gerrit-Change-Number: 49140 Gerrit-PatchSet: 3 Gerrit-Owner: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Comment-Date: Wed, 03 Feb 2021 23:23:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Benjamin Doron <benjamin.doron00(a)gmail.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: [WIP] soc/amd/cezanne: select ACPI support and make the compiler happy
by Felix Held (Code Review) 03 Feb '21

03 Feb '21

Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/50273 ) Change subject: [WIP] soc/amd/cezanne: select ACPI support and make the compiler happy ...................................................................... [WIP] soc/amd/cezanne: select ACPI support and make the compiler happy This isn't meant to be submitted in the current state. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: I9b806569154e46418fa7d4fa35575a0acfec9132 --- A src/mainboard/amd/majolica/dsdt.asl A src/mainboard/google/guybrush/dsdt.asl M src/soc/amd/cezanne/Kconfig M src/soc/amd/cezanne/Makefile.inc A src/soc/amd/cezanne/acpi.c A src/soc/amd/cezanne/include/soc/acpi.h A src/soc/amd/cezanne/include/soc/nvs.h M src/soc/amd/cezanne/include/soc/southbridge.h 8 files changed, 124 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/50273/1 diff --git a/src/mainboard/amd/majolica/dsdt.asl b/src/mainboard/amd/majolica/dsdt.asl new file mode 100644 index 0000000..637a6e5 --- /dev/null +++ b/src/mainboard/amd/majolica/dsdt.asl @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#define MAINBOARD_HAS_SPEAKER 1 + +/* DefinitionBlock Statement */ +#include <acpi/acpi.h> +DefinitionBlock ( + "dsdt.aml", + "DSDT", + ACPI_DSDT_REV_2, + OEM_ID, + ACPI_TABLE_CREATOR, + 0x00010001 /* OEM Revision */ + ) +{ /* Start of ASL file */ + #include <acpi/dsdt_top.asl> + + /* System Bus */ + Scope(\_SB) { /* Start \_SB scope */ + /* global utility methods expected within the \_SB scope */ + #include <arch/x86/acpi/globutil.asl> + + } /* End \_SB scope */ +} +/* End of ASL file */ + diff --git a/src/mainboard/google/guybrush/dsdt.asl b/src/mainboard/google/guybrush/dsdt.asl new file mode 100644 index 0000000..5b24c2e --- /dev/null +++ b/src/mainboard/google/guybrush/dsdt.asl @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +/* DefinitionBlock Statement */ +#include <acpi/acpi.h> +DefinitionBlock ( + "dsdt.aml", + "DSDT", + ACPI_DSDT_REV_2, + OEM_ID, + ACPI_TABLE_CREATOR, + 0x00010001 /* OEM Revision */ + ) +{ /* Start of ASL file */ + #include <acpi/dsdt_top.asl> + + /* System Bus */ + Scope(\_SB) { /* Start \_SB scope */ + /* global utility methods expected within the \_SB scope */ + #include <arch/x86/acpi/globutil.asl> + + } /* End \_SB scope */ +} +/* End of ASL file */ + diff --git a/src/soc/amd/cezanne/Kconfig b/src/soc/amd/cezanne/Kconfig index 611260d..3d6d23a 100644 --- a/src/soc/amd/cezanne/Kconfig +++ b/src/soc/amd/cezanne/Kconfig @@ -13,14 +13,17 @@ select ARCH_VERSTAGE_X86_32 select ARCH_ROMSTAGE_X86_32 select ARCH_RAMSTAGE_X86_32 + select ACPI_AMD_HARDWARE_SLEEP_VALUES select FSP_COMPRESS_FSP_M_LZMA select FSP_COMPRESS_FSP_S_LZMA + select HAVE_ACPI_TABLES select HAVE_CF9_RESET select IDT_IN_EVERY_STAGE select IOAPIC select PLATFORM_USES_FSP2_0 select RESET_VECTOR_IN_RAM select SOC_AMD_COMMON + select SOC_AMD_COMMON_BLOCK_ACPI select SOC_AMD_COMMON_BLOCK_ACPIMMIO select SOC_AMD_COMMON_BLOCK_AOAC select SOC_AMD_COMMON_BLOCK_BANKED_GPIOS diff --git a/src/soc/amd/cezanne/Makefile.inc b/src/soc/amd/cezanne/Makefile.inc index 9422a4d..e91e792 100644 --- a/src/soc/amd/cezanne/Makefile.inc +++ b/src/soc/amd/cezanne/Makefile.inc @@ -24,6 +24,7 @@ romstage-y += romstage.c romstage-y += uart.c +ramstage-y += acpi.c ramstage-y += chip.c ramstage-y += fch.c ramstage-y += fsp_params.c diff --git a/src/soc/amd/cezanne/acpi.c b/src/soc/amd/cezanne/acpi.c new file mode 100644 index 0000000..a80b426 --- /dev/null +++ b/src/soc/amd/cezanne/acpi.c @@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +/* ACPI - create the Fixed ACPI Description Tables (FADT) */ + +#include <acpi/acpi.h> + +unsigned long acpi_fill_mcfg(unsigned long current) +{ + + current += acpi_create_mcfg_mmconfig((acpi_mcfg_mmconfig_t *)current, + CONFIG_MMCONF_BASE_ADDRESS, + 0, + 0, + CONFIG_MMCONF_BUS_NUMBER - 1); + + return current; +} + +unsigned long acpi_fill_madt(unsigned long current) +{ + /* TODO */ + return 0; +} + +/* + * Reference section 5.2.9 Fixed ACPI Description Table (FADT) + * in the ACPI 3.0b specification. + */ +void acpi_fill_fadt(acpi_fadt_t *fadt) +{ + /* TODO */ +} + diff --git a/src/soc/amd/cezanne/include/soc/acpi.h b/src/soc/amd/cezanne/include/soc/acpi.h new file mode 100644 index 0000000..68b64aa --- /dev/null +++ b/src/soc/amd/cezanne/include/soc/acpi.h @@ -0,0 +1,6 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#ifndef AMD_CEZANNE_ACPI_H +#define AMD_CEZANNE_ACPI_H + +#endif /* AMD_CEZANNE_ACPI_H */ diff --git a/src/soc/amd/cezanne/include/soc/nvs.h b/src/soc/amd/cezanne/include/soc/nvs.h new file mode 100644 index 0000000..07053d5 --- /dev/null +++ b/src/soc/amd/cezanne/include/soc/nvs.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +/* + * NOTE: The layout of the global_nvs structure below must match the layout + * in soc/soc/amd/picasso/acpi/globalnvs.asl !!! + * + */ + +#ifndef AMD_CEZANNE_NVS_H +#define AMD_CEZANNE_NVS_H + +#include <stdint.h> + +struct __packed global_nvs { + /* Miscellaneous */ + uint8_t unused_was_pcnt; /* 0x00 - Processor Count */ + uint8_t lids; /* 0x01 - LID State */ + uint8_t pwrs; /* 0x02 - AC Power State */ + uint32_t cbmc; /* 0x03 - 0x06 - coreboot Memory Console */ + uint64_t pm1i; /* 0x07 - 0x0e - System Wake Source - PM1 Index */ + uint64_t gpei; /* 0x0f - 0x16 - GPE Wake Source */ + uint8_t tmps; /* 0x17 - Temperature Sensor ID */ + uint8_t tcrt; /* 0x18 - Critical Threshold */ + uint8_t tpsv; /* 0x19 - Passive Threshold */ +}; + +#endif /* AMD_CEZANNE_NVS_H */ diff --git a/src/soc/amd/cezanne/include/soc/southbridge.h b/src/soc/amd/cezanne/include/soc/southbridge.h index 8623063..f644f78 100644 --- a/src/soc/amd/cezanne/include/soc/southbridge.h +++ b/src/soc/amd/cezanne/include/soc/southbridge.h @@ -40,6 +40,10 @@ #define PM_ACPI_NB_PME_GEVENT BIT(28) #define PM_ACPI_RTC_WAKE_EN BIT(29) +#define PM1_LIMIT 16 +#define GPE0_LIMIT 32 +#define TOTAL_BITS(a) (8 * sizeof(a)) + #define FCH_LEGACY_UART_DECODE (ALINK_AHB_ADDRESS + 0x20) /* 0xfedc0020 */ /* FCH AOAC device offsets for AOAC_DEV_D3_CTL/AOAC_DEV_D3_STATE */ -- To view, visit https://review.coreboot.org/c/coreboot/+/50273 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9b806569154e46418fa7d4fa35575a0acfec9132 Gerrit-Change-Number: 50273 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: Documentation: Codify some guidelines for headers and chain-including
by Angel Pons (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Martin Roth, Julius Werner. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50247 ) Change subject: Documentation: Codify some guidelines for headers and chain-including ...................................................................... Patch Set 1: (1 comment) File Documentation/coding_style.md: https://review.coreboot.org/c/coreboot/+/50247/comment/f230b816_7ab758f4 PS1, Line 876: take advantage of the macro > I think the intention is to show what the macro does right away in this example. […] Ah, the diffstat tricked me -- To view, visit https://review.coreboot.org/c/coreboot/+/50247 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibbcde306a814f52b3a41b58c7a33bdd99b0187e0 Gerrit-Change-Number: 50247 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Comment-Date: Wed, 03 Feb 2021 22:46:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: Documentation: Codify some guidelines for headers and chain-including
by Julius Werner (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Martin Roth, Angel Pons. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50247 ) Change subject: Documentation: Codify some guidelines for headers and chain-including ...................................................................... Patch Set 1: (2 comments) Patchset: PS1: > here is what I've tried to list: […] I'm trying to just explain the general reasons where chain-including should be acceptable. I don't think we should try to maintain an exhaustive list somewhere, because these things change all the time (e.g. more stuff gets factored out into commonlib). The headers I'm mentioning there are just examples for the overall categories of cases (e.g. what applies to <cbfs.h> -> <commonlib/bsd/cbfs_serialized.h> also applies to <fmap.h> -> <commonlib/bsd/fmap_serialized.h>). I think the examples I mention already cover all the cases Elyes mentioned? File Documentation/coding_style.md: https://review.coreboot.org/c/coreboot/+/50247/comment/f99d6452_3f2bad77 PS1, Line 876: take advantage of the macro > take advantage of the `ARRAY_SIZE` macro: I think the intention is to show what the macro does right away in this example. This is just from the original kernel coding style, I don't really want to touch it, I just wanted to update the references that obviously don't make sense for coreboot. -- To view, visit https://review.coreboot.org/c/coreboot/+/50247 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibbcde306a814f52b3a41b58c7a33bdd99b0187e0 Gerrit-Change-Number: 50247 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Feb 2021 22:44:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Comment-In-Reply-To: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/mancomb: Add new mainboard
by Mathew King (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Martin Roth, Mathew King. Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Kangheui Won, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/50007 to look at the new patch set (#6). Change subject: mb/google/mancomb: Add new mainboard ...................................................................... mb/google/mancomb: Add new mainboard Mancomb is a new Google mainboard with an AMD SOC. BUG=b:175143925 TEST=builds Change-Id: I1264f44a0b986f7f7c89ac7b42f1e4e4119a35e6 Signed-off-by: Mathew King <mathewk(a)chromium.org> --- A src/mainboard/google/mancomb/Kconfig A src/mainboard/google/mancomb/Kconfig.name A src/mainboard/google/mancomb/Makefile.inc A src/mainboard/google/mancomb/board_info.txt A src/mainboard/google/mancomb/bootblock.c A src/mainboard/google/mancomb/chromeos.fmd A src/mainboard/google/mancomb/mainboard.c A src/mainboard/google/mancomb/variants/baseboard/devicetree.cb A src/mainboard/google/mancomb/variants/baseboard/include/baseboard/gpio.h A src/mainboard/google/mancomb/variants/baseboard/include/baseboard/variants.h 10 files changed, 124 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/50007/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/50007 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1264f44a0b986f7f7c89ac7b42f1e4e4119a35e6 Gerrit-Change-Number: 50007 Gerrit-PatchSet: 6 Gerrit-Owner: Mathew King <mathewk(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Bhanu Prakash Maiya <bhanumaiya(a)google.com> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Mathew King <mathewk(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: mb/google/brya: Add support for Hynix H9HCNNNBKMMLXR-NEE LP4x DRAM
by Furquan Shaikh (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Martin Roth, Tim Wawrzynczak, Aamir Bohra, Boris Mittelberg. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50271 ) Change subject: mb/google/brya: Add support for Hynix H9HCNNNBKMMLXR-NEE LP4x DRAM ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/50271 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icccfa3d1659e6c74c14a7372ea39c749a5921c64 Gerrit-Change-Number: 50271 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-Comment-Date: Wed, 03 Feb 2021 22:30:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/skylake/acpi: Add PEP table
by Michael Niewöhner (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Benjamin Doron. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49140 ) Change subject: soc/intel/skylake/acpi: Add PEP table ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/49140/comment/bc26ed0a_50e1ae85 PS3, Line 11: > > > > > > > > Awesome, then at least S0ix states work just fine. > > > > > > > > > > > > > > > > That error message comes from the kernel SlpS0 self-test. So, let's test something. I assume `cat /sys/power/mem_sleep` has `s2idle` selected? (With s0ix_enable=1 it should. If not, we have another problem.) > > > > > > > > > > > > > > Yes. (I think that's based on the FADT) > > > > > > > > > > > > Right, there is some bit in FADT for that. > > > > > > > > > > > > > > > > > > > > > Does your system stay in sleep after executing `echo freeze >/sys/power/state`? > > > > > > > > > > > > > > Is the alternative that it wakes immediately? Then yes, it stays in sleep. However, when it does wake, it has always been unlocked. I don't know if that's relevant. > > > > > > > > > > > > Unlocked vs. locked is user-space and depends on the desktop environment. XFCE has an option for locking before going to sleep but that only works when sleep was initiated from the DE. > > > > > > > > > > That makes sense; I thought it might have been a possibility. > > > > > > > > > > > If it wakes immediately with that error, the self-test failed. I'm surprised to see the self-test fail *without* immediate wake, though.... Maybe we have to exclude the SlpS0 state from the table LPIT, when s0ix is not supported on a board :S Could you test this, please? Insert a "return" right before the comment "System (Slp_S0) residency counter" in `src/soc/intel/common/block/acpi/lpit.c` and see if the error disappears. > > > > > > > > > > Based on https://01.org/blogs/qwang59/2020/linux-s0ix-troubleshooting, PC10 without S0ix residency is a possible case. If I understand correctly, that situation would be what I see with this board: No S0ix/failed self-test, but PC10 residency/no immediate wake. I will attempt to troubleshoot that sometime by disabling devices, I suppose, following that link. > > > > > > > > > > I'm also not certain what you mean. If S0ix is disabled, would LPIT factor? Besides, if S0ix does relate to hardware state, but not board-design, shouldn't LPIT always advertise both PC10 residency and system residency to warn about broken S0ix support for a board? > > > > > > > > Well, yes PC10 without S0ix is possible. s0ix_enable means means "PC10 and/or SlpS0 possible". Note that SlpS0 is a substate of s0ix but s0ix != SlpS0. > > > > > > > > The LPIT table contains two entries, one for PC10 (the first one) and SlpS0 (the second one. Based on these entries, the OS decides what *actually* is possible. When PC10 and SlpS0 is advertised but SlpS0 doesn't work, that error triggers. So, I want to test what happens, when SlpS0 is not advertised, because it doesn't apply to your board. > > > > > > I see the same error. > > > > Strange.... > > I can double-check. > > > > > > > Additionally, I noticed while looking over the vendor firmware's LPIT table that it defines the PC10 subtable twice, if I'm reading it correctly (two copies of address 0x632 as "FunctionalFixedHW" - the PKG_C10_RESIDENCY MSR?). Even so, the kernel warns that the system was unable to enter PC10. > > > > Oh yeah, I've seen that too somewhere but I doubt that's correct. > > > > > > > > So, while I was initially surprised to hear that SlpS0 might not apply to my board, perhaps that is the case. I can always try to troubleshoot some other time. > > > > > > > > But regardless: `return` or `return current`? The PC10 residency counter portion does increment "current" > > > > > > > > Yes, you're right, it should be `return current` ofc :-) > > > > > > Was this only for testing, or do you think that the S0ix subtable should only be advertised if S0ix is known to work? > > > > Very good question. S0ix isn't documented very good but I wouldn't advertise something that isn't usable. However, that will require us to introduce a dt option `slps0_supported`. That's already planned but I currently have higher-prio stuff. I'll come back to that asap. If you're curious, have a look on my WIP patches on s0ix. > > > > Since it doesn't seem to make a difference irt that error/warning, let's keep it as-is for now and resolve that as soon as I get back to these patches. Wdyt? > > The warning is informative and not harmful, so that's fine by me. > > > > If S0ix can be temperamental, I think that the OS should always be able to warn about errors. > > > > Well, when the board does not support SlpS0 (= SlpS0 is unconnected) I wouldn't want the OS to warn about it. That warning/error is triggered by the S0ix self-test, which may also be buggy btw. > > Ah. If the pad being disconnected means that lower S0ix states aren't possible, then there might not be anything to debug. Wait... I had some knot in my brain. SlpS0 wouldn't *do* anything (b/c it's nc) but it should still be asserted and that error is telling us that it isn't. We just don't know yet, why the error is triggered, when SlpS0 is not advertised... *That* could be the bug here, which would be in the kernel. So, we need to check if Linux only checks the FADT flag or differentiates C10-only and SlpS0 devices. > > > > Sorry it took me so long to get back to you. > > > > No problem :-) > > > > > > So... last question - I hope I didn't ask that already, but I haven't seen it above. Is this patch the cause for that error message or does the error message appear even without it? > > The error is produced by the intel_pmc_core driver, which is only loaded if the PEP table is present? Oh right... that was it :/ -- To view, visit https://review.coreboot.org/c/coreboot/+/49140 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I08d8c1fde4f447e9292a0508649f802fdc2721e1 Gerrit-Change-Number: 49140 Gerrit-PatchSet: 3 Gerrit-Owner: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Feb 2021 22:27:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Benjamin Doron <benjamin.doron00(a)gmail.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/guybrush: Add stubs to configure GPIOs
by Mathew King (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Martin Roth, Angel Pons. Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49954 ) Change subject: mb/google/guybrush: Add stubs to configure GPIOs ...................................................................... Patch Set 11: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/49954/comment/86b7cdef_57e24c1d PS10, Line 7: stubs to configure GPIOs > Add stubs to configure GPIOs Done -- To view, visit https://review.coreboot.org/c/coreboot/+/49954 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5afd2df396ba41f7d25fa7ff6879b7c1f82f438c Gerrit-Change-Number: 49954 Gerrit-PatchSet: 11 Gerrit-Owner: Mathew King <mathewk(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Feb 2021 22:17:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/guybrush: Add stubs to configure GPIOs
by Mathew King (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Martin Roth, Mathew King. Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Angel Pons, EricR Lai, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/49954 to look at the new patch set (#11). Change subject: mb/google/guybrush: Add stubs to configure GPIOs ...................................................................... mb/google/guybrush: Add stubs to configure GPIOs BUG=b:175143925 TEST=builds Signed-off-by: Mathew King <mathewk(a)chromium.org> Change-Id: I5afd2df396ba41f7d25fa7ff6879b7c1f82f438c --- M src/mainboard/google/guybrush/bootblock.c M src/mainboard/google/guybrush/mainboard.c A src/mainboard/google/guybrush/variants/baseboard/Makefile.inc A src/mainboard/google/guybrush/variants/baseboard/gpio.c M src/mainboard/google/guybrush/variants/baseboard/include/baseboard/variants.h 5 files changed, 72 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/49954/11 -- To view, visit https://review.coreboot.org/c/coreboot/+/49954 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5afd2df396ba41f7d25fa7ff6879b7c1f82f438c Gerrit-Change-Number: 49954 Gerrit-PatchSet: 11 Gerrit-Owner: Mathew King <mathewk(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Mathew King <mathewk(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: mb/apple/macbookair4_2: Fix USB ports config
by Evgeny Zinoviev (Code Review) 03 Feb '21

03 Feb '21

Evgeny Zinoviev has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/50272 ) Change subject: mb/apple/macbookair4_2: Fix USB ports config ...................................................................... mb/apple/macbookair4_2: Fix USB ports config Disable unused ports and add comments. Used 820-3023 board schematics as a source. Change-Id: I2862546ca6f6929a86e77fae7337368742bb9ba8 Signed-off-by: Evgeny Zinoviev <me(a)ch1p.io> --- M src/mainboard/apple/macbookair4_2/early_init.c 1 file changed, 14 insertions(+), 14 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/50272/1 diff --git a/src/mainboard/apple/macbookair4_2/early_init.c b/src/mainboard/apple/macbookair4_2/early_init.c index a32298d..7e2ec1b 100644 --- a/src/mainboard/apple/macbookair4_2/early_init.c +++ b/src/mainboard/apple/macbookair4_2/early_init.c @@ -6,20 +6,20 @@ #include <cbfs.h> const struct southbridge_usb_port mainboard_usb_ports[] = { - { 1, 0, -1 }, - { 1, 0, -1 }, - { 1, 0, -1 }, - { 1, 0, -1 }, - { 1, 0, -1 }, - { 1, 0, -1 }, - { 1, 0, -1 }, - { 1, 0, -1 }, - { 1, 0, -1 }, - { 1, 0, -1 }, - { 1, 0, -1 }, - { 1, 0, -1 }, - { 1, 0, -1 }, - { 1, 0, -1 }, + { 1, 0, -1 }, /* USB HUB 1 */ + { 0, 0, -1 }, /* Unused */ + { 0, 0, -1 }, /* Unused */ + { 0, 0, -1 }, /* Unused */ + { 0, 0, -1 }, /* Unused */ + { 0, 0, -1 }, /* Unused */ + { 0, 0, -1 }, /* Unused */ + { 0, 0, -1 }, /* Unused */ + { 1, 0, -1 }, /* USB HUB 2 */ + { 1, 0, -1 }, /* Camera */ + { 0, 0, -1 }, /* Unused */ + { 0, 0, -1 }, /* Unused */ + { 0, 0, -1 }, /* Unused */ + { 0, 0, -1 }, /* Unused */ }; void mainboard_get_spd(spd_raw_data *spd, bool id_only) -- To view, visit https://review.coreboot.org/c/coreboot/+/50272 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2862546ca6f6929a86e77fae7337368742bb9ba8 Gerrit-Change-Number: 50272 Gerrit-PatchSet: 1 Gerrit-Owner: Evgeny Zinoviev <me(a)ch1p.io> Gerrit-MessageType: newchange

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit February 2021