Change in ...coreboot[master]: device/root_device: Consolidate common _scan_bus() functions
Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31901
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
device/root_device: Consolidate common _scan_bus() functions
scan_usb_bus() and root_dev_scan_bus() had the very same implementation. So rename the latter to scan_static_bus() and use that for both cases.
Change-Id: If0aba9c690b23e3716f2d47ff7a8c3e8f6d82679 Signed-off-by: Nico Huber nico.huber@secunet.com --- M src/device/root_device.c M src/drivers/usb/acpi/usb_acpi.c M src/include/device/device.h M src/soc/amd/stoneyridge/usb.c M src/soc/intel/common/block/xhci/xhci.c 5 files changed, 6 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/31901/1
diff --git a/src/device/root_device.c b/src/device/root_device.c index 8f2d446..2fae5c3 100644 --- a/src/device/root_device.c +++ b/src/device/root_device.c @@ -72,21 +72,6 @@ printk(BIOS_SPEW, "%s for %s done\n", __func__, dev_path(bus)); }
-void scan_usb_bus(struct device *bus) -{ - struct bus *link; - - printk(BIOS_SPEW, "%s for %s\n", __func__, dev_path(bus)); - - enable_static_devices(bus); - - /* Scan bridges in case this device is a hub */ - for (link = bus->link_list; link; link = link->next) - scan_bridges(link); - - printk(BIOS_SPEW, "%s for %s done\n", __func__, dev_path(bus)); -} - void scan_generic_bus(struct device *bus) { struct device *child; @@ -123,14 +108,7 @@ scan_generic_bus(bus); }
-/** - * Scan root bus for generic systems. - * - * This function is the default scan_bus() method of the root device. - * - * @param root The root device structure. - */ -static void root_dev_scan_bus(struct device *bus) +void scan_static_bus(struct device *bus) { struct bus *link;
@@ -169,7 +147,7 @@ .set_resources = DEVICE_NOOP, .enable_resources = DEVICE_NOOP, .init = DEVICE_NOOP, - .scan_bus = root_dev_scan_bus, + .scan_bus = scan_static_bus, .reset_bus = root_dev_reset, #if CONFIG(HAVE_ACPI_TABLES) .acpi_name = root_dev_acpi_name, diff --git a/src/drivers/usb/acpi/usb_acpi.c b/src/drivers/usb/acpi/usb_acpi.c index 8c63d2a..e026eeb 100644 --- a/src/drivers/usb/acpi/usb_acpi.c +++ b/src/drivers/usb/acpi/usb_acpi.c @@ -86,7 +86,7 @@ .read_resources = DEVICE_NOOP, .set_resources = DEVICE_NOOP, .enable_resources = DEVICE_NOOP, - .scan_bus = scan_usb_bus, + .scan_bus = scan_static_bus, .acpi_fill_ssdt_generator = usb_acpi_fill_ssdt_generator, };
diff --git a/src/include/device/device.h b/src/include/device/device.h index cbee197..02fb072 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -282,7 +282,7 @@ void scan_smbus(struct device *bus); void scan_generic_bus(struct device *bus); void scan_lpc_bus(struct device *bus); -void scan_usb_bus(struct device *bus); +void scan_static_bus(struct device *bus);
#endif /* !defined(__ROMCC__) */
diff --git a/src/soc/amd/stoneyridge/usb.c b/src/soc/amd/stoneyridge/usb.c index f2fa3ba..2eedd97 100644 --- a/src/soc/amd/stoneyridge/usb.c +++ b/src/soc/amd/stoneyridge/usb.c @@ -63,7 +63,7 @@ .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, .init = set_usb_over_current, - .scan_bus = scan_usb_bus, + .scan_bus = scan_static_bus, .acpi_name = soc_acpi_name, .ops_pci = &lops_pci, }; diff --git a/src/soc/intel/common/block/xhci/xhci.c b/src/soc/intel/common/block/xhci/xhci.c index c429e7d..95315b2 100644 --- a/src/soc/intel/common/block/xhci/xhci.c +++ b/src/soc/intel/common/block/xhci/xhci.c @@ -28,7 +28,7 @@ .enable_resources = pci_dev_enable_resources, .init = soc_xhci_init, .ops_pci = &pci_dev_ops_pci, - .scan_bus = scan_usb_bus, + .scan_bus = scan_static_bus, #if CONFIG(HAVE_ACPI_TABLES) .acpi_name = soc_acpi_name, #endif
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31901 )
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31901/1/src/device/root_device.c File src/device/root_device.c:
https://review.coreboot.org/#/c/31901/1/src/device/root_device.c@115 PS1, Line 115: printk(BIOS_SPEW, "%s for %s\n", __func__, dev_path(bus)); Someone might want that __func__ to expand to the old strings in logs, that's my motivation for inlines with the names. Maybe mention this in the commit message, or better, not change log output?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31901 )
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31901/1/src/device/root_device.c File src/device/root_device.c:
https://review.coreboot.org/#/c/31901/1/src/device/root_device.c@115 PS1, Line 115: printk(BIOS_SPEW, "%s for %s\n", __func__, dev_path(bus));
Someone might want that __func__ to expand to the old strings in logs, that's my motivation for inli […]
Is that what you have in mind / would work:
static __always_inline void scan_static_bus(struct device *bus) { printk(..., "%s...", __func__, ...); ... }
void root_dev_scan_bus(struct device *bus) { scan_static_bus(bus); }
void scan_usb_bus(struct device *bus) { scan_static_bus(bus); }
?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31901 )
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31901/1/src/device/root_device.c File src/device/root_device.c:
https://review.coreboot.org/#/c/31901/1/src/device/root_device.c@115 PS1, Line 115: printk(BIOS_SPEW, "%s for %s\n", __func__, dev_path(bus));
Is that what you have in mind / would work:
No, the other way around.
void scan_static_bus(struct device *bus, const char *realname) { printk(..., "%s...", realname, ...); ... }
void root_dev_scan_bus(struct device *bus) { scan_static_bus(bus, __func__); }
void scan_usb_bus(struct device *bus) { scan_static_bus(bus, __func__); }
They way they are called made me realise the wrappers would not inline.
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31901
to look at the new patch set (#2).
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
device/root_device: Consolidate common _scan_bus() functions
scan_usb_bus() and root_dev_scan_bus() had the very same implementation. So rename the latter to scan_static_bus() and use that for both cases.
Change-Id: If0aba9c690b23e3716f2d47ff7a8c3e8f6d82679 Signed-off-by: Nico Huber nico.huber@secunet.com --- M src/device/root_device.c M src/drivers/usb/acpi/usb_acpi.c M src/include/device/device.h M src/soc/amd/picasso/usb.c M src/soc/amd/stoneyridge/usb.c M src/soc/intel/common/block/xhci/xhci.c 6 files changed, 7 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/31901/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31901 )
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
Patch Set 2: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31901 )
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/31901/2/src/device/root_device.c File src/device/root_device.c:
https://review.coreboot.org/c/coreboot/+/31901/2/src/device/root_device.c@a1... PS2, Line 126: /** : * Scan root bus for generic systems. : * : * This function is the default scan_bus() method of the root device. : * : * @param root The root device structure. : */ Why was the comment removed? I think it would be helpful to have a comment(if not here, then in device.h) indicating when this is supposed to be used.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31901 )
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
Patch Set 3: Code-Review+2
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31901
to look at the new patch set (#4).
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
device/root_device: Consolidate common _scan_bus() functions
scan_usb_bus() and root_dev_scan_bus() had the very same implementation. So rename the latter to scan_static_bus() and use that for both cases.
Change-Id: If0aba9c690b23e3716f2d47ff7a8c3e8f6d82679 Signed-off-by: Nico Huber nico.huber@secunet.com --- M 3rdparty/arm-trusted-firmware M 3rdparty/chromeec M 3rdparty/libgfxinit M 3rdparty/opensbi M 3rdparty/vboot M src/device/root_device.c M src/drivers/usb/acpi/usb_acpi.c M src/include/device/device.h M src/soc/amd/picasso/usb.c M src/soc/amd/stoneyridge/usb.c M src/soc/intel/common/block/xhci/xhci.c 11 files changed, 19 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/31901/4
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31901 )
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/31901/1/src/device/root_device.c File src/device/root_device.c:
https://review.coreboot.org/c/coreboot/+/31901/1/src/device/root_device.c@11... PS1, Line 115: printk(BIOS_SPEW, "%s for %s\n", __func__, dev_path(bus));
Is that what you have in mind / would work: […]
I was very undecided already (because it seems odd to call something scan_xx_bus() when xx has no means of scanning). Then I realized that this gets even worse with scan_usb_bus(). USB is a bus that can be scanned, but that will never be what this function does. IOW, I would change the name anyway, even if we'd keep USB in the name.
So to avoid future confusion, I hope we can just forget that these had different names?
https://review.coreboot.org/c/coreboot/+/31901/2/src/device/root_device.c File src/device/root_device.c:
https://review.coreboot.org/c/coreboot/+/31901/2/src/device/root_device.c@a1... PS2, Line 126: /** : * Scan root bus for generic systems. : * : * This function is the default scan_bus() method of the root device. : * : * @param root The root device structure. : */
Why was the comment removed? I think it would be helpful to have a comment(if not here, then in devi […]
Ack. I wrote a new comment. I wanted to see where this patch series leads us first. And hoped to integrate scan_smbus() as well. But that's stalled by CB:35456 which I don't like to finish because it will likely be obsolete after next month's code removal.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31901 )
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
Patch Set 4: Code-Review+2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31901 )
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
Patch Set 4:
unrelated to the patch itself, but something with the coreboot git submodules went wrong on this one
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31901
to look at the new patch set (#5).
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
device/root_device: Consolidate common _scan_bus() functions
scan_usb_bus() and root_dev_scan_bus() had the very same implementation. So rename the latter to scan_static_bus() and use that for both cases.
Change-Id: If0aba9c690b23e3716f2d47ff7a8c3e8f6d82679 Signed-off-by: Nico Huber nico.huber@secunet.com --- M src/device/root_device.c M src/drivers/usb/acpi/usb_acpi.c M src/include/device/device.h M src/soc/amd/picasso/usb.c M src/soc/amd/stoneyridge/usb.c M src/soc/intel/common/block/xhci/xhci.c 6 files changed, 14 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/31901/5
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31901 )
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
Patch Set 5:
unrelated to the patch itself, but something with the coreboot git submodules went wrong on this one
Thanks!
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31901 )
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
Patch Set 5: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/31901 )
Change subject: device/root_device: Consolidate common _scan_bus() functions ......................................................................
device/root_device: Consolidate common _scan_bus() functions
scan_usb_bus() and root_dev_scan_bus() had the very same implementation. So rename the latter to scan_static_bus() and use that for both cases.
Change-Id: If0aba9c690b23e3716f2d47ff7a8c3e8f6d82679 Signed-off-by: Nico Huber nico.huber@secunet.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31901 Reviewed-by: Felix Held felix-coreboot@felixheld.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/device/root_device.c M src/drivers/usb/acpi/usb_acpi.c M src/include/device/device.h M src/soc/amd/picasso/usb.c M src/soc/amd/stoneyridge/usb.c M src/soc/intel/common/block/xhci/xhci.c 6 files changed, 14 insertions(+), 26 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/src/device/root_device.c b/src/device/root_device.c index 0580914..84d3ba6 100644 --- a/src/device/root_device.c +++ b/src/device/root_device.c @@ -65,21 +65,6 @@ printk(BIOS_SPEW, "%s for %s done\n", __func__, dev_path(bus)); }
-void scan_usb_bus(struct device *bus) -{ - struct bus *link; - - printk(BIOS_SPEW, "%s for %s\n", __func__, dev_path(bus)); - - enable_static_devices(bus); - - /* Scan bridges in case this device is a hub */ - for (link = bus->link_list; link; link = link->next) - scan_bridges(link); - - printk(BIOS_SPEW, "%s for %s done\n", __func__, dev_path(bus)); -} - void scan_generic_bus(struct device *bus) { struct device *child; @@ -116,14 +101,17 @@ scan_generic_bus(bus); }
-/** - * Scan root bus for generic systems. +/* + * Default scan_bus() implementation * - * This function is the default scan_bus() method of the root device. + * This is the default implementation for buses that can't + * be probed at runtime. It simply walks through the topology + * given by the mainboard's `devicetree.cb`. * - * @param root The root device structure. + * First, all direct descendants of the given device are + * enabled. Then, downstream buses are scanned. */ -static void root_dev_scan_bus(struct device *bus) +void scan_static_bus(struct device *bus) { struct bus *link;
@@ -162,7 +150,7 @@ .set_resources = DEVICE_NOOP, .enable_resources = DEVICE_NOOP, .init = DEVICE_NOOP, - .scan_bus = root_dev_scan_bus, + .scan_bus = scan_static_bus, .reset_bus = root_dev_reset, #if CONFIG(HAVE_ACPI_TABLES) .acpi_name = root_dev_acpi_name, diff --git a/src/drivers/usb/acpi/usb_acpi.c b/src/drivers/usb/acpi/usb_acpi.c index 31a7a7f..2f2ad32 100644 --- a/src/drivers/usb/acpi/usb_acpi.c +++ b/src/drivers/usb/acpi/usb_acpi.c @@ -85,7 +85,7 @@ .read_resources = DEVICE_NOOP, .set_resources = DEVICE_NOOP, .enable_resources = DEVICE_NOOP, - .scan_bus = scan_usb_bus, + .scan_bus = scan_static_bus, .acpi_fill_ssdt_generator = usb_acpi_fill_ssdt_generator, };
diff --git a/src/include/device/device.h b/src/include/device/device.h index 96fc837..a7ba5a9 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -331,7 +331,7 @@ void scan_smbus(struct device *bus); void scan_generic_bus(struct device *bus); void scan_lpc_bus(struct device *bus); -void scan_usb_bus(struct device *bus); +void scan_static_bus(struct device *bus);
#endif /* !defined(__ROMCC__) */
diff --git a/src/soc/amd/picasso/usb.c b/src/soc/amd/picasso/usb.c index 66c8266..831b470 100644 --- a/src/soc/amd/picasso/usb.c +++ b/src/soc/amd/picasso/usb.c @@ -60,7 +60,7 @@ .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, .init = set_usb_over_current, - .scan_bus = scan_usb_bus, + .scan_bus = scan_static_bus, .acpi_name = soc_acpi_name, .ops_pci = &lops_pci, }; diff --git a/src/soc/amd/stoneyridge/usb.c b/src/soc/amd/stoneyridge/usb.c index 00f8237..3c62191 100644 --- a/src/soc/amd/stoneyridge/usb.c +++ b/src/soc/amd/stoneyridge/usb.c @@ -63,7 +63,7 @@ .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, .init = set_usb_over_current, - .scan_bus = scan_usb_bus, + .scan_bus = scan_static_bus, .acpi_name = soc_acpi_name, .ops_pci = &lops_pci, }; diff --git a/src/soc/intel/common/block/xhci/xhci.c b/src/soc/intel/common/block/xhci/xhci.c index c5c5e6c..293fd36 100644 --- a/src/soc/intel/common/block/xhci/xhci.c +++ b/src/soc/intel/common/block/xhci/xhci.c @@ -113,7 +113,7 @@ .enable_resources = pci_dev_enable_resources, .init = soc_xhci_init, .ops_pci = &pci_dev_ops_pci, - .scan_bus = scan_usb_bus, + .scan_bus = scan_static_bus, #if CONFIG(HAVE_ACPI_TABLES) .acpi_name = soc_acpi_name, #endif
-
Felix Held (Code Review) -
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)