Change in ...coreboot[master]: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHI...
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33189
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING
Skip GT specific programming in coreboot to support early parts without GT enable.
Change-Id: I231e13367cbfbafbfb0cb4235487dbcbcae76820 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/graphics/Kconfig M src/soc/intel/icelake/graphics.c 2 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/33189/1
diff --git a/src/soc/intel/common/block/graphics/Kconfig b/src/soc/intel/common/block/graphics/Kconfig index 4ab9200..36cac22 100644 --- a/src/soc/intel/common/block/graphics/Kconfig +++ b/src/soc/intel/common/block/graphics/Kconfig @@ -2,3 +2,11 @@ bool help Intel Processor common Graphics support + +config SKIP_GRAPHICS_ENABLING + bool + depends on SOC_INTEL_COMMON_BLOCK_GRAPHICS + default n + help + Skip GT specific programming in coreboot to support + early parts without GT enable. diff --git a/src/soc/intel/icelake/graphics.c b/src/soc/intel/icelake/graphics.c index 0fbddf0..0709033 100644 --- a/src/soc/intel/icelake/graphics.c +++ b/src/soc/intel/icelake/graphics.c @@ -34,6 +34,10 @@ { uint32_t ddi_buf_ctl;
+ /* Skip IGD GT programming */ + if (CONFIG(SKIP_GRAPHICS_ENABLING)) + return; + /* * Enable DDI-A (eDP) 4-lane operation if the link is not up yet. * This will allow the kernel to use 4-lane eDP links properly
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/common/block/graphics/... File src/soc/intel/common/block/graphics/Kconfig:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/common/block/graphics/... PS1, Line 6: config SKIP_GRAPHICS_ENABLING How would this differ from NO_GFX_INIT (src/device/Kconfig)?
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES This is a setting that usually is expected to be done by the firmware, why do you want to skip it?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/common/block/graphics/... File src/soc/intel/common/block/graphics/Kconfig:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/common/block/graphics/... PS1, Line 6: config SKIP_GRAPHICS_ENABLING
How would this differ from NO_GFX_INIT (src/device/Kconfig)?
this config to skip only GT programming, coreboot can still make use of software rendering to make use of graphics.
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
This is a setting that usually is expected to be done by the firmware, […]
SOC without GT fused won't be able to program GT register, it will cause hard hang.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
SOC without GT fused won't be able to program GT register, it will cause hard hang.
Ok, just for my understanding: There is a PCI device with the same PCI ID as usual, but it doesn't have this register?
If that is correct, what is the purpose of the device?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
Ok, just for my understanding: There is a PCI device with the same PCI ID […]
yes, you are right. those might be early soc without GT enable.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
yes, you are right. those might be early soc without GT enable.
Maybe there is some way how we could detect such an early part at runtime? e.g. a revision register? or the stepping?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
Maybe there is some way how we could detect such an early part at runtime? […]
no such way, it might only required for initial PO days I'm not sure what bothers you ? in soc po, we used to get few batch of soc and external customers for sure won't get that batch.
we are just keeping this cl for future soc code preparation as we might copy icl soc for future soc.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
no such way, it might only required for initial PO days I'm not sure what bothers you ? in soc po, w […]
If it gets merged, it adds maintenance burden. If it's only for early development days, I wonder why you would enable it (add PCI ID to common/blocks/graphics), then add a Kconfig to disable it again. It seems very odd and is already confusing in review, most likely more confusing when somebody tries to read and understand the code.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
If it gets merged, it adds maintenance burden. If it's only for early […]
It's more of enabling the software to deal with the realities of bringing up new silicon. There is obviously a transition between hobbled and non-hobbled silicon in which there's a logistical problem in supporting everything. It's helpful to have a consistent code base w/ a few knobs vs forking builds and code bases to accommodate the various populations.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
If it gets merged, it adds maintenance burden. If it's only for early […]
i don't understand your point, you are telling not to work on early soc right ? then how do we enable and validate early soc?
PCI enumeration is required because we might need to program GTTMMADR0 and GMADR to make use of sw rendering method even to bring chrome/chromium display on non-GT parts.
We are driving some initiatives where we can skip HW acceleration and still bring up display to continue our work.
Kindly have trust on us on working on early silicon and we will clean those code (Kconfig) without bothering community to clean the same once we have GT enable parts ready
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
It's more of enabling the software to deal with the realities of bringing up new silicon.
I think it's rather about the reality of readable code and working together.
There is obviously a transition between hobbled and non-hobbled silicon in which there's a logistical problem in supporting everything. It's helpful to have a consistent code base w/ a few knobs vs forking builds and code bases to accommodate the various populations.
There is already a knob. The PCI ID in `src/soc/intel/common/blocks/graphics/graphics.c`. Why does it need an additional 12 line knob???
i don't understand your point, you are telling not to work on early soc right ? then how do we enable and validate early soc?
That's about the opposite of my intention. It would be nice of both of you if you could stop bullying me for having an opinion that deviates from yours.
PCI enumeration is required because we might need to program GTTMMADR0 and GMADR to make use of sw rendering method even to bring chrome/chromium display on non-GT parts.
PCI enumeration is done anyway. You don't have to jump through hoops for it (in coreboot).
We are driving some initiatives where we can skip HW acceleration and still bring up display to continue our work.
Sorry, I have no idea what "HW acceleration" means in this content or what it has to do with display engine registers.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
It's more of enabling the software to deal with the realities of bringing up new silicon.
I think it's rather about the reality of readable code and working together.
What exactly do you mean here?
There is obviously a transition between hobbled and non-hobbled silicon in which there's a logistical problem in supporting everything. It's helpful to have a consistent code base w/ a few knobs vs forking builds and code bases to accommodate the various populations.
There is already a knob. The PCI ID in `src/soc/intel/common/blocks/graphics/graphics.c`. Why does it need an additional 12 line knob???
Do you mean someone modifying the src by removing and putting in a pci id based on builds? If so, that's definitely one way to do it, but it requires modifying this driver as opposed to placing a select in a mainboard or something.
I'm assuming the pci id doesn't change across such working silicon and non-working silicon. That situation does inherently require 2 different builds, but once there's no way to identify such a scenario it's needed. I guess the question is how aesthetically ugly one scenario is vs another. That said, I'm not sure it's clear if this whole driver should be ran or just part of it. Subrata needs to explain that bit better about the internal granularity of this driver needed.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
It's more of enabling the software to deal with the realities of bringing up new silicon. […]
i don't understand what for you are arguing? as i have mentioned in commit msg this CL is to skip GT programming for non-GT early soc sample. coreboot selected code which we are trying to skip is touching some GPU registers which will cause hang when GPU is not enable.
Its that simple, I don't think i can able to make you understand that we are trying to get display using framebuffer only mode that i call as sw rendering rather relying on GPU registers that i called as HW acceleration (i think ppl called that the same name)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
i don't understand what for you are arguing? as i have mentioned in commit msg this CL is to skip GT […]
Subrata needs to explain that bit better about the internal granularity of this driver needed.
Yes Aaron, you are right, if i remember correctly IGD DID: 8a71 was common for GT and non GT socs till the time we have started seeing hang in coreboot while executing graphics_soc_init(). we got to know later that few parts doesn't have GT enable, now when delivering single coreboot for all parts how we can distinguish which part has GT and non GT, for that purpose we have decided to skip those GT related programming for early parts when soc user selects SKIP_GRAPHICS_ENABLING kconfig. Plan is to remove this select option after we know all parts has fused graphics in it. i hope i'm clear on requirement parts
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
Its that simple, I don't think i can able to make you understand that we are trying to get display using framebuffer only mode that i call as sw rendering rather relying on GPU registers that i called as HW acceleration (i think ppl called that the same name)
I'm still confused about this naming. But we're making progress.
With this "software rendering", do you still see something on the display? If that is the case, it's probably only the single register write that is causing problems.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
Its that simple, I don't think i can able to make you understand that we are trying to get display […]
if soc doesn't have GPU enable then trying to access those registers will cause hang for sure and this function does that specific GT programming in coreboot.
By default coreboot relies on framebuffer which is equivalent to sw rendering mode and till chrome splash screen onwards everything is GPU dependent.
So right now on non-GT parts, we are seeing depthcharge screen using SW rendering but no chrome display. We are working towards atleast enable chrome OS console mode without using GPU registers
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
if soc doesn't have GPU enable then trying to access those registers will cause hang for sure and th […]
I'm starting to see the picture. Please allow me another question: Is there an IceLake part yet, that works with this code? i.e. without SKIP_GRAPHICS_ENABLING
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
I'm starting to see the picture. […]
yes, we have GPU enable skus now so we are okay now :-) as i told this code is more over for future soc to leverage.
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 1: Code-Review+1
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 2: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 2: Code-Review-1
(2 comments)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
yes, we have GPU enable skus now so we are okay now :-) […]
So it's just more code to copy-paste later. Probably without anyone ever checking if the code guarded by this makes sense for the new platform at all. It seems to me like a bad work- around for bad development practice.
If I understand you correctly, the display engine of your early samples works (you can see output on a display) but the graphics acceleration doesn't? But the code guarded here has absolutely nothing to do with graphics acceleration. Pushing this weird workaround with wrong explanations makes it very hard to work together in a community, I can't agree to it. But if you have to, go on.
https://review.coreboot.org/#/c/33189/2/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/2/src/soc/intel/icelake/graphics.c@50 PS2, Line 50: graphics_gtt_write(DDI_BUF_CTL_A, ddi_buf_ctl); Subrata, Aamir, do you know any ICL documentation about this register? As this is the only thing effectively guarded by SKIP_GRAPHICS_ENABLING, I wonder if this copy-pasta is correct for ICL at all.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
So it's just more code to copy-paste later. Probably without anyone ever checking if the code guarded by this makes sense for the new platform at all. It seems to me like a bad work- around for bad development practice.
[Subrata] yes, its a bad W/A for early platform agree thats the reason its default n. and its early platform nuisance that we might need to deal with. Irrespective if its happening first time on ICL or not.
If I understand you correctly, the display engine of your early samples works (you can see output on a display) but the graphics acceleration doesn't? But the code guarded here has absolutely nothing to do with graphics acceleration. Pushing this weird workaround with wrong explanations makes it very hard to work together in a community, I can't agree to it. But if you have to, go on.
[Subrata] as i have explained several time, you can bring display either by using sw framebuffer only mode or using hardware acceleration mode. Not using this config we would like to guard any transaction that goes to GT register which is not available on specific parts.
https://review.coreboot.org/#/c/33189/2/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/2/src/soc/intel/icelake/graphics.c@50 PS2, Line 50: graphics_gtt_write(DDI_BUF_CTL_A, ddi_buf_ctl);
Subrata, Aamir, do you know any ICL documentation about this […]
This registers are available in ICL EDS vol 1 and only thing that we like to skip if touching these registers on specific parts. Customer receiving early parts has DCL to tell if GT is enable or not.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
So it's just more code to copy-paste later. Probably without anyone ever checking if the code guarded by this makes sense for the new platform at all. It seems to me like a bad work- around for bad development practice.
[Subrata] yes, its a bad W/A for early platform agree thats the reason its default n.
I wasn't referring to a workaround for a hardware problem but a potential workaround for problems that arise from your software development practice here at coreboot. The code guarded here was copied from cannonlake/ without checking if it applies to ICL. And when it starts making trouble, you disable it instead of trying to understand what the code does. Still not checking if it applies to ICL at all. I have a hunch, that you can simply remove the code instead.
If I understand you correctly, the display engine of your early samples works (you can see output on a display) but the graphics acceleration doesn't? But the code guarded here has absolutely nothing to do with graphics acceleration. Pushing this weird workaround with wrong explanations makes it very hard to work together in a community, I can't agree to it. But if you have to, go on.
[Subrata] as i have explained several time, you can bring display either by using sw framebuffer only mode or using hardware acceleration mode. Not using this config we would like to guard any transaction that goes to GT register which is not available on specific parts.
In this notion, coreboot always uses sw framebuffer. I don't see how this is related to hardware acceleration. Keeping to tell so without giving any reference, won't help. You might have learned by now, that you cannot convince me by repeating what was already said. You need arguments to reach me :)
Please don't get me wrong, I don't doubt that it was necessary to disable this code. I just think you are telling the wrong reasons.
https://review.coreboot.org/#/c/33189/2/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/2/src/soc/intel/icelake/graphics.c@50 PS2, Line 50: graphics_gtt_write(DDI_BUF_CTL_A, ddi_buf_ctl);
This registers are available in ICL EDS vol 1 and only thing that we like to skip if touching these […]
I couldn't find a register description their, maybe my version is outdated. Which version are you looking at?
However, I noticed that the EDS doesn't talk about the DP bifurcation (that this is about according to the code comment above) anymore. Prior EDS for CNL, CFL etc. did. So I really think this code here is obsolete anyway.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
So it's just more code to copy-paste later. Probably without […]
Those registers are applicable for ICL as well. And also how do you know that we haven't check if removing those code not giving any problem ?
I guess comments are enough to understand the problem without those GT programming in coreboot
/* * Enable DDI-A (eDP) 4-lane operation if the link is not up yet. * This will allow the kernel to use 4-lane eDP links properly * if the VBIOS or GOP driver do not execute. */
It won't cause any CB issue rather kerner display won't come over eDP when we skip executing GOP/VBIOS, which is typically happening during normal mode.
More details here: https://review.coreboot.org/13690
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
Those registers are applicable for ICL as well. And also how do you know that we haven't check if removing those code not giving any problem ?
Because I currently assume that you would have removed it, then. As long as you can't provide any documentation that ICL knows this bifurcation setting, I'll assume that it doesn't.
I guess comments are enough to understand the problem without those GT programming in coreboot
/*
- Enable DDI-A (eDP) 4-lane operation if the link is not up yet.
- This will allow the kernel to use 4-lane eDP links properly
- if the VBIOS or GOP driver do not execute.
*/
It won't cause any CB issue rather kerner display won't come over eDP when we skip executing GOP/VBIOS, which is typically happening during normal mode.
More details here: https://review.coreboot.org/13690
Don't have to tell me what this code does / tries to do, I'm rather an expert for this part of Intel silicon. And showing reviews for SKL doesn't tell anything about ICL anyway.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c File src/soc/intel/icelake/graphics.c:
https://review.coreboot.org/#/c/33189/1/src/soc/intel/icelake/graphics.c@48 PS1, Line 48: DDI_A_4_LANES
Those registers are applicable for ICL as well. […]
I don't have doubt that you could be an expert but i'm telling you something that i'm living with. We just like that can't remove that code, its been added for an purpose and that purpose is still valid as i could see.
I guess i have provided enough justification why this code is needed and given my experiment details as well.
I don't know what else i can provide you. Request you to not block code development else we would be able to land early soc code into cb.
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33189 )
Change subject: soc/intel/common: Skip SoC GT programming based on CONFIG_SKIP_GRAPHICS_ENABLING ......................................................................
Patch Set 2: Code-Review+2
-
Aamir Bohra (Code Review) -
Aaron Durbin (Code Review)
-
Nico Huber (Code Review)
-
Ronak Kanabar (Code Review)
-
Subrata Banik (Code Review)
-
Wonkyu Kim (Code Review)