Change in coreboot[master]: ec/google/chromeec/acpi/superio: Add PS/2 Mouse ACPI entry

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Change in coreboot[master]: [WIP]...

Raul Rangel (Code Review)

21 May 2020 21 May '20

8:02 p.m.

Hello Martin Roth,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/41639

to review the following change.

Change subject: ec/google/chromeec/acpi/superio: Add PS/2 Mouse ACPI entry ......................................................................

ec/google/chromeec/acpi/superio: Add PS/2 Mouse ACPI entry

The PNP ID Means: PNP0F13 PS/2 Port for PS/2-style Mice

BUG=b:145575366 BRANCH=none TEST=Verified mouse was initialized

Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I2a4f071ad54730ea75f75ebf1633a4a08f7f2dd0 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Tested-by: Martin Roth martinroth@google.com Reviewed-by: Martin Roth martinroth@google.com Commit-Queue: Martin Roth martinroth@google.com --- M src/ec/google/chromeec/acpi/superio.asl 1 file changed, 27 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/41639/1

diff --git a/src/ec/google/chromeec/acpi/superio.asl b/src/ec/google/chromeec/acpi/superio.asl index cc91aba..05310be 100644 --- a/src/ec/google/chromeec/acpi/superio.asl +++ b/src/ec/google/chromeec/acpi/superio.asl @@ -157,3 +157,30 @@ } } #endif + +#ifdef SIO_EC_ENABLE_PS2M +Scope (_SB.PCI0) +{ + Device (PS2M) // Mouse + { + Name (_UID, 0) + Name (_HID, "GOOG0015") + Name (_CID, Package() { EISAID("PNP0F13") } ) + + Method (_STA, 0, NotSerialized) { + Return (0x0F) + } + + Name (_CRS, ResourceTemplate() + { + IO (Decode16, 0x60, 0x60, 0x01, 0x01) + IO (Decode16, 0x64, 0x64, 0x01, 0x01) +#ifdef SIO_EC_PS2M_IRQ + SIO_EC_PS2M_IRQ +#else + IRQ (Edge, ActiveHigh, Exclusive) {12} +#endif + }) + } +} +#endif

Attachments:

attachment.htm (text/html — 4.4 KB)

Show replies by date

Aaron Durbin (Code Review)

21 May 21 May

8:05 p.m.

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41639 )

Change subject: ec/google/chromeec/acpi/superio: Add PS/2 Mouse ACPI entry ......................................................................

Patch Set 1: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/41639 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a4f071ad54730ea75f75ebf1633a4a08f7f2dd0 Gerrit-Change-Number: 41639 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Comment-Date: Thu, 21 May 2020 23:05:58 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Felix Held (Code Review)

8:15 p.m.

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41639 )

Change subject: ec/google/chromeec/acpi/superio: Add PS/2 Mouse ACPI entry ......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41639/1/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/superio.asl:

https://review.coreboot.org/c/coreboot/+/41639/1/src/ec/google/chromeec/acpi... PS1, Line 176: IO (Decode16, 0x60, 0x60, 0x01, 0x01) : IO (Decode16, 0x64, 0x64, 0x01, 0x01) not sure if those should be added here again, since PS2K already uses those; ACPI might get unhappy about overlapping resources

https://review.coreboot.org/c/coreboot/+/41639/1/src/ec/google/chromeec/acpi... PS1, Line 184: } is it intended to not have a _PRS method here? the io resources are identical to the ones from PS2K, so they likely shouldn't be provided again, but the irq should be provided via _PRS

-- To view, visit https://review.coreboot.org/c/coreboot/+/41639 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a4f071ad54730ea75f75ebf1633a4a08f7f2dd0 Gerrit-Change-Number: 41639 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Comment-Date: Thu, 21 May 2020 23:15:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Raul Rangel (Code Review)

27 May 27 May

1:23 p.m.

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41639 )

Change subject: ec/google/chromeec/acpi/superio: Add PS/2 Mouse ACPI entry ......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41639/1/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/superio.asl:

https://review.coreboot.org/c/coreboot/+/41639/1/src/ec/google/chromeec/acpi... PS1, Line 176: IO (Decode16, 0x60, 0x60, 0x01, 0x01) : IO (Decode16, 0x64, 0x64, 0x01, 0x01)

...

not sure if those should be added here again, since PS2K already uses those; ACPI might get unhappy […]

The driver actually only uses either the PS2K or PS2M IO resources: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/th...

So they have to be identical.

https://review.coreboot.org/c/coreboot/+/41639/1/src/ec/google/chromeec/acpi... PS1, Line 184: }

...

is it intended to not have a _PRS method here? the io resources are identical to the ones from PS2K, […]

It was intentional. The linux kernel driver doesn't look at _PRS for resources, it only evaluates _CRS.

-- To view, visit https://review.coreboot.org/c/coreboot/+/41639 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a4f071ad54730ea75f75ebf1633a4a08f7f2dd0 Gerrit-Change-Number: 41639 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 27 May 2020 16:23:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Held felix-coreboot@felixheld.de Gerrit-MessageType: comment

Felix Held (Code Review)

1:28 p.m.

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41639 )

Change subject: ec/google/chromeec/acpi/superio: Add PS/2 Mouse ACPI entry ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41639/1/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/superio.asl:

https://review.coreboot.org/c/coreboot/+/41639/1/src/ec/google/chromeec/acpi... PS1, Line 184: }

...

It was intentional. […]

ok. would that cause problems when trying to boot windows on that machine?

-- To view, visit https://review.coreboot.org/c/coreboot/+/41639 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a4f071ad54730ea75f75ebf1633a4a08f7f2dd0 Gerrit-Change-Number: 41639 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 27 May 2020 16:28:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel rrangel@chromium.org Comment-In-Reply-To: Felix Held felix-coreboot@felixheld.de Gerrit-MessageType: comment

