Attention is currently required from: Arthur Heymans, Ashish Kumar Mishra, Jérémy Compostella, Saurabh Mishra.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81959?usp=email )
Change subject: [MTL-x64]arch/x86: Update X86_64 memcpy for 4 byte copy
......................................................................
Patch Set 3: Code-Review-1
(1 comment)
Patchset:
PS3:
It sounds like you are using memcpy to access MMIO, which is just wrong. memcpy could be updated in the future to use 16byte or 32byte writes.
Please instead fix your code accessing the MMIO space.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81959?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: I1110cc18f5aa59c864e3cc04b9e6b3501ec4d54d
Gerrit-Change-Number: 81959
Gerrit-PatchSet: 3
Gerrit-Owner: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Attention: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Wed, 17 Apr 2024 16:20:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Nico Huber, Patrick Rudolph.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/64185?usp=email )
Change subject: haswell NRI: Post-process selected timings
......................................................................
Patch Set 5:
(2 comments)
File src/northbridge/intel/haswell/native_raminit/lookup_timings.c:
https://review.coreboot.org/c/coreboot/+/64185/comment/9c63fd93_8c6d1afb :
PS5, Line 9: uint32_t
> uint16_t?
While I don't expect Haswell to run at more than 65535 MHz, changing this would require refactoring a bunch of things to use 16-bit types for `mem_clock_mhz`. Same thing for timings.
Do note that follow-up patches add more timing lookup functions, and not everything is up for review yet: I'm in the process of untangling the giant bowl of spaghetti (the mostly complete but extremely messy implementation I first made).
Does using smaller types provide any measurable advantage? If so, I could change this in a follow-up.
https://review.coreboot.org/c/coreboot/+/64185/comment/fa467318_5fdffb53 :
PS5, Line 10: uint32_t
> uint16_t?
Similar reasoning as above.
--
To view, visit https://review.coreboot.org/c/coreboot/+/64185?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: Id02caf858f75b9e08016762b3aefda282b274386
Gerrit-Change-Number: 64185
Gerrit-PatchSet: 5
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Martin L Roth <gaumless(a)gmail.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Wed, 17 Apr 2024 16:12:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-MessageType: comment
Attention is currently required from: Jakub Czapiga, Julius Werner, Martin L Roth.
Maximilian Brune has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81081?usp=email )
Change subject: lib/device_tree: Add some FDT helper functions
......................................................................
Patch Set 18:
(1 comment)
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/056e60fa_2941e765 :
PS18, Line 175: ""
> Yeah, you're right, sorry. […]
```
Right now I believe(?) you want node_offset in fdt_find_node() to be pointing the the start of an array of child nodes, right? But that's not how you're calling it from fdt_find_node_by_path(). You're calling it with fdt_hdr->struct_offset, which points to the very start of the root node, not to an array of its children. The root node is a full node that begins with a FDT_BEGIN_NODE token, then a name (which is normally just four '\0' bytes), then potentially a bunch of FDT_PROP tokens, then the whole subtree of child nodes, and finally an FDT_END_NODE. So that means that when you first enter fdt_find_node() and immediately jump into that do-while loop, you're trying to iterate over an array of nodes but you actually only have a single root node. I guess it doesn't matter as long as the path you're searching can always be found under that root node, but theoretically, if the path check wasn't stopping it, your code would continue running over the end of the root node (and thereby the whole struct block) in search for more nodes.
```
I don't expect it to point at the start of an array of child nodes. I simply expect `offset` to point at a node (nothing more). Sure the root is usually only 1 node, but my code doesn't care if there is 1, 2, 3 or n nodes or if the node you are starting at is the first, second or ... child. It treats the root node like any other node therefore offset can point to it. The path check will always stop it even if there is only a single root node and nothing else. That is the reason why the first part of the path is always an empty string ("") if you do an absolute lookup (-> relative to root node). The empty string will always match the root node (or anything else for that matter) so I don't see how it is possible to skip the root node and read something after that.
```
I think it would make more sense to pass the offset of the start of the properties (i.e. what you get after you call fdt_next_node_name()). Then it can either return that immediately if we've reached the NULL in the path array, or it can parse addr/size cell props and then go iterate through the children if it hasn't.
```
That would mean that all functions that work on the properties of a node always get the offset to the start of the properties instead of the offset to the node. Seems weird to be honest since you usually traverse all properties from a given node to get the property you want. I am also not sure if the naming scheme is correct then. Since `fdt_find_node` and `fdt_find_node_by_path` do not really find the node but more like the properties of a node.
```
u32 fdt_find_node(const void *blob, u32 prop_start_offset, const char** path, u32 *addrcp, u32 *sizecp)
{
size_t size;
const char *node_name;
if (!*path)
return prop_start_offset;
u32 offset = fdt_read_cell_props(blob, prop_start_offset, addrcp, sizecp);
while ((size = fdt_next_node_name(blob, offset, &node_name))) {
if (!strcmp(*path, node_name))
return fdt_find_node(blob, offset + size, path + 1, addrcp, sizecp);
offset += fdt_skip_node(blob, offset);
}
return 0;
}
```
your implementation always finds a node relative to a parent node. `fdt_find_node` currently always finds a node relative to another node on the same level (for performance reasons). Much the same as the `dt_find_node` equivalent I mentioned (with the difference being that you now return the offset to the first property). The idea originally was that you could continue the search from a given node without having to traverse all children nodes again. Therefore one could first search for `/cpus/cpu@0` and afterwards search for `cpu@1` node relative to the `cpu@0` node and not relative to the `/cpus ` node. In a node with many children that can potentially save a significant amount of time.
At this point though it doesn't make much sense anymore (it did when I still used the wildcard stuff) since it now implies that the order of the nodes in the same level is known (for example that cpu@1 comes after cpu@0 in the devicetree). I wanted to the most performant way to get the offset of a node relative to another node (without having to traverse all children every time). But no function currently makes use of that (since the wildcard is out now) so I don't feel strongly about keeping it that way.
```
(BTW, have you noticed that you're basically parsing each node name twice, because you need to first call fdt_next_node_name() manually to have the name to strcmp(), but then fdt_skip_node() wants to call fdt_next_node_name() again? It would probably be good to optimize that, maybe by adding a skip_name parameter to fdt_skip_node().)
```
I noticed, but fdt_next_node_name doesn't really do much except the `strlen()`. Also I am not sure what the skip_name parameter would do differently compared to `fdt_next_node_name`? Would `skip_name` imply that that the offset given to `fdt_skip_node()` points to the first property of the node instead of the name?
--
To view, visit https://review.coreboot.org/c/coreboot/+/81081?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: I2fb1d93c5b3e1cb2f7d9584db52bbce3767b63d8
Gerrit-Change-Number: 81081
Gerrit-PatchSet: 18
Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Alper Nebi Yasak <alpernebiyasak(a)gmail.com>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Wed, 17 Apr 2024 16:09:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Maximilian Brune, Nico Huber.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/64184?usp=email )
Change subject: haswell NRI: Initialise MPLL
......................................................................
Patch Set 5:
(1 comment)
File src/northbridge/intel/haswell/native_raminit/init_mpll.c:
https://review.coreboot.org/c/coreboot/+/64184/comment/90aa6584_09dc4889 :
PS5, Line 209: wait_for_first_rcomp
> can this be a separate step in cold_boot[] ?
It could be, but it wouldn't make sense: it's part of initialising the MPLL, and *has to* be done after programming `MC_BIOS_REQ`.
--
To view, visit https://review.coreboot.org/c/coreboot/+/64184?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: I978c352de68f6d8cecc76f4ae3c12daaf4be9ed6
Gerrit-Change-Number: 64184
Gerrit-PatchSet: 5
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-CC: Martin L Roth <gaumless(a)gmail.com>
Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Wed, 17 Apr 2024 16:03:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Christian Walter, David Hendricks, Jincheng Li, Johnny Lin, Jonathan Zhang, Patrick Rudolph, Shuo Liu, TangYiwei, Tim Chu.
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81317?usp=email )
Change subject: soc/intel/xeon_sp/gnr: Use OCP_VPD drivers
......................................................................
Patch Set 35: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/81317?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: Ifeca8cf4312ab66ca03188fe25af88a952073130
Gerrit-Change-Number: 81317
Gerrit-PatchSet: 35
Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Jincheng Li <jincheng.li(a)intel.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: TangYiwei
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Jincheng Li <jincheng.li(a)intel.com>
Gerrit-Attention: TangYiwei
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Wed, 17 Apr 2024 15:32:40 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Arthur Heymans, Chen, Gang C, Christian Walter, David Hendricks, Felix Held, Jincheng Li, Johnny Lin, Jonathan Zhang, Nico Huber, Patrick Rudolph, Paul Menzel, Shuo Liu, TangYiwei, Tim Chu.
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81316?usp=email )
Change subject: soc/intel/xeon_sp: Add Granite Rapids initial codes
......................................................................
Patch Set 46: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/81316?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: I3084e1b5abf25d8d9504bebeaed2a15b916ed56b
Gerrit-Change-Number: 81316
Gerrit-PatchSet: 46
Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Chen, Gang C <gang.c.chen(a)intel.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Jincheng Li <jincheng.li(a)intel.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: TangYiwei
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: TangYiwei
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Chen, Gang C <gang.c.chen(a)intel.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Jincheng Li <jincheng.li(a)intel.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Wed, 17 Apr 2024 15:32:22 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Martin L Roth.
Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81672?usp=email )
Change subject: util/crossgcc: Update CMake from 3.28.3 to 3.29.1
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
3.29.2 released...
As CMake is only used for LLVM, maybe wait when you are ready to upgrade LLVM then look for the latest CMake version.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81672?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: Iaf2d4f579d987fbfd4187ae41c1be5cec55e0e8e
Gerrit-Change-Number: 81672
Gerrit-PatchSet: 2
Gerrit-Owner: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Comment-Date: Wed, 17 Apr 2024 15:20:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment