Maxim Polyakov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47774 )
Change subject: ec/kontron/kempld: Add ACPI to access to control registers ......................................................................
ec/kontron/kempld: Add ACPI to access to control registers
Change-Id: If5088892936a48730d0d132b4ade1f3f600a9a40 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- A src/ec/kontron/kempld/acpi/cpld.asl 1 file changed, 29 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/47774/1
diff --git a/src/ec/kontron/kempld/acpi/cpld.asl b/src/ec/kontron/kempld/acpi/cpld.asl new file mode 100644 index 0000000..387af3a --- /dev/null +++ b/src/ec/kontron/kempld/acpi/cpld.asl @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#define KEMPLD_IO_BASE 0xa80 +#define KEMPLD_IO_RANGE 0xa81 + +Device (CPLD) +{ + Name (_HID, "KEU0001") // _HID: Hardware ID + Name (_UID, Zero) // _UID: Unique ID + + Method (_CRS, 0, Serialized) // _CRS: Current Resource Settings + { + Name (CPLR, ResourceTemplate () + { + IO (Decode16, + KEMPLD_IO_BASE, // Range Minimum + KEMPLD_IO_RANGE, // Range Maximum + 0, // Alignment + 2, // Length + ) + }) + Return (CPLR) + } + + Method (_STA, 0, Serialized) // _STA: Status + { + Return (0x0B) + } +}
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47774
to look at the new patch set (#3).
Change subject: ec/kontron/kempld: Add ACPI to access to internal registers ......................................................................
ec/kontron/kempld: Add ACPI to access to internal registers
Change-Id: If5088892936a48730d0d132b4ade1f3f600a9a40 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- A src/ec/kontron/kempld/acpi/cpld.asl 1 file changed, 29 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/47774/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47774 )
Change subject: ec/kontron/kempld: Add ACPI to access to internal registers ......................................................................
Patch Set 3:
(4 comments)
Sorry, but I don't see the point of this. I peeked at the follow-up, but it is equally reasoning-free. If there's a real reason to add this, please explain it.
https://review.coreboot.org/c/coreboot/+/47774/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47774/3//COMMIT_MSG@7 PS3, Line 7: to access to internal registers How? This only describes the device itself in ACPI
https://review.coreboot.org/c/coreboot/+/47774/3//COMMIT_MSG@8 PS3, Line 8: Please explain in the commit message what this change is actually useful for.
https://review.coreboot.org/c/coreboot/+/47774/3/src/ec/kontron/kempld/acpi/... File src/ec/kontron/kempld/acpi/cpld.asl:
https://review.coreboot.org/c/coreboot/+/47774/3/src/ec/kontron/kempld/acpi/... PS3, Line 9: Zero Did this come out of IASL?
https://review.coreboot.org/c/coreboot/+/47774/3/src/ec/kontron/kempld/acpi/... PS3, Line 19: 2 This is dependent on the #defined values above.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47774 )
Change subject: ec/kontron/kempld: Add ACPI to access to internal registers ......................................................................
Patch Set 3: Code-Review+1
Sorry, but I don't see the point of this. I peeked at the follow-up, but it is equally reasoning-free. If there's a real reason to add this, please explain it.
I was also confused by the commit message. Technically, what this does is advertising (and thereby reserving) the resource range _and_ (I guess that's the point) bind it to the `KEU0001` HID. This could then be picked up by an OS driver.
The HID is odd, though. As the Linux driver (`kempld-core`) uses `KEM0001`. So I wonder if this doesn't result in a warning when Linux loads the driver that can't find the ACPI part but uses the same resources? Or maybe not, because it sees that we don't use them in ASL? Dunno.
Anyway, I think this goes into the right direction. But we should check first if it really works.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47774 )
Change subject: ec/kontron/kempld: Add ACPI to access to internal registers ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47774/3/src/ec/kontron/kempld/acpi/... File src/ec/kontron/kempld/acpi/cpld.asl:
https://review.coreboot.org/c/coreboot/+/47774/3/src/ec/kontron/kempld/acpi/... PS3, Line 8: // _HID: Hardware ID I don't think we need these comments in every file.
https://review.coreboot.org/c/coreboot/+/47774/3/src/ec/kontron/kempld/acpi/... PS3, Line 13: Name (CPLR, ResourceTemplate () If you move this out of the method, it wouldn't need to be serialized. I'm not even sure if we need a named object at all, can't we
Return (ResourceTemplate () { IO (Decode16, ... ) })
https://review.coreboot.org/c/coreboot/+/47774/3/src/ec/kontron/kempld/acpi/... PS3, Line 25: Serialized Doesn't need to be serialized, does it?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47774 )
Change subject: ec/kontron/kempld: Add ACPI to access to internal registers ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47774/3/src/ec/kontron/kempld/acpi/... File src/ec/kontron/kempld/acpi/cpld.asl:
https://review.coreboot.org/c/coreboot/+/47774/3/src/ec/kontron/kempld/acpi/... PS3, Line 4: KEMPLD_IO_RANGE Just define the length and compute the range maximum as base + length - 1
Hello Felix Singer, build bot (Jenkins), Nico Huber, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47774
to look at the new patch set (#4).
Change subject: kontron: Reserve resources for EC/CPLD ......................................................................
kontron: Reserve resources for EC/CPLD
Use ACPI to reserve resources for the EC/CPLD device.
Change-Id: If5088892936a48730d0d132b4ade1f3f600a9a40 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- A src/ec/kontron/kempld/acpi/cpld.asl M src/mainboard/kontron/bsl6/dsdt.asl M src/mainboard/kontron/mal10/dsdt.asl 3 files changed, 37 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/47774/4