Raul Rangel (Code Review)

1:30 p.m.

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41639 )

Change subject: ec/google/chromeec/acpi/superio: Add PS/2 Mouse ACPI entry ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41639/1/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/superio.asl:

https://review.coreboot.org/c/coreboot/+/41639/1/src/ec/google/chromeec/acpi... PS1, Line 184: }

...

ok. […]

No idea. If it does we can add it then. Though I don't think it's required.

-- To view, visit https://review.coreboot.org/c/coreboot/+/41639 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a4f071ad54730ea75f75ebf1633a4a08f7f2dd0 Gerrit-Change-Number: 41639 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 27 May 2020 16:30:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel rrangel@chromium.org Comment-In-Reply-To: Felix Held felix-coreboot@felixheld.de Gerrit-MessageType: comment

Felix Held (Code Review)

2:09 p.m.

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41639 )

Change subject: ec/google/chromeec/acpi/superio: Add PS/2 Mouse ACPI entry ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41639/1/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/superio.asl:

https://review.coreboot.org/c/coreboot/+/41639/1/src/ec/google/chromeec/acpi... PS1, Line 184: }

...

No idea. If it does we can add it then. Though I don't think it's required.

-- To view, visit https://review.coreboot.org/c/coreboot/+/41639 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a4f071ad54730ea75f75ebf1633a4a08f7f2dd0 Gerrit-Change-Number: 41639 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 27 May 2020 17:09:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel rrangel@chromium.org Comment-In-Reply-To: Felix Held felix-coreboot@felixheld.de Gerrit-MessageType: comment

Felix Held (Code Review)

2:10 p.m.

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41639 )

Change subject: ec/google/chromeec/acpi/superio: Add PS/2 Mouse ACPI entry ......................................................................

ec/google/chromeec/acpi/superio: Add PS/2 Mouse ACPI entry

The PNP ID Means: PNP0F13 PS/2 Port for PS/2-style Mice

BUG=b:145575366 BRANCH=none TEST=Verified mouse was initialized

Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I2a4f071ad54730ea75f75ebf1633a4a08f7f2dd0 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Tested-by: Martin Roth martinroth@google.com Reviewed-by: Martin Roth martinroth@google.com Commit-Queue: Martin Roth martinroth@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/41639 Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/ec/google/chromeec/acpi/superio.asl 1 file changed, 27 insertions(+), 0 deletions(-)

Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved

-- To view, visit https://review.coreboot.org/c/coreboot/+/41639 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a4f071ad54730ea75f75ebf1633a4a08f7f2dd0 Gerrit-Change-Number: 41639 Gerrit-PatchSet: 2 Gerrit-Owner: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: merged

9elements QA (Code Review)

2:22 p.m.

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41639 )

Change subject: ec/google/chromeec/acpi/superio: Add PS/2 Mouse ACPI entry ......................................................................

Patch Set 2:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4205 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4204 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4203 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4202

Please note: This test is under development and might not be accurate at all!

-- To view, visit https://review.coreboot.org/c/coreboot/+/41639 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a4f071ad54730ea75f75ebf1633a4a08f7f2dd0 Gerrit-Change-Number: 41639 Gerrit-PatchSet: 2 Gerrit-Owner: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-Comment-Date: Wed, 27 May 2020 17:22:23 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Paul Menzel (Code Review)

3:15 p.m.

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41639 )

Change subject: ec/google/chromeec/acpi/superio: Add PS/2 Mouse ACPI entry ......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41639/2/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/superio.asl:

https://review.coreboot.org/c/coreboot/+/41639/2/src/ec/google/chromeec/acpi... PS2, Line 167: Name (_HID, "GOOG0015") Why is a separate HID needed?

https://review.coreboot.org/c/coreboot/+/41639/2/src/ec/google/chromeec/acpi... PS2, Line 185: } Why can’t `src/drivers/pc80/pc/ps2_controller.asl` be reused?

-- To view, visit https://review.coreboot.org/c/coreboot/+/41639 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a4f071ad54730ea75f75ebf1633a4a08f7f2dd0 Gerrit-Change-Number: 41639 Gerrit-PatchSet: 2 Gerrit-Owner: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 27 May 2020 18:15:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Raul Rangel (Code Review)

3:53 p.m.

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41639 )

Change subject: ec/google/chromeec/acpi/superio: Add PS/2 Mouse ACPI entry ......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41639/2/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/superio.asl:

https://review.coreboot.org/c/coreboot/+/41639/2/src/ec/google/chromeec/acpi... PS2, Line 167: Name (_HID, "GOOG0015")

...

Why is a separate HID needed?

Not strictly required. We don't have any kernel quirks based off of it.

https://review.coreboot.org/c/coreboot/+/41639/2/src/ec/google/chromeec/acpi... PS2, Line 185: }

...

Why can’t `src/drivers/pc80/pc/ps2_controller. […]

In theory it could, though have some #ifdefs

-- To view, visit https://review.coreboot.org/c/coreboot/+/41639 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a4f071ad54730ea75f75ebf1633a4a08f7f2dd0 Gerrit-Change-Number: 41639 Gerrit-PatchSet: 2 Gerrit-Owner: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 27 May 2020 18:53:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: comment

1792

days inactive

1798

days old

coreboot-gerrit@coreboot.org

10 comments

5 participants

tags (0)

participants (5)

9elements QA (Code Review)
Aaron Durbin (Code Review)
Felix Held (Code Review)
Paul Menzel (Code Review)
Raul Rangel (Code Review)