Change in ...coreboot[master]: ec/google/chromeec: Remove redundant use of ACPI offset operator

List overview All Threads
Download

newer

older

Change in em100[master]: Add...

Change in coreboot[master]:...

HAOUAS Elyes (Code Review)

2 Apr 2019 2 Apr '19

9:17 p.m.

HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32142

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

ec/google/chromeec: Remove redundant use of ACPI offset operator

Change-Id: Iedf67f1caafa9627491e8b8f91be69b551d07ae8 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/ec/google/chromeec/acpi/ec.asl M src/ec/google/chromeec/acpi/emem.asl 2 files changed, 0 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/32142/1

diff --git a/src/ec/google/chromeec/acpi/ec.asl b/src/ec/google/chromeec/acpi/ec.asl index 962988e..b6c2231 100644 --- a/src/ec/google/chromeec/acpi/ec.asl +++ b/src/ec/google/chromeec/acpi/ec.asl @@ -41,7 +41,6 @@ OperationRegion (ERAM, EmbeddedControl, 0x00, EC_ACPI_MEM_MAPPED_BEGIN) Field (ERAM, ByteAcc, Lock, Preserve) { - Offset (0x00), RAMV, 8, // EC RAM Version TSTB, 8, // Test Byte TSTC, 8, // Complement of Test Byte @@ -54,7 +53,6 @@ TBMD, 1, // Tablet mode DDPN, 3, // Device DPTF Profile Number // DFUD must be 0 for the other 31 values to be valid - Offset (0x0a), DFUD, 1, // Device Features Undefined FLSH, 1, // Flash commands present PFAN, 1, // PWM Fan control present @@ -88,7 +86,6 @@ RWSG, 1, // EC has RWSIG task enabled DEVE, 1, // EC supports device events // make sure we're within our space envelope - Offset (0x0e), Offset (0x12), BTID, 8, // Battery index that host wants to read USPP, 8, // USB Port Power diff --git a/src/ec/google/chromeec/acpi/emem.asl b/src/ec/google/chromeec/acpi/emem.asl index 982ec5b..77b4708 100644 --- a/src/ec/google/chromeec/acpi/emem.asl +++ b/src/ec/google/chromeec/acpi/emem.asl @@ -17,7 +17,6 @@ * EMEM data may be accessed through port 62/66 or through LPC at 900h. */

-Offset (0x00), TIN0, 8, // Temperature 0 TIN1, 8, // Temperature 1 TIN2, 8, // Temperature 2

Attachments:

attachment.htm (text/html — 3.8 KB)

Show replies by date

Furquan Shaikh (Code Review)

2 Apr 2 Apr

9:31 p.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32142 )

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/32142/1/src/ec/google/chromeec/acpi/ec.asl File src/ec/google/chromeec/acpi/ec.asl:

https://review.coreboot.org/#/c/32142/1/src/ec/google/chromeec/acpi/ec.asl@a... PS1, Line 57: I don't think this is redundant.

https://review.coreboot.org/#/c/32142/1/src/ec/google/chromeec/acpi/emem.asl File src/ec/google/chromeec/acpi/emem.asl:

https://review.coreboot.org/#/c/32142/1/src/ec/google/chromeec/acpi/emem.asl... PS1, Line 61: Offset (0x80), This seems redundant?

HAOUAS Elyes (Code Review)

9:49 p.m.

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32142 )

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/32142/1/src/ec/google/chromeec/acpi/ec.asl File src/ec/google/chromeec/acpi/ec.asl:

https://review.coreboot.org/#/c/32142/1/src/ec/google/chromeec/acpi/ec.asl@a... PS1, Line 57:

...

I don't think this is redundant.

you are right ! thank you

https://review.coreboot.org/#/c/32142/1/src/ec/google/chromeec/acpi/emem.asl File src/ec/google/chromeec/acpi/emem.asl:

https://review.coreboot.org/#/c/32142/1/src/ec/google/chromeec/acpi/emem.asl... PS1, Line 61: Offset (0x80),

...

This seems redundant?

Yes, Thank you

-- To view, visit https://review.coreboot.org/c/coreboot/+/32142 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iedf67f1caafa9627491e8b8f91be69b551d07ae8 Gerrit-Change-Number: 32142 Gerrit-PatchSet: 1 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Tue, 02 Apr 2019 19:49:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

