Attention is currently required from: Alexander Couzens, Nicholas Sudsgaard, Paul Menzel.
Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80343?usp=email )
Change subject: mainboard/lenovo: Add ThinkCentre M710s (Skylake)
......................................................................
Patch Set 3:
(2 comments)
File src/mainboard/lenovo/thinkcentre_m710s/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/80343/comment/2d7cbcae_e8e20ab5 :
PS3, Line 1: GPL-2.0-only
Should be CC-PDDC, refer to commit cf4722d317 (src/mb: Update unlicensable files with the CC-PDDC SPDX ID)
File src/mainboard/lenovo/thinkcentre_m710s/acpi/superio.asl:
https://review.coreboot.org/c/coreboot/+/80343/comment/df90abc6_6d54035a :
PS3, Line 1: GPL-2.0-onl
CC-PDDC
--
To view, visit https://review.coreboot.org/c/coreboot/+/80343?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I551753aecfbd2c0ee57d85bb22cb943eb21af3cc
Gerrit-Change-Number: 80343
Gerrit-PatchSet: 3
Gerrit-Owner: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Attention: Alexander Couzens <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Wed, 07 Feb 2024 15:43:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Alper Nebi Yasak.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80366?usp=email )
Change subject: mb/qemu/fw_cfg: Fix build when not generating SMBIOS tables
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Patchset:
PS1:
Alternatively, we could put the code into a separate file. It only uses
one already exported API function, AFAICS, so shouldn't make any trouble.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80366?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I3ff388d4574eb52686a5dda3dcbc3d64a7ce6f7b
Gerrit-Change-Number: 80366
Gerrit-PatchSet: 1
Gerrit-Owner: Alper Nebi Yasak <alpernebiyasak(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Alper Nebi Yasak <alpernebiyasak(a)gmail.com>
Gerrit-Comment-Date: Wed, 07 Feb 2024 15:42:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Alexander Couzens, Nicholas Sudsgaard, Paul Menzel.
Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80343?usp=email )
Change subject: mainboard/lenovo: Add ThinkCentre M710s (Skylake)
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
This needs to be rebased on top of your other patch in CB:80344 (superio/ifd: Add IT8629E) so that CONFIG_SUPERIO_ITE_IT8629E is defined, which is what Jenkins is complaining about. Should just be a `git rebase -i HEAD~2` with the superio patch checked out, and then swap the two lines to reorder them.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80343?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I551753aecfbd2c0ee57d85bb22cb943eb21af3cc
Gerrit-Change-Number: 80343
Gerrit-PatchSet: 3
Gerrit-Owner: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Attention: Alexander Couzens <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Wed, 07 Feb 2024 15:37:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Alper Nebi Yasak.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80364?usp=email )
Change subject: mainboard/qemu-aarch64: Get top of memory from device-tree blob
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/80364?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I4cc888b57cf98e0797ce7f9ddfa2eb34d14cd9c1
Gerrit-Change-Number: 80364
Gerrit-PatchSet: 2
Gerrit-Owner: Alper Nebi Yasak <alpernebiyasak(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Alper Nebi Yasak <alpernebiyasak(a)gmail.com>
Gerrit-Comment-Date: Wed, 07 Feb 2024 15:34:13 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Alper Nebi Yasak, Philipp Hug, ron minnich.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80363?usp=email )
Change subject: mainboard/qemu-riscv: Get top of memory from device-tree blob
......................................................................
Patch Set 2:
(1 comment)
File src/soc/ucb/riscv/cbmem.c:
https://review.coreboot.org/c/coreboot/+/80363/comment/f3660c6a_c89508ca :
PS2, Line 13: top = fdt_get_memory_top((void *)HLS()->fdt);
> Breaks mb/spike-riscv build. […]
I wonder why they share a SoC. Though to me, the most straight forward option
seems to be the `if (CONFIG(FLATENED_DEVICE_TREE))`.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80363?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I9e4a95f49ad373675939329eef40d7423a4132ab
Gerrit-Change-Number: 80363
Gerrit-PatchSet: 2
Gerrit-Owner: Alper Nebi Yasak <alpernebiyasak(a)gmail.com>
Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx>
Gerrit-Reviewer: ron minnich <rminnich(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Philipp Hug <philipp(a)hug.cx>
Gerrit-Attention: Alper Nebi Yasak <alpernebiyasak(a)gmail.com>
Gerrit-Attention: ron minnich <rminnich(a)gmail.com>
Gerrit-Comment-Date: Wed, 07 Feb 2024 15:32:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Alper Nebi Yasak <alpernebiyasak(a)gmail.com>
Gerrit-MessageType: comment
Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80383?usp=email )
Change subject: Makefile.mk: Include build/dsdt.d at common point of build
......................................................................
Patch Set 1:
This change is ready for review.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80383?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ie8271d1e172395917f2859c8bbfd2041ddc572ca
Gerrit-Change-Number: 80383
Gerrit-PatchSet: 1
Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Comment-Date: Wed, 07 Feb 2024 15:28:41 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Alper Nebi Yasak.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80322?usp=email )
Change subject: device_tree: Add function to get top of memory from a FDT blob
......................................................................
Patch Set 3: Code-Review+1
(6 comments)
Patchset:
PS3:
Thanks for the effort and the elaborate commit message!
Look pretty good to me. Though, I would extract a few common functions
like fast-forwarding to a specific node/property.
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/80322/comment/718f6b9b_8b6d2708 :
PS3, Line 188: int addr_cells = 0;
Please use `unsigned` so we don't have to think about potential overflows.
https://review.coreboot.org/c/coreboot/+/80322/comment/7a0259d6_ba745c36 :
PS3, Line 191: if (!strncmp(prop.name, "#address-cells", sizeof("#address-cells")))
Why the str*n*cmp()? Isn't "#address_cells" the whole string we should match?
https://review.coreboot.org/c/coreboot/+/80322/comment/d9dca217_7d7d4a59 :
PS3, Line 192: addr_cells = be32toh(*(uint32_t *)prop.data);
read_be32() would be easier to use here. <commonlib/endian.h>
https://review.coreboot.org/c/coreboot/+/80322/comment/a3471f23_9f900247 :
PS3, Line 229: return 0;
Should we also check that `reg_size >= (addr_cells + size_cells) * sizeof(uint32_t)`?
https://review.coreboot.org/c/coreboot/+/80322/comment/4346e473_74575c68 :
PS3, Line 253: }
It's not wrong to write it as a loop, but the code would become much clearer
if we'd just handle the two cases `size_cells == 1` and `size_cells == 2`.
Also, the logic could be extracted into a common function, so this could become e.g.
```
static uint64_t read_reg_value(const uint32_t *reg_data, unsigned int num_cells);
mem_base = read_reg_value(reg_data, addr_cells);
mem_size = read_reg_value(reg_data + addr_cells, size_cells);
```
--
To view, visit https://review.coreboot.org/c/coreboot/+/80322?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8bef09bc1bc4e324ebeaa37f78d67d3aa315f52c
Gerrit-Change-Number: 80322
Gerrit-PatchSet: 3
Gerrit-Owner: Alper Nebi Yasak <alpernebiyasak(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Alper Nebi Yasak <alpernebiyasak(a)gmail.com>
Gerrit-Comment-Date: Wed, 07 Feb 2024 15:27:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Bora Guvendik, Felix Singer, Shelley Chen.
Ashish Kumar Mishra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80334?usp=email )
Change subject: mb/google/brox: Handle GPI_INT pin lower to GPI_WAKE
......................................................................
Patch Set 3:
(1 comment)
File src/mainboard/google/brox/variants/baseboard/brox/gpio.c:
https://review.coreboot.org/c/coreboot/+/80334/comment/20a13f88_f9718bc2 :
PS3, Line 188: PAD_CFG_GPI_APIC_LOCK(GPP_E2, NONE, LEVEL, INVERT, LOCK_CONFIG),
> Just to confirm, since this pin is routed through IOAPIC this will not be part of free irq and hence […]
yes. Any pin that has IOAPIC flag set(in this case it is), will become
unavailable.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80334?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8c3d557e888b8d0ceac203f49b702910fba26d6d
Gerrit-Change-Number: 80334
Gerrit-PatchSet: 3
Gerrit-Owner: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Deepti Deshatty <deepti.deshatty(a)intel.com>
Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-CC: Haribalaraman Ramasubramanian <haribalaraman.r(a)intel.com>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Comment-Date: Wed, 07 Feb 2024 15:27:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: comment
Nicholas Chin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/80383?usp=email )
Change subject: Makefile.mk: Include build/dsdt.d at common point of build
......................................................................
Makefile.mk: Include build/dsdt.d at common point of build
Instead of including the dependency file generated in asl_template when
the template is evaluated, add it to the DEPENDENCIES variable so that
it is included when the rest of the .d files in the DEPENDENCIES are
included in the top level Makefile. This should be safe since template
is evaluated while calling includemakefiles, which is called before the
files in DEPENDENCIES are included.
Change-Id: Ie8271d1e172395917f2859c8bbfd2041ddc572ca
Signed-off-by: Nicholas Chin <nic.c3.14(a)gmail.com>
---
M Makefile.mk
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/80383/1
diff --git a/Makefile.mk b/Makefile.mk
index f7b98c7..f0fa7fd 100644
--- a/Makefile.mk
+++ b/Makefile.mk
@@ -302,7 +302,7 @@
$(CONFIG_CBFS_PREFIX)/$(1).aml-align = 64
endif
cbfs-files-$(if $(2),$(2),y) += $(CONFIG_CBFS_PREFIX)/$(1).aml
--include $(obj)/$(1).d
+DEPENDENCIES += $(obj)/$(1).d
$(obj)/$(1).aml: $(src)/mainboard/$(MAINBOARDDIR)/$(1).asl $(obj)/config.h
@printf " IASL $$(subst $(top)/,,$$(@))\n"
$(CC_ramstage) -x assembler-with-cpp -E -MMD -MT $$(@) $$(CPPFLAGS_ramstage) -D__ACPI__ -P -include $(src)/include/kconfig.h -I$(obj) -I$(src) -I$(src)/include -I$(src)/arch/$(ARCHDIR-$(ARCH-ramstage-y))/include -I$(src)/mainboard/$(MAINBOARDDIR) $$< -o $(obj)/$(1).asl
--
To view, visit https://review.coreboot.org/c/coreboot/+/80383?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ie8271d1e172395917f2859c8bbfd2041ddc572ca
Gerrit-Change-Number: 80383
Gerrit-PatchSet: 1
Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-MessageType: newchange