Change in coreboot[master]: drivers/intel/gma/edid: Use byte swap function

[Paul Menzel (Code Review)]

Paul Menzel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38647 )


Change subject: drivers/intel/gma/edid: Use byte swap function
......................................................................

drivers/intel/gma/edid: Use byte swap function

Instead of open coding the byte swapping (endianess), use the built-in.

The compiler can use the instruction `bswap` now. As EDID data is not
big, the difference in execution time won’t be measurable.

With gcc (Debian 9.2.1-25) 9.2.1 20200123:

     .LBE128:
     .LBE127:
    -       .loc 1 94 3 is_stmt 1 discriminator 3 view .LVU157
    -       addl    $4, %ebp
    -       .loc 1 94 15 is_stmt 0 discriminator 3 view .LVU158
    -       movb    %al, -4(%ebp)
    -       .loc 1 95 3 is_stmt 1 discriminator 3 view .LVU159
    -       .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU160
    -       movl    %eax, %edx
    -       .loc 1 95 19 discriminator 3 view .LVU161
    -       movb    %ah, -3(%ebp)
    -       .loc 1 96 3 is_stmt 1 discriminator 3 view .LVU162
    -       .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU163
    -       shrl    $16, %edx
    -       .loc 1 96 19 discriminator 3 view .LVU164
    -       movb    %dl, -2(%ebp)
    -       .loc 1 97 3 is_stmt 1 discriminator 3 view .LVU165
    -       .loc 1 97 35 is_stmt 0 discriminator 3 view .LVU166
    -       shrl    $24, %eax
    +       .loc 1 96 3 is_stmt 1 discriminator 3 view .LVU159
    +       .loc 1 96 15 is_stmt 0 discriminator 3 view .LVU160
    +       movl    76(%esp), %edx
    +       .loc 1 96 17 discriminator 3 view .LVU161
    +       bswap   %eax

Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Signed-off-by: Paul Menzel <[e-mail]>
---
M src/drivers/intel/gma/edid.c
1 file changed, 3 insertions(+), 4 deletions(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38647/1

diff --git a/src/drivers/intel/gma/edid.c b/src/drivers/intel/gma/edid.c
index 6885f98..5f1cfe8 100644
--- a/src/drivers/intel/gma/edid.c
+++ b/src/drivers/intel/gma/edid.c
@@ -15,6 +15,7 @@
 #include <device/mmio.h>
 #include <console/console.h>
 #include <delay.h>
+#include <swab.h>
 #include <timer.h>
 
 #include "i915_reg.h"
@@ -91,10 +92,8 @@
 		u32 reg32;
 		wait_rdy(mmio);
 		reg32 = read32(GMBUS3_ADDR);
-		edid[4 * i] = reg32 & 0xff;
-		edid[4 * i + 1] = (reg32 >> 8) & 0xff;
-		edid[4 * i + 2] = (reg32 >> 16) & 0xff;
-		edid[4 * i + 3] = (reg32 >> 24) & 0xff;
+		/* Writes 4 * i to (4 * i) + 3 */
+		edid[4 * i] = swab32(reg32);
 	}
 	wait_rdy(mmio);
 	write32(GMBUS1_ADDR, GMBUS_SW_RDY

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38647
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Gerrit-Change-Number: 38647
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-MessageType: newchange

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )

Change subject: drivers/intel/gma/edid: Use byte swap function
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
File src/drivers/intel/gma/edid.c:

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
PS1, Line 94: reg32
This variable shouldn't be necessary anymore.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38647
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Gerrit-Change-Number: 38647
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Thu, 30 Jan 2020 11:50:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Patrick Rudolph (Code Review)]

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

Change subject: drivers/intel/gma/edid: Use byte swap function
......................................................................


Patch Set 1: Code-Review-1

That looks wrong. edid is just 1 byte, so you drop 3 bytes on implicitly casting it.


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38647
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Gerrit-Change-Number: 38647
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Thu, 30 Jan 2020 19:56:19 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Paul Menzel (Code Review)]

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

Change subject: drivers/intel/gma/edid: Use byte swap function
......................................................................


Patch Set 1:

(1 comment)
> Patch Set 1: Code-Review-1
>
> That looks wrong. edid is just 1 byte, so you drop 3 bytes on implicitly casting it.

What do you mean?

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
File src/drivers/intel/gma/edid.c:

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
PS1, Line 94: reg32
> This variable shouldn't be necessary anymore.
> Do you mean `swab32(read32(GMBUS3_ADDR)`?