9:52 p.m.

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#2).

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

ec/google/chromeec: Remove redundant use of ACPI offset operator

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/32142/2

Furquan Shaikh (Code Review)

9:55 p.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32142 )

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32142/2/src/ec/google/chromeec/acpi/ec.asl File src/ec/google/chromeec/acpi/ec.asl:

https://review.coreboot.org/#/c/32142/2/src/ec/google/chromeec/acpi/ec.asl@a... PS2, Line 90: Looking back at this, it seems like this was done to ensure that we don't overrun the device features. Probably we can add DUMY, 1 after Offset(0x0e) to maintain the original intent. Thoughts?

Also adding Patrick who had added this code.

Patrick Georgi (Code Review)

10:05 p.m.

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32142 )

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32142/2/src/ec/google/chromeec/acpi/ec.asl File src/ec/google/chromeec/acpi/ec.asl:

https://review.coreboot.org/#/c/32142/2/src/ec/google/chromeec/acpi/ec.asl@a... PS2, Line 90:

...

Looking back at this, it seems like this was done to ensure that we don't overrun the device feature […]

It's unfortunate that the iasl devs now consider redundant Offsets worthy of a warning or error. I wonder if we should push for some kind of location assert to make the distinction explicit?

-- To view, visit https://review.coreboot.org/c/coreboot/+/32142 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iedf67f1caafa9627491e8b8f91be69b551d07ae8 Gerrit-Change-Number: 32142 Gerrit-PatchSet: 2 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Tue, 02 Apr 2019 20:05:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

10:07 p.m.

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32142 )

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32142/2/src/ec/google/chromeec/acpi/ec.asl File src/ec/google/chromeec/acpi/ec.asl:

https://review.coreboot.org/#/c/32142/2/src/ec/google/chromeec/acpi/ec.asl@a... PS2, Line 90:

...

It's unfortunate that the iasl devs now consider redundant Offsets worthy of a warning or error. […]

I think IASL will warn and will not "compile" redundant offset.

-- To view, visit https://review.coreboot.org/c/coreboot/+/32142 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iedf67f1caafa9627491e8b8f91be69b551d07ae8 Gerrit-Change-Number: 32142 Gerrit-PatchSet: 2 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Tue, 02 Apr 2019 20:07:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Georgi pgeorgi@google.com Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

10:13 p.m.

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32142 )

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

Patch Set 2:

...

(1 comment)

acpica/documents/changes.txt: "iASL: Implemented detection of extraneous/redundant uses of the Offset() operator within a Field Unit list. A remark is now issued for these. For example, the first two of the Offset() operators below are extraneous. Because both the compiler and the interpreter track the offsets automatically, these Offsets simply refer to the current offset and are unnecessary. Note, when optimization is enabled, the iASL compiler will in fact remove the redundant Offset operators and will not emit any AML code for them"

-- To view, visit https://review.coreboot.org/c/coreboot/+/32142 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iedf67f1caafa9627491e8b8f91be69b551d07ae8 Gerrit-Change-Number: 32142 Gerrit-PatchSet: 2 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Tue, 02 Apr 2019 20:13:29 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Martin Roth (Code Review)

3 Apr 3 Apr

4:46 a.m.

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32142 )

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

Patch Set 2:

The redundant offsets should not cause any build failures - they're just remarks.

dsdt.asl 63: Offset (0x00), Remark 2158 - ^ Unnecessary/redundant use of Offset operator

https://qa.coreboot.org/job/coreboot-toolchain/538/testReport/junit/board/ch...

-- To view, visit https://review.coreboot.org/c/coreboot/+/32142 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iedf67f1caafa9627491e8b8f91be69b551d07ae8 Gerrit-Change-Number: 32142 Gerrit-PatchSet: 2 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Martin Roth martinroth@google.com Gerrit-Comment-Date: Wed, 03 Apr 2019 02:46:38 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Martin Roth (Code Review)

4:48 a.m.

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32142 )

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32142/2/src/ec/google/chromeec/acpi/ec.asl File src/ec/google/chromeec/acpi/ec.asl:

https://review.coreboot.org/#/c/32142/2/src/ec/google/chromeec/acpi/ec.asl@a... PS2, Line 90:

