Change in coreboot[master]: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation
Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation
The FSP_CAR option needs additional configuration, so it should never be made the default without providing all other necessary defaults. It seems rather offensive to guard sane defaults with a vendor selection, so let's not add more guards to it and always make the working choice the default instead.
Change-Id: I4a880f3093887628ce6bc9c39758e43688345713 Signed-off-by: Nico Huber nico.huber@secunet.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig 2 files changed, 0 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/34166/1
diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index 5e405db..0258e91 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -271,8 +271,6 @@
choice prompt "Cache-as-ram implementation" - default USE_CANNONLAKE_CAR_NEM_ENHANCED if MAINBOARD_HAS_CHROMEOS - default USE_CANNONLAKE_FSP_CAR help This option allows you to select how cache-as-ram (CAR) is set up.
diff --git a/src/soc/intel/icelake/Kconfig b/src/soc/intel/icelake/Kconfig index f0b2918..3f66e02 100644 --- a/src/soc/intel/icelake/Kconfig +++ b/src/soc/intel/icelake/Kconfig @@ -174,8 +174,6 @@
choice prompt "Cache-as-ram implementation" - default USE_ICELAKE_CAR_NEM_ENHANCED if MAINBOARD_HAS_CHROMEOS - default USE_ICELAKE_FSP_CAR help This option allows you to select how cache-as-ram (CAR) is set up.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1: Code-Review-2
(1 comment)
i really don't know what is the problem with "insane" defaults as all chrome platforms are using CAR NEM enhance by default and IOTG users are using FSP-T for CAR. i'm not sure if our IOTG folks are using any internal patch to make FSP_CAR working. Till that clarifies my -2 here, as i know this patch will merge fast.
https://review.coreboot.org/c/coreboot/+/34166/1/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/34166/1/src/soc/intel/cannonlake/Kc... PS1, Line 274: Do you recommend the user now to select what is CAR option now ?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
@Lean/BT, please share your view on CFL FAR CAR usage.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
When I leave the blob default, I get prompted (in a different location non-obvious location) for CPU_MICROCODE_CBFS_LEN and CPU_MICROCODE_CBFS_LOC. Those have non-working default values (0). Resulting in a brick.
With a reasonable default here, the open-source NEM-enhanced, everything works fine.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
NB. I treat blob-defaults where a superior open-source solution exists as an insult to the open-source community. I think it's incredibly rude behavior from Intel.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
Patch Set 1:
When I leave the blob default, I get prompted (in a different location non-obvious location) for CPU_MICROCODE_CBFS_LEN and CPU_MICROCODE_CBFS_LOC. Those have non-working default values (0). Resulting in a brick.
With a reasonable default here, the open-source NEM-enhanced, everything works fine.
We can make NEM-enhanced a default option then but in past IOTG folks wishes to make FSP-T usage for them. i believe default can be overriden so they can still use FSP-CAR if wishes.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
When I leave the blob default, I get prompted (in a different location non-obvious location) for CPU_MICROCODE_CBFS_LEN and CPU_MICROCODE_CBFS_LOC. Those have non-working default values (0). Resulting in a brick.
With a reasonable default here, the open-source NEM-enhanced, everything works fine.
We can make NEM-enhanced a default option then but in past IOTG folks wishes to make FSP-T usage for them. i believe default can be overriden so they can still use FSP-CAR if wishes.
That's what this change does.
And for the future (not for you Subrata but any interested reader): I've already tried to discuss this on Gerrit, I tried to discuss it via email, but IOTG is ignoring me and pushes crappy code rela- ted to FSP-T. If I ever run into issues with FSP-T again, I'll push a patch to remove its support for any platform where an open-source solution exists.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
Patch Set 1:
When I leave the blob default, I get prompted (in a different location non-obvious location) for CPU_MICROCODE_CBFS_LEN and CPU_MICROCODE_CBFS_LOC. Those have non-working default values (0). Resulting in a brick.
With a reasonable default here, the open-source NEM-enhanced, everything works fine.
We can make NEM-enhanced a default option then but in past IOTG folks wishes to make FSP-T usage for them. i believe default can be overriden so they can still use FSP-CAR if wishes.
That's what this change does.
And for the future (not for you Subrata but any interested reader): I've already tried to discuss this on Gerrit, I tried to discuss it via email, but IOTG is ignoring me and pushes crappy code rela- ted to FSP-T. If I ever run into issues with FSP-T again, I'll push a patch to remove its support for any platform where an open-source solution exists.
I guess lean sheng and BT is from IOTG should help to explain what all required to enable any flow. And its not expected that one needs to struggle to enable such feature. Hopefully IOT folks will take care in future.
Boon Tiong Teo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
When I leave the blob default, I get prompted (in a different location non-obvious location) for CPU_MICROCODE_CBFS_LEN and CPU_MICROCODE_CBFS_LOC. Those have non-working default values (0). Resulting in a brick.
With a reasonable default here, the open-source NEM-enhanced, everything works fine.
We can make NEM-enhanced a default option then but in past IOTG folks wishes to make FSP-T usage for them. i believe default can be overriden so they can still use FSP-CAR if wishes.
That's what this change does.
And for the future (not for you Subrata but any interested reader): I've already tried to discuss this on Gerrit, I tried to discuss it via email, but IOTG is ignoring me and pushes crappy code rela- ted to FSP-T. If I ever run into issues with FSP-T again, I'll push a patch to remove its support for any platform where an open-source solution exists.
I guess lean sheng and BT is from IOTG should help to explain what all required to enable any flow. And its not expected that one needs to struggle to enable such feature. Hopefully IOT folks will take care in future.
Hi Nico, are you telling us that when you choose to use Fsp-Car, and with your config will pump into getting CPU_MICROCODE_CBFS_LEN and _LOC = 0? With that causing you having boot failure?
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1: Code-Review+1
Patch Set 1:
When I leave the blob default, I get prompted (in a different location non-obvious location) for CPU_MICROCODE_CBFS_LEN and CPU_MICROCODE_CBFS_LOC. Those have non-working default values (0). Resulting in a brick.
With a reasonable default here, the open-source NEM-enhanced, everything works fine.
We can make NEM-enhanced a default option then but in past IOTG folks wishes to make FSP-T usage for them. i believe default can be overriden so they can still use FSP-CAR if wishes.
That's what this change does.
And for the future (not for you Subrata but any interested reader): I've already tried to discuss this on Gerrit, I tried to discuss it via email, but IOTG is ignoring me and pushes crappy code rela- ted to FSP-T. If I ever run into issues with FSP-T again, I'll push a patch to remove its support for any platform where an open-source solution exists.
Hi Nico, we are alright to remove FSP CAR as default, and we already included steps on enabling FSP-T in our communication with customers. Thanks for bringing up the error you faced, we will also work to include FSP-T enabling details in documentation including 'CPU_MICROCODE_CBFS_LEN'and 'CPU_MICROCODE_CBFS_LOC' where you got into a brick, so anyone in this community will understand the technicality behind enabling FSP-T CAR.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
Hi Nico, are you telling us that when you choose to use Fsp-Car
No, I did not "choose" it. It is the default unless you work on Google's boards.
and with your config will pump into getting CPU_MICROCODE_CBFS_LEN and _LOC = 0? With that causing you having boot failure?
Again, not my config, but the defaults are 0. I only assumed that this is what is causing the failure, but I don't know it for sure. It's also possible that FSP-T is more broken or that some other default setting is wrong.
I'm not really looking for a way to make FSP-T work. FSP-T is a disgrace to coreboot. I don't care about it. What I do care about is working default settings.
In coreboot (the real coreboot without FSP), for most boards one simply has to select the mainboard vendor and model. If you leave everything else at its default, you get a bootable firmware. Alas, the FSP integration into coreboot is still five or six years behind in terms of quality.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
Hi Nico, we are alright to remove FSP CAR as default, and we already included steps on enabling FSP-T in our communication with customers.
Thank you.
Thanks for bringing up the error you faced, we will also work to include FSP-T enabling details in documentation including 'CPU_MICROCODE_CBFS_LEN'and 'CPU_MICROCODE_CBFS_LOC' where you got into a brick, so anyone in this community will understand the technicality behind enabling FSP-T CAR.
I think the time would be better spent to enhance coreboot's CAR implementation to the point that nobody needs FSP-T. AFAIK, nobody in the open-source community wants it anyway. Why do you or your customers need FSP-T?
Boon Tiong Teo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
Patch Set 1:
Hi Nico, we are alright to remove FSP CAR as default, and we already included steps on enabling FSP-T in our communication with customers.
Thank you.
Thanks for bringing up the error you faced, we will also work to include FSP-T enabling details in documentation including 'CPU_MICROCODE_CBFS_LEN'and 'CPU_MICROCODE_CBFS_LOC' where you got into a brick, so anyone in this community will understand the technicality behind enabling FSP-T CAR.
I think the time would be better spent to enhance coreboot's CAR implementation to the point that nobody needs FSP-T. AFAIK, nobody in the open-source community wants it anyway. Why do you or your customers need FSP-T?
Agreed on we should have a default option not to use FspT, but not agreed on remove it as really there is customer from IOT who need that it. Please give us sometime to work with program level on this open issue, then we can propose a better solution for it. Thanks.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
I agree with changing the defaults to something that works out-of-the-box. See my comment, though.
https://review.coreboot.org/c/coreboot/+/34166/1/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/34166/1/src/soc/intel/cannonlake/Kc... PS1, Line 273: Cache-as-ram implementation Where do these prompts get their default value from, now?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
(2 comments)
Subrata, is there anything you want to add to justify your score?
https://review.coreboot.org/c/coreboot/+/34166/1/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/34166/1/src/soc/intel/cannonlake/Kc... PS1, Line 274:
Do you recommend the user now to select what is CAR option now ?
This had a prompt all along, so I expect everyone to select what suits their use case. Generally, I would recommend to leave it at the new default `USE_CANNONLAKE_CAR_NEM_ENHANCED`.
https://review.coreboot.org/c/coreboot/+/34166/1/src/soc/intel/cannonlake/Kc... PS1, Line 273: Cache-as-ram implementation
Where do these prompts get their default value from, now?
If no `default` is given for a choice, it simply defaults to the first available entry.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/34166/1/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/34166/1/src/soc/intel/cannonlake/Kc... PS1, Line 273: Cache-as-ram implementation
If no `default` is given for a choice, it simply defaults to the first available entry.
So it defaults to USE_CANNONLAKE_CAR_NEM_ENHANCED, just as intended. Ack.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
Patch Set 1:
(2 comments)
Subrata, is there anything you want to add to justify your score?
possible to make CAR NEM default now, as i talked to Lean and understood they are okay and for all good reason CAR NEM is best for coreboot-intel
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
(2 comments)
Subrata, is there anything you want to add to justify your score?
possible to make CAR NEM default now, as i talked to Lean and understood they are okay and for all good reason CAR NEM is best for coreboot-intel
sorry, i mean CAR NEM enhance
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34166/1/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/34166/1/src/soc/intel/cannonlake/Kc... PS1, Line 274:
This had a prompt all along, so I expect everyone to select what suits their […]
i don;t think ppl know what suites their platform! we have seen issue randomly with CAR NEM hence i would recommend to fixed it with NEM enhance. I had 2 default to satisfy IOT ask in past and now if they are okay with it. i would like to go with USE_CANNONLAKE_CAR_NEM_ENHANCED
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1: -Code-Review
Agreed with the rest. Please set 1 as default.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
I don't follow. This patch already sets `USE_CANNONLAKE_ CAR_NEM_ENHANCED` as default (just implicitly because the first entry of a choice is always the default). If you want me to add an unnecessary but explicit `default USE_CANNONLAKE_CAR_NEM_ENHANCED`, let me know.
However, in that case I would also like to make it a con- vention. General Kconfig style shouldn't be part of this review. Somebody would have to write a lint test to check for choices without explicit default and we should discuss if that is what we want for the project.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
Patch Set 1:
I don't follow. This patch already sets `USE_CANNONLAKE_ CAR_NEM_ENHANCED` as default (just implicitly because the first entry of a choice is always the default). If you want me to add an unnecessary but explicit `default USE_CANNONLAKE_CAR_NEM_ENHANCED`, let me know.
However, in that case I would also like to make it a con- vention. General Kconfig style shouldn't be part of this review. Somebody would have to write a lint test to check for choices without explicit default and we should discuss if that is what we want for the project.
`default USE_CANNONLAKE_CAR_NEM_ENHANCED` this is what i meant just to make sure we are running with pre-validated configuration and noone need to debug some unnecessary hang issue
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
I don't follow. This patch already sets `USE_CANNONLAKE_ CAR_NEM_ENHANCED` as default (just implicitly because the first entry of a choice is always the default). If you want me to add an unnecessary but explicit `default USE_CANNONLAKE_CAR_NEM_ENHANCED`, let me know.
However, in that case I would also like to make it a con- vention. General Kconfig style shouldn't be part of this review. Somebody would have to write a lint test to check for choices without explicit default and we should discuss if that is what we want for the project.
`default USE_CANNONLAKE_CAR_NEM_ENHANCED` this is what i meant just to make sure we are running with pre-validated configuration and noone need to debug some unnecessary hang issue
Adding that line wouldn't change anything. Hence can't make anything sure. Please read what you quoted above, then provide a sound answer.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
Patch Set 1:
I don't follow. This patch already sets `USE_CANNONLAKE_ CAR_NEM_ENHANCED` as default (just implicitly because the first entry of a choice is always the default). If you want me to add an unnecessary but explicit `default USE_CANNONLAKE_CAR_NEM_ENHANCED`, let me know.
However, in that case I would also like to make it a con- vention. General Kconfig style shouldn't be part of this review. Somebody would have to write a lint test to check for choices without explicit default and we should discuss if that is what we want for the project.
`default USE_CANNONLAKE_CAR_NEM_ENHANCED` this is what i meant just to make sure we are running with pre-validated configuration and noone need to debug some unnecessary hang issue
Adding that line wouldn't change anything. Hence can't make anything sure. Please read what you quoted above, then provide a sound answer.
so you are telling USE_CANNONLAKE_CAR_NEM_ENHANCED would be default choice anyway just because its first option in choice list ? what if i change that order? how do one know that order is important, we have seen some issue in past where NEM alone is not enough for small LLC cache hence we added enhanced NEM option.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop insane defaults for CAR implementation ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34166/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34166/1//COMMIT_MSG@7 PS1, Line 7: insane also kindly find better word for this
Hello Patrick Rudolph, Subrata Banik, Angel Pons, Lean Sheng Tan, Teo Boon Tiong, Boon Tiong Teo, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34166
to look at the new patch set (#2).
Change subject: soc/intel/{cnl,icl}: Drop arrogant defaults for CAR implementation ......................................................................
soc/intel/{cnl,icl}: Drop arrogant defaults for CAR implementation
The FSP_CAR option needs additional configuration, so it should never be made the default without providing all other necessary defaults. It seems rather offensive to guard sane defaults with a vendor selection, so let's not add more guards to it and always make the working choice the default instead.
Change-Id: I4a880f3093887628ce6bc9c39758e43688345713 Signed-off-by: Nico Huber nico.huber@secunet.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig 2 files changed, 0 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/34166/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop arrogant defaults for CAR implementation ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
I don't follow. This patch already sets `USE_CANNONLAKE_ CAR_NEM_ENHANCED` as default (just implicitly because the first entry of a choice is always the default). If you want me to add an unnecessary but explicit `default USE_CANNONLAKE_CAR_NEM_ENHANCED`, let me know.
However, in that case I would also like to make it a con- vention. General Kconfig style shouldn't be part of this review. Somebody would have to write a lint test to check for choices without explicit default and we should discuss if that is what we want for the project.
`default USE_CANNONLAKE_CAR_NEM_ENHANCED` this is what i meant just to make sure we are running with pre-validated configuration and noone need to debug some unnecessary hang issue
Adding that line wouldn't change anything. Hence can't make anything sure. Please read what you quoted above, then provide a sound answer.
so you are telling USE_CANNONLAKE_CAR_NEM_ENHANCED would be default choice anyway just because its first option in choice list ? what if i change that order? how do one know that order is important, we have seen some issue in past where NEM alone is not enough for small LLC cache hence we added enhanced NEM option.
I tried to follow the discussion, but I don't get where the CAR_NEM is being used. The choice only consists of FSP-T and CAR_NEM_ENHANCED, doesn't it?
Also enhanced NEM needs documentation. Google doesn't give any results for that and I don't understand the Kconfig help text.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop arrogant defaults for CAR implementation ......................................................................
Patch Set 2:
I don't follow. This patch already sets `USE_CANNONLAKE_ CAR_NEM_ENHANCED` as default (just implicitly because the first entry of a choice is always the default). If you want me to add an unnecessary but explicit `default USE_CANNONLAKE_CAR_NEM_ENHANCED`, let me know.
However, in that case I would also like to make it a con- vention. General Kconfig style shouldn't be part of this review. Somebody would have to write a lint test to check for choices without explicit default and we should discuss if that is what we want for the project.
`default USE_CANNONLAKE_CAR_NEM_ENHANCED` this is what i meant just to make sure we are running with pre-validated configuration and noone need to debug some unnecessary hang issue
Adding that line wouldn't change anything. Hence can't make anything sure. Please read what you quoted above, then provide a sound answer.
so you are telling USE_CANNONLAKE_CAR_NEM_ENHANCED would be default choice anyway just because its first option in choice list ?
Yes.
what if i change that order?
Same as if you would change a `default` line, I couldn't stop you.
how do one know that order is important
I would assume that any reviewer giving +2 or -2 is responsible enough to make themselves familiar with the language they are reviewing before doing so. I know there are exceptions, irre- sponsible reviewers that I would rather not have in the Reviewers group. But I was told to play nice with them.
, we have seen some issue in past where NEM alone is not enough for small LLC cache hence we added enhanced NEM option.
Well, I won't pity you. I've been telling that FSP-T integration is a bad idea and causes only friction all along.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop arrogant defaults for CAR implementation ......................................................................
Patch Set 2:
I've been telling that FSP-T integration is a bad idea and causes only friction all along.
i was referring coreboot NEM mode not the FSP-T :).
https://github.com/coreboot/coreboot/blob/ac14a40d0e17571ad3495d45216842044d...
FSP-T also enable NEM Enhance mode, JFYI
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop arrogant defaults for CAR implementation ......................................................................
Patch Set 2:
(1 comment)
HI Martin/Patrick G/Stefan/Aaron,
Do we have COC in coreboot ? i'm not sure if "arrogant" kind of word can be used in CL.
https://review.coreboot.org/c/coreboot/+/34166/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34166/2//COMMIT_MSG@7 PS2, Line 7: arrogant Kindly be diplomatic which submitting CL, i don;t know if CB has any COC. Such adjectives can be avoided easily in work environment.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop arrogant defaults for CAR implementation ......................................................................
Patch Set 2:
(1 comment)
Patch Set 1:
Patch Set 1:
I don't follow. This patch already sets `USE_CANNONLAKE_ CAR_NEM_ENHANCED` as default (just implicitly because the first entry of a choice is always the default). If you want me to add an unnecessary but explicit `default USE_CANNONLAKE_CAR_NEM_ENHANCED`, let me know.
However, in that case I would also like to make it a con- vention. General Kconfig style shouldn't be part of this review. Somebody would have to write a lint test to check for choices without explicit default and we should discuss if that is what we want for the project.
`default USE_CANNONLAKE_CAR_NEM_ENHANCED` this is what i meant just to make sure we are running with pre-validated configuration and noone need to debug some unnecessary hang issue
Adding that line wouldn't change anything. Hence can't make anything sure. Please read what you quoted above, then provide a sound answer.
so you are telling USE_CANNONLAKE_CAR_NEM_ENHANCED would be default choice anyway just because its first option in choice list ? what if i change that order? how do one know that order is important, we have seen some issue in past where NEM alone is not enough for small LLC cache hence we added enhanced NEM option.
Hi Subrata,
I did not know Kconfig choices default to the first option in them, and this is why I asked before giving a +2. I know, it may not look obvious, so maybe it's worth debating if we should explicitly state default values for choices, but I would discuss this somewhere else: this is a Kconfig style question, and if we agree on a rule we would need to change all the Kconfig choices in a new CL anyway.
Maybe it is a good idea to bring up the topic on the mailing list?
https://review.coreboot.org/c/coreboot/+/34166/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34166/2//COMMIT_MSG@7 PS2, Line 7: arrogant
Kindly be diplomatic which submitting CL, i don;t know if CB has any COC. […]
Hi Subrata, which word would you suggest instead?
I was thinking of "non-working", since FSP_CAR needs additional configuration. I am open to ideas, though.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop arrogant defaults for CAR implementation ......................................................................
Patch Set 2:
Patch Set 2:
(1 comment)
Patch Set 1:
Patch Set 1:
I don't follow. This patch already sets `USE_CANNONLAKE_ CAR_NEM_ENHANCED` as default (just implicitly because the first entry of a choice is always the default). If you want me to add an unnecessary but explicit `default USE_CANNONLAKE_CAR_NEM_ENHANCED`, let me know.
However, in that case I would also like to make it a con- vention. General Kconfig style shouldn't be part of this review. Somebody would have to write a lint test to check for choices without explicit default and we should discuss if that is what we want for the project.
`default USE_CANNONLAKE_CAR_NEM_ENHANCED` this is what i meant just to make sure we are running with pre-validated configuration and noone need to debug some unnecessary hang issue
Adding that line wouldn't change anything. Hence can't make anything sure. Please read what you quoted above, then provide a sound answer.
so you are telling USE_CANNONLAKE_CAR_NEM_ENHANCED would be default choice anyway just because its first option in choice list ? what if i change that order? how do one know that order is important, we have seen some issue in past where NEM alone is not enough for small LLC cache hence we added enhanced NEM option.
Hi Subrata,
I did not know Kconfig choices default to the first option in them, and this is why I asked before giving a +2. I know, it may not look obvious, so maybe it's worth debating if we should explicitly state default values for choices, but I would discuss this somewhere else: this is a Kconfig style question, and if we agree on a rule we would need to change all the Kconfig choices in a new CL anyway.
Maybe it is a good idea to bring up the topic on the mailing list?
I'm totally okay to remove FSP_CAR support if no one is using. i was just advocating that ppl should know NEM_ENHANCE is something what should be default, as nico said if its first choice then automatically NEM_ENHANCE be default.
My -2 is just for commit msg. i guess we all know what needs to write when we think something been added in past and not required now. Do i really need to be that explicit ?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop arrogant defaults for CAR implementation ......................................................................
Patch Set 2:
(1 comment)
I don't follow. This patch already sets `USE_CANNONLAKE_ CAR_NEM_ENHANCED` as default (just implicitly because the first entry of a choice is always the default). If you want me to add an unnecessary but explicit `default USE_CANNONLAKE_CAR_NEM_ENHANCED`, let me know.
However, in that case I would also like to make it a con- vention. General Kconfig style shouldn't be part of this review. Somebody would have to write a lint test to check for choices without explicit default and we should discuss if that is what we want for the project.
`default USE_CANNONLAKE_CAR_NEM_ENHANCED` this is what i meant just to make sure we are running with pre-validated configuration and noone need to debug some unnecessary hang issue
Adding that line wouldn't change anything. Hence can't make anything sure. Please read what you quoted above, then provide a sound answer.
so you are telling USE_CANNONLAKE_CAR_NEM_ENHANCED would be default choice anyway just because its first option in choice list ? what if i change that order? how do one know that order is important, we have seen some issue in past where NEM alone is not enough for small LLC cache hence we added enhanced NEM option.
Hi Subrata,
I did not know Kconfig choices default to the first option in them, and this is why I asked before giving a +2. I know, it may not look obvious, so maybe it's worth debating if we should explicitly state default values for choices, but I would discuss this somewhere else: this is a Kconfig style question, and if we agree on a rule we would need to change all the Kconfig choices in a new CL anyway.
Maybe it is a good idea to bring up the topic on the mailing list?
I'm totally okay to remove FSP_CAR support if no one is using. i was just advocating that ppl should know NEM_ENHANCE is something what should be default, as nico said if its first choice then automatically NEM_ENHANCE be default.
My -2 is just for commit msg.
That's not true. It's clearly visible from the log of the comments that you are only looking for reasons not to remove it.
i guess we all know what needs to write when we think something been added in past and not required now. Do i really need to be that explicit ?
You asked me to change it, I did. You are getting even more annoying nevertheless. I'm not sure what to do. I never judge the history of code, never judge the persons who wrote, I only judge the current state of the code. And the current state is insane (for an open-source project). I don't know why you always have to make it personal.
https://review.coreboot.org/c/coreboot/+/34166/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34166/2//COMMIT_MSG@7 PS2, Line 7: arrogant
Kindly be diplomatic which submitting CL, i don;t know if CB has any COC. Such adjectives can be avoided easily in work environment.
You asked me to find *better* words, so I checked the review of the original change and came to the conclusion that the current defaults were chosen because of arrogance. Then checked a dictionary to make sure that it's a less offensive word. So I changed it.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Lean Sheng Tan, Teo Boon Tiong, Stefan Reinauer, Boon Tiong Teo, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34166
to look at the new patch set (#3).
Change subject: soc/intel/{cnl,icl}: Drop open-source-abusive defaults for CAR ......................................................................
soc/intel/{cnl,icl}: Drop open-source-abusive defaults for CAR
The FSP_CAR option needs additional configuration, so it should never be made the default without providing all other necessary defaults. It seems rather offensive to guard sane defaults with a vendor selection, so let's not add more guards to it and always make the working choice the default instead.
Change-Id: I4a880f3093887628ce6bc9c39758e43688345713 Signed-off-by: Nico Huber nico.huber@secunet.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig 2 files changed, 0 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/34166/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop open-source-abusive defaults for CAR ......................................................................
Patch Set 3:
Patch Set 2:
My -2 is just for commit msg. i guess we all know what needs to write when we think something been added in past and not required now. Do i really need to be that explicit ?
Most of us are not native English speakers, so misunderstandings can and will happen. Patience is crucial.
In any case, let's be constructive about the commit message: what do you think about this?
soc/intel/{cnl,icl}: Always use CAR NEM enhanced by default
The FSP_CAR option has additional configuration options whose default values result in boot failures. Since default values shall always boot, default to the open-source CAR NEM Enhanced implementation instead.
Additionally, drop the vendor-specific distinction for default values, as the distinction is not justified and does not make any sense.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop open-source-abusive defaults for CAR ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 2:
My -2 is just for commit msg. i guess we all know what needs to write when we think something been added in past and not required now. Do i really need to be that explicit ?
Most of us are not native English speakers, so misunderstandings can and will happen. Patience is crucial.
In any case, let's be constructive about the commit message: what do you think about this?
soc/intel/{cnl,icl}: Always use CAR NEM enhanced by default
The FSP_CAR option has additional configuration options whose default values result in boot failures. Since default values shall always boot, default to the open-source CAR NEM Enhanced implementation instead.
Additionally, drop the vendor-specific distinction for default values, as the distinction is not justified and does not make any sense.
Much better! I'd rephrase the last paragraph a bit, how about "With the open source implementation the default, we also get rid of a vendor specific special case."?
Nico Huber has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop open-source-abusive defaults for CAR ......................................................................
Abandoned
Intel/Google will have to find another slave to clean up behind them.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop open-source-abusive defaults for CAR ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 2:
My -2 is just for commit msg. i guess we all know what needs to write when we think something been added in past and not required now. Do i really need to be that explicit ?
Most of us are not native English speakers, so misunderstandings can and will happen. Patience is crucial.
In any case, let's be constructive about the commit message: what do you think about this?
soc/intel/{cnl,icl}: Always use CAR NEM enhanced by default
The FSP_CAR option has additional configuration options whose default values result in boot failures. Since default values shall always boot, default to the open-source CAR NEM Enhanced implementation instead.
Additionally, drop the vendor-specific distinction for default values, as the distinction is not justified and does not make any sense.
Much better! I'd rephrase the last paragraph a bit, how about "With the open source implementation the default, we also get rid of a vendor specific special case."?
Sounds good, I was in a rush while writing that last part. I came up with this:
soc/intel/{cnl,icl}: Always use CAR NEM enhanced by default
The FSP_CAR option has additional configuration options whose default values result in boot failures. Since default values shall always boot, default to the open-source CAR NEM Enhanced implementation instead. This also allows us to get rid of an unnecessary vendor-specific special case.
Patrick Georgi has restored this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop open-source-abusive defaults for CAR ......................................................................
Restored
Patrick Georgi has uploaded a new patch set (#4) to the change originally created by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Always use CAR NEM enhanced by default ......................................................................
soc/intel/{cnl,icl}: Always use CAR NEM enhanced by default
The FSP_CAR option has additional configuration options whose default values result in boot failures. Since default values shall always boot, default to the open-source CAR NEM Enhanced implementation instead. This also allows us to get rid of an unnecessary vendor-specific special case.
Change-Id: I4a880f3093887628ce6bc9c39758e43688345713 Signed-off-by: Nico Huber nico.huber@secunet.com Signed-off-by: Patrick Georgi patrick@georgi.software --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig 2 files changed, 0 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/34166/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Always use CAR NEM enhanced by default ......................................................................
Patch Set 4:
Oops, I made CB:34287 already. Which one should be kept?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Always use CAR NEM enhanced by default ......................................................................
Patch Set 3:
Folks, I know now that I shouldn't have used the word `insane` in the commit message. I looked it up, realized that it is a much stronger word than I expected and changed it immediately.
But... spending hours with a review that fixes a regression that only made it through the review process because the reviewing party got their comforting cushion in the patch? That really shows that something is wrong in the community. And then covering up for the tension in the community with an adapted commit message? I think that would even make it worse.
Please, don't ignore tension in the community. Please, make it visible wherever you can! If we cover it up, we can never learn from past faults. And I think we all have to learn and hope we will.
Hello Aaron Durbin, Patrick Rudolph, Angel Pons, Subrata Banik, Lean Sheng Tan, Teo Boon Tiong, Stefan Reinauer, Boon Tiong Teo, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34166
to look at the new patch set (#5).
Change subject: soc/intel/{cnl,icl}: Always use CAR NEM enhanced by default ......................................................................
soc/intel/{cnl,icl}: Always use CAR NEM enhanced by default
The FSP_CAR option has additional configuration options whose default values result in boot failures. Since default values shall always boot, default to the open-source CAR NEM Enhanced implementation instead. This also allows us to get rid of an unnecessary vendor-specific special case.
Change-Id: I4a880f3093887628ce6bc9c39758e43688345713 Signed-off-by: Patrick Georgi patrick@georgi.software --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig 2 files changed, 0 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/34166/5
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Always use CAR NEM enhanced by default ......................................................................
Patch Set 5: Code-Review-2
This is not what I authored.
Nico Huber has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Always use CAR NEM enhanced by default ......................................................................
Abandoned
Subrata Banik has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Always use CAR NEM enhanced by default ......................................................................
Removed Code-Review-2 by Subrata Banik subrata.banik@intel.com
-
Angel Pons (Code Review) -
Boon Tiong Teo (Code Review)
-
Lean Sheng Tan (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Subrata Banik (Code Review)