[Paul Menzel (Code Review)]

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

Change subject: drivers/intel/gma/edid: Use byte swap function
......................................................................


Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
File src/drivers/intel/gma/edid.c:

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
PS1, Line 96: 		edid[4 * i] = swab32(reg32);
I’d think, that all four bytes are written. But I still need to test it.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38647
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Gerrit-Change-Number: 38647
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Thu, 30 Jan 2020 22:51:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )

Change subject: drivers/intel/gma/edid: Use byte swap function
......................................................................


Patch Set 1: Code-Review-1

(3 comments)

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
File src/drivers/intel/gma/edid.c:

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
PS1, Line 94: reg32
> Do you mean `swab32(read32(GMBUS3_ADDR)`?
> Yes, but I think it won't work.

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
PS1, Line 96: 		edid[4 * i] = swab32(reg32);
> I’d think, that all four bytes are written. But I still need to test it.
> Well, if you looked at the disassembly right before this, can't you just check if the code will write the 32-bit value or will just cast it?

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
PS1, Line 96: edid
Call me cursed, buuut... What if you do a pointer cast and use an u32* here? This would make the byteswap unnecessary.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38647
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Gerrit-Change-Number: 38647
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Thu, 30 Jan 2020 23:21:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Angel Pons <[e-mail]>
Comment-In-Reply-To: Paul Menzel <[e-mail]>
Gerrit-MessageType: comment

[Paul Menzel (Code Review)]

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

Change subject: drivers/intel/gma/edid: Use byte swap function
......................................................................


Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
File src/drivers/intel/gma/edid.c:

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
PS1, Line 96: edid
> Call me cursed, buuut... […]
> I am not so good in assembly. Can you take a look please?

```
 .LBE128:
 .LBE127:
-       .loc 1 94 3 is_stmt 1 discriminator 3 view .LVU157
-       addl    $4, %ebp
-       .loc 1 94 15 is_stmt 0 discriminator 3 view .LVU158
-       movb    %al, -4(%ebp)
-       .loc 1 95 3 is_stmt 1 discriminator 3 view .LVU159
-       .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU160
-       movl    %eax, %edx
-       .loc 1 95 19 discriminator 3 view .LVU161
-       movb    %ah, -3(%ebp)
-       .loc 1 96 3 is_stmt 1 discriminator 3 view .LVU162
-       .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU163
-       shrl    $16, %edx
-       .loc 1 96 19 discriminator 3 view .LVU164
-       movb    %dl, -2(%ebp)
-       .loc 1 97 3 is_stmt 1 discriminator 3 view .LVU165
-       .loc 1 97 35 is_stmt 0 discriminator 3 view .LVU166
-       shrl    $24, %eax
+       .loc 1 96 3 is_stmt 1 discriminator 3 view .LVU159
+       .loc 1 96 15 is_stmt 0 discriminator 3 view .LVU160
+       movl    76(%esp), %edx
+       .loc 1 96 17 discriminator 3 view .LVU161
+       bswap   %eax
 .LVL45:
-       .loc 1 97 19 discriminator 3 view .LVU167
-       movb    %al, -1(%ebp)
+       .loc 1 96 15 discriminator 3 view .LVU162
+       movb    %al, (%edx,%ebp,4)
```

I guess the `movb` is the interesting part?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38647
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Gerrit-Change-Number: 38647
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Fri, 31 Jan 2020 19:37:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <[e-mail]>
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )

Change subject: drivers/intel/gma/edid: Use byte swap function
......................................................................


Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
File src/drivers/intel/gma/edid.c:

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
PS1, Line 96: edid
> I am not so good in assembly. Can you take a look please? […]
> Indeed, this won't work.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )

Change subject: drivers/intel/gma/edid: Use byte swap function
......................................................................


Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
File src/drivers/intel/gma/edid.c:

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
PS1, Line 94: 		edid[4 * i] = reg32 & 0xff;
            : 		edid[4 * i + 1] = (reg32 >> 8) & 0xff;
            : 		edid[4 * i + 2] = (reg32 >> 16) & 0xff;
            : 		edid[4 * i + 3] = (reg32 >> 24) & 0xff;
This looks like little-endian encoding, and we are in drivers/intel/. I don't
think there is anything to swap. It's also hard to argue about endianness
here, as this is just copying data from hardware to a stream (just leaving
the bytes in the order they were found should work).

