Christian Walter has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic.c: Assign resources behind device ......................................................................
superio/common/generic.c: Assign resources behind device
If multiple devices are behind a dev, we would only recognise port 0. We need to scan the complete 'bus' even though it is not a bridge.
Tested on ASpeed AST2500
Change-Id: Id80a2ae6e82c151b8d8adc9c5f35f38362d538fa Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/superio/common/generic.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/37607/1
diff --git a/src/superio/common/generic.c b/src/superio/common/generic.c index 429ee51..b954f36 100644 --- a/src/superio/common/generic.c +++ b/src/superio/common/generic.c @@ -21,6 +21,8 @@ { struct resource *res;
+ assign_resources(dev->link_list); + for (res = dev->resource_list; res; res = res->next) { if (!(res->flags & IORESOURCE_ASSIGNED)) continue;
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic.c: Assign resources behind device ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG@10 PS1, Line 10: need to scan the complete 'bus' even though it is not a bridge. If there is any device behind a device, the latter is a bridge by definition. The question is, do we need such a topology at all? For instance, do we need superio/aspeed/ast2400 devices behind the superio/common device? or could we have something like
chip superio/common device generic 2e.0 on end end chip superio/aspeed/ast2400 device pnp 2e.2 on ...
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic.c: Assign resources behind device ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG@7 PS1, Line 7: .c The suffix is not needed in the prefix.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic.c: Assign resources behind device ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG@10 PS1, Line 10: need to scan the complete 'bus' even though it is not a bridge.
If there is any device behind a device, the latter is a bridge by definition. […]
yes it's needed for SSDT generation to make sure the 2e.0 is the parent ACPI device of the LDN devices.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic.c: Assign resources behind device ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG@10 PS1, Line 10: need to scan the complete 'bus' even though it is not a bridge.
yes it's needed for SSDT generation to make sure the 2e. […]
Can you elaborate why it has to be the parent? i.e. why can't we do the same with a sibling device in ASL?
Hello Patrick Rudolph, Felix Held, Patrick Rudolph, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37607
to look at the new patch set (#2).
Change subject: superio/common/generic: Assign resources behind device ......................................................................
superio/common/generic: Assign resources behind device
If multiple devices are behind a dev, we would only recognise port 0. We need to scan the complete 'bus' even though it is not a bridge.
Tested on ASpeed AST2500
Change-Id: Id80a2ae6e82c151b8d8adc9c5f35f38362d538fa Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/superio/common/generic.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/37607/2
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic: Assign resources behind device ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG@7 PS1, Line 7: .c
The suffix is not needed in the prefix.
Ack
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic: Assign resources behind device ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG@10 PS1, Line 10: need to scan the complete 'bus' even though it is not a bridge.
Can you elaborate why it has to be the parent? i.e. why can't we do the same […]
1. There can be multiple siblings, we would need to reference one somehow (either in C or in ACPI) 2. Disabled devices do not generate SSDTs
With the parent/child approach that's not needed as there's only one parent, it's enabled by default and the ACPI path is known.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic: Assign resources behind device ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG@10 PS1, Line 10: need to scan the complete 'bus' even though it is not a bridge.
- There can be multiple siblings, we would need to reference one somehow (either in C or in ACPI) […]
I tried to find an example how the parent is accessed. There is none yet, right?
I have to admit, writing `"^^SMTH"` is easier than `"^^%s.SMTH", acpi_device_name(dev->...)`.
If we want hacks: It's possible to have a hierarchy in ASL but siblings in the devicetree: e.g. name the parent after the address "IO2E", and append that to the scope when writing the PNP devices.
If we don't want hacks: Please make this a proper bridge driver with .scan_bus and everything. assign_resources() is not the only thing missing here.
Hello Patrick Rudolph, Felix Held, Patrick Rudolph, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37607
to look at the new patch set (#3).
Change subject: superio/common/generic: Assign resources behind device ......................................................................
superio/common/generic: Assign resources behind device
If multiple devices are behind a dev, we would only recognise port 0. We need to scan the complete 'bus' even though it is not a bridge.
Tested on ASpeed AST2500
Change-Id: Id80a2ae6e82c151b8d8adc9c5f35f38362d538fa Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/superio/common/generic.c 1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/37607/3
Hello Patrick Rudolph, Felix Held, Patrick Rudolph, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37607
to look at the new patch set (#4).
Change subject: superio/common/generic: Assign resources behind device ......................................................................
superio/common/generic: Assign resources behind device
If multiple devices are behind a dev, we would only recognise port 0. We need to scan the complete 'bus' even though it is not a bridge.
Tested on ASpeed AST2500
Change-Id: Id80a2ae6e82c151b8d8adc9c5f35f38362d538fa Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/superio/common/generic.c 1 file changed, 3 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/37607/4
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic: Assign resources behind device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG@10 PS1, Line 10: need to scan the complete 'bus' even though it is not a bridge.
I tried to find an example how the parent is accessed. There is none […]
There's no user of that yet, I'll prepare a patch. Added .scanbus driver.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic: Assign resources behind device ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/37607/4/src/superio/common/generic.... File src/superio/common/generic.c:
https://review.coreboot.org/c/coreboot/+/37607/4/src/superio/common/generic.... PS4, Line 24: assign_resources(dev->link_list); Please check for null pointers. Also, this assumes there is only a single downstream bus. I guess that's ok for the expected use case.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic: Assign resources behind device ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG@10 PS1, Line 10: need to scan the complete 'bus' even though it is not a bridge.
There's no user of that yet, I'll prepare a patch. […]
Commit message still pretends that this is not a bridge.
Hello Patrick Rudolph, Felix Held, Patrick Rudolph, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37607
to look at the new patch set (#5).
Change subject: superio/common/generic: Assign resources behind device ......................................................................
superio/common/generic: Assign resources behind device
If multiple devices are behind a dev, we would only recognise port 0. We need to scan the complete 'bus'.
Tested on ASpeed AST2500
Change-Id: Id80a2ae6e82c151b8d8adc9c5f35f38362d538fa Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/superio/common/generic.c 1 file changed, 3 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/37607/5
Hello Patrick Rudolph, Felix Held, Patrick Rudolph, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37607
to look at the new patch set (#6).
Change subject: superio/common/generic: Assign resources behind device ......................................................................
superio/common/generic: Assign resources behind device
If multiple devices are behind a dev, we would only recognise port 0. We need to scan the complete 'bus'.
Tested on ASpeed AST2500
Change-Id: Id80a2ae6e82c151b8d8adc9c5f35f38362d538fa Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/superio/common/generic.c 1 file changed, 4 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/37607/6
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic: Assign resources behind device ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37607/1//COMMIT_MSG@10 PS1, Line 10: need to scan the complete 'bus' even though it is not a bridge.
Commit message still pretends that this is not a bridge.
Ack
https://review.coreboot.org/c/coreboot/+/37607/4/src/superio/common/generic.... File src/superio/common/generic.c:
https://review.coreboot.org/c/coreboot/+/37607/4/src/superio/common/generic.... PS4, Line 24: assign_resources(dev->link_list);
Please check for null pointers. Also, this assumes there is only a single […]
Ack
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic: Assign resources behind device ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37607/6/src/superio/common/generic.... File src/superio/common/generic.c:
https://review.coreboot.org/c/coreboot/+/37607/6/src/superio/common/generic.... PS6, Line 24: if (dev) We are called with a valid `dev` by contract. What can be NULL, depending on the devicetree, is `dev->link_list`.
Hello Patrick Rudolph, Felix Held, Patrick Rudolph, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37607
to look at the new patch set (#7).
Change subject: superio/common/generic: Assign resources behind device ......................................................................
superio/common/generic: Assign resources behind device
If multiple devices are behind a dev, we would only recognise port 0. We need to scan the complete 'bus'.
Tested on ASpeed AST2500
Change-Id: Id80a2ae6e82c151b8d8adc9c5f35f38362d538fa Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/superio/common/generic.c 1 file changed, 4 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/37607/7
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic: Assign resources behind device ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37607/6/src/superio/common/generic.... File src/superio/common/generic.c:
https://review.coreboot.org/c/coreboot/+/37607/6/src/superio/common/generic.... PS6, Line 24: if (dev)
We are called with a valid `dev` by contract. What can be NULL, depending on […]
Ack
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic: Assign resources behind device ......................................................................
Patch Set 8: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic: Assign resources behind device ......................................................................
Patch Set 8: Code-Review+2
Patrick Rudolph has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37607 )
Change subject: superio/common/generic: Assign resources behind device ......................................................................
superio/common/generic: Assign resources behind device
If multiple devices are behind a dev, we would only recognise port 0. We need to scan the complete 'bus'.
Tested on ASpeed AST2500
Change-Id: Id80a2ae6e82c151b8d8adc9c5f35f38362d538fa Signed-off-by: Christian Walter christian.walter@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37607 Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/superio/common/generic.c 1 file changed, 4 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Patrick Rudolph: Looks good to me, approved
diff --git a/src/superio/common/generic.c b/src/superio/common/generic.c index 429ee51..7ac1f83 100644 --- a/src/superio/common/generic.c +++ b/src/superio/common/generic.c @@ -21,6 +21,9 @@ { struct resource *res;
+ if (dev->link_list) + assign_resources(dev->link_list); + for (res = dev->resource_list; res; res = res->next) { if (!(res->flags & IORESOURCE_ASSIGNED)) continue; @@ -167,6 +170,7 @@ .read_resources = generic_read_resources, .set_resources = generic_set_resources, .enable_resources = DEVICE_NOOP, + .scan_bus = scan_static_bus, #if CONFIG(HAVE_ACPI_TABLES) .acpi_fill_ssdt_generator = generic_ssdt, .acpi_name = generic_acpi_name, @@ -182,11 +186,6 @@ else dev->ops = &ops;
- /* - * 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. - */ - enable_static_devices(dev); }
struct chip_operations superio_common_ops = {
-
Christian Walter (Code Review) -
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)