...

I think IASL will warn and will not "compile" redundant offset.

I replied at the top level, but I'll add it here too. These are just remarks, not warnings or errors, so the build should not fail because of them.

-- To view, visit https://review.coreboot.org/c/coreboot/+/32142 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iedf67f1caafa9627491e8b8f91be69b551d07ae8 Gerrit-Change-Number: 32142 Gerrit-PatchSet: 2 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Martin Roth martinroth@google.com Gerrit-Comment-Date: Wed, 03 Apr 2019 02:48:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: HAOUAS Elyes ehaouas@noos.fr Comment-In-Reply-To: Patrick Georgi pgeorgi@google.com Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

6:47 a.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32142 )

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32142/2/src/ec/google/chromeec/acpi/ec.asl File src/ec/google/chromeec/acpi/ec.asl:

https://review.coreboot.org/#/c/32142/2/src/ec/google/chromeec/acpi/ec.asl@a... PS2, Line 90: As Martin indicated, those are just remarks and do not fail compilation.

...

I wonder if we should push for some kind of location assert to make the distinction explicit?

That would be ideal!

-- To view, visit https://review.coreboot.org/c/coreboot/+/32142 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iedf67f1caafa9627491e8b8f91be69b551d07ae8 Gerrit-Change-Number: 32142 Gerrit-PatchSet: 2 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Martin Roth martinroth@google.com Gerrit-Comment-Date: Wed, 03 Apr 2019 04:47:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: HAOUAS Elyes ehaouas@noos.fr Comment-In-Reply-To: Martin Roth martinroth@google.com Comment-In-Reply-To: Patrick Georgi pgeorgi@google.com Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

10:20 p.m.

Hello build bot (Jenkins), Patrick Georgi,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#3).

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

ec/google/chromeec: Remove redundant use of ACPI offset operator

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/32142/3

Patrick Georgi (Code Review)

4 Apr 4 Apr

11:19 p.m.

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32142 )

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/32142/2/src/ec/google/chromeec/acpi/ec.asl File src/ec/google/chromeec/acpi/ec.asl:

https://review.coreboot.org/#/c/32142/2/src/ec/google/chromeec/acpi/ec.asl@a... PS2, Line 90:

...

As Martin indicated, those are just remarks and do not fail compilation. […]

I started a discussion over at acpica (https://lists.acpica.org/pipermail/devel/2019-April/001880.html), let's see what happens

-- To view, visit https://review.coreboot.org/c/coreboot/+/32142 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iedf67f1caafa9627491e8b8f91be69b551d07ae8 Gerrit-Change-Number: 32142 Gerrit-PatchSet: 3 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Martin Roth martinroth@google.com Gerrit-Comment-Date: Thu, 04 Apr 2019 21:19:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: HAOUAS Elyes ehaouas@noos.fr Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Martin Roth martinroth@google.com Comment-In-Reply-To: Patrick Georgi pgeorgi@google.com Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

24 Oct 24 Oct

8:03 a.m.

New subject: Change in coreboot[master]: ec/google/chromeec: Remove redundant use of ACPI offset operator

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32142 )

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

Patch Set 3:

do you have any update ? I'm ok, to drop this, as it is just a 'Remark' and not a warning or an error.

-- To view, visit https://review.coreboot.org/c/coreboot/+/32142 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iedf67f1caafa9627491e8b8f91be69b551d07ae8 Gerrit-Change-Number: 32142 Gerrit-PatchSet: 3 Gerrit-Owner: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Martin Roth martinroth@google.com Gerrit-Comment-Date: Thu, 24 Oct 2019 06:03:48 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

14 Nov 14 Nov

6:16 p.m.

New subject: Change in coreboot[master]: ec/google/chromeec: Remove redundant use of ACPI offset operator

HAOUAS Elyes has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/32142 )

Change subject: ec/google/chromeec: Remove redundant use of ACPI offset operator ......................................................................

Abandoned

see https://review.coreboot.org/c/coreboot/+/36857

1836

days inactive

2062

days old

coreboot-gerrit@coreboot.org

14 comments

4 participants

tags (0)

participants (4)

Furquan Shaikh (Code Review)
HAOUAS Elyes (Code Review)
Martin Roth (Code Review)
Patrick Georgi (Code Review)