I think the best way to express this would be to use a host-specific read
and write:

    ((uint32_t *)edid)[i] = read32(GMBUS3_ADDR);

or

    write32(edid + 4 * i, read32(GMBUS3_ADDR));

The former doesn't look too nice, the latter would (unnecessarily) treat the
RAM where `edid` resides like MMIO...

Or maybe just use memcpy()? ;)



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38647
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Gerrit-Change-Number: 38647
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Wed, 05 Feb 2020 09:15:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )

Change subject: drivers/intel/gma/edid: Use byte swap function
......................................................................


Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
File src/drivers/intel/gma/edid.c:

https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... 
PS1, Line 96: edid
> Call me cursed, buuut... What if you do a pointer cast and use an u32* here? This would make the byteswap unnecessary.

If the type is char or void, then aliasing to a different type does NOT result in nasal demons (undefined behavior, after which anything could happen like demons coming out of everyone's noses). So, it's an option.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38647
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Gerrit-Change-Number: 38647
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Wed, 05 Feb 2020 15:19:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <[e-mail]>
Comment-In-Reply-To: Paul Menzel <[e-mail]>
Gerrit-MessageType: comment

Change in coreboot[master]: drivers/intel/gma/edid: Directly assign 32-bit value

[Paul Menzel (Code Review)]

Hello Patrick Rudolph, Angel Pons, build bot (Jenkins), 

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

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

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

Change subject: drivers/intel/gma/edid: Directly assign 32-bit value
......................................................................

drivers/intel/gma/edid: Directly assign 32-bit value

Instead of manually assigning one byte at a time, write the four bytes
at once.

With gcc (Debian 9.2.1-25) 9.2.1 20200123 the assembly differences below
are made, showing that some instructions are saved.

            movl    12(%esi), %eax
     .LVL44:
    -       .loc 2 34 9 discriminator 3 view .LVU156
    -.LBE128:
    -.LBE127:
    -       .loc 1 94 3 is_stmt 1 discriminator 3 view .LVU157
    -       addl    $4, %ebp
    -       .loc 1 94 15 is_stmt 0 discriminator 3 view .LVU158
    -       movb    %al, -4(%ebp)
    -       .loc 1 95 3 is_stmt 1 discriminator 3 view .LVU159
    -       .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU160
    -       movl    %eax, %edx
    -       .loc 1 95 19 discriminator 3 view .LVU161
    -       movb    %ah, -3(%ebp)
    -       .loc 1 96 3 is_stmt 1 discriminator 3 view .LVU162
    -       .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU163
    -       shrl    $16, %edx
    -       .loc 1 96 19 discriminator 3 view .LVU164
    -       movb    %dl, -2(%ebp)
    -       .loc 1 97 3 is_stmt 1 discriminator 3 view .LVU165
    -       .loc 1 97 35 is_stmt 0 discriminator 3 view .LVU166
    -       shrl    $24, %eax
    -.LVL45:
    -       .loc 1 97 19 discriminator 3 view .LVU167
    -       movb    %al, -1(%ebp)
    -.LBE129:
    -       .loc 1 90 33 is_stmt 1 discriminator 3 view .LVU168
    +       .loc 2 34 9 discriminator 3 view .LVU155
    +       addl    $16, %edi
    +.LBE125:
    +.LBE124:
    +       .loc 1 94 29 discriminator 3 view .LVU156
    +       movl    %eax, -16(%edi)
    +       .loc 1 91 33 is_stmt 1 discriminator 3 view .LVU157
            jmp     .L12

Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Signed-off-by: Paul Menzel <[e-mail]>
---
M src/drivers/intel/gma/edid.c
1 file changed, 3 insertions(+), 6 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38647/2
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38647
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Gerrit-Change-Number: 38647
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-MessageType: newpatchset

Change in coreboot[master]: drivers/intel/gma/edid: Directly assign 32-bit value

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )

Change subject: drivers/intel/gma/edid: Directly assign 32-bit value
......................................................................


Patch Set 2:

(4 comments)

https://review.coreboot.org/c/coreboot/+/38647/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38647/2//COMMIT_MSG@13 
PS2, Line 13: are made, showing that some instructions are saved.
Why not use the reference toolchain?


https://review.coreboot.org/c/coreboot/+/38647/2//COMMIT_MSG@49 
PS2, Line 49:             jmp     .L12
The diff doesn't provide much value, imho. If you want to keep
it, it would be much more readable with the comments removed,
just `objdump -d` should do it.


