Attention is currently required from: Angel Pons, Arthur Heymans, Felix Held, Krystian Hebel, Kyösti Mälkki, Michał Żygowski.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77338?usp=email )
Change subject: device/pciexp_device.c: Fix setting Max Payload Size ......................................................................
Patch Set 17: Code-Review+1
(11 comments)
File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/77338/comment/b0ff2479_c8c36687 : PS11, Line 598: awlays
Thanks for the update and your patience. […]
This ended up in the wrong thread, so closing here.
https://review.coreboot.org/c/coreboot/+/77338/comment/c813a81c_03703a19 : PS11, Line 630: root->max_payload_set = 1;
Ad.2. […]
Current implementation looks good to me. Krystian, Patrick, do you want to have another look?
File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/77338/comment/125ac8bc_50c74bdd : PS15, Line 638: pciexp_dev_set_max_payload_size(child, max_payload);
Indeed, we only care about the MPS propagation upwards. Removed.
Acknowledged
https://review.coreboot.org/c/coreboot/+/77338/comment/04aae74e_074de614 : PS15, Line 641: if (max_payload != child_max_payload) : printk(BIOS_INFO, "%s: Max_Payload_Size adjusted to %d\n", dev_path(child), : (1 << (max_payload + 7)));
Removed.
Acknowledged
https://review.coreboot.org/c/coreboot/+/77338/comment/76bcb193_3e6c9a23 : PS15, Line 691: };
The whole function is not needed at all actually. […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/77338/comment/aaf3f2af_868bcb91 : PS15, Line 728: pciexp_dev_set_max_payload_size(dev, pciexp_dev_get_max_payload_size_cap(dev));
Yes, limited it to express endpoints and legacy endpoints.
Acknowledged
https://review.coreboot.org/c/coreboot/+/77338/comment/b58846aa_4032625d : PS15, Line 740: pciexp_dev_set_max_payload_size(bus->dev, max_payload);
Right, not sure what I was thinking... Removed.
Acknowledged
File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/77338/comment/9af8cd9b_0cee147d : PS17, Line 651: } Nit, pciexp_dev_set_max_payload_size() might be a better place for this check.
https://review.coreboot.org/c/coreboot/+/77338/comment/3dd6cea0_dbe8cecb : PS17, Line 771: Paylaod Payl*oa*d
https://review.coreboot.org/c/coreboot/+/77338/comment/6d28084c_eb7e2825 : PS17, Line 774: dvices d*e*vices
https://review.coreboot.org/c/coreboot/+/77338/comment/d517d683_d28cb894 : PS17, Line 785: } Just a note: We could also have a special _scan_bus() function for root ports, that would save us the `if`. I'm not sure how much work this would be and if it would be really cleaner, so I'll leave it for me to look into later.