[coreboot-gerrit] Change in coreboot[master]: nb/i945: Check if IGD is enabled before R/W to dev(0, 2)

[Elyes HAOUAS (Code Review)]

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

Change subject: nb/i945: Check if IGD is enabled before R/W to dev(0, 2)
......................................................................


Patch Set 1:

> >> I don't see a way IGD could be disabled at this stage. But what
 > about
 > >> SKUs that don't have it at all? are there any (on supported
 > boards)?
 > >
 > > 1 - In case we use an external GPU, the IGD is disabled at
 > > arly_init.c (line #686).
 > 
 > Which is run after raminit.
 > 
 > > (BTW: line +949 should be commented)
 > 
 > It doesn't work without that line. You probably mean 950 should
 > be implemented for the desktop version. Please, go ahead.
 > 
The board I have works without that line.
seems that nvidia external GPU didn't work properly (I don't have it for test), But ATI works just fine without that line. More, it works better without that line. please see https://review.coreboot.org/#/c/coreboot/+/27985/ (Arthur's comment on 
Patch Set 3 )

 > > 2 - there some desktop's version that do not have IGD at all.
 > 
 > That's why I was asking. Are the DEVEN bits hard-wired to 0 for
 > them?
 > 
I don't think so, but as you know, intel's datasheet are not helpful :(
maybe some one from intel can help ... 

 > > 3 - at function "sdram_power_management", we set
 > integrated_graphics
 > > = 1 and use it for test in raminit.c line #2305 .... this do not
 > > make sense.
 > 
 > It documents for the human reader that it should only be executed
 > when integrated graphics are present (or maybe only when enabled;
 > that's not clear). This can be useful, for instance, when somebody
 > wants to implement it for system without IGD.
here I mean why we define "integrated_graphics" = 1 ?
the test do not make sense :
if (1) {
....
}else {
..... <<=== this will never happen

}

so let give a sense to "integrated_graphics" and set it to 1 if IGD is enabled.


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

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51ab94393710ce0222b353ab0cef28621fafaacf
Gerrit-Change-Number: 28935
Gerrit-PatchSet: 1
Gerrit-Owner: Elyes HAOUAS <ehaouas at noos.fr>
Gerrit-Reviewer: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Reviewer: Elyes HAOUAS <ehaouas at noos.fr>
Gerrit-Reviewer: Nico Huber <nico.h at gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus at gmail.com>
Gerrit-Comment-Date: Fri, 05 Oct 2018 17:14:19 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20181005/c2625712/attachment.html>