https://review.coreboot.org/c/coreboot/+/38647/2/src/drivers/intel/gma/edid.... 
File src/drivers/intel/gma/edid.c:

https://review.coreboot.org/c/coreboot/+/38647/2/src/drivers/intel/gma/edid.... 
PS2, Line 18: #include <swab.h>
Not used anymore.


https://review.coreboot.org/c/coreboot/+/38647/2/src/drivers/intel/gma/edid.... 
PS2, Line 93: 		/* Only works on LE systems, writes 4 * i to (4 * i) + 3 */
It's much more complicated. I would have said the old code only works with LE.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38647
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Gerrit-Change-Number: 38647
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Wed, 05 Feb 2020 17:31:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in coreboot[master]: drivers/intel/gma/edid: Directly assign 32-bit value

[Patrick Rudolph (Code Review)]

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

Change subject: drivers/intel/gma/edid: Directly assign 32-bit value
......................................................................


Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38647/3/src/drivers/intel/gma/edid.... 
File src/drivers/intel/gma/edid.c:

https://review.coreboot.org/c/coreboot/+/38647/3/src/drivers/intel/gma/edid.... 
PS3, Line 82: edid
((uint32_t*)edid)[i] ?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38647
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Gerrit-Change-Number: 38647
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Fri, 12 Jun 2020 19:03:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in coreboot[master]: drivers/intel/gma/edid: Directly assign 32-bit value

[Paul Menzel (Code Review)]

Hello build bot (Jenkins), Angel Pons, Patrick Rudolph, 

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

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

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

Change subject: drivers/intel/gma/edid: Directly assign 32-bit value
......................................................................

drivers/intel/gma/edid: Directly assign 32-bit value

Instead of manually assigning one byte at a time, write the four bytes
at once.

With gcc (Debian 9.2.1-25) 9.2.1 20200123 the assembly differences below
are made, showing that some instructions are saved.

            movl    12(%esi), %eax
     .LVL44:
    -       .loc 2 34 9 discriminator 3 view .LVU156
    -.LBE128:
    -.LBE127:
    -       .loc 1 94 3 is_stmt 1 discriminator 3 view .LVU157
    -       addl    $4, %ebp
    -       .loc 1 94 15 is_stmt 0 discriminator 3 view .LVU158
    -       movb    %al, -4(%ebp)
    -       .loc 1 95 3 is_stmt 1 discriminator 3 view .LVU159
    -       .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU160
    -       movl    %eax, %edx
    -       .loc 1 95 19 discriminator 3 view .LVU161
    -       movb    %ah, -3(%ebp)
    -       .loc 1 96 3 is_stmt 1 discriminator 3 view .LVU162
    -       .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU163
    -       shrl    $16, %edx
    -       .loc 1 96 19 discriminator 3 view .LVU164
    -       movb    %dl, -2(%ebp)
    -       .loc 1 97 3 is_stmt 1 discriminator 3 view .LVU165
    -       .loc 1 97 35 is_stmt 0 discriminator 3 view .LVU166
    -       shrl    $24, %eax
    -.LVL45:
    -       .loc 1 97 19 discriminator 3 view .LVU167
    -       movb    %al, -1(%ebp)
    -.LBE129:
    -       .loc 1 90 33 is_stmt 1 discriminator 3 view .LVU168
    +       .loc 2 34 9 discriminator 3 view .LVU155
    +       addl    $16, %edi
    +.LBE125:
    +.LBE124:
    +       .loc 1 94 29 discriminator 3 view .LVU156
    +       movl    %eax, -16(%edi)
    +       .loc 1 91 33 is_stmt 1 discriminator 3 view .LVU157
            jmp     .L12

Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Signed-off-by: Paul Menzel <[e-mail]>
---
M src/drivers/intel/gma/edid.c
1 file changed, 3 insertions(+), 6 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38647/4
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38647
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Gerrit-Change-Number: 38647
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-MessageType: newpatchset

[XS] Change in coreboot[master]: drivers/intel/gma/edid: Directly assign 32-bit value

[Martin L Roth (Code Review)]

Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38647?usp=email )

Change subject: drivers/intel/gma/edid: Directly assign 32-bit value
......................................................................


Abandoned

This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/38647?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836
Gerrit-Change-Number: 38647
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Martin L Roth <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-MessageType: abandon