Change in coreboot[master]: mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size
Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35064 )
Change subject: mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size ......................................................................
mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size
Connected 4K monitor is not configured at max resolution. The framebuffer size is too small.
Increase the framebuffer size to 64MB. This is sufficient for max configuration of 1 HDMI monitor combined with internal LCD panel.
BUG=N/A TEST=4K HDMI monitor and LCD working fine on Facebook FBG-1701
Change-Id: I25d2cd696830fc5bda84ea2b87538f526373998e Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/mainboard/facebook/fbg1701/devicetree.cb 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/35064/1
diff --git a/src/mainboard/facebook/fbg1701/devicetree.cb b/src/mainboard/facebook/fbg1701/devicetree.cb index 3c82a03..a1999d8 100644 --- a/src/mainboard/facebook/fbg1701/devicetree.cb +++ b/src/mainboard/facebook/fbg1701/devicetree.cb @@ -9,7 +9,7 @@ register "PcdMrcInitMmioSize" = "0x0800" register "PcdMrcInitSpdAddr1" = "0xa0" register "PcdMrcInitSpdAddr2" = "0xa2" - register "PcdIgdDvmt50PreAlloc" = "1" + register "PcdIgdDvmt50PreAlloc" = "2" register "PcdApertureSize" = "2" register "PcdGttSize" = "1" register "PcdDvfsEnable" = "0"
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35064 )
Change subject: mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35064/1/src/mainboard/facebook/fbg1... File src/mainboard/facebook/fbg1701/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35064/1/src/mainboard/facebook/fbg1... PS1, Line 12: register "PcdIgdDvmt50PreAlloc" = "2" Add a comment, that this is 64 MB?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35064 )
Change subject: mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35064/1/src/mainboard/facebook/fbg1... File src/mainboard/facebook/fbg1701/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35064/1/src/mainboard/facebook/fbg1... PS1, Line 12: register "PcdIgdDvmt50PreAlloc" = "2"
Add a comment, that this is 64 MB?
It would be nice to have some macros giving some meaning to these values. baytrail fsp calls them IGD_MEMSIZE_xMB
Hello Patrick Rudolph, Felix Held, Arthur Heymans, Lance Zhao, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35064
to look at the new patch set (#2).
Change subject: mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size ......................................................................
mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size
Connected 4K monitor is not configured at max resolution. The framebuffer size is too small.
Increase the framebuffer size to 64MB. This is sufficient for max configuration of 1 HDMI monitor combined with internal LCD panel.
Add defines to have some more readable code.
BUG=N/A TEST=4K HDMI monitor and LCD working fine on Facebook FBG-1701
Change-Id: I25d2cd696830fc5bda84ea2b87538f526373998e Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/mainboard/facebook/fbg1701/devicetree.cb M src/soc/intel/braswell/chip.h M src/soc/intel/braswell/include/soc/gfx.h 3 files changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/35064/2
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35064 )
Change subject: mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size ......................................................................
Patch Set 2:
(1 comment)
Implement comments
https://review.coreboot.org/c/coreboot/+/35064/1/src/mainboard/facebook/fbg1... File src/mainboard/facebook/fbg1701/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35064/1/src/mainboard/facebook/fbg1... PS1, Line 12: register "PcdIgdDvmt50PreAlloc" = "2"
It would be nice to have some macros giving some meaning to these values. […]
Update to IGD_MEMSIZE_64MB
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35064 )
Change subject: mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
might be good to have the added defines and the change in the board's devicetree in separate patches. I'm not sure if it would be better to have the defines in the chip.h or in the gfx.h file; don't know that part of the code base well enough to be able to say for sure which would be the better location
https://review.coreboot.org/c/coreboot/+/35064/2/src/soc/intel/braswell/incl... File src/soc/intel/braswell/include/soc/gfx.h:
https://review.coreboot.org/c/coreboot/+/35064/2/src/soc/intel/braswell/incl... PS2, Line 20: #define IGD_MEMSIZE_32MB 0x01 : #define IGD_MEMSIZE_64MB 0x02 : #define IGD_MEMSIZE_96MB 0x03 : #define IGD_MEMSIZE_128MB 0x04 maybe add a comment that those values are for the configuration in the devicetree? or should these rather be in the chip.h file, since those are only for the interface to the FSP? see also my comment below
https://review.coreboot.org/c/coreboot/+/35064/2/src/soc/intel/braswell/incl... PS2, Line 38: # define GGC_GSM_SIZE_32MB (1 << 3) : # define GGC_GSM_SIZE_64MB (2 << 3) : # define GCC_GSM_SIZE_96MB (3 << 3) : # define GGC_GSM_SIZE_128MB (4 << 3) aren't those just shifted versions of the new defines above? or is the one above for a completely different API that happens to just put the values in the hardware registers in the end?
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35064 )
Change subject: mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size ......................................................................
Patch Set 2: Code-Review+1
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35064 )
Change subject: mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35064/2/src/soc/intel/braswell/incl... File src/soc/intel/braswell/include/soc/gfx.h:
https://review.coreboot.org/c/coreboot/+/35064/2/src/soc/intel/braswell/incl... PS2, Line 38: # define GGC_GSM_SIZE_32MB (1 << 3) : # define GGC_GSM_SIZE_64MB (2 << 3) : # define GCC_GSM_SIZE_96MB (3 << 3) : # define GGC_GSM_SIZE_128MB (4 << 3)
aren't those just shifted versions of the new defines above? or is the one above for a completely di […]
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35064 )
Change subject: mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35064/1/src/mainboard/facebook/fbg1... File src/mainboard/facebook/fbg1701/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35064/1/src/mainboard/facebook/fbg1... PS1, Line 12: register "PcdIgdDvmt50PreAlloc" = "2"
Update to IGD_MEMSIZE_64MB
Done
https://review.coreboot.org/c/coreboot/+/35064/2/src/soc/intel/braswell/incl... File src/soc/intel/braswell/include/soc/gfx.h:
https://review.coreboot.org/c/coreboot/+/35064/2/src/soc/intel/braswell/incl... PS2, Line 20: #define IGD_MEMSIZE_32MB 0x01 : #define IGD_MEMSIZE_64MB 0x02 : #define IGD_MEMSIZE_96MB 0x03 : #define IGD_MEMSIZE_128MB 0x04
maybe add a comment that those values are for the configuration in the devicetree? or should these r […]
Will move the SoC modification to separate patch set.
Hello Patrick Rudolph, Felix Held, Arthur Heymans, Lance Zhao, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35064
to look at the new patch set (#3).
Change subject: mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size ......................................................................
mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size
Connected 4K monitor is not configured at max resolution. The framebuffer size is too small.
Increase the framebuffer size to 64MB. This is sufficient for max configuration of 1 HDMI monitor combined with internal LCD panel.
BUG=N/A TEST=4K HDMI monitor and LCD working fine on Facebook FBG-1701
Change-Id: I25d2cd696830fc5bda84ea2b87538f526373998e Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/mainboard/facebook/fbg1701/devicetree.cb 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/35064/3
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35064 )
Change subject: mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size ......................................................................
Patch Set 4: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35064 )
Change subject: mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size ......................................................................
mb/facebook/fbg1701/devicetree.cb: Use 64MB framebuffer size
Connected 4K monitor is not configured at max resolution. The framebuffer size is too small.
Increase the framebuffer size to 64MB. This is sufficient for max configuration of 1 HDMI monitor combined with internal LCD panel.
BUG=N/A TEST=4K HDMI monitor and LCD working fine on Facebook FBG-1701
Change-Id: I25d2cd696830fc5bda84ea2b87538f526373998e Signed-off-by: Frans Hendriks fhendriks@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35064 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org --- M src/mainboard/facebook/fbg1701/devicetree.cb 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved
diff --git a/src/mainboard/facebook/fbg1701/devicetree.cb b/src/mainboard/facebook/fbg1701/devicetree.cb index 3c82a03..70e950c 100644 --- a/src/mainboard/facebook/fbg1701/devicetree.cb +++ b/src/mainboard/facebook/fbg1701/devicetree.cb @@ -9,7 +9,7 @@ register "PcdMrcInitMmioSize" = "0x0800" register "PcdMrcInitSpdAddr1" = "0xa0" register "PcdMrcInitSpdAddr2" = "0xa2" - register "PcdIgdDvmt50PreAlloc" = "1" + register "PcdIgdDvmt50PreAlloc" = "IGD_MEMSIZE_64MB" register "PcdApertureSize" = "2" register "PcdGttSize" = "1" register "PcdDvfsEnable" = "0"
-
Arthur Heymans (Code Review) -
Felix Held (Code Review)
-
Frans Hendriks (Code Review)
-
Lance Zhao (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)