Change in ...coreboot[master]: device: Rename scan_static_bus() -> enable_static_devices()
Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31900
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
device: Rename scan_static_bus() -> enable_static_devices()
The new name should reflect better what this function does, as that is only one specific step of the scanning.
Change-Id: I9c9dc437b6117112bb28550855a2c38044dfbfa5 Signed-off-by: Nico Huber nico.huber@secunet.com --- M src/device/root_device.c M src/include/device/device.h M src/soc/intel/common/block/dsp/dsp.c M src/soc/intel/common/block/hda/hda.c 4 files changed, 8 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/31900/1
diff --git a/src/device/root_device.c b/src/device/root_device.c index f8e2907..8f2d446 100644 --- a/src/device/root_device.c +++ b/src/device/root_device.c @@ -26,7 +26,7 @@ const char mainboard_name[] = CONFIG_MAINBOARD_VENDOR " " CONFIG_MAINBOARD_PART_NUMBER;
/** - * Scan devices on static buses. + * Enable devices on static buses. * * The enumeration of certain buses is purely static. The existence of * devices on those buses can be completely determined at compile time @@ -43,7 +43,7 @@ * @param bus Pointer to the device to which the static buses are attached to. */
-void scan_static_bus(struct device *bus) +void enable_static_devices(struct device *bus) { struct device *child; struct bus *link; @@ -67,7 +67,7 @@ { printk(BIOS_SPEW, "%s for %s\n", __func__, dev_path(bus));
- scan_static_bus(bus); + enable_static_devices(bus);
printk(BIOS_SPEW, "%s for %s done\n", __func__, dev_path(bus)); } @@ -78,7 +78,7 @@
printk(BIOS_SPEW, "%s for %s\n", __func__, dev_path(bus));
- scan_static_bus(bus); + enable_static_devices(bus);
/* Scan bridges in case this device is a hub */ for (link = bus->link_list; link; link = link->next) @@ -136,7 +136,7 @@
printk(BIOS_SPEW, "%s for %s\n", __func__, dev_path(bus));
- scan_static_bus(bus); + enable_static_devices(bus);
for (link = bus->link_list; link; link = link->next) scan_bridges(link); diff --git a/src/include/device/device.h b/src/include/device/device.h index 2e2cda9..cbee197 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -278,9 +278,9 @@ DEVTREE_CONST struct device *pcidev_path_on_root(pci_devfn_t devfn); DEVTREE_CONST struct device *pcidev_on_root(uint8_t dev, uint8_t fn);
+void enable_static_devices(struct device *bus); void scan_smbus(struct device *bus); void scan_generic_bus(struct device *bus); -void scan_static_bus(struct device *bus); void scan_lpc_bus(struct device *bus); void scan_usb_bus(struct device *bus);
diff --git a/src/soc/intel/common/block/dsp/dsp.c b/src/soc/intel/common/block/dsp/dsp.c index 2bd6599..7f7eb90 100644 --- a/src/soc/intel/common/block/dsp/dsp.c +++ b/src/soc/intel/common/block/dsp/dsp.c @@ -23,7 +23,7 @@ .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, .ops_pci = &pci_dev_ops_pci, - .scan_bus = scan_static_bus, + .scan_bus = enable_static_devices, };
static const unsigned short pci_device_ids[] = { diff --git a/src/soc/intel/common/block/hda/hda.c b/src/soc/intel/common/block/hda/hda.c index 8ab835e..c1426e0 100644 --- a/src/soc/intel/common/block/hda/hda.c +++ b/src/soc/intel/common/block/hda/hda.c @@ -69,7 +69,7 @@ .init = hda_init, #endif .ops_pci = &pci_dev_ops_pci, - .scan_bus = scan_static_bus, + .scan_bus = enable_static_devices, };
static const unsigned short pci_device_ids[] = {
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31900 )
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31900/1/src/soc/intel/common/block/hda/hda.c File src/soc/intel/common/block/hda/hda.c:
https://review.coreboot.org/#/c/31900/1/src/soc/intel/common/block/hda/hda.c... PS1, Line 72: .scan_bus = enable_static_devices, Can we deal with a device that has two different kind of busses behind it?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31900 )
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
Patch Set 1: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31900 )
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31900/1/src/soc/intel/common/block/hda/hda.c File src/soc/intel/common/block/hda/hda.c:
https://review.coreboot.org/#/c/31900/1/src/soc/intel/common/block/hda/hda.c... PS1, Line 72: .scan_bus = enable_static_devices,
Can we deal with a device that has two different kind of busses behind it?
I guess yes. We have a list of links each with a list of downstream devices. And we handle that in the C code. The links don't know their "kind" so it would be hard not to support it. I don't know, though, if sconfig sup- ports it, but I'd guess yes, too.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31900 )
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31900/1/src/soc/intel/common/block/hda/hda.c File src/soc/intel/common/block/hda/hda.c:
https://review.coreboot.org/#/c/31900/1/src/soc/intel/common/block/hda/hda.c... PS1, Line 72: .scan_bus = enable_static_devices,
I guess yes. We have a list of links each with a list […]
We are dealing with static bus with at least one child? So we can evaluate the path type from that and pick scan function accordingly?
Which brings us to another question; do we ever actually define an override on .scan_bus, or do we currently only use it to determine a scan function to match the path type?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31900 )
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31900/1/src/soc/intel/common/block/hda/hda.c File src/soc/intel/common/block/hda/hda.c:
https://review.coreboot.org/#/c/31900/1/src/soc/intel/common/block/hda/hda.c... PS1, Line 72: .scan_bus = enable_static_devices,
We are dealing with static bus with at least one child? So we can evaluate the path type from that and pick scan function accordingly?
For "static", i.e. dt pre-enumerated, buses, yes. But it's not that our static scan functions would differ much. I see three cases:
1. enable_static_devices() * enables all downstream devices
2. scan_generic_bus() * numbers downstream links/buses * roughly same impl. as enable_static_devices()
3. root_dev_scan_bus() etc. * call enable_static_devices() * scan downstream buses
With the exception of potentially changing bus numbers, we could just squash them into one implementation. If my assessment is correct, the only troublemaker is dev_find_ slot_on_smbus(). It's not used much. Maybe I'll check the devicetrees if they are susceptible to bus number changes.
If necessary, we could then optimize based on the path type, e.g. skip scan_bridges() call for types where we don't expect further bridges.
Which brings us to another question; do we ever actually define an override on .scan_bus, or do we currently only use it to determine a scan function to match the path type?
Yes, we do. A grep shows me at least three different imple- mentations for path type PCI (assuming HT is reusing that type, too). Doesn't make you wrong; could also be that our enumeration of path types is incomplete?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31900 )
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
Patch Set 1: Code-Review-2
blocking this for now. no need to merge this if it's overrun tomorrow
Hello Kyösti Mälkki, Patrick Rudolph, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31900
to look at the new patch set (#2).
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
device: Rename scan_static_bus() -> enable_static_devices()
The new name should reflect better what this function does, as that is only one specific step of the scanning.
Change-Id: I9c9dc437b6117112bb28550855a2c38044dfbfa5 Signed-off-by: Nico Huber nico.huber@secunet.com --- M src/device/root_device.c M src/include/device/device.h M src/soc/intel/common/block/dsp/dsp.c M src/soc/intel/common/block/hda/hda.c M src/superio/common/generic.c 5 files changed, 9 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/31900/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31900 )
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
Patch Set 2: -Code-Review
Patch Set 1: Code-Review-2
blocking this for now. no need to merge this if it's overrun tomorrow
Of course I had the best intentions to look deeper into getting rid of .scan_bus() but didn't take the time ._.
Now I don't even recall what could have overrun this. The first three patches look sane to me. The first two simply rename and consolidate things. The third allows bridge devices behind LPC (actually I guess now we should just always scan bridges every- where? I don't see why not).
I'll look into cases that could rely on predictable numbers for I2C (SMBus) buses now, hoping to get rid of scan_generic_bus() and scan_smbus(), too.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31900 )
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
Patch Set 2: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31900 )
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/31900/2/src/device/root_device.c File src/device/root_device.c:
https://review.coreboot.org/c/coreboot/+/31900/2/src/device/root_device.c@46 PS2, Line 46: enable_static_devices If this is really enable devices, then shouldn't it be called as part of enable_dev() and not scan_bus()?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31900 )
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/31900/2/src/device/root_device.c File src/device/root_device.c:
https://review.coreboot.org/c/coreboot/+/31900/2/src/device/root_device.c@46 PS2, Line 46: enable_static_devices
If this is really enable devices, then shouldn't it be called as part of enable_dev() and not scan_b […]
It is. scan_bus() actually is supposed to perform both, it first enables all devices on a bus, then descends further via scan_bridges(). However, this implementation here does not descend and I think the name should reflect that.
My goal is to bring all the implementations here, step by step to a single scan_static_bus() that does everything right :) I'm not sure yet what to do with the secondary bus number handling in scan_generic_bus() below. Apart from that everything is straight forward (an additional scan_bridges() doesn't hurt if there are no downstream devices and if there are, it was a bug not to descend).
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31900 )
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
Patch Set 3: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31900 )
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
Patch Set 3: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31900 )
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/31900/2/src/device/root_device.c File src/device/root_device.c:
https://review.coreboot.org/c/coreboot/+/31900/2/src/device/root_device.c@46 PS2, Line 46: enable_static_devices
It is. scan_bus() actually is supposed to perform both, it first enables all […]
Resolved, I guess
https://review.coreboot.org/c/coreboot/+/31900/1/src/soc/intel/common/block/... File src/soc/intel/common/block/hda/hda.c:
https://review.coreboot.org/c/coreboot/+/31900/1/src/soc/intel/common/block/... PS1, Line 72: .scan_bus = enable_static_devices,
We are dealing with static bus with at least one child? So we can evaluate the path type from that […]
Resolved, I guess
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/31900 )
Change subject: device: Rename scan_static_bus() -> enable_static_devices() ......................................................................
device: Rename scan_static_bus() -> enable_static_devices()
The new name should reflect better what this function does, as that is only one specific step of the scanning.
Change-Id: I9c9dc437b6117112bb28550855a2c38044dfbfa5 Signed-off-by: Nico Huber nico.huber@secunet.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31900 Reviewed-by: Felix Held felix-coreboot@felixheld.de Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Patrick Rudolph siro@das-labor.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/device/root_device.c M src/include/device/device.h M src/soc/intel/common/block/dsp/dsp.c M src/soc/intel/common/block/hda/hda.c M src/superio/common/generic.c 5 files changed, 9 insertions(+), 9 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved Felix Held: Looks good to me, approved Patrick Rudolph: Looks good to me, approved
diff --git a/src/device/root_device.c b/src/device/root_device.c index beeead2..0580914 100644 --- a/src/device/root_device.c +++ b/src/device/root_device.c @@ -19,7 +19,7 @@ const char mainboard_name[] = CONFIG_MAINBOARD_VENDOR " " CONFIG_MAINBOARD_PART_NUMBER;
/** - * Scan devices on static buses. + * Enable devices on static buses. * * The enumeration of certain buses is purely static. The existence of * devices on those buses can be completely determined at compile time @@ -36,7 +36,7 @@ * @param bus Pointer to the device to which the static buses are attached to. */
-void scan_static_bus(struct device *bus) +void enable_static_devices(struct device *bus) { struct device *child; struct bus *link; @@ -60,7 +60,7 @@ { printk(BIOS_SPEW, "%s for %s\n", __func__, dev_path(bus));
- scan_static_bus(bus); + enable_static_devices(bus);
printk(BIOS_SPEW, "%s for %s done\n", __func__, dev_path(bus)); } @@ -71,7 +71,7 @@
printk(BIOS_SPEW, "%s for %s\n", __func__, dev_path(bus));
- scan_static_bus(bus); + enable_static_devices(bus);
/* Scan bridges in case this device is a hub */ for (link = bus->link_list; link; link = link->next) @@ -129,7 +129,7 @@
printk(BIOS_SPEW, "%s for %s\n", __func__, dev_path(bus));
- scan_static_bus(bus); + enable_static_devices(bus);
for (link = bus->link_list; link; link = link->next) scan_bridges(link); diff --git a/src/include/device/device.h b/src/include/device/device.h index 991bd38..96fc837 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -327,9 +327,9 @@ return config_of(pcidev_on_root(0, 0)); }
+void enable_static_devices(struct device *bus); void scan_smbus(struct device *bus); void scan_generic_bus(struct device *bus); -void scan_static_bus(struct device *bus); void scan_lpc_bus(struct device *bus); void scan_usb_bus(struct device *bus);
diff --git a/src/soc/intel/common/block/dsp/dsp.c b/src/soc/intel/common/block/dsp/dsp.c index d268893..277b6c5 100644 --- a/src/soc/intel/common/block/dsp/dsp.c +++ b/src/soc/intel/common/block/dsp/dsp.c @@ -23,7 +23,7 @@ .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, .ops_pci = &pci_dev_ops_pci, - .scan_bus = scan_static_bus, + .scan_bus = enable_static_devices, };
static const unsigned short pci_device_ids[] = { diff --git a/src/soc/intel/common/block/hda/hda.c b/src/soc/intel/common/block/hda/hda.c index 0cf9baa..be8319c 100644 --- a/src/soc/intel/common/block/hda/hda.c +++ b/src/soc/intel/common/block/hda/hda.c @@ -68,7 +68,7 @@ .init = hda_init, #endif .ops_pci = &pci_dev_ops_pci, - .scan_bus = scan_static_bus, + .scan_bus = enable_static_devices, };
static const unsigned short pci_device_ids[] = { diff --git a/src/superio/common/generic.c b/src/superio/common/generic.c index 96307a3..76122a1 100644 --- a/src/superio/common/generic.c +++ b/src/superio/common/generic.c @@ -187,7 +187,7 @@ * Need to call enable_dev() on the devices "behind" the Generic Super I/O. * coreboot's generic allocator doesn't expect them behind PnP devices. */ - scan_static_bus(dev); + enable_static_devices(dev); }
struct chip_operations superio_common_ops = {
-
Felix Held (Code Review) -
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)